Re: [PATCH v2] nvdimm/btt: Handle preemption in BTT lane acquisition

[Alison Schofield <alison.schofield@intel.com>]

On Fri, May 01, 2026 at 05:01:15PM +0530, Aboorva Devarajan wrote:
> On Wed, 2026-04-29 at 19:46 -0700, Alison Schofield wrote:
> > BTT (Block Translation Table) makes persistent memory safe for block
> > I/O by guaranteeing atomic sector updates. It uses reserved lanes
> > for in-flight BTT operations, which must be used exclusively.
> > 
> > The btt-check unit test reports data mismatches during BTT I/O due
> > to a race in lane acquisition, leading to silent data corruption.
> > 
> > BTT lane acquisition uses per-CPU recursion tracking with
> > migrate_disable(). However, migrate_disable() does not prevent
> > preemption, so another task can run on the same CPU and share the
> > recursion state. That task can observe a non-zero recursion count,
> > bypass locking, and use the same lane at the same time.
> > 
> > Track lane ownership per task and only allow lockless recursion for
> > the owning task. Otherwise, serialize access with the lane spinlock.
> > Use spin_(un)lock_bh() so softirq re-entry on the same CPU cannot
> > bypass ownership checks or deadlock on the lane lock.
> > 
> > Found with the NDCTL unit test btt-check.sh
> > 
> > Fixes: 36c75ce3bd29 ("nd_btt: Make BTT lanes preemptible")
> > Assisted-by: Claude Sonnet 4.5
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> > 
> > Changes in v2:
> > Use spin_(un)lock_bh() (Sashiko AI)
> > Update commit log per softirq re-enty and spinlock change
> > 
> > A new unit test to stress this is under review here:
> > https://lore.kernel.org/nvdimm/20260424233633.3762217-1-alison.schofield@intel.com/
> > 
> > 
> >  drivers/nvdimm/nd.h          |  1 +
> >  drivers/nvdimm/region_devs.c | 48 +++++++++++++++++++++---------------
> >  2 files changed, 29 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> > index b199eea3260e..424c38ca4960 100644
> > --- a/drivers/nvdimm/nd.h
> > +++ b/drivers/nvdimm/nd.h
> > @@ -368,6 +368,7 @@ unsigned sizeof_namespace_label(struct nvdimm_drvdata *ndd);
> >  struct nd_percpu_lane {
> >  	int count;
> >  	spinlock_t lock;
> > +	struct task_struct *owner;
> >  };
> >  
> >  enum nd_label_flags {
> > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> > index e35c2e18518f..f1c6dcd95b5a 100644
> > --- a/drivers/nvdimm/region_devs.c
> > +++ b/drivers/nvdimm/region_devs.c
> > @@ -905,11 +905,10 @@ void nd_region_advance_seeds(struct nd_region *nd_region, struct device *dev)
> >   * @nd_region: region id and number of lanes possible
> >   *
> >   * A lane correlates to a BLK-data-window and/or a log slot in the BTT.
> > - * We optimize for the common case where there are 256 lanes, one
> > - * per-cpu.  For larger systems we need to lock to share lanes.  For now
> > - * this implementation assumes the cost of maintaining an allocator for
> > - * free lanes is on the order of the lock hold time, so it implements a
> > - * static lane = cpu % num_lanes mapping.
> > + * Lanes are shared across CPUs using a static lane = cpu % num_lanes
> > + * mapping, with a per-lane spinlock to serialize access when multiple
> > + * tasks share a lane (including when preemption causes multiple tasks
> > + * to run on the same CPU).
> >   *
> >   * In the case of a BTT instance on top of a BLK namespace a lane may be
> >   * acquired recursively.  We lock on the first instance.
> > @@ -920,35 +919,44 @@ void nd_region_advance_seeds(struct nd_region *nd_region, struct device *dev)
> >  unsigned int nd_region_acquire_lane(struct nd_region *nd_region)
> >  {
> >  	unsigned int cpu, lane;
> > +	struct nd_percpu_lane *ndl;
> >  
> >  	migrate_disable();
> >  	cpu = smp_processor_id();
> > -	if (nd_region->num_lanes < nr_cpu_ids) {
> > -		struct nd_percpu_lane *ndl_lock, *ndl_count;
> > -
> > +	if (nd_region->num_lanes < nr_cpu_ids)
> >  		lane = cpu % nd_region->num_lanes;
> > -		ndl_count = per_cpu_ptr(nd_region->lane, cpu);
> > -		ndl_lock = per_cpu_ptr(nd_region->lane, lane);
> > -		if (ndl_count->count++ == 0)
> > -			spin_lock(&ndl_lock->lock);
> > -	} else
> > +	else
> >  		lane = cpu;
> >  
> > +	/*
> > +	 * migrate_disable() keeps the lane stable, but does not prevent
> > +	 * preemption. Only the owning task may recurse without taking the
> > +	 * lock.
> > +	 */
> > +	ndl = per_cpu_ptr(nd_region->lane, lane);
> > +	if (READ_ONCE(ndl->owner) != current) {
> > +		spin_lock_bh(&ndl->lock);
> > +		WRITE_ONCE(ndl->owner, current);
> > +	}
> > +	ndl->count++;
> > +
> >  	return lane;
> >  }
> >  EXPORT_SYMBOL(nd_region_acquire_lane);
> >  
> >  void nd_region_release_lane(struct nd_region *nd_region, unsigned int lane)
> >  {
> > -	if (nd_region->num_lanes < nr_cpu_ids) {
> > -		unsigned int cpu = smp_processor_id();
> > -		struct nd_percpu_lane *ndl_lock, *ndl_count;
> > +	struct nd_percpu_lane *ndl = per_cpu_ptr(nd_region->lane, lane);
> >  
> > -		ndl_count = per_cpu_ptr(nd_region->lane, cpu);
> > -		ndl_lock = per_cpu_ptr(nd_region->lane, lane);
> > -		if (--ndl_count->count == 0)
> > -			spin_unlock(&ndl_lock->lock);
> > +	if (WARN_ON_ONCE(READ_ONCE(ndl->owner) != current))
> > +		goto out;
> > +
> > +	if (--ndl->count == 0) {
> > +		WRITE_ONCE(ndl->owner, NULL);
> > +		spin_unlock_bh(&ndl->lock);
> >  	}
> > +
> > +out:
> >  	migrate_enable();
> >  }
> >  EXPORT_SYMBOL(nd_region_release_lane);
> > 
> > base-commit: 028ef9c96e96197026887c0f092424679298aae8
> 
> Hi Alison,
> 
> Just a follow-up question.
> 
> I haven't reproduced this, just noticed it while reading the code.
> 
> After this patch, nd_region_acquire_lane() / nd_region_release_lane() always
> hold a spinlock, IIUC, anything that sleeps/blocks in this critical section will
> hit:
> 
>     BUG: scheduling while atomic: ...
> 
> BTT metadata writes go arena_write_bytes() -> nvdimm_write_bytes() ->
> nsio_rw_bytes(), which always calls nvdimm_flush() on write. That can call
> nd_region->flush():
> 
>   - virtio_pmem_flush() uses a wait_event(), so it can block on
>     every flush.
> 
>   - papr_scm_pmem_flush() only msleep() when the flush hcall
>     comes back busy; the fast path does not sleep, though this is rare case.
> 
> So BTT on virtio_pmem looks like it could trip the BUG on metadata
> writes, papr_scm only if the busy path is taken? Pre-patch, the same behaviour
> already existed on > 256-CPU boxes where the lane spinlock was taken.
> 
> Is this an actual concern, so are we essentially saying that no sleep /
> blocking wait is allowed anywhere reachable from the lane critical section?
> 
> Please correct me if I'm missing something here.

Thanks for the review. You found a real issue.

The BTT lane lock is held across BTT write paths that can reach
nvdimm_flush(), and provider flush callbacks (e.g. virtio_pmem and
papr_scm) can sleep. So the current design incorrectly assumes that
the lane critical section is fully atomic.

As you pointed out, this predates this patch. The shared-lane path
has held a spinlock across this same call chain since the original
BTT merge. This patch probably widens the exposure by taking the lock
unconditionally.

I'm reworking this as a small series. The first patch converts the
per-lane lock to a mutex so the lane critical section can safely
sleep.

I appreciate your testing and will probaly need to rely on it more
in the next version.

Thanks,
Alison

> 
> Thanks,
> Aboorva