Re: [PATCH v2 1/3] btrfs: file-item: use nodesize to determine whether we need readhead for btrfs_lookup_bio_sums()

[Josef Bacik <josef@toxicpanda.com>]

On 10/29/20 3:12 AM, Qu Wenruo wrote:
> In btrfs_lookup_bio_sums() if the bio is pretty large, we want to
> readahead the csum tree.
> 
> However the threshold is an immediate number, (PAGE_SIZE * 8), from the
> initial btrfs merge.
> 
> The value itself is pretty hard to guess the meaning, especially when
> the immediate number is from the age where 4K sectorsize is the default
> and only CRC32 is supported.
> 
> For the most common btrfs setup, CRC32 csum algorithme 4K sectorsize,
> it means just 32K read would kick readahead, while the csum itself is
> only 32 bytes in size.
> 
> Now let's be more reasonable by taking both csum size and node size into
> consideration.
> 
> If the csum size for the bio is larger than one node, then we kick the
> readahead.
> This means for current default btrfs, the threshold will be 16M.
> 
> This change should not change performance observably, thus this is mostly
> a readability enhancement.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/file-item.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 7d5ec71615b8..fbc60948b2c4 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -295,7 +295,11 @@ blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio,
>   		csum = dst;
>   	}
>   
> -	if (bio->bi_iter.bi_size > PAGE_SIZE * 8)
> +	/*
> +	 * If needed csum size is larger than a node, kick the readahead for
> +	 * csum tree would be a good idea.
> +	 */
> +	if (nblocks * csum_size > fs_info->nodesize)
>   		path->reada = READA_FORWARD;

Except if we have contiguous reads we could very well have all of our csums in a 
single item.  It makes more sense to do something like

if (nblocks * csum_size > MAX_CSUM_ITEMS() * csum_size)

so that we're only readahead'ing when we're likely to need to look up multiple 
items.  Thanks,

Josef