| Message ID | 20250106095613.847700-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/15] xfs: fix a double completion for buffers on in-memory targets | expand |
On Mon, Jan 06, 2025 at 10:54:41AM +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. > > Signed-off-by: Christoph Hellwig <hch@lst.de> That's much cleaner... Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> --D > --- > fs/xfs/xfs_buf.c | 36 +++++++++++++++++------------------- > 1 file changed, 17 insertions(+), 19 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 1927655fed13..a3484421a6d8 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -52,14 +52,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 int 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) > { > @@ -797,13 +791,18 @@ _xfs_buf_read( > struct xfs_buf *bp, > xfs_buf_flags_t flags) > { > + int error; > + > 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); > > - return xfs_buf_submit(bp); > + error = xfs_buf_submit(bp); > + if (!error && !(flags & XBF_ASYNC)) > + error = xfs_buf_iowait(bp); > + return error; > } > > /* > @@ -978,9 +977,10 @@ xfs_buf_read_uncached( > bp->b_flags |= XBF_READ; > bp->b_ops = ops; > > - xfs_buf_submit(bp); > - if (bp->b_error) { > - error = bp->b_error; > + error = xfs_buf_submit(bp); > + if (!error) > + error = xfs_buf_iowait(bp); > + if (error) { > xfs_buf_relse(bp); > return error; > } > @@ -1483,6 +1483,8 @@ xfs_bwrite( > XBF_DONE); > > error = xfs_buf_submit(bp); > + if (!error) > + error = xfs_buf_iowait(bp); > if (error) > xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); > return error; > @@ -1698,9 +1700,8 @@ xfs_buf_iowait( > * holds an additional reference itself. > */ > static int > -__xfs_buf_submit( > - struct xfs_buf *bp, > - bool wait) > +xfs_buf_submit( > + struct xfs_buf *bp) > { > int error = 0; > > @@ -1764,9 +1765,6 @@ __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 > @@ -2322,7 +2320,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 > >
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 1927655fed13..a3484421a6d8 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -52,14 +52,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 int 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) { @@ -797,13 +791,18 @@ _xfs_buf_read( struct xfs_buf *bp, xfs_buf_flags_t flags) { + int error; + 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); - return xfs_buf_submit(bp); + error = xfs_buf_submit(bp); + if (!error && !(flags & XBF_ASYNC)) + error = xfs_buf_iowait(bp); + return error; } /* @@ -978,9 +977,10 @@ xfs_buf_read_uncached( bp->b_flags |= XBF_READ; bp->b_ops = ops; - xfs_buf_submit(bp); - if (bp->b_error) { - error = bp->b_error; + error = xfs_buf_submit(bp); + if (!error) + error = xfs_buf_iowait(bp); + if (error) { xfs_buf_relse(bp); return error; } @@ -1483,6 +1483,8 @@ xfs_bwrite( XBF_DONE); error = xfs_buf_submit(bp); + if (!error) + error = xfs_buf_iowait(bp); if (error) xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); return error; @@ -1698,9 +1700,8 @@ xfs_buf_iowait( * holds an additional reference itself. */ static int -__xfs_buf_submit( - struct xfs_buf *bp, - bool wait) +xfs_buf_submit( + struct xfs_buf *bp) { int error = 0; @@ -1764,9 +1765,6 @@ __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 @@ -2322,7 +2320,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);
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. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_buf.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)