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@xxxxxx>
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@xxxxxxxxxx>
--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
>
>
