Re: [PATCH 14/17] btrfs: make btrfs_readpage_end_io_hook() follow sector size

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

[-- Attachment #1.1: Type: text/plain, Size: 2416 bytes --]



On 2020/9/10 上午1:34, Goldwyn Rodrigues wrote:
> On 15:52 08/09, Qu Wenruo wrote:
>> Currently btrfs_readpage_end_io_hook() just pass the whole page to
>> check_data_csum(), which is fine since we only support sectorsize ==
>> PAGE_SIZE.
>>
>> To support subpage RO support, we need to properly honor per-sector
>> checksum verification, just like what we did in dio read path.
>>
>> This patch will do the csum verification in a for loop, starts with
>> pg_off == start - page_offset(page), with sectorsize increasement for
>> each loop.
>>
>> For sectorsize == PAGE_SIZE case, the pg_off will always be 0, and we
>> will only finish with just one loop.
>>
>> For subpage, we do the proper loop.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/inode.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 078735aa0f68..8bd14dda2067 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -2851,9 +2851,12 @@ static int btrfs_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>>  				      u64 start, u64 end, int mirror)
>>  {
>>  	size_t offset = start - page_offset(page);
>> +	size_t pg_off;
>>  	struct inode *inode = page->mapping->host;
>>  	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
>>  	struct btrfs_root *root = BTRFS_I(inode)->root;
>> +	u32 sectorsize = root->fs_info->sectorsize;
>> +	bool found_err = false;
>>  
>>  	if (PageChecked(page)) {
>>  		ClearPageChecked(page);
>> @@ -2870,7 +2873,17 @@ static int btrfs_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>>  	}
>>  
>>  	phy_offset >>= inode->i_sb->s_blocksize_bits;
>> -	return check_data_csum(inode, io_bio, phy_offset, page, offset);
>> +	for (pg_off = offset; pg_off < end - page_offset(page);
>> +	     pg_off += sectorsize, phy_offset++) {
>> +		int ret;
>> +
>> +		ret = check_data_csum(inode, io_bio, phy_offset, page, pg_off);
>> +		if (ret < 0)
>> +			found_err = true;
>> +	}
>> +	if (found_err)
>> +		return -EIO;
>> +	return 0;
>>  }
> 
> We don't need found_err here. Just return ret when you encounter the
> first error.
> 
But that means, the whole range will be marked error, while some sectors
may still contain good data and pass the csum checking.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]