* [PATCH] btrfs: use fs_info as the block device holder @ 2025-06-05 9:45 Qu Wenruo 2025-06-05 17:05 ` Boris Burkov 0 siblings, 1 reply; 3+ messages in thread From: Qu Wenruo @ 2025-06-05 9:45 UTC (permalink / raw) To: linux-btrfs 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: use fs_info as the block device holder 2025-06-05 9:45 [PATCH] btrfs: use fs_info as the block device holder Qu Wenruo @ 2025-06-05 17:05 ` Boris Burkov 2025-06-05 22:17 ` Qu Wenruo 0 siblings, 1 reply; 3+ messages in thread From: Boris Burkov @ 2025-06-05 17:05 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: use fs_info as the block device holder 2025-06-05 17:05 ` Boris Burkov @ 2025-06-05 22:17 ` Qu Wenruo 0 siblings, 0 replies; 3+ messages in thread From: Qu Wenruo @ 2025-06-05 22:17 UTC (permalink / raw) To: Boris Burkov; +Cc: linux-btrfs 在 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 >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-06-05 22:18 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-05 9:45 [PATCH] btrfs: use fs_info as the block device holder Qu Wenruo 2025-06-05 17:05 ` Boris Burkov 2025-06-05 22:17 ` Qu Wenruo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox