| Message ID | 20240427211230.GF1495312@ZenIV (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] bcache_register(): don't bother with set_blocksize() | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Sat, Apr 27, 2024 at 10:12:30PM +0100, Al Viro wrote: > We don't have bdev opened exclusive there. And I'm rather dubious > about the need to do set_blocksize() anywhere in btrfs, to be > honest - there's some access to page cache of underlying block > devices in there, but it's nowhere near the hot paths, AFAICT. > > In any case, btrfs_get_dev_args_from_path() only needs to read > the on-disk superblock and copy several fields out of it; all > callers are only interested in devices that are already opened > and brought into per-filesystem set, so setting the block size > is redundant for those and actively harmful if we are given > a pathname of unrelated device. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- Looks good to me, Reviewed-by: Christian Brauner <brauner@kernel.org>
On Sat, Apr 27, 2024 at 10:12:30PM +0100, Al Viro wrote: > We don't have bdev opened exclusive there. And I'm rather dubious > about the need to do set_blocksize() anywhere in btrfs, to be > honest - there's some access to page cache of underlying block > devices in there, but it's nowhere near the hot paths, AFAICT. Long time ago we fixed a bug that involved set_blocksize(), 6f60cbd3ae44 ("btrfs: access superblock via pagecache in scan_one_device"). Concurrent access from scan, mount and mkfs could interact and some writes would be dropped, but the argument was rather not to use set_blocksize. I do not recall all the details but I think that the problem was when it was called in the middle of the other operation in progress. The only reason it's ever called is for the super block read and to call it explicitly from our code rather than rely on some eventual call from block layer. But it's more than 10 years ago and things have changed, we don't use buffer_head for superblock anymore. > In any case, btrfs_get_dev_args_from_path() only needs to read > the on-disk superblock and copy several fields out of it; all > callers are only interested in devices that are already opened > and brought into per-filesystem set, so setting the block size > is redundant for those and actively harmful if we are given > a pathname of unrelated device. Calling set_blocksize on already opened devices will avoid the scan/mount/mkfs interactions so this seems safe. > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/btrfs/volumes.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index f15591f3e54f..43af5a9fb547 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -482,10 +482,12 @@ btrfs_get_bdev_and_sb(const char *device_path, blk_mode_t flags, void *holder, > > if (flush) > sync_blockdev(bdev); > - ret = set_blocksize(bdev, BTRFS_BDEV_BLOCKSIZE); > - if (ret) { > - fput(*bdev_file); > - goto error; > + if (holder) { > + ret = set_blocksize(bdev, BTRFS_BDEV_BLOCKSIZE); The subject mentions a different function, you're removing it from btrfs_get_bdev_and_sb() not btrfs_get_dev_args_from_path(). > + if (ret) { > + fput(*bdev_file); > + goto error; > + } > } > invalidate_bdev(bdev); > *disk_super = btrfs_read_dev_super(bdev); > @@ -498,6 +500,7 @@ btrfs_get_bdev_and_sb(const char *device_path, blk_mode_t flags, void *holder, > return 0; > > error: > + *disk_super = NULL; > *bdev_file = NULL; > return ret; > } > -- > 2.39.2 >
On Mon, Apr 29, 2024 at 05:11:25PM +0200, David Sterba wrote: > > - if (ret) { > > - fput(*bdev_file); > > - goto error; > > + if (holder) { > > + ret = set_blocksize(bdev, BTRFS_BDEV_BLOCKSIZE); > > The subject mentions a different function, you're removing it from > btrfs_get_bdev_and_sb() not btrfs_get_dev_args_from_path(). ... conditional upon holder being NULL, which happens only when called by btrfs_get_dev_args_from_path().
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index f15591f3e54f..43af5a9fb547 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -482,10 +482,12 @@ btrfs_get_bdev_and_sb(const char *device_path, blk_mode_t flags, void *holder, if (flush) sync_blockdev(bdev); - ret = set_blocksize(bdev, BTRFS_BDEV_BLOCKSIZE); - if (ret) { - fput(*bdev_file); - goto error; + if (holder) { + ret = set_blocksize(bdev, BTRFS_BDEV_BLOCKSIZE); + if (ret) { + fput(*bdev_file); + goto error; + } } invalidate_bdev(bdev); *disk_super = btrfs_read_dev_super(bdev); @@ -498,6 +500,7 @@ btrfs_get_bdev_and_sb(const char *device_path, blk_mode_t flags, void *holder, return 0; error: + *disk_super = NULL; *bdev_file = NULL; return ret; }
We don't have bdev opened exclusive there. And I'm rather dubious about the need to do set_blocksize() anywhere in btrfs, to be honest - there's some access to page cache of underlying block devices in there, but it's nowhere near the hot paths, AFAICT. In any case, btrfs_get_dev_args_from_path() only needs to read the on-disk superblock and copy several fields out of it; all callers are only interested in devices that are already opened and brought into per-filesystem set, so setting the block size is redundant for those and actively harmful if we are given a pathname of unrelated device. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/btrfs/volumes.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)