linux-kernel - Re: [PATCH 4/6] writeback: sync expired inodes first in background writeback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100726121258.GE3280@quack.suse.cz>
Date:	Mon, 26 Jul 2010 14:12:59 +0200
From:	Jan Kara <jack@...e.cz>
To:	Wu Fengguang <fengguang.wu@...el.com>
Cc:	Jan Kara <jack@...e.cz>, Andrew Morton <akpm@...ux-foundation.org>,
	Dave Chinner <david@...morbit.com>,
	Christoph Hellwig <hch@...radead.org>,
	Mel Gorman <mel@....ul.ie>,
	Chris Mason <chris.mason@...cle.com>,
	Jens Axboe <jens.axboe@...cle.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH 4/6] writeback: sync expired inodes first in background
 writeback

On Mon 26-07-10 19:51:53, Wu Fengguang wrote:
> On Sat, Jul 24, 2010 at 02:15:21AM +0800, Jan Kara wrote:
> > On Thu 22-07-10 13:09:32, Wu Fengguang wrote:
> > > A background flush work may run for ever. So it's reasonable for it to
> > > mimic the kupdate behavior of syncing old/expired inodes first.
> > > 
> > > The policy is
> > > - enqueue all newly expired inodes at each queue_io() time
> > > - retry with halfed expire interval until get some inodes to sync
> >   Hmm, this logic looks a bit arbitrary to me. What I actually don't like
> > very much about this that when there aren't inodes older than say 2
> > seconds, you'll end up queueing just inodes between 2s and 1s. So I'd
> > rather just queue inodes older than the limit and if there are none, just
> > queue all other dirty inodes.
> 
> You are proposing
> 
> -				expire_interval >>= 1;
> +				expire_interval = 0;
> 
> IMO this does not really simplify code or concept. If we can get the
> "smoother" behavior in original patch without extra cost, why not? 
  I agree there's no substantial code simplification. But I see a
substantial "behavior" simplification (just two sweeps instead of 10 or
so). But I don't really insist on the two sweeps, it's just that I don't
see a justification for the exponencial back off here... I mean what's the
point if the interval we queue gets really small? Why not just use
expire_interval/2 as a step if you want a smoother behavior?

								Honza
> > > CC: Jan Kara <jack@...e.cz>
> > > Signed-off-by: Wu Fengguang <fengguang.wu@...el.com>
> > > ---
> > >  fs/fs-writeback.c |   20 ++++++++++++++------
> > >  1 file changed, 14 insertions(+), 6 deletions(-)
> > > 
> > > --- linux-next.orig/fs/fs-writeback.c	2010-07-22 12:56:42.000000000 +0800
> > > +++ linux-next/fs/fs-writeback.c	2010-07-22 13:07:51.000000000 +0800
> > > @@ -217,14 +217,14 @@ static void move_expired_inodes(struct l
> > >  				struct writeback_control *wbc)
> > >  {
> > >  	unsigned long expire_interval = 0;
> > > -	unsigned long older_than_this;
> > > +	unsigned long older_than_this = 0; /* reset to kill gcc warning */
> > >  	LIST_HEAD(tmp);
> > >  	struct list_head *pos, *node;
> > >  	struct super_block *sb = NULL;
> > >  	struct inode *inode;
> > >  	int do_sb_sort = 0;
> > >  
> > > -	if (wbc->for_kupdate) {
> > > +	if (wbc->for_kupdate || wbc->for_background) {
> > >  		expire_interval = msecs_to_jiffies(dirty_expire_interval * 10);
> > >  		older_than_this = jiffies - expire_interval;
> > >  	}
> > > @@ -232,8 +232,15 @@ static void move_expired_inodes(struct l
> > >  	while (!list_empty(delaying_queue)) {
> > >  		inode = list_entry(delaying_queue->prev, struct inode, i_list);
> > >  		if (expire_interval &&
> > > -		    inode_dirtied_after(inode, older_than_this))
> > > -			break;
> > > +		    inode_dirtied_after(inode, older_than_this)) {
> > > +			if (wbc->for_background &&
> > > +			    list_empty(dispatch_queue) && list_empty(&tmp)) {
> > > +				expire_interval >>= 1;
> > > +				older_than_this = jiffies - expire_interval;
> > > +				continue;
> > > +			} else
> > > +				break;
> > > +		}
> > >  		if (sb && sb != inode->i_sb)
> > >  			do_sb_sort = 1;
> > >  		sb = inode->i_sb;
> > > @@ -521,7 +528,8 @@ void writeback_inodes_wb(struct bdi_writ
> > >  
> > >  	wbc->wb_start = jiffies; /* livelock avoidance */
> > >  	spin_lock(&inode_lock);
> > > -	if (!wbc->for_kupdate || list_empty(&wb->b_io))
> > > +
> > > +	if (!(wbc->for_kupdate || wbc->for_background) || list_empty(&wb->b_io))
> > >  		queue_io(wb, wbc);
> > >  
> > >  	while (!list_empty(&wb->b_io)) {
> > > @@ -550,7 +558,7 @@ static void __writeback_inodes_sb(struct
> > >  
> > >  	wbc->wb_start = jiffies; /* livelock avoidance */
> > >  	spin_lock(&inode_lock);
> > > -	if (!wbc->for_kupdate || list_empty(&wb->b_io))
> > > +	if (!(wbc->for_kupdate || wbc->for_background) || list_empty(&wb->b_io))
> > >  		queue_io(wb, wbc);
> > >  	writeback_sb_inodes(sb, wb, wbc, true);
> > >  	spin_unlock(&inode_lock);
> > > 
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > > the body of a message to majordomo@...r.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > -- 
> > Jan Kara <jack@...e.cz>
> > SUSE Labs, CR
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/