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

[Wu Fengguang <fengguang.wu@intel.com>]

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

---
 fs/fs-writeback.c |   14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

--- mm.orig/fs/fs-writeback.c
+++ mm/fs/fs-writeback.c
@@ -182,24 +182,12 @@ static int write_inode(struct inode *ino
 /*
  * Redirty an inode: set its when-it-was dirtied timestamp and move it to the
  * 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.
  */
 static void redirty_tail(struct inode *inode)
 {
 	struct super_block *sb = inode->i_sb;
 
-	if (!list_empty(&sb->s_dirty)) {
-		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))
-			inode->dirtied_when = jiffies;
-	}
+	inode->dirtied_when = jiffies;
 	list_move(&inode->i_list, &sb->s_dirty);
 }