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

Qu Wenruo
2024-04-17T03:59:48Z

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