Re: [PATCH] btrfs: use fs_info as the block device holder

[Boris Burkov <boris@bur.io>]

On Thu, Jun 05, 2025 at 07:15:48PM +0930, Qu Wenruo wrote:
> Currently btrfs uses "btrfs_fs_type" as the bdev holder for all opened
> device, which means all btrfses shares the same holder value.
> 
> That's only fine when there is no blk_holder_ops provided, but we're
> going to implement blk_holder_ops soon, so replace the "btrfs_fs_type"
> holder usage, and replace it with a proper fs_info instead.
> 
> This means we can remove the btrfs_fs_info::bdev_holder completely.

I definitely support this, I always found it quite weird that we had a
single generic holder and relied on our own checking to ensure we only
mount each device once. I think this should help insulate us from
bizarre fsid bugs like we've seen with seed+sprout in the past.

However, I'm a little confused, as I thought Johannes and Christoph made
the change to using the super block as the holder a while ago:
https://lore.kernel.org/linux-btrfs/20231218044933.706042-1-hch@lst.de/

Did that end up failing to get in? Or it got reverted? I don't see it in
the git history.

Those patches have a lot more going on, so I'm also wondering if any of
that is necessary for making this change? Haven't fully refreshed myself
on that old series, though, so it's a fairly naive question. Also, it
does seem like other fs-es uses sb, but I don't have a strong opinion on
that and don't see why fs_info can't work.

Thanks,
Boris

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/dev-replace.c | 2 +-
>  fs/btrfs/fs.h          | 2 --
>  fs/btrfs/super.c       | 3 +--
>  fs/btrfs/volumes.c     | 4 ++--
>  4 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 2decb9fff445..cf63f4b29327 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -250,7 +250,7 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
>  	}
>  
>  	bdev_file = bdev_file_open_by_path(device_path, BLK_OPEN_WRITE,
> -					fs_info->bdev_holder, NULL);
> +					   fs_info, NULL);
>  	if (IS_ERR(bdev_file)) {
>  		btrfs_err(fs_info, "target device %s is invalid!", device_path);
>  		return PTR_ERR(bdev_file);
> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> index b239e4b8421c..d90304d4e32c 100644
> --- a/fs/btrfs/fs.h
> +++ b/fs/btrfs/fs.h
> @@ -715,8 +715,6 @@ struct btrfs_fs_info {
>  	u32 data_chunk_allocations;
>  	u32 metadata_ratio;
>  
> -	void *bdev_holder;
> -
>  	/* Private scrub information */
>  	struct mutex scrub_lock;
>  	atomic_t scrubs_running;
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 2d0d8c6e77b4..c1efd20166cc 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1865,7 +1865,7 @@ 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);
> +	ret = btrfs_open_devices(fs_devices, mode, fs_info);
>  	mutex_unlock(&uuid_mutex);
>  	if (ret)
>  		return ret;
> @@ -1905,7 +1905,6 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>  	} else {
>  		snprintf(sb->s_id, sizeof(sb->s_id), "%pg", 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);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index d3e749328e0f..606ddf42ddc3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2705,7 +2705,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  		return -EROFS;
>  
>  	bdev_file = bdev_file_open_by_path(device_path, BLK_OPEN_WRITE,
> -					fs_info->bdev_holder, NULL);
> +					   fs_info, NULL);
>  	if (IS_ERR(bdev_file))
>  		return PTR_ERR(bdev_file);
>  
> @@ -7168,7 +7168,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
>  	if (IS_ERR(fs_devices))
>  		return fs_devices;
>  
> -	ret = open_fs_devices(fs_devices, BLK_OPEN_READ, fs_info->bdev_holder);
> +	ret = open_fs_devices(fs_devices, BLK_OPEN_READ, fs_info);
>  	if (ret) {
>  		free_fs_devices(fs_devices);
>  		return ERR_PTR(ret);
> -- 
> 2.49.0
>