Re: [PATCH 2/8] btrfs: introduce a pure data checksum checking helper

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

On 2022/5/24 15:24, Christoph Hellwig wrote:
> On Mon, May 23, 2022 at 08:38:13AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2022/5/22 19:47, 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_sector_csum(), 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, rename per request]
>>
>> Mind to constify the @csum_expected parameter?
>
> This would be the incremental diff, if Dave cares deeply he can fold
> it in:
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index d982ea62c521b..f01ce82af8ca9 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3262,7 +3262,7 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path);
>   void btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>   			   int mirror_num, enum btrfs_compression_type compress_type);
>   int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
> -			    u32 pgoff, u8 *csum, u8 *csum_expected);
> +			    u32 pgoff, u8 *csum, u8 * const csum_expected);

Shouldn't it be "const u8 *" instead?

Anyway, normally David will handle it manually if needed.

I'm talking about this one just because my version is passing "const u8
*" for its csum_expected pointer, and caused warning here.

Thanks,
Qu

>   unsigned int btrfs_verify_data_csum(struct btrfs_bio *bbio,
>   				    u32 bio_offset, struct page *page,
>   				    u64 start, u64 end);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index bfc0b0035b03c..c344ed0e057ac 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3330,7 +3330,7 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
>    * depend on the type of I/O.
>    */
>   int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
> -			    u32 pgoff, u8 *csum, u8 *csum_expected)
> +			    u32 pgoff, u8 *csum, u8 * const csum_expected)
>   {
>   	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
>   	char *kaddr;