Re: [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray

[Nikolay Borisov <nborisov@suse.com>]

On 16.05.22 г. 14:02 ч., Dan Carpenter wrote:
> On Mon, May 16, 2022 at 01:39:52PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 16.05.22 г. 11:04 ч., Dan Carpenter wrote:
>>> Hello Gabriel Niebler,
>>>
>>> The patch 06a79e50ff00: "btrfs: turn fs_roots_radix in btrfs_fs_info
>>> into an XArray" from May 3, 2022, leads to the following Smatch
>>> static checker warning:
>>>
>>> 	fs/btrfs/disk-io.c:4560 btrfs_cleanup_fs_roots()
>>> 	warn: ignoring unreachable code.
>>>
>>> fs/btrfs/disk-io.c
>>>       4520 int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>>>       4521 {
>>>       4522         struct btrfs_root *roots[8];
>>>       4523         unsigned long index = 0;
>>>       4524         int i;
>>>       4525         int err = 0;
>>>       4526         int grabbed;
>>>       4527
>>>       4528         while (1) {
>>>       4529                 struct btrfs_root *root;
>>>       4530
>>>       4531                 spin_lock(&fs_info->fs_roots_lock);
>>>       4532                 if (!xa_find(&fs_info->fs_roots, &index, ULONG_MAX, XA_PRESENT)) {
>>>       4533                         spin_unlock(&fs_info->fs_roots_lock);
>>>       4534                         return err;
>>>       4535                 }
>>>       4536
>>>       4537                 grabbed = 0;
>>>       4538                 xa_for_each_start(&fs_info->fs_roots, index, root, index) {
>>>       4539                         /* Avoid grabbing roots in dead_roots */
>>>       4540                         if (btrfs_root_refs(&root->root_item) > 0)
>>>       4541                                 roots[grabbed++] = btrfs_grab_root(root);
>>>       4542                         if (grabbed >= ARRAY_SIZE(roots))
>>>       4543                                 break;
> 
> breaks out of xa_for_each_start() loop.

This is fine, as when we fill root[] then we'd process it in the 
subsequent for loop.

> 
>>>       4544                 }
>>>       4545                 spin_unlock(&fs_info->fs_roots_lock);
>>>       4546
>>>       4547                 for (i = 0; i < grabbed; i++) {
>>>       4548                         if (!roots[i])
>>>       4549                                 continue;
>>>       4550                         index = roots[i]->root_key.objectid;
>>>       4551                         err = btrfs_orphan_cleanup(roots[i]);
>>>       4552                         if (err)
>>>       4553                                 break;
> 
> breaks out of for loop.

However, this is indeed a genuine bug, and the fix for it is:

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index fe309db9f5ff..f33093513360 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4549,12 +4549,13 @@ int btrfs_cleanup_fs_roots(struct btrfs_fs_info 
*fs_info)
                         index = roots[i]->root_key.objectid;
                         err = btrfs_orphan_cleanup(roots[i]);
                         if (err)
-                               break;
+                               goto out;
                         btrfs_put_root(roots[i]);
                 }
                 index++;
         }

+out:
         /* Release the roots that remain uncleaned due to error */
         for (; i < grabbed; i++) {
                 if (roots[i])


> 
>>>       4554                         btrfs_put_root(roots[i]);
>>>       4555                 }
>>>       4556                 index++;
>>>       4557         }
>>>       4558
>>>       4559         /* Release the roots that remain uncleaned due to error */
>>> --> 4560         for (; i < grabbed; i++) {
>>>
>>> This code is unreachable now.
>>
>> How is it unreachable, if we error in the middle of btrfs_orphan_cleanup,
>> we'd break and this loop will cleanup the rest of the roots,
> 
> There are breaks inside the inner loops but nothing breaks out of the
> while (1) loop.
> 
> regards,
> dan carpenter
>