[PATCH 0/2] fix and optimize btrfs_lookup_bio_sums()

[PATCH 0/2] fix and optimize btrfs_lookup_bio_sums()

[Anand Jain]

The function btrfs_lookup_bio_sums() has %ret declaration two times
 blk_status_t ret = BLK_STS_OK;
and also in an if statement block.
 int ret;

Patch 1/2 addresses this issue, while patch 2/2 optimizes the return
value of search_file_offset_in_bio().

Anand Jain (2):
  btrfs: avoid reusing variable names in nested blocks
  btrfs: optimize search_file_offset_in_bio return value to bool

 fs/btrfs/file-item.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

-- 
2.31.1

[PATCH 1/2] btrfs: avoid reusing variable names in nested blocks

[Anand Jain]

The function btrfs_lookup_bio_sums() and a contained if statement declares
%ret respectively as blk_status_t and int.

There is no need to store the return value of search_file_offset_in_bio()
into %ret in the if statement block, remove it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/file-item.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 41c77a100853..89e9415b8f06 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -494,12 +494,11 @@ 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;
-				int ret;
 
-				ret = search_file_offset_in_bio(bio,
-						&inode->vfs_inode,
-						cur_disk_bytenr, &file_offset);
-				if (ret)
+				if (search_file_offset_in_bio(bio,
+							      &inode->vfs_inode,
+							      cur_disk_bytenr,
+							      &file_offset))
 					set_extent_bits(io_tree, file_offset,
 						file_offset + sectorsize - 1,
 						EXTENT_NODATASUM);
-- 
2.31.1

[PATCH 2/2] btrfs: optimize search_file_offset_in_bio return value to bool

[Anand Jain]

Function search_file_offset_in_bio() finds the file offset in the
%file_offset_ret, and we use the return value to indicate if it is
successful, so use bool.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/file-item.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 89e9415b8f06..a879210735aa 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -345,8 +345,8 @@ static int search_csum_tree(struct btrfs_fs_info *fs_info,
  *
  * @inode is used to determine if the bvec page really belongs to @inode.
  *
- * Return 0 if we can't find the file offset
- * Return >0 if we find the file offset and restore it to @file_offset_ret
+ * Return true if we can't find the file offset
+ * Return false 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)
@@ -354,7 +354,7 @@ static int search_file_offset_in_bio(struct bio *bio, struct inode *inode,
 	struct bvec_iter iter;
 	struct bio_vec bvec;
 	u64 cur = bio->bi_iter.bi_sector << SECTOR_SHIFT;
-	int ret = 0;
+	bool ret = false;
 
 	bio_for_each_segment(bvec, bio, iter) {
 		struct page *page = bvec.bv_page;
@@ -368,7 +368,7 @@ static int search_file_offset_in_bio(struct bio *bio, struct inode *inode,
 		ASSERT(in_range(disk_bytenr, cur, bvec.bv_len));
 		if (page->mapping && page->mapping->host &&
 		    page->mapping->host == inode) {
-			ret = 1;
+			ret = true;
 			*file_offset_ret = page_offset(page) + bvec.bv_offset +
 					   disk_bytenr - cur;
 			break;
-- 
2.31.1

Re: [PATCH 1/2] btrfs: avoid reusing variable names in nested blocks

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] btrfs: optimize search_file_offset_in_bio return value to bool

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] btrfs: optimize search_file_offset_in_bio return value to bool

[David Sterba]

On Sat, Feb 11, 2023 at 12:15:55AM +0800, Anand Jain wrote:
> Function search_file_offset_in_bio() finds the file offset in the
> %file_offset_ret, and we use the return value to indicate if it is
> successful, so use bool.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/file-item.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 89e9415b8f06..a879210735aa 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -345,8 +345,8 @@ static int search_csum_tree(struct btrfs_fs_info *fs_info,
>   *
>   * @inode is used to determine if the bvec page really belongs to @inode.
>   *
> - * Return 0 if we can't find the file offset
> - * Return >0 if we find the file offset and restore it to @file_offset_ret
> + * Return true if we can't find the file offset
> + * Return false if we find the file offset and restore it to @file_offset_ret

The comment seems to not match the code, true is returned when the
offset is found, previously >0. Fixed.

>   */
>  static int search_file_offset_in_bio(struct bio *bio, struct inode *inode,
>  				     u64 disk_bytenr, u64 *file_offset_ret)
> @@ -354,7 +354,7 @@ static int search_file_offset_in_bio(struct bio *bio, struct inode *inode,
>  	struct bvec_iter iter;
>  	struct bio_vec bvec;
>  	u64 cur = bio->bi_iter.bi_sector << SECTOR_SHIFT;
> -	int ret = 0;
> +	bool ret = false;
>  
>  	bio_for_each_segment(bvec, bio, iter) {
>  		struct page *page = bvec.bv_page;
> @@ -368,7 +368,7 @@ static int search_file_offset_in_bio(struct bio *bio, struct inode *inode,
>  		ASSERT(in_range(disk_bytenr, cur, bvec.bv_len));
>  		if (page->mapping && page->mapping->host &&
>  		    page->mapping->host == inode) {
> -			ret = 1;
> +			ret = true;
>  			*file_offset_ret = page_offset(page) + bvec.bv_offset +
>  					   disk_bytenr - cur;
>  			break;
> -- 
> 2.31.1

Re: [PATCH 0/2] fix and optimize btrfs_lookup_bio_sums()

[David Sterba]

On Sat, Feb 11, 2023 at 12:15:53AM +0800, Anand Jain wrote:
> The function btrfs_lookup_bio_sums() has %ret declaration two times
>  blk_status_t ret = BLK_STS_OK;
> and also in an if statement block.
>  int ret;
> 
> Patch 1/2 addresses this issue, while patch 2/2 optimizes the return
> value of search_file_offset_in_bio().
> 
> Anand Jain (2):
>   btrfs: avoid reusing variable names in nested blocks
>   btrfs: optimize search_file_offset_in_bio return value to bool

Added to misc-next with some fixups, thanks.

Re: [PATCH 2/2] btrfs: optimize search_file_offset_in_bio return value to bool

[Anand Jain]

On 2/21/23 04:16, David Sterba wrote:
> On Sat, Feb 11, 2023 at 12:15:55AM +0800, Anand Jain wrote:
>> Function search_file_offset_in_bio() finds the file offset in the
>> %file_offset_ret, and we use the return value to indicate if it is
>> successful, so use bool.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/file-item.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>> index 89e9415b8f06..a879210735aa 100644
>> --- a/fs/btrfs/file-item.c
>> +++ b/fs/btrfs/file-item.c
>> @@ -345,8 +345,8 @@ static int search_csum_tree(struct btrfs_fs_info *fs_info,
>>    *
>>    * @inode is used to determine if the bvec page really belongs to @inode.
>>    *
>> - * Return 0 if we can't find the file offset
>> - * Return >0 if we find the file offset and restore it to @file_offset_ret
>> + * Return true if we can't find the file offset
>> + * Return false if we find the file offset and restore it to @file_offset_ret
> 
> The comment seems to not match the code, true is returned when the
> offset is found, previously >0. Fixed.
> 
  Oh.  Thanks for fixing it.
-Anand.