Re: [PATCH] libnvdimm, pfn: during init, clear errors in the metadata area

[Dan Williams <dan.j.williams@intel.com>]

On Tue, Sep 18, 2018 at 1:08 PM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> If there are badblocks present in the 'struct page' area for pfn
> namespaces, until now, the only way to clear them has been to force the
> namespace into raw mode, clear the errors, and re-enable the fsdax mode.
> This is clunky, given that it should be easy enough for the pfn driver
> to do the same.
>
> Add a new helper that uses the most recently available badblocks list to
> check whether there are any badblocks that lie in the volatile struct
> page area. If so, before initializing the struct pages, send down
> targeted writes via nvdimm_write_bytes to write zeroes to the affected
> blocks, and thus clear errors.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/nvdimm/pfn_devs.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index 3f7ad5bc443e..04b341758cd6 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -361,8 +361,59 @@ struct device *nd_pfn_create(struct nd_region *nd_region)
>         return dev;
>  }
>

Perhaps a comment here about the fact that we are clearing the
volatile memmap metadata space associated with a pfn instance.

> +static int nd_pfn_clear_meta_errors(struct nd_pfn *nd_pfn)

Let's just say "memmap" rather than "meta" to be more explicit.

> +{
> +       struct nd_region *nd_region = to_nd_region(nd_pfn->dev.parent);
> +       struct nd_namespace_common *ndns = nd_pfn->ndns;
> +       void *zero_page = page_address(ZERO_PAGE(0));
> +       struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
> +       sector_t first_bad, meta_start;
> +       struct nd_namespace_io *nsio;
> +       int num_bad, meta_num, rc;
> +       bool bb_present;
> +
> +       nsio = to_nd_namespace_io(&ndns->dev);
> +       meta_start = (SZ_4K + sizeof(*pfn_sb)) >> 9;
> +       meta_num = (le64_to_cpu(pfn_sb->dataoff) >> 9) - meta_start;
> +
> +       do {
> +               unsigned long zero_len;
> +               u64 nsoff;
> +
> +               bb_present = !!badblocks_check(&nd_region->bb, meta_start,
> +                               meta_num, &first_bad, &num_bad);

The !! throws me off, I don't think it's necessary. You could just
make bb_present an int.

> +               if (bb_present) {
> +                       dev_dbg(&nd_pfn->dev, "meta: %x badblocks at %lx\n",
> +                                       num_bad, first_bad);
> +                       nsoff = (nd_region->ndr_start + (first_bad << 9)) -
> +                                       nsio->res.start;

Perhaps this should be ALIGN_DOWN((nd_region->ndr_start + (first_bad
<< 9)) - nsio->res.start, PAGE_SIZE)...

> +                       zero_len = num_bad << 9;

...and then make this ALIGN(num_bad << 9, PAGE_SIZE), or otherwise a
comment about zero_len being zero...

> +                       while (zero_len) {
> +                               unsigned long chunk = min(zero_len, PAGE_SIZE);

...because the min(x, PAGE_SIZE) seems arbitrary.

Aligned clearing may be preferable considering we may be needing to
fixup page protections.

> +
> +                               rc = nvdimm_write_bytes(ndns, nsoff, zero_page,
> +                                                       chunk, 0);
> +                               if (rc)
> +                                       break;
> +
> +                               zero_len -= chunk;
> +                               nsoff += chunk;
> +                       }
> +                       if (rc) {
> +                               dev_err(&nd_pfn->dev,
> +                                       "error clearing %x badblocks at %lx\n",
> +                                       num_bad, first_bad);
> +                               return rc;
> +                       }
> +               }
> +       } while (bb_present);
> +
> +       return 0;
> +}
> +
>  int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
>  {
> +       int rc;
>         u64 checksum, offset;
>         enum nd_pfn_mode mode;
>         struct nd_namespace_io *nsio;
> @@ -477,6 +528,12 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
>                 return -ENXIO;
>         }
>
> +       if (mode == PFN_MODE_PMEM) {
> +               rc = nd_pfn_clear_meta_errors(nd_pfn);
> +               if (rc)
> +                       return rc;

I think this can just be "return nd_pfn_clear_meta_errors()", and
maybe move the mode check inside nd_pfn_clear_meta_errors() to return
early if no volatile metadata to clear.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm