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

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: damenly.su@gmail.com, linux-btrfs@vger.kernel.org
Cc: Su Yue <Damenly_Su@gmx.com>
Subject: Re: [PATCH 09/10] btrfs-progs: refrom block groups caches structure
Date: Thu, 5 Dec 2019 15:51:53 +0800	[thread overview]
Message-ID: <ea48239e-3bba-beb7-8960-e847f70b4a6f@gmx.com> (raw)
In-Reply-To: <20191205042921.25316-10-Damenly_Su@gmx.com>

[-- Attachment #1.1: Type: text/plain, Size: 2110 bytes --]

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.

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

Thanks,
Qu

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]