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

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Christoph Hellwig <hch@lst.de>, Chris Mason <clm@fb.com>,
	Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>
Cc: Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org, Nikolay Borisov <nborisov@suse.com>
Subject: Re: [PATCH 2/8] btrfs: introduce a pure data checksum checking helper
Date: Mon, 23 May 2022 08:38:13 +0800	[thread overview]
Message-ID: <d459113f-7325-ebe9-77de-6639c646f0df@gmx.com> (raw)
In-Reply-To: <20220522114754.173685-3-hch@lst.de>

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?

Thanks,
Qu
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/compression.c | 13 ++++---------
>   fs/btrfs/ctree.h       |  2 ++
>   fs/btrfs/inode.c       | 38 ++++++++++++++++++++++++++++----------
>   3 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index f4564f32f6d93..6ab82e142f1f8 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -147,12 +147,10 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
>   				 u64 disk_start)
>   {
>   	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> -	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
>   	const u32 csum_size = fs_info->csum_size;
>   	const u32 sectorsize = fs_info->sectorsize;
>   	struct page *page;
>   	unsigned int i;
> -	char *kaddr;
>   	u8 csum[BTRFS_CSUM_SIZE];
>   	struct compressed_bio *cb = bio->bi_private;
>   	u8 *cb_sum = cb->sums;
> @@ -161,8 +159,6 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
>   	    test_bit(BTRFS_FS_STATE_NO_CSUMS, &fs_info->fs_state))
>   		return 0;
>
> -	shash->tfm = fs_info->csum_shash;
> -
>   	for (i = 0; i < cb->nr_pages; i++) {
>   		u32 pg_offset;
>   		u32 bytes_left = PAGE_SIZE;
> @@ -175,12 +171,11 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
>   		/* Hash through the page sector by sector */
>   		for (pg_offset = 0; pg_offset < bytes_left;
>   		     pg_offset += sectorsize) {
> -			kaddr = kmap_atomic(page);
> -			crypto_shash_digest(shash, kaddr + pg_offset,
> -					    sectorsize, csum);
> -			kunmap_atomic(kaddr);
> +			int ret;
>
> -			if (memcmp(&csum, cb_sum, csum_size) != 0) {
> +			ret = btrfs_check_sector_csum(fs_info, page, pg_offset,
> +						      csum, cb_sum);
> +			if (ret) {
>   				btrfs_print_data_csum_error(inode, disk_start,
>   						csum, cb_sum, cb->mirror_num);
>   				if (btrfs_bio(bio)->device)
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 0e49b1a0c0716..8dd7d36b83ecb 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3253,6 +3253,8 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path);
>   /* inode.c */
>   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);
>   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 da13bd0d10f12..e4acdec9ffc69 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3326,6 +3326,29 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
>   				       finish_ordered_fn, uptodate);
>   }
>
> +/*
> + * Verify the checksum for a single sector without any extra action that
> + * 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)
> +{
> +	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
> +	char *kaddr;
> +
> +	ASSERT(pgoff + fs_info->sectorsize <= PAGE_SIZE);
> +
> +	shash->tfm = fs_info->csum_shash;
> +
> +	kaddr = kmap_local_page(page) + pgoff;
> +	crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
> +	kunmap_local(kaddr);
> +
> +	if (memcmp(csum, csum_expected, fs_info->csum_size))
> +		return -EIO;
> +	return 0;
> +}
> +
>   /*
>    * check_data_csum - verify checksum of one sector of uncompressed data
>    * @inode:	inode
> @@ -3336,14 +3359,15 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
>    * @start:	logical offset in the file
>    *
>    * The length of such check is always one sector size.
> + *
> + * When csum mismatch detected, we will also report the error and fill the
> + * corrupted range with zero. (thus it needs the extra parameters)
>    */
>   static int check_data_csum(struct inode *inode, struct btrfs_bio *bbio,
>   			   u32 bio_offset, struct page *page, u32 pgoff,
>   			   u64 start)
>   {
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> -	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
> -	char *kaddr;
>   	u32 len = fs_info->sectorsize;
>   	const u32 csum_size = fs_info->csum_size;
>   	unsigned int offset_sectors;
> @@ -3355,16 +3379,10 @@ static int check_data_csum(struct inode *inode, struct btrfs_bio *bbio,
>   	offset_sectors = bio_offset >> fs_info->sectorsize_bits;
>   	csum_expected = ((u8 *)bbio->csum) + offset_sectors * csum_size;
>
> -	kaddr = kmap_atomic(page);
> -	shash->tfm = fs_info->csum_shash;
> -
> -	crypto_shash_digest(shash, kaddr + pgoff, len, csum);
> -	kunmap_atomic(kaddr);
> -
> -	if (memcmp(csum, csum_expected, csum_size))
> +	if (btrfs_check_sector_csum(fs_info, page, pgoff, csum, csum_expected))
>   		goto zeroit;
> -
>   	return 0;
> +
>   zeroit:
>   	btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected,
>   				    bbio->mirror_num);