Re: [PATCH 1/2] XFS: Don't wake the aild once per second

[Christoph Hellwig <hch@infradead.org>]

On Sat, Jan 02, 2010 at 01:43:34PM +1100, Dave Chinner wrote:
> Now that the AIL push algorithm is traversal safe, we don't need a
> watchdog function in the xfsaild to catch pushes that fail to make
> progress. Remove the watchdog timeout and make pushes purely driven
> by demand. This will remove the once-per-second wakeup that is seen
> when the filesystem is idle and make laptop power misers happy.

Looks good, but a few nitpicks below:

> -	long		tout = 0;
> +	long		tout = 1000; /* milliseconds */

Why do we use a timeout when starting up now?  If there's a good reason
for it the reason at least should be explained in a comment here.

>  	while (!kthread_should_stop()) {
> -		if (tout)
> -			schedule_timeout_interruptible(msecs_to_jiffies(tout));
> -		tout = 1000;
> +		tout = !tout ? MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(tout);

Why not just:

		schedule_timeout_interruptible(tout ?
				msecs_to_jiffies(tout) : MAX_SCHEDULE_TIMEOUT);

that avoids the assignment of different units to the same variable, and
also the negation.

> +	long		tout = 0;
>  	xfs_lsn_t	last_pushed_lsn = *last_lsn;
>  	xfs_lsn_t	target =  ailp->xa_target;
>  	xfs_lsn_t	lsn;
> @@ -279,7 +280,6 @@ xfsaild_push(
>  	 * prevents use from spinning when we can't do anything or there is
>  	 * lots of contention on the AIL lists.
>  	 */
> -	tout = 10;
>  	lsn = lip->li_lsn;
>  	flush_log = stuck = count = 0;
>  	while ((XFS_LSN_CMP(lip->li_lsn, target) < 0)) {
> @@ -376,14 +376,14 @@ xfsaild_push(
>  
>  	if (!count) {
>  		/* We're past our target or empty, so idle */
> -		tout = 1000;
> +		tout = 0;

tout always is 0 here already.


>  	} 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;
> +		tout = 50;
>  		last_pushed_lsn = 0;
>  	} else if ((stuck * 100) / count > 90) {
>  		/*
> @@ -395,11 +395,14 @@ xfsaild_push(
>  		 * Backoff a bit more to allow some I/O to complete before
>  		 * continuing from where we were.
>  		 */
> -		tout += 10;
> +		tout = 20;
> +	} else {
> +		/* more to do, but wait a short while before continuing */
> +		tout = 10;
>  	}
>  	*last_lsn = last_pushed_lsn;
>  	return tout;
> -}	/* xfsaild_push */
> +}

All the += and co here is a bit confusing.  We always return 0 now
except for those last two cases that return 20 or 10.  So I'd just
change them to a tout = 10/20;

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