xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] XFS: Don't wake the aild once per second
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 5 Jan 2010 10:28:35 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100104151616.GB24810@xxxxxxxxxxxxx>
References: <1262400215-19443-1-git-send-email-david@xxxxxxxxxxxxx> <1262400215-19443-2-git-send-email-david@xxxxxxxxxxxxx> <20100104151616.GB24810@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Mon, Jan 04, 2010 at 10:16:16AM -0500, Christoph Hellwig wrote:
> 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.

No good reason.

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

Much cleaner. Fixed.

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

Good catch. Hmm - I just noticed that we should be resetting the
last_lsn when we idle. I've fixed that now.

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

I'm not sure what you mean - isn't that what this patch does? i.e.
now looks like:

        if idle
                tout = 0
        else if we reach the target
                tout = 50
        else if stuck
                tout = 20
        else
                tout = 10

Updated patch below.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

XFS: Don't wake the aild once per second

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.

Signed-off-by: Dave Chinner <david@xxxxxxxxxxxxx>
---
 fs/xfs/linux-2.6/xfs_super.c |    7 +++----
 fs/xfs/xfs_trans_ail.c       |   19 +++++++++++--------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_super.c b/fs/xfs/linux-2.6/xfs_super.c
index 09783cc..0a4fd0e 100644
--- a/fs/xfs/linux-2.6/xfs_super.c
+++ b/fs/xfs/linux-2.6/xfs_super.c
@@ -877,12 +877,11 @@ xfsaild(
 {
        struct xfs_ail  *ailp = data;
        xfs_lsn_t       last_pushed_lsn = 0;
-       long            tout = 0;
+       long            tout = 0; /* milliseconds */
 
        while (!kthread_should_stop()) {
-               if (tout)
-                       schedule_timeout_interruptible(msecs_to_jiffies(tout));
-               tout = 1000;
+               schedule_timeout_interruptible(tout ?
+                               msecs_to_jiffies(tout) : MAX_SCHEDULE_TIMEOUT);
 
                /* swsusp */
                try_to_freeze();
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 2ffc570..063dfbd 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -237,14 +237,15 @@ out:
 }
 
 /*
- * Function that does the work of pushing on the AIL
+ * xfsaild_push does the work of pushing on the AIL.  Returning a timeout of
+ * zero indicates that the caller should sleep until woken.
  */
 long
 xfsaild_push(
        struct xfs_ail  *ailp,
        xfs_lsn_t       *last_lsn)
 {
-       long            tout = 1000; /* milliseconds */
+       long            tout = 0;
        xfs_lsn_t       last_pushed_lsn = *last_lsn;
        xfs_lsn_t       target =  ailp->xa_target;
        xfs_lsn_t       lsn;
@@ -262,7 +263,7 @@ xfsaild_push(
                 */
                xfs_trans_ail_cursor_done(ailp, cur);
                spin_unlock(&ailp->xa_lock);
-               last_pushed_lsn = 0;
+               *last_lsn = 0;
                return tout;
        }
 
@@ -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;
+               last_pushed_lsn = 0;
        } 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 */
+}
 
 
 /*

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