Skip to content

Commit 8d4b989

Browse files
nvdimm/btt: Handle preemption in BTT lane acquisition
BTT lanes serialize access to per-lane metadata and workspace state during BTT I/O. The btt-check unit test reports data mismatches during BTT writes due to a race in lane acquisition that can lead to silent data corruption. The existing lane model uses a spinlock together with a per-CPU recursion count. That recursion model stopped being valid after BTT lanes became preemptible: another task can run on the same CPU, observe a non-zero recursion count, bypass locking, and use the same lane concurrently. BTT lanes are also held across arena_write_bytes() calls. That path reaches nsio_rw_bytes(), which flushes writes with nvdimm_flush(). Some provider flush callbacks can sleep, making a spinlock the wrong primitive for the lane lifetime. Replace the spinlock-based recursion model with a dynamically allocated per-lane mutex array and take the lane lock unconditionally. Add might_sleep() to catch any future atomic-context caller. Found with the ndctl unit test btt-check.sh. Fixes: 36c75ce ("nd_btt: Make BTT lanes preemptible") Assisted-by: Claude-Sonnet:4.5 Tested-by: Aboorva Devarajan <aboorvad@linux.ibm.com> Reviewed-by: Aboorva Devarajan <aboorvad@linux.ibm.com> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> Link: https://patch.msgid.link/20260528021625.618462-1-alison.schofield@intel.com Signed-off-by: Alison Schofield <alison.schofield@intel.com>
1 parent e43ffb6 commit 8d4b989

3 files changed

Lines changed: 29 additions & 53 deletions

File tree

Documentation/driver-api/nvdimm/btt.rst

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -161,9 +161,8 @@ process::
161161
nlanes = min(nfree, num_cpus)
162162

163163
A lane number is obtained at the start of any IO, and is used for indexing into
164-
all the on-disk and in-memory data structures for the duration of the IO. If
165-
there are more CPUs than the max number of available lanes, than lanes are
166-
protected by spinlocks.
164+
all the on-disk and in-memory data structures for the duration of the IO. Lanes
165+
are protected by mutexes.
167166

168167

169168
d. In-memory data structure: Read Tracking Table (RTT)

drivers/nvdimm/nd.h

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -365,11 +365,6 @@ unsigned sizeof_namespace_label(struct nvdimm_drvdata *ndd);
365365
for (res = (ndd)->dpa.child, next = res ? res->sibling : NULL; \
366366
res; res = next, next = next ? next->sibling : NULL)
367367

368-
struct nd_percpu_lane {
369-
int count;
370-
spinlock_t lock;
371-
};
372-
373368
enum nd_label_flags {
374369
ND_LABEL_REAP,
375370
};
@@ -400,6 +395,10 @@ struct nd_mapping {
400395
struct nvdimm_drvdata *ndd;
401396
};
402397

398+
struct nd_lane {
399+
struct mutex lock; /* serialize lane access */
400+
} ____cacheline_aligned_in_smp;
401+
403402
struct nd_region {
404403
struct device dev;
405404
struct ida ns_ida;
@@ -420,7 +419,7 @@ struct nd_region {
420419
struct kernfs_node *bb_state;
421420
struct badblocks bb;
422421
struct nd_interleave_set *nd_set;
423-
struct nd_percpu_lane __percpu *lane;
422+
struct nd_lane *lane;
424423
int (*flush)(struct nd_region *nd_region, struct bio *bio);
425424
struct nd_mapping mapping[] __counted_by(ndr_mappings);
426425
};

drivers/nvdimm/region_devs.c

Lines changed: 22 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,9 @@ static void nd_region_release(struct device *dev)
192192

193193
put_device(&nvdimm->dev);
194194
}
195-
free_percpu(nd_region->lane);
195+
for (i = 0; i < nd_region->num_lanes; i++)
196+
mutex_destroy(&nd_region->lane[i].lock);
197+
kfree(nd_region->lane);
196198
if (!test_bit(ND_REGION_CXL, &nd_region->flags))
197199
memregion_free(nd_region->id);
198200
kfree(nd_region);
@@ -904,52 +906,30 @@ void nd_region_advance_seeds(struct nd_region *nd_region, struct device *dev)
904906
* nd_region_acquire_lane - allocate and lock a lane
905907
* @nd_region: region id and number of lanes possible
906908
*
907-
* A lane correlates to a BLK-data-window and/or a log slot in the BTT.
908-
* We optimize for the common case where there are 256 lanes, one
909-
* per-cpu. For larger systems we need to lock to share lanes. For now
910-
* this implementation assumes the cost of maintaining an allocator for
911-
* free lanes is on the order of the lock hold time, so it implements a
912-
* static lane = cpu % num_lanes mapping.
909+
* A lane correlates to a log slot in the BTT. Lanes are shared across
910+
* CPUs using a static lane = cpu % num_lanes mapping, with a per-lane
911+
* mutex to serialize access.
913912
*
914-
* In the case of a BTT instance on top of a BLK namespace a lane may be
915-
* acquired recursively. We lock on the first instance.
916-
*
917-
* In the case of a BTT instance on top of PMEM, we only acquire a lane
918-
* for the BTT metadata updates.
913+
* Callers must be in sleepable context. The only in-tree caller is
914+
* BTT's ->submit_bio handler (btt_read_pg / btt_write_pg).
919915
*/
920916
unsigned int nd_region_acquire_lane(struct nd_region *nd_region)
917+
__acquires(&nd_region->lane[lane].lock)
921918
{
922-
unsigned int cpu, lane;
923-
924-
migrate_disable();
925-
cpu = smp_processor_id();
926-
if (nd_region->num_lanes < nr_cpu_ids) {
927-
struct nd_percpu_lane *ndl_lock, *ndl_count;
919+
unsigned int lane;
928920

929-
lane = cpu % nd_region->num_lanes;
930-
ndl_count = per_cpu_ptr(nd_region->lane, cpu);
931-
ndl_lock = per_cpu_ptr(nd_region->lane, lane);
932-
if (ndl_count->count++ == 0)
933-
spin_lock(&ndl_lock->lock);
934-
} else
935-
lane = cpu;
921+
might_sleep();
936922

923+
lane = raw_smp_processor_id() % nd_region->num_lanes;
924+
mutex_lock(&nd_region->lane[lane].lock);
937925
return lane;
938926
}
939927
EXPORT_SYMBOL(nd_region_acquire_lane);
940928

941929
void nd_region_release_lane(struct nd_region *nd_region, unsigned int lane)
930+
__releases(&nd_region->lane[lane].lock)
942931
{
943-
if (nd_region->num_lanes < nr_cpu_ids) {
944-
unsigned int cpu = smp_processor_id();
945-
struct nd_percpu_lane *ndl_lock, *ndl_count;
946-
947-
ndl_count = per_cpu_ptr(nd_region->lane, cpu);
948-
ndl_lock = per_cpu_ptr(nd_region->lane, lane);
949-
if (--ndl_count->count == 0)
950-
spin_unlock(&ndl_lock->lock);
951-
}
952-
migrate_enable();
932+
mutex_unlock(&nd_region->lane[lane].lock);
953933
}
954934
EXPORT_SYMBOL(nd_region_release_lane);
955935

@@ -1019,17 +999,16 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
1019999
goto err_id;
10201000
}
10211001

1022-
nd_region->lane = alloc_percpu(struct nd_percpu_lane);
1002+
nd_region->num_lanes = ndr_desc->num_lanes;
1003+
if (!nd_region->num_lanes)
1004+
goto err_percpu;
1005+
nd_region->lane = kcalloc(nd_region->num_lanes,
1006+
sizeof(*nd_region->lane), GFP_KERNEL);
10231007
if (!nd_region->lane)
10241008
goto err_percpu;
10251009

1026-
for (i = 0; i < nr_cpu_ids; i++) {
1027-
struct nd_percpu_lane *ndl;
1028-
1029-
ndl = per_cpu_ptr(nd_region->lane, i);
1030-
spin_lock_init(&ndl->lock);
1031-
ndl->count = 0;
1032-
}
1010+
for (i = 0; i < nd_region->num_lanes; i++)
1011+
mutex_init(&nd_region->lane[i].lock);
10331012

10341013
for (i = 0; i < ndr_desc->num_mappings; i++) {
10351014
struct nd_mapping_desc *mapping = &ndr_desc->mapping[i];
@@ -1046,7 +1025,6 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
10461025
}
10471026
nd_region->provider_data = ndr_desc->provider_data;
10481027
nd_region->nd_set = ndr_desc->nd_set;
1049-
nd_region->num_lanes = ndr_desc->num_lanes;
10501028
nd_region->flags = ndr_desc->flags;
10511029
nd_region->ro = ro;
10521030
nd_region->numa_node = ndr_desc->numa_node;

0 commit comments

Comments
 (0)