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

[Su Yue <Damenly_Su@gmx.com>]

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
>