| Message ID | 20200715123835.8690-1-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: drain the buf delwri queue before xfsaild idles | expand |
On Wed, Jul 15, 2020 at 08:38:35AM -0400, Brian Foster wrote: > index c3be6e440134..6a6a79791fbb 100644 > --- a/fs/xfs/xfs_trans_ail.c > +++ b/fs/xfs/xfs_trans_ail.c > @@ -542,11 +542,11 @@ xfsaild_push( > xfs_trans_ail_cursor_done(&cur); > spin_unlock(&ailp->ail_lock); > > +out_done: > if (xfs_buf_delwri_submit_nowait(&ailp->ail_buf_list)) > ailp->ail_log_flush++; > > if (!count || XFS_LSN_CMP(lsn, target) >= 0) { > -out_done: Nit: if you move the out_done up a bit we can also de-duplicate the xfs_trans_ail_cursor_done and spin_unlock calls. Otherwise this looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Jul 15, 2020 at 06:52:07PM +0100, Christoph Hellwig wrote: > On Wed, Jul 15, 2020 at 08:38:35AM -0400, Brian Foster wrote: > > index c3be6e440134..6a6a79791fbb 100644 > > --- a/fs/xfs/xfs_trans_ail.c > > +++ b/fs/xfs/xfs_trans_ail.c > > @@ -542,11 +542,11 @@ xfsaild_push( > > xfs_trans_ail_cursor_done(&cur); > > spin_unlock(&ailp->ail_lock); > > > > +out_done: > > if (xfs_buf_delwri_submit_nowait(&ailp->ail_buf_list)) > > ailp->ail_log_flush++; > > > > if (!count || XFS_LSN_CMP(lsn, target) >= 0) { > > -out_done: > > Nit: if you move the out_done up a bit we can also de-duplicate the > xfs_trans_ail_cursor_done and spin_unlock calls. > Good point. v2 incoming.. Brian > Otherwise this looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> >
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c index c3be6e440134..6a6a79791fbb 100644 --- a/fs/xfs/xfs_trans_ail.c +++ b/fs/xfs/xfs_trans_ail.c @@ -542,11 +542,11 @@ xfsaild_push( xfs_trans_ail_cursor_done(&cur); spin_unlock(&ailp->ail_lock); +out_done: if (xfs_buf_delwri_submit_nowait(&ailp->ail_buf_list)) ailp->ail_log_flush++; if (!count || XFS_LSN_CMP(lsn, target) >= 0) { -out_done: /* * We reached the target or the AIL is empty, so wait a bit * longer for I/O to complete and remove pushed items from the @@ -638,7 +638,8 @@ xfsaild( */ smp_rmb(); if (!xfs_ail_min(ailp) && - ailp->ail_target == ailp->ail_target_prev) { + ailp->ail_target == ailp->ail_target_prev && + list_empty(&ailp->ail_buf_list)) { spin_unlock(&ailp->ail_lock); freezable_schedule(); tout = 0;
xfsaild is racy with respect to transaction abort and shutdown in that the task can idle or exit with an empty AIL but buffers still on the delwri queue. This was partly addressed by cancelling the delwri queue before the task exits to prevent memory leaks, but it's also possible for xfsaild to empty and idle with buffers on the delwri queue. For example, a transaction that pins a buffer that also happens to sit on the AIL delwri queue will explicitly remove the associated log item from the AIL if the transaction aborts. The side effect of this is an unmount hang in xfs_wait_buftarg() as the associated buffers remain held by the delwri queue indefinitely. This is reproduced on repeated runs of generic/531 with an fs format (-mrmapbt=1 -bsize=1k) that happens to also reproduce transaction aborts. Update xfsaild to not idle until both the AIL and associated delwri queue are empty and update the push code to continue delwri queue submission attempts even when the AIL is empty. This allows the AIL to eventually release aborted buffers stranded on the delwri queue when they are unlocked by the associated transaction. This should have no significant effect on normal runtime behavior because the xfsaild currently idles only when the AIL is empty and in practice the AIL is rarely empty with a populated delwri queue. The items must be AIL resident to land in the queue in the first place and generally aren't removed until writeback completes. Note that the pre-existing delwri queue cancel logic in the exit path is retained because task stop is external, could technically come at any point, and xfsaild is still responsible to release its buffer references before it exits. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_trans_ail.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)