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

Qu Wenruo
2020-09-11T10:13:53Z

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