linux-kernel - Re: [RFC PATCH] bcache: make bcache_is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <aVPDeUkQ1f_PqEDs@studio.local>
Date: Tue, 30 Dec 2025 20:22:29 +0800
From: "Coly Li" <colyli@...as.com>
To: "Wale Zhang" <wale.zhang.ftd@...il.com>
Cc: <kent.overstreet@...ux.dev>, <linux-bcache@...r.kernel.org>, 
	<linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] bcache: make bcache_is_reboot atomic.

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@...il.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