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

[Brian Foster <bfoster@redhat.com>]

On 05/23/2012 08:06 PM, Dave Chinner wrote:
> On Wed, May 23, 2012 at 01:48:54PM -0400, Brian Foster wrote:
>> On 05/22/2012 08:58 PM, Dave Chinner wrote: snip
>>>
>>> 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() - 
>>>
>>
>> Hi Dave,
>>
>> I have a working version of your suggested algorithm. It looks
>> mostly the same with the exception of a spin_unlock fix. I also
>> have the below version that uses a wait_queue and that I plan to
>> test overnight tonight:
> 
> See my previous mail about using an idle queue.
> 

Ok, I was a bit curious why you suggested that, but I figured it was for
aesthetic or consistency reasons. ;) No problem.

>> 	while (!kthread_should_stop()) {
>> 		if (tout && tout <= 20)
>> 			state = TASK_KILLABLE;
>> 		else
>> 			state = TASK_INTERRUPTIBLE;
>>
>> 		prepare_to_wait(&ailp->xa_idle, &wait, state);
>>
>> 		spin_lock(&ailp->xa_lock);
>> 		/* barrier matches the xa_target update in xfs_ail_push() */
>> 		smp_rmb();
>> 		if (!xfs_ail_min(ailp) && (ailp->xa_target == ailp->xa_target_prev)) {
>> 			/* the ail is empty and no change to the push target - idle */
>> 			spin_unlock(&ailp->xa_lock);
>> 			schedule();
>> 		} else if (tout) {
>> 			spin_unlock(&ailp->xa_lock);
>> 			/* more work to do soon */
>> 			schedule_timeout(msecs_to_jiffies(tout));
>> 		} else {
>> 			spin_unlock(&ailp->xa_lock);
>> 		}
> 
> Three separate unlocks? that's a recipe for future disasters. how
> about:
> 

FWIW, I started off with two just to fix the double unlock on return
from idle mode, then rearranged that for some reason when I added the
idle queue.

> 		if (!xfs_ail_min(ailp) && (ailp->xa_target == ailp->xa_target_prev)) {
> 			/* the ail is empty and no change to the push target - idle */
> 			spin_unlock(&ailp->xa_lock);
> 			schedule();
> 			tout = 0;
> 			continue;
> 		}
> 		spin_unlock(&ailp->xa_lock);
> 
> 		if (tout) {
> 			/* more work to do soon */
> 			schedule_timeout(msecs_to_jiffies(tout));
> 		}
> 
> So that we recheck the idle condition on wakeup from idle before
> doing anything. (i.e. handle spurious idle wakeups effectively). By
> setting the tout to zero, we then fall through immediately to
> pushing the AIL if it was a real wakeup that moved the target....
> 

That sounds good to me. Thanks again.

Brian

> Cheers,
> 
> Dave.

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