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

[Qu Wenruo <wqu@suse.com>]

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