Re: [PATCH v2 2/7] btrfs-progs: Refactor btrfs_read_block_groups()

Anand Jain
2019-10-17T05:10:09Z

On 10/17/19 12:33 PM, Qu Wenruo wrote:
>
>
> On 2019/10/17 上午11:23, Anand Jain wrote:
>> On 10/8/19 12:49 PM, Qu Wenruo wrote:
>>> This patch does the following refactor:
>>> - Refactor parameter from @root to @fs_info
>>>
>>> - Refactor the large loop body into another function
>>>    Now we have a helper function, read_one_block_group(), to handle
>>>    block group cache and space info related routine.
>>>
>>> - Refactor the return value
>>>    Even we have the code handling ret > 0 from find_first_block_group(),
>>>    it never works, as when there is no more block group,
>>>    find_first_block_group() just return -ENOENT other than 1.
>>
>>
>>  Can it be separated into patches? My concern is as it alters the return
>>  value of the rescue command. So we shall have clarity of a discrete
>>  patch to blame. Otherwise I agree its a good change.
>
> No problem.
>
> What about 3 patches split by the mentioned 3 refactors?
>>
>>
>>>    This is super confusing, it's almost a mircle it even works.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   ctree.h       |   2 +-
>>>   disk-io.c     |   9 ++-
>>>   extent-tree.c | 160 ++++++++++++++++++++++++++++----------------------
>>>   3 files changed, 97 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/ctree.h b/ctree.h
>>> index 8c7b3cb40151..2899de358613 100644
>>> --- a/ctree.h
>>> +++ b/ctree.h
>>> @@ -2550,7 +2550,7 @@ int update_space_info(struct btrfs_fs_info
>>> *info, u64 flags,
>>>                 u64 total_bytes, u64 bytes_used,
>>>                 struct btrfs_space_info **space_info);
>>>   int btrfs_free_block_groups(struct btrfs_fs_info *info);
>>> -int btrfs_read_block_groups(struct btrfs_root *root);
>>> +int btrfs_read_block_groups(struct btrfs_fs_info *info);
>>>   struct btrfs_block_group_cache *
>>>   btrfs_add_block_group(struct btrfs_fs_info *fs_info, u64 bytes_used,
>>> u64 type,
>>>                 u64 chunk_offset, u64 size);
>>> diff --git a/disk-io.c b/disk-io.c
>>> index be44eead5cef..8978f0cb60c7 100644
>>> --- a/disk-io.c
>>> +++ b/disk-io.c
>>> @@ -983,14 +983,17 @@ int btrfs_setup_all_roots(struct btrfs_fs_info
>>> *fs_info, u64 root_tree_bytenr,
>>>       fs_info->last_trans_committed = generation;
>>>       if (extent_buffer_uptodate(fs_info->extent_root->node) &&
>>>           !(flags & OPEN_CTREE_NO_BLOCK_GROUPS)) {
>>> -        ret = btrfs_read_block_groups(fs_info->tree_root);
>>> +        ret = btrfs_read_block_groups(fs_info);
>>>           /*
>>>            * If we don't find any blockgroups (ENOENT) we're either
>>>            * restoring or creating the filesystem, where it's expected,
>>>            * anything else is error
>>>            */
>>> -        if (ret != -ENOENT)
>>> -            return -EIO;
>>> +        if (ret < 0 && ret != -ENOENT) {
>>> +            errno = -ret;
>>> +            error("failed to read block groups: %m");
>>> +            return ret;
>>> +        }
>>>       }
>>
>>
>> As mentioned this alters the rescue command semantics as show below.
>> Earlier we had only -EIO as error now its much more and accurate
>> which is good. fstests is fine but anything else?
>>
>> cmd_rescue_chunk_recover()
>>   btrfs_recover_chunk_tree()
>>     open_ctree_with_broken_chunk()
>>       btrfs_setup_all_roots()
>
> I'm not sure if I got the point.
>
> Although btrfs_setup_all_roots() get called in above call chain, it
> doesn't have any special handling of -EIO or others.
>
> It just reads the extent tree root.
>
> Would you mind to explain a little more?

sure.
The above thread is in the call chain of the command
btrfs rescue chunk-recover [options] <device>"

And as the its return error code is being changed for the same problem,
so a separate patch not part of the bg-tree changes would make sense.

Thanks, Anand