xfs
[Top] [All Lists]

RE: [PATCH 3/3] XFS: Don't wake xfsbufd when idle

To: "Dave Chinner" <david@xxxxxxxxxxxxx>
Subject: RE: [PATCH 3/3] XFS: Don't wake xfsbufd when idle
From: "Alex Elder" <aelder@xxxxxxx>
Date: Thu, 14 Jan 2010 10:51:25 -0600
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <1263210599-25067-4-git-send-email-david@xxxxxxxxxxxxx>
Thread-index: AcqStqY0vAMRNtKHRKmRbGLCjcZ/NwCgseag
Thread-topic: [PATCH 3/3] XFS: Don't wake xfsbufd when idle
Dave Chinner wrote:
> 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.

Patch generally looks good but I have a question, below.

> Signed-off-by: Dave Chinner <david@xxxxxxxxxxxxx>
> ---
>  fs/xfs/linux-2.6/xfs_buf.c |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
> index 77b8be8..b51be4e 100644
> --- a/fs/xfs/linux-2.6/xfs_buf.c
> +++ b/fs/xfs/linux-2.6/xfs_buf.c
> @@ -1595,6 +1595,11 @@ xfs_buf_delwri_queue(
>               list_del(&bp->b_list);
>       }
> 
> +     if (list_empty(dwq)) {
> +             /* start xfsbufd as it is about to have something to do */
> +             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;
> @@ -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;

Why do you switch from using xfs_buf_timer_centisecs to
using xfs_buf_age_centisecs for the timeout (in the non-empty
delwrite queue case)?


> +
>               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));

Here is where the timeout was computed previously.

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

Effect of the timout change is here...

> +             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);

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