Re: [PATCH] pmem: report error on clear poison failure
On Thu, Oct 13, 2016 at 11:16 AM, Kani, Toshimitsu wrote: > On Thu, 2016-10-13 at 10:22 -0700, Dan Williams wrote: >> On Thu, Oct 13, 2016 at 9:08 AM, Kani, Toshimitsu > > wrote: >> > >> > On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote: >> > > >> > > On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani >> > > wrote: >> > > > >> > > > >> > > > ACPI Clear Uncorrectable Error DSM function may fail or may be >> > > > unsupported on a platform. pmem_clear_poison() returns without >> > > > clearing badblocks in such cases, which leads to a silent data >> > > > corruption. >> > > > >> > > > Change pmem_do_bvec() and pmem_clear_poison() to return -EIO >> > > > so that filesystem can log an error message. >> > > >> > > What's the silent data corruption scenario? If the clear poison >> > > fails I'm assuming that the poison will still be notified on the >> > > next >> > > read. >> > >> > I agree that the data is eventually read, but there is no guranteed >> > that when it is read soon enough, i.e. user might not access to the >> > data for a long time. >> >> ...but that's the same behavior for errors that we don't yet know >> about. That said, we indeed know that the write failed. I'd feel >> better about this patch if the justification / impact was clearer in >> the changelog, because "silent data corruption" is not the impact. > > Agreed. How about the following descritpion? > > === > ACPI Clear Uncorrectable Error DSM function may fail or may be > unsupported on a platform. pmem_clear_poison() returns without > clearing badblocks in such cases. This failure is detected at > the next read (-EIO). > > This behavior can lead to an issue when user keeps writing but > does not read immedicately. For instance, flight recorder file > may be only read when it is necessary for troubleshooting. > > Change pmem_do_bvec() and pmem_clear_poison() to return -EIO > so that filesystem can log an error message on a write error. > === Looks good, thanks Toshi. I'll update the nvdimm.git branches after -rc1 is out.
Re: [PATCH] pmem: report error on clear poison failure
On Thu, Oct 13, 2016 at 06:16:29PM +, Kani, Toshimitsu wrote: > On Thu, 2016-10-13 at 10:22 -0700, Dan Williams wrote: > > On Thu, Oct 13, 2016 at 9:08 AM, Kani, Toshimitsu > > wrote: > > > > > > On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote: > > > > > > > > On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani > > > > wrote: > > > > > > > > > > > > > > > ACPI Clear Uncorrectable Error DSM function may fail or may be > > > > > unsupported on a platform. pmem_clear_poison() returns without > > > > > clearing badblocks in such cases, which leads to a silent data > > > > > corruption. > > > > > > > > > > Change pmem_do_bvec() and pmem_clear_poison() to return -EIO > > > > > so that filesystem can log an error message. > > > > > > > > What's the silent data corruption scenario? If the clear poison > > > > fails I'm assuming that the poison will still be notified on the > > > > next > > > > read. > > > > > > I agree that the data is eventually read, but there is no guranteed > > > that when it is read soon enough, i.e. user might not access to the > > > data for a long time. > > > > ...but that's the same behavior for errors that we don't yet know > > about. That said, we indeed know that the write failed. I'd feel > > better about this patch if the justification / impact was clearer in > > the changelog, because "silent data corruption" is not the impact. > > Agreed. How about the following descritpion? > > === > ACPI Clear Uncorrectable Error DSM function may fail or may be > unsupported on a platform. pmem_clear_poison() returns without > clearing badblocks in such cases. This failure is detected at > the next read (-EIO). > > This behavior can lead to an issue when user keeps writing but > does not read immedicately. For instance, flight recorder file immediately > may be only read when it is necessary for troubleshooting. > > Change pmem_do_bvec() and pmem_clear_poison() to return -EIO > so that filesystem can log an error message on a write error. > === > > Thanks, > -Toshi > ___ > Linux-nvdimm mailing list > linux-nvd...@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] pmem: report error on clear poison failure
On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote: > On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani > wrote: > > > > ACPI Clear Uncorrectable Error DSM function may fail or may be > > unsupported on a platform. pmem_clear_poison() returns without > > clearing badblocks in such cases, which leads to a silent data > > corruption. > > > > Change pmem_do_bvec() and pmem_clear_poison() to return -EIO > > so that filesystem can log an error message. > > What's the silent data corruption scenario? If the clear poison > fails I'm assuming that the poison will still be notified on the next > read. I agree that the data is eventually read, but there is no guranteed that when it is read soon enough, i.e. user might not access to the data for a long time. Thanks, -Toshi
Re: [PATCH] pmem: report error on clear poison failure
On Thu, 2016-10-13 at 10:22 -0700, Dan Williams wrote: > On Thu, Oct 13, 2016 at 9:08 AM, Kani, Toshimitsu > wrote: > > > > On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote: > > > > > > On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani > > > wrote: > > > > > > > > > > > > ACPI Clear Uncorrectable Error DSM function may fail or may be > > > > unsupported on a platform. pmem_clear_poison() returns without > > > > clearing badblocks in such cases, which leads to a silent data > > > > corruption. > > > > > > > > Change pmem_do_bvec() and pmem_clear_poison() to return -EIO > > > > so that filesystem can log an error message. > > > > > > What's the silent data corruption scenario? If the clear poison > > > fails I'm assuming that the poison will still be notified on the > > > next > > > read. > > > > I agree that the data is eventually read, but there is no guranteed > > that when it is read soon enough, i.e. user might not access to the > > data for a long time. > > ...but that's the same behavior for errors that we don't yet know > about. That said, we indeed know that the write failed. I'd feel > better about this patch if the justification / impact was clearer in > the changelog, because "silent data corruption" is not the impact. Agreed. How about the following descritpion? === ACPI Clear Uncorrectable Error DSM function may fail or may be unsupported on a platform. pmem_clear_poison() returns without clearing badblocks in such cases. This failure is detected at the next read (-EIO). This behavior can lead to an issue when user keeps writing but does not read immedicately. For instance, flight recorder file may be only read when it is necessary for troubleshooting. Change pmem_do_bvec() and pmem_clear_poison() to return -EIO so that filesystem can log an error message on a write error. === Thanks, -Toshi
Re: [PATCH] pmem: report error on clear poison failure
On Thu, Oct 13, 2016 at 9:08 AM, Kani, Toshimitsu wrote: > On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote: >> On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani >> wrote: >> > >> > ACPI Clear Uncorrectable Error DSM function may fail or may be >> > unsupported on a platform. pmem_clear_poison() returns without >> > clearing badblocks in such cases, which leads to a silent data >> > corruption. >> > >> > Change pmem_do_bvec() and pmem_clear_poison() to return -EIO >> > so that filesystem can log an error message. >> >> What's the silent data corruption scenario? If the clear poison >> fails I'm assuming that the poison will still be notified on the next >> read. > > I agree that the data is eventually read, but there is no guranteed > that when it is read soon enough, i.e. user might not access to the > data for a long time. ...but that's the same behavior for errors that we don't yet know about. That said, we indeed know that the write failed. I'd feel better about this patch if the justification / impact was clearer in the changelog, because "silent data corruption" is not the impact.
Re: [PATCH] pmem: report error on clear poison failure
On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani wrote: > ACPI Clear Uncorrectable Error DSM function may fail or may be > unsupported on a platform. pmem_clear_poison() returns without > clearing badblocks in such cases, which leads to a silent data > corruption. > > Change pmem_do_bvec() and pmem_clear_poison() to return -EIO > so that filesystem can log an error message. What's the silent data corruption scenario? If the clear poison fails I'm assuming that the poison will still be notified on the next read.
[PATCH] pmem: report error on clear poison failure
ACPI Clear Uncorrectable Error DSM function may fail or may be
unsupported on a platform. pmem_clear_poison() returns without
clearing badblocks in such cases, which leads to a silent data
corruption.
Change pmem_do_bvec() and pmem_clear_poison() to return -EIO
so that filesystem can log an error message.
Signed-off-by: Toshi Kani
Cc: Dan Williams
Cc: Vishal Verma
---
drivers/nvdimm/pmem.c |8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 42b3a82..2461843 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -47,7 +47,7 @@ static struct nd_region *to_region(struct pmem_device *pmem)
return to_nd_region(to_dev(pmem)->parent);
}
-static void pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset,
+static int pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset,
unsigned int len)
{
struct device *dev = to_dev(pmem);
@@ -62,8 +62,12 @@ static void pmem_clear_poison(struct pmem_device *pmem,
phys_addr_t offset,
__func__, (unsigned long long) sector,
cleared / 512, cleared / 512 > 1 ? "s" : "");
badblocks_clear(&pmem->bb, sector, cleared / 512);
+ } else {
+ return -EIO;
}
+
invalidate_pmem(pmem->virt_addr + offset, len);
+ return 0;
}
static void write_pmem(void *pmem_addr, struct page *page,
@@ -123,7 +127,7 @@ static int pmem_do_bvec(struct pmem_device *pmem, struct
page *page,
flush_dcache_page(page);
write_pmem(pmem_addr, page, off, len);
if (unlikely(bad_pmem)) {
- pmem_clear_poison(pmem, pmem_off, len);
+ rc = pmem_clear_poison(pmem, pmem_off, len);
write_pmem(pmem_addr, page, off, len);
}
}
