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

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

[Brian Foster]

xfsaild idle mode logic currently leads to a couple hangs:

1.) If xfsaild is rescheduled in during an incremental scan
    (i.e., tout != 0) and the target has been updated since
    the previous run, we can hit the new target and go into
    idle mode with a still populated ail.
2.) A wake up is only issued when the target is pushed forward.
    The wake up can race with xfsaild if it is currently in the
    process of entering idle mode, causing future wake up
    events to be lost.

These hangs have been reproduced and verified as fixed by
running xfstests 273 in a loop on a slightly modified upstream
kernel. The kernel is modified to re-enable idle mode as
previously implemented (when count == 0) and with a revert of
commit 670ce93f, which includes performance improvements that
make this harder to reproduce.

The solution, the algorithm for which has been outlined by
Dave Chinner, is to modify xfsaild to enter idle mode only when
the ail is empty and the push target has not been moved forward
since the last push.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
Hi,

I haven't heard any further comments on v2 with regard to the
sleep logic being located in xfsaild(), so I've left it as is
and fixed a couple style issues for v3.

Brian

v3:
- Fix a couple style issues and elaborate on the idle mode
  comment based on Christoph Hellwig's review.
v2:
- Fixed up comments in xfsaild() based on Dave Chinner's
  review.

 fs/xfs/xfs_trans_ail.c  |   35 ++++++++++++++++++++++++++++++++---
 fs/xfs/xfs_trans_priv.h |    1 +
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 9c51448..6011ee6 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -383,6 +383,12 @@ xfsaild_push(
 	}
 
 	spin_lock(&ailp->xa_lock);
+
+	/* barrier matches the xa_target update in xfs_ail_push() */
+	smp_rmb();
+	target = ailp->xa_target;
+	ailp->xa_target_prev = target;
+
 	lip = xfs_trans_ail_cursor_first(ailp, &cur, ailp->xa_last_pushed_lsn);
 	if (!lip) {
 		/*
@@ -397,7 +403,6 @@ xfsaild_push(
 	XFS_STATS_INC(xs_push_ail);
 
 	lsn = lip->li_lsn;
-	target = ailp->xa_target;
 	while ((XFS_LSN_CMP(lip->li_lsn, target) <= 0)) {
 		int	lock_result;
 
@@ -527,8 +532,32 @@ xfsaild(
 			__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;
+		}
+		spin_unlock(&ailp->xa_lock);
+
+		if (tout)
+			schedule_timeout(msecs_to_jiffies(tout));
+
+		__set_current_state(TASK_RUNNING);
 
 		try_to_freeze();
 
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index fb62377..53b7c9b 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -67,6 +67,7 @@ struct xfs_ail {
 	struct task_struct	*xa_task;
 	struct list_head	xa_ail;
 	xfs_lsn_t		xa_target;
+	xfs_lsn_t		xa_target_prev;
 	struct list_head	xa_cursors;
 	spinlock_t		xa_lock;
 	xfs_lsn_t		xa_last_pushed_lsn;
-- 
1.7.7.6

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

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

[Dave Chinner]

On Thu, Jun 28, 2012 at 06:52:56AM -0400, Brian Foster wrote:
> xfsaild idle mode logic currently leads to a couple hangs:
> 
> 1.) If xfsaild is rescheduled in during an incremental scan
>     (i.e., tout != 0) and the target has been updated since
>     the previous run, we can hit the new target and go into
>     idle mode with a still populated ail.
> 2.) A wake up is only issued when the target is pushed forward.
>     The wake up can race with xfsaild if it is currently in the
>     process of entering idle mode, causing future wake up
>     events to be lost.
> 
> These hangs have been reproduced and verified as fixed by
> running xfstests 273 in a loop on a slightly modified upstream
> kernel. The kernel is modified to re-enable idle mode as
> previously implemented (when count == 0) and with a revert of
> commit 670ce93f, which includes performance improvements that
> make this harder to reproduce.
> 
> The solution, the algorithm for which has been outlined by
> Dave Chinner, is to modify xfsaild to enter idle mode only when
> the ail is empty and the push target has not been moved forward
> since the last push.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks OK to me, and hasn't caused any problems here.

Final question - did you confirm with powertop that the xfsaild is
no longer causing wakeups a minute or two after you stop writing to
the filesystem? (I haven't yet)

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

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

[Brian Foster]

On 07/01/2012 08:07 PM, Dave Chinner wrote:
> On Thu, Jun 28, 2012 at 06:52:56AM -0400, Brian Foster wrote:
>> xfsaild idle mode logic currently leads to a couple hangs:
>>
>> 1.) If xfsaild is rescheduled in during an incremental scan
>>     (i.e., tout != 0) and the target has been updated since
>>     the previous run, we can hit the new target and go into
>>     idle mode with a still populated ail.
>> 2.) A wake up is only issued when the target is pushed forward.
>>     The wake up can race with xfsaild if it is currently in the
>>     process of entering idle mode, causing future wake up
>>     events to be lost.
>>
>> These hangs have been reproduced and verified as fixed by
>> running xfstests 273 in a loop on a slightly modified upstream
>> kernel. The kernel is modified to re-enable idle mode as
>> previously implemented (when count == 0) and with a revert of
>> commit 670ce93f, which includes performance improvements that
>> make this harder to reproduce.
>>
>> The solution, the algorithm for which has been outlined by
>> Dave Chinner, is to modify xfsaild to enter idle mode only when
>> the ail is empty and the push target has not been moved forward
>> since the last push.
>>
>> Signed-off-by: Brian Foster <bfoster@redhat.com>
> 
> Looks OK to me, and hasn't caused any problems here.
> 
> Final question - did you confirm with powertop that the xfsaild is
> no longer causing wakeups a minute or two after you stop writing to
> the filesystem? (I haven't yet)
> 

I hadn't tested with powertop, but I had some tracepoints hacked in
around the idle/wake cases to verify the thread was actually scheduling
out. FWIW, I just gave powertop a quick test and it appears to work as
expected...

With current upstream on my rhel6.3 VM, I see the following after
running a 'touch /mnt/file;sync' and letting the fs idle for a bit:

   0.5% ( 19.9)      xfsaild/vdb1 : xfsaild (process_timeout)

and this drops off completely with the patch applied. Thanks for the tip.

Brian

> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 


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

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

[Dave Chinner]

On Mon, Jul 02, 2012 at 09:33:24AM -0400, Brian Foster wrote:
> On 07/01/2012 08:07 PM, Dave Chinner wrote:
> > On Thu, Jun 28, 2012 at 06:52:56AM -0400, Brian Foster wrote:
> >> xfsaild idle mode logic currently leads to a couple hangs:
> >>
> >> 1.) If xfsaild is rescheduled in during an incremental scan
> >>     (i.e., tout != 0) and the target has been updated since
> >>     the previous run, we can hit the new target and go into
> >>     idle mode with a still populated ail.
> >> 2.) A wake up is only issued when the target is pushed forward.
> >>     The wake up can race with xfsaild if it is currently in the
> >>     process of entering idle mode, causing future wake up
> >>     events to be lost.
> >>
> >> These hangs have been reproduced and verified as fixed by
> >> running xfstests 273 in a loop on a slightly modified upstream
> >> kernel. The kernel is modified to re-enable idle mode as
> >> previously implemented (when count == 0) and with a revert of
> >> commit 670ce93f, which includes performance improvements that
> >> make this harder to reproduce.
> >>
> >> The solution, the algorithm for which has been outlined by
> >> Dave Chinner, is to modify xfsaild to enter idle mode only when
> >> the ail is empty and the push target has not been moved forward
> >> since the last push.
> >>
> >> Signed-off-by: Brian Foster <bfoster@redhat.com>
> > 
> > Looks OK to me, and hasn't caused any problems here.
> > 
> > Final question - did you confirm with powertop that the xfsaild is
> > no longer causing wakeups a minute or two after you stop writing to
> > the filesystem? (I haven't yet)
> > 
> 
> I hadn't tested with powertop, but I had some tracepoints hacked in
> around the idle/wake cases to verify the thread was actually scheduling
> out.

If you've added tracepoints that were useful for
debugging/verification, then send that as a patch as well. If users
have trouble then simply asking them for event traces is very easy
to do and gives us much better insight into what is happening....

You can't have enough tracepoints when things are going wrong ;)

> FWIW, I just gave powertop a quick test and it appears to work as
> expected...
> 
> With current upstream on my rhel6.3 VM, I see the following after
> running a 'touch /mnt/file;sync' and letting the fs idle for a bit:
> 
>    0.5% ( 19.9)      xfsaild/vdb1 : xfsaild (process_timeout)
> 
> and this drops off completely with the patch applied. Thanks for the tip.

Great, then it is working exactly as expected.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Brian Foster]

On 07/02/2012 07:51 PM, Dave Chinner wrote:
> On Mon, Jul 02, 2012 at 09:33:24AM -0400, Brian Foster wrote:
>> On 07/01/2012 08:07 PM, Dave Chinner wrote:
>>> On Thu, Jun 28, 2012 at 06:52:56AM -0400, Brian Foster wrote:
>>>> xfsaild idle mode logic currently leads to a couple hangs:
>>>>
>>>> 1.) If xfsaild is rescheduled in during an incremental scan
>>>>     (i.e., tout != 0) and the target has been updated since
>>>>     the previous run, we can hit the new target and go into
>>>>     idle mode with a still populated ail.
>>>> 2.) A wake up is only issued when the target is pushed forward.
>>>>     The wake up can race with xfsaild if it is currently in the
>>>>     process of entering idle mode, causing future wake up
>>>>     events to be lost.
>>>>
>>>> These hangs have been reproduced and verified as fixed by
>>>> running xfstests 273 in a loop on a slightly modified upstream
>>>> kernel. The kernel is modified to re-enable idle mode as
>>>> previously implemented (when count == 0) and with a revert of
>>>> commit 670ce93f, which includes performance improvements that
>>>> make this harder to reproduce.
>>>>
>>>> The solution, the algorithm for which has been outlined by
>>>> Dave Chinner, is to modify xfsaild to enter idle mode only when
>>>> the ail is empty and the push target has not been moved forward
>>>> since the last push.
>>>>
>>>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>>>
>>> Looks OK to me, and hasn't caused any problems here.
>>>
>>> Final question - did you confirm with powertop that the xfsaild is
>>> no longer causing wakeups a minute or two after you stop writing to
>>> the filesystem? (I haven't yet)
>>>
>>
>> I hadn't tested with powertop, but I had some tracepoints hacked in
>> around the idle/wake cases to verify the thread was actually scheduling
>> out.
> 
> If you've added tracepoints that were useful for
> debugging/verification, then send that as a patch as well. If users
> have trouble then simply asking them for event traces is very easy
> to do and gives us much better insight into what is happening....
> 
> You can't have enough tracepoints when things are going wrong ;)
> 

Ok, duly noted. What I have right now is scattered about a few branches
and not immediately presentable. When I get some time I'll fix them up
and post. If I remember correctly, I had covered: xfsaild end (count,
skip, target, etc.), xfsaild idle, xa_target update (xfs_ail_push()) and
xfsaild wake (which might be extraneous at this point).

Brian

>> FWIW, I just gave powertop a quick test and it appears to work as
>> expected...
>>
>> With current upstream on my rhel6.3 VM, I see the following after
>> running a 'touch /mnt/file;sync' and letting the fs idle for a bit:
>>
>>    0.5% ( 19.9)      xfsaild/vdb1 : xfsaild (process_timeout)
>>
>> and this drops off completely with the patch applied. Thanks for the tip.
> 
> Great, then it is working exactly as expected.
> 
> Cheers,
> 
> Dave.
> 


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

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

[Christoph Hellwig]

Ok, let's go with Brian's current version.  I'll see if we can make it
look nicer later on.

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

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

[Christoph Hellwig]

>  			__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.

That doesn't mean I don't like the algorithm behind this patch, it just
needs to move into the right place.

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

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

[Dave Chinner]

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@fromorbit.com

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

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

[Brian Foster]

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

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

[Christoph Hellwig]

On Mon, Jul 02, 2012 at 09:51:08AM -0400, Brian Foster wrote:
> 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...

Let's take the patch as-is for 3.6 as we have enough other work in that
area queued.  We can clean it up later if needed.

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

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

[Christoph Hellwig]

On Tue, Jul 17, 2012 at 03:00:00AM -0400, Christoph Hellwig wrote:
> Let's take the patch as-is for 3.6 as we have enough other work in that
> area queued.  We can clean it up later if needed.

Ben, any reason this never got picked up?

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

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

[Ben Myers]

On Tue, Jul 24, 2012 at 09:22:46AM -0400, Christoph Hellwig wrote:
> On Tue, Jul 17, 2012 at 03:00:00AM -0400, Christoph Hellwig wrote:
> > Let's take the patch as-is for 3.6 as we have enough other work in that
> > area queued.  We can clean it up later if needed.
> 
> Ben, any reason this never got picked up?

A lost sheep!  I am planning on pulling this in for 3.6 just as you suggested.

-Ben

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