On 07/02/2012 04:29 AM, Dave Chinner wrote:
> 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...
>
Right... if we wanted to move this back into xfsaild_push(), the only
way I can see doing that correctly is to move the task state logic down
into that function as well, at which point the idle logic is now spread
across two functions. :/
Considering this patch introduces an independent check for the idle
logic from the timeout logic (i.e., we use xfs_ail_min() now instead of
the general scan state of xfsaild_push()), I personally find the
separation of idle from timeout to be a bit more clear, but of course
I'll try to implement whatever is most agreeable...
Brian
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
