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

[Aboorva Devarajan <aboorvad@linux.ibm.com>]

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,
Aboorva