xfs
[Top] [All Lists]

Re: [PATCH 5/5] mm: writeback: Prioritise dirty inodes encountered by di

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/5] mm: writeback: Prioritise dirty inodes encountered by direct reclaim for background flushing
From: Mel Gorman <mgorman@xxxxxxx>
Date: Thu, 14 Jul 2011 08:30:33 +0100
Cc: Linux-MM <linux-mm@xxxxxxxxx>, LKML <linux-kernel@xxxxxxxxxxxxxxx>, XFS <xfs@xxxxxxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, Johannes Weiner <jweiner@xxxxxxxxxx>, Wu Fengguang <fengguang.wu@xxxxxxxxx>, Jan Kara <jack@xxxxxxx>, Rik van Riel <riel@xxxxxxxxxx>, Minchan Kim <minchan.kim@xxxxxxxxx>
In-reply-to: <20110713235606.GX23038@dastard>
References: <1310567487-15367-1-git-send-email-mgorman@xxxxxxx> <1310567487-15367-6-git-send-email-mgorman@xxxxxxx> <20110713235606.GX23038@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Jul 14, 2011 at 09:56:06AM +1000, Dave Chinner wrote:
> On Wed, Jul 13, 2011 at 03:31:27PM +0100, Mel Gorman wrote:
> > It is preferable that no dirty pages are dispatched from the page
> > reclaim path. If reclaim is encountering dirty pages, it implies that
> > either reclaim is getting ahead of writeback or use-once logic has
> > prioritise pages for reclaiming that are young relative to when the
> > inode was dirtied.
> > 
> > When dirty pages are encounted on the LRU, this patch marks the inodes
> > I_DIRTY_RECLAIM and wakes the background flusher. When the background
> > flusher runs, it moves such inodes immediately to the dispatch queue
> > regardless of inode age. There is no guarantee that pages reclaim
> > cares about will be cleaned first but the expectation is that the
> > flusher threads will clean the page quicker than if reclaim tried to
> > clean a single page.
> > 
> > Signed-off-by: Mel Gorman <mgorman@xxxxxxx>
> > ---
> >  fs/fs-writeback.c         |   56 
> > ++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/fs.h        |    5 ++-
> >  include/linux/writeback.h |    1 +
> >  mm/vmscan.c               |   16 ++++++++++++-
> >  4 files changed, 74 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 0f015a0..1201052 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -257,9 +257,23 @@ static void move_expired_inodes(struct list_head 
> > *delaying_queue,
> >     LIST_HEAD(tmp);
> >     struct list_head *pos, *node;
> >     struct super_block *sb = NULL;
> > -   struct inode *inode;
> > +   struct inode *inode, *tinode;
> >     int do_sb_sort = 0;
> >  
> > +   /* Move inodes reclaim found at end of LRU to dispatch queue */
> > +   list_for_each_entry_safe(inode, tinode, delaying_queue, i_wb_list) {
> > +           /* Move any inode found at end of LRU to dispatch queue */
> > +           if (inode->i_state & I_DIRTY_RECLAIM) {
> > +                   inode->i_state &= ~I_DIRTY_RECLAIM;
> > +                   list_move(&inode->i_wb_list, &tmp);
> > +
> > +                   if (sb && sb != inode->i_sb)
> > +                           do_sb_sort = 1;
> > +                   sb = inode->i_sb;
> > +           }
> > +   }
> 
> This is not a good idea. move_expired_inodes() already sucks a large
> amount of CPU when there are lots of dirty inodes on the list (think
> hundreds of thousands), and that is when the traversal terminates at
> *older_than_this. It's not uncommon in my testing to see this
> one function consume 30-35% of the bdi-flusher thread CPU usage
> in such conditions.
> 

I thought this might be the case. I wasn't sure how bad it could be but
I mentioned in the leader it might be a problem. I'll consider other
ways that pages found at the end of the LRU could be prioritised for
writeback.

> > <SNIP>
> > +
> > +   sb = NULL;
> >     while (!list_empty(delaying_queue)) {
> >             inode = wb_inode(delaying_queue->prev);
> >             if (older_than_this &&
> > @@ -968,6 +982,46 @@ void wakeup_flusher_threads(long nr_pages)
> >     rcu_read_unlock();
> >  }
> >  
> > +/*
> > + * Similar to wakeup_flusher_threads except prioritise inodes contained
> > + * in the page_list regardless of age
> > + */
> > +void wakeup_flusher_threads_pages(long nr_pages, struct list_head 
> > *page_list)
> > +{
> > +   struct page *page;
> > +   struct address_space *mapping;
> > +   struct inode *inode;
> > +
> > +   list_for_each_entry(page, page_list, lru) {
> > +           if (!PageDirty(page))
> > +                   continue;
> > +
> > +           if (PageSwapBacked(page))
> > +                   continue;
> > +
> > +           lock_page(page);
> > +           mapping = page_mapping(page);
> > +           if (!mapping)
> > +                   goto unlock;
> > +
> > +           /*
> > +            * Test outside the lock to see as if it is already set. Inode
> > +            * should be pinned by the lock_page
> > +            */
> > +           inode = page->mapping->host;
> > +           if (inode->i_state & I_DIRTY_RECLAIM)
> > +                   goto unlock;
> > +
> > +           spin_lock(&inode->i_lock);
> > +           inode->i_state |= I_DIRTY_RECLAIM;
> > +           spin_unlock(&inode->i_lock);
> 
> Micro optimisations like this are unnecessary - the inode->i_lock is
> not contended.
> 

This patch was brought forward from a time when it would have been
taking the global inode_lock. I wasn't sure how badly inode->i_lock
was being contended and hadn't set up lock stats. Thanks for the
clarification.

> As it is, this code won't really work as you think it might.
> There's no guarantee a dirty inode is on the dirty - it might have
> already been expired, and it might even currently be under
> writeback.  In that case, if it is still dirty it goes to the
> b_more_io list and writeback bandwidth is shared between all the
> other dirty inodes and completely ignores this flag...
> 

Ok, it's a total bust. If I revisit this at all, it'll either be in
the context of Wu's approach or calling fdatawrite_range but but it
might be pointless and overall it might just be better for now to
leave kswapd calling ->writepage if reclaim is failing and priority
is raised.

-- 
Mel Gorman
SUSE Labs

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