Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 20:16:43 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Jane Chu <jane.chu@oracle.com>, <dan.j.williams@intel.com>,
        <vishal.l.verma@intel.com>, <dave.jiang@intel.com>,
        <ira.weiny@intel.com>, <willy@infradead.org>,
        <viro@zeniv.linux.org.uk>, <brauner@kernel.org>,
        <nvdimm@lists.linux.dev>, <linux-kernel@vger.kernel.org>,
        <linux-fsdevel@vger.kernel.org>
Subject: RE: [PATCH v4 1/1] dax: enable dax fault handler to report
 VM_FAULT_HWPOISON
Message-ID: <6482999b3afd8_142af82946e@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230509054714.1746956-1-jane.chu@oracle.com>
 <20230509054714.1746956-2-jane.chu@oracle.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230509054714.1746956-2-jane.chu@oracle.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?vgZKbrF962qGJBJtGLASNBSiEeSohxuBFYCqVikycMdDPOJmGcVp75NXmsJo?=
 =?us-ascii?Q?Hydp/Shqh6zj195wRQjpn8AVtuaBNgSxwcTr335Z/LuZczYxlJZF9BbJIWnF?=
 =?us-ascii?Q?dg48mhSkpHzuqTgmWFvLZRTD2t7wnH74QNCfu4sOJmmRo/W2/1NPc2P/2qJ7?=
 =?us-ascii?Q?63h1l93af5eZ3QZu5qwLPcPESnLK9tNAWrmQ3Hi8xAzF7rArLesR59HeZy3P?=
 =?us-ascii?Q?DfKUq0AhLKiXHDUwWgKcCQ+OjwameeFzJV4LKIyCl6TsEhmVGkjSuHAGNb7Y?=
 =?us-ascii?Q?mGPKUVzeVG8Mk/EI5k5Swi8h5IszrZsomAnc7GS+kNiHcaaHw2mSXA2yTLF9?=
 =?us-ascii?Q?Hwwlim9GixPs9nGGwPqI98atwJ1TTn/zUfL76VVqB3jZOkRt4Ac6/zr7HT00?=
 =?us-ascii?Q?bs/6aMsGbYLfAAxS9xFsDb32nX1HmtfUJCM+iOZYSRWKjWNfrHUbe3GhCdzF?=
 =?us-ascii?Q?wl736PFYpQxp+Iq7kBATPDB3CCb+M0O1Fy1c0FK/o9QKS8IGdKm13s8EEXG4?=
 =?us-ascii?Q?GWox99EJ2jYuuOsC+M5u812EgzUqrBU07I15lG+pbhQVsxyagtT8znT39mSR?=
 =?us-ascii?Q?W3Y9U7f4EgnGeFJXOYDSr0dwHwVuDEN39LQ++YrZJUOpmBU5o0zu8is1zfyy?=
 =?us-ascii?Q?X0hSecHc5Xuvyf+hSFiob5xmve2gIukO1U20EXoyxzKE92b93cnJ9xIrYV+o?=
 =?us-ascii?Q?m7BG2ncO2CZRvFT3WxXIV42M9EuKnR05V6kg4mqky+svrVNxOBKzxh24wC0f?=
 =?us-ascii?Q?4nexCaPy+lF6au3EOwV+oXeU036wzMT2Y/VTaBowe1Lqb8DmqTaU0JPsLiKG?=
 =?us-ascii?Q?7e/c3IKZA8fg02GoJ/uz3zy/8T5soaatt/mLD3gt+ae27mABHoD8Q1oEyinQ?=
 =?us-ascii?Q?x/t3to6ZZDb8n4b3Sn6QXAXUKUJR+jiUawEisSo6CqgIZCbJNGM9EYxfS/Ju?=
 =?us-ascii?Q?qMT1lGHgXB4jTVRG7ANSjw+DWgYPoxBjzvfKwfCWvGb9GNP+uAYbh2ZzrU3E?=
 =?us-ascii?Q?QaQyNqb8tAjiy5ScCF1Ph8CHypHmkIM4Xnn827aRLF4BYTEtfUoGMjalUQYM?=
 =?us-ascii?Q?b9CC/1hDOIo0CP6nOErURJ/ZDA06xLEwScsR5lrQTvsTzetNvRzm3IaqXxTy?=
 =?us-ascii?Q?039kvTVsZqGLYGP7bATgxCUxogTDcnW/dpOExKedeg19qRAB2c+Vonvs5b5d?=
 =?us-ascii?Q?p6NfOuT/Ab0JT5ivOLf06gGrcV4U0dl4HB59duCwrZ353H2r2gVXRwVHChiE?=
 =?us-ascii?Q?ZlBB/d5qUFM8R/G8A37YnZL9Jy7YMhT49f5iHdrDtzZt/+xCf8VcX1Uzecfg?=
 =?us-ascii?Q?yB2ym2M2U5OK/f3Q8jDleJ6MRSBycpaGT19jlOIg1SJQQiz6Hg88plPJ7OQa?=
 =?us-ascii?Q?W4LpxL4sfJJQkDMrXE0Py+iePDLQxlilUHSMWyJIw8pr4JfrSD9m606qFX3w?=
 =?us-ascii?Q?f2N8jHc0/lVnCYCAVRcdZfZ6l4HO5bnAZLiOQ3rJ5iaBAzOR3eLaIELAXIeG?=
 =?us-ascii?Q?z5D+uwdamhuFwSGwmbUM72sa8bCJUXZhZhfYKzfsaTN4HEI3RJwVvuPCpoLs?=
 =?us-ascii?Q?0LPqT0VtoSAeME210VrkT7FRXS8Pz7i+NohwGP5Fw/rqGodWpJIksaHEBOUs?=
 =?us-ascii?Q?LA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 21dd84ef-8a80-4cf2-cc94-08db6897f56b
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jun 2023 03:16:47.3169
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: TxFp7SZEp2+2LzywpjCqfXUL8rdzcO42FMxwPng37qqi9sFVsgJNk7aWkVtOAUPq1kWZLO1wzOSo0bjGXnVJzxGoewGx3knBEKWdhxKkO2c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5422
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Jane Chu wrote:
> When multiple processes mmap() a dax file, then at some point,
> a process issues a 'load' and consumes a hwpoison, the process
> receives a SIGBUS with si_code = BUS_MCEERR_AR and with si_lsb
> set for the poison scope. Soon after, any other process issues
> a 'load' to the poisoned page (that is unmapped from the kernel
> side by memory_failure), it receives a SIGBUS with
> si_code = BUS_ADRERR and without valid si_lsb.
> 
> This is confusing to user, and is different from page fault due
> to poison in RAM memory, also some helpful information is lost.
> 
> Channel dax backend driver's poison detection to the filesystem
> such that instead of reporting VM_FAULT_SIGBUS, it could report
> VM_FAULT_HWPOISON.
> 
> If user level block IO syscalls fail due to poison, the errno will
> be converted to EIO to maintain block API consistency.
> 
> Signed-off-by: Jane Chu <jane.chu@oracle.com>
> ---
>  drivers/dax/super.c          |  5 ++++-
>  drivers/nvdimm/pmem.c        |  2 +-
>  drivers/s390/block/dcssblk.c |  3 ++-
>  fs/dax.c                     | 11 ++++++-----
>  fs/fuse/virtio_fs.c          |  3 ++-
>  include/linux/dax.h          |  5 +++++
>  include/linux/mm.h           |  2 ++
>  7 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index c4c4728a36e4..0da9232ea175 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -203,6 +203,8 @@ size_t dax_copy_to_iter(struct dax_device *dax_dev, pgoff_t pgoff, void *addr,
>  int dax_zero_page_range(struct dax_device *dax_dev, pgoff_t pgoff,
>  			size_t nr_pages)
>  {
> +	int ret;
> +
>  	if (!dax_alive(dax_dev))
>  		return -ENXIO;
>  	/*
> @@ -213,7 +215,8 @@ int dax_zero_page_range(struct dax_device *dax_dev, pgoff_t pgoff,
>  	if (nr_pages != 1)
>  		return -EIO;
>  
> -	return dax_dev->ops->zero_page_range(dax_dev, pgoff, nr_pages);
> +	ret = dax_dev->ops->zero_page_range(dax_dev, pgoff, nr_pages);
> +	return dax_mem2blk_err(ret);
>  }
>  EXPORT_SYMBOL_GPL(dax_zero_page_range);
>  
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index ceea55f621cc..46e094e56159 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -260,7 +260,7 @@ __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff,
>  		long actual_nr;
>  
>  		if (mode != DAX_RECOVERY_WRITE)
> -			return -EIO;
> +			return -EHWPOISON;
>  
>  		/*
>  		 * Set the recovery stride is set to kernel page size because
> diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
> index c09f2e053bf8..ee47ac520cd4 100644
> --- a/drivers/s390/block/dcssblk.c
> +++ b/drivers/s390/block/dcssblk.c
> @@ -54,7 +54,8 @@ static int dcssblk_dax_zero_page_range(struct dax_device *dax_dev,
>  	rc = dax_direct_access(dax_dev, pgoff, nr_pages, DAX_ACCESS,
>  			&kaddr, NULL);
>  	if (rc < 0)
> -		return rc;
> +		return dax_mem2blk_err(rc);
> +
>  	memset(kaddr, 0, nr_pages << PAGE_SHIFT);
>  	dax_flush(dax_dev, kaddr, nr_pages << PAGE_SHIFT);
>  	return 0;
> diff --git a/fs/dax.c b/fs/dax.c
> index 2ababb89918d..a26eb5abfdc0 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -1148,7 +1148,7 @@ static int dax_iomap_copy_around(loff_t pos, uint64_t length, size_t align_size,
>  	if (!zero_edge) {
>  		ret = dax_iomap_direct_access(srcmap, pos, size, &saddr, NULL);
>  		if (ret)
> -			return ret;
> +			return dax_mem2blk_err(ret);
>  	}
>  
>  	if (copy_all) {
> @@ -1310,7 +1310,7 @@ static s64 dax_unshare_iter(struct iomap_iter *iter)
>  
>  out_unlock:
>  	dax_read_unlock(id);
> -	return ret;
> +	return dax_mem2blk_err(ret);
>  }
>  
>  int dax_file_unshare(struct inode *inode, loff_t pos, loff_t len,
> @@ -1342,7 +1342,8 @@ static int dax_memzero(struct iomap_iter *iter, loff_t pos, size_t size)
>  	ret = dax_direct_access(iomap->dax_dev, pgoff, 1, DAX_ACCESS, &kaddr,
>  				NULL);
>  	if (ret < 0)
> -		return ret;
> +		return dax_mem2blk_err(ret);
> +
>  	memset(kaddr + offset, 0, size);
>  	if (iomap->flags & IOMAP_F_SHARED)
>  		ret = dax_iomap_copy_around(pos, size, PAGE_SIZE, srcmap,
> @@ -1498,7 +1499,7 @@ static loff_t dax_iomap_iter(const struct iomap_iter *iomi,
>  
>  		map_len = dax_direct_access(dax_dev, pgoff, PHYS_PFN(size),
>  				DAX_ACCESS, &kaddr, NULL);
> -		if (map_len == -EIO && iov_iter_rw(iter) == WRITE) {
> +		if (map_len == -EHWPOISON && iov_iter_rw(iter) == WRITE) {
>  			map_len = dax_direct_access(dax_dev, pgoff,
>  					PHYS_PFN(size), DAX_RECOVERY_WRITE,
>  					&kaddr, NULL);
> @@ -1506,7 +1507,7 @@ static loff_t dax_iomap_iter(const struct iomap_iter *iomi,
>  				recovery = true;
>  		}
>  		if (map_len < 0) {
> -			ret = map_len;
> +			ret = dax_mem2blk_err(map_len);
>  			break;
>  		}
>  
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 4d8d4f16c727..5f1be1da92ce 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -775,7 +775,8 @@ static int virtio_fs_zero_page_range(struct dax_device *dax_dev,
>  	rc = dax_direct_access(dax_dev, pgoff, nr_pages, DAX_ACCESS, &kaddr,
>  			       NULL);
>  	if (rc < 0)
> -		return rc;
> +		return dax_mem2blk_err(rc);
> +
>  	memset(kaddr, 0, nr_pages << PAGE_SHIFT);
>  	dax_flush(dax_dev, kaddr, nr_pages << PAGE_SHIFT);
>  	return 0;
> diff --git a/include/linux/dax.h b/include/linux/dax.h
> index bf6258472e49..a4e97acf60f5 100644
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -261,6 +261,11 @@ static inline bool dax_mapping(struct address_space *mapping)
>  	return mapping->host && IS_DAX(mapping->host);
>  }
>  
> +static inline int dax_mem2blk_err(int err)
> +{
> +	return (err == -EHWPOISON) ? -EIO : err;
> +}

I think it is worth a comment on this function to indicate where this
helper is *not* used. I.e. it's easy to grep for where the error code is
converted for file I/O errors, but the subtlety of when the
dax_direct_acces() return value is not translated for fault handling
deserves a comment when we wonder why this function exists in a few
years time.