[PATCH 0/3] try to avoid locked inodes during log force

[PATCH 0/3] try to avoid locked inodes during log force

[Christoph Hellwig]

This series fixes a few places to unlock the inode before doing
synchronous log forces to avoid potential overly long log hold
times.

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

[PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync

[Christoph Hellwig]

[-- Attachment #1: xfs-optimize-fsync-2 --]
[-- Type: text/plain, Size: 3561 bytes --]

Only read the LSN we need to push to with the ilock held, and then release
it before we do the log force to improve concurrency.

This also removes the only direct caller of _xfs_trans_commit, thus
allowing it to be merged into the plain xfs_trans_commit again.

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

Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c	2011-09-07 10:59:55.742961175 +0200
+++ xfs/fs/xfs/xfs_file.c	2011-09-07 11:08:12.830462974 +0200
@@ -137,6 +137,7 @@ xfs_file_fsync(
 	struct xfs_trans	*tp;
 	int			error = 0;
 	int			log_flushed = 0;
+	xfs_lsn_t		lsn = 0;
 
 	trace_xfs_file_fsync(ip);
 
@@ -214,8 +215,10 @@ xfs_file_fsync(
 		 */
 		xfs_trans_ijoin(tp, ip);
 		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-		xfs_trans_set_sync(tp);
-		error = _xfs_trans_commit(tp, 0, &log_flushed);
+		error = xfs_trans_commit(tp, 0);
+
+		ASSERT(xfs_ipincount(ip));
+		lsn = ip->i_itemp->ili_last_lsn;
 
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	} else {
@@ -227,14 +230,14 @@ xfs_file_fsync(
 		 * disk yet, the inode will be still be pinned.  If it is,
 		 * force the log.
 		 */
-		if (xfs_ipincount(ip)) {
-			error = _xfs_log_force_lsn(mp,
-					ip->i_itemp->ili_last_lsn,
-					XFS_LOG_SYNC, &log_flushed);
-		}
+		if (xfs_ipincount(ip))
+			lsn = ip->i_itemp->ili_last_lsn;
 		xfs_iunlock(ip, XFS_ILOCK_SHARED);
 	}
 
+	if (!error && lsn)
+		error = _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed);
+
 	/*
 	 * If we only have a single device, and the log force about was
 	 * a no-op we might have to flush the data device cache here.
Index: xfs/fs/xfs/xfs_trans.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans.c	2011-09-07 10:59:55.702959720 +0200
+++ xfs/fs/xfs/xfs_trans.c	2011-09-07 11:06:20.686461455 +0200
@@ -1790,9 +1790,7 @@ xfs_trans_commit_cil(
 }
 
 /*
- * xfs_trans_commit
- *
- * Commit the given transaction to the log a/synchronously.
+ * Commit the given transaction to the log.
  *
  * XFS disk error handling mechanism is not based on a typical
  * transaction abort mechanism. Logically after the filesystem
@@ -1804,10 +1802,9 @@ xfs_trans_commit_cil(
  * Do not reference the transaction structure after this call.
  */
 int
-_xfs_trans_commit(
+xfs_trans_commit(
 	struct xfs_trans	*tp,
-	uint			flags,
-	int			*log_flushed)
+	uint			flags)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	xfs_lsn_t		commit_lsn = -1;
@@ -1866,7 +1863,7 @@ _xfs_trans_commit(
 	if (sync) {
 		if (!error) {
 			error = _xfs_log_force_lsn(mp, commit_lsn,
-				      XFS_LOG_SYNC, log_flushed);
+				      XFS_LOG_SYNC, NULL);
 		}
 		XFS_STATS_INC(xs_trans_sync);
 	} else {
Index: xfs/fs/xfs/xfs_trans.h
===================================================================
--- xfs.orig/fs/xfs/xfs_trans.h	2011-09-07 10:59:55.722959398 +0200
+++ xfs/fs/xfs/xfs_trans.h	2011-09-07 11:05:11.766461375 +0200
@@ -487,10 +487,7 @@ void		xfs_trans_log_efd_extent(xfs_trans
 					 struct xfs_efd_log_item *,
 					 xfs_fsblock_t,
 					 xfs_extlen_t);
-int		_xfs_trans_commit(xfs_trans_t *,
-				  uint flags,
-				  int *);
-#define xfs_trans_commit(tp, flags)	_xfs_trans_commit(tp, flags, NULL)
+int		xfs_trans_commit(xfs_trans_t *, uint flags);
 void		xfs_trans_cancel(xfs_trans_t *, int);
 int		xfs_trans_ail_init(struct xfs_mount *);
 void		xfs_trans_ail_destroy(struct xfs_mount *);

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

[PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata

[Christoph Hellwig]

[-- Attachment #1: xfs-optimize-commit_metadata --]
[-- Type: text/plain, Size: 1119 bytes --]

Only read the LSN we need to push to with the ilock held, and then release
it before we do the log force to improve concurrency.

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

Index: xfs/fs/xfs/xfs_export.c
===================================================================
--- xfs.orig/fs/xfs/xfs_export.c	2011-09-07 11:08:48.670961391 +0200
+++ xfs/fs/xfs/xfs_export.c	2011-09-07 11:08:56.515960258 +0200
@@ -229,16 +229,16 @@ xfs_fs_nfs_commit_metadata(
 {
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
-	int			error = 0;
+	xfs_lsn_t		lsn = 0;
 
 	xfs_ilock(ip, XFS_ILOCK_SHARED);
-	if (xfs_ipincount(ip)) {
-		error = _xfs_log_force_lsn(mp, ip->i_itemp->ili_last_lsn,
-				XFS_LOG_SYNC, NULL);
-	}
+	if (xfs_ipincount(ip))
+		lsn = ip->i_itemp->ili_last_lsn;
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
 
-	return error;
+	if (!lsn)
+		return 0;
+	return _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL);
 }
 
 const struct export_operations xfs_export_operations = {

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

[PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space

[Christoph Hellwig]

[-- Attachment #1: xfs-optimize-osync-prealloc --]
[-- Type: text/plain, Size: 1024 bytes --]

Let the transaction commit unlock the inode before it potentially causes
a synchronous log force.

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

Index: xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.c	2011-09-18 11:41:41.000000000 -0400
+++ xfs/fs/xfs/xfs_vnodeops.c	2011-09-18 11:43:05.951778942 -0400
@@ -2342,8 +2342,7 @@ xfs_change_file_space(
 	}
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-
-	xfs_trans_ijoin(tp, ip);
+	xfs_trans_ijoin_ref(tp, ip, XFS_ILOCK_EXCL);
 
 	if ((attr_flags & XFS_ATTR_DMI) == 0) {
 		ip->i_d.di_mode &= ~S_ISUID;
@@ -2368,10 +2367,5 @@ xfs_change_file_space(
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 	if (attr_flags & XFS_ATTR_SYNC)
 		xfs_trans_set_sync(tp);
-
-	error = xfs_trans_commit(tp, 0);
-
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
-
-	return error;
+	return xfs_trans_commit(tp, 0);
 }

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

Re: [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync

[Dave Chinner]

On Sun, Sep 18, 2011 at 04:47:49PM -0400, Christoph Hellwig wrote:
> Only read the LSN we need to push to with the ilock held, and then release
> it before we do the log force to improve concurrency.
> 
> This also removes the only direct caller of _xfs_trans_commit, thus
> allowing it to be merged into the plain xfs_trans_commit again.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_file.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_file.c	2011-09-07 10:59:55.742961175 +0200
> +++ xfs/fs/xfs/xfs_file.c	2011-09-07 11:08:12.830462974 +0200
> @@ -137,6 +137,7 @@ xfs_file_fsync(
>  	struct xfs_trans	*tp;
>  	int			error = 0;
>  	int			log_flushed = 0;
> +	xfs_lsn_t		lsn = 0;
>  
>  	trace_xfs_file_fsync(ip);
>  
> @@ -214,8 +215,10 @@ xfs_file_fsync(
>  		 */
>  		xfs_trans_ijoin(tp, ip);
>  		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -		xfs_trans_set_sync(tp);
> -		error = _xfs_trans_commit(tp, 0, &log_flushed);
> +		error = xfs_trans_commit(tp, 0);
> +
> +		ASSERT(xfs_ipincount(ip));

That's a racy assert. If the trans commit causes the CIL to be
pushed, that could complete and unpin the inode before we start
executing here again and check the pin count.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata

[Dave Chinner]

On Sun, Sep 18, 2011 at 04:47:50PM -0400, Christoph Hellwig wrote:
> Only read the LSN we need to push to with the ilock held, and then release
> it before we do the log force to improve concurrency.
> 
> 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 3/3] xfs: unlock the inode before log force in xfs_change_file_space

[Dave Chinner]

On Sun, Sep 18, 2011 at 04:47:51PM -0400, Christoph Hellwig wrote:
> Let the transaction commit unlock the inode before it potentially causes
> a synchronous log force.
> 
> 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

[PATCH 1/3 v2] xfs: unlock the inode before log force in xfs_fsync

[Christoph Hellwig]

Only read the LSN we need to push to with the ilock held, and then release
it before we do the log force to improve concurrency.

This also removes the only direct caller of _xfs_trans_commit, thus
allowing it to be merged into the plain xfs_trans_commit again.

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

Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c	2011-09-18 16:34:03.995780628 -0400
+++ xfs/fs/xfs/xfs_file.c	2011-09-19 10:47:07.679732387 -0400
@@ -137,6 +137,7 @@ xfs_file_fsync(
 	struct xfs_trans	*tp;
 	int			error = 0;
 	int			log_flushed = 0;
+	xfs_lsn_t		lsn = 0;
 
 	trace_xfs_file_fsync(ip);
 
@@ -214,9 +215,9 @@ xfs_file_fsync(
 		 */
 		xfs_trans_ijoin(tp, ip);
 		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-		xfs_trans_set_sync(tp);
-		error = _xfs_trans_commit(tp, 0, &log_flushed);
+		error = xfs_trans_commit(tp, 0);
 
+		lsn = ip->i_itemp->ili_last_lsn;
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	} else {
 		/*
@@ -227,14 +228,14 @@ xfs_file_fsync(
 		 * disk yet, the inode will be still be pinned.  If it is,
 		 * force the log.
 		 */
-		if (xfs_ipincount(ip)) {
-			error = _xfs_log_force_lsn(mp,
-					ip->i_itemp->ili_last_lsn,
-					XFS_LOG_SYNC, &log_flushed);
-		}
+		if (xfs_ipincount(ip))
+			lsn = ip->i_itemp->ili_last_lsn;
 		xfs_iunlock(ip, XFS_ILOCK_SHARED);
 	}
 
+	if (!error && lsn)
+		error = _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed);
+
 	/*
 	 * If we only have a single device, and the log force about was
 	 * a no-op we might have to flush the data device cache here.
Index: xfs/fs/xfs/xfs_trans.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans.c	2011-09-18 16:31:44.255777336 -0400
+++ xfs/fs/xfs/xfs_trans.c	2011-09-19 10:46:47.181231805 -0400
@@ -1790,9 +1790,7 @@ xfs_trans_commit_cil(
 }
 
 /*
- * xfs_trans_commit
- *
- * Commit the given transaction to the log a/synchronously.
+ * Commit the given transaction to the log.
  *
  * XFS disk error handling mechanism is not based on a typical
  * transaction abort mechanism. Logically after the filesystem
@@ -1804,10 +1802,9 @@ xfs_trans_commit_cil(
  * Do not reference the transaction structure after this call.
  */
 int
-_xfs_trans_commit(
+xfs_trans_commit(
 	struct xfs_trans	*tp,
-	uint			flags,
-	int			*log_flushed)
+	uint			flags)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	xfs_lsn_t		commit_lsn = -1;
@@ -1866,7 +1863,7 @@ _xfs_trans_commit(
 	if (sync) {
 		if (!error) {
 			error = _xfs_log_force_lsn(mp, commit_lsn,
-				      XFS_LOG_SYNC, log_flushed);
+				      XFS_LOG_SYNC, NULL);
 		}
 		XFS_STATS_INC(xs_trans_sync);
 	} else {
Index: xfs/fs/xfs/xfs_trans.h
===================================================================
--- xfs.orig/fs/xfs/xfs_trans.h	2011-09-18 16:31:44.271777154 -0400
+++ xfs/fs/xfs/xfs_trans.h	2011-09-19 10:46:47.081232193 -0400
@@ -487,10 +487,7 @@ void		xfs_trans_log_efd_extent(xfs_trans
 					 struct xfs_efd_log_item *,
 					 xfs_fsblock_t,
 					 xfs_extlen_t);
-int		_xfs_trans_commit(xfs_trans_t *,
-				  uint flags,
-				  int *);
-#define xfs_trans_commit(tp, flags)	_xfs_trans_commit(tp, flags, NULL)
+int		xfs_trans_commit(xfs_trans_t *, uint flags);
 void		xfs_trans_cancel(xfs_trans_t *, int);
 int		xfs_trans_ail_init(struct xfs_mount *);
 void		xfs_trans_ail_destroy(struct xfs_mount *);

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

Re: [PATCH 1/3] xfs: unlock the inode before log force in xfs_fsync

[Alex Elder]

On Sun, 2011-09-18 at 16:47 -0400, Christoph Hellwig wrote:
> Only read the LSN we need to push to with the ilock held, and then release
> it before we do the log force to improve concurrency.
> 
> This also removes the only direct caller of _xfs_trans_commit, thus
> allowing it to be merged into the plain xfs_trans_commit again.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

The idea here is that once the transaction is committed
(which also sets the LSN to use), everything we need to
know about any changes are in the log so there's no need
to prevent others from moving on from that point--allowing
the inode to (transactionally) change to a new state--while
the log gets forced out.  Right?

Reviewed-by: Alex Elder <aelder@sgi.com>



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

Re: [PATCH 2/3] xfs: unlock the inode before log force in xfs_fs_nfs_commit_metadata

[Alex Elder]

On Sun, 2011-09-18 at 16:47 -0400, Christoph Hellwig wrote:
> Only read the LSN we need to push to with the ilock held, and then release
> it before we do the log force to improve concurrency.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>

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

Re: [PATCH 3/3] xfs: unlock the inode before log force in xfs_change_file_space

[Alex Elder]

On Sun, 2011-09-18 at 16:47 -0400, Christoph Hellwig wrote:
> Let the transaction commit unlock the inode before it potentially causes
> a synchronous log force.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>


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