Re: [PATCH 2/2] btrfs: use more straightforward disk_bytenr to replace icsum for check_data_csum()

[Nikolay Borisov <nborisov@suse.com>]

On 9.11.20 г. 13:54 ч., Qu Wenruo wrote:
> Parameter @icsum for check_data_csum() is a little hard to understand.
> It is the offset in sectors compared to io_bio->logical.

This second sentence is confusing because io_bio->logical is used for repair/dio bios and not buffered whilst  icsum is calculated independently of io_bio->logical so I'd suggest you remove it. 
> 
> Instead of using the calculated value, let's go with disk_bytenr, as the
> new name is not only straightforward,  but also utilized in a lot of
> existing code for file items.

Just say that instead of passing in the calculated offset couple of levels deep you modify the code to instead pass disk_bytenr of currently processed biovec and use that to calculate the offset closer to actual users of it. Kind of like what I did below. 

> 
> To get the old @icsum value, we simply use
> (disk_bytenr - (io_bio->bio.bi_iter.bi_sector << 9)) >>
> fs_info->sectorsize_bits;
> 
> This patch would separate file offset with disk_bytenr completely, to
> reduce the confusion.

I find this description somewhat confusing, what you are doing is just moving the sector offset calculation closer to where it's being used, rather than calculating it in the top level endio handler and passing it several levels down to where it's actually used - in the csum verification function. So where is file offset involved?

Otherwise the code LGTM apart from some minor nits below. 

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 14 ++++++++------
>  fs/btrfs/inode.c     | 35 ++++++++++++++++++++++++++---------
>  2 files changed, 34 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);

needless parentheses.

>  	u64 start;
>  	u64 end;
>  	u64 len;

<snip>

> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c54e0ed0b938..eff987931f0d 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2843,19 +2843,27 @@ void btrfs_writepage_endio_finish_ordered(struct page *page, u64 start,
>   * The length of such check is always one sector size.
>   */

It's not evident from the hunk but you should also modify the parameter description of this function since we no longer have 'icsum'

>  static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,
> -			   int icsum, struct page *page, int pgoff)
> +			   u64 disk_bytenr, 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;
> +	u64 bio_disk_bytenr = (io_bio->bio.bi_iter.bi_sector << 9);

Again, extra parentheses, they don't bring anything in this particular expression. 

> +	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;
> +	/* Our disk_bytenr should be inside the io_bio */
> +	ASSERT(bio_disk_bytenr <= disk_bytenr &&
> +	       disk_bytenr < bio_disk_bytenr + io_bio->bio.bi_iter.bi_size);

nit: in_range(disk_bytenr, bio_disk_bytenr, io_bio->bio.bi_iter.bi_size); 

IMO the assert is redundant since it's obvious disk_bytenr will always be within range, but perhahps it's needed for your future subpage work so I'm not going to insist on removing it. 

> +
> +	offset_sectors = (disk_bytenr - bio_disk_bytenr) >>
> +			 fs_info->sectorsize_bits;
> +	csum_expected = ((u8 *)io_bio->csum) + offset_sectors * csum_size;
>  
>  	kaddr = kmap_atomic(page);
>  	shash->tfm = fs_info->csum_shash;
> @@ -2883,8 +2891,13 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,

<snip>