[PATCH 0/2] btrfs: paramater refactors for data and metadata endio call backs

[PATCH 0/2] btrfs: paramater refactors for data and metadata endio call backs

[Qu Wenruo]

This is another cleanup exposed when I'm fixing my subpage patchset.

Dating back to the old time where we still have hooks for data/metadata
endio, we have a parameter called @phy_offset for both hooks.

That @phy_offset is the number of sectors compared to the bio on-disk
bytenr, and is used to grab the csum from btrfs_io_bio.

This is far from straightforward, and costs reader tons of time to grasp
the basic.

This patchset will change it by:
- Remove phy_offset completely for metadata
  Since metadata doesn't use btrfs_io_bio::csums[] at all, there is no
  need for it.

- Use @disk_bytenr to replace @phy_offset/@icsum
  Let the callee, check_data_csum() to calculate the offset from
  @disk_bytenr and bio to get the csum offset.

Also, since we know the @disk_bytenr should alwasy be inside the bio
range, add ASSERT() to check such assumption.

Qu Wenruo (2):
  btrfs: remove the phy_offset parameter for
    btrfs_validate_metadata_buffer()
  btrfs: use more straightforward disk_bytenr to replace icsum for
    check_data_csum()

 fs/btrfs/disk-io.c   |  2 +-
 fs/btrfs/disk-io.h   |  2 +-
 fs/btrfs/extent_io.c | 16 +++++++++-------
 fs/btrfs/inode.c     | 35 ++++++++++++++++++++++++++---------
 4 files changed, 37 insertions(+), 18 deletions(-)

-- 
2.29.2

[PATCH 1/2] btrfs: remove the phy_offset parameter for btrfs_validate_metadata_buffer()

[Qu Wenruo]

Parameter @phy_offset is the offset against the io_bio->logical (which
is the disk bytenr).

@phy_offset is mostly for data io to lookup the csum in btrfs_io_bio.

But for metadata, it's completely useless as metadata stores their own
csum in its btrfs_header.

Remove this useless parameter from btrfs_validate_metadata_buffer().

Just an extra note for parameters @start and @end, they are not utilized
at all for current sectorsize == PAGE_SIZE, as we can grab eb directly
from page.

But those two parameters are very important for later subpage support,
thus @start/@len are not touched here.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c   | 2 +-
 fs/btrfs/disk-io.h   | 2 +-
 fs/btrfs/extent_io.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c70a52b44ceb..bd6e357dd280 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -524,7 +524,7 @@ static int check_tree_block_fsid(struct extent_buffer *eb)
 	return 1;
 }
 
-int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio, u64 phy_offset,
+int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio,
 				   struct page *page, u64 start, u64 end,
 				   int mirror)
 {
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 238b45223f2e..76ede62737fd 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -79,7 +79,7 @@ void btrfs_btree_balance_dirty(struct btrfs_fs_info *fs_info);
 void btrfs_btree_balance_dirty_nodelay(struct btrfs_fs_info *fs_info);
 void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
 				 struct btrfs_root *root);
-int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio, u64 phy_offset,
+int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio,
 				   struct page *page, u64 start, u64 end,
 				   int mirror);
 blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b4381d7ca52c..bd5a22bfee68 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2928,7 +2928,7 @@ static void end_bio_extent_readpage(struct bio *bio)
 							     start, end, mirror);
 			else
 				ret = btrfs_validate_metadata_buffer(io_bio,
-					offset, page, start, end, mirror);
+					page, start, end, mirror);
 			if (ret)
 				uptodate = 0;
 			else
-- 
2.29.2

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

[Qu Wenruo]

Parameter @icsum for check_data_csum() is a little hard to understand.
It is the offset in sectors compared to io_bio->logical.

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.

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.

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);
 	u64 start;
 	u64 end;
 	u64 len;
@@ -2924,8 +2924,9 @@ static void end_bio_extent_readpage(struct bio *bio)
 		mirror = io_bio->mirror_num;
 		if (likely(uptodate)) {
 			if (is_data_inode(inode))
-				ret = btrfs_verify_data_csum(io_bio, offset, page,
-							     start, end, mirror);
+				ret = btrfs_verify_data_csum(io_bio,
+						disk_bytenr, page, start, end,
+						mirror);
 			else
 				ret = btrfs_validate_metadata_buffer(io_bio,
 					page, start, end, mirror);
@@ -2953,12 +2954,13 @@ static void end_bio_extent_readpage(struct bio *bio)
 			 * If it can't handle the error it will return -EIO and
 			 * we remain responsible for that page.
 			 */
-			if (!btrfs_submit_read_repair(inode, bio, offset, page,
+			if (!btrfs_submit_read_repair(inode, bio, disk_bytenr,
+						page,
 						start - page_offset(page),
 						start, end, mirror,
 						btrfs_submit_data_bio)) {
 				uptodate = !bio->bi_status;
-				offset += len;
+				disk_bytenr += len;
 				continue;
 			}
 		} else {
@@ -2983,7 +2985,7 @@ static void end_bio_extent_readpage(struct bio *bio)
 			if (page->index == end_index && off)
 				zero_user_segment(page, off, PAGE_SIZE);
 		}
-		offset += len;
+		disk_bytenr += len;
 
 		endio_readpage_update_page_status(page, uptodate);
 		endio_readpage_release_extent(&processed, BTRFS_I(inode),
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.
  */
 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);
+	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);
+
+	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,
  * 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.
+ *
+ * @disk_bytenr: The on-disk bytenr of the range start
+ * @start:	 The file offset of the range start
+ * @end:	 The file offset of the range end (inclusive)
+ * @mirror:	 The mirror number
  */
-int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
+int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 disk_bytenr,
 			   struct page *page, u64 start, u64 end, int mirror)
 {
 	size_t offset = start - page_offset(page);
@@ -2909,8 +2922,7 @@ int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
 		return 0;
 	}
 
-	phy_offset >>= root->fs_info->sectorsize_bits;
-	return check_data_csum(inode, io_bio, phy_offset, page, offset);
+	return check_data_csum(inode, io_bio, disk_bytenr, page, offset);
 }
 
 /*
@@ -7616,7 +7628,12 @@ static blk_status_t btrfs_check_read_dio_bio(struct inode *inode,
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 	u64 start = io_bio->logical;
-	int icsum = 0;
+	/*
+	 * Dio io_bio uses file offset other than disk bytenr for
+	 * io_bio->logical.
+	 * Thus we need to grab disk_bytenr from bio.
+	 */
+	u64 disk_bytenr = (io_bio->bio.bi_iter.bi_sector << 9);
 	blk_status_t err = BLK_STS_OK;
 
 	__bio_for_each_segment(bvec, &io_bio->bio, iter, io_bio->iter) {
@@ -7627,8 +7644,8 @@ static blk_status_t btrfs_check_read_dio_bio(struct inode *inode,
 		for (i = 0; i < nr_sectors; i++) {
 			ASSERT(pgoff < PAGE_SIZE);
 			if (uptodate &&
-			    (!csum || !check_data_csum(inode, io_bio, icsum,
-						       bvec.bv_page, pgoff))) {
+			    (!csum || !check_data_csum(inode, io_bio,
+					disk_bytenr, bvec.bv_page, pgoff))) {
 				clean_io_failure(fs_info, failure_tree, io_tree,
 						 start, bvec.bv_page,
 						 btrfs_ino(BTRFS_I(inode)),
@@ -7648,7 +7665,7 @@ static blk_status_t btrfs_check_read_dio_bio(struct inode *inode,
 					err = status;
 			}
 			start += sectorsize;
-			icsum++;
+			disk_bytenr += sectorsize;
 			pgoff += sectorsize;
 		}
 	}
-- 
2.29.2

Re: [PATCH 0/2] btrfs: paramater refactors for data and metadata endio call backs

[Qu Wenruo]

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



On 2020/11/9 下午7:54, Qu Wenruo wrote:
> This is another cleanup exposed when I'm fixing my subpage patchset.
> 
> Dating back to the old time where we still have hooks for data/metadata
> endio, we have a parameter called @phy_offset for both hooks.
> 
> That @phy_offset is the number of sectors compared to the bio on-disk

Well, phy_offset is the number of *bytes*, other than sectors.

I guess the mistake has already prove the point, it's really easy to get
screwed up for the @phy_offset and @icsum bad naming.

Thanks,
Qu

> bytenr, and is used to grab the csum from btrfs_io_bio.
> 
> This is far from straightforward, and costs reader tons of time to grasp
> the basic.
> 
> This patchset will change it by:
> - Remove phy_offset completely for metadata
>   Since metadata doesn't use btrfs_io_bio::csums[] at all, there is no
>   need for it.
> 
> - Use @disk_bytenr to replace @phy_offset/@icsum
>   Let the callee, check_data_csum() to calculate the offset from
>   @disk_bytenr and bio to get the csum offset.
> 
> Also, since we know the @disk_bytenr should alwasy be inside the bio
> range, add ASSERT() to check such assumption.
> 
> Qu Wenruo (2):
>   btrfs: remove the phy_offset parameter for
>     btrfs_validate_metadata_buffer()
>   btrfs: use more straightforward disk_bytenr to replace icsum for
>     check_data_csum()
> 
>  fs/btrfs/disk-io.c   |  2 +-
>  fs/btrfs/disk-io.h   |  2 +-
>  fs/btrfs/extent_io.c | 16 +++++++++-------
>  fs/btrfs/inode.c     | 35 ++++++++++++++++++++++++++---------
>  4 files changed, 37 insertions(+), 18 deletions(-)
> 


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

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

[Nikolay Borisov]

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>

Re: [PATCH 1/2] btrfs: remove the phy_offset parameter for btrfs_validate_metadata_buffer()

[Nikolay Borisov]

On 9.11.20 г. 13:54 ч., Qu Wenruo wrote:
> Parameter @phy_offset is the offset against the io_bio->logical (which
> is the disk bytenr).
> 
> @phy_offset is mostly for data io to lookup the csum in btrfs_io_bio.
> 
> But for metadata, it's completely useless as metadata stores their own
> csum in its btrfs_header.
> 
> Remove this useless parameter from btrfs_validate_metadata_buffer().
> 
> Just an extra note for parameters @start and @end, they are not utilized
> at all for current sectorsize == PAGE_SIZE, as we can grab eb directly
> from page.
> 
> But those two parameters are very important for later subpage support,
> thus @start/@len are not touched here.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

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

[Qu Wenruo]

On 2020/11/9 下午8:19, Nikolay Borisov wrote:
>
>
> 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.

Right, I also find the io_bio::logical inconsistency.
What I want to say is (io_bio->bio.bi_iter.bi_sector << 9).
>>
>> 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?

Well, you see the parameter @start and @end of btrfs_verify_data_csum()
right?

That's why we want to distinguish the file offset from on-disk bytenr.

>
> 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.

The "bio_disk_bytenr <= disk_bytenr" is obvious and I'm fine to remove
it, but the later "disk_bytenr < bio_disk_bytenr +
io_bio->bio.bi_iter.bi_size" is a completely valid check.

This is even more important since we use disk_bytenr to calculate where
the expected csum is.
If disk_bytenr goes beyond bio range, it will cause memory access out of
boudary.

In fact, this assert() has already caught cases in my later patches
where I forgot bv_len can go beyond current page.

Thanks,
Qu
>
>> +
>> +	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>
>