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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2025/4/29 16:52, David Sterba 写道:
> 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.
> 

Sure, merged both super magic and super bytenr checks into the same if 
(), and return ERR_PTR(-EINVAL).

Will push the updated version (with Johannes and this one modified) to 
for-next after getting enough reviews.

Thanks,
Qu