On Fri, Mar 29, 2019 at 01:15:44PM -0700, Allison Henderson wrote: > Alrighty, thanks for the detailed commit message, it helps. Sometimes I > wonder if adding links to the corresponding discussion threads would help > tie the patches to the discussions that went into them? In any case, you > can add my review. > Er, yeah. I intended to cite the original thread in the cover letter but apparently forgot. If you haven't found it already, the thread is here: https://marc.info/?l=linux-xfs&m=155305825226667&w=2 > Reviewed-by: Allison Henderson <allison.henderson@xxxxxxxxxx> > Thanks for the reviews. Brian > On 3/29/19 6:40 AM, Brian Foster wrote: > > XFS shutdown deadlocks have been reproduced by fstest generic/475. > > The deadlock signature involves log I/O completion running error > > handling to abort logged items and waiting for an inode cluster > > buffer lock in the buffer item unpin handler. The buffer lock is > > held by xfsaild attempting to flush an inode. The buffer happens to > > be pinned and so xfs_iflush() triggers an async log force to begin > > work required to get it unpinned. The log force is blocked waiting > > on the commit completion, which never occurs and thus leaves the > > filesystem deadlocked. > > > > The root problem is that aborted log I/O completion pots commit > > completion behind callback completion, which is unexpected for async > > log forces. Under normal running conditions, an async log force > > returns to the caller once the CIL ctx has been formatted/submitted > > and the commit completion event triggered at the tail end of > > xlog_cil_push(). If the filesystem has shutdown, however, we rely on > > xlog_cil_committed() to trigger the completion event and it happens > > to do so after running log item unpin callbacks. This makes it > > unsafe to invoke an async log force from contexts that hold locks > > that might also be required in log completion processing. > > > > To address this problem, wake commit completion waiters before > > aborting log items in the log I/O completion handler. This ensures > > that an async log force will not deadlock on held locks if the > > filesystem happens to shutdown. Note that it is still unsafe to > > issue a sync log force while holding such locks because a sync log > > force explicitly waits on the force completion, which occurs after > > log I/O completion processing. > > > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx> > > --- > > fs/xfs/xfs_log_cil.c | 21 +++++++++++++-------- > > 1 file changed, 13 insertions(+), 8 deletions(-) > > > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > > index d3884e08b43c..5e595948bc5a 100644 > > --- a/fs/xfs/xfs_log_cil.c > > +++ b/fs/xfs/xfs_log_cil.c > > @@ -582,6 +582,19 @@ xlog_cil_committed( > > struct xfs_cil_ctx *ctx = args; > > struct xfs_mount *mp = ctx->cil->xc_log->l_mp; > > + /* > > + * If the I/O failed, we're aborting the commit and already shutdown. > > + * Wake any commit waiters before aborting the log items so we don't > > + * block async log pushers on callbacks. Async log pushers explicitly do > > + * not wait on log force completion because they may be holding locks > > + * required to unpin items. > > + */ > > + if (abort) { > > + spin_lock(&ctx->cil->xc_push_lock); > > + wake_up_all(&ctx->cil->xc_commit_wait); > > + spin_unlock(&ctx->cil->xc_push_lock); > > + } > > + > > xfs_trans_committed_bulk(ctx->cil->xc_log->l_ailp, ctx->lv_chain, > > ctx->start_lsn, abort); > > @@ -589,15 +602,7 @@ xlog_cil_committed( > > xfs_extent_busy_clear(mp, &ctx->busy_extents, > > (mp->m_flags & XFS_MOUNT_DISCARD) && !abort); > > - /* > > - * If we are aborting the commit, wake up anyone waiting on the > > - * committing list. If we don't, then a shutdown we can leave processes > > - * waiting in xlog_cil_force_lsn() waiting on a sequence commit that > > - * will never happen because we aborted it. > > - */ > > spin_lock(&ctx->cil->xc_push_lock); > > - if (abort) > > - wake_up_all(&ctx->cil->xc_commit_wait); > > list_del(&ctx->committing); > > spin_unlock(&ctx->cil->xc_push_lock); > >
