Re: [PATCH] writeback: reset inode dirty time when adding it backto empty s_dirty list

[Ian Kent]

Ian Kent wrote:
> On Mon, 23 Mar 2009, Jeff Layton wrote:
>
> > This may be a problem on other filesystems too, but the reproducer I
> > have involves NFS.
> >
> > On NFS, the __mark_inode_dirty() call after writing back the inode is
> > done in the rpc_release handler for COMMIT calls. This call is done
> > asynchronously after the call completes.
> >
> > Because there's no real coordination between __mark_inode_dirty() and
> > __sync_single_inode(), it's often the case that these two calls will
> > race and __mark_inode_dirty() will get called while I_SYNC is still set.
> > When this happens, __sync_single_inode() should detect that the inode
> > was redirtied while we were flushing it and call redirty_tail() to put
> > it back on the s_dirty list.
> >
> > When redirty_tail() puts it back on the list, it only resets the
> > dirtied_when value if it's necessary to maintain the list order. Given
> > the right situation (the right I/O patterns and a lot of luck), this
> > could result in dirtied_when never getting updated on an inode that's
> > constantly being redirtied while pdflush is writing it back.
> >
> > Since dirtied_when is based on jiffies, it's possible for it to persist
> > across 2 sign-bit flips of jiffies. When that happens, the time_after()
> > check in sync_sb_inodes no longer works correctly and writeouts by
> > pdflush of this inode and any inodes after it on the list stop.
> >
> > This patch fixes this by resetting the dirtied_when value on an inode
> > when we're adding it back onto an empty s_dirty list. Since we generally
> > write inodes from oldest to newest dirtied_when values, this has the
> > effect of making it so that these inodes don't end up with dirtied_when
> > values that are frozen.
> >
> > I've also taken the liberty of fixing up the comments a bit and changed
> > the !time_after_eq() check in redirty_tail to be time_before().  That
> > should be functionally equivalent but I think it's more readable.
> >
> > I wish this were just a theoretical problem, but we've had a customer
> > hit a variant of it in an older kernel. Newer upstream kernels have a
> > number of changes that make this problem less likely. As best I can tell
> > though, there is nothing that really prevents it.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Acked-by: Ian Kent <raven@xxxxxxxxxx>
>
> The assumption is that all inodes heading for the s_dirty list will get 
> their by calling redirty_tail(). It looks like that's the case but, Andrew 
> do you agree the assumption holds?

Oh .. hang on, that's now quite right.

Or that dirtied_when has been set to jiffies at the time of the move 
(aka a newly dirtied inode).

> > ---
> >  fs/fs-writeback.c |   22 +++++++++++++++++-----
> >  1 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index e3fe991..bd2a7ff 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -184,19 +184,31 @@ static int write_inode(struct inode *inode, int sync)
> >   * furthest end of its superblock's dirty-inode list.
> >   *
> >   * Before stamping the inode's ->dirtied_when, we check to see whether it is
> > - * already the most-recently-dirtied inode on the s_dirty list.  If that is
> > - * the case then the inode must have been redirtied while it was being written
> > - * out and we don't reset its dirtied_when.
> > + * "newer" or equal to that of the most-recently-dirtied inode on the s_dirty
> > + * list. If that is the case then we don't need to restamp it to maintain the
> > + * order of the list.
> > + *
> > + * If s_dirty is empty however, then we need to go ahead and update
> > + * dirtied_when for the inode. Not doing so will mean that inodes that are
> > + * constantly being redirtied can end up with "stuck" dirtied_when values if
> > + * they happen to consistently be the first one to go back on the list.
> > + *
> > + * Since we're using jiffies values in that field, letting dirtied_when grow
> > + * too old will be problematic if jiffies wraps. It may also be causing
> > + * pdflush to flush the inode too often since it'll always look like it was
> > + * dirtied a long time ago.
> >   */
> >  static void redirty_tail(struct inode *inode)
> >  {
> >  	struct super_block *sb = inode->i_sb;
> >  
> > -	if (!list_empty(&sb->s_dirty)) {
> > +	if (list_empty(&sb->s_dirty)) {
> > +		inode->dirtied_when = jiffies;
> > +	} else {
> >  		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;
> >  	}
> > --
> > 1.6.0.6
> >
> > --
> > 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/
> --
> 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/

--
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/