[patch] Prevent excessive xfsaild wakeups

[patch] Prevent excessive xfsaild wakeups

[David Chinner]

Idle state is not being detected properly by the xfsaild push
code. The current idle state is detected by an empty list
which may never happen with mostly idle filesystem or one
using lazy superblock counters. A single dirty item in the
list will result repeated looping to push everything past
the target when everything because it fails to check if we
managed to push anything.

Fix by considering a dirty list with everything past the target
as an idle state and set the timeout appropriately.

Signed-off-by: Dave Chinner <dgc@sgi.com>
---
 fs/xfs/xfs_trans_ail.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_trans_ail.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_trans_ail.c	2008-02-18 09:14:34.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_trans_ail.c	2008-02-18 09:18:52.070682570 +1100
@@ -261,14 +261,17 @@ xfsaild_push(
 		xfs_log_force(mp, (xfs_lsn_t)0, XFS_LOG_FORCE);
 	}
 
-	/*
-	 * We reached the target so wait a bit 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.
-	 */
-	if ((XFS_LSN_CMP(lsn, target) >= 0)) {
+	if (count && (XFS_LSN_CMP(lsn, target) >= 0)) {
+		/*
+		 * We reached the target so wait a bit 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 += 20;
 		last_pushed_lsn = 0;
+	} else if (!count) {
+		/* We're past our target or empty, so idle */
+		tout = 1000;
 	} else if ((restarts > XFS_TRANS_PUSH_AIL_RESTARTS) ||
 		   (count && ((stuck * 100) / count > 90))) {
 		/*

Re: [patch] Prevent excessive xfsaild wakeups

[David Chinner]

ping?

This is needed for 2.6.25-rcX....

On Tue, Feb 19, 2008 at 09:59:06AM +1100, David Chinner wrote:
> Idle state is not being detected properly by the xfsaild push
> code. The current idle state is detected by an empty list
> which may never happen with mostly idle filesystem or one
> using lazy superblock counters. A single dirty item in the
> list will result repeated looping to push everything past
> the target when everything because it fails to check if we
> managed to push anything.
> 
> Fix by considering a dirty list with everything past the target
> as an idle state and set the timeout appropriately.
> 
> Signed-off-by: Dave Chinner <dgc@sgi.com>
> ---
>  fs/xfs/xfs_trans_ail.c |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> Index: 2.6.x-xfs-new/fs/xfs/xfs_trans_ail.c
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_trans_ail.c	2008-02-18 09:14:34.000000000 +1100
> +++ 2.6.x-xfs-new/fs/xfs/xfs_trans_ail.c	2008-02-18 09:18:52.070682570 +1100
> @@ -261,14 +261,17 @@ xfsaild_push(
>  		xfs_log_force(mp, (xfs_lsn_t)0, XFS_LOG_FORCE);
>  	}
>  
> -	/*
> -	 * We reached the target so wait a bit 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.
> -	 */
> -	if ((XFS_LSN_CMP(lsn, target) >= 0)) {
> +	if (count && (XFS_LSN_CMP(lsn, target) >= 0)) {
> +		/*
> +		 * We reached the target so wait a bit 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 += 20;
>  		last_pushed_lsn = 0;
> +	} else if (!count) {
> +		/* We're past our target or empty, so idle */
> +		tout = 1000;
>  	} else if ((restarts > XFS_TRANS_PUSH_AIL_RESTARTS) ||
>  		   (count && ((stuck * 100) / count > 90))) {
>  		/*

-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [patch] Prevent excessive xfsaild wakeups

[Eric Sandeen]

David Chinner wrote:
> ping?
> 
> This is needed for 2.6.25-rcX....
> 
> On Tue, Feb 19, 2008 at 09:59:06AM +1100, David Chinner wrote:
>> Idle state is not being detected properly by the xfsaild push
>> code. The current idle state is detected by an empty list
>> which may never happen with mostly idle filesystem or one
>> using lazy superblock counters. A single dirty item in the
>> list will result repeated looping to push everything past
>> the target when everything because it fails to check if we
>> managed to push anything.
>>
>> Fix by considering a dirty list with everything past the target
>> as an idle state and set the timeout appropriately.

Seems good to me.

Acked-by: Eric Sandeen <sandeen@sandeen.net>

-Eric

>> Signed-off-by: Dave Chinner <dgc@sgi.com>
>> ---
>>  fs/xfs/xfs_trans_ail.c |   15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> Index: 2.6.x-xfs-new/fs/xfs/xfs_trans_ail.c
>> ===================================================================
>> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_trans_ail.c	2008-02-18 09:14:34.000000000 +1100
>> +++ 2.6.x-xfs-new/fs/xfs/xfs_trans_ail.c	2008-02-18 09:18:52.070682570 +1100
>> @@ -261,14 +261,17 @@ xfsaild_push(
>>  		xfs_log_force(mp, (xfs_lsn_t)0, XFS_LOG_FORCE);
>>  	}
>>  
>> -	/*
>> -	 * We reached the target so wait a bit 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.
>> -	 */
>> -	if ((XFS_LSN_CMP(lsn, target) >= 0)) {
>> +	if (count && (XFS_LSN_CMP(lsn, target) >= 0)) {
>> +		/*
>> +		 * We reached the target so wait a bit 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 += 20;
>>  		last_pushed_lsn = 0;
>> +	} else if (!count) {
>> +		/* We're past our target or empty, so idle */
>> +		tout = 1000;
>>  	} else if ((restarts > XFS_TRANS_PUSH_AIL_RESTARTS) ||
>>  		   (count && ((stuck * 100) / count > 90))) {
>>  		/*
> 

Re: [patch] Prevent excessive xfsaild wakeups

[Christoph Hellwig]

On Tue, Feb 19, 2008 at 09:59:06AM +1100, David Chinner wrote:
> +	if (count && (XFS_LSN_CMP(lsn, target) >= 0)) {
> +		/*
> +		 * We reached the target so wait a bit 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 += 20;
>  		last_pushed_lsn = 0;
> +	} else if (!count) {
> +		/* We're past our target or empty, so idle */
> +		tout = 1000;
>  	} else if ((restarts > XFS_TRANS_PUSH_AIL_RESTARTS) ||
>  		   (count && ((stuck * 100) / count > 90))) {
>  		/*

When looking at this conditions it confuses the hell out of me.  Having
count checked in each of three nested conditionals simply isn't readable
:)

Also some checks seem to be superflous, so how about:


	if (!count) {
		/*
		 * We're past our target or empty, so idle.
		 */
		 tout = 1000;
	} else if (XFS_LSN_CMP(lsn, target) >= 0) {
		/*
		 * We reached the target so wait a bit 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 += 20;
 		last_pushed_lsn = 0;
	} else (restarts > XFS_TRANS_PUSH_AIL_RESTARTS ||
	        ((stuck * 100) / count > 90)) {
		...
	}

Re: [patch] Prevent excessive xfsaild wakeups

[David Chinner]

On Wed, Feb 20, 2008 at 02:10:00PM -0500, Christoph Hellwig wrote:
> On Tue, Feb 19, 2008 at 09:59:06AM +1100, David Chinner wrote:
> > +	if (count && (XFS_LSN_CMP(lsn, target) >= 0)) {
> > +		/*
> > +		 * We reached the target so wait a bit 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 += 20;
> >  		last_pushed_lsn = 0;
> > +	} else if (!count) {
> > +		/* We're past our target or empty, so idle */
> > +		tout = 1000;
> >  	} else if ((restarts > XFS_TRANS_PUSH_AIL_RESTARTS) ||
> >  		   (count && ((stuck * 100) / count > 90))) {
> >  		/*
> 
> When looking at this conditions it confuses the hell out of me.  Having
> count checked in each of three nested conditionals simply isn't readable
> :)

Yeah, I noticed that when posting it. Got to keep you busy, Christoph. ;)

> Also some checks seem to be superflous, so how about:

Yup, fixed to be like this. I'll run it through a qa run
and go from there...

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group