| Message ID | 20250106095613.847700-6-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:42AM +0100, Christoph Hellwig wrote: > xfs_buf_delwri_pushbuf synchronously writes a buffer that is on a delwri > list already. Instead of doing a complicated dance with the delwri > and wait list, just leave them alone and open code the actual buffer > write. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_buf.c | 33 ++++++++------------------------- > 1 file changed, 8 insertions(+), 25 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index a3484421a6d8..7edd7a1e9dae 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -2391,14 +2391,9 @@ xfs_buf_delwri_submit( > * Push a single buffer on a delwri queue. > * > * The purpose of this function is to submit a single buffer of a delwri queue > - * and return with the buffer still on the original queue. The waiting delwri > - * buffer submission infrastructure guarantees transfer of the delwri queue > - * buffer reference to a temporary wait list. We reuse this infrastructure to > - * transfer the buffer back to the original queue. > + * and return with the buffer still on the original queue. > * > - * Note the buffer transitions from the queued state, to the submitted and wait > - * listed state and back to the queued state during this call. The buffer > - * locking and queue management logic between _delwri_pushbuf() and > + * The buffer locking and queue management logic between _delwri_pushbuf() and > * _delwri_queue() guarantee that the buffer cannot be queued to another list > * before returning. > */ > @@ -2407,33 +2402,21 @@ xfs_buf_delwri_pushbuf( > struct xfs_buf *bp, > struct list_head *buffer_list) > { > - LIST_HEAD (submit_list); > int error; > > ASSERT(bp->b_flags & _XBF_DELWRI_Q); > > trace_xfs_buf_delwri_pushbuf(bp, _RET_IP_); > > - /* > - * Isolate the buffer to a new local list so we can submit it for I/O > - * independently from the rest of the original list. > - */ > xfs_buf_lock(bp); > - list_move(&bp->b_list, &submit_list); > - xfs_buf_unlock(bp); > - > - /* > - * Delwri submission clears the DELWRI_Q buffer flag and returns with > - * the buffer on the wait list with the original reference. Rather than > - * bounce the buffer from a local wait list back to the original list > - * after I/O completion, reuse the original list as the wait list. > - */ > - xfs_buf_delwri_submit_buffers(&submit_list, buffer_list); > + bp->b_flags &= ~(_XBF_DELWRI_Q | XBF_ASYNC); > + bp->b_flags |= XBF_WRITE; > + xfs_buf_submit(bp); Why is it ok to ignore the return value here? Is it because the only error path in xfs_buf_submit is the xlog_is_shutdown case, in which case the buffer ioend will have been called already and the EIO will be returned by xfs_buf_iowait? --D > > /* > - * The buffer is now locked, under I/O and wait listed on the original > - * delwri queue. Wait for I/O completion, restore the DELWRI_Q flag and > - * return with the buffer unlocked and on the original queue. > + * The buffer is now locked, under I/O but still on the original delwri > + * queue. Wait for I/O completion, restore the DELWRI_Q flag and > + * return with the buffer unlocked and still on the original queue. > */ > error = xfs_buf_iowait(bp); > bp->b_flags |= _XBF_DELWRI_Q; > -- > 2.45.2 > >
On Mon, Jan 06, 2025 at 06:08:10PM -0800, Darrick J. Wong wrote: > > - * after I/O completion, reuse the original list as the wait list. > > - */ > > - xfs_buf_delwri_submit_buffers(&submit_list, buffer_list); > > + bp->b_flags &= ~(_XBF_DELWRI_Q | XBF_ASYNC); > > + bp->b_flags |= XBF_WRITE; > > + xfs_buf_submit(bp); > > Why is it ok to ignore the return value here? Is it because the only > error path in xfs_buf_submit is the xlog_is_shutdown case, in which case > the buffer ioend will have been called already and the EIO will be > returned by xfs_buf_iowait? A very good question to be asked to the author of the original xfs_buf_delwri_submit_buffers code that this go extracted from :) I think you're provided answer is correct and also implies that we should either get rid of the xfs_buf_submit return value or check it more consistently.
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index a3484421a6d8..7edd7a1e9dae 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -2391,14 +2391,9 @@ xfs_buf_delwri_submit( * Push a single buffer on a delwri queue. * * The purpose of this function is to submit a single buffer of a delwri queue - * and return with the buffer still on the original queue. The waiting delwri - * buffer submission infrastructure guarantees transfer of the delwri queue - * buffer reference to a temporary wait list. We reuse this infrastructure to - * transfer the buffer back to the original queue. + * and return with the buffer still on the original queue. * - * Note the buffer transitions from the queued state, to the submitted and wait - * listed state and back to the queued state during this call. The buffer - * locking and queue management logic between _delwri_pushbuf() and + * The buffer locking and queue management logic between _delwri_pushbuf() and * _delwri_queue() guarantee that the buffer cannot be queued to another list * before returning. */ @@ -2407,33 +2402,21 @@ xfs_buf_delwri_pushbuf( struct xfs_buf *bp, struct list_head *buffer_list) { - LIST_HEAD (submit_list); int error; ASSERT(bp->b_flags & _XBF_DELWRI_Q); trace_xfs_buf_delwri_pushbuf(bp, _RET_IP_); - /* - * Isolate the buffer to a new local list so we can submit it for I/O - * independently from the rest of the original list. - */ xfs_buf_lock(bp); - list_move(&bp->b_list, &submit_list); - xfs_buf_unlock(bp); - - /* - * Delwri submission clears the DELWRI_Q buffer flag and returns with - * the buffer on the wait list with the original reference. Rather than - * bounce the buffer from a local wait list back to the original list - * after I/O completion, reuse the original list as the wait list. - */ - xfs_buf_delwri_submit_buffers(&submit_list, buffer_list); + bp->b_flags &= ~(_XBF_DELWRI_Q | XBF_ASYNC); + bp->b_flags |= XBF_WRITE; + xfs_buf_submit(bp); /* - * The buffer is now locked, under I/O and wait listed on the original - * delwri queue. Wait for I/O completion, restore the DELWRI_Q flag and - * return with the buffer unlocked and on the original queue. + * The buffer is now locked, under I/O but still on the original delwri + * queue. Wait for I/O completion, restore the DELWRI_Q flag and + * return with the buffer unlocked and still on the original queue. */ error = xfs_buf_iowait(bp); bp->b_flags |= _XBF_DELWRI_Q;
xfs_buf_delwri_pushbuf synchronously writes a buffer that is on a delwri list already. Instead of doing a complicated dance with the delwri and wait list, just leave them alone and open code the actual buffer write. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_buf.c | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-)