Re: [RFC PATCH] bcache: make bcache_is_reboot atomic.

On Tue, Dec 30, 2025 at 8:22 PM Coly Li <[email protected]> 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 <[email protected]>
> > ---
> >  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