Date: Tue, 24 Mar 2009 10:46:57 -0400
From: Jeff Layton <jlayton@redhat.com>
To: Jeff Layton <jlayton@redhat.com>
Cc: Wu Fengguang <fengguang.wu@intel.com>, linux-kernel@vger.kernel.org,
       linux-fsdevel@vger.kernel.org, jens.axboe@oracle.com,
       akpm@linux-foundation.org, hch@infradead.org
Subject: Re: [PATCH] writeback: reset inode dirty time when adding it back
 to empty s_dirty list
Message-ID: <20090324104657.6907b19e@tleilax.poochiereds.net>
In-Reply-To: <20090324102806.4f38fd26@tleilax.poochiereds.net>
References: <1237840233-11045-1-git-send-email-jlayton@redhat.com>
	<20090324135720.GA25314@localhost>
	<20090324102806.4f38fd26@tleilax.poochiereds.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6765
Lines: 146

On Tue, 24 Mar 2009 10:28:06 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Tue, 24 Mar 2009 21:57:20 +0800
> Wu Fengguang <fengguang.wu@intel.com> wrote:
> 
> > Hi Jeff,
> > 
> > On Mon, Mar 23, 2009 at 04:30:33PM -0400, 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@redhat.com>
> > > ---
> > >  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;
> > >  	}
> > 
> > I'm afraid you patch is equivalent to the following one.
> > Because once the first inode's dirtied_when is set to jiffies,
> > in order to keep the list in order, the following ones (mostly)
> > will also be updated. A domino effect.
> > 
> > Thanks,
> > Fengguang
> > 
> 
> Good point. One of our other engineers proposed a similar patch
> originally. I considered it but wasn't clear whether there could be a
> situation where unconditionally resetting dirtied_when would be a
> problem. Now that I think about it though, I think you're right...
> 
> So maybe something like the patch below is the right thing to do? Or,
> maybe when we believe that the inode was fully cleaned and then
> redirtied, we'd just unconditionally stamp dirtied_when. Something like
> this maybe?
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index bd2a7ff..596c96e 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -364,7 +364,8 @@ __sync_single_inode(struct inode *inode, struct writeback_control *wbc)
>  			 * Someone redirtied the inode while were writing back
>  			 * the pages.
>  			 */
> -			redirty_tail(inode);
> +			inode->dirtied_when = jiffies;
> +			list_move(&inode->i_list, &sb->s_dirty);
>  		} else if (atomic_read(&inode->i_count)) {
>  			/*
>  			 * The inode is clean, inuse

Hmm...though it is still possible that you could consistently race in
such a way that after writepages(), I_DIRTY is never set but the
PAGECACHE_TAG_DIRTY is still set on the mapping. And then we'd be back
to the same problem of a stuck dirtied_when value.

So maybe someone can explain to me why we take such great pains to
preserve the dirtied_when value when we're putting the inode back on
the tail of s_dirty? Why not just unconditionally reset it?

-- 
Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/