Re: [PATCH 04/10] btrfs-progs: reform the function block_group_cache_tree_search()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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



On 2019/12/5 下午12:29, damenly.su@gmail.com wrote:
> From: Su Yue <Damenly_Su@gmx.com>
> 
> Add the new value 2 of @contains in block_group_cache_tree_search().
> The new values means the function will return the block group that
> contains bytenr, otherwise return the next one that starts after
> @bytenr. Will be used in later commit.
> 
> Signed-off-by: Su Yue <Damenly_Su@gmx.com>
> ---
>  extent-tree.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/extent-tree.c b/extent-tree.c
> index ab576f8732a2..1d8535049eaf 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -196,13 +196,16 @@ static int btrfs_add_block_group_cache(struct btrfs_fs_info *info,
>  }
>  
>  /*
> - * This will return the block group at or after bytenr if contains is 0, else
> - * it will return the block group that contains the bytenr
> + * @contains:
> + *   if 0, return the block group at or after bytenr if contains is 0.
> + *   if 1, return the block group that contains the bytenr.
> + *   if 2, return the block group that contains bytenr, otherwise return the
> + *     next one that starts after @bytenr.

Thats a creative solution, good job on that.

However since contains is no longer just simple 1 or 0, it's better to
enum to define the behavior, other than using the immediate numbers.

>   */
>  static struct btrfs_block_group_cache *block_group_cache_tree_search(
>  		struct btrfs_fs_info *info, u64 bytenr, int contains)
>  {
> -	struct btrfs_block_group_cache *cache, *ret = NULL;
> +	struct btrfs_block_group_cache *cache, *ret = NULL, *tmp = NULL;
>  	struct rb_node *n;
>  	u64 end, start;
>  
> @@ -215,8 +218,8 @@ static struct btrfs_block_group_cache *block_group_cache_tree_search(
>  		start = cache->key.objectid;
>  
>  		if (bytenr < start) {
> -			if (!contains && (!ret || start < ret->key.objectid))
> -				ret = cache;
> +			if (!tmp || start < tmp->key.objectid)
> +				tmp = cache;

This doesn't look correct.

I was expecting something based on last found node, other than doing
something strange in the rb-tree iteration code.

At least this breaks readability. It would be much better to handle this
after the rb tree while loop.

Thanks,
Qu
>  			n = n->rb_left;
>  		} else if (bytenr > start) {
>  			if (contains && bytenr <= end) {
> @@ -229,6 +232,13 @@ static struct btrfs_block_group_cache *block_group_cache_tree_search(
>  			break;
>  		}
>  	}
> +
> +	/*
> +	 * If ret is NULL, means not found any block group cotanins @bytenr.
> +	 * So just except the case that cotanins equals 1.
> +	 */
> +	if (!ret && contains != 1)
> +		ret = tmp;
>  	return ret;
>  }
>  
> 


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