* [PATCH 1/9] bcache: get rid of discard code from journal
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
@ 2025-11-13 5:36 ` colyli
2025-11-13 5:36 ` [PATCH 2/9] bcache: remove discard code from alloc.c colyli
` (8 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: colyli @ 2025-11-13 5:36 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block, Coly Li
From: Coly Li <colyli@fnnas.com>
In bcache journal there is discard functionality but almost useless in
reality. Because discard happens after a journal bucket is reclaimed,
and the reclaimed bucket is allocated for new journaling immediately.
There is no time for underlying SSD to use the discard hint for internal
data management.
The discard code in bcache journal doesn't bring any performance
optimization and wastes CPU cycles for issuing discard bios. Therefore
this patch gits rid of it from journal.c and journal.h.
Signed-off-by: Coly Li <colyli@fnnas.com>
---
drivers/md/bcache/journal.c | 93 ++++---------------------------------
drivers/md/bcache/journal.h | 13 ------
2 files changed, 8 insertions(+), 98 deletions(-)
diff --git a/drivers/md/bcache/journal.c b/drivers/md/bcache/journal.c
index d50eb82ccb4f..144693b7c46a 100644
--- a/drivers/md/bcache/journal.c
+++ b/drivers/md/bcache/journal.c
@@ -275,8 +275,7 @@ int bch_journal_read(struct cache_set *c, struct list_head *list)
* ja->cur_idx
*/
ja->cur_idx = i;
- ja->last_idx = ja->discard_idx = (i + 1) %
- ca->sb.njournal_buckets;
+ ja->last_idx = (i + 1) % ca->sb.njournal_buckets;
}
@@ -336,16 +335,6 @@ void bch_journal_mark(struct cache_set *c, struct list_head *list)
}
}
-static bool is_discard_enabled(struct cache_set *s)
-{
- struct cache *ca = s->cache;
-
- if (ca->discard)
- return true;
-
- return false;
-}
-
int bch_journal_replay(struct cache_set *s, struct list_head *list)
{
int ret = 0, keys = 0, entries = 0;
@@ -360,15 +349,10 @@ int bch_journal_replay(struct cache_set *s, struct list_head *list)
BUG_ON(i->pin && atomic_read(i->pin) != 1);
if (n != i->j.seq) {
- if (n == start && is_discard_enabled(s))
- pr_info("journal entries %llu-%llu may be discarded! (replaying %llu-%llu)\n",
- n, i->j.seq - 1, start, end);
- else {
- pr_err("journal entries %llu-%llu missing! (replaying %llu-%llu)\n",
- n, i->j.seq - 1, start, end);
- ret = -EIO;
- goto err;
- }
+ pr_err("journal entries %llu-%llu missing! (replaying %llu-%llu)\n",
+ n, i->j.seq - 1, start, end);
+ ret = -EIO;
+ goto err;
}
for (k = i->j.start;
@@ -568,65 +552,6 @@ static void btree_flush_write(struct cache_set *c)
#define last_seq(j) ((j)->seq - fifo_used(&(j)->pin) + 1)
-static void journal_discard_endio(struct bio *bio)
-{
- struct journal_device *ja =
- container_of(bio, struct journal_device, discard_bio);
- struct cache *ca = container_of(ja, struct cache, journal);
-
- atomic_set(&ja->discard_in_flight, DISCARD_DONE);
-
- closure_wake_up(&ca->set->journal.wait);
- closure_put(&ca->set->cl);
-}
-
-static void journal_discard_work(struct work_struct *work)
-{
- struct journal_device *ja =
- container_of(work, struct journal_device, discard_work);
-
- submit_bio(&ja->discard_bio);
-}
-
-static void do_journal_discard(struct cache *ca)
-{
- struct journal_device *ja = &ca->journal;
- struct bio *bio = &ja->discard_bio;
-
- if (!ca->discard) {
- ja->discard_idx = ja->last_idx;
- return;
- }
-
- switch (atomic_read(&ja->discard_in_flight)) {
- case DISCARD_IN_FLIGHT:
- return;
-
- case DISCARD_DONE:
- ja->discard_idx = (ja->discard_idx + 1) %
- ca->sb.njournal_buckets;
-
- atomic_set(&ja->discard_in_flight, DISCARD_READY);
- fallthrough;
-
- case DISCARD_READY:
- if (ja->discard_idx == ja->last_idx)
- return;
-
- atomic_set(&ja->discard_in_flight, DISCARD_IN_FLIGHT);
-
- bio_init_inline(bio, ca->bdev, 1, REQ_OP_DISCARD);
- bio->bi_iter.bi_sector = bucket_to_sector(ca->set,
- ca->sb.d[ja->discard_idx]);
- bio->bi_iter.bi_size = bucket_bytes(ca);
- bio->bi_end_io = journal_discard_endio;
-
- closure_get(&ca->set->cl);
- INIT_WORK(&ja->discard_work, journal_discard_work);
- queue_work(bch_journal_wq, &ja->discard_work);
- }
-}
-
static unsigned int free_journal_buckets(struct cache_set *c)
{
struct journal *j = &c->journal;
@@ -635,10 +560,10 @@ static unsigned int free_journal_buckets(struct cache_set *c)
unsigned int n;
/* In case njournal_buckets is not power of 2 */
- if (ja->cur_idx >= ja->discard_idx)
- n = ca->sb.njournal_buckets + ja->discard_idx - ja->cur_idx;
+ if (ja->cur_idx >= ja->last_idx)
+ n = ca->sb.njournal_buckets + ja->last_idx - ja->cur_idx;
else
- n = ja->discard_idx - ja->cur_idx;
+ n = ja->last_idx - ja->cur_idx;
if (n > (1 + j->do_reserve))
return n - (1 + j->do_reserve);
@@ -668,8 +593,6 @@ static void journal_reclaim(struct cache_set *c)
ja->last_idx = (ja->last_idx + 1) %
ca->sb.njournal_buckets;
- do_journal_discard(ca);
-
if (c->journal.blocks_free)
goto out;
diff --git a/drivers/md/bcache/journal.h b/drivers/md/bcache/journal.h
index cd316b4a1e95..9e9d1b3016a5 100644
--- a/drivers/md/bcache/journal.h
+++ b/drivers/md/bcache/journal.h
@@ -139,19 +139,6 @@ struct journal_device {
/* Last journal bucket that still contains an open journal entry */
unsigned int last_idx;
- /* Next journal bucket to be discarded */
- unsigned int discard_idx;
-
-#define DISCARD_READY 0
-#define DISCARD_IN_FLIGHT 1
-#define DISCARD_DONE 2
- /* 1 - discard in flight, -1 - discard completed */
- atomic_t discard_in_flight;
-
- struct work_struct discard_work;
- struct bio discard_bio;
- struct bio_vec discard_bv;
-
/* Bio for journal reads/writes to this device */
struct bio bio;
struct bio_vec bv[8];
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/9] bcache: remove discard code from alloc.c
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
2025-11-13 5:36 ` [PATCH 1/9] bcache: get rid of discard code from journal colyli
@ 2025-11-13 5:36 ` colyli
2025-11-13 5:36 ` [PATCH 3/9] bcache: drop discard sysfs interface colyli
` (7 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: colyli @ 2025-11-13 5:36 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block, Coly Li
From: Coly Li <colyli@fnnas.com>
Bcache allocator initially has no free space to allocate. Firstly it
does a garbage collection which is triggered by a cache device write
and fills free space into ca->free[] lists. The discard happens after
the free bucket is handled by garbage collection added into one of the
ca->free[] lists. But normally this bucket will be allocated out very
soon to requester and filled data onto it. The discard hint on this
bucket LBA range doesn't help SSD control to improve internal erasure
performance, and waste extra CPU cycles to issue discard bios.
This patch removes the almost-useless discard code from alloc.c.
Signed-off-by: Coly Li <colyli@fnnas.com>
---
drivers/md/bcache/alloc.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)
diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index 48ce750bf70a..db3684819e38 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -24,21 +24,18 @@
* Since the gens and priorities are all stored contiguously on disk, we can
* batch this up: We fill up the free_inc list with freshly invalidated buckets,
* call prio_write(), and when prio_write() finishes we pull buckets off the
- * free_inc list and optionally discard them.
+ * free_inc list.
*
* free_inc isn't the only freelist - if it was, we'd often to sleep while
* priorities and gens were being written before we could allocate. c->free is a
* smaller freelist, and buckets on that list are always ready to be used.
*
- * If we've got discards enabled, that happens when a bucket moves from the
- * free_inc list to the free list.
- *
* There is another freelist, because sometimes we have buckets that we know
* have nothing pointing into them - these we can reuse without waiting for
* priorities to be rewritten. These come from freed btree nodes and buckets
* that garbage collection discovered no longer had valid keys pointing into
* them (because they were overwritten). That's the unused list - buckets on the
- * unused list move to the free list, optionally being discarded in the process.
+ * unused list move to the free list.
*
* It's also important to ensure that gens don't wrap around - with respect to
* either the oldest gen in the btree or the gen on disk. This is quite
@@ -118,8 +115,7 @@ void bch_rescale_priorities(struct cache_set *c, int sectors)
/*
* Background allocation thread: scans for buckets to be invalidated,
* invalidates them, rewrites prios/gens (marking them as invalidated on disk),
- * then optionally issues discard commands to the newly free buckets, then puts
- * them on the various freelists.
+ * then puts them on the various freelists.
*/
static inline bool can_inc_bucket_gen(struct bucket *b)
@@ -321,8 +317,7 @@ static int bch_allocator_thread(void *arg)
while (1) {
/*
* First, we pull buckets off of the unused and free_inc lists,
- * possibly issue discards to them, then we add the bucket to
- * the free list:
+ * then we add the bucket to the free list:
*/
while (1) {
long bucket;
@@ -330,14 +325,6 @@ static int bch_allocator_thread(void *arg)
if (!fifo_pop(&ca->free_inc, bucket))
break;
- if (ca->discard) {
- mutex_unlock(&ca->set->bucket_lock);
- blkdev_issue_discard(ca->bdev,
- bucket_to_sector(ca->set, bucket),
- ca->sb.bucket_size, GFP_KERNEL);
- mutex_lock(&ca->set->bucket_lock);
- }
-
allocator_wait(ca, bch_allocator_push(ca, bucket));
wake_up(&ca->set->btree_cache_wait);
wake_up(&ca->set->bucket_wait);
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 3/9] bcache: drop discard sysfs interface
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
2025-11-13 5:36 ` [PATCH 1/9] bcache: get rid of discard code from journal colyli
2025-11-13 5:36 ` [PATCH 2/9] bcache: remove discard code from alloc.c colyli
@ 2025-11-13 5:36 ` colyli
2025-11-13 5:36 ` [PATCH 4/9] bcache: remove discard sysfs interface document colyli
` (6 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: colyli @ 2025-11-13 5:36 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block, Coly Li
From: Coly Li <colyli@fnnas.com>
Since discard code is removed, now the sysfs interface to enable discard
is useless. This patch removes the corresponding sysfs entry, and remove
bool variable 'discard' from struct cache as well.
Signed-off-by: Coly Li <colyli@fnnas.com>
---
drivers/md/bcache/bcache.h | 5 +----
drivers/md/bcache/super.c | 3 ---
drivers/md/bcache/sysfs.c | 15 ---------------
drivers/md/bcache/writeback.c | 3 +--
4 files changed, 2 insertions(+), 24 deletions(-)
diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 1d33e40d26ea..b8bd6d4a4298 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -447,8 +447,7 @@ struct cache {
* free_inc: Incoming buckets - these are buckets that currently have
* cached data in them, and we can't reuse them until after we write
* their new gen to disk. After prio_write() finishes writing the new
- * gens/prios, they'll be moved to the free list (and possibly discarded
- * in the process)
+ * gens/prios, they'll be moved to the free list.
*/
DECLARE_FIFO(long, free)[RESERVE_NR];
DECLARE_FIFO(long, free_inc);
@@ -467,8 +466,6 @@ struct cache {
*/
unsigned int invalidate_needs_gc;
- bool discard; /* Get rid of? */
-
struct journal_device journal;
/* The rest of this all shows up in sysfs */
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 6d250e366412..91a98ebc3f80 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2382,9 +2382,6 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
ca->bdev = file_bdev(bdev_file);
ca->sb_disk = sb_disk;
- if (bdev_max_discard_sectors(file_bdev(bdev_file)))
- ca->discard = CACHE_DISCARD(&ca->sb);
-
ret = cache_alloc(ca);
if (ret != 0) {
if (ret == -ENOMEM)
diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index 826b14cae4e5..72f38e5b6f5c 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -134,7 +134,6 @@ read_attribute(partial_stripes_expensive);
rw_attribute(synchronous);
rw_attribute(journal_delay_ms);
rw_attribute(io_disable);
-rw_attribute(discard);
rw_attribute(running);
rw_attribute(label);
rw_attribute(errors);
@@ -1036,7 +1035,6 @@ SHOW(__bch_cache)
sysfs_hprint(bucket_size, bucket_bytes(ca));
sysfs_hprint(block_size, block_bytes(ca));
sysfs_print(nbuckets, ca->sb.nbuckets);
- sysfs_print(discard, ca->discard);
sysfs_hprint(written, atomic_long_read(&ca->sectors_written) << 9);
sysfs_hprint(btree_written,
atomic_long_read(&ca->btree_sectors_written) << 9);
@@ -1142,18 +1140,6 @@ STORE(__bch_cache)
if (bcache_is_reboot)
return -EBUSY;
- if (attr == &sysfs_discard) {
- bool v = strtoul_or_return(buf);
-
- if (bdev_max_discard_sectors(ca->bdev))
- ca->discard = v;
-
- if (v != CACHE_DISCARD(&ca->sb)) {
- SET_CACHE_DISCARD(&ca->sb, v);
- bcache_write_super(ca->set);
- }
- }
-
if (attr == &sysfs_cache_replacement_policy) {
v = __sysfs_match_string(cache_replacement_policies, -1, buf);
if (v < 0)
@@ -1185,7 +1171,6 @@ static struct attribute *bch_cache_attrs[] = {
&sysfs_block_size,
&sysfs_nbuckets,
&sysfs_priority_stats,
- &sysfs_discard,
&sysfs_written,
&sysfs_btree_written,
&sysfs_metadata_written,
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 6ba73dc1a3df..cffef33b4acf 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -805,8 +805,7 @@ static int bch_writeback_thread(void *arg)
* may set BCH_ENABLE_AUTO_GC via sysfs, then when
* BCH_DO_AUTO_GC is set, garbage collection thread
* will be wake up here. After moving gc, the shrunk
- * btree and discarded free buckets SSD space may be
- * helpful for following write requests.
+ * btree may be helpful for following write requests.
*/
if (c->gc_after_writeback ==
(BCH_ENABLE_AUTO_GC|BCH_DO_AUTO_GC)) {
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 4/9] bcache: remove discard sysfs interface document
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
` (2 preceding siblings ...)
2025-11-13 5:36 ` [PATCH 3/9] bcache: drop discard sysfs interface colyli
@ 2025-11-13 5:36 ` colyli
2025-11-13 5:36 ` [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time colyli
` (5 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: colyli @ 2025-11-13 5:36 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block, Coly Li
From: Coly Li <colyli@fnnas.com>
This patch removes documents of bcache discard sysfs interface, it
drops discard related sections from,
- Documentation/ABI/testing/sysfs-block-bcache
- Documentation/admin-guide/bcache.rst
Signed-off-by: Coly Li <colyli@fnnas.com>
---
Documentation/ABI/testing/sysfs-block-bcache | 7 -------
Documentation/admin-guide/bcache.rst | 13 ++-----------
2 files changed, 2 insertions(+), 18 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-block-bcache b/Documentation/ABI/testing/sysfs-block-bcache
index 9e4bbc5d51fd..9344a657ca70 100644
--- a/Documentation/ABI/testing/sysfs-block-bcache
+++ b/Documentation/ABI/testing/sysfs-block-bcache
@@ -106,13 +106,6 @@ Description:
will be discarded from the cache. Should not be turned off with
writeback caching enabled.
-What: /sys/block/<disk>/bcache/discard
-Date: November 2010
-Contact: Kent Overstreet <kent.overstreet@gmail.com>
-Description:
- For a cache, a boolean allowing discard/TRIM to be turned off
- or back on if the device supports it.
-
What: /sys/block/<disk>/bcache/bucket_size
Date: November 2010
Contact: Kent Overstreet <kent.overstreet@gmail.com>
diff --git a/Documentation/admin-guide/bcache.rst b/Documentation/admin-guide/bcache.rst
index 6fdb495ac466..f71f349553e4 100644
--- a/Documentation/admin-guide/bcache.rst
+++ b/Documentation/admin-guide/bcache.rst
@@ -17,8 +17,7 @@ The latest bcache kernel code can be found from mainline Linux kernel:
It's designed around the performance characteristics of SSDs - it only allocates
in erase block sized buckets, and it uses a hybrid btree/log to track cached
extents (which can be anywhere from a single sector to the bucket size). It's
-designed to avoid random writes at all costs; it fills up an erase block
-sequentially, then issues a discard before reusing it.
+designed to avoid random writes at all costs.
Both writethrough and writeback caching are supported. Writeback defaults to
off, but can be switched on and off arbitrarily at runtime. Bcache goes to
@@ -618,19 +617,11 @@ bucket_size
cache_replacement_policy
One of either lru, fifo or random.
-discard
- Boolean; if on a discard/TRIM will be issued to each bucket before it is
- reused. Defaults to off, since SATA TRIM is an unqueued command (and thus
- slow).
-
freelist_percent
Size of the freelist as a percentage of nbuckets. Can be written to to
increase the number of buckets kept on the freelist, which lets you
artificially reduce the size of the cache at runtime. Mostly for testing
- purposes (i.e. testing how different size caches affect your hit rate), but
- since buckets are discarded when they move on to the freelist will also make
- the SSD's garbage collection easier by effectively giving it more reserved
- space.
+ purposes (i.e. testing how different size caches affect your hit rate).
io_errors
Number of errors that have occurred, decayed by io_error_halflife.
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
` (3 preceding siblings ...)
2025-11-13 5:36 ` [PATCH 4/9] bcache: remove discard sysfs interface document colyli
@ 2025-11-13 5:36 ` colyli
2026-07-17 0:50 ` Robert Pang
2025-11-13 5:36 ` [PATCH 6/9] bcache: remove redundant __GFP_NOWARN colyli
` (4 subsequent siblings)
9 siblings, 1 reply; 16+ messages in thread
From: colyli @ 2025-11-13 5:36 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block, Coly Li, Robert Pang, Mingzhe Zou
From: Coly Li <colyli@fnnas.com>
When bcache device is busy for high I/O loads, there are two methods to
reduce the garbage collection latency,
- Process less nodes in eac loop of incremental garbage collection in
btree_gc_recurse().
- Sleep less time between two full garbage collection in
bch_btree_gc().
This patch introduces to hleper routines to provide different garbage
collection nodes number and sleep intervel time.
- btree_gc_min_nodes()
If there is no front end I/O, return 128 nodes to process in each
incremental loop, otherwise only 10 nodes are returned. Then front I/O
is able to access the btree earlier.
- btree_gc_sleep_ms()
If there is no synchronized wait for bucket allocation, sleep 100 ms
between two incremental GC loop. Othersize only sleep 10 ms before
incremental GC loop. Then a faster GC may provide available buckets
earlier, to avoid most of bcache working threads from being starved by
buckets allocation.
The idea is inspired by works from Mingzhe Zou and Robert Pang, but much
simpler and the expected behavior is more predictable.
Signed-off-by: Coly Li <colyli@fnnas.com>
Signed-off-by: Robert Pang <robertpang@google.com>
Signed-off-by: Mingzhe Zou <mingzhe.zou@easystack.cn>
---
drivers/md/bcache/alloc.c | 4 ++++
drivers/md/bcache/bcache.h | 1 +
drivers/md/bcache/btree.c | 48 +++++++++++++++++++-------------------
3 files changed, 29 insertions(+), 24 deletions(-)
diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index db3684819e38..7708d92df23e 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -399,7 +399,11 @@ long bch_bucket_alloc(struct cache *ca, unsigned int reserve, bool wait)
TASK_UNINTERRUPTIBLE);
mutex_unlock(&ca->set->bucket_lock);
+
+ atomic_inc(&ca->set->bucket_wait_cnt);
schedule();
+ atomic_dec(&ca->set->bucket_wait_cnt);
+
mutex_lock(&ca->set->bucket_lock);
} while (!fifo_pop(&ca->free[RESERVE_NONE], r) &&
!fifo_pop(&ca->free[reserve], r));
diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index b8bd6d4a4298..8ccacba85547 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -604,6 +604,7 @@ struct cache_set {
*/
atomic_t prio_blocked;
wait_queue_head_t bucket_wait;
+ atomic_t bucket_wait_cnt;
/*
* For any bio we don't skip we subtract the number of sectors from
diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 210b59007d98..5d922d301ab6 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -89,8 +89,9 @@
* Test module load/unload
*/
-#define MAX_GC_TIMES 100
-#define MIN_GC_NODES 100
+#define MAX_GC_TIMES_SHIFT 7 /* 128 loops */
+#define GC_NODES_MIN 10
+#define GC_SLEEP_MS_MIN 10
#define GC_SLEEP_MS 100
#define PTR_DIRTY_BIT (((uint64_t) 1 << 36))
@@ -1578,29 +1579,29 @@ static unsigned int btree_gc_count_keys(struct btree *b)
static size_t btree_gc_min_nodes(struct cache_set *c)
{
- size_t min_nodes;
+ size_t min_nodes = GC_NODES_MIN;
- /*
- * Since incremental GC would stop 100ms when front
- * side I/O comes, so when there are many btree nodes,
- * if GC only processes constant (100) nodes each time,
- * GC would last a long time, and the front side I/Os
- * would run out of the buckets (since no new bucket
- * can be allocated during GC), and be blocked again.
- * So GC should not process constant nodes, but varied
- * nodes according to the number of btree nodes, which
- * realized by dividing GC into constant(100) times,
- * so when there are many btree nodes, GC can process
- * more nodes each time, otherwise, GC will process less
- * nodes each time (but no less than MIN_GC_NODES)
- */
- min_nodes = c->gc_stats.nodes / MAX_GC_TIMES;
- if (min_nodes < MIN_GC_NODES)
- min_nodes = MIN_GC_NODES;
+ if (atomic_read(&c->search_inflight) == 0) {
+ size_t n = c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
+
+ if (min_nodes < n)
+ min_nodes = n;
+ }
return min_nodes;
}
+static uint64_t btree_gc_sleep_ms(struct cache_set *c)
+{
+ uint64_t sleep_ms;
+
+ if (atomic_read(&c->bucket_wait_cnt) > 0)
+ sleep_ms = GC_SLEEP_MS_MIN;
+ else
+ sleep_ms = GC_SLEEP_MS;
+
+ return sleep_ms;
+}
static int btree_gc_recurse(struct btree *b, struct btree_op *op,
struct closure *writes, struct gc_stat *gc)
@@ -1668,8 +1669,7 @@ static int btree_gc_recurse(struct btree *b, struct btree_op *op,
memmove(r + 1, r, sizeof(r[0]) * (GC_MERGE_NODES - 1));
r->b = NULL;
- if (atomic_read(&b->c->search_inflight) &&
- gc->nodes >= gc->nodes_pre + btree_gc_min_nodes(b->c)) {
+ if (gc->nodes >= (gc->nodes_pre + btree_gc_min_nodes(b->c))) {
gc->nodes_pre = gc->nodes;
ret = -EAGAIN;
break;
@@ -1846,8 +1846,8 @@ static void bch_btree_gc(struct cache_set *c)
cond_resched();
if (ret == -EAGAIN)
- schedule_timeout_interruptible(msecs_to_jiffies
- (GC_SLEEP_MS));
+ schedule_timeout_interruptible(
+ msecs_to_jiffies(btree_gc_sleep_ms(c)));
else if (ret)
pr_warn("gc failed!\n");
} while (ret && !test_bit(CACHE_SET_IO_DISABLE, &c->flags));
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time
2025-11-13 5:36 ` [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time colyli
@ 2026-07-17 0:50 ` Robert Pang
2026-07-18 4:20 ` Coly Li
0 siblings, 1 reply; 16+ messages in thread
From: Robert Pang @ 2026-07-17 0:50 UTC (permalink / raw)
To: colyli; +Cc: linux-bcache, mingzhe.zou, robertpang
Hi Coly,
I was reviewing this patch again and noticed that btree_gc_min_nodes() reads the
atomic counter 'search_inflight'. This counter is incremented and decremented by
clients issuing front-side I/O. In this scenario, should we add a memory barrier
(such as `smp_mb__before_atomic`) prior to `atomic_read()`?
My concern is that if btree_gc_min_nodes() is inlined in the caller
btree_gc_recurse(), the compiler might hoist the `atomic_read()` outside of the
while loop where btree_gc_min_nodes() is inlined, preventing us from fetching the
latest counter value. Adding the barrier would guarantee we read the updated
value.
What are your thoughts on this?
Best regards,
Robert Pang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time
2026-07-17 0:50 ` Robert Pang
@ 2026-07-18 4:20 ` Coly Li
2026-07-21 18:55 ` Robert Pang
0 siblings, 1 reply; 16+ messages in thread
From: Coly Li @ 2026-07-18 4:20 UTC (permalink / raw)
To: Robert Pang; +Cc: linux-bcache, mingzhe.zou
> 2026年7月17日 08:50，Robert Pang <robertpang@google.com> 写道：
>
> Hi Coly,
>
> I was reviewing this patch again and noticed that btree_gc_min_nodes() reads the
> atomic counter 'search_inflight'. This counter is incremented and decremented by
> clients issuing front-side I/O. In this scenario, should we add a memory barrier
> (such as `smp_mb__before_atomic`) prior to `atomic_read()`?
>
> My concern is that if btree_gc_min_nodes() is inlined in the caller
> btree_gc_recurse(), the compiler might hoist the `atomic_read()` outside of the
> while loop where btree_gc_min_nodes() is inlined, preventing us from fetching the
> latest counter value. Adding the barrier would guarantee we read the updated
> value.
>
> What are your thoughts on this?
Hi Robert,
At the first glance I feel the code was fine. But, it was almost 8 months ago, to
make sure I understand you correctly, can you place your comments with the exact
code together, then let me response you more accurately.
Thanks for the review.
Coly Li
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time
2026-07-18 4:20 ` Coly Li
@ 2026-07-21 18:55 ` Robert Pang
2026-08-06 8:36 ` Coly Li
0 siblings, 1 reply; 16+ messages in thread
From: Robert Pang @ 2026-07-21 18:55 UTC (permalink / raw)
To: Coly Li; +Cc: linux-bcache, mingzhe.zou
On Fri, Jul 17, 2026 at 9:20 PM Coly Li <colyli@fygo.io> wrote:
>
> > 2026年7月17日 08:50，Robert Pang <robertpang@google.com> 写道：
> >
> > Hi Coly,
> >
> > I was reviewing this patch again and noticed that btree_gc_min_nodes() reads the
> > atomic counter 'search_inflight'. This counter is incremented and decremented by
> > clients issuing front-side I/O. In this scenario, should we add a memory barrier
> > (such as `smp_mb__before_atomic`) prior to `atomic_read()`?
> >
> > My concern is that if btree_gc_min_nodes() is inlined in the caller
> > btree_gc_recurse(), the compiler might hoist the `atomic_read()` outside of the
> > while loop where btree_gc_min_nodes() is inlined, preventing us from fetching the
> > latest counter value. Adding the barrier would guarantee we read the updated
> > value.
> >
> > What are your thoughts on this?
>
> Hi Robert,
>
> At the first glance I feel the code was fine. But, it was almost 8 months ago, to
> make sure I understand you correctly, can you place your comments with the exact
> code together, then let me response you more accurately.
>
> Thanks for the review.
>
> Coly Li
Hi Coly
My apology for missing the code context in the earlier email. Here are
the code snippets where my comments relate:
diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 210b59007d98..5d922d301ab6 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -1578,29 +1579,29 @@ static unsigned int btree_gc_count_keys(struct btree *b)
static size_t btree_gc_min_nodes(struct cache_set *c)
{
- size_t min_nodes;
+ size_t min_nodes = GC_NODES_MIN;
- /*
- * Since incremental GC would stop 100ms when front
- * side I/O comes, so when there are many btree nodes,
- * if GC only processes constant (100) nodes each time,
- * GC would last a long time, and the front side I/Os
- * would run out of the buckets (since no new bucket
- * can be allocated during GC), and be blocked again.
- * So GC should not process constant nodes, but varied
- * nodes according to the number of btree nodes, which
- * realized by dividing GC into constant(100) times,
- * so when there are many btree nodes, GC can process
- * more nodes each time, otherwise, GC will process less
- * nodes each time (but no less than MIN_GC_NODES)
- */
- min_nodes = c->gc_stats.nodes / MAX_GC_TIMES;
- if (min_nodes < MIN_GC_NODES)
- min_nodes = MIN_GC_NODES;
+ if (atomic_read(&c->search_inflight) == 0) {
+ size_t n = c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
+
+ if (min_nodes < n)
+ min_nodes = n;
+ }
return min_nodes;
}
In the above code change of this patch, btree_gc_min_nodes() reads the
atomic counter 'search_inflight'. And this counter is incremented and
decremented by clients issuing front-side I/O below:
drivers/md/bcache/request.c:
static CLOSURE_CALLBACK(search_free)
{
closure_type(s, struct search, cl);
atomic_dec(&s->iop.c->search_inflight);
if (s->iop.bio)
bio_put(s->iop.bio);
bio_complete(s);
closure_debug_destroy(cl);
mempool_free(s, &s->iop.c->search);
}
static inline struct search *search_alloc(struct bio *bio,
struct bcache_device *d, struct block_device *orig_bdev,
unsigned long start_time)
{
struct search *s;
s = mempool_alloc(&d->c->search, GFP_NOIO);
closure_init(&s->cl, NULL);
do_bio_hook(s, bio, request_endio);
atomic_inc(&d->c->search_inflight);
...
}
My concern is that if btree_gc_min_nodes() is inlined in the caller
btree_gc_recurse() below, the compiler might hoist the `atomic_read()`
outside of the while loop where btree_gc_min_nodes() is inlined,
preventing us from fetching the latest counter value after each btree
node is traversed.
drivers/md/bcache/btree.c:
static int btree_gc_recurse(struct btree *b, struct btree_op *op,
struct closure *writes, struct gc_stat *gc)
{
int ret = 0;
bool should_rewrite;
struct bkey *k;
struct btree_iter_stack iter;
struct gc_merge_info r[GC_MERGE_NODES];
struct gc_merge_info *i, *last = r + ARRAY_SIZE(r) - 1;
bch_btree_iter_stack_init(&b->keys, &iter, &b->c->gc_done);
for (i = r; i < r + ARRAY_SIZE(r); i++)
i->b = ERR_PTR(-EINTR);
while (1) {
k = bch_btree_iter_next_filter(&iter.iter, &b->keys,
bch_ptr_bad);
...
if (gc->nodes >= (gc->nodes_pre + btree_gc_min_nodes(b->c))) {
gc->nodes_pre = gc->nodes;
ret = -EAGAIN;
break;
}
..
}
In this scenario, should we add a memory barrier (such as
`smp_mb__before_atomic`) prior to `atomic_read()` to guarantee we read
the updated value? I.e.
diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 377052cbde5c..659b8706031a 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -1582,6 +1582,8 @@ static size_t btree_gc_min_nodes(struct cache_set *c)
{
size_t min_nodes = GC_NODES_MIN;
+ /* Fetch latest search_inflight count */
+ smp_mb__before_atomic();
if (atomic_read(&c->search_inflight) == 0) {
size_t n = c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
What are your thoughts on this?
Best regards
Robert Pang
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time
2026-07-21 18:55 ` Robert Pang
@ 2026-08-06 8:36 ` Coly Li
2026-08-10 21:38 ` Robert Pang
0 siblings, 1 reply; 16+ messages in thread
From: Coly Li @ 2026-08-06 8:36 UTC (permalink / raw)
To: Robert Pang; +Cc: linux-bcache, mingzhe.zou
On Tue, Jul 21, 2026 at 11:55:53AM +0800, Robert Pang wrote:
> On Fri, Jul 17, 2026 at 9:20 PM Coly Li <colyli@fygo.io> wrote:
> >
> > > 2026年7月17日 08:50，Robert Pang <robertpang@google.com> 写道：
> > >
> > > Hi Coly,
> > >
> > > I was reviewing this patch again and noticed that btree_gc_min_nodes() reads the
> > > atomic counter 'search_inflight'. This counter is incremented and decremented by
> > > clients issuing front-side I/O. In this scenario, should we add a memory barrier
> > > (such as `smp_mb__before_atomic`) prior to `atomic_read()`?
> > >
> > > My concern is that if btree_gc_min_nodes() is inlined in the caller
> > > btree_gc_recurse(), the compiler might hoist the `atomic_read()` outside of the
> > > while loop where btree_gc_min_nodes() is inlined, preventing us from fetching the
> > > latest counter value. Adding the barrier would guarantee we read the updated
> > > value.
> > >
> > > What are your thoughts on this?
> >
> > Hi Robert,
> >
> > At the first glance I feel the code was fine. But, it was almost 8 months ago, to
> > make sure I understand you correctly, can you place your comments with the exact
> > code together, then let me response you more accurately.
> >
> > Thanks for the review.
> >
> > Coly Li
>
> Hi Coly
>
> My apology for missing the code context in the earlier email. Here are
> the code snippets where my comments relate:
>
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 210b59007d98..5d922d301ab6 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
>
> @@ -1578,29 +1579,29 @@ static unsigned int btree_gc_count_keys(struct btree *b)
>
> static size_t btree_gc_min_nodes(struct cache_set *c)
> {
> - size_t min_nodes;
> + size_t min_nodes = GC_NODES_MIN;
>
> - /*
> - * Since incremental GC would stop 100ms when front
> - * side I/O comes, so when there are many btree nodes,
> - * if GC only processes constant (100) nodes each time,
> - * GC would last a long time, and the front side I/Os
> - * would run out of the buckets (since no new bucket
> - * can be allocated during GC), and be blocked again.
> - * So GC should not process constant nodes, but varied
> - * nodes according to the number of btree nodes, which
> - * realized by dividing GC into constant(100) times,
> - * so when there are many btree nodes, GC can process
> - * more nodes each time, otherwise, GC will process less
> - * nodes each time (but no less than MIN_GC_NODES)
> - */
> - min_nodes = c->gc_stats.nodes / MAX_GC_TIMES;
> - if (min_nodes < MIN_GC_NODES)
> - min_nodes = MIN_GC_NODES;
> + if (atomic_read(&c->search_inflight) == 0) {
> + size_t n = c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
> +
If atomic_read(&c->search_inflight) == 0, it means currently
no front end I/O. Then the gc nodes can be a bit more for a
more aggressive garbage collection.
Because the front end I/Os compete btree locks with gc threads,
I set a more aggresive garbage collection only when there is no
front I/O coming.
> + if (min_nodes < n)
> + min_nodes = n;
> + }
>
> return min_nodes;
> }
>
> In the above code change of this patch, btree_gc_min_nodes() reads the
> atomic counter 'search_inflight'. And this counter is incremented and
> decremented by clients issuing front-side I/O below:
>
> drivers/md/bcache/request.c:
>
> static CLOSURE_CALLBACK(search_free)
> {
> closure_type(s, struct search, cl);
>
> atomic_dec(&s->iop.c->search_inflight);
>
> if (s->iop.bio)
> bio_put(s->iop.bio);
>
> bio_complete(s);
> closure_debug_destroy(cl);
> mempool_free(s, &s->iop.c->search);
> }
>
> static inline struct search *search_alloc(struct bio *bio,
> struct bcache_device *d, struct block_device *orig_bdev,
> unsigned long start_time)
> {
> struct search *s;
>
> s = mempool_alloc(&d->c->search, GFP_NOIO);
>
> closure_init(&s->cl, NULL);
> do_bio_hook(s, bio, request_endio);
> atomic_inc(&d->c->search_inflight);
> ...
> }
>
> My concern is that if btree_gc_min_nodes() is inlined in the caller
> btree_gc_recurse() below, the compiler might hoist the `atomic_read()`
> outside of the while loop where btree_gc_min_nodes() is inlined,
> preventing us from fetching the latest counter value after each btree
> node is traversed.
>
It is possible, but I do this on purpose. Because accurately catching
zero inflight counter is unncessary. Let me explain in next text block.
> drivers/md/bcache/btree.c:
>
> static int btree_gc_recurse(struct btree *b, struct btree_op *op,
> struct closure *writes, struct gc_stat *gc)
> {
> int ret = 0;
> bool should_rewrite;
> struct bkey *k;
> struct btree_iter_stack iter;
> struct gc_merge_info r[GC_MERGE_NODES];
> struct gc_merge_info *i, *last = r + ARRAY_SIZE(r) - 1;
>
> bch_btree_iter_stack_init(&b->keys, &iter, &b->c->gc_done);
>
> for (i = r; i < r + ARRAY_SIZE(r); i++)
> i->b = ERR_PTR(-EINTR);
>
> while (1) {
> k = bch_btree_iter_next_filter(&iter.iter, &b->keys,
> bch_ptr_bad);
> ...
> if (gc->nodes >= (gc->nodes_pre + btree_gc_min_nodes(b->c))) {
> gc->nodes_pre = gc->nodes;
> ret = -EAGAIN;
> break;
> }
> ..
> }
>
> In this scenario, should we add a memory barrier (such as
> `smp_mb__before_atomic`) prior to `atomic_read()` to guarantee we read
> the updated value? I.e.
>
Such memory barrier hurts performance, and for hot I/O path, it might
introduce obviouos negative performance impect. Fortunately for the
condition you are concerned, it works well without memory barrier.
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 377052cbde5c..659b8706031a 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -1582,6 +1582,8 @@ static size_t btree_gc_min_nodes(struct cache_set *c)
> {
> size_t min_nodes = GC_NODES_MIN;
>
> + /* Fetch latest search_inflight count */
> + smp_mb__before_atomic();
> if (atomic_read(&c->search_inflight) == 0) {
> size_t n = c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
>
> What are your thoughts on this?
The motivation is, when the bcache device is idle (no front end I/Os),
make garbage collection be more aggressive by scanning more btree nodes
in each iteration. This is a try-best effort, the inflight counter is
unncessary to be accurate 0. If the check in btree_gc_min_nodes(),
atomic_read(&c->search_inflight) == 0
fails, then just fails. And if the bcache device is really idle for a
while, finally the above check will be true and a larger min_nodes will
be returned from btree_gc_min_nodes().
But if adding a single memory barrier only in btree_gc_min_nodes(), indeed
it does't help too much. Memory barrier is also necessary for locations
where the inflight counter are changed. Then this is why I name it as
negative performance impact in previous text block.
Maybe I need to add code comments to explain why memory barrier is
unncessary in current btree_gc_min_nodes(). Does it help?
Thanks for the quesiton.
Coly Li
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time
2026-08-06 8:36 ` Coly Li
@ 2026-08-10 21:38 ` Robert Pang
0 siblings, 0 replies; 16+ messages in thread
From: Robert Pang @ 2026-08-10 21:38 UTC (permalink / raw)
To: Coly Li; +Cc: linux-bcache, mingzhe.zou
Hi Coly
Thank you so much for taking the time to look into this and share your
insights and ease my concerns. Your explanation in this thread will
provide sufficient context for future reference, so I think we can
save the additional code comments.
Best regards
Robert Pang
On Thu, Aug 6, 2026 at 1:36 AM Coly Li <colyli@fygo.io> wrote:
>
> On Tue, Jul 21, 2026 at 11:55:53AM +0800, Robert Pang wrote:
> > On Fri, Jul 17, 2026 at 9:20 PM Coly Li <colyli@fygo.io> wrote:
> > >
> > > > 2026年7月17日 08:50，Robert Pang <robertpang@google.com> 写道：
> > > >
> > > > Hi Coly,
> > > >
> > > > I was reviewing this patch again and noticed that btree_gc_min_nodes() reads the
> > > > atomic counter 'search_inflight'. This counter is incremented and decremented by
> > > > clients issuing front-side I/O. In this scenario, should we add a memory barrier
> > > > (such as `smp_mb__before_atomic`) prior to `atomic_read()`?
> > > >
> > > > My concern is that if btree_gc_min_nodes() is inlined in the caller
> > > > btree_gc_recurse(), the compiler might hoist the `atomic_read()` outside of the
> > > > while loop where btree_gc_min_nodes() is inlined, preventing us from fetching the
> > > > latest counter value. Adding the barrier would guarantee we read the updated
> > > > value.
> > > >
> > > > What are your thoughts on this?
> > >
> > > Hi Robert,
> > >
> > > At the first glance I feel the code was fine. But, it was almost 8 months ago, to
> > > make sure I understand you correctly, can you place your comments with the exact
> > > code together, then let me response you more accurately.
> > >
> > > Thanks for the review.
> > >
> > > Coly Li
> >
> > Hi Coly
> >
> > My apology for missing the code context in the earlier email. Here are
> > the code snippets where my comments relate:
> >
> > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> > index 210b59007d98..5d922d301ab6 100644
> > --- a/drivers/md/bcache/btree.c
> > +++ b/drivers/md/bcache/btree.c
> >
> > @@ -1578,29 +1579,29 @@ static unsigned int btree_gc_count_keys(struct btree *b)
> >
> > static size_t btree_gc_min_nodes(struct cache_set *c)
> > {
> > - size_t min_nodes;
> > + size_t min_nodes = GC_NODES_MIN;
> >
> > - /*
> > - * Since incremental GC would stop 100ms when front
> > - * side I/O comes, so when there are many btree nodes,
> > - * if GC only processes constant (100) nodes each time,
> > - * GC would last a long time, and the front side I/Os
> > - * would run out of the buckets (since no new bucket
> > - * can be allocated during GC), and be blocked again.
> > - * So GC should not process constant nodes, but varied
> > - * nodes according to the number of btree nodes, which
> > - * realized by dividing GC into constant(100) times,
> > - * so when there are many btree nodes, GC can process
> > - * more nodes each time, otherwise, GC will process less
> > - * nodes each time (but no less than MIN_GC_NODES)
> > - */
> > - min_nodes = c->gc_stats.nodes / MAX_GC_TIMES;
> > - if (min_nodes < MIN_GC_NODES)
> > - min_nodes = MIN_GC_NODES;
> > + if (atomic_read(&c->search_inflight) == 0) {
> > + size_t n = c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
> > +
>
> If atomic_read(&c->search_inflight) == 0, it means currently
> no front end I/O. Then the gc nodes can be a bit more for a
> more aggressive garbage collection.
>
> Because the front end I/Os compete btree locks with gc threads,
> I set a more aggresive garbage collection only when there is no
> front I/O coming.
>
>
> > + if (min_nodes < n)
> > + min_nodes = n;
> > + }
> >
> > return min_nodes;
> > }
> >
> > In the above code change of this patch, btree_gc_min_nodes() reads the
> > atomic counter 'search_inflight'. And this counter is incremented and
> > decremented by clients issuing front-side I/O below:
> >
> > drivers/md/bcache/request.c:
> >
> > static CLOSURE_CALLBACK(search_free)
> > {
> > closure_type(s, struct search, cl);
> >
> > atomic_dec(&s->iop.c->search_inflight);
> >
> > if (s->iop.bio)
> > bio_put(s->iop.bio);
> >
> > bio_complete(s);
> > closure_debug_destroy(cl);
> > mempool_free(s, &s->iop.c->search);
> > }
> >
> > static inline struct search *search_alloc(struct bio *bio,
> > struct bcache_device *d, struct block_device *orig_bdev,
> > unsigned long start_time)
> > {
> > struct search *s;
> >
> > s = mempool_alloc(&d->c->search, GFP_NOIO);
> >
> > closure_init(&s->cl, NULL);
> > do_bio_hook(s, bio, request_endio);
> > atomic_inc(&d->c->search_inflight);
> > ...
> > }
> >
> > My concern is that if btree_gc_min_nodes() is inlined in the caller
> > btree_gc_recurse() below, the compiler might hoist the `atomic_read()`
> > outside of the while loop where btree_gc_min_nodes() is inlined,
> > preventing us from fetching the latest counter value after each btree
> > node is traversed.
> >
>
> It is possible, but I do this on purpose. Because accurately catching
> zero inflight counter is unncessary. Let me explain in next text block.
>
> > drivers/md/bcache/btree.c:
> >
> > static int btree_gc_recurse(struct btree *b, struct btree_op *op,
> > struct closure *writes, struct gc_stat *gc)
> > {
> > int ret = 0;
> > bool should_rewrite;
> > struct bkey *k;
> > struct btree_iter_stack iter;
> > struct gc_merge_info r[GC_MERGE_NODES];
> > struct gc_merge_info *i, *last = r + ARRAY_SIZE(r) - 1;
> >
> > bch_btree_iter_stack_init(&b->keys, &iter, &b->c->gc_done);
> >
> > for (i = r; i < r + ARRAY_SIZE(r); i++)
> > i->b = ERR_PTR(-EINTR);
> >
> > while (1) {
> > k = bch_btree_iter_next_filter(&iter.iter, &b->keys,
> > bch_ptr_bad);
> > ...
> > if (gc->nodes >= (gc->nodes_pre + btree_gc_min_nodes(b->c))) {
> > gc->nodes_pre = gc->nodes;
> > ret = -EAGAIN;
> > break;
> > }
> > ..
> > }
> >
> > In this scenario, should we add a memory barrier (such as
> > `smp_mb__before_atomic`) prior to `atomic_read()` to guarantee we read
> > the updated value? I.e.
> >
>
> Such memory barrier hurts performance, and for hot I/O path, it might
> introduce obviouos negative performance impect. Fortunately for the
> condition you are concerned, it works well without memory barrier.
>
>
> > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> > index 377052cbde5c..659b8706031a 100644
> > --- a/drivers/md/bcache/btree.c
> > +++ b/drivers/md/bcache/btree.c
> > @@ -1582,6 +1582,8 @@ static size_t btree_gc_min_nodes(struct cache_set *c)
> > {
> > size_t min_nodes = GC_NODES_MIN;
> >
> > + /* Fetch latest search_inflight count */
> > + smp_mb__before_atomic();
> > if (atomic_read(&c->search_inflight) == 0) {
> > size_t n = c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
> >
> > What are your thoughts on this?
>
> The motivation is, when the bcache device is idle (no front end I/Os),
> make garbage collection be more aggressive by scanning more btree nodes
> in each iteration. This is a try-best effort, the inflight counter is
> unncessary to be accurate 0. If the check in btree_gc_min_nodes(),
> atomic_read(&c->search_inflight) == 0
> fails, then just fails. And if the bcache device is really idle for a
> while, finally the above check will be true and a larger min_nodes will
> be returned from btree_gc_min_nodes().
>
> But if adding a single memory barrier only in btree_gc_min_nodes(), indeed
> it does't help too much. Memory barrier is also necessary for locations
> where the inflight counter are changed. Then this is why I name it as
> negative performance impact in previous text block.
>
> Maybe I need to add code comments to explain why memory barrier is
> unncessary in current btree_gc_min_nodes(). Does it help?
>
> Thanks for the quesiton.
>
> Coly Li
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 6/9] bcache: remove redundant __GFP_NOWARN
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
` (4 preceding siblings ...)
2025-11-13 5:36 ` [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time colyli
@ 2025-11-13 5:36 ` colyli
2025-11-13 5:36 ` [PATCH 7/9] bcache: replace use of system_wq with system_percpu_wq colyli
` (3 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: colyli @ 2025-11-13 5:36 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block, Qianfeng Rong, Coly Li
From: Qianfeng Rong <rongqianfeng@vivo.com>
GFP_NOWAIT already includes __GFP_NOWARN, so let's remove the redundant
__GFP_NOWARN.
Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
Acked-by: Coly Li <colyli@fnnas.com>
---
drivers/md/bcache/btree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 5d922d301ab6..24ddc353cb30 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -372,7 +372,7 @@ static void do_btree_node_write(struct btree *b)
SET_PTR_OFFSET(&k.key, 0, PTR_OFFSET(&k.key, 0) +
bset_sector_offset(&b->keys, i));
- if (!bch_bio_alloc_pages(b->bio, __GFP_NOWARN|GFP_NOWAIT)) {
+ if (!bch_bio_alloc_pages(b->bio, GFP_NOWAIT)) {
struct bio_vec *bv;
void *addr = (void *) ((unsigned long) i & ~(PAGE_SIZE - 1));
struct bvec_iter_all iter_all;
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 7/9] bcache: replace use of system_wq with system_percpu_wq
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
` (5 preceding siblings ...)
2025-11-13 5:36 ` [PATCH 6/9] bcache: remove redundant __GFP_NOWARN colyli
@ 2025-11-13 5:36 ` colyli
2025-11-13 5:36 ` [PATCH 8/9] bcache: WQ_PERCPU added to alloc_workqueue users colyli
` (2 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: colyli @ 2025-11-13 5:36 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block, Marco Crivellari, Tejun Heo, Coly Li
From: Marco Crivellari <marco.crivellari@suse.com>
Currently if a user enqueues a work item using schedule_delayed_work() the
used wq is "system_wq" (per-cpu wq) while queue_delayed_work() use
WORK_CPU_UNBOUND (used when a cpu is not specified). The same applies to
schedule_work() that is using system_wq and queue_work(), that makes use
again of WORK_CPU_UNBOUND.
This lack of consistency cannot be addressed without refactoring the API.
This patch continues the effort to refactor worqueue APIs, which has begun
with the change introducing new workqueues and a new alloc_workqueue flag:
commit 128ea9f6ccfb ("workqueue: Add system_percpu_wq and system_dfl_wq")
commit 930c2ea566af ("workqueue: Add new WQ_PERCPU flag")
system_wq should be the per-cpu workqueue, yet in this name nothing makes
that clear, so replace system_wq with system_percpu_wq.
The old wq (system_wq) will be kept for a few release cycles.
Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
Signed-off-by: Coly Li <colyli@fnnas.com>
---
drivers/md/bcache/super.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 91a98ebc3f80..92ced6b28cb2 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1388,7 +1388,7 @@ static CLOSURE_CALLBACK(cached_dev_flush)
bch_cache_accounting_destroy(&dc->accounting);
kobject_del(&d->kobj);
- continue_at(cl, cached_dev_free, system_wq);
+ continue_at(cl, cached_dev_free, system_percpu_wq);
}
static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
@@ -1400,7 +1400,7 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
__module_get(THIS_MODULE);
INIT_LIST_HEAD(&dc->list);
closure_init(&dc->disk.cl, NULL);
- set_closure_fn(&dc->disk.cl, cached_dev_flush, system_wq);
+ set_closure_fn(&dc->disk.cl, cached_dev_flush, system_percpu_wq);
kobject_init(&dc->disk.kobj, &bch_cached_dev_ktype);
INIT_WORK(&dc->detach, cached_dev_detach_finish);
sema_init(&dc->sb_write_mutex, 1);
@@ -1513,7 +1513,7 @@ static CLOSURE_CALLBACK(flash_dev_flush)
bcache_device_unlink(d);
mutex_unlock(&bch_register_lock);
kobject_del(&d->kobj);
- continue_at(cl, flash_dev_free, system_wq);
+ continue_at(cl, flash_dev_free, system_percpu_wq);
}
static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
@@ -1525,7 +1525,7 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
goto err_ret;
closure_init(&d->cl, NULL);
- set_closure_fn(&d->cl, flash_dev_flush, system_wq);
+ set_closure_fn(&d->cl, flash_dev_flush, system_percpu_wq);
kobject_init(&d->kobj, &bch_flash_dev_ktype);
@@ -1833,7 +1833,7 @@ static CLOSURE_CALLBACK(__cache_set_unregister)
mutex_unlock(&bch_register_lock);
- continue_at(cl, cache_set_flush, system_wq);
+ continue_at(cl, cache_set_flush, system_percpu_wq);
}
void bch_cache_set_stop(struct cache_set *c)
@@ -1863,10 +1863,10 @@ struct cache_set *bch_cache_set_alloc(struct cache_sb *sb)
__module_get(THIS_MODULE);
closure_init(&c->cl, NULL);
- set_closure_fn(&c->cl, cache_set_free, system_wq);
+ set_closure_fn(&c->cl, cache_set_free, system_percpu_wq);
closure_init(&c->caching, &c->cl);
- set_closure_fn(&c->caching, __cache_set_unregister, system_wq);
+ set_closure_fn(&c->caching, __cache_set_unregister, system_percpu_wq);
/* Maybe create continue_at_noreturn() and use it here? */
closure_set_stopped(&c->cl);
@@ -2528,7 +2528,7 @@ static void register_device_async(struct async_reg_args *args)
INIT_DELAYED_WORK(&args->reg_work, register_cache_worker);
/* 10 jiffies is enough for a delay */
- queue_delayed_work(system_wq, &args->reg_work, 10);
+ queue_delayed_work(system_percpu_wq, &args->reg_work, 10);
}
static void *alloc_holder_object(struct cache_sb *sb)
@@ -2909,11 +2909,11 @@ static int __init bcache_init(void)
/*
* Let's not make this `WQ_MEM_RECLAIM` for the following reasons:
*
- * 1. It used `system_wq` before which also does no memory reclaim.
+ * 1. It used `system_percpu_wq` before which also does no memory reclaim.
* 2. With `WQ_MEM_RECLAIM` desktop stalls, increased boot times, and
* reduced throughput can be observed.
*
- * We still want to user our own queue to not congest the `system_wq`.
+ * We still want to user our own queue to not congest the `system_percpu_wq`.
*/
bch_flush_wq = alloc_workqueue("bch_flush", 0, 0);
if (!bch_flush_wq)
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 8/9] bcache: WQ_PERCPU added to alloc_workqueue users
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
` (6 preceding siblings ...)
2025-11-13 5:36 ` [PATCH 7/9] bcache: replace use of system_wq with system_percpu_wq colyli
@ 2025-11-13 5:36 ` colyli
2025-11-13 5:36 ` [PATCH 9/9] bcache: Avoid -Wflex-array-member-not-at-end warning colyli
2025-11-13 16:26 ` [PATCH 0/9] bcache patches for Linux 6.19 Jens Axboe
9 siblings, 0 replies; 16+ messages in thread
From: colyli @ 2025-11-13 5:36 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block, Marco Crivellari, Tejun Heo, Coly Li
From: Marco Crivellari <marco.crivellari@suse.com>
Currently if a user enqueue a work item using schedule_delayed_work() the
used wq is "system_wq" (per-cpu wq) while queue_delayed_work() use
WORK_CPU_UNBOUND (used when a cpu is not specified). The same applies to
schedule_work() that is using system_wq and queue_work(), that makes use
again of WORK_CPU_UNBOUND.
This lack of consistentcy cannot be addressed without refactoring the API.
alloc_workqueue() treats all queues as per-CPU by default, while unbound
workqueues must opt-in via WQ_UNBOUND.
This default is suboptimal: most workloads benefit from unbound queues,
allowing the scheduler to place worker threads where they’re needed and
reducing noise when CPUs are isolated.
This patch continues the effort to refactor worqueue APIs, which has begun
with the change introducing new workqueues and a new alloc_workqueue flag:
commit 128ea9f6ccfb ("workqueue: Add system_percpu_wq and system_dfl_wq")
commit 930c2ea566af ("workqueue: Add new WQ_PERCPU flag")
This change adds a new WQ_PERCPU flag to explicitly request
alloc_workqueue() to be per-cpu when WQ_UNBOUND has not been specified.
With the introduction of the WQ_PERCPU flag (equivalent to !WQ_UNBOUND),
any alloc_workqueue() caller that doesn’t explicitly specify WQ_UNBOUND
must now use WQ_PERCPU.
Once migration is complete, WQ_UNBOUND can be removed and unbound will
become the implicit default.
Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
Signed-off-by: Coly Li <colyli@fnnas.com>
---
drivers/md/bcache/btree.c | 3 ++-
drivers/md/bcache/super.c | 10 ++++++----
drivers/md/bcache/writeback.c | 2 +-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 24ddc353cb30..3ed39c823826 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -2822,7 +2822,8 @@ void bch_btree_exit(void)
int __init bch_btree_init(void)
{
- btree_io_wq = alloc_workqueue("bch_btree_io", WQ_MEM_RECLAIM, 0);
+ btree_io_wq = alloc_workqueue("bch_btree_io",
+ WQ_MEM_RECLAIM | WQ_PERCPU, 0);
if (!btree_io_wq)
return -ENOMEM;
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 92ced6b28cb2..c17d4517af22 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1939,7 +1939,8 @@ struct cache_set *bch_cache_set_alloc(struct cache_sb *sb)
if (!c->uuids)
goto err;
- c->moving_gc_wq = alloc_workqueue("bcache_gc", WQ_MEM_RECLAIM, 0);
+ c->moving_gc_wq = alloc_workqueue("bcache_gc",
+ WQ_MEM_RECLAIM | WQ_PERCPU, 0);
if (!c->moving_gc_wq)
goto err;
@@ -2902,7 +2903,7 @@ static int __init bcache_init(void)
if (bch_btree_init())
goto err;
- bcache_wq = alloc_workqueue("bcache", WQ_MEM_RECLAIM, 0);
+ bcache_wq = alloc_workqueue("bcache", WQ_MEM_RECLAIM | WQ_PERCPU, 0);
if (!bcache_wq)
goto err;
@@ -2915,11 +2916,12 @@ static int __init bcache_init(void)
*
* We still want to user our own queue to not congest the `system_percpu_wq`.
*/
- bch_flush_wq = alloc_workqueue("bch_flush", 0, 0);
+ bch_flush_wq = alloc_workqueue("bch_flush", WQ_PERCPU, 0);
if (!bch_flush_wq)
goto err;
- bch_journal_wq = alloc_workqueue("bch_journal", WQ_MEM_RECLAIM, 0);
+ bch_journal_wq = alloc_workqueue("bch_journal",
+ WQ_MEM_RECLAIM | WQ_PERCPU, 0);
if (!bch_journal_wq)
goto err;
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index cffef33b4acf..4b237074f453 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -1075,7 +1075,7 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc)
int bch_cached_dev_writeback_start(struct cached_dev *dc)
{
dc->writeback_write_wq = alloc_workqueue("bcache_writeback_wq",
- WQ_MEM_RECLAIM, 0);
+ WQ_MEM_RECLAIM | WQ_PERCPU, 0);
if (!dc->writeback_write_wq)
return -ENOMEM;
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 9/9] bcache: Avoid -Wflex-array-member-not-at-end warning
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
` (7 preceding siblings ...)
2025-11-13 5:36 ` [PATCH 8/9] bcache: WQ_PERCPU added to alloc_workqueue users colyli
@ 2025-11-13 5:36 ` colyli
2025-11-13 16:26 ` [PATCH 0/9] bcache patches for Linux 6.19 Jens Axboe
9 siblings, 0 replies; 16+ messages in thread
From: colyli @ 2025-11-13 5:36 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block, Gustavo A. R. Silva, Coly Li
From: "Gustavo A. R. Silva" <gustavoars@kernel.org>
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are
getting ready to enable it, globally.
Use the new TRAILING_OVERLAP() helper to fix the following warning:
drivers/md/bcache/bset.h:330:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
This helper creates a union between a flexible-array member (FAM) and a
set of MEMBERS that would otherwise follow it.
This overlays the trailing MEMBER struct btree_iter_set stack_data[MAX_BSETS];
onto the FAM struct btree_iter::data[], while keeping the FAM and the start
of MEMBER aligned.
The static_assert() ensures this alignment remains, and it's
intentionally placed immediately after the corresponding structures --no
blank line in between.
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Coly Li <colyli@fnnas.com>
---
drivers/md/bcache/bset.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/md/bcache/bset.h b/drivers/md/bcache/bset.h
index 011f6062c4c0..6ee2c6a506a2 100644
--- a/drivers/md/bcache/bset.h
+++ b/drivers/md/bcache/bset.h
@@ -327,9 +327,13 @@ struct btree_iter {
/* Fixed-size btree_iter that can be allocated on the stack */
struct btree_iter_stack {
- struct btree_iter iter;
- struct btree_iter_set stack_data[MAX_BSETS];
+ /* Must be last as it ends in a flexible-array member. */
+ TRAILING_OVERLAP(struct btree_iter, iter, data,
+ struct btree_iter_set stack_data[MAX_BSETS];
+ );
};
+static_assert(offsetof(struct btree_iter_stack, iter.data) ==
+ offsetof(struct btree_iter_stack, stack_data));
typedef bool (*ptr_filter_fn)(struct btree_keys *b, const struct bkey *k);
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 0/9] bcache patches for Linux 6.19
2025-11-13 5:36 [PATCH 0/9] bcache patches for Linux 6.19 colyli
` (8 preceding siblings ...)
2025-11-13 5:36 ` [PATCH 9/9] bcache: Avoid -Wflex-array-member-not-at-end warning colyli
@ 2025-11-13 16:26 ` Jens Axboe
9 siblings, 0 replies; 16+ messages in thread
From: Jens Axboe @ 2025-11-13 16:26 UTC (permalink / raw)
To: colyli; +Cc: linux-bcache, linux-block
On Thu, 13 Nov 2025 13:36:21 +0800, colyli@fnnas.com wrote:
> This is the first wave bcache patches for Linux 6.19.
>
> The major change is from me, which is to remove useless discard
> interface and code for cache device (not the backing device). And the
> last patch about gc latency is a cooperative result from Robert Pang
> (Google), Mingzhe Zou (Easystack) and me, by inspired from their
> previous works, I compose the final version and Robert prvides positive
> benchmark result.
>
> [...]
Applied, thanks!
[1/9] bcache: get rid of discard code from journal
commit: 0c72e9fcc156caaf123a6291321bc9bd74cd1b61
[2/9] bcache: remove discard code from alloc.c
commit: b4056afbd4b90f5bdbdc53cca2768f9b8872a2dd
[3/9] bcache: drop discard sysfs interface
commit: 73a004f83cf024e785b74243ba9817a329423379
[4/9] bcache: remove discard sysfs interface document
commit: 7bf90cd740bf87dd1692cf74d49bb1dc849dcd11
[5/9] bcache: reduce gc latency by processing less nodes and sleep less time
commit: 70bc173ce06be90b026bb00ea175567c91f006e4
[6/9] bcache: remove redundant __GFP_NOWARN
commit: 21194c44b6bdf50a27a0e065683d94bae16f69cb
[7/9] bcache: replace use of system_wq with system_percpu_wq
commit: fd82071814d06c7b760fe8d90b932d8a66cffc63
[8/9] bcache: WQ_PERCPU added to alloc_workqueue users
commit: c0c808214249c32a8961999e0779b953095b0074
[9/9] bcache: Avoid -Wflex-array-member-not-at-end warning
commit: 699122b590ebbc450737eebde3ab8f5b871cc7f0
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread