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

Anand Jain
2024-04-17T03:24:17Z

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;
>>   }
>>
>>   /*