Re: [PATCH 02/17] btrfs: calculate inline extent buffer page size based on page size

[Qu Wenruo <wqu@suse.com>]

On 2020/9/11 下午5:56, Nikolay Borisov wrote:
> 
> 
> On 8.09.20 г. 10:52 ч., Qu Wenruo wrote:
>> Btrfs only support 64K as max node size, thus for 4K page system, we
>> would have at most 16 pages for one extent buffer.
>>
>> But for 64K system, we only need and always need one page for extent
>> buffer.
> 
> -EAMBIGIOUS. It should be "For a system using 64k pages we would really
> have have just a single page"
> 
>> This stays true even for future subpage sized sector size support (as
>> long as extent buffer doesn't cross 64K boundary).
>>
>> So this patch will change how INLINE_EXTENT_BUFFER_PAGES is calculated.
>>
>> Instead of using fixed 16 pages, use (64K / PAGE_SIZE) as the result.
>> This should save some bytes for extent buffer structure for 64K
>> systems.
> 
> I'd rephrase the whole changelog as something along the lines of :
> 
> "Currently btrfs hardcodes the number of inline pages it uses to 16
> which in turn has an effect on MAX_INLINE_EXTENT_BUFFER_SIZE effectively
> defining the upper limit of the size of extent buffer. For systems using
> 4k pages this works out fine but on 64k page systems this results in
> unnecessary memory overhead. That's due to the fact
> BTRFS_MAX_METADATA_BLOCKSIZE is defined as 64k so having
> INLINE_EXTENT_BUFFER_PAGES set to 16 on a 64k system results in
> extent_buffer::pages being unnecessarily large since an eb can be mapped
> with just a single page but the pages array would be 16 entries large.

Really? Turning 3 small sentences into one paragraph without much
separation?

It doesn't improve the readability to me...

> 
> Fix this by changing the way we calculate the size of the pages array by
> exploiting the fact an eb can't be larger than 64k so choosing enough
> pages to fit it"
> 
> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> This patch must be split into 2:
> 1. Changing the defines
> 2. Simplifying num_extent_pages

That's OK to do.

> 
>> ---
>>  fs/btrfs/extent_io.h | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
>> index 00a88f2eb5ab..e16c5449ba48 100644
>> --- a/fs/btrfs/extent_io.h
>> +++ b/fs/btrfs/extent_io.h
>> @@ -86,8 +86,8 @@ struct extent_io_ops {
>>  };
>>  
>>  
>> -#define INLINE_EXTENT_BUFFER_PAGES 16
>> -#define MAX_INLINE_EXTENT_BUFFER_SIZE (INLINE_EXTENT_BUFFER_PAGES * PAGE_SIZE)
>> +#define MAX_INLINE_EXTENT_BUFFER_SIZE 	SZ_64K
>> +#define INLINE_EXTENT_BUFFER_PAGES 	(MAX_INLINE_EXTENT_BUFFER_SIZE / PAGE_SIZE)
> 
> Actually having the defines like that it makes no sense to keep
> MAX_INLINE_EXTENT_BUFFER_SIZE around since it has the same value as
> BTRFS_MAX_METADATA_BLOCKSIZE. So why not just remove
> MAX_INLINE_EXTENT_BUFFER_SIZE and use BTRFS_MAX_METADATA_BLOCKSIZE when
> calculating INLINE_EXTENT_BUFFER_PAGES.

That's indeed much better.

Thanks,
Qu

> 
> 
>>  struct extent_buffer {
>>  	u64 start;
>>  	unsigned long len;
>> @@ -227,8 +227,15 @@ void wait_on_extent_buffer_writeback(struct extent_buffer *eb);
>>  
>>  static inline int num_extent_pages(const struct extent_buffer *eb)
>>  {
>> -	return (round_up(eb->start + eb->len, PAGE_SIZE) >> PAGE_SHIFT) -
>> -	       (eb->start >> PAGE_SHIFT);
>> +	/*
>> +	 * For sectorsize == PAGE_SIZE case, since eb is always aligned to
>> +	 * sectorsize, it's just (eb->len / PAGE_SIZE) >> PAGE_SHIFT.
>> +	 *
>> +	 * For sectorsize < PAGE_SIZE case, we only want to support 64K
>> +	 * PAGE_SIZE, and ensured all tree blocks won't cross page boundary.
>> +	 * So in that case we always got 1 page.
>> +	 */
>> +	return (round_up(eb->len, PAGE_SIZE) >> PAGE_SHIFT);
>>  }
>>  
>>  static inline int extent_buffer_uptodate(const struct extent_buffer *eb)
>>
>