[PATCH v2 0/2] Device name and RCU string

[PATCH v2 0/2] Device name and RCU string

[David Sterba]

v2:
- drop patch adding RCU protection around update_dev_time(), there's a
  sleeping allocation inside; after revisiting the RCU requirements, I
  don't think it's needed and we can access the raw string (the same way
  it was done with rcu_string), the time update is called from scratch
  superblocks and after the ->dev_list hook is removed, so there's no
  chance it can be reached from device_list_add

After recent simplifications of the RCU usage in messages, this patchset
implements the RCU protection directly without the RCU string so this
API can be removed completely as we don't have nor plan anything else to
use it for.

David Sterba (2):
  btrfs: open code RCU for device name
  btrfs: remove struct rcu_string

 fs/btrfs/rcu-string.h | 40 ----------------------------------------
 fs/btrfs/volumes.c    | 29 +++++++++++++++++------------
 fs/btrfs/volumes.h    |  6 +++---
 fs/btrfs/zoned.c      | 23 +++++++++++------------
 4 files changed, 31 insertions(+), 67 deletions(-)
 delete mode 100644 fs/btrfs/rcu-string.h

-- 
2.49.0

[PATCH v2 1/2] btrfs: open code RCU for device name

[David Sterba]

The RCU protected string is only used for a device name, and RCU is used
so we can print the name and eventually synchronize against the rare
device rename in device_list_add().

We don't need the whole API just for that. Open code all the helpers and
access to the string itself.

Notable change is in device_list_add() when the device name is changed,
which is the only place that can actually happen at the same time as
message prints using the device name under RCU read lock.

Previously there was kfree_rcu() which used the embedded rcu_head to
delay freeing the object depending on the RCU mechanism. Now there's
kfree_rcu_mightsleep() which does not need the rcu_head and waits for
the grace period.

Sleeping is safe in this context and as this is a rare event it won't
interfere with the rest as it's holding the device_list_mutex.

Straightforward changes:

- rcu_string_strdup -> kstrdup
- rcu_str_deref -> rcu_dereference
- drop ->str from safe contexts

Historical notes:

Introduced in 606686eeac45 ("Btrfs: use rcu to protect device->name")
with a vague reference of the potential problem described in
https://lore.kernel.org/all/20120531155304.GF11775@ZenIV.linux.org.uk/ .

The RCU protection looks like the easiest and most lightweight way of
protecting the rare event of device rename racing device_list_add()
with a random printk() that uses the device name.

Alternatives: a spin lock would require to protect the printk
anyway, a fixed buffer for the name would be eventually wrong in case
the new name is overwritten when being printed, an array switching
pointers and cleaning them up eventually resembles RCU too much.

The cleanups up to this patch should hide special case of RCU to the
minimum that only the name needs rcu_dereference(), which can be further
cleaned up to use btrfs_dev_name().

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 28 +++++++++++++++++-----------
 fs/btrfs/volumes.h |  5 +++--
 fs/btrfs/zoned.c   | 22 +++++++++++-----------
 3 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 29282f26336e..9ab2ed7cce8d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -658,7 +658,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 	if (!device->name)
 		return -EINVAL;
 
-	ret = btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1,
+	ret = btrfs_get_bdev_and_sb(device->name, flags, holder, 1,
 				    &bdev_file, &disk_super);
 	if (ret)
 		return ret;
@@ -702,7 +702,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 	if (device->devt != device->bdev->bd_dev) {
 		btrfs_warn(NULL,
 			   "device %s maj:min changed from %d:%d to %d:%d",
-			   device->name->str, MAJOR(device->devt),
+			   device->name, MAJOR(device->devt),
 			   MINOR(device->devt), MAJOR(device->bdev->bd_dev),
 			   MINOR(device->bdev->bd_dev));
 
@@ -750,7 +750,7 @@ static bool is_same_device(struct btrfs_device *device, const char *new_path)
 		goto out;
 
 	rcu_read_lock();
-	ret = strscpy(old_path, rcu_str_deref(device->name), PATH_MAX);
+	ret = strscpy(old_path, rcu_dereference(device->name), PATH_MAX);
 	rcu_read_unlock();
 	if (ret < 0)
 		goto out;
@@ -783,7 +783,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *fs_devices = NULL;
-	struct rcu_string *name;
+	const char *name;
 	u64 found_transid = btrfs_super_generation(disk_super);
 	u64 devid = btrfs_stack_device_id(&disk_super->dev_item);
 	dev_t path_devt;
@@ -891,6 +891,8 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 				current->comm, task_pid_nr(current));
 
 	} else if (!device->name || !is_same_device(device, path)) {
+		const char *old_name;
+
 		/*
 		 * When FS is already mounted.
 		 * 1. If you are here and if the device->name is NULL that
@@ -958,13 +960,17 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 					  task_pid_nr(current));
 		}
 
-		name = rcu_string_strdup(path, GFP_NOFS);
+		name = kstrdup(path, GFP_NOFS);
 		if (!name) {
 			mutex_unlock(&fs_devices->device_list_mutex);
 			return ERR_PTR(-ENOMEM);
 		}
-		kfree_rcu(device->name, rcu);
+		rcu_read_lock();
+		old_name = rcu_dereference(device->name);
+		rcu_read_unlock();
 		rcu_assign_pointer(device->name, name);
+		kfree_rcu_mightsleep(old_name);
+
 		if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state)) {
 			fs_devices->missing_devices--;
 			clear_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state);
@@ -1013,7 +1019,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 		 * uuid mutex so nothing we touch in here is going to disappear.
 		 */
 		if (orig_dev->name)
-			dev_path = orig_dev->name->str;
+			dev_path = orig_dev->name;
 
 		device = btrfs_alloc_device(NULL, &orig_dev->devid,
 					    orig_dev->uuid, dev_path);
@@ -1415,7 +1421,7 @@ static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
 
 		list_for_each_entry(device, &fs_devices->devices, dev_list) {
 			if (device->bdev && (device->bdev->bd_dev == devt) &&
-			    strcmp(device->name->str, path) != 0) {
+			    strcmp(device->name, path) != 0) {
 				mutex_unlock(&fs_devices->device_list_mutex);
 
 				/* Do not skip registration. */
@@ -2165,7 +2171,7 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info, struct btrfs_devic
 	btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
 
 	/* Update ctime/mtime for device path for libblkid */
-	update_dev_time(device->name->str);
+	update_dev_time(device->name);
 }
 
 int btrfs_rm_device(struct btrfs_fs_info *fs_info,
@@ -6924,9 +6930,9 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 		generate_random_uuid(dev->uuid);
 
 	if (path) {
-		struct rcu_string *name;
+		const char *name;
 
-		name = rcu_string_strdup(path, GFP_KERNEL);
+		name = kstrdup(path, GFP_KERNEL);
 		if (!name) {
 			btrfs_free_device(dev);
 			return ERR_PTR(-ENOMEM);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 6d8b1f38e3ee..149434e42f7a 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -114,7 +114,8 @@ struct btrfs_device {
 	struct btrfs_fs_devices *fs_devices;
 	struct btrfs_fs_info *fs_info;
 
-	struct rcu_string __rcu *name;
+	/* Device path or NULL if missing. */
+	const char __rcu *name;
 
 	u64 generation;
 
@@ -846,7 +847,7 @@ static inline const char *btrfs_dev_name(const struct btrfs_device *device)
 	if (!device || test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
 		return "<missing disk>";
 	else
-		return rcu_str_deref(device->name);
+		return rcu_dereference(device->name);
 }
 
 static inline void btrfs_warn_unknown_chunk_allocation(enum btrfs_chunk_allocation_policy pol)
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index bd987c90a05c..27264db4b7ca 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -266,7 +266,7 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
 	if (ret < 0) {
 		btrfs_err(device->fs_info,
 				 "zoned: failed to read zone %llu on %s (devid %llu)",
-				 pos, rcu_str_deref(device->name),
+				 pos, rcu_dereference(device->name),
 				 device->devid);
 		return ret;
 	}
@@ -398,14 +398,14 @@ int btrfs_get_dev_zone_info(struct btrfs_device *device, bool populate_cache)
 	if (zone_info->zone_size > BTRFS_MAX_ZONE_SIZE) {
 		btrfs_err(fs_info,
 		"zoned: %s: zone size %llu larger than supported maximum %llu",
-				 rcu_str_deref(device->name),
+				 rcu_dereference(device->name),
 				 zone_info->zone_size, BTRFS_MAX_ZONE_SIZE);
 		ret = -EINVAL;
 		goto out;
 	} else if (zone_info->zone_size < BTRFS_MIN_ZONE_SIZE) {
 		btrfs_err(fs_info,
 		"zoned: %s: zone size %llu smaller than supported minimum %u",
-				 rcu_str_deref(device->name),
+				 rcu_dereference(device->name),
 				 zone_info->zone_size, BTRFS_MIN_ZONE_SIZE);
 		ret = -EINVAL;
 		goto out;
@@ -421,7 +421,7 @@ int btrfs_get_dev_zone_info(struct btrfs_device *device, bool populate_cache)
 	if (max_active_zones && max_active_zones < BTRFS_MIN_ACTIVE_ZONES) {
 		btrfs_err(fs_info,
 "zoned: %s: max active zones %u is too small, need at least %u active zones",
-				 rcu_str_deref(device->name), max_active_zones,
+				 rcu_dereference(device->name), max_active_zones,
 				 BTRFS_MIN_ACTIVE_ZONES);
 		ret = -EINVAL;
 		goto out;
@@ -463,7 +463,7 @@ int btrfs_get_dev_zone_info(struct btrfs_device *device, bool populate_cache)
 		if (!zone_info->zone_cache) {
 			btrfs_err(device->fs_info,
 				"zoned: failed to allocate zone cache for %s",
-				rcu_str_deref(device->name));
+				rcu_dereference(device->name));
 			ret = -ENOMEM;
 			goto out;
 		}
@@ -500,7 +500,7 @@ int btrfs_get_dev_zone_info(struct btrfs_device *device, bool populate_cache)
 	if (nreported != zone_info->nr_zones) {
 		btrfs_err(device->fs_info,
 				 "inconsistent number of zones on %s (%u/%u)",
-				 rcu_str_deref(device->name), nreported,
+				 rcu_dereference(device->name), nreported,
 				 zone_info->nr_zones);
 		ret = -EIO;
 		goto out;
@@ -510,7 +510,7 @@ int btrfs_get_dev_zone_info(struct btrfs_device *device, bool populate_cache)
 		if (nactive > max_active_zones) {
 			btrfs_err(device->fs_info,
 			"zoned: %u active zones on %s exceeds max_active_zones %u",
-					 nactive, rcu_str_deref(device->name),
+					 nactive, rcu_dereference(device->name),
 					 max_active_zones);
 			ret = -EIO;
 			goto out;
@@ -578,7 +578,7 @@ int btrfs_get_dev_zone_info(struct btrfs_device *device, bool populate_cache)
 
 	btrfs_info(fs_info,
 		"%s block device %s, %u %szones of %llu bytes",
-		model, rcu_str_deref(device->name), zone_info->nr_zones,
+		model, rcu_dereference(device->name), zone_info->nr_zones,
 		emulated, zone_info->zone_size);
 
 	return 0;
@@ -1186,7 +1186,7 @@ int btrfs_ensure_empty_zones(struct btrfs_device *device, u64 start, u64 size)
 		btrfs_warn(
 			device->fs_info,
 		"zoned: resetting device %s (devid %llu) zone %llu for allocation",
-			rcu_str_deref(device->name), device->devid, pos >> shift);
+			rcu_dereference(device->name), device->devid, pos >> shift);
 		WARN_ON_ONCE(1);
 
 		ret = btrfs_reset_device_zone(device, pos, zinfo->zone_size,
@@ -1348,7 +1348,7 @@ static int btrfs_load_zone_info(struct btrfs_fs_info *fs_info, int zone_idx,
 	if (zone.type == BLK_ZONE_TYPE_CONVENTIONAL) {
 		btrfs_err(fs_info,
 		"zoned: unexpected conventional zone %llu on device %s (devid %llu)",
-			zone.start << SECTOR_SHIFT, rcu_str_deref(device->name),
+			zone.start << SECTOR_SHIFT, rcu_dereference(device->name),
 			device->devid);
 		up_read(&dev_replace->rwsem);
 		return -EIO;
@@ -1362,7 +1362,7 @@ static int btrfs_load_zone_info(struct btrfs_fs_info *fs_info, int zone_idx,
 		btrfs_err(fs_info,
 		"zoned: offline/readonly zone %llu on device %s (devid %llu)",
 			  (info->physical >> device->zone_info->zone_size_shift),
-			  rcu_str_deref(device->name), device->devid);
+			  rcu_dereference(device->name), device->devid);
 		info->alloc_offset = WP_MISSING_DEV;
 		break;
 	case BLK_ZONE_COND_EMPTY:
-- 
2.49.0

Re: [PATCH v2 1/2] btrfs: open code RCU for device name

[Wang Yugui]

Hi,

> The RCU protected string is only used for a device name, and RCU is used
> so we can print the name and eventually synchronize against the rare
> device rename in device_list_add().
> 
> We don't need the whole API just for that. Open code all the helpers and
> access to the string itself.
> 
> Notable change is in device_list_add() when the device name is changed,
> which is the only place that can actually happen at the same time as
> message prints using the device name under RCU read lock.
> 
> Previously there was kfree_rcu() which used the embedded rcu_head to
> delay freeing the object depending on the RCU mechanism. Now there's
> kfree_rcu_mightsleep() which does not need the rcu_head and waits for
> the grace period.
> 
> Sleeping is safe in this context and as this is a rare event it won't
> interfere with the rest as it's holding the device_list_mutex.
> 
> Straightforward changes:
> 
> - rcu_string_strdup -> kstrdup
> - rcu_str_deref -> rcu_dereference
> - drop ->str from safe contexts
> 
> Historical notes:
> 
> Introduced in 606686eeac45 ("Btrfs: use rcu to protect device->name")
> with a vague reference of the potential problem described in
> https://lore.kernel.org/all/20120531155304.GF11775@ZenIV.linux.org.uk/ .
> 
> The RCU protection looks like the easiest and most lightweight way of
> protecting the rare event of device rename racing device_list_add()
> with a random printk() that uses the device name.
> 
> Alternatives: a spin lock would require to protect the printk
> anyway, a fixed buffer for the name would be eventually wrong in case
> the new name is overwritten when being printed, an array switching
> pointers and cleaning them up eventually resembles RCU too much.
> 
> The cleanups up to this patch should hide special case of RCU to the
> minimum that only the name needs rcu_dereference(), which can be further
> cleaned up to use btrfs_dev_name().
> 

There is still rcu warning when 'make  W=1 C=1'

/usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21: warning: incorrect type in argument 1 (different address spaces)
/usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    expected void const *objp
/usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    got char const [noderef] __rcu *name

static void btrfs_free_device(struct btrfs_device *device)
{
    WARN_ON(!list_empty(&device->post_commit_list));
    /* No need to call kfree_rcu(), nothing is reading the device name. */
L405:    kfree(device->name);

do we need rcu_dereference here?
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -402,7 +402,7 @@ static void btrfs_free_device(struct btrfs_device *device)
 {
        WARN_ON(!list_empty(&device->post_commit_list));
        /* No need to call kfree_rcu(), nothing is reading the device name. */
-       kfree(device->name);
+       kfree(rcu_dereference(device->name));

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2025/06/30

Re: [PATCH v2 1/2] btrfs: open code RCU for device name

[David Sterba]

On Mon, Jun 30, 2025 at 10:24:57AM +0800, Wang Yugui wrote:
> Hi,
> 
> > The RCU protected string is only used for a device name, and RCU is used
> > so we can print the name and eventually synchronize against the rare
> > device rename in device_list_add().
> > 
> > We don't need the whole API just for that. Open code all the helpers and
> > access to the string itself.
> > 
> > Notable change is in device_list_add() when the device name is changed,
> > which is the only place that can actually happen at the same time as
> > message prints using the device name under RCU read lock.
> > 
> > Previously there was kfree_rcu() which used the embedded rcu_head to
> > delay freeing the object depending on the RCU mechanism. Now there's
> > kfree_rcu_mightsleep() which does not need the rcu_head and waits for
> > the grace period.
> > 
> > Sleeping is safe in this context and as this is a rare event it won't
> > interfere with the rest as it's holding the device_list_mutex.
> > 
> > Straightforward changes:
> > 
> > - rcu_string_strdup -> kstrdup
> > - rcu_str_deref -> rcu_dereference
> > - drop ->str from safe contexts
> > 
> > Historical notes:
> > 
> > Introduced in 606686eeac45 ("Btrfs: use rcu to protect device->name")
> > with a vague reference of the potential problem described in
> > https://lore.kernel.org/all/20120531155304.GF11775@ZenIV.linux.org.uk/ .
> > 
> > The RCU protection looks like the easiest and most lightweight way of
> > protecting the rare event of device rename racing device_list_add()
> > with a random printk() that uses the device name.
> > 
> > Alternatives: a spin lock would require to protect the printk
> > anyway, a fixed buffer for the name would be eventually wrong in case
> > the new name is overwritten when being printed, an array switching
> > pointers and cleaning them up eventually resembles RCU too much.
> > 
> > The cleanups up to this patch should hide special case of RCU to the
> > minimum that only the name needs rcu_dereference(), which can be further
> > cleaned up to use btrfs_dev_name().
> > 
> 
> There is still rcu warning when 'make  W=1 C=1'
> 
> /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21: warning: incorrect type in argument 1 (different address spaces)
> /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    expected void const *objp
> /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    got char const [noderef] __rcu *name
> 
> static void btrfs_free_device(struct btrfs_device *device)
> {
>     WARN_ON(!list_empty(&device->post_commit_list));
>     /* No need to call kfree_rcu(), nothing is reading the device name. */
> L405:    kfree(device->name);
> 
> do we need rcu_dereference here?
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -402,7 +402,7 @@ static void btrfs_free_device(struct btrfs_device *device)
>  {
>         WARN_ON(!list_empty(&device->post_commit_list));
>         /* No need to call kfree_rcu(), nothing is reading the device name. */
> -       kfree(device->name);
> +       kfree(rcu_dereference(device->name));

I got notified by the build bots (not CCed to the mailinglis) about
this. The dereference is not needed, the comment says why. The checkers
do not distinguish the context, some of them are safe like when the
device is being set up and not yet accessible by other processes, and at
deletion time, like here.

As we want to keep the __rcu annotation the rcu dereference is the
easiest workaround.

Re: [PATCH v2 1/2] btrfs: open code RCU for device name

[David Sterba]

On Mon, Jun 30, 2025 at 06:21:30PM +0200, David Sterba wrote:
> On Mon, Jun 30, 2025 at 10:24:57AM +0800, Wang Yugui wrote:
> > Hi,
> > 
> > > The RCU protected string is only used for a device name, and RCU is used
> > > so we can print the name and eventually synchronize against the rare
> > > device rename in device_list_add().
> > > 
> > > We don't need the whole API just for that. Open code all the helpers and
> > > access to the string itself.
> > > 
> > > Notable change is in device_list_add() when the device name is changed,
> > > which is the only place that can actually happen at the same time as
> > > message prints using the device name under RCU read lock.
> > > 
> > > Previously there was kfree_rcu() which used the embedded rcu_head to
> > > delay freeing the object depending on the RCU mechanism. Now there's
> > > kfree_rcu_mightsleep() which does not need the rcu_head and waits for
> > > the grace period.
> > > 
> > > Sleeping is safe in this context and as this is a rare event it won't
> > > interfere with the rest as it's holding the device_list_mutex.
> > > 
> > > Straightforward changes:
> > > 
> > > - rcu_string_strdup -> kstrdup
> > > - rcu_str_deref -> rcu_dereference
> > > - drop ->str from safe contexts
> > > 
> > > Historical notes:
> > > 
> > > Introduced in 606686eeac45 ("Btrfs: use rcu to protect device->name")
> > > with a vague reference of the potential problem described in
> > > https://lore.kernel.org/all/20120531155304.GF11775@ZenIV.linux.org.uk/ .
> > > 
> > > The RCU protection looks like the easiest and most lightweight way of
> > > protecting the rare event of device rename racing device_list_add()
> > > with a random printk() that uses the device name.
> > > 
> > > Alternatives: a spin lock would require to protect the printk
> > > anyway, a fixed buffer for the name would be eventually wrong in case
> > > the new name is overwritten when being printed, an array switching
> > > pointers and cleaning them up eventually resembles RCU too much.
> > > 
> > > The cleanups up to this patch should hide special case of RCU to the
> > > minimum that only the name needs rcu_dereference(), which can be further
> > > cleaned up to use btrfs_dev_name().
> > > 
> > 
> > There is still rcu warning when 'make  W=1 C=1'
> > 
> > /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21: warning: incorrect type in argument 1 (different address spaces)
> > /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    expected void const *objp
> > /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    got char const [noderef] __rcu *name
> > 
> > static void btrfs_free_device(struct btrfs_device *device)
> > {
> >     WARN_ON(!list_empty(&device->post_commit_list));
> >     /* No need to call kfree_rcu(), nothing is reading the device name. */
> > L405:    kfree(device->name);
> > 
> > do we need rcu_dereference here?
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -402,7 +402,7 @@ static void btrfs_free_device(struct btrfs_device *device)
> >  {
> >         WARN_ON(!list_empty(&device->post_commit_list));
> >         /* No need to call kfree_rcu(), nothing is reading the device name. */
> > -       kfree(device->name);
> > +       kfree(rcu_dereference(device->name));
> 
> I got notified by the build bots (not CCed to the mailinglis) about
> this. The dereference is not needed, the comment says why. The checkers
> do not distinguish the context, some of them are safe like when the
> device is being set up and not yet accessible by other processes, and at
> deletion time, like here.
> 
> As we want to keep the __rcu annotation the rcu dereference is the
> easiest workaround.

I can't seem to reproduce the warning with the command, I'm going to apply this
fixup:

--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -402,9 +402,17 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid)
 
 static void btrfs_free_device(struct btrfs_device *device)
 {
+       const char *name;
+
        WARN_ON(!list_empty(&device->post_commit_list));
-       /* No need to call kfree_rcu(), nothing is reading the device name. */
-       kfree(device->name);
+       /*
+        * No need to call kfree_rcu() or do RCU lock/unlock, nothing is
+        * reading the device name but the checkers complain.
+        */
+       rcu_read_lock();
+       name = rcu_dereference(device->name);
+       rcu_read_unlock();
+       kfree(name);
        btrfs_extent_io_tree_release(&device->alloc_state);
        btrfs_destroy_dev_zone_info(device);
        kfree(device);

Re: [PATCH v2 1/2] btrfs: open code RCU for device name

[Alan Huang]

On Jul 1, 2025, at 00:43, David Sterba <dsterba@suse.cz> wrote:
> 
> On Mon, Jun 30, 2025 at 06:21:30PM +0200, David Sterba wrote:
>> On Mon, Jun 30, 2025 at 10:24:57AM +0800, Wang Yugui wrote:
>>> Hi,
>>> 
>>>> The RCU protected string is only used for a device name, and RCU is used
>>>> so we can print the name and eventually synchronize against the rare
>>>> device rename in device_list_add().
>>>> 
>>>> We don't need the whole API just for that. Open code all the helpers and
>>>> access to the string itself.
>>>> 
>>>> Notable change is in device_list_add() when the device name is changed,
>>>> which is the only place that can actually happen at the same time as
>>>> message prints using the device name under RCU read lock.
>>>> 
>>>> Previously there was kfree_rcu() which used the embedded rcu_head to
>>>> delay freeing the object depending on the RCU mechanism. Now there's
>>>> kfree_rcu_mightsleep() which does not need the rcu_head and waits for
>>>> the grace period.
>>>> 
>>>> Sleeping is safe in this context and as this is a rare event it won't
>>>> interfere with the rest as it's holding the device_list_mutex.
>>>> 
>>>> Straightforward changes:
>>>> 
>>>> - rcu_string_strdup -> kstrdup
>>>> - rcu_str_deref -> rcu_dereference
>>>> - drop ->str from safe contexts
>>>> 
>>>> Historical notes:
>>>> 
>>>> Introduced in 606686eeac45 ("Btrfs: use rcu to protect device->name")
>>>> with a vague reference of the potential problem described in
>>>> https://lore.kernel.org/all/20120531155304.GF11775@ZenIV.linux.org.uk/ .
>>>> 
>>>> The RCU protection looks like the easiest and most lightweight way of
>>>> protecting the rare event of device rename racing device_list_add()
>>>> with a random printk() that uses the device name.
>>>> 
>>>> Alternatives: a spin lock would require to protect the printk
>>>> anyway, a fixed buffer for the name would be eventually wrong in case
>>>> the new name is overwritten when being printed, an array switching
>>>> pointers and cleaning them up eventually resembles RCU too much.
>>>> 
>>>> The cleanups up to this patch should hide special case of RCU to the
>>>> minimum that only the name needs rcu_dereference(), which can be further
>>>> cleaned up to use btrfs_dev_name().
>>>> 
>>> 
>>> There is still rcu warning when 'make  W=1 C=1'
>>> 
>>> /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21: warning: incorrect type in argument 1 (different address spaces)
>>> /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    expected void const *objp
>>> /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    got char const [noderef] __rcu *name
>>> 
>>> static void btrfs_free_device(struct btrfs_device *device)
>>> {
>>>    WARN_ON(!list_empty(&device->post_commit_list));
>>>    /* No need to call kfree_rcu(), nothing is reading the device name. */
>>> L405:    kfree(device->name);
>>> 
>>> do we need rcu_dereference here?
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -402,7 +402,7 @@ static void btrfs_free_device(struct btrfs_device *device)
>>> {
>>>        WARN_ON(!list_empty(&device->post_commit_list));
>>>        /* No need to call kfree_rcu(), nothing is reading the device name. */
>>> -       kfree(device->name);
>>> +       kfree(rcu_dereference(device->name));
>> 
>> I got notified by the build bots (not CCed to the mailinglis) about
>> this. The dereference is not needed, the comment says why. The checkers
>> do not distinguish the context, some of them are safe like when the
>> device is being set up and not yet accessible by other processes, and at
>> deletion time, like here.
>> 
>> As we want to keep the __rcu annotation the rcu dereference is the
>> easiest workaround.
> 
> I can't seem to reproduce the warning with the command, I'm going to apply this
> fixup:
> 
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -402,9 +402,17 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid)
> 
> static void btrfs_free_device(struct btrfs_device *device)
> {
> +       const char *name;
> +
>        WARN_ON(!list_empty(&device->post_commit_list));
> -       /* No need to call kfree_rcu(), nothing is reading the device name. */
> -       kfree(device->name);
> +       /*
> +        * No need to call kfree_rcu() or do RCU lock/unlock, nothing is
> +        * reading the device name but the checkers complain.
> +        */
> +       rcu_read_lock();
> +       name = rcu_dereference(device->name);

Since it’s safe here, can we use rcu_dereference_raw without  rcu_read_lock() ?

> +       rcu_read_unlock();
> +       kfree(name);
>        btrfs_extent_io_tree_release(&device->alloc_state);
>        btrfs_destroy_dev_zone_info(device);
>        kfree(device);
> 

Re: [PATCH v2 1/2] btrfs: open code RCU for device name

[David Sterba]

On Tue, Jul 01, 2025 at 01:07:06AM +0800, Alan Huang wrote:
> >        WARN_ON(!list_empty(&device->post_commit_list));
> > -       /* No need to call kfree_rcu(), nothing is reading the device name. */
> > -       kfree(device->name);
> > +       /*
> > +        * No need to call kfree_rcu() or do RCU lock/unlock, nothing is
> > +        * reading the device name but the checkers complain.
> > +        */
> > +       rcu_read_lock();
> > +       name = rcu_dereference(device->name);
> 
> Since it’s safe here, can we use rcu_dereference_raw without  rcu_read_lock() ?

Right, thanks for the hint.

Re: [PATCH v2 1/2] btrfs: open code RCU for device name

[Wang Yugui]

Hi,

> On Mon, Jun 30, 2025 at 06:21:30PM +0200, David Sterba wrote:
> > On Mon, Jun 30, 2025 at 10:24:57AM +0800, Wang Yugui wrote:
> > > Hi,
> > > 
> > > > The RCU protected string is only used for a device name, and RCU is used
> > > > so we can print the name and eventually synchronize against the rare
> > > > device rename in device_list_add().
> > > > 
> > > > We don't need the whole API just for that. Open code all the helpers and
> > > > access to the string itself.
> > > > 
> > > > Notable change is in device_list_add() when the device name is changed,
> > > > which is the only place that can actually happen at the same time as
> > > > message prints using the device name under RCU read lock.
> > > > 
> > > > Previously there was kfree_rcu() which used the embedded rcu_head to
> > > > delay freeing the object depending on the RCU mechanism. Now there's
> > > > kfree_rcu_mightsleep() which does not need the rcu_head and waits for
> > > > the grace period.
> > > > 
> > > > Sleeping is safe in this context and as this is a rare event it won't
> > > > interfere with the rest as it's holding the device_list_mutex.
> > > > 
> > > > Straightforward changes:
> > > > 
> > > > - rcu_string_strdup -> kstrdup
> > > > - rcu_str_deref -> rcu_dereference
> > > > - drop ->str from safe contexts
> > > > 
> > > > Historical notes:
> > > > 
> > > > Introduced in 606686eeac45 ("Btrfs: use rcu to protect device->name")
> > > > with a vague reference of the potential problem described in
> > > > https://lore.kernel.org/all/20120531155304.GF11775@ZenIV.linux.org.uk/ .
> > > > 
> > > > The RCU protection looks like the easiest and most lightweight way of
> > > > protecting the rare event of device rename racing device_list_add()
> > > > with a random printk() that uses the device name.
> > > > 
> > > > Alternatives: a spin lock would require to protect the printk
> > > > anyway, a fixed buffer for the name would be eventually wrong in case
> > > > the new name is overwritten when being printed, an array switching
> > > > pointers and cleaning them up eventually resembles RCU too much.
> > > > 
> > > > The cleanups up to this patch should hide special case of RCU to the
> > > > minimum that only the name needs rcu_dereference(), which can be further
> > > > cleaned up to use btrfs_dev_name().
> > > > 
> > > 
> > > There is still rcu warning when 'make  W=1 C=1'
> > > 
> > > /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21: warning: incorrect type in argument 1 (different address spaces)
> > > /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    expected void const *objp
> > > /usr/hpc-bio/linux-6.12.35/fs/btrfs/volumes.c:405:21:    got char const [noderef] __rcu *name
> > > 
> > > static void btrfs_free_device(struct btrfs_device *device)
> > > {
> > >     WARN_ON(!list_empty(&device->post_commit_list));
> > >     /* No need to call kfree_rcu(), nothing is reading the device name. */
> > > L405:    kfree(device->name);
> > > 
> > > do we need rcu_dereference here?
> > > --- a/fs/btrfs/volumes.c
> > > +++ b/fs/btrfs/volumes.c
> > > @@ -402,7 +402,7 @@ static void btrfs_free_device(struct btrfs_device *device)
> > >  {
> > >         WARN_ON(!list_empty(&device->post_commit_list));
> > >         /* No need to call kfree_rcu(), nothing is reading the device name. */
> > > -       kfree(device->name);
> > > +       kfree(rcu_dereference(device->name));
> > 
> > I got notified by the build bots (not CCed to the mailinglis) about
> > this. The dereference is not needed, the comment says why. The checkers
> > do not distinguish the context, some of them are safe like when the
> > device is being set up and not yet accessible by other processes, and at
> > deletion time, like here.
> > 
> > As we want to keep the __rcu annotation the rcu dereference is the
> > easiest workaround.
> 
> I can't seem to reproduce the warning with the command, I'm going to apply this
> fixup:

steps to reproduce the warning:
1) install https://kojipkgs.fedoraproject.org/packages/sparse/0.6.4/
2) make btrfs module(kernel 6.16) with the commands
#uname_r=$(uname -r)
uname_r=$(ls /boot/vmlinuz-6.16.* 2>/dev/null)
uname_r=${uname_r##/boot/vmlinuz-}
pwd_dir=$(pwd)
make -C /lib/modules/${uname_r}/build M=${pwd_dir} modules -j 20 W=1 C=1 CF="-Wnocontext"


Then we can see other 4 warnings.

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bb91a7b..5080ab2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -699,7 +699,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
 	if (!device->name)
 		return -EINVAL;
 
-	ret = btrfs_get_bdev_and_sb(device->name, flags, holder, 1,
+	ret = btrfs_get_bdev_and_sb(rcu_dereference(device->name), flags, holder, 1,
 				    &bdev_file, &disk_super);
 	if (ret)
 		return ret;
@@ -1061,7 +1061,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 		 * uuid mutex so nothing we touch in here is going to disappear.
 		 */
 		if (orig_dev->name)
-			dev_path = orig_dev->name;
+			dev_path = rcu_dereference(orig_dev->name);
 
 		device = btrfs_alloc_device(NULL, &orig_dev->devid,
 					    orig_dev->uuid, dev_path);
@@ -1436,7 +1436,7 @@ static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
 
 		list_for_each_entry(device, &fs_devices->devices, dev_list) {
 			if (device->bdev && (device->bdev->bd_dev == devt) &&
-			    strcmp(device->name, path) != 0) {
+			    strcmp(rcu_dereference(device->name), path) != 0) {
 				mutex_unlock(&fs_devices->device_list_mutex);
 
 				/* Do not skip registration. */
@@ -2197,7 +2197,7 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info, struct btrfs_devic
 	btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
 
 	/* Update ctime/mtime for device path for libblkid */
-	update_dev_time(device->name);
+	update_dev_time(rcu_dereference(device->name));
 }
 
 int btrfs_rm_device(struct btrfs_fs_info *fs_info,

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2025/07/02

Re: [PATCH v2 1/2] btrfs: open code RCU for device name

[David Sterba]

On Wed, Jul 02, 2025 at 07:29:47AM +0800, Wang Yugui wrote:
> > I can't seem to reproduce the warning with the command, I'm going to apply this
> > fixup:
> 
> steps to reproduce the warning:
> 1) install https://kojipkgs.fedoraproject.org/packages/sparse/0.6.4/
> 2) make btrfs module(kernel 6.16) with the commands
> #uname_r=$(uname -r)
> uname_r=$(ls /boot/vmlinuz-6.16.* 2>/dev/null)
> uname_r=${uname_r##/boot/vmlinuz-}
> pwd_dir=$(pwd)
> make -C /lib/modules/${uname_r}/build M=${pwd_dir} modules -j 20 W=1 C=1 CF="-Wnocontext"

I did try the W=1 C=1 build but there were no reports, even with update
sparse (which is what is run with bare C=1 I believe).

> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb91a7b..5080ab2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -699,7 +699,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
>  	if (!device->name)
>  		return -EINVAL;
>  
> -	ret = btrfs_get_bdev_and_sb(device->name, flags, holder, 1,
> +	ret = btrfs_get_bdev_and_sb(rcu_dereference(device->name), flags, holder, 1,
>  				    &bdev_file, &disk_super);
>  	if (ret)
>  		return ret;
> @@ -1061,7 +1061,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
>  		 * uuid mutex so nothing we touch in here is going to disappear.
>  		 */
>  		if (orig_dev->name)
> -			dev_path = orig_dev->name;
> +			dev_path = rcu_dereference(orig_dev->name);
>  
>  		device = btrfs_alloc_device(NULL, &orig_dev->devid,
>  					    orig_dev->uuid, dev_path);
> @@ -1436,7 +1436,7 @@ static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
>  
>  		list_for_each_entry(device, &fs_devices->devices, dev_list) {
>  			if (device->bdev && (device->bdev->bd_dev == devt) &&
> -			    strcmp(device->name, path) != 0) {
> +			    strcmp(rcu_dereference(device->name), path) != 0) {
>  				mutex_unlock(&fs_devices->device_list_mutex);
>  
>  				/* Do not skip registration. */
> @@ -2197,7 +2197,7 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info, struct btrfs_devic
>  	btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
>  
>  	/* Update ctime/mtime for device path for libblkid */
> -	update_dev_time(device->name);
> +	update_dev_time(rcu_dereference(device->name));
>  }

I've updated the patch, this is basically equivalent to the same
expression but does not report any RCU violation. This should be safe in
all cases so the _raw's are justified for this patchset. The handling of
device->name perhaps does not fit any common pattern for RCU so we can't
avoid it. The protection involves no mounted filesystem (the scanning
ioctl) and then access when the filesystem is mounted. We have the
uuid_mutex for serializing everything.

For linux-next, I've updated the patch to use rcu_dereference_raw(),
depending on when the branch is picked for merge there should be no
warnings anymore.

Re: [PATCH v2 1/2] btrfs: open code RCU for device name

[David Sterba]

On Fri, Jul 04, 2025 at 05:14:39AM +0200, David Sterba wrote:
> I've updated the patch, this is basically equivalent to the same
> expression but does not report any RCU violation. This should be safe in
> all cases so the _raw's are justified for this patchset. The handling of
> device->name perhaps does not fit any common pattern for RCU so we can't
> avoid it. The protection involves no mounted filesystem (the scanning
> ioctl) and then access when the filesystem is mounted. We have the
> uuid_mutex for serializing everything.
> 
> For linux-next, I've updated the patch to use rcu_dereference_raw(),
> depending on when the branch is picked for merge there should be no
> warnings anymore.

No new warnings reported.

[PATCH v2 2/2] btrfs: remove struct rcu_string

[David Sterba]

The only use for device name has been removed so we can kill the RCU
string API.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/rcu-string.h | 40 ----------------------------------------
 fs/btrfs/volumes.c    |  1 -
 fs/btrfs/volumes.h    |  1 -
 fs/btrfs/zoned.c      |  1 -
 4 files changed, 43 deletions(-)
 delete mode 100644 fs/btrfs/rcu-string.h

diff --git a/fs/btrfs/rcu-string.h b/fs/btrfs/rcu-string.h
deleted file mode 100644
index 70b1e19b50e6..000000000000
--- a/fs/btrfs/rcu-string.h
+++ /dev/null
@@ -1,40 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Copyright (C) 2012 Red Hat.  All rights reserved.
- */
-
-#ifndef BTRFS_RCU_STRING_H
-#define BTRFS_RCU_STRING_H
-
-#include <linux/types.h>
-#include <linux/string.h>
-#include <linux/slab.h>
-#include <linux/rcupdate.h>
-#include <linux/printk.h>
-
-struct rcu_string {
-	struct rcu_head rcu;
-	char str[];
-};
-
-static inline struct rcu_string *rcu_string_strdup(const char *src, gfp_t mask)
-{
-	size_t len = strlen(src) + 1;
-	struct rcu_string *ret = kzalloc(sizeof(struct rcu_string) +
-					 (len * sizeof(char)), mask);
-	if (!ret)
-		return ret;
-	/* Warn if the source got unexpectedly truncated. */
-	if (WARN_ON(strscpy(ret->str, src, len) < 0)) {
-		kfree(ret);
-		return NULL;
-	}
-	return ret;
-}
-
-#define rcu_str_deref(rcu_str) ({				\
-	struct rcu_string *__str = rcu_dereference(rcu_str);	\
-	__str->str;						\
-})
-
-#endif
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9ab2ed7cce8d..535c7bd1d076 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -18,7 +18,6 @@
 #include "transaction.h"
 #include "volumes.h"
 #include "raid56.h"
-#include "rcu-string.h"
 #include "dev-replace.h"
 #include "sysfs.h"
 #include "tree-checker.h"
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 149434e42f7a..945b562c1644 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -21,7 +21,6 @@
 #include <uapi/linux/btrfs.h>
 #include <uapi/linux/btrfs_tree.h>
 #include "messages.h"
-#include "rcu-string.h"
 #include "extent-io-tree.h"
 
 struct block_device;
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 27264db4b7ca..245e813ecd78 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -9,7 +9,6 @@
 #include "ctree.h"
 #include "volumes.h"
 #include "zoned.h"
-#include "rcu-string.h"
 #include "disk-io.h"
 #include "block-group.h"
 #include "dev-replace.h"
-- 
2.49.0

Re: [PATCH v2 0/2] Device name and RCU string

[Daniel Vacek]

On Wed, 25 Jun 2025 at 15:37, David Sterba <dsterba@suse.com> wrote:
>
> v2:
> - drop patch adding RCU protection around update_dev_time(), there's a
>   sleeping allocation inside; after revisiting the RCU requirements, I
>   don't think it's needed and we can access the raw string (the same way
>   it was done with rcu_string), the time update is called from scratch
>   superblocks and after the ->dev_list hook is removed, so there's no
>   chance it can be reached from device_list_add

Right, I missed that.

> After recent simplifications of the RCU usage in messages, this patchset
> implements the RCU protection directly without the RCU string so this
> API can be removed completely as we don't have nor plan anything else to
> use it for.
>
> David Sterba (2):
>   btrfs: open code RCU for device name
>   btrfs: remove struct rcu_string

The rest still looks good.
Reviewed-by: Daniel Vacek <neelx@suse.com>

>  fs/btrfs/rcu-string.h | 40 ----------------------------------------
>  fs/btrfs/volumes.c    | 29 +++++++++++++++++------------
>  fs/btrfs/volumes.h    |  6 +++---
>  fs/btrfs/zoned.c      | 23 +++++++++++------------
>  4 files changed, 31 insertions(+), 67 deletions(-)
>  delete mode 100644 fs/btrfs/rcu-string.h
>
> --
> 2.49.0
>
>