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

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

[Qu Wenruo]

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.

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

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

[Boris Burkov]

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
> 

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

[Qu Wenruo]

在 2025/6/6 02:35, Boris Burkov 写道:
> 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.

I guess that series didn't get in as no review?

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

The holder change itself is independent, and the main reason I'm pushing 
is, I'm on the way implementing the blk_holder_ops (mostly for the 
mark_dead() callback), thus need a way to grab the fs info from a bdev.

If we can passing fs_info directly around, without a sb wrapper, I think 
it should be fine.
(Bcachefs passes a structure holding its fs_info too)

Thanks,
Qu

> 
> 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
>>