Re: [PATCH] fs: make sure the timestamps for lazytime inodes eventually get written

[Theodore Ts'o <tytso@mit.edu>]

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