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

[Nikolay Borisov <nborisov@suse.com>]

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.

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

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


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