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

Qu Wenruo
2022-05-23T00:38:32Z

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);