Re: [PATCH v3 2/2] btrfs: pass bio_offset to check_data_csum() directly

[David Sterba <dsterba@suse.cz>]

On Thu, Nov 12, 2020 at 04:47:58PM +0800, Qu Wenruo wrote:
> Parameter @icsum for check_data_csum() is a little hard to understand.
> So is the @phy_offset for btrfs_verify_data_csum().
> 
> Both parameters are calculated values for csum lookup.
> 
> Instead of some calculated value, just pass @bio_offset and let the
> final and only user, check_data_csum(), to calculate whatever it needs.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

This has been sent independently and is also part of the subpage
patchset but without the reviewed-by tags.

>  static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,
> -			   int icsum, struct page *page, int pgoff)
> +			   u64 bio_offset, struct page *page, int pgoff)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
>  	char *kaddr;
>  	u32 len = fs_info->sectorsize;
>  	const u32 csum_size = fs_info->csum_size;
> +	int offset_sectors;
>  	u8 *csum_expected;
>  	u8 csum[BTRFS_CSUM_SIZE];
>  
>  	ASSERT(pgoff + len <= PAGE_SIZE);
>  
> -	csum_expected = ((u8 *)io_bio->csum) + icsum * csum_size;
> +	offset_sectors = bio_offset >> fs_info->sectorsize_bits;

	int = u64 >> u32

This does not match up, either bio_offset is unnecessarily wide or
offset_sectors needs to be widened.

> +	csum_expected = ((u8 *)io_bio->csum) + offset_sectors * csum_size;
>  
>  	kaddr = kmap_atomic(page);
>  	shash->tfm = fs_info->csum_shash;
> @@ -2978,8 +2980,13 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,
>   * when reads are done, we need to check csums to verify the data is correct
>   * if there's a match, we allow the bio to finish.  If not, the code in
>   * extent_io.c will try to find good copies for us.
> + *
> + * @bio_offset:	The offset to the begining of the bio (in bytes)

For parameter description please drop the initial 'The'

> + * @start:	The file offset of the range start
> + * @end:	The file offset of the range end (inclusive)
> + * @mirror:	The mirror number