Re: [PATCH 1/7] btrfs: Don't call readpage_end_io_hook for the btree inode

[Nikolay Borisov <nborisov@suse.com>]

On 21.09.20 г. 20:45 ч., David Sterba wrote:
> On Fri, Sep 18, 2020 at 04:34:33PM +0300, Nikolay Borisov wrote:
>> Instead of relying on indirect calls to implement metadata buffer
>> validation simply check if the inode whose page we are processing equals
>> the btree inode. If it does call the necessary function.
>>
>> This is an improvement in 2 directions:
>> 1. We aren't paying the penalty of indirect calls in a post-speculation
>>    attacks world.
>>
>> 2. The function is now named more explicitly so it's obvious what's
>>    going on
> 
> The new naming is not making things clear, btrfs_check_csum sounds very
> generic while it does a very specific thing just by the number and type
> of the parameters. Similar for btrfs_validate_metadata_buffer.
> 
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -2851,9 +2851,12 @@ static void end_bio_extent_readpage(struct bio *bio)
>>  
>>  		mirror = io_bio->mirror_num;
>>  		if (likely(uptodate)) {
>> -			ret = tree->ops->readpage_end_io_hook(io_bio, offset,
>> -							      page, start, end,
>> -							      mirror);
>> +			if (data_inode)
>> +				ret = btrfs_check_csum(io_bio, offset, page,
>> +						       start, end, mirror);
>> +			else
>> +				ret = btrfs_validate_metadata_buffer(io_bio,
>> +					offset, page, start, end, mirror);
> 
> In the context where the functions are used I'd expect some symmetry,
> data/metadata. Something like btrfs_validate_data_bio.
> 

The reason for this naming is that btrfs_vlidate_metadata_buffer
actually validates as in "tree-checker style validation" of the extent
buffer not simply calculating the checksum. So to me it feels like a
more complete,heavyweight operations hence "validating", whlist
btrfs_check_csum just checks the csum of a single sector/blocksize in
the bio. I think the metadata function's name conveys what it's doing in
full:

1. It's doing validation as per aforementioned explanation
2. It's doing it for a whole extent buffer and not just a chunk of it.

I agree that the data function's name is somewhat generic, perhahps it
could be renamed so that it points to the fact it's validating a single
sector/blocksize? I.e btrfs_check_ blocksize_csum or something like that ?