Re: [PATCH 3/6] xfs: run callbacks before waking waiters in xlog_state_shutdown_callbacks

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 3/6] xfs: run callbacks before waking waiters in xlog_state_shutdown_callbacks
Date: Tue, 29 Mar 2022 10:13:15 +1100	[thread overview]
Message-ID: <20220328231315.GW1544202@dread.disaster.area> (raw)
In-Reply-To: <20220328230531.GB27713@magnolia>

On Mon, Mar 28, 2022 at 04:05:31PM -0700, Darrick J. Wong wrote:
> On Thu, Mar 24, 2022 at 11:21:00AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Brian reported a null pointer dereference failure during unmount in
> > xfs/006. He tracked the problem down to the AIL being torn down
> > before a log shutdown had completed and removed all the items from
> > the AIL. The failure occurred in this path while unmount was
> > proceeding in another task:
> > 
> >  xfs_trans_ail_delete+0x102/0x130 [xfs]
> >  xfs_buf_item_done+0x22/0x30 [xfs]
> >  xfs_buf_ioend+0x73/0x4d0 [xfs]
> >  xfs_trans_committed_bulk+0x17e/0x2f0 [xfs]
> >  xlog_cil_committed+0x2a9/0x300 [xfs]
> >  xlog_cil_process_committed+0x69/0x80 [xfs]
> >  xlog_state_shutdown_callbacks+0xce/0xf0 [xfs]
> >  xlog_force_shutdown+0xdf/0x150 [xfs]
> >  xfs_do_force_shutdown+0x5f/0x150 [xfs]
> >  xlog_ioend_work+0x71/0x80 [xfs]
> >  process_one_work+0x1c5/0x390
> >  worker_thread+0x30/0x350
> >  kthread+0xd7/0x100
> >  ret_from_fork+0x1f/0x30
> > 
> > This is processing an EIO error to a log write, and it's
> > triggering a force shutdown. This causes the log to be shut down,
> > and then it is running attached iclog callbacks from the shutdown
> > context. That means the fs and log has already been marked as
> > xfs_is_shutdown/xlog_is_shutdown and so high level code will abort
> > (e.g. xfs_trans_commit(), xfs_log_force(), etc) with an error
> > because of shutdown.
> > 
> > The umount would have been blocked waiting for a log force
> > completion inside xfs_log_cover() -> xfs_sync_sb(). The first thing
> > for this situation to occur is for xfs_sync_sb() to exit without
> > waiting for the iclog buffer to be comitted to disk. The
> > above trace is the completion routine for the iclog buffer, and
> > it is shutting down the filesystem.
> > 
> > xlog_state_shutdown_callbacks() does this:
> > 
> > {
> >         struct xlog_in_core     *iclog;
> >         LIST_HEAD(cb_list);
> > 
> >         spin_lock(&log->l_icloglock);
> >         iclog = log->l_iclog;
> >         do {
> >                 if (atomic_read(&iclog->ic_refcnt)) {
> >                         /* Reference holder will re-run iclog callbacks. */
> >                         continue;
> >                 }
> >                 list_splice_init(&iclog->ic_callbacks, &cb_list);
> > >>>>>>           wake_up_all(&iclog->ic_write_wait);
> > >>>>>>           wake_up_all(&iclog->ic_force_wait);
> >         } while ((iclog = iclog->ic_next) != log->l_iclog);
> > 
> >         wake_up_all(&log->l_flush_wait);
> >         spin_unlock(&log->l_icloglock);
> > 
> > >>>>>>  xlog_cil_process_committed(&cb_list);
> > }
> > 
> > It wakes forces waiters before shutdown processes all the pending
> > callbacks.
> 
> I'm not sure what this means.

"It wakes force waiters" i.e. any process in xfs_log_force() waiting
on iclog->ic_force_wait...

> Are you saying that log shutdown wakes up iclog waiters before it
> processes pending callbacks?  And then anyone who waits on an iclog (log
> forces, I guess?) will wake up and race with the callbacks?

Yes, exactly that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com