Re: [PATCH 09/12] xfs: use goto based error unwinding in xfs_buf_read_map

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

On Wed, Jul 15, 2026 at 04:51:02PM +0200, Christoph Hellwig wrote:
> This keeps the I/O error handling contained at the end of the function
> and removes the indentation for it.  It also allows to reorder the
> comments so that they are closer to the logic that they describe.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c | 47 ++++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 4430da3259f1..21c21491d72e 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -699,6 +699,23 @@ xfs_buf_read_map(
>  		ASSERT(bp->b_ops != NULL || ops == NULL);
>  	}
>  
> +	if (error)
> +		goto out_ioerror;
> +
> +	*bpp = bp;
> +	return 0;
> +
> +out_ioerror:
> +	/*
> +	 * Check against log shutdown for error reporting because metadata
> +	 * writeback may require a read first and we need to report errors in
> +	 * metadata writeback until the log is shut down.  High level
> +	 * transaction read functions already check against mount shutdown, so
> +	 * we only need to be concerned about low level/ IO interactions here.
> +	 */
> +	if (!xlog_is_shutdown(target->bt_mount->m_log))
> +		xfs_buf_ioerror_alert(bp, fa);
> +
>  	/*
>  	 * If we've had a read error, then the contents of the buffer are
>  	 * invalid and should not be used. To ensure that a followup read tries
> @@ -708,30 +725,14 @@ xfs_buf_read_map(
>  	 * future cache lookups will also treat it as an empty, uninitialised
>  	 * buffer.
>  	 */
> -	if (error) {
> -		/*
> -		 * Check against log shutdown for error reporting because
> -		 * metadata writeback may require a read first and we need to
> -		 * report errors in metadata writeback until the log is shut
> -		 * down. High level transaction read functions already check
> -		 * against mount shutdown, anyway, so we only need to be
> -		 * concerned about low level IO interactions here.
> -		 */
> -		if (!xlog_is_shutdown(target->bt_mount->m_log))
> -			xfs_buf_ioerror_alert(bp, fa);
> -
> -		xfs_buf_clear_flags(bp, XBF_DONE);
> -		xfs_buf_stale(bp);
> -		xfs_buf_relse(bp);
> -
> -		/* bad CRC means corrupted metadata */
> -		if (error == -EFSBADCRC)
> -			error = -EFSCORRUPTED;
> -		return error;
> -	}
> +	xfs_buf_clear_flags(bp, XBF_DONE);
> +	xfs_buf_stale(bp);
> +	xfs_buf_relse(bp);
>  
> -	*bpp = bp;
> -	return 0;
> +	/* bad CRC means corrupted metadata */
> +	if (error == -EFSBADCRC)
> +		return -EFSCORRUPTED;
> +	return error;
>  }
>  
>  /*
> -- 
> 2.53.0
> 
>