Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[Nikolay Borisov <nborisov@suse.com>]

On 21.08.20 г. 16:59 ч., Josef Bacik wrote:
> On 8/21/20 3:31 AM, Nikolay Borisov wrote:
>>
>>
>> On 20.08.20 г. 23:00 ч., Josef Bacik wrote:
>>> While testing a weird problem with -o degraded, I noticed I was getting
>>> leaked root errors
>>>
>>> BTRFS warning (device loop0): writable mount is not allowed due to
>>> too many missing devices
>>> BTRFS error (device loop0): open_ctree failed
>>> BTRFS error (device loop0): leaked root -9-0 refcount 1
>>>
>>> This is the DATA_RELOC root, which gets read before the other fs roots,
>>> but is included in the fs roots radix tree.  Handle this by adding a
>>> btrfs_drop_and_free_fs_root() on the data reloc root if it exists.  This
>>> is ok to do here if we fail further up because we will only drop the ref
>>> if we delete the root from the radix tree, and all other cleanup won't
>>> be duplicated.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>>   fs/btrfs/disk-io.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index 814f8de395fe..ac6d6fddd5f4 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -3418,6 +3418,8 @@ int __cold open_ctree(struct super_block *sb,
>>> struct btrfs_fs_devices *fs_device
>>>       btrfs_put_block_group_cache(fs_info);
>>>     fail_tree_roots:
>>> +    if (fs_info->data_reloc_root)
>>> +        btrfs_drop_and_free_fs_root(fs_info, fs_info->data_reloc_root);
>>
>> But will this really free the root? So the newly allocated
>> data_reloc_root has it's ref set to 1 from
>> btrfs_get_root_ref->btrfs_read_tree_root->btrfs_alloc_root and to 2 from
>> being added to the radix tree in btrfs_insert_fs_root().
>>
>> But btrfs_drop_and_free_fs_root makes a single call to btrfs_put_root.
>> So won't the reloc tree be left with a refcount of 1 ?
> 
> It's a global root, so it's final put happens in btrfs_free_fs_info(),
> we just need to drop the radix tree ref here.  Thanks,


Fair enough, but this really shows that btrfs_drop_and_free_fs_root has
a horrible name which doesn't reflect what it does fully...

Any case the patch itself is good, so :

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> 
> Josef
>