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

[Dave Chinner <david@fromorbit.com>]

On Mon, Jan 04, 2010 at 10:20:48AM -0500, Christoph Hellwig wrote:
> On Sat, Jan 02, 2010 at 01:43:35PM +1100, 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.
> > 
> > Signed-off-by: Dave Chinner <david@fromorbit.com>
> > ---
> >  fs/xfs/linux-2.6/xfs_buf.c |   28 +++++++++++++++++++---------
> >  1 files changed, 19 insertions(+), 9 deletions(-)
> > 
> 
> >  STATIC int xfsbufd(void *);
> > -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....

> > @@ -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?

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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs