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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2289 bytes --]



On 2020/10/30 上午2:57, Josef Bacik wrote:
> 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)

Oh, thanks for this.
I was looking for things like that, but didn't find a handy one.

This indeed looks better.
> 
> so that we're only readahead'ing when we're likely to need to look up
> multiple items.  Thanks,
> 
> Josef


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]