[ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Vishal Verma]

If we have poison/badblocks in the BTT metadata sections, the mmap-reads
happening in the checker will trigger a SIGBUS, and the program will
halt abruptly. Add a sigbus handler which notifies the user of this, and
prints out a relevant error message:

  namespace5.0: namespace_check: checking namespace5.0
  namespace5.0: btt_discover_arenas: found 1 BTT arena
  namespace5.0: btt_check_arenas: checking arena 0
  namespace5.0: namespace_check: Received a SIGBUS
  namespace5.0: namespace_check: Metadata corruption found, recovery is not possible
  error checking namespaces: Bad address

Cc: Dan Williams <dan.j.williams@intel.com>
Reported-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/check.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/ndctl/check.c b/ndctl/check.c
index 3b30a98..3775c2e 100644
--- a/ndctl/check.c
+++ b/ndctl/check.c
@@ -13,6 +13,8 @@
 #include <stdio.h>
 #include <fcntl.h>
 #include <errno.h>
+#include <setjmp.h>
+#include <signal.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -40,6 +42,13 @@
 #include <ndctl.h>
 #endif
 
+static sigjmp_buf sj_env;
+
+static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
+{
+	siglongjmp(sj_env, 1);
+}
+
 static int repair_msg(struct btt_chk *bttc)
 {
 	info(bttc, "  Run with --repair to make the changes\n");
@@ -872,6 +881,7 @@ int namespace_check(struct ndctl_namespace *ndns, struct check_opts *opts)
 	int raw_mode, rc, disabled_flag = 0, open_flags;
 	struct btt_sb *btt_sb;
 	struct btt_chk *bttc;
+	struct sigaction act;
 	char path[50];
 
 	bttc = calloc(1, sizeof(*bttc));
@@ -882,6 +892,15 @@ int namespace_check(struct ndctl_namespace *ndns, struct check_opts *opts)
 	if (opts->verbose)
 		bttc->ctx.log_priority = LOG_DEBUG;
 
+	memset(&act, 0, sizeof(act));
+	act.sa_sigaction = sigbus_hdl;
+	act.sa_flags = SA_SIGINFO;
+
+	if (sigaction(SIGBUS, &act, 0)) {
+		err(bttc, "Unable to set sigaction\n");
+		return -errno;
+	}
+
 	bttc->opts = opts;
 	bttc->start_off = BTT_START_OFFSET;
 	bttc->sys_page_size = sysconf(_SC_PAGESIZE);
@@ -949,6 +968,18 @@ int namespace_check(struct ndctl_namespace *ndns, struct check_opts *opts)
 		goto out_sb;
 	}
 
+	/*
+	 * This is where we jump to if we receive a SIGBUS, prior to doing any
+	 * mmaped reads, and can safely abort
+	 */
+	if (sigsetjmp(sj_env, 1)) {
+		err(bttc, "Received a SIGBUS\n");
+		err(bttc,
+			"Metadata corruption found, recovery is not possible\n");
+		rc = -EFAULT;
+		goto out_close;
+	}
+
 	rc = btt_info_read_verify(bttc, btt_sb, bttc->start_off);
 	if (rc) {
 		rc = btt_recover_first_sb(bttc);
-- 
2.9.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Rudoff, Andy]

> If we have poison/badblocks in the BTT metadata sections, the mmap-reads
> happening in the checker will trigger a SIGBUS, and the program will
> halt abruptly. Add a sigbus handler which notifies the user of this, and
> prints out a relevant error message:
> 
>  namespace5.0: namespace_check: checking namespace5.0
>  namespace5.0: btt_discover_arenas: found 1 BTT arena
>  namespace5.0: btt_check_arenas: checking arena 0
>  namespace5.0: namespace_check: Received a SIGBUS
>  namespace5.0: namespace_check: Metadata corruption found, recovery is not possible
>  error checking namespaces: Bad address

If in repair mode, should a SIGBUS cause you to set the error state in the associated arena?

-andy

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Verma, Vishal L]

On Fri, 2017-04-14 at 00:10 +0000, Rudoff, Andy wrote:
> > If we have poison/badblocks in the BTT metadata sections, the mmap-
> > reads
> > happening in the checker will trigger a SIGBUS, and the program
> > will
> > halt abruptly. Add a sigbus handler which notifies the user of
> > this, and
> > prints out a relevant error message:
> > 
> >  namespace5.0: namespace_check: checking namespace5.0
> >  namespace5.0: btt_discover_arenas: found 1 BTT arena
> >  namespace5.0: btt_check_arenas: checking arena 0
> >  namespace5.0: namespace_check: Received a SIGBUS
> >  namespace5.0: namespace_check: Metadata corruption found, recovery
> > is not possible
> >  error checking namespaces: Bad address
> 
> If in repair mode, should a SIGBUS cause you to set the error state
> in the associated arena?
> 
> -andy
> 
So I thought a bit about this -
While we can set the error state, that leaves the BTT with no way to
get any data off it (currently).

If we allow the kernel to enable the namespace, the user has some
chance to do at least partial recovery until they hit the badblock.
(assuming the badblock is in the map, which is the largest on-disk
structure). If it is on the flog or info block, then, yes, there is no
recovery.

	-Vishal
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Dan Williams]

On Fri, Apr 14, 2017 at 12:00 PM, Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
> On Fri, 2017-04-14 at 00:10 +0000, Rudoff, Andy wrote:
>> > If we have poison/badblocks in the BTT metadata sections, the mmap-
>> > reads
>> > happening in the checker will trigger a SIGBUS, and the program
>> > will
>> > halt abruptly. Add a sigbus handler which notifies the user of
>> > this, and
>> > prints out a relevant error message:
>> >
>> >  namespace5.0: namespace_check: checking namespace5.0
>> >  namespace5.0: btt_discover_arenas: found 1 BTT arena
>> >  namespace5.0: btt_check_arenas: checking arena 0
>> >  namespace5.0: namespace_check: Received a SIGBUS
>> >  namespace5.0: namespace_check: Metadata corruption found, recovery
>> > is not possible
>> >  error checking namespaces: Bad address
>>
>> If in repair mode, should a SIGBUS cause you to set the error state
>> in the associated arena?
>>
>> -andy
>>
> So I thought a bit about this -
> While we can set the error state, that leaves the BTT with no way to
> get any data off it (currently).
>
> If we allow the kernel to enable the namespace, the user has some
> chance to do at least partial recovery until they hit the badblock.
> (assuming the badblock is in the map, which is the largest on-disk
> structure). If it is on the flog or info block, then, yes, there is no
> recovery.

Also, the kernel currently does nothing with the error bit besides
print out that it saw it.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Rudoff, Andy]

    On Fri, Apr 14, 2017 at 12:00 PM, Verma, Vishal L
    <vishal.l.verma@intel.com> wrote:
    > On Fri, 2017-04-14 at 00:10 +0000, Rudoff, Andy wrote:
    >> > If we have poison/badblocks in the BTT metadata sections, the mmap-
    >> > reads
    >> > happening in the checker will trigger a SIGBUS, and the program
    >> > will
    >> > halt abruptly. Add a sigbus handler which notifies the user of
    >> > this, and
    >> > prints out a relevant error message:
    >> >
    >> >  namespace5.0: namespace_check: checking namespace5.0
    >> >  namespace5.0: btt_discover_arenas: found 1 BTT arena
    >> >  namespace5.0: btt_check_arenas: checking arena 0
    >> >  namespace5.0: namespace_check: Received a SIGBUS
    >> >  namespace5.0: namespace_check: Metadata corruption found, recovery
    >> > is not possible
    >> >  error checking namespaces: Bad address
    >>
    >> If in repair mode, should a SIGBUS cause you to set the error state
    >> in the associated arena?
    >>
    >> -andy
    >>
    > So I thought a bit about this -
    > While we can set the error state, that leaves the BTT with no way to
    > get any data off it (currently).
    >
    > If we allow the kernel to enable the namespace, the user has some
    > chance to do at least partial recovery until they hit the badblock.
    > (assuming the badblock is in the map, which is the largest on-disk
    > structure). If it is on the flog or info block, then, yes, there is no
    > recovery.
    
Yeah, I thought about this a little more too.  Here’s what I think should happen:

Without --repair, I agree with this patch, although the error message might
tell the poor user that running with --repair is the next step if they want to
try to fix it (see below).

With --repair, the repair code should try to replace the poison with
repaired metadata, clearing the appropriate error bits when done.

For example, if the poison is in the map, the repair code should figure
out which entries have no map entry, clear the poison, and update the
map, printing a strong warning that the blocks were repaired but may
not be in the right place any more.  If the poison is in something like
the arena info block, a replacement can be constructed from the
backup info.  The point is that the tool shouldn’t die on poison, it
should be the way you fix it and get rid of the poison.

Finally, the kernel should be setting and honoring the error bits, both
in map entries and arena info blocks.  When poison is discovered in
metadata, the arena info block error state should be set and that
should not disallow getting to any available data as you stated, but
should instead make that arena read-only.  The only way to get out
of read-only mode for the arena should be to run the check tool
with --repair and all the stuff I said above then happens.
 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Verma, Vishal L]

On Fri, 2017-04-14 at 19:26 +0000, Rudoff, Andy wrote:
>     On Fri, Apr 14, 2017 at 12:00 PM, Verma, Vishal L
[...]
>     
> Yeah, I thought about this a little more too.  Here’s what I think
> should happen:
> 
> Without --repair, I agree with this patch, although the error message
> might
> tell the poor user that running with --repair is the next step if
> they want to
> try to fix it (see below).
> 
> With --repair, the repair code should try to replace the poison with
> repaired metadata, clearing the appropriate error bits when done.
> 
> For example, if the poison is in the map, the repair code should
> figure
> out which entries have no map entry, clear the poison, and update the
> map, printing a strong warning that the blocks were repaired but may
> not be in the right place any more.  

If we hit a known badblock, that is 512B worth of map entries (128).
Should we really (almost certainly) scramble 64 blocks? :)
If it is a latent error, it will still be at least a cache line worth
of map entries, i.e. 16.

If an error is in the log, then in the badblock case, we lose both log
and log' for four lanes. Which means we can't tell is one of those 4
entries needed a map update, leaving a potential corruption window
open.

> If the poison is in something like
> the arena info block, a replacement can be constructed from the
> backup info.  The point is that the tool shouldn’t die on poison, it
> should be the way you fix it and get rid of the poison.

This might be the only case where we can clear the poison
successfully..

> 
> Finally, the kernel should be setting and honoring the error bits,
> both
> in map entries and arena info blocks.  When poison is discovered in
> metadata, the arena info block error state should be set and that
> should not disallow getting to any available data as you stated, but
> should instead make that arena read-only.  

Agreed about the error bit and read-only, I will look at that.

> The only way to get out
> of read-only mode for the arena should be to run the check tool
> with --repair and all the stuff I said above then happens.
>  
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Rudoff, Andy]

    >If we hit a known badblock, that is 512B worth of map entries (128).
    >Should we really (almost certainly) scramble 64 blocks? :)
    >If it is a latent error, it will still be at least a cache line worth
    >of map entries, i.e. 16.
    >
    >If an error is in the log, then in the badblock case, we lose both log
    >and log' for four lanes. Which means we can't tell is one of those 4
    >entries needed a map update, leaving a potential corruption window
    >open.
    
Before responding to this, can you walk me through the steps
a user is expected to take if poison appears the flog, for example?
Say I just want to copy out as much data as I can get, then recreate
a fresh BTT.  I think you’re telling me that a poisoned flog prevents
me from using the entire device, but maybe I’m wrong and you’re
saying I can still get to the other arenas?

Anyway, after copying out what I can, how do I recreate the BTT?
When the BTT creation code starts writing the new flog, will that
clear the poison?  Does the BTT creation code do big enough writes
so the driver will issue Clear Uncorrectable commands?

With your answers, I think we can create a typical user flow,
where the user gets error messages and tries to use our tools
to fix things.  That should tell us if we’re on the right track
or if we’re stranding the poor user with no way to recover…

Thanks,

-andy


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Verma, Vishal L]

On Fri, 2017-04-14 at 19:52 +0000, Rudoff, Andy wrote:
>     >If we hit a known badblock, that is 512B worth of map entries
> (128).
>     >Should we really (almost certainly) scramble 64 blocks? :)
>     >If it is a latent error, it will still be at least a cache line
> worth
>     >of map entries, i.e. 16.
>     >
>     >If an error is in the log, then in the badblock case, we lose
> both log
>     >and log' for four lanes. Which means we can't tell is one of
> those 4
>     >entries needed a map update, leaving a potential corruption
> window
>     >open.
>     
> Before responding to this, can you walk me through the steps
> a user is expected to take if poison appears the flog, for example?
> Say I just want to copy out as much data as I can get, then recreate
> a fresh BTT.  I think you’re telling me that a poisoned flog prevents
> me from using the entire device, but maybe I’m wrong and you’re
> saying I can still get to the other arenas?

So I think currently the best way is to recreate the namespace and
restore from a backup.. I wonder if we have a problem hiding even here,
but more on that below.
I don't think all access will be prevented, even to that arena, and
certainly not the whole device. My point was only that we can't clear
the poison, because then all notification of "something went wrong" is
lost.

One option could be to clear the bock of poisoned map entries, and
replace them with 'error' entries (the error bit in each map entry).
That way reads will continue to fail, and writes to those entries will
fix them. I'll have to work through it and make sure this works on the
kernel side first..

> 
> Anyway, after copying out what I can, how do I recreate the BTT?
> When the BTT creation code starts writing the new flog, will that
> clear the poison?  Does the BTT creation code do big enough writes
> so the driver will issue Clear Uncorrectable commands?

After 4.12, we will at least have the ability to clear poison for data
blocks. i.e. when the requests going through rw_bytes are aligned and
sized to 512B sectors. Map and flog writes are not large enough, and so
won't clear errors..
And maybe this is something we need to fix otherwise.

Consider a scenario where we had metadata poison, we delete and
recreate the namespace. This in itself doesn't hit the poison. We will
end up hitting CMCIs when we write metaddata and Uncorrectables on
reads again since the poison wasn't cleared.

Maybe what we need here is during BTT creation, check for at least
known badblocks, and initialize them so that we know at least new BTTs
are clean.

> 
> With your answers, I think we can create a typical user flow,
> where the user gets error messages and tries to use our tools
> to fix things.  That should tell us if we’re on the right track
> or if we’re stranding the poor user with no way to recover…

Agreed, current recovery path (from poison) sure involves slaying
dragons :)

> 
> Thanks,
> 
> -andy
> 
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Rudoff, Andy]

>Maybe what we need here is during BTT creation, check for at least
>known badblocks, and initialize them so that we know at least new BTTs
>are clean.

I totally agree with this one.  Sounds like a must-have to me.  The
idea of poison in a location that sticks around even after you recreate
the BTT makes me shudder.

Thanks,

-andy
 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Jeff Moyer]

Vishal Verma <vishal.l.verma@intel.com> writes:

> If we have poison/badblocks in the BTT metadata sections, the mmap-reads
> happening in the checker will trigger a SIGBUS, and the program will
> halt abruptly. Add a sigbus handler which notifies the user of this, and
> prints out a relevant error message:

This isn't much better than just segfaulting.  Did you consider whether
it was possible to reconstruct corrupt metadata in any circumstances?

-Jeff

>
>   namespace5.0: namespace_check: checking namespace5.0
>   namespace5.0: btt_discover_arenas: found 1 BTT arena
>   namespace5.0: btt_check_arenas: checking arena 0
>   namespace5.0: namespace_check: Received a SIGBUS
>   namespace5.0: namespace_check: Metadata corruption found, recovery is not possible
>   error checking namespaces: Bad address
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Reported-by: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl/check.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/ndctl/check.c b/ndctl/check.c
> index 3b30a98..3775c2e 100644
> --- a/ndctl/check.c
> +++ b/ndctl/check.c
> @@ -13,6 +13,8 @@
>  #include <stdio.h>
>  #include <fcntl.h>
>  #include <errno.h>
> +#include <setjmp.h>
> +#include <signal.h>
>  #include <stdint.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> @@ -40,6 +42,13 @@
>  #include <ndctl.h>
>  #endif
>  
> +static sigjmp_buf sj_env;
> +
> +static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
> +{
> +	siglongjmp(sj_env, 1);
> +}
> +
>  static int repair_msg(struct btt_chk *bttc)
>  {
>  	info(bttc, "  Run with --repair to make the changes\n");
> @@ -872,6 +881,7 @@ int namespace_check(struct ndctl_namespace *ndns, struct check_opts *opts)
>  	int raw_mode, rc, disabled_flag = 0, open_flags;
>  	struct btt_sb *btt_sb;
>  	struct btt_chk *bttc;
> +	struct sigaction act;
>  	char path[50];
>  
>  	bttc = calloc(1, sizeof(*bttc));
> @@ -882,6 +892,15 @@ int namespace_check(struct ndctl_namespace *ndns, struct check_opts *opts)
>  	if (opts->verbose)
>  		bttc->ctx.log_priority = LOG_DEBUG;
>  
> +	memset(&act, 0, sizeof(act));
> +	act.sa_sigaction = sigbus_hdl;
> +	act.sa_flags = SA_SIGINFO;
> +
> +	if (sigaction(SIGBUS, &act, 0)) {
> +		err(bttc, "Unable to set sigaction\n");
> +		return -errno;
> +	}
> +
>  	bttc->opts = opts;
>  	bttc->start_off = BTT_START_OFFSET;
>  	bttc->sys_page_size = sysconf(_SC_PAGESIZE);
> @@ -949,6 +968,18 @@ int namespace_check(struct ndctl_namespace *ndns, struct check_opts *opts)
>  		goto out_sb;
>  	}
>  
> +	/*
> +	 * This is where we jump to if we receive a SIGBUS, prior to doing any
> +	 * mmaped reads, and can safely abort
> +	 */
> +	if (sigsetjmp(sj_env, 1)) {
> +		err(bttc, "Received a SIGBUS\n");
> +		err(bttc,
> +			"Metadata corruption found, recovery is not possible\n");
> +		rc = -EFAULT;
> +		goto out_close;
> +	}
> +
>  	rc = btt_info_read_verify(bttc, btt_sb, bttc->start_off);
>  	if (rc) {
>  		rc = btt_recover_first_sb(bttc);
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl, check: Add a sigbus handler to detect metadata corruption

[Verma, Vishal L]

On Mon, 2017-04-17 at 11:37 -0400, Jeff Moyer wrote:
> Vishal Verma <vishal.l.verma@intel.com> writes:
> 
> > If we have poison/badblocks in the BTT metadata sections, the mmap-
> > reads
> > happening in the checker will trigger a SIGBUS, and the program
> > will
> > halt abruptly. Add a sigbus handler which notifies the user of
> > this, and
> > prints out a relevant error message:
> 
> This isn't much better than just segfaulting.  Did you consider
> whether
> it was possible to reconstruct corrupt metadata in any circumstances?
> 

The only spot we can actually correct metadata would be for an info
block, if the copy is intact. For any other poison in the map or flog,
I don't think we have any way to recover. Was that (info blocks) what
you meant, or something else?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm