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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20100726115153.GF6284@localhost>
Date:	Mon, 26 Jul 2010 19:51:53 +0800
From:	Wu Fengguang <fengguang.wu@...el.com>
To:	Jan Kara <jack@...e.cz>
Cc:	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 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? 

Thanks,
Fengguang


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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ