Re: [PATCH v2 2/2] btrfs: pass disk_bytenr directly for check_data_csum()

[Josef Bacik <josef@toxicpanda.com>]

On 11/9/20 9:09 PM, 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 @disk_bytenr and let the
> final and only user, check_data_csum(), to calculate whatever it needs.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/extent_io.c | 14 ++++++++------
>   fs/btrfs/inode.c     | 26 +++++++++++++++++---------
>   2 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index bd5a22bfee68..f8b5d3d4e5b0 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2878,7 +2878,7 @@ static void end_bio_extent_readpage(struct bio *bio)
>   	struct btrfs_io_bio *io_bio = btrfs_io_bio(bio);
>   	struct extent_io_tree *tree, *failure_tree;
>   	struct processed_extent processed = { 0 };
> -	u64 offset = 0;
> +	u64 disk_bytenr = (bio->bi_iter.bi_sector << 9);

This doesn't work, bi_sector can be remapped based on the underlying device, and 
thus can be different between submit and endio.  To illustrate this point, make 
2 partitions on a single device, mkfs the second partition, and then run 
xfstests with this patch applied, all sorts of fun will happen.

In fact we should probably add such a test to xfstests to catch anybody relying 
on bi_sector to stay the same.  Thanks,

Josef