From mboxrd@z Thu Jan 1 00:00:00 1970 From: Theodore Ts'o Subject: Re: [PATCH] fs: make sure the timestamps for lazytime inodes eventually get written Date: Sun, 8 Mar 2015 15:06:54 -0400 Message-ID: <20150308190653.GG3317@thunk.org> References: <20150225145258.GB11217@thunk.org> <20150225162506.GE22736@quack.suse.cz> <20150226043304.GC11217@thunk.org> <20150226083455.GA10894@quack.suse.cz> <20150226134553.GF11217@thunk.org> <20150226143828.GA21485@quack.suse.cz> <20150226192735.GB17174@thunk.org> <20150302082907.GA4199@quack.suse.cz> <20150307053408.GA18002@thunk.org> <20150308100650.GA3743@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Linus Torvalds , linux-fsdevel , Alexander Viro To: Jan Kara Return-path: Received: from imap.thunk.org ([74.207.234.97]:55220 "EHLO imap.thunk.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750918AbbCHTG4 (ORCPT ); Sun, 8 Mar 2015 15:06:56 -0400 Content-Disposition: inline In-Reply-To: <20150308100650.GA3743@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Sun, Mar 08, 2015 at 11:06:50AM +0100, Jan Kara wrote: > > @@ -275,8 +278,8 @@ static int move_expired_inodes(struct list_head *delaying_queue, > > > > if ((flags & EXPIRE_DIRTY_ATIME) == 0) > > older_than_this = work->older_than_this; > > - else if ((work->reason == WB_REASON_SYNC) == 0) { > > - expire_time = jiffies - (HZ * 86400); > > + else if (!work->for_sync) { > > + expire_time = jiffies - (dirtytime_expire_interval * HZ); > > older_than_this = &expire_time; > > } > > while (!list_empty(delaying_queue)) { > This hunk should be a separate patch since it's completely unrelated. Along with all of the other changes that relate to adding a sysctl tunable? Sure, I can do that. BTW, I know that originally we talked about not needing the tunable, but it my experience it **really** helps with testing the future. If we ever want to try to create a automated test suite, it really helps to have the tunable. > > @@ -741,6 +745,13 @@ static long writeback_sb_inodes(struct super_block *sb, > > spin_lock(&inode->i_lock); > > if (!(inode->i_state & I_DIRTY_ALL)) > > wrote++; > > + if ((inode->i_state & I_DIRTY_TIME) && > > + ((start_time - inode->dirtied_time_when) > > > + (dirtytime_expire_interval * HZ))) { > > + inode->i_state &= ~I_DIRTY_TIME; > > + inode->i_state |= I_DIRTY_SYNC; > > + trace_writeback_lazytime(inode); > > + } > Hum, why is this here? A more logical place for it would IMO be in > __writeback_single_inode() where we modify inode state. Also we would then > immediately end up writing the inode instead of just queueing it to a > different writeback queue. Good point, it woud be much better to put it there. I'll move it in the next version of the patch. > > @@ -1269,6 +1330,10 @@ void __mark_inode_dirty(struct inode *inode, int flags) > > } > > > > inode->dirtied_when = jiffies; > > + if (dirtytime) > > + inode->dirtied_time_when = jiffies; > > + if (flags & I_DIRTY_PAGES) > > + dirtytime = 0; > > list_move(&inode->i_wb_list, dirtytime ? > > &bdi->wb.b_dirty_time : &bdi->wb.b_dirty); > > spin_unlock(&bdi->wb.list_lock); > I guess this would be more readable as: > if (dirtytime) > inode->dirtied_time_when = jiffies; > if (inode->i_state & (I_DIRTY_INODE | I_DIRTY_PAGES)) > list_move(&inode->i_wb_list, &bdi->wb.b_dirty); > else { > list_move(&inode->i_wb_list, > &bdi->wb.b_dirty_time); > } > Since that will clearly express the inode needs to end up in the list > which corresponds to current inode state. Also preferably the change in the > condition deciding in which list inode ends up should be split in a > separate patch since that's unrelated problem to the issue described in the > changelog. Agreed, I'll change this and resend. - Ted