Re: [RFC PATCH v3 2/2] xfs: fix xfsaild hang due to lost wake ups

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs