Skip to contentSkip to conversation

XFS: Run kernel threads on demand

PATCH In review Dave Chinner 2 participants 7 messages Jan 2, 2010 – Jan 4, 2010 XFS filesystem

lore
Share & tools
lore

Dave Chinner

author
#0 lore raw

cover XFS: Run kernel threads on demand

We run the xfsbufd and the xfsaild once a second even when there is no work to do. Make them demand driven so that we don't needlessly wake idle CPUs to do nothing. This should make power misers with XFS filesystems happy.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#1 lore raw

PATCH 1/2 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 <[email protected]>
---
 fs/xfs/linux-2.6/xfs_super.c |    7 +++----
 fs/xfs/xfs_trans_ail.c       |   17 ++++++++++-------
 2 files changed, 13 insertions(+), 11 deletions(-)
fs/xfs/linux-2.6/xfs_super.c +3 -4
index 09783cc..84ce77a 100644
@@ -877,12 +877,11 @@ xfsaild(
 {
 	struct xfs_ail	*ailp = data;
 	xfs_lsn_t	last_pushed_lsn = 0;
-	long		tout = 0;
+	long		tout = 1000; /* milliseconds */
 
 	while (!kthread_should_stop()) {
-		if (tout)
-			schedule_timeout_interruptible(msecs_to_jiffies(tout));
-		tout = 1000;
+		tout = !tout ? MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(tout);
+		schedule_timeout_interruptible(tout);
 
 		/* swsusp */
 		try_to_freeze();
fs/xfs/xfs_trans_ail.c +10 -7
index 2ffc570..8ca123e 100644
@@ -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;

@@ -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;
 	} 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 */
+}
 
 
 /*
Signature · 6 lines
1.6.5

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#2 lore raw

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

Quoted 5 lines · Dave Chinner

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:

fs/xfs/linux-2.6/xfs_super.c xfsaild( · line 880

Quoted 2 lines
fs/xfs/linux-2.6/xfs_super.c +1 -1
-	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.

fs/xfs/linux-2.6/xfs_super.c xfsaild( · line 883

Quoted 5 lines
fs/xfs/linux-2.6/xfs_super.c +1 -3
 	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.

Quoted 13 lines
fs/xfs/xfs_trans_ail.c +1 -1
+	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)) {

fs/xfs/xfs_trans_ail.c xfsaild_push( · line 379

Quoted 5 lines
fs/xfs/xfs_trans_ail.c +1 -1
@@ -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.

Quoted 15 lines
fs/xfs/xfs_trans_ail.c +1 -1
 	} 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.
 		 */

fs/xfs/xfs_trans_ail.c xfsaild_push( · line 405

Quoted 10 lines
fs/xfs/xfs_trans_ail.c +5 -2
-		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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#3 lore raw

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

Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Dave Chinner

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.

Quoted 7 lines

Looks good, but a few nitpicks below:

Review

Quoted 2 lines
(quoted diff) +1 -1
-	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.

Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +1 -3
 	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:

Quoted 5 lines
		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.

Quoted 14 lines
Quoted 14 lines
(quoted diff) +1 -1
+	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(
Quoted 6 lines

Review xfsaild_push( · line 379

Quoted 4 lines
(quoted diff) +1 -1
 	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.

Quoted 21 lines
Quoted 21 lines
(quoted diff) +5 -2
 	} 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;
 	}
Quoted 8 lines

Review xfsaild_push( · line 405

Quoted 4 lines
(quoted diff) +1 -1
 	*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.

Signature · 16 lines
Dave Chinner
[email protected]

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 <[email protected]>
---
 fs/xfs/linux-2.6/xfs_super.c |    7 +++----
 fs/xfs/xfs_trans_ail.c       |   19 +++++++++++--------
 2 files changed, 14 insertions(+), 12 deletions(-)
fs/xfs/linux-2.6/xfs_super.c +3 -4
index 09783cc..0a4fd0e 100644
@@ -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();
fs/xfs/xfs_trans_ail.c +11 -8
index 2ffc570..063dfbd 100644
@@ -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 */
+}
 
 
 /*

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#4 lore raw

PATCH 2/2 XFS: Don't wake xfsbufd when idle

In reply to Dave Chinner

The xfsbufd wakes every xfsbufd_centisecs (once per second by default) for each filesystem even when the filesystem is idle. If the xfsbufd has nothing to do, put it into a long term sleep and only wake it up when there is work pending (i.e. dirty buffers to flush soon). This will make laptop power misers happy.

Signed-off-by Dave Chinner <[email protected]>
---
 fs/xfs/linux-2.6/xfs_buf.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)
fs/xfs/linux-2.6/xfs_buf.c +19 -9
index 77b8be8..759cbaf 100644
@@ -43,10 +43,10 @@
 
 static kmem_zone_t *xfs_buf_zone;
 STATIC int xfsbufd(void *);
-STATIC int xfsbufd_wakeup(int, gfp_t);
+STATIC int xfsbufd_wakeup_all(int, gfp_t);
 STATIC void xfs_buf_delwri_queue(xfs_buf_t *, int);
 static struct shrinker xfs_buf_shake = {
-	.shrink = xfsbufd_wakeup,
+	.shrink = xfsbufd_wakeup_all,
 	.seeks = DEFAULT_SEEKS,
 };
 

@@ -385,7 +385,7 @@ _xfs_buf_lookup_pages(
 					__func__, gfp_mask);
 
 			XFS_STATS_INC(xb_page_retries);
-			xfsbufd_wakeup(0, gfp_mask);
+			xfsbufd_wakeup_all(0, gfp_mask);
 			congestion_wait(BLK_RW_ASYNC, HZ/50);
 			goto retry;
 		}

@@ -1595,6 +1595,11 @@ xfs_buf_delwri_queue(
 		list_del(&bp->b_list);
 	}
 
+	if (list_empty(dwq)) {
+		/* start xfsbufd as it has something to do now */
+		wake_up_process(bp->b_target->bt_task);
+	}
+
 	bp->b_flags |= _XBF_DELWRI_Q;
 	list_add_tail(&bp->b_list, dwq);
 	bp->b_queuetime = jiffies;

@@ -1634,7 +1639,7 @@ xfs_buf_runall_queues(
 }
 
 STATIC int
-xfsbufd_wakeup(
+xfsbufd_wakeup_all(
 	int			priority,
 	gfp_t			mask)
 {

@@ -1644,6 +1649,8 @@ xfsbufd_wakeup(
 	list_for_each_entry(btp, &xfs_buftarg_list, bt_list) {
 		if (test_bit(XBT_FORCE_SLEEP, &btp->bt_flags))
 			continue;
+		if (list_empty(&btp->bt_delwrite_queue))
+			continue;
 		set_bit(XBT_FORCE_FLUSH, &btp->bt_flags);
 		wake_up_process(btp->bt_task);
 	}

@@ -1708,6 +1715,9 @@ xfsbufd(
 	set_freezable();
 
 	do {
+		long	age = xfs_buf_age_centisecs * msecs_to_jiffies(10);
+		long	tout = age;
+
 		if (unlikely(freezing(current))) {
 			set_bit(XBT_FORCE_SLEEP, &target->bt_flags);
 			refrigerator();

@@ -1715,12 +1725,12 @@ xfsbufd(
 			clear_bit(XBT_FORCE_SLEEP, &target->bt_flags);
 		}
 
-		schedule_timeout_interruptible(
-			xfs_buf_timer_centisecs * msecs_to_jiffies(10));
-
-		xfs_buf_delwri_split(target, &tmp,
-				xfs_buf_age_centisecs * msecs_to_jiffies(10));
+		/* sleep for a long time if there is nothing to do. */
+		if (list_empty(&target->bt_delwrite_queue))
+			tout = MAX_SCHEDULE_TIMEOUT;
+		schedule_timeout_interruptible(tout);
 
+		xfs_buf_delwri_split(target, &tmp, age);
 		count = 0;
 		while (!list_empty(&tmp)) {
 			bp = list_entry(tmp.next, xfs_buf_t, b_list);
Signature · 6 lines
1.6.5

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#5 lore raw

Re: [PATCH 2/2] XFS: Don't wake xfsbufd when idle

Quoted 13 lines · Dave Chinner

The xfsbufd wakes every xfsbufd_centisecs (once per second by default) for each filesystem even when the filesystem is idle. If the xfsbufd has nothing to do, put it into a long term sleep and only wake it up when there is work pending (i.e. dirty buffers to flush soon). This will make laptop power misers happy.

Signed-off-by Dave Chinner <[email protected]>
---
 fs/xfs/linux-2.6/xfs_buf.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

STATIC int xfsbufd(void *);

fs/xfs/linux-2.6/xfs_buf.c line 46

Quoted 2 lines
fs/xfs/linux-2.6/xfs_buf.c +1 -1
-STATIC int xfsbufd_wakeup(int, gfp_t);
+STATIC int xfsbufd_wakeup_all(int, gfp_t);

this rename seems unrelated to the rest of the patch.

fs/xfs/linux-2.6/xfs_buf.c xfs_buf_delwri_queue( · line 1601

Quoted 8 lines
fs/xfs/linux-2.6/xfs_buf.c +4 -0
@@ -1595,6 +1595,11 @@ xfs_buf_delwri_queue(
 		list_del(&bp->b_list);
 	}
 
+	if (list_empty(dwq)) {
+		/* start xfsbufd as it has something to do now */
+		wake_up_process(bp->b_target->bt_task);
+	}

Does it make sense to wake xfsbufd before actually adding the item and unlocking the queue lock? Shouldn't this be defered until after the addition?

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#6 lore raw

Re: [PATCH 2/2] XFS: Don't wake xfsbufd when idle

Quoted 13 lines · Christoph Hellwig
Quoted 11 lines · Dave Chinner

The xfsbufd wakes every xfsbufd_centisecs (once per second by default) for each filesystem even when the filesystem is idle. If the xfsbufd has nothing to do, put it into a long term sleep and only wake it up when there is work pending (i.e. dirty buffers to flush soon). This will make laptop power misers happy.

Signed-off-by Dave Chinner <[email protected]>
---
 fs/xfs/linux-2.6/xfs_buf.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)
Quoted 5 lines

fs/xfs/linux-2.6/xfs_buf.c line 46

Quoted 3 lines

STATIC int xfsbufd(void *);

fs/xfs/linux-2.6/xfs_buf.c +1 -1
-STATIC int xfsbufd_wakeup(int, gfp_t);
+STATIC int xfsbufd_wakeup_all(int, gfp_t);

this rename seems unrelated to the rest of the patch.

For memory reclaim we want to wake up the xfsbufd threads on every single filesystem to free up as much memory as possible. Hence with the addition of demand flushing we have a situation now where we can wakeup either a single xfsbufd or we need to wake up all of them. It seemed right to make the distinction clear by renaming the function. I can drop it if this doesn't make sense....

Quoted 4 lines
Quoted 4 lines
fs/xfs/linux-2.6/xfs_buf.c +0 -0
@@ -1595,6 +1595,11 @@ xfs_buf_delwri_queue(
 		list_del(&bp->b_list);
 	}
Quoted 8 lines

fs/xfs/linux-2.6/xfs_buf.c xfs_buf_delwri_queue( · line 1601

Quoted 4 lines
fs/xfs/linux-2.6/xfs_buf.c +4 -0
+	if (list_empty(dwq)) {
+		/* start xfsbufd as it has something to do now */
+		wake_up_process(bp->b_target->bt_task);
+	}

Does it make sense to wake xfsbufd before actually adding the item and unlocking the queue lock? Shouldn't this be defered until after the addition?

I did it to avoid a temp var - if the xfsbufd runs before we finish here then is will spin on the lock until we have added the buffer to the list and dropped the lock. wake_up_process() is safe to call under a spinlock, so that is not an issue here.

Also, the xfsbufd checks for an empty list before it sleeps, so on wakeup it will always see the newly added buffer because it tries unconditionally to dequeue buffers on wakeup. Hence I think this is safe and race free.

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs