Re: [PATCH 2/4] xfs: decouple buffer readahead from the normal buffer read path

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

On Mon, Feb 17, 2025 at 10:31:27AM +0100, Christoph Hellwig wrote:
> xfs_buf_readahead_map is the only caller of xfs_buf_read_map and thus
> _xfs_buf_read that is not synchronous.  Split it from xfs_buf_read_map
> so that the asynchronous path is self-contained and the now purely
> synchronous xfs_buf_read_map / _xfs_buf_read implementation can be
> simplified.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks for adding the ASSERT as a guardrail against misuse of
xfs_buf_read.

This appears to be a straightforward split, so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c         | 41 ++++++++++++++++++++--------------------
>  fs/xfs/xfs_buf.h         |  2 +-
>  fs/xfs/xfs_log_recover.c |  2 +-
>  fs/xfs/xfs_trace.h       |  1 +
>  4 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 050f2c2f6a40..52fb85c42e94 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -794,18 +794,13 @@ xfs_buf_get_map(
>  
>  int
>  _xfs_buf_read(
> -	struct xfs_buf		*bp,
> -	xfs_buf_flags_t		flags)
> +	struct xfs_buf		*bp)
>  {
> -	ASSERT(!(flags & XBF_WRITE));
>  	ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL);
>  
>  	bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD | XBF_DONE);
> -	bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD);
> -
> +	bp->b_flags |= XBF_READ;
>  	xfs_buf_submit(bp);
> -	if (flags & XBF_ASYNC)
> -		return 0;
>  	return xfs_buf_iowait(bp);
>  }
>  
> @@ -857,6 +852,8 @@ xfs_buf_read_map(
>  	struct xfs_buf		*bp;
>  	int			error;
>  
> +	ASSERT(!(flags & (XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD)));
> +
>  	flags |= XBF_READ;
>  	*bpp = NULL;
>  
> @@ -870,21 +867,11 @@ xfs_buf_read_map(
>  		/* Initiate the buffer read and wait. */
>  		XFS_STATS_INC(target->bt_mount, xb_get_read);
>  		bp->b_ops = ops;
> -		error = _xfs_buf_read(bp, flags);
> -
> -		/* Readahead iodone already dropped the buffer, so exit. */
> -		if (flags & XBF_ASYNC)
> -			return 0;
> +		error = _xfs_buf_read(bp);
>  	} else {
>  		/* Buffer already read; all we need to do is check it. */
>  		error = xfs_buf_reverify(bp, ops);
>  
> -		/* Readahead already finished; drop the buffer and exit. */
> -		if (flags & XBF_ASYNC) {
> -			xfs_buf_relse(bp);
> -			return 0;
> -		}
> -
>  		/* We do not want read in the flags */
>  		bp->b_flags &= ~XBF_READ;
>  		ASSERT(bp->b_ops != NULL || ops == NULL);
> @@ -936,6 +923,7 @@ xfs_buf_readahead_map(
>  	int			nmaps,
>  	const struct xfs_buf_ops *ops)
>  {
> +	const xfs_buf_flags_t	flags = XBF_READ | XBF_ASYNC | XBF_READ_AHEAD;
>  	struct xfs_buf		*bp;
>  
>  	/*
> @@ -945,9 +933,20 @@ xfs_buf_readahead_map(
>  	if (xfs_buftarg_is_mem(target))
>  		return;
>  
> -	xfs_buf_read_map(target, map, nmaps,
> -		     XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD, &bp, ops,
> -		     __this_address);
> +	if (xfs_buf_get_map(target, map, nmaps, flags | XBF_TRYLOCK, &bp))
> +		return;
> +	trace_xfs_buf_readahead(bp, 0, _RET_IP_);
> +
> +	if (bp->b_flags & XBF_DONE) {
> +		xfs_buf_reverify(bp, ops);
> +		xfs_buf_relse(bp);
> +		return;
> +	}
> +	XFS_STATS_INC(target->bt_mount, xb_get_read);
> +	bp->b_ops = ops;
> +	bp->b_flags &= ~(XBF_WRITE | XBF_DONE);
> +	bp->b_flags |= flags;
> +	xfs_buf_submit(bp);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 3b4ed42e11c0..2e747555ad3f 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -291,7 +291,7 @@ int xfs_buf_get_uncached(struct xfs_buftarg *target, size_t numblks,
>  int xfs_buf_read_uncached(struct xfs_buftarg *target, xfs_daddr_t daddr,
>  		size_t numblks, xfs_buf_flags_t flags, struct xfs_buf **bpp,
>  		const struct xfs_buf_ops *ops);
> -int _xfs_buf_read(struct xfs_buf *bp, xfs_buf_flags_t flags);
> +int _xfs_buf_read(struct xfs_buf *bp);
>  void xfs_buf_hold(struct xfs_buf *bp);
>  
>  /* Releasing Buffers */
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index b3c27dbccce8..2f76531842f8 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3380,7 +3380,7 @@ xlog_do_recover(
>  	 */
>  	xfs_buf_lock(bp);
>  	xfs_buf_hold(bp);
> -	error = _xfs_buf_read(bp, XBF_READ);
> +	error = _xfs_buf_read(bp);
>  	if (error) {
>  		if (!xlog_is_shutdown(log)) {
>  			xfs_buf_ioerror_alert(bp, __this_address);
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index b29462363b81..bfc2f1249022 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -593,6 +593,7 @@ DEFINE_EVENT(xfs_buf_flags_class, name, \
>  DEFINE_BUF_FLAGS_EVENT(xfs_buf_find);
>  DEFINE_BUF_FLAGS_EVENT(xfs_buf_get);
>  DEFINE_BUF_FLAGS_EVENT(xfs_buf_read);
> +DEFINE_BUF_FLAGS_EVENT(xfs_buf_readahead);
>  
>  TRACE_EVENT(xfs_buf_ioerror,
>  	TP_PROTO(struct xfs_buf *bp, int error, xfs_failaddr_t caller_ip),
> -- 
> 2.45.2
> 
>