nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "Williams, Dan J" <dan.j.williams@intel.com>
Cc: "linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH] libnvdimm, pfn: during init, clear errors in the metadata area
Date: Tue, 18 Sep 2018 22:21:22 +0000	[thread overview]
Message-ID: <1537309280.5995.246.camel@intel.com> (raw)
In-Reply-To: <CAPcyv4gnrEbSvQeVR_FpcZmWPMvHoQ3Gg3qsdYvp_7dyvOn-sw@mail.gmail.com>


On Tue, 2018-09-18 at 13:53 -0700, Dan Williams wrote:
> 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.

I was following the precedent of is_bad_pmem(), but int works too.

> 
> > +               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.

The min() is because our source of zeroes is PAGE_SIZE. zero_len
*should* never be zero as a badblocks entry with a 'count' of zero
would be a bug.. But aligned clearing makes sense too - we just
potentially end up writing more than we absolutely need to.

What do you mean by fixup page protections?

I agree with the rest of the comment, and will fix those up.

> 
> > +
> > +                               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

      reply	other threads:[~2018-09-18 22:21 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-18 20:07 Vishal Verma
2018-09-18 20:53 ` Dan Williams
2018-09-18 22:21   ` Verma, Vishal L [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1537309280.5995.246.camel@intel.com \
    --to=vishal.l.verma@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=linux-nvdimm@lists.01.org \
    --subject='Re: [PATCH] libnvdimm, pfn: during init, clear errors in the metadata area' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).