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

Qu Wenruo
2019-12-05T07:38:54Z

[-- 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 --]