On Mon, 2017-07-17 at 15:00 -0700, Dan Williams wrote:
> On Mon, Jul 17, 2017 at 2:27 PM, Vishal Verma <[email protected]
> m> wrote:
> > Clearing errors or badblocks during a BTT write requires sending an
> > ACPI
> > DSM, which means potentially sleeping. Since a BTT IO happens in
> > atomic
> > context (preemption disabled, spinlocks may be held), we cannot
> > perform
> > error clearing in the course of an IO. Due to this error clearing
> > for
> > BTT IOs has hitherto been disabled.
> >
> > In this patch we move error clearing out of the atomic section, and
> > thus
> > re-enable error clearing with BTTs. When we are about to add a block
> > to
> > the free list, we check if it was previously marked as an error, and
> > if
> > it was, we add it to the freelist, but also set a flag that says
> > error
> > clearing will be required. We then drop the lane (ending the atomic
> > context), and send a zero buffer so that the error can be cleared.
> > The
> > error flag in the free list is protected by the nd 'lane', and is
> > set
> > only be a thread while it holds that lane. When the error is
> > cleared,
> > the flag is cleared, but while holding a mutex for that freelist
> > index.
> >
> > When writing, we check for two things -
> > 1/ If the freelist mutex is held or if the error flag is set. If so,
> > this is an error block that is being (or about to be) cleared.
> > 2/ If the block is a known badblock based on nsio->bb
> >
> > The second check is required because the BTT map error flag for a
> > map
> > entry only gets set when an error LBA is read. If we write to a new
> > location that may not have the map error flag set, but still might
> > be in
> > the region's badblock list, we can trigger an EIO on the write,
> > which is
> > undesirable and completely avoidable.
> >
> > Cc: Jeff Moyer <[email protected]>
> > Cc: Toshi Kani <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Signed-off-by: Vishal Verma <[email protected]>
> > ---
> > drivers/nvdimm/btt.c | 89
> > +++++++++++++++++++++++++++++++++++++++++++++-----
> > drivers/nvdimm/btt.h | 5 +++
> > drivers/nvdimm/claim.c | 8 -----
> > 3 files changed, 85 insertions(+), 17 deletions(-)
> >
<>
> > +static int arena_clear_freelist_error(struct arena_info *arena, u32
> > lane)
> > +{
> > + int ret = 0;
> > +
> > + if (arena->freelist[lane].has_err) {
> > + u32 lba = arena->freelist[lane].block;
> > + u64 nsoff = to_namespace_offset(arena, lba);
> > + void *zerobuf;
> > +
> > + zerobuf = kzalloc(arena->sector_size, GFP_KERNEL);
> > + if (!zerobuf)
> > + return -ENOMEM;
>
> Hmm, can this just use the system zero page (empty_zero_page)? Also,
> even if it can't, I'd prefer the NOIO fix for this case be folded into
> this patch. The NOIO fixup for the acpi dsm path should be separate.
>
> Another consideration if we can't use the system zero page, do we need
> to allocate this on demand? Seems like something we could just
> pre-allocate at init time.
I had started out using the empty_zero_page, but to use that you have to
kmap_atomic the page, which ends up creating an atomic section again :)
Sounds good on preallocating a zero page at init.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
