cleanup log I/O error handling v2

cleanup log I/O error handling v2

[Christoph Hellwig]

Hi all,

this series cleans up the XFS log I/O error handling and removes the
XLOG_STATE_IOERROR iclog state.

Changes since v1:
 - split up more into more patches
 - additional refactoring
 - additional cleanups
 - dropped patches already merged

[PATCH 01/14] xfs: merge xlog_cil_push into xlog_cil_push_work

[Christoph Hellwig]

xlog_cil_push is only called by xlog_cil_push_work, so merge the two
functions.

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

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 48435cf2aa16..6a6278b8eb2d 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -626,24 +626,26 @@ xlog_cil_process_committed(
 }
 
 /*
- * Push the Committed Item List to the log. If @push_seq flag is zero, then it
- * is a background flush and so we can chose to ignore it. Otherwise, if the
- * current sequence is the same as @push_seq we need to do a flush. If
- * @push_seq is less than the current sequence, then it has already been
+ * Push the Committed Item List to the log.
+ *
+ * If the current sequence is the same as xc_push_seq we need to do a flush. If
+ * xc_push_seq is less than the current sequence, then it has already been
  * flushed and we don't need to do anything - the caller will wait for it to
  * complete if necessary.
  *
- * @push_seq is a value rather than a flag because that allows us to do an
- * unlocked check of the sequence number for a match. Hence we can allows log
- * forces to run racily and not issue pushes for the same sequence twice. If we
- * get a race between multiple pushes for the same sequence they will block on
- * the first one and then abort, hence avoiding needless pushes.
+ * xc_push_seq is checked unlocked against the sequence number for a match.
+ * Hence we can allows log forces to run racily and not issue pushes for the
+ * same sequence twice.  If we get a race between multiple pushes for the same
+ * sequence they will block on the first one and then abort, hence avoiding
+ * needless pushes.
  */
-STATIC int
-xlog_cil_push(
-	struct xlog		*log)
+static void
+xlog_cil_push_work(
+	struct work_struct	*work)
 {
-	struct xfs_cil		*cil = log->l_cilp;
+	struct xfs_cil		*cil =
+		container_of(work, struct xfs_cil, xc_push_work);
+	struct xlog		*log = cil->xc_log;
 	struct xfs_log_vec	*lv;
 	struct xfs_cil_ctx	*ctx;
 	struct xfs_cil_ctx	*new_ctx;
@@ -657,9 +659,6 @@ xlog_cil_push(
 	xfs_lsn_t		commit_lsn;
 	xfs_lsn_t		push_seq;
 
-	if (!cil)
-		return 0;
-
 	new_ctx = kmem_zalloc(sizeof(*new_ctx), KM_NOFS);
 	new_ctx->ticket = xlog_cil_ticket_alloc(log);
 
@@ -867,28 +866,19 @@ xlog_cil_push(
 	spin_unlock(&cil->xc_push_lock);
 
 	/* release the hounds! */
-	return xfs_log_release_iclog(log->l_mp, commit_iclog);
+	xfs_log_release_iclog(log->l_mp, commit_iclog);
+	return;
 
 out_skip:
 	up_write(&cil->xc_ctx_lock);
 	xfs_log_ticket_put(new_ctx->ticket);
 	kmem_free(new_ctx);
-	return 0;
+	return;
 
 out_abort_free_ticket:
 	xfs_log_ticket_put(tic);
 out_abort:
 	xlog_cil_committed(ctx, true);
-	return -EIO;
-}
-
-static void
-xlog_cil_push_work(
-	struct work_struct	*work)
-{
-	struct xfs_cil		*cil = container_of(work, struct xfs_cil,
-							xc_push_work);
-	xlog_cil_push(cil->xc_log);
 }
 
 /*
-- 
2.24.1

Re: [PATCH 01/14] xfs: merge xlog_cil_push into xlog_cil_push_work

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:20PM +0100, Christoph Hellwig wrote:
> xlog_cil_push is only called by xlog_cil_push_work, so merge the two
> functions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks like a simple enough refactoring,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log_cil.c | 46 +++++++++++++++++---------------------------
>  1 file changed, 18 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 48435cf2aa16..6a6278b8eb2d 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -626,24 +626,26 @@ xlog_cil_process_committed(
>  }
>  
>  /*
> - * Push the Committed Item List to the log. If @push_seq flag is zero, then it
> - * is a background flush and so we can chose to ignore it. Otherwise, if the
> - * current sequence is the same as @push_seq we need to do a flush. If
> - * @push_seq is less than the current sequence, then it has already been
> + * Push the Committed Item List to the log.
> + *
> + * If the current sequence is the same as xc_push_seq we need to do a flush. If
> + * xc_push_seq is less than the current sequence, then it has already been
>   * flushed and we don't need to do anything - the caller will wait for it to
>   * complete if necessary.
>   *
> - * @push_seq is a value rather than a flag because that allows us to do an
> - * unlocked check of the sequence number for a match. Hence we can allows log
> - * forces to run racily and not issue pushes for the same sequence twice. If we
> - * get a race between multiple pushes for the same sequence they will block on
> - * the first one and then abort, hence avoiding needless pushes.
> + * xc_push_seq is checked unlocked against the sequence number for a match.
> + * Hence we can allows log forces to run racily and not issue pushes for the
> + * same sequence twice.  If we get a race between multiple pushes for the same
> + * sequence they will block on the first one and then abort, hence avoiding
> + * needless pushes.
>   */
> -STATIC int
> -xlog_cil_push(
> -	struct xlog		*log)
> +static void
> +xlog_cil_push_work(
> +	struct work_struct	*work)
>  {
> -	struct xfs_cil		*cil = log->l_cilp;
> +	struct xfs_cil		*cil =
> +		container_of(work, struct xfs_cil, xc_push_work);
> +	struct xlog		*log = cil->xc_log;
>  	struct xfs_log_vec	*lv;
>  	struct xfs_cil_ctx	*ctx;
>  	struct xfs_cil_ctx	*new_ctx;
> @@ -657,9 +659,6 @@ xlog_cil_push(
>  	xfs_lsn_t		commit_lsn;
>  	xfs_lsn_t		push_seq;
>  
> -	if (!cil)
> -		return 0;
> -
>  	new_ctx = kmem_zalloc(sizeof(*new_ctx), KM_NOFS);
>  	new_ctx->ticket = xlog_cil_ticket_alloc(log);
>  
> @@ -867,28 +866,19 @@ xlog_cil_push(
>  	spin_unlock(&cil->xc_push_lock);
>  
>  	/* release the hounds! */
> -	return xfs_log_release_iclog(log->l_mp, commit_iclog);
> +	xfs_log_release_iclog(log->l_mp, commit_iclog);
> +	return;
>  
>  out_skip:
>  	up_write(&cil->xc_ctx_lock);
>  	xfs_log_ticket_put(new_ctx->ticket);
>  	kmem_free(new_ctx);
> -	return 0;
> +	return;
>  
>  out_abort_free_ticket:
>  	xfs_log_ticket_put(tic);
>  out_abort:
>  	xlog_cil_committed(ctx, true);
> -	return -EIO;
> -}
> -
> -static void
> -xlog_cil_push_work(
> -	struct work_struct	*work)
> -{
> -	struct xfs_cil		*cil = container_of(work, struct xfs_cil,
> -							xc_push_work);
> -	xlog_cil_push(cil->xc_log);
>  }
>  
>  /*
> -- 
> 2.24.1
> 

Re: [PATCH 01/14] xfs: merge xlog_cil_push into xlog_cil_push_work

[Dave Chinner]

On Mon, Mar 16, 2020 at 03:42:20PM +0100, Christoph Hellwig wrote:
> xlog_cil_push is only called by xlog_cil_push_work, so merge the two
> functions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log_cil.c | 46 +++++++++++++++++---------------------------
>  1 file changed, 18 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 48435cf2aa16..6a6278b8eb2d 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -626,24 +626,26 @@ xlog_cil_process_committed(
>  }
>  
>  /*
> - * Push the Committed Item List to the log. If @push_seq flag is zero, then it
> - * is a background flush and so we can chose to ignore it. Otherwise, if the
> - * current sequence is the same as @push_seq we need to do a flush. If
> - * @push_seq is less than the current sequence, then it has already been
> + * Push the Committed Item List to the log.
> + *
> + * If the current sequence is the same as xc_push_seq we need to do a flush. If
> + * xc_push_seq is less than the current sequence, then it has already been
>   * flushed and we don't need to do anything - the caller will wait for it to
>   * complete if necessary.
>   *
> - * @push_seq is a value rather than a flag because that allows us to do an
> - * unlocked check of the sequence number for a match. Hence we can allows log
> - * forces to run racily and not issue pushes for the same sequence twice. If we
> - * get a race between multiple pushes for the same sequence they will block on
> - * the first one and then abort, hence avoiding needless pushes.
> + * xc_push_seq is checked unlocked against the sequence number for a match.
> + * Hence we can allows log forces to run racily and not issue pushes for the
                   ^^^^^^
		   allow

> + * same sequence twice.  If we get a race between multiple pushes for the same
> + * sequence they will block on the first one and then abort, hence avoiding
> + * needless pushes.

Otherwise looks ok.

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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 01/14] xfs: merge xlog_cil_push into xlog_cil_push_work

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:20PM +0100, Christoph Hellwig wrote:
> xlog_cil_push is only called by xlog_cil_push_work, so merge the two
> functions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log_cil.c | 46 +++++++++++++++++---------------------------
>  1 file changed, 18 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 48435cf2aa16..6a6278b8eb2d 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -626,24 +626,26 @@ xlog_cil_process_committed(
>  }
>  
>  /*
> - * Push the Committed Item List to the log. If @push_seq flag is zero, then it
> - * is a background flush and so we can chose to ignore it. Otherwise, if the
> - * current sequence is the same as @push_seq we need to do a flush. If
> - * @push_seq is less than the current sequence, then it has already been
> + * Push the Committed Item List to the log.
> + *
> + * If the current sequence is the same as xc_push_seq we need to do a flush. If
> + * xc_push_seq is less than the current sequence, then it has already been
>   * flushed and we don't need to do anything - the caller will wait for it to
>   * complete if necessary.
>   *
> - * @push_seq is a value rather than a flag because that allows us to do an
> - * unlocked check of the sequence number for a match. Hence we can allows log
> - * forces to run racily and not issue pushes for the same sequence twice. If we
> - * get a race between multiple pushes for the same sequence they will block on
> - * the first one and then abort, hence avoiding needless pushes.
> + * xc_push_seq is checked unlocked against the sequence number for a match.
> + * Hence we can allows log forces to run racily and not issue pushes for the
> + * same sequence twice.  If we get a race between multiple pushes for the same
> + * sequence they will block on the first one and then abort, hence avoiding
> + * needless pushes.
>   */
> -STATIC int
> -xlog_cil_push(
> -	struct xlog		*log)
> +static void
> +xlog_cil_push_work(
> +	struct work_struct	*work)
>  {
> -	struct xfs_cil		*cil = log->l_cilp;
> +	struct xfs_cil		*cil =
> +		container_of(work, struct xfs_cil, xc_push_work);
> +	struct xlog		*log = cil->xc_log;
>  	struct xfs_log_vec	*lv;
>  	struct xfs_cil_ctx	*ctx;
>  	struct xfs_cil_ctx	*new_ctx;
> @@ -657,9 +659,6 @@ xlog_cil_push(
>  	xfs_lsn_t		commit_lsn;
>  	xfs_lsn_t		push_seq;
>  
> -	if (!cil)
> -		return 0;
> -
>  	new_ctx = kmem_zalloc(sizeof(*new_ctx), KM_NOFS);
>  	new_ctx->ticket = xlog_cil_ticket_alloc(log);
>  
> @@ -867,28 +866,19 @@ xlog_cil_push(
>  	spin_unlock(&cil->xc_push_lock);
>  
>  	/* release the hounds! */
> -	return xfs_log_release_iclog(log->l_mp, commit_iclog);
> +	xfs_log_release_iclog(log->l_mp, commit_iclog);
> +	return;
>  
>  out_skip:
>  	up_write(&cil->xc_ctx_lock);
>  	xfs_log_ticket_put(new_ctx->ticket);
>  	kmem_free(new_ctx);
> -	return 0;
> +	return;
>  
>  out_abort_free_ticket:
>  	xfs_log_ticket_put(tic);
>  out_abort:
>  	xlog_cil_committed(ctx, true);
> -	return -EIO;
> -}
> -
> -static void
> -xlog_cil_push_work(
> -	struct work_struct	*work)
> -{
> -	struct xfs_cil		*cil = container_of(work, struct xfs_cil,
> -							xc_push_work);
> -	xlog_cil_push(cil->xc_log);
>  }
>  
>  /*
> -- 
> 2.24.1
> 

[PATCH 02/14] xfs: factor out a xlog_wait_on_iclog helper

[Christoph Hellwig]

Factor out the shared code to wait for a log force into a new helper.
This helper uses the XLOG_FORCED_SHUTDOWN check previous only used
by the unmount code over the equivalent iclog ioerror state used by
the other two functions.

There is a slight behavior change in that the force of the unmount
record is now accounted in the log force statistics.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 0986983ef6b5..955df2902c2c 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -859,6 +859,31 @@ xfs_log_mount_cancel(
 	xfs_log_unmount(mp);
 }
 
+/*
+ * Wait for the iclog to be written disk, or return an error if the log has been
+ * shut down.
+ */
+static int
+xlog_wait_on_iclog(
+	struct xlog_in_core	*iclog)
+		__releases(iclog->ic_log->l_icloglock)
+{
+	struct xlog		*log = iclog->ic_log;
+
+	if (!XLOG_FORCED_SHUTDOWN(log) &&
+	    iclog->ic_state != XLOG_STATE_ACTIVE &&
+	    iclog->ic_state != XLOG_STATE_DIRTY) {
+		XFS_STATS_INC(log->l_mp, xs_log_force_sleep);
+		xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
+	} else {
+		spin_unlock(&log->l_icloglock);
+	}
+
+	if (XLOG_FORCED_SHUTDOWN(log))
+		return -EIO;
+	return 0;
+}
+
 /*
  * Final log writes as part of unmount.
  *
@@ -926,18 +951,7 @@ xfs_log_write_unmount_record(
 	atomic_inc(&iclog->ic_refcnt);
 	xlog_state_want_sync(log, iclog);
 	error = xlog_state_release_iclog(log, iclog);
-	switch (iclog->ic_state) {
-	default:
-		if (!XLOG_FORCED_SHUTDOWN(log)) {
-			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
-			break;
-		}
-		/* fall through */
-	case XLOG_STATE_ACTIVE:
-	case XLOG_STATE_DIRTY:
-		spin_unlock(&log->l_icloglock);
-		break;
-	}
+	xlog_wait_on_iclog(iclog);
 
 	if (tic) {
 		trace_xfs_log_umount_write(log, tic);
@@ -3230,9 +3244,6 @@ xfs_log_force(
 		 * previous iclog and go to sleep.
 		 */
 		iclog = iclog->ic_prev;
-		if (iclog->ic_state == XLOG_STATE_ACTIVE ||
-		    iclog->ic_state == XLOG_STATE_DIRTY)
-			goto out_unlock;
 	} else if (iclog->ic_state == XLOG_STATE_ACTIVE) {
 		if (atomic_read(&iclog->ic_refcnt) == 0) {
 			/*
@@ -3248,8 +3259,7 @@ xfs_log_force(
 			if (xlog_state_release_iclog(log, iclog))
 				goto out_error;
 
-			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn ||
-			    iclog->ic_state == XLOG_STATE_DIRTY)
+			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn)
 				goto out_unlock;
 		} else {
 			/*
@@ -3269,17 +3279,8 @@ xfs_log_force(
 		;
 	}
 
-	if (!(flags & XFS_LOG_SYNC))
-		goto out_unlock;
-
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
-		goto out_error;
-	XFS_STATS_INC(mp, xs_log_force_sleep);
-	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
-		return -EIO;
-	return 0;
-
+	if (flags & XFS_LOG_SYNC)
+		return xlog_wait_on_iclog(iclog);
 out_unlock:
 	spin_unlock(&log->l_icloglock);
 	return 0;
@@ -3310,9 +3311,6 @@ __xfs_log_force_lsn(
 			goto out_unlock;
 	}
 
-	if (iclog->ic_state == XLOG_STATE_DIRTY)
-		goto out_unlock;
-
 	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
 		/*
 		 * We sleep here if we haven't already slept (e.g. this is the
@@ -3346,20 +3344,8 @@ __xfs_log_force_lsn(
 			*log_flushed = 1;
 	}
 
-	if (!(flags & XFS_LOG_SYNC) ||
-	    (iclog->ic_state == XLOG_STATE_ACTIVE ||
-	     iclog->ic_state == XLOG_STATE_DIRTY))
-		goto out_unlock;
-
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
-		goto out_error;
-
-	XFS_STATS_INC(mp, xs_log_force_sleep);
-	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
-		return -EIO;
-	return 0;
-
+	if (flags & XFS_LOG_SYNC)
+		return xlog_wait_on_iclog(iclog);
 out_unlock:
 	spin_unlock(&log->l_icloglock);
 	return 0;
-- 
2.24.1

Re: [PATCH 02/14] xfs: factor out a xlog_wait_on_iclog helper

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:21PM +0100, Christoph Hellwig wrote:
> Factor out the shared code to wait for a log force into a new helper.
> This helper uses the XLOG_FORCED_SHUTDOWN check previous only used
> by the unmount code over the equivalent iclog ioerror state used by
> the other two functions.
> 
> There is a slight behavior change in that the force of the unmount
> record is now accounted in the log force statistics.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok, though I reserve the right to become confused later on. :)
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c | 76 ++++++++++++++++++++----------------------------
>  1 file changed, 31 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 0986983ef6b5..955df2902c2c 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -859,6 +859,31 @@ xfs_log_mount_cancel(
>  	xfs_log_unmount(mp);
>  }
>  
> +/*
> + * Wait for the iclog to be written disk, or return an error if the log has been
> + * shut down.
> + */
> +static int
> +xlog_wait_on_iclog(
> +	struct xlog_in_core	*iclog)
> +		__releases(iclog->ic_log->l_icloglock)
> +{
> +	struct xlog		*log = iclog->ic_log;
> +
> +	if (!XLOG_FORCED_SHUTDOWN(log) &&
> +	    iclog->ic_state != XLOG_STATE_ACTIVE &&
> +	    iclog->ic_state != XLOG_STATE_DIRTY) {
> +		XFS_STATS_INC(log->l_mp, xs_log_force_sleep);
> +		xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> +	} else {
> +		spin_unlock(&log->l_icloglock);
> +	}
> +
> +	if (XLOG_FORCED_SHUTDOWN(log))
> +		return -EIO;
> +	return 0;
> +}
> +
>  /*
>   * Final log writes as part of unmount.
>   *
> @@ -926,18 +951,7 @@ xfs_log_write_unmount_record(
>  	atomic_inc(&iclog->ic_refcnt);
>  	xlog_state_want_sync(log, iclog);
>  	error = xlog_state_release_iclog(log, iclog);
> -	switch (iclog->ic_state) {
> -	default:
> -		if (!XLOG_FORCED_SHUTDOWN(log)) {
> -			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -			break;
> -		}
> -		/* fall through */
> -	case XLOG_STATE_ACTIVE:
> -	case XLOG_STATE_DIRTY:
> -		spin_unlock(&log->l_icloglock);
> -		break;
> -	}
> +	xlog_wait_on_iclog(iclog);
>  
>  	if (tic) {
>  		trace_xfs_log_umount_write(log, tic);
> @@ -3230,9 +3244,6 @@ xfs_log_force(
>  		 * previous iclog and go to sleep.
>  		 */
>  		iclog = iclog->ic_prev;
> -		if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> -		    iclog->ic_state == XLOG_STATE_DIRTY)
> -			goto out_unlock;
>  	} else if (iclog->ic_state == XLOG_STATE_ACTIVE) {
>  		if (atomic_read(&iclog->ic_refcnt) == 0) {
>  			/*
> @@ -3248,8 +3259,7 @@ xfs_log_force(
>  			if (xlog_state_release_iclog(log, iclog))
>  				goto out_error;
>  
> -			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn ||
> -			    iclog->ic_state == XLOG_STATE_DIRTY)
> +			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn)
>  				goto out_unlock;
>  		} else {
>  			/*
> @@ -3269,17 +3279,8 @@ xfs_log_force(
>  		;
>  	}
>  
> -	if (!(flags & XFS_LOG_SYNC))
> -		goto out_unlock;
> -
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		goto out_error;
> -	XFS_STATS_INC(mp, xs_log_force_sleep);
> -	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		return -EIO;
> -	return 0;
> -
> +	if (flags & XFS_LOG_SYNC)
> +		return xlog_wait_on_iclog(iclog);
>  out_unlock:
>  	spin_unlock(&log->l_icloglock);
>  	return 0;
> @@ -3310,9 +3311,6 @@ __xfs_log_force_lsn(
>  			goto out_unlock;
>  	}
>  
> -	if (iclog->ic_state == XLOG_STATE_DIRTY)
> -		goto out_unlock;
> -
>  	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
>  		/*
>  		 * We sleep here if we haven't already slept (e.g. this is the
> @@ -3346,20 +3344,8 @@ __xfs_log_force_lsn(
>  			*log_flushed = 1;
>  	}
>  
> -	if (!(flags & XFS_LOG_SYNC) ||
> -	    (iclog->ic_state == XLOG_STATE_ACTIVE ||
> -	     iclog->ic_state == XLOG_STATE_DIRTY))
> -		goto out_unlock;
> -
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		goto out_error;
> -
> -	XFS_STATS_INC(mp, xs_log_force_sleep);
> -	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		return -EIO;
> -	return 0;
> -
> +	if (flags & XFS_LOG_SYNC)
> +		return xlog_wait_on_iclog(iclog);
>  out_unlock:
>  	spin_unlock(&log->l_icloglock);
>  	return 0;
> -- 
> 2.24.1
> 

Re: [PATCH 02/14] xfs: factor out a xlog_wait_on_iclog helper

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:21PM +0100, Christoph Hellwig wrote:
> Factor out the shared code to wait for a log force into a new helper.
> This helper uses the XLOG_FORCED_SHUTDOWN check previous only used
> by the unmount code over the equivalent iclog ioerror state used by
> the other two functions.
> 
> There is a slight behavior change in that the force of the unmount
> record is now accounted in the log force statistics.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 76 ++++++++++++++++++++----------------------------
>  1 file changed, 31 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 0986983ef6b5..955df2902c2c 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -859,6 +859,31 @@ xfs_log_mount_cancel(
>  	xfs_log_unmount(mp);
>  }
>  
> +/*
> + * Wait for the iclog to be written disk, or return an error if the log has been
> + * shut down.
> + */
> +static int
> +xlog_wait_on_iclog(
> +	struct xlog_in_core	*iclog)
> +		__releases(iclog->ic_log->l_icloglock)
> +{
> +	struct xlog		*log = iclog->ic_log;
> +
> +	if (!XLOG_FORCED_SHUTDOWN(log) &&
> +	    iclog->ic_state != XLOG_STATE_ACTIVE &&
> +	    iclog->ic_state != XLOG_STATE_DIRTY) {
> +		XFS_STATS_INC(log->l_mp, xs_log_force_sleep);
> +		xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> +	} else {
> +		spin_unlock(&log->l_icloglock);
> +	}
> +
> +	if (XLOG_FORCED_SHUTDOWN(log))
> +		return -EIO;
> +	return 0;
> +}
> +
>  /*
>   * Final log writes as part of unmount.
>   *
> @@ -926,18 +951,7 @@ xfs_log_write_unmount_record(
>  	atomic_inc(&iclog->ic_refcnt);
>  	xlog_state_want_sync(log, iclog);
>  	error = xlog_state_release_iclog(log, iclog);
> -	switch (iclog->ic_state) {
> -	default:
> -		if (!XLOG_FORCED_SHUTDOWN(log)) {
> -			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -			break;
> -		}
> -		/* fall through */
> -	case XLOG_STATE_ACTIVE:
> -	case XLOG_STATE_DIRTY:
> -		spin_unlock(&log->l_icloglock);
> -		break;
> -	}
> +	xlog_wait_on_iclog(iclog);
>  
>  	if (tic) {
>  		trace_xfs_log_umount_write(log, tic);
> @@ -3230,9 +3244,6 @@ xfs_log_force(
>  		 * previous iclog and go to sleep.
>  		 */
>  		iclog = iclog->ic_prev;
> -		if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> -		    iclog->ic_state == XLOG_STATE_DIRTY)
> -			goto out_unlock;
>  	} else if (iclog->ic_state == XLOG_STATE_ACTIVE) {
>  		if (atomic_read(&iclog->ic_refcnt) == 0) {
>  			/*
> @@ -3248,8 +3259,7 @@ xfs_log_force(
>  			if (xlog_state_release_iclog(log, iclog))
>  				goto out_error;
>  
> -			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn ||
> -			    iclog->ic_state == XLOG_STATE_DIRTY)
> +			if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn)
>  				goto out_unlock;
>  		} else {
>  			/*
> @@ -3269,17 +3279,8 @@ xfs_log_force(
>  		;
>  	}
>  
> -	if (!(flags & XFS_LOG_SYNC))
> -		goto out_unlock;
> -
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		goto out_error;
> -	XFS_STATS_INC(mp, xs_log_force_sleep);
> -	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		return -EIO;
> -	return 0;
> -
> +	if (flags & XFS_LOG_SYNC)
> +		return xlog_wait_on_iclog(iclog);
>  out_unlock:
>  	spin_unlock(&log->l_icloglock);
>  	return 0;
> @@ -3310,9 +3311,6 @@ __xfs_log_force_lsn(
>  			goto out_unlock;
>  	}
>  
> -	if (iclog->ic_state == XLOG_STATE_DIRTY)
> -		goto out_unlock;
> -
>  	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
>  		/*
>  		 * We sleep here if we haven't already slept (e.g. this is the
> @@ -3346,20 +3344,8 @@ __xfs_log_force_lsn(
>  			*log_flushed = 1;
>  	}
>  
> -	if (!(flags & XFS_LOG_SYNC) ||
> -	    (iclog->ic_state == XLOG_STATE_ACTIVE ||
> -	     iclog->ic_state == XLOG_STATE_DIRTY))
> -		goto out_unlock;
> -
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		goto out_error;
> -
> -	XFS_STATS_INC(mp, xs_log_force_sleep);
> -	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		return -EIO;
> -	return 0;
> -
> +	if (flags & XFS_LOG_SYNC)
> +		return xlog_wait_on_iclog(iclog);
>  out_unlock:
>  	spin_unlock(&log->l_icloglock);
>  	return 0;
> -- 
> 2.24.1
> 

[PATCH 03/14] xfs: simplify the xfs_log_release_iclog calling convention

[Christoph Hellwig]

The only caller of xfs_log_release_iclog doesn't care about the return
value, so remove it.  Also don't bother passing the mount pointer,
given that we can trivially derive it from the iclog.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 955df2902c2c..17ba92b115ea 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -597,12 +597,11 @@ xlog_state_release_iclog(
 	return 0;
 }
 
-int
+void
 xfs_log_release_iclog(
-	struct xfs_mount        *mp,
 	struct xlog_in_core	*iclog)
 {
-	struct xlog		*log = mp->m_log;
+	struct xlog		*log = iclog->ic_log;
 	bool			sync;
 
 	if (iclog->ic_state == XLOG_STATE_IOERROR)
@@ -618,10 +617,9 @@ xfs_log_release_iclog(
 		if (sync)
 			xlog_sync(log, iclog);
 	}
-	return 0;
+	return;
 error:
-	xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
-	return -EIO;
+	xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
 }
 
 /*
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index 84e06805160f..b38602216c5a 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -121,8 +121,7 @@ void	xfs_log_mount_cancel(struct xfs_mount *);
 xfs_lsn_t xlog_assign_tail_lsn(struct xfs_mount *mp);
 xfs_lsn_t xlog_assign_tail_lsn_locked(struct xfs_mount *mp);
 void	  xfs_log_space_wake(struct xfs_mount *mp);
-int	  xfs_log_release_iclog(struct xfs_mount *mp,
-			 struct xlog_in_core	 *iclog);
+void	  xfs_log_release_iclog(struct xlog_in_core *iclog);
 int	  xfs_log_reserve(struct xfs_mount *mp,
 			  int		   length,
 			  int		   count,
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 6a6278b8eb2d..047ac253edfe 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -866,7 +866,7 @@ xlog_cil_push_work(
 	spin_unlock(&cil->xc_push_lock);
 
 	/* release the hounds! */
-	xfs_log_release_iclog(log->l_mp, commit_iclog);
+	xfs_log_release_iclog(commit_iclog);
 	return;
 
 out_skip:
-- 
2.24.1

Re: [PATCH 03/14] xfs: simplify the xfs_log_release_iclog calling convention

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:22PM +0100, Christoph Hellwig wrote:
> The only caller of xfs_log_release_iclog doesn't care about the return
> value, so remove it.  Also don't bother passing the mount pointer,
> given that we can trivially derive it from the iclog.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log.c     | 10 ++++------
>  fs/xfs/xfs_log.h     |  3 +--
>  fs/xfs/xfs_log_cil.c |  2 +-
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 955df2902c2c..17ba92b115ea 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -597,12 +597,11 @@ xlog_state_release_iclog(
>  	return 0;
>  }
>  
> -int
> +void
>  xfs_log_release_iclog(
> -	struct xfs_mount        *mp,
>  	struct xlog_in_core	*iclog)
>  {
> -	struct xlog		*log = mp->m_log;
> +	struct xlog		*log = iclog->ic_log;
>  	bool			sync;
>  
>  	if (iclog->ic_state == XLOG_STATE_IOERROR)
> @@ -618,10 +617,9 @@ xfs_log_release_iclog(
>  		if (sync)
>  			xlog_sync(log, iclog);
>  	}
> -	return 0;
> +	return;
>  error:
> -	xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
> -	return -EIO;
> +	xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index 84e06805160f..b38602216c5a 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -121,8 +121,7 @@ void	xfs_log_mount_cancel(struct xfs_mount *);
>  xfs_lsn_t xlog_assign_tail_lsn(struct xfs_mount *mp);
>  xfs_lsn_t xlog_assign_tail_lsn_locked(struct xfs_mount *mp);
>  void	  xfs_log_space_wake(struct xfs_mount *mp);
> -int	  xfs_log_release_iclog(struct xfs_mount *mp,
> -			 struct xlog_in_core	 *iclog);
> +void	  xfs_log_release_iclog(struct xlog_in_core *iclog);
>  int	  xfs_log_reserve(struct xfs_mount *mp,
>  			  int		   length,
>  			  int		   count,
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 6a6278b8eb2d..047ac253edfe 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -866,7 +866,7 @@ xlog_cil_push_work(
>  	spin_unlock(&cil->xc_push_lock);
>  
>  	/* release the hounds! */
> -	xfs_log_release_iclog(log->l_mp, commit_iclog);
> +	xfs_log_release_iclog(commit_iclog);
>  	return;
>  
>  out_skip:
> -- 
> 2.24.1
> 

Re: [PATCH 03/14] xfs: simplify the xfs_log_release_iclog calling convention

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:22PM +0100, Christoph Hellwig wrote:
> The only caller of xfs_log_release_iclog doesn't care about the return
> value, so remove it.  Also don't bother passing the mount pointer,
> given that we can trivially derive it from the iclog.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c     | 10 ++++------
>  fs/xfs/xfs_log.h     |  3 +--
>  fs/xfs/xfs_log_cil.c |  2 +-
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 955df2902c2c..17ba92b115ea 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -597,12 +597,11 @@ xlog_state_release_iclog(
>  	return 0;
>  }
>  
> -int
> +void
>  xfs_log_release_iclog(
> -	struct xfs_mount        *mp,
>  	struct xlog_in_core	*iclog)
>  {
> -	struct xlog		*log = mp->m_log;
> +	struct xlog		*log = iclog->ic_log;
>  	bool			sync;
>  
>  	if (iclog->ic_state == XLOG_STATE_IOERROR)
> @@ -618,10 +617,9 @@ xfs_log_release_iclog(
>  		if (sync)
>  			xlog_sync(log, iclog);
>  	}
> -	return 0;
> +	return;
>  error:
> -	xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
> -	return -EIO;
> +	xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index 84e06805160f..b38602216c5a 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -121,8 +121,7 @@ void	xfs_log_mount_cancel(struct xfs_mount *);
>  xfs_lsn_t xlog_assign_tail_lsn(struct xfs_mount *mp);
>  xfs_lsn_t xlog_assign_tail_lsn_locked(struct xfs_mount *mp);
>  void	  xfs_log_space_wake(struct xfs_mount *mp);
> -int	  xfs_log_release_iclog(struct xfs_mount *mp,
> -			 struct xlog_in_core	 *iclog);
> +void	  xfs_log_release_iclog(struct xlog_in_core *iclog);
>  int	  xfs_log_reserve(struct xfs_mount *mp,
>  			  int		   length,
>  			  int		   count,
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 6a6278b8eb2d..047ac253edfe 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -866,7 +866,7 @@ xlog_cil_push_work(
>  	spin_unlock(&cil->xc_push_lock);
>  
>  	/* release the hounds! */
> -	xfs_log_release_iclog(log->l_mp, commit_iclog);
> +	xfs_log_release_iclog(commit_iclog);
>  	return;
>  
>  out_skip:
> -- 
> 2.24.1
> 

[PATCH 04/14] xfs: simplify log shutdown checking in xfs_log_release_iclog

[Christoph Hellwig]

There is no need to check for the ioerror state before the lock, as
the shutdown case is not a fast path.  Also remove the call to force
shutdown the file system, as it must have been shut down already
for an iclog to be in the ioerror state.  Also clean up the flow of
the function a bit.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 17ba92b115ea..7af9c292540b 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -602,24 +602,16 @@ xfs_log_release_iclog(
 	struct xlog_in_core	*iclog)
 {
 	struct xlog		*log = iclog->ic_log;
-	bool			sync;
-
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
-		goto error;
+	bool			sync = false;
 
 	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
-		if (iclog->ic_state == XLOG_STATE_IOERROR) {
-			spin_unlock(&log->l_icloglock);
-			goto error;
-		}
-		sync = __xlog_state_release_iclog(log, iclog);
+		if (iclog->ic_state != XLOG_STATE_IOERROR)
+			sync = __xlog_state_release_iclog(log, iclog);
 		spin_unlock(&log->l_icloglock);
-		if (sync)
-			xlog_sync(log, iclog);
 	}
-	return;
-error:
-	xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
+
+	if (sync)
+		xlog_sync(log, iclog);
 }
 
 /*
-- 
2.24.1

Re: [PATCH 04/14] xfs: simplify log shutdown checking in xfs_log_release_iclog

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:23PM +0100, Christoph Hellwig wrote:
> There is no need to check for the ioerror state before the lock, as
> the shutdown case is not a fast path.  Also remove the call to force
> shutdown the file system, as it must have been shut down already
> for an iclog to be in the ioerror state.  Also clean up the flow of
> the function a bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 17ba92b115ea..7af9c292540b 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -602,24 +602,16 @@ xfs_log_release_iclog(
>  	struct xlog_in_core	*iclog)
>  {
>  	struct xlog		*log = iclog->ic_log;
> -	bool			sync;
> -
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		goto error;
> +	bool			sync = false;
>  
>  	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
> -		if (iclog->ic_state == XLOG_STATE_IOERROR) {
> -			spin_unlock(&log->l_icloglock);
> -			goto error;
> -		}
> -		sync = __xlog_state_release_iclog(log, iclog);
> +		if (iclog->ic_state != XLOG_STATE_IOERROR)
> +			sync = __xlog_state_release_iclog(log, iclog);
>  		spin_unlock(&log->l_icloglock);
> -		if (sync)
> -			xlog_sync(log, iclog);
>  	}
> -	return;
> -error:
> -	xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
> +
> +	if (sync)
> +		xlog_sync(log, iclog);
>  }
>  
>  /*
> -- 
> 2.24.1
> 

Re: [PATCH 04/14] xfs: simplify log shutdown checking in xfs_log_release_iclog

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:23PM +0100, Christoph Hellwig wrote:
> There is no need to check for the ioerror state before the lock, as
> the shutdown case is not a fast path.  Also remove the call to force
> shutdown the file system, as it must have been shut down already
> for an iclog to be in the ioerror state.  Also clean up the flow of
> the function a bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 17ba92b115ea..7af9c292540b 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -602,24 +602,16 @@ xfs_log_release_iclog(
>  	struct xlog_in_core	*iclog)
>  {
>  	struct xlog		*log = iclog->ic_log;
> -	bool			sync;
> -
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> -		goto error;
> +	bool			sync = false;
>  
>  	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
> -		if (iclog->ic_state == XLOG_STATE_IOERROR) {
> -			spin_unlock(&log->l_icloglock);
> -			goto error;
> -		}
> -		sync = __xlog_state_release_iclog(log, iclog);
> +		if (iclog->ic_state != XLOG_STATE_IOERROR)
> +			sync = __xlog_state_release_iclog(log, iclog);
>  		spin_unlock(&log->l_icloglock);
> -		if (sync)
> -			xlog_sync(log, iclog);
>  	}
> -	return;
> -error:
> -	xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
> +
> +	if (sync)
> +		xlog_sync(log, iclog);
>  }
>  
>  /*
> -- 
> 2.24.1
> 

[PATCH 05/14] xfs: remove the aborted parameter to xlog_state_done_syncing

[Christoph Hellwig]

We can just check for a shut down log all the way down in
xlog_cil_committed instead of passing the parameter.  This means a
slight behavior change in that we now also abort log items if the
shutdown came in halfway into the I/O completion processing, which
actually is the right thing to do.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c     | 48 +++++++++++++++-----------------------------
 fs/xfs/xfs_log.h     |  2 +-
 fs/xfs/xfs_log_cil.c | 12 +++++------
 3 files changed, 23 insertions(+), 39 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 7af9c292540b..8ede2852f104 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -47,8 +47,7 @@ xlog_dealloc_log(
 
 /* local state machine functions */
 STATIC void xlog_state_done_syncing(
-	struct xlog_in_core	*iclog,
-	bool			aborted);
+	struct xlog_in_core	*iclog);
 STATIC int
 xlog_state_get_iclog_space(
 	struct xlog		*log,
@@ -1254,7 +1253,6 @@ xlog_ioend_work(
 	struct xlog_in_core     *iclog =
 		container_of(work, struct xlog_in_core, ic_end_io_work);
 	struct xlog		*log = iclog->ic_log;
-	bool			aborted = false;
 	int			error;
 
 	error = blk_status_to_errno(iclog->ic_bio.bi_status);
@@ -1270,17 +1268,9 @@ xlog_ioend_work(
 	if (XFS_TEST_ERROR(error, log->l_mp, XFS_ERRTAG_IODONE_IOERR)) {
 		xfs_alert(log->l_mp, "log I/O error %d", error);
 		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
-		/*
-		 * This flag will be propagated to the trans-committed
-		 * callback routines to let them know that the log-commit
-		 * didn't succeed.
-		 */
-		aborted = true;
-	} else if (iclog->ic_state == XLOG_STATE_IOERROR) {
-		aborted = true;
 	}
 
-	xlog_state_done_syncing(iclog, aborted);
+	xlog_state_done_syncing(iclog);
 	bio_uninit(&iclog->ic_bio);
 
 	/*
@@ -1759,7 +1749,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, true);
+		xlog_state_done_syncing(iclog);
 		up(&iclog->ic_sema);
 		return;
 	}
@@ -2783,8 +2773,7 @@ xlog_state_iodone_process_iclog(
 static void
 xlog_state_do_iclog_callbacks(
 	struct xlog		*log,
-	struct xlog_in_core	*iclog,
-	bool			aborted)
+	struct xlog_in_core	*iclog)
 		__releases(&log->l_icloglock)
 		__acquires(&log->l_icloglock)
 {
@@ -2796,7 +2785,7 @@ xlog_state_do_iclog_callbacks(
 		list_splice_init(&iclog->ic_callbacks, &tmp);
 
 		spin_unlock(&iclog->ic_callback_lock);
-		xlog_cil_process_committed(&tmp, aborted);
+		xlog_cil_process_committed(&tmp);
 		spin_lock(&iclog->ic_callback_lock);
 	}
 
@@ -2811,8 +2800,7 @@ xlog_state_do_iclog_callbacks(
 
 STATIC void
 xlog_state_do_callback(
-	struct xlog		*log,
-	bool			aborted)
+	struct xlog		*log)
 {
 	struct xlog_in_core	*iclog;
 	struct xlog_in_core	*first_iclog;
@@ -2853,7 +2841,7 @@ xlog_state_do_callback(
 			 * we'll have to run at least one more complete loop.
 			 */
 			cycled_icloglock = true;
-			xlog_state_do_iclog_callbacks(log, iclog, aborted);
+			xlog_state_do_iclog_callbacks(log, iclog);
 
 			xlog_state_clean_iclog(log, iclog);
 			iclog = iclog->ic_next;
@@ -2891,25 +2879,22 @@ xlog_state_do_callback(
  */
 STATIC void
 xlog_state_done_syncing(
-	struct xlog_in_core	*iclog,
-	bool			aborted)
+	struct xlog_in_core	*iclog)
 {
 	struct xlog		*log = iclog->ic_log;
 
 	spin_lock(&log->l_icloglock);
-
 	ASSERT(atomic_read(&iclog->ic_refcnt) == 0);
 
 	/*
 	 * If we got an error, either on the first buffer, or in the case of
-	 * split log writes, on the second, we mark ALL iclogs STATE_IOERROR,
-	 * and none should ever be attempted to be written to disk
-	 * again.
+	 * split log writes, on the second, we shut down the file system and
+	 * none should ever be attempted to be written to disk again.
 	 */
-	if (iclog->ic_state == XLOG_STATE_SYNCING)
+	if (!XLOG_FORCED_SHUTDOWN(log)) {
+		ASSERT(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
@@ -2918,9 +2903,8 @@ xlog_state_done_syncing(
 	 */
 	wake_up_all(&iclog->ic_write_wait);
 	spin_unlock(&log->l_icloglock);
-	xlog_state_do_callback(log, aborted);	/* also cleans log */
-}	/* xlog_state_done_syncing */
-
+	xlog_state_do_callback(log);	/* also cleans log */
+}
 
 /*
  * If the head of the in-core log ring is not (ACTIVE or DIRTY), then we must
@@ -3884,7 +3868,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);
+	xlog_state_do_callback(log);
 
 	/* return non-zero if log IOERROR transition had already happened */
 	return retval;
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index b38602216c5a..cc77cc36560a 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -137,7 +137,7 @@ void	  xfs_log_ticket_put(struct xlog_ticket *ticket);
 
 void	xfs_log_commit_cil(struct xfs_mount *mp, struct xfs_trans *tp,
 				xfs_lsn_t *commit_lsn, bool regrant);
-void	xlog_cil_process_committed(struct list_head *list, bool aborted);
+void	xlog_cil_process_committed(struct list_head *list);
 bool	xfs_log_item_in_current_chkpt(struct xfs_log_item *lip);
 
 void	xfs_log_work_queue(struct xfs_mount *mp);
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 047ac253edfe..adc4af336c97 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -574,10 +574,10 @@ xlog_discard_busy_extents(
  */
 static void
 xlog_cil_committed(
-	struct xfs_cil_ctx	*ctx,
-	bool			abort)
+	struct xfs_cil_ctx	*ctx)
 {
 	struct xfs_mount	*mp = ctx->cil->xc_log->l_mp;
+	bool			abort = XLOG_FORCED_SHUTDOWN(ctx->cil->xc_log);
 
 	/*
 	 * If the I/O failed, we're aborting the commit and already shutdown.
@@ -613,15 +613,14 @@ xlog_cil_committed(
 
 void
 xlog_cil_process_committed(
-	struct list_head	*list,
-	bool			aborted)
+	struct list_head	*list)
 {
 	struct xfs_cil_ctx	*ctx;
 
 	while ((ctx = list_first_entry_or_null(list,
 			struct xfs_cil_ctx, iclog_entry))) {
 		list_del(&ctx->iclog_entry);
-		xlog_cil_committed(ctx, aborted);
+		xlog_cil_committed(ctx);
 	}
 }
 
@@ -878,7 +877,8 @@ xlog_cil_push_work(
 out_abort_free_ticket:
 	xfs_log_ticket_put(tic);
 out_abort:
-	xlog_cil_committed(ctx, true);
+	ASSERT(XLOG_FORCED_SHUTDOWN(log));
+	xlog_cil_committed(ctx);
 }
 
 /*
-- 
2.24.1

Re: [PATCH 05/14] xfs: remove the aborted parameter to xlog_state_done_syncing

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:24PM +0100, Christoph Hellwig wrote:
> We can just check for a shut down log all the way down in
> xlog_cil_committed instead of passing the parameter.  This means a
> slight behavior change in that we now also abort log items if the
> shutdown came in halfway into the I/O completion processing, which
> actually is the right thing to do.

"if the shutdown came in halfway into the I/O completion..."

Does this refer to a shutdown triggered by some other thread?  As in:
this thread is processing log IO completion; meanwhile someone on the
front end cancels a dirty transaction and causes the fs/log to shut
down; and so this thread now tells all the log items that they were
aborted?  Whereas before, the only reason we'd tell the log items that
they were aborted is if the IO completion itself signalled some kind of
error?

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c     | 48 +++++++++++++++-----------------------------
>  fs/xfs/xfs_log.h     |  2 +-
>  fs/xfs/xfs_log_cil.c | 12 +++++------
>  3 files changed, 23 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 7af9c292540b..8ede2852f104 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -47,8 +47,7 @@ xlog_dealloc_log(
>  
>  /* local state machine functions */
>  STATIC void xlog_state_done_syncing(
> -	struct xlog_in_core	*iclog,
> -	bool			aborted);
> +	struct xlog_in_core	*iclog);
>  STATIC int
>  xlog_state_get_iclog_space(
>  	struct xlog		*log,
> @@ -1254,7 +1253,6 @@ xlog_ioend_work(
>  	struct xlog_in_core     *iclog =
>  		container_of(work, struct xlog_in_core, ic_end_io_work);
>  	struct xlog		*log = iclog->ic_log;
> -	bool			aborted = false;
>  	int			error;
>  
>  	error = blk_status_to_errno(iclog->ic_bio.bi_status);
> @@ -1270,17 +1268,9 @@ xlog_ioend_work(
>  	if (XFS_TEST_ERROR(error, log->l_mp, XFS_ERRTAG_IODONE_IOERR)) {
>  		xfs_alert(log->l_mp, "log I/O error %d", error);
>  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
> -		/*
> -		 * This flag will be propagated to the trans-committed
> -		 * callback routines to let them know that the log-commit
> -		 * didn't succeed.
> -		 */
> -		aborted = true;
> -	} else if (iclog->ic_state == XLOG_STATE_IOERROR) {
> -		aborted = true;
>  	}
>  
> -	xlog_state_done_syncing(iclog, aborted);
> +	xlog_state_done_syncing(iclog);
>  	bio_uninit(&iclog->ic_bio);
>  
>  	/*
> @@ -1759,7 +1749,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, true);
> +		xlog_state_done_syncing(iclog);
>  		up(&iclog->ic_sema);
>  		return;
>  	}
> @@ -2783,8 +2773,7 @@ xlog_state_iodone_process_iclog(
>  static void
>  xlog_state_do_iclog_callbacks(
>  	struct xlog		*log,
> -	struct xlog_in_core	*iclog,
> -	bool			aborted)
> +	struct xlog_in_core	*iclog)
>  		__releases(&log->l_icloglock)
>  		__acquires(&log->l_icloglock)
>  {
> @@ -2796,7 +2785,7 @@ xlog_state_do_iclog_callbacks(
>  		list_splice_init(&iclog->ic_callbacks, &tmp);
>  
>  		spin_unlock(&iclog->ic_callback_lock);
> -		xlog_cil_process_committed(&tmp, aborted);
> +		xlog_cil_process_committed(&tmp);
>  		spin_lock(&iclog->ic_callback_lock);
>  	}
>  
> @@ -2811,8 +2800,7 @@ xlog_state_do_iclog_callbacks(
>  
>  STATIC void
>  xlog_state_do_callback(
> -	struct xlog		*log,
> -	bool			aborted)
> +	struct xlog		*log)
>  {
>  	struct xlog_in_core	*iclog;
>  	struct xlog_in_core	*first_iclog;
> @@ -2853,7 +2841,7 @@ xlog_state_do_callback(
>  			 * we'll have to run at least one more complete loop.
>  			 */
>  			cycled_icloglock = true;
> -			xlog_state_do_iclog_callbacks(log, iclog, aborted);
> +			xlog_state_do_iclog_callbacks(log, iclog);
>  
>  			xlog_state_clean_iclog(log, iclog);
>  			iclog = iclog->ic_next;
> @@ -2891,25 +2879,22 @@ xlog_state_do_callback(
>   */
>  STATIC void
>  xlog_state_done_syncing(
> -	struct xlog_in_core	*iclog,
> -	bool			aborted)
> +	struct xlog_in_core	*iclog)
>  {
>  	struct xlog		*log = iclog->ic_log;
>  
>  	spin_lock(&log->l_icloglock);
> -
>  	ASSERT(atomic_read(&iclog->ic_refcnt) == 0);
>  
>  	/*
>  	 * If we got an error, either on the first buffer, or in the case of
> -	 * split log writes, on the second, we mark ALL iclogs STATE_IOERROR,
> -	 * and none should ever be attempted to be written to disk
> -	 * again.
> +	 * split log writes, on the second, we shut down the file system and
> +	 * none should ever be attempted to be written to disk again.
>  	 */
> -	if (iclog->ic_state == XLOG_STATE_SYNCING)
> +	if (!XLOG_FORCED_SHUTDOWN(log)) {
> +		ASSERT(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
> @@ -2918,9 +2903,8 @@ xlog_state_done_syncing(
>  	 */
>  	wake_up_all(&iclog->ic_write_wait);
>  	spin_unlock(&log->l_icloglock);
> -	xlog_state_do_callback(log, aborted);	/* also cleans log */
> -}	/* xlog_state_done_syncing */
> -
> +	xlog_state_do_callback(log);	/* also cleans log */
> +}
>  
>  /*
>   * If the head of the in-core log ring is not (ACTIVE or DIRTY), then we must
> @@ -3884,7 +3868,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);
> +	xlog_state_do_callback(log);
>  
>  	/* return non-zero if log IOERROR transition had already happened */
>  	return retval;
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index b38602216c5a..cc77cc36560a 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -137,7 +137,7 @@ void	  xfs_log_ticket_put(struct xlog_ticket *ticket);
>  
>  void	xfs_log_commit_cil(struct xfs_mount *mp, struct xfs_trans *tp,
>  				xfs_lsn_t *commit_lsn, bool regrant);
> -void	xlog_cil_process_committed(struct list_head *list, bool aborted);
> +void	xlog_cil_process_committed(struct list_head *list);
>  bool	xfs_log_item_in_current_chkpt(struct xfs_log_item *lip);
>  
>  void	xfs_log_work_queue(struct xfs_mount *mp);
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 047ac253edfe..adc4af336c97 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -574,10 +574,10 @@ xlog_discard_busy_extents(
>   */
>  static void
>  xlog_cil_committed(
> -	struct xfs_cil_ctx	*ctx,
> -	bool			abort)
> +	struct xfs_cil_ctx	*ctx)
>  {
>  	struct xfs_mount	*mp = ctx->cil->xc_log->l_mp;
> +	bool			abort = XLOG_FORCED_SHUTDOWN(ctx->cil->xc_log);
>  
>  	/*
>  	 * If the I/O failed, we're aborting the commit and already shutdown.
> @@ -613,15 +613,14 @@ xlog_cil_committed(
>  
>  void
>  xlog_cil_process_committed(
> -	struct list_head	*list,
> -	bool			aborted)
> +	struct list_head	*list)
>  {
>  	struct xfs_cil_ctx	*ctx;
>  
>  	while ((ctx = list_first_entry_or_null(list,
>  			struct xfs_cil_ctx, iclog_entry))) {
>  		list_del(&ctx->iclog_entry);
> -		xlog_cil_committed(ctx, aborted);
> +		xlog_cil_committed(ctx);
>  	}
>  }
>  
> @@ -878,7 +877,8 @@ xlog_cil_push_work(
>  out_abort_free_ticket:
>  	xfs_log_ticket_put(tic);
>  out_abort:
> -	xlog_cil_committed(ctx, true);
> +	ASSERT(XLOG_FORCED_SHUTDOWN(log));
> +	xlog_cil_committed(ctx);
>  }
>  
>  /*
> -- 
> 2.24.1
> 

Re: [PATCH 05/14] xfs: remove the aborted parameter to xlog_state_done_syncing

[Christoph Hellwig]

On Mon, Mar 16, 2020 at 01:50:41PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 16, 2020 at 03:42:24PM +0100, Christoph Hellwig wrote:
> > We can just check for a shut down log all the way down in
> > xlog_cil_committed instead of passing the parameter.  This means a
> > slight behavior change in that we now also abort log items if the
> > shutdown came in halfway into the I/O completion processing, which
> > actually is the right thing to do.
> 
> "if the shutdown came in halfway into the I/O completion..."
> 
> Does this refer to a shutdown triggered by some other thread?  As in:
> this thread is processing log IO completion; meanwhile someone on the
> front end cancels a dirty transaction and causes the fs/log to shut
> down; and so this thread now tells all the log items that they were
> aborted?  Whereas before, the only reason we'd tell the log items that
> they were aborted is if the IO completion itself signalled some kind of
> error?

No, before we also checked for XLOG_STATE_IOERROR in
ẋlog_ioend_work.  But the I/O end processing drops l_icloglock in
various places, in which case another thread could shut down the log
as well.

Re: [PATCH 05/14] xfs: remove the aborted parameter to xlog_state_done_syncing

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:24PM +0100, Christoph Hellwig wrote:
> We can just check for a shut down log all the way down in
> xlog_cil_committed instead of passing the parameter.  This means a
> slight behavior change in that we now also abort log items if the
> shutdown came in halfway into the I/O completion processing, which
> actually is the right thing to do.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c     | 48 +++++++++++++++-----------------------------
>  fs/xfs/xfs_log.h     |  2 +-
>  fs/xfs/xfs_log_cil.c | 12 +++++------
>  3 files changed, 23 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 7af9c292540b..8ede2852f104 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
...
> @@ -2891,25 +2879,22 @@ xlog_state_do_callback(
>   */
>  STATIC void
>  xlog_state_done_syncing(
> -	struct xlog_in_core	*iclog,
> -	bool			aborted)
> +	struct xlog_in_core	*iclog)
>  {
>  	struct xlog		*log = iclog->ic_log;
>  
>  	spin_lock(&log->l_icloglock);
> -
>  	ASSERT(atomic_read(&iclog->ic_refcnt) == 0);
>  
>  	/*
>  	 * If we got an error, either on the first buffer, or in the case of
> -	 * split log writes, on the second, we mark ALL iclogs STATE_IOERROR,
> -	 * and none should ever be attempted to be written to disk
> -	 * again.
> +	 * split log writes, on the second, we shut down the file system and
> +	 * none should ever be attempted to be written to disk again.

"none" refers to iclogs in the original comment, which has been removed.
This could probably read "... and iclogs should never ..." instead.

With that fixed:

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

>  	 */
> -	if (iclog->ic_state == XLOG_STATE_SYNCING)
> +	if (!XLOG_FORCED_SHUTDOWN(log)) {
> +		ASSERT(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
> @@ -2918,9 +2903,8 @@ xlog_state_done_syncing(
>  	 */
>  	wake_up_all(&iclog->ic_write_wait);
>  	spin_unlock(&log->l_icloglock);
> -	xlog_state_do_callback(log, aborted);	/* also cleans log */
> -}	/* xlog_state_done_syncing */
> -
> +	xlog_state_do_callback(log);	/* also cleans log */
> +}
>  
>  /*
>   * If the head of the in-core log ring is not (ACTIVE or DIRTY), then we must
> @@ -3884,7 +3868,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);
> +	xlog_state_do_callback(log);
>  
>  	/* return non-zero if log IOERROR transition had already happened */
>  	return retval;
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index b38602216c5a..cc77cc36560a 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -137,7 +137,7 @@ void	  xfs_log_ticket_put(struct xlog_ticket *ticket);
>  
>  void	xfs_log_commit_cil(struct xfs_mount *mp, struct xfs_trans *tp,
>  				xfs_lsn_t *commit_lsn, bool regrant);
> -void	xlog_cil_process_committed(struct list_head *list, bool aborted);
> +void	xlog_cil_process_committed(struct list_head *list);
>  bool	xfs_log_item_in_current_chkpt(struct xfs_log_item *lip);
>  
>  void	xfs_log_work_queue(struct xfs_mount *mp);
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 047ac253edfe..adc4af336c97 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -574,10 +574,10 @@ xlog_discard_busy_extents(
>   */
>  static void
>  xlog_cil_committed(
> -	struct xfs_cil_ctx	*ctx,
> -	bool			abort)
> +	struct xfs_cil_ctx	*ctx)
>  {
>  	struct xfs_mount	*mp = ctx->cil->xc_log->l_mp;
> +	bool			abort = XLOG_FORCED_SHUTDOWN(ctx->cil->xc_log);
>  
>  	/*
>  	 * If the I/O failed, we're aborting the commit and already shutdown.
> @@ -613,15 +613,14 @@ xlog_cil_committed(
>  
>  void
>  xlog_cil_process_committed(
> -	struct list_head	*list,
> -	bool			aborted)
> +	struct list_head	*list)
>  {
>  	struct xfs_cil_ctx	*ctx;
>  
>  	while ((ctx = list_first_entry_or_null(list,
>  			struct xfs_cil_ctx, iclog_entry))) {
>  		list_del(&ctx->iclog_entry);
> -		xlog_cil_committed(ctx, aborted);
> +		xlog_cil_committed(ctx);
>  	}
>  }
>  
> @@ -878,7 +877,8 @@ xlog_cil_push_work(
>  out_abort_free_ticket:
>  	xfs_log_ticket_put(tic);
>  out_abort:
> -	xlog_cil_committed(ctx, true);
> +	ASSERT(XLOG_FORCED_SHUTDOWN(log));
> +	xlog_cil_committed(ctx);
>  }
>  
>  /*
> -- 
> 2.24.1
> 

[PATCH 06/14] xfs: refactor xlog_state_clean_iclog

[Christoph Hellwig]

Factor out a few self-container helper from xlog_state_clean_iclog, and
update the documentation so it primarily documents why things happens
instead of how.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 8ede2852f104..23979d08a2a3 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2540,112 +2540,106 @@ xlog_write(
  *****************************************************************************
  */
 
+static void
+xlog_state_activate_iclog(
+	struct xlog_in_core	*iclog,
+	int			*iclogs_changed)
+{
+	ASSERT(list_empty_careful(&iclog->ic_callbacks));
+
+	/*
+	 * If the number of ops in this iclog indicate it just contains the
+	 * dummy transaction, we can change state into IDLE (the second time
+	 * around). Otherwise we should change the state into NEED a dummy.
+	 * We don't need to cover the dummy.
+	 */
+	if (*iclogs_changed == 0 &&
+	    iclog->ic_header.h_num_logops == cpu_to_be32(XLOG_COVER_OPS)) {
+		*iclogs_changed = 1;
+	} else {
+		/*
+		 * We have two dirty iclogs so start over.  This could also be
+		 * num of ops indicating this is not the dummy going out.
+		 */
+		*iclogs_changed = 2;
+	}
+
+	iclog->ic_state	= XLOG_STATE_ACTIVE;
+	iclog->ic_offset = 0;
+	iclog->ic_header.h_num_logops = 0;
+	memset(iclog->ic_header.h_cycle_data, 0,
+		sizeof(iclog->ic_header.h_cycle_data));
+	iclog->ic_header.h_lsn = 0;
+}
+
 /*
- * An iclog has just finished IO completion processing, so we need to update
- * the iclog state and propagate that up into the overall log state. Hence we
- * prepare the iclog for cleaning, and then clean all the pending dirty iclogs
- * starting from the head, and then wake up any threads that are waiting for the
- * iclog to be marked clean.
- *
- * The ordering of marking iclogs ACTIVE must be maintained, so an iclog
- * doesn't become ACTIVE beyond one that is SYNCING.  This is also required to
- * maintain the notion that we use a ordered wait queue to hold off would be
- * writers to the log when every iclog is trying to sync to disk.
- *
- * Caller must hold the icloglock before calling us.
- *
- * State Change: !IOERROR -> DIRTY -> ACTIVE
+ * Loop through all iclogs and mark all iclogs currently marked DIRTY as
+ * ACTIVE after iclog I/O has completed.
  */
-STATIC void
-xlog_state_clean_iclog(
+static void
+xlog_state_activate_iclogs(
 	struct xlog		*log,
-	struct xlog_in_core	*dirty_iclog)
+	int			*iclogs_changed)
 {
-	struct xlog_in_core	*iclog;
-	int			changed = 0;
-
-	/* Prepare the completed iclog. */
-	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
-		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
+	struct xlog_in_core	*iclog = log->l_iclog;
 
-	/* Walk all the iclogs to update the ordered active state. */
-	iclog = log->l_iclog;
 	do {
-		if (iclog->ic_state == XLOG_STATE_DIRTY) {
-			iclog->ic_state	= XLOG_STATE_ACTIVE;
-			iclog->ic_offset       = 0;
-			ASSERT(list_empty_careful(&iclog->ic_callbacks));
-			/*
-			 * If the number of ops in this iclog indicate it just
-			 * contains the dummy transaction, we can
-			 * change state into IDLE (the second time around).
-			 * Otherwise we should change the state into
-			 * NEED a dummy.
-			 * We don't need to cover the dummy.
-			 */
-			if (!changed &&
-			   (be32_to_cpu(iclog->ic_header.h_num_logops) ==
-			   		XLOG_COVER_OPS)) {
-				changed = 1;
-			} else {
-				/*
-				 * We have two dirty iclogs so start over
-				 * This could also be num of ops indicates
-				 * this is not the dummy going out.
-				 */
-				changed = 2;
-			}
-			iclog->ic_header.h_num_logops = 0;
-			memset(iclog->ic_header.h_cycle_data, 0,
-			      sizeof(iclog->ic_header.h_cycle_data));
-			iclog->ic_header.h_lsn = 0;
-		} else if (iclog->ic_state == XLOG_STATE_ACTIVE)
-			/* do nothing */;
-		else
-			break;	/* stop cleaning */
-		iclog = iclog->ic_next;
-	} while (iclog != log->l_iclog);
-
+		if (iclog->ic_state == XLOG_STATE_DIRTY)
+			xlog_state_activate_iclog(iclog, iclogs_changed);
+		/*
+		 * The ordering of marking iclogs ACTIVE must be maintained, so
+		 * an iclog doesn't become ACTIVE beyond one that is SYNCING.
+		 */
+		else if (iclog->ic_state != XLOG_STATE_ACTIVE)
+			break;
+	} while ((iclog = iclog->ic_next) != log->l_iclog);
+}
 
+static int
+xlog_covered_state(
+	struct xlog		*log,
+	int			iclogs_changed)
+{
 	/*
-	 * Wake up threads waiting in xfs_log_force() for the dirty iclog
-	 * to be cleaned.
+	 * We usually go to NEED. But we go to NEED2 if the changed indicates we
+	 * are done writing the dummy record.  If we are done with the second
+	 * dummy recored (DONE2), then we go to IDLE.
 	 */
-	wake_up_all(&dirty_iclog->ic_force_wait);
+	switch (log->l_covered_state) {
+	case XLOG_STATE_COVER_IDLE:
+	case XLOG_STATE_COVER_NEED:
+	case XLOG_STATE_COVER_NEED2:
+		break;
+	case XLOG_STATE_COVER_DONE:
+		if (iclogs_changed == 1)
+			return XLOG_STATE_COVER_NEED2;
+		break;
+	case XLOG_STATE_COVER_DONE2:
+		if (iclogs_changed == 1)
+			return XLOG_STATE_COVER_IDLE;
+		break;
+	default:
+		ASSERT(0);
+	}
 
-	/*
-	 * Change state for the dummy log recording.
-	 * We usually go to NEED. But we go to NEED2 if the changed indicates
-	 * we are done writing the dummy record.
-	 * If we are done with the second dummy recored (DONE2), then
-	 * we go to IDLE.
-	 */
-	if (changed) {
-		switch (log->l_covered_state) {
-		case XLOG_STATE_COVER_IDLE:
-		case XLOG_STATE_COVER_NEED:
-		case XLOG_STATE_COVER_NEED2:
-			log->l_covered_state = XLOG_STATE_COVER_NEED;
-			break;
+	return XLOG_STATE_COVER_NEED;
+}
 
-		case XLOG_STATE_COVER_DONE:
-			if (changed == 1)
-				log->l_covered_state = XLOG_STATE_COVER_NEED2;
-			else
-				log->l_covered_state = XLOG_STATE_COVER_NEED;
-			break;
+STATIC void
+xlog_state_clean_iclog(
+	struct xlog		*log,
+	struct xlog_in_core	*dirty_iclog)
+{
+	int			iclogs_changed = 0;
 
-		case XLOG_STATE_COVER_DONE2:
-			if (changed == 1)
-				log->l_covered_state = XLOG_STATE_COVER_IDLE;
-			else
-				log->l_covered_state = XLOG_STATE_COVER_NEED;
-			break;
+	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
+		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
 
-		default:
-			ASSERT(0);
-		}
-	}
+	xlog_state_activate_iclogs(log, &iclogs_changed);
+	wake_up_all(&dirty_iclog->ic_force_wait);
+
+	if (iclogs_changed)
+		log->l_covered_state = xlog_covered_state(log, iclogs_changed);
 }
 
 STATIC xfs_lsn_t
-- 
2.24.1

Re: [PATCH 06/14] xfs: refactor xlog_state_clean_iclog

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:25PM +0100, Christoph Hellwig wrote:
> Factor out a few self-container helper from xlog_state_clean_iclog, and

"self-contained" ?

> update the documentation so it primarily documents why things happens
> instead of how.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok otherwise, I think I saw where all the pieces landed :)
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c | 180 +++++++++++++++++++++++------------------------
>  1 file changed, 87 insertions(+), 93 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 8ede2852f104..23979d08a2a3 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2540,112 +2540,106 @@ xlog_write(
>   *****************************************************************************
>   */
>  
> +static void
> +xlog_state_activate_iclog(
> +	struct xlog_in_core	*iclog,
> +	int			*iclogs_changed)
> +{
> +	ASSERT(list_empty_careful(&iclog->ic_callbacks));
> +
> +	/*
> +	 * If the number of ops in this iclog indicate it just contains the
> +	 * dummy transaction, we can change state into IDLE (the second time
> +	 * around). Otherwise we should change the state into NEED a dummy.
> +	 * We don't need to cover the dummy.
> +	 */
> +	if (*iclogs_changed == 0 &&
> +	    iclog->ic_header.h_num_logops == cpu_to_be32(XLOG_COVER_OPS)) {
> +		*iclogs_changed = 1;
> +	} else {
> +		/*
> +		 * We have two dirty iclogs so start over.  This could also be
> +		 * num of ops indicating this is not the dummy going out.
> +		 */
> +		*iclogs_changed = 2;
> +	}
> +
> +	iclog->ic_state	= XLOG_STATE_ACTIVE;
> +	iclog->ic_offset = 0;
> +	iclog->ic_header.h_num_logops = 0;
> +	memset(iclog->ic_header.h_cycle_data, 0,
> +		sizeof(iclog->ic_header.h_cycle_data));
> +	iclog->ic_header.h_lsn = 0;
> +}
> +
>  /*
> - * An iclog has just finished IO completion processing, so we need to update
> - * the iclog state and propagate that up into the overall log state. Hence we
> - * prepare the iclog for cleaning, and then clean all the pending dirty iclogs
> - * starting from the head, and then wake up any threads that are waiting for the
> - * iclog to be marked clean.
> - *
> - * The ordering of marking iclogs ACTIVE must be maintained, so an iclog
> - * doesn't become ACTIVE beyond one that is SYNCING.  This is also required to
> - * maintain the notion that we use a ordered wait queue to hold off would be
> - * writers to the log when every iclog is trying to sync to disk.
> - *
> - * Caller must hold the icloglock before calling us.
> - *
> - * State Change: !IOERROR -> DIRTY -> ACTIVE
> + * Loop through all iclogs and mark all iclogs currently marked DIRTY as
> + * ACTIVE after iclog I/O has completed.
>   */
> -STATIC void
> -xlog_state_clean_iclog(
> +static void
> +xlog_state_activate_iclogs(
>  	struct xlog		*log,
> -	struct xlog_in_core	*dirty_iclog)
> +	int			*iclogs_changed)
>  {
> -	struct xlog_in_core	*iclog;
> -	int			changed = 0;
> -
> -	/* Prepare the completed iclog. */
> -	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
> -		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
> +	struct xlog_in_core	*iclog = log->l_iclog;
>  
> -	/* Walk all the iclogs to update the ordered active state. */
> -	iclog = log->l_iclog;
>  	do {
> -		if (iclog->ic_state == XLOG_STATE_DIRTY) {
> -			iclog->ic_state	= XLOG_STATE_ACTIVE;
> -			iclog->ic_offset       = 0;
> -			ASSERT(list_empty_careful(&iclog->ic_callbacks));
> -			/*
> -			 * If the number of ops in this iclog indicate it just
> -			 * contains the dummy transaction, we can
> -			 * change state into IDLE (the second time around).
> -			 * Otherwise we should change the state into
> -			 * NEED a dummy.
> -			 * We don't need to cover the dummy.
> -			 */
> -			if (!changed &&
> -			   (be32_to_cpu(iclog->ic_header.h_num_logops) ==
> -			   		XLOG_COVER_OPS)) {
> -				changed = 1;
> -			} else {
> -				/*
> -				 * We have two dirty iclogs so start over
> -				 * This could also be num of ops indicates
> -				 * this is not the dummy going out.
> -				 */
> -				changed = 2;
> -			}
> -			iclog->ic_header.h_num_logops = 0;
> -			memset(iclog->ic_header.h_cycle_data, 0,
> -			      sizeof(iclog->ic_header.h_cycle_data));
> -			iclog->ic_header.h_lsn = 0;
> -		} else if (iclog->ic_state == XLOG_STATE_ACTIVE)
> -			/* do nothing */;
> -		else
> -			break;	/* stop cleaning */
> -		iclog = iclog->ic_next;
> -	} while (iclog != log->l_iclog);
> -
> +		if (iclog->ic_state == XLOG_STATE_DIRTY)
> +			xlog_state_activate_iclog(iclog, iclogs_changed);
> +		/*
> +		 * The ordering of marking iclogs ACTIVE must be maintained, so
> +		 * an iclog doesn't become ACTIVE beyond one that is SYNCING.
> +		 */
> +		else if (iclog->ic_state != XLOG_STATE_ACTIVE)
> +			break;
> +	} while ((iclog = iclog->ic_next) != log->l_iclog);
> +}
>  
> +static int
> +xlog_covered_state(
> +	struct xlog		*log,
> +	int			iclogs_changed)
> +{
>  	/*
> -	 * Wake up threads waiting in xfs_log_force() for the dirty iclog
> -	 * to be cleaned.
> +	 * We usually go to NEED. But we go to NEED2 if the changed indicates we
> +	 * are done writing the dummy record.  If we are done with the second
> +	 * dummy recored (DONE2), then we go to IDLE.
>  	 */
> -	wake_up_all(&dirty_iclog->ic_force_wait);
> +	switch (log->l_covered_state) {
> +	case XLOG_STATE_COVER_IDLE:
> +	case XLOG_STATE_COVER_NEED:
> +	case XLOG_STATE_COVER_NEED2:
> +		break;
> +	case XLOG_STATE_COVER_DONE:
> +		if (iclogs_changed == 1)
> +			return XLOG_STATE_COVER_NEED2;
> +		break;
> +	case XLOG_STATE_COVER_DONE2:
> +		if (iclogs_changed == 1)
> +			return XLOG_STATE_COVER_IDLE;
> +		break;
> +	default:
> +		ASSERT(0);
> +	}
>  
> -	/*
> -	 * Change state for the dummy log recording.
> -	 * We usually go to NEED. But we go to NEED2 if the changed indicates
> -	 * we are done writing the dummy record.
> -	 * If we are done with the second dummy recored (DONE2), then
> -	 * we go to IDLE.
> -	 */
> -	if (changed) {
> -		switch (log->l_covered_state) {
> -		case XLOG_STATE_COVER_IDLE:
> -		case XLOG_STATE_COVER_NEED:
> -		case XLOG_STATE_COVER_NEED2:
> -			log->l_covered_state = XLOG_STATE_COVER_NEED;
> -			break;
> +	return XLOG_STATE_COVER_NEED;
> +}
>  
> -		case XLOG_STATE_COVER_DONE:
> -			if (changed == 1)
> -				log->l_covered_state = XLOG_STATE_COVER_NEED2;
> -			else
> -				log->l_covered_state = XLOG_STATE_COVER_NEED;
> -			break;
> +STATIC void
> +xlog_state_clean_iclog(
> +	struct xlog		*log,
> +	struct xlog_in_core	*dirty_iclog)
> +{
> +	int			iclogs_changed = 0;
>  
> -		case XLOG_STATE_COVER_DONE2:
> -			if (changed == 1)
> -				log->l_covered_state = XLOG_STATE_COVER_IDLE;
> -			else
> -				log->l_covered_state = XLOG_STATE_COVER_NEED;
> -			break;
> +	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
> +		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>  
> -		default:
> -			ASSERT(0);
> -		}
> -	}
> +	xlog_state_activate_iclogs(log, &iclogs_changed);
> +	wake_up_all(&dirty_iclog->ic_force_wait);
> +
> +	if (iclogs_changed)
> +		log->l_covered_state = xlog_covered_state(log, iclogs_changed);
>  }
>  
>  STATIC xfs_lsn_t
> -- 
> 2.24.1
> 

Re: [PATCH 06/14] xfs: refactor xlog_state_clean_iclog

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:25PM +0100, Christoph Hellwig wrote:
> Factor out a few self-container helper from xlog_state_clean_iclog, and
> update the documentation so it primarily documents why things happens
> instead of how.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 180 +++++++++++++++++++++++------------------------
>  1 file changed, 87 insertions(+), 93 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 8ede2852f104..23979d08a2a3 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2540,112 +2540,106 @@ xlog_write(
...
> +static int
> +xlog_covered_state(
> +	struct xlog		*log,
> +	int			iclogs_changed)
> +{
>  	/*
> -	 * Wake up threads waiting in xfs_log_force() for the dirty iclog
> -	 * to be cleaned.
> +	 * We usually go to NEED. But we go to NEED2 if the changed indicates we
> +	 * are done writing the dummy record.  If we are done with the second
> +	 * dummy recored (DONE2), then we go to IDLE.
>  	 */
> -	wake_up_all(&dirty_iclog->ic_force_wait);
> +	switch (log->l_covered_state) {
> +	case XLOG_STATE_COVER_IDLE:
> +	case XLOG_STATE_COVER_NEED:
> +	case XLOG_STATE_COVER_NEED2:
> +		break;
> +	case XLOG_STATE_COVER_DONE:
> +		if (iclogs_changed == 1)
> +			return XLOG_STATE_COVER_NEED2;
> +		break;
> +	case XLOG_STATE_COVER_DONE2:
> +		if (iclogs_changed == 1)
> +			return XLOG_STATE_COVER_IDLE;
> +		break;
> +	default:
> +		ASSERT(0);
> +	}

The code looks mostly fine, but I'm not a fan of this factoring where we
deref ->l_covered_state here and return a value only for the caller to
assign it to ->l_covered_state again. Can we just let this function
assign ->l_covered_state itself (i.e. assign a local variable rather than
return within the switch)?

Brian

>  
> -	/*
> -	 * Change state for the dummy log recording.
> -	 * We usually go to NEED. But we go to NEED2 if the changed indicates
> -	 * we are done writing the dummy record.
> -	 * If we are done with the second dummy recored (DONE2), then
> -	 * we go to IDLE.
> -	 */
> -	if (changed) {
> -		switch (log->l_covered_state) {
> -		case XLOG_STATE_COVER_IDLE:
> -		case XLOG_STATE_COVER_NEED:
> -		case XLOG_STATE_COVER_NEED2:
> -			log->l_covered_state = XLOG_STATE_COVER_NEED;
> -			break;
> +	return XLOG_STATE_COVER_NEED;
> +}
>  
> -		case XLOG_STATE_COVER_DONE:
> -			if (changed == 1)
> -				log->l_covered_state = XLOG_STATE_COVER_NEED2;
> -			else
> -				log->l_covered_state = XLOG_STATE_COVER_NEED;
> -			break;
> +STATIC void
> +xlog_state_clean_iclog(
> +	struct xlog		*log,
> +	struct xlog_in_core	*dirty_iclog)
> +{
> +	int			iclogs_changed = 0;
>  
> -		case XLOG_STATE_COVER_DONE2:
> -			if (changed == 1)
> -				log->l_covered_state = XLOG_STATE_COVER_IDLE;
> -			else
> -				log->l_covered_state = XLOG_STATE_COVER_NEED;
> -			break;
> +	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
> +		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>  
> -		default:
> -			ASSERT(0);
> -		}
> -	}
> +	xlog_state_activate_iclogs(log, &iclogs_changed);
> +	wake_up_all(&dirty_iclog->ic_force_wait);
> +
> +	if (iclogs_changed)
> +		log->l_covered_state = xlog_covered_state(log, iclogs_changed);
>  }
>  
>  STATIC xfs_lsn_t
> -- 
> 2.24.1
> 

Re: [PATCH 06/14] xfs: refactor xlog_state_clean_iclog

[Christoph Hellwig]

On Tue, Mar 17, 2020 at 09:25:05AM -0400, Brian Foster wrote:
> The code looks mostly fine, but I'm not a fan of this factoring where we
> deref ->l_covered_state here and return a value only for the caller to
> assign it to ->l_covered_state again. Can we just let this function
> assign ->l_covered_state itself (i.e. assign a local variable rather than
> return within the switch)?

I did that earlier, but this version looked easier to understand to me.
I can change it if there is a strong preference.

[PATCH 07/14] xfs: move the ioerror check out of xlog_state_clean_iclog

[Christoph Hellwig]

Use the shutdown flag in the log to bypass xlog_state_clean_iclog
entirely in case of a shut down log.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 23979d08a2a3..c490c5b0d8b7 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2632,8 +2632,7 @@ xlog_state_clean_iclog(
 {
 	int			iclogs_changed = 0;
 
-	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
-		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
+	dirty_iclog->ic_state = XLOG_STATE_DIRTY;
 
 	xlog_state_activate_iclogs(log, &iclogs_changed);
 	wake_up_all(&dirty_iclog->ic_force_wait);
@@ -2836,8 +2835,10 @@ xlog_state_do_callback(
 			 */
 			cycled_icloglock = true;
 			xlog_state_do_iclog_callbacks(log, iclog);
-
-			xlog_state_clean_iclog(log, iclog);
+			if (XLOG_FORCED_SHUTDOWN(log))
+				wake_up_all(&iclog->ic_force_wait);
+			else
+				xlog_state_clean_iclog(log, iclog);
 			iclog = iclog->ic_next;
 		} while (first_iclog != iclog);
 
-- 
2.24.1

Re: [PATCH 07/14] xfs: move the ioerror check out of xlog_state_clean_iclog

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:26PM +0100, Christoph Hellwig wrote:
> Use the shutdown flag in the log to bypass xlog_state_clean_iclog
> entirely in case of a shut down log.
> 
> 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 | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 23979d08a2a3..c490c5b0d8b7 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2632,8 +2632,7 @@ xlog_state_clean_iclog(
>  {
>  	int			iclogs_changed = 0;
>  
> -	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
> -		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
> +	dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>  
>  	xlog_state_activate_iclogs(log, &iclogs_changed);
>  	wake_up_all(&dirty_iclog->ic_force_wait);
> @@ -2836,8 +2835,10 @@ xlog_state_do_callback(
>  			 */
>  			cycled_icloglock = true;
>  			xlog_state_do_iclog_callbacks(log, iclog);
> -
> -			xlog_state_clean_iclog(log, iclog);
> +			if (XLOG_FORCED_SHUTDOWN(log))
> +				wake_up_all(&iclog->ic_force_wait);
> +			else
> +				xlog_state_clean_iclog(log, iclog);
>  			iclog = iclog->ic_next;
>  		} while (first_iclog != iclog);
>  
> -- 
> 2.24.1
> 

Re: [PATCH 07/14] xfs: move the ioerror check out of xlog_state_clean_iclog

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:26PM +0100, Christoph Hellwig wrote:
> Use the shutdown flag in the log to bypass xlog_state_clean_iclog
> entirely in case of a shut down log.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 23979d08a2a3..c490c5b0d8b7 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2632,8 +2632,7 @@ xlog_state_clean_iclog(
>  {
>  	int			iclogs_changed = 0;
>  
> -	if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
> -		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
> +	dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>  
>  	xlog_state_activate_iclogs(log, &iclogs_changed);
>  	wake_up_all(&dirty_iclog->ic_force_wait);
> @@ -2836,8 +2835,10 @@ xlog_state_do_callback(
>  			 */
>  			cycled_icloglock = true;
>  			xlog_state_do_iclog_callbacks(log, iclog);
> -
> -			xlog_state_clean_iclog(log, iclog);
> +			if (XLOG_FORCED_SHUTDOWN(log))
> +				wake_up_all(&iclog->ic_force_wait);
> +			else
> +				xlog_state_clean_iclog(log, iclog);
>  			iclog = iclog->ic_next;
>  		} while (first_iclog != iclog);
>  
> -- 
> 2.24.1
> 

[PATCH 08/14] xfs: move xlog_state_do_iclog_callbacks up

[Christoph Hellwig]

Move xlog_state_do_iclog_callbacks a little up, to avoid the need for a
forward declaration with upcoming changes.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index c490c5b0d8b7..c534d7007aa3 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2701,6 +2701,43 @@ xlog_state_set_callback(
 	xlog_grant_push_ail(log, 0);
 }
 
+/*
+ * Keep processing entries in the iclog callback list until we come around and
+ * it is empty.  We need to atomically see that the list is empty and change the
+ * state to DIRTY so that we don't miss any more callbacks being added.
+ *
+ * This function is called with the icloglock held and returns with it held. We
+ * drop it while running callbacks, however, as holding it over thousands of
+ * callbacks is unnecessary and causes excessive contention if we do.
+ */
+static void
+xlog_state_do_iclog_callbacks(
+	struct xlog		*log,
+	struct xlog_in_core	*iclog)
+		__releases(&log->l_icloglock)
+		__acquires(&log->l_icloglock)
+{
+	spin_unlock(&log->l_icloglock);
+	spin_lock(&iclog->ic_callback_lock);
+	while (!list_empty(&iclog->ic_callbacks)) {
+		LIST_HEAD(tmp);
+
+		list_splice_init(&iclog->ic_callbacks, &tmp);
+
+		spin_unlock(&iclog->ic_callback_lock);
+		xlog_cil_process_committed(&tmp);
+		spin_lock(&iclog->ic_callback_lock);
+	}
+
+	/*
+	 * Pick up the icloglock while still holding the callback lock so we
+	 * serialise against anyone trying to add more callbacks to this iclog
+	 * now we've finished processing.
+	 */
+	spin_lock(&log->l_icloglock);
+	spin_unlock(&iclog->ic_callback_lock);
+}
+
 /*
  * Return true if we need to stop processing, false to continue to the next
  * iclog. The caller will need to run callbacks if the iclog is returned in the
@@ -2754,43 +2791,6 @@ xlog_state_iodone_process_iclog(
 	}
 }
 
-/*
- * Keep processing entries in the iclog callback list until we come around and
- * it is empty.  We need to atomically see that the list is empty and change the
- * state to DIRTY so that we don't miss any more callbacks being added.
- *
- * This function is called with the icloglock held and returns with it held. We
- * drop it while running callbacks, however, as holding it over thousands of
- * callbacks is unnecessary and causes excessive contention if we do.
- */
-static void
-xlog_state_do_iclog_callbacks(
-	struct xlog		*log,
-	struct xlog_in_core	*iclog)
-		__releases(&log->l_icloglock)
-		__acquires(&log->l_icloglock)
-{
-	spin_unlock(&log->l_icloglock);
-	spin_lock(&iclog->ic_callback_lock);
-	while (!list_empty(&iclog->ic_callbacks)) {
-		LIST_HEAD(tmp);
-
-		list_splice_init(&iclog->ic_callbacks, &tmp);
-
-		spin_unlock(&iclog->ic_callback_lock);
-		xlog_cil_process_committed(&tmp);
-		spin_lock(&iclog->ic_callback_lock);
-	}
-
-	/*
-	 * Pick up the icloglock while still holding the callback lock so we
-	 * serialise against anyone trying to add more callbacks to this iclog
-	 * now we've finished processing.
-	 */
-	spin_lock(&log->l_icloglock);
-	spin_unlock(&iclog->ic_callback_lock);
-}
-
 STATIC void
 xlog_state_do_callback(
 	struct xlog		*log)
-- 
2.24.1

Re: [PATCH 08/14] xfs: move xlog_state_do_iclog_callbacks up

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:27PM +0100, Christoph Hellwig wrote:
> Move xlog_state_do_iclog_callbacks a little up, to avoid the need for a
> forward declaration with upcoming changes.
> 
> 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 | 74 ++++++++++++++++++++++++------------------------
>  1 file changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index c490c5b0d8b7..c534d7007aa3 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2701,6 +2701,43 @@ xlog_state_set_callback(
>  	xlog_grant_push_ail(log, 0);
>  }
>  
> +/*
> + * Keep processing entries in the iclog callback list until we come around and
> + * it is empty.  We need to atomically see that the list is empty and change the
> + * state to DIRTY so that we don't miss any more callbacks being added.
> + *
> + * This function is called with the icloglock held and returns with it held. We
> + * drop it while running callbacks, however, as holding it over thousands of
> + * callbacks is unnecessary and causes excessive contention if we do.
> + */
> +static void
> +xlog_state_do_iclog_callbacks(
> +	struct xlog		*log,
> +	struct xlog_in_core	*iclog)
> +		__releases(&log->l_icloglock)
> +		__acquires(&log->l_icloglock)
> +{
> +	spin_unlock(&log->l_icloglock);
> +	spin_lock(&iclog->ic_callback_lock);
> +	while (!list_empty(&iclog->ic_callbacks)) {
> +		LIST_HEAD(tmp);
> +
> +		list_splice_init(&iclog->ic_callbacks, &tmp);
> +
> +		spin_unlock(&iclog->ic_callback_lock);
> +		xlog_cil_process_committed(&tmp);
> +		spin_lock(&iclog->ic_callback_lock);
> +	}
> +
> +	/*
> +	 * Pick up the icloglock while still holding the callback lock so we
> +	 * serialise against anyone trying to add more callbacks to this iclog
> +	 * now we've finished processing.
> +	 */
> +	spin_lock(&log->l_icloglock);
> +	spin_unlock(&iclog->ic_callback_lock);
> +}
> +
>  /*
>   * Return true if we need to stop processing, false to continue to the next
>   * iclog. The caller will need to run callbacks if the iclog is returned in the
> @@ -2754,43 +2791,6 @@ xlog_state_iodone_process_iclog(
>  	}
>  }
>  
> -/*
> - * Keep processing entries in the iclog callback list until we come around and
> - * it is empty.  We need to atomically see that the list is empty and change the
> - * state to DIRTY so that we don't miss any more callbacks being added.
> - *
> - * This function is called with the icloglock held and returns with it held. We
> - * drop it while running callbacks, however, as holding it over thousands of
> - * callbacks is unnecessary and causes excessive contention if we do.
> - */
> -static void
> -xlog_state_do_iclog_callbacks(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog)
> -		__releases(&log->l_icloglock)
> -		__acquires(&log->l_icloglock)
> -{
> -	spin_unlock(&log->l_icloglock);
> -	spin_lock(&iclog->ic_callback_lock);
> -	while (!list_empty(&iclog->ic_callbacks)) {
> -		LIST_HEAD(tmp);
> -
> -		list_splice_init(&iclog->ic_callbacks, &tmp);
> -
> -		spin_unlock(&iclog->ic_callback_lock);
> -		xlog_cil_process_committed(&tmp);
> -		spin_lock(&iclog->ic_callback_lock);
> -	}
> -
> -	/*
> -	 * Pick up the icloglock while still holding the callback lock so we
> -	 * serialise against anyone trying to add more callbacks to this iclog
> -	 * now we've finished processing.
> -	 */
> -	spin_lock(&log->l_icloglock);
> -	spin_unlock(&iclog->ic_callback_lock);
> -}
> -
>  STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log)
> -- 
> 2.24.1
> 

Re: [PATCH 08/14] xfs: move xlog_state_do_iclog_callbacks up

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:27PM +0100, Christoph Hellwig wrote:
> Move xlog_state_do_iclog_callbacks a little up, to avoid the need for a
> forward declaration with upcoming changes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 74 ++++++++++++++++++++++++------------------------
>  1 file changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index c490c5b0d8b7..c534d7007aa3 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2701,6 +2701,43 @@ xlog_state_set_callback(
>  	xlog_grant_push_ail(log, 0);
>  }
>  
> +/*
> + * Keep processing entries in the iclog callback list until we come around and
> + * it is empty.  We need to atomically see that the list is empty and change the
> + * state to DIRTY so that we don't miss any more callbacks being added.
> + *
> + * This function is called with the icloglock held and returns with it held. We
> + * drop it while running callbacks, however, as holding it over thousands of
> + * callbacks is unnecessary and causes excessive contention if we do.
> + */
> +static void
> +xlog_state_do_iclog_callbacks(
> +	struct xlog		*log,
> +	struct xlog_in_core	*iclog)
> +		__releases(&log->l_icloglock)
> +		__acquires(&log->l_icloglock)
> +{
> +	spin_unlock(&log->l_icloglock);
> +	spin_lock(&iclog->ic_callback_lock);
> +	while (!list_empty(&iclog->ic_callbacks)) {
> +		LIST_HEAD(tmp);
> +
> +		list_splice_init(&iclog->ic_callbacks, &tmp);
> +
> +		spin_unlock(&iclog->ic_callback_lock);
> +		xlog_cil_process_committed(&tmp);
> +		spin_lock(&iclog->ic_callback_lock);
> +	}
> +
> +	/*
> +	 * Pick up the icloglock while still holding the callback lock so we
> +	 * serialise against anyone trying to add more callbacks to this iclog
> +	 * now we've finished processing.
> +	 */
> +	spin_lock(&log->l_icloglock);
> +	spin_unlock(&iclog->ic_callback_lock);
> +}
> +
>  /*
>   * Return true if we need to stop processing, false to continue to the next
>   * iclog. The caller will need to run callbacks if the iclog is returned in the
> @@ -2754,43 +2791,6 @@ xlog_state_iodone_process_iclog(
>  	}
>  }
>  
> -/*
> - * Keep processing entries in the iclog callback list until we come around and
> - * it is empty.  We need to atomically see that the list is empty and change the
> - * state to DIRTY so that we don't miss any more callbacks being added.
> - *
> - * This function is called with the icloglock held and returns with it held. We
> - * drop it while running callbacks, however, as holding it over thousands of
> - * callbacks is unnecessary and causes excessive contention if we do.
> - */
> -static void
> -xlog_state_do_iclog_callbacks(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog)
> -		__releases(&log->l_icloglock)
> -		__acquires(&log->l_icloglock)
> -{
> -	spin_unlock(&log->l_icloglock);
> -	spin_lock(&iclog->ic_callback_lock);
> -	while (!list_empty(&iclog->ic_callbacks)) {
> -		LIST_HEAD(tmp);
> -
> -		list_splice_init(&iclog->ic_callbacks, &tmp);
> -
> -		spin_unlock(&iclog->ic_callback_lock);
> -		xlog_cil_process_committed(&tmp);
> -		spin_lock(&iclog->ic_callback_lock);
> -	}
> -
> -	/*
> -	 * Pick up the icloglock while still holding the callback lock so we
> -	 * serialise against anyone trying to add more callbacks to this iclog
> -	 * now we've finished processing.
> -	 */
> -	spin_lock(&log->l_icloglock);
> -	spin_unlock(&iclog->ic_callback_lock);
> -}
> -
>  STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log)
> -- 
> 2.24.1
> 

[PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog

[Christoph Hellwig]

Move handling of a shut down log out of xlog_state_iodone_process_iclog
and into xlog_state_do_callback so that it can be moved into an entirely
separate branch.  While doing so switch to using XLOG_FORCED_SHUTDOWN to
check the shutdown condition global to the log instead of the per-iclog
flag, and make sure the comments match reality.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index c534d7007aa3..4efaa248a03d 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2746,8 +2746,7 @@ xlog_state_do_iclog_callbacks(
 static bool
 xlog_state_iodone_process_iclog(
 	struct xlog		*log,
-	struct xlog_in_core	*iclog,
-	bool			*ioerror)
+	struct xlog_in_core	*iclog)
 {
 	xfs_lsn_t		lowest_lsn;
 	xfs_lsn_t		header_lsn;
@@ -2759,15 +2758,6 @@ xlog_state_iodone_process_iclog(
 		 * Skip all iclogs in the ACTIVE & DIRTY states:
 		 */
 		return false;
-	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;
 	case XLOG_STATE_DONE_SYNC:
 		/*
 		 * Now that we have an iclog that is in the DONE_SYNC state, do
@@ -2795,39 +2785,41 @@ STATIC void
 xlog_state_do_callback(
 	struct xlog		*log)
 {
-	struct xlog_in_core	*iclog;
-	struct xlog_in_core	*first_iclog;
 	bool			cycled_icloglock;
-	bool			ioerror;
 	int			flushcnt = 0;
 	int			repeats = 0;
 
+	/*
+	 * Scan all iclogs starting with the one pointed to by the log.  Reset
+	 * this starting point each time the log is unlocked (during callbacks).
+	 *
+	 * Keep looping through iclogs until one full pass is made without
+	 * running any callbacks.
+	 *
+	 * If the log has been shut down, still perform the callbacks once per
+	 * iclog to abort all log items, but don't bother to restart the loop
+	 * after dropping the log as no new callbacks can show up.
+	 */
 	spin_lock(&log->l_icloglock);
 	do {
-		/*
-		 * Scan all iclogs starting with the one pointed to by the
-		 * log.  Reset this starting point each time the log is
-		 * unlocked (during callbacks).
-		 *
-		 * Keep looping through iclogs until one full pass is made
-		 * without running any callbacks.
-		 */
-		first_iclog = log->l_iclog;
-		iclog = log->l_iclog;
+		struct xlog_in_core	*first_iclog = log->l_iclog;
+		struct xlog_in_core	*iclog = first_iclog;
+
 		cycled_icloglock = false;
-		ioerror = false;
 		repeats++;
 
 		do {
-			if (xlog_state_iodone_process_iclog(log, iclog,
-							&ioerror))
+			if (XLOG_FORCED_SHUTDOWN(log)) {
+				xlog_state_do_iclog_callbacks(log, iclog);
+				wake_up_all(&iclog->ic_force_wait);
+				continue;
+			}
+
+			if (xlog_state_iodone_process_iclog(log, iclog))
 				break;
 
-			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
-			    iclog->ic_state != XLOG_STATE_IOERROR) {
-				iclog = iclog->ic_next;
+			if (iclog->ic_state != XLOG_STATE_CALLBACK)
 				continue;
-			}
 
 			/*
 			 * Running callbacks will drop the icloglock which means
@@ -2835,12 +2827,8 @@ xlog_state_do_callback(
 			 */
 			cycled_icloglock = true;
 			xlog_state_do_iclog_callbacks(log, iclog);
-			if (XLOG_FORCED_SHUTDOWN(log))
-				wake_up_all(&iclog->ic_force_wait);
-			else
-				xlog_state_clean_iclog(log, iclog);
-			iclog = iclog->ic_next;
-		} while (first_iclog != iclog);
+			xlog_state_clean_iclog(log, iclog);
+		} while ((iclog = iclog->ic_next) != first_iclog);
 
 		if (repeats > 5000) {
 			flushcnt += repeats;
@@ -2849,7 +2837,7 @@ xlog_state_do_callback(
 				"%s: possible infinite loop (%d iterations)",
 				__func__, flushcnt);
 		}
-	} while (!ioerror && cycled_icloglock);
+	} while (cycled_icloglock);
 
 	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
 	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
-- 
2.24.1

Re: [PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:28PM +0100, Christoph Hellwig wrote:
> Move handling of a shut down log out of xlog_state_iodone_process_iclog
> and into xlog_state_do_callback so that it can be moved into an entirely
> separate branch.  While doing so switch to using XLOG_FORCED_SHUTDOWN to
> check the shutdown condition global to the log instead of the per-iclog
> flag, and make sure the comments match reality.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log.c | 64 ++++++++++++++++++++----------------------------
>  1 file changed, 26 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index c534d7007aa3..4efaa248a03d 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2746,8 +2746,7 @@ xlog_state_do_iclog_callbacks(
>  static bool
>  xlog_state_iodone_process_iclog(
>  	struct xlog		*log,
> -	struct xlog_in_core	*iclog,
> -	bool			*ioerror)
> +	struct xlog_in_core	*iclog)
>  {
>  	xfs_lsn_t		lowest_lsn;
>  	xfs_lsn_t		header_lsn;
> @@ -2759,15 +2758,6 @@ xlog_state_iodone_process_iclog(
>  		 * Skip all iclogs in the ACTIVE & DIRTY states:
>  		 */
>  		return false;
> -	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;
>  	case XLOG_STATE_DONE_SYNC:
>  		/*
>  		 * Now that we have an iclog that is in the DONE_SYNC state, do
> @@ -2795,39 +2785,41 @@ STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log)
>  {
> -	struct xlog_in_core	*iclog;
> -	struct xlog_in_core	*first_iclog;
>  	bool			cycled_icloglock;
> -	bool			ioerror;
>  	int			flushcnt = 0;
>  	int			repeats = 0;
>  
> +	/*
> +	 * Scan all iclogs starting with the one pointed to by the log.  Reset
> +	 * this starting point each time the log is unlocked (during callbacks).
> +	 *
> +	 * Keep looping through iclogs until one full pass is made without
> +	 * running any callbacks.
> +	 *
> +	 * If the log has been shut down, still perform the callbacks once per
> +	 * iclog to abort all log items, but don't bother to restart the loop
> +	 * after dropping the log as no new callbacks can show up.
> +	 */
>  	spin_lock(&log->l_icloglock);
>  	do {
> -		/*
> -		 * Scan all iclogs starting with the one pointed to by the
> -		 * log.  Reset this starting point each time the log is
> -		 * unlocked (during callbacks).
> -		 *
> -		 * Keep looping through iclogs until one full pass is made
> -		 * without running any callbacks.
> -		 */
> -		first_iclog = log->l_iclog;
> -		iclog = log->l_iclog;
> +		struct xlog_in_core	*first_iclog = log->l_iclog;
> +		struct xlog_in_core	*iclog = first_iclog;
> +
>  		cycled_icloglock = false;
> -		ioerror = false;
>  		repeats++;
>  
>  		do {
> -			if (xlog_state_iodone_process_iclog(log, iclog,
> -							&ioerror))
> +			if (XLOG_FORCED_SHUTDOWN(log)) {
> +				xlog_state_do_iclog_callbacks(log, iclog);
> +				wake_up_all(&iclog->ic_force_wait);
> +				continue;
> +			}
> +
> +			if (xlog_state_iodone_process_iclog(log, iclog))
>  				break;
>  
> -			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
> -			    iclog->ic_state != XLOG_STATE_IOERROR) {
> -				iclog = iclog->ic_next;
> +			if (iclog->ic_state != XLOG_STATE_CALLBACK)
>  				continue;
> -			}
>  
>  			/*
>  			 * Running callbacks will drop the icloglock which means
> @@ -2835,12 +2827,8 @@ xlog_state_do_callback(
>  			 */
>  			cycled_icloglock = true;
>  			xlog_state_do_iclog_callbacks(log, iclog);
> -			if (XLOG_FORCED_SHUTDOWN(log))
> -				wake_up_all(&iclog->ic_force_wait);
> -			else
> -				xlog_state_clean_iclog(log, iclog);
> -			iclog = iclog->ic_next;
> -		} while (first_iclog != iclog);
> +			xlog_state_clean_iclog(log, iclog);
> +		} while ((iclog = iclog->ic_next) != first_iclog);
>  
>  		if (repeats > 5000) {
>  			flushcnt += repeats;
> @@ -2849,7 +2837,7 @@ xlog_state_do_callback(
>  				"%s: possible infinite loop (%d iterations)",
>  				__func__, flushcnt);
>  		}
> -	} while (!ioerror && cycled_icloglock);
> +	} while (cycled_icloglock);
>  
>  	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
>  	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
> -- 
> 2.24.1
> 

Re: [PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:28PM +0100, Christoph Hellwig wrote:
> Move handling of a shut down log out of xlog_state_iodone_process_iclog
> and into xlog_state_do_callback so that it can be moved into an entirely
> separate branch.  While doing so switch to using XLOG_FORCED_SHUTDOWN to
> check the shutdown condition global to the log instead of the per-iclog
> flag, and make sure the comments match reality.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 64 ++++++++++++++++++++----------------------------
>  1 file changed, 26 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index c534d7007aa3..4efaa248a03d 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
...
> @@ -2795,39 +2785,41 @@ STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log)
>  {
> -	struct xlog_in_core	*iclog;
> -	struct xlog_in_core	*first_iclog;
>  	bool			cycled_icloglock;
> -	bool			ioerror;
>  	int			flushcnt = 0;
>  	int			repeats = 0;
>  
> +	/*
> +	 * Scan all iclogs starting with the one pointed to by the log.  Reset
> +	 * this starting point each time the log is unlocked (during callbacks).
> +	 *
> +	 * Keep looping through iclogs until one full pass is made without
> +	 * running any callbacks.
> +	 *
> +	 * If the log has been shut down, still perform the callbacks once per
> +	 * iclog to abort all log items, but don't bother to restart the loop
> +	 * after dropping the log as no new callbacks can show up.
> +	 */

"after dropping the lock ..."

>  	spin_lock(&log->l_icloglock);
>  	do {
> -		/*
> -		 * Scan all iclogs starting with the one pointed to by the
> -		 * log.  Reset this starting point each time the log is
> -		 * unlocked (during callbacks).
> -		 *
> -		 * Keep looping through iclogs until one full pass is made
> -		 * without running any callbacks.
> -		 */
> -		first_iclog = log->l_iclog;
> -		iclog = log->l_iclog;
> +		struct xlog_in_core	*first_iclog = log->l_iclog;
> +		struct xlog_in_core	*iclog = first_iclog;
> +
>  		cycled_icloglock = false;
> -		ioerror = false;
>  		repeats++;
>  
>  		do {
> -			if (xlog_state_iodone_process_iclog(log, iclog,
> -							&ioerror))
> +			if (XLOG_FORCED_SHUTDOWN(log)) {
> +				xlog_state_do_iclog_callbacks(log, iclog);
> +				wake_up_all(&iclog->ic_force_wait);
> +				continue;
> +			}
> +

Why do we need to change the flow here? The current code looks like it
proceeds with the _do_iclog_callbacks() call below..

As it is, I also don't think this reflects the comment above because if
we catch the shutdown partway through a loop, the outer loop will
execute one more time through. That doesn't look like a problem at a
glance, but I think we should try to retain closer to existing behavior
by folding the shutdown check into the ic_state check below as well as
the outer loop conditional.

This (and the next patch) also raises the issue of whether to maintain
state validity once the iclog ioerror state goes away. Currently we see
the IOERROR state and kind of have free reign on busting through the
normal runtime logic to clear out callbacks, etc. on the iclog
regardless of what the pre-error state was. It certainly makes sense to
continue to do that based on XLOG_FORCED_SHUTDOWN(), but the iclog state
sort of provides a platform that allows us to do that because any
particular context can see it and handle an iclog with care. With
IOERROR replaced with the (potentially racy) log flag check, I think we
should try to maintain the coherence of other states wherever possible.
IOW, XLOG_FORCED_SHUTDOWN() means we can run callbacks and abort and
whatnot, but we should probably still consider and update the iclog
state as we progress (as opposed to leaving it in the DONE_SYNC state,
for example) because there's no guarantee some other context will
(always) behave just as it did with IOERROR.

Brian

> +			if (xlog_state_iodone_process_iclog(log, iclog))
>  				break;
>  
> -			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
> -			    iclog->ic_state != XLOG_STATE_IOERROR) {
> -				iclog = iclog->ic_next;
> +			if (iclog->ic_state != XLOG_STATE_CALLBACK)
>  				continue;
> -			}
>  
>  			/*
>  			 * Running callbacks will drop the icloglock which means
> @@ -2835,12 +2827,8 @@ xlog_state_do_callback(
>  			 */
>  			cycled_icloglock = true;
>  			xlog_state_do_iclog_callbacks(log, iclog);
> -			if (XLOG_FORCED_SHUTDOWN(log))
> -				wake_up_all(&iclog->ic_force_wait);
> -			else
> -				xlog_state_clean_iclog(log, iclog);
> -			iclog = iclog->ic_next;
> -		} while (first_iclog != iclog);
> +			xlog_state_clean_iclog(log, iclog);
> +		} while ((iclog = iclog->ic_next) != first_iclog);
>  
>  		if (repeats > 5000) {
>  			flushcnt += repeats;
> @@ -2849,7 +2837,7 @@ xlog_state_do_callback(
>  				"%s: possible infinite loop (%d iterations)",
>  				__func__, flushcnt);
>  		}
> -	} while (!ioerror && cycled_icloglock);
> +	} while (cycled_icloglock);
>  
>  	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
>  	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
> -- 
> 2.24.1
> 

Re: [PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog

[Christoph Hellwig]

On Wed, Mar 18, 2020 at 10:48:25AM -0400, Brian Foster wrote:
> >  		do {
> > -			if (xlog_state_iodone_process_iclog(log, iclog,
> > -							&ioerror))
> > +			if (XLOG_FORCED_SHUTDOWN(log)) {
> > +				xlog_state_do_iclog_callbacks(log, iclog);
> > +				wake_up_all(&iclog->ic_force_wait);
> > +				continue;
> > +			}
> > +
> 
> Why do we need to change the flow here? The current code looks like it
> proceeds with the _do_iclog_callbacks() call below..
>
> As it is, I also don't think this reflects the comment above because if
> we catch the shutdown partway through a loop, the outer loop will
> execute one more time through. That doesn't look like a problem at a
> glance, but I think we should try to retain closer to existing behavior
> by folding the shutdown check into the ic_state check below as well as
> the outer loop conditional.

True.  I think we just need to clear cycled_icloglock in the
shutdown branch.  I prefer that flow over falling through to the
main loop body as that clearly separates out the shutdown case.

> This (and the next patch) also raises the issue of whether to maintain
> state validity once the iclog ioerror state goes away. Currently we see
> the IOERROR state and kind of have free reign on busting through the
> normal runtime logic to clear out callbacks, etc. on the iclog
> regardless of what the pre-error state was. It certainly makes sense to
> continue to do that based on XLOG_FORCED_SHUTDOWN(), but the iclog state
> sort of provides a platform that allows us to do that because any
> particular context can see it and handle an iclog with care. With
> IOERROR replaced with the (potentially racy) log flag check, I think we
> should try to maintain the coherence of other states wherever possible.
> IOW, XLOG_FORCED_SHUTDOWN() means we can run callbacks and abort and
> whatnot, but we should probably still consider and update the iclog
> state as we progress (as opposed to leaving it in the DONE_SYNC state,
> for example) because there's no guarantee some other context will
> (always) behave just as it did with IOERROR.

I actually very much disagree with that, and this series moves into
the other direction.  We only really changes the states when
writing to iclogs, syncing them to disk, and I/O completion.  And
all the paths just error out at a high level when the log is shut
down, so there is no need to move the state along.  Faking state
changes when they don't correspond to the actual I/O just seems like
a really bad idea.

Also if you look at what state checks are left, the are all (except
for the debug check in xfs_log_unmount_verify_iclog) under
l_icloglock and guarded by a shutdown check.

Re: [PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog

[Brian Foster]

On Wed, Mar 18, 2020 at 05:34:29PM +0100, Christoph Hellwig wrote:
> On Wed, Mar 18, 2020 at 10:48:25AM -0400, Brian Foster wrote:
> > >  		do {
> > > -			if (xlog_state_iodone_process_iclog(log, iclog,
> > > -							&ioerror))
> > > +			if (XLOG_FORCED_SHUTDOWN(log)) {
> > > +				xlog_state_do_iclog_callbacks(log, iclog);
> > > +				wake_up_all(&iclog->ic_force_wait);
> > > +				continue;
> > > +			}
> > > +
> > 
> > Why do we need to change the flow here? The current code looks like it
> > proceeds with the _do_iclog_callbacks() call below..
> >
> > As it is, I also don't think this reflects the comment above because if
> > we catch the shutdown partway through a loop, the outer loop will
> > execute one more time through. That doesn't look like a problem at a
> > glance, but I think we should try to retain closer to existing behavior
> > by folding the shutdown check into the ic_state check below as well as
> > the outer loop conditional.
> 
> True.  I think we just need to clear cycled_icloglock in the
> shutdown branch.  I prefer that flow over falling through to the
> main loop body as that clearly separates out the shutdown case.
> 

Sure, but a shutdown can still happen at any point so this is just a
duplicate branch to maintain.

> > This (and the next patch) also raises the issue of whether to maintain
> > state validity once the iclog ioerror state goes away. Currently we see
> > the IOERROR state and kind of have free reign on busting through the
> > normal runtime logic to clear out callbacks, etc. on the iclog
> > regardless of what the pre-error state was. It certainly makes sense to
> > continue to do that based on XLOG_FORCED_SHUTDOWN(), but the iclog state
> > sort of provides a platform that allows us to do that because any
> > particular context can see it and handle an iclog with care. With
> > IOERROR replaced with the (potentially racy) log flag check, I think we
> > should try to maintain the coherence of other states wherever possible.
> > IOW, XLOG_FORCED_SHUTDOWN() means we can run callbacks and abort and
> > whatnot, but we should probably still consider and update the iclog
> > state as we progress (as opposed to leaving it in the DONE_SYNC state,
> > for example) because there's no guarantee some other context will
> > (always) behave just as it did with IOERROR.
> 
> I actually very much disagree with that, and this series moves into
> the other direction.  We only really changes the states when
> writing to iclogs, syncing them to disk, and I/O completion.  And
> all the paths just error out at a high level when the log is shut
> down, so there is no need to move the state along.  Faking state
> changes when they don't correspond to the actual I/O just seems like
> a really bad idea.
> 

I think you're misreading me. I'm not suggesting to fake state changes.
I'd argue that's actually what the special case shutdown branch does.
And to the contrary, this patch already implements what I'm suggesting,
it's just not consistent behavior..

First, we basically already go from whatever state we're in to "logical
CALLBACK" during shutdown. This is just forcibly implemented via the
IOERROR state. With IOERROR eventually removed, this highlights things
like whether it's actually safe to make some of those arbitrary
transitions. It's actually not, because going from WANT_SYNC -> CALLBACK
is a potential use after free vector of the CIL ctx (as soon as the ctx
is added to the callback list in the CIL push code). This is yet another
functional problem that should be fixed before removing IOERROR, IMO
(and is reproducible via kasan splat, btw). At this point I think some
of these shutdown checks associated with CALLBACK are simply to ensure
IOERROR remains persistent once it's set on an iclog. We don't need to
carry that logic around if IOERROR is going away.

SYNCING -> CALLBACK is another hokey transition in the existing code,
even if it doesn't currently manifest in a bug that I can see, because
we should probably still expect (wait for) an I/O completion despite
that the filesystem had shutdown in the meantime. Fixing that one might
require tweaks to how the shutdown code actually works (i.e. waiting on
an I/O vs. running callbacks while in-flight). It's not immediately
clear to me what the best solution is for that, but I suspect it could
tie in with fixing the problem noted above.

With regard to this patch, consider that shutdown can happen at any
point and xlog_state_do_iclog_callbacks() cycles icloglock. That means
that as of this patch, we actually can go from IOERROR -> DIRTY and
possibly from DIRTY -> ACTIVE depending on where the iclog lies in the
list. Removing IOERROR will subtley change that behavior yet again to
make the latter transition potentially more likely.

Note that I think that's probably fine. What I'm suggesting is to just
drop the duplicate shutdown branch and instead lets evaluate whether
some of the code these checks intend to avoid is really problematic. I
don't think it is in the completion path since we're just resetting
in-core headers and such. That means we could probably just let the
iclogs fall through those state transitions naturally, reduce the number
of shutdown checks splattered throughout the code and simplify the
overall error handling logic in the process by handling all iclogs
consistently during shutdown.

> Also if you look at what state checks are left, the are all (except
> for the debug check in xfs_log_unmount_verify_iclog) under
> l_icloglock and guarded by a shutdown check.
> 

That's not quite enough IMO. I think the whole IOERROR problem is not
primarily a matter of mechanically factoring it out. It's fragile
functionality that should be fixed/simplified first before there's any
real value to removing IOERROR.

Brian

Re: [PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog

[Christoph Hellwig]

On Thu, Mar 19, 2020 at 07:36:03AM -0400, Brian Foster wrote:
> > True.  I think we just need to clear cycled_icloglock in the
> > shutdown branch.  I prefer that flow over falling through to the
> > main loop body as that clearly separates out the shutdown case.
> > 
> 
> Sure, but a shutdown can still happen at any point so this is just a
> duplicate branch to maintain.

I don't understand.  We are in the inner loop and under l_icloglock.
The next time a shutdown can come in is when
xlog_state_do_iclog_callbacks drops l_icloglock.  That is at the end
of the inner loop, which means we will always go back to the
force shutdown check quickly.  So how is the branch duplicate?  Yes,
it also calls xlog_state_do_iclog_callbacks and does the wakeup,
but in doing that early it avoid a whole lot of complicated logic
in the previous code base.

> I think you're misreading me. I'm not suggesting to fake state changes.
> I'd argue that's actually what the special case shutdown branch does.
> And to the contrary, this patch already implements what I'm suggesting,
> it's just not consistent behavior..

I'm rather confused now.

> First, we basically already go from whatever state we're in to "logical
> CALLBACK" during shutdown. This is just forcibly implemented via the
> IOERROR state. With IOERROR eventually removed, this highlights things
> like whether it's actually safe to make some of those arbitrary
> transitions. It's actually not, because going from WANT_SYNC -> CALLBACK
> is a potential use after free vector of the CIL ctx (as soon as the ctx
> is added to the callback list in the CIL push code). This is yet another
> functional problem that should be fixed before removing IOERROR, IMO
> (and is reproducible via kasan splat, btw). At this point I think some
> of these shutdown checks associated with CALLBACK are simply to ensure
> IOERROR remains persistent once it's set on an iclog. We don't need to
> carry that logic around if IOERROR is going away.

What shutdown check associated with CALLBACK?

> SYNCING -> CALLBACK is another hokey transition in the existing code,
> even if it doesn't currently manifest in a bug that I can see, because
> we should probably still expect (wait for) an I/O completion despite
> that the filesystem had shutdown in the meantime. Fixing that one might
> require tweaks to how the shutdown code actually works (i.e. waiting on
> an I/O vs. running callbacks while in-flight). It's not immediately
> clear to me what the best solution is for that, but I suspect it could
> tie in with fixing the problem noted above.

True, actually running callbacks on various kinds of "in-flight" iclogs
seems rather dangerous.  So should I interpret your above comments
in that we should fix that first before killing of the IOERROR state?

Re: [PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog

[Brian Foster]

On Thu, Mar 19, 2020 at 02:05:36PM +0100, Christoph Hellwig wrote:
> On Thu, Mar 19, 2020 at 07:36:03AM -0400, Brian Foster wrote:
> > > True.  I think we just need to clear cycled_icloglock in the
> > > shutdown branch.  I prefer that flow over falling through to the
> > > main loop body as that clearly separates out the shutdown case.
> > > 
> > 
> > Sure, but a shutdown can still happen at any point so this is just a
> > duplicate branch to maintain.
> 
> I don't understand.  We are in the inner loop and under l_icloglock.
> The next time a shutdown can come in is when
> xlog_state_do_iclog_callbacks drops l_icloglock.  That is at the end
> of the inner loop, which means we will always go back to the
> force shutdown check quickly.  So how is the branch duplicate?  Yes,
> it also calls xlog_state_do_iclog_callbacks and does the wakeup,
> but in doing that early it avoid a whole lot of complicated logic
> in the previous code base.
> 

We'll get back to the shutdown check for the next iclog, but not the
iclog we're running callbacks on. So basically we can be in CALLBACK, a
shutdown can set IOERROR where _do_iclog_callbacks() cycles the lock,
the callback picks up the shutdown state and aborts, and then the first
thing we do after that function returns is:

	iclog->ic_state = XLOG_STATE_DIRTY;
	xlog_state_activate_iclogs(log, &iclogs_changed);

... and thus finish the loop by reactivating the (IOERROR) iclog. So for
any particular iclog, we might process it for shutdown normally or in
the special shutdown branch based on timing.

> > I think you're misreading me. I'm not suggesting to fake state changes.
> > I'd argue that's actually what the special case shutdown branch does.
> > And to the contrary, this patch already implements what I'm suggesting,
> > it's just not consistent behavior..
> 
> I'm rather confused now.
> 

Sorry. What I'm saying can probably be simplified to the following
question: if we just removed the special shutdown branch and let the
iclogs fall through the normal completion sequence (once IOERROR is out
of the picture) during shutdown, is that actually a problem?

It seems to me it isn't (subject to testing of course). If that is true,
then that is more simple and consistent than what we seem to be doing in
this patch, which to my eyes seems to want to maintain some of the
IOERROR functional cruft even though the state itself is being removed.
Also note I think it would be reasonable to lift it out in a
later/separate patch if that was more straightforward than reworking
these patches.

If it is a problem, I think that's a potential argument for leaving the
IOERROR state around because then the state technically has meaning.

> > First, we basically already go from whatever state we're in to "logical
> > CALLBACK" during shutdown. This is just forcibly implemented via the
> > IOERROR state. With IOERROR eventually removed, this highlights things
> > like whether it's actually safe to make some of those arbitrary
> > transitions. It's actually not, because going from WANT_SYNC -> CALLBACK
> > is a potential use after free vector of the CIL ctx (as soon as the ctx
> > is added to the callback list in the CIL push code). This is yet another
> > functional problem that should be fixed before removing IOERROR, IMO
> > (and is reproducible via kasan splat, btw). At this point I think some
> > of these shutdown checks associated with CALLBACK are simply to ensure
> > IOERROR remains persistent once it's set on an iclog. We don't need to
> > carry that logic around if IOERROR is going away.
> 
> What shutdown check associated with CALLBACK?
> 

The one(s) that issue callbacks on an IOERROR iclog (note that I'm
referring to the mainline code). Specifically the IOERROR check in
_process_iclog() and the following logic in the caller:

                if (iclog->ic_state != XLOG_STATE_CALLBACK &&
                    iclog->ic_state != XLOG_STATE_IOERROR) {
                        iclog = iclog->ic_next;
                        continue;
                }

IOW, the current code treats an IOERROR iclog as if it were CALLBACK
with the sole exception of updating ic_state.

> > SYNCING -> CALLBACK is another hokey transition in the existing code,
> > even if it doesn't currently manifest in a bug that I can see, because
> > we should probably still expect (wait for) an I/O completion despite
> > that the filesystem had shutdown in the meantime. Fixing that one might
> > require tweaks to how the shutdown code actually works (i.e. waiting on
> > an I/O vs. running callbacks while in-flight). It's not immediately
> > clear to me what the best solution is for that, but I suspect it could
> > tie in with fixing the problem noted above.
> 
> True, actually running callbacks on various kinds of "in-flight" iclogs
> seems rather dangerous.  So should I interpret your above comments
> in that we should fix that first before killing of the IOERROR state?
> 

Yes. Note that the WANT_SYNC -> CALLBACK (IOERROR) behavior is
explicitly problematic in the current code as well because the submitter
context still has the ctx while the callbacks could be freeing it.
That's a reproducible use after free in the current code.

Brian

[PATCH 10/14] xfs: refactor xlog_state_iodone_process_iclog

[Christoph Hellwig]

Move all state checks into the caller to make the loop flow more clear,
and instead move the callback processing together with marking the iclog
for callbacks.

This also allows to easily indicate when we actually dropped the
icloglock instead of assuming we do so for any iclog processed.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 4efaa248a03d..a38d495b6e81 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2738,47 +2738,28 @@ xlog_state_do_iclog_callbacks(
 	spin_unlock(&iclog->ic_callback_lock);
 }
 
-/*
- * Return true if we need to stop processing, false to continue to the next
- * iclog. The caller will need to run callbacks if the iclog is returned in the
- * XLOG_STATE_CALLBACK state.
- */
 static bool
 xlog_state_iodone_process_iclog(
 	struct xlog		*log,
 	struct xlog_in_core	*iclog)
 {
-	xfs_lsn_t		lowest_lsn;
-	xfs_lsn_t		header_lsn;
+	xfs_lsn_t		header_lsn, lowest_lsn;
 
-	switch (iclog->ic_state) {
-	case XLOG_STATE_ACTIVE:
-	case XLOG_STATE_DIRTY:
-		/*
-		 * Skip all iclogs in the ACTIVE & DIRTY states:
-		 */
-		return false;
-	case XLOG_STATE_DONE_SYNC:
-		/*
-		 * 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);
-		if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
-			return false;
-		xlog_state_set_callback(log, iclog, header_lsn);
+	/*
+	 * 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);
+	if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
 		return false;
-	default:
-		/*
-		 * Can only perform callbacks in order.  Since this iclog is not
-		 * in the DONE_SYNC state, we skip the rest and just try to
-		 * clean up.
-		 */
-		return true;
-	}
+
+	xlog_state_set_callback(log, iclog, header_lsn);
+	xlog_state_do_iclog_callbacks(log, iclog);
+	xlog_state_clean_iclog(log, iclog);
+	return true;
 }
 
 STATIC void
@@ -2795,10 +2776,6 @@ xlog_state_do_callback(
 	 *
 	 * Keep looping through iclogs until one full pass is made without
 	 * running any callbacks.
-	 *
-	 * If the log has been shut down, still perform the callbacks once per
-	 * iclog to abort all log items, but don't bother to restart the loop
-	 * after dropping the log as no new callbacks can show up.
 	 */
 	spin_lock(&log->l_icloglock);
 	do {
@@ -2809,25 +2786,40 @@ xlog_state_do_callback(
 		repeats++;
 
 		do {
+			/*
+			 * If the log has been shut down, still perform the
+			 * callbacks to abort all log items to clean up any
+			 * allocate resource, but don't bother to restart the
+			 * loop after dropping the log as no new callbacks can
+			 * be attached now.
+			 */
 			if (XLOG_FORCED_SHUTDOWN(log)) {
 				xlog_state_do_iclog_callbacks(log, iclog);
 				wake_up_all(&iclog->ic_force_wait);
 				continue;
 			}
 
-			if (xlog_state_iodone_process_iclog(log, iclog))
-				break;
-
-			if (iclog->ic_state != XLOG_STATE_CALLBACK)
+			/*
+			 * Skip all iclogs in the ACTIVE & DIRTY states:
+			 */
+			if (iclog->ic_state == XLOG_STATE_ACTIVE ||
+			    iclog->ic_state == XLOG_STATE_DIRTY)
 				continue;
 
+			/*
+			 * We can only perform callbacks in order.  If this
+			 * iclog is not in the DONE_SYNC state, we skip the rest
+			 * and just try to clean up.
+			 */
+			if (iclog->ic_state != XLOG_STATE_DONE_SYNC)
+				break;
+
 			/*
 			 * Running callbacks will drop the icloglock which means
 			 * we'll have to run at least one more complete loop.
 			 */
-			cycled_icloglock = true;
-			xlog_state_do_iclog_callbacks(log, iclog);
-			xlog_state_clean_iclog(log, iclog);
+			if (xlog_state_iodone_process_iclog(log, iclog))
+				cycled_icloglock = true;
 		} while ((iclog = iclog->ic_next) != first_iclog);
 
 		if (repeats > 5000) {
-- 
2.24.1

Re: [PATCH 10/14] xfs: refactor xlog_state_iodone_process_iclog

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:29PM +0100, Christoph Hellwig wrote:
> Move all state checks into the caller to make the loop flow more clear,
> and instead move the callback processing together with marking the iclog
> for callbacks.
> 
> This also allows to easily indicate when we actually dropped the
> icloglock instead of assuming we do so for any iclog processed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Ok, I think this is a pretty straightfoward hoisting of the the state
checksk, as promised...

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

--D

> ---
>  fs/xfs/xfs_log.c | 82 ++++++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 4efaa248a03d..a38d495b6e81 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2738,47 +2738,28 @@ xlog_state_do_iclog_callbacks(
>  	spin_unlock(&iclog->ic_callback_lock);
>  }
>  
> -/*
> - * Return true if we need to stop processing, false to continue to the next
> - * iclog. The caller will need to run callbacks if the iclog is returned in the
> - * XLOG_STATE_CALLBACK state.
> - */
>  static bool
>  xlog_state_iodone_process_iclog(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog)
>  {
> -	xfs_lsn_t		lowest_lsn;
> -	xfs_lsn_t		header_lsn;
> +	xfs_lsn_t		header_lsn, lowest_lsn;
>  
> -	switch (iclog->ic_state) {
> -	case XLOG_STATE_ACTIVE:
> -	case XLOG_STATE_DIRTY:
> -		/*
> -		 * Skip all iclogs in the ACTIVE & DIRTY states:
> -		 */
> -		return false;
> -	case XLOG_STATE_DONE_SYNC:
> -		/*
> -		 * 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);
> -		if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
> -			return false;
> -		xlog_state_set_callback(log, iclog, header_lsn);
> +	/*
> +	 * 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);
> +	if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
>  		return false;
> -	default:
> -		/*
> -		 * Can only perform callbacks in order.  Since this iclog is not
> -		 * in the DONE_SYNC state, we skip the rest and just try to
> -		 * clean up.
> -		 */
> -		return true;
> -	}
> +
> +	xlog_state_set_callback(log, iclog, header_lsn);
> +	xlog_state_do_iclog_callbacks(log, iclog);
> +	xlog_state_clean_iclog(log, iclog);
> +	return true;
>  }
>  
>  STATIC void
> @@ -2795,10 +2776,6 @@ xlog_state_do_callback(
>  	 *
>  	 * Keep looping through iclogs until one full pass is made without
>  	 * running any callbacks.
> -	 *
> -	 * If the log has been shut down, still perform the callbacks once per
> -	 * iclog to abort all log items, but don't bother to restart the loop
> -	 * after dropping the log as no new callbacks can show up.
>  	 */
>  	spin_lock(&log->l_icloglock);
>  	do {
> @@ -2809,25 +2786,40 @@ xlog_state_do_callback(
>  		repeats++;
>  
>  		do {
> +			/*
> +			 * If the log has been shut down, still perform the
> +			 * callbacks to abort all log items to clean up any
> +			 * allocate resource, but don't bother to restart the
> +			 * loop after dropping the log as no new callbacks can
> +			 * be attached now.
> +			 */
>  			if (XLOG_FORCED_SHUTDOWN(log)) {
>  				xlog_state_do_iclog_callbacks(log, iclog);
>  				wake_up_all(&iclog->ic_force_wait);
>  				continue;
>  			}
>  
> -			if (xlog_state_iodone_process_iclog(log, iclog))
> -				break;
> -
> -			if (iclog->ic_state != XLOG_STATE_CALLBACK)
> +			/*
> +			 * Skip all iclogs in the ACTIVE & DIRTY states:
> +			 */
> +			if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> +			    iclog->ic_state == XLOG_STATE_DIRTY)
>  				continue;
>  
> +			/*
> +			 * We can only perform callbacks in order.  If this
> +			 * iclog is not in the DONE_SYNC state, we skip the rest
> +			 * and just try to clean up.
> +			 */
> +			if (iclog->ic_state != XLOG_STATE_DONE_SYNC)
> +				break;
> +
>  			/*
>  			 * Running callbacks will drop the icloglock which means
>  			 * we'll have to run at least one more complete loop.
>  			 */
> -			cycled_icloglock = true;
> -			xlog_state_do_iclog_callbacks(log, iclog);
> -			xlog_state_clean_iclog(log, iclog);
> +			if (xlog_state_iodone_process_iclog(log, iclog))
> +				cycled_icloglock = true;
>  		} while ((iclog = iclog->ic_next) != first_iclog);
>  
>  		if (repeats > 5000) {
> -- 
> 2.24.1
> 

[PATCH 11/14] xfs: merge xlog_state_clean_iclog into xlog_state_iodone_process_iclog

[Christoph Hellwig]

Merge xlog_state_clean_iclog into its only caller, which makes the iclog
I/O completion handling a little easier to follow.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a38d495b6e81..899c324d07e2 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2625,22 +2625,6 @@ xlog_covered_state(
 	return XLOG_STATE_COVER_NEED;
 }
 
-STATIC void
-xlog_state_clean_iclog(
-	struct xlog		*log,
-	struct xlog_in_core	*dirty_iclog)
-{
-	int			iclogs_changed = 0;
-
-	dirty_iclog->ic_state = XLOG_STATE_DIRTY;
-
-	xlog_state_activate_iclogs(log, &iclogs_changed);
-	wake_up_all(&dirty_iclog->ic_force_wait);
-
-	if (iclogs_changed)
-		log->l_covered_state = xlog_covered_state(log, iclogs_changed);
-}
-
 STATIC xfs_lsn_t
 xlog_get_lowest_lsn(
 	struct xlog		*log)
@@ -2744,6 +2728,7 @@ xlog_state_iodone_process_iclog(
 	struct xlog_in_core	*iclog)
 {
 	xfs_lsn_t		header_lsn, lowest_lsn;
+	int			iclogs_changed = 0;
 
 	/*
 	 * Now that we have an iclog that is in the DONE_SYNC state, do one more
@@ -2758,7 +2743,13 @@ xlog_state_iodone_process_iclog(
 
 	xlog_state_set_callback(log, iclog, header_lsn);
 	xlog_state_do_iclog_callbacks(log, iclog);
-	xlog_state_clean_iclog(log, iclog);
+
+	iclog->ic_state = XLOG_STATE_DIRTY;
+	xlog_state_activate_iclogs(log, &iclogs_changed);
+
+	wake_up_all(&iclog->ic_force_wait);
+	if (iclogs_changed)
+		log->l_covered_state = xlog_covered_state(log, iclogs_changed);
 	return true;
 }
 
-- 
2.24.1

Re: [PATCH 11/14] xfs: merge xlog_state_clean_iclog into xlog_state_iodone_process_iclog

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:30PM +0100, Christoph Hellwig wrote:
> Merge xlog_state_clean_iclog into its only caller, which makes the iclog
> I/O completion handling a little easier to follow.
> 
> 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 | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a38d495b6e81..899c324d07e2 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2625,22 +2625,6 @@ xlog_covered_state(
>  	return XLOG_STATE_COVER_NEED;
>  }
>  
> -STATIC void
> -xlog_state_clean_iclog(
> -	struct xlog		*log,
> -	struct xlog_in_core	*dirty_iclog)
> -{
> -	int			iclogs_changed = 0;
> -
> -	dirty_iclog->ic_state = XLOG_STATE_DIRTY;
> -
> -	xlog_state_activate_iclogs(log, &iclogs_changed);
> -	wake_up_all(&dirty_iclog->ic_force_wait);
> -
> -	if (iclogs_changed)
> -		log->l_covered_state = xlog_covered_state(log, iclogs_changed);
> -}
> -
>  STATIC xfs_lsn_t
>  xlog_get_lowest_lsn(
>  	struct xlog		*log)
> @@ -2744,6 +2728,7 @@ xlog_state_iodone_process_iclog(
>  	struct xlog_in_core	*iclog)
>  {
>  	xfs_lsn_t		header_lsn, lowest_lsn;
> +	int			iclogs_changed = 0;
>  
>  	/*
>  	 * Now that we have an iclog that is in the DONE_SYNC state, do one more
> @@ -2758,7 +2743,13 @@ xlog_state_iodone_process_iclog(
>  
>  	xlog_state_set_callback(log, iclog, header_lsn);
>  	xlog_state_do_iclog_callbacks(log, iclog);
> -	xlog_state_clean_iclog(log, iclog);
> +
> +	iclog->ic_state = XLOG_STATE_DIRTY;
> +	xlog_state_activate_iclogs(log, &iclogs_changed);
> +
> +	wake_up_all(&iclog->ic_force_wait);
> +	if (iclogs_changed)
> +		log->l_covered_state = xlog_covered_state(log, iclogs_changed);
>  	return true;
>  }
>  
> -- 
> 2.24.1
> 

Re: [PATCH 11/14] xfs: merge xlog_state_clean_iclog into xlog_state_iodone_process_iclog

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:30PM +0100, Christoph Hellwig wrote:
> Merge xlog_state_clean_iclog into its only caller, which makes the iclog
> I/O completion handling a little easier to follow.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a38d495b6e81..899c324d07e2 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2625,22 +2625,6 @@ xlog_covered_state(
>  	return XLOG_STATE_COVER_NEED;
>  }
>  
> -STATIC void
> -xlog_state_clean_iclog(
> -	struct xlog		*log,
> -	struct xlog_in_core	*dirty_iclog)
> -{
> -	int			iclogs_changed = 0;
> -
> -	dirty_iclog->ic_state = XLOG_STATE_DIRTY;
> -
> -	xlog_state_activate_iclogs(log, &iclogs_changed);
> -	wake_up_all(&dirty_iclog->ic_force_wait);
> -
> -	if (iclogs_changed)
> -		log->l_covered_state = xlog_covered_state(log, iclogs_changed);
> -}
> -
>  STATIC xfs_lsn_t
>  xlog_get_lowest_lsn(
>  	struct xlog		*log)
> @@ -2744,6 +2728,7 @@ xlog_state_iodone_process_iclog(
>  	struct xlog_in_core	*iclog)
>  {
>  	xfs_lsn_t		header_lsn, lowest_lsn;
> +	int			iclogs_changed = 0;
>  
>  	/*
>  	 * Now that we have an iclog that is in the DONE_SYNC state, do one more
> @@ -2758,7 +2743,13 @@ xlog_state_iodone_process_iclog(
>  
>  	xlog_state_set_callback(log, iclog, header_lsn);
>  	xlog_state_do_iclog_callbacks(log, iclog);
> -	xlog_state_clean_iclog(log, iclog);
> +
> +	iclog->ic_state = XLOG_STATE_DIRTY;
> +	xlog_state_activate_iclogs(log, &iclogs_changed);
> +
> +	wake_up_all(&iclog->ic_force_wait);
> +	if (iclogs_changed)
> +		log->l_covered_state = xlog_covered_state(log, iclogs_changed);
>  	return true;
>  }
>  
> -- 
> 2.24.1
> 

[PATCH 12/14] xfs: merge xlog_state_set_callback into xlog_state_iodone_process_iclog

[Christoph Hellwig]

Merge xlog_state_set_callback into its only caller, which makes the iclog
I/O completion handling a little easier to follow.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 899c324d07e2..865dd1e08679 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2645,46 +2645,6 @@ xlog_get_lowest_lsn(
 	return lowest_lsn;
 }
 
-/*
- * Completion of a iclog IO does not imply that a transaction has completed, as
- * transactions can be large enough to span many iclogs. We cannot change the
- * tail of the log half way through a transaction as this may be the only
- * transaction in the log and moving the tail to point to the middle of it
- * will prevent recovery from finding the start of the transaction. Hence we
- * should only update the last_sync_lsn if this iclog contains transaction
- * completion callbacks on it.
- *
- * We have to do this before we drop the icloglock to ensure we are the only one
- * that can update it.
- *
- * If we are moving the last_sync_lsn forwards, we also need to ensure we kick
- * the reservation grant head pushing. This is due to the fact that the push
- * target is bound by the current last_sync_lsn value. Hence if we have a large
- * amount of log space bound up in this committing transaction then the
- * last_sync_lsn value may be the limiting factor preventing tail pushing from
- * freeing space in the log. Hence once we've updated the last_sync_lsn we
- * should push the AIL to ensure the push target (and hence the grant head) is
- * no longer bound by the old log head location and can move forwards and make
- * progress again.
- */
-static void
-xlog_state_set_callback(
-	struct xlog		*log,
-	struct xlog_in_core	*iclog,
-	xfs_lsn_t		header_lsn)
-{
-	iclog->ic_state = XLOG_STATE_CALLBACK;
-
-	ASSERT(XFS_LSN_CMP(atomic64_read(&log->l_last_sync_lsn),
-			   header_lsn) <= 0);
-
-	if (list_empty_careful(&iclog->ic_callbacks))
-		return;
-
-	atomic64_set(&log->l_last_sync_lsn, header_lsn);
-	xlog_grant_push_ail(log, 0);
-}
-
 /*
  * Keep processing entries in the iclog callback list until we come around and
  * it is empty.  We need to atomically see that the list is empty and change the
@@ -2741,7 +2701,39 @@ xlog_state_iodone_process_iclog(
 	if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
 		return false;
 
-	xlog_state_set_callback(log, iclog, header_lsn);
+	iclog->ic_state = XLOG_STATE_CALLBACK;
+
+	ASSERT(XFS_LSN_CMP(atomic64_read(&log->l_last_sync_lsn),
+			   header_lsn) <= 0);
+
+	/*
+	 * Completion of an iclog I/O does not imply that a transaction has
+	 * completed, as transactions can be large enough to span multiple
+	 * iclogs.  We cannot change the tail of the log half way through a
+	 * transaction as this may be the only transaction in the log and moving
+	 * the tail to point to the middle of it will prevent recovery from
+	 * finding the start of the transaction. Hence we should only update
+	 * the last_sync_lsn if this iclog contains transaction completion
+	 * callbacks on it.
+	 *
+	 * We have to do this before we drop the icloglock to ensure we are the
+	 * only one that can update it.
+	 *
+	 * If we are moving last_sync_lsn forwards, we also need to ensure we
+	 * kick the reservation grant head pushing. This is due to the fact that
+	 * the push target is bound by the current last_sync_lsn value.  If we
+	 * have a large amount of log space bound up in this committing
+	 * transaction then the last_sync_lsn value may be the limiting factor
+	 * preventing tail pushing from freeing space in the log.  Hence once
+	 * we've updated the last_sync_lsn we should push the AIL to ensure the
+	 * push target (and hence the grant head) is no longer bound by the old
+	 * log head location and can move forwards and make progress again.
+	 */
+	if (!list_empty_careful(&iclog->ic_callbacks)) {
+		atomic64_set(&log->l_last_sync_lsn, header_lsn);
+		xlog_grant_push_ail(log, 0);
+	}
+
 	xlog_state_do_iclog_callbacks(log, iclog);
 
 	iclog->ic_state = XLOG_STATE_DIRTY;
-- 
2.24.1

Re: [PATCH 12/14] xfs: merge xlog_state_set_callback into xlog_state_iodone_process_iclog

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:31PM +0100, Christoph Hellwig wrote:
> Merge xlog_state_set_callback into its only caller, which makes the iclog
> I/O completion handling a little easier to follow.
> 
> 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 | 74 +++++++++++++++++++++---------------------------
>  1 file changed, 33 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 899c324d07e2..865dd1e08679 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2645,46 +2645,6 @@ xlog_get_lowest_lsn(
>  	return lowest_lsn;
>  }
>  
> -/*
> - * Completion of a iclog IO does not imply that a transaction has completed, as
> - * transactions can be large enough to span many iclogs. We cannot change the
> - * tail of the log half way through a transaction as this may be the only
> - * transaction in the log and moving the tail to point to the middle of it
> - * will prevent recovery from finding the start of the transaction. Hence we
> - * should only update the last_sync_lsn if this iclog contains transaction
> - * completion callbacks on it.
> - *
> - * We have to do this before we drop the icloglock to ensure we are the only one
> - * that can update it.
> - *
> - * If we are moving the last_sync_lsn forwards, we also need to ensure we kick
> - * the reservation grant head pushing. This is due to the fact that the push
> - * target is bound by the current last_sync_lsn value. Hence if we have a large
> - * amount of log space bound up in this committing transaction then the
> - * last_sync_lsn value may be the limiting factor preventing tail pushing from
> - * freeing space in the log. Hence once we've updated the last_sync_lsn we
> - * should push the AIL to ensure the push target (and hence the grant head) is
> - * no longer bound by the old log head location and can move forwards and make
> - * progress again.
> - */
> -static void
> -xlog_state_set_callback(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog,
> -	xfs_lsn_t		header_lsn)
> -{
> -	iclog->ic_state = XLOG_STATE_CALLBACK;
> -
> -	ASSERT(XFS_LSN_CMP(atomic64_read(&log->l_last_sync_lsn),
> -			   header_lsn) <= 0);
> -
> -	if (list_empty_careful(&iclog->ic_callbacks))
> -		return;
> -
> -	atomic64_set(&log->l_last_sync_lsn, header_lsn);
> -	xlog_grant_push_ail(log, 0);
> -}
> -
>  /*
>   * Keep processing entries in the iclog callback list until we come around and
>   * it is empty.  We need to atomically see that the list is empty and change the
> @@ -2741,7 +2701,39 @@ xlog_state_iodone_process_iclog(
>  	if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
>  		return false;
>  
> -	xlog_state_set_callback(log, iclog, header_lsn);
> +	iclog->ic_state = XLOG_STATE_CALLBACK;
> +
> +	ASSERT(XFS_LSN_CMP(atomic64_read(&log->l_last_sync_lsn),
> +			   header_lsn) <= 0);
> +
> +	/*
> +	 * Completion of an iclog I/O does not imply that a transaction has
> +	 * completed, as transactions can be large enough to span multiple
> +	 * iclogs.  We cannot change the tail of the log half way through a
> +	 * transaction as this may be the only transaction in the log and moving
> +	 * the tail to point to the middle of it will prevent recovery from
> +	 * finding the start of the transaction. Hence we should only update
> +	 * the last_sync_lsn if this iclog contains transaction completion
> +	 * callbacks on it.
> +	 *
> +	 * We have to do this before we drop the icloglock to ensure we are the
> +	 * only one that can update it.
> +	 *
> +	 * If we are moving last_sync_lsn forwards, we also need to ensure we
> +	 * kick the reservation grant head pushing. This is due to the fact that
> +	 * the push target is bound by the current last_sync_lsn value.  If we
> +	 * have a large amount of log space bound up in this committing
> +	 * transaction then the last_sync_lsn value may be the limiting factor
> +	 * preventing tail pushing from freeing space in the log.  Hence once
> +	 * we've updated the last_sync_lsn we should push the AIL to ensure the
> +	 * push target (and hence the grant head) is no longer bound by the old
> +	 * log head location and can move forwards and make progress again.
> +	 */
> +	if (!list_empty_careful(&iclog->ic_callbacks)) {
> +		atomic64_set(&log->l_last_sync_lsn, header_lsn);
> +		xlog_grant_push_ail(log, 0);
> +	}
> +
>  	xlog_state_do_iclog_callbacks(log, iclog);
>  
>  	iclog->ic_state = XLOG_STATE_DIRTY;
> -- 
> 2.24.1
> 

Re: [PATCH 12/14] xfs: merge xlog_state_set_callback into xlog_state_iodone_process_iclog

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:31PM +0100, Christoph Hellwig wrote:
> Merge xlog_state_set_callback into its only caller, which makes the iclog
> I/O completion handling a little easier to follow.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 74 +++++++++++++++++++++---------------------------
>  1 file changed, 33 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 899c324d07e2..865dd1e08679 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2645,46 +2645,6 @@ xlog_get_lowest_lsn(
>  	return lowest_lsn;
>  }
>  
> -/*
> - * Completion of a iclog IO does not imply that a transaction has completed, as
> - * transactions can be large enough to span many iclogs. We cannot change the
> - * tail of the log half way through a transaction as this may be the only
> - * transaction in the log and moving the tail to point to the middle of it
> - * will prevent recovery from finding the start of the transaction. Hence we
> - * should only update the last_sync_lsn if this iclog contains transaction
> - * completion callbacks on it.
> - *
> - * We have to do this before we drop the icloglock to ensure we are the only one
> - * that can update it.
> - *
> - * If we are moving the last_sync_lsn forwards, we also need to ensure we kick
> - * the reservation grant head pushing. This is due to the fact that the push
> - * target is bound by the current last_sync_lsn value. Hence if we have a large
> - * amount of log space bound up in this committing transaction then the
> - * last_sync_lsn value may be the limiting factor preventing tail pushing from
> - * freeing space in the log. Hence once we've updated the last_sync_lsn we
> - * should push the AIL to ensure the push target (and hence the grant head) is
> - * no longer bound by the old log head location and can move forwards and make
> - * progress again.
> - */
> -static void
> -xlog_state_set_callback(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog,
> -	xfs_lsn_t		header_lsn)
> -{
> -	iclog->ic_state = XLOG_STATE_CALLBACK;
> -
> -	ASSERT(XFS_LSN_CMP(atomic64_read(&log->l_last_sync_lsn),
> -			   header_lsn) <= 0);
> -
> -	if (list_empty_careful(&iclog->ic_callbacks))
> -		return;
> -
> -	atomic64_set(&log->l_last_sync_lsn, header_lsn);
> -	xlog_grant_push_ail(log, 0);
> -}
> -
>  /*
>   * Keep processing entries in the iclog callback list until we come around and
>   * it is empty.  We need to atomically see that the list is empty and change the
> @@ -2741,7 +2701,39 @@ xlog_state_iodone_process_iclog(
>  	if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
>  		return false;
>  
> -	xlog_state_set_callback(log, iclog, header_lsn);
> +	iclog->ic_state = XLOG_STATE_CALLBACK;
> +
> +	ASSERT(XFS_LSN_CMP(atomic64_read(&log->l_last_sync_lsn),
> +			   header_lsn) <= 0);
> +
> +	/*
> +	 * Completion of an iclog I/O does not imply that a transaction has
> +	 * completed, as transactions can be large enough to span multiple
> +	 * iclogs.  We cannot change the tail of the log half way through a
> +	 * transaction as this may be the only transaction in the log and moving
> +	 * the tail to point to the middle of it will prevent recovery from
> +	 * finding the start of the transaction. Hence we should only update
> +	 * the last_sync_lsn if this iclog contains transaction completion
> +	 * callbacks on it.
> +	 *
> +	 * We have to do this before we drop the icloglock to ensure we are the
> +	 * only one that can update it.
> +	 *
> +	 * If we are moving last_sync_lsn forwards, we also need to ensure we
> +	 * kick the reservation grant head pushing. This is due to the fact that
> +	 * the push target is bound by the current last_sync_lsn value.  If we
> +	 * have a large amount of log space bound up in this committing
> +	 * transaction then the last_sync_lsn value may be the limiting factor
> +	 * preventing tail pushing from freeing space in the log.  Hence once
> +	 * we've updated the last_sync_lsn we should push the AIL to ensure the
> +	 * push target (and hence the grant head) is no longer bound by the old
> +	 * log head location and can move forwards and make progress again.
> +	 */
> +	if (!list_empty_careful(&iclog->ic_callbacks)) {
> +		atomic64_set(&log->l_last_sync_lsn, header_lsn);
> +		xlog_grant_push_ail(log, 0);
> +	}
> +
>  	xlog_state_do_iclog_callbacks(log, iclog);
>  
>  	iclog->ic_state = XLOG_STATE_DIRTY;
> -- 
> 2.24.1
> 

[PATCH 13/14] xfs: remove xlog_state_want_sync

[Christoph Hellwig]

Open code the xlog_state_want_sync logic in its two callers given that
this function is a trivial wrapper around xlog_state_switch_iclogs.

Move the lockdep assert into xlog_state_switch_iclogs to not lose this
debugging aid, and improve the comment that documents
xlog_state_switch_iclogs as well.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 865dd1e08679..761b138d97ec 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -62,11 +62,6 @@ xlog_state_switch_iclogs(
 	struct xlog_in_core	*iclog,
 	int			eventual_size);
 STATIC void
-xlog_state_want_sync(
-	struct xlog		*log,
-	struct xlog_in_core	*iclog);
-
-STATIC void
 xlog_grant_push_ail(
 	struct xlog		*log,
 	int			need_bytes);
@@ -938,7 +933,11 @@ xfs_log_write_unmount_record(
 	spin_lock(&log->l_icloglock);
 	iclog = log->l_iclog;
 	atomic_inc(&iclog->ic_refcnt);
-	xlog_state_want_sync(log, iclog);
+	if (iclog->ic_state == XLOG_STATE_ACTIVE)
+		xlog_state_switch_iclogs(log, iclog, 0);
+	else
+		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
+		       iclog->ic_state == XLOG_STATE_IOERROR);
 	error = xlog_state_release_iclog(log, iclog);
 	xlog_wait_on_iclog(iclog);
 
@@ -2293,7 +2292,11 @@ xlog_write_copy_finish(
 		*record_cnt = 0;
 		*data_cnt = 0;
 
-		xlog_state_want_sync(log, iclog);
+		if (iclog->ic_state == XLOG_STATE_ACTIVE)
+			xlog_state_switch_iclogs(log, iclog, 0);
+		else
+			ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
+			       iclog->ic_state == XLOG_STATE_IOERROR);
 		if (!commit_iclog)
 			goto release_iclog;
 		spin_unlock(&log->l_icloglock);
@@ -3069,11 +3072,12 @@ xlog_ungrant_log_space(
 }
 
 /*
- * 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.
- * When this routine is called from xlog_state_get_iclog_space(), the
- * exact size of the iclog has not yet been determined.  All we know is
- * that every data block.  We have run out of space in this log record.
+ * Mark the current iclog in the ring as WANT_SYNC and move the current iclog
+ * pointer to the next iclog in the ring.
+ *
+ * When called from xlog_state_get_iclog_space(), the exact size of the iclog
+ * has not yet been determined, all we know is that we have run out of space in
+ * the current iclog.
  */
 STATIC void
 xlog_state_switch_iclogs(
@@ -3082,6 +3086,8 @@ xlog_state_switch_iclogs(
 	int			eventual_size)
 {
 	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
+	assert_spin_locked(&log->l_icloglock);
+
 	if (!eventual_size)
 		eventual_size = iclog->ic_offset;
 	iclog->ic_state = XLOG_STATE_WANT_SYNC;
@@ -3323,26 +3329,6 @@ xfs_log_force_lsn(
 	return ret;
 }
 
-/*
- * Called when we want to mark the current iclog as being ready to sync to
- * disk.
- */
-STATIC void
-xlog_state_want_sync(
-	struct xlog		*log,
-	struct xlog_in_core	*iclog)
-{
-	assert_spin_locked(&log->l_icloglock);
-
-	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
-		xlog_state_switch_iclogs(log, iclog, 0);
-	} else {
-		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
-		       iclog->ic_state == XLOG_STATE_IOERROR);
-	}
-}
-
-
 /*****************************************************************************
  *
  *		TICKET functions
-- 
2.24.1

Re: [PATCH 13/14] xfs: remove xlog_state_want_sync

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:32PM +0100, Christoph Hellwig wrote:
> Open code the xlog_state_want_sync logic in its two callers given that
> this function is a trivial wrapper around xlog_state_switch_iclogs.
> 
> Move the lockdep assert into xlog_state_switch_iclogs to not lose this
> debugging aid, and improve the comment that documents
> xlog_state_switch_iclogs as well.
> 
> 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 | 50 +++++++++++++++++-------------------------------
>  1 file changed, 18 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 865dd1e08679..761b138d97ec 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -62,11 +62,6 @@ xlog_state_switch_iclogs(
>  	struct xlog_in_core	*iclog,
>  	int			eventual_size);
>  STATIC void
> -xlog_state_want_sync(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog);
> -
> -STATIC void
>  xlog_grant_push_ail(
>  	struct xlog		*log,
>  	int			need_bytes);
> @@ -938,7 +933,11 @@ xfs_log_write_unmount_record(
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
>  	atomic_inc(&iclog->ic_refcnt);
> -	xlog_state_want_sync(log, iclog);
> +	if (iclog->ic_state == XLOG_STATE_ACTIVE)
> +		xlog_state_switch_iclogs(log, iclog, 0);
> +	else
> +		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> +		       iclog->ic_state == XLOG_STATE_IOERROR);
>  	error = xlog_state_release_iclog(log, iclog);
>  	xlog_wait_on_iclog(iclog);
>  
> @@ -2293,7 +2292,11 @@ xlog_write_copy_finish(
>  		*record_cnt = 0;
>  		*data_cnt = 0;
>  
> -		xlog_state_want_sync(log, iclog);
> +		if (iclog->ic_state == XLOG_STATE_ACTIVE)
> +			xlog_state_switch_iclogs(log, iclog, 0);
> +		else
> +			ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> +			       iclog->ic_state == XLOG_STATE_IOERROR);
>  		if (!commit_iclog)
>  			goto release_iclog;
>  		spin_unlock(&log->l_icloglock);
> @@ -3069,11 +3072,12 @@ xlog_ungrant_log_space(
>  }
>  
>  /*
> - * 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.
> - * When this routine is called from xlog_state_get_iclog_space(), the
> - * exact size of the iclog has not yet been determined.  All we know is
> - * that every data block.  We have run out of space in this log record.
> + * Mark the current iclog in the ring as WANT_SYNC and move the current iclog
> + * pointer to the next iclog in the ring.
> + *
> + * When called from xlog_state_get_iclog_space(), the exact size of the iclog
> + * has not yet been determined, all we know is that we have run out of space in
> + * the current iclog.
>   */
>  STATIC void
>  xlog_state_switch_iclogs(
> @@ -3082,6 +3086,8 @@ xlog_state_switch_iclogs(
>  	int			eventual_size)
>  {
>  	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
> +	assert_spin_locked(&log->l_icloglock);
> +
>  	if (!eventual_size)
>  		eventual_size = iclog->ic_offset;
>  	iclog->ic_state = XLOG_STATE_WANT_SYNC;
> @@ -3323,26 +3329,6 @@ xfs_log_force_lsn(
>  	return ret;
>  }
>  
> -/*
> - * Called when we want to mark the current iclog as being ready to sync to
> - * disk.
> - */
> -STATIC void
> -xlog_state_want_sync(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog)
> -{
> -	assert_spin_locked(&log->l_icloglock);
> -
> -	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
> -		xlog_state_switch_iclogs(log, iclog, 0);
> -	} else {
> -		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -		       iclog->ic_state == XLOG_STATE_IOERROR);
> -	}
> -}
> -
> -
>  /*****************************************************************************
>   *
>   *		TICKET functions
> -- 
> 2.24.1
> 

Re: [PATCH 13/14] xfs: remove xlog_state_want_sync

[Brian Foster]

On Mon, Mar 16, 2020 at 03:42:32PM +0100, Christoph Hellwig wrote:
> Open code the xlog_state_want_sync logic in its two callers given that
> this function is a trivial wrapper around xlog_state_switch_iclogs.
> 
> Move the lockdep assert into xlog_state_switch_iclogs to not lose this
> debugging aid, and improve the comment that documents
> xlog_state_switch_iclogs as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 50 +++++++++++++++++-------------------------------
>  1 file changed, 18 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 865dd1e08679..761b138d97ec 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -62,11 +62,6 @@ xlog_state_switch_iclogs(
>  	struct xlog_in_core	*iclog,
>  	int			eventual_size);
>  STATIC void
> -xlog_state_want_sync(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog);
> -
> -STATIC void
>  xlog_grant_push_ail(
>  	struct xlog		*log,
>  	int			need_bytes);
> @@ -938,7 +933,11 @@ xfs_log_write_unmount_record(
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
>  	atomic_inc(&iclog->ic_refcnt);
> -	xlog_state_want_sync(log, iclog);
> +	if (iclog->ic_state == XLOG_STATE_ACTIVE)
> +		xlog_state_switch_iclogs(log, iclog, 0);
> +	else
> +		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> +		       iclog->ic_state == XLOG_STATE_IOERROR);
>  	error = xlog_state_release_iclog(log, iclog);
>  	xlog_wait_on_iclog(iclog);
>  
> @@ -2293,7 +2292,11 @@ xlog_write_copy_finish(
>  		*record_cnt = 0;
>  		*data_cnt = 0;
>  
> -		xlog_state_want_sync(log, iclog);
> +		if (iclog->ic_state == XLOG_STATE_ACTIVE)
> +			xlog_state_switch_iclogs(log, iclog, 0);
> +		else
> +			ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> +			       iclog->ic_state == XLOG_STATE_IOERROR);
>  		if (!commit_iclog)
>  			goto release_iclog;
>  		spin_unlock(&log->l_icloglock);
> @@ -3069,11 +3072,12 @@ xlog_ungrant_log_space(
>  }
>  
>  /*
> - * 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.
> - * When this routine is called from xlog_state_get_iclog_space(), the
> - * exact size of the iclog has not yet been determined.  All we know is
> - * that every data block.  We have run out of space in this log record.
> + * Mark the current iclog in the ring as WANT_SYNC and move the current iclog
> + * pointer to the next iclog in the ring.
> + *
> + * When called from xlog_state_get_iclog_space(), the exact size of the iclog
> + * has not yet been determined, all we know is that we have run out of space in
> + * the current iclog.
>   */
>  STATIC void
>  xlog_state_switch_iclogs(
> @@ -3082,6 +3086,8 @@ xlog_state_switch_iclogs(
>  	int			eventual_size)
>  {
>  	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
> +	assert_spin_locked(&log->l_icloglock);
> +
>  	if (!eventual_size)
>  		eventual_size = iclog->ic_offset;
>  	iclog->ic_state = XLOG_STATE_WANT_SYNC;
> @@ -3323,26 +3329,6 @@ xfs_log_force_lsn(
>  	return ret;
>  }
>  
> -/*
> - * Called when we want to mark the current iclog as being ready to sync to
> - * disk.
> - */
> -STATIC void
> -xlog_state_want_sync(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog)
> -{
> -	assert_spin_locked(&log->l_icloglock);
> -
> -	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
> -		xlog_state_switch_iclogs(log, iclog, 0);
> -	} else {
> -		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -		       iclog->ic_state == XLOG_STATE_IOERROR);
> -	}
> -}
> -
> -
>  /*****************************************************************************
>   *
>   *		TICKET functions
> -- 
> 2.24.1
> 

[PATCH 14/14] xfs: remove XLOG_STATE_IOERROR

[Christoph Hellwig]

Just check the shutdown flag in struct xlog, instead of replicating the
information into each iclog and checking it there.

As the iclog state is now not overload with the shut down information
various places that check for a specific state now don't need to account
for the fake IOERROR state.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 761b138d97ec..07023372ccbd 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -578,7 +578,7 @@ xlog_state_release_iclog(
 {
 	lockdep_assert_held(&log->l_icloglock);
 
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
+	if (XLOG_FORCED_SHUTDOWN(log))
 		return -EIO;
 
 	if (atomic_dec_and_test(&iclog->ic_refcnt) &&
@@ -599,7 +599,7 @@ xfs_log_release_iclog(
 	bool			sync = false;
 
 	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
-		if (iclog->ic_state != XLOG_STATE_IOERROR)
+		if (!XLOG_FORCED_SHUTDOWN(log))
 			sync = __xlog_state_release_iclog(log, iclog);
 		spin_unlock(&log->l_icloglock);
 	}
@@ -924,7 +924,7 @@ xfs_log_write_unmount_record(
 	error = xlog_write(log, &vec, tic, &lsn, NULL, flags);
 	/*
 	 * At this point, we're umounting anyway, so there's no point in
-	 * transitioning log state to IOERROR. Just continue...
+	 * transitioning log state to IO_ERROR. Just continue...
 	 */
 out_err:
 	if (error)
@@ -936,8 +936,7 @@ xfs_log_write_unmount_record(
 	if (iclog->ic_state == XLOG_STATE_ACTIVE)
 		xlog_state_switch_iclogs(log, iclog, 0);
 	else
-		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
-		       iclog->ic_state == XLOG_STATE_IOERROR);
+		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC);
 	error = xlog_state_release_iclog(log, iclog);
 	xlog_wait_on_iclog(iclog);
 
@@ -1740,7 +1739,7 @@ xlog_write_iclog(
 	 * across the log IO to archieve that.
 	 */
 	down(&iclog->ic_sema);
-	if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) {
+	if (XLOG_FORCED_SHUTDOWN(log)) {
 		/*
 		 * It would seem logical to return EIO here, but we rely on
 		 * the log state machine to propagate I/O errors instead of
@@ -2295,8 +2294,7 @@ xlog_write_copy_finish(
 		if (iclog->ic_state == XLOG_STATE_ACTIVE)
 			xlog_state_switch_iclogs(log, iclog, 0);
 		else
-			ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
-			       iclog->ic_state == XLOG_STATE_IOERROR);
+			ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC);
 		if (!commit_iclog)
 			goto release_iclog;
 		spin_unlock(&log->l_icloglock);
@@ -2817,8 +2815,7 @@ xlog_state_do_callback(
 		}
 	} while (cycled_icloglock);
 
-	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
-	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
+	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE)
 		wake_up_all(&log->l_flush_wait);
 
 	spin_unlock(&log->l_icloglock);
@@ -3167,7 +3164,7 @@ xfs_log_force(
 
 	spin_lock(&log->l_icloglock);
 	iclog = log->l_iclog;
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
+	if (XLOG_FORCED_SHUTDOWN(log))
 		goto out_error;
 
 	if (iclog->ic_state == XLOG_STATE_DIRTY ||
@@ -3240,7 +3237,7 @@ __xfs_log_force_lsn(
 
 	spin_lock(&log->l_icloglock);
 	iclog = log->l_iclog;
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
+	if (XLOG_FORCED_SHUTDOWN(log))
 		goto out_error;
 
 	while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
@@ -3691,34 +3688,6 @@ xlog_verify_iclog(
 }	/* xlog_verify_iclog */
 #endif
 
-/*
- * Mark all iclogs IOERROR. l_icloglock is held by the caller.
- */
-STATIC int
-xlog_state_ioerror(
-	struct xlog	*log)
-{
-	xlog_in_core_t	*iclog, *ic;
-
-	iclog = log->l_iclog;
-	if (iclog->ic_state != XLOG_STATE_IOERROR) {
-		/*
-		 * Mark all the incore logs IOERROR.
-		 * From now on, no log flushes will result.
-		 */
-		ic = iclog;
-		do {
-			ic->ic_state = XLOG_STATE_IOERROR;
-			ic = ic->ic_next;
-		} while (ic != iclog);
-		return 0;
-	}
-	/*
-	 * Return non-zero, if state transition has already happened.
-	 */
-	return 1;
-}
-
 /*
  * This is called from xfs_force_shutdown, when we're forcibly
  * shutting down the filesystem, typically because of an IO error.
@@ -3740,10 +3709,8 @@ xfs_log_force_umount(
 	struct xfs_mount	*mp,
 	int			logerror)
 {
-	struct xlog	*log;
-	int		retval;
-
-	log = mp->m_log;
+	struct xlog		*log = mp->m_log;
+	int			retval = 0;
 
 	/*
 	 * If this happens during log recovery, don't worry about
@@ -3761,10 +3728,8 @@ 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) {
-		ASSERT(XLOG_FORCED_SHUTDOWN(log));
+	if (logerror && XLOG_FORCED_SHUTDOWN(log))
 		return 1;
-	}
 
 	/*
 	 * Flush all the completed transactions to disk before marking the log
@@ -3786,11 +3751,13 @@ xfs_log_force_umount(
 		mp->m_sb_bp->b_flags |= XBF_DONE;
 
 	/*
-	 * Mark the log and the iclogs with IO error flags to prevent any
-	 * further log IO from being issued or completed.
+	 * Mark the log as shut down to prevent any further log IO from being
+	 * issued or completed.  Return non-zero if log IO_ERROR transition had
+	 * already happened so that the caller can skip further processing.
 	 */
+	if (XLOG_FORCED_SHUTDOWN(log))
+		retval = 1;
 	log->l_flags |= XLOG_IO_ERROR;
-	retval = xlog_state_ioerror(log);
 	spin_unlock(&log->l_icloglock);
 
 	/*
@@ -3814,7 +3781,6 @@ xfs_log_force_umount(
 	spin_unlock(&log->l_cilp->xc_push_lock);
 	xlog_state_do_callback(log);
 
-	/* return non-zero if log IOERROR transition had already happened */
 	return retval;
 }
 
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index adc4af336c97..a93097cf0990 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -845,7 +845,7 @@ xlog_cil_push_work(
 		goto out_abort;
 
 	spin_lock(&commit_iclog->ic_callback_lock);
-	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
+	if (XLOG_FORCED_SHUTDOWN(log)) {
 		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 2b0aec37e73e..f3f4a35ce153 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -47,7 +47,6 @@ enum xlog_iclog_state {
 	XLOG_STATE_DONE_SYNC,	/* Done syncing to disk */
 	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.24.1

Re: [PATCH 14/14] xfs: remove XLOG_STATE_IOERROR

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 03:42:33PM +0100, Christoph Hellwig wrote:
> Just check the shutdown flag in struct xlog, instead of replicating the
> information into each iclog and checking it there.
> 
> As the iclog state is now not overload with the shut down information
> various places that check for a specific state now don't need to account
> for the fake IOERROR state.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c      | 68 +++++++++++--------------------------------
>  fs/xfs/xfs_log_cil.c  |  2 +-
>  fs/xfs/xfs_log_priv.h |  1 -
>  3 files changed, 18 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 761b138d97ec..07023372ccbd 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -578,7 +578,7 @@ xlog_state_release_iclog(
>  {
>  	lockdep_assert_held(&log->l_icloglock);
>  
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> +	if (XLOG_FORCED_SHUTDOWN(log))
>  		return -EIO;
>  
>  	if (atomic_dec_and_test(&iclog->ic_refcnt) &&
> @@ -599,7 +599,7 @@ xfs_log_release_iclog(
>  	bool			sync = false;
>  
>  	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
> -		if (iclog->ic_state != XLOG_STATE_IOERROR)
> +		if (!XLOG_FORCED_SHUTDOWN(log))
>  			sync = __xlog_state_release_iclog(log, iclog);
>  		spin_unlock(&log->l_icloglock);
>  	}
> @@ -924,7 +924,7 @@ xfs_log_write_unmount_record(
>  	error = xlog_write(log, &vec, tic, &lsn, NULL, flags);
>  	/*
>  	 * At this point, we're umounting anyway, so there's no point in
> -	 * transitioning log state to IOERROR. Just continue...
> +	 * transitioning log state to IO_ERROR. Just continue...

"...so there's no point in marking the log as shut down."?

There's no IOERROR state anymore, right?

--D

>  	 */
>  out_err:
>  	if (error)
> @@ -936,8 +936,7 @@ xfs_log_write_unmount_record(
>  	if (iclog->ic_state == XLOG_STATE_ACTIVE)
>  		xlog_state_switch_iclogs(log, iclog, 0);
>  	else
> -		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -		       iclog->ic_state == XLOG_STATE_IOERROR);
> +		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC);
>  	error = xlog_state_release_iclog(log, iclog);
>  	xlog_wait_on_iclog(iclog);
>  
> @@ -1740,7 +1739,7 @@ xlog_write_iclog(
>  	 * across the log IO to archieve that.
>  	 */
>  	down(&iclog->ic_sema);
> -	if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) {
> +	if (XLOG_FORCED_SHUTDOWN(log)) {
>  		/*
>  		 * It would seem logical to return EIO here, but we rely on
>  		 * the log state machine to propagate I/O errors instead of
> @@ -2295,8 +2294,7 @@ xlog_write_copy_finish(
>  		if (iclog->ic_state == XLOG_STATE_ACTIVE)
>  			xlog_state_switch_iclogs(log, iclog, 0);
>  		else
> -			ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -			       iclog->ic_state == XLOG_STATE_IOERROR);
> +			ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC);
>  		if (!commit_iclog)
>  			goto release_iclog;
>  		spin_unlock(&log->l_icloglock);
> @@ -2817,8 +2815,7 @@ xlog_state_do_callback(
>  		}
>  	} while (cycled_icloglock);
>  
> -	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
> -	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
> +	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE)
>  		wake_up_all(&log->l_flush_wait);
>  
>  	spin_unlock(&log->l_icloglock);
> @@ -3167,7 +3164,7 @@ xfs_log_force(
>  
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> +	if (XLOG_FORCED_SHUTDOWN(log))
>  		goto out_error;
>  
>  	if (iclog->ic_state == XLOG_STATE_DIRTY ||
> @@ -3240,7 +3237,7 @@ __xfs_log_force_lsn(
>  
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> +	if (XLOG_FORCED_SHUTDOWN(log))
>  		goto out_error;
>  
>  	while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
> @@ -3691,34 +3688,6 @@ xlog_verify_iclog(
>  }	/* xlog_verify_iclog */
>  #endif
>  
> -/*
> - * Mark all iclogs IOERROR. l_icloglock is held by the caller.
> - */
> -STATIC int
> -xlog_state_ioerror(
> -	struct xlog	*log)
> -{
> -	xlog_in_core_t	*iclog, *ic;
> -
> -	iclog = log->l_iclog;
> -	if (iclog->ic_state != XLOG_STATE_IOERROR) {
> -		/*
> -		 * Mark all the incore logs IOERROR.
> -		 * From now on, no log flushes will result.
> -		 */
> -		ic = iclog;
> -		do {
> -			ic->ic_state = XLOG_STATE_IOERROR;
> -			ic = ic->ic_next;
> -		} while (ic != iclog);
> -		return 0;
> -	}
> -	/*
> -	 * Return non-zero, if state transition has already happened.
> -	 */
> -	return 1;
> -}
> -
>  /*
>   * This is called from xfs_force_shutdown, when we're forcibly
>   * shutting down the filesystem, typically because of an IO error.
> @@ -3740,10 +3709,8 @@ xfs_log_force_umount(
>  	struct xfs_mount	*mp,
>  	int			logerror)
>  {
> -	struct xlog	*log;
> -	int		retval;
> -
> -	log = mp->m_log;
> +	struct xlog		*log = mp->m_log;
> +	int			retval = 0;
>  
>  	/*
>  	 * If this happens during log recovery, don't worry about
> @@ -3761,10 +3728,8 @@ 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) {
> -		ASSERT(XLOG_FORCED_SHUTDOWN(log));
> +	if (logerror && XLOG_FORCED_SHUTDOWN(log))
>  		return 1;
> -	}
>  
>  	/*
>  	 * Flush all the completed transactions to disk before marking the log
> @@ -3786,11 +3751,13 @@ xfs_log_force_umount(
>  		mp->m_sb_bp->b_flags |= XBF_DONE;
>  
>  	/*
> -	 * Mark the log and the iclogs with IO error flags to prevent any
> -	 * further log IO from being issued or completed.
> +	 * Mark the log as shut down to prevent any further log IO from being
> +	 * issued or completed.  Return non-zero if log IO_ERROR transition had
> +	 * already happened so that the caller can skip further processing.
>  	 */
> +	if (XLOG_FORCED_SHUTDOWN(log))
> +		retval = 1;
>  	log->l_flags |= XLOG_IO_ERROR;
> -	retval = xlog_state_ioerror(log);
>  	spin_unlock(&log->l_icloglock);
>  
>  	/*
> @@ -3814,7 +3781,6 @@ xfs_log_force_umount(
>  	spin_unlock(&log->l_cilp->xc_push_lock);
>  	xlog_state_do_callback(log);
>  
> -	/* return non-zero if log IOERROR transition had already happened */
>  	return retval;
>  }
>  
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index adc4af336c97..a93097cf0990 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -845,7 +845,7 @@ xlog_cil_push_work(
>  		goto out_abort;
>  
>  	spin_lock(&commit_iclog->ic_callback_lock);
> -	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
> +	if (XLOG_FORCED_SHUTDOWN(log)) {
>  		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 2b0aec37e73e..f3f4a35ce153 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -47,7 +47,6 @@ enum xlog_iclog_state {
>  	XLOG_STATE_DONE_SYNC,	/* Done syncing to disk */
>  	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.24.1
> 

Re: [PATCH 14/14] xfs: remove XLOG_STATE_IOERROR

[Christoph Hellwig]

On Mon, Mar 16, 2020 at 02:25:39PM -0700, Darrick J. Wong wrote:
> >  	 * At this point, we're umounting anyway, so there's no point in
> > -	 * transitioning log state to IOERROR. Just continue...
> > +	 * transitioning log state to IO_ERROR. Just continue...
> 
> "...so there's no point in marking the log as shut down."?
> 
> There's no IOERROR state anymore, right?

There is on the log itself:

        log->l_flags |= XLOG_IO_ERROR;

but I think your version is nicer anyway.