[PATCH 0/3] Device name and RCU string

[PATCH 0/3] Device name and RCU string

[David Sterba]

After recent simplifactions of the RCU usage in messages, this pathset
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 (3):
  btrfs: protect reading device name by RCU in update_dev_time()
  btrfs: open code RCU for device name
  btrfs: remove struct rcu_string

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

-- 
2.47.1

[PATCH 1/3] btrfs: protect reading device name by RCU in update_dev_time()

[David Sterba]

The device time update is called from places where the device is still
connected to the lists and could be potentially used for printing the
name. Add RCU read protection around kern_path() that makes a copy of
the input path. Change the parameter to struct btrfs_device so the RCU
section can be minimal.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 134b7754347e..a5fbbd6bb1ed 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1992,12 +1992,14 @@ static int btrfs_add_dev_item(struct btrfs_trans_handle *trans,
  *
  * We don't care about errors here, this is just to be kind to userspace.
  */
-static void update_dev_time(const char *device_path)
+static void update_dev_time(const struct btrfs_device *device)
 {
 	struct path path;
 	int ret;
 
-	ret = kern_path(device_path, LOOKUP_FOLLOW, &path);
+	rcu_read_lock();
+	ret = kern_path(rcu_dereference(device->name), LOOKUP_FOLLOW, &path);
+	rcu_read_unlock();
 	if (ret)
 		return;
 
@@ -2165,7 +2167,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);
 }
 
 int btrfs_rm_device(struct btrfs_fs_info *fs_info,
@@ -2892,7 +2894,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	btrfs_forget_devices(device->devt);
 
 	/* Update ctime/mtime for blkid or udev */
-	update_dev_time(device_path);
+	update_dev_time(device);
 
 	return ret;
 
-- 
2.47.1

[PATCH 2/3] 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 | 26 +++++++++++++++-----------
 fs/btrfs/volumes.h |  5 +++--
 fs/btrfs/zoned.c   | 22 +++++++++++-----------
 3 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a5fbbd6bb1ed..002a215176ca 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,15 @@ 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);
+		old_name = rcu_dereference(device->name);
 		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 +1017,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 +1419,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. */
@@ -2167,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);
+	update_dev_time(device;
 }
 
 int btrfs_rm_device(struct btrfs_fs_info *fs_info,
@@ -6927,9 +6931,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 6a2f9c73c685..cc270787d0ce 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;
 
@@ -847,7 +848,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.47.1

[PATCH 3/3] 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 002a215176ca..8cfd731f05ef 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 cc270787d0ce..16aaa25155dc 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.47.1

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

[Daniel Vacek]

On Wed, 18 Jun 2025 at 13:10, David Sterba <dsterba@suse.com> wrote:
>
> 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 | 26 +++++++++++++++-----------
>  fs/btrfs/volumes.h |  5 +++--
>  fs/btrfs/zoned.c   | 22 +++++++++++-----------
>  3 files changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a5fbbd6bb1ed..002a215176ca 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,15 @@ 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);
> +               old_name = rcu_dereference(device->name);
>                 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 +1017,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 +1419,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. */
> @@ -2167,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);
> +       update_dev_time(device;

This looks like a mistake. I believe the hunk should be removed,
otherwise it won't compile.

>  }
>
>  int btrfs_rm_device(struct btrfs_fs_info *fs_info,
> @@ -6927,9 +6931,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 6a2f9c73c685..cc270787d0ce 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;
>
> @@ -847,7 +848,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.47.1
>
>

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

[Daniel Vacek]

On Wed, 18 Jun 2025 at 13:09, David Sterba <dsterba@suse.com> wrote:
>
> After recent simplifactions of the RCU usage in messages, this pathset
> 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 (3):
>   btrfs: protect reading device name by RCU in update_dev_time()
>   btrfs: open code RCU for device name
>   btrfs: remove struct rcu_string

With the nit in the second patch, the series looks good to me.

Reviewed-by: Daniel Vacek <neelx@suse.com>

>  fs/btrfs/rcu-string.h | 40 ----------------------------------------
>  fs/btrfs/volumes.c    | 35 ++++++++++++++++++++---------------
>  fs/btrfs/volumes.h    |  6 +++---
>  fs/btrfs/zoned.c      | 23 +++++++++++------------
>  4 files changed, 34 insertions(+), 70 deletions(-)
>  delete mode 100644 fs/btrfs/rcu-string.h
>
> --
> 2.47.1
>
>

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

[David Sterba]

On Thu, Jun 19, 2025 at 12:15:25PM +0200, Daniel Vacek wrote:
> > @@ -2167,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);
> > +       update_dev_time(device;
> 
> This looks like a mistake. I believe the hunk should be removed,
> otherwise it won't compile.

Yes it's a mistake. I've checked how it got there, most likely after
reordering the patch moving RCU to update_dev_time, the parameter
changed from device->name to device.

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

[Filipe Manana]

On Fri, Jun 20, 2025 at 1:12 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Thu, Jun 19, 2025 at 12:15:25PM +0200, Daniel Vacek wrote:
> > > @@ -2167,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);
> > > +       update_dev_time(device;
> >
> > This looks like a mistake. I believe the hunk should be removed,
> > otherwise it won't compile.
>
> Yes it's a mistake. I've checked how it got there, most likely after
> reordering the patch moving RCU to update_dev_time, the parameter
> changed from device->name to device.

I think you also forgot to test the patchset.

After applying it (rebasing to latest for-next), running fstests the
following trace can be triggered in many tests such as btrfs/003:

[183075.918418] BUG: sleeping function called from invalid context at
./include/linux/sched/mm.h:321
[183075.919728] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid:
882479, name: btrfs
[183075.920839] preempt_count: 0, expected: 0
[183075.921421] RCU nest depth: 1, expected: 0
[183075.921987] CPU: 11 UID: 0 PID: 882479 Comm: btrfs Tainted: G
  W           6.16.0-rc2-btrfs-next-201+ #1 PREEMPT(full)
[183075.921990] Tainted: [W]=WARN
[183075.921991] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.16.2-0-gea1b7a073390-prebuilt.qemu.org 04/01/2014
[183075.921992] Call Trace:
[183075.921994]  <TASK>
[183075.921996]  dump_stack_lvl+0x56/0x80
[183075.921999]  __might_resched.cold+0xd6/0x10f
[183075.922002]  kmem_cache_alloc_noprof+0x2b5/0x3a0
[183075.922005]  ? btrfs_free_stale_devices+0x143/0x280 [btrfs]
[183075.922085]  getname_kernel+0x25/0x110
[183075.922087]  kern_path+0x13/0x40
[183075.922090]  update_dev_time+0x31/0x80 [btrfs]
[183075.922151]  btrfs_init_new_device+0xa3b/0x12c0 [btrfs]
[183075.922212]  ? __kmalloc_node_track_caller_noprof+0xba/0x490
[183075.922214]  ? _copy_from_user+0x49/0x80
[183075.922217]  btrfs_ioctl+0x2011/0x2660 [btrfs]
[183075.922277]  ? preempt_count_add+0x47/0xa0
[183075.922280]  ? __virt_addr_valid+0xe4/0x1a0
[183075.922282]  ? __check_object_size+0x1ca/0x1f0
[183075.922284]  ? mntput_no_expire+0x49/0x270
[183075.922287]  __x64_sys_ioctl+0x92/0xe0
[183075.922290]  do_syscall_64+0x50/0x1e0
[183075.922294]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[183075.922296] RIP: 0033:0x7f2cfe0388db

Also, the first patch alone breaks compilation:

  CC [M]  fs/btrfs/volumes.o
In file included from ./include/linux/rbtree.h:24,
                 from ./include/linux/mm_types.h:11,
                 from ./include/linux/sched/mm.h:8,
                 from fs/btrfs/volumes.c:7:
fs/btrfs/volumes.c: In function ‘update_dev_time’:
./include/linux/rcupdate.h:530:2: error: passing argument 1 of
‘kern_path’ from incompatible pointer type
[-Wincompatible-pointer-types]
  530 | ({ \
      | ~^~~
      |  |
      |  struct rcu_string *
  531 |         /* Dependency order vs. p above. */ \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  532 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  533 |         RCU_LOCKDEP_WARN(!(c), "suspicious
rcu_dereference_check() usage"); \
      |
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  534 |         rcu_check_sparse(p, space); \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  535 |         ((typeof(*p) __force __kernel *)(local)); \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  536 | })
      | ~~
./include/linux/rcupdate.h:680:9: note: in expansion of macro
‘__rcu_dereference_check’
  680 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
      |         ^~~~~~~~~~~~~~~~~~~~~~~
./include/linux/rcupdate.h:752:28: note: in expansion of macro
‘rcu_dereference_check’
  752 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
      |                            ^~~~~~~~~~~~~~~~~~~~~
fs/btrfs/volumes.c:2001:25: note: in expansion of macro ‘rcu_dereference’
 2001 |         ret = kern_path(rcu_dereference(device->name),
LOOKUP_FOLLOW, &path);
      |                         ^~~~~~~~~~~~~~~
In file included from fs/btrfs/volumes.c:14:
./include/linux/namei.h:59:22: note: expected ‘const char *’ but
argument is of type ‘struct rcu_string *’
   59 | extern int kern_path(const char *, unsigned, struct path *);
      |                      ^~~~~~~~~~~~
make[4]: *** [scripts/Makefile.build:287: fs/btrfs/volumes.o] Error 1
make[3]: *** [scripts/Makefile.build:554: fs/btrfs] Error 2
make[2]: *** [scripts/Makefile.build:554: fs] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/home/fdmanana/git/hub/btrfs-next/Makefile:2003: .] Error 2
make: *** [Makefile:248: __sub-make] Error 2

Thanks.


>

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

[David Sterba]

On Fri, Jun 20, 2025 at 06:01:30PM +0100, Filipe Manana wrote:
> On Fri, Jun 20, 2025 at 1:12 PM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Thu, Jun 19, 2025 at 12:15:25PM +0200, Daniel Vacek wrote:
> > > > @@ -2167,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);
> > > > +       update_dev_time(device;
> > >
> > > This looks like a mistake. I believe the hunk should be removed,
> > > otherwise it won't compile.
> >
> > Yes it's a mistake. I've checked how it got there, most likely after
> > reordering the patch moving RCU to update_dev_time, the parameter
> > changed from device->name to device.
> 
> I think you also forgot to test the patchset.
> 
> After applying it (rebasing to latest for-next), running fstests the
> following trace can be triggered in many tests such as btrfs/003:

Apparently yes, I don't know where I lost track of the patchset so I'll
redo it from start. Patches removed from for-next.