[PATCH 1/1] xfs: log the inode in ->write_inode calls for kupdate

[PATCH 1/1] xfs: log the inode in ->write_inode calls for kupdate

[Christoph Hellwig]

If the writeback code writes back an inode because it has expired we currently
use the non-blockin ->write_inode path.  This means any inode that is pinned
is skipped.  With delayed logging and a workload that has very little log
traffic otherwise it is very likely that an inode that gets constantly
written to is always pinned, and thus we keep refusing to write it.  The VM
writeback code at that point redirties it and doesn't try to write it again
for another 30 seconds.  This means under certain scenarious time based
metadata writeback never happens.

Fix this by calling into xfs_log_inode for kupdate in addition to data
integrity syncs, and thus transfer the inode to the log ASAP.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/xfs_super.c	2011-12-14 05:33:07.193262189 -0800
+++ xfs/fs/xfs/xfs_super.c	2011-12-14 05:38:56.108038623 -0800
@@ -905,7 +884,7 @@ xfs_fs_write_inode(
 	if (!ip->i_update_core)
 		return 0;
 
-	if (wbc->sync_mode == WB_SYNC_ALL) {
+	if (wbc->sync_mode == WB_SYNC_ALL || wbc->for_kupdate) {
 		/*
 		 * Make sure the inode has made it it into the log.  Instead
 		 * of forcing it all the way to stable storage using a

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Christoph Hellwig]

Since Linux 2.6.36 the writeback code has introduces various measures for
live lock prevention during sync().  Unfortunately some of these are
actively harmful for the XFS model, where the inode gets marked dirty for
metadata from the data I/O handler.

The older_than_this checks that are now more strictly enforced since

    writeback: avoid livelocking WB_SYNC_ALL writeback

by only calling into __writeback_inodes_sb and thus only sampling the
current cut off time once.  But on a slow enough devices the previous
asynchronous sync pass might not have fully completed yet, and thus XFS
might mark metadata dirty only after that sampling of the cut off time for
the blocking pass already happened.  I have not myself reproduced this
myself on a real system, but by introducing artificial delay into the
XFS I/O completion workqueues it can be reproduced easily.

Fix this by iterating over all XFS inodes in ->sync_fs and log all that
are dirty.  This might log inode that only got redirtied after the
previous pass, but given how cheap delayed logging of inodes is it
isn't a major concern for performance.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/xfs_sync.c	2011-12-14 05:33:06.436599621 -0800
+++ xfs/fs/xfs/xfs_sync.c	2011-12-14 05:38:49.084743337 -0800
@@ -336,6 +336,29 @@ xfs_sync_fsdata(
 	return error;
 }
 
+int
+xfs_log_inode(
+	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
+	int			flags)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_trans	*tp;
+	int			error;
+
+	tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
+	error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
+	if (error) {
+		xfs_trans_cancel(tp, 0);
+		return error;
+	}
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+	return xfs_trans_commit(tp, 0);
+}
+
 /*
  * When remounting a filesystem read-only or freezing the filesystem, we have
  * two phases to execute. This first phase is syncing the data before we
@@ -359,6 +382,16 @@ xfs_quiesce_data(
 {
 	int			error, error2 = 0;
 
+	/*
+	 * Log all pending size and timestamp updates.  The vfs writeback
+	 * code is supposed to do this, but due to its overagressive
+	 * livelock detection it will skip inodes where appending writes
+	 * were written out in the first non-blocking sync phase if their
+	 * completion took long enough that it happened after taking the
+	 * timestamp for the cut-off in the blocking phase.
+	 */
+	xfs_inode_ag_iterator(mp, xfs_log_inode, 0);
+
 	xfs_qm_sync(mp, SYNC_TRYLOCK);
 	xfs_qm_sync(mp, SYNC_WAIT);
 
Index: xfs/fs/xfs/xfs_sync.h
===================================================================
--- xfs.orig/fs/xfs/xfs_sync.h	2011-10-17 00:28:57.255149593 -0700
+++ xfs/fs/xfs/xfs_sync.h	2011-12-14 05:39:23.187891918 -0800
@@ -34,6 +34,8 @@ void xfs_quiesce_attr(struct xfs_mount *
 
 void xfs_flush_inodes(struct xfs_inode *ip);
 
+int xfs_log_inode(struct xfs_inode *ip, struct xfs_perag *pag, int flags);
+
 int xfs_reclaim_inodes(struct xfs_mount *mp, int mode);
 int xfs_reclaim_inodes_count(struct xfs_mount *mp);
 void xfs_reclaim_inodes_nr(struct xfs_mount *mp, int nr_to_scan);
Index: xfs/fs/xfs/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/xfs_super.c	2011-12-14 05:33:07.193262189 -0800
+++ xfs/fs/xfs/xfs_super.c	2011-12-14 05:38:56.108038623 -0800
@@ -869,27 +869,6 @@ xfs_fs_dirty_inode(
 }
 
 STATIC int
-xfs_log_inode(
-	struct xfs_inode	*ip)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*tp;
-	int			error;
-
-	tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
-	error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
-	if (error) {
-		xfs_trans_cancel(tp, 0);
-		return error;
-	}
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-	return xfs_trans_commit(tp, 0);
-}
-
-STATIC int
 xfs_fs_write_inode(
 	struct inode		*inode,
 	struct writeback_control *wbc)
@@ -913,7 +892,7 @@ xfs_fs_write_inode(
 		 * ->sync_fs call do that for thus, which reduces the number
 		 * of synchronous log forces dramatically.
 		 */
-		error = xfs_log_inode(ip);
+		error = xfs_log_inode(ip, NULL, 0);
 		if (error)
 			goto out;
 		return 0;

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Sean Thomas Caron]

Thanks, Christoph. I'll try to get a kernel built tomorrow that  
incorporates these patches and give it some testing.

Best,

-Sean

Quoting Christoph Hellwig <hch@infradead.org>:

> Since Linux 2.6.36 the writeback code has introduces various measures for
> live lock prevention during sync().  Unfortunately some of these are
> actively harmful for the XFS model, where the inode gets marked dirty for
> metadata from the data I/O handler.
>
> The older_than_this checks that are now more strictly enforced since
>
>     writeback: avoid livelocking WB_SYNC_ALL writeback
>
> by only calling into __writeback_inodes_sb and thus only sampling the
> current cut off time once.  But on a slow enough devices the previous
> asynchronous sync pass might not have fully completed yet, and thus XFS
> might mark metadata dirty only after that sampling of the cut off time for
> the blocking pass already happened.  I have not myself reproduced this
> myself on a real system, but by introducing artificial delay into the
> XFS I/O completion workqueues it can be reproduced easily.
>
> Fix this by iterating over all XFS inodes in ->sync_fs and log all that
> are dirty.  This might log inode that only got redirtied after the
> previous pass, but given how cheap delayed logging of inodes is it
> isn't a major concern for performance.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfs/fs/xfs/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_sync.c	2011-12-14 05:33:06.436599621 -0800
> +++ xfs/fs/xfs/xfs_sync.c	2011-12-14 05:38:49.084743337 -0800
> @@ -336,6 +336,29 @@ xfs_sync_fsdata(
>  	return error;
>  }
>
> +int
> +xfs_log_inode(
> +	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
> +	int			flags)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_trans	*tp;
> +	int			error;
> +
> +	tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
> +	error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
> +	if (error) {
> +		xfs_trans_cancel(tp, 0);
> +		return error;
> +	}
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +	return xfs_trans_commit(tp, 0);
> +}
> +
>  /*
>   * When remounting a filesystem read-only or freezing the  
> filesystem, we have
>   * two phases to execute. This first phase is syncing the data before we
> @@ -359,6 +382,16 @@ xfs_quiesce_data(
>  {
>  	int			error, error2 = 0;
>
> +	/*
> +	 * Log all pending size and timestamp updates.  The vfs writeback
> +	 * code is supposed to do this, but due to its overagressive
> +	 * livelock detection it will skip inodes where appending writes
> +	 * were written out in the first non-blocking sync phase if their
> +	 * completion took long enough that it happened after taking the
> +	 * timestamp for the cut-off in the blocking phase.
> +	 */
> +	xfs_inode_ag_iterator(mp, xfs_log_inode, 0);
> +
>  	xfs_qm_sync(mp, SYNC_TRYLOCK);
>  	xfs_qm_sync(mp, SYNC_WAIT);
>
> Index: xfs/fs/xfs/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_sync.h	2011-10-17 00:28:57.255149593 -0700
> +++ xfs/fs/xfs/xfs_sync.h	2011-12-14 05:39:23.187891918 -0800
> @@ -34,6 +34,8 @@ void xfs_quiesce_attr(struct xfs_mount *
>
>  void xfs_flush_inodes(struct xfs_inode *ip);
>
> +int xfs_log_inode(struct xfs_inode *ip, struct xfs_perag *pag, int flags);
> +
>  int xfs_reclaim_inodes(struct xfs_mount *mp, int mode);
>  int xfs_reclaim_inodes_count(struct xfs_mount *mp);
>  void xfs_reclaim_inodes_nr(struct xfs_mount *mp, int nr_to_scan);
> Index: xfs/fs/xfs/xfs_super.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_super.c	2011-12-14 05:33:07.193262189 -0800
> +++ xfs/fs/xfs/xfs_super.c	2011-12-14 05:38:56.108038623 -0800
> @@ -869,27 +869,6 @@ xfs_fs_dirty_inode(
>  }
>
>  STATIC int
> -xfs_log_inode(
> -	struct xfs_inode	*ip)
> -{
> -	struct xfs_mount	*mp = ip->i_mount;
> -	struct xfs_trans	*tp;
> -	int			error;
> -
> -	tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
> -	error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
> -	if (error) {
> -		xfs_trans_cancel(tp, 0);
> -		return error;
> -	}
> -
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> -	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -	return xfs_trans_commit(tp, 0);
> -}
> -
> -STATIC int
>  xfs_fs_write_inode(
>  	struct inode		*inode,
>  	struct writeback_control *wbc)
> @@ -913,7 +892,7 @@ xfs_fs_write_inode(
>  		 * ->sync_fs call do that for thus, which reduces the number
>  		 * of synchronous log forces dramatically.
>  		 */
> -		error = xfs_log_inode(ip);
> +		error = xfs_log_inode(ip, NULL, 0);
>  		if (error)
>  			goto out;
>  		return 0;
>
>
>


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Christoph Hellwig]

On Sun, Dec 18, 2011 at 03:03:04PM -0500, Sean Thomas Caron wrote:
> Thanks, Christoph. I'll try to get a kernel built tomorrow that
> incorporates these patches and give it some testing.

They are equivalent to the test patch I gave to you, just split into
two, properly documented and against 3.2-rc instead of 3.0.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Dave Chinner]

On Sun, Dec 18, 2011 at 10:50:15AM -0500, Christoph Hellwig wrote:
> Since Linux 2.6.36 the writeback code has introduces various measures for
> live lock prevention during sync().  Unfortunately some of these are
> actively harmful for the XFS model, where the inode gets marked dirty for
> metadata from the data I/O handler.
> 
> The older_than_this checks that are now more strictly enforced since
> 
>     writeback: avoid livelocking WB_SYNC_ALL writeback
> 
> by only calling into __writeback_inodes_sb and thus only sampling the
> current cut off time once.  But on a slow enough devices the previous
> asynchronous sync pass might not have fully completed yet, and thus XFS
> might mark metadata dirty only after that sampling of the cut off time for
> the blocking pass already happened.  I have not myself reproduced this
> myself on a real system, but by introducing artificial delay into the
> XFS I/O completion workqueues it can be reproduced easily.
> 
> Fix this by iterating over all XFS inodes in ->sync_fs and log all that
> are dirty.  This might log inode that only got redirtied after the
> previous pass, but given how cheap delayed logging of inodes is it
> isn't a major concern for performance.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_sync.c	2011-12-14 05:33:06.436599621 -0800
> +++ xfs/fs/xfs/xfs_sync.c	2011-12-14 05:38:49.084743337 -0800
> @@ -336,6 +336,29 @@ xfs_sync_fsdata(
>  	return error;
>  }
>  
> +int
> +xfs_log_inode(
> +	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
> +	int			flags)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_trans	*tp;
> +	int			error;
> +
> +	tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
> +	error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
> +	if (error) {
> +		xfs_trans_cancel(tp, 0);
> +		return error;
> +	}
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +	return xfs_trans_commit(tp, 0);
> +}

This will do a transaction on the inode, clean or dirty. That's an
awful lot of overhead for the few inodes (out of perhaps millions in
memory) that actually need it. with the ->dirty_inode callback from
the VFS, we know the only inodes that need logging are those with
i_update_core set....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Christoph Hellwig]

On Mon, Dec 19, 2011 at 09:17:07AM +1100, Dave Chinner wrote:
> This will do a transaction on the inode, clean or dirty. That's an
> awful lot of overhead for the few inodes (out of perhaps millions in
> memory) that actually need it. with the ->dirty_inode callback from
> the VFS, we know the only inodes that need logging are those with
> i_update_core set....

Ooops, I messed that up when forwarding the RFC patch I sent to
Paul & Sean, and that I had been testing with most of the time.  The
original one moved the i_update_core check into xfs_log_inode and that
is how it should be done.  I'll resend it.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Christoph Hellwig]

Since Linux 2.6.36 the writeback code has introduces various measures for
live lock prevention during sync().  Unfortunately some of these are
actively harmful for the XFS model, where the inode gets marked dirty for
metadata from the data I/O handler.

The older_than_this checks that are now more strictly enforced since

    writeback: avoid livelocking WB_SYNC_ALL writeback

by only calling into __writeback_inodes_sb and thus only sampling the
current cut off time once.  But on a slow enough devices the previous
asynchronous sync pass might not have fully completed yet, and thus XFS
might mark metadata dirty only after that sampling of the cut off time for
the blocking pass already happened.  I have not myself reproduced this
myself on a real system, but by introducing artificial delay into the
XFS I/O completion workqueues it can be reproduced easily.

Fix this by iterating over all XFS inodes in ->sync_fs and log all that
are dirty.  This might log inode that only got redirtied after the
previous pass, but given how cheap delayed logging of inodes is it
isn't a major concern for performance.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/xfs_sync.c	2011-12-18 23:35:30.909834592 +0100
+++ xfs/fs/xfs/xfs_sync.c	2011-12-18 23:36:40.823167508 +0100
@@ -336,6 +336,32 @@ xfs_sync_fsdata(
 	return error;
 }
 
+int
+xfs_log_dirty_inode(
+	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
+	int			flags)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_trans	*tp;
+	int			error;
+
+	if (!ip->i_update_core)
+		return 0;
+
+	tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
+	error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
+	if (error) {
+		xfs_trans_cancel(tp, 0);
+		return error;
+	}
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+	return xfs_trans_commit(tp, 0);
+}
+
 /*
  * When remounting a filesystem read-only or freezing the filesystem, we have
  * two phases to execute. This first phase is syncing the data before we
@@ -359,6 +385,16 @@ xfs_quiesce_data(
 {
 	int			error, error2 = 0;
 
+	/*
+	 * Log all pending size and timestamp updates.  The vfs writeback
+	 * code is supposed to do this, but due to its overagressive
+	 * livelock detection it will skip inodes where appending writes
+	 * were written out in the first non-blocking sync phase if their
+	 * completion took long enough that it happened after taking the
+	 * timestamp for the cut-off in the blocking phase.
+	 */
+	xfs_inode_ag_iterator(mp, xfs_log_dirty_inode, 0);
+
 	xfs_qm_sync(mp, SYNC_TRYLOCK);
 	xfs_qm_sync(mp, SYNC_WAIT);
 
Index: xfs/fs/xfs/xfs_sync.h
===================================================================
--- xfs.orig/fs/xfs/xfs_sync.h	2011-12-18 16:47:25.000000000 +0100
+++ xfs/fs/xfs/xfs_sync.h	2011-12-18 23:36:45.119834149 +0100
@@ -34,6 +34,8 @@ void xfs_quiesce_attr(struct xfs_mount *
 
 void xfs_flush_inodes(struct xfs_inode *ip);
 
+int xfs_log_dirty_inode(struct xfs_inode *ip, struct xfs_perag *pag, int flags);
+
 int xfs_reclaim_inodes(struct xfs_mount *mp, int mode);
 int xfs_reclaim_inodes_count(struct xfs_mount *mp);
 void xfs_reclaim_inodes_nr(struct xfs_mount *mp, int nr_to_scan);
Index: xfs/fs/xfs/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/xfs_super.c	2011-12-18 23:35:42.863167854 +0100
+++ xfs/fs/xfs/xfs_super.c	2011-12-18 23:36:42.963167495 +0100
@@ -869,27 +869,6 @@ xfs_fs_dirty_inode(
 }
 
 STATIC int
-xfs_log_inode(
-	struct xfs_inode	*ip)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*tp;
-	int			error;
-
-	tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
-	error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
-	if (error) {
-		xfs_trans_cancel(tp, 0);
-		return error;
-	}
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-	return xfs_trans_commit(tp, 0);
-}
-
-STATIC int
 xfs_fs_write_inode(
 	struct inode		*inode,
 	struct writeback_control *wbc)
@@ -902,8 +881,6 @@ xfs_fs_write_inode(
 
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -XFS_ERROR(EIO);
-	if (!ip->i_update_core)
-		return 0;
 
 	if (wbc->sync_mode == WB_SYNC_ALL || wbc->for_kupdate) {
 		/*
@@ -913,11 +890,14 @@ xfs_fs_write_inode(
 		 * ->sync_fs call do that for thus, which reduces the number
 		 * of synchronous log forces dramatically.
 		 */
-		error = xfs_log_inode(ip);
+		error = xfs_log_dirty_inode(ip, NULL, 0);
 		if (error)
 			goto out;
 		return 0;
 	} else {
+		if (!ip->i_update_core)
+			return 0;
+
 		/*
 		 * We make this non-blocking if the inode is contended, return
 		 * EAGAIN to indicate to the caller that they did not succeed.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/1] xfs: log the inode in ->write_inode calls for kupdate

[Dave Chinner]

On Sun, Dec 18, 2011 at 10:49:55AM -0500, Christoph Hellwig wrote:
> If the writeback code writes back an inode because it has expired we currently
> use the non-blockin ->write_inode path.  This means any inode that is pinned
> is skipped.  With delayed logging and a workload that has very little log
> traffic otherwise it is very likely that an inode that gets constantly
> written to is always pinned, and thus we keep refusing to write it.  The VM
> writeback code at that point redirties it and doesn't try to write it again
> for another 30 seconds.  This means under certain scenarious time based
> metadata writeback never happens.
> 
> Fix this by calling into xfs_log_inode for kupdate in addition to data
> integrity syncs, and thus transfer the inode to the log ASAP.

Makes sense.

> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Dave Chinner]

On Tue, Dec 20, 2011 at 03:08:41PM -0500, Christoph Hellwig wrote:
> Since Linux 2.6.36 the writeback code has introduces various measures for
> live lock prevention during sync().  Unfortunately some of these are
> actively harmful for the XFS model, where the inode gets marked dirty for
> metadata from the data I/O handler.
> 
> The older_than_this checks that are now more strictly enforced since
> 
>     writeback: avoid livelocking WB_SYNC_ALL writeback
> 
> by only calling into __writeback_inodes_sb and thus only sampling the
> current cut off time once.  But on a slow enough devices the previous
> asynchronous sync pass might not have fully completed yet, and thus XFS
> might mark metadata dirty only after that sampling of the cut off time for
> the blocking pass already happened.  I have not myself reproduced this
> myself on a real system, but by introducing artificial delay into the
> XFS I/O completion workqueues it can be reproduced easily.
> 
> Fix this by iterating over all XFS inodes in ->sync_fs and log all that
> are dirty.  This might log inode that only got redirtied after the
> previous pass, but given how cheap delayed logging of inodes is it
> isn't a major concern for performance.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks OK now.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: sync fixes

[Christoph Hellwig]

Sending out the intro mail again as it appears to have been eaten
alive by the list manager.

On Sun, Dec 18, 2011 at 10:49:36AM -0500, Christoph Hellwig wrote:
> These two small fixes fix issues with peridic metadata writeback and
> sync not beeing able write file size updates to disk in some cases.
> 
> The executive summary is:  the writeback code slowly decided that the
> XFS behaviour in corner cases equals to livelocking, and XFS made some
> of this behaviour much more likely since the introduction of the
> delaylog mode.
---end quoted text---

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/1] xfs: log the inode in ->write_inode calls for kupdate

[Mark Tinguely]

Tested-by: Mark Tinguely <tinguely@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Mark Tinguely]

Tested-by: Mark Tinguely <tinguely@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/1] xfs: log the inode in ->write_inode calls for kupdate

[Ben Myers]

On Sun, Dec 18, 2011 at 10:49:55AM -0500, Christoph Hellwig wrote:
> If the writeback code writes back an inode because it has expired we currently
> use the non-blockin ->write_inode path.  This means any inode that is pinned
> is skipped.  With delayed logging and a workload that has very little log
> traffic otherwise it is very likely that an inode that gets constantly
> written to is always pinned, and thus we keep refusing to write it.  The VM
> writeback code at that point redirties it and doesn't try to write it again
> for another 30 seconds.  This means under certain scenarious time based
> metadata writeback never happens.
> 
> Fix this by calling into xfs_log_inode for kupdate in addition to data
> integrity syncs, and thus transfer the inode to the log ASAP.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_super.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_super.c	2011-12-14 05:33:07.193262189 -0800
> +++ xfs/fs/xfs/xfs_super.c	2011-12-14 05:38:56.108038623 -0800
> @@ -905,7 +884,7 @@ xfs_fs_write_inode(

I could not reproduce a situation where this was called with for_kupdate
set and the inode was pinned for very long.  I'm curious if there was an
easy workload to reproduce this.

>  	if (!ip->i_update_core)
>  		return 0;
>  
> -	if (wbc->sync_mode == WB_SYNC_ALL) {
> +	if (wbc->sync_mode == WB_SYNC_ALL || wbc->for_kupdate) {

It looks to me that this is the relevant codepath:

       bdi_writeback_thread .or. bdi_forker_thread
         wb_do_writeback
           wb_check_old_data_flush
             wb_writeback
               writeback_sb_inodes
                 writeback_single_inode
                   write_inode
                     .write_inode
                       xfs_fs_write_inode

AFAICS the only place that for_kupdate is set is in
wb_check_old_data_flush.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>
Reviewed-by: Ben Myers <bpm@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Ben Myers]

Hey Christoph,

On Tue, Dec 20, 2011 at 03:08:41PM -0500, Christoph Hellwig wrote:
> Since Linux 2.6.36 the writeback code has introduces various measures for
> live lock prevention during sync().  Unfortunately some of these are
> actively harmful for the XFS model, where the inode gets marked dirty for
> metadata from the data I/O handler.
> 
> The older_than_this checks that are now more strictly enforced since
> 
>     writeback: avoid livelocking WB_SYNC_ALL writeback
> 
> by only calling into __writeback_inodes_sb and thus only sampling the

Do you mean __writeback_inodes_wb, or perhaps writeback_sb_inodes?

So far I'm not seeing the connection with the commit you've referenced
above, which seems to be related to nr_to_write.  It looks like you're
referring to older fs/fs-writeback.c..

This one seems like it might be relevant:

commit 7624ee7
mm: avoid resetting wb_start after each writeback round

Anyway.. in the 3.2-rcX code it appears that older_than_this is only set
at the start of wb_writeback (excluding for_kupdate being set)...

> current cut off time once.  But on a slow enough devices the previous
> asynchronous sync pass might not have fully completed yet, and thus XFS
> might mark metadata dirty only after that sampling of the cut off time for
> the blocking pass already happened. 

Are you referring to sync_filesystem() calling
__sync_filesystem(sb, 0)
and then
__sync_filesystem(sb, 1 /* wait */)?

Ah... and with that I think I understand what you're after here:  After
__sync_filesystem calls sync_inodes_sb and waits for completion... which
will dirty more inodes in completion handlers... you have the
opportunity to clean up those dirty inodes in .sync_fs.  If that's what
you intend:  I'd say this looks good.

Reviewed-by: Ben Myers <bpm@sgi.com>

Mark also reviewed this.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

THanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Dave Chinner]

On Fri, Dec 23, 2011 at 03:47:03PM -0600, Ben Myers wrote:
> 
> Reviewed-by: Ben Myers <bpm@sgi.com>
> 
> Mark also reviewed this.
> 
> Reviewed-by: Mark Tinguely <tinguely@sgi.com>

Just a process note here: if Mark reviewed the code and is happy
with it, then he needs to send his reviewed-by tag himself. If he's
got concerns, then he needs to discuss them on the list with the
patch author, not just in private with you. If a person's questions
are not posted to the mailing list or posted by proxy and they
didn't aprticipate in discussions on the list, then there is no
evidence that the person ever reviewed the patch. Hence the tag has
no value because it is not verifiable.

More importantly, tags are a semi-formal statement that a set of
actions has been taken by that person - see
Documentation/SubmittingPatches for the actions different tags
imply. Hence it is important the actions they imply are verifiable,
and it also reinforces the fact that they only have value when they
are issued by the email address (or a known alias) in the tag....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/1] xfs: log the inode in ->write_inode calls for kupdate

[Hans-Peter Jansen]

On Sunday 18 December 2011, 16:49:55 Christoph Hellwig wrote:
> If the writeback code writes back an inode because it has expired we
> currently use the non-blockin ->write_inode path.  This means any
> inode that is pinned is skipped.  With delayed logging and a workload
> that has very little log traffic otherwise it is very likely that an
> inode that gets constantly written to is always pinned, and thus we
> keep refusing to write it.  The VM writeback code at that point
> redirties it and doesn't try to write it again for another 30
> seconds.  This means under certain scenarious time based metadata
> writeback never happens.

Wouldn't this qualify as STABLE material then?

> Fix this by calling into xfs_log_inode for kupdate in addition to
> data integrity syncs, and thus transfer the inode to the log ASAP.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfs/fs/xfs/xfs_super.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_super.c	2011-12-14 05:33:07.193262189 -0800
> +++ xfs/fs/xfs/xfs_super.c	2011-12-14 05:38:56.108038623 -0800
> @@ -905,7 +884,7 @@ xfs_fs_write_inode(
>  	if (!ip->i_update_core)
>  		return 0;
>
> -	if (wbc->sync_mode == WB_SYNC_ALL) {
> +	if (wbc->sync_mode == WB_SYNC_ALL || wbc->for_kupdate) {
>  		/*
>  		 * Make sure the inode has made it it into the log.  Instead
>  		 * of forcing it all the way to stable storage using a
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/1] xfs: log the inode in ->write_inode calls for kupdate

[Christoph Hellwig]

On Wed, Dec 28, 2011 at 10:35:56PM +0100, Hans-Peter Jansen wrote:
> On Sunday 18 December 2011, 16:49:55 Christoph Hellwig wrote:
> > If the writeback code writes back an inode because it has expired we
> > currently use the non-blockin ->write_inode path.  This means any
> > inode that is pinned is skipped.  With delayed logging and a workload
> > that has very little log traffic otherwise it is very likely that an
> > inode that gets constantly written to is always pinned, and thus we
> > keep refusing to write it.  The VM writeback code at that point
> > redirties it and doesn't try to write it again for another 30
> > seconds.  This means under certain scenarious time based metadata
> > writeback never happens.
> 
> Wouldn't this qualify as STABLE material then?

Yes, it does.  But for something as complicated as XFS I'm not going to
do a simple Cc: to stable but will apply each patch individually and do
explicit testing of the backport.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Ben Myers]

Hi Dave,

On Mon, Dec 26, 2011 at 11:13:02PM +1100, Dave Chinner wrote:
> On Fri, Dec 23, 2011 at 03:47:03PM -0600, Ben Myers wrote:
> > 
> > Reviewed-by: Ben Myers <bpm@sgi.com>
> > 
> > Mark also reviewed this.
> > 
> > Reviewed-by: Mark Tinguely <tinguely@sgi.com>
> 
> Just a process note here: if Mark reviewed the code and is happy
> with it, then he needs to send his reviewed-by tag himself. If he's
> got concerns, then he needs to discuss them on the list with the
> patch author, not just in private with you. If a person's questions
> are not posted to the mailing list or posted by proxy and they
> didn't aprticipate in discussions on the list, then there is no
> evidence that the person ever reviewed the patch. Hence the tag has
> no value because it is not verifiable.

I tend to agree that it is important to discuss things openly on the
list.  Will make an effort to do more of this.

> More importantly, tags are a semi-formal statement that a set of
> actions has been taken by that person - see
> Documentation/SubmittingPatches for the actions different tags
> imply. Hence it is important the actions they imply are verifiable,
> and it also reinforces the fact that they only have value when they
> are issued by the email address (or a known alias) in the tag....

I don't see anything in SubmittingPatches that says the address on the
>From line not matching a tag is a dealbreaker, and I think that we
should give credit where it is due.  Mark did some work to review and
understand this code in addition to his testing.  I have him a call and
asked him if I could add a 'Reviewed-by' to his 'Tested-by' because I
was suprised he didn't... Next time I'll ask him to send it himself.

I'd like to point out that plenty of the conversation surrounding this
pair of patches seems not to have made it to the list either.   

Anyway... SGI folk will try to keep discussion on the list or on irc
next time.

Folks from umich:  if you offer a 'Tested-by' we'll add that too.

Thanks,
	Ben

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Dave Chinner]

On Thu, Dec 29, 2011 at 09:42:07AM -0600, Ben Myers wrote:
> Hi Dave,
> 
> On Mon, Dec 26, 2011 at 11:13:02PM +1100, Dave Chinner wrote:
> > On Fri, Dec 23, 2011 at 03:47:03PM -0600, Ben Myers wrote:
> > > 
> > > Reviewed-by: Ben Myers <bpm@sgi.com>
> > > 
> > > Mark also reviewed this.
> > > 
> > > Reviewed-by: Mark Tinguely <tinguely@sgi.com>
> > 
> > Just a process note here: if Mark reviewed the code and is happy
> > with it, then he needs to send his reviewed-by tag himself. If he's
> > got concerns, then he needs to discuss them on the list with the
> > patch author, not just in private with you. If a person's questions
> > are not posted to the mailing list or posted by proxy and they
> > didn't aprticipate in discussions on the list, then there is no
> > evidence that the person ever reviewed the patch. Hence the tag has
> > no value because it is not verifiable.
> 
> I tend to agree that it is important to discuss things openly on the
> list.  Will make an effort to do more of this.
> 
> > More importantly, tags are a semi-formal statement that a set of
> > actions has been taken by that person - see
> > Documentation/SubmittingPatches for the actions different tags
> > imply. Hence it is important the actions they imply are verifiable,
> > and it also reinforces the fact that they only have value when they
> > are issued by the email address (or a known alias) in the tag....
> 
> I don't see anything in SubmittingPatches that says the address on the
> From line not matching a tag is a dealbreaker, and I think that we
> should give credit where it is due.  Mark did some work to review and
> understand this code in addition to his testing.

But credit is not what the Reviewed-by tag means - it's a statement
of fact about actions performed by the reviewer. A partial review or
partaking in part of a review does not mean a person can put a
"reviewed-by" tag on a commit. An Acked-by my be appropriate in that
case (though I see them a worthless by their very nature), but
reviewed-by tags are not for "giving credit" to other people that
may have helped you.

Further, keep in mind that I've only ever seen 2 emails from Mark,
so I have no idea who he is or what his capabilities are, so I do
not yet know how much to trust his reviews or testing. Until I've
spend some time interacting with him directly, I won't be able to
form those opinions, and so such tags start at the lower end of
value. They have even less value when they don't come from him and
he hasn't commented where I can see it.

> I have him a call and
> asked him if I could add a 'Reviewed-by' to his 'Tested-by' because I
> was suprised he didn't... Next time I'll ask him to send it himself.

So, perhaps he didn't consider what he did met all the criteria of a
reviewed-by tag? See what I mean about tags being worthless when sent
by proxy?

> I'd like to point out that plenty of the conversation surrounding this
> pair of patches seems not to have made it to the list either.   

Happens all the time, especially on #xfs. The difference is, though,
that such conversations are not "reviews" or result in one person
sending reviewed-by tags for all the participants in the
conversation....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs

[Mark Tinguely]

On 12/29/11 15:44, Dave Chinner wrote:
> On Thu, Dec 29, 2011 at 09:42:07AM -0600, Ben Myers wrote:
>> Hi Dave,
>>
>> On Mon, Dec 26, 2011 at 11:13:02PM +1100, Dave Chinner wrote:
>>> On Fri, Dec 23, 2011 at 03:47:03PM -0600, Ben Myers wrote:
>>>>
>>>> Reviewed-by: Ben Myers<bpm@sgi.com>
>>>>
>>>> Mark also reviewed this.
>>>>
>>>> Reviewed-by: Mark Tinguely<tinguely@sgi.com>
>>>
>>> Just a process note here: if Mark reviewed the code and is happy
>>> with it, then he needs to send his reviewed-by tag himself. If he's
>>> got concerns, then he needs to discuss them on the list with the
>>> patch author, not just in private with you. If a person's questions
>>> are not posted to the mailing list or posted by proxy and they
>>> didn't aprticipate in discussions on the list, then there is no
>>> evidence that the person ever reviewed the patch. Hence the tag has
>>> no value because it is not verifiable.
>>
>> I tend to agree that it is important to discuss things openly on the
>> list.  Will make an effort to do more of this.
>>
>>> More importantly, tags are a semi-formal statement that a set of
>>> actions has been taken by that person - see
>>> Documentation/SubmittingPatches for the actions different tags
>>> imply. Hence it is important the actions they imply are verifiable,
>>> and it also reinforces the fact that they only have value when they
>>> are issued by the email address (or a known alias) in the tag....
>>
>> I don't see anything in SubmittingPatches that says the address on the
>>  From line not matching a tag is a dealbreaker, and I think that we
>> should give credit where it is due.  Mark did some work to review and
>> understand this code in addition to his testing.
>
> But credit is not what the Reviewed-by tag means - it's a statement
> of fact about actions performed by the reviewer. A partial review or
> partaking in part of a review does not mean a person can put a
> "reviewed-by" tag on a commit. An Acked-by my be appropriate in that
> case (though I see them a worthless by their very nature), but
> reviewed-by tags are not for "giving credit" to other people that
> may have helped you.
>
> Further, keep in mind that I've only ever seen 2 emails from Mark,
> so I have no idea who he is or what his capabilities are, so I do
> not yet know how much to trust his reviews or testing. Until I've
> spend some time interacting with him directly, I won't be able to
> form those opinions, and so such tags start at the lower end of
> value. They have even less value when they don't come from him and
> he hasn't commented where I can see it.
>
>> I have him a call and
>> asked him if I could add a 'Reviewed-by' to his 'Tested-by' because I
>> was suprised he didn't... Next time I'll ask him to send it himself.
>
> So, perhaps he didn't consider what he did met all the criteria of a
> reviewed-by tag? See what I mean about tags being worthless when sent
> by proxy?
>
>> I'd like to point out that plenty of the conversation surrounding this
>> pair of patches seems not to have made it to the list either.
>
> Happens all the time, especially on #xfs. The difference is, though,
> that such conversations are not "reviews" or result in one person
> sending reviewed-by tags for all the participants in the
> conversation....
>
> Cheers,
>
> Dave.

Thank-you for the feedback. There are several good points made in this 
thread.

--Mark Tinguely.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs