xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v3] xfs: re-enable xfsaild idle mode and fix associated races
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Mon, 02 Jul 2012 09:51:08 -0400
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20120702082952.GR19223@dastard>
References: <1340880776-45730-1-git-send-email-bfoster@xxxxxxxxxx> <20120702070502.GA25568@xxxxxxxxxxxxx> <20120702082952.GR19223@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0
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.
> 


<Prev in Thread] Current Thread [Next in Thread>