Re: [PATCH RFC] btrfs: allow extent buffer helpers to skip cross-page handling

[David Sterba <dsterba@suse.cz>]

On Thu, Nov 16, 2023 at 03:49:06PM +1030, Qu Wenruo wrote:
> Currently btrfs extent buffer helpers are doing all the cross-page
> handling, as there is no guarantee that all those eb pages are
> contiguous.
> 
> However on systems with enough memory, there is a very high chance the
> page cache for btree_inode are allocated with physically contiguous
> pages.
> 
> In that case, we can skip all the complex cross-page handling, thus
> speeding up the code.
> 
> This patch adds a new member, extent_buffer::addr, which is only set to
> non-NULL if all the extent buffer pages are physically contiguous.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Reason for RFC:
> 
> This change would increase the code size for all extent buffer helpers,
> and since there one more branch introduced, it may even slow down the
> system if most ebs do not have physically contiguous pages.
> 
> But I still believe this is worthy trying, as my previous attempt to
> use virtually contiguous pages are rejected due to possible slow down in
> vm_map() call.
> 
> I don't have convincing benchmark yet, but so far no obvious performance
> drop observed either.
> ---
>  fs/btrfs/disk-io.c   |  9 +++++++-
>  fs/btrfs/extent_io.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/extent_io.h |  7 ++++++
>  3 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5ac6789ca55f..7fc78171a262 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -80,8 +80,16 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
>  	char *kaddr;
>  	int i;
>  
> +	memset(result, 0, BTRFS_CSUM_SIZE);
>  	shash->tfm = fs_info->csum_shash;
>  	crypto_shash_init(shash);
> +
> +	if (buf->addr) {
> +		crypto_shash_digest(shash, buf->addr + offset_in_page(buf->start) + BTRFS_CSUM_SIZE,
> +				    buf->len - BTRFS_CSUM_SIZE, result);
> +		return;
> +	}
> +
>  	kaddr = page_address(buf->pages[0]) + offset_in_page(buf->start);
>  	crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
>  			    first_page_part - BTRFS_CSUM_SIZE);
> @@ -90,7 +98,6 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
>  		kaddr = page_address(buf->pages[i]);
>  		crypto_shash_update(shash, kaddr, PAGE_SIZE);
>  	}
> -	memset(result, 0, BTRFS_CSUM_SIZE);

This is not related to the contig pages but the result buffer for
checksum should be always cleared before storing the digest.

>  	crypto_shash_final(shash, result);
>  }
>  
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 03cef28d9e37..004b0ba6b1c7 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3476,6 +3476,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>  	struct address_space *mapping = fs_info->btree_inode->i_mapping;
>  	struct btrfs_subpage *prealloc = NULL;
>  	u64 lockdep_owner = owner_root;
> +	bool page_contig = true;
>  	int uptodate = 1;
>  	int ret;
>  
> @@ -3562,6 +3563,14 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>  
>  		WARN_ON(btrfs_page_test_dirty(fs_info, p, eb->start, eb->len));
>  		eb->pages[i] = p;
> +
> +		/*
> +		 * Check if the current page is physically contiguous with previous eb
> +		 * page.
> +		 */
> +		if (i && eb->pages[i - 1] + 1 != p)
> +			page_contig = false;

This hasn't been fixed from last time, this has almost zero chance to
succeed once the system is up for some time. Page addresses returned
from allocator are random. What I was suggesting is to use alloc_page()
with the given order (16K pages are 2).

This works for all eb sizes we need, the prolematic one could be for 64K
because this is order 4 and PAGE_ALLOC_COSTLY_ORDER is 3, so this would
cost more on the MM side. But who uses 64K node size on x8_64.

> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -77,6 +77,13 @@ struct extent_buffer {
>  	unsigned long len;
>  	unsigned long bflags;
>  	struct btrfs_fs_info *fs_info;
> +
> +	/*
> +	 * The address where the eb can be accessed without any cross-page handling.
> +	 * This can be NULL if not possible.
> +	 */
> +	void *addr;

So this is a remnant of the vm_map, we would not need to store the
address in case all the pages are contiguous, it would be the address of
pages[0]. That it's contiguous could be tracked as a bit in the flags.

> +
>  	spinlock_t refs_lock;
>  	atomic_t refs;
>  	int read_mirror;
> -- 
> 2.42.1
>