Re: [PATCH] btrfs: Turn fs_roots_radix in btrfs_fs_info into an XArray

[Gabriel Niebler <gniebler@suse.com>]

I had another look at this and have to correct myself:

Am 02.05.22 um 10:59 schrieb Gabriel Niebler:
> Am 28.04.22 um 13:59 schrieb Nikolay Borisov:
>> On 27.04.22 г. 0:45 ч., Gabriel Niebler wrote:
>>> … rename it to simply fs_roots and adjust all usages of this object 
>>> to use
>>> the XArray API, because it is notionally easier to use and 
>>> unserstand, as
>>> it provides array semantics, and also takes care of locking for us,
>>> further simplifying the code.
>>>
>>> Also do some refactoring, esp. where the API change requires largely
>>> rewriting some functions, anyway.
>>>
>>> Signed-off-by: Gabriel Niebler <gniebler@suse.com>
>>> ---
>>>   fs/btrfs/ctree.h       |   5 +-
>>>   fs/btrfs/disk-io.c     | 176 ++++++++++++++++++++---------------------
>>>   fs/btrfs/inode.c       |  13 +--
>>>   fs/btrfs/transaction.c |  67 +++++++---------
>>>   4 files changed, 126 insertions(+), 135 deletions(-)

<snip>

 >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
 >>> index 126f244cdf88..a8577f659f66 100644
 >>> --- a/fs/btrfs/disk-io.c
 >>> +++ b/fs/btrfs/disk-io.c

<snip>

>>> @@ -4512,50 +4501,54 @@ void btrfs_drop_and_free_fs_root(struct 
>>> btrfs_fs_info *fs_info,
>>>   int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>>>   {
>>> -    u64 root_objectid = 0;
>>> -    struct btrfs_root *gang[8];
>>> -    int i = 0;
>>> +    struct btrfs_root *roots[8];
>>> +    unsigned long index = 0;
>>> +    int i;
>> nit: This can be defined into the 2 loops that use the variable.
> 
> Sure, why not.

I was wrong here, actually. The second for-loop that uses 'i' does not 
initialise it to zero, but deliberately starts with i at the last value 
it had when the first for-loop that uses it was last exited, because 
that one may have been broken out of, due to some cleanup error, and 
thus there may be unreleased roots still in the array.

This is how the code used to work before and I just kept that logic.

But then it's obviously important to declare 'i' outside of both loops 
so it can keep the value between them.

>>>       int err = 0;
>>> -    unsigned int ret = 0;
>>> +    int grabbed;
>>>       while (1) {
>>> -        spin_lock(&fs_info->fs_roots_radix_lock);
>>> -        ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>>> -                         (void **)gang, root_objectid,
>>> -                         ARRAY_SIZE(gang));
>>> -        if (!ret) {
>>> -            spin_unlock(&fs_info->fs_roots_radix_lock);
>>> +        struct btrfs_root *root;
>>> +
>>> +        spin_lock(&fs_info->fs_roots_lock);
>>> +        if (!xa_find(&fs_info->fs_roots, &index,
>>> +                 ULONG_MAX, XA_PRESENT)) {
>>> +            spin_unlock(&fs_info->fs_roots_lock);
>>>               break;
>>>           }
>>> -        root_objectid = gang[ret - 1]->root_key.objectid + 1;
>>> -        for (i = 0; i < ret; i++) {
>>> -            /* Avoid to grab roots in dead_roots */
>>> -            if (btrfs_root_refs(&gang[i]->root_item) == 0) {
>>> -                gang[i] = NULL;
>>> -                continue;
>>> +        grabbed = 0;
>>> +        xa_for_each_start(&fs_info->fs_roots, index, root,
>>> +                  index) {
>>> +            /* Avoid grabbing roots in dead_roots */
>>> +            if (btrfs_root_refs(&root->root_item) == 0) {
>>> +                roots[grabbed] = NULL;
>>> +            } else {
>>> +                /* Grab all the search results for later use */
>>> +                roots[grabbed] = btrfs_grab_root(root);
>>>               }
>>> -            /* grab all the search result for later use */
>>> -            gang[i] = btrfs_grab_root(gang[i]);
>>> +            grabbed++;
>>> +            if (grabbed >= ARRAY_SIZE(roots))
>>> +                break;
>>>           }
>>> -        spin_unlock(&fs_info->fs_roots_radix_lock);
>>> +        spin_unlock(&fs_info->fs_roots_lock);
>>> -        for (i = 0; i < ret; i++) {
>>> -            if (!gang[i])
>>> +        for (i = 0; i < grabbed; i++) {
>>> +            if (!roots[i])
>>>                   continue;
>>> -            root_objectid = gang[i]->root_key.objectid;
>>> -            err = btrfs_orphan_cleanup(gang[i]);
>>> +            index = roots[i]->root_key.objectid;
>>> +            err = btrfs_orphan_cleanup(roots[i]);
>>>               if (err)
>>>                   break;
>>> -            btrfs_put_root(gang[i]);
>>> +            btrfs_put_root(roots[i]);
>>>           }
>>> -        root_objectid++;
>>> +        index++;
>>>       }
>>> -    /* release the uncleaned roots due to error */
>>> -    for (; i < ret; i++) {
>>> -        if (gang[i])
>>> -            btrfs_put_root(gang[i]);
>>> +    /* Release the roots that remain uncleaned due to error */
>>> +    for (; i < grabbed; i++) {
>>> +        if (roots[i])
>>> +            btrfs_put_root(roots[i]);
>>>       }
>>>       return err;
>>>   }

<snip>

I will do all the other changes we discussed and resend, but I'll leave 
this one alone.