Re: [PATCH v2 4/7] btrfs-progs: lowmem: search key of root again after check_fs_root() under repair

[Nikolay Borisov <nborisov@suse.com>]

On 14.09.2018 03:58, Su Yue wrote:
> 
> 
> On 09/14/2018 07:37 AM, Qu Wenruo wrote:
>>
>>
>> On 2018/9/13 上午4:49, damenly.su@gmail.com wrote:
>>> From: Su Yue <suy.fnst@cn.fujitsu.com>
>>>
>>> In check_fs_roots_lowmem(), we do search and follow the resulted path
>>> to call check_fs_root(), then call btrfs_next_item() to check next
>>> root.
>>> However, if repair is enabled, the root tree can be cowed, the
>>> existed path can cause strange errors.
>>>
>>> Solution:
>>>    If repair, save the key before calling check_fs_root,
>>>    search the saved key again before checking next root.
>>
>> Both reason and solution looks good.
>>
>>>
>>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
>>> ---
>>>   check/mode-lowmem.c | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
>>> index 89a304bbdd69..8fc9edab1d66 100644
>>> --- a/check/mode-lowmem.c
>>> +++ b/check/mode-lowmem.c
>>> @@ -4967,9 +4967,13 @@ int check_fs_roots_lowmem(struct btrfs_fs_info
>>> *fs_info)
>>>       }
>>>         while (1) {
>>> +        struct btrfs_key saved_key;
>>> +
>>>           node = path.nodes[0];
>>>           slot = path.slots[0];
>>>           btrfs_item_key_to_cpu(node, &key, slot);
>>> +        if (repair)
>>> +            saved_key = key;
>>>           if (key.objectid > BTRFS_LAST_FREE_OBJECTID)
>>>               goto out;
>>>           if (key.type == BTRFS_ROOT_ITEM_KEY &&
>>> @@ -5000,6 +5004,17 @@ int check_fs_roots_lowmem(struct btrfs_fs_info
>>> *fs_info)
>>>               err |= ret;
>>>           }
>>>   next:
>>> +        /*
>>> +         * Since root tree can be cowed during repair,
>>> +         * here search the saved key again.
>>> +         */
>>> +        if (repair) {
>>> +            btrfs_release_path(&path);
>>> +            ret = btrfs_search_slot(NULL, fs_info->tree_root,
>>> +                        &saved_key, &path, 0, 0);
>>> +            /* Repair never deletes trees, search must succeed. */
>>> +            BUG_ON(ret);
>>
>> But this doesn't look good to me.
>>
>> Your assumption here is valid (at least for now), but it's possible that
>> some tree blocks get corrupted in a large root tree, and in that case,
>> we could still read part of the root tree, but btrfs_search_slot() could
>> still return -EIO for certain search key.
>>
>> So I still prefer to do some error handling other than BUG_ON(ret).
>>
> Okay, will try it.

Just to emphasize Qu's point - we should strive to remove existing
BUG_ON and should never introduce new ones. btrfs-progs is already quite
messy and we should be improving that.

> 
> Thanks,
> Su
>> Thanks,
>> Qu
>>
>>> +        }
>>>           ret = btrfs_next_item(tree_root, &path);
>>>           if (ret > 0)
>>>               goto out;
>>>
>>
>>
> 
> 
>