[PATCH] pmem: report error on clear poison failure

[PATCH] pmem: report error on clear poison failure

[Toshi Kani]

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 <toshi.kani@hpe.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
---
 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);
 		}
 	}

Re: [PATCH] pmem: report error on clear poison failure

[Dan Williams]

On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani <toshi.kani@hpe.com> 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.

Re: [PATCH] pmem: report error on clear poison failure

[Kani, Toshimitsu]

On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote:
> On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani <toshi.kani@hpe.com>
> 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

[Dan Williams]

On Thu, Oct 13, 2016 at 9:08 AM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote:
>> On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani <toshi.kani@hpe.com>
>> 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

[Kani, Toshimitsu]

On Thu, 2016-10-13 at 10:22 -0700, Dan Williams wrote:
> On Thu, Oct 13, 2016 at 9:08 AM, Kani, Toshimitsu <toshi.kani@hpe.com
> > wrote:
> > 
> > On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote:
> > > 
> > > On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani <toshi.kani@hpe.com>
> > > 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

[Ross Zwisler]

On Thu, Oct 13, 2016 at 06:16:29PM +0000, 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 <toshi.kani@hpe.com
> > > wrote:
> > > 
> > > On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote:
> > > > 
> > > > On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani <toshi.kani@hpe.com>
> > > > 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-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] pmem: report error on clear poison failure

[Dan Williams]

On Thu, Oct 13, 2016 at 11:16 AM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> On Thu, 2016-10-13 at 10:22 -0700, Dan Williams wrote:
>> On Thu, Oct 13, 2016 at 9:08 AM, Kani, Toshimitsu <toshi.kani@hpe.com
>> > wrote:
>> >
>> > On Thu, 2016-10-13 at 09:01 -0700, Dan Williams wrote:
>> > >
>> > > On Thu, Oct 13, 2016 at 8:54 AM, Toshi Kani <toshi.kani@hpe.com>
>> > > 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.