Re: [PATCH 01/15] btrfs: introduce a pure data checksum checking helper

[Nikolay Borisov <nborisov@suse.com>]

On 17.05.22 г. 17:50 ч., Christoph Hellwig wrote:
> From: Qu Wenruo <wqu@suse.com>
> 
> Although we have several data csum verification code, we never have a
> function really just to verify checksum for one sector.
> 
> Function check_data_csum() do extra work for error reporting, thus it
> requires a lot of extra things like file offset, bio_offset etc.
> 
> Function btrfs_verify_data_csum() is even worse, it will utizlie page
> checked flag, which means it can not be utilized for direct IO pages.
> 
> Here we introduce a new helper, btrfs_check_data_sector(), which really
> only accept a sector in page, and expected checksum pointer.
> 
> We use this function to implement check_data_csum(), and export it for
> incoming patch.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> [hch: keep passing the csum array as an arguments, as the callers want
>        to print it]
> Signed-off-by: Christoph Hellwig <hch@lst.de>


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Though I find the naming of the function suboptimal because we now have 
a bunch of *_check_* function and unless you go in and read the body of 
the new helper you have no idea what 'check' entails and that it is 
about verifying check sums.


For data reads we have:

btrfs_verify_data_csum -> check_data_csum -> btrfs_check_data_sector

I.e the newly introduced function should have csum in its name i.e 
btrfs_check_data_sector_csum.