Re: [PATCH 03/21] xfs: add a xfs_buf_read_async buffer cache API

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

On Thu, Sep 24, 2026 at 11:59:35AM +0200, Christoph Hellwig wrote:
> Add a new helper that reads a buffer asynchronously.  This is similar
> to readahead, but doesn't become a no-op under memory or I/O congestion
> and returns the buffer to be read.
> 
> The intended use is to kick off a read of data checksum buffers at
> roughly the same time as the data read so that they are available
> in the I/O completion handler.

Does there need to be a "wait until this async-read buffer reaches
XBF_DONE" function too?  Or how do callers do that?

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_buf.c   | 130 ++++++++++++++++++++++++++++++++++++---------
>  fs/xfs/xfs_buf.h   |   4 ++
>  fs/xfs/xfs_trace.h |   3 ++
>  3 files changed, 113 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index eee491c01d8c..966c06bffeff 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -627,6 +627,40 @@ _xfs_buf_read(
>  	return xfs_buf_iowait(bp);
>  }
>  
> +/*
> + * 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 to pull the buffer
> + * from disk again, we clear the XBF_DONE flag and mark the buffer stale.
> + * This ensures that anyone who has a current reference to the buffer will
> + * interpret it's contents correctly and future cache lookups will also treat it
> + * as an empty, uninitialised buffer.
> + */
> +static int
> +xfs_buf_read_error(
> +	struct xfs_buf		*bp,
> +	xfs_failaddr_t		fa,
> +	int			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(bp->b_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)
> +		return -EFSCORRUPTED;
> +	return error;
> +}
> +
>  int
>  xfs_buf_read_map(
>  	struct xfs_buftarg	*target,
> @@ -699,39 +733,87 @@ xfs_buf_read_map(
>  	}
>  
>  	if (error)
> -		goto out_ioerror;
> -
> +		return xfs_buf_read_error(bp, fa, error);
>  	*bpp = bp;
>  	return 0;
> +}
>  
> -out_ioerror:
> +int
> +xfs_buf_read_async_wait(
> +	struct xfs_buf		*bp)
> +{
>  	/*
> -	 * 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.
> +	 * Protect against the case where the checksum read is slower than the
> +	 * data read.
>  	 */
> -	if (!xlog_is_shutdown(target->bt_mount->m_log))
> -		xfs_buf_ioerror_alert(bp, fa);
> +	if ((READ_ONCE(bp->b_flags) & (XBF_DONE | XBF_STALE)) == XBF_DONE &&
> +	    !bp->b_error) {
> +		trace_xfs_buf_read_async_wait(bp, 0, _RET_IP_);
> +		return 0;
> +	}
> +
> +	/* xfs_buf_find_lock can't return an error with 0 flags */
> +	xfs_buf_find_lock(bp, 0);
> +	trace_xfs_buf_read_async_lock(bp, 0, _RET_IP_);
> +	if (bp->b_error)
> +		return xfs_buf_read_error(bp, __builtin_return_address(0),
> +				bp->b_error);
> +	ASSERT(bp->b_ops);
> +	xfs_buf_clear_flags(bp, XBF_READ);
> +	xfs_buf_unlock(bp);
> +	return 0;
> +}
> +
> +/*
> + * Kick off an asynchronous read.  Unlike readahead, this returns a reference
> + * to the buffer, and reliably reads the data instead of skipping the read on
> + * memory pressure.
> + *
> + * The buffer may be locked when I/O is kicked off, but the I/O completion
> + * handler will unlock it.  The caller needs to lock itself if need to prevent
> + * concurrent access or to synchronize with I/O completion.
> + */
> +int
> +xfs_buf_read_async(
> +	struct xfs_buftarg	*btp,
> +	xfs_daddr_t		daddr,
> +	size_t			numblks,
> +	const struct xfs_buf_ops *ops,
> +	struct xfs_buf		**bpp)
> +{
> +	DEFINE_SINGLE_BUF_MAP(map, daddr, numblks);
> +	struct xfs_buf		*bp;
> +	int			error;
> +
> +	ASSERT(!xfs_buftarg_is_mem(btp));
> +
> +	error = xfs_find_get_buf(btp, &map, 1, XBF_READ, &bp);
> +	if (error)
> +		return error;
>  
>  	/*
> -	 * 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
> -	 * to pull the buffer from disk again, we clear the XBF_DONE flag and
> -	 * mark the buffer stale. This ensures that anyone who has a current
> -	 * reference to the buffer will interpret it's contents correctly and
> -	 * future cache lookups will also treat it as an empty, uninitialised
> -	 * buffer.
> +	 * Do a lockless fast path check for a valid uptodate buffer and avoid
> +	 * locking entirely in this case.
>  	 */
> -	xfs_buf_clear_flags(bp, XBF_DONE);
> -	xfs_buf_stale(bp);
> -	xfs_buf_relse(bp);
> +	if ((READ_ONCE(bp->b_flags) & (XBF_DONE | XBF_STALE)) == XBF_DONE)
> +		goto done;
>  
> -	/* bad CRC means corrupted metadata */
> -	if (error == -EFSBADCRC)
> -		return -EFSCORRUPTED;
> -	return error;
> +	/* xfs_buf_find_lock can't return an error with 0 flags */
> +	xfs_buf_find_lock(bp, 0);
> +	if (bp->b_flags & XBF_DONE) {
> +		xfs_buf_unlock(bp);
> +		goto done;
> +	}
> +	trace_xfs_buf_read_async(bp, 0, _RET_IP_);
> +	XFS_STATS_INC(btp->bt_mount, xb_get_read);
> +	xfs_buf_hold(bp);
> +	bp->b_ops = ops;
> +	xfs_buf_clear_flags(bp, XBF_WRITE);
> +	xfs_buf_set_flags(bp, XBF_READ | XBF_ASYNC);
> +	xfs_buf_submit(bp);
> +done:
> +	*bpp = bp;
> +	return 0;
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index a4729253b56f..1b352ec91aa6 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -255,6 +255,10 @@ xfs_buf_readahead(
>  	return xfs_buf_readahead_map(target, &map, 1, ops);
>  }
>  
> +int xfs_buf_read_async(struct xfs_buftarg *btp, xfs_daddr_t daddr,
> +		size_t numblks, const struct xfs_buf_ops *ops,
> +		struct xfs_buf **bpp);
> +int xfs_buf_read_async_wait(struct xfs_buf *bp);
>  int xfs_buf_get_uncached(struct xfs_buftarg *target, size_t numblks,
>  		struct xfs_buf **bpp);
>  int xfs_buf_read_uncached(struct xfs_buftarg *target, xfs_daddr_t daddr,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 2af9a1429ae9..afaabd3adc73 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -840,6 +840,9 @@ DEFINE_EVENT(xfs_buf_flags_class, name, \
>  	TP_ARGS(bp, flags, caller_ip))
>  DEFINE_BUF_FLAGS_EVENT(xfs_buf_get);
>  DEFINE_BUF_FLAGS_EVENT(xfs_buf_read);
> +DEFINE_BUF_FLAGS_EVENT(xfs_buf_read_async);
> +DEFINE_BUF_FLAGS_EVENT(xfs_buf_read_async_wait);
> +DEFINE_BUF_FLAGS_EVENT(xfs_buf_read_async_lock);
>  DEFINE_BUF_FLAGS_EVENT(xfs_buf_readahead);
>  
>  TRACE_EVENT(xfs_buf_ioerror,
> -- 
> 2.53.0
> 
>