[PATCH v2 0/5] btrfs: use the super_block as bdev holder

[PATCH v2 0/5] btrfs: use the super_block as bdev holder

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

This is a series I've picked up from Christoph, it changes the
block_device's bdev holder from fs_type to the super block.

As the re-base was non trivial, I opted to drop Boris' Reviewed-by tags.

Here's the original cover letter:
Hi all,

this series contains the btrfs parts of the "remove get_super" from June
that managed to get lost.

I've dropped all the reviews from back then as the rebase against the new
mount API conversion led to a lot of non-trivial conflicts.

Josef kindly ran it through the CI farm and provided a fixup based on that.



Link to rebase v1:
https://lore.kernel.org/linux-btrfs/20240214-hch-device-open-v1-0-b153428b4f72@wdc.com/

Link to original posting:
https://lore.kernel.org/linux-btrfs/b083ae24-2273-479f-8c9e-96cb9ef083b8@wdc.com/

Christoph Hellwig (5):
  btrfs: always open the device read-only in btrfs_scan_one_device
  btrfs: call btrfs_close_devices from ->kill_sb
  btrfs: split btrfs_fs_devices.opened
  btrfs: open block devices after superblock creation
  btrfs: use the super_block as holder when mounting file systems

 fs/btrfs/disk-io.c |  4 +--
 fs/btrfs/super.c   | 70 +++++++++++++++++++++++++++-------------------
 fs/btrfs/volumes.c | 62 ++++++++++++++++++++--------------------
 fs/btrfs/volumes.h |  8 ++++--
 4 files changed, 80 insertions(+), 64 deletions(-)

-- 
2.49.0

[PATCH v2 1/5] btrfs: always open the device read-only in btrfs_scan_one_device

[Johannes Thumshirn]

From: Christoph Hellwig <hch@lst.de>

btrfs_scan_one_device opens the block device only to read the super
block.  Instead of passing a blk_mode_t argument to sometimes open
it for writing, just hard code BLK_OPEN_READ as it will never write
to the device or hand the block_device out to someone else.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/super.c   | 9 ++++-----
 fs/btrfs/volumes.c | 4 ++--
 fs/btrfs/volumes.h | 2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 2d0d8c6e77b4..b9e08a59da4e 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -364,10 +364,9 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		break;
 	case Opt_device: {
 		struct btrfs_device *device;
-		blk_mode_t mode = btrfs_open_mode(fc);
 
 		mutex_lock(&uuid_mutex);
-		device = btrfs_scan_one_device(param->string, mode, false);
+		device = btrfs_scan_one_device(param->string, false);
 		mutex_unlock(&uuid_mutex);
 		if (IS_ERR(device))
 			return PTR_ERR(device);
@@ -1855,7 +1854,7 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 	 * With 'true' passed to btrfs_scan_one_device() (mount time) we expect
 	 * either a valid device or an error.
 	 */
-	device = btrfs_scan_one_device(fc->source, mode, true);
+	device = btrfs_scan_one_device(fc->source, true);
 	ASSERT(device != NULL);
 	if (IS_ERR(device)) {
 		mutex_unlock(&uuid_mutex);
@@ -2233,7 +2232,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 		 * Scanning outside of mount can return NULL which would turn
 		 * into 0 error code.
 		 */
-		device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ, false);
+		device = btrfs_scan_one_device(vol->name, false);
 		ret = PTR_ERR_OR_ZERO(device);
 		mutex_unlock(&uuid_mutex);
 		break;
@@ -2251,7 +2250,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 		 * Scanning outside of mount can return NULL which would turn
 		 * into 0 error code.
 		 */
-		device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ, false);
+		device = btrfs_scan_one_device(vol->name, false);
 		if (IS_ERR_OR_NULL(device)) {
 			mutex_unlock(&uuid_mutex);
 			if (IS_ERR(device))
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1535a425e8f9..1ebfc69012a2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1440,7 +1440,7 @@ static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
  * the device or return an error. Multi-device and seeding devices are registered
  * in both cases.
  */
-struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
+struct btrfs_device *btrfs_scan_one_device(const char *path,
 					   bool mount_arg_dev)
 {
 	struct btrfs_super_block *disk_super;
@@ -1461,7 +1461,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 	 * values temporarily, as the device paths of the fsid are the only
 	 * required information for assembling the volume.
 	 */
-	bdev_file = bdev_file_open_by_path(path, flags, NULL, NULL);
+	bdev_file = bdev_file_open_by_path(path, BLK_OPEN_READ, NULL, NULL);
 	if (IS_ERR(bdev_file))
 		return ERR_CAST(bdev_file);
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 6d8b1f38e3ee..afa71d315c46 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -719,7 +719,7 @@ struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
 void btrfs_mapping_tree_free(struct btrfs_fs_info *fs_info);
 int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       blk_mode_t flags, void *holder);
-struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
+struct btrfs_device *btrfs_scan_one_device(const char *path,
 					   bool mount_arg_dev);
 int btrfs_forget_devices(dev_t devt);
 void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
-- 
2.49.0

Re: [PATCH v2 1/5] btrfs: always open the device read-only in btrfs_scan_one_device

[Qu Wenruo]

在 2025/6/11 19:32, Johannes Thumshirn 写道:
> From: Christoph Hellwig <hch@lst.de>
> 
> btrfs_scan_one_device opens the block device only to read the super
> block.  Instead of passing a blk_mode_t argument to sometimes open
> it for writing, just hard code BLK_OPEN_READ as it will never write
> to the device or hand the block_device out to someone else.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/super.c   | 9 ++++-----
>   fs/btrfs/volumes.c | 4 ++--
>   fs/btrfs/volumes.h | 2 +-
>   3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 2d0d8c6e77b4..b9e08a59da4e 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -364,10 +364,9 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>   		break;
>   	case Opt_device: {
>   		struct btrfs_device *device;
> -		blk_mode_t mode = btrfs_open_mode(fc);
>   
>   		mutex_lock(&uuid_mutex);
> -		device = btrfs_scan_one_device(param->string, mode, false);
> +		device = btrfs_scan_one_device(param->string, false);
>   		mutex_unlock(&uuid_mutex);
>   		if (IS_ERR(device))
>   			return PTR_ERR(device);
> @@ -1855,7 +1854,7 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>   	 * With 'true' passed to btrfs_scan_one_device() (mount time) we expect
>   	 * either a valid device or an error.
>   	 */
> -	device = btrfs_scan_one_device(fc->source, mode, true);
> +	device = btrfs_scan_one_device(fc->source, true);
>   	ASSERT(device != NULL);
>   	if (IS_ERR(device)) {
>   		mutex_unlock(&uuid_mutex);
> @@ -2233,7 +2232,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>   		 * Scanning outside of mount can return NULL which would turn
>   		 * into 0 error code.
>   		 */
> -		device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ, false);
> +		device = btrfs_scan_one_device(vol->name, false);
>   		ret = PTR_ERR_OR_ZERO(device);
>   		mutex_unlock(&uuid_mutex);
>   		break;
> @@ -2251,7 +2250,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>   		 * Scanning outside of mount can return NULL which would turn
>   		 * into 0 error code.
>   		 */
> -		device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ, false);
> +		device = btrfs_scan_one_device(vol->name, false);
>   		if (IS_ERR_OR_NULL(device)) {
>   			mutex_unlock(&uuid_mutex);
>   			if (IS_ERR(device))
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1535a425e8f9..1ebfc69012a2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1440,7 +1440,7 @@ static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
>    * the device or return an error. Multi-device and seeding devices are registered
>    * in both cases.
>    */
> -struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
> +struct btrfs_device *btrfs_scan_one_device(const char *path,
>   					   bool mount_arg_dev)
>   {
>   	struct btrfs_super_block *disk_super;
> @@ -1461,7 +1461,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>   	 * values temporarily, as the device paths of the fsid are the only
>   	 * required information for assembling the volume.
>   	 */
> -	bdev_file = bdev_file_open_by_path(path, flags, NULL, NULL);
> +	bdev_file = bdev_file_open_by_path(path, BLK_OPEN_READ, NULL, NULL);
>   	if (IS_ERR(bdev_file))
>   		return ERR_CAST(bdev_file);
>   
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 6d8b1f38e3ee..afa71d315c46 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -719,7 +719,7 @@ struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
>   void btrfs_mapping_tree_free(struct btrfs_fs_info *fs_info);
>   int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>   		       blk_mode_t flags, void *holder);
> -struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
> +struct btrfs_device *btrfs_scan_one_device(const char *path,
>   					   bool mount_arg_dev);
>   int btrfs_forget_devices(dev_t devt);
>   void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);

[PATCH v2 2/5] btrfs: call btrfs_close_devices from ->kill_sb

[Johannes Thumshirn]

From: Christoph Hellwig <hch@lst.de>

blkdev_put must not be called under sb->s_umount to avoid a lock order
reversal with disk->open_mutex once call backs from block devices to
the file system using the holder ops are supported.  Move the call
to btrfs_close_devices into btrfs_free_fs_info so that it is closed
from ->kill_sb (which is also called from the mount failure handling
path unlike ->put_super) as well as when an fs_info is freed because
an existing superblock already exists.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/disk-io.c |  4 ++--
 fs/btrfs/super.c   | 29 ++++++++++++++++-------------
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0d6ad7512f21..6360d44acaa9 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1247,6 +1247,8 @@ void btrfs_free_fs_info(struct btrfs_fs_info *fs_info)
 	struct percpu_counter *em_counter = &fs_info->evictable_extent_maps;
 
 	percpu_counter_destroy(&fs_info->stats_read_blocks);
+	if (fs_info->fs_devices)
+		btrfs_close_devices(fs_info->fs_devices);
 	percpu_counter_destroy(&fs_info->dirty_metadata_bytes);
 	percpu_counter_destroy(&fs_info->delalloc_bytes);
 	percpu_counter_destroy(&fs_info->ordered_bytes);
@@ -3681,7 +3683,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 
 	iput(fs_info->btree_inode);
 fail:
-	btrfs_close_devices(fs_info->fs_devices);
 	ASSERT(ret < 0);
 	return ret;
 }
@@ -4428,7 +4429,6 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	iput(fs_info->btree_inode);
 
 	btrfs_mapping_tree_free(fs_info);
-	btrfs_close_devices(fs_info->fs_devices);
 }
 
 void btrfs_mark_buffer_dirty(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index b9e08a59da4e..eaecf1525078 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1869,10 +1869,8 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 	if (ret)
 		return ret;
 
-	if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0) {
-		ret = -EACCES;
-		goto error;
-	}
+	if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0)
+		return -EACCES;
 
 	bdev = fs_devices->latest_dev->bdev;
 
@@ -1886,21 +1884,20 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 	 * otherwise it's tied to the lifetime of the super_block.
 	 */
 	sb = sget_fc(fc, btrfs_fc_test_super, set_anon_super_fc);
-	if (IS_ERR(sb)) {
-		ret = PTR_ERR(sb);
-		goto error;
-	}
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
 
 	set_device_specific_options(fs_info);
 
 	if (sb->s_root) {
-		btrfs_close_devices(fs_devices);
 		/*
 		 * At this stage we may have RO flag mismatch between
 		 * fc->sb_flags and sb->s_flags.  Caller should detect such
 		 * mismatch and reconfigure with sb->s_umount rwsem held if
 		 * needed.
 		 */
+		if ((fc->sb_flags ^ sb->s_flags) & SB_RDONLY)
+			ret = -EBUSY;
 	} else {
 		snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
 		shrinker_debugfs_rename(sb->s_shrink, "sb-btrfs:%s", sb->s_id);
@@ -1916,10 +1913,6 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 
 	fc->root = dget(sb->s_root);
 	return 0;
-
-error:
-	btrfs_close_devices(fs_devices);
-	return ret;
 }
 
 /*
@@ -1997,8 +1990,18 @@ static int btrfs_get_tree_super(struct fs_context *fc)
  */
 static int btrfs_reconfigure_for_mount(struct fs_context *fc)
 {
+	struct btrfs_fs_info *fs_info = fc->s_fs_info;
 	int ret = 0;
 
+	/*
+	 * We got a reference to our fs_devices, so we need to close it here to
+	 * make sure we don't leak our reference on the fs_devices.
+	 */
+	if (fs_info->fs_devices) {
+		btrfs_close_devices(fs_info->fs_devices);
+		fs_info->fs_devices = NULL;
+	}
+
 	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
 		ret = btrfs_reconfigure(fc);
 
-- 
2.49.0

Re: [PATCH v2 2/5] btrfs: call btrfs_close_devices from ->kill_sb

[Qu Wenruo]

在 2025/6/11 19:33, Johannes Thumshirn 写道:
> From: Christoph Hellwig <hch@lst.de>
> 
> blkdev_put must not be called under sb->s_umount

Btrfs doesn't utilize blkdev_put(), it only calls regular fput() which 
do delayed cleanup.

So you may want to apply this patch first:

https://lore.kernel.org/linux-btrfs/eaf6ba3672a82ba940678d269ab16f8a702eaf32.1749446257.git.wqu@suse.com/T/#u

> to avoid a lock order
> reversal with disk->open_mutex once call backs from block devices to
> the file system using the holder ops are supported.  Move the call
> to btrfs_close_devices into btrfs_free_fs_info so that it is closed
> from ->kill_sb (which is also called from the mount failure handling
> path unlike ->put_super) as well as when an fs_info is freed because
> an existing superblock already exists.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>   fs/btrfs/disk-io.c |  4 ++--
>   fs/btrfs/super.c   | 29 ++++++++++++++++-------------
>   2 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 0d6ad7512f21..6360d44acaa9 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1247,6 +1247,8 @@ void btrfs_free_fs_info(struct btrfs_fs_info *fs_info)
>   	struct percpu_counter *em_counter = &fs_info->evictable_extent_maps;
>   
>   	percpu_counter_destroy(&fs_info->stats_read_blocks);
> +	if (fs_info->fs_devices)
> +		btrfs_close_devices(fs_info->fs_devices);
>   	percpu_counter_destroy(&fs_info->dirty_metadata_bytes);
>   	percpu_counter_destroy(&fs_info->delalloc_bytes);
>   	percpu_counter_destroy(&fs_info->ordered_bytes);
> @@ -3681,7 +3683,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>   
>   	iput(fs_info->btree_inode);
>   fail:
> -	btrfs_close_devices(fs_info->fs_devices);
>   	ASSERT(ret < 0);
>   	return ret;
>   }
> @@ -4428,7 +4429,6 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
>   	iput(fs_info->btree_inode);
>   
>   	btrfs_mapping_tree_free(fs_info);
> -	btrfs_close_devices(fs_info->fs_devices);
>   }
>   
>   void btrfs_mark_buffer_dirty(struct btrfs_trans_handle *trans,
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index b9e08a59da4e..eaecf1525078 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1869,10 +1869,8 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>   	if (ret)
>   		return ret;
>   
> -	if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0) {
> -		ret = -EACCES;
> -		goto error;
> -	}
> +	if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0)
> +		return -EACCES;
>   
>   	bdev = fs_devices->latest_dev->bdev;
>   
> @@ -1886,21 +1884,20 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>   	 * otherwise it's tied to the lifetime of the super_block.
>   	 */
>   	sb = sget_fc(fc, btrfs_fc_test_super, set_anon_super_fc);
> -	if (IS_ERR(sb)) {
> -		ret = PTR_ERR(sb);
> -		goto error;
> -	}
> +	if (IS_ERR(sb))
> +		return PTR_ERR(sb);
>   
>   	set_device_specific_options(fs_info);
>   
>   	if (sb->s_root) {
> -		btrfs_close_devices(fs_devices);
>   		/*
>   		 * At this stage we may have RO flag mismatch between
>   		 * fc->sb_flags and sb->s_flags.  Caller should detect such
>   		 * mismatch and reconfigure with sb->s_umount rwsem held if
>   		 * needed.
>   		 */
> +		if ((fc->sb_flags ^ sb->s_flags) & SB_RDONLY)
> +			ret = -EBUSY;
>   	} else {
>   		snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
>   		shrinker_debugfs_rename(sb->s_shrink, "sb-btrfs:%s", sb->s_id);
> @@ -1916,10 +1913,6 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>   
>   	fc->root = dget(sb->s_root);
>   	return 0;
> -
> -error:
> -	btrfs_close_devices(fs_devices);
> -	return ret;
>   }
>   
>   /*
> @@ -1997,8 +1990,18 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>    */
>   static int btrfs_reconfigure_for_mount(struct fs_context *fc)
>   {
> +	struct btrfs_fs_info *fs_info = fc->s_fs_info;
>   	int ret = 0;
>   
> +	/*
> +	 * We got a reference to our fs_devices, so we need to close it here to
> +	 * make sure we don't leak our reference on the fs_devices.
> +	 */
> +	if (fs_info->fs_devices) {
> +		btrfs_close_devices(fs_info->fs_devices);
> +		fs_info->fs_devices = NULL;
> +	}
> +

This is what I didn't get, there is really nothing special for 
btrfs_reconfigure_for_mount() nowadays, we always go into this function 
and I didn't see any special reason why we always want to close all 
devices here.

Mind to explain it more like where the reference is from, and why it can 
be leaked?

Thanks,
Qu

>   	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
>   		ret = btrfs_reconfigure(fc);
>   

[PATCH v2 3/5] btrfs: split btrfs_fs_devices.opened

[Johannes Thumshirn]

From: Christoph Hellwig <hch@lst.de>

The btrfs_fs_devices.opened member mixes an in use counter for the
fs_devices structure that prevents it from being garbage collected with
a flag if the underlying devices were actually opened.  This not only
makes the code hard to follow, but also prevents btrfs from switching
to opening the block device only after super block creation.  Split it
into an in_use counter and an is_open boolean flag instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/volumes.c | 53 ++++++++++++++++++++++++++--------------------
 fs/btrfs/volumes.h |  6 ++++--
 2 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1ebfc69012a2..00b64f98e3bd 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -413,7 +413,8 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_device *device;
 
-	WARN_ON(fs_devices->opened);
+	WARN_ON_ONCE(fs_devices->in_use);
+	WARN_ON_ONCE(fs_devices->is_open);
 	while (!list_empty(&fs_devices->devices)) {
 		device = list_first_entry(&fs_devices->devices,
 					  struct btrfs_device, dev_list);
@@ -541,7 +542,7 @@ static int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device
 				continue;
 			if (devt && devt != device->devt)
 				continue;
-			if (fs_devices->opened) {
+			if (fs_devices->in_use) {
 				if (devt)
 					ret = -EBUSY;
 				break;
@@ -613,7 +614,7 @@ static struct btrfs_fs_devices *find_fsid_by_device(
 	if (found_by_devt) {
 		/* Existing device. */
 		if (fsid_fs_devices == NULL) {
-			if (devt_fs_devices->opened == 0) {
+			if (devt_fs_devices->in_use == 0) {
 				/* Stale device. */
 				return NULL;
 			} else {
@@ -848,7 +849,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	if (!device) {
 		unsigned int nofs_flag;
 
-		if (fs_devices->opened) {
+		if (fs_devices->in_use) {
 			btrfs_err(NULL,
 "device %s (%d:%d) belongs to fsid %pU, and the fs is already mounted, scanned by %s (%d)",
 				  path, MAJOR(path_devt), MINOR(path_devt),
@@ -916,7 +917,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		 * tracking a problem where systems fail mount by subvolume id
 		 * when we reject replacement on a mounted FS.
 		 */
-		if (!fs_devices->opened && found_transid < device->generation) {
+		if (!fs_devices->in_use && found_transid < device->generation) {
 			/*
 			 * That is if the FS is _not_ mounted and if you
 			 * are here, that means there is more than one
@@ -977,7 +978,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	 * it back. We need it to pick the disk with largest generation
 	 * (as above).
 	 */
-	if (!fs_devices->opened) {
+	if (!fs_devices->in_use) {
 		device->generation = found_transid;
 		fs_devices->latest_generation = max_t(u64, found_transid,
 						fs_devices->latest_generation);
@@ -1177,15 +1178,19 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
 
 	lockdep_assert_held(&uuid_mutex);
 
-	if (--fs_devices->opened > 0)
+	if (--fs_devices->in_use > 0)
 		return;
 
+	if (!fs_devices->is_open)
+		goto done;
+
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list)
 		btrfs_close_one_device(device);
 
 	WARN_ON(fs_devices->open_devices);
 	WARN_ON(fs_devices->rw_devices);
-	fs_devices->opened = 0;
+	fs_devices->is_open = false;
+done:
 	fs_devices->seeding = false;
 	fs_devices->fs_info = NULL;
 }
@@ -1197,7 +1202,7 @@ void btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 
 	mutex_lock(&uuid_mutex);
 	close_fs_devices(fs_devices);
-	if (!fs_devices->opened) {
+	if (!fs_devices->in_use) {
 		list_splice_init(&fs_devices->seed_list, &list);
 
 		/*
@@ -1253,7 +1258,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 		return -EINVAL;
 	}
 
-	fs_devices->opened = 1;
+	fs_devices->is_open = true;
 	fs_devices->latest_dev = latest_dev;
 	fs_devices->total_rw_bytes = 0;
 	fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_REGULAR;
@@ -1306,16 +1311,14 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 	 * We also don't need the lock here as this is called during mount and
 	 * exclusion is provided by uuid_mutex
 	 */
-
-	if (fs_devices->opened) {
-		fs_devices->opened++;
-		ret = 0;
-	} else {
+	if (!fs_devices->is_open) {
 		list_sort(NULL, &fs_devices->devices, devid_cmp);
 		ret = open_fs_devices(fs_devices, flags, holder);
+		if (ret)
+			return ret;
 	}
-
-	return ret;
+	fs_devices->in_use++;
+	return 0;
 }
 
 void btrfs_release_disk_super(struct btrfs_super_block *super)
@@ -1407,7 +1410,7 @@ static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
 
 		mutex_lock(&fs_devices->device_list_mutex);
 
-		if (!fs_devices->opened) {
+		if (!fs_devices->is_open) {
 			mutex_unlock(&fs_devices->device_list_mutex);
 			continue;
 		}
@@ -2314,13 +2317,14 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
 	 * This can happen if cur_devices is the private seed devices list.  We
 	 * cannot call close_fs_devices() here because it expects the uuid_mutex
 	 * to be held, but in fact we don't need that for the private
-	 * seed_devices, we can simply decrement cur_devices->opened and then
+	 * seed_devices, we can simply decrement cur_devices->in_use and then
 	 * remove it from our list and free the fs_devices.
 	 */
 	if (cur_devices->num_devices == 0) {
 		list_del_init(&cur_devices->seed_list);
-		ASSERT(cur_devices->opened == 1, "opened=%d", cur_devices->opened);
-		cur_devices->opened--;
+		ASSERT(cur_devices->in_use == 1, "opened=%d", cur_devices->in_use);
+		cur_devices->in_use--;
+		cur_devices->is_open = false;
 		free_fs_devices(cur_devices);
 	}
 
@@ -2549,7 +2553,8 @@ static struct btrfs_fs_devices *btrfs_init_sprout(struct btrfs_fs_info *fs_info)
 	list_add(&old_devices->fs_list, &fs_uuids);
 
 	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
-	seed_devices->opened = 1;
+	seed_devices->in_use = 1;
+	seed_devices->is_open = true;
 	INIT_LIST_HEAD(&seed_devices->devices);
 	INIT_LIST_HEAD(&seed_devices->alloc_list);
 	mutex_init(&seed_devices->device_list_mutex);
@@ -7162,7 +7167,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 			return fs_devices;
 
 		fs_devices->seeding = true;
-		fs_devices->opened = 1;
+		fs_devices->in_use = 1;
+		fs_devices->is_open = true;
 		return fs_devices;
 	}
 
@@ -7179,6 +7185,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 		free_fs_devices(fs_devices);
 		return ERR_PTR(ret);
 	}
+	fs_devices->in_use = 1;
 
 	if (!fs_devices->seeding) {
 		close_fs_devices(fs_devices);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index afa71d315c46..06cf8c99befe 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -419,8 +419,10 @@ struct btrfs_fs_devices {
 
 	struct list_head seed_list;
 
-	/* Count fs-devices opened. */
-	int opened;
+	/* Count if fs_device is in used. */
+	unsigned int in_use;
+	/* True if the devices were opened. */
+	bool is_open;
 
 	/* Set when we find or add a device that doesn't have the nonrot flag set. */
 	bool rotating;
-- 
2.49.0

Re: [PATCH v2 3/5] btrfs: split btrfs_fs_devices.opened

[Qu Wenruo]

在 2025/6/11 19:33, Johannes Thumshirn 写道:
> From: Christoph Hellwig <hch@lst.de>
> 
> The btrfs_fs_devices.opened member mixes an in use counter for the
> fs_devices structure that prevents it from being garbage collected with
> a flag if the underlying devices were actually opened.  This not only
> makes the code hard to follow, but also prevents btrfs from switching
> to opening the block device only after super block creation.  Split it
> into an in_use counter and an is_open boolean flag instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/volumes.c | 53 ++++++++++++++++++++++++++--------------------
>   fs/btrfs/volumes.h |  6 ++++--
>   2 files changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1ebfc69012a2..00b64f98e3bd 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -413,7 +413,8 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
>   {
>   	struct btrfs_device *device;
>   
> -	WARN_ON(fs_devices->opened);
> +	WARN_ON_ONCE(fs_devices->in_use);
> +	WARN_ON_ONCE(fs_devices->is_open);
>   	while (!list_empty(&fs_devices->devices)) {
>   		device = list_first_entry(&fs_devices->devices,
>   					  struct btrfs_device, dev_list);
> @@ -541,7 +542,7 @@ static int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device
>   				continue;
>   			if (devt && devt != device->devt)
>   				continue;
> -			if (fs_devices->opened) {
> +			if (fs_devices->in_use) {
>   				if (devt)
>   					ret = -EBUSY;
>   				break;
> @@ -613,7 +614,7 @@ static struct btrfs_fs_devices *find_fsid_by_device(
>   	if (found_by_devt) {
>   		/* Existing device. */
>   		if (fsid_fs_devices == NULL) {
> -			if (devt_fs_devices->opened == 0) {
> +			if (devt_fs_devices->in_use == 0) {
>   				/* Stale device. */
>   				return NULL;
>   			} else {
> @@ -848,7 +849,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   	if (!device) {
>   		unsigned int nofs_flag;
>   
> -		if (fs_devices->opened) {
> +		if (fs_devices->in_use) {
>   			btrfs_err(NULL,
>   "device %s (%d:%d) belongs to fsid %pU, and the fs is already mounted, scanned by %s (%d)",
>   				  path, MAJOR(path_devt), MINOR(path_devt),
> @@ -916,7 +917,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   		 * tracking a problem where systems fail mount by subvolume id
>   		 * when we reject replacement on a mounted FS.
>   		 */
> -		if (!fs_devices->opened && found_transid < device->generation) {
> +		if (!fs_devices->in_use && found_transid < device->generation) {
>   			/*
>   			 * That is if the FS is _not_ mounted and if you
>   			 * are here, that means there is more than one
> @@ -977,7 +978,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   	 * it back. We need it to pick the disk with largest generation
>   	 * (as above).
>   	 */
> -	if (!fs_devices->opened) {
> +	if (!fs_devices->in_use) {
>   		device->generation = found_transid;
>   		fs_devices->latest_generation = max_t(u64, found_transid,
>   						fs_devices->latest_generation);
> @@ -1177,15 +1178,19 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
>   
>   	lockdep_assert_held(&uuid_mutex);
>   
> -	if (--fs_devices->opened > 0)
> +	if (--fs_devices->in_use > 0)
>   		return;
>   
> +	if (!fs_devices->is_open)
> +		goto done;
> +
>   	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list)
>   		btrfs_close_one_device(device);
>   
>   	WARN_ON(fs_devices->open_devices);
>   	WARN_ON(fs_devices->rw_devices);
> -	fs_devices->opened = 0;
> +	fs_devices->is_open = false;
> +done:
>   	fs_devices->seeding = false;
>   	fs_devices->fs_info = NULL;
>   }
> @@ -1197,7 +1202,7 @@ void btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
>   
>   	mutex_lock(&uuid_mutex);
>   	close_fs_devices(fs_devices);
> -	if (!fs_devices->opened) {
> +	if (!fs_devices->in_use) {
>   		list_splice_init(&fs_devices->seed_list, &list);
>   
>   		/*
> @@ -1253,7 +1258,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>   		return -EINVAL;
>   	}
>   
> -	fs_devices->opened = 1;
> +	fs_devices->is_open = true;
>   	fs_devices->latest_dev = latest_dev;
>   	fs_devices->total_rw_bytes = 0;
>   	fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_REGULAR;
> @@ -1306,16 +1311,14 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>   	 * We also don't need the lock here as this is called during mount and
>   	 * exclusion is provided by uuid_mutex
>   	 */
> -
> -	if (fs_devices->opened) {
> -		fs_devices->opened++;
> -		ret = 0;
> -	} else {
> +	if (!fs_devices->is_open) {
>   		list_sort(NULL, &fs_devices->devices, devid_cmp);
>   		ret = open_fs_devices(fs_devices, flags, holder);
> +		if (ret)
> +			return ret;
>   	}
> -
> -	return ret;
> +	fs_devices->in_use++;
> +	return 0;
>   }
>   
>   void btrfs_release_disk_super(struct btrfs_super_block *super)
> @@ -1407,7 +1410,7 @@ static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
>   
>   		mutex_lock(&fs_devices->device_list_mutex);
>   
> -		if (!fs_devices->opened) {
> +		if (!fs_devices->is_open) {
>   			mutex_unlock(&fs_devices->device_list_mutex);
>   			continue;
>   		}
> @@ -2314,13 +2317,14 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
>   	 * This can happen if cur_devices is the private seed devices list.  We
>   	 * cannot call close_fs_devices() here because it expects the uuid_mutex
>   	 * to be held, but in fact we don't need that for the private
> -	 * seed_devices, we can simply decrement cur_devices->opened and then
> +	 * seed_devices, we can simply decrement cur_devices->in_use and then
>   	 * remove it from our list and free the fs_devices.
>   	 */
>   	if (cur_devices->num_devices == 0) {
>   		list_del_init(&cur_devices->seed_list);
> -		ASSERT(cur_devices->opened == 1, "opened=%d", cur_devices->opened);
> -		cur_devices->opened--;
> +		ASSERT(cur_devices->in_use == 1, "opened=%d", cur_devices->in_use);
> +		cur_devices->in_use--;
> +		cur_devices->is_open = false;
>   		free_fs_devices(cur_devices);
>   	}
>   
> @@ -2549,7 +2553,8 @@ static struct btrfs_fs_devices *btrfs_init_sprout(struct btrfs_fs_info *fs_info)
>   	list_add(&old_devices->fs_list, &fs_uuids);
>   
>   	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
> -	seed_devices->opened = 1;
> +	seed_devices->in_use = 1;
> +	seed_devices->is_open = true;
>   	INIT_LIST_HEAD(&seed_devices->devices);
>   	INIT_LIST_HEAD(&seed_devices->alloc_list);
>   	mutex_init(&seed_devices->device_list_mutex);
> @@ -7162,7 +7167,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
>   			return fs_devices;
>   
>   		fs_devices->seeding = true;
> -		fs_devices->opened = 1;
> +		fs_devices->in_use = 1;
> +		fs_devices->is_open = true;
>   		return fs_devices;
>   	}
>   
> @@ -7179,6 +7185,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
>   		free_fs_devices(fs_devices);
>   		return ERR_PTR(ret);
>   	}
> +	fs_devices->in_use = 1;
>   
>   	if (!fs_devices->seeding) {
>   		close_fs_devices(fs_devices);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index afa71d315c46..06cf8c99befe 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -419,8 +419,10 @@ struct btrfs_fs_devices {
>   
>   	struct list_head seed_list;
>   
> -	/* Count fs-devices opened. */
> -	int opened;
> +	/* Count if fs_device is in used. */
> +	unsigned int in_use;
> +	/* True if the devices were opened. */
> +	bool is_open;
>   
>   	/* Set when we find or add a device that doesn't have the nonrot flag set. */
>   	bool rotating;

Re: [PATCH v2 3/5] btrfs: split btrfs_fs_devices.opened

[Qu Wenruo]

在 2025/6/11 19:33, Johannes Thumshirn 写道:
> From: Christoph Hellwig <hch@lst.de>
> 
> The btrfs_fs_devices.opened member mixes an in use counter for the
> fs_devices structure that prevents it from being garbage collected with
> a flag if the underlying devices were actually opened.  This not only
> makes the code hard to follow, but also prevents btrfs from switching
> to opening the block device only after super block creation.  Split it
> into an in_use counter and an is_open boolean flag instead.

I'm hitting problems with this patch applied (previous patch 1 and 2 
also applied and tested), that it causes problem setting up other dm 
devices.
Thus failing future tests that requires SCRATCH_DEV for dm setup.

Furthermore at btrfs unload time, the warnings at free_fs_devices() got 
triggered.

But without this one (only patch 1 & 2), I have not hit anything like 
that at all.

So this means, even without extra in_use without opening cases, this is 
already causing problems.


Furthermore the in-use-but-not-opened state for the next patch (delayed 
devices opening), may not be that important anymore.

With extra comments added into btrfs_get_tree_super(), we can explain 
the cleanup properly.
And even if there are races between devices open (protected by 
uuid_mutex) and sget_fc(), and different threads win the two races, it 
doesn't really matter.

As long as the final btrfs_fill_super() get all its devices opened, and 
the opened ref count is working, there is nothing to worry.

So I'm afraid both patch 3 and 4 may be dropped.

Thanks,
Qu

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>   fs/btrfs/volumes.c | 53 ++++++++++++++++++++++++++--------------------
>   fs/btrfs/volumes.h |  6 ++++--
>   2 files changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1ebfc69012a2..00b64f98e3bd 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -413,7 +413,8 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
>   {
>   	struct btrfs_device *device;
>   
> -	WARN_ON(fs_devices->opened);
> +	WARN_ON_ONCE(fs_devices->in_use);
> +	WARN_ON_ONCE(fs_devices->is_open);
>   	while (!list_empty(&fs_devices->devices)) {
>   		device = list_first_entry(&fs_devices->devices,
>   					  struct btrfs_device, dev_list);
> @@ -541,7 +542,7 @@ static int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device
>   				continue;
>   			if (devt && devt != device->devt)
>   				continue;
> -			if (fs_devices->opened) {
> +			if (fs_devices->in_use) {
>   				if (devt)
>   					ret = -EBUSY;
>   				break;
> @@ -613,7 +614,7 @@ static struct btrfs_fs_devices *find_fsid_by_device(
>   	if (found_by_devt) {
>   		/* Existing device. */
>   		if (fsid_fs_devices == NULL) {
> -			if (devt_fs_devices->opened == 0) {
> +			if (devt_fs_devices->in_use == 0) {
>   				/* Stale device. */
>   				return NULL;
>   			} else {
> @@ -848,7 +849,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   	if (!device) {
>   		unsigned int nofs_flag;
>   
> -		if (fs_devices->opened) {
> +		if (fs_devices->in_use) {
>   			btrfs_err(NULL,
>   "device %s (%d:%d) belongs to fsid %pU, and the fs is already mounted, scanned by %s (%d)",
>   				  path, MAJOR(path_devt), MINOR(path_devt),
> @@ -916,7 +917,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   		 * tracking a problem where systems fail mount by subvolume id
>   		 * when we reject replacement on a mounted FS.
>   		 */
> -		if (!fs_devices->opened && found_transid < device->generation) {
> +		if (!fs_devices->in_use && found_transid < device->generation) {
>   			/*
>   			 * That is if the FS is _not_ mounted and if you
>   			 * are here, that means there is more than one
> @@ -977,7 +978,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   	 * it back. We need it to pick the disk with largest generation
>   	 * (as above).
>   	 */
> -	if (!fs_devices->opened) {
> +	if (!fs_devices->in_use) {
>   		device->generation = found_transid;
>   		fs_devices->latest_generation = max_t(u64, found_transid,
>   						fs_devices->latest_generation);
> @@ -1177,15 +1178,19 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
>   
>   	lockdep_assert_held(&uuid_mutex);
>   
> -	if (--fs_devices->opened > 0)
> +	if (--fs_devices->in_use > 0)
>   		return;
>   
> +	if (!fs_devices->is_open)
> +		goto done;
> +
>   	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list)
>   		btrfs_close_one_device(device);
>   
>   	WARN_ON(fs_devices->open_devices);
>   	WARN_ON(fs_devices->rw_devices);
> -	fs_devices->opened = 0;
> +	fs_devices->is_open = false;
> +done:
>   	fs_devices->seeding = false;
>   	fs_devices->fs_info = NULL;
>   }
> @@ -1197,7 +1202,7 @@ void btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
>   
>   	mutex_lock(&uuid_mutex);
>   	close_fs_devices(fs_devices);
> -	if (!fs_devices->opened) {
> +	if (!fs_devices->in_use) {
>   		list_splice_init(&fs_devices->seed_list, &list);
>   
>   		/*
> @@ -1253,7 +1258,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>   		return -EINVAL;
>   	}
>   
> -	fs_devices->opened = 1;
> +	fs_devices->is_open = true;
>   	fs_devices->latest_dev = latest_dev;
>   	fs_devices->total_rw_bytes = 0;
>   	fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_REGULAR;
> @@ -1306,16 +1311,14 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>   	 * We also don't need the lock here as this is called during mount and
>   	 * exclusion is provided by uuid_mutex
>   	 */
> -
> -	if (fs_devices->opened) {
> -		fs_devices->opened++;
> -		ret = 0;
> -	} else {
> +	if (!fs_devices->is_open) {
>   		list_sort(NULL, &fs_devices->devices, devid_cmp);
>   		ret = open_fs_devices(fs_devices, flags, holder);
> +		if (ret)
> +			return ret;
>   	}
> -
> -	return ret;
> +	fs_devices->in_use++;
> +	return 0;
>   }
>   
>   void btrfs_release_disk_super(struct btrfs_super_block *super)
> @@ -1407,7 +1410,7 @@ static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
>   
>   		mutex_lock(&fs_devices->device_list_mutex);
>   
> -		if (!fs_devices->opened) {
> +		if (!fs_devices->is_open) {
>   			mutex_unlock(&fs_devices->device_list_mutex);
>   			continue;
>   		}
> @@ -2314,13 +2317,14 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
>   	 * This can happen if cur_devices is the private seed devices list.  We
>   	 * cannot call close_fs_devices() here because it expects the uuid_mutex
>   	 * to be held, but in fact we don't need that for the private
> -	 * seed_devices, we can simply decrement cur_devices->opened and then
> +	 * seed_devices, we can simply decrement cur_devices->in_use and then
>   	 * remove it from our list and free the fs_devices.
>   	 */
>   	if (cur_devices->num_devices == 0) {
>   		list_del_init(&cur_devices->seed_list);
> -		ASSERT(cur_devices->opened == 1, "opened=%d", cur_devices->opened);
> -		cur_devices->opened--;
> +		ASSERT(cur_devices->in_use == 1, "opened=%d", cur_devices->in_use);
> +		cur_devices->in_use--;
> +		cur_devices->is_open = false;
>   		free_fs_devices(cur_devices);
>   	}
>   
> @@ -2549,7 +2553,8 @@ static struct btrfs_fs_devices *btrfs_init_sprout(struct btrfs_fs_info *fs_info)
>   	list_add(&old_devices->fs_list, &fs_uuids);
>   
>   	memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
> -	seed_devices->opened = 1;
> +	seed_devices->in_use = 1;
> +	seed_devices->is_open = true;
>   	INIT_LIST_HEAD(&seed_devices->devices);
>   	INIT_LIST_HEAD(&seed_devices->alloc_list);
>   	mutex_init(&seed_devices->device_list_mutex);
> @@ -7162,7 +7167,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
>   			return fs_devices;
>   
>   		fs_devices->seeding = true;
> -		fs_devices->opened = 1;
> +		fs_devices->in_use = 1;
> +		fs_devices->is_open = true;
>   		return fs_devices;
>   	}
>   
> @@ -7179,6 +7185,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
>   		free_fs_devices(fs_devices);
>   		return ERR_PTR(ret);
>   	}
> +	fs_devices->in_use = 1;
>   
>   	if (!fs_devices->seeding) {
>   		close_fs_devices(fs_devices);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index afa71d315c46..06cf8c99befe 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -419,8 +419,10 @@ struct btrfs_fs_devices {
>   
>   	struct list_head seed_list;
>   
> -	/* Count fs-devices opened. */
> -	int opened;
> +	/* Count if fs_device is in used. */
> +	unsigned int in_use;
> +	/* True if the devices were opened. */
> +	bool is_open;
>   
>   	/* Set when we find or add a device that doesn't have the nonrot flag set. */
>   	bool rotating;

Re: [PATCH v2 3/5] btrfs: split btrfs_fs_devices.opened

[Qu Wenruo]

在 2025/6/16 19:56, Qu Wenruo 写道:
> 
> 
> 在 2025/6/11 19:33, Johannes Thumshirn 写道:
>> From: Christoph Hellwig <hch@lst.de>
>>
>> The btrfs_fs_devices.opened member mixes an in use counter for the
>> fs_devices structure that prevents it from being garbage collected with
>> a flag if the underlying devices were actually opened.  This not only
>> makes the code hard to follow, but also prevents btrfs from switching
>> to opening the block device only after super block creation.  Split it
>> into an in_use counter and an is_open boolean flag instead.
> 
> I'm hitting problems with this patch applied (previous patch 1 and 2 
> also applied and tested), that it causes problem setting up other dm 
> devices.
> Thus failing future tests that requires SCRATCH_DEV for dm setup.
> 
> Furthermore at btrfs unload time, the warnings at free_fs_devices() got 
> triggered.
> 
> But without this one (only patch 1 & 2), I have not hit anything like 
> that at all.
> 
> So this means, even without extra in_use without opening cases, this is 
> already causing problems.
> 
> 
> Furthermore the in-use-but-not-opened state for the next patch (delayed 
> devices opening), may not be that important anymore.
> 
> With extra comments added into btrfs_get_tree_super(), we can explain 
> the cleanup properly.
> And even if there are races between devices open (protected by 
> uuid_mutex) and sget_fc(), and different threads win the two races, it 
> doesn't really matter.
> 
> As long as the final btrfs_fill_super() get all its devices opened, and 
> the opened ref count is working, there is nothing to worry.
> 
> So I'm afraid both patch 3 and 4 may be dropped.

But we still need the delay of btrfs_open_devices() after sget_fc() to 
use the super block as blk holder.

So my current version is to expand the critical section of uuid_mutex to 
cover sget_fc(), then call btrfs_open_devices(), and finally unlock 
uuid_mutex.

Still running the tests but so far so good.

Thanks,
Qu

> 
> Thanks,
> Qu
> 
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> ---
>>   fs/btrfs/volumes.c | 53 ++++++++++++++++++++++++++--------------------
>>   fs/btrfs/volumes.h |  6 ++++--
>>   2 files changed, 34 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 1ebfc69012a2..00b64f98e3bd 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -413,7 +413,8 @@ static void free_fs_devices(struct 
>> btrfs_fs_devices *fs_devices)
>>   {
>>       struct btrfs_device *device;
>> -    WARN_ON(fs_devices->opened);
>> +    WARN_ON_ONCE(fs_devices->in_use);
>> +    WARN_ON_ONCE(fs_devices->is_open);
>>       while (!list_empty(&fs_devices->devices)) {
>>           device = list_first_entry(&fs_devices->devices,
>>                         struct btrfs_device, dev_list);
>> @@ -541,7 +542,7 @@ static int btrfs_free_stale_devices(dev_t devt, 
>> struct btrfs_device *skip_device
>>                   continue;
>>               if (devt && devt != device->devt)
>>                   continue;
>> -            if (fs_devices->opened) {
>> +            if (fs_devices->in_use) {
>>                   if (devt)
>>                       ret = -EBUSY;
>>                   break;
>> @@ -613,7 +614,7 @@ static struct btrfs_fs_devices *find_fsid_by_device(
>>       if (found_by_devt) {
>>           /* Existing device. */
>>           if (fsid_fs_devices == NULL) {
>> -            if (devt_fs_devices->opened == 0) {
>> +            if (devt_fs_devices->in_use == 0) {
>>                   /* Stale device. */
>>                   return NULL;
>>               } else {
>> @@ -848,7 +849,7 @@ static noinline struct btrfs_device 
>> *device_list_add(const char *path,
>>       if (!device) {
>>           unsigned int nofs_flag;
>> -        if (fs_devices->opened) {
>> +        if (fs_devices->in_use) {
>>               btrfs_err(NULL,
>>   "device %s (%d:%d) belongs to fsid %pU, and the fs is already 
>> mounted, scanned by %s (%d)",
>>                     path, MAJOR(path_devt), MINOR(path_devt),
>> @@ -916,7 +917,7 @@ static noinline struct btrfs_device 
>> *device_list_add(const char *path,
>>            * tracking a problem where systems fail mount by subvolume id
>>            * when we reject replacement on a mounted FS.
>>            */
>> -        if (!fs_devices->opened && found_transid < device->generation) {
>> +        if (!fs_devices->in_use && found_transid < device->generation) {
>>               /*
>>                * That is if the FS is _not_ mounted and if you
>>                * are here, that means there is more than one
>> @@ -977,7 +978,7 @@ static noinline struct btrfs_device 
>> *device_list_add(const char *path,
>>        * it back. We need it to pick the disk with largest generation
>>        * (as above).
>>        */
>> -    if (!fs_devices->opened) {
>> +    if (!fs_devices->in_use) {
>>           device->generation = found_transid;
>>           fs_devices->latest_generation = max_t(u64, found_transid,
>>                           fs_devices->latest_generation);
>> @@ -1177,15 +1178,19 @@ static void close_fs_devices(struct 
>> btrfs_fs_devices *fs_devices)
>>       lockdep_assert_held(&uuid_mutex);
>> -    if (--fs_devices->opened > 0)
>> +    if (--fs_devices->in_use > 0)
>>           return;
>> +    if (!fs_devices->is_open)
>> +        goto done;
>> +
>>       list_for_each_entry_safe(device, tmp, &fs_devices->devices, 
>> dev_list)
>>           btrfs_close_one_device(device);
>>       WARN_ON(fs_devices->open_devices);
>>       WARN_ON(fs_devices->rw_devices);
>> -    fs_devices->opened = 0;
>> +    fs_devices->is_open = false;
>> +done:
>>       fs_devices->seeding = false;
>>       fs_devices->fs_info = NULL;
>>   }
>> @@ -1197,7 +1202,7 @@ void btrfs_close_devices(struct btrfs_fs_devices 
>> *fs_devices)
>>       mutex_lock(&uuid_mutex);
>>       close_fs_devices(fs_devices);
>> -    if (!fs_devices->opened) {
>> +    if (!fs_devices->in_use) {
>>           list_splice_init(&fs_devices->seed_list, &list);
>>           /*
>> @@ -1253,7 +1258,7 @@ static int open_fs_devices(struct 
>> btrfs_fs_devices *fs_devices,
>>           return -EINVAL;
>>       }
>> -    fs_devices->opened = 1;
>> +    fs_devices->is_open = true;
>>       fs_devices->latest_dev = latest_dev;
>>       fs_devices->total_rw_bytes = 0;
>>       fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_REGULAR;
>> @@ -1306,16 +1311,14 @@ int btrfs_open_devices(struct btrfs_fs_devices 
>> *fs_devices,
>>        * We also don't need the lock here as this is called during 
>> mount and
>>        * exclusion is provided by uuid_mutex
>>        */
>> -
>> -    if (fs_devices->opened) {
>> -        fs_devices->opened++;
>> -        ret = 0;
>> -    } else {
>> +    if (!fs_devices->is_open) {
>>           list_sort(NULL, &fs_devices->devices, devid_cmp);
>>           ret = open_fs_devices(fs_devices, flags, holder);
>> +        if (ret)
>> +            return ret;
>>       }
>> -
>> -    return ret;
>> +    fs_devices->in_use++;
>> +    return 0;
>>   }
>>   void btrfs_release_disk_super(struct btrfs_super_block *super)
>> @@ -1407,7 +1410,7 @@ static bool btrfs_skip_registration(struct 
>> btrfs_super_block *disk_super,
>>           mutex_lock(&fs_devices->device_list_mutex);
>> -        if (!fs_devices->opened) {
>> +        if (!fs_devices->is_open) {
>>               mutex_unlock(&fs_devices->device_list_mutex);
>>               continue;
>>           }
>> @@ -2314,13 +2317,14 @@ int btrfs_rm_device(struct btrfs_fs_info 
>> *fs_info,
>>        * This can happen if cur_devices is the private seed devices 
>> list.  We
>>        * cannot call close_fs_devices() here because it expects the 
>> uuid_mutex
>>        * to be held, but in fact we don't need that for the private
>> -     * seed_devices, we can simply decrement cur_devices->opened and 
>> then
>> +     * seed_devices, we can simply decrement cur_devices->in_use and 
>> then
>>        * remove it from our list and free the fs_devices.
>>        */
>>       if (cur_devices->num_devices == 0) {
>>           list_del_init(&cur_devices->seed_list);
>> -        ASSERT(cur_devices->opened == 1, "opened=%d", cur_devices- 
>> >opened);
>> -        cur_devices->opened--;
>> +        ASSERT(cur_devices->in_use == 1, "opened=%d", cur_devices- 
>> >in_use);
>> +        cur_devices->in_use--;
>> +        cur_devices->is_open = false;
>>           free_fs_devices(cur_devices);
>>       }
>> @@ -2549,7 +2553,8 @@ static struct btrfs_fs_devices 
>> *btrfs_init_sprout(struct btrfs_fs_info *fs_info)
>>       list_add(&old_devices->fs_list, &fs_uuids);
>>       memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
>> -    seed_devices->opened = 1;
>> +    seed_devices->in_use = 1;
>> +    seed_devices->is_open = true;
>>       INIT_LIST_HEAD(&seed_devices->devices);
>>       INIT_LIST_HEAD(&seed_devices->alloc_list);
>>       mutex_init(&seed_devices->device_list_mutex);
>> @@ -7162,7 +7167,8 @@ static struct btrfs_fs_devices 
>> *open_seed_devices(struct btrfs_fs_info *fs_info,
>>               return fs_devices;
>>           fs_devices->seeding = true;
>> -        fs_devices->opened = 1;
>> +        fs_devices->in_use = 1;
>> +        fs_devices->is_open = true;
>>           return fs_devices;
>>       }
>> @@ -7179,6 +7185,7 @@ static struct btrfs_fs_devices 
>> *open_seed_devices(struct btrfs_fs_info *fs_info,
>>           free_fs_devices(fs_devices);
>>           return ERR_PTR(ret);
>>       }
>> +    fs_devices->in_use = 1;
>>       if (!fs_devices->seeding) {
>>           close_fs_devices(fs_devices);
>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
>> index afa71d315c46..06cf8c99befe 100644
>> --- a/fs/btrfs/volumes.h
>> +++ b/fs/btrfs/volumes.h
>> @@ -419,8 +419,10 @@ struct btrfs_fs_devices {
>>       struct list_head seed_list;
>> -    /* Count fs-devices opened. */
>> -    int opened;
>> +    /* Count if fs_device is in used. */
>> +    unsigned int in_use;
>> +    /* True if the devices were opened. */
>> +    bool is_open;
>>       /* Set when we find or add a device that doesn't have the nonrot 
>> flag set. */
>>       bool rotating;
> 
> 

[PATCH v2 4/5] btrfs: open block devices after superblock creation

[Johannes Thumshirn]

From: Christoph Hellwig <hch@lst.de>

Currently btrfs_mount_root opens the block devices before committing to
allocating a super block. That creates problems for restricting the
number of writers to a device, and also leads to a unusual and not very
helpful holder (the fs_type).

Reorganize the code to first check whether the superblock for a
particular fsid does already exist and open the block devices only if it
doesn't, mirroring the recent changes to the VFS mount helpers.  To do
this the increment of the in_use counter moves out of btrfs_open_devices
and into the only caller in btrfs_mount_root so that it happens before
dropping uuid_mutex around the call to sget.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/super.c   | 40 +++++++++++++++++++++++++---------------
 fs/btrfs/volumes.c | 15 +++++----------
 2 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index eaecf1525078..eafa524c7c81 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1841,7 +1841,6 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 	struct btrfs_fs_info *fs_info = fc->s_fs_info;
 	struct btrfs_fs_context *ctx = fc->fs_private;
 	struct btrfs_fs_devices *fs_devices = NULL;
-	struct block_device *bdev;
 	struct btrfs_device *device;
 	struct super_block *sb;
 	blk_mode_t mode = btrfs_open_mode(fc);
@@ -1864,15 +1863,8 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 	fs_devices = device->fs_devices;
 	fs_info->fs_devices = fs_devices;
 
-	ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
+	fs_devices->in_use++;
 	mutex_unlock(&uuid_mutex);
-	if (ret)
-		return ret;
-
-	if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0)
-		return -EACCES;
-
-	bdev = fs_devices->latest_dev->bdev;
 
 	/*
 	 * From now on the error handling is not straightforward.
@@ -1896,23 +1888,41 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		 * mismatch and reconfigure with sb->s_umount rwsem held if
 		 * needed.
 		 */
-		if ((fc->sb_flags ^ sb->s_flags) & SB_RDONLY)
+		if ((fc->sb_flags ^ sb->s_flags) & SB_RDONLY) {
 			ret = -EBUSY;
+			goto error_deactivate;
+		}
 	} else {
-		snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
+		struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+
+		mutex_lock(&uuid_mutex);
+		ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
+		mutex_unlock(&uuid_mutex);
+		if (ret)
+			goto error_deactivate;
+
+		if (!(fc->sb_flags & SB_RDONLY) && !fs_devices->rw_devices) {
+			ret = -EACCES;
+			goto error_deactivate;
+		}
+
+		snprintf(sb->s_id, sizeof(sb->s_id), "%pg",
+			 fs_devices->latest_dev->bdev);
 		shrinker_debugfs_rename(sb->s_shrink, "sb-btrfs:%s", sb->s_id);
 		btrfs_sb(sb)->bdev_holder = &btrfs_fs_type;
 		ret = btrfs_fill_super(sb, fs_devices);
-		if (ret) {
-			deactivate_locked_super(sb);
-			return ret;
-		}
+		if (ret)
+			goto error_deactivate;
 	}
 
 	btrfs_clear_oneshot_options(fs_info);
 
 	fc->root = dget(sb->s_root);
 	return 0;
+
+error_deactivate:
+	deactivate_locked_super(sb);
+	return ret;
 }
 
 /*
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 00b64f98e3bd..3e219a1a4c75 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1301,8 +1301,6 @@ static int devid_cmp(void *priv, const struct list_head *a,
 int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       blk_mode_t flags, void *holder)
 {
-	int ret;
-
 	lockdep_assert_held(&uuid_mutex);
 	/*
 	 * The device_list_mutex cannot be taken here in case opening the
@@ -1311,14 +1309,11 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 	 * We also don't need the lock here as this is called during mount and
 	 * exclusion is provided by uuid_mutex
 	 */
-	if (!fs_devices->is_open) {
-		list_sort(NULL, &fs_devices->devices, devid_cmp);
-		ret = open_fs_devices(fs_devices, flags, holder);
-		if (ret)
-			return ret;
-	}
-	fs_devices->in_use++;
-	return 0;
+	ASSERT(fs_devices->in_use);
+	if (fs_devices->is_open)
+		return 0;
+	list_sort(NULL, &fs_devices->devices, devid_cmp);
+	return open_fs_devices(fs_devices, flags, holder);
 }
 
 void btrfs_release_disk_super(struct btrfs_super_block *super)
-- 
2.49.0

[PATCH v2 5/5] btrfs: use the super_block as holder when mounting file systems

[Johannes Thumshirn]

From: Christoph Hellwig <hch@lst.de>

The file system type is not a very useful holder as it doesn't allow us
to go back to the actual file system instance.  Pass the super_block
instead which is useful when passed back to the file system driver.

This matches what is done for all other block device based file systems.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index eafa524c7c81..30c928562558 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1896,7 +1896,7 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 
 		mutex_lock(&uuid_mutex);
-		ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
+		ret = btrfs_open_devices(fs_devices, mode, sb);
 		mutex_unlock(&uuid_mutex);
 		if (ret)
 			goto error_deactivate;
@@ -1909,7 +1909,7 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 		snprintf(sb->s_id, sizeof(sb->s_id), "%pg",
 			 fs_devices->latest_dev->bdev);
 		shrinker_debugfs_rename(sb->s_shrink, "sb-btrfs:%s", sb->s_id);
-		btrfs_sb(sb)->bdev_holder = &btrfs_fs_type;
+		btrfs_sb(sb)->bdev_holder = sb;
 		ret = btrfs_fill_super(sb, fs_devices);
 		if (ret)
 			goto error_deactivate;
-- 
2.49.0

Re: [PATCH v2 0/5] btrfs: use the super_block as bdev holder

[Qu Wenruo]

在 2025/6/11 19:32, Johannes Thumshirn 写道:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> This is a series I've picked up from Christoph, it changes the
> block_device's bdev holder from fs_type to the super block.
> 
> As the re-base was non trivial, I opted to drop Boris' Reviewed-by tags.
> 
> Here's the original cover letter:
> Hi all,
> 
> this series contains the btrfs parts of the "remove get_super" from June
> that managed to get lost.
> 
> I've dropped all the reviews from back then as the rebase against the new
> mount API conversion led to a lot of non-trivial conflicts.
> 
> Josef kindly ran it through the CI farm and provided a fixup based on that.

Unfortunately it crashed immediately on btrfs/001:

[   23.878153] BTRFS info (device dm-0): using free-space-tree
[   23.891318] BUG: kernel NULL pointer dereference, address: 
00000000000006f0
[   23.894047] #PF: supervisor read access in kernel mode
[   23.896010] #PF: error_code(0x0000) - not-present page
[   23.897982] PGD 0 P4D 0
[   23.899016] Oops: Oops: 0000 [#1] SMP NOPTI
[   23.900738] CPU: 9 UID: 0 PID: 768 Comm: mount Not tainted 
6.16.0-rc1-custom+ #253 PREEMPT(full)
[   23.904288] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
unknown 02/02/2022
[   23.907696] RIP: 0010:btrfs_get_tree+0x29c/0x670 [btrfs]
[   23.910090] Code: 89 83 50 02 00 00 e9 b0 fe ff ff 48 c7 c7 a0 4c 6f 
a0 48 89 04 24 e8 63 6d 67 e1 8b 2c 24 e9 e9 fe ff ff 49 8b ae 80 00 00 
00 <48> 8b bd f0 06 00 00 48 85 ff 74 10 e8 03 c6 05 00 48 c7 85 f0 06
[   23.917698] RSP: 0018:ffffc90002663e00 EFLAGS: 00010246
[   23.919769] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 
0000000000000000
[   23.922817] RDX: 7fffffffffffffff RSI: 0000000000000000 RDI: 
ffffffff835d9d80
[   23.925869] RBP: 0000000000000000 R08: 0000000000000005 R09: 
ffff888101ba6000
[   23.928916] R10: 00000000000000c8 R11: ffff88810a8fe280 R12: 
ffff888109932d80
[   23.932083] R13: ffff8881061ef640 R14: ffff888104dab840 R15: 
ffff888109932d80
[   23.935160] FS:  00007f9aa721ab80(0000) GS:ffff8882f4762000(0000) 
knlGS:0000000000000000
[   23.938582] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   23.941060] CR2: 00000000000006f0 CR3: 0000000128bc2000 CR4: 
00000000000006f0
[   23.944122] Call Trace:
[   23.945228]  <TASK>
[   23.946189]  ? fscontext_read+0x118/0x130
[   23.947947]  vfs_get_tree+0x29/0xd0
[   23.949507]  vfs_cmd_create+0x57/0xd0
[   23.951131]  __do_sys_fsconfig+0x4b6/0x650
[   23.952891]  do_syscall_64+0x54/0x1d0
[   23.954522]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   23.956703] RIP: 0033:0x7f9aa736fefe
[   23.958313] Code: 73 01 c3 48 8b 0d 12 be 0c 00 f7 d8 64 89 01 48 83 
c8 ff c3 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 49 89 ca b8 af 01 00 00 0f 
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e2 bd 0c 00 f7 d8 64 89 01 48
[   23.966225] RSP: 002b:00007ffe79405e18 EFLAGS: 00000246 ORIG_RAX: 
00000000000001af
[   23.969431] RAX: ffffffffffffffda RBX: 000055f970791430 RCX: 
00007f9aa736fefe
[   23.972466] RDX: 0000000000000000 RSI: 0000000000000006 RDI: 
0000000000000003
[   23.975485] RBP: 00007ffe79405e50 R08: 0000000000000000 R09: 
0000000000000000
[   23.978527] R10: 0000000000000000 R11: 0000000000000246 R12: 
00007f9aa7499980
[   23.981535] R13: 0000000000000000 R14: 000055f970792720 R15: 
00007f9aa748e8e0
[   23.984548]  </TASK>
[   23.985537] Modules linked in: crc32c_cryptoapi vfat fat btrfs 
blake2b_generic xor zstd_compress iTCO_wdt iTCO_vendor_support psmouse 
pcspkr lpc_ich i2c_i801 i2c_smbus joydev intel_agp intel_gtt mousedev 
agpgart raid6_pq drm fuse loop qemu_fw_cfg ext4 crc16 mbcache jbd2 
dm_mod virtio_net net_failover virtio_rng failover virtio_balloon 
virtio_console virtio_blk rng_core virtio_scsi virtio_pci serio_raw 
usbhid virtio_pci_legacy_dev virtio_pci_modern_dev
[   24.002094] Dumping ftrace buffer:
[   24.003601]    (ftrace buffer empty)
[   24.005188] CR2: 00000000000006f0
[   24.006735] ---[ end trace 0000000000000000 ]---
[   25.685915] RIP: 0010:btrfs_get_tree+0x29c/0x670 [btrfs]
[   25.688118] Code: 89 83 50 02 00 00 e9 b0 fe ff ff 48 c7 c7 a0 4c 6f 
a0 48 89 04 24 e8 63 6d 67 e1 8b 2c 24 e9 e9 fe ff ff 49 8b ae 80 00 00 
00 <48> 8b bd f0 06 00 00 48 85 ff 74 10 e8 03 c6 05 00 48 c7 85 f0 06
[   25.695103] RSP: 0018:ffffc90002663e00 EFLAGS: 00010246
[   25.697125] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 
0000000000000000
[   25.699813] RDX: 7fffffffffffffff RSI: 0000000000000000 RDI: 
ffffffff835d9d80
[   25.702464] RBP: 0000000000000000 R08: 0000000000000005 R09: 
ffff888101ba6000
[   25.705185] R10: 00000000000000c8 R11: ffff88810a8fe280 R12: 
ffff888109932d80
[   25.708000] R13: ffff8881061ef640 R14: ffff888104dab840 R15: 
ffff888109932d80
[   25.710720] FS:  00007f9aa721ab80(0000) GS:ffff8882f4762000(0000) 
knlGS:0000000000000000
[   25.713772] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   25.716003] CR2: 00000000000006f0 CR3: 0000000128bc2000 CR4: 
00000000000006f0
[   25.718657] Kernel panic - not syncing: Fatal exception
[   25.721077] Dumping ftrace buffer:
[   25.722398]    (ftrace buffer empty)
[   25.723775] Kernel Offset: disabled
[   27.228888] Rebooting in 5 seconds..>
> 
> 
> Link to rebase v1:
> https://lore.kernel.org/linux-btrfs/20240214-hch-device-open-v1-0-b153428b4f72@wdc.com/
> 
> Link to original posting:
> https://lore.kernel.org/linux-btrfs/b083ae24-2273-479f-8c9e-96cb9ef083b8@wdc.com/
> 
> Christoph Hellwig (5):
>    btrfs: always open the device read-only in btrfs_scan_one_device
>    btrfs: call btrfs_close_devices from ->kill_sb
>    btrfs: split btrfs_fs_devices.opened
>    btrfs: open block devices after superblock creation
>    btrfs: use the super_block as holder when mounting file systems
> 
>   fs/btrfs/disk-io.c |  4 +--
>   fs/btrfs/super.c   | 70 +++++++++++++++++++++++++++-------------------
>   fs/btrfs/volumes.c | 62 ++++++++++++++++++++--------------------
>   fs/btrfs/volumes.h |  8 ++++--
>   4 files changed, 80 insertions(+), 64 deletions(-)
> 

Re: [PATCH v2 0/5] btrfs: use the super_block as bdev holder

[Qu Wenruo]

在 2025/6/12 07:19, Qu Wenruo 写道:
> 
> 
> 在 2025/6/11 19:32, Johannes Thumshirn 写道:
>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> This is a series I've picked up from Christoph, it changes the
>> block_device's bdev holder from fs_type to the super block.
>>
>> As the re-base was non trivial, I opted to drop Boris' Reviewed-by tags.
>>
>> Here's the original cover letter:
>> Hi all,
>>
>> this series contains the btrfs parts of the "remove get_super" from June
>> that managed to get lost.
>>
>> I've dropped all the reviews from back then as the rebase against the new
>> mount API conversion led to a lot of non-trivial conflicts.
>>
>> Josef kindly ran it through the CI farm and provided a fixup based on 
>> that.
> 
> Unfortunately it crashed immediately on btrfs/001:
> 
> [   23.878153] BTRFS info (device dm-0): using free-space-tree
> [   23.891318] BUG: kernel NULL pointer dereference, address: 
> 00000000000006f0
> [   23.894047] #PF: supervisor read access in kernel mode
> [   23.896010] #PF: error_code(0x0000) - not-present page
> [   23.897982] PGD 0 P4D 0
> [   23.899016] Oops: Oops: 0000 [#1] SMP NOPTI
> [   23.900738] CPU: 9 UID: 0 PID: 768 Comm: mount Not tainted 6.16.0- 
> rc1-custom+ #253 PREEMPT(full)
> [   23.904288] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
> unknown 02/02/2022
> [   23.907696] RIP: 0010:btrfs_get_tree+0x29c/0x670 [btrfs]

This is at the line "if (fs_info->fs_devices)", so at this stage fs_info 
is NULL already.

And it's again back to the original comment on the 2nd patch, why we 
need to close the devices at btrfs_reconfigure_for_mount().

Thanks,
Qu

> [   23.910090] Code: 89 83 50 02 00 00 e9 b0 fe ff ff 48 c7 c7 a0 4c 6f 
> a0 48 89 04 24 e8 63 6d 67 e1 8b 2c 24 e9 e9 fe ff ff 49 8b ae 80 00 00 
> 00 <48> 8b bd f0 06 00 00 48 85 ff 74 10 e8 03 c6 05 00 48 c7 85 f0 06
> [   23.917698] RSP: 0018:ffffc90002663e00 EFLAGS: 00010246
> [   23.919769] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 
> 0000000000000000
> [   23.922817] RDX: 7fffffffffffffff RSI: 0000000000000000 RDI: 
> ffffffff835d9d80
> [   23.925869] RBP: 0000000000000000 R08: 0000000000000005 R09: 
> ffff888101ba6000
> [   23.928916] R10: 00000000000000c8 R11: ffff88810a8fe280 R12: 
> ffff888109932d80
> [   23.932083] R13: ffff8881061ef640 R14: ffff888104dab840 R15: 
> ffff888109932d80
> [   23.935160] FS:  00007f9aa721ab80(0000) GS:ffff8882f4762000(0000) 
> knlGS:0000000000000000
> [   23.938582] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   23.941060] CR2: 00000000000006f0 CR3: 0000000128bc2000 CR4: 
> 00000000000006f0
> [   23.944122] Call Trace:
> [   23.945228]  <TASK>
> [   23.946189]  ? fscontext_read+0x118/0x130
> [   23.947947]  vfs_get_tree+0x29/0xd0
> [   23.949507]  vfs_cmd_create+0x57/0xd0
> [   23.951131]  __do_sys_fsconfig+0x4b6/0x650
> [   23.952891]  do_syscall_64+0x54/0x1d0
> [   23.954522]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [   23.956703] RIP: 0033:0x7f9aa736fefe
> [   23.958313] Code: 73 01 c3 48 8b 0d 12 be 0c 00 f7 d8 64 89 01 48 83 
> c8 ff c3 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 49 89 ca b8 af 01 00 00 0f 
> 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e2 bd 0c 00 f7 d8 64 89 01 48
> [   23.966225] RSP: 002b:00007ffe79405e18 EFLAGS: 00000246 ORIG_RAX: 
> 00000000000001af
> [   23.969431] RAX: ffffffffffffffda RBX: 000055f970791430 RCX: 
> 00007f9aa736fefe
> [   23.972466] RDX: 0000000000000000 RSI: 0000000000000006 RDI: 
> 0000000000000003
> [   23.975485] RBP: 00007ffe79405e50 R08: 0000000000000000 R09: 
> 0000000000000000
> [   23.978527] R10: 0000000000000000 R11: 0000000000000246 R12: 
> 00007f9aa7499980
> [   23.981535] R13: 0000000000000000 R14: 000055f970792720 R15: 
> 00007f9aa748e8e0
> [   23.984548]  </TASK>
> [   23.985537] Modules linked in: crc32c_cryptoapi vfat fat btrfs 
> blake2b_generic xor zstd_compress iTCO_wdt iTCO_vendor_support psmouse 
> pcspkr lpc_ich i2c_i801 i2c_smbus joydev intel_agp intel_gtt mousedev 
> agpgart raid6_pq drm fuse loop qemu_fw_cfg ext4 crc16 mbcache jbd2 
> dm_mod virtio_net net_failover virtio_rng failover virtio_balloon 
> virtio_console virtio_blk rng_core virtio_scsi virtio_pci serio_raw 
> usbhid virtio_pci_legacy_dev virtio_pci_modern_dev
> [   24.002094] Dumping ftrace buffer:
> [   24.003601]    (ftrace buffer empty)
> [   24.005188] CR2: 00000000000006f0
> [   24.006735] ---[ end trace 0000000000000000 ]---
> [   25.685915] RIP: 0010:btrfs_get_tree+0x29c/0x670 [btrfs]
> [   25.688118] Code: 89 83 50 02 00 00 e9 b0 fe ff ff 48 c7 c7 a0 4c 6f 
> a0 48 89 04 24 e8 63 6d 67 e1 8b 2c 24 e9 e9 fe ff ff 49 8b ae 80 00 00 
> 00 <48> 8b bd f0 06 00 00 48 85 ff 74 10 e8 03 c6 05 00 48 c7 85 f0 06
> [   25.695103] RSP: 0018:ffffc90002663e00 EFLAGS: 00010246
> [   25.697125] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 
> 0000000000000000
> [   25.699813] RDX: 7fffffffffffffff RSI: 0000000000000000 RDI: 
> ffffffff835d9d80
> [   25.702464] RBP: 0000000000000000 R08: 0000000000000005 R09: 
> ffff888101ba6000
> [   25.705185] R10: 00000000000000c8 R11: ffff88810a8fe280 R12: 
> ffff888109932d80
> [   25.708000] R13: ffff8881061ef640 R14: ffff888104dab840 R15: 
> ffff888109932d80
> [   25.710720] FS:  00007f9aa721ab80(0000) GS:ffff8882f4762000(0000) 
> knlGS:0000000000000000
> [   25.713772] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   25.716003] CR2: 00000000000006f0 CR3: 0000000128bc2000 CR4: 
> 00000000000006f0
> [   25.718657] Kernel panic - not syncing: Fatal exception
> [   25.721077] Dumping ftrace buffer:
> [   25.722398]    (ftrace buffer empty)
> [   25.723775] Kernel Offset: disabled
> [   27.228888] Rebooting in 5 seconds..>
>>
>>
>> Link to rebase v1:
>> https://lore.kernel.org/linux-btrfs/20240214-hch-device-open-v1-0- 
>> b153428b4f72@wdc.com/
>>
>> Link to original posting:
>> https://lore.kernel.org/linux-btrfs/ 
>> b083ae24-2273-479f-8c9e-96cb9ef083b8@wdc.com/
>>
>> Christoph Hellwig (5):
>>    btrfs: always open the device read-only in btrfs_scan_one_device
>>    btrfs: call btrfs_close_devices from ->kill_sb
>>    btrfs: split btrfs_fs_devices.opened
>>    btrfs: open block devices after superblock creation
>>    btrfs: use the super_block as holder when mounting file systems
>>
>>   fs/btrfs/disk-io.c |  4 +--
>>   fs/btrfs/super.c   | 70 +++++++++++++++++++++++++++-------------------
>>   fs/btrfs/volumes.c | 62 ++++++++++++++++++++--------------------
>>   fs/btrfs/volumes.h |  8 ++++--
>>   4 files changed, 80 insertions(+), 64 deletions(-)
>>
> 
> 

Re: [PATCH v2 0/5] btrfs: use the super_block as bdev holder

[Johannes Thumshirn]

On 12.06.25 00:06, Qu Wenruo wrote:
> This is at the line "if (fs_info->fs_devices)", so at this stage fs_info
> is NULL already.
> 
> And it's again back to the original comment on the 2nd patch, why we
> need to close the devices at btrfs_reconfigure_for_mount().

Sorry for not running the series through fstests after the rebase.

As for your question, give me some time to answer it. I'm currently
only "working" on this while test runs are ongoing for different,
more urgent problems I'm focused on, sorry.

One thing I've added on top is:

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 30c928562558..edf335a7382d 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2007,7 +2007,8 @@ static int btrfs_reconfigure_for_mount(struct fs_context *fc)
          * We got a reference to our fs_devices, so we need to close it here to
          * make sure we don't leak our reference on the fs_devices.
          */
-       if (fs_info->fs_devices) {
+       if (fs_info && fs_info->fs_devices) {
+               ASSERT(fs_info->fs_devices->is_open);
                 btrfs_close_devices(fs_info->fs_devices);
                 fs_info->fs_devices = NULL;
         }

So if we end up in btrfs_reconfigure_for_mount() and fs_info and
fs_info->fs_devices are set, I see the is_open flag being set as
well. But the fstests run isn't 100% finished yet (and it's only
been a -g quick run anyways).

Re: [PATCH v2 0/5] btrfs: use the super_block as bdev holder

[Qu Wenruo]

在 2025/6/12 21:45, Johannes Thumshirn 写道:
> On 12.06.25 00:06, Qu Wenruo wrote:
>> This is at the line "if (fs_info->fs_devices)", so at this stage fs_info
>> is NULL already.
>>
>> And it's again back to the original comment on the 2nd patch, why we
>> need to close the devices at btrfs_reconfigure_for_mount().
> 
> Sorry for not running the series through fstests after the rebase.
> 
> As for your question, give me some time to answer it. I'm currently
> only "working" on this while test runs are ongoing for different,
> more urgent problems I'm focused on, sorry.
> 
> One thing I've added on top is:
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 30c928562558..edf335a7382d 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2007,7 +2007,8 @@ static int btrfs_reconfigure_for_mount(struct fs_context *fc)
>            * We got a reference to our fs_devices, so we need to close it here to
>            * make sure we don't leak our reference on the fs_devices.
>            */
> -       if (fs_info->fs_devices) {
> +       if (fs_info && fs_info->fs_devices) {
> +               ASSERT(fs_info->fs_devices->is_open);
>                   btrfs_close_devices(fs_info->fs_devices);
>                   fs_info->fs_devices = NULL;
>           }
> 
> So if we end up in btrfs_reconfigure_for_mount() and fs_info and
> fs_info->fs_devices are set, I see the is_open flag being set as
> well. But the fstests run isn't 100% finished yet (and it's only
> been a -g quick run anyways).

Since I'm also working on cleaning up the mount process, I'm getting a 
little familiar with this part, but if HCH can comment on this, it will 
be a great help.

At this stage, we either:

- Created a new super in btrfs_get_tree_super()
   This means it's the first mount of this fs.

   In that case, sget_fc() will set dup_fc->s_fs_info to NULL.

   So we do nothing for this case.

- Grabbed an existing super in btrfs_get_tree_super()
   In this case dup_fc->s_fs_info is not touched, but it's still assigned
   to a temporary fs_info allocated inside btrfs_get_tree_subvol().

   And since this patch removed the btrfs_close_devices() calls, we have
   to close the devices somewhere, and since there is no new sb
   allocated we can not rely on kill_sb() calls.

   So we have to close the fs_devices here.

   But I do not think it's the correct timing. Close of fs_devices should
   happen immediately after we know there is already an existing sb.

   So the old "if (sb->s_root) { btrfs_close_devices(); }" is the correct
   timing, and we should not remove it in this patch.

   And the close inside btrfs_reconfigure_for_mount() looks more like an
   incorrect hot fix to patch up that removal.

So overall, I still do not think we should do any device close inside 
btrfs_reconfigure_for_mount(), but keep the close_devices() call inside 
btrfs_get_tree_super() and add extra comments on that.

Thanks,
Qu

Re: [PATCH v2 0/5] btrfs: use the super_block as bdev holder

[hch]

On Fri, Jun 13, 2025 at 07:51:25AM +0930, Qu Wenruo wrote:
>> -       if (fs_info->fs_devices) {
>> +       if (fs_info && fs_info->fs_devices) {
>> +               ASSERT(fs_info->fs_devices->is_open);
>>                   btrfs_close_devices(fs_info->fs_devices);
>>                   fs_info->fs_devices = NULL;
>>           }
>>
>> So if we end up in btrfs_reconfigure_for_mount() and fs_info and
>> fs_info->fs_devices are set, I see the is_open flag being set as
>> well. But the fstests run isn't 100% finished yet (and it's only
>> been a -g quick run anyways).
>
> Since I'm also working on cleaning up the mount process, I'm getting a 
> little familiar with this part, but if HCH can comment on this, it will be 
> a great help.

I wish I could.  A lot of this mount stuff has been entirely paged out
of my memory, I'm sorry.  Note that Christian did some fairly big rework
in this area, and now the new mount API came in as well.  So things
around it looks pretty different.

I think the parts of the series that are valueable as is are the
"open read-only for scanning" and split the inuse counter bits,
which are pretty obvious.  Everything else might need a more or less
big redo with all the surrounding changes.

Re: [PATCH v2 0/5] btrfs: use the super_block as bdev holder

[Qu Wenruo]

在 2025/6/13 15:29, hch 写道:
> On Fri, Jun 13, 2025 at 07:51:25AM +0930, Qu Wenruo wrote:
>>> -       if (fs_info->fs_devices) {
>>> +       if (fs_info && fs_info->fs_devices) {
>>> +               ASSERT(fs_info->fs_devices->is_open);
>>>                    btrfs_close_devices(fs_info->fs_devices);
>>>                    fs_info->fs_devices = NULL;
>>>            }
>>>
>>> So if we end up in btrfs_reconfigure_for_mount() and fs_info and
>>> fs_info->fs_devices are set, I see the is_open flag being set as
>>> well. But the fstests run isn't 100% finished yet (and it's only
>>> been a -g quick run anyways).
>>
>> Since I'm also working on cleaning up the mount process, I'm getting a
>> little familiar with this part, but if HCH can comment on this, it will be
>> a great help.
> 
> I wish I could.  A lot of this mount stuff has been entirely paged out
> of my memory, I'm sorry.  Note that Christian did some fairly big rework
> in this area, and now the new mount API came in as well.  So things
> around it looks pretty different.

That's totally fine.

And since I'm already working on the surrounding code, I'll definitely 
make the involved code easier to read.

> 
> I think the parts of the series that are valueable as is are the
> "open read-only for scanning" and split the inuse counter bits,
> which are pretty obvious.  Everything else might need a more or less
> big redo with all the surrounding changes.
> 

And the "open block devices after super block creation" patch.

It's the existing code making the sb creation way harder to grasp.

With that improved, the benefit of delaying device open should be very 
obvious.

If Johannes is fine, I can integrate those patches into my upcoming 
get_tree cleanup.

Thanks,
Qu

Re: [PATCH v2 0/5] btrfs: use the super_block as bdev holder

[Johannes Thumshirn]

On 13.06.25 10:01, Qu Wenruo wrote:
> That's totally fine.
> 
> And since I'm already working on the surrounding code, I'll definitely
> make the involved code easier to read.
> 
>>
>> I think the parts of the series that are valueable as is are the
>> "open read-only for scanning" and split the inuse counter bits,
>> which are pretty obvious.  Everything else might need a more or less
>> big redo with all the surrounding changes.
>>
> 
> And the "open block devices after super block creation" patch.
> 
> It's the existing code making the sb creation way harder to grasp.
> 
> With that improved, the benefit of delaying device open should be very
> obvious.
> 
> If Johannes is fine, I can integrate those patches into my upcoming
> get_tree cleanup.

Sure, go for it.