On Mon, Jul 02, 2012 at 03:05:02AM -0400, Christoph Hellwig wrote:
> > __set_current_state(TASK_KILLABLE);
> > else
> > __set_current_state(TASK_INTERRUPTIBLE);
> > - schedule_timeout(tout ?
> > - msecs_to_jiffies(tout) : MAX_SCHEDULE_TIMEOUT);
> > +
> > + spin_lock(&ailp->xa_lock);
> > +
> > + /*
> > + * Idle if the AIL is empty and we are not racing with a target
> > + * update. We check the AIL after we set the task to a sleep
> > + * state to guarantee that we either catch an xa_target update
> > + * or that a wake_up resets the state to TASK_RUNNING.
> > + * Otherwise, we run the risk of sleeping indefinitely.
> > + *
> > + * The barrier matches the xa_target update in xfs_ail_push().
> > + */
> > + smp_rmb();
> > + if (!xfs_ail_min(ailp) &&
> > + ailp->xa_target == ailp->xa_target_prev) {
> > + spin_unlock(&ailp->xa_lock);
> > + schedule();
> > + tout = 0;
> > + continue;
> > + }
>
> I still don't like this at all all - we have one place to do all the
> timeout decisions, and that is and then end of xfsaild_push. Splitting
> this decision over two functions makes the code a lot harder to
> understand and maintain over the long run.
The timeout decision is separate to idling, though - the idle check
has to be done when we are already in
TASK_INTERRUPTIBLE/TASK_KILLABLE state. If we do the check before
changing the task state, we can miss wakeups when the target is
changed between the "are we really idle" check and the schedule()
call because the wakeup is ignored if the task is still in the
running state.
> That doesn't mean I don't like the algorithm behind this patch, it just
> needs to move into the right place.
I'm not sure it can be moved into xfsaild_push and still be nice and
clean because of the above requirement...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
