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

[PATCH v3 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.

Changelog:
v2:
- Update commit message to remove the wrong comment on
  btrfs_io_bio->logical
  That logical is mess, it has different meanings for different use
  cases.
  What we should refer to is bio->bi_iter.bi_sector.

- Remove the false-alert prone ASSERT()
  Even at endio time. bio->bi_iter.bi_size can change due to incoming
  finished IOs.
  This means we can't really rely on bio->bi_iter.bi_size to check if
  our disk_bytenr is still valid.

v3:
- Rename the @offset/phy_offset to @bio_offset to avoid confusion
  It turns out that, the name used in v2, @disk_bytenr, can also be
  confusing, as in endio time, the bi_sector can be remapped for
  real device offset.
  Although it doesn't cause problem since we're only using that value to
  calculate the offset to the beginning of the bio.

  But still, we're trying to remove confusion, not adding more, thus
  rename them to bio_offset.


Qu Wenruo (2):
  btrfs: remove the phy_offset parameter for
    btrfs_validate_metadata_buffer()
  btrfs: pass bio_offset to check_data_csum() directly

 fs/btrfs/disk-io.c   |  2 +-
 fs/btrfs/disk-io.h   |  2 +-
 fs/btrfs/extent_io.c | 16 +++++++++-------
 fs/btrfs/inode.c     | 26 ++++++++++++++++----------
 4 files changed, 27 insertions(+), 19 deletions(-)

-- 
2.29.2

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

[Qu Wenruo]

Parameter @phy_offset is the offset against the bio->bi_iter.bi_sector.
@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 case, 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>
---
 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 f0161d2ae2a4..8a558a43818d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -593,7 +593,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
 	return ret;
 }
 
-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 e75ea6092942..40e81d6e481e 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 e17429d3bd3a..d26677745757 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 v3 2/2] btrfs: pass bio_offset to check_data_csum() directly

[Qu Wenruo]

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>
---
 fs/btrfs/ctree.h     |  2 +-
 fs/btrfs/extent_io.c | 14 ++++++++------
 fs/btrfs/inode.c     | 26 ++++++++++++++++----------
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 679da4920c92..99955b6bfc62 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3046,7 +3046,7 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path);
 /* inode.c */
 blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
 				   int mirror_num, unsigned long bio_flags);
-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 bio_offset,
 			   struct page *page, u64 start, u64 end, int mirror);
 struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
 					   u64 start, u64 len);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d26677745757..72aac9a007ee 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 bio_offset = 0;
 	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,
+						bio_offset, 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, bio_offset,
+						page,
 						start - page_offset(page),
 						start, end, mirror,
 						btrfs_submit_data_bio)) {
 				uptodate = !bio->bi_status;
-				offset += len;
+				bio_offset += 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;
+		bio_offset += 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 435b270430e3..bcf3152d0efb 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2931,26 +2931,28 @@ void btrfs_writepage_endio_finish_ordered(struct page *page, u64 start,
  * check_data_csum - verify checksum of one sector of uncompressed data
  * @inode:	the inode
  * @io_bio:	btrfs_io_bio which contains the csum
- * @icsum:	checksum index in the io_bio->csum array, size of csum_size
+ * @bio_offset:	the offset to the beginning of the bio (in bytes)
  * @page:	page where is the data to be verified
  * @pgoff:	offset inside the page
  *
  * 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 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;
+	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)
+ * @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 bio_offset,
 			   struct page *page, u64 start, u64 end, int mirror)
 {
 	size_t offset = start - page_offset(page);
@@ -3004,8 +3011,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, bio_offset, page, offset);
 }
 
 /*
@@ -7716,7 +7722,7 @@ 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;
+	u64 bio_offset = 0;
 	blk_status_t err = BLK_STS_OK;
 
 	__bio_for_each_segment(bvec, &io_bio->bio, iter, io_bio->iter) {
@@ -7727,8 +7733,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,
+					bio_offset, bvec.bv_page, pgoff))) {
 				clean_io_failure(fs_info, failure_tree, io_tree,
 						 start, bvec.bv_page,
 						 btrfs_ino(BTRFS_I(inode)),
@@ -7748,7 +7754,7 @@ static blk_status_t btrfs_check_read_dio_bio(struct inode *inode,
 					err = status;
 			}
 			start += sectorsize;
-			icsum++;
+			bio_offset += sectorsize;
 			pgoff += sectorsize;
 		}
 	}
-- 
2.29.2

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

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

[Nikolay Borisov]

On 12.11.20 г. 10:47 ч., 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>


That seems to be a simple rename + moving the conversion between
bytes->sectors into check_data_csum.

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

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

[Josef Bacik]

On 11/12/20 3:47 AM, Qu Wenruo wrote:
> Parameter @phy_offset is the offset against the bio->bi_iter.bi_sector.
> @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 case, 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>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

[Josef Bacik]

On 11/12/20 3:47 AM, 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>
> ---
>   fs/btrfs/ctree.h     |  2 +-
>   fs/btrfs/extent_io.c | 14 ++++++++------
>   fs/btrfs/inode.c     | 26 ++++++++++++++++----------
>   3 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 679da4920c92..99955b6bfc62 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3046,7 +3046,7 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path);
>   /* inode.c */
>   blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>   				   int mirror_num, unsigned long bio_flags);
> -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 bio_offset,
>   			   struct page *page, u64 start, u64 end, int mirror);
>   struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
>   					   u64 start, u64 len);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index d26677745757..72aac9a007ee 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 bio_offset = 0;
>   	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,
> +						bio_offset, 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, bio_offset,
> +						page,
>   						start - page_offset(page),
>   						start, end, mirror,
>   						btrfs_submit_data_bio)) {
>   				uptodate = !bio->bi_status;
> -				offset += len;
> +				bio_offset += 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;
> +		bio_offset += 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 435b270430e3..bcf3152d0efb 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2931,26 +2931,28 @@ void btrfs_writepage_endio_finish_ordered(struct page *page, u64 start,
>    * check_data_csum - verify checksum of one sector of uncompressed data
>    * @inode:	the inode
>    * @io_bio:	btrfs_io_bio which contains the csum
> - * @icsum:	checksum index in the io_bio->csum array, size of csum_size
> + * @bio_offset:	the offset to the beginning of the bio (in bytes)
>    * @page:	page where is the data to be verified
>    * @pgoff:	offset inside the page
>    *
>    * 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 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;
> +	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)

beginning, I'd recommend doing the btrfs-setup-git-hooks to catch these things 
before you submit.  Other than that you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

[David Sterba]

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