[PATCH 1/1] btrfs: simplify lookup_data_extent()

[PATCH 1/1] btrfs: simplify lookup_data_extent()

[Heinrich Schuchardt]

After returning if ret <= 0 we know that ret > 0. No need to check it.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 fs/btrfs/inode.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d00b515333..0173d30cd8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -546,15 +546,12 @@ static int lookup_data_extent(struct btrfs_root *root, struct btrfs_path *path,
 	/* Error or we're already at the file extent */
 	if (ret <= 0)
 		return ret;
-	if (ret > 0) {
-		/* Check previous file extent */
-		ret = btrfs_previous_item(root, path, ino,
-					  BTRFS_EXTENT_DATA_KEY);
-		if (ret < 0)
-			return ret;
-		if (ret > 0)
-			goto check_next;
-	}
+	/* Check previous file extent */
+	ret = btrfs_previous_item(root, path, ino, BTRFS_EXTENT_DATA_KEY);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		goto check_next;
 	/* Now the key.offset must be smaller than @file_offset */
 	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
 	if (key.objectid != ino ||
-- 
2.34.1

Re: [PATCH 1/1] btrfs: simplify lookup_data_extent()

[Qu Wenruo]

On 2022/5/11 03:43, Heinrich Schuchardt wrote:
> After returning if ret <= 0 we know that ret > 0. No need to check it.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/inode.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d00b515333..0173d30cd8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -546,15 +546,12 @@ static int lookup_data_extent(struct btrfs_root *root, struct btrfs_path *path,
>   	/* Error or we're already at the file extent */
>   	if (ret <= 0)
>   		return ret;
> -	if (ret > 0) {
> -		/* Check previous file extent */
> -		ret = btrfs_previous_item(root, path, ino,
> -					  BTRFS_EXTENT_DATA_KEY);
> -		if (ret < 0)
> -			return ret;
> -		if (ret > 0)
> -			goto check_next;
> -	}
> +	/* Check previous file extent */
> +	ret = btrfs_previous_item(root, path, ino, BTRFS_EXTENT_DATA_KEY);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		goto check_next;
>   	/* Now the key.offset must be smaller than @file_offset */
>   	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>   	if (key.objectid != ino ||

Re: [PATCH 1/1] btrfs: simplify lookup_data_extent()

[Qu Wenruo]

On 2022/5/11 06:48, Qu Wenruo wrote:
>
>
> On 2022/5/11 03:43, Heinrich Schuchardt wrote:
>> After returning if ret <= 0 we know that ret > 0. No need to check it.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Just to mention for other guys in the btrfs list, this patch is for
U-boot btrfs implementation.

And I also checked btrfs-fuse project, which has a similar function,
lookup_file_extent(), it already does the check properly and even do
extra quick exit for (ret > 0 && path->slots[0] == 0) case.

So you may want to also check btrfs-fuse project to find some possible
optimization and cross-port to U-boot.
(So far btrfs-fuse has better test coverage using fsstress, and
cross-checked against kernel).

Thanks,
Qu
>
> Thanks,
> Qu
>> ---
>>   fs/btrfs/inode.c | 15 ++++++---------
>>   1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index d00b515333..0173d30cd8 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -546,15 +546,12 @@ static int lookup_data_extent(struct btrfs_root
>> *root, struct btrfs_path *path,
>>       /* Error or we're already at the file extent */
>>       if (ret <= 0)
>>           return ret;
>> -    if (ret > 0) {
>> -        /* Check previous file extent */
>> -        ret = btrfs_previous_item(root, path, ino,
>> -                      BTRFS_EXTENT_DATA_KEY);
>> -        if (ret < 0)
>> -            return ret;
>> -        if (ret > 0)
>> -            goto check_next;
>> -    }
>> +    /* Check previous file extent */
>> +    ret = btrfs_previous_item(root, path, ino, BTRFS_EXTENT_DATA_KEY);
>> +    if (ret < 0)
>> +        return ret;
>> +    if (ret > 0)
>> +        goto check_next;
>>       /* Now the key.offset must be smaller than @file_offset */
>>       btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>       if (key.objectid != ino ||

Re: [PATCH 1/1] btrfs: simplify lookup_data_extent()

[Anand Jain]

On 5/11/22 01:13, Heinrich Schuchardt wrote:
> After returning if ret <= 0 we know that ret > 0. No need to check it.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Reviewed-by: Anand Jain <anand.jain>


> ---
>   fs/btrfs/inode.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d00b515333..0173d30cd8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -546,15 +546,12 @@ static int lookup_data_extent(struct btrfs_root *root, struct btrfs_path *path,
>   	/* Error or we're already at the file extent */
>   	if (ret <= 0)
>   		return ret;
> -	if (ret > 0) {
> -		/* Check previous file extent */
> -		ret = btrfs_previous_item(root, path, ino,
> -					  BTRFS_EXTENT_DATA_KEY);
> -		if (ret < 0)
> -			return ret;
> -		if (ret > 0)
> -			goto check_next;
> -	}
> +	/* Check previous file extent */
> +	ret = btrfs_previous_item(root, path, ino, BTRFS_EXTENT_DATA_KEY);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		goto check_next;
>   	/* Now the key.offset must be smaller than @file_offset */
>   	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>   	if (key.objectid != ino ||

Re: [PATCH 1/1] btrfs: simplify lookup_data_extent()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Tue, May 10, 2022 at 09:43:38PM +0200, Heinrich Schuchardt wrote:

> After returning if ret <= 0 we know that ret > 0. No need to check it.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Anand Jain <anand.jain>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]