* [RFC PATCH] bcache: make bcache_is_reboot atomic.
@ 2025-12-30 11:33 Wale Zhang
2025-12-30 12:22 ` Coly Li
0 siblings, 1 reply; 3+ messages in thread
From: Wale Zhang @ 2025-12-30 11:33 UTC (permalink / raw)
To: colyli; +Cc: kent.overstreet, linux-bcache, linux-kernel, Wale Zhang
bcache: make bcache_is_reboot atomic.
The smp_mb is mainly used to ensure the dependency relationship between
variables, but there is only one variable bcache_is_reboot. Therefore,
using smp_mb is not very appropriate.
When bcache_reboot and register_bcache occur concurrently, register_bcache
cannot immediately detect that bcache_is_reboot has been set to true.
cpu0 cpu1
bcache_reboot
bcache_is_reboot = true;
smp_mb(); register_bcache
smp_mb();
if (bcache_is_reboot)
// bcache_is_reboot may still be false.
Signed-off-by: Wale Zhang <wale.zhang.ftd@gmail.com>
---
drivers/md/bcache/super.c | 19 ++++++-------------
drivers/md/bcache/sysfs.c | 14 +++++++-------
2 files changed, 13 insertions(+), 20 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index c17d4517af22..0b2098aa7234 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -41,7 +41,7 @@ static const char invalid_uuid[] = {
static struct kobject *bcache_kobj;
struct mutex bch_register_lock;
-bool bcache_is_reboot;
+atomic_t bcache_is_reboot = ATOMIC_INIT(0);
LIST_HEAD(bch_cache_sets);
static LIST_HEAD(uncached_devices);
@@ -2561,10 +2561,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
if (!try_module_get(THIS_MODULE))
goto out;
- /* For latest state of bcache_is_reboot */
- smp_mb();
err = "bcache is in reboot";
- if (bcache_is_reboot)
+ if (atomic_read(&bcache_is_reboot))
goto out_module_put;
ret = -ENOMEM;
@@ -2735,7 +2733,7 @@ static ssize_t bch_pending_bdevs_cleanup(struct kobject *k,
static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
{
- if (bcache_is_reboot)
+ if (atomic_read(&bcache_is_reboot))
return NOTIFY_DONE;
if (code == SYS_DOWN ||
@@ -2750,16 +2748,11 @@ static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
mutex_lock(&bch_register_lock);
- if (bcache_is_reboot)
+ if (atomic_read(&bcache_is_reboot))
goto out;
/* New registration is rejected since now */
- bcache_is_reboot = true;
- /*
- * Make registering caller (if there is) on other CPU
- * core know bcache_is_reboot set to true earlier
- */
- smp_mb();
+ atomic_set(&bcache_is_reboot, 1);
if (list_empty(&bch_cache_sets) &&
list_empty(&uncached_devices))
@@ -2935,7 +2928,7 @@ static int __init bcache_init(void)
bch_debug_init();
- bcache_is_reboot = false;
+ atomic_set(&bcache_is_reboot, 0);
return 0;
err:
diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index 72f38e5b6f5c..5384653c5bbb 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -17,7 +17,7 @@
#include <linux/sort.h>
#include <linux/sched/clock.h>
-extern bool bcache_is_reboot;
+extern atomic_t bcache_is_reboot;
/* Default is 0 ("writethrough") */
static const char * const bch_cache_modes[] = {
@@ -296,7 +296,7 @@ STORE(__cached_dev)
struct kobj_uevent_env *env;
/* no user space access if system is rebooting */
- if (bcache_is_reboot)
+ if (atomic_read(&bcache_is_reboot))
return -EBUSY;
#define d_strtoul(var) sysfs_strtoul(var, dc->var)
@@ -459,7 +459,7 @@ STORE(bch_cached_dev)
disk.kobj);
/* no user space access if system is rebooting */
- if (bcache_is_reboot)
+ if (atomic_read(&bcache_is_reboot))
return -EBUSY;
mutex_lock(&bch_register_lock);
@@ -571,7 +571,7 @@ STORE(__bch_flash_dev)
struct uuid_entry *u = &d->c->uuids[d->id];
/* no user space access if system is rebooting */
- if (bcache_is_reboot)
+ if (atomic_read(&bcache_is_reboot))
return -EBUSY;
sysfs_strtoul(data_csum, d->data_csum);
@@ -814,7 +814,7 @@ STORE(__bch_cache_set)
ssize_t v;
/* no user space access if system is rebooting */
- if (bcache_is_reboot)
+ if (atomic_read(&bcache_is_reboot))
return -EBUSY;
if (attr == &sysfs_unregister)
@@ -941,7 +941,7 @@ STORE(bch_cache_set_internal)
struct cache_set *c = container_of(kobj, struct cache_set, internal);
/* no user space access if system is rebooting */
- if (bcache_is_reboot)
+ if (atomic_read(&bcache_is_reboot))
return -EBUSY;
return bch_cache_set_store(&c->kobj, attr, buf, size);
@@ -1137,7 +1137,7 @@ STORE(__bch_cache)
ssize_t v;
/* no user space access if system is rebooting */
- if (bcache_is_reboot)
+ if (atomic_read(&bcache_is_reboot))
return -EBUSY;
if (attr == &sysfs_cache_replacement_policy) {
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [RFC PATCH] bcache: make bcache_is_reboot atomic.
2025-12-30 11:33 [RFC PATCH] bcache: make bcache_is_reboot atomic Wale Zhang
@ 2025-12-30 12:22 ` Coly Li
2025-12-31 11:02 ` wale zhang
0 siblings, 1 reply; 3+ messages in thread
From: Coly Li @ 2025-12-30 12:22 UTC (permalink / raw)
To: Wale Zhang; +Cc: kent.overstreet, linux-bcache, linux-kernel
On Tue, Dec 30, 2025 at 07:33:57PM +0800, Wale Zhang wrote:
> bcache: make bcache_is_reboot atomic.
>
> The smp_mb is mainly used to ensure the dependency relationship between
> variables, but there is only one variable bcache_is_reboot. Therefore,
> using smp_mb is not very appropriate.
>
> When bcache_reboot and register_bcache occur concurrently, register_bcache
> cannot immediately detect that bcache_is_reboot has been set to true.
>
> cpu0 cpu1
> bcache_reboot
> bcache_is_reboot = true;
> smp_mb(); register_bcache
> smp_mb();
> if (bcache_is_reboot)
> // bcache_is_reboot may still be false.
>
From the above comments, I don't see what problem this patch tries to solve.
And read or write bcache_is_reboot is atomic indeed, I don't see the difference
this patch makes.
Thanks.
Coly Li
> Signed-off-by: Wale Zhang <wale.zhang.ftd@gmail.com>
> ---
> drivers/md/bcache/super.c | 19 ++++++-------------
> drivers/md/bcache/sysfs.c | 14 +++++++-------
> 2 files changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index c17d4517af22..0b2098aa7234 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -41,7 +41,7 @@ static const char invalid_uuid[] = {
>
> static struct kobject *bcache_kobj;
> struct mutex bch_register_lock;
> -bool bcache_is_reboot;
> +atomic_t bcache_is_reboot = ATOMIC_INIT(0);
> LIST_HEAD(bch_cache_sets);
> static LIST_HEAD(uncached_devices);
>
> @@ -2561,10 +2561,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> if (!try_module_get(THIS_MODULE))
> goto out;
>
> - /* For latest state of bcache_is_reboot */
> - smp_mb();
> err = "bcache is in reboot";
> - if (bcache_is_reboot)
> + if (atomic_read(&bcache_is_reboot))
> goto out_module_put;
>
> ret = -ENOMEM;
> @@ -2735,7 +2733,7 @@ static ssize_t bch_pending_bdevs_cleanup(struct kobject *k,
>
> static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
> {
> - if (bcache_is_reboot)
> + if (atomic_read(&bcache_is_reboot))
> return NOTIFY_DONE;
>
> if (code == SYS_DOWN ||
> @@ -2750,16 +2748,11 @@ static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
>
> mutex_lock(&bch_register_lock);
>
> - if (bcache_is_reboot)
> + if (atomic_read(&bcache_is_reboot))
> goto out;
>
> /* New registration is rejected since now */
> - bcache_is_reboot = true;
> - /*
> - * Make registering caller (if there is) on other CPU
> - * core know bcache_is_reboot set to true earlier
> - */
> - smp_mb();
> + atomic_set(&bcache_is_reboot, 1);
>
> if (list_empty(&bch_cache_sets) &&
> list_empty(&uncached_devices))
> @@ -2935,7 +2928,7 @@ static int __init bcache_init(void)
>
> bch_debug_init();
>
> - bcache_is_reboot = false;
> + atomic_set(&bcache_is_reboot, 0);
>
> return 0;
> err:
> diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> index 72f38e5b6f5c..5384653c5bbb 100644
> --- a/drivers/md/bcache/sysfs.c
> +++ b/drivers/md/bcache/sysfs.c
> @@ -17,7 +17,7 @@
> #include <linux/sort.h>
> #include <linux/sched/clock.h>
>
> -extern bool bcache_is_reboot;
> +extern atomic_t bcache_is_reboot;
>
> /* Default is 0 ("writethrough") */
> static const char * const bch_cache_modes[] = {
> @@ -296,7 +296,7 @@ STORE(__cached_dev)
> struct kobj_uevent_env *env;
>
> /* no user space access if system is rebooting */
> - if (bcache_is_reboot)
> + if (atomic_read(&bcache_is_reboot))
> return -EBUSY;
>
> #define d_strtoul(var) sysfs_strtoul(var, dc->var)
> @@ -459,7 +459,7 @@ STORE(bch_cached_dev)
> disk.kobj);
>
> /* no user space access if system is rebooting */
> - if (bcache_is_reboot)
> + if (atomic_read(&bcache_is_reboot))
> return -EBUSY;
>
> mutex_lock(&bch_register_lock);
> @@ -571,7 +571,7 @@ STORE(__bch_flash_dev)
> struct uuid_entry *u = &d->c->uuids[d->id];
>
> /* no user space access if system is rebooting */
> - if (bcache_is_reboot)
> + if (atomic_read(&bcache_is_reboot))
> return -EBUSY;
>
> sysfs_strtoul(data_csum, d->data_csum);
> @@ -814,7 +814,7 @@ STORE(__bch_cache_set)
> ssize_t v;
>
> /* no user space access if system is rebooting */
> - if (bcache_is_reboot)
> + if (atomic_read(&bcache_is_reboot))
> return -EBUSY;
>
> if (attr == &sysfs_unregister)
> @@ -941,7 +941,7 @@ STORE(bch_cache_set_internal)
> struct cache_set *c = container_of(kobj, struct cache_set, internal);
>
> /* no user space access if system is rebooting */
> - if (bcache_is_reboot)
> + if (atomic_read(&bcache_is_reboot))
> return -EBUSY;
>
> return bch_cache_set_store(&c->kobj, attr, buf, size);
> @@ -1137,7 +1137,7 @@ STORE(__bch_cache)
> ssize_t v;
>
> /* no user space access if system is rebooting */
> - if (bcache_is_reboot)
> + if (atomic_read(&bcache_is_reboot))
> return -EBUSY;
>
> if (attr == &sysfs_cache_replacement_policy) {
> --
> 2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [RFC PATCH] bcache: make bcache_is_reboot atomic.
2025-12-30 12:22 ` Coly Li
@ 2025-12-31 11:02 ` wale zhang
0 siblings, 0 replies; 3+ messages in thread
From: wale zhang @ 2025-12-31 11:02 UTC (permalink / raw)
To: Coly Li; +Cc: kent.overstreet, linux-bcache, linux-kernel
On Tue, Dec 30, 2025 at 8:22 PM Coly Li <colyli@fnnas.com> wrote:
>
> On Tue, Dec 30, 2025 at 07:33:57PM +0800, Wale Zhang wrote:
> > bcache: make bcache_is_reboot atomic.
> >
> > The smp_mb is mainly used to ensure the dependency relationship between
> > variables, but there is only one variable bcache_is_reboot. Therefore,
> > using smp_mb is not very appropriate.
> >
> > When bcache_reboot and register_bcache occur concurrently, register_bcache
> > cannot immediately detect that bcache_is_reboot has been set to true.
> >
> > cpu0 cpu1
> > bcache_reboot
> > bcache_is_reboot = true;
> > smp_mb(); register_bcache
> > smp_mb();
> > if (bcache_is_reboot)
> > // bcache_is_reboot may still be false.
> >
>
> From the above comments, I don't see what problem this patch tries to solve.
> And read or write bcache_is_reboot is atomic indeed, I don't see the difference
> this patch makes.
Hello Coly,
I'm puzzled by the use of smp_mb in this bcache_is_reboot scenario.
I know that smp_mb is typically used for the scenarios where data
dependency is involved, such as
---------------------------------
CPU0 CPU1
x=1 y=1
smp_mb smb_mb
load y load x
---------------------------------
If CPU1 observe x==1, then CPU0 must also observe y==1.
Thank you for clearing up my puzzlement.
Thanks
Wale
> Thanks.
>
> Coly Li
>
> > Signed-off-by: Wale Zhang <wale.zhang.ftd@gmail.com>
> > ---
> > drivers/md/bcache/super.c | 19 ++++++-------------
> > drivers/md/bcache/sysfs.c | 14 +++++++-------
> > 2 files changed, 13 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index c17d4517af22..0b2098aa7234 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -41,7 +41,7 @@ static const char invalid_uuid[] = {
> >
> > static struct kobject *bcache_kobj;
> > struct mutex bch_register_lock;
> > -bool bcache_is_reboot;
> > +atomic_t bcache_is_reboot = ATOMIC_INIT(0);
> > LIST_HEAD(bch_cache_sets);
> > static LIST_HEAD(uncached_devices);
> >
> > @@ -2561,10 +2561,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> > if (!try_module_get(THIS_MODULE))
> > goto out;
> >
> > - /* For latest state of bcache_is_reboot */
> > - smp_mb();
> > err = "bcache is in reboot";
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > goto out_module_put;
> >
> > ret = -ENOMEM;
> > @@ -2735,7 +2733,7 @@ static ssize_t bch_pending_bdevs_cleanup(struct kobject *k,
> >
> > static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
> > {
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return NOTIFY_DONE;
> >
> > if (code == SYS_DOWN ||
> > @@ -2750,16 +2748,11 @@ static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
> >
> > mutex_lock(&bch_register_lock);
> >
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > goto out;
> >
> > /* New registration is rejected since now */
> > - bcache_is_reboot = true;
> > - /*
> > - * Make registering caller (if there is) on other CPU
> > - * core know bcache_is_reboot set to true earlier
> > - */
> > - smp_mb();
> > + atomic_set(&bcache_is_reboot, 1);
> >
> > if (list_empty(&bch_cache_sets) &&
> > list_empty(&uncached_devices))
> > @@ -2935,7 +2928,7 @@ static int __init bcache_init(void)
> >
> > bch_debug_init();
> >
> > - bcache_is_reboot = false;
> > + atomic_set(&bcache_is_reboot, 0);
> >
> > return 0;
> > err:
> > diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> > index 72f38e5b6f5c..5384653c5bbb 100644
> > --- a/drivers/md/bcache/sysfs.c
> > +++ b/drivers/md/bcache/sysfs.c
> > @@ -17,7 +17,7 @@
> > #include <linux/sort.h>
> > #include <linux/sched/clock.h>
> >
> > -extern bool bcache_is_reboot;
> > +extern atomic_t bcache_is_reboot;
> >
> > /* Default is 0 ("writethrough") */
> > static const char * const bch_cache_modes[] = {
> > @@ -296,7 +296,7 @@ STORE(__cached_dev)
> > struct kobj_uevent_env *env;
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > #define d_strtoul(var) sysfs_strtoul(var, dc->var)
> > @@ -459,7 +459,7 @@ STORE(bch_cached_dev)
> > disk.kobj);
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > mutex_lock(&bch_register_lock);
> > @@ -571,7 +571,7 @@ STORE(__bch_flash_dev)
> > struct uuid_entry *u = &d->c->uuids[d->id];
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > sysfs_strtoul(data_csum, d->data_csum);
> > @@ -814,7 +814,7 @@ STORE(__bch_cache_set)
> > ssize_t v;
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > if (attr == &sysfs_unregister)
> > @@ -941,7 +941,7 @@ STORE(bch_cache_set_internal)
> > struct cache_set *c = container_of(kobj, struct cache_set, internal);
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > return bch_cache_set_store(&c->kobj, attr, buf, size);
> > @@ -1137,7 +1137,7 @@ STORE(__bch_cache)
> > ssize_t v;
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > if (attr == &sysfs_cache_replacement_policy) {
> > --
> > 2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-12-31 11:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-30 11:33 [RFC PATCH] bcache: make bcache_is_reboot atomic Wale Zhang
2025-12-30 12:22 ` Coly Li
2025-12-31 11:02 ` wale zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox