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

From: Nikolay Borisov <nborisov@suse.com>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 03/10] btrfs: Simplify metadata pages reading
Date: Mon, 14 Sep 2020 11:08:45 +0300	[thread overview]
Message-ID: <7a1ae6a0-30d9-63de-f3f9-2b6f3c9653f2@suse.com> (raw)
In-Reply-To: <d4a8e47d-04ea-79dd-3dd9-0080b611112b@gmx.com>

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.

> 
> 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 {
>>
> 