Re: [PATCH 6/7] btrfs_get_dev_args_from_path(): don't call set_blocksize()

From: David Sterba <dsterba@suse.cz>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Christian Brauner <brauner@kernel.org>,
	Christoph Hellwig <hch@lst.de>,
	linux-block@vger.kernel.org, Jens Axboe <axboe@kernel.dk>,
	linux-btrfs@vger.kernel.org,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 6/7] btrfs_get_dev_args_from_path(): don't call set_blocksize()
Date: Mon, 29 Apr 2024 17:11:25 +0200	[thread overview]
Message-ID: <20240429151124.GC2585@twin.jikos.cz> (raw)
In-Reply-To: <20240427211230.GF1495312@ZenIV>

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
> 