[PATCH v3 0/2] bcache: track active bypass writes to fix read miss race

[PATCH v3 0/2] bcache: track active bypass writes to fix read miss race

[Ankit Kapoor]

This is the v3 patch series to fix a race condition between read cache
misses and bypass writes in bcache.

This patch series fixes the race condition through two complementary
mechanisms split across two logical patches:

1. [PATCH v3 1/2]: bcache: track active bypass writes to fix read miss race
   - Dynamically tracks active bypass writes across the backing device
     using 32MB chunks and refcounted pages (a single 4KB page covers
     32GB of disk).
   - On a read cache miss, bcache checks for overlapping active bypass
     writes and forces the read to bypass the cache if a collision is
     detected.
   - Serializes btree key invalidation so it executes in
     cached_dev_write_complete() after the payload reaches the disk,
     preventing early invalidations from opening a race window.
   - Resilient memory management: counter pages are backed by a
     dedicated 16-page mempool to guarantee forward progress under
     severe memory pressure, and the top-level array is allocated via
     kvcalloc() to reliably support very large backing devices.

2. [PATCH v3 2/2]: bcache: inspect active bypass writes lock-free via RCU
   - Optimizes the cache miss read path by inspecting active bypass
     counters lock-free under RCU (rcu_read_lock / rcu_dereference),
     removing spinlock contention from latency-sensitive reads.
   - Writers continue to use localized page-level spinlocks to
     synchronize counter updates and mempool allocations.

Changes since v2:
Link: https://lore.kernel.org/linux-bcache/20260617103356.3287775-1-ankitkap@google.com/

Patch 1 (Core Tracking):
 - Upgraded chunk counters from u16 to u32 to prevent any possibility
   of counter overflow under extreme IO concurrency.
 - Replaced GFP_KERNEL/fallback allocations with a dedicated 16-page
   mempool (mempool_alloc with GFP_NOIO), guaranteeing that bypass
   tracking never fails under memory pressure and eliminating the need
   for untracked fallbacks.
 - Used kvcalloc() for the top-level page array to support massive
   backing devices without requiring large contiguous physical
   allocations.
 - Removed debug sysfs counters and tracepoints to keep the patch
   footprint minimal and focused on the core synchronization. These
   can be added in a follow-up patch.

Patch 2 (RCU Optimization):
 - Introduced this entirely new modular commit to separate the lock-free
   read path optimization from the core race synchronization logic.

Memory Consumption:
Note: An additional 16-page mempool standby reserve is maintained
(4 KB/page for Spinlock, ~4.1 KB/page for RCU).

Idle Memory Consumption (Zero active bypass writes)
Backing Disk Size  | Spinlock         | RCU
1 TB               | 0.5 KB           | 0.5 KB
10 TB              | 5.0 KB           | 5.0 KB
100 TB             | 50.0 KB          | 50.0 KB

Peak Memory Consumption (All tracking pages allocated)
Backing Disk Size  | Spinlock         | RCU
1 TB               | 128.5 KB         | ~140.0 KB
10 TB              | 1.28 MB          | ~1.4 MB
100 TB             | 12.5 MB          | ~14.0 MB

Setup:
 - CPU: 32 vCPU, Intel Cascade Lake x86_64 (n2-standard-32 GCP VM)
 - Memory: 128 GB RAM
 - OS: Linux 6.12.94 (Google COS)
 - Storage: Google Cloud Extreme PD (1000 GB) + Local SSD (375 GB)

Performance Results

FIO config:
rw=randrw, bs=(R) 4096B-4096B, (W) 128KiB-128KiB, (T) 128KiB-128KiB,
ioengine=libaio, iodepth=32

NVMe SSD - 10 GB Working Set with 16 FIO jobs (1 active tracking page)
Metric                 | Baseline | Patch 1 (Spinlock) | Patch 2 (RCU)
Read IOPS              | 20,945   | 20,832             | 20,839
Write IOPS             | 8,978    | 8,936              | 8,935
Total IOPS             | 29,923   | 29,769             | 29,775
Avg Read Lat (ns)      | 33,292   | 33,858             | 33,871
Avg Write Lat (ns)     | 36,478   | 35,752             | 35,693
Agg Kernel CPU (sys %)*| 48.62%   | 50.80%             | 48.46%
NVMe Utilization       | 70.38%   | 44.29%             | 44.44%
*Note: Kernel CPU reported as aggregate across cores (~3% per core).

NVMe SSD - 320 GB Working Set with 16 FIO jobs (10 tracking pages)
Metric                 | Baseline | Patch 1 (Spinlock) | Patch 2 (RCU)
Read IOPS              | 20,968   | 20,955             | 20,959
Write IOPS             | 8,983    | 8,982              | 8,983
Total IOPS             | 29,951   | 29,937             | 29,942
Avg Read Lat (ns)      | 33,484   | 33,509             | 33,524
Avg Write Lat (ns)     | 35,932   | 35,926             | 35,884
Agg Kernel CPU (sys %)*| 52.54%   | 50.84%             | 52.15%
NVMe Utilization       | 68.11%   | 67.26%             | 69.00%
*Note: Kernel CPU reported as aggregate across cores (~3% per core).

Analysis
Active bypass write tracking shows no measurable performance
degradation on real NVMe storage hardware. While a minor NVMe
utilization dip is observed on small working sets (10 GB), device
utilization remains completely normal on larger, realistic workloads
(320 GB).

Comparing RCU and Spinlock

Because physical NVMe hardware latency masks microsecond-level software
differences, we also benchmarked against a zero-latency null block
device (null_blk) at 64 jobs to remove the hardware bottleneck and
isolate true block-layer locking overhead.

Null Device (null_blk) - 10 GB Working Set with 64 FIO jobs
Metric                 | Baseline | Patch 1 (Spinlock) | Patch 2 (RCU)
Read IOPS              | 64,296   | 58,394             | 58,962
Write IOPS             | 27,551   | 25,012             | 25,267
Total IOPS             | 91,847   | 83,406             | 84,229
Avg Read Lat (ns)      | 45,463   | 50,104             | 49,678
Avg Write Lat (ns)     | 45,362   | 50,451             | 50,025
Agg Kernel CPU (sys %)*| 2119.08% | 2830.73%           | 2838.42%
NVMe Utilization       | 0.00%    | 0.00%              | 0.00%
*Note: Kernel CPU reported as aggregate across 64 threads (~44%/core).

Why Patch 2 (RCU Read-Path Optimization)?
While the absolute throughput improvement on our 32-core test system
is modest (~1.0% / +823 IOPS and ~426 ns lower latency in a 64-job
null_blk stress test), the primary motivation for Patch 2 is to maintain
a lock-free read path and improve scalability on higher-core-count
servers.

Thanks,
Ankit

Ankit Kapoor (2):
  bcache: track active bypass writes to fix read miss race
  bcache: inspect active bypass writes lock-free via RCU

 drivers/md/bcache/bcache.h  |  43 ++++++++++
 drivers/md/bcache/request.c | 151 +++++++++++++++++++++++++++++++++++-
 drivers/md/bcache/super.c   |  42 ++++++++++
 3 files changed, 233 insertions(+), 3 deletions(-)

-- 
2.55.0.679.g6767b8d81c-goog

[PATCH v3 1/2] bcache: track active bypass writes to fix read miss race

[Ankit Kapoor]

A race condition exists between a read cache miss and a bypass write
due to either congestion or sequential bypass, which causes stale data
to be cached when the read cache miss runs concurrently with a bypass
write targeting the same sectors. If the read cache miss fetches data
from the backing device before the write to the backing device finishes,
stale data populates the cache.

The root cause is that bcache currently executes btree key
invalidation in parallel with (or prior to) writing the actual data
payload to the backing device. Under this sequence, a concurrent read
path can register a cache miss and insert a placeholder key. If the
write's btree key invalidation completes before the read finishes
fetching old data from the backing device, the read's subsequent key
replacement will not detect a collision, allowing stale data to
persist in the cache.

Fix this by tracking active bypass writes and serializing cache
invalidation.

First, divide the backing device space into 32MB chunks and track
concurrent bypass writes using refcounts. The tracking counters are
stored in dynamically allocated pages backed by a dedicated mempool
to prevent allocation failures under memory pressure, minimizing overall
footprint (a single 4KB page supports 32GB of disk space using u32
counters).

On a cache miss read, bcache checks if there are any active bypass
writes overlapping the target sectors. If an active bypass write is
detected, the read is forced to bypass the cache to ensure data
consistency.

Second, serialize the btree key invalidation (bch_data_insert) for
bypass writes so that it executes in cached_dev_write_complete(), after
the payload has been written to the backing device. This prevents early
invalidation from opening a window where a concurrent read miss could
fetch and re-cache stale data before the bypass write reaches the disk.

Suggested-by: Coly Li <colyli@fygo.io>
Signed-off-by: Ankit Kapoor <ankitkap@google.com>
---
 drivers/md/bcache/bcache.h  |  35 ++++++++++
 drivers/md/bcache/request.c | 131 +++++++++++++++++++++++++++++++++++-
 drivers/md/bcache/super.c   |  39 +++++++++++
 3 files changed, 202 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index ec9ff9715081..2e50526a52fc 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -299,6 +299,12 @@ enum stop_on_failure {
 	BCH_CACHED_DEV_STOP_MODE_MAX,
 };
 
+struct bch_bypass_page {
+	u32		*counts;
+	unsigned int	active;
+	spinlock_t	lock;
+};
+
 struct cached_dev {
 	struct list_head	list;
 	struct bcache_device	disk;
@@ -407,8 +413,37 @@ struct cached_dev {
 	 */
 #define BCH_WBRATE_UPDATE_MAX_SKIPS	15
 	unsigned int		rate_update_retry;
+
+	/* For tracking active bypass writes */
+#define BCH_BYPASS_CHUNK_SHIFT		16  /* 2^16 sectors = 32MB */
+#define BCH_BYPASS_PAGE_COUNTERS	(PAGE_SIZE / sizeof(u32))
+#define BCH_BYPASS_PAGE_SHIFT		(PAGE_SHIFT - 2)
+#define BCH_BYPASS_PAGE_MASK		((1UL << BCH_BYPASS_PAGE_SHIFT) - 1)
+	struct bch_bypass_page	*bypass_pages;
+	unsigned long		bypass_num_pages;
+	mempool_t		bypass_mempool;
 };
 
+static inline unsigned long sector_to_bypass_chunk(sector_t sector)
+{
+	return sector >> BCH_BYPASS_CHUNK_SHIFT;
+}
+
+static inline unsigned long bypass_chunk_to_page(unsigned long chunk)
+{
+	return chunk >> BCH_BYPASS_PAGE_SHIFT;
+}
+
+static inline unsigned long bypass_chunk_to_offset(unsigned long chunk)
+{
+	return chunk & BCH_BYPASS_PAGE_MASK;
+}
+
+static inline sector_t bypass_chunk_to_sector(unsigned long chunk)
+{
+	return (sector_t)chunk << BCH_BYPASS_CHUNK_SHIFT;
+}
+
 enum alloc_reserve {
 	RESERVE_BTREE,
 	RESERVE_PRIO,
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 3fa3b13a410f..bfd28b68f499 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -492,6 +492,9 @@ struct search {
 	struct block_device	*orig_bdev;
 	unsigned long		start_time;
 
+	sector_t		bypass_sector;
+	unsigned int		bypass_sectors;
+
 	struct btree_op		op;
 	struct data_insert_op	iop;
 };
@@ -763,11 +766,16 @@ static inline struct search *search_alloc(struct bio *bio,
 
 /* Cached devices */
 
+static void bch_bypass_write_end(struct cached_dev *dc, sector_t sector, unsigned int sectors);
+
 static CLOSURE_CALLBACK(cached_dev_bio_complete)
 {
 	closure_type(s, struct search, cl);
 	struct cached_dev *dc = container_of(s->d, struct cached_dev, disk);
 
+	if (s->iop.bypass && op_is_write(bio_op(s->orig_bio)))
+		bch_bypass_write_end(dc, s->bypass_sector, s->bypass_sectors);
+
 	cached_dev_put(dc);
 	search_free(&cl->work);
 }
@@ -830,6 +838,110 @@ static CLOSURE_CALLBACK(cached_dev_cache_miss_done)
 	closure_put(&d->cl);
 }
 
+static void bch_bypass_write_start(struct cached_dev *dc, sector_t sector, unsigned int sectors)
+{
+	unsigned long start_chunk = sector_to_bypass_chunk(sector);
+	unsigned long end_chunk = sector_to_bypass_chunk(sector + sectors - 1);
+	unsigned long end_pg_idx = bypass_chunk_to_page(end_chunk);
+	unsigned long chunk;
+
+	if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
+		return;
+
+	for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
+		unsigned long pg_idx = bypass_chunk_to_page(chunk);
+		unsigned long pg_off = bypass_chunk_to_offset(chunk);
+		struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
+		u32 *new_counts;
+		u32 *dup_counts = NULL;
+		unsigned long flags;
+
+		spin_lock_irqsave(&pg->lock, flags);
+		if (!pg->counts) {
+			spin_unlock_irqrestore(&pg->lock, flags);
+			new_counts = mempool_alloc(&dc->bypass_mempool, GFP_NOIO);
+			memset(new_counts, 0, PAGE_SIZE);
+			spin_lock_irqsave(&pg->lock, flags);
+			if (pg->counts)
+				dup_counts = new_counts;
+			else
+				pg->counts = new_counts;
+		}
+		pg->counts[pg_off]++;
+		pg->active++;
+		spin_unlock_irqrestore(&pg->lock, flags);
+
+		if (dup_counts)
+			mempool_free(dup_counts, &dc->bypass_mempool);
+	}
+}
+
+static void bch_bypass_write_end(struct cached_dev *dc, sector_t sector, unsigned int sectors)
+{
+	unsigned long start_chunk = sector_to_bypass_chunk(sector);
+	unsigned long end_chunk = sector_to_bypass_chunk(sector + sectors - 1);
+	unsigned long end_pg_idx = bypass_chunk_to_page(end_chunk);
+	unsigned long chunk;
+
+	if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
+		return;
+
+	for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
+		unsigned long pg_idx = bypass_chunk_to_page(chunk);
+		unsigned long pg_off = bypass_chunk_to_offset(chunk);
+		struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
+		u32 *counts = NULL;
+		unsigned long flags;
+
+		spin_lock_irqsave(&pg->lock, flags);
+		if (WARN_ON_ONCE(!pg->counts || !pg->counts[pg_off])) {
+			spin_unlock_irqrestore(&pg->lock, flags);
+			continue;
+		}
+
+		pg->counts[pg_off]--;
+		pg->active--;
+		if (!pg->active) {
+			counts = pg->counts;
+			pg->counts = NULL;
+		}
+		spin_unlock_irqrestore(&pg->lock, flags);
+
+		if (counts)
+			mempool_free(counts, &dc->bypass_mempool);
+	}
+}
+
+static bool bch_has_active_bypass_writes(struct cached_dev *dc, sector_t sector,
+					 unsigned int sectors)
+{
+	unsigned long start_chunk = sector_to_bypass_chunk(sector);
+	unsigned long end_chunk = sector_to_bypass_chunk(sector + sectors - 1);
+	unsigned long end_pg_idx = bypass_chunk_to_page(end_chunk);
+	unsigned long chunk;
+	bool has_active = false;
+
+	if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
+		return false;
+
+	for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
+		unsigned long pg_idx = bypass_chunk_to_page(chunk);
+		unsigned long pg_off = bypass_chunk_to_offset(chunk);
+		struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
+		unsigned long flags;
+
+		spin_lock_irqsave(&pg->lock, flags);
+		if (pg->counts && pg->counts[pg_off] > 0) {
+			has_active = true;
+			spin_unlock_irqrestore(&pg->lock, flags);
+			break;
+		}
+		spin_unlock_irqrestore(&pg->lock, flags);
+	}
+
+	return has_active;
+}
+
 static CLOSURE_CALLBACK(cached_dev_read_done)
 {
 	closure_type(s, struct search, cl);
@@ -864,7 +976,9 @@ static CLOSURE_CALLBACK(cached_dev_read_done)
 	bio_complete(s);
 
 	if (s->iop.bio &&
-	    !test_bit(CACHE_SET_STOPPING, &s->iop.c->flags)) {
+	    !test_bit(CACHE_SET_STOPPING, &s->iop.c->flags) &&
+	    !bch_has_active_bypass_writes(dc, s->iop.bio->bi_iter.bi_sector,
+					  bio_sectors(s->iop.bio))) {
 		BUG_ON(!s->iop.replace);
 		closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
 	}
@@ -975,7 +1089,13 @@ static CLOSURE_CALLBACK(cached_dev_write_complete)
 	struct cached_dev *dc = container_of(s->d, struct cached_dev, disk);
 
 	up_read_non_owner(&dc->writeback_lock);
-	cached_dev_bio_complete(&cl->work);
+
+	if (s->iop.bypass) {
+		closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
+		continue_at(cl, cached_dev_bio_complete, NULL);
+	} else {
+		cached_dev_bio_complete(&cl->work);
+	}
 }
 
 static void cached_dev_write(struct cached_dev *dc, struct search *s)
@@ -1018,6 +1138,10 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s)
 		s->iop.bio = s->orig_bio;
 		bio_get(s->iop.bio);
 
+		s->bypass_sector = bio->bi_iter.bi_sector;
+		s->bypass_sectors = bio_sectors(bio);
+		bch_bypass_write_start(dc, s->bypass_sector, s->bypass_sectors);
+
 		if (bio_op(bio) == REQ_OP_DISCARD &&
 		    !bdev_max_discard_sectors(dc->bdev))
 			goto insert_data;
@@ -1058,7 +1182,8 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s)
 	}
 
 insert_data:
-	closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
+	if (!s->iop.bypass)
+		closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
 	continue_at(cl, cached_dev_write_complete, NULL);
 }
 
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 97d9adb0bf96..b994483511de 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1346,6 +1346,16 @@ void bch_cached_dev_release(struct kobject *kobj)
 {
 	struct cached_dev *dc = container_of(kobj, struct cached_dev,
 					     disk.kobj);
+	if (dc->bypass_pages) {
+		unsigned long i;
+
+		for (i = 0; i < dc->bypass_num_pages; i++) {
+			if (dc->bypass_pages[i].counts)
+				mempool_free(dc->bypass_pages[i].counts, &dc->bypass_mempool);
+		}
+		kvfree(dc->bypass_pages);
+		mempool_exit(&dc->bypass_mempool);
+	}
 	kfree(dc);
 	module_put(THIS_MODULE);
 }
@@ -1407,6 +1417,31 @@ static CLOSURE_CALLBACK(cached_dev_flush)
 	continue_at(cl, cached_dev_free, system_percpu_wq);
 }
 
+static int bch_cached_dev_bypass_init(struct cached_dev *dc, sector_t sectors)
+{
+	unsigned long chunks =
+		sector_to_bypass_chunk(sectors + (1UL << BCH_BYPASS_CHUNK_SHIFT) - 1);
+	unsigned long i;
+
+	dc->bypass_num_pages = DIV_ROUND_UP(chunks, BCH_BYPASS_PAGE_COUNTERS);
+	dc->bypass_pages = kvcalloc(dc->bypass_num_pages,
+				   sizeof(struct bch_bypass_page),
+				   GFP_KERNEL);
+	if (!dc->bypass_pages)
+		return -ENOMEM;
+
+	for (i = 0; i < dc->bypass_num_pages; i++)
+		spin_lock_init(&dc->bypass_pages[i].lock);
+
+	if (mempool_init_kmalloc_pool(&dc->bypass_mempool, 16, PAGE_SIZE)) {
+		kvfree(dc->bypass_pages);
+		dc->bypass_pages = NULL;
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
 static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
 {
 	int ret;
@@ -1447,6 +1482,10 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
 	/* default to auto */
 	dc->stop_when_cache_set_failed = BCH_CACHED_DEV_STOP_AUTO;
 
+	ret = bch_cached_dev_bypass_init(dc, bdev_nr_sectors(dc->bdev));
+	if (ret)
+		return ret;
+
 	bch_cached_dev_request_init(dc);
 	bch_cached_dev_writeback_init(dc);
 	return 0;
-- 
2.55.0.679.g6767b8d81c-goog

[PATCH v3 2/2] bcache: inspect active bypass writes lock-free via RCU

[Ankit Kapoor]

Checking for active bypass writes on the cache miss read path currently
requires acquiring page-level spinlocks across the target sector range.
While contention is low under normal operation, acquiring a lock in the
latency-sensitive read path introduces unnecessary overhead.

Optimize the read path by using RCU to inspect active bypass write
counters lock-free. Writers continue to use page-level spinlocks to
synchronize counter updates and allocations, while readers inspect the
tracking array under rcu_read_lock().

Suggested-by: Coly Li <colyli@fygo.io>
Signed-off-by: Ankit Kapoor <ankitkap@google.com>
---
 drivers/md/bcache/bcache.h  | 14 +++++++--
 drivers/md/bcache/request.c | 58 +++++++++++++++++++++++++------------
 drivers/md/bcache/super.c   |  9 ++++--
 3 files changed, 56 insertions(+), 25 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 2e50526a52fc..2ecff48d8902 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -299,10 +299,18 @@ enum stop_on_failure {
 	BCH_CACHED_DEV_STOP_MODE_MAX,
 };
 
+extern struct kmem_cache *bch_bypass_cache;
+
+struct bch_bypass_counts {
+	struct rcu_head		rcu;
+	struct cached_dev	*dc;
+	u32			counts[PAGE_SIZE / sizeof(u32)];
+};
+
 struct bch_bypass_page {
-	u32		*counts;
-	unsigned int	active;
-	spinlock_t	lock;
+	struct bch_bypass_counts __rcu *counts;
+	unsigned int		active;
+	spinlock_t		lock;
 };
 
 struct cached_dev {
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index bfd28b68f499..9221db669050 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -24,6 +24,7 @@
 #define CUTOFF_CACHE_READA	90
 
 struct kmem_cache *bch_search_cache;
+struct kmem_cache *bch_bypass_cache;
 
 static CLOSURE_CALLBACK(bch_data_insert_start);
 
@@ -852,22 +853,28 @@ static void bch_bypass_write_start(struct cached_dev *dc, sector_t sector, unsig
 		unsigned long pg_idx = bypass_chunk_to_page(chunk);
 		unsigned long pg_off = bypass_chunk_to_offset(chunk);
 		struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
-		u32 *new_counts;
-		u32 *dup_counts = NULL;
+		struct bch_bypass_counts *new_counts;
+		struct bch_bypass_counts *dup_counts = NULL;
+		struct bch_bypass_counts *counts;
 		unsigned long flags;
 
 		spin_lock_irqsave(&pg->lock, flags);
-		if (!pg->counts) {
+		counts = rcu_dereference_protected(pg->counts, lockdep_is_held(&pg->lock));
+		if (!counts) {
 			spin_unlock_irqrestore(&pg->lock, flags);
 			new_counts = mempool_alloc(&dc->bypass_mempool, GFP_NOIO);
-			memset(new_counts, 0, PAGE_SIZE);
+			memset(new_counts->counts, 0, PAGE_SIZE);
+			new_counts->dc = dc;
 			spin_lock_irqsave(&pg->lock, flags);
-			if (pg->counts)
+			counts = rcu_dereference_protected(pg->counts, lockdep_is_held(&pg->lock));
+			if (counts) {
 				dup_counts = new_counts;
-			else
-				pg->counts = new_counts;
+			} else {
+				counts = new_counts;
+				rcu_assign_pointer(pg->counts, counts);
+			}
 		}
-		pg->counts[pg_off]++;
+		WRITE_ONCE(counts->counts[pg_off], counts->counts[pg_off] + 1);
 		pg->active++;
 		spin_unlock_irqrestore(&pg->lock, flags);
 
@@ -876,6 +883,13 @@ static void bch_bypass_write_start(struct cached_dev *dc, sector_t sector, unsig
 	}
 }
 
+static void bch_bypass_counts_free_rcu(struct rcu_head *rcu)
+{
+	struct bch_bypass_counts *counts = container_of(rcu, struct bch_bypass_counts, rcu);
+
+	mempool_free(counts, &counts->dc->bypass_mempool);
+}
+
 static void bch_bypass_write_end(struct cached_dev *dc, sector_t sector, unsigned int sectors)
 {
 	unsigned long start_chunk = sector_to_bypass_chunk(sector);
@@ -890,25 +904,27 @@ static void bch_bypass_write_end(struct cached_dev *dc, sector_t sector, unsigne
 		unsigned long pg_idx = bypass_chunk_to_page(chunk);
 		unsigned long pg_off = bypass_chunk_to_offset(chunk);
 		struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
-		u32 *counts = NULL;
+		struct bch_bypass_counts *counts = NULL;
+		struct bch_bypass_counts *current_counts;
 		unsigned long flags;
 
 		spin_lock_irqsave(&pg->lock, flags);
-		if (WARN_ON_ONCE(!pg->counts || !pg->counts[pg_off])) {
+		current_counts = rcu_dereference_protected(pg->counts, lockdep_is_held(&pg->lock));
+		if (WARN_ON_ONCE(!current_counts || !current_counts->counts[pg_off])) {
 			spin_unlock_irqrestore(&pg->lock, flags);
 			continue;
 		}
 
-		pg->counts[pg_off]--;
+		WRITE_ONCE(current_counts->counts[pg_off], current_counts->counts[pg_off] - 1);
 		pg->active--;
 		if (!pg->active) {
-			counts = pg->counts;
-			pg->counts = NULL;
+			counts = current_counts;
+			rcu_assign_pointer(pg->counts, NULL);
 		}
 		spin_unlock_irqrestore(&pg->lock, flags);
 
 		if (counts)
-			mempool_free(counts, &dc->bypass_mempool);
+			call_rcu(&counts->rcu, bch_bypass_counts_free_rcu);
 	}
 }
 
@@ -924,20 +940,19 @@ static bool bch_has_active_bypass_writes(struct cached_dev *dc, sector_t sector,
 	if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
 		return false;
 
+	rcu_read_lock();
 	for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
 		unsigned long pg_idx = bypass_chunk_to_page(chunk);
 		unsigned long pg_off = bypass_chunk_to_offset(chunk);
 		struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
-		unsigned long flags;
+		struct bch_bypass_counts *current_counts = rcu_dereference(pg->counts);
 
-		spin_lock_irqsave(&pg->lock, flags);
-		if (pg->counts && pg->counts[pg_off] > 0) {
+		if (current_counts && READ_ONCE(current_counts->counts[pg_off]) > 0) {
 			has_active = true;
-			spin_unlock_irqrestore(&pg->lock, flags);
 			break;
 		}
-		spin_unlock_irqrestore(&pg->lock, flags);
 	}
+	rcu_read_unlock();
 
 	return has_active;
 }
@@ -1458,6 +1473,7 @@ void bch_flash_dev_request_init(struct bcache_device *d)
 
 void bch_request_exit(void)
 {
+	kmem_cache_destroy(bch_bypass_cache);
 	kmem_cache_destroy(bch_search_cache);
 }
 
@@ -1467,5 +1483,9 @@ int __init bch_request_init(void)
 	if (!bch_search_cache)
 		return -ENOMEM;
 
+	bch_bypass_cache = KMEM_CACHE(bch_bypass_counts, 0);
+	if (!bch_bypass_cache)
+		return -ENOMEM;
+
 	return 0;
 }
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index b994483511de..75aa6bb2e00b 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1350,9 +1350,12 @@ void bch_cached_dev_release(struct kobject *kobj)
 		unsigned long i;
 
 		for (i = 0; i < dc->bypass_num_pages; i++) {
-			if (dc->bypass_pages[i].counts)
-				mempool_free(dc->bypass_pages[i].counts, &dc->bypass_mempool);
+			struct bch_bypass_counts *counts =
+				rcu_dereference_protected(dc->bypass_pages[i].counts, 1);
+			if (counts)
+				mempool_free(counts, &dc->bypass_mempool);
 		}
+		rcu_barrier();
 		kvfree(dc->bypass_pages);
 		mempool_exit(&dc->bypass_mempool);
 	}
@@ -1433,7 +1436,7 @@ static int bch_cached_dev_bypass_init(struct cached_dev *dc, sector_t sectors)
 	for (i = 0; i < dc->bypass_num_pages; i++)
 		spin_lock_init(&dc->bypass_pages[i].lock);
 
-	if (mempool_init_kmalloc_pool(&dc->bypass_mempool, 16, PAGE_SIZE)) {
+	if (mempool_init_slab_pool(&dc->bypass_mempool, 16, bch_bypass_cache)) {
 		kvfree(dc->bypass_pages);
 		dc->bypass_pages = NULL;
 		return -ENOMEM;
-- 
2.55.0.679.g6767b8d81c-goog

Re: [PATCH v3 1/2] bcache: track active bypass writes to fix read miss race

[Coly Li]

On Sat, Aug 08, 2026 at 04:05:48AM +0800, Ankit Kapoor wrote:
> A race condition exists between a read cache miss and a bypass write
> due to either congestion or sequential bypass, which causes stale data
> to be cached when the read cache miss runs concurrently with a bypass
> write targeting the same sectors. If the read cache miss fetches data
> from the backing device before the write to the backing device finishes,
> stale data populates the cache.
> 
> The root cause is that bcache currently executes btree key
> invalidation in parallel with (or prior to) writing the actual data
> payload to the backing device. Under this sequence, a concurrent read
> path can register a cache miss and insert a placeholder key. If the
> write's btree key invalidation completes before the read finishes
> fetching old data from the backing device, the read's subsequent key
> replacement will not detect a collision, allowing stale data to
> persist in the cache.
> 
> Fix this by tracking active bypass writes and serializing cache
> invalidation.
> 
> First, divide the backing device space into 32MB chunks and track
> concurrent bypass writes using refcounts. The tracking counters are
> stored in dynamically allocated pages backed by a dedicated mempool
> to prevent allocation failures under memory pressure, minimizing overall
> footprint (a single 4KB page supports 32GB of disk space using u32
> counters).
> 
> On a cache miss read, bcache checks if there are any active bypass
> writes overlapping the target sectors. If an active bypass write is
> detected, the read is forced to bypass the cache to ensure data
> consistency.
> 
> Second, serialize the btree key invalidation (bch_data_insert) for
> bypass writes so that it executes in cached_dev_write_complete(), after
> the payload has been written to the backing device. This prevents early
> invalidation from opening a window where a concurrent read miss could
> fetch and re-cache stale data before the bypass write reaches the disk.
> 
> Suggested-by: Coly Li <colyli@fygo.io>
> Signed-off-by: Ankit Kapoor <ankitkap@google.com>

Hi Ankit,

I have seen the patch 2/2 with the RCU usage. The implementation is not
ideal as I expected. Your code help me to realize maybe choosing RCU isn't
the best method. 

I try to compose a kind of lock-less refcount check method without RCU,
and it looks fine, but I don't test it yet. 

Here I replay the idea inline with your code.

> ---
>  drivers/md/bcache/bcache.h  |  35 ++++++++++
>  drivers/md/bcache/request.c | 131 +++++++++++++++++++++++++++++++++++-
>  drivers/md/bcache/super.c   |  39 +++++++++++
>  3 files changed, 202 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index ec9ff9715081..2e50526a52fc 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -299,6 +299,12 @@ enum stop_on_failure {
>  	BCH_CACHED_DEV_STOP_MODE_MAX,
>  };
>  
> +struct bch_bypass_page {
> +	u32		*counts;
> +	unsigned int	active;
> +	spinlock_t	lock;
> +};
> +
>  struct cached_dev {
>  	struct list_head	list;
>  	struct bcache_device	disk;
> @@ -407,8 +413,37 @@ struct cached_dev {
>  	 */
>  #define BCH_WBRATE_UPDATE_MAX_SKIPS	15
>  	unsigned int		rate_update_retry;
> +
> +	/* For tracking active bypass writes */
> +#define BCH_BYPASS_CHUNK_SHIFT		16  /* 2^16 sectors = 32MB */
> +#define BCH_BYPASS_PAGE_COUNTERS	(PAGE_SIZE / sizeof(u32))
> +#define BCH_BYPASS_PAGE_SHIFT		(PAGE_SHIFT - 2)
> +#define BCH_BYPASS_PAGE_MASK		((1UL << BCH_BYPASS_PAGE_SHIFT) - 1)
> +	struct bch_bypass_page	*bypass_pages;
> +	unsigned long		bypass_num_pages;
> +	mempool_t		bypass_mempool;
>  };
>  
> +static inline unsigned long sector_to_bypass_chunk(sector_t sector)
> +{
> +	return sector >> BCH_BYPASS_CHUNK_SHIFT;
> +}
> +
> +static inline unsigned long bypass_chunk_to_page(unsigned long chunk)
> +{
> +	return chunk >> BCH_BYPASS_PAGE_SHIFT;
> +}
> +
> +static inline unsigned long bypass_chunk_to_offset(unsigned long chunk)
> +{
> +	return chunk & BCH_BYPASS_PAGE_MASK;
> +}
> +
> +static inline sector_t bypass_chunk_to_sector(unsigned long chunk)
> +{
> +	return (sector_t)chunk << BCH_BYPASS_CHUNK_SHIFT;
> +}
> +

The above code is good.



>  enum alloc_reserve {
>  	RESERVE_BTREE,
>  	RESERVE_PRIO,
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 3fa3b13a410f..bfd28b68f499 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -492,6 +492,9 @@ struct search {
>  	struct block_device	*orig_bdev;
>  	unsigned long		start_time;
>  
> +	sector_t		bypass_sector;
> +	unsigned int		bypass_sectors;
> +
>  	struct btree_op		op;
>  	struct data_insert_op	iop;
>  };

Make sense.


> @@ -763,11 +766,16 @@ static inline struct search *search_alloc(struct bio *bio,
>  
>  /* Cached devices */
>  
> +static void bch_bypass_write_end(struct cached_dev *dc, sector_t sector, unsigned int sectors);
> +
>  static CLOSURE_CALLBACK(cached_dev_bio_complete)
>  {
>  	closure_type(s, struct search, cl);
>  	struct cached_dev *dc = container_of(s->d, struct cached_dev, disk);
>  
> +	if (s->iop.bypass && op_is_write(bio_op(s->orig_bio)))
> +		bch_bypass_write_end(dc, s->bypass_sector, s->bypass_sectors);
> +

Looks good.


>  	cached_dev_put(dc);
>  	search_free(&cl->work);
>  }
> @@ -830,6 +838,110 @@ static CLOSURE_CALLBACK(cached_dev_cache_miss_done)
>  	closure_put(&d->cl);
>  }
>  
> +static void bch_bypass_write_start(struct cached_dev *dc, sector_t sector, unsigned int sectors)
> +{
> +	unsigned long start_chunk = sector_to_bypass_chunk(sector);
> +	unsigned long end_chunk = sector_to_bypass_chunk(sector + sectors - 1);
> +	unsigned long end_pg_idx = bypass_chunk_to_page(end_chunk);
> +	unsigned long chunk;
> +
> +	if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
> +		return;
> +
> +	for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
> +		unsigned long pg_idx = bypass_chunk_to_page(chunk);
> +		unsigned long pg_off = bypass_chunk_to_offset(chunk);
> +		struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
> +		u32 *new_counts;
> +		u32 *dup_counts = NULL;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&pg->lock, flags);
> +		if (!pg->counts) {
> +			spin_unlock_irqrestore(&pg->lock, flags);
> +			new_counts = mempool_alloc(&dc->bypass_mempool, GFP_NOIO);
> +			memset(new_counts, 0, PAGE_SIZE);
> +			spin_lock_irqsave(&pg->lock, flags);
> +			if (pg->counts)
> +				dup_counts = new_counts;
> +			else
> +				pg->counts = new_counts;
> +		}
> +		pg->counts[pg_off]++;
> +		pg->active++;
> +		spin_unlock_irqrestore(&pg->lock, flags);
> +
> +		if (dup_counts)
> +			mempool_free(dup_counts, &dc->bypass_mempool);
> +	}
> +}
> +
> +static void bch_bypass_write_end(struct cached_dev *dc, sector_t sector, unsigned int sectors)
> +{
> +	unsigned long start_chunk = sector_to_bypass_chunk(sector);
> +	unsigned long end_chunk = sector_to_bypass_chunk(sector + sectors - 1);
> +	unsigned long end_pg_idx = bypass_chunk_to_page(end_chunk);
> +	unsigned long chunk;
> +
> +	if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
> +		return;
> +
> +	for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
> +		unsigned long pg_idx = bypass_chunk_to_page(chunk);
> +		unsigned long pg_off = bypass_chunk_to_offset(chunk);
> +		struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
> +		u32 *counts = NULL;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&pg->lock, flags);
> +		if (WARN_ON_ONCE(!pg->counts || !pg->counts[pg_off])) {
> +			spin_unlock_irqrestore(&pg->lock, flags);
> +			continue;
> +		}
> +
> +		pg->counts[pg_off]--;
> +		pg->active--;
> +		if (!pg->active) {
> +			counts = pg->counts;
> +			pg->counts = NULL;
> +		}
> +		spin_unlock_irqrestore(&pg->lock, flags);
> +
> +		if (counts)
> +			mempool_free(counts, &dc->bypass_mempool);
> +	}
> +}
> +

I like the code to increase/decrease refcounts from small to large LBA.
I don't like spin_lock_irqsave()/spin_unlock_irqsave() here,
1. irqsave/restore might be unncessary, see my comments in following
   cached_dev_write_complete().
2, comparing to cmpxchg, using spin lock has an extra store operation,
   also also has more memory barrier operation on non-x86_64 archs.

I try to compose bch_bypass_write_start() and bch_bypass_write_end()
for your reference. I try to show what I thought based on your perfect
code base.


static void bch_bypass_page_put(struct cached_dev *dc,
                               struct bch_bypass_page *pg)
{
       unsigned long flags;

       if (refcount_dec_and_lock_irqsave(&pg->ref, &pg->lock,
					 &flags)) {
               atomic_t *counts = pg->counts;

               pg->counts = NULL;
               spin_unlock_irqrestore(&pg->lock, flags);
               mempool_free(counts, &dc->bypass_mempool);
       }
}

static void bch_bypass_write_start(struct cached_dev *dc,
				   sector_t sector,
				   unsigned int sectors)
{
       unsigned long start_chunk = sector_to_bypass_chunk(sector);
       unsigned long end_chunk = sector_to_bypass_chunk(sector + sectors - 1);
       unsigned long end_pg_idx = bypass_chunk_to_page(end_chunk);
       unsigned long chunk;

       if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
               return;

       for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
               unsigned long pg_idx = bypass_chunk_to_page(chunk);
               unsigned long pg_off = bypass_chunk_to_offset(chunk);
               struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
               atomic_t *counts;
               unsigned long flags;

retry:
               counts = READ_ONCE(pg->counts);
               if (!counts) {
                       atomic_t *new_counts;

                       new_counts = mempool_alloc(&dc->bypass_mempool,
                                                  GFP_NOIO);
                       memset(new_counts, 0, PAGE_SIZE);

                       spin_lock_irqsave(&pg->lock, flags);
                       if (pg->counts) {
                               refcount_inc(&pg->ref);
                               counts = pg->counts;
                               spin_unlock_irqrestore(&pg->lock, flags);
                               mempool_free(new_counts,
					    &dc->bypass_mempool);
                       } else {
                               pg->counts = new_counts;
                               refcount_set(&pg->ref, 1);
                               counts = new_counts;
                               spin_unlock_irqrestore(&pg->lock, flags);
                       }
               } else {
                       if (!refcount_inc_not_zero(&pg->ref))
                               goto retry;

                       if (unlikely(READ_ONCE(pg->counts) != counts)) {
                               bch_bypass_page_put(dc, pg);
                               goto retry;
                       }
               }

               atomic_inc(&counts[pg_off]);
       }
}

static void bch_bypass_write_end(struct cached_dev *dc,
				 sector_t sector,
				 unsigned int sectors)
{
       unsigned long start_chunk = sector_to_bypass_chunk(sector);
       unsigned long end_chunk = sector_to_bypass_chunk(sector + sectors - 1);
       unsigned long end_pg_idx = bypass_chunk_to_page(end_chunk);
       unsigned long chunk;

       if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
               return;

       for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
               unsigned long pg_idx = bypass_chunk_to_page(chunk);
               unsigned long pg_off = bypass_chunk_to_offset(chunk);
               struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
               atomic_t *counts;

               counts = READ_ONCE(pg->counts);

               if (!WARN_ON_ONCE(!counts || !atomic_read(&counts[pg_off])))
                       atomic_dec(&counts[pg_off]);

               bch_bypass_page_put(dc, pg);
       }
}

In the above code, pg->ref is used to make sure during accessing the refcount at
counts[pg_off], the counters page are refrenced and won't be freed. It is similar
to pg->active in your patch.


> +static bool bch_has_active_bypass_writes(struct cached_dev *dc, sector_t sector,
> +					 unsigned int sectors)
> +{
> +	unsigned long start_chunk = sector_to_bypass_chunk(sector);
> +	unsigned long end_chunk = sector_to_bypass_chunk(sector + sectors - 1);
> +	unsigned long end_pg_idx = bypass_chunk_to_page(end_chunk);
> +	unsigned long chunk;
> +	bool has_active = false;
> +
> +	if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
> +		return false;
> +
> +	for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
> +		unsigned long pg_idx = bypass_chunk_to_page(chunk);
> +		unsigned long pg_off = bypass_chunk_to_offset(chunk);
> +		struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&pg->lock, flags);
> +		if (pg->counts && pg->counts[pg_off] > 0) {
> +			has_active = true;
> +			spin_unlock_irqrestore(&pg->lock, flags);
> +			break;
> +		}
> +		spin_unlock_irqrestore(&pg->lock, flags);
> +	}
> +
> +	return has_active;
> +}
> +

From the above code, firstly spin_lock_irqsave() is unnecessary. And I try
my best to avoid to use spin_lock/unlock routines. Although indeed the
location to call bch_has_active_bypass_writes() is not very hot, make it
less cost is always better.

static bool bch_has_active_bypass_writes(struct cached_dev *dc,
					 sector_t sector,
					 unsigned int sectors)
{
       unsigned long start_chunk = sector_to_bypass_chunk(sector);
       unsigned long end_chunk = sector_to_bypass_chunk(sector + sectors - 1);
       unsigned long end_pg_idx = bypass_chunk_to_page(end_chunk);
       unsigned long chunk;
       bool has_active = false;

       if (WARN_ON_ONCE(end_pg_idx >= dc->bypass_num_pages))
               return false;

       for (chunk = start_chunk; chunk <= end_chunk; chunk++) {
               unsigned long pg_idx = bypass_chunk_to_page(chunk);
               unsigned long pg_off = bypass_chunk_to_offset(chunk);
               struct bch_bypass_page *pg = &dc->bypass_pages[pg_idx];
               atomic_t *counts;
               int val = 0;

retry:
               counts = READ_ONCE(pg->counts);
               if (!counts)
                       continue;

               if (!refcount_inc_not_zero(&pg->ref))
                       goto retry;

               if (unlikely(READ_ONCE(pg->counts) != counts)) {
                       bch_bypass_page_put(dc, pg);
                       goto retry;
               }

               val = atomic_read(&counts[pg_off]);
               bch_bypass_page_put(dc, pg);

               if (val > 0) {
                       has_active = true;
                       break;
               }
       }

       return has_active;
}


>  static CLOSURE_CALLBACK(cached_dev_read_done)
>  {
>  	closure_type(s, struct search, cl);
> @@ -864,7 +976,9 @@ static CLOSURE_CALLBACK(cached_dev_read_done)
>  	bio_complete(s);
>  
>  	if (s->iop.bio &&
> -	    !test_bit(CACHE_SET_STOPPING, &s->iop.c->flags)) {
> +	    !test_bit(CACHE_SET_STOPPING, &s->iop.c->flags) &&
> +	    !bch_has_active_bypass_writes(dc, s->iop.bio->bi_iter.bi_sector,
> +					  bio_sectors(s->iop.bio))) {
>  		BUG_ON(!s->iop.replace);
>  		closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
>  	}

Yes, this is key change. If there is bypass write flying after
the check key inserted, don't insert the replacement key back into btree.



> @@ -975,7 +1089,13 @@ static CLOSURE_CALLBACK(cached_dev_write_complete)
>  	struct cached_dev *dc = container_of(s->d, struct cached_dev, disk);
>  
>  	up_read_non_owner(&dc->writeback_lock);
> -	cached_dev_bio_complete(&cl->work);
> +
> +	if (s->iop.bypass) {
> +		closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
> +		continue_at(cl, cached_dev_bio_complete, NULL);

The ordering this correct, but the usage of continue_at() here is wrong.
cached_dev_write_complete() might be called in irq context, call continue_at
without a workqueue will run cached_dev_bio_complete() directly in irq context.
There are potential blocking operations inside cached_dev_bio_complete(),
so this is not wanted.

You should use a workqueue, e.g. s->iop.wq, or directly bcache_wq,
	continue_at(cl, cached_dev_bio_complete, s->iop.wq);


> +	} else {
> +		cached_dev_bio_complete(&cl->work);
> +	}
>  }
>  
>  static void cached_dev_write(struct cached_dev *dc, struct search *s)
> @@ -1018,6 +1138,10 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s)
>  		s->iop.bio = s->orig_bio;
>  		bio_get(s->iop.bio);
>  
> +		s->bypass_sector = bio->bi_iter.bi_sector;
> +		s->bypass_sectors = bio_sectors(bio);
> +		bch_bypass_write_start(dc, s->bypass_sector, s->bypass_sectors);
> +
>  		if (bio_op(bio) == REQ_OP_DISCARD &&
>  		    !bdev_max_discard_sectors(dc->bdev))
>  			goto insert_data;
> @@ -1058,7 +1182,8 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s)
>  	}
>  
>  insert_data:
> -	closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
> +	if (!s->iop.bypass)
> +		closure_call(&s->iop.cl, bch_data_insert, NULL, cl);
>  	continue_at(cl, cached_dev_write_complete, NULL);
>  }
> 

The above changes look good to me.


 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 97d9adb0bf96..b994483511de 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1346,6 +1346,16 @@ void bch_cached_dev_release(struct kobject *kobj)
>  {
>  	struct cached_dev *dc = container_of(kobj, struct cached_dev,
>  					     disk.kobj);
> +	if (dc->bypass_pages) {
> +		unsigned long i;
> +
> +		for (i = 0; i < dc->bypass_num_pages; i++) {
> +			if (dc->bypass_pages[i].counts)
> +				mempool_free(dc->bypass_pages[i].counts, &dc->bypass_mempool);
> +		}
> +		kvfree(dc->bypass_pages);
> +		mempool_exit(&dc->bypass_mempool);
> +	}
>  	kfree(dc);
>  	module_put(THIS_MODULE);
>  }

free bypass_pages should not be executed here. See my last comments in this
email after cached_dev_init().


> @@ -1407,6 +1417,31 @@ static CLOSURE_CALLBACK(cached_dev_flush)
>  	continue_at(cl, cached_dev_free, system_percpu_wq);
>  }
>  
> +static int bch_cached_dev_bypass_init(struct cached_dev *dc, sector_t sectors)
> +{
> +	unsigned long chunks =
> +		sector_to_bypass_chunk(sectors + (1UL << BCH_BYPASS_CHUNK_SHIFT) - 1);
> +	unsigned long i;
> +
> +	dc->bypass_num_pages = DIV_ROUND_UP(chunks, BCH_BYPASS_PAGE_COUNTERS);
> +	dc->bypass_pages = kvcalloc(dc->bypass_num_pages,
> +				   sizeof(struct bch_bypass_page),
> +				   GFP_KERNEL);
> +	if (!dc->bypass_pages)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < dc->bypass_num_pages; i++)
> +		spin_lock_init(&dc->bypass_pages[i].lock);
> +
> +	if (mempool_init_kmalloc_pool(&dc->bypass_mempool, 16, PAGE_SIZE)) {
> +		kvfree(dc->bypass_pages);
> +		dc->bypass_pages = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}

In general the above code is fine, but I suggest to change the order of
some lines, and the result code looks in the following shape.

static int bch_cached_dev_bypass_init(struct cached_dev *dc, sector_t sectors)
{
       unsigned long chunks;
       unsigned long i;

       if (dc->bypass_pages)
               return 0;

       chunks = DIV_ROUND_UP(sectors, 1UL << BCH_BYPASS_CHUNK_SHIFT);
       dc->bypass_num_pages = DIV_ROUND_UP(chunks, BCH_BYPASS_PAGE_COUNTERS);
       dc->bypass_pages = kvcalloc(dc->bypass_num_pages,
                                   sizeof(struct bch_bypass_page),
                                   GFP_KERNEL);
       if (!dc->bypass_pages)
               return -ENOMEM;

       for (i = 0; i < dc->bypass_num_pages; i++)
               spin_lock_init(&dc->bypass_pages[i].lock);

       if (mempool_init_kmalloc_pool(&dc->bypass_mempool, 16, PAGE_SIZE)) {
               kvfree(dc->bypass_pages);
               dc->bypass_pages = NULL;
               return -ENOMEM;
       }

       return 0;
}

> +
>  static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
>  {
>  	int ret;
> @@ -1447,6 +1482,10 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
>  	/* default to auto */
>  	dc->stop_when_cache_set_failed = BCH_CACHED_DEV_STOP_AUTO;
>  
> +	ret = bch_cached_dev_bypass_init(dc, bdev_nr_sectors(dc->bdev));
> +	if (ret)
> +		return ret;
> +
>  	bch_cached_dev_request_init(dc);
>  	bch_cached_dev_writeback_init(dc);
>  	return 0;

The init and exit code should be called in bcache device attach/detach time.
Allocating the mempool of slab cache for bcache device not attached to a cache
set is a waste of memory.

Maybe the code can be look in the following shape.

1, Add a helper to free counter pages and the bypass_mempool.

static void bch_cached_dev_bypass_exit(struct cached_dev *dc)
{
       unsigned long i;

       if (!dc->bypass_pages)
               return;

       for (i = 0; i < dc->bypass_num_pages; i++) {
               if (dc->bypass_pages[i].counts)
                       mempool_free(dc->bypass_pages[i].counts,
                                    &dc->bypass_mempool);
       }
       kvfree(dc->bypass_pages);
       dc->bypass_pages = NULL;
       dc->bypass_num_pages = 0;
       mempool_exit(&dc->bypass_mempool);
}


2, Call bch_cached_dev_bypass_exit() when detach or stop the bcache
   device. If the bcache device is stopping, bch_cached_dev_bypass_exit()
   might be called twice, therefore if (!dc->bypass_pages) is neccessary.

 static void cached_dev_detach_finish(struct work_struct *w)
 {
        struct cached_dev *dc = container_of(w, struct cached_dev, detach);
        struct cache_set *c = dc->disk.c;

        BUG_ON(!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags));
        BUG_ON(refcount_read(&dc->count));

+       bch_cached_dev_bypass_exit(dc);

        if (test_and_clear_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags))
                cancel_writeback_rate_update_dwork(dc);

        if (!IS_ERR_OR_NULL(dc->writeback_thread)) {



int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
 /* when dc->disk.kobj released */
 void bch_cached_dev_release(struct kobject *kobj)
 {
        struct cached_dev *dc = container_of(kobj, struct cached_dev,
                                             disk.kobj);

+       bch_cached_dev_bypass_exit(dc);
        kfree(dc);
        module_put(THIS_MODULE);
 }


3, Initialize the mempool at attachtime, not in cached_dev_init(),
int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
        } else {
                u->last_reg = rtime;
                bch_uuid_write(c);
        }

+       ret = bch_cached_dev_bypass_init(dc, bdev_nr_sectors(dc->bdev));
+       if (ret)
+               return ret;
+
        bcache_device_attach(&dc->disk, c, u - c->uuids);
        list_move(&dc->list, &c->cached_devs);
        calc_cached_dev_sectors(c);

        /*


By all the above changes, the second patch with RCU can be dropped, and
only this patch should work for the fix.

But the stale clean data race between bypass write and cache miss read
is still not 100% solved. The last issue is,
1, If the data of bypass write hits hard driver platter firstly.
2, There is an overlapped clean key in btree, and not invlidated yet.
3, Power failure happens.
Then the stale clean key is still in btree and new data is on hard drive.

To fix the above very very rare condition need to update the bcache
super block to mark an unclear stop. Then after boot up if this mark
detected then all clean keys will be invalidated. This fix will change
on-disk format, I will compose a patch latter.

Current patch will solve most of the situation of the addressed stale
clean key race. Because no on-disk format change, it should be much
easier to do backport for down stream kernel maintainers.

I feel for the fix it self, after you update all the suggested stuffs
in my comments, it should be good. If you want to add another patch for
the sysfs interface files, please post them in next version.

NOTE: all code in my comments are just to show my thought, they don't
pass compiling, and not test at all. You need to make them work.

Thanks.

Coly Li