Re: [PATCH 09/10] btrfs-progs: refrom block groups caches structure

Su Yue
2019-12-05T08:09:48Z

On 2019/12/5 3:51 PM, Qu Wenruo wrote:
>
>
> On 2019/12/5 下午12:29, damenly.su@gmail.com wrote:
>> From: Su Yue <Damenly_Su@gmx.com>
>>
>> This commit organises block groups cache in
>> btrfs_fs_info::block_group_cache_tree. And any dirty block groups are
>> linked in transaction_handle::dirty_bgs.
>>
>> To keep coherence of bisect, it does almost replace in place:
>> 1. Replace the old btrfs group lookup functions with new functions
>> introduced in former commits.
>> 2. set_extent_bits(..., BLOCK_GROUP_DIRYT) things are replaced by linking
>> the block group cache into trans::dirty_bgs. Checking and clearing bits
>> are transformed too.
>> 3. set_extent_bits(..., bit | EXTENT_LOCKED) things are replaced by
>> new the btrfs_add_block_group_cache() which inserts caches into
>> btrfs_fs_info::block_group_cache_tree directly. Other operations are
>> converted to tree operations.
>
> Great cleanup and code unification.
>
> Overall looks good, just small nitpicks inlined below.
>>
>> Signed-off-by: Su Yue <Damenly_Su@gmx.com>
>> ---
>> cmds/rescue-chunk-recover.c | 4 +-
>> extent-tree.c | 211 ++++++------------------------------
>> image/main.c | 5 +-
>> transaction.c | 3 +-
>> 4 files changed, 38 insertions(+), 185 deletions(-)
>>
>> diff --git a/cmds/rescue-chunk-recover.c b/cmds/rescue-chunk-recover.c
>> index 461b66c6e13b..a13acc015d11 100644
>
>> @@ -2699,25 +2571,22 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info)
>> struct btrfs_block_group_cache *cache;
>> u64 start;
>> u64 end;
>> - u64 ptr;
>> int ret;
>>
>> - while(1) {
>> - ret = find_first_extent_bit(&info->block_group_cache, 0,
>> - &start, &end, (unsigned int)-1);
>> - if (ret)
>> + while (rb_first(&info->block_group_cache_tree)) {
>> + cache = btrfs_lookup_first_block_group(info, 0);
>> + if (!cache)
>
> Since we're freeing all block groups, what about
> rbtree_postorder_for_each_entry_safe()?
>
> That would be faster than rb_first() as we don't need to balance the tree.
>
Oh! Thanks a lot. Will use the one.

> Despite that, the patch looks great to me.
> Especially for that -185 part.
>
> Thanks,
> Qu
>