xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] XFS: Don't wake the aild once per second
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 4 Jan 2010 10:16:16 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1262400215-19443-2-git-send-email-david@xxxxxxxxxxxxx>
References: <1262400215-19443-1-git-send-email-david@xxxxxxxxxxxxx> <1262400215-19443-2-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
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;

<Prev in Thread] Current Thread [Next in Thread>