[PATCH 1/2] XFS: Don't wake the aild once per second
Dave Chinner
david at fromorbit.com
Mon Jan 4 17:28:35 CST 2010
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 at fromorbit.com
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 at fromorbit.com>
---
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 */
+}
/*
More information about the xfs
mailing list