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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
