Re: [PATCH v3] xfs: re-enable xfsaild idle mode and fix associated races

[Brian Foster <bfoster@redhat.com>]

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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs