On Tue, May 22, 2012 at 12:38:34PM -0400, Brian Foster wrote:
> Running xfstests 273 in a loop reproduces an XFS lockup due to
> xfsaild entering idle mode indefinitely. The following
> high-level sequence of events leads to the hang:
>
> - xfsaild is running with a cached target lsn
> - xfs_ail_push() is invoked, updates ailp->xa_target_lsn and
> invokes wake_up_process(). wake_up_process() returns 0
> because xfsaild is already running.
> - xfsaild enters idle mode having met its current target.
>
> Once in the described state, xfs_ail_push() is invoked many
> more times with the already set threshold_lsn, but these calls
> do not lead to wake_up_process() calls because no further
> invocations result in moving the threshold_lsn forward. Add a
> flag to xfs_ail to capture whether an issued wake actually
> succeeds. If not, continue issuing wakes until we know one has
> been successful for the current target.
Hi Brian - here's kind of what I was thinking when we were talking
on IRC. basically we move all the idling logic into xfsaild() to
keep it out of xfsaild_push(), and make sure we only idle on an
empty AIL when we haven't raced with a target update.
So, I was thinking that we add a previous target variable to the
xfs_ail structure. Then xfsaild would become something like:
while (!kthread_should_stop()) {
spin_lock(&ailp->xa_lock);
__set_current_state(TASK_INTERRUPTIBLE);
/* barrier matches the xa_target update in xfs_ail_push() */
smp_rmb();
if (!xfs_ail_min(ailp) && ailp->xa_target == ailp->xa_prev_target) {
/* empty ail, not change to push target - idle */
spin_unlock(&ailp->xa_lock);
schedule();
tout = 0;
}
spin_unlock(&ailp->xa_lock);
if (tout) {
/* more work to do soon */
schedule_timeout(msecs_to_jiffies(tout));
}
__set_current_state(TASK_RUNNING);
try_to_freeze();
tout = xfsaild_push(ailp);
}
And in xfsaild_push(), move where we sample the push target to before the cursor
setup, and keep a snapshot of it:
/* barrier matches the xa_target update in xfs_ail_push() */
smp_rmb();
target = ailp->xa_target;
ailp->xa_prev_target = target;
This means we do not idle if a new push target was set while we were pushing,
even if we emptied the AIL (call it paranoia!).
We can avoid the returning of a zero timeout from xfsaild_push, too,
because the idling is not based on the state that we return from the
push. Hence we always will return a 10, 20 or 50ms timeout and we
can avoid complicating xfsaild_push logic with idling logic. i.e.
the logic that is there right now should not need modification...
Finally, rather than calling wake_up_process() in the
xfs_ail_push*() functions, call wake_up(&ailp->xa_idle); There can
only be one thread sleeping on that (the xfsaild) so there is no
need to use the wake_up_all() variant...
FWIW, you might be able to do this without the idle wait queue and
just use wake_up_process() -
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
