[RFC PATCH v3 0/2] xfs: fix xfsaild races and re-enable idle mode

[RFC PATCH v3 0/2] xfs: fix xfsaild races and re-enable idle mode

[Brian Foster]

Hi all,

We reproduced and debugged several hangs in a rhel6.3 kernel that
happened to still support xfsaild idle mode. Our short term fix was
to disable idle mode as in upstream, but I'd like to fire out a
couple potential fixes that allow us to re-enable idle mode, assuming
there aren't any other problems I'm not aware of. The details of the
bug are at:

https://bugzilla.redhat.com/show_bug.cgi?id=813137

... but I'll try to provide all relevant data in this post. The
reproducer is xfstests 273 running in a 100-iteration loop. I have
reproduced this hang on upstream kernels quite reliably with commit
670ce93f reverted. The performance enhancement in that commit makes
this much harder to reproduce.

With the proposed modifications, I've probably run 5+ 100-loop
iterations of test 273 without reproducing a hang. Previously, I
was able to reproduce the first hang with 100% reliability and the
second hang reproduced 10 minutes or so after starting a second
100-loop test (with the first fix applied).

I still have to run a full xfstests but the changes are small enough
that I wanted to send them out before I got too far. Thanks.

Changes since v2:
- Rebased against git://oss.sgi.com/xfs/xfs.
- Combined patches 1 and 2.
Changes since v1:
- Rebased against a pristine tree.

Brian Foster (2):
  xfs: re-enable xfsaild idle mode when the ail is empty
  xfs: fix xfsaild hang due to lost wake ups

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

-- 
1.7.7.6

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

[RFC PATCH v3 1/2] xfs: re-enable xfsaild idle mode when the ail is empty

[Brian Foster]

Running xfstests 273 in a loop on older kernels with xfsaild idle
mode support reproduces a lockup due to xfsaild entering idle mode
indefinitely. The following high-level sequence of events leads to
the hang:

- xfsaild is running, hits the stuck item threshold and reschedules,
  setting xa_last_pushed_lsn appropriately.
- xa_threshold is updated.
- xfsaild restarts from the previous xa_last_pushed_lsn, hits the
  new target and enters idle mode, even though the previously
  stuck items still populate the ail.

Modify the tout logic to only enter idle mode when the ail is empty.
IOW, if we hit the target but did not perform the current scan from
the start of the ail, reschedule at least one more time.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_trans_ail.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 9c51448..8a122d3 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -488,7 +488,10 @@ out_done:
 		 * longer for I/O to complete and remove pushed items from the
 		 * AIL before we start the next scan from the start of the AIL.
 		 */
-		tout = 50;
+		if (!count && !ailp->xa_last_pushed_lsn)
+			tout = 0;
+		else
+			tout = 50;
 		ailp->xa_last_pushed_lsn = 0;
 	} else if (((stuck + flushing) * 100) / count > 90) {
 		/*
-- 
1.7.7.6

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

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

[Brian Foster]

Running xfstests 273 in a loop reproduces an XFS lockup due to
xfsaild entering idle mode indefinitely. The following
high-level sequence of events leads to the hang:

- xfsaild is running with a cached target lsn
- xfs_ail_push() is invoked, updates ailp->xa_target_lsn and
  invokes wake_up_process(). wake_up_process() returns 0
  because xfsaild is already running.
- xfsaild enters idle mode having met its current target.

Once in the described state, xfs_ail_push() is invoked many
more times with the already set threshold_lsn, but these calls
do not lead to wake_up_process() calls because no further
invocations result in moving the threshold_lsn forward. Add a
flag to xfs_ail to capture whether an issued wake actually
succeeds. If not, continue issuing wakes until we know one has
been successful for the current target.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_trans_ail.c  |    4 ++--
 fs/xfs/xfs_trans_priv.h |    1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 8a122d3..8b49be8 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -564,7 +564,7 @@ xfs_ail_push(
 
 	lip = xfs_ail_min(ailp);
 	if (!lip || XFS_FORCED_SHUTDOWN(ailp->xa_mount) ||
-	    XFS_LSN_CMP(threshold_lsn, ailp->xa_target) <= 0)
+	    ((XFS_LSN_CMP(threshold_lsn, ailp->xa_target) <= 0) && !ailp->xa_pending_wake))
 		return;
 
 	/*
@@ -575,7 +575,7 @@ xfs_ail_push(
 	xfs_trans_ail_copy_lsn(ailp, &ailp->xa_target, &threshold_lsn);
 	smp_wmb();
 
-	wake_up_process(ailp->xa_task);
+	ailp->xa_pending_wake = !wake_up_process(ailp->xa_task);
 }
 
 /*
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index fb62377..688ef73 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -71,6 +71,7 @@ struct xfs_ail {
 	spinlock_t		xa_lock;
 	xfs_lsn_t		xa_last_pushed_lsn;
 	int			xa_log_flush;
+	int			xa_pending_wake;
 	struct list_head	xa_buf_list;
 	wait_queue_head_t	xa_empty;
 };
-- 
1.7.7.6

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

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

[Dave Chinner]

On Tue, May 22, 2012 at 12:38:34PM -0400, Brian Foster wrote:
> Running xfstests 273 in a loop reproduces an XFS lockup due to
> xfsaild entering idle mode indefinitely. The following
> high-level sequence of events leads to the hang:
> 
> - xfsaild is running with a cached target lsn
> - xfs_ail_push() is invoked, updates ailp->xa_target_lsn and
>   invokes wake_up_process(). wake_up_process() returns 0
>   because xfsaild is already running.
> - xfsaild enters idle mode having met its current target.
> 
> Once in the described state, xfs_ail_push() is invoked many
> more times with the already set threshold_lsn, but these calls
> do not lead to wake_up_process() calls because no further
> invocations result in moving the threshold_lsn forward. Add a
> flag to xfs_ail to capture whether an issued wake actually
> succeeds. If not, continue issuing wakes until we know one has
> been successful for the current target.

Hi Brian - here's kind of what I was thinking when we were talking
on IRC. basically we move all the idling logic into xfsaild() to
keep it out of xfsaild_push(), and make sure we only idle on an
empty AIL when we haven't raced with a target update.

So, I was thinking that we add a previous target variable to the
xfs_ail structure. Then xfsaild would become something like:


	while (!kthread_should_stop()) {

		spin_lock(&ailp->xa_lock);
		__set_current_state(TASK_INTERRUPTIBLE);

		/* barrier matches the xa_target update in xfs_ail_push() */
		smp_rmb();
		if (!xfs_ail_min(ailp) && ailp->xa_target == ailp->xa_prev_target) {
			/* empty ail, not change to push target - idle */
			spin_unlock(&ailp->xa_lock);
			schedule();
			tout = 0;
		}
		spin_unlock(&ailp->xa_lock);

		if (tout) {
			/* more work to do soon */
			schedule_timeout(msecs_to_jiffies(tout));
		}
		__set_current_state(TASK_RUNNING);

		try_to_freeze();

		tout = xfsaild_push(ailp);
	}

And in xfsaild_push(), move where we sample the push target to before the cursor
setup, and keep a snapshot of it:

	/* barrier matches the xa_target update in xfs_ail_push() */
	smp_rmb();
	target = ailp->xa_target;
	ailp->xa_prev_target = target;

This means we do not idle if a new push target was set while we were pushing,
even if we emptied the AIL (call it paranoia!).

We can avoid the returning of a zero timeout from xfsaild_push, too,
because the idling is not based on the state that we return from the
push. Hence we always will return a 10, 20 or 50ms timeout and we
can avoid complicating xfsaild_push logic with idling logic. i.e.
the logic that is there right now should not need modification...

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() - 

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Brian Foster]

On 05/22/2012 08:58 PM, Dave Chinner wrote:
snip

> 
> Hi Brian - here's kind of what I was thinking when we were talking
> on IRC. basically we move all the idling logic into xfsaild() to
> keep it out of xfsaild_push(), and make sure we only idle on an
> empty AIL when we haven't raced with a target update.
> 
> So, I was thinking that we add a previous target variable to the
> xfs_ail structure. Then xfsaild would become something like:
> 
> 
> 	while (!kthread_should_stop()) {
> 
> 		spin_lock(&ailp->xa_lock);
> 		__set_current_state(TASK_INTERRUPTIBLE);
> 
> 		/* barrier matches the xa_target update in xfs_ail_push() */
> 		smp_rmb();
> 		if (!xfs_ail_min(ailp) && ailp->xa_target == ailp->xa_prev_target) {

Ok... IIUC, two things can happen here: 1.) we either detect an xa_target update and continue on or 2.) if an _ail_push() occurs any time between now and when we schedule out, it will issue the wakeup successfully because we've already set the task state above (thus avoiding the race).

> 			/* empty ail, not change to push target - idle */
> 			spin_unlock(&ailp->xa_lock);
> 			schedule();
> 			tout = 0;
> 		}
> 		spin_unlock(&ailp->xa_lock);
> 
> 		if (tout) {
> 			/* more work to do soon */
> 			schedule_timeout(msecs_to_jiffies(tout));
> 		}
> 		__set_current_state(TASK_RUNNING);
> 
> 		try_to_freeze();
> 
> 		tout = xfsaild_push(ailp);
> 	}
> 
> And in xfsaild_push(), move where we sample the push target to before the cursor
> setup, and keep a snapshot of it:
> 
> 	/* barrier matches the xa_target update in xfs_ail_push() */
> 	smp_rmb();
> 	target = ailp->xa_target;
> 	ailp->xa_prev_target = target;
> 

The rest is pretty clear...

> This means we do not idle if a new push target was set while we were pushing,
> even if we emptied the AIL (call it paranoia!).
> 

Sounds reasonable. It looks like the only place we update the push target corresponds to a wake anyway, so this is probably not a departure from intended behavior.

> We can avoid the returning of a zero timeout from xfsaild_push, too,
> because the idling is not based on the state that we return from the
> push. Hence we always will return a 10, 20 or 50ms timeout and we
> can avoid complicating xfsaild_push logic with idling logic. i.e.
> the logic that is there right now should not need modification...
> 
> 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() - 
> 

Ok... I'll look into using a wait queue once I have the basics working as is and put the whole thing through my reproducer. Thanks again!

Brian

> Cheers,
> 
> Dave.

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

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

[Brian Foster]

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:

	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);
		}

		finish_wait(&ailp->xa_idle, &wait);

		try_to_freeze();

		tout = xfsaild_push(ailp);
	}

... and obviously the xfs_ail_push() side changes to:

	wake_up(&ailp->xa_idle);

Does this wait_queue version look sane to you? Thanks again..

Brian

> Cheers,
> 
> Dave.

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

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

[Mark Tinguely]

On 05/23/12 12:48, 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:
>
...

FYI. Test 273 in a loop will still cause the sync_worker to lock when it 
tries to allocate a dummy transaction.

PID: 29214  TASK: ffff8807e66404c0  CPU: 1   COMMAND: "kworker/1:15"
  #0 [ffff88081f551b60] __schedule at ffffffff814175d0
  #1 [ffff88081f551ca8] schedule at ffffffff81417944
  #2 [ffff88081f551cb8] xlog_grant_head_wait at ffffffffa055a6d5 [xfs]
  #3 [ffff88081f551d08] xlog_grant_head_check at ffffffffa055a856 [xfs]
  #4 [ffff88081f551d48] xfs_log_reserve at ffffffffa055a95f [xfs]
  #5 [ffff88081f551d88] xfs_trans_reserve at ffffffffa0557ee4 [xfs]
  #6 [ffff88081f551dd8] xfs_fs_log_dummy at ffffffffa050cf88 [xfs]
  #7 [ffff88081f551df8] xfs_sync_worker at ffffffffa0518454 [xfs]
  #8 [ffff88081f551e18] process_one_work at ffffffff810564ad
  #9 [ffff88081f551e68] worker_thread at ffffffff81059203
#10 [ffff88081f551ee8] kthread at ffffffff8105dd2e
#11 [ffff88081f551f48] kernel_thread_helper at ffffffff81421a64

I understand why the dummy transaction was added and I think we can 
anticipate the hang before it happens and avoid it.


--Mark T.

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

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

[Brian Foster]

On 05/23/2012 02:19 PM, Mark Tinguely wrote:
> On 05/23/12 12:48, 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:
>>
> ...
> 
> FYI. Test 273 in a loop will still cause the sync_worker to lock when it tries to allocate a dummy transaction.
> 

Interesting, I don't think I've seen this one in my testing. To be clear, are you testing the xfs tree with both of the v2 patches? My testing has been focused on a slightly modified upstream tree because it's known to fail. I'll have to make a point to test the xfs tree as well. How long did this run before failing?

> PID: 29214  TASK: ffff8807e66404c0  CPU: 1   COMMAND: "kworker/1:15"
>  #0 [ffff88081f551b60] __schedule at ffffffff814175d0
>  #1 [ffff88081f551ca8] schedule at ffffffff81417944
>  #2 [ffff88081f551cb8] xlog_grant_head_wait at ffffffffa055a6d5 [xfs]
>  #3 [ffff88081f551d08] xlog_grant_head_check at ffffffffa055a856 [xfs]
>  #4 [ffff88081f551d48] xfs_log_reserve at ffffffffa055a95f [xfs]
>  #5 [ffff88081f551d88] xfs_trans_reserve at ffffffffa0557ee4 [xfs]
>  #6 [ffff88081f551dd8] xfs_fs_log_dummy at ffffffffa050cf88 [xfs]
>  #7 [ffff88081f551df8] xfs_sync_worker at ffffffffa0518454 [xfs]
>  #8 [ffff88081f551e18] process_one_work at ffffffff810564ad
>  #9 [ffff88081f551e68] worker_thread at ffffffff81059203
> #10 [ffff88081f551ee8] kthread at ffffffff8105dd2e
> #11 [ffff88081f551f48] kernel_thread_helper at ffffffff81421a64
> 
> I understand why the dummy transaction was added and I think we can anticipate the hang before it happens and avoid it.
> 

I'm not familiar with what the dummy transaction is for... but I also wonder whether Dave's improvement to make xfsaild smarter about going into idle (as opposed to my original approach of trying to avoid the race on the wake side) would catch this. 

Brian

> 
> --Mark T.

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

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

[Dave Chinner]

On Wed, May 23, 2012 at 01:19:31PM -0500, Mark Tinguely wrote:
> On 05/23/12 12:48, 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:
> >
> ...
> 
> FYI. Test 273 in a loop will still cause the sync_worker to lock
> when it tries to allocate a dummy transaction.
> 
> PID: 29214  TASK: ffff8807e66404c0  CPU: 1   COMMAND: "kworker/1:15"
>  #0 [ffff88081f551b60] __schedule at ffffffff814175d0
>  #1 [ffff88081f551ca8] schedule at ffffffff81417944
>  #2 [ffff88081f551cb8] xlog_grant_head_wait at ffffffffa055a6d5 [xfs]
>  #3 [ffff88081f551d08] xlog_grant_head_check at ffffffffa055a856 [xfs]
>  #4 [ffff88081f551d48] xfs_log_reserve at ffffffffa055a95f [xfs]
>  #5 [ffff88081f551d88] xfs_trans_reserve at ffffffffa0557ee4 [xfs]
>  #6 [ffff88081f551dd8] xfs_fs_log_dummy at ffffffffa050cf88 [xfs]
>  #7 [ffff88081f551df8] xfs_sync_worker at ffffffffa0518454 [xfs]
>  #8 [ffff88081f551e18] process_one_work at ffffffff810564ad
>  #9 [ffff88081f551e68] worker_thread at ffffffff81059203
> #10 [ffff88081f551ee8] kthread at ffffffff8105dd2e
> #11 [ffff88081f551f48] kernel_thread_helper at ffffffff81421a64
> 
> I understand why the dummy transaction was added and I think we can
> anticipate the hang before it happens and avoid it.

I don't think this hang has anything to do with the idle patches -
it is most likely related to the CIL stall we are chasing down.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Dave Chinner]

[ Brian, can you line wrap your text at 72 columns? ]

On Wed, May 23, 2012 at 09:05:05AM -0400, Brian Foster wrote:
> On 05/22/2012 08:58 PM, Dave Chinner wrote:
> snip
> 
> > 
> > Hi Brian - here's kind of what I was thinking when we were talking
> > on IRC. basically we move all the idling logic into xfsaild() to
> > keep it out of xfsaild_push(), and make sure we only idle on an
> > empty AIL when we haven't raced with a target update.
> > 
> > So, I was thinking that we add a previous target variable to the
> > xfs_ail structure. Then xfsaild would become something like:
> > 
> > 
> > 	while (!kthread_should_stop()) {
> > 
> > 		spin_lock(&ailp->xa_lock);
> > 		__set_current_state(TASK_INTERRUPTIBLE);
> > 
> > 		/* barrier matches the xa_target update in xfs_ail_push() */
> > 		smp_rmb();
> > 		if (!xfs_ail_min(ailp) && ailp->xa_target == ailp->xa_prev_target) {
> 
> Ok... IIUC, two things can happen here: 1.) we either detect an
> xa_target update and continue on or 2.) if an _ail_push() occurs
> any time between now and when we schedule out, it will issue the
> wakeup successfully because we've already set the task state above
> (thus avoiding the race).

Exactly.

> > FWIW, you might be able to do this without the idle wait queue
> > and just use wake_up_process() - 
> > 
> 
> Ok... I'll look into using a wait queue once I have the basics
> working as is and put the whole thing through my reproducer.

Ah, I forgot to remove that line from the email before I sent it. I
originally thought an idle wake queue would be necessary, but then
realised it wasn't and removed it from the code I wrote
above. So, no, and idle wait queue is not necessary....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Dave Chinner]

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.

> 	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:

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Brian Foster]

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

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

[Mark Tinguely]

On 05/23/12 18:53, Dave Chinner wrote:
> On Wed, May 23, 2012 at 01:19:31PM -0500, Mark Tinguely wrote:
>> On 05/23/12 12:48, 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:
>>>
>> ...
>>
>> FYI. Test 273 in a loop will still cause the sync_worker to lock
>> when it tries to allocate a dummy transaction.
>>
>> PID: 29214  TASK: ffff8807e66404c0  CPU: 1   COMMAND: "kworker/1:15"
>>   #0 [ffff88081f551b60] __schedule at ffffffff814175d0
>>   #1 [ffff88081f551ca8] schedule at ffffffff81417944
>>   #2 [ffff88081f551cb8] xlog_grant_head_wait at ffffffffa055a6d5 [xfs]
>>   #3 [ffff88081f551d08] xlog_grant_head_check at ffffffffa055a856 [xfs]
>>   #4 [ffff88081f551d48] xfs_log_reserve at ffffffffa055a95f [xfs]
>>   #5 [ffff88081f551d88] xfs_trans_reserve at ffffffffa0557ee4 [xfs]
>>   #6 [ffff88081f551dd8] xfs_fs_log_dummy at ffffffffa050cf88 [xfs]
>>   #7 [ffff88081f551df8] xfs_sync_worker at ffffffffa0518454 [xfs]
>>   #8 [ffff88081f551e18] process_one_work at ffffffff810564ad
>>   #9 [ffff88081f551e68] worker_thread at ffffffff81059203
>> #10 [ffff88081f551ee8] kthread at ffffffff8105dd2e
>> #11 [ffff88081f551f48] kernel_thread_helper at ffffffff81421a64
>>
>> I understand why the dummy transaction was added and I think we can
>> anticipate the hang before it happens and avoid it.
>
> I don't think this hang has anything to do with the idle patches -
> it is most likely related to the CIL stall we are chasing down.
>
> Cheers,
>
> Dave.

Correct, this problem is not caused nor can be corrected by the idle
patches. See thread:

  Subject: Still seeing hangs in xlog_grant_log_space

Brian, the FYI is just a warning that your replicator of running XFS
test 173 in a loop is triggering dummy ticket allocation stalls in the
sync_worker. Most of the time, they are quickly given space, but 
eventually things will line up and XFS will lock up.

It took me over 200 iterations of test 173 to get the above lock up,
and yes your v2 patches were in code, but that does not matter.

I did not want you to mistake a sync_worker lock up as being caused by
your code.

--Mark.

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