xfs_log_force related cleanups

xfs_log_force related cleanups

[Christoph Hellwig]

A couple cleanups and refactorig around xfs_log_force and friends.

[PATCH 1/8] xfs: remove misleading comment text on xfs_inode_item_unlock

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode_item.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index d5037f060d6f..b83fa38f1c74 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -579,9 +579,6 @@ xfs_inode_item_push(
 
 /*
  * Unlock the inode associated with the inode log item.
- * Clear the fields of the inode and inode log item that
- * are specific to the current transaction.  If the
- * hold flags is set, do not unlock the inode.
  */
 STATIC void
 xfs_inode_item_unlock(
-- 
2.14.2

Re: [PATCH 1/8] xfs: remove misleading comment text on xfs_inode_item_unlock

[Darrick J. Wong]

On Tue, Mar 13, 2018 at 11:49:20AM +0100, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_inode_item.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index d5037f060d6f..b83fa38f1c74 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -579,9 +579,6 @@ xfs_inode_item_push(
>  
>  /*
>   * Unlock the inode associated with the inode log item.
> - * Clear the fields of the inode and inode log item that
> - * are specific to the current transaction.  If the
> - * hold flags is set, do not unlock the inode.
>   */
>  STATIC void
>  xfs_inode_item_unlock(
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/8] xfs: remove an outdated comment for xfs_inode_item_committing

[Christoph Hellwig]

The function now does something, and that something is central to our
inode logging scheme.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode_item.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index b83fa38f1c74..31f2b32b7e27 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -634,10 +634,6 @@ xfs_inode_item_committed(
 	return lsn;
 }
 
-/*
- * XXX rcc - this one really has to do something.  Probably needs
- * to stamp in a new field in the incore inode.
- */
 STATIC void
 xfs_inode_item_committing(
 	struct xfs_log_item	*lip,
-- 
2.14.2

Re: [PATCH 2/8] xfs: remove an outdated comment for xfs_inode_item_committing

[Darrick J. Wong]

On Tue, Mar 13, 2018 at 11:49:21AM +0100, Christoph Hellwig wrote:
> The function now does something, and that something is central to our
> inode logging scheme.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_inode_item.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index b83fa38f1c74..31f2b32b7e27 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -634,10 +634,6 @@ xfs_inode_item_committed(
>  	return lsn;
>  }
>  
> -/*
> - * XXX rcc - this one really has to do something.  Probably needs
> - * to stamp in a new field in the incore inode.
> - */
>  STATIC void
>  xfs_inode_item_committing(
>  	struct xfs_log_item	*lip,
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/8] xfs: remove the unused log_flushed variable in xfs_extent_busy_flush

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extent_busy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index 77760dbf0242..1b439db95634 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -611,10 +611,10 @@ xfs_extent_busy_flush(
 	unsigned		busy_gen)
 {
 	DEFINE_WAIT		(wait);
-	int			log_flushed = 0, error;
+	int			error;
 
 	trace_xfs_log_force(mp, 0, _THIS_IP_);
-	error = _xfs_log_force(mp, XFS_LOG_SYNC, &log_flushed);
+	error = _xfs_log_force(mp, XFS_LOG_SYNC, NULL);
 	if (error)
 		return;
 
-- 
2.14.2

Re: [PATCH 3/8] xfs: remove the unused log_flushed variable in xfs_extent_busy_flush

[Darrick J. Wong]

On Tue, Mar 13, 2018 at 11:49:22AM +0100, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_extent_busy.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> index 77760dbf0242..1b439db95634 100644
> --- a/fs/xfs/xfs_extent_busy.c
> +++ b/fs/xfs/xfs_extent_busy.c
> @@ -611,10 +611,10 @@ xfs_extent_busy_flush(
>  	unsigned		busy_gen)
>  {
>  	DEFINE_WAIT		(wait);
> -	int			log_flushed = 0, error;
> +	int			error;
>  
>  	trace_xfs_log_force(mp, 0, _THIS_IP_);
> -	error = _xfs_log_force(mp, XFS_LOG_SYNC, &log_flushed);
> +	error = _xfs_log_force(mp, XFS_LOG_SYNC, NULL);
>  	if (error)
>  		return;
>  
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/8] xfs: merge _xfs_log_force and xfs_log_force

[Christoph Hellwig]

Switch to a single interface for flushing the whole log, which gives
consistent trace point coverage, and removes the unused log_flushed
argument for the previous _xfs_log_force callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/common.c    |  2 +-
 fs/xfs/xfs_extent_busy.c |  3 +--
 fs/xfs/xfs_log.c         | 26 +++++---------------------
 fs/xfs/xfs_log.h         |  6 +-----
 4 files changed, 8 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 8033ab9d8f47..ddcdda336402 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -619,7 +619,7 @@ xfs_scrub_checkpoint_log(
 {
 	int			error;
 
-	error = _xfs_log_force(mp, XFS_LOG_SYNC, NULL);
+	error = xfs_log_force(mp, XFS_LOG_SYNC);
 	if (error)
 		return error;
 	xfs_ail_push_all_sync(mp->m_ail);
diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index 1b439db95634..13e3d1a69e76 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -613,8 +613,7 @@ xfs_extent_busy_flush(
 	DEFINE_WAIT		(wait);
 	int			error;
 
-	trace_xfs_log_force(mp, 0, _THIS_IP_);
-	error = _xfs_log_force(mp, XFS_LOG_SYNC, NULL);
+	error = xfs_log_force(mp, XFS_LOG_SYNC);
 	if (error)
 		return;
 
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 3e5ba1ecc080..dbb0d69445e8 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -869,7 +869,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 		return 0;
 	}
 
-	error = _xfs_log_force(mp, XFS_LOG_SYNC, NULL);
+	error = xfs_log_force(mp, XFS_LOG_SYNC);
 	ASSERT(error || !(XLOG_FORCED_SHUTDOWN(log)));
 
 #ifdef DEBUG
@@ -3304,16 +3304,16 @@ xlog_state_switch_iclogs(
  *		not in the active nor dirty state.
  */
 int
-_xfs_log_force(
+xfs_log_force(
 	struct xfs_mount	*mp,
-	uint			flags,
-	int			*log_flushed)
+	uint			flags)
 {
 	struct xlog		*log = mp->m_log;
 	struct xlog_in_core	*iclog;
 	xfs_lsn_t		lsn;
 
 	XFS_STATS_INC(mp, xs_log_force);
+	trace_xfs_log_force(mp, 0, _RET_IP_);
 
 	xlog_cil_force(log);
 
@@ -3362,8 +3362,6 @@ _xfs_log_force(
 				if (xlog_state_release_iclog(log, iclog))
 					return -EIO;
 
-				if (log_flushed)
-					*log_flushed = 1;
 				spin_lock(&log->l_icloglock);
 				if (be64_to_cpu(iclog->ic_header.h_lsn) == lsn &&
 				    iclog->ic_state != XLOG_STATE_DIRTY)
@@ -3415,20 +3413,6 @@ _xfs_log_force(
 	return 0;
 }
 
-/*
- * Wrapper for _xfs_log_force(), to be used when caller doesn't care
- * about errors or whether the log was flushed or not. This is the normal
- * interface to use when trying to unpin items or move the log forward.
- */
-void
-xfs_log_force(
-	xfs_mount_t	*mp,
-	uint		flags)
-{
-	trace_xfs_log_force(mp, 0, _RET_IP_);
-	_xfs_log_force(mp, flags, NULL);
-}
-
 /*
  * Force the in-core log to disk for a specific LSN.
  *
@@ -4035,7 +4019,7 @@ xfs_log_force_umount(
 	 * to guarantee this.
 	 */
 	if (!logerror)
-		_xfs_log_force(mp, XFS_LOG_SYNC, NULL);
+		xfs_log_force(mp, XFS_LOG_SYNC);
 
 	/*
 	 * mark the filesystem and the as in a shutdown state and wake
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index bf212772595c..726dd9a330b4 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -129,11 +129,7 @@ xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
 		       struct xlog_ticket *ticket,
 		       struct xlog_in_core **iclog,
 		       bool regrant);
-int	  _xfs_log_force(struct xfs_mount *mp,
-			 uint		flags,
-			 int		*log_forced);
-void	  xfs_log_force(struct xfs_mount	*mp,
-			uint			flags);
+int	  xfs_log_force(struct xfs_mount *mp, uint flags);
 int	  _xfs_log_force_lsn(struct xfs_mount *mp,
 			     xfs_lsn_t		lsn,
 			     uint		flags,
-- 
2.14.2

Re: [PATCH 4/8] xfs: merge _xfs_log_force and xfs_log_force

[Darrick J. Wong]

On Tue, Mar 13, 2018 at 11:49:23AM +0100, Christoph Hellwig wrote:
> Switch to a single interface for flushing the whole log, which gives
> consistent trace point coverage, and removes the unused log_flushed
> argument for the previous _xfs_log_force callers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/scrub/common.c    |  2 +-
>  fs/xfs/xfs_extent_busy.c |  3 +--
>  fs/xfs/xfs_log.c         | 26 +++++---------------------
>  fs/xfs/xfs_log.h         |  6 +-----
>  4 files changed, 8 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index 8033ab9d8f47..ddcdda336402 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -619,7 +619,7 @@ xfs_scrub_checkpoint_log(
>  {
>  	int			error;
>  
> -	error = _xfs_log_force(mp, XFS_LOG_SYNC, NULL);
> +	error = xfs_log_force(mp, XFS_LOG_SYNC);
>  	if (error)
>  		return error;
>  	xfs_ail_push_all_sync(mp->m_ail);
> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> index 1b439db95634..13e3d1a69e76 100644
> --- a/fs/xfs/xfs_extent_busy.c
> +++ b/fs/xfs/xfs_extent_busy.c
> @@ -613,8 +613,7 @@ xfs_extent_busy_flush(
>  	DEFINE_WAIT		(wait);
>  	int			error;
>  
> -	trace_xfs_log_force(mp, 0, _THIS_IP_);
> -	error = _xfs_log_force(mp, XFS_LOG_SYNC, NULL);
> +	error = xfs_log_force(mp, XFS_LOG_SYNC);
>  	if (error)
>  		return;
>  
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 3e5ba1ecc080..dbb0d69445e8 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -869,7 +869,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		return 0;
>  	}
>  
> -	error = _xfs_log_force(mp, XFS_LOG_SYNC, NULL);
> +	error = xfs_log_force(mp, XFS_LOG_SYNC);
>  	ASSERT(error || !(XLOG_FORCED_SHUTDOWN(log)));
>  
>  #ifdef DEBUG
> @@ -3304,16 +3304,16 @@ xlog_state_switch_iclogs(
>   *		not in the active nor dirty state.
>   */
>  int
> -_xfs_log_force(
> +xfs_log_force(
>  	struct xfs_mount	*mp,
> -	uint			flags,
> -	int			*log_flushed)
> +	uint			flags)
>  {
>  	struct xlog		*log = mp->m_log;
>  	struct xlog_in_core	*iclog;
>  	xfs_lsn_t		lsn;
>  
>  	XFS_STATS_INC(mp, xs_log_force);
> +	trace_xfs_log_force(mp, 0, _RET_IP_);
>  
>  	xlog_cil_force(log);
>  
> @@ -3362,8 +3362,6 @@ _xfs_log_force(
>  				if (xlog_state_release_iclog(log, iclog))
>  					return -EIO;
>  
> -				if (log_flushed)
> -					*log_flushed = 1;
>  				spin_lock(&log->l_icloglock);
>  				if (be64_to_cpu(iclog->ic_header.h_lsn) == lsn &&
>  				    iclog->ic_state != XLOG_STATE_DIRTY)
> @@ -3415,20 +3413,6 @@ _xfs_log_force(
>  	return 0;
>  }
>  
> -/*
> - * Wrapper for _xfs_log_force(), to be used when caller doesn't care
> - * about errors or whether the log was flushed or not. This is the normal
> - * interface to use when trying to unpin items or move the log forward.
> - */
> -void
> -xfs_log_force(
> -	xfs_mount_t	*mp,
> -	uint		flags)
> -{
> -	trace_xfs_log_force(mp, 0, _RET_IP_);
> -	_xfs_log_force(mp, flags, NULL);
> -}
> -
>  /*
>   * Force the in-core log to disk for a specific LSN.
>   *
> @@ -4035,7 +4019,7 @@ xfs_log_force_umount(
>  	 * to guarantee this.
>  	 */
>  	if (!logerror)
> -		_xfs_log_force(mp, XFS_LOG_SYNC, NULL);
> +		xfs_log_force(mp, XFS_LOG_SYNC);
>  
>  	/*
>  	 * mark the filesystem and the as in a shutdown state and wake
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index bf212772595c..726dd9a330b4 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -129,11 +129,7 @@ xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
>  		       struct xlog_ticket *ticket,
>  		       struct xlog_in_core **iclog,
>  		       bool regrant);
> -int	  _xfs_log_force(struct xfs_mount *mp,
> -			 uint		flags,
> -			 int		*log_forced);
> -void	  xfs_log_force(struct xfs_mount	*mp,
> -			uint			flags);
> +int	  xfs_log_force(struct xfs_mount *mp, uint flags);
>  int	  _xfs_log_force_lsn(struct xfs_mount *mp,
>  			     xfs_lsn_t		lsn,
>  			     uint		flags,
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/8] xfs: merge _xfs_log_force_lsn and xfs_log_force_lsn

[Christoph Hellwig]

Switch to a single interface for flushing the log to a specific LSN, which
gives consistent trace point coverage and a less confusing interface.

The was only a single user of the previous xfs_log_force_lsn function,
which now also passes a NULL log_flushed argument.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_export.c |  2 +-
 fs/xfs/xfs_file.c   |  4 ++--
 fs/xfs/xfs_inode.c  |  2 +-
 fs/xfs/xfs_log.c    | 18 ++----------------
 fs/xfs/xfs_log.h    |  9 ++-------
 fs/xfs/xfs_trans.c  |  2 +-
 6 files changed, 9 insertions(+), 28 deletions(-)

diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
index fe1bfee35898..761f3189eff2 100644
--- a/fs/xfs/xfs_export.c
+++ b/fs/xfs/xfs_export.c
@@ -237,7 +237,7 @@ xfs_fs_nfs_commit_metadata(
 
 	if (!lsn)
 		return 0;
-	return _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL);
+	return xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL);
 }
 
 const struct export_operations xfs_export_operations = {
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 9ea08326f876..f5c5dbbf1792 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -122,7 +122,7 @@ xfs_dir_fsync(
 
 	if (!lsn)
 		return 0;
-	return _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL);
+	return xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL);
 }
 
 STATIC int
@@ -182,7 +182,7 @@ xfs_file_fsync(
 	}
 
 	if (lsn) {
-		error = _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed);
+		error = xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed);
 		ip->i_itemp->ili_fsync_fields = 0;
 	}
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 604ee384a00a..b7872e82fdad 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2497,7 +2497,7 @@ xfs_iunpin(
 	trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
 
 	/* Give the log a push to start the unpinning I/O */
-	xfs_log_force_lsn(ip->i_mount, ip->i_itemp->ili_last_lsn, 0);
+	xfs_log_force_lsn(ip->i_mount, ip->i_itemp->ili_last_lsn, 0, NULL);
 
 }
 
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index dbb0d69445e8..14ab660a0bae 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3429,7 +3429,7 @@ xfs_log_force(
  * sv.
  */
 int
-_xfs_log_force_lsn(
+xfs_log_force_lsn(
 	struct xfs_mount	*mp,
 	xfs_lsn_t		lsn,
 	uint			flags,
@@ -3442,6 +3442,7 @@ _xfs_log_force_lsn(
 	ASSERT(lsn != 0);
 
 	XFS_STATS_INC(mp, xs_log_force);
+	trace_xfs_log_force(mp, lsn, _RET_IP_);
 
 	lsn = xlog_cil_force_lsn(log, lsn);
 	if (lsn == NULLCOMMITLSN)
@@ -3538,21 +3539,6 @@ _xfs_log_force_lsn(
 	return 0;
 }
 
-/*
- * Wrapper for _xfs_log_force_lsn(), to be used when caller doesn't care
- * about errors or whether the log was flushed or not. This is the normal
- * interface to use when trying to unpin items or move the log forward.
- */
-void
-xfs_log_force_lsn(
-	xfs_mount_t	*mp,
-	xfs_lsn_t	lsn,
-	uint		flags)
-{
-	trace_xfs_log_force(mp, lsn, _RET_IP_);
-	_xfs_log_force_lsn(mp, lsn, flags, NULL);
-}
-
 /*
  * Called when we want to mark the current iclog as being ready to sync to
  * disk.
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index 726dd9a330b4..7e2d62922a16 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -130,13 +130,8 @@ xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
 		       struct xlog_in_core **iclog,
 		       bool regrant);
 int	  xfs_log_force(struct xfs_mount *mp, uint flags);
-int	  _xfs_log_force_lsn(struct xfs_mount *mp,
-			     xfs_lsn_t		lsn,
-			     uint		flags,
-			     int		*log_forced);
-void	  xfs_log_force_lsn(struct xfs_mount	*mp,
-			    xfs_lsn_t		lsn,
-			    uint		flags);
+int	  xfs_log_force_lsn(struct xfs_mount *mp, xfs_lsn_t lsn, uint flags,
+		int *log_forced);
 int	  xfs_log_mount(struct xfs_mount	*mp,
 			struct xfs_buftarg	*log_target,
 			xfs_daddr_t		start_block,
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 86f92df32c42..97d18bafe556 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -966,7 +966,7 @@ __xfs_trans_commit(
 	 * log out now and wait for it.
 	 */
 	if (sync) {
-		error = _xfs_log_force_lsn(mp, commit_lsn, XFS_LOG_SYNC, NULL);
+		error = xfs_log_force_lsn(mp, commit_lsn, XFS_LOG_SYNC, NULL);
 		XFS_STATS_INC(mp, xs_trans_sync);
 	} else {
 		XFS_STATS_INC(mp, xs_trans_async);
-- 
2.14.2

Re: [PATCH 5/8] xfs: merge _xfs_log_force_lsn and xfs_log_force_lsn

[Darrick J. Wong]

On Tue, Mar 13, 2018 at 11:49:24AM +0100, Christoph Hellwig wrote:
> Switch to a single interface for flushing the log to a specific LSN, which
> gives consistent trace point coverage and a less confusing interface.
> 
> The was only a single user of the previous xfs_log_force_lsn function,
> which now also passes a NULL log_flushed argument.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_export.c |  2 +-
>  fs/xfs/xfs_file.c   |  4 ++--
>  fs/xfs/xfs_inode.c  |  2 +-
>  fs/xfs/xfs_log.c    | 18 ++----------------
>  fs/xfs/xfs_log.h    |  9 ++-------
>  fs/xfs/xfs_trans.c  |  2 +-
>  6 files changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c
> index fe1bfee35898..761f3189eff2 100644
> --- a/fs/xfs/xfs_export.c
> +++ b/fs/xfs/xfs_export.c
> @@ -237,7 +237,7 @@ xfs_fs_nfs_commit_metadata(
>  
>  	if (!lsn)
>  		return 0;
> -	return _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL);
> +	return xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL);
>  }
>  
>  const struct export_operations xfs_export_operations = {
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 9ea08326f876..f5c5dbbf1792 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -122,7 +122,7 @@ xfs_dir_fsync(
>  
>  	if (!lsn)
>  		return 0;
> -	return _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL);
> +	return xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, NULL);
>  }
>  
>  STATIC int
> @@ -182,7 +182,7 @@ xfs_file_fsync(
>  	}
>  
>  	if (lsn) {
> -		error = _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed);
> +		error = xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed);
>  		ip->i_itemp->ili_fsync_fields = 0;
>  	}
>  	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 604ee384a00a..b7872e82fdad 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2497,7 +2497,7 @@ xfs_iunpin(
>  	trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
>  
>  	/* Give the log a push to start the unpinning I/O */
> -	xfs_log_force_lsn(ip->i_mount, ip->i_itemp->ili_last_lsn, 0);
> +	xfs_log_force_lsn(ip->i_mount, ip->i_itemp->ili_last_lsn, 0, NULL);
>  
>  }
>  
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index dbb0d69445e8..14ab660a0bae 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3429,7 +3429,7 @@ xfs_log_force(
>   * sv.
>   */
>  int
> -_xfs_log_force_lsn(
> +xfs_log_force_lsn(
>  	struct xfs_mount	*mp,
>  	xfs_lsn_t		lsn,
>  	uint			flags,
> @@ -3442,6 +3442,7 @@ _xfs_log_force_lsn(
>  	ASSERT(lsn != 0);
>  
>  	XFS_STATS_INC(mp, xs_log_force);
> +	trace_xfs_log_force(mp, lsn, _RET_IP_);
>  
>  	lsn = xlog_cil_force_lsn(log, lsn);
>  	if (lsn == NULLCOMMITLSN)
> @@ -3538,21 +3539,6 @@ _xfs_log_force_lsn(
>  	return 0;
>  }
>  
> -/*
> - * Wrapper for _xfs_log_force_lsn(), to be used when caller doesn't care
> - * about errors or whether the log was flushed or not. This is the normal
> - * interface to use when trying to unpin items or move the log forward.
> - */
> -void
> -xfs_log_force_lsn(
> -	xfs_mount_t	*mp,
> -	xfs_lsn_t	lsn,
> -	uint		flags)
> -{
> -	trace_xfs_log_force(mp, lsn, _RET_IP_);
> -	_xfs_log_force_lsn(mp, lsn, flags, NULL);
> -}
> -
>  /*
>   * Called when we want to mark the current iclog as being ready to sync to
>   * disk.
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index 726dd9a330b4..7e2d62922a16 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -130,13 +130,8 @@ xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
>  		       struct xlog_in_core **iclog,
>  		       bool regrant);
>  int	  xfs_log_force(struct xfs_mount *mp, uint flags);
> -int	  _xfs_log_force_lsn(struct xfs_mount *mp,
> -			     xfs_lsn_t		lsn,
> -			     uint		flags,
> -			     int		*log_forced);
> -void	  xfs_log_force_lsn(struct xfs_mount	*mp,
> -			    xfs_lsn_t		lsn,
> -			    uint		flags);
> +int	  xfs_log_force_lsn(struct xfs_mount *mp, xfs_lsn_t lsn, uint flags,
> +		int *log_forced);
>  int	  xfs_log_mount(struct xfs_mount	*mp,
>  			struct xfs_buftarg	*log_target,
>  			xfs_daddr_t		start_block,
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 86f92df32c42..97d18bafe556 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -966,7 +966,7 @@ __xfs_trans_commit(
>  	 * log out now and wait for it.
>  	 */
>  	if (sync) {
> -		error = _xfs_log_force_lsn(mp, commit_lsn, XFS_LOG_SYNC, NULL);
> +		error = xfs_log_force_lsn(mp, commit_lsn, XFS_LOG_SYNC, NULL);
>  		XFS_STATS_INC(mp, xs_trans_sync);
>  	} else {
>  		XFS_STATS_INC(mp, xs_trans_async);
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/8] xfs: refactor xfs_log_force

[Christoph Hellwig]

Streamline the conditionals so that it is more obvious which specific case
form the top of the function comments is being handled.  Use gotos only
for early returns.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 144 ++++++++++++++++++++++++-------------------------------
 1 file changed, 63 insertions(+), 81 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 14ab660a0bae..a37a8defcd39 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3318,99 +3318,81 @@ xfs_log_force(
 	xlog_cil_force(log);
 
 	spin_lock(&log->l_icloglock);
-
 	iclog = log->l_iclog;
-	if (iclog->ic_state & XLOG_STATE_IOERROR) {
-		spin_unlock(&log->l_icloglock);
-		return -EIO;
-	}
+	if (iclog->ic_state & XLOG_STATE_IOERROR)
+		goto out_error;
 
-	/* If the head iclog is not active nor dirty, we just attach
-	 * ourselves to the head and go to sleep.
-	 */
-	if (iclog->ic_state == XLOG_STATE_ACTIVE ||
-	    iclog->ic_state == XLOG_STATE_DIRTY) {
+	if (iclog->ic_state == XLOG_STATE_DIRTY ||
+	    (iclog->ic_state == XLOG_STATE_ACTIVE &&
+	     atomic_read(&iclog->ic_refcnt) == 0 && iclog->ic_offset == 0)) {
 		/*
-		 * If the head is dirty or (active and empty), then
-		 * we need to look at the previous iclog.  If the previous
-		 * iclog is active or dirty we are done.  There is nothing
-		 * to sync out.  Otherwise, we attach ourselves to the
+		 * If the head is dirty or (active and empty), then we need to
+		 * look at the previous iclog.
+		 *
+		 * If the previous iclog is active or dirty we are done.  There
+		 * is nothing to sync out. Otherwise, we attach ourselves to the
 		 * previous iclog and go to sleep.
 		 */
-		if (iclog->ic_state == XLOG_STATE_DIRTY ||
-		    (atomic_read(&iclog->ic_refcnt) == 0
-		     && iclog->ic_offset == 0)) {
-			iclog = iclog->ic_prev;
-			if (iclog->ic_state == XLOG_STATE_ACTIVE ||
-			    iclog->ic_state == XLOG_STATE_DIRTY)
-				goto no_sleep;
-			else
-				goto maybe_sleep;
-		} else {
-			if (atomic_read(&iclog->ic_refcnt) == 0) {
-				/* We are the only one with access to this
-				 * iclog.  Flush it out now.  There should
-				 * be a roundoff of zero to show that someone
-				 * has already taken care of the roundoff from
-				 * the previous sync.
-				 */
-				atomic_inc(&iclog->ic_refcnt);
-				lsn = be64_to_cpu(iclog->ic_header.h_lsn);
-				xlog_state_switch_iclogs(log, iclog, 0);
-				spin_unlock(&log->l_icloglock);
-
-				if (xlog_state_release_iclog(log, iclog))
-					return -EIO;
+		iclog = iclog->ic_prev;
+		if (iclog->ic_state == XLOG_STATE_ACTIVE ||
+		    iclog->ic_state == XLOG_STATE_DIRTY)
+			goto out_unlock;
+	} else if (iclog->ic_state == XLOG_STATE_ACTIVE) {
+		if (atomic_read(&iclog->ic_refcnt) == 0) {
+			/*
+			 * We are the only one with access to this iclog.
+			 *
+			 * Flush it out now.  There should be a roundoff of zero
+			 * to show that someone has already taken care of the
+			 * roundoff from the previous sync.
+			 */
+			atomic_inc(&iclog->ic_refcnt);
+			lsn = be64_to_cpu(iclog->ic_header.h_lsn);
+			xlog_state_switch_iclogs(log, iclog, 0);
+			spin_unlock(&log->l_icloglock);
 
-				spin_lock(&log->l_icloglock);
-				if (be64_to_cpu(iclog->ic_header.h_lsn) == lsn &&
-				    iclog->ic_state != XLOG_STATE_DIRTY)
-					goto maybe_sleep;
-				else
-					goto no_sleep;
-			} else {
-				/* Someone else is writing to this iclog.
-				 * Use its call to flush out the data.  However,
-				 * the other thread may not force out this LR,
-				 * so we mark it WANT_SYNC.
-				 */
-				xlog_state_switch_iclogs(log, iclog, 0);
-				goto maybe_sleep;
-			}
-		}
-	}
+			if (xlog_state_release_iclog(log, iclog))
+				return -EIO;
 
-	/* By the time we come around again, the iclog could've been filled
-	 * which would give it another lsn.  If we have a new lsn, just
-	 * return because the relevant data has been flushed.
-	 */
-maybe_sleep:
-	if (flags & XFS_LOG_SYNC) {
-		/*
-		 * We must check if we're shutting down here, before
-		 * we wait, while we're holding the l_icloglock.
-		 * Then we check again after waking up, in case our
-		 * sleep was disturbed by a bad news.
-		 */
-		if (iclog->ic_state & XLOG_STATE_IOERROR) {
-			spin_unlock(&log->l_icloglock);
-			return -EIO;
+			spin_lock(&log->l_icloglock);
+			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn ||
+			    iclog->ic_state == XLOG_STATE_DIRTY)
+				goto out_unlock;
+		} else {
+			/*
+			 * Someone else is writing to this iclog.
+			 *
+			 * Use its call to flush out the data.  However, the
+			 * other thread may not force out this LR, so we mark
+			 * it WANT_SYNC.
+			 */
+			xlog_state_switch_iclogs(log, iclog, 0);
 		}
-		XFS_STATS_INC(mp, xs_log_force_sleep);
-		xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
+	} else {
 		/*
-		 * No need to grab the log lock here since we're
-		 * only deciding whether or not to return EIO
-		 * and the memory read should be atomic.
+		 * If the head iclog is not active nor dirty, we just attach
+		 * ourselves to the head and go to sleep if necessary.
 		 */
-		if (iclog->ic_state & XLOG_STATE_IOERROR)
-			return -EIO;
-	} else {
-
-no_sleep:
-		spin_unlock(&log->l_icloglock);
+		;
 	}
+
+	if (!(flags & XFS_LOG_SYNC))
+		goto out_unlock;
+
+	if (iclog->ic_state & XLOG_STATE_IOERROR)
+		goto out_error;
+	XFS_STATS_INC(mp, xs_log_force_sleep);
+	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
+	if (iclog->ic_state & XLOG_STATE_IOERROR)
+		return -EIO;
 	return 0;
+
+out_unlock:
+	spin_unlock(&log->l_icloglock);
+	return 0;
+out_error:
+	spin_unlock(&log->l_icloglock);
+	return -EIO;
 }
 
 /*
-- 
2.14.2

Re: [PATCH 6/8] xfs: refactor xfs_log_force

[Darrick J. Wong]

On Tue, Mar 13, 2018 at 11:49:25AM +0100, Christoph Hellwig wrote:
> Streamline the conditionals so that it is more obvious which specific case
> form the top of the function comments is being handled.  Use gotos only
> for early returns.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Well, that made my brain hurt... :)
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c | 144 ++++++++++++++++++++++++-------------------------------
>  1 file changed, 63 insertions(+), 81 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 14ab660a0bae..a37a8defcd39 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3318,99 +3318,81 @@ xfs_log_force(
>  	xlog_cil_force(log);
>  
>  	spin_lock(&log->l_icloglock);
> -
>  	iclog = log->l_iclog;
> -	if (iclog->ic_state & XLOG_STATE_IOERROR) {
> -		spin_unlock(&log->l_icloglock);
> -		return -EIO;
> -	}
> +	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +		goto out_error;
>  
> -	/* If the head iclog is not active nor dirty, we just attach
> -	 * ourselves to the head and go to sleep.
> -	 */
> -	if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> -	    iclog->ic_state == XLOG_STATE_DIRTY) {
> +	if (iclog->ic_state == XLOG_STATE_DIRTY ||
> +	    (iclog->ic_state == XLOG_STATE_ACTIVE &&
> +	     atomic_read(&iclog->ic_refcnt) == 0 && iclog->ic_offset == 0)) {
>  		/*
> -		 * If the head is dirty or (active and empty), then
> -		 * we need to look at the previous iclog.  If the previous
> -		 * iclog is active or dirty we are done.  There is nothing
> -		 * to sync out.  Otherwise, we attach ourselves to the
> +		 * If the head is dirty or (active and empty), then we need to
> +		 * look at the previous iclog.
> +		 *
> +		 * If the previous iclog is active or dirty we are done.  There
> +		 * is nothing to sync out. Otherwise, we attach ourselves to the
>  		 * previous iclog and go to sleep.
>  		 */
> -		if (iclog->ic_state == XLOG_STATE_DIRTY ||
> -		    (atomic_read(&iclog->ic_refcnt) == 0
> -		     && iclog->ic_offset == 0)) {
> -			iclog = iclog->ic_prev;
> -			if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> -			    iclog->ic_state == XLOG_STATE_DIRTY)
> -				goto no_sleep;
> -			else
> -				goto maybe_sleep;
> -		} else {
> -			if (atomic_read(&iclog->ic_refcnt) == 0) {
> -				/* We are the only one with access to this
> -				 * iclog.  Flush it out now.  There should
> -				 * be a roundoff of zero to show that someone
> -				 * has already taken care of the roundoff from
> -				 * the previous sync.
> -				 */
> -				atomic_inc(&iclog->ic_refcnt);
> -				lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> -				xlog_state_switch_iclogs(log, iclog, 0);
> -				spin_unlock(&log->l_icloglock);
> -
> -				if (xlog_state_release_iclog(log, iclog))
> -					return -EIO;
> +		iclog = iclog->ic_prev;
> +		if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> +		    iclog->ic_state == XLOG_STATE_DIRTY)
> +			goto out_unlock;
> +	} else if (iclog->ic_state == XLOG_STATE_ACTIVE) {
> +		if (atomic_read(&iclog->ic_refcnt) == 0) {
> +			/*
> +			 * We are the only one with access to this iclog.
> +			 *
> +			 * Flush it out now.  There should be a roundoff of zero
> +			 * to show that someone has already taken care of the
> +			 * roundoff from the previous sync.
> +			 */
> +			atomic_inc(&iclog->ic_refcnt);
> +			lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> +			xlog_state_switch_iclogs(log, iclog, 0);
> +			spin_unlock(&log->l_icloglock);
>  
> -				spin_lock(&log->l_icloglock);
> -				if (be64_to_cpu(iclog->ic_header.h_lsn) == lsn &&
> -				    iclog->ic_state != XLOG_STATE_DIRTY)
> -					goto maybe_sleep;
> -				else
> -					goto no_sleep;
> -			} else {
> -				/* Someone else is writing to this iclog.
> -				 * Use its call to flush out the data.  However,
> -				 * the other thread may not force out this LR,
> -				 * so we mark it WANT_SYNC.
> -				 */
> -				xlog_state_switch_iclogs(log, iclog, 0);
> -				goto maybe_sleep;
> -			}
> -		}
> -	}
> +			if (xlog_state_release_iclog(log, iclog))
> +				return -EIO;
>  
> -	/* By the time we come around again, the iclog could've been filled
> -	 * which would give it another lsn.  If we have a new lsn, just
> -	 * return because the relevant data has been flushed.
> -	 */
> -maybe_sleep:
> -	if (flags & XFS_LOG_SYNC) {
> -		/*
> -		 * We must check if we're shutting down here, before
> -		 * we wait, while we're holding the l_icloglock.
> -		 * Then we check again after waking up, in case our
> -		 * sleep was disturbed by a bad news.
> -		 */
> -		if (iclog->ic_state & XLOG_STATE_IOERROR) {
> -			spin_unlock(&log->l_icloglock);
> -			return -EIO;
> +			spin_lock(&log->l_icloglock);
> +			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn ||
> +			    iclog->ic_state == XLOG_STATE_DIRTY)
> +				goto out_unlock;
> +		} else {
> +			/*
> +			 * Someone else is writing to this iclog.
> +			 *
> +			 * Use its call to flush out the data.  However, the
> +			 * other thread may not force out this LR, so we mark
> +			 * it WANT_SYNC.
> +			 */
> +			xlog_state_switch_iclogs(log, iclog, 0);
>  		}
> -		XFS_STATS_INC(mp, xs_log_force_sleep);
> -		xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> +	} else {
>  		/*
> -		 * No need to grab the log lock here since we're
> -		 * only deciding whether or not to return EIO
> -		 * and the memory read should be atomic.
> +		 * If the head iclog is not active nor dirty, we just attach
> +		 * ourselves to the head and go to sleep if necessary.
>  		 */
> -		if (iclog->ic_state & XLOG_STATE_IOERROR)
> -			return -EIO;
> -	} else {
> -
> -no_sleep:
> -		spin_unlock(&log->l_icloglock);
> +		;
>  	}
> +
> +	if (!(flags & XFS_LOG_SYNC))
> +		goto out_unlock;
> +
> +	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +		goto out_error;
> +	XFS_STATS_INC(mp, xs_log_force_sleep);
> +	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> +	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +		return -EIO;
>  	return 0;
> +
> +out_unlock:
> +	spin_unlock(&log->l_icloglock);
> +	return 0;
> +out_error:
> +	spin_unlock(&log->l_icloglock);
> +	return -EIO;
>  }
>  
>  /*
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/8] xfs: refactor xfs_log_force_lsn

[Christoph Hellwig]

Use the the smallest possible loop as preable to find the correct iclog
buffer, and then use gotos for unwinding to straighten the code.

Also fix the top of function comment while we're at it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 142 ++++++++++++++++++++++++-------------------------------
 1 file changed, 62 insertions(+), 80 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a37a8defcd39..b6c6f227b2d7 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3404,11 +3404,10 @@ xfs_log_force(
  *		state and go to sleep or return.
  *	If it is in any other state, go to sleep or return.
  *
- * Synchronous forces are implemented with a signal variable. All callers
- * to force a given lsn to disk will wait on a the sv attached to the
- * specific in-core log.  When given in-core log finally completes its
- * write to disk, that thread will wake up all threads waiting on the
- * sv.
+ * Synchronous forces are implemented with a wait queue.  All callers to force a
+ * given lsn to disk will wait on a the queue attached to the specific in-core
+ * log.  When given in-core log finally completes its write to disk, that thread
+ * will wake up all threads waiting on the queue.
  */
 int
 xfs_log_force_lsn(
@@ -3433,92 +3432,75 @@ xfs_log_force_lsn(
 try_again:
 	spin_lock(&log->l_icloglock);
 	iclog = log->l_iclog;
-	if (iclog->ic_state & XLOG_STATE_IOERROR) {
-		spin_unlock(&log->l_icloglock);
-		return -EIO;
-	}
+	if (iclog->ic_state & XLOG_STATE_IOERROR)
+		goto out_error;
 
-	do {
-		if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
-			iclog = iclog->ic_next;
-			continue;
-		}
+	while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
+		iclog = iclog->ic_next;
+		if (iclog == log->l_iclog)
+			goto out_unlock;
+	}
 
-		if (iclog->ic_state == XLOG_STATE_DIRTY) {
-			spin_unlock(&log->l_icloglock);
-			return 0;
-		}
+	if (iclog->ic_state == XLOG_STATE_DIRTY)
+		goto out_unlock;
 
-		if (iclog->ic_state == XLOG_STATE_ACTIVE) {
-			/*
-			 * We sleep here if we haven't already slept (e.g.
-			 * this is the first time we've looked at the correct
-			 * iclog buf) and the buffer before us is going to
-			 * be sync'ed. The reason for this is that if we
-			 * are doing sync transactions here, by waiting for
-			 * the previous I/O to complete, we can allow a few
-			 * more transactions into this iclog before we close
-			 * it down.
-			 *
-			 * Otherwise, we mark the buffer WANT_SYNC, and bump
-			 * up the refcnt so we can release the log (which
-			 * drops the ref count).  The state switch keeps new
-			 * transaction commits from using this buffer.  When
-			 * the current commits finish writing into the buffer,
-			 * the refcount will drop to zero and the buffer will
-			 * go out then.
-			 */
-			if (!already_slept &&
-			    (iclog->ic_prev->ic_state &
-			     (XLOG_STATE_WANT_SYNC | XLOG_STATE_SYNCING))) {
-				ASSERT(!(iclog->ic_state & XLOG_STATE_IOERROR));
+	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
+		/*
+		 * We sleep here if we haven't already slept (e.g. this is the
+		 * first time we've looked at the correct iclog buf) and the
+		 * buffer before us is going to be sync'ed.  The reason for this
+		 * is that if we are doing sync transactions here, by waiting
+		 * for the previous I/O to complete, we can allow a few more
+		 * transactions into this iclog before we close it down.
+		 *
+		 * Otherwise, we mark the buffer WANT_SYNC, and bump up the
+		 * refcnt so we can release the log (which drops the ref count).
+		 * The state switch keeps new transaction commits from using
+		 * this buffer.  When the current commits finish writing into
+		 * the buffer, the refcount will drop to zero and the buffer
+		 * will go out then.
+		 */
+		if (!already_slept &&
+		    (iclog->ic_prev->ic_state &
+		     (XLOG_STATE_WANT_SYNC | XLOG_STATE_SYNCING))) {
+			ASSERT(!(iclog->ic_state & XLOG_STATE_IOERROR));
 
-				XFS_STATS_INC(mp, xs_log_force_sleep);
+			XFS_STATS_INC(mp, xs_log_force_sleep);
 
-				xlog_wait(&iclog->ic_prev->ic_write_wait,
-							&log->l_icloglock);
-				already_slept = 1;
-				goto try_again;
-			}
-			atomic_inc(&iclog->ic_refcnt);
-			xlog_state_switch_iclogs(log, iclog, 0);
-			spin_unlock(&log->l_icloglock);
-			if (xlog_state_release_iclog(log, iclog))
-				return -EIO;
-			if (log_flushed)
-				*log_flushed = 1;
-			spin_lock(&log->l_icloglock);
+			xlog_wait(&iclog->ic_prev->ic_write_wait,
+					&log->l_icloglock);
+			already_slept = 1;
+			goto try_again;
 		}
+		atomic_inc(&iclog->ic_refcnt);
+		xlog_state_switch_iclogs(log, iclog, 0);
+		spin_unlock(&log->l_icloglock);
+		if (xlog_state_release_iclog(log, iclog))
+			return -EIO;
+		if (log_flushed)
+			*log_flushed = 1;
+		spin_lock(&log->l_icloglock);
+	}
 
-		if ((flags & XFS_LOG_SYNC) && /* sleep */
-		    !(iclog->ic_state &
-		      (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY))) {
-			/*
-			 * Don't wait on completion if we know that we've
-			 * gotten a log write error.
-			 */
-			if (iclog->ic_state & XLOG_STATE_IOERROR) {
-				spin_unlock(&log->l_icloglock);
-				return -EIO;
-			}
-			XFS_STATS_INC(mp, xs_log_force_sleep);
-			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
-			/*
-			 * No need to grab the log lock here since we're
-			 * only deciding whether or not to return EIO
-			 * and the memory read should be atomic.
-			 */
-			if (iclog->ic_state & XLOG_STATE_IOERROR)
-				return -EIO;
-		} else {		/* just return */
-			spin_unlock(&log->l_icloglock);
-		}
+	if (!(flags & XFS_LOG_SYNC) ||
+	    (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY)))
+		goto out_unlock;
 
-		return 0;
-	} while (iclog != log->l_iclog);
+	if (iclog->ic_state & XLOG_STATE_IOERROR)
+		goto out_error;
+
+	XFS_STATS_INC(mp, xs_log_force_sleep);
+	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
+	if (iclog->ic_state & XLOG_STATE_IOERROR)
+		return -EIO;
+	return 0;
 
+out_unlock:
 	spin_unlock(&log->l_icloglock);
 	return 0;
+out_error:
+	spin_unlock(&log->l_icloglock);
+	return -EIO;
 }
 
 /*
-- 
2.14.2

Re: [PATCH 7/8] xfs: refactor xfs_log_force_lsn

[Darrick J. Wong]

On Tue, Mar 13, 2018 at 11:49:26AM +0100, Christoph Hellwig wrote:
> Use the the smallest possible loop as preable to find the correct iclog
> buffer, and then use gotos for unwinding to straighten the code.

"Use the smallest possible loop to find the correct iclog..." ?

> 
> Also fix the top of function comment while we're at it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 142 ++++++++++++++++++++++++-------------------------------
>  1 file changed, 62 insertions(+), 80 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a37a8defcd39..b6c6f227b2d7 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3404,11 +3404,10 @@ xfs_log_force(
>   *		state and go to sleep or return.
>   *	If it is in any other state, go to sleep or return.
>   *
> - * Synchronous forces are implemented with a signal variable. All callers
> - * to force a given lsn to disk will wait on a the sv attached to the
> - * specific in-core log.  When given in-core log finally completes its
> - * write to disk, that thread will wake up all threads waiting on the
> - * sv.
> + * Synchronous forces are implemented with a wait queue.  All callers to force a
> + * given lsn to disk will wait on a the queue attached to the specific in-core

"All callers trying to force a given lsn to disk must wait on the queue
attached to the specific in-core log."... I think?

--D

> + * log.  When given in-core log finally completes its write to disk, that thread
> + * will wake up all threads waiting on the queue.
>   */
>  int
>  xfs_log_force_lsn(
> @@ -3433,92 +3432,75 @@ xfs_log_force_lsn(
>  try_again:
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
> -	if (iclog->ic_state & XLOG_STATE_IOERROR) {
> -		spin_unlock(&log->l_icloglock);
> -		return -EIO;
> -	}
> +	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +		goto out_error;
>  
> -	do {
> -		if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
> -			iclog = iclog->ic_next;
> -			continue;
> -		}
> +	while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
> +		iclog = iclog->ic_next;
> +		if (iclog == log->l_iclog)
> +			goto out_unlock;
> +	}
>  
> -		if (iclog->ic_state == XLOG_STATE_DIRTY) {
> -			spin_unlock(&log->l_icloglock);
> -			return 0;
> -		}
> +	if (iclog->ic_state == XLOG_STATE_DIRTY)
> +		goto out_unlock;
>  
> -		if (iclog->ic_state == XLOG_STATE_ACTIVE) {
> -			/*
> -			 * We sleep here if we haven't already slept (e.g.
> -			 * this is the first time we've looked at the correct
> -			 * iclog buf) and the buffer before us is going to
> -			 * be sync'ed. The reason for this is that if we
> -			 * are doing sync transactions here, by waiting for
> -			 * the previous I/O to complete, we can allow a few
> -			 * more transactions into this iclog before we close
> -			 * it down.
> -			 *
> -			 * Otherwise, we mark the buffer WANT_SYNC, and bump
> -			 * up the refcnt so we can release the log (which
> -			 * drops the ref count).  The state switch keeps new
> -			 * transaction commits from using this buffer.  When
> -			 * the current commits finish writing into the buffer,
> -			 * the refcount will drop to zero and the buffer will
> -			 * go out then.
> -			 */
> -			if (!already_slept &&
> -			    (iclog->ic_prev->ic_state &
> -			     (XLOG_STATE_WANT_SYNC | XLOG_STATE_SYNCING))) {
> -				ASSERT(!(iclog->ic_state & XLOG_STATE_IOERROR));
> +	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
> +		/*
> +		 * We sleep here if we haven't already slept (e.g. this is the
> +		 * first time we've looked at the correct iclog buf) and the
> +		 * buffer before us is going to be sync'ed.  The reason for this
> +		 * is that if we are doing sync transactions here, by waiting
> +		 * for the previous I/O to complete, we can allow a few more
> +		 * transactions into this iclog before we close it down.
> +		 *
> +		 * Otherwise, we mark the buffer WANT_SYNC, and bump up the
> +		 * refcnt so we can release the log (which drops the ref count).
> +		 * The state switch keeps new transaction commits from using
> +		 * this buffer.  When the current commits finish writing into
> +		 * the buffer, the refcount will drop to zero and the buffer
> +		 * will go out then.
> +		 */
> +		if (!already_slept &&
> +		    (iclog->ic_prev->ic_state &
> +		     (XLOG_STATE_WANT_SYNC | XLOG_STATE_SYNCING))) {
> +			ASSERT(!(iclog->ic_state & XLOG_STATE_IOERROR));
>  
> -				XFS_STATS_INC(mp, xs_log_force_sleep);
> +			XFS_STATS_INC(mp, xs_log_force_sleep);
>  
> -				xlog_wait(&iclog->ic_prev->ic_write_wait,
> -							&log->l_icloglock);
> -				already_slept = 1;
> -				goto try_again;
> -			}
> -			atomic_inc(&iclog->ic_refcnt);
> -			xlog_state_switch_iclogs(log, iclog, 0);
> -			spin_unlock(&log->l_icloglock);
> -			if (xlog_state_release_iclog(log, iclog))
> -				return -EIO;
> -			if (log_flushed)
> -				*log_flushed = 1;
> -			spin_lock(&log->l_icloglock);
> +			xlog_wait(&iclog->ic_prev->ic_write_wait,
> +					&log->l_icloglock);
> +			already_slept = 1;
> +			goto try_again;
>  		}
> +		atomic_inc(&iclog->ic_refcnt);
> +		xlog_state_switch_iclogs(log, iclog, 0);
> +		spin_unlock(&log->l_icloglock);
> +		if (xlog_state_release_iclog(log, iclog))
> +			return -EIO;
> +		if (log_flushed)
> +			*log_flushed = 1;
> +		spin_lock(&log->l_icloglock);
> +	}
>  
> -		if ((flags & XFS_LOG_SYNC) && /* sleep */
> -		    !(iclog->ic_state &
> -		      (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY))) {
> -			/*
> -			 * Don't wait on completion if we know that we've
> -			 * gotten a log write error.
> -			 */
> -			if (iclog->ic_state & XLOG_STATE_IOERROR) {
> -				spin_unlock(&log->l_icloglock);
> -				return -EIO;
> -			}
> -			XFS_STATS_INC(mp, xs_log_force_sleep);
> -			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -			/*
> -			 * No need to grab the log lock here since we're
> -			 * only deciding whether or not to return EIO
> -			 * and the memory read should be atomic.
> -			 */
> -			if (iclog->ic_state & XLOG_STATE_IOERROR)
> -				return -EIO;
> -		} else {		/* just return */
> -			spin_unlock(&log->l_icloglock);
> -		}
> +	if (!(flags & XFS_LOG_SYNC) ||
> +	    (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY)))
> +		goto out_unlock;
>  
> -		return 0;
> -	} while (iclog != log->l_iclog);
> +	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +		goto out_error;
> +
> +	XFS_STATS_INC(mp, xs_log_force_sleep);
> +	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> +	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +		return -EIO;
> +	return 0;
>  
> +out_unlock:
>  	spin_unlock(&log->l_icloglock);
>  	return 0;
> +out_error:
> +	spin_unlock(&log->l_icloglock);
> +	return -EIO;
>  }
>  
>  /*
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/8] xfs: unwind the try_again loop in xfs_log_force

[Christoph Hellwig]

Instead split out a __xfs_log_fore_lsn helper that gets called again
with the already_slept flag set to true in case we had to sleep.

This prepares for aio_fsync support.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 72 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index b6c6f227b2d7..9a2008646d57 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3395,41 +3395,17 @@ xfs_log_force(
 	return -EIO;
 }
 
-/*
- * Force the in-core log to disk for a specific LSN.
- *
- * Find in-core log with lsn.
- *	If it is in the DIRTY state, just return.
- *	If it is in the ACTIVE state, move the in-core log into the WANT_SYNC
- *		state and go to sleep or return.
- *	If it is in any other state, go to sleep or return.
- *
- * Synchronous forces are implemented with a wait queue.  All callers to force a
- * given lsn to disk will wait on a the queue attached to the specific in-core
- * log.  When given in-core log finally completes its write to disk, that thread
- * will wake up all threads waiting on the queue.
- */
-int
-xfs_log_force_lsn(
+static int
+__xfs_log_force_lsn(
 	struct xfs_mount	*mp,
 	xfs_lsn_t		lsn,
 	uint			flags,
-	int			*log_flushed)
+	int			*log_flushed,
+	bool			already_slept)
 {
 	struct xlog		*log = mp->m_log;
 	struct xlog_in_core	*iclog;
-	int			already_slept = 0;
-
-	ASSERT(lsn != 0);
-
-	XFS_STATS_INC(mp, xs_log_force);
-	trace_xfs_log_force(mp, lsn, _RET_IP_);
-
-	lsn = xlog_cil_force_lsn(log, lsn);
-	if (lsn == NULLCOMMITLSN)
-		return 0;
 
-try_again:
 	spin_lock(&log->l_icloglock);
 	iclog = log->l_iclog;
 	if (iclog->ic_state & XLOG_STATE_IOERROR)
@@ -3469,8 +3445,7 @@ xfs_log_force_lsn(
 
 			xlog_wait(&iclog->ic_prev->ic_write_wait,
 					&log->l_icloglock);
-			already_slept = 1;
-			goto try_again;
+			return -EAGAIN;
 		}
 		atomic_inc(&iclog->ic_refcnt);
 		xlog_state_switch_iclogs(log, iclog, 0);
@@ -3503,6 +3478,43 @@ xfs_log_force_lsn(
 	return -EIO;
 }
 
+/*
+ * Force the in-core log to disk for a specific LSN.
+ *
+ * Find in-core log with lsn.
+ *	If it is in the DIRTY state, just return.
+ *	If it is in the ACTIVE state, move the in-core log into the WANT_SYNC
+ *		state and go to sleep or return.
+ *	If it is in any other state, go to sleep or return.
+ *
+ * Synchronous forces are implemented with a wait queue.  All callers to force a
+ * given lsn to disk will wait on a the queue attached to the specific in-core
+ * log.  When given in-core log finally completes its write to disk, that thread
+ * will wake up all threads waiting on the queue.
+ */
+int
+xfs_log_force_lsn(
+	struct xfs_mount	*mp,
+	xfs_lsn_t		lsn,
+	uint			flags,
+	int			*log_flushed)
+{
+	int			ret;
+	ASSERT(lsn != 0);
+
+	XFS_STATS_INC(mp, xs_log_force);
+	trace_xfs_log_force(mp, lsn, _RET_IP_);
+
+	lsn = xlog_cil_force_lsn(mp->m_log, lsn);
+	if (lsn == NULLCOMMITLSN)
+		return 0;
+
+	ret = __xfs_log_force_lsn(mp, lsn, flags, log_flushed, false);
+	if (ret == -EAGAIN)
+		ret = __xfs_log_force_lsn(mp, lsn, flags, log_flushed, true);
+	return ret;
+}
+
 /*
  * Called when we want to mark the current iclog as being ready to sync to
  * disk.
-- 
2.14.2

Re: [PATCH 8/8] xfs: unwind the try_again loop in xfs_log_force

[Darrick J. Wong]

On Tue, Mar 13, 2018 at 11:49:27AM +0100, Christoph Hellwig wrote:
> Instead split out a __xfs_log_fore_lsn helper that gets called again

I assume you meant __xfs_log_force_lsn?

Looks ok modulo fixing the tortured sentence in the comment in the
previous patch that gets moved around in this one,

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


> with the already_slept flag set to true in case we had to sleep.
> 
> This prepares for aio_fsync support.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 72 +++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 42 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index b6c6f227b2d7..9a2008646d57 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3395,41 +3395,17 @@ xfs_log_force(
>  	return -EIO;
>  }
>  
> -/*
> - * Force the in-core log to disk for a specific LSN.
> - *
> - * Find in-core log with lsn.
> - *	If it is in the DIRTY state, just return.
> - *	If it is in the ACTIVE state, move the in-core log into the WANT_SYNC
> - *		state and go to sleep or return.
> - *	If it is in any other state, go to sleep or return.
> - *
> - * Synchronous forces are implemented with a wait queue.  All callers to force a
> - * given lsn to disk will wait on a the queue attached to the specific in-core
> - * log.  When given in-core log finally completes its write to disk, that thread
> - * will wake up all threads waiting on the queue.
> - */
> -int
> -xfs_log_force_lsn(
> +static int
> +__xfs_log_force_lsn(
>  	struct xfs_mount	*mp,
>  	xfs_lsn_t		lsn,
>  	uint			flags,
> -	int			*log_flushed)
> +	int			*log_flushed,
> +	bool			already_slept)
>  {
>  	struct xlog		*log = mp->m_log;
>  	struct xlog_in_core	*iclog;
> -	int			already_slept = 0;
> -
> -	ASSERT(lsn != 0);
> -
> -	XFS_STATS_INC(mp, xs_log_force);
> -	trace_xfs_log_force(mp, lsn, _RET_IP_);
> -
> -	lsn = xlog_cil_force_lsn(log, lsn);
> -	if (lsn == NULLCOMMITLSN)
> -		return 0;
>  
> -try_again:
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
>  	if (iclog->ic_state & XLOG_STATE_IOERROR)
> @@ -3469,8 +3445,7 @@ xfs_log_force_lsn(
>  
>  			xlog_wait(&iclog->ic_prev->ic_write_wait,
>  					&log->l_icloglock);
> -			already_slept = 1;
> -			goto try_again;
> +			return -EAGAIN;
>  		}
>  		atomic_inc(&iclog->ic_refcnt);
>  		xlog_state_switch_iclogs(log, iclog, 0);
> @@ -3503,6 +3478,43 @@ xfs_log_force_lsn(
>  	return -EIO;
>  }
>  
> +/*
> + * Force the in-core log to disk for a specific LSN.
> + *
> + * Find in-core log with lsn.
> + *	If it is in the DIRTY state, just return.
> + *	If it is in the ACTIVE state, move the in-core log into the WANT_SYNC
> + *		state and go to sleep or return.
> + *	If it is in any other state, go to sleep or return.
> + *
> + * Synchronous forces are implemented with a wait queue.  All callers to force a
> + * given lsn to disk will wait on a the queue attached to the specific in-core
> + * log.  When given in-core log finally completes its write to disk, that thread
> + * will wake up all threads waiting on the queue.
> + */
> +int
> +xfs_log_force_lsn(
> +	struct xfs_mount	*mp,
> +	xfs_lsn_t		lsn,
> +	uint			flags,
> +	int			*log_flushed)
> +{
> +	int			ret;
> +	ASSERT(lsn != 0);
> +
> +	XFS_STATS_INC(mp, xs_log_force);
> +	trace_xfs_log_force(mp, lsn, _RET_IP_);
> +
> +	lsn = xlog_cil_force_lsn(mp->m_log, lsn);
> +	if (lsn == NULLCOMMITLSN)
> +		return 0;
> +
> +	ret = __xfs_log_force_lsn(mp, lsn, flags, log_flushed, false);
> +	if (ret == -EAGAIN)
> +		ret = __xfs_log_force_lsn(mp, lsn, flags, log_flushed, true);
> +	return ret;
> +}
> +
>  /*
>   * Called when we want to mark the current iclog as being ready to sync to
>   * disk.
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html