On Mon, Jan 13, 2025 at 03:12:08PM +0100, Christoph Hellwig wrote:
> There is no good reason to pass a bool argument to wait for a buffer when
> the callers that want that can easily just wait themselves.
>
> This means the wait moves out of the extra hold of the buffer, but as the
> callers of synchronous buffer I/O need to hold a reference anyway that is
> perfectly fine.
>
> Because all async buffer submitters ignore the error return value, and
> the synchronous ones catch the error condition through b_error and
> xfs_buf_iowait this also means the new xfs_buf_submit doesn't have to
> return an error code.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Looks good to me now,
Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_buf.c | 42 ++++++++++++++++--------------------------
> 1 file changed, 16 insertions(+), 26 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 5702cad9ccc9..5abada2b4a4a 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -53,14 +53,8 @@ struct kmem_cache *xfs_buf_cache;
> * b_lock (trylock due to inversion)
> */
>
> -static int __xfs_buf_submit(struct xfs_buf *bp, bool wait);
> -
> -static inline int
> -xfs_buf_submit(
> - struct xfs_buf *bp)
> -{
> - return __xfs_buf_submit(bp, !(bp->b_flags & XBF_ASYNC));
> -}
> +static void xfs_buf_submit(struct xfs_buf *bp);
> +static int xfs_buf_iowait(struct xfs_buf *bp);
>
> static inline bool xfs_buf_is_uncached(struct xfs_buf *bp)
> {
> @@ -804,7 +798,10 @@ _xfs_buf_read(
> bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD | XBF_DONE);
> bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD);
>
> - return xfs_buf_submit(bp);
> + xfs_buf_submit(bp);
> + if (flags & XBF_ASYNC)
> + return 0;
> + return xfs_buf_iowait(bp);
> }
>
> /*
> @@ -980,8 +977,8 @@ xfs_buf_read_uncached(
> bp->b_ops = ops;
>
> xfs_buf_submit(bp);
> - if (bp->b_error) {
> - error = bp->b_error;
> + error = xfs_buf_iowait(bp);
> + if (error) {
> xfs_buf_relse(bp);
> return error;
> }
> @@ -1483,7 +1480,8 @@ xfs_bwrite(
> bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
> XBF_DONE);
>
> - error = xfs_buf_submit(bp);
> + xfs_buf_submit(bp);
> + error = xfs_buf_iowait(bp);
> if (error)
> xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
> return error;
> @@ -1698,13 +1696,10 @@ xfs_buf_iowait(
> * safe to reference the buffer after a call to this function unless the caller
> * holds an additional reference itself.
> */
> -static int
> -__xfs_buf_submit(
> - struct xfs_buf *bp,
> - bool wait)
> +static void
> +xfs_buf_submit(
> + struct xfs_buf *bp)
> {
> - int error = 0;
> -
> trace_xfs_buf_submit(bp, _RET_IP_);
>
> ASSERT(!(bp->b_flags & _XBF_DELWRI_Q));
> @@ -1724,10 +1719,9 @@ __xfs_buf_submit(
> * state here rather than mount state to avoid corrupting the log tail
> * on shutdown.
> */
> - if (bp->b_mount->m_log &&
> - xlog_is_shutdown(bp->b_mount->m_log)) {
> + if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log)) {
> xfs_buf_ioend_fail(bp);
> - return -EIO;
> + return;
> }
>
> /*
> @@ -1765,16 +1759,12 @@ __xfs_buf_submit(
> xfs_buf_ioend_async(bp);
> }
>
> - if (wait)
> - error = xfs_buf_iowait(bp);
> -
> /*
> * Release the hold that keeps the buffer referenced for the entire
> * I/O. Note that if the buffer is async, it is not safe to reference
> * after this release.
> */
> xfs_buf_rele(bp);
> - return error;
> }
>
> void *
> @@ -2323,7 +2313,7 @@ xfs_buf_delwri_submit_buffers(
> bp->b_flags |= XBF_ASYNC;
> xfs_buf_list_del(bp);
> }
> - __xfs_buf_submit(bp, false);
> + xfs_buf_submit(bp);
> }
> blk_finish_plug(&plug);
>
> --
> 2.45.2
>
>
