misc iclog state machine cleanups

misc iclog state machine cleanups

[Christoph Hellwig]

Hi all,

this series has a few patches from tidying up the iclog state
machine.

[PATCH 1/8] xfs: pass the correct flag to xlog_write_iclog

[Christoph Hellwig]

xlog_write_iclog expects a bool for the second argument.  While any
non-0 value happens to work fine this makes all calls consistent.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a2beee9f74da..cd90871c2101 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1735,7 +1735,7 @@ xlog_write_iclog(
 		 * the buffer manually, the code needs to be kept in sync
 		 * with the I/O completion path.
 		 */
-		xlog_state_done_syncing(iclog, XFS_LI_ABORTED);
+		xlog_state_done_syncing(iclog, true);
 		up(&iclog->ic_sema);
 		return;
 	}
-- 
2.20.1

Re: [PATCH 1/8] xfs: pass the correct flag to xlog_write_iclog

[Darrick J. Wong]

On Wed, Oct 09, 2019 at 04:27:41PM +0200, Christoph Hellwig wrote:
> xlog_write_iclog expects a bool for the second argument.  While any
> non-0 value happens to work fine this makes all calls consistent.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a2beee9f74da..cd90871c2101 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1735,7 +1735,7 @@ xlog_write_iclog(
>  		 * the buffer manually, the code needs to be kept in sync
>  		 * with the I/O completion path.
>  		 */
> -		xlog_state_done_syncing(iclog, XFS_LI_ABORTED);
> +		xlog_state_done_syncing(iclog, true);
>  		up(&iclog->ic_sema);
>  		return;
>  	}
> -- 
> 2.20.1
> 

Re: [PATCH 1/8] xfs: pass the correct flag to xlog_write_iclog

[Brian Foster]

On Wed, Oct 09, 2019 at 04:27:41PM +0200, Christoph Hellwig wrote:
> xlog_write_iclog expects a bool for the second argument.  While any
> non-0 value happens to work fine this makes all calls consistent.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a2beee9f74da..cd90871c2101 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1735,7 +1735,7 @@ xlog_write_iclog(
>  		 * the buffer manually, the code needs to be kept in sync
>  		 * with the I/O completion path.
>  		 */
> -		xlog_state_done_syncing(iclog, XFS_LI_ABORTED);
> +		xlog_state_done_syncing(iclog, true);
>  		up(&iclog->ic_sema);
>  		return;
>  	}
> -- 
> 2.20.1
> 

[PATCH 2/8] xfs: remove the unused ic_io_size field from xlog_in_core

[Christoph Hellwig]

ic_io_size is only used inside xlog_write_iclog, where we can just use
the count parameter intead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 6 ++----
 fs/xfs/xfs_log_priv.h | 3 ---
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index cd90871c2101..4f5927ddfa40 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1740,8 +1740,6 @@ xlog_write_iclog(
 		return;
 	}
 
-	iclog->ic_io_size = count;
-
 	bio_init(&iclog->ic_bio, iclog->ic_bvec, howmany(count, PAGE_SIZE));
 	bio_set_dev(&iclog->ic_bio, log->l_targ->bt_bdev);
 	iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart + bno;
@@ -1751,9 +1749,9 @@ xlog_write_iclog(
 	if (need_flush)
 		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
 
-	xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, iclog->ic_io_size);
+	xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count);
 	if (is_vmalloc_addr(iclog->ic_data))
-		flush_kernel_vmap_range(iclog->ic_data, iclog->ic_io_size);
+		flush_kernel_vmap_range(iclog->ic_data, count);
 
 	/*
 	 * If this log buffer would straddle the end of the log we will have
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index b880c23cb6e4..90e210e433cf 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -179,8 +179,6 @@ typedef struct xlog_ticket {
  * - ic_next is the pointer to the next iclog in the ring.
  * - ic_log is a pointer back to the global log structure.
  * - ic_size is the full size of the log buffer, minus the cycle headers.
- * - ic_io_size is the size of the currently pending log buffer write, which
- *	might be smaller than ic_size
  * - ic_offset is the current number of bytes written to in this iclog.
  * - ic_refcnt is bumped when someone is writing to the log.
  * - ic_state is the state of the iclog.
@@ -205,7 +203,6 @@ typedef struct xlog_in_core {
 	struct xlog_in_core	*ic_prev;
 	struct xlog		*ic_log;
 	u32			ic_size;
-	u32			ic_io_size;
 	u32			ic_offset;
 	unsigned short		ic_state;
 	char			*ic_datap;	/* pointer to iclog data */
-- 
2.20.1

Re: [PATCH 2/8] xfs: remove the unused ic_io_size field from xlog_in_core

[Darrick J. Wong]

On Wed, Oct 09, 2019 at 04:27:42PM +0200, Christoph Hellwig wrote:
> ic_io_size is only used inside xlog_write_iclog, where we can just use
> the count parameter intead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log.c      | 6 ++----
>  fs/xfs/xfs_log_priv.h | 3 ---
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index cd90871c2101..4f5927ddfa40 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1740,8 +1740,6 @@ xlog_write_iclog(
>  		return;
>  	}
>  
> -	iclog->ic_io_size = count;
> -
>  	bio_init(&iclog->ic_bio, iclog->ic_bvec, howmany(count, PAGE_SIZE));
>  	bio_set_dev(&iclog->ic_bio, log->l_targ->bt_bdev);
>  	iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart + bno;
> @@ -1751,9 +1749,9 @@ xlog_write_iclog(
>  	if (need_flush)
>  		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
>  
> -	xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, iclog->ic_io_size);
> +	xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count);
>  	if (is_vmalloc_addr(iclog->ic_data))
> -		flush_kernel_vmap_range(iclog->ic_data, iclog->ic_io_size);
> +		flush_kernel_vmap_range(iclog->ic_data, count);
>  
>  	/*
>  	 * If this log buffer would straddle the end of the log we will have
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index b880c23cb6e4..90e210e433cf 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -179,8 +179,6 @@ typedef struct xlog_ticket {
>   * - ic_next is the pointer to the next iclog in the ring.
>   * - ic_log is a pointer back to the global log structure.
>   * - ic_size is the full size of the log buffer, minus the cycle headers.
> - * - ic_io_size is the size of the currently pending log buffer write, which
> - *	might be smaller than ic_size
>   * - ic_offset is the current number of bytes written to in this iclog.
>   * - ic_refcnt is bumped when someone is writing to the log.
>   * - ic_state is the state of the iclog.
> @@ -205,7 +203,6 @@ typedef struct xlog_in_core {
>  	struct xlog_in_core	*ic_prev;
>  	struct xlog		*ic_log;
>  	u32			ic_size;
> -	u32			ic_io_size;
>  	u32			ic_offset;
>  	unsigned short		ic_state;
>  	char			*ic_datap;	/* pointer to iclog data */
> -- 
> 2.20.1
> 

Re: [PATCH 2/8] xfs: remove the unused ic_io_size field from xlog_in_core

[Brian Foster]

On Wed, Oct 09, 2019 at 04:27:42PM +0200, Christoph Hellwig wrote:
> ic_io_size is only used inside xlog_write_iclog, where we can just use
> the count parameter intead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c      | 6 ++----
>  fs/xfs/xfs_log_priv.h | 3 ---
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index cd90871c2101..4f5927ddfa40 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1740,8 +1740,6 @@ xlog_write_iclog(
>  		return;
>  	}
>  
> -	iclog->ic_io_size = count;
> -
>  	bio_init(&iclog->ic_bio, iclog->ic_bvec, howmany(count, PAGE_SIZE));
>  	bio_set_dev(&iclog->ic_bio, log->l_targ->bt_bdev);
>  	iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart + bno;
> @@ -1751,9 +1749,9 @@ xlog_write_iclog(
>  	if (need_flush)
>  		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
>  
> -	xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, iclog->ic_io_size);
> +	xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count);
>  	if (is_vmalloc_addr(iclog->ic_data))
> -		flush_kernel_vmap_range(iclog->ic_data, iclog->ic_io_size);
> +		flush_kernel_vmap_range(iclog->ic_data, count);
>  
>  	/*
>  	 * If this log buffer would straddle the end of the log we will have
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index b880c23cb6e4..90e210e433cf 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -179,8 +179,6 @@ typedef struct xlog_ticket {
>   * - ic_next is the pointer to the next iclog in the ring.
>   * - ic_log is a pointer back to the global log structure.
>   * - ic_size is the full size of the log buffer, minus the cycle headers.
> - * - ic_io_size is the size of the currently pending log buffer write, which
> - *	might be smaller than ic_size
>   * - ic_offset is the current number of bytes written to in this iclog.
>   * - ic_refcnt is bumped when someone is writing to the log.
>   * - ic_state is the state of the iclog.
> @@ -205,7 +203,6 @@ typedef struct xlog_in_core {
>  	struct xlog_in_core	*ic_prev;
>  	struct xlog		*ic_log;
>  	u32			ic_size;
> -	u32			ic_io_size;
>  	u32			ic_offset;
>  	unsigned short		ic_state;
>  	char			*ic_datap;	/* pointer to iclog data */
> -- 
> 2.20.1
> 

[PATCH 3/8] xfs: move the locking from xlog_state_finish_copy to the callers

[Christoph Hellwig]

This will allow optimizing various locking cycles in the following
patches.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 4f5927ddfa40..860a555772fe 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1967,7 +1967,6 @@ xlog_dealloc_log(
 /*
  * Update counters atomically now that memcpy is done.
  */
-/* ARGSUSED */
 static inline void
 xlog_state_finish_copy(
 	struct xlog		*log,
@@ -1975,16 +1974,11 @@ xlog_state_finish_copy(
 	int			record_cnt,
 	int			copy_bytes)
 {
-	spin_lock(&log->l_icloglock);
+	lockdep_assert_held(&log->l_icloglock);
 
 	be32_add_cpu(&iclog->ic_header.h_num_logops, record_cnt);
 	iclog->ic_offset += copy_bytes;
-
-	spin_unlock(&log->l_icloglock);
-}	/* xlog_state_finish_copy */
-
-
-
+}
 
 /*
  * print out info relating to regions written which consume
@@ -2266,7 +2260,9 @@ xlog_write_copy_finish(
 		 * This iclog has already been marked WANT_SYNC by
 		 * xlog_state_get_iclog_space.
 		 */
+		spin_lock(&log->l_icloglock);
 		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
+		spin_unlock(&log->l_icloglock);
 		*record_cnt = 0;
 		*data_cnt = 0;
 		return xlog_state_release_iclog(log, iclog);
@@ -2277,11 +2273,11 @@ xlog_write_copy_finish(
 
 	if (iclog->ic_size - log_offset <= sizeof(xlog_op_header_t)) {
 		/* no more space in this iclog - push it. */
+		spin_lock(&log->l_icloglock);
 		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
 		*record_cnt = 0;
 		*data_cnt = 0;
 
-		spin_lock(&log->l_icloglock);
 		xlog_state_want_sync(log, iclog);
 		spin_unlock(&log->l_icloglock);
 
@@ -2504,7 +2500,9 @@ xlog_write(
 
 	ASSERT(len == 0);
 
+	spin_lock(&log->l_icloglock);
 	xlog_state_finish_copy(log, iclog, record_cnt, data_cnt);
+	spin_unlock(&log->l_icloglock);
 	if (!commit_iclog)
 		return xlog_state_release_iclog(log, iclog);
 
-- 
2.20.1

Re: [PATCH 3/8] xfs: move the locking from xlog_state_finish_copy to the callers

[Darrick J. Wong]

On Wed, Oct 09, 2019 at 04:27:43PM +0200, Christoph Hellwig wrote:
> This will allow optimizing various locking cycles in the following
> patches.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 4f5927ddfa40..860a555772fe 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1967,7 +1967,6 @@ xlog_dealloc_log(
>  /*
>   * Update counters atomically now that memcpy is done.
>   */
> -/* ARGSUSED */
>  static inline void
>  xlog_state_finish_copy(
>  	struct xlog		*log,
> @@ -1975,16 +1974,11 @@ xlog_state_finish_copy(
>  	int			record_cnt,
>  	int			copy_bytes)
>  {
> -	spin_lock(&log->l_icloglock);
> +	lockdep_assert_held(&log->l_icloglock);
>  
>  	be32_add_cpu(&iclog->ic_header.h_num_logops, record_cnt);
>  	iclog->ic_offset += copy_bytes;
> -
> -	spin_unlock(&log->l_icloglock);
> -}	/* xlog_state_finish_copy */
> -
> -
> -
> +}
>  
>  /*
>   * print out info relating to regions written which consume
> @@ -2266,7 +2260,9 @@ xlog_write_copy_finish(
>  		 * This iclog has already been marked WANT_SYNC by
>  		 * xlog_state_get_iclog_space.
>  		 */
> +		spin_lock(&log->l_icloglock);
>  		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
> +		spin_unlock(&log->l_icloglock);
>  		*record_cnt = 0;
>  		*data_cnt = 0;
>  		return xlog_state_release_iclog(log, iclog);
> @@ -2277,11 +2273,11 @@ xlog_write_copy_finish(
>  
>  	if (iclog->ic_size - log_offset <= sizeof(xlog_op_header_t)) {
>  		/* no more space in this iclog - push it. */
> +		spin_lock(&log->l_icloglock);
>  		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
>  		*record_cnt = 0;
>  		*data_cnt = 0;
>  
> -		spin_lock(&log->l_icloglock);
>  		xlog_state_want_sync(log, iclog);
>  		spin_unlock(&log->l_icloglock);
>  
> @@ -2504,7 +2500,9 @@ xlog_write(
>  
>  	ASSERT(len == 0);
>  
> +	spin_lock(&log->l_icloglock);
>  	xlog_state_finish_copy(log, iclog, record_cnt, data_cnt);
> +	spin_unlock(&log->l_icloglock);
>  	if (!commit_iclog)
>  		return xlog_state_release_iclog(log, iclog);
>  
> -- 
> 2.20.1
> 

Re: [PATCH 3/8] xfs: move the locking from xlog_state_finish_copy to the callers

[Brian Foster]

On Wed, Oct 09, 2019 at 04:27:43PM +0200, Christoph Hellwig wrote:
> This will allow optimizing various locking cycles in the following
> patches.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 4f5927ddfa40..860a555772fe 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1967,7 +1967,6 @@ xlog_dealloc_log(
>  /*
>   * Update counters atomically now that memcpy is done.
>   */
> -/* ARGSUSED */
>  static inline void
>  xlog_state_finish_copy(
>  	struct xlog		*log,
> @@ -1975,16 +1974,11 @@ xlog_state_finish_copy(
>  	int			record_cnt,
>  	int			copy_bytes)
>  {
> -	spin_lock(&log->l_icloglock);
> +	lockdep_assert_held(&log->l_icloglock);
>  
>  	be32_add_cpu(&iclog->ic_header.h_num_logops, record_cnt);
>  	iclog->ic_offset += copy_bytes;
> -
> -	spin_unlock(&log->l_icloglock);
> -}	/* xlog_state_finish_copy */
> -
> -
> -
> +}
>  
>  /*
>   * print out info relating to regions written which consume
> @@ -2266,7 +2260,9 @@ xlog_write_copy_finish(
>  		 * This iclog has already been marked WANT_SYNC by
>  		 * xlog_state_get_iclog_space.
>  		 */
> +		spin_lock(&log->l_icloglock);
>  		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
> +		spin_unlock(&log->l_icloglock);
>  		*record_cnt = 0;
>  		*data_cnt = 0;
>  		return xlog_state_release_iclog(log, iclog);
> @@ -2277,11 +2273,11 @@ xlog_write_copy_finish(
>  
>  	if (iclog->ic_size - log_offset <= sizeof(xlog_op_header_t)) {
>  		/* no more space in this iclog - push it. */
> +		spin_lock(&log->l_icloglock);
>  		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
>  		*record_cnt = 0;
>  		*data_cnt = 0;
>  
> -		spin_lock(&log->l_icloglock);
>  		xlog_state_want_sync(log, iclog);
>  		spin_unlock(&log->l_icloglock);
>  
> @@ -2504,7 +2500,9 @@ xlog_write(
>  
>  	ASSERT(len == 0);
>  
> +	spin_lock(&log->l_icloglock);
>  	xlog_state_finish_copy(log, iclog, record_cnt, data_cnt);
> +	spin_unlock(&log->l_icloglock);
>  	if (!commit_iclog)
>  		return xlog_state_release_iclog(log, iclog);
>  
> -- 
> 2.20.1
> 

[PATCH 4/8] xfs: call xlog_state_release_iclog with l_icloglock held

[Christoph Hellwig]

All but one caller of xlog_state_release_iclog hold l_icloglock and need
to drop and reacquire it to call xlog_state_release_iclog.  Switch the
xlog_state_release_iclog calling conventions to expect the lock to be
held, and open code the logic (using a shared helper) in the only
remaining caller that does not have the lock (and where not holding it
is a nice performance optimization).  Also move the refactored code to
require the least amount of forward declarations.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 860a555772fe..67a767d90ebf 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -57,10 +57,6 @@ xlog_state_get_iclog_space(
 	struct xlog_ticket	*ticket,
 	int			*continued_write,
 	int			*logoffsetp);
-STATIC int
-xlog_state_release_iclog(
-	struct xlog		*log,
-	struct xlog_in_core	*iclog);
 STATIC void
 xlog_state_switch_iclogs(
 	struct xlog		*log,
@@ -83,7 +79,10 @@ STATIC void
 xlog_ungrant_log_space(
 	struct xlog		*log,
 	struct xlog_ticket	*ticket);
-
+STATIC void
+xlog_sync(
+	struct xlog		*log,
+	struct xlog_in_core	*iclog);
 #if defined(DEBUG)
 STATIC void
 xlog_verify_dest_ptr(
@@ -552,16 +551,71 @@ xfs_log_done(
 	return lsn;
 }
 
+static bool
+__xlog_state_release_iclog(
+	struct xlog		*log,
+	struct xlog_in_core	*iclog)
+{
+	lockdep_assert_held(&log->l_icloglock);
+
+	if (iclog->ic_state == XLOG_STATE_WANT_SYNC) {
+		/* update tail before writing to iclog */
+		xfs_lsn_t tail_lsn = xlog_assign_tail_lsn(log->l_mp);
+
+		iclog->ic_state = XLOG_STATE_SYNCING;
+		iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
+		xlog_verify_tail_lsn(log, iclog, tail_lsn);
+		/* cycle incremented when incrementing curr_block */
+		return true;
+	}
+
+	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
+	return false;
+}
+
+/*
+ * Flush iclog to disk if this is the last reference to the given iclog and the
+ * it is in the WANT_SYNC state.
+ */
+static int
+xlog_state_release_iclog(
+	struct xlog		*log,
+	struct xlog_in_core	*iclog)
+{
+	lockdep_assert_held(&log->l_icloglock);
+
+	if (iclog->ic_state & XLOG_STATE_IOERROR)
+		return -EIO;
+
+	if (atomic_dec_and_test(&iclog->ic_refcnt) &&
+	    __xlog_state_release_iclog(log, iclog)) {
+		spin_unlock(&log->l_icloglock);
+		xlog_sync(log, iclog);
+		spin_lock(&log->l_icloglock);
+	}
+
+	return 0;
+}
+
 int
 xfs_log_release_iclog(
-	struct xfs_mount	*mp,
+	struct xfs_mount        *mp,
 	struct xlog_in_core	*iclog)
 {
-	if (xlog_state_release_iclog(mp->m_log, iclog)) {
+	struct xlog		*log = mp->m_log;
+	bool			sync;
+
+	if (iclog->ic_state & XLOG_STATE_IOERROR) {
 		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
 		return -EIO;
 	}
 
+	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
+		sync = __xlog_state_release_iclog(log, iclog);
+		spin_unlock(&log->l_icloglock);
+		if (sync)
+			xlog_sync(log, iclog);
+	}
 	return 0;
 }
 
@@ -866,10 +920,7 @@ xfs_log_write_unmount_record(
 	iclog = log->l_iclog;
 	atomic_inc(&iclog->ic_refcnt);
 	xlog_state_want_sync(log, iclog);
-	spin_unlock(&log->l_icloglock);
 	error = xlog_state_release_iclog(log, iclog);
-
-	spin_lock(&log->l_icloglock);
 	switch (iclog->ic_state) {
 	default:
 		if (!XLOG_FORCED_SHUTDOWN(log)) {
@@ -950,13 +1001,9 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 		spin_lock(&log->l_icloglock);
 		iclog = log->l_iclog;
 		atomic_inc(&iclog->ic_refcnt);
-
 		xlog_state_want_sync(log, iclog);
-		spin_unlock(&log->l_icloglock);
 		error =  xlog_state_release_iclog(log, iclog);
 
-		spin_lock(&log->l_icloglock);
-
 		if ( ! (   iclog->ic_state == XLOG_STATE_ACTIVE
 			|| iclog->ic_state == XLOG_STATE_DIRTY
 			|| iclog->ic_state == XLOG_STATE_IOERROR) ) {
@@ -2255,6 +2302,8 @@ xlog_write_copy_finish(
 	int			log_offset,
 	struct xlog_in_core	**commit_iclog)
 {
+	int			error;
+
 	if (*partial_copy) {
 		/*
 		 * This iclog has already been marked WANT_SYNC by
@@ -2262,10 +2311,9 @@ xlog_write_copy_finish(
 		 */
 		spin_lock(&log->l_icloglock);
 		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
-		spin_unlock(&log->l_icloglock);
 		*record_cnt = 0;
 		*data_cnt = 0;
-		return xlog_state_release_iclog(log, iclog);
+		goto release_iclog;
 	}
 
 	*partial_copy = 0;
@@ -2279,15 +2327,19 @@ xlog_write_copy_finish(
 		*data_cnt = 0;
 
 		xlog_state_want_sync(log, iclog);
-		spin_unlock(&log->l_icloglock);
-
 		if (!commit_iclog)
-			return xlog_state_release_iclog(log, iclog);
+			goto release_iclog;
+		spin_unlock(&log->l_icloglock);
 		ASSERT(flags & XLOG_COMMIT_TRANS);
 		*commit_iclog = iclog;
 	}
 
 	return 0;
+
+release_iclog:
+	error = xlog_state_release_iclog(log, iclog);
+	spin_unlock(&log->l_icloglock);
+	return error;
 }
 
 /*
@@ -2349,7 +2401,7 @@ xlog_write(
 	int			contwr = 0;
 	int			record_cnt = 0;
 	int			data_cnt = 0;
-	int			error;
+	int			error = 0;
 
 	*start_lsn = 0;
 
@@ -2502,13 +2554,15 @@ xlog_write(
 
 	spin_lock(&log->l_icloglock);
 	xlog_state_finish_copy(log, iclog, record_cnt, data_cnt);
+	if (commit_iclog) {
+		ASSERT(flags & XLOG_COMMIT_TRANS);
+		*commit_iclog = iclog;
+	} else {
+		error = xlog_state_release_iclog(log, iclog);
+	}
 	spin_unlock(&log->l_icloglock);
-	if (!commit_iclog)
-		return xlog_state_release_iclog(log, iclog);
 
-	ASSERT(flags & XLOG_COMMIT_TRANS);
-	*commit_iclog = iclog;
-	return 0;
+	return error;
 }
 
 
@@ -2979,7 +3033,6 @@ xlog_state_get_iclog_space(
 	int		  log_offset;
 	xlog_rec_header_t *head;
 	xlog_in_core_t	  *iclog;
-	int		  error;
 
 restart:
 	spin_lock(&log->l_icloglock);
@@ -3028,24 +3081,22 @@ xlog_state_get_iclog_space(
 	 * can fit into remaining data section.
 	 */
 	if (iclog->ic_size - iclog->ic_offset < 2*sizeof(xlog_op_header_t)) {
+		int		error = 0;
+
 		xlog_state_switch_iclogs(log, iclog, iclog->ic_size);
 
 		/*
-		 * If I'm the only one writing to this iclog, sync it to disk.
-		 * We need to do an atomic compare and decrement here to avoid
-		 * racing with concurrent atomic_dec_and_lock() calls in
+		 * If we are the only one writing to this iclog, sync it to
+		 * disk.  We need to do an atomic compare and decrement here to
+		 * avoid racing with concurrent atomic_dec_and_lock() calls in
 		 * xlog_state_release_iclog() when there is more than one
 		 * reference to the iclog.
 		 */
-		if (!atomic_add_unless(&iclog->ic_refcnt, -1, 1)) {
-			/* we are the only one */
-			spin_unlock(&log->l_icloglock);
+		if (!atomic_add_unless(&iclog->ic_refcnt, -1, 1))
 			error = xlog_state_release_iclog(log, iclog);
-			if (error)
-				return error;
-		} else {
-			spin_unlock(&log->l_icloglock);
-		}
+		spin_unlock(&log->l_icloglock);
+		if (error)
+			return error;
 		goto restart;
 	}
 
@@ -3156,60 +3207,6 @@ xlog_ungrant_log_space(
 	xfs_log_space_wake(log->l_mp);
 }
 
-/*
- * Flush iclog to disk if this is the last reference to the given iclog and
- * the WANT_SYNC bit is set.
- *
- * When this function is entered, the iclog is not necessarily in the
- * WANT_SYNC state.  It may be sitting around waiting to get filled.
- *
- *
- */
-STATIC int
-xlog_state_release_iclog(
-	struct xlog		*log,
-	struct xlog_in_core	*iclog)
-{
-	int		sync = 0;	/* do we sync? */
-
-	if (iclog->ic_state & XLOG_STATE_IOERROR)
-		return -EIO;
-
-	ASSERT(atomic_read(&iclog->ic_refcnt) > 0);
-	if (!atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock))
-		return 0;
-
-	if (iclog->ic_state & XLOG_STATE_IOERROR) {
-		spin_unlock(&log->l_icloglock);
-		return -EIO;
-	}
-	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE ||
-	       iclog->ic_state == XLOG_STATE_WANT_SYNC);
-
-	if (iclog->ic_state == XLOG_STATE_WANT_SYNC) {
-		/* update tail before writing to iclog */
-		xfs_lsn_t tail_lsn = xlog_assign_tail_lsn(log->l_mp);
-		sync++;
-		iclog->ic_state = XLOG_STATE_SYNCING;
-		iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
-		xlog_verify_tail_lsn(log, iclog, tail_lsn);
-		/* cycle incremented when incrementing curr_block */
-	}
-	spin_unlock(&log->l_icloglock);
-
-	/*
-	 * We let the log lock go, so it's possible that we hit a log I/O
-	 * error or some other SHUTDOWN condition that marks the iclog
-	 * as XLOG_STATE_IOERROR before the bwrite. However, we know that
-	 * this iclog has consistent data, so we ignore IOERROR
-	 * flags after this point.
-	 */
-	if (sync)
-		xlog_sync(log, iclog);
-	return 0;
-}	/* xlog_state_release_iclog */
-
-
 /*
  * This routine will mark the current iclog in the ring as WANT_SYNC
  * and move the current iclog pointer to the next iclog in the ring.
@@ -3333,12 +3330,9 @@ xfs_log_force(
 			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;
+				goto out_error;	
 
-			spin_lock(&log->l_icloglock);
 			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn ||
 			    iclog->ic_state == XLOG_STATE_DIRTY)
 				goto out_unlock;
@@ -3433,12 +3427,10 @@ __xfs_log_force_lsn(
 		}
 		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;
+			goto out_error;
 		if (log_flushed)
 			*log_flushed = 1;
-		spin_lock(&log->l_icloglock);
 	}
 
 	if (!(flags & XFS_LOG_SYNC) ||
-- 
2.20.1

Re: [PATCH 4/8] xfs: call xlog_state_release_iclog with l_icloglock held

[Darrick J. Wong]

On Wed, Oct 09, 2019 at 04:27:44PM +0200, Christoph Hellwig wrote:
> All but one caller of xlog_state_release_iclog hold l_icloglock and need
> to drop and reacquire it to call xlog_state_release_iclog.  Switch the
> xlog_state_release_iclog calling conventions to expect the lock to be
> held, and open code the logic (using a shared helper) in the only
> remaining caller that does not have the lock (and where not holding it
> is a nice performance optimization).  Also move the refactored code to
> require the least amount of forward declarations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 188 +++++++++++++++++++++++------------------------
>  1 file changed, 90 insertions(+), 98 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 860a555772fe..67a767d90ebf 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -57,10 +57,6 @@ xlog_state_get_iclog_space(
>  	struct xlog_ticket	*ticket,
>  	int			*continued_write,
>  	int			*logoffsetp);
> -STATIC int
> -xlog_state_release_iclog(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog);
>  STATIC void
>  xlog_state_switch_iclogs(
>  	struct xlog		*log,
> @@ -83,7 +79,10 @@ STATIC void
>  xlog_ungrant_log_space(
>  	struct xlog		*log,
>  	struct xlog_ticket	*ticket);
> -
> +STATIC void
> +xlog_sync(
> +	struct xlog		*log,
> +	struct xlog_in_core	*iclog);
>  #if defined(DEBUG)
>  STATIC void
>  xlog_verify_dest_ptr(
> @@ -552,16 +551,71 @@ xfs_log_done(
>  	return lsn;
>  }
>  
> +static bool
> +__xlog_state_release_iclog(
> +	struct xlog		*log,
> +	struct xlog_in_core	*iclog)
> +{
> +	lockdep_assert_held(&log->l_icloglock);
> +
> +	if (iclog->ic_state == XLOG_STATE_WANT_SYNC) {
> +		/* update tail before writing to iclog */
> +		xfs_lsn_t tail_lsn = xlog_assign_tail_lsn(log->l_mp);
> +
> +		iclog->ic_state = XLOG_STATE_SYNCING;
> +		iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
> +		xlog_verify_tail_lsn(log, iclog, tail_lsn);
> +		/* cycle incremented when incrementing curr_block */
> +		return true;
> +	}
> +
> +	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
> +	return false;
> +}
> +
> +/*
> + * Flush iclog to disk if this is the last reference to the given iclog and the
> + * it is in the WANT_SYNC state.
> + */
> +static int
> +xlog_state_release_iclog(
> +	struct xlog		*log,
> +	struct xlog_in_core	*iclog)
> +{
> +	lockdep_assert_held(&log->l_icloglock);
> +
> +	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +		return -EIO;
> +
> +	if (atomic_dec_and_test(&iclog->ic_refcnt) &&
> +	    __xlog_state_release_iclog(log, iclog)) {
> +		spin_unlock(&log->l_icloglock);
> +		xlog_sync(log, iclog);
> +		spin_lock(&log->l_icloglock);
> +	}
> +
> +	return 0;
> +}
> +
>  int
>  xfs_log_release_iclog(
> -	struct xfs_mount	*mp,
> +	struct xfs_mount        *mp,
>  	struct xlog_in_core	*iclog)
>  {
> -	if (xlog_state_release_iclog(mp->m_log, iclog)) {
> +	struct xlog		*log = mp->m_log;
> +	bool			sync;
> +
> +	if (iclog->ic_state & XLOG_STATE_IOERROR) {
>  		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
>  		return -EIO;
>  	}
>  
> +	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
> +		sync = __xlog_state_release_iclog(log, iclog);
> +		spin_unlock(&log->l_icloglock);
> +		if (sync)
> +			xlog_sync(log, iclog);
> +	}
>  	return 0;
>  }
>  
> @@ -866,10 +920,7 @@ xfs_log_write_unmount_record(
>  	iclog = log->l_iclog;
>  	atomic_inc(&iclog->ic_refcnt);
>  	xlog_state_want_sync(log, iclog);
> -	spin_unlock(&log->l_icloglock);
>  	error = xlog_state_release_iclog(log, iclog);
> -
> -	spin_lock(&log->l_icloglock);
>  	switch (iclog->ic_state) {
>  	default:
>  		if (!XLOG_FORCED_SHUTDOWN(log)) {
> @@ -950,13 +1001,9 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		spin_lock(&log->l_icloglock);
>  		iclog = log->l_iclog;
>  		atomic_inc(&iclog->ic_refcnt);
> -
>  		xlog_state_want_sync(log, iclog);
> -		spin_unlock(&log->l_icloglock);
>  		error =  xlog_state_release_iclog(log, iclog);
>  
> -		spin_lock(&log->l_icloglock);
> -
>  		if ( ! (   iclog->ic_state == XLOG_STATE_ACTIVE
>  			|| iclog->ic_state == XLOG_STATE_DIRTY
>  			|| iclog->ic_state == XLOG_STATE_IOERROR) ) {
> @@ -2255,6 +2302,8 @@ xlog_write_copy_finish(
>  	int			log_offset,
>  	struct xlog_in_core	**commit_iclog)
>  {
> +	int			error;
> +
>  	if (*partial_copy) {
>  		/*
>  		 * This iclog has already been marked WANT_SYNC by
> @@ -2262,10 +2311,9 @@ xlog_write_copy_finish(
>  		 */
>  		spin_lock(&log->l_icloglock);
>  		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
> -		spin_unlock(&log->l_icloglock);
>  		*record_cnt = 0;
>  		*data_cnt = 0;
> -		return xlog_state_release_iclog(log, iclog);
> +		goto release_iclog;
>  	}
>  
>  	*partial_copy = 0;
> @@ -2279,15 +2327,19 @@ xlog_write_copy_finish(
>  		*data_cnt = 0;
>  
>  		xlog_state_want_sync(log, iclog);
> -		spin_unlock(&log->l_icloglock);
> -
>  		if (!commit_iclog)
> -			return xlog_state_release_iclog(log, iclog);
> +			goto release_iclog;
> +		spin_unlock(&log->l_icloglock);
>  		ASSERT(flags & XLOG_COMMIT_TRANS);
>  		*commit_iclog = iclog;
>  	}
>  
>  	return 0;
> +
> +release_iclog:
> +	error = xlog_state_release_iclog(log, iclog);
> +	spin_unlock(&log->l_icloglock);
> +	return error;
>  }
>  
>  /*
> @@ -2349,7 +2401,7 @@ xlog_write(
>  	int			contwr = 0;
>  	int			record_cnt = 0;
>  	int			data_cnt = 0;
> -	int			error;
> +	int			error = 0;
>  
>  	*start_lsn = 0;
>  
> @@ -2502,13 +2554,15 @@ xlog_write(
>  
>  	spin_lock(&log->l_icloglock);
>  	xlog_state_finish_copy(log, iclog, record_cnt, data_cnt);
> +	if (commit_iclog) {
> +		ASSERT(flags & XLOG_COMMIT_TRANS);
> +		*commit_iclog = iclog;
> +	} else {
> +		error = xlog_state_release_iclog(log, iclog);
> +	}
>  	spin_unlock(&log->l_icloglock);
> -	if (!commit_iclog)
> -		return xlog_state_release_iclog(log, iclog);
>  
> -	ASSERT(flags & XLOG_COMMIT_TRANS);
> -	*commit_iclog = iclog;
> -	return 0;
> +	return error;
>  }
>  
>  
> @@ -2979,7 +3033,6 @@ xlog_state_get_iclog_space(
>  	int		  log_offset;
>  	xlog_rec_header_t *head;
>  	xlog_in_core_t	  *iclog;
> -	int		  error;
>  
>  restart:
>  	spin_lock(&log->l_icloglock);
> @@ -3028,24 +3081,22 @@ xlog_state_get_iclog_space(
>  	 * can fit into remaining data section.
>  	 */
>  	if (iclog->ic_size - iclog->ic_offset < 2*sizeof(xlog_op_header_t)) {
> +		int		error = 0;
> +
>  		xlog_state_switch_iclogs(log, iclog, iclog->ic_size);
>  
>  		/*
> -		 * If I'm the only one writing to this iclog, sync it to disk.
> -		 * We need to do an atomic compare and decrement here to avoid
> -		 * racing with concurrent atomic_dec_and_lock() calls in
> +		 * If we are the only one writing to this iclog, sync it to
> +		 * disk.  We need to do an atomic compare and decrement here to
> +		 * avoid racing with concurrent atomic_dec_and_lock() calls in
>  		 * xlog_state_release_iclog() when there is more than one
>  		 * reference to the iclog.
>  		 */
> -		if (!atomic_add_unless(&iclog->ic_refcnt, -1, 1)) {
> -			/* we are the only one */
> -			spin_unlock(&log->l_icloglock);
> +		if (!atomic_add_unless(&iclog->ic_refcnt, -1, 1))
>  			error = xlog_state_release_iclog(log, iclog);
> -			if (error)
> -				return error;
> -		} else {
> -			spin_unlock(&log->l_icloglock);
> -		}
> +		spin_unlock(&log->l_icloglock);
> +		if (error)
> +			return error;
>  		goto restart;
>  	}
>  
> @@ -3156,60 +3207,6 @@ xlog_ungrant_log_space(
>  	xfs_log_space_wake(log->l_mp);
>  }
>  
> -/*
> - * Flush iclog to disk if this is the last reference to the given iclog and
> - * the WANT_SYNC bit is set.
> - *
> - * When this function is entered, the iclog is not necessarily in the
> - * WANT_SYNC state.  It may be sitting around waiting to get filled.
> - *
> - *
> - */
> -STATIC int
> -xlog_state_release_iclog(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog)
> -{
> -	int		sync = 0;	/* do we sync? */
> -
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> -		return -EIO;
> -
> -	ASSERT(atomic_read(&iclog->ic_refcnt) > 0);
> -	if (!atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock))
> -		return 0;
> -
> -	if (iclog->ic_state & XLOG_STATE_IOERROR) {
> -		spin_unlock(&log->l_icloglock);
> -		return -EIO;
> -	}
> -	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE ||
> -	       iclog->ic_state == XLOG_STATE_WANT_SYNC);
> -
> -	if (iclog->ic_state == XLOG_STATE_WANT_SYNC) {
> -		/* update tail before writing to iclog */
> -		xfs_lsn_t tail_lsn = xlog_assign_tail_lsn(log->l_mp);
> -		sync++;
> -		iclog->ic_state = XLOG_STATE_SYNCING;
> -		iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
> -		xlog_verify_tail_lsn(log, iclog, tail_lsn);
> -		/* cycle incremented when incrementing curr_block */
> -	}
> -	spin_unlock(&log->l_icloglock);
> -
> -	/*
> -	 * We let the log lock go, so it's possible that we hit a log I/O
> -	 * error or some other SHUTDOWN condition that marks the iclog
> -	 * as XLOG_STATE_IOERROR before the bwrite. However, we know that
> -	 * this iclog has consistent data, so we ignore IOERROR
> -	 * flags after this point.
> -	 */
> -	if (sync)
> -		xlog_sync(log, iclog);
> -	return 0;
> -}	/* xlog_state_release_iclog */
> -
> -
>  /*
>   * This routine will mark the current iclog in the ring as WANT_SYNC
>   * and move the current iclog pointer to the next iclog in the ring.
> @@ -3333,12 +3330,9 @@ xfs_log_force(
>  			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;
> +				goto out_error;	

Extra whitespace here                          ^

I kinda wish the relocation and refactoring of xlog_state_release_iclog
had happened as separate patches so that the redistribution of locking
responsibilities in this patch would be more straightforward, but oh
well, I got through it.

With the whitespace nit fixed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  
> -			spin_lock(&log->l_icloglock);
>  			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn ||
>  			    iclog->ic_state == XLOG_STATE_DIRTY)
>  				goto out_unlock;
> @@ -3433,12 +3427,10 @@ __xfs_log_force_lsn(
>  		}
>  		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;
> +			goto out_error;
>  		if (log_flushed)
>  			*log_flushed = 1;
> -		spin_lock(&log->l_icloglock);
>  	}
>  
>  	if (!(flags & XFS_LOG_SYNC) ||
> -- 
> 2.20.1
> 

Re: [PATCH 4/8] xfs: call xlog_state_release_iclog with l_icloglock held

[Brian Foster]

On Wed, Oct 09, 2019 at 04:27:44PM +0200, Christoph Hellwig wrote:
> All but one caller of xlog_state_release_iclog hold l_icloglock and need
> to drop and reacquire it to call xlog_state_release_iclog.  Switch the
> xlog_state_release_iclog calling conventions to expect the lock to be
> held, and open code the logic (using a shared helper) in the only
> remaining caller that does not have the lock (and where not holding it
> is a nice performance optimization).  Also move the refactored code to
> require the least amount of forward declarations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Modulo the whitespace thing:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c | 188 +++++++++++++++++++++++------------------------
>  1 file changed, 90 insertions(+), 98 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 860a555772fe..67a767d90ebf 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -57,10 +57,6 @@ xlog_state_get_iclog_space(
>  	struct xlog_ticket	*ticket,
>  	int			*continued_write,
>  	int			*logoffsetp);
> -STATIC int
> -xlog_state_release_iclog(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog);
>  STATIC void
>  xlog_state_switch_iclogs(
>  	struct xlog		*log,
> @@ -83,7 +79,10 @@ STATIC void
>  xlog_ungrant_log_space(
>  	struct xlog		*log,
>  	struct xlog_ticket	*ticket);
> -
> +STATIC void
> +xlog_sync(
> +	struct xlog		*log,
> +	struct xlog_in_core	*iclog);
>  #if defined(DEBUG)
>  STATIC void
>  xlog_verify_dest_ptr(
> @@ -552,16 +551,71 @@ xfs_log_done(
>  	return lsn;
>  }
>  
> +static bool
> +__xlog_state_release_iclog(
> +	struct xlog		*log,
> +	struct xlog_in_core	*iclog)
> +{
> +	lockdep_assert_held(&log->l_icloglock);
> +
> +	if (iclog->ic_state == XLOG_STATE_WANT_SYNC) {
> +		/* update tail before writing to iclog */
> +		xfs_lsn_t tail_lsn = xlog_assign_tail_lsn(log->l_mp);
> +
> +		iclog->ic_state = XLOG_STATE_SYNCING;
> +		iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
> +		xlog_verify_tail_lsn(log, iclog, tail_lsn);
> +		/* cycle incremented when incrementing curr_block */
> +		return true;
> +	}
> +
> +	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
> +	return false;
> +}
> +
> +/*
> + * Flush iclog to disk if this is the last reference to the given iclog and the
> + * it is in the WANT_SYNC state.
> + */
> +static int
> +xlog_state_release_iclog(
> +	struct xlog		*log,
> +	struct xlog_in_core	*iclog)
> +{
> +	lockdep_assert_held(&log->l_icloglock);
> +
> +	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +		return -EIO;
> +
> +	if (atomic_dec_and_test(&iclog->ic_refcnt) &&
> +	    __xlog_state_release_iclog(log, iclog)) {
> +		spin_unlock(&log->l_icloglock);
> +		xlog_sync(log, iclog);
> +		spin_lock(&log->l_icloglock);
> +	}
> +
> +	return 0;
> +}
> +
>  int
>  xfs_log_release_iclog(
> -	struct xfs_mount	*mp,
> +	struct xfs_mount        *mp,
>  	struct xlog_in_core	*iclog)
>  {
> -	if (xlog_state_release_iclog(mp->m_log, iclog)) {
> +	struct xlog		*log = mp->m_log;
> +	bool			sync;
> +
> +	if (iclog->ic_state & XLOG_STATE_IOERROR) {
>  		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
>  		return -EIO;
>  	}
>  
> +	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
> +		sync = __xlog_state_release_iclog(log, iclog);
> +		spin_unlock(&log->l_icloglock);
> +		if (sync)
> +			xlog_sync(log, iclog);
> +	}
>  	return 0;
>  }
>  
> @@ -866,10 +920,7 @@ xfs_log_write_unmount_record(
>  	iclog = log->l_iclog;
>  	atomic_inc(&iclog->ic_refcnt);
>  	xlog_state_want_sync(log, iclog);
> -	spin_unlock(&log->l_icloglock);
>  	error = xlog_state_release_iclog(log, iclog);
> -
> -	spin_lock(&log->l_icloglock);
>  	switch (iclog->ic_state) {
>  	default:
>  		if (!XLOG_FORCED_SHUTDOWN(log)) {
> @@ -950,13 +1001,9 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		spin_lock(&log->l_icloglock);
>  		iclog = log->l_iclog;
>  		atomic_inc(&iclog->ic_refcnt);
> -
>  		xlog_state_want_sync(log, iclog);
> -		spin_unlock(&log->l_icloglock);
>  		error =  xlog_state_release_iclog(log, iclog);
>  
> -		spin_lock(&log->l_icloglock);
> -
>  		if ( ! (   iclog->ic_state == XLOG_STATE_ACTIVE
>  			|| iclog->ic_state == XLOG_STATE_DIRTY
>  			|| iclog->ic_state == XLOG_STATE_IOERROR) ) {
> @@ -2255,6 +2302,8 @@ xlog_write_copy_finish(
>  	int			log_offset,
>  	struct xlog_in_core	**commit_iclog)
>  {
> +	int			error;
> +
>  	if (*partial_copy) {
>  		/*
>  		 * This iclog has already been marked WANT_SYNC by
> @@ -2262,10 +2311,9 @@ xlog_write_copy_finish(
>  		 */
>  		spin_lock(&log->l_icloglock);
>  		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
> -		spin_unlock(&log->l_icloglock);
>  		*record_cnt = 0;
>  		*data_cnt = 0;
> -		return xlog_state_release_iclog(log, iclog);
> +		goto release_iclog;
>  	}
>  
>  	*partial_copy = 0;
> @@ -2279,15 +2327,19 @@ xlog_write_copy_finish(
>  		*data_cnt = 0;
>  
>  		xlog_state_want_sync(log, iclog);
> -		spin_unlock(&log->l_icloglock);
> -
>  		if (!commit_iclog)
> -			return xlog_state_release_iclog(log, iclog);
> +			goto release_iclog;
> +		spin_unlock(&log->l_icloglock);
>  		ASSERT(flags & XLOG_COMMIT_TRANS);
>  		*commit_iclog = iclog;
>  	}
>  
>  	return 0;
> +
> +release_iclog:
> +	error = xlog_state_release_iclog(log, iclog);
> +	spin_unlock(&log->l_icloglock);
> +	return error;
>  }
>  
>  /*
> @@ -2349,7 +2401,7 @@ xlog_write(
>  	int			contwr = 0;
>  	int			record_cnt = 0;
>  	int			data_cnt = 0;
> -	int			error;
> +	int			error = 0;
>  
>  	*start_lsn = 0;
>  
> @@ -2502,13 +2554,15 @@ xlog_write(
>  
>  	spin_lock(&log->l_icloglock);
>  	xlog_state_finish_copy(log, iclog, record_cnt, data_cnt);
> +	if (commit_iclog) {
> +		ASSERT(flags & XLOG_COMMIT_TRANS);
> +		*commit_iclog = iclog;
> +	} else {
> +		error = xlog_state_release_iclog(log, iclog);
> +	}
>  	spin_unlock(&log->l_icloglock);
> -	if (!commit_iclog)
> -		return xlog_state_release_iclog(log, iclog);
>  
> -	ASSERT(flags & XLOG_COMMIT_TRANS);
> -	*commit_iclog = iclog;
> -	return 0;
> +	return error;
>  }
>  
>  
> @@ -2979,7 +3033,6 @@ xlog_state_get_iclog_space(
>  	int		  log_offset;
>  	xlog_rec_header_t *head;
>  	xlog_in_core_t	  *iclog;
> -	int		  error;
>  
>  restart:
>  	spin_lock(&log->l_icloglock);
> @@ -3028,24 +3081,22 @@ xlog_state_get_iclog_space(
>  	 * can fit into remaining data section.
>  	 */
>  	if (iclog->ic_size - iclog->ic_offset < 2*sizeof(xlog_op_header_t)) {
> +		int		error = 0;
> +
>  		xlog_state_switch_iclogs(log, iclog, iclog->ic_size);
>  
>  		/*
> -		 * If I'm the only one writing to this iclog, sync it to disk.
> -		 * We need to do an atomic compare and decrement here to avoid
> -		 * racing with concurrent atomic_dec_and_lock() calls in
> +		 * If we are the only one writing to this iclog, sync it to
> +		 * disk.  We need to do an atomic compare and decrement here to
> +		 * avoid racing with concurrent atomic_dec_and_lock() calls in
>  		 * xlog_state_release_iclog() when there is more than one
>  		 * reference to the iclog.
>  		 */
> -		if (!atomic_add_unless(&iclog->ic_refcnt, -1, 1)) {
> -			/* we are the only one */
> -			spin_unlock(&log->l_icloglock);
> +		if (!atomic_add_unless(&iclog->ic_refcnt, -1, 1))
>  			error = xlog_state_release_iclog(log, iclog);
> -			if (error)
> -				return error;
> -		} else {
> -			spin_unlock(&log->l_icloglock);
> -		}
> +		spin_unlock(&log->l_icloglock);
> +		if (error)
> +			return error;
>  		goto restart;
>  	}
>  
> @@ -3156,60 +3207,6 @@ xlog_ungrant_log_space(
>  	xfs_log_space_wake(log->l_mp);
>  }
>  
> -/*
> - * Flush iclog to disk if this is the last reference to the given iclog and
> - * the WANT_SYNC bit is set.
> - *
> - * When this function is entered, the iclog is not necessarily in the
> - * WANT_SYNC state.  It may be sitting around waiting to get filled.
> - *
> - *
> - */
> -STATIC int
> -xlog_state_release_iclog(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog)
> -{
> -	int		sync = 0;	/* do we sync? */
> -
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> -		return -EIO;
> -
> -	ASSERT(atomic_read(&iclog->ic_refcnt) > 0);
> -	if (!atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock))
> -		return 0;
> -
> -	if (iclog->ic_state & XLOG_STATE_IOERROR) {
> -		spin_unlock(&log->l_icloglock);
> -		return -EIO;
> -	}
> -	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE ||
> -	       iclog->ic_state == XLOG_STATE_WANT_SYNC);
> -
> -	if (iclog->ic_state == XLOG_STATE_WANT_SYNC) {
> -		/* update tail before writing to iclog */
> -		xfs_lsn_t tail_lsn = xlog_assign_tail_lsn(log->l_mp);
> -		sync++;
> -		iclog->ic_state = XLOG_STATE_SYNCING;
> -		iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
> -		xlog_verify_tail_lsn(log, iclog, tail_lsn);
> -		/* cycle incremented when incrementing curr_block */
> -	}
> -	spin_unlock(&log->l_icloglock);
> -
> -	/*
> -	 * We let the log lock go, so it's possible that we hit a log I/O
> -	 * error or some other SHUTDOWN condition that marks the iclog
> -	 * as XLOG_STATE_IOERROR before the bwrite. However, we know that
> -	 * this iclog has consistent data, so we ignore IOERROR
> -	 * flags after this point.
> -	 */
> -	if (sync)
> -		xlog_sync(log, iclog);
> -	return 0;
> -}	/* xlog_state_release_iclog */
> -
> -
>  /*
>   * This routine will mark the current iclog in the ring as WANT_SYNC
>   * and move the current iclog pointer to the next iclog in the ring.
> @@ -3333,12 +3330,9 @@ xfs_log_force(
>  			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;
> +				goto out_error;	
>  
> -			spin_lock(&log->l_icloglock);
>  			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn ||
>  			    iclog->ic_state == XLOG_STATE_DIRTY)
>  				goto out_unlock;
> @@ -3433,12 +3427,10 @@ __xfs_log_force_lsn(
>  		}
>  		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;
> +			goto out_error;
>  		if (log_flushed)
>  			*log_flushed = 1;
> -		spin_lock(&log->l_icloglock);
>  	}
>  
>  	if (!(flags & XFS_LOG_SYNC) ||
> -- 
> 2.20.1
> 

[PATCH 5/8] xfs: remove dead ifdef XFSERRORDEBUG code

[Christoph Hellwig]

XFSERRORDEBUG is never set and the code isn't all that useful, so remove
it.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 67a767d90ebf..7a429e5dc27c 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3996,19 +3996,6 @@ xfs_log_force_umount(
 	spin_unlock(&log->l_cilp->xc_push_lock);
 	xlog_state_do_callback(log, true, NULL);
 
-#ifdef XFSERRORDEBUG
-	{
-		xlog_in_core_t	*iclog;
-
-		spin_lock(&log->l_icloglock);
-		iclog = log->l_iclog;
-		do {
-			ASSERT(iclog->ic_callback == 0);
-			iclog = iclog->ic_next;
-		} while (iclog != log->l_iclog);
-		spin_unlock(&log->l_icloglock);
-	}
-#endif
 	/* return non-zero if log IOERROR transition had already happened */
 	return retval;
 }
-- 
2.20.1

Re: [PATCH 5/8] xfs: remove dead ifdef XFSERRORDEBUG code

[Darrick J. Wong]

On Wed, Oct 09, 2019 at 04:27:45PM +0200, Christoph Hellwig wrote:
> XFSERRORDEBUG is never set and the code isn't all that useful, so remove
> it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 67a767d90ebf..7a429e5dc27c 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3996,19 +3996,6 @@ xfs_log_force_umount(
>  	spin_unlock(&log->l_cilp->xc_push_lock);
>  	xlog_state_do_callback(log, true, NULL);
>  
> -#ifdef XFSERRORDEBUG
> -	{
> -		xlog_in_core_t	*iclog;
> -
> -		spin_lock(&log->l_icloglock);
> -		iclog = log->l_iclog;
> -		do {
> -			ASSERT(iclog->ic_callback == 0);
> -			iclog = iclog->ic_next;
> -		} while (iclog != log->l_iclog);
> -		spin_unlock(&log->l_icloglock);
> -	}
> -#endif
>  	/* return non-zero if log IOERROR transition had already happened */
>  	return retval;
>  }
> -- 
> 2.20.1
> 

Re: [PATCH 5/8] xfs: remove dead ifdef XFSERRORDEBUG code

[Brian Foster]

On Wed, Oct 09, 2019 at 04:27:45PM +0200, Christoph Hellwig wrote:
> XFSERRORDEBUG is never set and the code isn't all that useful, so remove
> it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 67a767d90ebf..7a429e5dc27c 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3996,19 +3996,6 @@ xfs_log_force_umount(
>  	spin_unlock(&log->l_cilp->xc_push_lock);
>  	xlog_state_do_callback(log, true, NULL);
>  
> -#ifdef XFSERRORDEBUG
> -	{
> -		xlog_in_core_t	*iclog;
> -
> -		spin_lock(&log->l_icloglock);
> -		iclog = log->l_iclog;
> -		do {
> -			ASSERT(iclog->ic_callback == 0);
> -			iclog = iclog->ic_next;
> -		} while (iclog != log->l_iclog);
> -		spin_unlock(&log->l_icloglock);
> -	}
> -#endif
>  	/* return non-zero if log IOERROR transition had already happened */
>  	return retval;
>  }
> -- 
> 2.20.1
> 

[PATCH 6/8] xfs: remove the unused XLOG_STATE_ALL and XLOG_STATE_UNUSED flags

[Christoph Hellwig]

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

diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 90e210e433cf..66bd370ae60a 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -49,8 +49,6 @@ static inline uint xlog_get_client_id(__be32 i)
 #define XLOG_STATE_CALLBACK  0x0020 /* Callback functions now */
 #define XLOG_STATE_DIRTY     0x0040 /* Dirty IC log, not ready for ACTIVE status*/
 #define XLOG_STATE_IOERROR   0x0080 /* IO error happened in sync'ing log */
-#define XLOG_STATE_ALL	     0x7FFF /* All possible valid flags */
-#define XLOG_STATE_NOTUSED   0x8000 /* This IC log not being used */
 
 /*
  * Flags to log ticket
-- 
2.20.1

Re: [PATCH 6/8] xfs: remove the unused XLOG_STATE_ALL and XLOG_STATE_UNUSED flags

[Darrick J. Wong]

The subject line should say "XLOG_STATE_NOTUSED", not "XLOG_STATE_UNUSED".

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

--D


On Wed, Oct 09, 2019 at 04:27:46PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log_priv.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 90e210e433cf..66bd370ae60a 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -49,8 +49,6 @@ static inline uint xlog_get_client_id(__be32 i)
>  #define XLOG_STATE_CALLBACK  0x0020 /* Callback functions now */
>  #define XLOG_STATE_DIRTY     0x0040 /* Dirty IC log, not ready for ACTIVE status*/
>  #define XLOG_STATE_IOERROR   0x0080 /* IO error happened in sync'ing log */
> -#define XLOG_STATE_ALL	     0x7FFF /* All possible valid flags */
> -#define XLOG_STATE_NOTUSED   0x8000 /* This IC log not being used */
>  
>  /*
>   * Flags to log ticket
> -- 
> 2.20.1
> 

Re: [PATCH 6/8] xfs: remove the unused XLOG_STATE_ALL and XLOG_STATE_UNUSED flags

[Brian Foster]

On Wed, Oct 09, 2019 at 04:27:46PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log_priv.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 90e210e433cf..66bd370ae60a 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -49,8 +49,6 @@ static inline uint xlog_get_client_id(__be32 i)
>  #define XLOG_STATE_CALLBACK  0x0020 /* Callback functions now */
>  #define XLOG_STATE_DIRTY     0x0040 /* Dirty IC log, not ready for ACTIVE status*/
>  #define XLOG_STATE_IOERROR   0x0080 /* IO error happened in sync'ing log */
> -#define XLOG_STATE_ALL	     0x7FFF /* All possible valid flags */
> -#define XLOG_STATE_NOTUSED   0x8000 /* This IC log not being used */
>  
>  /*
>   * Flags to log ticket
> -- 
> 2.20.1
> 

[PATCH 7/8] xfs: turn ic_state into an enum

[Christoph Hellwig]

ic_state really is a set of different states, even if the values are
encoded as non-conflicting bits and we sometimes use logical and
operations to check for them.  Switch all comparisms to check for
exact values (and use switch statements in a few places to make it
more clear) and turn the values into an implicitly enumerated enum
type.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 159 ++++++++++++++++++++----------------------
 fs/xfs/xfs_log_cil.c  |   2 +-
 fs/xfs/xfs_log_priv.h |  21 +++---
 3 files changed, 88 insertions(+), 94 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 7a429e5dc27c..001a9586cb56 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -584,7 +584,7 @@ xlog_state_release_iclog(
 {
 	lockdep_assert_held(&log->l_icloglock);
 
-	if (iclog->ic_state & XLOG_STATE_IOERROR)
+	if (iclog->ic_state == XLOG_STATE_IOERROR)
 		return -EIO;
 
 	if (atomic_dec_and_test(&iclog->ic_refcnt) &&
@@ -605,7 +605,7 @@ xfs_log_release_iclog(
 	struct xlog		*log = mp->m_log;
 	bool			sync;
 
-	if (iclog->ic_state & XLOG_STATE_IOERROR) {
+	if (iclog->ic_state == XLOG_STATE_IOERROR) {
 		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
 		return -EIO;
 	}
@@ -975,8 +975,8 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 #ifdef DEBUG
 	first_iclog = iclog = log->l_iclog;
 	do {
-		if (!(iclog->ic_state & XLOG_STATE_IOERROR)) {
-			ASSERT(iclog->ic_state & XLOG_STATE_ACTIVE);
+		if (iclog->ic_state != XLOG_STATE_IOERROR) {
+			ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
 			ASSERT(iclog->ic_offset == 0);
 		}
 		iclog = iclog->ic_next;
@@ -1003,15 +1003,15 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 		atomic_inc(&iclog->ic_refcnt);
 		xlog_state_want_sync(log, iclog);
 		error =  xlog_state_release_iclog(log, iclog);
-
-		if ( ! (   iclog->ic_state == XLOG_STATE_ACTIVE
-			|| iclog->ic_state == XLOG_STATE_DIRTY
-			|| iclog->ic_state == XLOG_STATE_IOERROR) ) {
-
-				xlog_wait(&iclog->ic_force_wait,
-							&log->l_icloglock);
-		} else {
+		switch (iclog->ic_state) {
+		case XLOG_STATE_ACTIVE:
+		case XLOG_STATE_DIRTY:
+		case XLOG_STATE_IOERROR:
 			spin_unlock(&log->l_icloglock);
+			break;
+		default:
+			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
+			break;
 		}
 	}
 
@@ -1301,7 +1301,7 @@ xlog_ioend_work(
 		 * didn't succeed.
 		 */
 		aborted = true;
-	} else if (iclog->ic_state & XLOG_STATE_IOERROR) {
+	} else if (iclog->ic_state == XLOG_STATE_IOERROR) {
 		aborted = true;
 	}
 
@@ -1774,7 +1774,7 @@ xlog_write_iclog(
 	 * across the log IO to archieve that.
 	 */
 	down(&iclog->ic_sema);
-	if (unlikely(iclog->ic_state & XLOG_STATE_IOERROR)) {
+	if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) {
 		/*
 		 * It would seem logical to return EIO here, but we rely on
 		 * the log state machine to propagate I/O errors instead of
@@ -2598,7 +2598,7 @@ xlog_state_clean_iclog(
 	int			changed = 0;
 
 	/* Prepare the completed iclog. */
-	if (!(dirty_iclog->ic_state & XLOG_STATE_IOERROR))
+	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
 		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
 
 	/* Walk all the iclogs to update the ordered active state. */
@@ -2689,7 +2689,8 @@ xlog_get_lowest_lsn(
 	xfs_lsn_t		lowest_lsn = 0, lsn;
 
 	do {
-		if (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY))
+		if (iclog->ic_state == XLOG_STATE_ACTIVE ||
+		    iclog->ic_state == XLOG_STATE_DIRTY)
 			continue;
 
 		lsn = be64_to_cpu(iclog->ic_header.h_lsn);
@@ -2755,55 +2756,50 @@ xlog_state_iodone_process_iclog(
 	xfs_lsn_t		lowest_lsn;
 	xfs_lsn_t		header_lsn;
 
-	/* Skip all iclogs in the ACTIVE & DIRTY states */
-	if (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY))
+	switch (iclog->ic_state) {
+	case XLOG_STATE_ACTIVE:
+	case XLOG_STATE_DIRTY:
+		/*
+		 * Skip all iclogs in the ACTIVE & DIRTY states:
+		 */
 		return false;
-
-	/*
-	 * Between marking a filesystem SHUTDOWN and stopping the log, we do
-	 * flush all iclogs to disk (if there wasn't a log I/O error). So, we do
-	 * want things to go smoothly in case of just a SHUTDOWN  w/o a
-	 * LOG_IO_ERROR.
-	 */
-	if (iclog->ic_state & XLOG_STATE_IOERROR) {
+	case XLOG_STATE_IOERROR:
+		/*
+		 * Between marking a filesystem SHUTDOWN and stopping the log,
+		 * we do flush all iclogs to disk (if there wasn't a log I/O
+		 * error). So, we do want things to go smoothly in case of just
+		 * a SHUTDOWN  w/o a LOG_IO_ERROR.
+		 */
 		*ioerror = true;
 		return false;
-	}
-
-	/*
-	 * Can only perform callbacks in order.  Since this iclog is not in the
-	 * DONE_SYNC/ DO_CALLBACK state, we skip the rest and just try to clean
-	 * up.  If we set our iclog to DO_CALLBACK, we will not process it when
-	 * we retry since a previous iclog is in the CALLBACK and the state
-	 * cannot change since we are holding the l_icloglock.
-	 */
-	if (!(iclog->ic_state &
-			(XLOG_STATE_DONE_SYNC | XLOG_STATE_DO_CALLBACK))) {
+	case XLOG_STATE_DONE_SYNC:
+	case XLOG_STATE_DO_CALLBACK:
+		/*
+		 * Now that we have an iclog that is in either the DO_CALLBACK
+		 * or DONE_SYNC states, do one more check here to see if we have
+		 * chased our tail around.  If this is not the lowest lsn iclog,
+		 * then we will leave it for another completion to process.
+		 */
+		header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
+		lowest_lsn = xlog_get_lowest_lsn(log);
+		if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
+			return false;
+		xlog_state_set_callback(log, iclog, header_lsn);
+		return false;
+	default:
+		/*
+		 * Can only perform callbacks in order.  Since this iclog is not
+		 * in the DONE_SYNC or DO_CALLBACK states, we skip the rest and
+		 * just try to clean up.  If we set our iclog to DO_CALLBACK, we
+		 * will not process it when we retry since a previous iclog is
+		 * in the CALLBACK and the state cannot change since we are
+		 * holding l_icloglock.
+		 */
 		if (completed_iclog &&
-		    (completed_iclog->ic_state == XLOG_STATE_DONE_SYNC)) {
+		    (completed_iclog->ic_state == XLOG_STATE_DONE_SYNC))
 			completed_iclog->ic_state = XLOG_STATE_DO_CALLBACK;
-		}
 		return true;
 	}
-
-	/*
-	 * We now have an iclog that is in either the DO_CALLBACK or DONE_SYNC
-	 * states. The other states (WANT_SYNC, SYNCING, or CALLBACK were caught
-	 * by the above if and are going to clean (i.e. we aren't doing their
-	 * callbacks) see the above if.
-	 *
-	 * We will do one more check here to see if we have chased our tail
-	 * around. If this is not the lowest lsn iclog, then we will leave it
-	 * for another completion to process.
-	 */
-	header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
-	lowest_lsn = xlog_get_lowest_lsn(log);
-	if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
-		return false;
-
-	xlog_state_set_callback(log, iclog, header_lsn);
-	return false;
-
 }
 
 /*
@@ -2850,9 +2846,6 @@ xlog_state_do_iclog_callbacks(
  * are deferred to the completion of the earlier iclog. Walk the iclogs in order
  * and make sure that no iclog is in DO_CALLBACK unless an earlier iclog is in
  * one of the syncing states.
- *
- * Note that SYNCING|IOERROR is a valid state so we cannot just check for
- * ic_state == SYNCING.
  */
 static void
 xlog_state_callback_check_state(
@@ -2873,7 +2866,7 @@ xlog_state_callback_check_state(
 		 * IOERROR - give up hope all ye who enter here
 		 */
 		if (iclog->ic_state == XLOG_STATE_WANT_SYNC ||
-		    iclog->ic_state & XLOG_STATE_SYNCING ||
+		    iclog->ic_state == XLOG_STATE_SYNCING ||
 		    iclog->ic_state == XLOG_STATE_DONE_SYNC ||
 		    iclog->ic_state == XLOG_STATE_IOERROR )
 			break;
@@ -2919,8 +2912,8 @@ xlog_state_do_callback(
 							ciclog, &ioerror))
 				break;
 
-			if (!(iclog->ic_state &
-			      (XLOG_STATE_CALLBACK | XLOG_STATE_IOERROR))) {
+			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
+			    iclog->ic_state != XLOG_STATE_IOERROR) {
 				iclog = iclog->ic_next;
 				continue;
 			}
@@ -2950,7 +2943,8 @@ xlog_state_do_callback(
 	if (did_callbacks)
 		xlog_state_callback_check_state(log);
 
-	if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE|XLOG_STATE_IOERROR))
+	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
+	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
 		wake_up_all(&log->l_flush_wait);
 
 	spin_unlock(&log->l_icloglock);
@@ -2979,8 +2973,6 @@ xlog_state_done_syncing(
 
 	spin_lock(&log->l_icloglock);
 
-	ASSERT(iclog->ic_state == XLOG_STATE_SYNCING ||
-	       iclog->ic_state == XLOG_STATE_IOERROR);
 	ASSERT(atomic_read(&iclog->ic_refcnt) == 0);
 
 	/*
@@ -2989,8 +2981,10 @@ xlog_state_done_syncing(
 	 * and none should ever be attempted to be written to disk
 	 * again.
 	 */
-	if (iclog->ic_state != XLOG_STATE_IOERROR)
+	if (iclog->ic_state == XLOG_STATE_SYNCING)
 		iclog->ic_state = XLOG_STATE_DONE_SYNC;
+	else
+		ASSERT(iclog->ic_state == XLOG_STATE_IOERROR);
 
 	/*
 	 * Someone could be sleeping prior to writing out the next
@@ -3300,7 +3294,7 @@ xfs_log_force(
 
 	spin_lock(&log->l_icloglock);
 	iclog = log->l_iclog;
-	if (iclog->ic_state & XLOG_STATE_IOERROR)
+	if (iclog->ic_state == XLOG_STATE_IOERROR)
 		goto out_error;
 
 	if (iclog->ic_state == XLOG_STATE_DIRTY ||
@@ -3357,11 +3351,11 @@ xfs_log_force(
 	if (!(flags & XFS_LOG_SYNC))
 		goto out_unlock;
 
-	if (iclog->ic_state & XLOG_STATE_IOERROR)
+	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)
+	if (iclog->ic_state == XLOG_STATE_IOERROR)
 		return -EIO;
 	return 0;
 
@@ -3386,7 +3380,7 @@ __xfs_log_force_lsn(
 
 	spin_lock(&log->l_icloglock);
 	iclog = log->l_iclog;
-	if (iclog->ic_state & XLOG_STATE_IOERROR)
+	if (iclog->ic_state == XLOG_STATE_IOERROR)
 		goto out_error;
 
 	while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
@@ -3415,10 +3409,8 @@ __xfs_log_force_lsn(
 		 * 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));
-
+		    (iclog->ic_prev->ic_state == XLOG_STATE_WANT_SYNC ||
+		     iclog->ic_prev->ic_state == XLOG_STATE_SYNCING)) {
 			XFS_STATS_INC(mp, xs_log_force_sleep);
 
 			xlog_wait(&iclog->ic_prev->ic_write_wait,
@@ -3434,15 +3426,16 @@ __xfs_log_force_lsn(
 	}
 
 	if (!(flags & XFS_LOG_SYNC) ||
-	    (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY)))
+	    (iclog->ic_state == XLOG_STATE_ACTIVE ||
+	     iclog->ic_state == XLOG_STATE_DIRTY))
 		goto out_unlock;
 
-	if (iclog->ic_state & XLOG_STATE_IOERROR)
+	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)
+	if (iclog->ic_state == XLOG_STATE_IOERROR)
 		return -EIO;
 	return 0;
 
@@ -3505,8 +3498,8 @@ xlog_state_want_sync(
 	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
 		xlog_state_switch_iclogs(log, iclog, 0);
 	} else {
-		ASSERT(iclog->ic_state &
-			(XLOG_STATE_WANT_SYNC|XLOG_STATE_IOERROR));
+		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
+		       iclog->ic_state == XLOG_STATE_IOERROR);
 	}
 }
 
@@ -3883,7 +3876,7 @@ xlog_state_ioerror(
 	xlog_in_core_t	*iclog, *ic;
 
 	iclog = log->l_iclog;
-	if (! (iclog->ic_state & XLOG_STATE_IOERROR)) {
+	if (iclog->ic_state != XLOG_STATE_IOERROR) {
 		/*
 		 * Mark all the incore logs IOERROR.
 		 * From now on, no log flushes will result.
@@ -3943,7 +3936,7 @@ xfs_log_force_umount(
 	 * Somebody could've already done the hard work for us.
 	 * No need to get locks for this.
 	 */
-	if (logerror && log->l_iclog->ic_state & XLOG_STATE_IOERROR) {
+	if (logerror && log->l_iclog->ic_state == XLOG_STATE_IOERROR) {
 		ASSERT(XLOG_FORCED_SHUTDOWN(log));
 		return 1;
 	}
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index ef652abd112c..a1204424a938 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -847,7 +847,7 @@ xlog_cil_push(
 		goto out_abort;
 
 	spin_lock(&commit_iclog->ic_callback_lock);
-	if (commit_iclog->ic_state & XLOG_STATE_IOERROR) {
+	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
 		spin_unlock(&commit_iclog->ic_callback_lock);
 		goto out_abort;
 	}
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 66bd370ae60a..bf076893f516 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -40,15 +40,16 @@ static inline uint xlog_get_client_id(__be32 i)
 /*
  * In core log state
  */
-#define XLOG_STATE_ACTIVE    0x0001 /* Current IC log being written to */
-#define XLOG_STATE_WANT_SYNC 0x0002 /* Want to sync this iclog; no more writes */
-#define XLOG_STATE_SYNCING   0x0004 /* This IC log is syncing */
-#define XLOG_STATE_DONE_SYNC 0x0008 /* Done syncing to disk */
-#define XLOG_STATE_DO_CALLBACK \
-			     0x0010 /* Process callback functions */
-#define XLOG_STATE_CALLBACK  0x0020 /* Callback functions now */
-#define XLOG_STATE_DIRTY     0x0040 /* Dirty IC log, not ready for ACTIVE status*/
-#define XLOG_STATE_IOERROR   0x0080 /* IO error happened in sync'ing log */
+enum xlog_iclog_state {
+	XLOG_STATE_ACTIVE,	/* Current IC log being written to */
+	XLOG_STATE_WANT_SYNC,	/* Want to sync this iclog; no more writes */
+	XLOG_STATE_SYNCING,	/* This IC log is syncing */
+	XLOG_STATE_DONE_SYNC,	/* Done syncing to disk */
+	XLOG_STATE_DO_CALLBACK,	/* Process callback functions */
+	XLOG_STATE_CALLBACK,	/* Callback functions now */
+	XLOG_STATE_DIRTY,	/* Dirty IC log, not ready for ACTIVE status */
+	XLOG_STATE_IOERROR,	/* IO error happened in sync'ing log */
+};
 
 /*
  * Flags to log ticket
@@ -202,7 +203,7 @@ typedef struct xlog_in_core {
 	struct xlog		*ic_log;
 	u32			ic_size;
 	u32			ic_offset;
-	unsigned short		ic_state;
+	enum xlog_iclog_state	ic_state;
 	char			*ic_datap;	/* pointer to iclog data */
 
 	/* Callback structures need their own cacheline */
-- 
2.20.1

Re: [PATCH 7/8] xfs: turn ic_state into an enum

[Darrick J. Wong]

On Wed, Oct 09, 2019 at 04:27:47PM +0200, Christoph Hellwig wrote:
> ic_state really is a set of different states, even if the values are
> encoded as non-conflicting bits and we sometimes use logical and
> operations to check for them.  Switch all comparisms to check for
> exact values (and use switch statements in a few places to make it
> more clear) and turn the values into an implicitly enumerated enum
> type.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log.c      | 159 ++++++++++++++++++++----------------------
>  fs/xfs/xfs_log_cil.c  |   2 +-
>  fs/xfs/xfs_log_priv.h |  21 +++---
>  3 files changed, 88 insertions(+), 94 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 7a429e5dc27c..001a9586cb56 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -584,7 +584,7 @@ xlog_state_release_iclog(
>  {
>  	lockdep_assert_held(&log->l_icloglock);
>  
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		return -EIO;
>  
>  	if (atomic_dec_and_test(&iclog->ic_refcnt) &&
> @@ -605,7 +605,7 @@ xfs_log_release_iclog(
>  	struct xlog		*log = mp->m_log;
>  	bool			sync;
>  
> -	if (iclog->ic_state & XLOG_STATE_IOERROR) {
> +	if (iclog->ic_state == XLOG_STATE_IOERROR) {
>  		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
>  		return -EIO;
>  	}
> @@ -975,8 +975,8 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  #ifdef DEBUG
>  	first_iclog = iclog = log->l_iclog;
>  	do {
> -		if (!(iclog->ic_state & XLOG_STATE_IOERROR)) {
> -			ASSERT(iclog->ic_state & XLOG_STATE_ACTIVE);
> +		if (iclog->ic_state != XLOG_STATE_IOERROR) {
> +			ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
>  			ASSERT(iclog->ic_offset == 0);
>  		}
>  		iclog = iclog->ic_next;
> @@ -1003,15 +1003,15 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		atomic_inc(&iclog->ic_refcnt);
>  		xlog_state_want_sync(log, iclog);
>  		error =  xlog_state_release_iclog(log, iclog);
> -
> -		if ( ! (   iclog->ic_state == XLOG_STATE_ACTIVE
> -			|| iclog->ic_state == XLOG_STATE_DIRTY
> -			|| iclog->ic_state == XLOG_STATE_IOERROR) ) {
> -
> -				xlog_wait(&iclog->ic_force_wait,
> -							&log->l_icloglock);
> -		} else {
> +		switch (iclog->ic_state) {
> +		case XLOG_STATE_ACTIVE:
> +		case XLOG_STATE_DIRTY:
> +		case XLOG_STATE_IOERROR:
>  			spin_unlock(&log->l_icloglock);
> +			break;
> +		default:
> +			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> +			break;
>  		}
>  	}
>  
> @@ -1301,7 +1301,7 @@ xlog_ioend_work(
>  		 * didn't succeed.
>  		 */
>  		aborted = true;
> -	} else if (iclog->ic_state & XLOG_STATE_IOERROR) {
> +	} else if (iclog->ic_state == XLOG_STATE_IOERROR) {
>  		aborted = true;
>  	}
>  
> @@ -1774,7 +1774,7 @@ xlog_write_iclog(
>  	 * across the log IO to archieve that.
>  	 */
>  	down(&iclog->ic_sema);
> -	if (unlikely(iclog->ic_state & XLOG_STATE_IOERROR)) {
> +	if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) {
>  		/*
>  		 * It would seem logical to return EIO here, but we rely on
>  		 * the log state machine to propagate I/O errors instead of
> @@ -2598,7 +2598,7 @@ xlog_state_clean_iclog(
>  	int			changed = 0;
>  
>  	/* Prepare the completed iclog. */
> -	if (!(dirty_iclog->ic_state & XLOG_STATE_IOERROR))
> +	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
>  		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>  
>  	/* Walk all the iclogs to update the ordered active state. */
> @@ -2689,7 +2689,8 @@ xlog_get_lowest_lsn(
>  	xfs_lsn_t		lowest_lsn = 0, lsn;
>  
>  	do {
> -		if (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY))
> +		if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> +		    iclog->ic_state == XLOG_STATE_DIRTY)
>  			continue;
>  
>  		lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> @@ -2755,55 +2756,50 @@ xlog_state_iodone_process_iclog(
>  	xfs_lsn_t		lowest_lsn;
>  	xfs_lsn_t		header_lsn;
>  
> -	/* Skip all iclogs in the ACTIVE & DIRTY states */
> -	if (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY))
> +	switch (iclog->ic_state) {
> +	case XLOG_STATE_ACTIVE:
> +	case XLOG_STATE_DIRTY:
> +		/*
> +		 * Skip all iclogs in the ACTIVE & DIRTY states:
> +		 */
>  		return false;
> -
> -	/*
> -	 * Between marking a filesystem SHUTDOWN and stopping the log, we do
> -	 * flush all iclogs to disk (if there wasn't a log I/O error). So, we do
> -	 * want things to go smoothly in case of just a SHUTDOWN  w/o a
> -	 * LOG_IO_ERROR.
> -	 */
> -	if (iclog->ic_state & XLOG_STATE_IOERROR) {
> +	case XLOG_STATE_IOERROR:
> +		/*
> +		 * Between marking a filesystem SHUTDOWN and stopping the log,
> +		 * we do flush all iclogs to disk (if there wasn't a log I/O
> +		 * error). So, we do want things to go smoothly in case of just
> +		 * a SHUTDOWN  w/o a LOG_IO_ERROR.
> +		 */
>  		*ioerror = true;
>  		return false;
> -	}
> -
> -	/*
> -	 * Can only perform callbacks in order.  Since this iclog is not in the
> -	 * DONE_SYNC/ DO_CALLBACK state, we skip the rest and just try to clean
> -	 * up.  If we set our iclog to DO_CALLBACK, we will not process it when
> -	 * we retry since a previous iclog is in the CALLBACK and the state
> -	 * cannot change since we are holding the l_icloglock.
> -	 */
> -	if (!(iclog->ic_state &
> -			(XLOG_STATE_DONE_SYNC | XLOG_STATE_DO_CALLBACK))) {
> +	case XLOG_STATE_DONE_SYNC:
> +	case XLOG_STATE_DO_CALLBACK:
> +		/*
> +		 * Now that we have an iclog that is in either the DO_CALLBACK
> +		 * or DONE_SYNC states, do one more check here to see if we have
> +		 * chased our tail around.  If this is not the lowest lsn iclog,
> +		 * then we will leave it for another completion to process.
> +		 */
> +		header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> +		lowest_lsn = xlog_get_lowest_lsn(log);
> +		if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
> +			return false;
> +		xlog_state_set_callback(log, iclog, header_lsn);
> +		return false;
> +	default:
> +		/*
> +		 * Can only perform callbacks in order.  Since this iclog is not
> +		 * in the DONE_SYNC or DO_CALLBACK states, we skip the rest and
> +		 * just try to clean up.  If we set our iclog to DO_CALLBACK, we
> +		 * will not process it when we retry since a previous iclog is
> +		 * in the CALLBACK and the state cannot change since we are
> +		 * holding l_icloglock.
> +		 */
>  		if (completed_iclog &&
> -		    (completed_iclog->ic_state == XLOG_STATE_DONE_SYNC)) {
> +		    (completed_iclog->ic_state == XLOG_STATE_DONE_SYNC))
>  			completed_iclog->ic_state = XLOG_STATE_DO_CALLBACK;
> -		}
>  		return true;
>  	}
> -
> -	/*
> -	 * We now have an iclog that is in either the DO_CALLBACK or DONE_SYNC
> -	 * states. The other states (WANT_SYNC, SYNCING, or CALLBACK were caught
> -	 * by the above if and are going to clean (i.e. we aren't doing their
> -	 * callbacks) see the above if.
> -	 *
> -	 * We will do one more check here to see if we have chased our tail
> -	 * around. If this is not the lowest lsn iclog, then we will leave it
> -	 * for another completion to process.
> -	 */
> -	header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> -	lowest_lsn = xlog_get_lowest_lsn(log);
> -	if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
> -		return false;
> -
> -	xlog_state_set_callback(log, iclog, header_lsn);
> -	return false;
> -
>  }
>  
>  /*
> @@ -2850,9 +2846,6 @@ xlog_state_do_iclog_callbacks(
>   * are deferred to the completion of the earlier iclog. Walk the iclogs in order
>   * and make sure that no iclog is in DO_CALLBACK unless an earlier iclog is in
>   * one of the syncing states.
> - *
> - * Note that SYNCING|IOERROR is a valid state so we cannot just check for
> - * ic_state == SYNCING.
>   */
>  static void
>  xlog_state_callback_check_state(
> @@ -2873,7 +2866,7 @@ xlog_state_callback_check_state(
>  		 * IOERROR - give up hope all ye who enter here
>  		 */
>  		if (iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -		    iclog->ic_state & XLOG_STATE_SYNCING ||
> +		    iclog->ic_state == XLOG_STATE_SYNCING ||
>  		    iclog->ic_state == XLOG_STATE_DONE_SYNC ||
>  		    iclog->ic_state == XLOG_STATE_IOERROR )
>  			break;
> @@ -2919,8 +2912,8 @@ xlog_state_do_callback(
>  							ciclog, &ioerror))
>  				break;
>  
> -			if (!(iclog->ic_state &
> -			      (XLOG_STATE_CALLBACK | XLOG_STATE_IOERROR))) {
> +			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
> +			    iclog->ic_state != XLOG_STATE_IOERROR) {
>  				iclog = iclog->ic_next;
>  				continue;
>  			}
> @@ -2950,7 +2943,8 @@ xlog_state_do_callback(
>  	if (did_callbacks)
>  		xlog_state_callback_check_state(log);
>  
> -	if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE|XLOG_STATE_IOERROR))
> +	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
> +	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
>  		wake_up_all(&log->l_flush_wait);
>  
>  	spin_unlock(&log->l_icloglock);
> @@ -2979,8 +2973,6 @@ xlog_state_done_syncing(
>  
>  	spin_lock(&log->l_icloglock);
>  
> -	ASSERT(iclog->ic_state == XLOG_STATE_SYNCING ||
> -	       iclog->ic_state == XLOG_STATE_IOERROR);
>  	ASSERT(atomic_read(&iclog->ic_refcnt) == 0);
>  
>  	/*
> @@ -2989,8 +2981,10 @@ xlog_state_done_syncing(
>  	 * and none should ever be attempted to be written to disk
>  	 * again.
>  	 */
> -	if (iclog->ic_state != XLOG_STATE_IOERROR)
> +	if (iclog->ic_state == XLOG_STATE_SYNCING)
>  		iclog->ic_state = XLOG_STATE_DONE_SYNC;
> +	else
> +		ASSERT(iclog->ic_state == XLOG_STATE_IOERROR);
>  
>  	/*
>  	 * Someone could be sleeping prior to writing out the next
> @@ -3300,7 +3294,7 @@ xfs_log_force(
>  
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		goto out_error;
>  
>  	if (iclog->ic_state == XLOG_STATE_DIRTY ||
> @@ -3357,11 +3351,11 @@ xfs_log_force(
>  	if (!(flags & XFS_LOG_SYNC))
>  		goto out_unlock;
>  
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	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)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		return -EIO;
>  	return 0;
>  
> @@ -3386,7 +3380,7 @@ __xfs_log_force_lsn(
>  
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		goto out_error;
>  
>  	while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
> @@ -3415,10 +3409,8 @@ __xfs_log_force_lsn(
>  		 * 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));
> -
> +		    (iclog->ic_prev->ic_state == XLOG_STATE_WANT_SYNC ||
> +		     iclog->ic_prev->ic_state == XLOG_STATE_SYNCING)) {
>  			XFS_STATS_INC(mp, xs_log_force_sleep);
>  
>  			xlog_wait(&iclog->ic_prev->ic_write_wait,
> @@ -3434,15 +3426,16 @@ __xfs_log_force_lsn(
>  	}
>  
>  	if (!(flags & XFS_LOG_SYNC) ||
> -	    (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY)))
> +	    (iclog->ic_state == XLOG_STATE_ACTIVE ||
> +	     iclog->ic_state == XLOG_STATE_DIRTY))
>  		goto out_unlock;
>  
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	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)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		return -EIO;
>  	return 0;
>  
> @@ -3505,8 +3498,8 @@ xlog_state_want_sync(
>  	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
>  		xlog_state_switch_iclogs(log, iclog, 0);
>  	} else {
> -		ASSERT(iclog->ic_state &
> -			(XLOG_STATE_WANT_SYNC|XLOG_STATE_IOERROR));
> +		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> +		       iclog->ic_state == XLOG_STATE_IOERROR);
>  	}
>  }
>  
> @@ -3883,7 +3876,7 @@ xlog_state_ioerror(
>  	xlog_in_core_t	*iclog, *ic;
>  
>  	iclog = log->l_iclog;
> -	if (! (iclog->ic_state & XLOG_STATE_IOERROR)) {
> +	if (iclog->ic_state != XLOG_STATE_IOERROR) {
>  		/*
>  		 * Mark all the incore logs IOERROR.
>  		 * From now on, no log flushes will result.
> @@ -3943,7 +3936,7 @@ xfs_log_force_umount(
>  	 * Somebody could've already done the hard work for us.
>  	 * No need to get locks for this.
>  	 */
> -	if (logerror && log->l_iclog->ic_state & XLOG_STATE_IOERROR) {
> +	if (logerror && log->l_iclog->ic_state == XLOG_STATE_IOERROR) {
>  		ASSERT(XLOG_FORCED_SHUTDOWN(log));
>  		return 1;
>  	}
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index ef652abd112c..a1204424a938 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -847,7 +847,7 @@ xlog_cil_push(
>  		goto out_abort;
>  
>  	spin_lock(&commit_iclog->ic_callback_lock);
> -	if (commit_iclog->ic_state & XLOG_STATE_IOERROR) {
> +	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
>  		spin_unlock(&commit_iclog->ic_callback_lock);
>  		goto out_abort;
>  	}
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 66bd370ae60a..bf076893f516 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -40,15 +40,16 @@ static inline uint xlog_get_client_id(__be32 i)
>  /*
>   * In core log state
>   */
> -#define XLOG_STATE_ACTIVE    0x0001 /* Current IC log being written to */
> -#define XLOG_STATE_WANT_SYNC 0x0002 /* Want to sync this iclog; no more writes */
> -#define XLOG_STATE_SYNCING   0x0004 /* This IC log is syncing */
> -#define XLOG_STATE_DONE_SYNC 0x0008 /* Done syncing to disk */
> -#define XLOG_STATE_DO_CALLBACK \
> -			     0x0010 /* Process callback functions */
> -#define XLOG_STATE_CALLBACK  0x0020 /* Callback functions now */
> -#define XLOG_STATE_DIRTY     0x0040 /* Dirty IC log, not ready for ACTIVE status*/
> -#define XLOG_STATE_IOERROR   0x0080 /* IO error happened in sync'ing log */
> +enum xlog_iclog_state {
> +	XLOG_STATE_ACTIVE,	/* Current IC log being written to */
> +	XLOG_STATE_WANT_SYNC,	/* Want to sync this iclog; no more writes */
> +	XLOG_STATE_SYNCING,	/* This IC log is syncing */
> +	XLOG_STATE_DONE_SYNC,	/* Done syncing to disk */
> +	XLOG_STATE_DO_CALLBACK,	/* Process callback functions */
> +	XLOG_STATE_CALLBACK,	/* Callback functions now */
> +	XLOG_STATE_DIRTY,	/* Dirty IC log, not ready for ACTIVE status */
> +	XLOG_STATE_IOERROR,	/* IO error happened in sync'ing log */
> +};
>  
>  /*
>   * Flags to log ticket
> @@ -202,7 +203,7 @@ typedef struct xlog_in_core {
>  	struct xlog		*ic_log;
>  	u32			ic_size;
>  	u32			ic_offset;
> -	unsigned short		ic_state;
> +	enum xlog_iclog_state	ic_state;
>  	char			*ic_datap;	/* pointer to iclog data */
>  
>  	/* Callback structures need their own cacheline */
> -- 
> 2.20.1
> 

Re: [PATCH 7/8] xfs: turn ic_state into an enum

[Brian Foster]

On Wed, Oct 09, 2019 at 04:27:47PM +0200, Christoph Hellwig wrote:
> ic_state really is a set of different states, even if the values are
> encoded as non-conflicting bits and we sometimes use logical and
> operations to check for them.  Switch all comparisms to check for
> exact values (and use switch statements in a few places to make it
> more clear) and turn the values into an implicitly enumerated enum
> type.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c      | 159 ++++++++++++++++++++----------------------
>  fs/xfs/xfs_log_cil.c  |   2 +-
>  fs/xfs/xfs_log_priv.h |  21 +++---
>  3 files changed, 88 insertions(+), 94 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 7a429e5dc27c..001a9586cb56 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -584,7 +584,7 @@ xlog_state_release_iclog(
>  {
>  	lockdep_assert_held(&log->l_icloglock);
>  
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		return -EIO;
>  
>  	if (atomic_dec_and_test(&iclog->ic_refcnt) &&
> @@ -605,7 +605,7 @@ xfs_log_release_iclog(
>  	struct xlog		*log = mp->m_log;
>  	bool			sync;
>  
> -	if (iclog->ic_state & XLOG_STATE_IOERROR) {
> +	if (iclog->ic_state == XLOG_STATE_IOERROR) {
>  		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
>  		return -EIO;
>  	}
> @@ -975,8 +975,8 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  #ifdef DEBUG
>  	first_iclog = iclog = log->l_iclog;
>  	do {
> -		if (!(iclog->ic_state & XLOG_STATE_IOERROR)) {
> -			ASSERT(iclog->ic_state & XLOG_STATE_ACTIVE);
> +		if (iclog->ic_state != XLOG_STATE_IOERROR) {
> +			ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
>  			ASSERT(iclog->ic_offset == 0);
>  		}
>  		iclog = iclog->ic_next;
> @@ -1003,15 +1003,15 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		atomic_inc(&iclog->ic_refcnt);
>  		xlog_state_want_sync(log, iclog);
>  		error =  xlog_state_release_iclog(log, iclog);
> -
> -		if ( ! (   iclog->ic_state == XLOG_STATE_ACTIVE
> -			|| iclog->ic_state == XLOG_STATE_DIRTY
> -			|| iclog->ic_state == XLOG_STATE_IOERROR) ) {
> -
> -				xlog_wait(&iclog->ic_force_wait,
> -							&log->l_icloglock);
> -		} else {
> +		switch (iclog->ic_state) {
> +		case XLOG_STATE_ACTIVE:
> +		case XLOG_STATE_DIRTY:
> +		case XLOG_STATE_IOERROR:
>  			spin_unlock(&log->l_icloglock);
> +			break;
> +		default:
> +			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> +			break;
>  		}
>  	}
>  
> @@ -1301,7 +1301,7 @@ xlog_ioend_work(
>  		 * didn't succeed.
>  		 */
>  		aborted = true;
> -	} else if (iclog->ic_state & XLOG_STATE_IOERROR) {
> +	} else if (iclog->ic_state == XLOG_STATE_IOERROR) {
>  		aborted = true;
>  	}
>  
> @@ -1774,7 +1774,7 @@ xlog_write_iclog(
>  	 * across the log IO to archieve that.
>  	 */
>  	down(&iclog->ic_sema);
> -	if (unlikely(iclog->ic_state & XLOG_STATE_IOERROR)) {
> +	if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) {
>  		/*
>  		 * It would seem logical to return EIO here, but we rely on
>  		 * the log state machine to propagate I/O errors instead of
> @@ -2598,7 +2598,7 @@ xlog_state_clean_iclog(
>  	int			changed = 0;
>  
>  	/* Prepare the completed iclog. */
> -	if (!(dirty_iclog->ic_state & XLOG_STATE_IOERROR))
> +	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
>  		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>  
>  	/* Walk all the iclogs to update the ordered active state. */
> @@ -2689,7 +2689,8 @@ xlog_get_lowest_lsn(
>  	xfs_lsn_t		lowest_lsn = 0, lsn;
>  
>  	do {
> -		if (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY))
> +		if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> +		    iclog->ic_state == XLOG_STATE_DIRTY)
>  			continue;
>  
>  		lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> @@ -2755,55 +2756,50 @@ xlog_state_iodone_process_iclog(
>  	xfs_lsn_t		lowest_lsn;
>  	xfs_lsn_t		header_lsn;
>  
> -	/* Skip all iclogs in the ACTIVE & DIRTY states */
> -	if (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY))
> +	switch (iclog->ic_state) {
> +	case XLOG_STATE_ACTIVE:
> +	case XLOG_STATE_DIRTY:
> +		/*
> +		 * Skip all iclogs in the ACTIVE & DIRTY states:
> +		 */
>  		return false;
> -
> -	/*
> -	 * Between marking a filesystem SHUTDOWN and stopping the log, we do
> -	 * flush all iclogs to disk (if there wasn't a log I/O error). So, we do
> -	 * want things to go smoothly in case of just a SHUTDOWN  w/o a
> -	 * LOG_IO_ERROR.
> -	 */
> -	if (iclog->ic_state & XLOG_STATE_IOERROR) {
> +	case XLOG_STATE_IOERROR:
> +		/*
> +		 * Between marking a filesystem SHUTDOWN and stopping the log,
> +		 * we do flush all iclogs to disk (if there wasn't a log I/O
> +		 * error). So, we do want things to go smoothly in case of just
> +		 * a SHUTDOWN  w/o a LOG_IO_ERROR.
> +		 */
>  		*ioerror = true;
>  		return false;
> -	}
> -
> -	/*
> -	 * Can only perform callbacks in order.  Since this iclog is not in the
> -	 * DONE_SYNC/ DO_CALLBACK state, we skip the rest and just try to clean
> -	 * up.  If we set our iclog to DO_CALLBACK, we will not process it when
> -	 * we retry since a previous iclog is in the CALLBACK and the state
> -	 * cannot change since we are holding the l_icloglock.
> -	 */
> -	if (!(iclog->ic_state &
> -			(XLOG_STATE_DONE_SYNC | XLOG_STATE_DO_CALLBACK))) {
> +	case XLOG_STATE_DONE_SYNC:
> +	case XLOG_STATE_DO_CALLBACK:
> +		/*
> +		 * Now that we have an iclog that is in either the DO_CALLBACK
> +		 * or DONE_SYNC states, do one more check here to see if we have
> +		 * chased our tail around.  If this is not the lowest lsn iclog,
> +		 * then we will leave it for another completion to process.
> +		 */
> +		header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> +		lowest_lsn = xlog_get_lowest_lsn(log);
> +		if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
> +			return false;
> +		xlog_state_set_callback(log, iclog, header_lsn);
> +		return false;
> +	default:
> +		/*
> +		 * Can only perform callbacks in order.  Since this iclog is not
> +		 * in the DONE_SYNC or DO_CALLBACK states, we skip the rest and
> +		 * just try to clean up.  If we set our iclog to DO_CALLBACK, we
> +		 * will not process it when we retry since a previous iclog is
> +		 * in the CALLBACK and the state cannot change since we are
> +		 * holding l_icloglock.
> +		 */
>  		if (completed_iclog &&
> -		    (completed_iclog->ic_state == XLOG_STATE_DONE_SYNC)) {
> +		    (completed_iclog->ic_state == XLOG_STATE_DONE_SYNC))
>  			completed_iclog->ic_state = XLOG_STATE_DO_CALLBACK;
> -		}
>  		return true;
>  	}
> -
> -	/*
> -	 * We now have an iclog that is in either the DO_CALLBACK or DONE_SYNC
> -	 * states. The other states (WANT_SYNC, SYNCING, or CALLBACK were caught
> -	 * by the above if and are going to clean (i.e. we aren't doing their
> -	 * callbacks) see the above if.
> -	 *
> -	 * We will do one more check here to see if we have chased our tail
> -	 * around. If this is not the lowest lsn iclog, then we will leave it
> -	 * for another completion to process.
> -	 */
> -	header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> -	lowest_lsn = xlog_get_lowest_lsn(log);
> -	if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
> -		return false;
> -
> -	xlog_state_set_callback(log, iclog, header_lsn);
> -	return false;
> -
>  }
>  
>  /*
> @@ -2850,9 +2846,6 @@ xlog_state_do_iclog_callbacks(
>   * are deferred to the completion of the earlier iclog. Walk the iclogs in order
>   * and make sure that no iclog is in DO_CALLBACK unless an earlier iclog is in
>   * one of the syncing states.
> - *
> - * Note that SYNCING|IOERROR is a valid state so we cannot just check for
> - * ic_state == SYNCING.
>   */
>  static void
>  xlog_state_callback_check_state(
> @@ -2873,7 +2866,7 @@ xlog_state_callback_check_state(
>  		 * IOERROR - give up hope all ye who enter here
>  		 */
>  		if (iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -		    iclog->ic_state & XLOG_STATE_SYNCING ||
> +		    iclog->ic_state == XLOG_STATE_SYNCING ||
>  		    iclog->ic_state == XLOG_STATE_DONE_SYNC ||
>  		    iclog->ic_state == XLOG_STATE_IOERROR )
>  			break;
> @@ -2919,8 +2912,8 @@ xlog_state_do_callback(
>  							ciclog, &ioerror))
>  				break;
>  
> -			if (!(iclog->ic_state &
> -			      (XLOG_STATE_CALLBACK | XLOG_STATE_IOERROR))) {
> +			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
> +			    iclog->ic_state != XLOG_STATE_IOERROR) {
>  				iclog = iclog->ic_next;
>  				continue;
>  			}
> @@ -2950,7 +2943,8 @@ xlog_state_do_callback(
>  	if (did_callbacks)
>  		xlog_state_callback_check_state(log);
>  
> -	if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE|XLOG_STATE_IOERROR))
> +	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
> +	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
>  		wake_up_all(&log->l_flush_wait);
>  
>  	spin_unlock(&log->l_icloglock);
> @@ -2979,8 +2973,6 @@ xlog_state_done_syncing(
>  
>  	spin_lock(&log->l_icloglock);
>  
> -	ASSERT(iclog->ic_state == XLOG_STATE_SYNCING ||
> -	       iclog->ic_state == XLOG_STATE_IOERROR);
>  	ASSERT(atomic_read(&iclog->ic_refcnt) == 0);
>  
>  	/*
> @@ -2989,8 +2981,10 @@ xlog_state_done_syncing(
>  	 * and none should ever be attempted to be written to disk
>  	 * again.
>  	 */
> -	if (iclog->ic_state != XLOG_STATE_IOERROR)
> +	if (iclog->ic_state == XLOG_STATE_SYNCING)
>  		iclog->ic_state = XLOG_STATE_DONE_SYNC;
> +	else
> +		ASSERT(iclog->ic_state == XLOG_STATE_IOERROR);
>  
>  	/*
>  	 * Someone could be sleeping prior to writing out the next
> @@ -3300,7 +3294,7 @@ xfs_log_force(
>  
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		goto out_error;
>  
>  	if (iclog->ic_state == XLOG_STATE_DIRTY ||
> @@ -3357,11 +3351,11 @@ xfs_log_force(
>  	if (!(flags & XFS_LOG_SYNC))
>  		goto out_unlock;
>  
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	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)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		return -EIO;
>  	return 0;
>  
> @@ -3386,7 +3380,7 @@ __xfs_log_force_lsn(
>  
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		goto out_error;
>  
>  	while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
> @@ -3415,10 +3409,8 @@ __xfs_log_force_lsn(
>  		 * 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));
> -
> +		    (iclog->ic_prev->ic_state == XLOG_STATE_WANT_SYNC ||
> +		     iclog->ic_prev->ic_state == XLOG_STATE_SYNCING)) {
>  			XFS_STATS_INC(mp, xs_log_force_sleep);
>  
>  			xlog_wait(&iclog->ic_prev->ic_write_wait,
> @@ -3434,15 +3426,16 @@ __xfs_log_force_lsn(
>  	}
>  
>  	if (!(flags & XFS_LOG_SYNC) ||
> -	    (iclog->ic_state & (XLOG_STATE_ACTIVE | XLOG_STATE_DIRTY)))
> +	    (iclog->ic_state == XLOG_STATE_ACTIVE ||
> +	     iclog->ic_state == XLOG_STATE_DIRTY))
>  		goto out_unlock;
>  
> -	if (iclog->ic_state & XLOG_STATE_IOERROR)
> +	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)
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
>  		return -EIO;
>  	return 0;
>  
> @@ -3505,8 +3498,8 @@ xlog_state_want_sync(
>  	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
>  		xlog_state_switch_iclogs(log, iclog, 0);
>  	} else {
> -		ASSERT(iclog->ic_state &
> -			(XLOG_STATE_WANT_SYNC|XLOG_STATE_IOERROR));
> +		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> +		       iclog->ic_state == XLOG_STATE_IOERROR);
>  	}
>  }
>  
> @@ -3883,7 +3876,7 @@ xlog_state_ioerror(
>  	xlog_in_core_t	*iclog, *ic;
>  
>  	iclog = log->l_iclog;
> -	if (! (iclog->ic_state & XLOG_STATE_IOERROR)) {
> +	if (iclog->ic_state != XLOG_STATE_IOERROR) {
>  		/*
>  		 * Mark all the incore logs IOERROR.
>  		 * From now on, no log flushes will result.
> @@ -3943,7 +3936,7 @@ xfs_log_force_umount(
>  	 * Somebody could've already done the hard work for us.
>  	 * No need to get locks for this.
>  	 */
> -	if (logerror && log->l_iclog->ic_state & XLOG_STATE_IOERROR) {
> +	if (logerror && log->l_iclog->ic_state == XLOG_STATE_IOERROR) {
>  		ASSERT(XLOG_FORCED_SHUTDOWN(log));
>  		return 1;
>  	}
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index ef652abd112c..a1204424a938 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -847,7 +847,7 @@ xlog_cil_push(
>  		goto out_abort;
>  
>  	spin_lock(&commit_iclog->ic_callback_lock);
> -	if (commit_iclog->ic_state & XLOG_STATE_IOERROR) {
> +	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
>  		spin_unlock(&commit_iclog->ic_callback_lock);
>  		goto out_abort;
>  	}
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 66bd370ae60a..bf076893f516 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -40,15 +40,16 @@ static inline uint xlog_get_client_id(__be32 i)
>  /*
>   * In core log state
>   */
> -#define XLOG_STATE_ACTIVE    0x0001 /* Current IC log being written to */
> -#define XLOG_STATE_WANT_SYNC 0x0002 /* Want to sync this iclog; no more writes */
> -#define XLOG_STATE_SYNCING   0x0004 /* This IC log is syncing */
> -#define XLOG_STATE_DONE_SYNC 0x0008 /* Done syncing to disk */
> -#define XLOG_STATE_DO_CALLBACK \
> -			     0x0010 /* Process callback functions */
> -#define XLOG_STATE_CALLBACK  0x0020 /* Callback functions now */
> -#define XLOG_STATE_DIRTY     0x0040 /* Dirty IC log, not ready for ACTIVE status*/
> -#define XLOG_STATE_IOERROR   0x0080 /* IO error happened in sync'ing log */
> +enum xlog_iclog_state {
> +	XLOG_STATE_ACTIVE,	/* Current IC log being written to */
> +	XLOG_STATE_WANT_SYNC,	/* Want to sync this iclog; no more writes */
> +	XLOG_STATE_SYNCING,	/* This IC log is syncing */
> +	XLOG_STATE_DONE_SYNC,	/* Done syncing to disk */
> +	XLOG_STATE_DO_CALLBACK,	/* Process callback functions */
> +	XLOG_STATE_CALLBACK,	/* Callback functions now */
> +	XLOG_STATE_DIRTY,	/* Dirty IC log, not ready for ACTIVE status */
> +	XLOG_STATE_IOERROR,	/* IO error happened in sync'ing log */
> +};
>  
>  /*
>   * Flags to log ticket
> @@ -202,7 +203,7 @@ typedef struct xlog_in_core {
>  	struct xlog		*ic_log;
>  	u32			ic_size;
>  	u32			ic_offset;
> -	unsigned short		ic_state;
> +	enum xlog_iclog_state	ic_state;
>  	char			*ic_datap;	/* pointer to iclog data */
>  
>  	/* Callback structures need their own cacheline */
> -- 
> 2.20.1
> 

[PATCH 8/8] xfs: remove the XLOG_STATE_DO_CALLBACK state

[Christoph Hellwig]

XLOG_STATE_DO_CALLBACK is only entered through XLOG_STATE_DONE_SYNC
and just used in a single debug check.  Remove the flag and thus
simplify the calling conventions for xlog_state_do_callback and
xlog_state_iodone_process_iclog.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 76 +++++++------------------------------------
 fs/xfs/xfs_log_priv.h |  1 -
 2 files changed, 11 insertions(+), 66 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 001a9586cb56..d158b6d56a26 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2750,7 +2750,6 @@ static bool
 xlog_state_iodone_process_iclog(
 	struct xlog		*log,
 	struct xlog_in_core	*iclog,
-	struct xlog_in_core	*completed_iclog,
 	bool			*ioerror)
 {
 	xfs_lsn_t		lowest_lsn;
@@ -2768,17 +2767,16 @@ xlog_state_iodone_process_iclog(
 		 * Between marking a filesystem SHUTDOWN and stopping the log,
 		 * we do flush all iclogs to disk (if there wasn't a log I/O
 		 * error). So, we do want things to go smoothly in case of just
-		 * a SHUTDOWN  w/o a LOG_IO_ERROR.
+		 * a SHUTDOWN w/o a LOG_IO_ERROR.
 		 */
 		*ioerror = true;
 		return false;
 	case XLOG_STATE_DONE_SYNC:
-	case XLOG_STATE_DO_CALLBACK:
 		/*
-		 * Now that we have an iclog that is in either the DO_CALLBACK
-		 * or DONE_SYNC states, do one more check here to see if we have
-		 * chased our tail around.  If this is not the lowest lsn iclog,
-		 * then we will leave it for another completion to process.
+		 * Now that we have an iclog that is in the DONE_SYNC state, do
+		 * one more check here to see if we have chased our tail around.
+		 * If this is not the lowest lsn iclog, then we will leave it
+		 * for another completion to process.
 		 */
 		header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
 		lowest_lsn = xlog_get_lowest_lsn(log);
@@ -2789,15 +2787,9 @@ xlog_state_iodone_process_iclog(
 	default:
 		/*
 		 * Can only perform callbacks in order.  Since this iclog is not
-		 * in the DONE_SYNC or DO_CALLBACK states, we skip the rest and
-		 * just try to clean up.  If we set our iclog to DO_CALLBACK, we
-		 * will not process it when we retry since a previous iclog is
-		 * in the CALLBACK and the state cannot change since we are
-		 * holding l_icloglock.
+		 * in the DONE_SYNC state, we skip the rest and just try to
+		 * clean up.
 		 */
-		if (completed_iclog &&
-		    (completed_iclog->ic_state == XLOG_STATE_DONE_SYNC))
-			completed_iclog->ic_state = XLOG_STATE_DO_CALLBACK;
 		return true;
 	}
 }
@@ -2838,54 +2830,13 @@ xlog_state_do_iclog_callbacks(
 	spin_unlock(&iclog->ic_callback_lock);
 }
 
-#ifdef DEBUG
-/*
- * Make one last gasp attempt to see if iclogs are being left in limbo.  If the
- * above loop finds an iclog earlier than the current iclog and in one of the
- * syncing states, the current iclog is put into DO_CALLBACK and the callbacks
- * are deferred to the completion of the earlier iclog. Walk the iclogs in order
- * and make sure that no iclog is in DO_CALLBACK unless an earlier iclog is in
- * one of the syncing states.
- */
-static void
-xlog_state_callback_check_state(
-	struct xlog		*log)
-{
-	struct xlog_in_core	*first_iclog = log->l_iclog;
-	struct xlog_in_core	*iclog = first_iclog;
-
-	do {
-		ASSERT(iclog->ic_state != XLOG_STATE_DO_CALLBACK);
-		/*
-		 * Terminate the loop if iclogs are found in states
-		 * which will cause other threads to clean up iclogs.
-		 *
-		 * SYNCING - i/o completion will go through logs
-		 * DONE_SYNC - interrupt thread should be waiting for
-		 *              l_icloglock
-		 * IOERROR - give up hope all ye who enter here
-		 */
-		if (iclog->ic_state == XLOG_STATE_WANT_SYNC ||
-		    iclog->ic_state == XLOG_STATE_SYNCING ||
-		    iclog->ic_state == XLOG_STATE_DONE_SYNC ||
-		    iclog->ic_state == XLOG_STATE_IOERROR )
-			break;
-		iclog = iclog->ic_next;
-	} while (first_iclog != iclog);
-}
-#else
-#define xlog_state_callback_check_state(l)	((void)0)
-#endif
-
 STATIC void
 xlog_state_do_callback(
 	struct xlog		*log,
-	bool			aborted,
-	struct xlog_in_core	*ciclog)
+	bool			aborted)
 {
 	struct xlog_in_core	*iclog;
 	struct xlog_in_core	*first_iclog;
-	bool			did_callbacks = false;
 	bool			cycled_icloglock;
 	bool			ioerror;
 	int			flushcnt = 0;
@@ -2909,7 +2860,7 @@ xlog_state_do_callback(
 
 		do {
 			if (xlog_state_iodone_process_iclog(log, iclog,
-							ciclog, &ioerror))
+							&ioerror))
 				break;
 
 			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
@@ -2929,8 +2880,6 @@ xlog_state_do_callback(
 			iclog = iclog->ic_next;
 		} while (first_iclog != iclog);
 
-		did_callbacks |= cycled_icloglock;
-
 		if (repeats > 5000) {
 			flushcnt += repeats;
 			repeats = 0;
@@ -2940,9 +2889,6 @@ xlog_state_do_callback(
 		}
 	} while (!ioerror && cycled_icloglock);
 
-	if (did_callbacks)
-		xlog_state_callback_check_state(log);
-
 	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
 	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
 		wake_up_all(&log->l_flush_wait);
@@ -2993,7 +2939,7 @@ xlog_state_done_syncing(
 	 */
 	wake_up_all(&iclog->ic_write_wait);
 	spin_unlock(&log->l_icloglock);
-	xlog_state_do_callback(log, aborted, iclog);	/* also cleans log */
+	xlog_state_do_callback(log, aborted);	/* also cleans log */
 }	/* xlog_state_done_syncing */
 
 
@@ -3987,7 +3933,7 @@ xfs_log_force_umount(
 	spin_lock(&log->l_cilp->xc_push_lock);
 	wake_up_all(&log->l_cilp->xc_commit_wait);
 	spin_unlock(&log->l_cilp->xc_push_lock);
-	xlog_state_do_callback(log, true, NULL);
+	xlog_state_do_callback(log, true);
 
 	/* return non-zero if log IOERROR transition had already happened */
 	return retval;
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index bf076893f516..4f19375f6592 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -45,7 +45,6 @@ enum xlog_iclog_state {
 	XLOG_STATE_WANT_SYNC,	/* Want to sync this iclog; no more writes */
 	XLOG_STATE_SYNCING,	/* This IC log is syncing */
 	XLOG_STATE_DONE_SYNC,	/* Done syncing to disk */
-	XLOG_STATE_DO_CALLBACK,	/* Process callback functions */
 	XLOG_STATE_CALLBACK,	/* Callback functions now */
 	XLOG_STATE_DIRTY,	/* Dirty IC log, not ready for ACTIVE status */
 	XLOG_STATE_IOERROR,	/* IO error happened in sync'ing log */
-- 
2.20.1

Re: [PATCH 8/8] xfs: remove the XLOG_STATE_DO_CALLBACK state

[Darrick J. Wong]

On Wed, Oct 09, 2019 at 04:27:48PM +0200, Christoph Hellwig wrote:
> XLOG_STATE_DO_CALLBACK is only entered through XLOG_STATE_DONE_SYNC
> and just used in a single debug check.  Remove the flag and thus
> simplify the calling conventions for xlog_state_do_callback and
> xlog_state_iodone_process_iclog.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c      | 76 +++++++------------------------------------
>  fs/xfs/xfs_log_priv.h |  1 -
>  2 files changed, 11 insertions(+), 66 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 001a9586cb56..d158b6d56a26 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2750,7 +2750,6 @@ static bool
>  xlog_state_iodone_process_iclog(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog,
> -	struct xlog_in_core	*completed_iclog,
>  	bool			*ioerror)
>  {
>  	xfs_lsn_t		lowest_lsn;
> @@ -2768,17 +2767,16 @@ xlog_state_iodone_process_iclog(
>  		 * Between marking a filesystem SHUTDOWN and stopping the log,
>  		 * we do flush all iclogs to disk (if there wasn't a log I/O
>  		 * error). So, we do want things to go smoothly in case of just
> -		 * a SHUTDOWN  w/o a LOG_IO_ERROR.
> +		 * a SHUTDOWN w/o a LOG_IO_ERROR.
>  		 */
>  		*ioerror = true;
>  		return false;
>  	case XLOG_STATE_DONE_SYNC:
> -	case XLOG_STATE_DO_CALLBACK:
>  		/*
> -		 * Now that we have an iclog that is in either the DO_CALLBACK
> -		 * or DONE_SYNC states, do one more check here to see if we have
> -		 * chased our tail around.  If this is not the lowest lsn iclog,
> -		 * then we will leave it for another completion to process.
> +		 * Now that we have an iclog that is in the DONE_SYNC state, do
> +		 * one more check here to see if we have chased our tail around.
> +		 * If this is not the lowest lsn iclog, then we will leave it
> +		 * for another completion to process.
>  		 */
>  		header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
>  		lowest_lsn = xlog_get_lowest_lsn(log);
> @@ -2789,15 +2787,9 @@ xlog_state_iodone_process_iclog(
>  	default:
>  		/*
>  		 * Can only perform callbacks in order.  Since this iclog is not
> -		 * in the DONE_SYNC or DO_CALLBACK states, we skip the rest and
> -		 * just try to clean up.  If we set our iclog to DO_CALLBACK, we
> -		 * will not process it when we retry since a previous iclog is
> -		 * in the CALLBACK and the state cannot change since we are
> -		 * holding l_icloglock.
> +		 * in the DONE_SYNC state, we skip the rest and just try to
> +		 * clean up.
>  		 */
> -		if (completed_iclog &&
> -		    (completed_iclog->ic_state == XLOG_STATE_DONE_SYNC))
> -			completed_iclog->ic_state = XLOG_STATE_DO_CALLBACK;
>  		return true;
>  	}
>  }
> @@ -2838,54 +2830,13 @@ xlog_state_do_iclog_callbacks(
>  	spin_unlock(&iclog->ic_callback_lock);
>  }
>  
> -#ifdef DEBUG
> -/*
> - * Make one last gasp attempt to see if iclogs are being left in limbo.  If the
> - * above loop finds an iclog earlier than the current iclog and in one of the
> - * syncing states, the current iclog is put into DO_CALLBACK and the callbacks
> - * are deferred to the completion of the earlier iclog. Walk the iclogs in order
> - * and make sure that no iclog is in DO_CALLBACK unless an earlier iclog is in
> - * one of the syncing states.
> - */
> -static void
> -xlog_state_callback_check_state(
> -	struct xlog		*log)
> -{
> -	struct xlog_in_core	*first_iclog = log->l_iclog;
> -	struct xlog_in_core	*iclog = first_iclog;
> -
> -	do {
> -		ASSERT(iclog->ic_state != XLOG_STATE_DO_CALLBACK);
> -		/*
> -		 * Terminate the loop if iclogs are found in states
> -		 * which will cause other threads to clean up iclogs.
> -		 *
> -		 * SYNCING - i/o completion will go through logs
> -		 * DONE_SYNC - interrupt thread should be waiting for
> -		 *              l_icloglock
> -		 * IOERROR - give up hope all ye who enter here
> -		 */
> -		if (iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -		    iclog->ic_state == XLOG_STATE_SYNCING ||
> -		    iclog->ic_state == XLOG_STATE_DONE_SYNC ||
> -		    iclog->ic_state == XLOG_STATE_IOERROR )
> -			break;
> -		iclog = iclog->ic_next;
> -	} while (first_iclog != iclog);
> -}
> -#else
> -#define xlog_state_callback_check_state(l)	((void)0)
> -#endif

So, uh... does this debugging functionality just disappear?  Is it
unnecessary?  It's not clear (to me anyway) why it's ok for the extra
checking to go away.

--D

> -
>  STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log,
> -	bool			aborted,
> -	struct xlog_in_core	*ciclog)
> +	bool			aborted)
>  {
>  	struct xlog_in_core	*iclog;
>  	struct xlog_in_core	*first_iclog;
> -	bool			did_callbacks = false;
>  	bool			cycled_icloglock;
>  	bool			ioerror;
>  	int			flushcnt = 0;
> @@ -2909,7 +2860,7 @@ xlog_state_do_callback(
>  
>  		do {
>  			if (xlog_state_iodone_process_iclog(log, iclog,
> -							ciclog, &ioerror))
> +							&ioerror))
>  				break;
>  
>  			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
> @@ -2929,8 +2880,6 @@ xlog_state_do_callback(
>  			iclog = iclog->ic_next;
>  		} while (first_iclog != iclog);
>  
> -		did_callbacks |= cycled_icloglock;
> -
>  		if (repeats > 5000) {
>  			flushcnt += repeats;
>  			repeats = 0;
> @@ -2940,9 +2889,6 @@ xlog_state_do_callback(
>  		}
>  	} while (!ioerror && cycled_icloglock);
>  
> -	if (did_callbacks)
> -		xlog_state_callback_check_state(log);
> -
>  	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
>  	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
>  		wake_up_all(&log->l_flush_wait);
> @@ -2993,7 +2939,7 @@ xlog_state_done_syncing(
>  	 */
>  	wake_up_all(&iclog->ic_write_wait);
>  	spin_unlock(&log->l_icloglock);
> -	xlog_state_do_callback(log, aborted, iclog);	/* also cleans log */
> +	xlog_state_do_callback(log, aborted);	/* also cleans log */
>  }	/* xlog_state_done_syncing */
>  
>  
> @@ -3987,7 +3933,7 @@ xfs_log_force_umount(
>  	spin_lock(&log->l_cilp->xc_push_lock);
>  	wake_up_all(&log->l_cilp->xc_commit_wait);
>  	spin_unlock(&log->l_cilp->xc_push_lock);
> -	xlog_state_do_callback(log, true, NULL);
> +	xlog_state_do_callback(log, true);
>  
>  	/* return non-zero if log IOERROR transition had already happened */
>  	return retval;
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index bf076893f516..4f19375f6592 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -45,7 +45,6 @@ enum xlog_iclog_state {
>  	XLOG_STATE_WANT_SYNC,	/* Want to sync this iclog; no more writes */
>  	XLOG_STATE_SYNCING,	/* This IC log is syncing */
>  	XLOG_STATE_DONE_SYNC,	/* Done syncing to disk */
> -	XLOG_STATE_DO_CALLBACK,	/* Process callback functions */
>  	XLOG_STATE_CALLBACK,	/* Callback functions now */
>  	XLOG_STATE_DIRTY,	/* Dirty IC log, not ready for ACTIVE status */
>  	XLOG_STATE_IOERROR,	/* IO error happened in sync'ing log */
> -- 
> 2.20.1
> 

Re: [PATCH 8/8] xfs: remove the XLOG_STATE_DO_CALLBACK state

[Christoph Hellwig]

On Fri, Oct 11, 2019 at 05:41:45PM -0700, Darrick J. Wong wrote:
> > -#else
> > -#define xlog_state_callback_check_state(l)	((void)0)
> > -#endif
> 
> So, uh... does this debugging functionality just disappear?  Is it
> unnecessary?  It's not clear (to me anyway) why it's ok for the extra
> checking to go away.

Lets ask the counter question:  What kind of bug do you think this
check would catch?

Re: [PATCH 8/8] xfs: remove the XLOG_STATE_DO_CALLBACK state

[Darrick J. Wong]

On Mon, Oct 14, 2019 at 09:22:24AM +0200, Christoph Hellwig wrote:
> On Fri, Oct 11, 2019 at 05:41:45PM -0700, Darrick J. Wong wrote:
> > > -#else
> > > -#define xlog_state_callback_check_state(l)	((void)0)
> > > -#endif
> > 
> > So, uh... does this debugging functionality just disappear?  Is it
> > unnecessary?  It's not clear (to me anyway) why it's ok for the extra
> > checking to go away.
> 
> Lets ask the counter question:  What kind of bug do you think this
> check would catch?

Dunno, that's why I'm asking /you/ :)

I think the answer is that DO_CALLBACK is pretty pointless since we
already have a CALLBACK state, and the removed debugging function checks
among all the iclogs for a state that shouldn't be possible (getting
ready to be doing callbacks when there are other iclogs further along in
the list that are still syncing or ioerror'd)?  Particularly since we
probably end up moving the iclog to the actual CALLBACK state pretty
quickly anyway?

--D

Re: [PATCH 8/8] xfs: remove the XLOG_STATE_DO_CALLBACK state

[Brian Foster]

On Wed, Oct 09, 2019 at 04:27:48PM +0200, Christoph Hellwig wrote:
> XLOG_STATE_DO_CALLBACK is only entered through XLOG_STATE_DONE_SYNC
> and just used in a single debug check.  Remove the flag and thus
> simplify the calling conventions for xlog_state_do_callback and
> xlog_state_iodone_process_iclog.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c      | 76 +++++++------------------------------------
>  fs/xfs/xfs_log_priv.h |  1 -
>  2 files changed, 11 insertions(+), 66 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 001a9586cb56..d158b6d56a26 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2750,7 +2750,6 @@ static bool
>  xlog_state_iodone_process_iclog(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog,
> -	struct xlog_in_core	*completed_iclog,
>  	bool			*ioerror)
>  {
>  	xfs_lsn_t		lowest_lsn;
> @@ -2768,17 +2767,16 @@ xlog_state_iodone_process_iclog(
>  		 * Between marking a filesystem SHUTDOWN and stopping the log,
>  		 * we do flush all iclogs to disk (if there wasn't a log I/O
>  		 * error). So, we do want things to go smoothly in case of just
> -		 * a SHUTDOWN  w/o a LOG_IO_ERROR.
> +		 * a SHUTDOWN w/o a LOG_IO_ERROR.
>  		 */
>  		*ioerror = true;
>  		return false;
>  	case XLOG_STATE_DONE_SYNC:
> -	case XLOG_STATE_DO_CALLBACK:
>  		/*
> -		 * Now that we have an iclog that is in either the DO_CALLBACK
> -		 * or DONE_SYNC states, do one more check here to see if we have
> -		 * chased our tail around.  If this is not the lowest lsn iclog,
> -		 * then we will leave it for another completion to process.
> +		 * Now that we have an iclog that is in the DONE_SYNC state, do
> +		 * one more check here to see if we have chased our tail around.
> +		 * If this is not the lowest lsn iclog, then we will leave it
> +		 * for another completion to process.
>  		 */
>  		header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
>  		lowest_lsn = xlog_get_lowest_lsn(log);
> @@ -2789,15 +2787,9 @@ xlog_state_iodone_process_iclog(
>  	default:
>  		/*
>  		 * Can only perform callbacks in order.  Since this iclog is not
> -		 * in the DONE_SYNC or DO_CALLBACK states, we skip the rest and
> -		 * just try to clean up.  If we set our iclog to DO_CALLBACK, we
> -		 * will not process it when we retry since a previous iclog is
> -		 * in the CALLBACK and the state cannot change since we are
> -		 * holding l_icloglock.
> +		 * in the DONE_SYNC state, we skip the rest and just try to
> +		 * clean up.
>  		 */
> -		if (completed_iclog &&
> -		    (completed_iclog->ic_state == XLOG_STATE_DONE_SYNC))
> -			completed_iclog->ic_state = XLOG_STATE_DO_CALLBACK;
>  		return true;
>  	}
>  }
> @@ -2838,54 +2830,13 @@ xlog_state_do_iclog_callbacks(
>  	spin_unlock(&iclog->ic_callback_lock);
>  }
>  
> -#ifdef DEBUG
> -/*
> - * Make one last gasp attempt to see if iclogs are being left in limbo.  If the
> - * above loop finds an iclog earlier than the current iclog and in one of the
> - * syncing states, the current iclog is put into DO_CALLBACK and the callbacks
> - * are deferred to the completion of the earlier iclog. Walk the iclogs in order
> - * and make sure that no iclog is in DO_CALLBACK unless an earlier iclog is in
> - * one of the syncing states.
> - */
> -static void
> -xlog_state_callback_check_state(
> -	struct xlog		*log)
> -{
> -	struct xlog_in_core	*first_iclog = log->l_iclog;
> -	struct xlog_in_core	*iclog = first_iclog;
> -
> -	do {
> -		ASSERT(iclog->ic_state != XLOG_STATE_DO_CALLBACK);
> -		/*
> -		 * Terminate the loop if iclogs are found in states
> -		 * which will cause other threads to clean up iclogs.
> -		 *
> -		 * SYNCING - i/o completion will go through logs
> -		 * DONE_SYNC - interrupt thread should be waiting for
> -		 *              l_icloglock
> -		 * IOERROR - give up hope all ye who enter here
> -		 */
> -		if (iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -		    iclog->ic_state == XLOG_STATE_SYNCING ||
> -		    iclog->ic_state == XLOG_STATE_DONE_SYNC ||
> -		    iclog->ic_state == XLOG_STATE_IOERROR )
> -			break;
> -		iclog = iclog->ic_next;
> -	} while (first_iclog != iclog);
> -}
> -#else
> -#define xlog_state_callback_check_state(l)	((void)0)
> -#endif
> -
>  STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log,
> -	bool			aborted,
> -	struct xlog_in_core	*ciclog)
> +	bool			aborted)
>  {
>  	struct xlog_in_core	*iclog;
>  	struct xlog_in_core	*first_iclog;
> -	bool			did_callbacks = false;
>  	bool			cycled_icloglock;
>  	bool			ioerror;
>  	int			flushcnt = 0;
> @@ -2909,7 +2860,7 @@ xlog_state_do_callback(
>  
>  		do {
>  			if (xlog_state_iodone_process_iclog(log, iclog,
> -							ciclog, &ioerror))
> +							&ioerror))
>  				break;
>  
>  			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
> @@ -2929,8 +2880,6 @@ xlog_state_do_callback(
>  			iclog = iclog->ic_next;
>  		} while (first_iclog != iclog);
>  
> -		did_callbacks |= cycled_icloglock;
> -
>  		if (repeats > 5000) {
>  			flushcnt += repeats;
>  			repeats = 0;
> @@ -2940,9 +2889,6 @@ xlog_state_do_callback(
>  		}
>  	} while (!ioerror && cycled_icloglock);
>  
> -	if (did_callbacks)
> -		xlog_state_callback_check_state(log);
> -
>  	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
>  	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
>  		wake_up_all(&log->l_flush_wait);
> @@ -2993,7 +2939,7 @@ xlog_state_done_syncing(
>  	 */
>  	wake_up_all(&iclog->ic_write_wait);
>  	spin_unlock(&log->l_icloglock);
> -	xlog_state_do_callback(log, aborted, iclog);	/* also cleans log */
> +	xlog_state_do_callback(log, aborted);	/* also cleans log */
>  }	/* xlog_state_done_syncing */
>  
>  
> @@ -3987,7 +3933,7 @@ xfs_log_force_umount(
>  	spin_lock(&log->l_cilp->xc_push_lock);
>  	wake_up_all(&log->l_cilp->xc_commit_wait);
>  	spin_unlock(&log->l_cilp->xc_push_lock);
> -	xlog_state_do_callback(log, true, NULL);
> +	xlog_state_do_callback(log, true);
>  
>  	/* return non-zero if log IOERROR transition had already happened */
>  	return retval;
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index bf076893f516..4f19375f6592 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -45,7 +45,6 @@ enum xlog_iclog_state {
>  	XLOG_STATE_WANT_SYNC,	/* Want to sync this iclog; no more writes */
>  	XLOG_STATE_SYNCING,	/* This IC log is syncing */
>  	XLOG_STATE_DONE_SYNC,	/* Done syncing to disk */
> -	XLOG_STATE_DO_CALLBACK,	/* Process callback functions */
>  	XLOG_STATE_CALLBACK,	/* Callback functions now */
>  	XLOG_STATE_DIRTY,	/* Dirty IC log, not ready for ACTIVE status */
>  	XLOG_STATE_IOERROR,	/* IO error happened in sync'ing log */
> -- 
> 2.20.1
>