Re: [PATCH] writeback: guard against jiffies wraparound on inode->dirtied_when checks

[Jeff Layton <jlayton@redhat.com>]

On Tue, 31 Mar 2009 16:33:35 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 30 Mar 2009 12:40:08 -0400
> Jeff Layton <jlayton@redhat.com> wrote:
> 
> > 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.
> > 
> > This patch fixes 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
> > as not to matter.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > Acked-by: Wu Fengguang <fengguang.wu@intel.com>
> > Acked-by: Ian Kent <raven@themaw.net>
> > ---
> >  fs/fs-writeback.c |   11 +++++++----
> >  1 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index e3fe991..dba69a5 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -196,8 +196,9 @@ static void redirty_tail(struct inode *inode)
> >  		struct inode *tail_inode;
> >  
> >  		tail_inode = list_entry(sb->s_dirty.next, struct inode, i_list);
> > -		if (!time_after_eq(inode->dirtied_when,
> > -				tail_inode->dirtied_when))
> > +		if (time_before(inode->dirtied_when,
> > +				tail_inode->dirtied_when) ||
> > +		    time_after(inode->dirtied_when, jiffies))
> >  			inode->dirtied_when = jiffies;
> >  	}
> >  	list_move(&inode->i_list, &sb->s_dirty);
> > @@ -231,7 +232,8 @@ static void move_expired_inodes(struct list_head *delaying_queue,
> >  		struct inode *inode = list_entry(delaying_queue->prev,
> >  						struct inode, i_list);
> >  		if (older_than_this &&
> > -			time_after(inode->dirtied_when, *older_than_this))
> > +			time_after(inode->dirtied_when, *older_than_this) &&
> > +			time_before_eq(inode->dirtied_when, jiffies))
> >  			break;
> >  		list_move(&inode->i_list, dispatch_queue);
> >  	}
> > @@ -493,7 +495,8 @@ void generic_sync_sb_inodes(struct super_block *sb,
> >  		}
> >  
> >  		/* Was this inode dirtied after sync_sb_inodes was called? */
> > -		if (time_after(inode->dirtied_when, start))
> > +		if (time_after(inode->dirtied_when, start) &&
> > +		    time_before_eq(inode->dirtied_when, jiffies))
> >  			break;
> >  
> 
> It'd be nice to add/update the comments to explain what's going on. 
> Otherwise it's a wee bit obscure, no?
> 

Thanks for picking this up, Andrew...

Good point. I had some comments in the patch that I backported for
RHEL5. I'll add some and send a respin tomorrow.

Cheers,
-- 
Jeff Layton <jlayton@redhat.com>