[PATCH 00/29] trivial adjustments for return variable coding style

[PATCH 00/29] trivial adjustments for return variable coding style

[Anand Jain]

Rename variable 'err'; instead, use 'ret', and the 'ret' helper variable
is renamed to 'ret2'.

https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#code

In functions where 'ret' is already used as a return helper (but not the
actual return), to avoid oversight, first rename the original 'ret'
variable to 'ret2', compile it, and then rename 'err' to 'ret'.

Anand Jain (29):
  btrfs: btrfs_cleanup_fs_roots rename ret to ret2 and err to ret
  btrfs: btrfs_initxattrs rename err to ret
  btrfs: send_extent_data rename err to ret
  btrfs: btrfs_rmdir rename err to ret
  btrfs: btrfs_cont_expand rename err to ret
  btrfs: btrfs_setsize rename err to ret2
  btrfs: btrfs_find_orphan_roots rename ret to ret2 and err to ret
  btrfs: btrfs_ioctl_snap_destroy rename err to ret
  btrfs: __set_extent_bit rename err to ret
  btrfs: convert_extent_bit rename err to ret
  btrfs: __btrfs_end_transaction rename err to ret
  btrfs: btrfs_write_marked_extents rename werr to ret err to ret2
  btrfs: __btrfs_wait_marked_extents rename werr to ret err to ret2
  btrfs: build_backref_tree rename err to ret and ret to ret2
  btrfs: relocate_tree_blocks rename ret to ret2 and err to ret
  btrfs: relocate_block_group rename ret to ret2 and err to ret
  btrfs: create_reloc_inode rename err to ret
  btrfs: btrfs_relocate_block_group rename ret to ret2 and err ro ret
  btrfs: mark_garbage_root rename err to ret2
  btrfs: btrfs_recover_relocation rename ret to ret2 and err to ret
  btrfs: quick_update_accounting rename err to ret2
  btrfs: btrfs_qgroup_rescan_worker rename ret to ret2 and err to ret
  btrfs: lookup_extent_data_ref rename ret to ret2 and err to ret
  btrfs: btrfs_drop_snapshot rename ret to ret2 and err to ret
  btrfs: btrfs_drop_subtree rename retw to ret2
  btrfs: btrfs_dirty_pages rename variable err to ret
  btrfs: prepare_pages rename err to ret
  btrfs: btrfs_direct_write rename err to ret
  btrfs: fixup_tree_root_location rename ret to ret2 and err to ret

 fs/btrfs/disk-io.c        |  22 +--
 fs/btrfs/extent-io-tree.c |  58 ++++----
 fs/btrfs/extent-tree.c    | 122 ++++++++---------
 fs/btrfs/file.c           |  80 +++++------
 fs/btrfs/inode.c          |  88 ++++++------
 fs/btrfs/ioctl.c          |  66 ++++-----
 fs/btrfs/qgroup.c         |  44 +++---
 fs/btrfs/relocation.c     | 280 +++++++++++++++++++-------------------
 fs/btrfs/root-tree.c      |  36 ++---
 fs/btrfs/send.c           |   4 +-
 fs/btrfs/transaction.c    |  56 ++++----
 fs/btrfs/xattr.c          |  10 +-
 12 files changed, 434 insertions(+), 432 deletions(-)

-- 
2.38.1

[PATCH 01/29] btrfs: btrfs_cleanup_fs_roots rename ret to ret2 and err to ret

[Anand Jain]

Since err represents the function return value, rename it as ret,
and rename the original ret, which serves as a helper return value,
to ret2.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 3df5477d48a8..d28de2cfb7b4 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
 	u64 root_objectid = 0;
 	struct btrfs_root *gang[8];
 	int i = 0;
-	int err = 0;
-	unsigned int ret = 0;
+	int ret = 0;
+	unsigned int ret2 = 0;
 
 	while (1) {
 		spin_lock(&fs_info->fs_roots_radix_lock);
-		ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
+		ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
 					     (void **)gang, root_objectid,
 					     ARRAY_SIZE(gang));
-		if (!ret) {
+		if (!ret2) {
 			spin_unlock(&fs_info->fs_roots_radix_lock);
 			break;
 		}
-		root_objectid = gang[ret - 1]->root_key.objectid + 1;
+		root_objectid = gang[ret2 - 1]->root_key.objectid + 1;
 
-		for (i = 0; i < ret; i++) {
+		for (i = 0; i < ret2; i++) {
 			/* Avoid to grab roots in dead_roots. */
 			if (btrfs_root_refs(&gang[i]->root_item) == 0) {
 				gang[i] = NULL;
@@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
 		}
 		spin_unlock(&fs_info->fs_roots_radix_lock);
 
-		for (i = 0; i < ret; i++) {
+		for (i = 0; i < ret2; i++) {
 			if (!gang[i])
 				continue;
 			root_objectid = gang[i]->root_key.objectid;
-			err = btrfs_orphan_cleanup(gang[i]);
-			if (err)
+			ret = btrfs_orphan_cleanup(gang[i]);
+			if (ret)
 				goto out;
 			btrfs_put_root(gang[i]);
 		}
@@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
 	}
 out:
 	/* Release the uncleaned roots due to error. */
-	for (; i < ret; i++) {
+	for (; i < ret2; i++) {
 		if (gang[i])
 			btrfs_put_root(gang[i]);
 	}
-	return err;
+	return ret;
 }
 
 /*
-- 
2.38.1

Re: [PATCH 01/29] btrfs: btrfs_cleanup_fs_roots rename ret to ret2 and err to ret

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:09PM +0530, Anand Jain wrote:
> Since err represents the function return value, rename it as ret,
> and rename the original ret, which serves as a helper return value,
> to ret2.
> 

I hate to be that guy right out the gate, but since we're just using ret2 as the
return value of radix_tree_gang_lookup, and that's just the number of fs_roots,
lets instead a variation of

unsigned int nr_found = 0;
unsigned int nr = 0;
unsigned int found = 0;

since we know this isn't just a temporary ret value, it actually corresponds to
something.  Thanks,

Josef

Re: [PATCH 01/29] btrfs: btrfs_cleanup_fs_roots rename ret to ret2 and err to ret

[Qu Wenruo]

在 2024/3/20 01:25, Anand Jain 写道:
> Since err represents the function return value, rename it as ret,
> and rename the original ret, which serves as a helper return value,
> to ret2.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   fs/btrfs/disk-io.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 3df5477d48a8..d28de2cfb7b4 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>   	u64 root_objectid = 0;
>   	struct btrfs_root *gang[8];
>   	int i = 0;
> -	int err = 0;
> -	unsigned int ret = 0;
> +	int ret = 0;
> +	unsigned int ret2 = 0;

I really hate the same @ret2.

Since it's mostly the found number of radix tree gang lookup, can we
change it to something like @found?

>
>   	while (1) {

Another thing is, the @ret2 is only utilized inside the loop except the
final cleanup.

Can't we only declare @ret2 (or the new name) inside the loop and make
the cleanup also happen inside the loop (or a dedicated helper?)

Thanks,
Qu
>   		spin_lock(&fs_info->fs_roots_radix_lock);
> -		ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
> +		ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>   					     (void **)gang, root_objectid,
>   					     ARRAY_SIZE(gang));
> -		if (!ret) {
> +		if (!ret2) {
>   			spin_unlock(&fs_info->fs_roots_radix_lock);
>   			break;
>   		}
> -		root_objectid = gang[ret - 1]->root_key.objectid + 1;
> +		root_objectid = gang[ret2 - 1]->root_key.objectid + 1;
>
> -		for (i = 0; i < ret; i++) {
> +		for (i = 0; i < ret2; i++) {
>   			/* Avoid to grab roots in dead_roots. */
>   			if (btrfs_root_refs(&gang[i]->root_item) == 0) {
>   				gang[i] = NULL;
> @@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>   		}
>   		spin_unlock(&fs_info->fs_roots_radix_lock);
>
> -		for (i = 0; i < ret; i++) {
> +		for (i = 0; i < ret2; i++) {
>   			if (!gang[i])
>   				continue;
>   			root_objectid = gang[i]->root_key.objectid;
> -			err = btrfs_orphan_cleanup(gang[i]);
> -			if (err)
> +			ret = btrfs_orphan_cleanup(gang[i]);
> +			if (ret)
>   				goto out;
>   			btrfs_put_root(gang[i]);
>   		}
> @@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>   	}
>   out:
>   	/* Release the uncleaned roots due to error. */
> -	for (; i < ret; i++) {
> +	for (; i < ret2; i++) {
>   		if (gang[i])
>   			btrfs_put_root(gang[i]);
>   	}
> -	return err;
> +	return ret;
>   }
>
>   /*

Re: [PATCH 01/29] btrfs: btrfs_cleanup_fs_roots rename ret to ret2 and err to ret

[Anand Jain]

On 3/20/24 04:43, Qu Wenruo wrote:
> 
> 
> 在 2024/3/20 01:25, Anand Jain 写道:
>> Since err represents the function return value, rename it as ret,
>> and rename the original ret, which serves as a helper return value,
>> to ret2.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/disk-io.c | 22 +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 3df5477d48a8..d28de2cfb7b4 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct 
>> btrfs_fs_info *fs_info)
>>       u64 root_objectid = 0;
>>       struct btrfs_root *gang[8];
>>       int i = 0;
>> -    int err = 0;
>> -    unsigned int ret = 0;
>> +    int ret = 0;
>> +    unsigned int ret2 = 0;
> 
> I really hate the same @ret2.
> 
> Since it's mostly the found number of radix tree gang lookup, can we
> change it to something like @found?
> 
>>
>>       while (1) {
> 
> Another thing is, the @ret2 is only utilized inside the loop except the
> final cleanup.
> 
> Can't we only declare @ret2 (or the new name) inside the loop and make
> the cleanup also happen inside the loop (or a dedicated helper?)
> 

Cleanup inside the loop is possible, but it would be something like
below, what do you think?


diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c2dc88f909b0..d1d23736de3c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2926,22 +2926,23 @@ static int btrfs_cleanup_fs_roots(struct 
btrfs_fs_info *fs_info)
  {
         u64 root_objectid = 0;
         struct btrfs_root *gang[8];
-       int i = 0;
-       int err = 0;
-       unsigned int ret = 0;
+       int ret = 0;

         while (1) {
+               unsigned int i;
+               unsigned int found;
+
                 spin_lock(&fs_info->fs_roots_radix_lock);
-               ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
+               found = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
                                              (void **)gang, root_objectid,
                                              ARRAY_SIZE(gang));
-               if (!ret) {
+               if (!found) {
                         spin_unlock(&fs_info->fs_roots_radix_lock);
                         break;
                 }
-               root_objectid = btrfs_root_id(gang[ret - 1]) + 1;
+               root_objectid = btrfs_root_id(gang[found - 1]) + 1;

-               for (i = 0; i < ret; i++) {
+               for (i = 0; i < found; i++) {
                         /* Avoid to grab roots in dead_roots. */
                         if (btrfs_root_refs(&gang[i]->root_item) == 0) {
                                 gang[i] = NULL;

@@ -2952,24 +2953,20 @@ static int btrfs_cleanup_fs_roots(struct 
btrfs_fs_info *fs_info)
                 }
                 spin_unlock(&fs_info->fs_roots_radix_lock);

-               for (i = 0; i < ret; i++) {
+               for (i = 0; i < found; i++) {
                         if (!gang[i])
                                 continue;
                         root_objectid = btrfs_root_id(gang[i]);
-                       err = btrfs_orphan_cleanup(gang[i]);
-                       if (err)
-                               goto out;
+                       if (!ret)
+                               ret = btrfs_orphan_cleanup(gang[i]);
                         btrfs_put_root(gang[i]);
                 }
+               if (ret)
+                       break;
+
                 root_objectid++;
         }
-out:
-       /* Release the uncleaned roots due to error. */
-       for (; i < ret; i++) {
-               if (gang[i])
-                       btrfs_put_root(gang[i]);
-       }
-       return err;
+       return ret;
  }

Thanks,

Anand



> Thanks,
> Qu
>>           spin_lock(&fs_info->fs_roots_radix_lock);
>> -        ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>> +        ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>>                            (void **)gang, root_objectid,
>>                            ARRAY_SIZE(gang));
>> -        if (!ret) {
>> +        if (!ret2) {
>>               spin_unlock(&fs_info->fs_roots_radix_lock);
>>               break;
>>           }
>> -        root_objectid = gang[ret - 1]->root_key.objectid + 1;
>> +        root_objectid = gang[ret2 - 1]->root_key.objectid + 1;
>>
>> -        for (i = 0; i < ret; i++) {
>> +        for (i = 0; i < ret2; i++) {
>>               /* Avoid to grab roots in dead_roots. */
>>               if (btrfs_root_refs(&gang[i]->root_item) == 0) {
>>                   gang[i] = NULL;
>> @@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct 
>> btrfs_fs_info *fs_info)
>>           }
>>           spin_unlock(&fs_info->fs_roots_radix_lock);
>>
>> -        for (i = 0; i < ret; i++) {
>> +        for (i = 0; i < ret2; i++) {
>>               if (!gang[i])
>>                   continue;
>>               root_objectid = gang[i]->root_key.objectid;
>> -            err = btrfs_orphan_cleanup(gang[i]);
>> -            if (err)
>> +            ret = btrfs_orphan_cleanup(gang[i]);
>> +            if (ret)
>>                   goto out;
>>               btrfs_put_root(gang[i]);
>>           }
>> @@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct 
>> btrfs_fs_info *fs_info)
>>       }
>>   out:
>>       /* Release the uncleaned roots due to error. */
>> -    for (; i < ret; i++) {
>> +    for (; i < ret2; i++) {
>>           if (gang[i])
>>               btrfs_put_root(gang[i]);
>>       }
>> -    return err;
>> +    return ret;
>>   }
>>
>>   /*

Re: [PATCH 01/29] btrfs: btrfs_cleanup_fs_roots rename ret to ret2 and err to ret

[Qu Wenruo]

在 2024/4/17 12:54, Anand Jain 写道:
> On 3/20/24 04:43, Qu Wenruo wrote:
>>
>>
>> 在 2024/3/20 01:25, Anand Jain 写道:
>>> Since err represents the function return value, rename it as ret,
>>> and rename the original ret, which serves as a helper return value,
>>> to ret2.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/disk-io.c | 22 +++++++++++-----------
>>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index 3df5477d48a8..d28de2cfb7b4 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct 
>>> btrfs_fs_info *fs_info)
>>>       u64 root_objectid = 0;
>>>       struct btrfs_root *gang[8];
>>>       int i = 0;
>>> -    int err = 0;
>>> -    unsigned int ret = 0;
>>> +    int ret = 0;
>>> +    unsigned int ret2 = 0;
>>
>> I really hate the same @ret2.
>>
>> Since it's mostly the found number of radix tree gang lookup, can we
>> change it to something like @found?
>>
>>>
>>>       while (1) {
>>
>> Another thing is, the @ret2 is only utilized inside the loop except the
>> final cleanup.
>>
>> Can't we only declare @ret2 (or the new name) inside the loop and make
>> the cleanup also happen inside the loop (or a dedicated helper?)
>>
> 
> Cleanup inside the loop is possible, but it would be something like
> below, what do you think?

One less @ret, one less goto tag.

Only the btrfs_orphan_cleanup() call part needs some extra check, but 
still looks good to me.

Thanks,
Qu

> 
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index c2dc88f909b0..d1d23736de3c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2926,22 +2926,23 @@ static int btrfs_cleanup_fs_roots(struct 
> btrfs_fs_info *fs_info)
>   {
>          u64 root_objectid = 0;
>          struct btrfs_root *gang[8];
> -       int i = 0;
> -       int err = 0;
> -       unsigned int ret = 0;
> +       int ret = 0;
> 
>          while (1) {
> +               unsigned int i;
> +               unsigned int found;
> +
>                  spin_lock(&fs_info->fs_roots_radix_lock);
> -               ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
> +               found = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>                                               (void **)gang, root_objectid,
>                                               ARRAY_SIZE(gang));
> -               if (!ret) {
> +               if (!found) {
>                          spin_unlock(&fs_info->fs_roots_radix_lock);
>                          break;
>                  }
> -               root_objectid = btrfs_root_id(gang[ret - 1]) + 1;
> +               root_objectid = btrfs_root_id(gang[found - 1]) + 1;
> 
> -               for (i = 0; i < ret; i++) {
> +               for (i = 0; i < found; i++) {
>                          /* Avoid to grab roots in dead_roots. */
>                          if (btrfs_root_refs(&gang[i]->root_item) == 0) {
>                                  gang[i] = NULL;
> 
> @@ -2952,24 +2953,20 @@ static int btrfs_cleanup_fs_roots(struct 
> btrfs_fs_info *fs_info)
>                  }
>                  spin_unlock(&fs_info->fs_roots_radix_lock);
> 
> -               for (i = 0; i < ret; i++) {
> +               for (i = 0; i < found; i++) {
>                          if (!gang[i])
>                                  continue;
>                          root_objectid = btrfs_root_id(gang[i]);
> -                       err = btrfs_orphan_cleanup(gang[i]);
> -                       if (err)
> -                               goto out;
> +                       if (!ret)
> +                               ret = btrfs_orphan_cleanup(gang[i]);
>                          btrfs_put_root(gang[i]);
>                  }
> +               if (ret)
> +                       break;
> +
>                  root_objectid++;
>          }
> -out:
> -       /* Release the uncleaned roots due to error. */
> -       for (; i < ret; i++) {
> -               if (gang[i])
> -                       btrfs_put_root(gang[i]);
> -       }
> -       return err;
> +       return ret;
>   }
> 
> Thanks,
> 
> Anand
> 
> 
> 
>> Thanks,
>> Qu
>>>           spin_lock(&fs_info->fs_roots_radix_lock);
>>> -        ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>>> +        ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>>>                            (void **)gang, root_objectid,
>>>                            ARRAY_SIZE(gang));
>>> -        if (!ret) {
>>> +        if (!ret2) {
>>>               spin_unlock(&fs_info->fs_roots_radix_lock);
>>>               break;
>>>           }
>>> -        root_objectid = gang[ret - 1]->root_key.objectid + 1;
>>> +        root_objectid = gang[ret2 - 1]->root_key.objectid + 1;
>>>
>>> -        for (i = 0; i < ret; i++) {
>>> +        for (i = 0; i < ret2; i++) {
>>>               /* Avoid to grab roots in dead_roots. */
>>>               if (btrfs_root_refs(&gang[i]->root_item) == 0) {
>>>                   gang[i] = NULL;
>>> @@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct 
>>> btrfs_fs_info *fs_info)
>>>           }
>>>           spin_unlock(&fs_info->fs_roots_radix_lock);
>>>
>>> -        for (i = 0; i < ret; i++) {
>>> +        for (i = 0; i < ret2; i++) {
>>>               if (!gang[i])
>>>                   continue;
>>>               root_objectid = gang[i]->root_key.objectid;
>>> -            err = btrfs_orphan_cleanup(gang[i]);
>>> -            if (err)
>>> +            ret = btrfs_orphan_cleanup(gang[i]);
>>> +            if (ret)
>>>                   goto out;
>>>               btrfs_put_root(gang[i]);
>>>           }
>>> @@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct 
>>> btrfs_fs_info *fs_info)
>>>       }
>>>   out:
>>>       /* Release the uncleaned roots due to error. */
>>> -    for (; i < ret; i++) {
>>> +    for (; i < ret2; i++) {
>>>           if (gang[i])
>>>               btrfs_put_root(gang[i]);
>>>       }
>>> -    return err;
>>> +    return ret;
>>>   }
>>>
>>>   /*
> 
> 

Re: [PATCH 01/29] btrfs: btrfs_cleanup_fs_roots rename ret to ret2 and err to ret

[Goffredo Baroncelli]

On 19/03/2024 15.55, Anand Jain wrote:
> Since err represents the function return value, rename it as ret,
> and rename the original ret, which serves as a helper return value,
> to ret2.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   fs/btrfs/disk-io.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 3df5477d48a8..d28de2cfb7b4 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>   	u64 root_objectid = 0;
>   	struct btrfs_root *gang[8];
>   	int i = 0;

I suggest to change also the line above in
         unsigned int = 0;

This to avoid a comparation signed with unsigned in the two for() loop.
In this case this should be not a problem but in general is better to avoid
mix signed and unsigned.


> -	int err = 0;
> -	unsigned int ret = 0;
> +	int ret = 0;
> +	unsigned int ret2 = 0;

In this *specific* case, instead of renaming 'ret' in 'ret2', it would be better
rename 'ret' in 'items_count' or something like that. This because
radix_tree_gang_lookup() doesn't return a status (ok or an error), but the number of
the items found.

>   
>   	while (1) {
>   		spin_lock(&fs_info->fs_roots_radix_lock);
> -		ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
> +		ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>   					     (void **)gang, root_objectid,
>   					     ARRAY_SIZE(gang));
> -		if (!ret) {
> +		if (!ret2) {
>   			spin_unlock(&fs_info->fs_roots_radix_lock);
>   			break;
>   		}
> -		root_objectid = gang[ret - 1]->root_key.objectid + 1;
> +		root_objectid = gang[ret2 - 1]->root_key.objectid + 1;
>   
> -		for (i = 0; i < ret; i++) {
> +		for (i = 0; i < ret2; i++) {
>   			/* Avoid to grab roots in dead_roots. */
>   			if (btrfs_root_refs(&gang[i]->root_item) == 0) {
>   				gang[i] = NULL;
> @@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>   		}
>   		spin_unlock(&fs_info->fs_roots_radix_lock);
>   
> -		for (i = 0; i < ret; i++) {
> +		for (i = 0; i < ret2; i++) {

Comparation signed (i) with unsigned (ret2).

>   			if (!gang[i])
>   				continue;
>   			root_objectid = gang[i]->root_key.objectid;
> -			err = btrfs_orphan_cleanup(gang[i]);
> -			if (err)
> +			ret = btrfs_orphan_cleanup(gang[i]);
> +			if (ret)
>   				goto out;
>   			btrfs_put_root(gang[i]);
>   		}
> @@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>   	}
>   out:
>   	/* Release the uncleaned roots due to error. */
> -	for (; i < ret; i++) {
> +	for (; i < ret2; i++) {

Comparation signed (i) with unsigned (ret2).

>   		if (gang[i])
>   			btrfs_put_root(gang[i]);
>   	}
> -	return err;
> +	return ret;
>   }
>   
>   /*

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

[PATCH 02/29] btrfs: btrfs_initxattrs rename err to ret

[Anand Jain]

Simple renaming err to ret.

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

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 6287763fdccc..15d0999e340e 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -504,7 +504,7 @@ static int btrfs_initxattrs(struct inode *inode,
 	const struct xattr *xattr;
 	unsigned int nofs_flag;
 	char *name;
-	int err = 0;
+	int ret = 0;
 
 	/*
 	 * We're holding a transaction handle, so use a NOFS memory allocation
@@ -515,7 +515,7 @@ static int btrfs_initxattrs(struct inode *inode,
 		name = kmalloc(XATTR_SECURITY_PREFIX_LEN +
 			       strlen(xattr->name) + 1, GFP_KERNEL);
 		if (!name) {
-			err = -ENOMEM;
+			ret = -ENOMEM;
 			break;
 		}
 		strcpy(name, XATTR_SECURITY_PREFIX);
@@ -524,14 +524,14 @@ static int btrfs_initxattrs(struct inode *inode,
 		if (strcmp(name, XATTR_NAME_CAPS) == 0)
 			clear_bit(BTRFS_INODE_NO_CAP_XATTR, &BTRFS_I(inode)->runtime_flags);
 
-		err = btrfs_setxattr(trans, inode, name, xattr->value,
+		ret = btrfs_setxattr(trans, inode, name, xattr->value,
 				     xattr->value_len, 0);
 		kfree(name);
-		if (err < 0)
+		if (ret < 0)
 			break;
 	}
 	memalloc_nofs_restore(nofs_flag);
-	return err;
+	return ret;
 }
 
 int btrfs_xattr_security_init(struct btrfs_trans_handle *trans,
-- 
2.38.1

[PATCH 03/29] btrfs: send_extent_data rename err to ret

[Anand Jain]

Simple renaming of the local variable err to ret.

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

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 50b4a76ac88e..7b67c884b8e1 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -5748,10 +5748,10 @@ static int send_extent_data(struct send_ctx *sctx, struct btrfs_path *path,
 
 		sctx->cur_inode = btrfs_iget(root->fs_info->sb, sctx->cur_ino, root);
 		if (IS_ERR(sctx->cur_inode)) {
-			int err = PTR_ERR(sctx->cur_inode);
+			int ret = PTR_ERR(sctx->cur_inode);
 
 			sctx->cur_inode = NULL;
-			return err;
+			return ret;
 		}
 		memset(&sctx->ra, 0, sizeof(struct file_ra_state));
 		file_ra_state_init(&sctx->ra, sctx->cur_inode->i_mapping);
-- 
2.38.1

Re: [PATCH 03/29] btrfs: send_extent_data rename err to ret

[Filipe Manana]

On Tue, Mar 19, 2024 at 2:56 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> Simple renaming of the local variable err to ret.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/send.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 50b4a76ac88e..7b67c884b8e1 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -5748,10 +5748,10 @@ static int send_extent_data(struct send_ctx *sctx, struct btrfs_path *path,
>
>                 sctx->cur_inode = btrfs_iget(root->fs_info->sb, sctx->cur_ino, root);
>                 if (IS_ERR(sctx->cur_inode)) {
> -                       int err = PTR_ERR(sctx->cur_inode);
> +                       int ret = PTR_ERR(sctx->cur_inode);
>
>                         sctx->cur_inode = NULL;
> -                       return err;
> +                       return ret;

I think you're taking the consensus of having a return value variable
named 'ret' to the extreme.
That's generally what we should do, especially if the variable is
declared in a function's top level scope, there are many variables and
it's not clear what's used for the return value.

But here?
It's a very short scope for an error path and it's clear enough we are
returning an error and there's no room for confusion here with other
variables.

Doing this type of change causes unnecessary churn and extra work for
future backports of bug fixes for example.

Thanks.

>                 }
>                 memset(&sctx->ra, 0, sizeof(struct file_ra_state));
>                 file_ra_state_init(&sctx->ra, sctx->cur_inode->i_mapping);
> --
> 2.38.1
>
>

[PATCH 04/29] btrfs: btrfs_rmdir rename err to ret

[Anand Jain]

Simple renaming of the local variable err to ret.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 37701531eeb1..a2ad3bc8900b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4635,7 +4635,7 @@ static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
 	struct btrfs_fs_info *fs_info = BTRFS_I(inode)->root->fs_info;
-	int err = 0;
+	int ret = 0;
 	struct btrfs_trans_handle *trans;
 	u64 last_unlink_trans;
 	struct fscrypt_name fname;
@@ -4651,33 +4651,33 @@ static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
 		return btrfs_delete_subvolume(BTRFS_I(dir), dentry);
 	}
 
-	err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &fname);
-	if (err)
-		return err;
+	ret = fscrypt_setup_filename(dir, &dentry->d_name, 1, &fname);
+	if (ret)
+		return ret;
 
 	/* This needs to handle no-key deletions later on */
 
 	trans = __unlink_start_trans(BTRFS_I(dir));
 	if (IS_ERR(trans)) {
-		err = PTR_ERR(trans);
+		ret = PTR_ERR(trans);
 		goto out_notrans;
 	}
 
 	if (unlikely(btrfs_ino(BTRFS_I(inode)) == BTRFS_EMPTY_SUBVOL_DIR_OBJECTID)) {
-		err = btrfs_unlink_subvol(trans, BTRFS_I(dir), dentry);
+		ret = btrfs_unlink_subvol(trans, BTRFS_I(dir), dentry);
 		goto out;
 	}
 
-	err = btrfs_orphan_add(trans, BTRFS_I(inode));
-	if (err)
+	ret = btrfs_orphan_add(trans, BTRFS_I(inode));
+	if (ret)
 		goto out;
 
 	last_unlink_trans = BTRFS_I(inode)->last_unlink_trans;
 
 	/* now the directory is empty */
-	err = btrfs_unlink_inode(trans, BTRFS_I(dir), BTRFS_I(d_inode(dentry)),
+	ret = btrfs_unlink_inode(trans, BTRFS_I(dir), BTRFS_I(d_inode(dentry)),
 				 &fname.disk_name);
-	if (!err) {
+	if (!ret) {
 		btrfs_i_size_write(BTRFS_I(inode), 0);
 		/*
 		 * Propagate the last_unlink_trans value of the deleted dir to
@@ -4699,7 +4699,7 @@ static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
 	btrfs_btree_balance_dirty(fs_info);
 	fscrypt_free_filename(&fname);
 
-	return err;
+	return ret;
 }
 
 /*
-- 
2.38.1

[PATCH 05/29] btrfs: btrfs_cont_expand rename err to ret

[Anand Jain]

Simple renaming of the local variable err to ret.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a2ad3bc8900b..27183225ac54 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4923,16 +4923,16 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
 	u64 last_byte;
 	u64 cur_offset;
 	u64 hole_size;
-	int err = 0;
+	int ret = 0;
 
 	/*
 	 * If our size started in the middle of a block we need to zero out the
 	 * rest of the block before we expand the i_size, otherwise we could
 	 * expose stale data.
 	 */
-	err = btrfs_truncate_block(inode, oldsize, 0, 0);
-	if (err)
-		return err;
+	ret = btrfs_truncate_block(inode, oldsize, 0, 0);
+	if (ret)
+		return ret;
 
 	if (size <= hole_start)
 		return 0;
@@ -4943,7 +4943,7 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
 	while (1) {
 		em = btrfs_get_extent(inode, NULL, cur_offset, block_end - cur_offset);
 		if (IS_ERR(em)) {
-			err = PTR_ERR(em);
+			ret = PTR_ERR(em);
 			em = NULL;
 			break;
 		}
@@ -4954,13 +4954,13 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
 		if (!(em->flags & EXTENT_FLAG_PREALLOC)) {
 			struct extent_map *hole_em;
 
-			err = maybe_insert_hole(inode, cur_offset, hole_size);
-			if (err)
+			ret = maybe_insert_hole(inode, cur_offset, hole_size);
+			if (ret)
 				break;
 
-			err = btrfs_inode_set_file_extent_range(inode,
+			ret = btrfs_inode_set_file_extent_range(inode,
 							cur_offset, hole_size);
-			if (err)
+			if (ret)
 				break;
 
 			hole_em = alloc_extent_map();
@@ -4981,12 +4981,12 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
 			hole_em->ram_bytes = hole_size;
 			hole_em->generation = btrfs_get_fs_generation(fs_info);
 
-			err = btrfs_replace_extent_map_range(inode, hole_em, true);
+			ret = btrfs_replace_extent_map_range(inode, hole_em, true);
 			free_extent_map(hole_em);
 		} else {
-			err = btrfs_inode_set_file_extent_range(inode,
+			ret = btrfs_inode_set_file_extent_range(inode,
 							cur_offset, hole_size);
-			if (err)
+			if (ret)
 				break;
 		}
 next:
@@ -4998,7 +4998,7 @@ int btrfs_cont_expand(struct btrfs_inode *inode, loff_t oldsize, loff_t size)
 	}
 	free_extent_map(em);
 	unlock_extent(io_tree, hole_start, block_end - 1, &cached_state);
-	return err;
+	return ret;
 }
 
 static int btrfs_setsize(struct inode *inode, struct iattr *attr)
-- 
2.38.1

[PATCH 06/29] btrfs: btrfs_setsize rename err to ret2

[Anand Jain]

Simple renaming of the local variable err to ret2.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 27183225ac54..952c92e6dfcf 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5077,7 +5077,7 @@ static int btrfs_setsize(struct inode *inode, struct iattr *attr)
 
 		ret = btrfs_truncate(BTRFS_I(inode), newsize == oldsize);
 		if (ret && inode->i_nlink) {
-			int err;
+			int ret2;
 
 			/*
 			 * Truncate failed, so fix up the in-memory size. We
@@ -5085,9 +5085,9 @@ static int btrfs_setsize(struct inode *inode, struct iattr *attr)
 			 * wait for disk_i_size to be stable and then update the
 			 * in-memory size to match.
 			 */
-			err = btrfs_wait_ordered_range(inode, 0, (u64)-1);
-			if (err)
-				return err;
+			ret2 = btrfs_wait_ordered_range(inode, 0, (u64)-1);
+			if (ret2)
+				return ret2;
 			i_size_write(inode, BTRFS_I(inode)->disk_i_size);
 		}
 	}
-- 
2.38.1

[PATCH 07/29] btrfs: btrfs_find_orphan_roots rename ret to ret2 and err to ret

[Anand Jain]

Variable err is the actual return value of this function, and variable ret
is a helper variable for err. So, rename them to ret and ret2 respectively.

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

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 4bb538a372ce..869b50f05f39 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -221,8 +221,8 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 	struct btrfs_path *path;
 	struct btrfs_key key;
 	struct btrfs_root *root;
-	int err = 0;
-	int ret;
+	int ret = 0;
+	int ret2;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -235,18 +235,18 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 	while (1) {
 		u64 root_objectid;
 
-		ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
-		if (ret < 0) {
-			err = ret;
+		ret2 = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
+		if (ret2 < 0) {
+			ret = ret2;
 			break;
 		}
 
 		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
-			ret = btrfs_next_leaf(tree_root, path);
-			if (ret < 0)
-				err = ret;
-			if (ret != 0)
+			ret2 = btrfs_next_leaf(tree_root, path);
+			if (ret2 < 0)
+				ret = ret2;
+			if (ret2 != 0)
 				break;
 			leaf = path->nodes[0];
 		}
@@ -262,26 +262,26 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 		key.offset++;
 
 		root = btrfs_get_fs_root(fs_info, root_objectid, false);
-		err = PTR_ERR_OR_ZERO(root);
-		if (err && err != -ENOENT) {
+		ret = PTR_ERR_OR_ZERO(root);
+		if (ret && ret != -ENOENT) {
 			break;
-		} else if (err == -ENOENT) {
+		} else if (ret == -ENOENT) {
 			struct btrfs_trans_handle *trans;
 
 			btrfs_release_path(path);
 
 			trans = btrfs_join_transaction(tree_root);
 			if (IS_ERR(trans)) {
-				err = PTR_ERR(trans);
-				btrfs_handle_fs_error(fs_info, err,
+				ret = PTR_ERR(trans);
+				btrfs_handle_fs_error(fs_info, ret,
 					    "Failed to start trans to delete orphan item");
 				break;
 			}
-			err = btrfs_del_orphan_item(trans, tree_root,
+			ret = btrfs_del_orphan_item(trans, tree_root,
 						    root_objectid);
 			btrfs_end_transaction(trans);
-			if (err) {
-				btrfs_handle_fs_error(fs_info, err,
+			if (ret) {
+				btrfs_handle_fs_error(fs_info, ret,
 					    "Failed to delete root orphan item");
 				break;
 			}
@@ -312,7 +312,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 	}
 
 	btrfs_free_path(path);
-	return err;
+	return ret;
 }
 
 /* drop the root item for 'key' from the tree root */
-- 
2.38.1

Re: [PATCH 07/29] btrfs: btrfs_find_orphan_roots rename ret to ret2 and err to ret

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:15PM +0530, Anand Jain wrote:
> Variable err is the actual return value of this function, and variable ret
> is a helper variable for err. So, rename them to ret and ret2 respectively.
> 

This is just overkill, we don't ever need to hold both ret and ret2 in
consistent state, you can just to a straight conversion of err to ret and it'll
be fine.  Thanks,

Josef

Re: [PATCH 07/29] btrfs: btrfs_find_orphan_roots rename ret to ret2 and err to ret

[Qu Wenruo]

在 2024/3/20 01:25, Anand Jain 写道:
> Variable err is the actual return value of this function, and variable ret
> is a helper variable for err. So, rename them to ret and ret2 respectively.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Agree with Josef, I didn't see any special reason why we want to hold
both @ret and @err.

We directly assign @err after btrfs_get_fs_root() already, and only
continue if that @err is 0, so no need to hold two different values.

And again, I don't like the @ret2 naming anyway.

If we really need to hold two return values, I'd prefer to extract a
function to do own init/cleanup work inside, so we only need to hold one
@ret at higher level.

Thanks,
Qu
> ---
>   fs/btrfs/root-tree.c | 36 ++++++++++++++++++------------------
>   1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 4bb538a372ce..869b50f05f39 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -221,8 +221,8 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>   	struct btrfs_path *path;
>   	struct btrfs_key key;
>   	struct btrfs_root *root;
> -	int err = 0;
> -	int ret;
> +	int ret = 0;
> +	int ret2;
>
>   	path = btrfs_alloc_path();
>   	if (!path)
> @@ -235,18 +235,18 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>   	while (1) {
>   		u64 root_objectid;
>
> -		ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
> -		if (ret < 0) {
> -			err = ret;
> +		ret2 = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
> +		if (ret2 < 0) {
> +			ret = ret2;
>   			break;
>   		}
>
>   		leaf = path->nodes[0];
>   		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
> -			ret = btrfs_next_leaf(tree_root, path);
> -			if (ret < 0)
> -				err = ret;
> -			if (ret != 0)
> +			ret2 = btrfs_next_leaf(tree_root, path);
> +			if (ret2 < 0)
> +				ret = ret2;
> +			if (ret2 != 0)
>   				break;
>   			leaf = path->nodes[0];
>   		}
> @@ -262,26 +262,26 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>   		key.offset++;
>
>   		root = btrfs_get_fs_root(fs_info, root_objectid, false);
> -		err = PTR_ERR_OR_ZERO(root);
> -		if (err && err != -ENOENT) {
> +		ret = PTR_ERR_OR_ZERO(root);
> +		if (ret && ret != -ENOENT) {
>   			break;
> -		} else if (err == -ENOENT) {
> +		} else if (ret == -ENOENT) {
>   			struct btrfs_trans_handle *trans;
>
>   			btrfs_release_path(path);
>
>   			trans = btrfs_join_transaction(tree_root);
>   			if (IS_ERR(trans)) {
> -				err = PTR_ERR(trans);
> -				btrfs_handle_fs_error(fs_info, err,
> +				ret = PTR_ERR(trans);
> +				btrfs_handle_fs_error(fs_info, ret,
>   					    "Failed to start trans to delete orphan item");
>   				break;
>   			}
> -			err = btrfs_del_orphan_item(trans, tree_root,
> +			ret = btrfs_del_orphan_item(trans, tree_root,
>   						    root_objectid);
>   			btrfs_end_transaction(trans);
> -			if (err) {
> -				btrfs_handle_fs_error(fs_info, err,
> +			if (ret) {
> +				btrfs_handle_fs_error(fs_info, ret,
>   					    "Failed to delete root orphan item");
>   				break;
>   			}
> @@ -312,7 +312,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>   	}
>
>   	btrfs_free_path(path);
> -	return err;
> +	return ret;
>   }
>
>   /* drop the root item for 'key' from the tree root */

[PATCH 08/29] btrfs: btrfs_ioctl_snap_destroy rename err to ret

[Anand Jain]

Simple renaming of the local variable err to ret.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 38459a89b27c..692f6ca9f897 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2367,7 +2367,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	struct mnt_idmap *idmap = file_mnt_idmap(file);
 	char *subvol_name, *subvol_name_ptr = NULL;
 	int subvol_namelen;
-	int err = 0;
+	int ret = 0;
 	bool destroy_parent = false;
 
 	/* We don't support snapshots with extent tree v2 yet. */
@@ -2383,7 +2383,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 			return PTR_ERR(vol_args2);
 
 		if (vol_args2->flags & ~BTRFS_SUBVOL_DELETE_ARGS_MASK) {
-			err = -EOPNOTSUPP;
+			ret = -EOPNOTSUPP;
 			goto out;
 		}
 
@@ -2392,31 +2392,31 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		 * name, same as v1 currently does.
 		 */
 		if (!(vol_args2->flags & BTRFS_SUBVOL_SPEC_BY_ID)) {
-			err = btrfs_check_ioctl_vol_args2_subvol_name(vol_args2);
-			if (err < 0)
+			ret = btrfs_check_ioctl_vol_args2_subvol_name(vol_args2);
+			if (ret < 0)
 				goto out;
 			subvol_name = vol_args2->name;
 
-			err = mnt_want_write_file(file);
-			if (err)
+			ret = mnt_want_write_file(file);
+			if (ret)
 				goto out;
 		} else {
 			struct inode *old_dir;
 
 			if (vol_args2->subvolid < BTRFS_FIRST_FREE_OBJECTID) {
-				err = -EINVAL;
+				ret = -EINVAL;
 				goto out;
 			}
 
-			err = mnt_want_write_file(file);
-			if (err)
+			ret = mnt_want_write_file(file);
+			if (ret)
 				goto out;
 
 			dentry = btrfs_get_dentry(fs_info->sb,
 					BTRFS_FIRST_FREE_OBJECTID,
 					vol_args2->subvolid, 0);
 			if (IS_ERR(dentry)) {
-				err = PTR_ERR(dentry);
+				ret = PTR_ERR(dentry);
 				goto out_drop_write;
 			}
 
@@ -2436,7 +2436,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 			 */
 			dput(dentry);
 			if (IS_ERR(parent)) {
-				err = PTR_ERR(parent);
+				ret = PTR_ERR(parent);
 				goto out_drop_write;
 			}
 			old_dir = dir;
@@ -2460,14 +2460,14 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 			 * to delete without an idmapped mount.
 			 */
 			if (old_dir != dir && idmap != &nop_mnt_idmap) {
-				err = -EOPNOTSUPP;
+				ret = -EOPNOTSUPP;
 				goto free_parent;
 			}
 
 			subvol_name_ptr = btrfs_get_subvol_name_from_objectid(
 						fs_info, vol_args2->subvolid);
 			if (IS_ERR(subvol_name_ptr)) {
-				err = PTR_ERR(subvol_name_ptr);
+				ret = PTR_ERR(subvol_name_ptr);
 				goto free_parent;
 			}
 			/* subvol_name_ptr is already nul terminated */
@@ -2478,14 +2478,14 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		if (IS_ERR(vol_args))
 			return PTR_ERR(vol_args);
 
-		err = btrfs_check_ioctl_vol_args_path(vol_args);
-		if (err < 0)
+		ret = btrfs_check_ioctl_vol_args_path(vol_args);
+		if (ret < 0)
 			goto out;
 
 		subvol_name = vol_args->name;
 
-		err = mnt_want_write_file(file);
-		if (err)
+		ret = mnt_want_write_file(file);
+		if (ret)
 			goto out;
 	}
 
@@ -2493,26 +2493,26 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 
 	if (strchr(subvol_name, '/') ||
 	    strncmp(subvol_name, "..", subvol_namelen) == 0) {
-		err = -EINVAL;
+		ret = -EINVAL;
 		goto free_subvol_name;
 	}
 
 	if (!S_ISDIR(dir->i_mode)) {
-		err = -ENOTDIR;
+		ret = -ENOTDIR;
 		goto free_subvol_name;
 	}
 
-	err = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
-	if (err == -EINTR)
+	ret = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
+	if (ret == -EINTR)
 		goto free_subvol_name;
 	dentry = lookup_one(idmap, subvol_name, parent, subvol_namelen);
 	if (IS_ERR(dentry)) {
-		err = PTR_ERR(dentry);
+		ret = PTR_ERR(dentry);
 		goto out_unlock_dir;
 	}
 
 	if (d_really_is_negative(dentry)) {
-		err = -ENOENT;
+		ret = -ENOENT;
 		goto out_dput;
 	}
 
@@ -2532,7 +2532,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		 * Users who want to delete empty subvols should try
 		 * rmdir(2).
 		 */
-		err = -EPERM;
+		ret = -EPERM;
 		if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED))
 			goto out_dput;
 
@@ -2543,29 +2543,29 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		 * of the subvol, not a random directory contained
 		 * within it.
 		 */
-		err = -EINVAL;
+		ret = -EINVAL;
 		if (root == dest)
 			goto out_dput;
 
-		err = inode_permission(idmap, inode, MAY_WRITE | MAY_EXEC);
-		if (err)
+		ret = inode_permission(idmap, inode, MAY_WRITE | MAY_EXEC);
+		if (ret)
 			goto out_dput;
 	}
 
 	/* check if subvolume may be deleted by a user */
-	err = btrfs_may_delete(idmap, dir, dentry, 1);
-	if (err)
+	ret = btrfs_may_delete(idmap, dir, dentry, 1);
+	if (ret)
 		goto out_dput;
 
 	if (btrfs_ino(BTRFS_I(inode)) != BTRFS_FIRST_FREE_OBJECTID) {
-		err = -EINVAL;
+		ret = -EINVAL;
 		goto out_dput;
 	}
 
 	btrfs_inode_lock(BTRFS_I(inode), 0);
-	err = btrfs_delete_subvolume(BTRFS_I(dir), dentry);
+	ret = btrfs_delete_subvolume(BTRFS_I(dir), dentry);
 	btrfs_inode_unlock(BTRFS_I(inode), 0);
-	if (!err)
+	if (!ret)
 		d_delete_notify(dir, dentry);
 
 out_dput:
@@ -2582,7 +2582,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 out:
 	kfree(vol_args2);
 	kfree(vol_args);
-	return err;
+	return ret;
 }
 
 static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
-- 
2.38.1

[PATCH 09/29] btrfs: __set_extent_bit rename err to ret

[Anand Jain]

Simple renaming of the local variable err to ret.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/extent-io-tree.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/extent-io-tree.c b/fs/btrfs/extent-io-tree.c
index c09b428823d7..0d564860464d 100644
--- a/fs/btrfs/extent-io-tree.c
+++ b/fs/btrfs/extent-io-tree.c
@@ -1059,7 +1059,7 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 	struct extent_state *prealloc = NULL;
 	struct rb_node **p = NULL;
 	struct rb_node *parent = NULL;
-	int err = 0;
+	int ret = 0;
 	u64 last_start;
 	u64 last_end;
 	u32 exclusive_bits = (bits & EXTENT_LOCKED);
@@ -1122,7 +1122,7 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 		if (state->state & exclusive_bits) {
 			*failed_start = state->start;
 			cache_state(state, failed_state);
-			err = -EEXIST;
+			ret = -EEXIST;
 			goto out;
 		}
 
@@ -1158,7 +1158,7 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 		if (state->state & exclusive_bits) {
 			*failed_start = start;
 			cache_state(state, failed_state);
-			err = -EEXIST;
+			ret = -EEXIST;
 			goto out;
 		}
 
@@ -1175,12 +1175,12 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 		prealloc = alloc_extent_state_atomic(prealloc);
 		if (!prealloc)
 			goto search_again;
-		err = split_state(tree, state, prealloc, start);
-		if (err)
-			extent_io_tree_panic(tree, state, "split", err);
+		ret = split_state(tree, state, prealloc, start);
+		if (ret)
+			extent_io_tree_panic(tree, state, "split", ret);
 
 		prealloc = NULL;
-		if (err)
+		if (ret)
 			goto out;
 		if (state->end <= end) {
 			set_state_bits(tree, state, bits, changeset);
@@ -1224,8 +1224,8 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 		prealloc->end = this_end;
 		inserted_state = insert_state(tree, prealloc, bits, changeset);
 		if (IS_ERR(inserted_state)) {
-			err = PTR_ERR(inserted_state);
-			extent_io_tree_panic(tree, prealloc, "insert", err);
+			ret = PTR_ERR(inserted_state);
+			extent_io_tree_panic(tree, prealloc, "insert", ret);
 		}
 
 		cache_state(inserted_state, cached_state);
@@ -1244,16 +1244,16 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 		if (state->state & exclusive_bits) {
 			*failed_start = start;
 			cache_state(state, failed_state);
-			err = -EEXIST;
+			ret = -EEXIST;
 			goto out;
 		}
 
 		prealloc = alloc_extent_state_atomic(prealloc);
 		if (!prealloc)
 			goto search_again;
-		err = split_state(tree, state, prealloc, end + 1);
-		if (err)
-			extent_io_tree_panic(tree, state, "split", err);
+		ret = split_state(tree, state, prealloc, end + 1);
+		if (ret)
+			extent_io_tree_panic(tree, state, "split", ret);
 
 		set_state_bits(tree, prealloc, bits, changeset);
 		cache_state(prealloc, cached_state);
@@ -1275,7 +1275,7 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 	if (prealloc)
 		free_extent_state(prealloc);
 
-	return err;
+	return ret;
 
 }
 
-- 
2.38.1

[PATCH 10/29] btrfs: convert_extent_bit rename err to ret

[Anand Jain]

Simple renaming of the local variable err to ret.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/extent-io-tree.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/extent-io-tree.c b/fs/btrfs/extent-io-tree.c
index 0d564860464d..ed2cfc3d5d8a 100644
--- a/fs/btrfs/extent-io-tree.c
+++ b/fs/btrfs/extent-io-tree.c
@@ -1312,7 +1312,7 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 	struct extent_state *prealloc = NULL;
 	struct rb_node **p = NULL;
 	struct rb_node *parent = NULL;
-	int err = 0;
+	int ret = 0;
 	u64 last_start;
 	u64 last_end;
 	bool first_iteration = true;
@@ -1351,7 +1351,7 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 	if (!state) {
 		prealloc = alloc_extent_state_atomic(prealloc);
 		if (!prealloc) {
-			err = -ENOMEM;
+			ret = -ENOMEM;
 			goto out;
 		}
 		prealloc->start = start;
@@ -1402,14 +1402,14 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 	if (state->start < start) {
 		prealloc = alloc_extent_state_atomic(prealloc);
 		if (!prealloc) {
-			err = -ENOMEM;
+			ret = -ENOMEM;
 			goto out;
 		}
-		err = split_state(tree, state, prealloc, start);
-		if (err)
-			extent_io_tree_panic(tree, state, "split", err);
+		ret = split_state(tree, state, prealloc, start);
+		if (ret)
+			extent_io_tree_panic(tree, state, "split", ret);
 		prealloc = NULL;
-		if (err)
+		if (ret)
 			goto out;
 		if (state->end <= end) {
 			set_state_bits(tree, state, bits, NULL);
@@ -1442,7 +1442,7 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 
 		prealloc = alloc_extent_state_atomic(prealloc);
 		if (!prealloc) {
-			err = -ENOMEM;
+			ret = -ENOMEM;
 			goto out;
 		}
 
@@ -1454,8 +1454,8 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 		prealloc->end = this_end;
 		inserted_state = insert_state(tree, prealloc, bits, NULL);
 		if (IS_ERR(inserted_state)) {
-			err = PTR_ERR(inserted_state);
-			extent_io_tree_panic(tree, prealloc, "insert", err);
+			ret = PTR_ERR(inserted_state);
+			extent_io_tree_panic(tree, prealloc, "insert", ret);
 		}
 		cache_state(inserted_state, cached_state);
 		if (inserted_state == prealloc)
@@ -1472,13 +1472,13 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 	if (state->start <= end && state->end > end) {
 		prealloc = alloc_extent_state_atomic(prealloc);
 		if (!prealloc) {
-			err = -ENOMEM;
+			ret = -ENOMEM;
 			goto out;
 		}
 
-		err = split_state(tree, state, prealloc, end + 1);
-		if (err)
-			extent_io_tree_panic(tree, state, "split", err);
+		ret = split_state(tree, state, prealloc, end + 1);
+		if (ret)
+			extent_io_tree_panic(tree, state, "split", ret);
 
 		set_state_bits(tree, prealloc, bits, NULL);
 		cache_state(prealloc, cached_state);
@@ -1500,7 +1500,7 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 	if (prealloc)
 		free_extent_state(prealloc);
 
-	return err;
+	return ret;
 }
 
 /*
-- 
2.38.1

[PATCH 11/29] btrfs: __btrfs_end_transaction rename err to ret

[Anand Jain]

Simple renaming of the local variable err to ret.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 5b6536c1f6d1..feffff91c6fe 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1053,7 +1053,7 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_fs_info *info = trans->fs_info;
 	struct btrfs_transaction *cur_trans = trans->transaction;
-	int err = 0;
+	int ret = 0;
 
 	if (refcount_read(&trans->use_count) > 1) {
 		refcount_dec(&trans->use_count);
@@ -1092,13 +1092,13 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 	if (TRANS_ABORTED(trans) || BTRFS_FS_ERROR(info)) {
 		wake_up_process(info->transaction_kthread);
 		if (TRANS_ABORTED(trans))
-			err = trans->aborted;
+			ret = trans->aborted;
 		else
-			err = -EROFS;
+			ret = -EROFS;
 	}
 
 	kmem_cache_free(btrfs_trans_handle_cachep, trans);
-	return err;
+	return ret;
 }
 
 int btrfs_end_transaction(struct btrfs_trans_handle *trans)
-- 
2.38.1

[PATCH 12/29] btrfs: btrfs_write_marked_extents rename werr to ret err to ret2

[Anand Jain]

Rename the function's local variable werr to ret and err to ret2, then
move ret2 to the local variable of the while loop. Drop the initialization
there since it's immediately assigned below.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index feffff91c6fe..167893457b58 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans)
 int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 			       struct extent_io_tree *dirty_pages, int mark)
 {
-	int err = 0;
-	int werr = 0;
+	int ret = 0;
 	struct address_space *mapping = fs_info->btree_inode->i_mapping;
 	struct extent_state *cached_state = NULL;
 	u64 start = 0;
@@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 
 	while (find_first_extent_bit(dirty_pages, start, &start, &end,
 				     mark, &cached_state)) {
+		int ret2;
 		bool wait_writeback = false;
 
-		err = convert_extent_bit(dirty_pages, start, end,
+		ret2 = convert_extent_bit(dirty_pages, start, end,
 					 EXTENT_NEED_WAIT,
 					 mark, &cached_state);
 		/*
@@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 		 * We cleanup any entries left in the io tree when committing
 		 * the transaction (through extent_io_tree_release()).
 		 */
-		if (err == -ENOMEM) {
-			err = 0;
+		if (ret2 == -ENOMEM) {
+			ret2 = 0;
 			wait_writeback = true;
 		}
-		if (!err)
-			err = filemap_fdatawrite_range(mapping, start, end);
-		if (err)
-			werr = err;
+		if (!ret2)
+			ret2 = filemap_fdatawrite_range(mapping, start, end);
+		if (ret2)
+			ret = ret2;
 		else if (wait_writeback)
-			werr = filemap_fdatawait_range(mapping, start, end);
+			ret = filemap_fdatawait_range(mapping, start, end);
 		free_extent_state(cached_state);
 		cached_state = NULL;
 		cond_resched();
 		start = end + 1;
 	}
-	return werr;
+	return ret;
 }
 
 /*
-- 
2.38.1

Re: [PATCH 12/29] btrfs: btrfs_write_marked_extents rename werr to ret err to ret2

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:20PM +0530, Anand Jain wrote:
> Rename the function's local variable werr to ret and err to ret2, then
> move ret2 to the local variable of the while loop. Drop the initialization
> there since it's immediately assigned below.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/transaction.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index feffff91c6fe..167893457b58 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans)
>  int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>  			       struct extent_io_tree *dirty_pages, int mark)
>  {
> -	int err = 0;
> -	int werr = 0;
> +	int ret = 0;
>  	struct address_space *mapping = fs_info->btree_inode->i_mapping;
>  	struct extent_state *cached_state = NULL;
>  	u64 start = 0;
> @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>  
>  	while (find_first_extent_bit(dirty_pages, start, &start, &end,
>  				     mark, &cached_state)) {
> +		int ret2;
>  		bool wait_writeback = false;
>  
> -		err = convert_extent_bit(dirty_pages, start, end,
> +		ret2 = convert_extent_bit(dirty_pages, start, end,
>  					 EXTENT_NEED_WAIT,
>  					 mark, &cached_state);
>  		/*
> @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>  		 * We cleanup any entries left in the io tree when committing
>  		 * the transaction (through extent_io_tree_release()).
>  		 */
> -		if (err == -ENOMEM) {
> -			err = 0;
> +		if (ret2 == -ENOMEM) {
> +			ret2 = 0;
>  			wait_writeback = true;
>  		}
> -		if (!err)
> -			err = filemap_fdatawrite_range(mapping, start, end);
> -		if (err)
> -			werr = err;
> +		if (!ret2)
> +			ret2 = filemap_fdatawrite_range(mapping, start, end);
> +		if (ret2)
> +			ret = ret2;
>  		else if (wait_writeback)
> -			werr = filemap_fdatawait_range(mapping, start, end);
> +			ret = filemap_fdatawait_range(mapping, start, end);

Ok so this is a correct conversion, but we'll lose "ret" here.  Can you follow
up with a different series to fix this?  I think we just say

free_extent_state(cached_state);
if (ret)
	break;

otherwise this patch looks fine, you can add

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

Thanks,

Josef

Re: [PATCH 12/29] btrfs: btrfs_write_marked_extents rename werr to ret err to ret2

[Anand Jain]

On 3/20/24 01:53, Josef Bacik wrote:
> On Tue, Mar 19, 2024 at 08:25:20PM +0530, Anand Jain wrote:
>> Rename the function's local variable werr to ret and err to ret2, then
>> move ret2 to the local variable of the while loop. Drop the initialization
>> there since it's immediately assigned below.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/transaction.c | 22 +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index feffff91c6fe..167893457b58 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans)
>>   int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>>   			       struct extent_io_tree *dirty_pages, int mark)
>>   {
>> -	int err = 0;
>> -	int werr = 0;
>> +	int ret = 0;
>>   	struct address_space *mapping = fs_info->btree_inode->i_mapping;
>>   	struct extent_state *cached_state = NULL;
>>   	u64 start = 0;
>> @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>>   
>>   	while (find_first_extent_bit(dirty_pages, start, &start, &end,
>>   				     mark, &cached_state)) {
>> +		int ret2;
>>   		bool wait_writeback = false;
>>   
>> -		err = convert_extent_bit(dirty_pages, start, end,
>> +		ret2 = convert_extent_bit(dirty_pages, start, end,
>>   					 EXTENT_NEED_WAIT,
>>   					 mark, &cached_state);
>>   		/*
>> @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>>   		 * We cleanup any entries left in the io tree when committing
>>   		 * the transaction (through extent_io_tree_release()).
>>   		 */
>> -		if (err == -ENOMEM) {
>> -			err = 0;
>> +		if (ret2 == -ENOMEM) {
>> +			ret2 = 0;
>>   			wait_writeback = true;
>>   		}
>> -		if (!err)
>> -			err = filemap_fdatawrite_range(mapping, start, end);
>> -		if (err)
>> -			werr = err;
>> +		if (!ret2)
>> +			ret2 = filemap_fdatawrite_range(mapping, start, end);
>> +		if (ret2)
>> +			ret = ret2;
>>   		else if (wait_writeback)
>> -			werr = filemap_fdatawait_range(mapping, start, end);
>> +			ret = filemap_fdatawait_range(mapping, start, end);
> 
> Ok so this is a correct conversion, but we'll lose "ret" here.  Can you follow
> up with a different series to fix this?  I think we just say
> 

> free_extent_state(cached_state);
> if (ret)
> 	break;
> 

Sure. Checked the function's stack will propagate the error here back
to the parent functions.

Thanks, Anand

> otherwise this patch looks fine, you can add
> 
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> 
> Thanks,
> 
> Josef

Re: [PATCH 12/29] btrfs: btrfs_write_marked_extents rename werr to ret err to ret2

[Qu Wenruo]

在 2024/3/20 01:25, Anand Jain 写道:
> Rename the function's local variable werr to ret and err to ret2, then
> move ret2 to the local variable of the while loop. Drop the initialization
> there since it's immediately assigned below.

I love the local return value inside the loop, but I still think we can
do further improvement.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   fs/btrfs/transaction.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index feffff91c6fe..167893457b58 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans)
>   int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>   			       struct extent_io_tree *dirty_pages, int mark)
>   {
> -	int err = 0;
> -	int werr = 0;
> +	int ret = 0;

Can we rename it to something like global_ret or whatever can indicate
that variable is our final result?


>   	struct address_space *mapping = fs_info->btree_inode->i_mapping;
>   	struct extent_state *cached_state = NULL;
>   	u64 start = 0;
> @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>
>   	while (find_first_extent_bit(dirty_pages, start, &start, &end,
>   				     mark, &cached_state)) {
> +		int ret2;

And @ret inside the loop.

>   		bool wait_writeback = false;
>
> -		err = convert_extent_bit(dirty_pages, start, end,
> +		ret2 = convert_extent_bit(dirty_pages, start, end,
>   					 EXTENT_NEED_WAIT,
>   					 mark, &cached_state);
>   		/*
> @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>   		 * We cleanup any entries left in the io tree when committing
>   		 * the transaction (through extent_io_tree_release()).
>   		 */
> -		if (err == -ENOMEM) {
> -			err = 0;
> +		if (ret2 == -ENOMEM) {
> +			ret2 = 0;
>   			wait_writeback = true;
>   		}
> -		if (!err)
> -			err = filemap_fdatawrite_range(mapping, start, end);
> -		if (err)
> -			werr = err;
> +		if (!ret2)
> +			ret2 = filemap_fdatawrite_range(mapping, start, end);
> +		if (ret2)
> +			ret = ret2;
>   		else if (wait_writeback)
> -			werr = filemap_fdatawait_range(mapping, start, end);
> +			ret = filemap_fdatawait_range(mapping, start, end);

This does not follow the regular update sequence, we always update the
local one @ret2/@err, but here we directly update the @ret/@werr.

Just for the sake of consistency, can we only use @ret2 for all the
functions calls?

>   		free_extent_state(cached_state);
>   		cached_state = NULL;
>   		cond_resched();
>   		start = end + 1;

Another thing is, we can move most of the writeback code (aka code
inside the loop) into a helper function, and make things much simpler at
least for the caller.

{
	int global_ret = 0;

    	while (find_first_extent_bit(dirty_pages, start, &start, &end,
    				     mark, &cached_state)) {
		int ret;

		ret = writeback_range();
		if (ret < 0)
			global_ret = ret;
		cond_resched();
		start = end + 1
	}
	return global_ret;
}

Thanks,
Qu
>   	}
> -	return werr;
> +	return ret;
>   }
>
>   /*

Re: [PATCH 12/29] btrfs: btrfs_write_marked_extents rename werr to ret err to ret2

[Anand Jain]

On 3/20/24 05:25, Qu Wenruo wrote:
> 
> 
> 在 2024/3/20 01:25, Anand Jain 写道:
>> Rename the function's local variable werr to ret and err to ret2, then
>> move ret2 to the local variable of the while loop. Drop the 
>> initialization
>> there since it's immediately assigned below.
> 
> I love the local return value inside the loop, but I still think we can
> do further improvement.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/transaction.c | 22 +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index feffff91c6fe..167893457b58 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct 
>> btrfs_trans_handle *trans)
>>   int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>>                      struct extent_io_tree *dirty_pages, int mark)
>>   {
>> -    int err = 0;
>> -    int werr = 0;
>> +    int ret = 0;
> 
> Can we rename it to something like global_ret or whatever can indicate
> that variable is our final result?


@ret is the actual function return variable and is consistent with doc:

  https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#code

Thanks, Anand

> 
> 
>>       struct address_space *mapping = fs_info->btree_inode->i_mapping;
>>       struct extent_state *cached_state = NULL;
>>       u64 start = 0;
>> @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct 
>> btrfs_fs_info *fs_info,
>>
>>       while (find_first_extent_bit(dirty_pages, start, &start, &end,
>>                        mark, &cached_state)) {
>> +        int ret2;
> 
> And @ret inside the loop.
> 
>>           bool wait_writeback = false;
>>
>> -        err = convert_extent_bit(dirty_pages, start, end,
>> +        ret2 = convert_extent_bit(dirty_pages, start, end,
>>                        EXTENT_NEED_WAIT,
>>                        mark, &cached_state);
>>           /*
>> @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct 
>> btrfs_fs_info *fs_info,
>>            * We cleanup any entries left in the io tree when committing
>>            * the transaction (through extent_io_tree_release()).
>>            */
>> -        if (err == -ENOMEM) {
>> -            err = 0;
>> +        if (ret2 == -ENOMEM) {
>> +            ret2 = 0;
>>               wait_writeback = true;
>>           }
>> -        if (!err)
>> -            err = filemap_fdatawrite_range(mapping, start, end);
>> -        if (err)
>> -            werr = err;
>> +        if (!ret2)
>> +            ret2 = filemap_fdatawrite_range(mapping, start, end);
>> +        if (ret2)
>> +            ret = ret2;
>>           else if (wait_writeback)
>> -            werr = filemap_fdatawait_range(mapping, start, end);
>> +            ret = filemap_fdatawait_range(mapping, start, end);
> 
> This does not follow the regular update sequence, we always update the
> local one @ret2/@err, but here we directly update the @ret/@werr.
> 
> Just for the sake of consistency, can we only use @ret2 for all the
> functions calls?
> 
>>           free_extent_state(cached_state);
>>           cached_state = NULL;
>>           cond_resched();
>>           start = end + 1;
> 
> Another thing is, we can move most of the writeback code (aka code
> inside the loop) into a helper function, and make things much simpler at
> least for the caller.
> 
> {
>      int global_ret = 0;
> 
>         while (find_first_extent_bit(dirty_pages, start, &start, &end,
>                          mark, &cached_state)) {
>          int ret;
> 
>          ret = writeback_range();
>          if (ret < 0)
>              global_ret = ret;
>          cond_resched();
>          start = end + 1
>      }
>      return global_ret;
> }
> 
> Thanks,
> Qu
>>       }
>> -    return werr;
>> +    return ret;
>>   }
>>
>>   /*

[PATCH 13/29] btrfs: __btrfs_wait_marked_extents rename werr to ret err to ret2

[Anand Jain]

Rename the function's local variable werr to ret, and err to ret2.
Also, align these two variable declarations with the other declarations in
the function for better function space alignment.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 167893457b58..f344f97a6035 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1173,12 +1173,12 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 static int __btrfs_wait_marked_extents(struct btrfs_fs_info *fs_info,
 				       struct extent_io_tree *dirty_pages)
 {
-	int err = 0;
-	int werr = 0;
 	struct address_space *mapping = fs_info->btree_inode->i_mapping;
 	struct extent_state *cached_state = NULL;
 	u64 start = 0;
 	u64 end;
+	int ret = 0;
+	int ret2 = 0;
 
 	while (find_first_extent_bit(dirty_pages, start, &start, &end,
 				     EXTENT_NEED_WAIT, &cached_state)) {
@@ -1190,22 +1190,22 @@ static int __btrfs_wait_marked_extents(struct btrfs_fs_info *fs_info,
 		 * concurrently - we do it only at transaction commit time when
 		 * it's safe to do it (through extent_io_tree_release()).
 		 */
-		err = clear_extent_bit(dirty_pages, start, end,
-				       EXTENT_NEED_WAIT, &cached_state);
-		if (err == -ENOMEM)
-			err = 0;
-		if (!err)
-			err = filemap_fdatawait_range(mapping, start, end);
-		if (err)
-			werr = err;
+		ret2 = clear_extent_bit(dirty_pages, start, end,
+					EXTENT_NEED_WAIT, &cached_state);
+		if (ret2 == -ENOMEM)
+			ret2 = 0;
+		if (!ret2)
+			ret2 = filemap_fdatawait_range(mapping, start, end);
+		if (ret2)
+			ret = ret2;
 		free_extent_state(cached_state);
 		cached_state = NULL;
 		cond_resched();
 		start = end + 1;
 	}
-	if (err)
-		werr = err;
-	return werr;
+	if (ret2)
+		ret = ret2;
+	return ret;
 }
 
 static int btrfs_wait_extents(struct btrfs_fs_info *fs_info,
-- 
2.38.1

Re: [PATCH 13/29] btrfs: __btrfs_wait_marked_extents rename werr to ret err to ret2

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:21PM +0530, Anand Jain wrote:
> Rename the function's local variable werr to ret, and err to ret2.
> Also, align these two variable declarations with the other declarations in
> the function for better function space alignment.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/transaction.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 167893457b58..f344f97a6035 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1173,12 +1173,12 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>  static int __btrfs_wait_marked_extents(struct btrfs_fs_info *fs_info,
>  				       struct extent_io_tree *dirty_pages)
>  {
> -	int err = 0;
> -	int werr = 0;
>  	struct address_space *mapping = fs_info->btree_inode->i_mapping;
>  	struct extent_state *cached_state = NULL;
>  	u64 start = 0;
>  	u64 end;
> +	int ret = 0;
> +	int ret2 = 0;
>  
>  	while (find_first_extent_bit(dirty_pages, start, &start, &end,
>  				     EXTENT_NEED_WAIT, &cached_state)) {
> @@ -1190,22 +1190,22 @@ static int __btrfs_wait_marked_extents(struct btrfs_fs_info *fs_info,
>  		 * concurrently - we do it only at transaction commit time when
>  		 * it's safe to do it (through extent_io_tree_release()).
>  		 */
> -		err = clear_extent_bit(dirty_pages, start, end,
> -				       EXTENT_NEED_WAIT, &cached_state);
> -		if (err == -ENOMEM)
> -			err = 0;
> -		if (!err)
> -			err = filemap_fdatawait_range(mapping, start, end);
> -		if (err)
> -			werr = err;
> +		ret2 = clear_extent_bit(dirty_pages, start, end,
> +					EXTENT_NEED_WAIT, &cached_state);
> +		if (ret2 == -ENOMEM)
> +			ret2 = 0;
> +		if (!ret2)
> +			ret2 = filemap_fdatawait_range(mapping, start, end);
> +		if (ret2)
> +			ret = ret2;

Same comment here as the previous one.  In fact now that I think about it I'd
rather you fix this problem first, and then do the cleanup, so we can easily
backport the fix to stable kernels.

Then once you clean everything up you can change this to just use one ret
variable.  Thanks,

Josef

Re: [PATCH 13/29] btrfs: __btrfs_wait_marked_extents rename werr to ret err to ret2

[Qu Wenruo]

在 2024/3/20 01:25, Anand Jain 写道:
> Rename the function's local variable werr to ret, and err to ret2.
> Also, align these two variable declarations with the other declarations in
> the function for better function space alignment.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Again, not a fan of @ret2.

And IIRC we can declare the variable used inside the loop local, by
making sure we set the global ret properly before breaking the loop.

Thanks,
Qu
> ---
>   fs/btrfs/transaction.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 167893457b58..f344f97a6035 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1173,12 +1173,12 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>   static int __btrfs_wait_marked_extents(struct btrfs_fs_info *fs_info,
>   				       struct extent_io_tree *dirty_pages)
>   {
> -	int err = 0;
> -	int werr = 0;
>   	struct address_space *mapping = fs_info->btree_inode->i_mapping;
>   	struct extent_state *cached_state = NULL;
>   	u64 start = 0;
>   	u64 end;
> +	int ret = 0;
> +	int ret2 = 0;
>
>   	while (find_first_extent_bit(dirty_pages, start, &start, &end,
>   				     EXTENT_NEED_WAIT, &cached_state)) {
> @@ -1190,22 +1190,22 @@ static int __btrfs_wait_marked_extents(struct btrfs_fs_info *fs_info,
>   		 * concurrently - we do it only at transaction commit time when
>   		 * it's safe to do it (through extent_io_tree_release()).
>   		 */
> -		err = clear_extent_bit(dirty_pages, start, end,
> -				       EXTENT_NEED_WAIT, &cached_state);
> -		if (err == -ENOMEM)
> -			err = 0;
> -		if (!err)
> -			err = filemap_fdatawait_range(mapping, start, end);
> -		if (err)
> -			werr = err;
> +		ret2 = clear_extent_bit(dirty_pages, start, end,
> +					EXTENT_NEED_WAIT, &cached_state);
> +		if (ret2 == -ENOMEM)
> +			ret2 = 0;
> +		if (!ret2)
> +			ret2 = filemap_fdatawait_range(mapping, start, end);
> +		if (ret2)
> +			ret = ret2;
>   		free_extent_state(cached_state);
>   		cached_state = NULL;
>   		cond_resched();
>   		start = end + 1;
>   	}
> -	if (err)
> -		werr = err;
> -	return werr;
> +	if (ret2)
> +		ret = ret2;
> +	return ret;
>   }
>
>   static int btrfs_wait_extents(struct btrfs_fs_info *fs_info,

[PATCH 14/29] btrfs: build_backref_tree rename err to ret and ret to ret2

[Anand Jain]

Code sytle fix in the function build_backref_tree().

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index f96f267fb4aa..535d5657777b 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -472,21 +472,21 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
 	struct btrfs_backref_node *cur;
 	struct btrfs_backref_node *node = NULL;
 	struct btrfs_backref_edge *edge;
-	int ret;
-	int err = 0;
+	int ret = 0;
+	int ret2;
 
 	iter = btrfs_backref_iter_alloc(rc->extent_root->fs_info);
 	if (!iter)
 		return ERR_PTR(-ENOMEM);
 	path = btrfs_alloc_path();
 	if (!path) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
 	node = btrfs_backref_alloc_node(cache, bytenr, level);
 	if (!node) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
@@ -495,10 +495,10 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
 
 	/* Breadth-first search to build backref cache */
 	do {
-		ret = btrfs_backref_add_tree_node(trans, cache, path, iter,
+		ret2 = btrfs_backref_add_tree_node(trans, cache, path, iter,
 						  node_key, cur);
-		if (ret < 0) {
-			err = ret;
+		if (ret2 < 0) {
+			ret = ret2;
 			goto out;
 		}
 		edge = list_first_entry_or_null(&cache->pending_edge,
@@ -514,9 +514,9 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
 	} while (edge);
 
 	/* Finish the upper linkage of newly added edges/nodes */
-	ret = btrfs_backref_finish_upper_links(cache, node);
-	if (ret < 0) {
-		err = ret;
+	ret2 = btrfs_backref_finish_upper_links(cache, node);
+	if (ret2 < 0) {
+		ret = ret2;
 		goto out;
 	}
 
@@ -526,9 +526,9 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
 	btrfs_free_path(iter->path);
 	kfree(iter);
 	btrfs_free_path(path);
-	if (err) {
+	if (ret) {
 		btrfs_backref_error_cleanup(cache, node);
-		return ERR_PTR(err);
+		return ERR_PTR(ret);
 	}
 	ASSERT(!node || !node->detached);
 	ASSERT(list_empty(&cache->useless_node) &&
-- 
2.38.1

Re: [PATCH 14/29] btrfs: build_backref_tree rename err to ret and ret to ret2

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:22PM +0530, Anand Jain wrote:
> Code sytle fix in the function build_backref_tree().
> 

This doesn't need 2 ret values, we can get away with just one here.  Thanks,

Josef

[PATCH 15/29] btrfs: relocate_tree_blocks rename ret to ret2 and err to ret

[Anand Jain]

Coding style fixes the function relocate_tree_blocks().
After the fix, ret is the return value variable, and ret2 is the
function's local interim return variable.

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 535d5657777b..47b564df4340 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2776,12 +2776,12 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 	struct btrfs_path *path;
 	struct tree_block *block;
 	struct tree_block *next;
-	int ret;
-	int err = 0;
+	int ret = 0;
+	int ret2;
 
 	path = btrfs_alloc_path();
 	if (!path) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out_free_blocks;
 	}
 
@@ -2796,8 +2796,8 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 	/* Get first keys */
 	rbtree_postorder_for_each_entry_safe(block, next, blocks, rb_node) {
 		if (!block->key_ready) {
-			err = get_tree_block_key(fs_info, block);
-			if (err)
+			ret = get_tree_block_key(fs_info, block);
+			if (ret)
 				goto out_free_path;
 		}
 	}
@@ -2807,25 +2807,25 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 		node = build_backref_tree(trans, rc, &block->key,
 					  block->level, block->bytenr);
 		if (IS_ERR(node)) {
-			err = PTR_ERR(node);
+			ret = PTR_ERR(node);
 			goto out;
 		}
 
-		ret = relocate_tree_block(trans, rc, node, &block->key,
+		ret2 = relocate_tree_block(trans, rc, node, &block->key,
 					  path);
-		if (ret < 0) {
-			err = ret;
+		if (ret2 < 0) {
+			ret = ret2;
 			break;
 		}
 	}
 out:
-	err = finish_pending_nodes(trans, rc, path, err);
+	ret = finish_pending_nodes(trans, rc, path, ret);
 
 out_free_path:
 	btrfs_free_path(path);
 out_free_blocks:
 	free_block_list(blocks);
-	return err;
+	return ret;
 }
 
 static noinline_for_stack int prealloc_file_extent_cluster(
-- 
2.38.1

Re: [PATCH 15/29] btrfs: relocate_tree_blocks rename ret to ret2 and err to ret

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:23PM +0530, Anand Jain wrote:
> Coding style fixes the function relocate_tree_blocks().
> After the fix, ret is the return value variable, and ret2 is the
> function's local interim return variable.
> 

Same comment here, we only need one ret value.  Thanks,

Josef

[PATCH 16/29] btrfs: relocate_block_group rename ret to ret2 and err to ret

[Anand Jain]

Variable %err carries the function return value; rename it to ret and
the original ret is renamed to ret2.

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 47b564df4340..412d328bfbfd 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3683,8 +3683,8 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 	struct btrfs_path *path;
 	struct btrfs_extent_item *ei;
 	u64 flags;
-	int ret;
-	int err = 0;
+	int ret2;
+	int ret = 0;
 	int progress = 0;
 
 	path = btrfs_alloc_path();
@@ -3692,25 +3692,25 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 		return -ENOMEM;
 	path->reada = READA_FORWARD;
 
-	ret = prepare_to_relocate(rc);
-	if (ret) {
-		err = ret;
+	ret2 = prepare_to_relocate(rc);
+	if (ret2) {
+		ret = ret2;
 		goto out_free;
 	}
 
 	while (1) {
 		rc->reserved_bytes = 0;
-		ret = btrfs_block_rsv_refill(fs_info, rc->block_rsv,
+		ret2 = btrfs_block_rsv_refill(fs_info, rc->block_rsv,
 					     rc->block_rsv->size,
 					     BTRFS_RESERVE_FLUSH_ALL);
-		if (ret) {
-			err = ret;
+		if (ret2) {
+			ret = ret2;
 			break;
 		}
 		progress++;
 		trans = btrfs_start_transaction(rc->extent_root, 0);
 		if (IS_ERR(trans)) {
-			err = PTR_ERR(trans);
+			ret = PTR_ERR(trans);
 			trans = NULL;
 			break;
 		}
@@ -3721,10 +3721,10 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 			continue;
 		}
 
-		ret = find_next_extent(rc, path, &key);
-		if (ret < 0)
-			err = ret;
-		if (ret != 0)
+		ret2 = find_next_extent(rc, path, &key);
+		if (ret2 < 0)
+			ret = ret2;
+		if (ret2 != 0)
 			break;
 
 		rc->extents_found++;
@@ -3749,24 +3749,24 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 		}
 
 		if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) {
-			ret = add_tree_block(rc, &key, path, &blocks);
+			ret2 = add_tree_block(rc, &key, path, &blocks);
 		} else if (rc->stage == UPDATE_DATA_PTRS &&
 			   (flags & BTRFS_EXTENT_FLAG_DATA)) {
-			ret = add_data_references(rc, &key, path, &blocks);
+			ret2 = add_data_references(rc, &key, path, &blocks);
 		} else {
 			btrfs_release_path(path);
-			ret = 0;
+			ret2 = 0;
 		}
-		if (ret < 0) {
-			err = ret;
+		if (ret2 < 0) {
+			ret = ret2;
 			break;
 		}
 
 		if (!RB_EMPTY_ROOT(&blocks)) {
-			ret = relocate_tree_blocks(trans, rc, &blocks);
-			if (ret < 0) {
-				if (ret != -EAGAIN) {
-					err = ret;
+			ret2 = relocate_tree_blocks(trans, rc, &blocks);
+			if (ret2 < 0) {
+				if (ret2 != -EAGAIN) {
+					ret = ret2;
 					break;
 				}
 				rc->extents_found--;
@@ -3781,22 +3781,22 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 		if (rc->stage == MOVE_DATA_EXTENTS &&
 		    (flags & BTRFS_EXTENT_FLAG_DATA)) {
 			rc->found_file_extent = true;
-			ret = relocate_data_extent(rc->data_inode,
+			ret2 = relocate_data_extent(rc->data_inode,
 						   &key, &rc->cluster);
-			if (ret < 0) {
-				err = ret;
+			if (ret2 < 0) {
+				ret = ret2;
 				break;
 			}
 		}
 		if (btrfs_should_cancel_balance(fs_info)) {
-			err = -ECANCELED;
+			ret = -ECANCELED;
 			break;
 		}
 	}
-	if (trans && progress && err == -ENOSPC) {
-		ret = btrfs_force_chunk_alloc(trans, rc->block_group->flags);
-		if (ret == 1) {
-			err = 0;
+	if (trans && progress && ret == -ENOSPC) {
+		ret2 = btrfs_force_chunk_alloc(trans, rc->block_group->flags);
+		if (ret2 == 1) {
+			ret = 0;
 			progress = 0;
 			goto restart;
 		}
@@ -3810,11 +3810,11 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 		btrfs_btree_balance_dirty(fs_info);
 	}
 
-	if (!err) {
-		ret = relocate_file_extent_cluster(rc->data_inode,
+	if (!ret) {
+		ret2 = relocate_file_extent_cluster(rc->data_inode,
 						   &rc->cluster);
-		if (ret < 0)
-			err = ret;
+		if (ret2 < 0)
+			ret = ret2;
 	}
 
 	rc->create_reloc_tree = false;
@@ -3831,7 +3831,7 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 	 * mark all reloc trees orphan, then queue them for cleanup in
 	 * merge_reloc_roots()
 	 */
-	err = prepare_to_merge(rc, err);
+	ret = prepare_to_merge(rc, ret);
 
 	merge_reloc_roots(rc);
 
@@ -3842,19 +3842,19 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 	/* get rid of pinned extents */
 	trans = btrfs_join_transaction(rc->extent_root);
 	if (IS_ERR(trans)) {
-		err = PTR_ERR(trans);
+		ret = PTR_ERR(trans);
 		goto out_free;
 	}
-	ret = btrfs_commit_transaction(trans);
-	if (ret && !err)
-		err = ret;
+	ret2 = btrfs_commit_transaction(trans);
+	if (ret2 && !ret)
+		ret = ret2;
 out_free:
-	ret = clean_dirty_subvols(rc);
-	if (ret < 0 && !err)
-		err = ret;
+	ret2 = clean_dirty_subvols(rc);
+	if (ret2 < 0 && !ret)
+		ret = ret2;
 	btrfs_free_block_rsv(fs_info, rc->block_rsv);
 	btrfs_free_path(path);
-	return err;
+	return ret;
 }
 
 static int __insert_orphan_inode(struct btrfs_trans_handle *trans,
-- 
2.38.1

[PATCH 17/29] btrfs: create_reloc_inode rename err to ret

[Anand Jain]

Coding style fixes: Rename the variable err to ret, as it represents the
return value.

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 412d328bfbfd..16a3882a4a7c 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3928,7 +3928,7 @@ static noinline_for_stack struct inode *create_reloc_inode(
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root;
 	u64 objectid;
-	int err = 0;
+	int ret = 0;
 
 	root = btrfs_grab_root(fs_info->data_reloc_root);
 	trans = btrfs_start_transaction(root, 6);
@@ -3937,31 +3937,31 @@ static noinline_for_stack struct inode *create_reloc_inode(
 		return ERR_CAST(trans);
 	}
 
-	err = btrfs_get_free_objectid(root, &objectid);
-	if (err)
+	ret = btrfs_get_free_objectid(root, &objectid);
+	if (ret)
 		goto out;
 
-	err = __insert_orphan_inode(trans, root, objectid);
-	if (err)
+	ret = __insert_orphan_inode(trans, root, objectid);
+	if (ret)
 		goto out;
 
 	inode = btrfs_iget(fs_info->sb, objectid, root);
 	if (IS_ERR(inode)) {
 		delete_orphan_inode(trans, root, objectid);
-		err = PTR_ERR(inode);
+		ret = PTR_ERR(inode);
 		inode = NULL;
 		goto out;
 	}
 	BTRFS_I(inode)->index_cnt = group->start;
 
-	err = btrfs_orphan_add(trans, BTRFS_I(inode));
+	ret = btrfs_orphan_add(trans, BTRFS_I(inode));
 out:
 	btrfs_put_root(root);
 	btrfs_end_transaction(trans);
 	btrfs_btree_balance_dirty(fs_info);
-	if (err) {
+	if (ret) {
 		iput(inode);
-		inode = ERR_PTR(err);
+		inode = ERR_PTR(ret);
 	}
 	return inode;
 }
-- 
2.38.1

[PATCH 18/29] btrfs: btrfs_relocate_block_group rename ret to ret2 and err ro ret

[Anand Jain]

Renaem the existing ret variable is renamed to ret2.
Since the variable err is the return variable of the function,
rename it to ret. 

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 16a3882a4a7c..030262943190 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4070,18 +4070,18 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
 	struct reloc_control *rc;
 	struct inode *inode;
 	struct btrfs_path *path;
-	int ret;
+	int ret2;
 	int rw = 0;
-	int err = 0;
+	int ret = 0;
 
 	/*
 	 * This only gets set if we had a half-deleted snapshot on mount.  We
 	 * cannot allow relocation to start while we're still trying to clean up
 	 * these pending deletions.
 	 */
-	ret = wait_on_bit(&fs_info->flags, BTRFS_FS_UNFINISHED_DROPS, TASK_INTERRUPTIBLE);
-	if (ret)
-		return ret;
+	ret2 = wait_on_bit(&fs_info->flags, BTRFS_FS_UNFINISHED_DROPS, TASK_INTERRUPTIBLE);
+	if (ret2)
+		return ret2;
 
 	/* We may have been woken up by close_ctree, so bail if we're closing. */
 	if (btrfs_fs_closing(fs_info))
@@ -4113,25 +4113,25 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
 		return -ENOMEM;
 	}
 
-	ret = reloc_chunk_start(fs_info);
-	if (ret < 0) {
-		err = ret;
+	ret2 = reloc_chunk_start(fs_info);
+	if (ret2 < 0) {
+		ret = ret2;
 		goto out_put_bg;
 	}
 
 	rc->extent_root = extent_root;
 	rc->block_group = bg;
 
-	ret = btrfs_inc_block_group_ro(rc->block_group, true);
-	if (ret) {
-		err = ret;
+	ret2 = btrfs_inc_block_group_ro(rc->block_group, true);
+	if (ret2) {
+		ret = ret2;
 		goto out;
 	}
 	rw = 1;
 
 	path = btrfs_alloc_path();
 	if (!path) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
@@ -4139,18 +4139,18 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
 	btrfs_free_path(path);
 
 	if (!IS_ERR(inode))
-		ret = delete_block_group_cache(fs_info, rc->block_group, inode, 0);
+		ret2 = delete_block_group_cache(fs_info, rc->block_group, inode, 0);
 	else
-		ret = PTR_ERR(inode);
+		ret2 = PTR_ERR(inode);
 
-	if (ret && ret != -ENOENT) {
-		err = ret;
+	if (ret2 && ret2 != -ENOENT) {
+		ret = ret2;
 		goto out;
 	}
 
 	rc->data_inode = create_reloc_inode(fs_info, rc->block_group);
 	if (IS_ERR(rc->data_inode)) {
-		err = PTR_ERR(rc->data_inode);
+		ret = PTR_ERR(rc->data_inode);
 		rc->data_inode = NULL;
 		goto out;
 	}
@@ -4163,17 +4163,17 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
 				 rc->block_group->start,
 				 rc->block_group->length);
 
-	ret = btrfs_zone_finish(rc->block_group);
-	WARN_ON(ret && ret != -EAGAIN);
+	ret2 = btrfs_zone_finish(rc->block_group);
+	WARN_ON(ret2 && ret2 != -EAGAIN);
 
 	while (1) {
 		enum reloc_stage finishes_stage;
 
 		mutex_lock(&fs_info->cleaner_mutex);
-		ret = relocate_block_group(rc);
+		ret2 = relocate_block_group(rc);
 		mutex_unlock(&fs_info->cleaner_mutex);
-		if (ret < 0)
-			err = ret;
+		if (ret2 < 0)
+			ret = ret2;
 
 		finishes_stage = rc->stage;
 		/*
@@ -4186,16 +4186,16 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
 		 * out of the loop if we hit an error.
 		 */
 		if (rc->stage == MOVE_DATA_EXTENTS && rc->found_file_extent) {
-			ret = btrfs_wait_ordered_range(rc->data_inode, 0,
+			ret2 = btrfs_wait_ordered_range(rc->data_inode, 0,
 						       (u64)-1);
-			if (ret)
-				err = ret;
+			if (ret2)
+				ret = ret2;
 			invalidate_mapping_pages(rc->data_inode->i_mapping,
 						 0, -1);
 			rc->stage = UPDATE_DATA_PTRS;
 		}
 
-		if (err < 0)
+		if (ret < 0)
 			goto out;
 
 		if (rc->extents_found == 0)
@@ -4209,14 +4209,14 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
 	WARN_ON(rc->block_group->reserved > 0);
 	WARN_ON(rc->block_group->used > 0);
 out:
-	if (err && rw)
+	if (ret && rw)
 		btrfs_dec_block_group_ro(rc->block_group);
 	iput(rc->data_inode);
 out_put_bg:
 	btrfs_put_block_group(bg);
 	reloc_chunk_end(fs_info);
 	free_reloc_control(rc);
-	return err;
+	return ret;
 }
 
 static noinline_for_stack int mark_garbage_root(struct btrfs_root *root)
-- 
2.38.1

Re: [PATCH 18/29] btrfs: btrfs_relocate_block_group rename ret to ret2 and err ro ret

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:26PM +0530, Anand Jain wrote:
> Renaem the existing ret variable is renamed to ret2.
> Since the variable err is the return variable of the function,
> rename it to ret. 
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/relocation.c | 56 +++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 16a3882a4a7c..030262943190 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4070,18 +4070,18 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
>  	struct reloc_control *rc;
>  	struct inode *inode;
>  	struct btrfs_path *path;
> -	int ret;
> +	int ret2;
>  	int rw = 0;
> -	int err = 0;
> +	int ret = 0;
>  
>  	/*
>  	 * This only gets set if we had a half-deleted snapshot on mount.  We
>  	 * cannot allow relocation to start while we're still trying to clean up
>  	 * these pending deletions.
>  	 */
> -	ret = wait_on_bit(&fs_info->flags, BTRFS_FS_UNFINISHED_DROPS, TASK_INTERRUPTIBLE);
> -	if (ret)
> -		return ret;
> +	ret2 = wait_on_bit(&fs_info->flags, BTRFS_FS_UNFINISHED_DROPS, TASK_INTERRUPTIBLE);
> +	if (ret2)
> +		return ret2;
>  
>  	/* We may have been woken up by close_ctree, so bail if we're closing. */
>  	if (btrfs_fs_closing(fs_info))
> @@ -4113,25 +4113,25 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
>  		return -ENOMEM;
>  	}
>  
> -	ret = reloc_chunk_start(fs_info);
> -	if (ret < 0) {
> -		err = ret;
> +	ret2 = reloc_chunk_start(fs_info);
> +	if (ret2 < 0) {
> +		ret = ret2;
>  		goto out_put_bg;
>  	}
>  
>  	rc->extent_root = extent_root;
>  	rc->block_group = bg;
>  
> -	ret = btrfs_inc_block_group_ro(rc->block_group, true);
> -	if (ret) {
> -		err = ret;
> +	ret2 = btrfs_inc_block_group_ro(rc->block_group, true);
> +	if (ret2) {
> +		ret = ret2;
>  		goto out;
>  	}
>  	rw = 1;
>  
>  	path = btrfs_alloc_path();
>  	if (!path) {
> -		err = -ENOMEM;
> +		ret = -ENOMEM;
>  		goto out;
>  	}
>  
> @@ -4139,18 +4139,18 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
>  	btrfs_free_path(path);
>  
>  	if (!IS_ERR(inode))
> -		ret = delete_block_group_cache(fs_info, rc->block_group, inode, 0);
> +		ret2 = delete_block_group_cache(fs_info, rc->block_group, inode, 0);
>  	else
> -		ret = PTR_ERR(inode);
> +		ret2 = PTR_ERR(inode);
>  
> -	if (ret && ret != -ENOENT) {
> -		err = ret;
> +	if (ret2 && ret2 != -ENOENT) {
> +		ret = ret2;
>  		goto out;
>  	}
>  
>  	rc->data_inode = create_reloc_inode(fs_info, rc->block_group);
>  	if (IS_ERR(rc->data_inode)) {
> -		err = PTR_ERR(rc->data_inode);
> +		ret = PTR_ERR(rc->data_inode);
>  		rc->data_inode = NULL;
>  		goto out;
>  	}
> @@ -4163,17 +4163,17 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
>  				 rc->block_group->start,
>  				 rc->block_group->length);
>  
> -	ret = btrfs_zone_finish(rc->block_group);
> -	WARN_ON(ret && ret != -EAGAIN);
> +	ret2 = btrfs_zone_finish(rc->block_group);
> +	WARN_ON(ret2 && ret2 != -EAGAIN);
>  
>  	while (1) {
>  		enum reloc_stage finishes_stage;
>  
>  		mutex_lock(&fs_info->cleaner_mutex);
> -		ret = relocate_block_group(rc);
> +		ret2 = relocate_block_group(rc);
>  		mutex_unlock(&fs_info->cleaner_mutex);
> -		if (ret < 0)
> -			err = ret;
> +		if (ret2 < 0)
> +			ret = ret2;
>  
>  		finishes_stage = rc->stage;
>  		/*
> @@ -4186,16 +4186,16 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
>  		 * out of the loop if we hit an error.
>  		 */
>  		if (rc->stage == MOVE_DATA_EXTENTS && rc->found_file_extent) {
> -			ret = btrfs_wait_ordered_range(rc->data_inode, 0,
> +			ret2 = btrfs_wait_ordered_range(rc->data_inode, 0,
>  						       (u64)-1);
> -			if (ret)
> -				err = ret;
> +			if (ret2)
> +				ret = ret2;

Ok this is another place where we'll lose ret if it was already set from higher
up.  It's less bad here because we're only doing it if there was an error in
btrfs_wait_ordered_range().  But nonetheless I would like to preserve the error
code from higher up.  So add this to the series you write to fixup the other
error handling.

Then once you've done that, fix this patch to _only_ use ret2 in this block,
because for the rest of the code we can use ret for everything else.  Thanks,

Josef

[PATCH 19/29] btrfs: mark_garbage_root rename err to ret2

[Anand Jain]

In this function, the variable err is used as the second return value. If
it is not zero, rename it to ret2.

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 030262943190..6f8747c907d5 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4223,7 +4223,8 @@ static noinline_for_stack int mark_garbage_root(struct btrfs_root *root)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_trans_handle *trans;
-	int ret, err;
+	int ret;
+	int ret2;
 
 	trans = btrfs_start_transaction(fs_info->tree_root, 0);
 	if (IS_ERR(trans))
@@ -4236,9 +4237,10 @@ static noinline_for_stack int mark_garbage_root(struct btrfs_root *root)
 	ret = btrfs_update_root(trans, fs_info->tree_root,
 				&root->root_key, &root->root_item);
 
-	err = btrfs_end_transaction(trans);
-	if (err)
-		return err;
+	ret2 = btrfs_end_transaction(trans);
+	if (ret2)
+		return ret2;
+
 	return ret;
 }
 
-- 
2.38.1

Re: [PATCH 19/29] btrfs: mark_garbage_root rename err to ret2

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:27PM +0530, Anand Jain wrote:
> In this function, the variable err is used as the second return value. If
> it is not zero, rename it to ret2.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

This is fine but terrible, a good follow up cleanup would be to make
btrfs_end_transaction() a void and remove the random places we check for an
error.  We don't really need it to tell use we have EROFS, we'll get it in some
other operation down the line if we didn't get it somewhere in here.  Thanks,

Josef

Re: [PATCH 19/29] btrfs: mark_garbage_root rename err to ret2

[David Sterba]

On Tue, Mar 19, 2024 at 02:07:48PM -0400, Josef Bacik wrote:
> On Tue, Mar 19, 2024 at 08:25:27PM +0530, Anand Jain wrote:
> > In this function, the variable err is used as the second return value. If
> > it is not zero, rename it to ret2.
> > 
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> This is fine but terrible, a good follow up cleanup would be to make
> btrfs_end_transaction() a void and remove the random places we check for an
> error.  We don't really need it to tell use we have EROFS, we'll get it in some
> other operation down the line if we didn't get it somewhere in here.  Thanks,

I'm not sure we can ignore it everywhere, in many places it's ok, namely
after transaction abort but if some control flow depends on the return
value and starts doing updates that would hit the abort much later then
I'd rather keep it.

Examples I found:

btrfs_delete_subvolume() line 4611, restore dead root status back
btrfs_ioctl_qgroup_assign() if end_transaction fails we don't have any
                            way to communicate errors from the ioctl
btrfs_ioctl_qgroup_create() same
btrfs_ioctl_qgroup_limit() same
... and there are similar cases

It's not just EROFS but also the error code of transaction abort. I'd
rather see the failures detected as early as possible, the delayed error
would only become confusing to debug.

We can have 2 versions so it's semantically clear which one is supposed
to be handled and leave one void for the majority of cases.

[PATCH 20/29] btrfs: btrfs_recover_relocation rename ret to ret2 and err to ret

[Anand Jain]

Fix the code style for the return variable. First, rename ret to ret2,
compile it, and then rename err to ret. This method of changing helped
confirm that there are no instances of the old ret not renamed to ret2.

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 6f8747c907d5..ab3d3d2ed853 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4260,8 +4260,8 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 	struct extent_buffer *leaf;
 	struct reloc_control *rc = NULL;
 	struct btrfs_trans_handle *trans;
-	int ret;
-	int err = 0;
+	int ret2;
+	int ret = 0;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -4273,13 +4273,13 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 	key.offset = (u64)-1;
 
 	while (1) {
-		ret = btrfs_search_slot(NULL, fs_info->tree_root, &key,
+		ret2 = btrfs_search_slot(NULL, fs_info->tree_root, &key,
 					path, 0, 0);
-		if (ret < 0) {
-			err = ret;
+		if (ret2 < 0) {
+			ret = ret2;
 			goto out;
 		}
-		if (ret > 0) {
+		if (ret2 > 0) {
 			if (path->slots[0] == 0)
 				break;
 			path->slots[0]--;
@@ -4294,7 +4294,7 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 		reloc_root = btrfs_read_tree_root(fs_info->tree_root, &key);
 		if (IS_ERR(reloc_root)) {
-			err = PTR_ERR(reloc_root);
+			ret = PTR_ERR(reloc_root);
 			goto out;
 		}
 
@@ -4305,14 +4305,14 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 			fs_root = btrfs_get_fs_root(fs_info,
 					reloc_root->root_key.offset, false);
 			if (IS_ERR(fs_root)) {
-				ret = PTR_ERR(fs_root);
-				if (ret != -ENOENT) {
-					err = ret;
+				ret2 = PTR_ERR(fs_root);
+				if (ret2 != -ENOENT) {
+					ret = ret2;
 					goto out;
 				}
-				ret = mark_garbage_root(reloc_root);
-				if (ret < 0) {
-					err = ret;
+				ret2 = mark_garbage_root(reloc_root);
+				if (ret2 < 0) {
+					ret = ret2;
 					goto out;
 				}
 			} else {
@@ -4332,13 +4332,13 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 	rc = alloc_reloc_control(fs_info);
 	if (!rc) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
-	ret = reloc_chunk_start(fs_info);
-	if (ret < 0) {
-		err = ret;
+	ret2 = reloc_chunk_start(fs_info);
+	if (ret2 < 0) {
+		ret = ret2;
 		goto out_end;
 	}
 
@@ -4348,7 +4348,7 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 	trans = btrfs_join_transaction(rc->extent_root);
 	if (IS_ERR(trans)) {
-		err = PTR_ERR(trans);
+		ret = PTR_ERR(trans);
 		goto out_unset;
 	}
 
@@ -4368,15 +4368,15 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 		fs_root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset,
 					    false);
 		if (IS_ERR(fs_root)) {
-			err = PTR_ERR(fs_root);
+			ret = PTR_ERR(fs_root);
 			list_add_tail(&reloc_root->root_list, &reloc_roots);
 			btrfs_end_transaction(trans);
 			goto out_unset;
 		}
 
-		err = __add_reloc_root(reloc_root);
-		ASSERT(err != -EEXIST);
-		if (err) {
+		ret = __add_reloc_root(reloc_root);
+		ASSERT(ret != -EEXIST);
+		if (ret) {
 			list_add_tail(&reloc_root->root_list, &reloc_roots);
 			btrfs_put_root(fs_root);
 			btrfs_end_transaction(trans);
@@ -4386,8 +4386,8 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 		btrfs_put_root(fs_root);
 	}
 
-	err = btrfs_commit_transaction(trans);
-	if (err)
+	ret = btrfs_commit_transaction(trans);
+	if (ret)
 		goto out_unset;
 
 	merge_reloc_roots(rc);
@@ -4396,14 +4396,14 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 	trans = btrfs_join_transaction(rc->extent_root);
 	if (IS_ERR(trans)) {
-		err = PTR_ERR(trans);
+		ret = PTR_ERR(trans);
 		goto out_clean;
 	}
-	err = btrfs_commit_transaction(trans);
+	ret = btrfs_commit_transaction(trans);
 out_clean:
-	ret = clean_dirty_subvols(rc);
-	if (ret < 0 && !err)
-		err = ret;
+	ret2 = clean_dirty_subvols(rc);
+	if (ret2 < 0 && !ret)
+		ret = ret2;
 out_unset:
 	unset_reloc_control(rc);
 out_end:
@@ -4414,14 +4414,14 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 	btrfs_free_path(path);
 
-	if (err == 0) {
+	if (ret == 0) {
 		/* cleanup orphan inode in data relocation tree */
 		fs_root = btrfs_grab_root(fs_info->data_reloc_root);
 		ASSERT(fs_root);
-		err = btrfs_orphan_cleanup(fs_root);
+		ret = btrfs_orphan_cleanup(fs_root);
 		btrfs_put_root(fs_root);
 	}
-	return err;
+	return ret;
 }
 
 /*
-- 
2.38.1

[PATCH 21/29] btrfs: quick_update_accounting rename err to ret2

[Anand Jain]

In quick_update_accounting err is used as 2nd return value, rename it to
ret2.

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

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 5f90f0605b12..23a08910fa67 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1541,16 +1541,16 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_qgroup *qgroup;
 	int ret = 1;
-	int err = 0;
+	int ret2 = 0;
 
 	qgroup = find_qgroup_rb(fs_info, src);
 	if (!qgroup)
 		goto out;
 	if (qgroup->excl == qgroup->rfer) {
 		ret = 0;
-		err = __qgroup_excl_accounting(fs_info, dst, qgroup, sign);
-		if (err < 0) {
-			ret = err;
+		ret2 = __qgroup_excl_accounting(fs_info, dst, qgroup, sign);
+		if (ret2 < 0) {
+			ret = ret2;
 			goto out;
 		}
 	}
-- 
2.38.1

Re: [PATCH 21/29] btrfs: quick_update_accounting rename err to ret2

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:29PM +0530, Anand Jain wrote:
> In quick_update_accounting err is used as 2nd return value, rename it to
> ret2.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/qgroup.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 5f90f0605b12..23a08910fa67 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1541,16 +1541,16 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>  {
>  	struct btrfs_qgroup *qgroup;
>  	int ret = 1;
> -	int err = 0;
> +	int ret2 = 0;
>  
>  	qgroup = find_qgroup_rb(fs_info, src);
>  	if (!qgroup)
>  		goto out;
>  	if (qgroup->excl == qgroup->rfer) {
>  		ret = 0;
> -		err = __qgroup_excl_accounting(fs_info, dst, qgroup, sign);
> -		if (err < 0) {
> -			ret = err;
> +		ret2 = __qgroup_excl_accounting(fs_info, dst, qgroup, sign);
> +		if (ret2 < 0) {
> +			ret = ret2;
>  			goto out;
>  		}

This can be reworked to

ret = __qgroup_excl_accounting(fs_info, dst, qgroup, sign);
if (ret < 0)
	goto out;
ret = 0;

Thanks,

Josef

[PATCH 22/29] btrfs: btrfs_qgroup_rescan_worker rename ret to ret2 and err to ret

[Anand Jain]

Rename ret to ret2 compile and then err to ret. Also, new ret2 is found to
be localized within the 'if (trans)' statement, so move its declaration there.

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

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 23a08910fa67..3ba4c4ddb627 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3709,8 +3709,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 						     qgroup_rescan_work);
 	struct btrfs_path *path;
 	struct btrfs_trans_handle *trans = NULL;
-	int err = -ENOMEM;
-	int ret = 0;
+	int ret = -ENOMEM;
 	bool stopped = false;
 	bool did_leaf_rescans = false;
 
@@ -3727,18 +3726,18 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	path->search_commit_root = 1;
 	path->skip_locking = 1;
 
-	err = 0;
-	while (!err && !(stopped = rescan_should_stop(fs_info))) {
+	ret = 0;
+	while (!ret && !(stopped = rescan_should_stop(fs_info))) {
 		trans = btrfs_start_transaction(fs_info->fs_root, 0);
 		if (IS_ERR(trans)) {
-			err = PTR_ERR(trans);
+			ret = PTR_ERR(trans);
 			break;
 		}
 
-		err = qgroup_rescan_leaf(trans, path);
+		ret = qgroup_rescan_leaf(trans, path);
 		did_leaf_rescans = true;
 
-		if (err > 0)
+		if (ret > 0)
 			btrfs_commit_transaction(trans);
 		else
 			btrfs_end_transaction(trans);
@@ -3748,10 +3747,10 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	btrfs_free_path(path);
 
 	mutex_lock(&fs_info->qgroup_rescan_lock);
-	if (err > 0 &&
+	if (ret > 0 &&
 	    fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT) {
 		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
-	} else if (err < 0 || stopped) {
+	} else if (ret < 0 || stopped) {
 		fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
 	}
 	mutex_unlock(&fs_info->qgroup_rescan_lock);
@@ -3766,11 +3765,11 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	if (did_leaf_rescans) {
 		trans = btrfs_start_transaction(fs_info->quota_root, 1);
 		if (IS_ERR(trans)) {
-			err = PTR_ERR(trans);
+			ret = PTR_ERR(trans);
 			trans = NULL;
 			btrfs_err(fs_info,
 				  "fail to start transaction for status update: %d",
-				  err);
+				  ret);
 		}
 	} else {
 		trans = NULL;
@@ -3781,11 +3780,12 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	    fs_info->qgroup_flags & BTRFS_QGROUP_RUNTIME_FLAG_CANCEL_RESCAN)
 		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN;
 	if (trans) {
-		ret = update_qgroup_status_item(trans);
-		if (ret < 0) {
-			err = ret;
+		int ret2 = update_qgroup_status_item(trans);
+
+		if (ret2 < 0) {
+			ret = ret2;
 			btrfs_err(fs_info, "fail to update qgroup status: %d",
-				  err);
+				  ret);
 		}
 	}
 	fs_info->qgroup_rescan_running = false;
@@ -3802,11 +3802,11 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 		btrfs_info(fs_info, "qgroup scan paused");
 	} else if (fs_info->qgroup_flags & BTRFS_QGROUP_RUNTIME_FLAG_CANCEL_RESCAN) {
 		btrfs_info(fs_info, "qgroup scan cancelled");
-	} else if (err >= 0) {
+	} else if (ret >= 0) {
 		btrfs_info(fs_info, "qgroup scan completed%s",
-			err > 0 ? " (inconsistency flag cleared)" : "");
+			ret > 0 ? " (inconsistency flag cleared)" : "");
 	} else {
-		btrfs_err(fs_info, "qgroup scan failed with %d", err);
+		btrfs_err(fs_info, "qgroup scan failed with %d", ret);
 	}
 }
 
-- 
2.38.1

[PATCH 23/29] btrfs: lookup_extent_data_ref rename ret to ret2 and err to ret

[Anand Jain]

First, rename ret to ret2, compile, and then rename err to 'ret',
to ensure that no original ret remains as the new ret.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 1a1191efe59e..4b0a55e66a55 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -448,9 +448,9 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
 	struct btrfs_extent_data_ref *ref;
 	struct extent_buffer *leaf;
 	u32 nritems;
-	int ret;
+	int ret2;
 	int recow;
-	int err = -ENOENT;
+	int ret = -ENOENT;
 
 	key.objectid = bytenr;
 	if (parent) {
@@ -463,14 +463,14 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
 	}
 again:
 	recow = 0;
-	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
-	if (ret < 0) {
-		err = ret;
+	ret2 = btrfs_search_slot(trans, root, &key, path, -1, 1);
+	if (ret2 < 0) {
+		ret = ret2;
 		goto fail;
 	}
 
 	if (parent) {
-		if (!ret)
+		if (!ret2)
 			return 0;
 		goto fail;
 	}
@@ -479,10 +479,10 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
 	nritems = btrfs_header_nritems(leaf);
 	while (1) {
 		if (path->slots[0] >= nritems) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				err = ret;
-			if (ret)
+			ret2 = btrfs_next_leaf(root, path);
+			if (ret2 < 0)
+				ret = ret2;
+			if (ret2)
 				goto fail;
 
 			leaf = path->nodes[0];
@@ -504,13 +504,13 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
 				btrfs_release_path(path);
 				goto again;
 			}
-			err = 0;
+			ret = 0;
 			break;
 		}
 		path->slots[0]++;
 	}
 fail:
-	return err;
+	return ret;
 }
 
 static noinline int insert_extent_data_ref(struct btrfs_trans_handle *trans,
-- 
2.38.1

Re: [PATCH 23/29] btrfs: lookup_extent_data_ref rename ret to ret2 and err to ret

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:31PM +0530, Anand Jain wrote:
> First, rename ret to ret2, compile, and then rename err to 'ret',
> to ensure that no original ret remains as the new ret.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/extent-tree.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 1a1191efe59e..4b0a55e66a55 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -448,9 +448,9 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
>  	struct btrfs_extent_data_ref *ref;
>  	struct extent_buffer *leaf;
>  	u32 nritems;
> -	int ret;
> +	int ret2;
>  	int recow;
> -	int err = -ENOENT;
> +	int ret = -ENOENT;
>  
>  	key.objectid = bytenr;
>  	if (parent) {
> @@ -463,14 +463,14 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
>  	}
>  again:
>  	recow = 0;
> -	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
> -	if (ret < 0) {
> -		err = ret;
> +	ret2 = btrfs_search_slot(trans, root, &key, path, -1, 1);
> +	if (ret2 < 0) {
> +		ret = ret2;
>  		goto fail;
>  	}
>  
>  	if (parent) {
> -		if (!ret)
> +		if (!ret2)
>  			return 0;

You don't need ret2, you can just rework this to

if (parent) {
	if (ret)
		return -ENOENT;
	return 0;
}

>  		goto fail;
>  	}
> @@ -479,10 +479,10 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
>  	nritems = btrfs_header_nritems(leaf);
>  	while (1) {
>  		if (path->slots[0] >= nritems) {
> -			ret = btrfs_next_leaf(root, path);
> -			if (ret < 0)
> -				err = ret;
> -			if (ret)
> +			ret2 = btrfs_next_leaf(root, path);
> +			if (ret2 < 0)
> +				ret = ret2;
> +			if (ret2)
>  				goto fail;

Just rework this to

ret = btrfs_next_leaf(root, path);
if (ret) {
	if (ret > 1)
		return -ENOENT;
	return ret;
}

Thanks,

Josef	

Re: [PATCH 23/29] btrfs: lookup_extent_data_ref rename ret to ret2 and err to ret

[Anand Jain]

On 3/20/24 02:17, Josef Bacik wrote:
> On Tue, Mar 19, 2024 at 08:25:31PM +0530, Anand Jain wrote:
>> First, rename ret to ret2, compile, and then rename err to 'ret',
>> to ensure that no original ret remains as the new ret.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/extent-tree.c | 24 ++++++++++++------------
>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 1a1191efe59e..4b0a55e66a55 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -448,9 +448,9 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
>>   	struct btrfs_extent_data_ref *ref;
>>   	struct extent_buffer *leaf;
>>   	u32 nritems;
>> -	int ret;
>> +	int ret2;
>>   	int recow;
>> -	int err = -ENOENT;
>> +	int ret = -ENOENT;
>>   
>>   	key.objectid = bytenr;
>>   	if (parent) {
>> @@ -463,14 +463,14 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
>>   	}
>>   again:
>>   	recow = 0;
>> -	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
>> -	if (ret < 0) {
>> -		err = ret;
>> +	ret2 = btrfs_search_slot(trans, root, &key, path, -1, 1);
>> +	if (ret2 < 0) {
>> +		ret = ret2;
>>   		goto fail;
>>   	}
>>   
>>   	if (parent) {
>> -		if (!ret)
>> +		if (!ret2)
>>   			return 0;
> 
> You don't need ret2, you can just rework this to
> 
> if (parent) {
> 	if (ret)
> 		return -ENOENT;
> 	return 0;
> }
> 
>>   		goto fail;
>>   	}
>> @@ -479,10 +479,10 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,


This changes applied with the reinitialize of ret.

+ ret = -ENOENT;

Thanks, Anand

>>   	nritems = btrfs_header_nritems(leaf);
>>   	while (1) {
>>   		if (path->slots[0] >= nritems) {
>> -			ret = btrfs_next_leaf(root, path);
>> -			if (ret < 0)
>> -				err = ret;
>> -			if (ret)
>> +			ret2 = btrfs_next_leaf(root, path);
>> +			if (ret2 < 0)
>> +				ret = ret2;
>> +			if (ret2)
>>   				goto fail;
> 
> Just rework this to
> 
> ret = btrfs_next_leaf(root, path);
> if (ret) {
> 	if (ret > 1)
> 		return -ENOENT;
> 	return ret;
> }
> 
> Thanks,
> 
> Josef	

[PATCH 24/29] btrfs: btrfs_drop_snapshot rename ret to ret2 and err to ret

[Anand Jain]

To fix code style for the return variable, first rename ret to ret2
comopile and then rename err to ret.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 4b0a55e66a55..acea2a7be4e5 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5858,8 +5858,8 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 	struct btrfs_root_item *root_item = &root->root_item;
 	struct walk_control *wc;
 	struct btrfs_key key;
-	int err = 0;
-	int ret;
+	int ret = 0;
+	int ret2;
 	int level;
 	bool root_dropped = false;
 	bool unfinished_drop = false;
@@ -5868,14 +5868,14 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 
 	path = btrfs_alloc_path();
 	if (!path) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
 	wc = kzalloc(sizeof(*wc), GFP_NOFS);
 	if (!wc) {
 		btrfs_free_path(path);
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
@@ -5888,12 +5888,12 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 	else
 		trans = btrfs_start_transaction(tree_root, 0);
 	if (IS_ERR(trans)) {
-		err = PTR_ERR(trans);
+		ret = PTR_ERR(trans);
 		goto out_free;
 	}
 
-	err = btrfs_run_delayed_items(trans);
-	if (err)
+	ret = btrfs_run_delayed_items(trans);
+	if (ret)
 		goto out_end_trans;
 
 	/*
@@ -5922,13 +5922,13 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 		level = btrfs_root_drop_level(root_item);
 		BUG_ON(level == 0);
 		path->lowest_level = level;
-		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
+		ret2 = btrfs_search_slot(NULL, root, &key, path, 0, 0);
 		path->lowest_level = 0;
-		if (ret < 0) {
-			err = ret;
+		if (ret2 < 0) {
+			ret = ret2;
 			goto out_end_trans;
 		}
-		WARN_ON(ret > 0);
+		WARN_ON(ret2 > 0);
 
 		/*
 		 * unlock our path, this is safe because only this
@@ -5941,12 +5941,12 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 			btrfs_tree_lock(path->nodes[level]);
 			path->locks[level] = BTRFS_WRITE_LOCK;
 
-			ret = btrfs_lookup_extent_info(trans, fs_info,
+			ret2 = btrfs_lookup_extent_info(trans, fs_info,
 						path->nodes[level]->start,
 						level, 1, &wc->refs[level],
 						&wc->flags[level], NULL);
-			if (ret < 0) {
-				err = ret;
+			if (ret2 < 0) {
+				ret = ret2;
 				goto out_end_trans;
 			}
 			BUG_ON(wc->refs[level] == 0);
@@ -5971,21 +5971,21 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 
 	while (1) {
 
-		ret = walk_down_tree(trans, root, path, wc);
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			err = ret;
+		ret2 = walk_down_tree(trans, root, path, wc);
+		if (ret2 < 0) {
+			btrfs_abort_transaction(trans, ret2);
+			ret = ret2;
 			break;
 		}
 
-		ret = walk_up_tree(trans, root, path, wc, BTRFS_MAX_LEVEL);
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			err = ret;
+		ret2 = walk_up_tree(trans, root, path, wc, BTRFS_MAX_LEVEL);
+		if (ret2 < 0) {
+			btrfs_abort_transaction(trans, ret2);
+			ret = ret2;
 			break;
 		}
 
-		if (ret > 0) {
+		if (ret2 > 0) {
 			BUG_ON(wc->stage != DROP_REFERENCE);
 			break;
 		}
@@ -6003,12 +6003,12 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 		BUG_ON(wc->level == 0);
 		if (btrfs_should_end_transaction(trans) ||
 		    (!for_reloc && btrfs_need_cleaner_sleep(fs_info))) {
-			ret = btrfs_update_root(trans, tree_root,
+			ret2 = btrfs_update_root(trans, tree_root,
 						&root->root_key,
 						root_item);
-			if (ret) {
-				btrfs_abort_transaction(trans, ret);
-				err = ret;
+			if (ret2) {
+				btrfs_abort_transaction(trans, ret2);
+				ret = ret2;
 				goto out_end_trans;
 			}
 
@@ -6019,7 +6019,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 			if (!for_reloc && btrfs_need_cleaner_sleep(fs_info)) {
 				btrfs_debug(fs_info,
 					    "drop snapshot early exit");
-				err = -EAGAIN;
+				ret = -EAGAIN;
 				goto out_free;
 			}
 
@@ -6033,30 +6033,30 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 			else
 				trans = btrfs_start_transaction(tree_root, 0);
 			if (IS_ERR(trans)) {
-				err = PTR_ERR(trans);
+				ret = PTR_ERR(trans);
 				goto out_free;
 			}
 		}
 	}
 	btrfs_release_path(path);
-	if (err)
+	if (ret)
 		goto out_end_trans;
 
-	ret = btrfs_del_root(trans, &root->root_key);
-	if (ret) {
-		btrfs_abort_transaction(trans, ret);
-		err = ret;
+	ret2 = btrfs_del_root(trans, &root->root_key);
+	if (ret2) {
+		btrfs_abort_transaction(trans, ret2);
+		ret = ret2;
 		goto out_end_trans;
 	}
 
 	if (!is_reloc_root) {
-		ret = btrfs_find_root(tree_root, &root->root_key, path,
+		ret2 = btrfs_find_root(tree_root, &root->root_key, path,
 				      NULL, NULL);
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			err = ret;
+		if (ret2 < 0) {
+			btrfs_abort_transaction(trans, ret2);
+			ret = ret2;
 			goto out_end_trans;
-		} else if (ret > 0) {
+		} else if (ret2 > 0) {
 			/* if we fail to delete the orphan item this time
 			 * around, it'll get picked up the next time.
 			 *
@@ -6093,7 +6093,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 	 * We were an unfinished drop root, check to see if there are any
 	 * pending, and if not clear and wake up any waiters.
 	 */
-	if (!err && unfinished_drop)
+	if (!ret && unfinished_drop)
 		btrfs_maybe_wake_unfinished_drop(fs_info);
 
 	/*
@@ -6105,7 +6105,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 	 */
 	if (!for_reloc && !root_dropped)
 		btrfs_add_dead_root(root);
-	return err;
+	return ret;
 }
 
 /*
-- 
2.38.1

Re: [PATCH 24/29] btrfs: btrfs_drop_snapshot rename ret to ret2 and err to ret

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:32PM +0530, Anand Jain wrote:
> To fix code style for the return variable, first rename ret to ret2
> comopile and then rename err to ret.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/extent-tree.c | 82 +++++++++++++++++++++---------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 4b0a55e66a55..acea2a7be4e5 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -5858,8 +5858,8 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  	struct btrfs_root_item *root_item = &root->root_item;
>  	struct walk_control *wc;
>  	struct btrfs_key key;
> -	int err = 0;
> -	int ret;
> +	int ret = 0;
> +	int ret2;
>  	int level;
>  	bool root_dropped = false;
>  	bool unfinished_drop = false;
> @@ -5868,14 +5868,14 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  
>  	path = btrfs_alloc_path();
>  	if (!path) {
> -		err = -ENOMEM;
> +		ret = -ENOMEM;
>  		goto out;
>  	}
>  
>  	wc = kzalloc(sizeof(*wc), GFP_NOFS);
>  	if (!wc) {
>  		btrfs_free_path(path);
> -		err = -ENOMEM;
> +		ret = -ENOMEM;
>  		goto out;
>  	}
>  
> @@ -5888,12 +5888,12 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  	else
>  		trans = btrfs_start_transaction(tree_root, 0);
>  	if (IS_ERR(trans)) {
> -		err = PTR_ERR(trans);
> +		ret = PTR_ERR(trans);
>  		goto out_free;
>  	}
>  
> -	err = btrfs_run_delayed_items(trans);
> -	if (err)
> +	ret = btrfs_run_delayed_items(trans);
> +	if (ret)
>  		goto out_end_trans;
>  
>  	/*
> @@ -5922,13 +5922,13 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  		level = btrfs_root_drop_level(root_item);
>  		BUG_ON(level == 0);
>  		path->lowest_level = level;
> -		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> +		ret2 = btrfs_search_slot(NULL, root, &key, path, 0, 0);
>  		path->lowest_level = 0;
> -		if (ret < 0) {
> -			err = ret;
> +		if (ret2 < 0) {
> +			ret = ret2;
>  			goto out_end_trans;
>  		}
> -		WARN_ON(ret > 0);
> +		WARN_ON(ret2 > 0);
>  
>  		/*
>  		 * unlock our path, this is safe because only this
> @@ -5941,12 +5941,12 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  			btrfs_tree_lock(path->nodes[level]);
>  			path->locks[level] = BTRFS_WRITE_LOCK;
>  
> -			ret = btrfs_lookup_extent_info(trans, fs_info,
> +			ret2 = btrfs_lookup_extent_info(trans, fs_info,
>  						path->nodes[level]->start,
>  						level, 1, &wc->refs[level],
>  						&wc->flags[level], NULL);
> -			if (ret < 0) {
> -				err = ret;
> +			if (ret2 < 0) {
> +				ret = ret2;
>  				goto out_end_trans;
>  			}
>  			BUG_ON(wc->refs[level] == 0);
> @@ -5971,21 +5971,21 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  
>  	while (1) {
>  
> -		ret = walk_down_tree(trans, root, path, wc);
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			err = ret;
> +		ret2 = walk_down_tree(trans, root, path, wc);
> +		if (ret2 < 0) {
> +			btrfs_abort_transaction(trans, ret2);
> +			ret = ret2;
>  			break;
>  		}
>  
> -		ret = walk_up_tree(trans, root, path, wc, BTRFS_MAX_LEVEL);
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			err = ret;
> +		ret2 = walk_up_tree(trans, root, path, wc, BTRFS_MAX_LEVEL);
> +		if (ret2 < 0) {
> +			btrfs_abort_transaction(trans, ret2);
> +			ret = ret2;
>  			break;
>  		}
>  
> -		if (ret > 0) {
> +		if (ret2 > 0) {
>  			BUG_ON(wc->stage != DROP_REFERENCE);

This can be changed to

if (ret > 0) {
	BUG_ON(wc->stage != DROP_REFERENCE);
	ret = 0;
	break;
}

>  			break;
>  		}
> @@ -6003,12 +6003,12 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  		BUG_ON(wc->level == 0);
>  		if (btrfs_should_end_transaction(trans) ||
>  		    (!for_reloc && btrfs_need_cleaner_sleep(fs_info))) {
> -			ret = btrfs_update_root(trans, tree_root,
> +			ret2 = btrfs_update_root(trans, tree_root,
>  						&root->root_key,
>  						root_item);
> -			if (ret) {
> -				btrfs_abort_transaction(trans, ret);
> -				err = ret;
> +			if (ret2) {
> +				btrfs_abort_transaction(trans, ret2);
> +				ret = ret2;
>  				goto out_end_trans;
>  			}
>  
> @@ -6019,7 +6019,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  			if (!for_reloc && btrfs_need_cleaner_sleep(fs_info)) {
>  				btrfs_debug(fs_info,
>  					    "drop snapshot early exit");
> -				err = -EAGAIN;
> +				ret = -EAGAIN;
>  				goto out_free;
>  			}
>  
> @@ -6033,30 +6033,30 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  			else
>  				trans = btrfs_start_transaction(tree_root, 0);
>  			if (IS_ERR(trans)) {
> -				err = PTR_ERR(trans);
> +				ret = PTR_ERR(trans);
>  				goto out_free;
>  			}
>  		}
>  	}
>  	btrfs_release_path(path);
> -	if (err)
> +	if (ret)
>  		goto out_end_trans;
>  
> -	ret = btrfs_del_root(trans, &root->root_key);
> -	if (ret) {
> -		btrfs_abort_transaction(trans, ret);
> -		err = ret;
> +	ret2 = btrfs_del_root(trans, &root->root_key);
> +	if (ret2) {
> +		btrfs_abort_transaction(trans, ret2);
> +		ret = ret2;
>  		goto out_end_trans;
>  	}
>  
>  	if (!is_reloc_root) {
> -		ret = btrfs_find_root(tree_root, &root->root_key, path,
> +		ret2 = btrfs_find_root(tree_root, &root->root_key, path,
>  				      NULL, NULL);
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			err = ret;
> +		if (ret2 < 0) {
> +			btrfs_abort_transaction(trans, ret2);
> +			ret = ret2;
>  			goto out_end_trans;
> -		} else if (ret > 0) {
> +		} else if (ret2 > 0) {

And then here we just set ret = 0 again, and then we have no need for ret2.
Thanks,

Josef

[PATCH 25/29] btrfs: btrfs_drop_subtree rename retw to ret2

[Anand Jain]

retw is a helper return variable used to update the actual return value
ret. Rename it to ret2.

Additionally, ret2 is used only inside 'while (1)', which could potentially
be declared inside the loop. I chose not to do that to minimize the number
of times the 'while' loop has to allocate/deallocate.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index acea2a7be4e5..5064cdd55d2f 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6125,7 +6125,7 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
 	int level;
 	int parent_level;
 	int ret = 0;
-	int wret;
+	int ret2;
 
 	BUG_ON(root->root_key.objectid != BTRFS_TREE_RELOC_OBJECTID);
 
@@ -6161,16 +6161,16 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
 	wc->reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
 
 	while (1) {
-		wret = walk_down_tree(trans, root, path, wc);
-		if (wret < 0) {
-			ret = wret;
+		ret2 = walk_down_tree(trans, root, path, wc);
+		if (ret2 < 0) {
+			ret = ret2;
 			break;
 		}
 
-		wret = walk_up_tree(trans, root, path, wc, parent_level);
-		if (wret < 0)
-			ret = wret;
-		if (wret != 0)
+		ret2 = walk_up_tree(trans, root, path, wc, parent_level);
+		if (ret2 < 0)
+			ret = ret2;
+		if (ret2 != 0)
 			break;
 	}
 
-- 
2.38.1

Re: [PATCH 25/29] btrfs: btrfs_drop_subtree rename retw to ret2

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:33PM +0530, Anand Jain wrote:
> retw is a helper return variable used to update the actual return value
> ret. Rename it to ret2.
> 
> Additionally, ret2 is used only inside 'while (1)', which could potentially
> be declared inside the loop. I chose not to do that to minimize the number
> of times the 'while' loop has to allocate/deallocate.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/extent-tree.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index acea2a7be4e5..5064cdd55d2f 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6125,7 +6125,7 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
>  	int level;
>  	int parent_level;
>  	int ret = 0;
> -	int wret;
> +	int ret2;
>  
>  	BUG_ON(root->root_key.objectid != BTRFS_TREE_RELOC_OBJECTID);
>  
> @@ -6161,16 +6161,16 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
>  	wc->reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
>  
>  	while (1) {
> -		wret = walk_down_tree(trans, root, path, wc);
> -		if (wret < 0) {
> -			ret = wret;
> +		ret2 = walk_down_tree(trans, root, path, wc);
> +		if (ret2 < 0) {
> +			ret = ret2;
>  			break;
>  		}
>  
> -		wret = walk_up_tree(trans, root, path, wc, parent_level);
> -		if (wret < 0)
> -			ret = wret;
> -		if (wret != 0)
> +		ret2 = walk_up_tree(trans, root, path, wc, parent_level);
> +		if (ret2 < 0)
> +			ret = ret2;
> +		if (ret2 != 0)

This can be reworked to

ret = walk_up_tree();
if (ret) {
	if (ret > 0)
		ret = 0;
	break;
}

and then we don't need ret2.  Thanks,

Josef

[PATCH 26/29] btrfs: btrfs_dirty_pages rename variable err to ret

[Anand Jain]

Rename local variable err to ret. To make it consistent with the rest of
the code.

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

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index f9d76072398d..f55ac15d727a 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -128,7 +128,7 @@ int btrfs_dirty_pages(struct btrfs_inode *inode, struct page **pages,
 		      struct extent_state **cached, bool noreserve)
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
-	int err = 0;
+	int ret = 0;
 	int i;
 	u64 num_bytes;
 	u64 start_pos;
@@ -158,10 +158,10 @@ int btrfs_dirty_pages(struct btrfs_inode *inode, struct page **pages,
 			 EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING | EXTENT_DEFRAG,
 			 cached);
 
-	err = btrfs_set_extent_delalloc(inode, start_pos, end_of_last_block,
+	ret = btrfs_set_extent_delalloc(inode, start_pos, end_of_last_block,
 					extra_bits, cached);
-	if (err)
-		return err;
+	if (ret)
+		return ret;
 
 	for (i = 0; i < num_pages; i++) {
 		struct page *p = pages[i];
-- 
2.38.1

[PATCH 27/29] btrfs: prepare_pages rename err to ret

[Anand Jain]

A simple, trivial rename of err to ret to maintain consistent coding
style and reduce confusion

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

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index f55ac15d727a..c22264c9cc45 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -915,7 +915,7 @@ static noinline int prepare_pages(struct inode *inode, struct page **pages,
 	unsigned long index = pos >> PAGE_SHIFT;
 	gfp_t mask = get_prepare_gfp_flags(inode, nowait);
 	fgf_t fgp_flags = get_prepare_fgp_flags(nowait);
-	int err = 0;
+	int ret = 0;
 	int faili;
 
 	for (i = 0; i < num_pages; i++) {
@@ -925,28 +925,28 @@ static noinline int prepare_pages(struct inode *inode, struct page **pages,
 		if (!pages[i]) {
 			faili = i - 1;
 			if (nowait)
-				err = -EAGAIN;
+				ret = -EAGAIN;
 			else
-				err = -ENOMEM;
+				ret = -ENOMEM;
 			goto fail;
 		}
 
-		err = set_page_extent_mapped(pages[i]);
-		if (err < 0) {
+		ret = set_page_extent_mapped(pages[i]);
+		if (ret < 0) {
 			faili = i;
 			goto fail;
 		}
 
 		if (i == 0)
-			err = prepare_uptodate_page(inode, pages[i], pos,
+			ret = prepare_uptodate_page(inode, pages[i], pos,
 						    force_uptodate);
-		if (!err && i == num_pages - 1)
-			err = prepare_uptodate_page(inode, pages[i],
+		if (!ret && i == num_pages - 1)
+			ret = prepare_uptodate_page(inode, pages[i],
 						    pos + write_bytes, false);
-		if (err) {
+		if (ret) {
 			put_page(pages[i]);
-			if (!nowait && err == -EAGAIN) {
-				err = 0;
+			if (!nowait && ret == -EAGAIN) {
+				ret = 0;
 				goto again;
 			}
 			faili = i - 1;
@@ -962,7 +962,7 @@ static noinline int prepare_pages(struct inode *inode, struct page **pages,
 		put_page(pages[faili]);
 		faili--;
 	}
-	return err;
+	return ret;
 
 }
 
-- 
2.38.1

[PATCH 28/29] btrfs: btrfs_direct_write rename err to ret

[Anand Jain]

A simple, trivial rename of err to ret to maintain consistent coding style
and reduce confusion.

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

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index c22264c9cc45..0c23053951be 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1465,7 +1465,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	ssize_t written_buffered;
 	size_t prev_left = 0;
 	loff_t endbyte;
-	ssize_t err;
+	ssize_t ret;
 	unsigned int ilock_flags = 0;
 	struct iomap_dio *dio;
 
@@ -1482,9 +1482,9 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 		ilock_flags |= BTRFS_ILOCK_SHARED;
 
 relock:
-	err = btrfs_inode_lock(BTRFS_I(inode), ilock_flags);
-	if (err < 0)
-		return err;
+	ret = btrfs_inode_lock(BTRFS_I(inode), ilock_flags);
+	if (ret < 0)
+		return ret;
 
 	/* Shared lock cannot be used with security bits set. */
 	if ((ilock_flags & BTRFS_ILOCK_SHARED) && !IS_NOSEC(inode)) {
@@ -1493,14 +1493,14 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 		goto relock;
 	}
 
-	err = generic_write_checks(iocb, from);
-	if (err <= 0) {
+	ret = generic_write_checks(iocb, from);
+	if (ret <= 0) {
 		btrfs_inode_unlock(BTRFS_I(inode), ilock_flags);
-		return err;
+		return ret;
 	}
 
-	err = btrfs_write_check(iocb, from, err);
-	if (err < 0) {
+	ret = btrfs_write_check(iocb, from, ret);
+	if (ret < 0) {
 		btrfs_inode_unlock(BTRFS_I(inode), ilock_flags);
 		goto out;
 	}
@@ -1552,15 +1552,15 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	btrfs_inode_unlock(BTRFS_I(inode), ilock_flags);
 
 	if (IS_ERR_OR_NULL(dio))
-		err = PTR_ERR_OR_ZERO(dio);
+		ret = PTR_ERR_OR_ZERO(dio);
 	else
-		err = iomap_dio_complete(dio);
+		ret = iomap_dio_complete(dio);
 
 	/* No increment (+=) because iomap returns a cumulative value. */
-	if (err > 0)
-		written = err;
+	if (ret > 0)
+		written = ret;
 
-	if (iov_iter_count(from) > 0 && (err == -EFAULT || err > 0)) {
+	if (iov_iter_count(from) > 0 && (ret == -EFAULT || ret > 0)) {
 		const size_t left = iov_iter_count(from);
 		/*
 		 * We have more data left to write. Try to fault in as many as
@@ -1577,7 +1577,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 		 * to buffered IO in case we haven't made any progress.
 		 */
 		if (left == prev_left) {
-			err = -ENOTBLK;
+			ret = -ENOTBLK;
 		} else {
 			fault_in_iov_iter_readable(from, left);
 			prev_left = left;
@@ -1586,10 +1586,10 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	}
 
 	/*
-	 * If 'err' is -ENOTBLK or we have not written all data, then it means
+	 * If 'ret' is -ENOTBLK or we have not written all data, then it means
 	 * we must fallback to buffered IO.
 	 */
-	if ((err < 0 && err != -ENOTBLK) || !iov_iter_count(from))
+	if ((ret < 0 && ret != -ENOTBLK) || !iov_iter_count(from))
 		goto out;
 
 buffered:
@@ -1600,14 +1600,14 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	 * below, we will block when flushing and waiting for the IO.
 	 */
 	if (iocb->ki_flags & IOCB_NOWAIT) {
-		err = -EAGAIN;
+		ret = -EAGAIN;
 		goto out;
 	}
 
 	pos = iocb->ki_pos;
 	written_buffered = btrfs_buffered_write(iocb, from);
 	if (written_buffered < 0) {
-		err = written_buffered;
+		ret = written_buffered;
 		goto out;
 	}
 	/*
@@ -1615,18 +1615,18 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	 * able to read what was just written.
 	 */
 	endbyte = pos + written_buffered - 1;
-	err = btrfs_fdatawrite_range(inode, pos, endbyte);
-	if (err)
+	ret = btrfs_fdatawrite_range(inode, pos, endbyte);
+	if (ret)
 		goto out;
-	err = filemap_fdatawait_range(inode->i_mapping, pos, endbyte);
-	if (err)
+	ret = filemap_fdatawait_range(inode->i_mapping, pos, endbyte);
+	if (ret)
 		goto out;
 	written += written_buffered;
 	iocb->ki_pos = pos + written_buffered;
 	invalidate_mapping_pages(file->f_mapping, pos >> PAGE_SHIFT,
 				 endbyte >> PAGE_SHIFT);
 out:
-	return err < 0 ? err : written;
+	return ret < 0 ? ret : written;
 }
 
 static ssize_t btrfs_encoded_write(struct kiocb *iocb, struct iov_iter *from,
-- 
2.38.1

[PATCH 29/29] btrfs: fixup_tree_root_location rename ret to ret2 and err to ret

[Anand Jain]

Fix the code style for the return variable. First, rename ret to ret2,
compile it, and then rename err to ret. This helps confirm that there are
no instances of the old ret not renamed to ret2.

Also, there is an opportunity to drop the initialization of ret to 0,
with the first use of ret2 replaced with ret. However, due to the confusing
git-diff, I refrain from doing that as of now.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 952c92e6dfcf..d890cb5ab548 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5443,29 +5443,29 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
 	struct btrfs_root_ref *ref;
 	struct extent_buffer *leaf;
 	struct btrfs_key key;
-	int ret;
-	int err = 0;
+	int ret2;
+	int ret = 0;
 	struct fscrypt_name fname;
 
-	ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 0, &fname);
-	if (ret)
-		return ret;
+	ret2 = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 0, &fname);
+	if (ret2)
+		return ret2;
 
 	path = btrfs_alloc_path();
 	if (!path) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
-	err = -ENOENT;
+	ret = -ENOENT;
 	key.objectid = dir->root->root_key.objectid;
 	key.type = BTRFS_ROOT_REF_KEY;
 	key.offset = location->objectid;
 
-	ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
-	if (ret) {
-		if (ret < 0)
-			err = ret;
+	ret2 = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
+	if (ret2) {
+		if (ret2 < 0)
+			ret = ret2;
 		goto out;
 	}
 
@@ -5475,16 +5475,16 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
 	    btrfs_root_ref_name_len(leaf, ref) != fname.disk_name.len)
 		goto out;
 
-	ret = memcmp_extent_buffer(leaf, fname.disk_name.name,
+	ret2 = memcmp_extent_buffer(leaf, fname.disk_name.name,
 				   (unsigned long)(ref + 1), fname.disk_name.len);
-	if (ret)
+	if (ret2)
 		goto out;
 
 	btrfs_release_path(path);
 
 	new_root = btrfs_get_fs_root(fs_info, location->objectid, true);
 	if (IS_ERR(new_root)) {
-		err = PTR_ERR(new_root);
+		ret = PTR_ERR(new_root);
 		goto out;
 	}
 
@@ -5492,11 +5492,11 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
 	location->objectid = btrfs_root_dirid(&new_root->root_item);
 	location->type = BTRFS_INODE_ITEM_KEY;
 	location->offset = 0;
-	err = 0;
+	ret = 0;
 out:
 	btrfs_free_path(path);
 	fscrypt_free_filename(&fname);
-	return err;
+	return ret;
 }
 
 static void inode_tree_add(struct btrfs_inode *inode)
-- 
2.38.1

Re: [PATCH 29/29] btrfs: fixup_tree_root_location rename ret to ret2 and err to ret

[Josef Bacik]

On Tue, Mar 19, 2024 at 08:25:37PM +0530, Anand Jain wrote:
> Fix the code style for the return variable. First, rename ret to ret2,
> compile it, and then rename err to ret. This helps confirm that there are
> no instances of the old ret not renamed to ret2.
> 
> Also, there is an opportunity to drop the initialization of ret to 0,
> with the first use of ret2 replaced with ret. However, due to the confusing
> git-diff, I refrain from doing that as of now.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/inode.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 952c92e6dfcf..d890cb5ab548 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5443,29 +5443,29 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
>  	struct btrfs_root_ref *ref;
>  	struct extent_buffer *leaf;
>  	struct btrfs_key key;
> -	int ret;
> -	int err = 0;
> +	int ret2;
> +	int ret = 0;
>  	struct fscrypt_name fname;
>  
> -	ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 0, &fname);
> -	if (ret)
> -		return ret;
> +	ret2 = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 0, &fname);
> +	if (ret2)
> +		return ret2;
>  
>  	path = btrfs_alloc_path();
>  	if (!path) {
> -		err = -ENOMEM;
> +		ret = -ENOMEM;
>  		goto out;
>  	}
>  
> -	err = -ENOENT;
> +	ret = -ENOENT;
>  	key.objectid = dir->root->root_key.objectid;
>  	key.type = BTRFS_ROOT_REF_KEY;
>  	key.offset = location->objectid;
>  
> -	ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
> -	if (ret) {
> -		if (ret < 0)
> -			err = ret;
> +	ret2 = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
> +	if (ret2) {
> +		if (ret2 < 0)
> +			ret = ret2;
>  		goto out;
>  	}
>  

This is another place where we can simply remove the ret2, just do

ret = btrfs_search_slot();
if (ret) {
	if (ret > 0)
		ret = 0;
	goto out;
}

> @@ -5475,16 +5475,16 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
>  	    btrfs_root_ref_name_len(leaf, ref) != fname.disk_name.len)
>  		goto out;
>  
> -	ret = memcmp_extent_buffer(leaf, fname.disk_name.name,
> +	ret2 = memcmp_extent_buffer(leaf, fname.disk_name.name,
>  				   (unsigned long)(ref + 1), fname.disk_name.len);
> -	if (ret)
> +	if (ret2)
>  		goto out;

And here simply do

if (ret) {
	ret = 0;
	goto out;
}

Thanks,

Josef

Re: [PATCH 29/29] btrfs: fixup_tree_root_location rename ret to ret2 and err to ret

[Anand Jain]

On 3/20/24 02:24, Josef Bacik wrote:
> On Tue, Mar 19, 2024 at 08:25:37PM +0530, Anand Jain wrote:
>> Fix the code style for the return variable. First, rename ret to ret2,
>> compile it, and then rename err to ret. This helps confirm that there are
>> no instances of the old ret not renamed to ret2.
>>
>> Also, there is an opportunity to drop the initialization of ret to 0,
>> with the first use of ret2 replaced with ret. However, due to the confusing
>> git-diff, I refrain from doing that as of now.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/inode.c | 32 ++++++++++++++++----------------
>>   1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 952c92e6dfcf..d890cb5ab548 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -5443,29 +5443,29 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
>>   	struct btrfs_root_ref *ref;
>>   	struct extent_buffer *leaf;
>>   	struct btrfs_key key;
>> -	int ret;
>> -	int err = 0;
>> +	int ret2;
>> +	int ret = 0;
>>   	struct fscrypt_name fname;
>>   
>> -	ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 0, &fname);
>> -	if (ret)
>> -		return ret;
>> +	ret2 = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 0, &fname);
>> +	if (ret2)
>> +		return ret2;
>>   
>>   	path = btrfs_alloc_path();
>>   	if (!path) {
>> -		err = -ENOMEM;
>> +		ret = -ENOMEM;
>>   		goto out;
>>   	}
>>   
>> -	err = -ENOENT;
>> +	ret = -ENOENT;
>>   	key.objectid = dir->root->root_key.objectid;
>>   	key.type = BTRFS_ROOT_REF_KEY;
>>   	key.offset = location->objectid;
>>   
>> -	ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
>> -	if (ret) {
>> -		if (ret < 0)
>> -			err = ret;
>> +	ret2 = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
>> +	if (ret2) {
>> +		if (ret2 < 0)
>> +			ret = ret2;
>>   		goto out;
>>   	}
>>   
> 
> This is another place where we can simply remove the ret2, just do
> 
> ret = btrfs_search_slot();
> if (ret) {
> 	if (ret > 0)
> 		ret = 0;

Original code returned -ENOENT if btrfs_search_slot() return is > 0.
I will retain it (instead of 0 as you suggested), I think it is correct.

Thanks, Anand

> 	goto out;
> }
> 
>> @@ -5475,16 +5475,16 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
>>   	    btrfs_root_ref_name_len(leaf, ref) != fname.disk_name.len)
>>   		goto out;
>>   
>> -	ret = memcmp_extent_buffer(leaf, fname.disk_name.name,
>> +	ret2 = memcmp_extent_buffer(leaf, fname.disk_name.name,
>>   				   (unsigned long)(ref + 1), fname.disk_name.len);
>> -	if (ret)
>> +	if (ret2)
>>   		goto out;
> 
> And here simply do
> 
> if (ret) {
> 	ret = 0;
> 	goto out;
> }
> 
> Thanks,
> 
> Josef

Re: [PATCH 29/29] btrfs: fixup_tree_root_location rename ret to ret2 and err to ret

[Anand Jain]

On 4/16/24 18:42, Anand Jain wrote:
> On 3/20/24 02:24, Josef Bacik wrote:
>> On Tue, Mar 19, 2024 at 08:25:37PM +0530, Anand Jain wrote:
>>> Fix the code style for the return variable. First, rename ret to ret2,
>>> compile it, and then rename err to ret. This helps confirm that there 
>>> are
>>> no instances of the old ret not renamed to ret2.
>>>
>>> Also, there is an opportunity to drop the initialization of ret to 0,
>>> with the first use of ret2 replaced with ret. However, due to the 
>>> confusing
>>> git-diff, I refrain from doing that as of now.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/inode.c | 32 ++++++++++++++++----------------
>>>   1 file changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 952c92e6dfcf..d890cb5ab548 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -5443,29 +5443,29 @@ static int fixup_tree_root_location(struct 
>>> btrfs_fs_info *fs_info,
>>>       struct btrfs_root_ref *ref;
>>>       struct extent_buffer *leaf;
>>>       struct btrfs_key key;
>>> -    int ret;
>>> -    int err = 0;
>>> +    int ret2;
>>> +    int ret = 0;
>>>       struct fscrypt_name fname;
>>> -    ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 
>>> 0, &fname);
>>> -    if (ret)
>>> -        return ret;
>>> +    ret2 = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 
>>> 0, &fname);
>>> +    if (ret2)
>>> +        return ret2;
>>>       path = btrfs_alloc_path();
>>>       if (!path) {
>>> -        err = -ENOMEM;
>>> +        ret = -ENOMEM;
>>>           goto out;
>>>       }
>>> -    err = -ENOENT;
>>> +    ret = -ENOENT;
>>>       key.objectid = dir->root->root_key.objectid;
>>>       key.type = BTRFS_ROOT_REF_KEY;
>>>       key.offset = location->objectid;
>>> -    ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 
>>> 0);
>>> -    if (ret) {
>>> -        if (ret < 0)
>>> -            err = ret;
>>> +    ret2 = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 
>>> 0, 0);
>>> +    if (ret2) {
>>> +        if (ret2 < 0)
>>> +            ret = ret2;
>>>           goto out;
>>>       }
>>
>> This is another place where we can simply remove the ret2, just do
>>
>> ret = btrfs_search_slot();
>> if (ret) {
>>     if (ret > 0)
>>         ret = 0;
> 
> Original code returned -ENOENT if btrfs_search_slot() return is > 0.
> I will retain it (instead of 0 as you suggested), I think it is correct.
> 
> Thanks, Anand
> 
>>     goto out;
>> }
>>
>>> @@ -5475,16 +5475,16 @@ static int fixup_tree_root_location(struct 
>>> btrfs_fs_info *fs_info,
>>>           btrfs_root_ref_name_len(leaf, ref) != fname.disk_name.len)
>>>           goto out;
>>> -    ret = memcmp_extent_buffer(leaf, fname.disk_name.name,
>>> +    ret2 = memcmp_extent_buffer(leaf, fname.disk_name.name,
>>>                      (unsigned long)(ref + 1), fname.disk_name.len);
>>> -    if (ret)
>>> +    if (ret2)
>>>           goto out;
>>
>> And here simply do
>>
>> if (ret) {
>>     ret = 0;
>>     goto out;
>> }
>>

And here as well, if the name doesn't match, we returned -ENOENT
in the original code.

Thanks, Anand


>> Thanks,
>>
>> Josef
> 

Re: [PATCH 00/29] trivial adjustments for return variable coding style

[David Sterba]

On Tue, Mar 19, 2024 at 08:25:08PM +0530, Anand Jain wrote:
> Rename variable 'err'; instead, use 'ret', and the 'ret' helper variable
> is renamed to 'ret2'.
> 
> https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#code
> 
> In functions where 'ret' is already used as a return helper (but not the
> actual return), to avoid oversight, first rename the original 'ret'
> variable to 'ret2', compile it, and then rename 'err' to 'ret'.
> 
> Anand Jain (29):
>   btrfs: btrfs_cleanup_fs_roots rename ret to ret2 and err to ret
>   btrfs: btrfs_initxattrs rename err to ret
>   btrfs: send_extent_data rename err to ret
>   btrfs: btrfs_rmdir rename err to ret
>   btrfs: btrfs_cont_expand rename err to ret
>   btrfs: btrfs_setsize rename err to ret2
>   btrfs: btrfs_find_orphan_roots rename ret to ret2 and err to ret
>   btrfs: btrfs_ioctl_snap_destroy rename err to ret
>   btrfs: __set_extent_bit rename err to ret
>   btrfs: convert_extent_bit rename err to ret
>   btrfs: __btrfs_end_transaction rename err to ret
>   btrfs: btrfs_write_marked_extents rename werr to ret err to ret2
>   btrfs: __btrfs_wait_marked_extents rename werr to ret err to ret2
>   btrfs: build_backref_tree rename err to ret and ret to ret2
>   btrfs: relocate_tree_blocks rename ret to ret2 and err to ret
>   btrfs: relocate_block_group rename ret to ret2 and err to ret
>   btrfs: create_reloc_inode rename err to ret
>   btrfs: btrfs_relocate_block_group rename ret to ret2 and err ro ret
>   btrfs: mark_garbage_root rename err to ret2
>   btrfs: btrfs_recover_relocation rename ret to ret2 and err to ret
>   btrfs: quick_update_accounting rename err to ret2
>   btrfs: btrfs_qgroup_rescan_worker rename ret to ret2 and err to ret
>   btrfs: lookup_extent_data_ref rename ret to ret2 and err to ret
>   btrfs: btrfs_drop_snapshot rename ret to ret2 and err to ret
>   btrfs: btrfs_drop_subtree rename retw to ret2
>   btrfs: btrfs_dirty_pages rename variable err to ret
>   btrfs: prepare_pages rename err to ret
>   btrfs: btrfs_direct_write rename err to ret
>   btrfs: fixup_tree_root_location rename ret to ret2 and err to ret

Several patches got coments that would need an update but I think for
the simple err -> ret renames that did not involve anything else you can
add it to for-next. If you're not sure about some case then send it in
v2.

Re: [PATCH 00/29] trivial adjustments for return variable coding style

[Anand Jain]

On 3/22/24 08:02, David Sterba wrote:
> On Tue, Mar 19, 2024 at 08:25:08PM +0530, Anand Jain wrote:
>> Rename variable 'err'; instead, use 'ret', and the 'ret' helper variable
>> is renamed to 'ret2'.
>>
>> https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#code
>>
>> In functions where 'ret' is already used as a return helper (but not the
>> actual return), to avoid oversight, first rename the original 'ret'
>> variable to 'ret2', compile it, and then rename 'err' to 'ret'.
>>
>> Anand Jain (29):
>>    btrfs: btrfs_cleanup_fs_roots rename ret to ret2 and err to ret
>>    btrfs: btrfs_initxattrs rename err to ret
>>    btrfs: send_extent_data rename err to ret
>>    btrfs: btrfs_rmdir rename err to ret
>>    btrfs: btrfs_cont_expand rename err to ret
>>    btrfs: btrfs_setsize rename err to ret2
>>    btrfs: btrfs_find_orphan_roots rename ret to ret2 and err to ret
>>    btrfs: btrfs_ioctl_snap_destroy rename err to ret
>>    btrfs: __set_extent_bit rename err to ret
>>    btrfs: convert_extent_bit rename err to ret
>>    btrfs: __btrfs_end_transaction rename err to ret
>>    btrfs: btrfs_write_marked_extents rename werr to ret err to ret2
>>    btrfs: __btrfs_wait_marked_extents rename werr to ret err to ret2
>>    btrfs: build_backref_tree rename err to ret and ret to ret2
>>    btrfs: relocate_tree_blocks rename ret to ret2 and err to ret
>>    btrfs: relocate_block_group rename ret to ret2 and err to ret
>>    btrfs: create_reloc_inode rename err to ret
>>    btrfs: btrfs_relocate_block_group rename ret to ret2 and err ro ret
>>    btrfs: mark_garbage_root rename err to ret2
>>    btrfs: btrfs_recover_relocation rename ret to ret2 and err to ret
>>    btrfs: quick_update_accounting rename err to ret2
>>    btrfs: btrfs_qgroup_rescan_worker rename ret to ret2 and err to ret
>>    btrfs: lookup_extent_data_ref rename ret to ret2 and err to ret
>>    btrfs: btrfs_drop_snapshot rename ret to ret2 and err to ret
>>    btrfs: btrfs_drop_subtree rename retw to ret2
>>    btrfs: btrfs_dirty_pages rename variable err to ret
>>    btrfs: prepare_pages rename err to ret
>>    btrfs: btrfs_direct_write rename err to ret
>>    btrfs: fixup_tree_root_location rename ret to ret2 and err to ret
> 
> Several patches got coments that would need an update but I think for
> the simple err -> ret renames that did not involve anything else you can
> add it to for-next.

Yeah, to better manage the patches, I have just pushed the patch
which are direct err->ret renames and does not involve ret2 and
have comments/suggestions for better ideas.

I'll be sending v2 for the remaining patches.

Thanks, Anand

> If you're not sure about some case then send it in
> v2.