Re: [PATCH 7/7] xfs: implement block-metadata based data checksums

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Feb 03, 2025 at 10:43:11AM +0100, Christoph Hellwig wrote:
> This is a quick hack to demonstrate how data checksumming can be
> implemented when it can be stored in the out of line metadata for each
> logical block.  It builds on top of the previous PI infrastructure
> and instead of generating/verifying protection information it simply
> generates and verifies a crc32c checksum and stores it in the non-PI

PI can do crc32c now?  I thought it could only do that old crc16 from
like 15 years ago and crc64?  If we try to throw crc32c at a device,
won't it then reject the "incorrect" checksums?  Or is there some other
magic in here where it works and I'm just too out of date to know?

<shrug>

The crc32c generation and validation looks decent though we're
definitely going to want an inode flag so that we're not stuck with
stable page writes.

--D

> metadata.  It misses a feature bit in the superblock, checking that
> enough size is available in the metadata and many other things.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_data_csum.c | 79 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_data_csum.c b/fs/xfs/xfs_data_csum.c
> index d9d3620654b1..862388803398 100644
> --- a/fs/xfs/xfs_data_csum.c
> +++ b/fs/xfs/xfs_data_csum.c
> @@ -14,6 +14,73 @@
>  #include <linux/blk-integrity.h>
>  #include <linux/bio-integrity.h>
>  
> +static inline void *xfs_csum_buf(struct bio *bio)
> +{
> +	return bvec_virt(bio_integrity(bio)->bip_vec);
> +}
> +
> +static inline __le32
> +xfs_data_csum(
> +	void			*data,
> +	unsigned int		len)
> +{
> +	return xfs_end_cksum(crc32c(XFS_CRC_SEED, data, len));
> +}
> +
> +static void
> +__xfs_data_csum_generate(
> +	struct bio		*bio)
> +{
> +	unsigned int		ssize = bdev_logical_block_size(bio->bi_bdev);
> +	__le32			*csum_buf = xfs_csum_buf(bio);
> +	struct bvec_iter_all	iter;
> +	struct bio_vec		*bv;
> +	int			c = 0;
> +
> +	bio_for_each_segment_all(bv, bio, iter) {
> +		void		*p;
> +		unsigned int	off;
> +
> +		p = bvec_kmap_local(bv);
> +		for (off = 0; off < bv->bv_len; off += ssize)
> +			csum_buf[c++] = xfs_data_csum(p + off, ssize);
> +		kunmap_local(p);
> +	}
> +}
> +
> +static int
> +__xfs_data_csum_verify(
> +	struct bio		*bio,
> +	struct xfs_inode	*ip,
> +	xfs_off_t		file_offset)
> +{
> +	unsigned int		ssize = bdev_logical_block_size(bio->bi_bdev);
> +	__le32			*csum_buf = xfs_csum_buf(bio);
> +	int			c = 0;
> +	struct bvec_iter_all	iter;
> +	struct bio_vec		*bv;
> +
> +	bio_for_each_segment_all(bv, bio, iter) {
> +		void		*p;
> +		unsigned int	off;
> +
> +		p = bvec_kmap_local(bv);
> +		for (off = 0; off < bv->bv_len; off += ssize) {
> +			if (xfs_data_csum(p + off, ssize) != csum_buf[c++]) {
> +				kunmap_local(p);
> +				xfs_warn(ip->i_mount,
> +"checksum mismatch at inode 0x%llx offset %lld",
> +					ip->i_ino, file_offset);
> +				return -EFSBADCRC;
> +			}
> +			file_offset += ssize;
> +		}
> +		kunmap_local(p);
> +	}
> +
> +	return 0;
> +}
> +
>  void *
>  xfs_data_csum_alloc(
>  	struct bio		*bio)
> @@ -53,11 +120,14 @@ xfs_data_csum_generate(
>  {
>  	struct blk_integrity	*bi = blk_get_integrity(bio->bi_bdev->bd_disk);
>  
> -	if (!bi || !bi->csum_type)
> +	if (!bi)
>  		return;
>  
>  	xfs_data_csum_alloc(bio);
> -	blk_integrity_generate(bio);
> +	if (!bi->csum_type)
> +		__xfs_data_csum_generate(bio);
> +	else
> +		blk_integrity_generate(bio);
>  }
>  
>  int
> @@ -67,7 +137,10 @@ xfs_data_csum_verify(
>  	struct bio		*bio = &ioend->io_bio;
>  	struct blk_integrity	*bi = blk_get_integrity(bio->bi_bdev->bd_disk);
>  
> -	if (!bi || !bi->csum_type)
> +	if (!bi)
>  		return 0;
> +	if (!bi->csum_type)
> +		return __xfs_data_csum_verify(&ioend->io_bio,
> +				XFS_I(ioend->io_inode), ioend->io_offset);
>  	return blk_integrity_verify_all(bio, ioend->io_sector);
>  }
> -- 
> 2.45.2
> 
>