Re: [PATCH 17/21] xfs: verify data checksums during media verification

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

On Thu, Sep 24, 2026 at 11:59:49AM +0200, Christoph Hellwig wrote:
> Wire up reading and verifying data checksums during media verification.
> This is very similar to the file read path in that it kicks of an async
> read for the checksum buffer before reading the data, and then validating
> once both are read in.
> 
> To support this, split the per-read logic in xfs_verify_media into a
> separate helpers for the data checksums vs no checksum cases.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_verify_media.c | 131 +++++++++++++++++++++++++++++++-------
>  1 file changed, 107 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_verify_media.c b/fs/xfs/xfs_verify_media.c
> index 71f4d6c832a9..46a19405c9e5 100644
> --- a/fs/xfs/xfs_verify_media.c
> +++ b/fs/xfs/xfs_verify_media.c
> @@ -22,6 +22,7 @@
>  #include "xfs_rtrmap_btree.h"
>  #include "xfs_health.h"
>  #include "xfs_healthmon.h"
> +#include "xfs_rtcsum.h"
>  #include "xfs_trace.h"
>  #include "xfs_verify_media.h"
>  
> @@ -261,6 +262,95 @@ xfs_verify_media_error(
>  	}
>  }
>  
> +static int
> +xfs_submit_verify_bio(
> +	struct xfs_mount	*mp,
> +	struct xfs_verify_media	*me,
> +	struct xfs_buftarg	*btp,
> +	struct folio		*folio,
> +	xfs_daddr_t		*daddr,
> +	uint64_t		*bbcount)
> +{
> +	unsigned int		bio_bbcount;
> +	int			error;
> +
> +	bio_bbcount = min(*bbcount, folio_size(folio) >> SECTOR_SHIFT);
> +	error = bdev_rw_virt(btp->bt_bdev, *daddr, folio_address(folio),
> +			bio_bbcount << SECTOR_SHIFT,
> +			REQ_OP_READ);
> +	if (error) {
> +		xfs_verify_media_error(mp, me, btp, *daddr, bio_bbcount, error);
> +		return 1;
> +	}
> +
> +	*daddr += bio_bbcount;
> +	*bbcount -= bio_bbcount;
> +	return 0;
> +}
> +
> +static int
> +xfs_submit_verify_bio_csum(

What's the return value convention here?  1 for media error, 0 for
success, or negative errno if we failed to issue the read?

> +	struct xfs_mount	*mp,
> +	struct xfs_verify_media	*me,
> +	struct xfs_buftarg	*btp,
> +	struct folio		*folio,
> +	xfs_daddr_t		*daddr,
> +	uint64_t		*bbcount)
> +{
> +	struct xfs_buf		*csum_bp = NULL;
> +	unsigned int		bio_bbcount;
> +	struct bvec_iter	saved_iter;
> +	xfs_fsblock_t		bno, end;
> +	xfs_filblks_t		len;
> +	struct bio		bio;
> +	struct bio_vec		bv;
> +	int			error;
> +
> +	bno = xfs_daddr_to_rtb(mp, *daddr);
> +	end = xfs_daddr_to_rtb(mp, *daddr + *bbcount);

It occurs to me that xfs_daddr_to_rtb rounds its argument down.  So if
you pass in daddr==0 and bbcount==2, you'll get bno==end==0 and do no
verification.  I would hope that callers won't pass in parameters like
that, but who knows?

So I think this should be:

	end = xfs_daddr_to_rtb(mp,
			*daddr + *bbcount + XFS_FSB_TO_BB(mp, 1) - 1);

Which I admit is a bit gross.

--D

> +	len = min(end - bno, XFS_B_TO_FSBT(mp, folio_size(folio)));
> +	len = min(len, xfs_rtcsum_max_len(mp, bno));
> +
> +	error = xfs_rtcsum_read_async(mp, bno, &csum_bp);
> +	if (error)
> +		return error;
> +
> +	*daddr = xfs_rtb_to_daddr(mp, bno);
> +	bio_bbcount = XFS_FSB_TO_BB(mp, len);
> +
> +	bio_init(&bio, btp->bt_bdev, &bv, 1, REQ_OP_READ);
> +	bio.bi_iter.bi_sector = *daddr;
> +	bio_add_folio_nofail(&bio, folio,
> +			min(bio_bbcount << SECTOR_SHIFT, folio_size(folio)), 0);
> +	saved_iter = bio.bi_iter;
> +
> +	error = submit_bio_wait(&bio);
> +	if (error)
> +		goto out_media_error;
> +
> +	error = xfs_buf_read_async_wait(csum_bp);
> +	if (error)
> +		goto out_buf_rele;
> +
> +	error = xfs_csum_verify(mp, &bio, &saved_iter,
> +			csum_bp->b_addr + xfs_rtb_to_rtcsumoff(mp, bno), bno,
> +			false);
> +	if (error)
> +		goto out_media_error;
> +
> +	*daddr += bio_bbcount;
> +	*bbcount -= bio_bbcount;
> +
> +out_buf_rele:
> +	xfs_buf_rele(csum_bp);
> +	bio_uninit(&bio);
> +	return error;
> +out_media_error:
> +	xfs_verify_media_error(mp, me, btp, *daddr, bio_bbcount, error);
> +	error = 1;
> +	goto out_buf_rele;
> +}
> +
>  /* Verify the media of an xfs device by submitting read requests to the disk. */
>  static int
>  xfs_verify_media(
> @@ -310,18 +400,13 @@ xfs_verify_media(
>  		return 0;
>  
>  	/*
> -	 * There are three ranges involved here:
> -	 *
> -	 *  - [me->me_start_daddr, me->me_end_daddr) is the range that the
> -	 *    user wants to verify.  end_daddr can be beyond the end of the
> -	 *    disk; we'll constrain it to the end if necessary.
> +	 * [me->me_start_daddr, me->me_end_daddr) is the range that the user
> +	 * wants to verify.  end_daddr can be beyond the end of the disk; we'll
> +	 * constrain it to the end if necessary.
>  	 *
> -	 *  - [daddr, me->me_end_daddr) is the range that we have not yet
> -	 *    verified.  We update daddr after each successful read.
> -	 *    me->me_start_daddr is set to daddr before returning.
> -	 *
> -	 *  - [daddr, daddr + bio_bbcount) is the range that we're currently
> -	 *    verifying.
> +	 * [daddr, me->me_end_daddr) is the range that we have not yet verified.
> +	 * We update daddr after each successful read.  me->me_start_daddr is
> +	 * set to daddr before returning.
>  	 */
>  	daddr = me->me_start_daddr;
>  	bbcount = min_t(sector_t, me->me_end_daddr, btp->bt_nr_sectors) -
> @@ -334,22 +419,20 @@ xfs_verify_media(
>  	trace_xfs_verify_media(mp, me, btp->bt_dev, daddr, bbcount, folio);
>  
>  	for (;;) {
> -		unsigned int	bio_bbcount;
> -
> -		bio_bbcount = min(bbcount, folio_size(folio) >> SECTOR_SHIFT);
> -		error = bdev_rw_virt(btp->bt_bdev, daddr, folio_address(folio),
> -				bio_bbcount << SECTOR_SHIFT,
> -				REQ_OP_READ);
> +		if (IS_ENABLED(CONFIG_XFS_RT) &&
> +		    me->me_dev == XFS_DEV_RT &&
> +		    xfs_has_rtcsum(mp)) {
> +			error = xfs_submit_verify_bio_csum(mp, me, btp, folio,
> +					&daddr, &bbcount);
> +		} else {
> +			error = xfs_submit_verify_bio(mp, me, btp, folio,
> +					&daddr, &bbcount);
> +		}
>  		if (error) {
> -			xfs_verify_media_error(mp, me, btp, daddr, bio_bbcount,
> -					error);
> -			error = 0;
> +			if (error == 1)
> +				error = 0;
>  			break;
>  		}
> -
> -		daddr += bio_bbcount;
> -		bbcount -= bio_bbcount;
> -
>  		if (bbcount == 0)
>  			break;
>  
> -- 
> 2.53.0
> 
>