[PATCH 1/2] btrfs-progs: add comments of block group lookup functions

[PATCH 1/2] btrfs-progs: add comments of block group lookup functions

[damenly.su]

From: Su Yue <Damenly_Su@gmx.com>

The progs side function btrfs_lookup_first_block_group() calls
find_first_extent_bit() to find block group which contains bytenr
or after the bytenr. This behavior differs from kernel code, so
add the comments.

Add the coments of btrfs_lookup_block_group() too, this one works
like kernel side.

Signed-off-by: Su Yue <Damenly_Su@gmx.com>
---
 extent-tree.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/extent-tree.c b/extent-tree.c
index d67e4098351f..f690ae999f37 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -164,6 +164,9 @@ err:
 	return 0;
 }
 
+/*
+ * Return the block group that contains or after bytenr
+ */
 struct btrfs_block_group_cache *btrfs_lookup_first_block_group(struct
 						       btrfs_fs_info *info,
 						       u64 bytenr)
@@ -193,6 +196,9 @@ struct btrfs_block_group_cache *btrfs_lookup_first_block_group(struct
 	return block_group;
 }
 
+/*
+ * Return the block group that contains the given bytenr
+ */
 struct btrfs_block_group_cache *btrfs_lookup_block_group(struct
 							 btrfs_fs_info *info,
 							 u64 bytenr)
-- 
2.23.0

[PATCH 2/2] btrfs-progs: check: call btrfs_lookup_block_group() instead in check_extent_type()

[damenly.su]

From: Su Yue <Damenly_Su@gmx.com>

check_extent_type() wants to lookup the block group which contains the
extent, not the other contains or after the extent start.

Use btrfs_lookup_block_group() instead of
btrfs_lookup_first_btrfs_group().

Signed-off-by: Su Yue <Damenly_Su@gmx.com>
---
 check/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index a0e5ac47c152..d18d8e9fa80b 100644
--- a/check/main.c
+++ b/check/main.c
@@ -4530,7 +4530,7 @@ static void check_extent_type(struct extent_record *rec)
 {
 	struct btrfs_block_group_cache *bg_cache;
 
-	bg_cache = btrfs_lookup_first_block_group(global_info, rec->start);
+	bg_cache = btrfs_lookup_block_group(global_info, rec->start);
 	if (!bg_cache)
 		return;
 
-- 
2.23.0

Re: [PATCH 1/2] btrfs-progs: add comments of block group lookup functions

[Qu Wenruo]

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



On 2019/11/11 下午4:42, damenly.su@gmail.com wrote:
> From: Su Yue <Damenly_Su@gmx.com>
> 
> The progs side function btrfs_lookup_first_block_group() calls
> find_first_extent_bit() to find block group which contains bytenr
> or after the bytenr. This behavior differs from kernel code, so
> add the comments.
> 
> Add the coments of btrfs_lookup_block_group() too, this one works
> like kernel side.
> 
> Signed-off-by: Su Yue <Damenly_Su@gmx.com>
> ---
>  extent-tree.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/extent-tree.c b/extent-tree.c
> index d67e4098351f..f690ae999f37 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -164,6 +164,9 @@ err:
>  	return 0;
>  }
>  
> +/*
> + * Return the block group that contains or after bytenr
> + */

What about "Return the block group thart starts at or after @bytenr" ?

Thanks,
Qu

>  struct btrfs_block_group_cache *btrfs_lookup_first_block_group(struct
>  						       btrfs_fs_info *info,
>  						       u64 bytenr)
> @@ -193,6 +196,9 @@ struct btrfs_block_group_cache *btrfs_lookup_first_block_group(struct
>  	return block_group;
>  }
>  
> +/*
> + * Return the block group that contains the given bytenr
> + */
>  struct btrfs_block_group_cache *btrfs_lookup_block_group(struct
>  							 btrfs_fs_info *info,
>  							 u64 bytenr)
> 


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

Re: [PATCH 1/2] btrfs-progs: add comments of block group lookup functions

[Su Yue]

On 2019/11/11 5:28 PM, Qu Wenruo wrote:
>
>
> On 2019/11/11 下午4:42, damenly.su@gmail.com wrote:
>> From: Su Yue <Damenly_Su@gmx.com>
>>
>> The progs side function btrfs_lookup_first_block_group() calls
>> find_first_extent_bit() to find block group which contains bytenr
>> or after the bytenr. This behavior differs from kernel code, so
>> add the comments.
>>
>> Add the coments of btrfs_lookup_block_group() too, this one works
>> like kernel side.
>>
>> Signed-off-by: Su Yue <Damenly_Su@gmx.com>
>> ---
>>   extent-tree.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/extent-tree.c b/extent-tree.c
>> index d67e4098351f..f690ae999f37 100644
>> --- a/extent-tree.c
>> +++ b/extent-tree.c
>> @@ -164,6 +164,9 @@ err:
>>   	return 0;
>>   }
>>
>> +/*
>> + * Return the block group that contains or after bytenr
>> + */
>
> What about "Return the block group thart starts at or after @bytenr" ?
>

That's what documented in kernel already.
The thing I try to express is "contains".
For such a block group marked as B[n, m).
btrfs_lookup_first_block_group(x) (x > n && x < m). Kernel code will
return the block group next to B. However, progs side will return the
block group B.

Thanks.
> Thanks,
> Qu
>
>>   struct btrfs_block_group_cache *btrfs_lookup_first_block_group(struct
>>   						       btrfs_fs_info *info,
>>   						       u64 bytenr)
>> @@ -193,6 +196,9 @@ struct btrfs_block_group_cache *btrfs_lookup_first_block_group(struct
>>   	return block_group;
>>   }
>>
>> +/*
>> + * Return the block group that contains the given bytenr
>> + */
>>   struct btrfs_block_group_cache *btrfs_lookup_block_group(struct
>>   							 btrfs_fs_info *info,
>>   							 u64 bytenr)
>>
>

Re: [PATCH 1/2] btrfs-progs: add comments of block group lookup functions

[Qu Wenruo]

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



On 2019/11/11 下午5:49, Su Yue wrote:
> 
> 
> On 2019/11/11 5:28 PM, Qu Wenruo wrote:
>>
>>
>> On 2019/11/11 下午4:42, damenly.su@gmail.com wrote:
>>> From: Su Yue <Damenly_Su@gmx.com>
>>>
>>> The progs side function btrfs_lookup_first_block_group() calls
>>> find_first_extent_bit() to find block group which contains bytenr
>>> or after the bytenr. This behavior differs from kernel code, so
>>> add the comments.
>>>
>>> Add the coments of btrfs_lookup_block_group() too, this one works
>>> like kernel side.
>>>
>>> Signed-off-by: Su Yue <Damenly_Su@gmx.com>
>>> ---
>>>   extent-tree.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/extent-tree.c b/extent-tree.c
>>> index d67e4098351f..f690ae999f37 100644
>>> --- a/extent-tree.c
>>> +++ b/extent-tree.c
>>> @@ -164,6 +164,9 @@ err:
>>>       return 0;
>>>   }
>>>
>>> +/*
>>> + * Return the block group that contains or after bytenr
>>> + */
>>
>> What about "Return the block group thart starts at or after @bytenr" ?
>>
> 
> That's what documented in kernel already.
> The thing I try to express is "contains".
> For such a block group marked as B[n, m).
> btrfs_lookup_first_block_group(x) (x > n && x < m). Kernel code will
> return the block group next to B. However, progs side will return the
> block group B.

"Contains" indeed covers your example.
But the "after @bytenr" part looks strange to me.

Did you mean if @bytenr is not covered by any block group, then the next
block group starts after @bytenr is returned?

Then the comment is good.

Thanks,
Qu

> 
> Thanks.
>> Thanks,
>> Qu
>>
>>>   struct btrfs_block_group_cache *btrfs_lookup_first_block_group(struct
>>>                                  btrfs_fs_info *info,
>>>                                  u64 bytenr)
>>> @@ -193,6 +196,9 @@ struct btrfs_block_group_cache
>>> *btrfs_lookup_first_block_group(struct
>>>       return block_group;
>>>   }
>>>
>>> +/*
>>> + * Return the block group that contains the given bytenr
>>> + */
>>>   struct btrfs_block_group_cache *btrfs_lookup_block_group(struct
>>>                                btrfs_fs_info *info,
>>>                                u64 bytenr)
>>>
>>


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

Re: [PATCH 1/2] btrfs-progs: add comments of block group lookup functions

[Su Yue]

On 2019/11/11 6:03 PM, Qu Wenruo wrote:
>
>
> On 2019/11/11 下午5:49, Su Yue wrote:
>>
>>
>> On 2019/11/11 5:28 PM, Qu Wenruo wrote:
>>>
>>>
>>> On 2019/11/11 下午4:42, damenly.su@gmail.com wrote:
>>>> From: Su Yue <Damenly_Su@gmx.com>
>>>>
>>>> The progs side function btrfs_lookup_first_block_group() calls
>>>> find_first_extent_bit() to find block group which contains bytenr
>>>> or after the bytenr. This behavior differs from kernel code, so
>>>> add the comments.
>>>>
>>>> Add the coments of btrfs_lookup_block_group() too, this one works
>>>> like kernel side.
>>>>
>>>> Signed-off-by: Su Yue <Damenly_Su@gmx.com>
>>>> ---
>>>>    extent-tree.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/extent-tree.c b/extent-tree.c
>>>> index d67e4098351f..f690ae999f37 100644
>>>> --- a/extent-tree.c
>>>> +++ b/extent-tree.c
>>>> @@ -164,6 +164,9 @@ err:
>>>>        return 0;
>>>>    }
>>>>
>>>> +/*
>>>> + * Return the block group that contains or after bytenr
>>>> + */
>>>
>>> What about "Return the block group thart starts at or after @bytenr" ?
>>>
>>
>> That's what documented in kernel already.
>> The thing I try to express is "contains".
>> For such a block group marked as B[n, m).
>> btrfs_lookup_first_block_group(x) (x > n && x < m). Kernel code will
>> return the block group next to B. However, progs side will return the
>> block group B.
>
> "Contains" indeed covers your example.
> But the "after @bytenr" part looks strange to me.
>
> Did you mean if @bytenr is not covered by any block group, then the next
> block group starts after @bytenr is returned?
>
Yes, that's precisely what I mean.

Thanks.
> Then the comment is good.
>
> Thanks,
> Qu
>
>>
>> Thanks.
>>> Thanks,
>>> Qu
>>>
>>>>    struct btrfs_block_group_cache *btrfs_lookup_first_block_group(struct
>>>>                                   btrfs_fs_info *info,
>>>>                                   u64 bytenr)
>>>> @@ -193,6 +196,9 @@ struct btrfs_block_group_cache
>>>> *btrfs_lookup_first_block_group(struct
>>>>        return block_group;
>>>>    }
>>>>
>>>> +/*
>>>> + * Return the block group that contains the given bytenr
>>>> + */
>>>>    struct btrfs_block_group_cache *btrfs_lookup_block_group(struct
>>>>                                 btrfs_fs_info *info,
>>>>                                 u64 bytenr)
>>>>
>>>
>

Re: [PATCH 1/2] btrfs-progs: add comments of block group lookup functions

[David Sterba]

On Mon, Nov 11, 2019 at 06:17:17PM +0800, Su Yue wrote:
> >>>> + * Return the block group that contains or after bytenr
> >>>> + */
> >>>
> >>> What about "Return the block group thart starts at or after @bytenr" ?
> >>>
> >>
> >> That's what documented in kernel already.
> >> The thing I try to express is "contains".
> >> For such a block group marked as B[n, m).
> >> btrfs_lookup_first_block_group(x) (x > n && x < m). Kernel code will
> >> return the block group next to B. However, progs side will return the
> >> block group B.
> >
> > "Contains" indeed covers your example.
> > But the "after @bytenr" part looks strange to me.
> >
> > Did you mean if @bytenr is not covered by any block group, then the next
> > block group starts after @bytenr is returned?
> >
> Yes, that's precisely what I mean.

I'll use the wording as suggested by Qu. Thanks.

Re: [PATCH 1/2] btrfs-progs: add comments of block group lookup functions

[David Sterba]

On Mon, Nov 11, 2019 at 04:42:25PM +0800, damenly.su@gmail.com wrote:
> From: Su Yue <Damenly_Su@gmx.com>
> 
> The progs side function btrfs_lookup_first_block_group() calls
> find_first_extent_bit() to find block group which contains bytenr
> or after the bytenr. This behavior differs from kernel code, so
> add the comments.
> 
> Add the coments of btrfs_lookup_block_group() too, this one works
> like kernel side.
> 
> Signed-off-by: Su Yue <Damenly_Su@gmx.com>

1 and 2 added to devel, thanks.