Re: [PATCH] writeback: guard against jiffies wraparound oninode->dirtied_when checks (try #2)

[Wu Fengguang]

On Wed, Apr 01, 2009 at 07:53:20PM +0800, Jeff Layton wrote:
> On Wed, 1 Apr 2009 14:56:18 +0800
> Wu Fengguang <fengguang.wu@xxxxxxxxx> wrote:
> 
> > On Tue, Mar 31, 2009 at 06:07:30PM -0700, Andrew Morton wrote:
> > > On Tue, 31 Mar 2009 20:50:18 -0400 Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > 
> > > > On Tue, 31 Mar 2009 17:20:31 -0700
> > > > Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > > 
> > > > > On Tue, 31 Mar 2009 20:03:59 -0400
> > > > > Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > > > 
> > > > > > +		 * It's not sufficient to just do a time_after() check on
> > > > > > +		 * dirtied_when. That assumes that dirtied_when will always
> > > > > > +		 * change within a period of jiffies that encompasses half the
> > > > > > +		 * machine word size (2^31 jiffies on 32-bit arch). That's not
> > > > > > +		 * necessarily the case if an inode is being constantly
> > > > > > +		 * redirtied. Since dirtied_when can never be in the future,
> > > > > > +		 * we can assume that if it appears to be so then it is
> > > > > > +		 * actually in the distant past.
> > > > > 
> > > > > so this really is a 32-bit-only thing.
> > > > > 
> > > > > I guess that isn't worth optimising for though.
> > > > > 
> > > > 
> > > > Yeah, it's pretty much impossible to hit this on a 64-bit machine.
> > > > 
> > > > > otoh, given that all three comparisons are the same:
> > > > > 
> > > > > +			time_after(inode->dirtied_when, *older_than_this) &&
> > > > > +			time_before_eq(inode->dirtied_when, jiffies))
> > > > > 
> > > > > (although one is inverted (i think?)), it might end up nicer if this was all done
> > > > > in a little helper function?
> > > > > 
> > > > > That way we only need to comment what's going on at a single site, and
> > > > > we could omit the additional test if !CONFIG_64BIT.
> > > > 
> > > > Ok, that seems reasonable.
> > > > 
> > > > At one point I had a macro similar to time_in_range(), but dropped it
> > > > primarily because time_after_but_before_eq() wasn't easy on the eyes.
> > > > Thoughts on better names?
> > > 
> > > I was thinking
> > > 
> > > 	bool inode_dirtied_after(...);
> > > 
> > > and just leave the innards using time_after() and time_before_eq()?
> > 
> > Andrew, here is the updated patch. Note that the first chunk for
> > redirty_tail() was not absolutely necessary and so removed.
> > 
> > Thanks,
> > Fengguang
> > ---
> > Subject: writeback: guard against jiffies wraparound on inode->dirtied_when checks
> > From:	Jeff Layton <jlayton@xxxxxxxxxx>
> > 
> > The dirtied_when value on an inode is supposed to represent the first
> > time that an inode has one of its pages dirtied. This value is in units
> > of jiffies. It's used in several places in the writeback code to
> > determine when to write out an inode.
> > 
> > The problem is that these checks assume that dirtied_when is updated
> > periodically. If an inode is continuously being used for I/O it can be
> > persistently marked as dirty and will continue to age. Once the time
> > difference between dirtied_when and the jiffies value it is being
> > compared to is greater than or equal to half the maximum of the jiffies
> > type, the logic of the time_*() macros inverts and the opposite of what
> > is needed is returned. On 32-bit architectures that's just under 25 days
> > (assuming HZ == 1000).
> > 
> > As the least-recently dirtied inode, it'll end up being the first one
> > that pdflush will try to write out. sync_sb_inodes() does this check:
> > 
> > 	/* Was this inode dirtied after sync_sb_inodes was called? */
> >  	if (time_after(inode->dirtied_when, start))
> >  		break;
> > 
> > ...but now dirtied_when appears to be in the future. sync_sb_inodes()
> > bails out without attempting to write any dirty inodes. When this
> > occurs, pdflush will stop writing out inodes for this superblock.
> > Nothing can unwedge it until jiffies moves out of the problematic
> > window.
> > 
> > Fix this problem by changing the checks against dirtied_when to also
> > check whether it appears to be in the future. If it does, then we
> > consider the value to be far in the past.
> > 
> > This should shrink the problematic window of time to such a small
> > period(30s) as not to matter.
> > 
> > Acked-by: Ian Kent <raven@xxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > Signed-off-by: Wu Fengguang <fengguang.wu@xxxxxxxxx>
> > ---
> >  fs/fs-writeback.c |   26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> > 
> > --- mm.orig/fs/fs-writeback.c
> > +++ mm/fs/fs-writeback.c
> > @@ -196,7 +196,7 @@ static void redirty_tail(struct inode *i
> >  		struct inode *tail_inode;
> >  
> >  		tail_inode = list_entry(sb->s_dirty.next, struct inode, i_list);
> > -		if (!time_after_eq(inode->dirtied_when,
> > +		if (time_before(inode->dirtied_when,
> >  				tail_inode->dirtied_when))
> >  			inode->dirtied_when = jiffies;
> >  	}
> 
> I think we need a similar change in this function in order to maintain
> the list order.
> 
> Consider this case:
> 
> We have an s_dirty list with a head inode that appears to be in the
> future. We start writeback and clear out s_dirty (all of the inodes are
> moved to s_io). A new inode is dirtied, and goes onto the empty s_dirty
> list with a dirtied_when value that equals now. The inode with the
> dirtied_when value that looks like it's in the future is redirtied while
> being written and redirty_tail is called. It goes back on the list
> without resetting dirtied_when even though it's actually older than the
> inode at the tail.

What's the difference? It _is_ the past because all 2 reference sites
are now taught to think so.

So s_dirty is still in order, and the writeback process won't be blocked.

> There is another option too that I'll throw out here...
> 
> We could just make dirtied_when a 64 bit value on 32 bit machines and
> use jiffies_64 there. On the upside there is no "problematic
> window" with that. The downside is that struct inode would grow by 4
> bytes on 32 bit arches, and checking jiffies_64 on such an arch is
> more computationally intensive. We'd also have to change the size of
> older_than_this value in the writeback_control struct too if we want to
> go this route...

Yes that could eliminate the 30s or more temporary writeback stillness.
The only problem is the extra costs for normal cases, especially the
space cost.

Thanks,
Fengguang

> 
> > @@ -220,6 +220,21 @@ static void inode_sync_complete(struct i
> >  	wake_up_bit(&inode->i_state, __I_SYNC);
> >  }
> >  
> > +static bool inode_dirtied_after(struct inode *inode, unsigned long t)
> > +{
> > +	bool ret = time_after(inode->dirtied_when, t);
> > +#ifndef CONFIG_64BIT
> > +	/*
> > +	 * For inodes being constantly redirtied, dirtied_when can get stuck.
> > +	 * It _appears_ to be in the future, but is actually in distant past.
> > +	 * This test is necessary to prevent such wrapped-around relative times
> > +	 * from permanently stopping the whole pdflush writeback.
> > +	 */
> > +	ret = ret && time_before_eq(inode->dirtied_when, jiffies);
> > +#endif
> > +	return ret;
> > +}
> > +
> >  /*
> >   * Move expired dirty inodes from @delaying_queue to @dispatch_queue.
> >   */
> > @@ -231,7 +246,7 @@ static void move_expired_inodes(struct l
> >  		struct inode *inode = list_entry(delaying_queue->prev,
> >  						struct inode, i_list);
> >  		if (older_than_this &&
> > -			time_after(inode->dirtied_when, *older_than_this))
> > +				inode_dirtied_after(inode, *older_than_this))
> >  			break;
> >  		list_move(&inode->i_list, dispatch_queue);
> >  	}
> > @@ -492,8 +507,11 @@ void generic_sync_sb_inodes(struct super
> >  			continue;		/* blockdev has wrong queue */
> >  		}
> >  
> > -		/* Was this inode dirtied after sync_sb_inodes was called? */
> > -		if (time_after(inode->dirtied_when, start))
> > +		/*
> > +		 * Was this inode dirtied after sync_sb_inodes was called?
> > +		 * This keeps sync from extra jobs and livelock.
> > +		 */
> > +		if (inode_dirtied_after(inode, start))
> >  			break;
> >  
> >  		/* Is another pdflush already flushing this queue? */
> 
> 
> -- 
> Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/