[04/10] btrfs-progs: reform the function block_group_cache_tree_search()
diff mbox series

Message ID 20191205042921.25316-5-Damenly_Su@gmx.com
State New
Headers show
Series
  • unify origanization structure of block group cache
Related show

Commit Message

Damenly Su Dec. 5, 2019, 4:29 a.m. UTC
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(-)

Comments

Qu Wenruo Dec. 5, 2019, 7:38 a.m. UTC | #1
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;
>  }
>  
>
Su Yue Dec. 5, 2019, 8:08 a.m. UTC | #2
On 2019/12/5 3:38 PM, Qu Wenruo wrote:
>
>
> 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.

Nice suggestion, will do.
>
>>    */
>>   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.
> Spent much brain power on this trick, this line means we always keep the
next block block to @bytenr. The original code stores the block group
cache in the @ret, which here I do is to store it into @tmp.

This method doesn't change efficiency only little memory copies.
If put the logic after the whole loop, I'm afraid it will require more
code lines and lower the search efficiency.

Thanks
> 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;
>>   }
>>
>>
>

Patch
diff mbox series

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.
  */
 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;
 			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;
 }