Re: [PATCH 1/2] btrfs: merge btrfs_read_dev_one_super() into btrfs_read_disk_super()

[David Sterba <dsterba@suse.cz>]

On Mon, Apr 28, 2025 at 10:45:51AM +0930, Qu Wenruo wrote:
> -struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
> -						   int copy_num, bool drop_cache)
> -{

> -	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
> -		btrfs_release_disk_super(super);
> -		return ERR_PTR(-ENODATA);
> -	}
> -
> -	if (btrfs_super_bytenr(super) != bytenr_orig) {
> -		btrfs_release_disk_super(super);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
> -	return super;
> -}
> -
> -
>  struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev)
>  {
>  	struct btrfs_super_block *super, *latest = NULL;
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1401,48 +1401,62 @@ void btrfs_release_disk_super(struct btrfs_super_block *super)
>  	put_page(page);
>  }
>  
> -static struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev,
> -						       u64 bytenr, u64 bytenr_orig)
> +struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev,
> +						int copy_num, bool drop_cache)
>  {
> -	struct btrfs_super_block *disk_super;
> +	struct btrfs_super_block *super;
>  	struct page *page;
> -	void *p;
> -	pgoff_t index;
> +	u64 bytenr, bytenr_orig;
> +	struct address_space *mapping = bdev->bd_mapping;
> +	int ret;
>  
> -	/* make sure our super fits in the device */
> -	if (bytenr + PAGE_SIZE >= bdev_nr_bytes(bdev))
> +	bytenr_orig = btrfs_sb_offset(copy_num);
> +	ret = btrfs_sb_log_location_bdev(bdev, copy_num, READ, &bytenr);
> +	if (ret == -ENOENT)
> +		return ERR_PTR(-EINVAL);
> +	else if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (bytenr + BTRFS_SUPER_INFO_SIZE >= bdev_nr_bytes(bdev))
>  		return ERR_PTR(-EINVAL);
>  
> -	/* make sure our super fits in the page */
> -	if (sizeof(*disk_super) > PAGE_SIZE)
> -		return ERR_PTR(-EINVAL);
> +	if (drop_cache) {
> +		/* This should only be called with the primary sb. */
> +		ASSERT(copy_num == 0);
>  
> -	/* make sure our super doesn't straddle pages on disk */
> -	index = bytenr >> PAGE_SHIFT;
> -	if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_SHIFT != index)
> -		return ERR_PTR(-EINVAL);
> -
> -	/* pull in the page with our super */
> -	page = read_cache_page_gfp(bdev->bd_mapping, index, GFP_KERNEL);
> +		/*
> +		 * Drop the page of the primary superblock, so later read will
> +		 * always read from the device.
> +		 */
> +		invalidate_inode_pages2_range(mapping,
> +				bytenr >> PAGE_SHIFT,
> +				(bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT);
> +	}
>  
> +	page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);
>  	if (IS_ERR(page))
>  		return ERR_CAST(page);
>  
> -	p = page_address(page);
> +	super = page_address(page);
> +	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
> +		btrfs_release_disk_super(super);
> +		return ERR_PTR(-ENODATA);

This was in the other function but I think we should unify that to
-EINVAL, other filesystems do not distinguish between a failed magic
check and other errors.