| Message ID | 20230406230127.716716-1-jane.chu@oracle.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v2] dax: enable dax fault handler to report VM_FAULT_HWPOISON | expand |
Ping, any comment? thanks, -jane On 4/6/2023 4:01 PM, Jane Chu wrote: > When dax fault handler fails to provision the fault page due to > hwpoison, it returns VM_FAULT_SIGBUS which lead to a sigbus delivered > to userspace with .si_code BUS_ADRERR. Channel dax backend driver's > detection on hwpoison to the filesystem to provide the precise reason > for the fault. > > Signed-off-by: Jane Chu <jane.chu@oracle.com> > --- > drivers/nvdimm/pmem.c | 2 +- > fs/dax.c | 2 +- > include/linux/mm.h | 2 ++ > 3 files changed, 4 insertions(+), 2 deletions(-) > > 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/fs/dax.c b/fs/dax.c > index 3e457a16c7d1..c93191cd4802 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -1456,7 +1456,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); > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 1f79667824eb..e4c974587659 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3217,6 +3217,8 @@ static inline vm_fault_t vmf_error(int err) > { > if (err == -ENOMEM) > return VM_FAULT_OOM; > + else if (err == -EHWPOISON) > + return VM_FAULT_HWPOISON; > return VM_FAULT_SIGBUS; > } >
Jane Chu wrote: > When dax fault handler fails to provision the fault page due to > hwpoison, it returns VM_FAULT_SIGBUS which lead to a sigbus delivered > to userspace with .si_code BUS_ADRERR. Channel dax backend driver's > detection on hwpoison to the filesystem to provide the precise reason > for the fault. It's not yet clear to me by this description why this is an improvement or will not cause other confusion. In this case the reason for the SIGBUS is because the driver wants to prevent access to poison, not that the CPU consumed poison. Can you clarify what is lost by *not* making this change? > > Signed-off-by: Jane Chu <jane.chu@oracle.com> > --- > drivers/nvdimm/pmem.c | 2 +- > fs/dax.c | 2 +- > include/linux/mm.h | 2 ++ > 3 files changed, 4 insertions(+), 2 deletions(-) > > 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/fs/dax.c b/fs/dax.c > index 3e457a16c7d1..c93191cd4802 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -1456,7 +1456,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); This change results in EHWPOISON leaking to usersapce in the case of read(2), that's not a return code that block I/O applications have ever had to contend with before. Just as badblocks cause EIO to be returned, so should poisoned cachelines for pmem. > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 1f79667824eb..e4c974587659 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3217,6 +3217,8 @@ static inline vm_fault_t vmf_error(int err) > { > if (err == -ENOMEM) > return VM_FAULT_OOM; > + else if (err == -EHWPOISON) > + return VM_FAULT_HWPOISON; > return VM_FAULT_SIGBUS; > } > > -- > 2.18.4 > >
Hi, Dan, On 4/27/2023 2:36 PM, Dan Williams wrote: > Jane Chu wrote: >> When dax fault handler fails to provision the fault page due to >> hwpoison, it returns VM_FAULT_SIGBUS which lead to a sigbus delivered >> to userspace with .si_code BUS_ADRERR. Channel dax backend driver's >> detection on hwpoison to the filesystem to provide the precise reason >> for the fault. > > It's not yet clear to me by this description why this is an improvement > or will not cause other confusion. In this case the reason for the > SIGBUS is because the driver wants to prevent access to poison, not that > the CPU consumed poison. Can you clarify what is lost by *not* making > this change? Elsewhere when hwpoison is detected by page fault handler and helpers as the direct cause to failure, VM_FAULT_HWPOISON or VM_FAULT_HWPOISON_LARGE is flagged to ensure accurate SIGBUS payload is produced, such as wp_page_copy() in COW case, do_swap_page() from handle_pte_fault(), hugetlb_fault() in hugetlb page fault case where the huge fault size would be indicated in the payload. But dax fault has been an exception in that the SIGBUS payload does not indicate poison, nor fault size. I don't see why it should be though, recall an internal user expressing confusion regarding the different SIGBUS payloads. > >> >> Signed-off-by: Jane Chu <jane.chu@oracle.com> >> --- >> drivers/nvdimm/pmem.c | 2 +- >> fs/dax.c | 2 +- >> include/linux/mm.h | 2 ++ >> 3 files changed, 4 insertions(+), 2 deletions(-) >> >> 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/fs/dax.c b/fs/dax.c >> index 3e457a16c7d1..c93191cd4802 100644 >> --- a/fs/dax.c >> +++ b/fs/dax.c >> @@ -1456,7 +1456,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); > > This change results in EHWPOISON leaking to usersapce in the case of > read(2), that's not a return code that block I/O applications have ever > had to contend with before. Just as badblocks cause EIO to be returned, > so should poisoned cachelines for pmem. The read(2) man page (https://man.archlinux.org/man/read.2) says "On error, -1 is returned, and errno is set to indicate the error. In this case, it is left unspecified whether the file position (if any) changes." If read(2) users haven't dealt with EHWPOISON before, they may discover that with pmem backed dax file, it's possible. Thanks! -jane
Hi, Dan, On 4/27/2023 2:36 PM, Dan Williams wrote: > Jane Chu wrote: >> When dax fault handler fails to provision the fault page due to >> hwpoison, it returns VM_FAULT_SIGBUS which lead to a sigbus delivered >> to userspace with .si_code BUS_ADRERR. Channel dax backend driver's >> detection on hwpoison to the filesystem to provide the precise reason >> for the fault. > > It's not yet clear to me by this description why this is an improvement > or will not cause other confusion. In this case the reason for the > SIGBUS is because the driver wants to prevent access to poison, not that > the CPU consumed poison. Can you clarify what is lost by *not* making > this change? Elsewhere when hwpoison is detected by page fault handler and helpers as the direct cause to failure, VM_FAULT_HWPOISON or VM_FAULT_HWPOISON_LARGE is flagged to ensure accurate SIGBUS payload is produced, such as wp_page_copy() in COW case, do_swap_page() from handle_pte_fault(), hugetlb_fault() in hugetlb page fault case where the huge fault size would be indicated in the payload. But dax fault has been an exception in that the SIGBUS payload does not indicate poison, nor fault size. I don't see why it should be though, recall an internal user expressing confusion regarding the different SIGBUS payloads. > >> >> Signed-off-by: Jane Chu <jane.chu@oracle.com> >> --- >> drivers/nvdimm/pmem.c | 2 +- >> fs/dax.c | 2 +- >> include/linux/mm.h | 2 ++ >> 3 files changed, 4 insertions(+), 2 deletions(-) >> >> 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/fs/dax.c b/fs/dax.c >> index 3e457a16c7d1..c93191cd4802 100644 >> --- a/fs/dax.c >> +++ b/fs/dax.c >> @@ -1456,7 +1456,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); > > This change results in EHWPOISON leaking to usersapce in the case of > read(2), that's not a return code that block I/O applications have ever > had to contend with before. Just as badblocks cause EIO to be returned, > so should poisoned cachelines for pmem. The read(2) man page (https://man.archlinux.org/man/read.2) says "On error, -1 is returned, and errno is set to indicate the error. In this case, it is left unspecified whether the file position (if any) changes." If users haven't dealt with EHWPOISON before, they may discover that with pmem backed dax, it's possible. Thanks! -jane
On Thu, Apr 27, 2023 at 04:36:58PM -0700, Jane Chu wrote: > > This change results in EHWPOISON leaking to usersapce in the case of > > read(2), that's not a return code that block I/O applications have ever > > had to contend with before. Just as badblocks cause EIO to be returned, > > so should poisoned cachelines for pmem. > > The read(2) man page (https://man.archlinux.org/man/read.2) says > "On error, -1 is returned, and errno is set to indicate the error. In this > case, it is left unspecified whether the file position (if any) changes." > > If read(2) users haven't dealt with EHWPOISON before, they may discover that > with pmem backed dax file, it's possible. I don't think they should. While syscalls are allowed to return errnos other than the ones listed in POSIX, I don't think this is a worthwhile difference. We should be abstracting from the user that this is pmem rather than spinning rust or nand. So we should convert the EHWPOISON to EIO as Dan suggests.
On 4/27/2023 4:48 PM, Matthew Wilcox wrote: > On Thu, Apr 27, 2023 at 04:36:58PM -0700, Jane Chu wrote: >>> This change results in EHWPOISON leaking to usersapce in the case of >>> read(2), that's not a return code that block I/O applications have ever >>> had to contend with before. Just as badblocks cause EIO to be returned, >>> so should poisoned cachelines for pmem. >> >> The read(2) man page (https://man.archlinux.org/man/read.2) says >> "On error, -1 is returned, and errno is set to indicate the error. In this >> case, it is left unspecified whether the file position (if any) changes." >> >> If read(2) users haven't dealt with EHWPOISON before, they may discover that >> with pmem backed dax file, it's possible. > > I don't think they should. While syscalls are allowed to return errnos > other than the ones listed in POSIX, I don't think this is a worthwhile > difference. We should be abstracting from the user that this is pmem > rather than spinning rust or nand. So we should convert the EHWPOISON > to EIO as Dan suggests. Got it, I'll add errno conversion in the respin. thanks, -jane
Jane Chu wrote: > Hi, Dan, > > On 4/27/2023 2:36 PM, Dan Williams wrote: > > Jane Chu wrote: > >> When dax fault handler fails to provision the fault page due to > >> hwpoison, it returns VM_FAULT_SIGBUS which lead to a sigbus delivered > >> to userspace with .si_code BUS_ADRERR. Channel dax backend driver's > >> detection on hwpoison to the filesystem to provide the precise reason > >> for the fault. > > > > It's not yet clear to me by this description why this is an improvement > > or will not cause other confusion. In this case the reason for the > > SIGBUS is because the driver wants to prevent access to poison, not that > > the CPU consumed poison. Can you clarify what is lost by *not* making > > this change? > > Elsewhere when hwpoison is detected by page fault handler and helpers as > the direct cause to failure, VM_FAULT_HWPOISON or > VM_FAULT_HWPOISON_LARGE is flagged to ensure accurate SIGBUS payload is > produced, such as wp_page_copy() in COW case, do_swap_page() from > handle_pte_fault(), hugetlb_fault() in hugetlb page fault case where the > huge fault size would be indicated in the payload. > > But dax fault has been an exception in that the SIGBUS payload does not > indicate poison, nor fault size. I don't see why it should be though, > recall an internal user expressing confusion regarding the different > SIGBUS payloads. ...but again this the typical behavior with block devices. If a block device has badblock that causes page cache page not to be populated that's a SIGBUS without hwpoison information. If the page cache is properly populated and then the CPU consumes poison that's a SIGBUS with the additional hwpoison information. Applications should have a consistent error response regardless of pmem or dax.
On Thu, Apr 27, 2023 at 06:35:57PM -0700, Dan Williams wrote: > Jane Chu wrote: > > Hi, Dan, > > > > On 4/27/2023 2:36 PM, Dan Williams wrote: > > > Jane Chu wrote: > > >> When dax fault handler fails to provision the fault page due to > > >> hwpoison, it returns VM_FAULT_SIGBUS which lead to a sigbus delivered > > >> to userspace with .si_code BUS_ADRERR. Channel dax backend driver's > > >> detection on hwpoison to the filesystem to provide the precise reason > > >> for the fault. > > > > > > It's not yet clear to me by this description why this is an improvement > > > or will not cause other confusion. In this case the reason for the > > > SIGBUS is because the driver wants to prevent access to poison, not that > > > the CPU consumed poison. Can you clarify what is lost by *not* making > > > this change? > > > > Elsewhere when hwpoison is detected by page fault handler and helpers as > > the direct cause to failure, VM_FAULT_HWPOISON or > > VM_FAULT_HWPOISON_LARGE is flagged to ensure accurate SIGBUS payload is > > produced, such as wp_page_copy() in COW case, do_swap_page() from > > handle_pte_fault(), hugetlb_fault() in hugetlb page fault case where the > > huge fault size would be indicated in the payload. > > > > But dax fault has been an exception in that the SIGBUS payload does not > > indicate poison, nor fault size. I don't see why it should be though, > > recall an internal user expressing confusion regarding the different > > SIGBUS payloads. > > ...but again this the typical behavior with block devices. If a block > device has badblock that causes page cache page not to be populated > that's a SIGBUS without hwpoison information. If the page cache is > properly populated and then the CPU consumes poison that's a SIGBUS with > the additional hwpoison information. I'm not sure that's true when we mmap(). Yes, it's not consistent with -EIO from read(), but we have additional information here, and it's worth providing it. You can think of it as *in this instance*, the error is found "in the page cache", because that's effectively where the error is from the point of view of the application?
Matthew Wilcox wrote: > On Thu, Apr 27, 2023 at 06:35:57PM -0700, Dan Williams wrote: > > Jane Chu wrote: > > > Hi, Dan, > > > > > > On 4/27/2023 2:36 PM, Dan Williams wrote: > > > > Jane Chu wrote: > > > >> When dax fault handler fails to provision the fault page due to > > > >> hwpoison, it returns VM_FAULT_SIGBUS which lead to a sigbus delivered > > > >> to userspace with .si_code BUS_ADRERR. Channel dax backend driver's > > > >> detection on hwpoison to the filesystem to provide the precise reason > > > >> for the fault. > > > > > > > > It's not yet clear to me by this description why this is an improvement > > > > or will not cause other confusion. In this case the reason for the > > > > SIGBUS is because the driver wants to prevent access to poison, not that > > > > the CPU consumed poison. Can you clarify what is lost by *not* making > > > > this change? > > > > > > Elsewhere when hwpoison is detected by page fault handler and helpers as > > > the direct cause to failure, VM_FAULT_HWPOISON or > > > VM_FAULT_HWPOISON_LARGE is flagged to ensure accurate SIGBUS payload is > > > produced, such as wp_page_copy() in COW case, do_swap_page() from > > > handle_pte_fault(), hugetlb_fault() in hugetlb page fault case where the > > > huge fault size would be indicated in the payload. > > > > > > But dax fault has been an exception in that the SIGBUS payload does not > > > indicate poison, nor fault size. I don't see why it should be though, > > > recall an internal user expressing confusion regarding the different > > > SIGBUS payloads. > > > > ...but again this the typical behavior with block devices. If a block > > device has badblock that causes page cache page not to be populated > > that's a SIGBUS without hwpoison information. If the page cache is > > properly populated and then the CPU consumes poison that's a SIGBUS with > > the additional hwpoison information. > > I'm not sure that's true when we mmap(). Yes, it's not consistent with > -EIO from read(), but we have additional information here, and it's worth > providing it. You can think of it as *in this instance*, the error is > found "in the page cache", because that's effectively where the error > is from the point of view of the application? It's true there is additional information, and applications mostly cannot tell the difference between fault on failure to populate and fault on access after populate. So while it is inconsistent with what happens for typical page cache, but you're right there's no downside to conveying the extra information here.
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/fs/dax.c b/fs/dax.c index 3e457a16c7d1..c93191cd4802 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -1456,7 +1456,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); diff --git a/include/linux/mm.h b/include/linux/mm.h index 1f79667824eb..e4c974587659 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3217,6 +3217,8 @@ static inline vm_fault_t vmf_error(int err) { if (err == -ENOMEM) return VM_FAULT_OOM; + else if (err == -EHWPOISON) + return VM_FAULT_HWPOISON; return VM_FAULT_SIGBUS; }
When dax fault handler fails to provision the fault page due to hwpoison, it returns VM_FAULT_SIGBUS which lead to a sigbus delivered to userspace with .si_code BUS_ADRERR. Channel dax backend driver's detection on hwpoison to the filesystem to provide the precise reason for the fault. Signed-off-by: Jane Chu <jane.chu@oracle.com> --- drivers/nvdimm/pmem.c | 2 +- fs/dax.c | 2 +- include/linux/mm.h | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-)