diff mbox series

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

Message ID 20250217093207.3769550-3-hch@lst.de (mailing list archive)
State New
Headers show
Series [1/4] xfs: reduce context switches for synchronous buffered I/O | expand

Commit Message

Christoph Hellwig Feb. 17, 2025, 9:31 a.m. UTC 
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>
---
 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(-)

Comments

Darrick J. Wong Feb. 18, 2025, 8:10 p.m. UTC | #1 
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
> 
>
Christoph Hellwig Feb. 19, 2025, 5:32 a.m. UTC | #2 
On Tue, Feb 18, 2025 at 12:10:29PM -0800, Darrick J. Wong wrote:
> 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.

I was just going to remove the argument, but that would clash with the
refactoring in the zoned series.  But I plan to send that later.
diff mbox series

Patch

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),