[PATCH v3] bcache: avoid holding bch_register_lock across cache_set recovery

[PATCH v3] bcache: avoid holding bch_register_lock across cache_set recovery

[Qiliang Yuan]

Large-scale bcache device registration and concurrent sysfs access can
hang unrelated tasks for a long time, e.g.:

[  243.082130] INFO: task bcache_cache_se:3496 blocked for more than 121 seconds.
[  243.130817] Call trace:
[  243.134161]  __switch_to+0x7c/0xbc
[  243.138461]  __schedule+0x338/0x6f0
[  243.142847]  schedule+0x50/0xe0
[  243.146884]  schedule_preempt_disabled+0x18/0x24
[  243.152400]  __mutex_lock.constprop.0+0x1d4/0x5ec
[  243.158002]  __mutex_lock_slowpath+0x1c/0x30
[  243.163170]  mutex_lock+0x50/0x60
[  243.167397]  bch_cache_set_store+0x40/0x80 [bcache]

register_cache() holds bch_register_lock for the entire duration of
register_cache_set(), which calls run_cache_set() to do journal replay,
btree check and allocator start for a newly registered cache device.
Every sysfs show/store on any cache_set or cached_dev in the system
takes the same global bch_register_lock, so an unrelated device's
sysfs access blocks for as long as the new device's recovery takes.

Converting bch_register_lock to a rw_semaphore does not fix this:
register_cache() still holds the lock (as a writer) for the whole
register_cache_set() call, and sysfs store paths still need exclusive
access, so a store on an unrelated device still blocks for the same
duration. Downgrading the lock to a reader during recovery was tried,
but the cache_set's kobjects and its bch_cache_sets list entry are
already published before recovery starts, so a concurrent unregister
could hit the cache_set while it is only partially initialized.

Fix this at the root instead of changing the lock type: keep
bch_register_lock a mutex, and only hold it for the actual bookkeeping
that needs it, not for the multi-second recovery. Split registering a
brand new cache_set into two phases. Recovery (journal replay, btree
check, allocator start) runs on a cache_set that is linked into
bch_cache_sets and marked CACHE_SET_REGISTERING, but not yet added to
sysfs, without holding bch_register_lock at all: nothing else can
reach it through a kobject that does not exist yet, and the
REGISTERING flag keeps bch_cached_dev_attach() from attaching a
backing device to it before it is ready. Publishing (adding the
kobjects, attaching pending backing/flash devices) runs under
bch_register_lock afterwards, and is no more expensive than what
register_bdev_worker() already does under the same lock today.

Registering the new cache_set into bch_cache_sets and marking it
CACHE_SET_REGISTERING happens in the same critical section as
bch_cache_set_alloc(), which already sets c->cache, so the existing
duplicate-uuid check keeps working unchanged: a second registration
for the same uuid still sees c->cache set and is rejected, instead of
racing to recover the same uuid twice. __uuid_write()'s
lockdep_assert_held() is relaxed to also accept a cache_set that is
still CACHE_SET_REGISTERING, since it is called from the unlocked
recovery path on the fresh-cache branch. The per-cache sysfs show/store
handlers refuse to touch cache_set state while CACHE_SET_REGISTERING
is set, since that state is still being written by recovery.

Attaching an additional cache device to an already-published cache_set
(the pre-existing "found" case in register_cache_set(), when a cache
device is re-attached to a cache_set that lost its device earlier) is
left untouched: that cache_set is already reachable through sysfs, so
it keeps running recovery and publishing as one operation under
bch_register_lock, exactly as before.

Co-developed-by: Qiliang Yuan <yuanql9@chinatelecom.cn>
Signed-off-by: Qiliang Yuan <yuanql9@chinatelecom.cn>
Signed-off-by: Jing Wu <realwujing@gmail.com>
---
register_cache() holds the global bch_register_lock for the entire
duration of registering a new cache_set, including journal replay,
btree check and allocator start. Every sysfs show/store on any other
cache_set or cached_dev in the system needs the same lock, so a single
slow cache_set registration blocks unrelated sysfs access for as long
as recovery takes (see the call trace in the commit message).

v1/v2 tried to fix this by converting bch_register_lock from a mutex
to a rw_semaphore so sysfs reads could proceed concurrently. Coly
pointed out this does not actually help the reported hang (store paths
still need exclusive access for the same duration), raised a KABI
concern about changing the global's type, and found a real regression
in v1's downgrade_write()/up_read()/down_write() dance around
bch_btree_check().

v3 drops the lock-type change entirely and fixes the actual problem:
bch_register_lock stays a mutex, and registering a brand new cache_set
is split so the expensive recovery runs without holding it at all, on
a cache_set that is provisionally linked into bch_cache_sets (for
duplicate-uuid detection) but not yet visible in sysfs or attachable.
Publishing (kobjects, attaching pending devices) still runs under the
mutex afterwards, at the same cost register_bdev_worker() already pays
under the same lock today. Full design rationale is in the commit
message.
---
V2 -> V3:
- Drop the bch_register_lock mutex -> rw_semaphore conversion entirely
  (addresses Coly's KABI concern by not touching the lock's type at
  all).
- Root-cause fix instead: split registering a brand new cache_set into
  an unlocked recovery phase (journal replay, btree check, allocator
  start) and a locked publish phase (kobject_add, attach pending
  devices), so bch_register_lock is only held for the cheap
  bookkeeping, not the multi-second recovery that caused the reported
  hang.
- Add CACHE_SET_REGISTERING flag: the cache_set is linked into
  bch_cache_sets immediately (so duplicate-uuid detection and
  bcache_reboot() still see it) but kept out of sysfs and out of
  bch_cached_dev_attach()'s reach until recovery finishes.
- Relax __uuid_write()'s lockdep_assert_held() to also accept a
  CACHE_SET_REGISTERING cache_set, since it is called from the
  unlocked recovery path.
- Gate the per-cache sysfs show/store handlers so they refuse to touch
  cache_set state while CACHE_SET_REGISTERING is set.
- Leave the "found:" case (attaching an additional cache device to an
  already-published cache_set) fully serialized under
  bch_register_lock, unchanged from v1/v2.

V1 -> V2:
- Remove downgrade_write()/up_read()/down_write() lock manipulation in
  run_cache_set() to eliminate race window during partial initialization
  that could lead to use-after-free if unregister is triggered between
  up_read() and down_write(). Keep write lock held throughout instead.
  (Coly)
- Simplify all sysfs store wrappers to unconditionally use down_write()
  instead of per-attribute read/write lock selection, which was fragile
  and could miss newly added attributes.
- Fix STORE_LOCKED macro to use down_write()/up_write() for store
  operations (was incorrectly using down_read()/up_read()).
- Expand bch_flash_dev store from STORE_LOCKED macro to explicit
  wrapper with down_write() and bcache_is_reboot check.
- Fix whitespace indentation in bcache_init() error path.
- Note on KABI: bch_register_lock is a module-internal global variable
  with no EXPORT_SYMBOL in the entire bcache driver. Changing its type
  from struct mutex to struct rw_semaphore does not affect the kernel
  ABI, as the symbol is never exported to other modules.

v2: https://lore.kernel.org/r/20260706-feat-bcache-v2-1-70a4b6e246c0@gmail.com
v1: https://lore.kernel.org/r/20260318-wujing-bcache-v1-1-f0b9aaf3f81d@gmail.com
---
 drivers/md/bcache/bcache.h |   9 +++
 drivers/md/bcache/super.c  | 145 +++++++++++++++++++++++++++++++++++++++------
 drivers/md/bcache/sysfs.c  |  16 +++++
 3 files changed, 153 insertions(+), 17 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index ec9ff97150812..6763ac1c886ed 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -507,11 +507,20 @@ struct gc_stat {
  * CACHE_SET_IO_DISABLE is set when bcache is stopping the whold cache set, all
  * external and internal I/O should be denied when this flag is set.
  *
+ * CACHE_SET_REGISTERING is set while a freshly allocated cache set is running
+ * its recovery path (journal replay, btree check, allocator start) without
+ * holding bch_register_lock. The cache set is already linked into
+ * bch_cache_sets at this point (so duplicate-uuid detection and reboot
+ * bookkeeping can see it), but its kobjects are not added to sysfs yet, so
+ * it is not reachable from userspace or from cache-set lookups that attach
+ * backing devices. Cleared once the cache set is fully published.
+ *
  */
 #define CACHE_SET_UNREGISTERING		0
 #define	CACHE_SET_STOPPING		1
 #define	CACHE_SET_RUNNING		2
 #define CACHE_SET_IO_DISABLE		3
+#define CACHE_SET_REGISTERING		4
 
 struct cache_set {
 	struct closure		cl;
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 97d9adb0bf96b..0146961a061f1 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -503,7 +503,14 @@ static int __uuid_write(struct cache_set *c)
 	unsigned int size;
 
 	closure_init_stack(&cl);
-	lockdep_assert_held(&bch_register_lock);
+	/*
+	 * Called either with bch_register_lock held, or from
+	 * bch_cache_set_recover() on a cache_set that is still
+	 * CACHE_SET_REGISTERING and therefore not yet reachable by any
+	 * other thread.
+	 */
+	if (!test_bit(CACHE_SET_REGISTERING, &c->flags))
+		lockdep_assert_held(&bch_register_lock);
 
 	if (bch_bucket_alloc_set(c, RESERVE_BTREE, &k.key, true))
 		return 1;
@@ -1215,6 +1222,12 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
 		return -EINVAL;
 	}
 
+	if (test_bit(CACHE_SET_REGISTERING, &c->flags)) {
+		pr_err("Can't attach %pg: cache set is still registering\n",
+		       dc->bdev);
+		return -EINVAL;
+	}
+
 	if (dc->sb.block_size < c->cache->sb.block_size) {
 		/* Will die */
 		pr_err("Couldn't attach %pg: block size less than set's block size\n",
@@ -1983,10 +1996,23 @@ struct cache_set *bch_cache_set_alloc(struct cache_sb *sb)
 	return NULL;
 }
 
-static int run_cache_set(struct cache_set *c)
+/*
+ * Recover a cache_set from disk (journal replay, btree check, allocator
+ * start) or initialize a brand new one. This is the expensive, I/O-bound
+ * part of bringing a cache_set up.
+ *
+ * For a freshly allocated cache_set, this is called without
+ * bch_register_lock held: the cache_set is marked CACHE_SET_REGISTERING
+ * and is not yet reachable via c->kobj/sysfs or via any attach lookup (see
+ * bch_cached_dev_attach()'s CACHE_SET_REGISTERING check), so nothing else
+ * can observe it while it is only partially recovered. No failure path may
+ * exist below after bch_gc_thread_start(), since cache_set_flush()'s
+ * teardown assumes the gc thread is either fully started or
+ * IS_ERR_OR_NULL.
+ */
+static int bch_cache_set_recover(struct cache_set *c)
 {
 	const char *err = "cannot allocate memory";
-	struct cached_dev *dc, *t;
 	struct cache *ca = c->cache;
 	struct closure cl;
 	LIST_HEAD(journal);
@@ -2137,13 +2163,6 @@ static int run_cache_set(struct cache_set *c)
 	if (bch_has_feature_obso_large_bucket(&c->cache->sb))
 		pr_err("Detect obsoleted large bucket layout, all attached bcache device will be read-only\n");
 
-	list_for_each_entry_safe(dc, t, &uncached_devices, list)
-		bch_cached_dev_attach(dc, c, NULL);
-
-	flash_devs_run(c);
-
-	bch_journal_space_reserve(&c->journal);
-	set_bit(CACHE_SET_RUNNING, &c->flags);
 	return 0;
 err:
 	while (!list_empty(&journal)) {
@@ -2159,23 +2178,114 @@ static int run_cache_set(struct cache_set *c)
 	return -EIO;
 }
 
+/*
+ * Publish a recovered cache_set: attach pending backing/flash devices and
+ * mark it running. Must be called with bch_register_lock held. Unlike
+ * bch_cache_set_recover(), nothing here can fail.
+ */
+static void bch_cache_set_attach_devices(struct cache_set *c)
+{
+	struct cached_dev *dc, *t;
+
+	lockdep_assert_held(&bch_register_lock);
+
+	list_for_each_entry_safe(dc, t, &uncached_devices, list)
+		bch_cached_dev_attach(dc, c, NULL);
+
+	flash_devs_run(c);
+
+	bch_journal_space_reserve(&c->journal);
+	set_bit(CACHE_SET_RUNNING, &c->flags);
+}
+
+/*
+ * Recover and publish a cache_set as a single locked unit. Used only for
+ * attaching a cache device to an already-published cache_set (the "found:"
+ * case in register_cache_set()) — that cache_set is already reachable via
+ * bch_cache_sets/sysfs, so it must not go through the unlocked recovery
+ * path used for a brand new cache_set.
+ */
+static int run_cache_set(struct cache_set *c)
+{
+	int ret;
+
+	lockdep_assert_held(&bch_register_lock);
+
+	ret = bch_cache_set_recover(c);
+	if (ret)
+		return ret;
+
+	bch_cache_set_attach_devices(c);
+	return 0;
+}
+
+/*
+ * Register a cache device against a (possibly brand new) cache_set.
+ *
+ * A brand new cache_set only needs bch_register_lock for the cheap
+ * bookkeeping below (duplicate-uuid check, allocation, list linkage,
+ * kobject creation) — the expensive recovery in bch_cache_set_recover()
+ * runs unlocked on a cache_set marked CACHE_SET_REGISTERING, which keeps
+ * it out of sysfs and out of reach of cache-set lookups
+ * (bch_cached_dev_attach()) while still keeping it visible to duplicate
+ * detection and to bcache_reboot() via bch_cache_sets. This avoids
+ * blocking every other cache_set's/cached_dev's sysfs show/store for the
+ * whole recovery duration, which a plain lock-type change cannot fix,
+ * since the lock would still have to be held continuously across
+ * bch_cache_set_recover() for the same reason it does today.
+ *
+ * Attaching an additional cache device to an already-published cache_set
+ * (the "found:" case) is left fully serialized under bch_register_lock,
+ * exactly as before: that cache_set is already reachable via sysfs, so
+ * recovering it outside the lock would reintroduce the half-initialized
+ * -but-visible window this design avoids for the fresh case.
+ */
 static const char *register_cache_set(struct cache *ca)
 {
 	char buf[12];
 	const char *err = "cannot allocate memory";
 	struct cache_set *c;
+	bool fresh = false;
+
+	mutex_lock(&bch_register_lock);
 
 	list_for_each_entry(c, &bch_cache_sets, list)
 		if (!memcmp(c->set_uuid, ca->sb.set_uuid, 16)) {
-			if (c->cache)
+			if (c->cache) {
+				mutex_unlock(&bch_register_lock);
 				return "duplicate cache set member";
+			}
 
 			goto found;
 		}
 
 	c = bch_cache_set_alloc(&ca->sb);
-	if (!c)
+	if (!c) {
+		mutex_unlock(&bch_register_lock);
 		return err;
+	}
+
+	/*
+	 * Link the new cache_set into bch_cache_sets right away, marked
+	 * CACHE_SET_REGISTERING, before dropping the lock for recovery.
+	 * bch_cache_set_alloc() already set c->cache = ca above, so the
+	 * duplicate-uuid check above will correctly reject a second
+	 * concurrent registration for the same uuid while this one is
+	 * mid-recovery.
+	 */
+	set_bit(CACHE_SET_REGISTERING, &c->flags);
+	list_add(&c->list, &bch_cache_sets);
+	fresh = true;
+
+	mutex_unlock(&bch_register_lock);
+
+	err = "failed to recover cache set";
+	if (bch_cache_set_recover(c) < 0) {
+		mutex_lock(&bch_register_lock);
+		goto err;
+	}
+
+	mutex_lock(&bch_register_lock);
 
 	err = "error creating kobject";
 	if (kobject_add(&c->kobj, bcache_kobj, "%pU", c->set_uuid) ||
@@ -2186,8 +2296,7 @@ static const char *register_cache_set(struct cache *ca)
 		goto err;
 
 	bch_debug_init_cache_set(c);
-
-	list_add(&c->list, &bch_cache_sets);
+	clear_bit(CACHE_SET_REGISTERING, &c->flags);
 found:
 	sprintf(buf, "cache%i", ca->sb.nr_this_dev);
 	if (sysfs_create_link(&ca->kobj, &c->kobj, "set") ||
@@ -2199,11 +2308,15 @@ static const char *register_cache_set(struct cache *ca)
 	ca->set->cache = ca;
 
 	err = "failed to run cache set";
-	if (run_cache_set(c) < 0)
+	if (fresh)
+		bch_cache_set_attach_devices(c);
+	else if (run_cache_set(c) < 0)
 		goto err;
 
+	mutex_unlock(&bch_register_lock);
 	return NULL;
 err:
+	mutex_unlock(&bch_register_lock);
 	bch_cache_set_unregister(c);
 	return err;
 }
@@ -2424,9 +2537,7 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
 		goto out;
 	}
 
-	mutex_lock(&bch_register_lock);
 	err = register_cache_set(ca);
-	mutex_unlock(&bch_register_lock);
 
 	if (err) {
 		ret = -ENODEV;
diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index cfac56caa8047..d3b3f807b3e48 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -1032,6 +1032,14 @@ SHOW(__bch_cache)
 {
 	struct cache *ca = container_of(kobj, struct cache, kobj);
 
+	/*
+	 * ca->set is still being recovered without bch_register_lock held
+	 * (see bch_cache_set_recover()); its bucket_lock and on-disk
+	 * superblock are not safe to touch yet.
+	 */
+	if (ca->set && test_bit(CACHE_SET_REGISTERING, &ca->set->flags))
+		return -EAGAIN;
+
 	sysfs_hprint(bucket_size,	bucket_bytes(ca));
 	sysfs_hprint(block_size,	block_bytes(ca));
 	sysfs_print(nbuckets,		ca->sb.nbuckets);
@@ -1140,6 +1148,14 @@ STORE(__bch_cache)
 	if (bcache_is_reboot)
 		return -EBUSY;
 
+	/*
+	 * ca->set is still being recovered without bch_register_lock held
+	 * (see bch_cache_set_recover()); its bucket_lock and on-disk
+	 * superblock are not safe to touch yet.
+	 */
+	if (ca->set && test_bit(CACHE_SET_REGISTERING, &ca->set->flags))
+		return -EAGAIN;
+
 	if (attr == &sysfs_cache_replacement_policy) {
 		v = __sysfs_match_string(cache_replacement_policies, -1, buf);
 		if (v < 0)

---
base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
change-id: 20260706-feat-bcache-30e964a17dd2

Best regards,
-- 
Jing Wu <realwujing@gmail.com>

Re: [PATCH v3] bcache: avoid holding bch_register_lock across cache_set recovery

[Coly Li]

Hi Qliang,

Thanks for the continuous effort.

> 2026年7月6日 22:08，Qiliang Yuan <realwujing@gmail.com> 写道：
> 
> Large-scale bcache device registration and concurrent sysfs access can
> hang unrelated tasks for a long time, e.g.:
> 
> [  243.082130] INFO: task bcache_cache_se:3496 blocked for more than 121 seconds.
> [  243.130817] Call trace:
> [  243.134161]  __switch_to+0x7c/0xbc
> [  243.138461]  __schedule+0x338/0x6f0
> [  243.142847]  schedule+0x50/0xe0
> [  243.146884]  schedule_preempt_disabled+0x18/0x24
> [  243.152400]  __mutex_lock.constprop.0+0x1d4/0x5ec
> [  243.158002]  __mutex_lock_slowpath+0x1c/0x30
> [  243.163170]  mutex_lock+0x50/0x60
> [  243.167397]  bch_cache_set_store+0x40/0x80 [bcache]
> 
> register_cache() holds bch_register_lock for the entire duration of
> register_cache_set(), which calls run_cache_set() to do journal replay,
> btree check and allocator start for a newly registered cache device.
> Every sysfs show/store on any cache_set or cached_dev in the system
> takes the same global bch_register_lock, so an unrelated device's
> sysfs access blocks for as long as the new device's recovery takes.

I feel you should explain the problem you try to solve. Is it that some
sysfs interfaces that you thought should not be blocked but blocked indeed?

If yes, then it is better to list which bcache interfaces you think they should
not not be blocked during the bcache device initialization.

> 
> Converting bch_register_lock to a rw_semaphore does not fix this:
> register_cache() still holds the lock (as a writer) for the whole
> register_cache_set() call, and sysfs store paths still need exclusive
> access, so a store on an unrelated device still blocks for the same
> duration. Downgrading the lock to a reader during recovery was tried,
> but the cache_set's kobjects and its bch_cache_sets list entry are
> already published before recovery starts, so a concurrent unregister
> could hit the cache_set while it is only partially initialized.
> 

Thanks for giving up such effort.


> Fix this at the root instead of changing the lock type: keep
> bch_register_lock a mutex, and only hold it for the actual bookkeeping
> that needs it, not for the multi-second recovery. Split registering a
> brand new cache_set into two phases. Recovery (journal replay, btree
> check, allocator start) runs on a cache_set that is linked into
> bch_cache_sets and marked CACHE_SET_REGISTERING, but not yet added to
> sysfs, without holding bch_register_lock at all: nothing else can
> reach it through a kobject that does not exist yet, and the
> REGISTERING flag keeps bch_cached_dev_attach() from attaching a
> backing device to it before it is ready. Publishing (adding the
> kobjects, attaching pending backing/flash devices) runs under
> bch_register_lock afterwards, and is no more expensive than what
> register_bdev_worker() already does under the same lock today.
> 
> Registering the new cache_set into bch_cache_sets and marking it
> CACHE_SET_REGISTERING happens in the same critical section as
> bch_cache_set_alloc(), which already sets c->cache, so the existing
> duplicate-uuid check keeps working unchanged: a second registration
> for the same uuid still sees c->cache set and is rejected, instead of
> racing to recover the same uuid twice. __uuid_write()'s
> lockdep_assert_held() is relaxed to also accept a cache_set that is
> still CACHE_SET_REGISTERING, since it is called from the unlocked
> recovery path on the fresh-cache branch. The per-cache sysfs show/store
> handlers refuse to touch cache_set state while CACHE_SET_REGISTERING
> is set, since that state is still being written by recovery.
> 
> Attaching an additional cache device to an already-published cache_set
> (the pre-existing "found" case in register_cache_set(), when a cache
> device is re-attached to a cache_set that lost its device earlier) is
> left untouched: that cache_set is already reachable through sysfs, so
> it keeps running recovery and publishing as one operation under
> bch_register_lock, exactly as before.


If your motivation is to get ride of the timeout trace message, it should be much simpler patch.

If the issue to solve is to avoid blocking on some specific sysfs files for some purpose e.g.
for the system monitoring processes to get some information from the bcache subsystem, then
it is better to list the interfaces you are accessing (read or write), in which conditions the processes
are blocked and what is the expected behavior.

Then it will help me to understand (not guess) the problem you trend to solve firstly.



> 
> Co-developed-by: Qiliang Yuan <yuanql9@chinatelecom.cn>
> Signed-off-by: Qiliang Yuan <yuanql9@chinatelecom.cn>
> Signed-off-by: Jing Wu <realwujing@gmail.com>
> ---
> register_cache() holds the global bch_register_lock for the entire
> duration of registering a new cache_set, including journal replay,
> btree check and allocator start. Every sysfs show/store on any other
> cache_set or cached_dev in the system needs the same lock, so a single
> slow cache_set registration blocks unrelated sysfs access for as long
> as recovery takes (see the call trace in the commit message).
> 
> v1/v2 tried to fix this by converting bch_register_lock from a mutex
> to a rw_semaphore so sysfs reads could proceed concurrently. Coly
> pointed out this does not actually help the reported hang (store paths
> still need exclusive access for the same duration), raised a KABI
> concern about changing the global's type, and found a real regression
> in v1's downgrade_write()/up_read()/down_write() dance around
> bch_btree_check().
> 
> v3 drops the lock-type change entirely and fixes the actual problem:
> bch_register_lock stays a mutex, and registering a brand new cache_set
> is split so the expensive recovery runs without holding it at all, on
> a cache_set that is provisionally linked into bch_cache_sets (for
> duplicate-uuid detection) but not yet visible in sysfs or attachable.
> Publishing (kobjects, attaching pending devices) still runs under the
> mutex afterwards, at the same cost register_bdev_worker() already pays
> under the same lock today. Full design rationale is in the commit
> message.

The important message for me is what the problem is, and why you feel
it is an issue and what is the expected behavior.

Help me to be on the same page as you are :-)


> ---
> V2 -> V3:
> - Drop the bch_register_lock mutex -> rw_semaphore conversion entirely
>  (addresses Coly's KABI concern by not touching the lock's type at
>  all).
> - Root-cause fix instead: split registering a brand new cache_set into
>  an unlocked recovery phase (journal replay, btree check, allocator
>  start) and a locked publish phase (kobject_add, attach pending
>  devices), so bch_register_lock is only held for the cheap
>  bookkeeping, not the multi-second recovery that caused the reported
>  hang.
> - Add CACHE_SET_REGISTERING flag: the cache_set is linked into
>  bch_cache_sets immediately (so duplicate-uuid detection and
>  bcache_reboot() still see it) but kept out of sysfs and out of
>  bch_cached_dev_attach()'s reach until recovery finishes.
> - Relax __uuid_write()'s lockdep_assert_held() to also accept a
>  CACHE_SET_REGISTERING cache_set, since it is called from the
>  unlocked recovery path.
> - Gate the per-cache sysfs show/store handlers so they refuse to touch
>  cache_set state while CACHE_SET_REGISTERING is set.
> - Leave the "found:" case (attaching an additional cache device to an
>  already-published cache_set) fully serialized under
>  bch_register_lock, unchanged from v1/v2.
> 
> V1 -> V2:
> - Remove downgrade_write()/up_read()/down_write() lock manipulation in
>  run_cache_set() to eliminate race window during partial initialization
>  that could lead to use-after-free if unregister is triggered between
>  up_read() and down_write(). Keep write lock held throughout instead.
>  (Coly)
> - Simplify all sysfs store wrappers to unconditionally use down_write()
>  instead of per-attribute read/write lock selection, which was fragile
>  and could miss newly added attributes.
> - Fix STORE_LOCKED macro to use down_write()/up_write() for store
>  operations (was incorrectly using down_read()/up_read()).
> - Expand bch_flash_dev store from STORE_LOCKED macro to explicit
>  wrapper with down_write() and bcache_is_reboot check.
> - Fix whitespace indentation in bcache_init() error path.
> - Note on KABI: bch_register_lock is a module-internal global variable
>  with no EXPORT_SYMBOL in the entire bcache driver. Changing its type
>  from struct mutex to struct rw_semaphore does not affect the kernel
>  ABI, as the symbol is never exported to other modules.
> 
> v2: https://lore.kernel.org/r/20260706-feat-bcache-v2-1-70a4b6e246c0@gmail.com
> v1: https://lore.kernel.org/r/20260318-wujing-bcache-v1-1-f0b9aaf3f81d@gmail.com
> ---
> drivers/md/bcache/bcache.h |   9 +++
> drivers/md/bcache/super.c  | 145 +++++++++++++++++++++++++++++++++++++++------
> drivers/md/bcache/sysfs.c  |  16 +++++
> 3 files changed, 153 insertions(+), 17 deletions(-)

At the first glance, I feel this patch doesn’t have enough testing. e.g. the race between bcache_reboot() and bcd_cache_set_recover().
So focus on explaining the problem firstly, help me to be on same page as you are.

Thanks.

Coly Li

Re: [PATCH v3] bcache: avoid holding bch_register_lock across cache_set recovery

[Jing Wu]

From: Qiliang Yuan <realwujing@gmail.com>

Hi Coly,

Thanks for the quick reply. You are right that I jumped straight to the
fix without explaining the problem well enough. Let me back up and
describe the actual production incident this patch comes from, with the
real call traces and the operational consequence, not just the timeout
message.

> I feel you should explain the problem you try to solve. Is it that some
> sysfs interfaces that you thought should not be blocked but blocked indeed?
>
> If yes, then it is better to list which bcache interfaces you think they should
> not not be blocked during the bcache device initialization.

This is from a Ceph OSD node: 2 NVMe drives, each partitioned into 6
partitions (12 cache_sets total), caching 12 HDD backing devices. At
boot, udev triggers a per-device script as each bcacheN device shows
up, which writes tuning values under its sysfs directory. Three
interfaces are concretely affected, all blocked in mutex_lock(&bch_register_lock)
while an unrelated cache_set is still in run_cache_set():

1. bch_cache_set_store(), writing congested_read_threshold_us /
   congested_write_threshold_us for an already-running, unrelated
   cache_set:

   task:bcache_cache_se state:D stack:    0 pid: 3492 ppid:     1
   Call trace:
    __mutex_lock.constprop.0+0x1d4/0x5ec
    __mutex_lock_slowpath+0x1c/0x30
    mutex_lock+0x50/0x60
    bch_cache_set_store+0x40/0x80 [bcache]
    sysfs_kf_write+0x4c/0x5c
    kernfs_fop_write_iter+0x130/0x1c0

2. bch_cached_dev_store(), writing sequential_cutoff for an already
   attached, unrelated backing device (same incident window).

3. register_cache() itself, i.e. registering a *different*, unrelated
   cache device, blocked behind the same lock:

   task:bcache          state:D stack:    0 pid: 8104 ppid:     1
   Call trace:
    __mutex_lock.constprop.0+0x1d4/0x5ec
    __mutex_lock_slowpath+0x1c/0x30
    mutex_lock+0x50/0x60
    register_cache+0x11c/0x1a0 [bcache]
    register_bcache+0x1d4/0x3cc [bcache]
    kobj_attr_store+0x18/0x30

So it is not just "show" vs "store", and it is not limited to the
device currently initializing: an unrelated device's own registration
can be blocked too.

> If your motivation is to get ride of the timeout trace message, it should be
> much simpler patch.
>
> If the issue to solve is to avoid blocking on some specific sysfs files for
> some purpose e.g. for the system monitoring processes to get some
> information from the bcache subsystem, then it is better to list the
> interfaces you are accessing (read or write), in which conditions the
> processes are blocked and what is the expected behavior.

It is not about the timeout message. The concrete, measurable
consequence in this incident is that systemd-udevd kills the udev
worker once it hits its own timeout, and the device add event fails
and gets retried:

  systemd-udevd: bcache7: Spawned process '/usr/sbin/bcache_cached_dev_conf.sh ...' timed out after 2min 59s, killing
  systemd-udevd: bcache7: Worker [1879] failed
  systemd-udevd: bcache7: Retry 1 times.

This is not a one-off: the same kill/retry cycle repeats roughly every
3 minutes (11:04:03, 11:07:04, 11:10:05, ...) and by 11:04:03 it hits
essentially every NVMe partition on the box (nvme0n1p7-12,
nvme1n1p8/10/11/12). So the whole bcache management plane on this node
fails to come up cleanly at boot and keeps retrying for several
minutes, which in turn delays the LVM PV activation / Ceph OSD startup
that depends on these devices being ready (interleaved in the same
log window).

Expected behavior: recovering one cache_set (journal replay + btree
check, whose duration scales with device capacity/health and is not
bounded) should only make operations on *that* device wait. It should
not block sysfs access to other, already-running cache_sets/cached_devs,
and it should not block registration of other, unrelated cache
devices.

> At the first glance, I feel this patch doesn't have enough testing. e.g. the
> race between bcache_reboot() and bcd_cache_set_recover().

That is a fair concern, and I want to be precise about what I have and
have not actually verified, rather than just argue from the code.

What I did check: bch_cache_set_recover() only runs unlocked on a
cache_set that was just allocated by bch_cache_set_alloc(). The
CACHE_SET_REGISTERING flag is set and the cache_set is list_add()'d
into bch_cache_sets in the *same* bch_register_lock critical section as
the allocation, before the lock is dropped for recovery. So
bcache_reboot()'s list_empty(&bch_cache_sets) checks and its
mutex_lock()-protected wait loop see this cache_set for its entire
recovery, the same as any other cache_set that takes a while to stop -
there is no window where it is invisible to bcache_reboot().

What I have not done is actually exercise this interleaving: trigger a
reboot while a cache_set is mid-recovery and confirm bcache_reboot()
waits for it correctly instead of assuming based on the code path. I'd
rather get that data than keep arguing from the source. If you have a
specific interleaving in mind that you think breaks this, please point
it out and I will either fix it or build a targeted reproduction
(e.g. an artificial delay inside bch_cache_set_recover() to widen the
window and drive bcache_reboot() through it) and report back with
results before asking you to look at the patch again.

Thanks,
Qiliang