cleanup btrfs_lookup_bio_sums

cleanup btrfs_lookup_bio_sums

[Christoph Hellwig]

Hi all,

this series takes advantage of the file_offset now provided in each btrfs_bio
to clean up btrfs_lookup_bio_sums.

Diffstat:
 file-item.c |   81 ++++++++----------------------------------------------------
 1 file changed, 11 insertions(+), 70 deletions(-)

[PATCH 1/2] btrfs: remove search_file_offset_in_bio

[Christoph Hellwig]

There is no need to search for a file offset in a bio, it is now always
provided in bbio->file_offset.  Just use that adjusted to the offset
into the bio.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/file-item.c | 52 +++-----------------------------------------
 1 file changed, 3 insertions(+), 49 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index fff09e5635e5f2..9df9b91dbc6463 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -335,48 +335,6 @@ static int search_csum_tree(struct btrfs_fs_info *fs_info,
 	return ret;
 }
 
-/*
- * Locate the file_offset of @cur_disk_bytenr of a @bio.
- *
- * Bio of btrfs represents read range of
- * [bi_sector << 9, bi_sector << 9 + bi_size).
- * Knowing this, we can iterate through each bvec to locate the page belong to
- * @cur_disk_bytenr and get the file offset.
- *
- * @inode is used to determine if the bvec page really belongs to @inode.
- *
- * Return false if we can't find the file offset
- * Return true if we find the file offset and restore it to @file_offset_ret
- */
-static int search_file_offset_in_bio(struct bio *bio, struct inode *inode,
-				     u64 disk_bytenr, u64 *file_offset_ret)
-{
-	struct bvec_iter iter;
-	struct bio_vec bvec;
-	u64 cur = bio->bi_iter.bi_sector << SECTOR_SHIFT;
-	bool ret = false;
-
-	bio_for_each_segment(bvec, bio, iter) {
-		struct page *page = bvec.bv_page;
-
-		if (cur > disk_bytenr)
-			break;
-		if (cur + bvec.bv_len <= disk_bytenr) {
-			cur += bvec.bv_len;
-			continue;
-		}
-		ASSERT(in_range(disk_bytenr, cur, bvec.bv_len));
-		if (page->mapping && page->mapping->host &&
-		    page->mapping->host == inode) {
-			ret = true;
-			*file_offset_ret = page_offset(page) + bvec.bv_offset +
-					   disk_bytenr - cur;
-			break;
-		}
-	}
-	return ret;
-}
-
 /*
  * Lookup the checksum for the read bio in csum tree.
  *
@@ -386,7 +344,6 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 {
 	struct btrfs_inode *inode = bbio->inode;
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
-	struct extent_io_tree *io_tree = &inode->io_tree;
 	struct bio *bio = &bbio->bio;
 	struct btrfs_path *path;
 	const u32 sectorsize = fs_info->sectorsize;
@@ -493,13 +450,10 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 
 			if (inode->root->root_key.objectid ==
 			    BTRFS_DATA_RELOC_TREE_OBJECTID) {
-				u64 file_offset;
+				u64 file_offset = bbio->file_offset +
+					cur_disk_bytenr - orig_disk_bytenr;
 
-				if (search_file_offset_in_bio(bio,
-							      &inode->vfs_inode,
-							      cur_disk_bytenr,
-							      &file_offset))
-					set_extent_bits(io_tree, file_offset,
+				set_extent_bits(&inode->io_tree, file_offset,
 						file_offset + sectorsize - 1,
 						EXTENT_NODATASUM);
 			} else {
-- 
2.39.1

Re: [PATCH 1/2] btrfs: remove search_file_offset_in_bio

[Johannes Thumshirn]

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

Re: [PATCH 1/2] btrfs: remove search_file_offset_in_bio

[Anand Jain]

LGTM.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

[PATCH 2/2] btrfs: cleanup btrfs_lookup_bio_sums

[Christoph Hellwig]

Introduce a bio_offset variable for the current offset into the bio
instead of recalculating it over and over, remove the now superflous
search_len and reduce variable scope as applicable as well as switch
count to and unsigned type for the unsigned values it holds.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/file-item.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 9df9b91dbc6463..efc914b2e17e08 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -350,10 +350,9 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 	const u32 csum_size = fs_info->csum_size;
 	u32 orig_len = bio->bi_iter.bi_size;
 	u64 orig_disk_bytenr = bio->bi_iter.bi_sector << SECTOR_SHIFT;
-	u64 cur_disk_bytenr;
 	const unsigned int nblocks = orig_len >> fs_info->sectorsize_bits;
-	int count = 0;
 	blk_status_t ret = BLK_STS_OK;
+	u32 bio_offset = 0;
 
 	if ((inode->flags & BTRFS_INODE_NODATASUM) ||
 	    test_bit(BTRFS_FS_STATE_NO_CSUMS, &fs_info->fs_state))
@@ -404,25 +403,12 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 		path->skip_locking = 1;
 	}
 
-	for (cur_disk_bytenr = orig_disk_bytenr;
-	     cur_disk_bytenr < orig_disk_bytenr + orig_len;
-	     cur_disk_bytenr += (count * sectorsize)) {
-		u64 search_len = orig_disk_bytenr + orig_len - cur_disk_bytenr;
-		unsigned int sector_offset;
-		u8 *csum_dst;
-
-		/*
-		 * Although both cur_disk_bytenr and orig_disk_bytenr is u64,
-		 * we're calculating the offset to the bio start.
-		 *
-		 * Bio size is limited to UINT_MAX, thus unsigned int is large
-		 * enough to contain the raw result, not to mention the right
-		 * shifted result.
-		 */
-		ASSERT(cur_disk_bytenr - orig_disk_bytenr < UINT_MAX);
-		sector_offset = (cur_disk_bytenr - orig_disk_bytenr) >>
-				fs_info->sectorsize_bits;
-		csum_dst = bbio->csum + sector_offset * csum_size;
+	while (bio_offset < orig_len) {
+		u64 cur_disk_bytenr = orig_disk_bytenr + bio_offset;
+		u64 search_len = orig_len - bio_offset;
+		u8 *csum_dst = bbio->csum +
+			(bio_offset >> fs_info->sectorsize_bits) * csum_size;
+		u32 count = 0;
 
 		count = search_csum_tree(fs_info, path, cur_disk_bytenr,
 					 search_len, csum_dst);
@@ -451,7 +437,7 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 			if (inode->root->root_key.objectid ==
 			    BTRFS_DATA_RELOC_TREE_OBJECTID) {
 				u64 file_offset = bbio->file_offset +
-					cur_disk_bytenr - orig_disk_bytenr;
+					bio_offset;
 
 				set_extent_bits(&inode->io_tree, file_offset,
 						file_offset + sectorsize - 1,
@@ -462,6 +448,7 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 				cur_disk_bytenr, cur_disk_bytenr + sectorsize);
 			}
 		}
+		bio_offset += count * sectorsize;
 	}
 
 	btrfs_free_path(path);
-- 
2.39.1

Re: [PATCH 2/2] btrfs: cleanup btrfs_lookup_bio_sums

[Johannes Thumshirn]

On 21.02.23 21:57, Christoph Hellwig wrote:
> +	while (bio_offset < orig_len) {
> +		u64 cur_disk_bytenr = orig_disk_bytenr + bio_offset;
> +		u64 search_len = orig_len - bio_offset;
> +		u8 *csum_dst = bbio->csum +
> +			(bio_offset >> fs_info->sectorsize_bits) * csum_size;
> +		u32 count = 0;
>  
>  		count = search_csum_tree(fs_info, path, cur_disk_bytenr,
>  					 search_len, csum_dst);

That wont work, as a) search_csum_tree() returns an 'int' and b)
count is checked to be < 0 (aka error) the line below this diff:


                count = search_csum_tree(fs_info, path, cur_disk_bytenr,                                                                                                                                                                                                                                                                                                   
                                         search_len, csum_dst);                
                if (count < 0) {                                               
                        ret = errno_to_blk_status(count);                      
                        if (bbio->csum != bbio->csum_inline)                   
                                kfree(bbio->csum);                             
                        bbio->csum = NULL;                                     
                        break;                                                 
                }           


So count has to stay 'int'.

Thanks,
	Johannes

Re: [PATCH 2/2] btrfs: cleanup btrfs_lookup_bio_sums

[Christoph Hellwig]

On Wed, Feb 22, 2023 at 11:57:12AM +0000, Johannes Thumshirn wrote:
> 
> That wont work, as a) search_csum_tree() returns an 'int' and b)
> count is checked to be < 0 (aka error) the line below this diff:

Indeed.  I'll resend.

cleanup btrfs_lookup_bio_sums v2

[Christoph Hellwig]

Hi all,

this series takes advantage of the file_offset now provided in each btrfs_bio
to clean up btrfs_lookup_bio_sums.

Changs since v1:
 - keep the count variable signed
 - remove the search_len variable

Diffstat:
 file-item.c |   82 ++++++++----------------------------------------------------
 1 file changed, 11 insertions(+), 71 deletions(-)

[PATCH 2/2] btrfs: cleanup btrfs_lookup_bio_sums

[Christoph Hellwig]

Introduce a bio_offset variable for the current offset into the bio
instead of recalculating it over and over.   Remove the now only
used once search_len and sector_offset variables, and reduce the
scope for count and cur_disk_bytenr.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/file-item.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 9df9b91dbc6463..3e0995ce7a2c83 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -350,10 +350,9 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 	const u32 csum_size = fs_info->csum_size;
 	u32 orig_len = bio->bi_iter.bi_size;
 	u64 orig_disk_bytenr = bio->bi_iter.bi_sector << SECTOR_SHIFT;
-	u64 cur_disk_bytenr;
 	const unsigned int nblocks = orig_len >> fs_info->sectorsize_bits;
-	int count = 0;
 	blk_status_t ret = BLK_STS_OK;
+	u32 bio_offset = 0;
 
 	if ((inode->flags & BTRFS_INODE_NODATASUM) ||
 	    test_bit(BTRFS_FS_STATE_NO_CSUMS, &fs_info->fs_state))
@@ -404,28 +403,14 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 		path->skip_locking = 1;
 	}
 
-	for (cur_disk_bytenr = orig_disk_bytenr;
-	     cur_disk_bytenr < orig_disk_bytenr + orig_len;
-	     cur_disk_bytenr += (count * sectorsize)) {
-		u64 search_len = orig_disk_bytenr + orig_len - cur_disk_bytenr;
-		unsigned int sector_offset;
-		u8 *csum_dst;
-
-		/*
-		 * Although both cur_disk_bytenr and orig_disk_bytenr is u64,
-		 * we're calculating the offset to the bio start.
-		 *
-		 * Bio size is limited to UINT_MAX, thus unsigned int is large
-		 * enough to contain the raw result, not to mention the right
-		 * shifted result.
-		 */
-		ASSERT(cur_disk_bytenr - orig_disk_bytenr < UINT_MAX);
-		sector_offset = (cur_disk_bytenr - orig_disk_bytenr) >>
-				fs_info->sectorsize_bits;
-		csum_dst = bbio->csum + sector_offset * csum_size;
+	while (bio_offset < orig_len) {
+		u64 cur_disk_bytenr = orig_disk_bytenr + bio_offset;
+		u8 *csum_dst = bbio->csum +
+			(bio_offset >> fs_info->sectorsize_bits) * csum_size;
+		int count;
 
 		count = search_csum_tree(fs_info, path, cur_disk_bytenr,
-					 search_len, csum_dst);
+					 orig_len - bio_offset, csum_dst);
 		if (count < 0) {
 			ret = errno_to_blk_status(count);
 			if (bbio->csum != bbio->csum_inline)
@@ -451,7 +436,7 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 			if (inode->root->root_key.objectid ==
 			    BTRFS_DATA_RELOC_TREE_OBJECTID) {
 				u64 file_offset = bbio->file_offset +
-					cur_disk_bytenr - orig_disk_bytenr;
+					bio_offset;
 
 				set_extent_bits(&inode->io_tree, file_offset,
 						file_offset + sectorsize - 1,
@@ -462,6 +447,7 @@ blk_status_t btrfs_lookup_bio_sums(struct btrfs_bio *bbio)
 				cur_disk_bytenr, cur_disk_bytenr + sectorsize);
 			}
 		}
+		bio_offset += count * sectorsize;
 	}
 
 	btrfs_free_path(path);
-- 
2.39.1

Re: [PATCH 2/2] btrfs: cleanup btrfs_lookup_bio_sums

[Johannes Thumshirn]

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