Re: [PATCH 07/15] xfs: move write verification out of _xfs_buf_ioapply

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

On Mon, Jan 06, 2025 at 10:54:44AM +0100, Christoph Hellwig wrote:
> Split the write verification logic out of _xfs_buf_ioapply into a new
> xfs_buf_verify_write helper called by xfs_buf_submit given that it isn't
> about applying the I/O and doesn't really fit in with the rest of
> _xfs_buf_ioapply.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yeah, it's useful to break up this function...
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c | 67 ++++++++++++++++++++++++++----------------------
>  1 file changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index e48d796c786b..18e830c4e990 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1615,36 +1615,6 @@ _xfs_buf_ioapply(
>  
>  	if (bp->b_flags & XBF_WRITE) {
>  		op = REQ_OP_WRITE;
> -
> -		/*
> -		 * Run the write verifier callback function if it exists. If
> -		 * this function fails it will mark the buffer with an error and
> -		 * the IO should not be dispatched.
> -		 */
> -		if (bp->b_ops) {
> -			bp->b_ops->verify_write(bp);
> -			if (bp->b_error) {
> -				xfs_force_shutdown(bp->b_mount,
> -						   SHUTDOWN_CORRUPT_INCORE);
> -				return;
> -			}
> -		} else if (bp->b_rhash_key != XFS_BUF_DADDR_NULL) {
> -			struct xfs_mount *mp = bp->b_mount;
> -
> -			/*
> -			 * non-crc filesystems don't attach verifiers during
> -			 * log recovery, so don't warn for such filesystems.
> -			 */
> -			if (xfs_has_crc(mp)) {
> -				xfs_warn(mp,
> -					"%s: no buf ops on daddr 0x%llx len %d",
> -					__func__, xfs_buf_daddr(bp),
> -					bp->b_length);
> -				xfs_hex_dump(bp->b_addr,
> -						XFS_CORRUPTION_DUMP_LEN);
> -				dump_stack();
> -			}
> -		}
>  	} else {
>  		op = REQ_OP_READ;
>  		if (bp->b_flags & XBF_READ_AHEAD)
> @@ -1693,6 +1663,36 @@ xfs_buf_iowait(
>  	return bp->b_error;
>  }
>  
> +/*
> + * Run the write verifier callback function if it exists. If this fails, mark
> + * the buffer with an error and do not dispatch the I/O.
> + */
> +static bool
> +xfs_buf_verify_write(
> +	struct xfs_buf		*bp)
> +{
> +	if (bp->b_ops) {
> +		bp->b_ops->verify_write(bp);
> +		if (bp->b_error)
> +			return false;
> +	} else if (bp->b_rhash_key != XFS_BUF_DADDR_NULL) {
> +		/*
> +		 * Non-crc filesystems don't attach verifiers during log
> +		 * recovery, so don't warn for such filesystems.
> +		 */
> +		if (xfs_has_crc(bp->b_mount)) {
> +			xfs_warn(bp->b_mount,
> +				"%s: no buf ops on daddr 0x%llx len %d",
> +				__func__, xfs_buf_daddr(bp),
> +				bp->b_length);
> +			xfs_hex_dump(bp->b_addr, XFS_CORRUPTION_DUMP_LEN);
> +			dump_stack();
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  /*
>   * Buffer I/O submission path, read or write. Asynchronous submission transfers
>   * the buffer lock ownership and the current reference to the IO. It is not
> @@ -1751,8 +1751,15 @@ xfs_buf_submit(
>  	atomic_set(&bp->b_io_remaining, 1);
>  	if (bp->b_flags & XBF_ASYNC)
>  		xfs_buf_ioacct_inc(bp);
> +
> +	if ((bp->b_flags & XBF_WRITE) && !xfs_buf_verify_write(bp)) {
> +		xfs_force_shutdown(bp->b_mount, SHUTDOWN_CORRUPT_INCORE);
> +		goto done;
> +	}
> +
>  	_xfs_buf_ioapply(bp);
>  
> +done:
>  	/*
>  	 * If _xfs_buf_ioapply failed, we can get back here with only the IO
>  	 * reference we took above. If we drop it to zero, run completion so
> -- 
> 2.45.2
> 
>