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

[Anand Jain <anand.jain@oracle.com>]

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