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@xxxxxx>
Yeah, it's useful to break up this function...
Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
--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
>
>
