Re: [PATCH 03/10] btrfs: Simplify metadata pages reading

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Nikolay Borisov <nborisov@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 03/10] btrfs: Simplify metadata pages reading
Date: Mon, 14 Sep 2020 16:22:39 +0800	[thread overview]
Message-ID: <81ec5378-3ed0-7cfb-eab1-93c958fa426b@gmx.com> (raw)
In-Reply-To: <7a1ae6a0-30d9-63de-f3f9-2b6f3c9653f2@suse.com>

On 2020/9/14 下午4:08, Nikolay Borisov wrote:
>
>
> On 9.09.20 г. 14:20 ч., Qu Wenruo wrote:
>>
>>
>> On 2020/9/9 下午5:49, Nikolay Borisov wrote:
>>> Metadata pages currently use __do_readpage to read metadata pages,
>>> unfortunately this function is also used to deal with ordinary data
>>> pages. This makes the metadata pages reading code to go through multiple
>>> hoops in order to adhere to __do_readpage invariants. Most of these are
>>> necessary for data pages which could be compressed. For metadata it's
>>> enough to simply build a bio and submit it.
>>>
>>> To this effect simply call submit_extent_page directly from
>>> read_extent_buffer_pages which is the only callpath used to populate
>>> extent_buffers with data. This in turn enables further cleanups.
>>
>> This is awesome!!!
>>
>> And the code also looks pretty good to me.
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Just a note for further enhancement inlined below.
>>
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>  fs/btrfs/extent_io.c | 18 ++++++------------
>>>  1 file changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>> index ac92c0ab1402..1789a7931312 100644
>>> --- a/fs/btrfs/extent_io.c
>>> +++ b/fs/btrfs/extent_io.c
>>> @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>>>  			}
>>>
>>>  			ClearPageError(page);
>>> -			err = __extent_read_full_page(page,
>>> -						      btree_get_extent, &bio,
>>> -						      mirror_num, &bio_flags,
>>> -						      REQ_META);
>>> +			err = submit_extent_page(REQ_OP_READ | REQ_META, NULL,
>>> +					 page, page_offset(page), PAGE_SIZE, 0,
>>
>> It would be better to enhance the comment for submit_extent_page() of
>> @offset.
>> It's in fact btrfs logical bytenr.
>>
>> For metadata, page_offset(page) is also the btrfs logical bytenr so it's
>> completely fine.
>> But it can be different for data inodes, so it's better to make it more
>> clear in the comment.
>
> How can it be different for data node? page_offset is always page->index
> << PAGE_SHIFT, meaning it's the offset in the file that this page refers
> to. I.e page 5 would refer to 20k.

For data inode, its page_offset() is the file offset, not logical bytenr.

Thanks,
Qu

>
>>
>> Thanks,
>> Qu
>>
>>> +					 &bio, end_bio_extent_readpage,
>>> +					 mirror_num, 0, 0, false);
>>>  			if (err) {
>>>  				ret = err;
>>> -				/*
>>> -				 * We use &bio in above __extent_read_full_page,
>>> -				 * so we ensure that if it returns error, the
>>> -				 * current page fails to add itself to bio and
>>> -				 * it's been unlocked.
>>> -				 *
>>> -				 * We must dec io_pages by ourselves.
>>> -				 */
>>> +				SetPageError(page);
>>> +				unlock_page(page);
>>>  				atomic_dec(&eb->io_pages);
>>>  			}
>>>  		} else {
>>>
>>