Re: [PATCH v2 07/18] btrfs: extent_io: make grab_extent_buffer_from_page() to handle subpage case

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

On 2020/12/10 下午11:39, Nikolay Borisov wrote:
>
>
> On 10.12.20 г. 8:38 ч., Qu Wenruo wrote:
>> For subpage case, grab_extent_buffer_from_page() can't really get an
>> extent buffer just from btrfs_subpage.
>>
>> Although we have btrfs_subpage::tree_block_bitmap, which can be used to
>> grab the bytenr of an existing extent buffer, and can then go radix tree
>> search to grab that existing eb.
>>
>> However we are still doing radix tree insert check in
>> alloc_extent_buffer(), thus we don't really need to do the extra hassle,
>> just let alloc_extent_buffer() to handle existing eb in radix tree.
>>
>> So for grab_extent_buffer_from_page(), just always return NULL for
>> subpage case.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/extent_io.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 51dd7ec3c2b3..b99bd0402130 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -5278,10 +5278,19 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
>>   }
>>   #endif
>>
>> -static struct extent_buffer *grab_extent_buffer_from_page(struct page *page)
>> +static struct extent_buffer *grab_extent_buffer_from_page(
>> +		struct btrfs_fs_info *fs_info, struct page *page)
>>   {
>>   	struct extent_buffer *exists;
>>
>> +	/*
>> +	 * For subpage case, we completely rely on radix tree to ensure we
>> +	 * don't try to insert two eb for the same bytenr.
>> +	 * So here we alwasy return NULL and just continue.
>> +	 */
>> +	if (fs_info->sectorsize < PAGE_SIZE)
>> +		return NULL;
>> +
>
> Instead of hiding this in the function, just open-code it in the only caller. It would look like:
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index b99bd0402130..440dab207944 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5370,8 +5370,9 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>                  }
>
>                  spin_lock(&mapping->private_lock);
> -               exists = grab_extent_buffer_from_page(fs_info, p);
> -               if (exists) {
> +               if (fs_info->sectorsize == PAGE_SIZE &&
> +                   (exists = grab_extent_buffer_from_page(fs_info, p)));
> +               {
>                          spin_unlock(&mapping->private_lock);
>                          unlock_page(p);
>                          put_page(p);
>
>
> Admittedly that exist = ... in the if condition is a bit of an anti-pattern but given it's used in only 1 place
> and makes the flow of code more linear I'd say it's a win. But would like to hear David's opinion.

Personally speaking, the (exists == *) inside the condition really looks
ugly and hard to grasp.

And since grab_extent_buffer_from_page() is only called once, the
generated code shouldn't be that much different anyway as the compiler
would mostly just inline it.

So I still prefer the current code, not to mention it also provides
extra space for the comment.

Thanks,
Qu
>
>>   	/* Page not yet attached to an extent buffer */
>>   	if (!PagePrivate(page))
>>   		return NULL;
>> @@ -5361,7 +5370,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>>   		}
>>
>>   		spin_lock(&mapping->private_lock);
>> -		exists = grab_extent_buffer_from_page(p);
>> +		exists = grab_extent_buffer_from_page(fs_info, p);
>>   		if (exists) {
>>   			spin_unlock(&mapping->private_lock);
>>   			unlock_page(p);
>>