cleanup log I/O error handling

cleanup log I/O error handling

[Christoph Hellwig]

Hi all,

this series cleans up the XFS log I/O error handling and removes the
XLOG_STATE_IOERROR iclog state.  There first three are pretty harmless
cleanups, which I think might make sense to merge early.  The others
are more invasive and sometimes change behavior a bit, so careful
review is required.

[PATCH 1/7] xfs: remove the unused return value from xfs_log_unmount_write

[Christoph Hellwig]

Remove the ignored return value from xfs_log_unmount_write, and also
remove a rather pointless assert on the return value from xfs_log_force.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 796ff37d5bb5..fa499ddedb94 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -953,8 +953,7 @@ xfs_log_write_unmount_record(
  * currently architecture converted and "Unmount" is a bit foo.
  * As far as I know, there weren't any dependencies on the old behaviour.
  */
-
-static int
+static void
 xfs_log_unmount_write(xfs_mount_t *mp)
 {
 	struct xlog	 *log = mp->m_log;
@@ -962,7 +961,6 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 #ifdef DEBUG
 	xlog_in_core_t	 *first_iclog;
 #endif
-	int		 error;
 
 	/*
 	 * Don't write out unmount record on norecovery mounts or ro devices.
@@ -971,11 +969,10 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 	if (mp->m_flags & XFS_MOUNT_NORECOVERY ||
 	    xfs_readonly_buftarg(log->l_targ)) {
 		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
-		return 0;
+		return;
 	}
 
-	error = xfs_log_force(mp, XFS_LOG_SYNC);
-	ASSERT(error || !(XLOG_FORCED_SHUTDOWN(log)));
+	xfs_log_force(mp, XFS_LOG_SYNC);
 
 #ifdef DEBUG
 	first_iclog = iclog = log->l_iclog;
@@ -1007,7 +1004,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 		iclog = log->l_iclog;
 		atomic_inc(&iclog->ic_refcnt);
 		xlog_state_want_sync(log, iclog);
-		error =  xlog_state_release_iclog(log, iclog);
+		xlog_state_release_iclog(log, iclog);
 		switch (iclog->ic_state) {
 		case XLOG_STATE_ACTIVE:
 		case XLOG_STATE_DIRTY:
@@ -1019,9 +1016,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 			break;
 		}
 	}
-
-	return error;
-}	/* xfs_log_unmount_write */
+}
 
 /*
  * Empty the log for unmount/freeze.
-- 
2.24.1

Re: [PATCH 1/7] xfs: remove the unused return value from xfs_log_unmount_write

[Brian Foster]

On Fri, Mar 06, 2020 at 07:31:31AM -0700, Christoph Hellwig wrote:
> Remove the ignored return value from xfs_log_unmount_write, and also
> remove a rather pointless assert on the return value from xfs_log_force.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

I guess there's going to be obvious conflicts with Dave's series and
some of these changes. I'm just going to ignore that and you guys can
figure it out. :)

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

>  fs/xfs/xfs_log.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 796ff37d5bb5..fa499ddedb94 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -953,8 +953,7 @@ xfs_log_write_unmount_record(
>   * currently architecture converted and "Unmount" is a bit foo.
>   * As far as I know, there weren't any dependencies on the old behaviour.
>   */
> -
> -static int
> +static void
>  xfs_log_unmount_write(xfs_mount_t *mp)
>  {
>  	struct xlog	 *log = mp->m_log;
> @@ -962,7 +961,6 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  #ifdef DEBUG
>  	xlog_in_core_t	 *first_iclog;
>  #endif
> -	int		 error;
>  
>  	/*
>  	 * Don't write out unmount record on norecovery mounts or ro devices.
> @@ -971,11 +969,10 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  	if (mp->m_flags & XFS_MOUNT_NORECOVERY ||
>  	    xfs_readonly_buftarg(log->l_targ)) {
>  		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
> -		return 0;
> +		return;
>  	}
>  
> -	error = xfs_log_force(mp, XFS_LOG_SYNC);
> -	ASSERT(error || !(XLOG_FORCED_SHUTDOWN(log)));
> +	xfs_log_force(mp, XFS_LOG_SYNC);
>  
>  #ifdef DEBUG
>  	first_iclog = iclog = log->l_iclog;
> @@ -1007,7 +1004,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		iclog = log->l_iclog;
>  		atomic_inc(&iclog->ic_refcnt);
>  		xlog_state_want_sync(log, iclog);
> -		error =  xlog_state_release_iclog(log, iclog);
> +		xlog_state_release_iclog(log, iclog);
>  		switch (iclog->ic_state) {
>  		case XLOG_STATE_ACTIVE:
>  		case XLOG_STATE_DIRTY:
> @@ -1019,9 +1016,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  			break;
>  		}
>  	}
> -
> -	return error;
> -}	/* xfs_log_unmount_write */
> +}
>  
>  /*
>   * Empty the log for unmount/freeze.
> -- 
> 2.24.1
> 

Re: [PATCH 1/7] xfs: remove the unused return value from xfs_log_unmount_write

[Christoph Hellwig]

On Fri, Mar 06, 2020 at 11:09:17AM -0500, Brian Foster wrote:
> On Fri, Mar 06, 2020 at 07:31:31AM -0700, Christoph Hellwig wrote:
> > Remove the ignored return value from xfs_log_unmount_write, and also
> > remove a rather pointless assert on the return value from xfs_log_force.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> 
> I guess there's going to be obvious conflicts with Dave's series and
> some of these changes. I'm just going to ignore that and you guys can
> figure it out. :)

I'm glad to rebase this on top of the parts of his series that I think
make sense.  Just wanted to send this out for now to show what I have
in mind in this area.

Re: [PATCH 1/7] xfs: remove the unused return value from xfs_log_unmount_write

[Dave Chinner]

On Mon, Mar 09, 2020 at 09:02:52AM +0100, Christoph Hellwig wrote:
> On Fri, Mar 06, 2020 at 11:09:17AM -0500, Brian Foster wrote:
> > On Fri, Mar 06, 2020 at 07:31:31AM -0700, Christoph Hellwig wrote:
> > > Remove the ignored return value from xfs_log_unmount_write, and also
> > > remove a rather pointless assert on the return value from xfs_log_force.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > 
> > I guess there's going to be obvious conflicts with Dave's series and
> > some of these changes. I'm just going to ignore that and you guys can
> > figure it out. :)
> 
> I'm glad to rebase this on top of the parts of his series that I think
> make sense.  Just wanted to send this out for now to show what I have
> in mind in this area.

FWIW, I'm typing limited at the moment because of a finger injury.

I was planning to rebase mine on the first 6 patches of this series
(i.e. all but the IOERROR removal patch) a couple of days ago, but
I'm really slow at getting stuff done at the moment. So if Darrick
is happy with this patchset, don't let my cleanup hold it up.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/7] xfs: remove the unused return value from xfs_log_unmount_write

[Darrick J. Wong]

On Wed, Mar 11, 2020 at 09:28:56AM +1100, Dave Chinner wrote:
> On Mon, Mar 09, 2020 at 09:02:52AM +0100, Christoph Hellwig wrote:
> > On Fri, Mar 06, 2020 at 11:09:17AM -0500, Brian Foster wrote:
> > > On Fri, Mar 06, 2020 at 07:31:31AM -0700, Christoph Hellwig wrote:
> > > > Remove the ignored return value from xfs_log_unmount_write, and also
> > > > remove a rather pointless assert on the return value from xfs_log_force.
> > > > 
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > ---
> > > 
> > > I guess there's going to be obvious conflicts with Dave's series and
> > > some of these changes. I'm just going to ignore that and you guys can
> > > figure it out. :)
> > 
> > I'm glad to rebase this on top of the parts of his series that I think
> > make sense.  Just wanted to send this out for now to show what I have
> > in mind in this area.
> 
> FWIW, I'm typing limited at the moment because of a finger injury.
> 
> I was planning to rebase mine on the first 6 patches of this series
> (i.e. all but the IOERROR removal patch) a couple of days ago, but
> I'm really slow at getting stuff done at the moment. So if Darrick
> is happy with this patchset, don't let my cleanup hold it up.

Ahh, I'd held back to see what would develop. :)

I'll have a look then.

--D

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

Re: [PATCH 1/7] xfs: remove the unused return value from xfs_log_unmount_write

[Christoph Hellwig]

On Wed, Mar 11, 2020 at 09:28:56AM +1100, Dave Chinner wrote:
> FWIW, I'm typing limited at the moment because of a finger injury.
> 
> I was planning to rebase mine on the first 6 patches of this series
> (i.e. all but the IOERROR removal patch) a couple of days ago, but
> I'm really slow at getting stuff done at the moment. So if Darrick
> is happy with this patchset, don't let my cleanup hold it up.

Get better soon.  I'll resend just the trivial cleanups for now.

[PATCH 2/7] xfs: remove dead code from xfs_log_unmount_write

[Christoph Hellwig]

When the log is shut down all iclogs are in the XLOG_STATE_IOERROR state,
which means that xlog_state_want_sync and xlog_state_release_iclog are
no-ops.  Remove the whole section of code.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index fa499ddedb94..b56432d4a9b8 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -984,38 +984,9 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 		iclog = iclog->ic_next;
 	} while (iclog != first_iclog);
 #endif
-	if (! (XLOG_FORCED_SHUTDOWN(log))) {
-		xfs_log_write_unmount_record(mp);
-	} else {
-		/*
-		 * We're already in forced_shutdown mode, couldn't
-		 * even attempt to write out the unmount transaction.
-		 *
-		 * Go through the motions of sync'ing and releasing
-		 * the iclog, even though no I/O will actually happen,
-		 * we need to wait for other log I/Os that may already
-		 * be in progress.  Do this as a separate section of
-		 * code so we'll know if we ever get stuck here that
-		 * we're in this odd situation of trying to unmount
-		 * a file system that went into forced_shutdown as
-		 * the result of an unmount..
-		 */
-		spin_lock(&log->l_icloglock);
-		iclog = log->l_iclog;
-		atomic_inc(&iclog->ic_refcnt);
-		xlog_state_want_sync(log, iclog);
-		xlog_state_release_iclog(log, iclog);
-		switch (iclog->ic_state) {
-		case XLOG_STATE_ACTIVE:
-		case XLOG_STATE_DIRTY:
-		case XLOG_STATE_IOERROR:
-			spin_unlock(&log->l_icloglock);
-			break;
-		default:
-			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
-			break;
-		}
-	}
+	if (XLOG_FORCED_SHUTDOWN(log))
+		return;
+	xfs_log_write_unmount_record(mp);
 }
 
 /*
-- 
2.24.1

Re: [PATCH 2/7] xfs: remove dead code from xfs_log_unmount_write

[Brian Foster]

On Fri, Mar 06, 2020 at 07:31:32AM -0700, Christoph Hellwig wrote:
> When the log is shut down all iclogs are in the XLOG_STATE_IOERROR state,
> which means that xlog_state_want_sync and xlog_state_release_iclog are
> no-ops.  Remove the whole section of code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 35 +++--------------------------------
>  1 file changed, 3 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index fa499ddedb94..b56432d4a9b8 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -984,38 +984,9 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		iclog = iclog->ic_next;
>  	} while (iclog != first_iclog);
>  #endif
> -	if (! (XLOG_FORCED_SHUTDOWN(log))) {
> -		xfs_log_write_unmount_record(mp);
> -	} else {
> -		/*
> -		 * We're already in forced_shutdown mode, couldn't
> -		 * even attempt to write out the unmount transaction.
> -		 *
> -		 * Go through the motions of sync'ing and releasing
> -		 * the iclog, even though no I/O will actually happen,
> -		 * we need to wait for other log I/Os that may already
> -		 * be in progress.  Do this as a separate section of
> -		 * code so we'll know if we ever get stuck here that
> -		 * we're in this odd situation of trying to unmount
> -		 * a file system that went into forced_shutdown as
> -		 * the result of an unmount..
> -		 */
> -		spin_lock(&log->l_icloglock);
> -		iclog = log->l_iclog;
> -		atomic_inc(&iclog->ic_refcnt);
> -		xlog_state_want_sync(log, iclog);
> -		xlog_state_release_iclog(log, iclog);
> -		switch (iclog->ic_state) {
> -		case XLOG_STATE_ACTIVE:
> -		case XLOG_STATE_DIRTY:
> -		case XLOG_STATE_IOERROR:
> -			spin_unlock(&log->l_icloglock);
> -			break;
> -		default:
> -			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -			break;
> -		}
> -	}
> +	if (XLOG_FORCED_SHUTDOWN(log))
> +		return;
> +	xfs_log_write_unmount_record(mp);
>  }
>  
>  /*
> -- 
> 2.24.1
> 

[PATCH 3/7] xfs: cleanup xfs_log_unmount_write

[Christoph Hellwig]

Move the code for verifying the iclog state on a clean unmount into a
helper, and instead of checking the iclog state just rely on the shutdown
check as they are equivalent.  Also remove the ifdef DEBUG as the
compiler is smart enough to eliminate the dead code for non-debug builds.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index b56432d4a9b8..89f2e68eb570 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -946,6 +946,18 @@ xfs_log_write_unmount_record(
 	}
 }
 
+static void
+xfs_log_unmount_verify_iclog(
+	struct xlog	 	*log)
+{
+	struct xlog_in_core	 *iclog = log->l_iclog;
+
+	do {
+		ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
+		ASSERT(iclog->ic_offset == 0);
+	} while ((iclog = iclog->ic_next) != log->l_iclog);
+}
+
 /*
  * Unmount record used to have a string "Unmount filesystem--" in the
  * data section where the "Un" was really a magic number (XLOG_UNMOUNT_TYPE).
@@ -954,13 +966,10 @@ xfs_log_write_unmount_record(
  * As far as I know, there weren't any dependencies on the old behaviour.
  */
 static void
-xfs_log_unmount_write(xfs_mount_t *mp)
+xfs_log_unmount_write(
+	struct xfs_mount	*mp)
 {
-	struct xlog	 *log = mp->m_log;
-	xlog_in_core_t	 *iclog;
-#ifdef DEBUG
-	xlog_in_core_t	 *first_iclog;
-#endif
+	struct xlog	 	*log = mp->m_log;
 
 	/*
 	 * Don't write out unmount record on norecovery mounts or ro devices.
@@ -974,18 +983,9 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 
 	xfs_log_force(mp, XFS_LOG_SYNC);
 
-#ifdef DEBUG
-	first_iclog = iclog = log->l_iclog;
-	do {
-		if (iclog->ic_state != XLOG_STATE_IOERROR) {
-			ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
-			ASSERT(iclog->ic_offset == 0);
-		}
-		iclog = iclog->ic_next;
-	} while (iclog != first_iclog);
-#endif
 	if (XLOG_FORCED_SHUTDOWN(log))
 		return;
+	xfs_log_unmount_verify_iclog(log);
 	xfs_log_write_unmount_record(mp);
 }
 
-- 
2.24.1

Re: [PATCH 3/7] xfs: cleanup xfs_log_unmount_write

[Brian Foster]

On Fri, Mar 06, 2020 at 07:31:33AM -0700, Christoph Hellwig wrote:
> Move the code for verifying the iclog state on a clean unmount into a
> helper, and instead of checking the iclog state just rely on the shutdown
> check as they are equivalent.  Also remove the ifdef DEBUG as the
> compiler is smart enough to eliminate the dead code for non-debug builds.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index b56432d4a9b8..89f2e68eb570 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -946,6 +946,18 @@ xfs_log_write_unmount_record(
>  	}
>  }
>  
> +static void
> +xfs_log_unmount_verify_iclog(
> +	struct xlog	 	*log)
> +{
> +	struct xlog_in_core	 *iclog = log->l_iclog;
> +
> +	do {
> +		ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
> +		ASSERT(iclog->ic_offset == 0);
> +	} while ((iclog = iclog->ic_next) != log->l_iclog);
> +}
> +
>  /*
>   * Unmount record used to have a string "Unmount filesystem--" in the
>   * data section where the "Un" was really a magic number (XLOG_UNMOUNT_TYPE).
> @@ -954,13 +966,10 @@ xfs_log_write_unmount_record(
>   * As far as I know, there weren't any dependencies on the old behaviour.
>   */
>  static void
> -xfs_log_unmount_write(xfs_mount_t *mp)
> +xfs_log_unmount_write(
> +	struct xfs_mount	*mp)
>  {
> -	struct xlog	 *log = mp->m_log;
> -	xlog_in_core_t	 *iclog;
> -#ifdef DEBUG
> -	xlog_in_core_t	 *first_iclog;
> -#endif
> +	struct xlog	 	*log = mp->m_log;
>  
>  	/*
>  	 * Don't write out unmount record on norecovery mounts or ro devices.
> @@ -974,18 +983,9 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  
>  	xfs_log_force(mp, XFS_LOG_SYNC);
>  
> -#ifdef DEBUG
> -	first_iclog = iclog = log->l_iclog;
> -	do {
> -		if (iclog->ic_state != XLOG_STATE_IOERROR) {
> -			ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
> -			ASSERT(iclog->ic_offset == 0);
> -		}
> -		iclog = iclog->ic_next;
> -	} while (iclog != first_iclog);
> -#endif
>  	if (XLOG_FORCED_SHUTDOWN(log))
>  		return;
> +	xfs_log_unmount_verify_iclog(log);
>  	xfs_log_write_unmount_record(mp);
>  }
>  
> -- 
> 2.24.1
> 

[PATCH 4/7] 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 | 11 +++++-----
 3 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 89f2e68eb570..45f7a6eaddea 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,
@@ -1250,7 +1249,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);
@@ -1266,17 +1264,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);
 
 	/*
@@ -1755,7 +1745,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;
 	}
@@ -2779,8 +2769,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)
 {
@@ -2792,7 +2781,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);
 	}
 
@@ -2807,8 +2796,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;
@@ -2849,7 +2837,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;
@@ -2887,25 +2875,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
@@ -2914,9 +2899,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
@@ -3908,7 +3892,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 84e06805160f..ad87c5593ebd 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -138,7 +138,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 48435cf2aa16..b5c4a45c208c 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,7 @@ xlog_cil_push(
 out_abort_free_ticket:
 	xfs_log_ticket_put(tic);
 out_abort:
-	xlog_cil_committed(ctx, true);
+	xlog_cil_committed(ctx);
 	return -EIO;
 }
 
-- 
2.24.1

Re: [PATCH 4/7] xfs: remove the aborted parameter to xlog_state_done_syncing

[Brian Foster]

On Fri, Mar 06, 2020 at 07:31:34AM -0700, 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 | 11 +++++-----
>  3 files changed, 22 insertions(+), 39 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 48435cf2aa16..b5c4a45c208c 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
...
> @@ -878,7 +877,7 @@ xlog_cil_push(
>  out_abort_free_ticket:
>  	xfs_log_ticket_put(tic);
>  out_abort:
> -	xlog_cil_committed(ctx, true);
> +	xlog_cil_committed(ctx);

Error paths like this might warrant an assert. It's not really clear
that we expect to be shutdown based on the context. Otherwise looks Ok.

Brian

>  	return -EIO;
>  }
>  
> -- 
> 2.24.1
> 

Re: [PATCH 4/7] xfs: remove the aborted parameter to xlog_state_done_syncing

[Christoph Hellwig]

On Fri, Mar 06, 2020 at 12:12:01PM -0500, Brian Foster wrote:
> >  out_abort:
> > -	xlog_cil_committed(ctx, true);
> > +	xlog_cil_committed(ctx);
> 
> Error paths like this might warrant an assert. It's not really clear
> that we expect to be shutdown based on the context. Otherwise looks Ok.

I've added an assert to the next version.

[PATCH 5/7] xfs: factor out a xlog_state_activate_iclog helper

[Christoph Hellwig]

Factor out the code to mark an iclog a active into a new helper.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 45f7a6eaddea..d1accad13af4 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2536,6 +2536,38 @@ xlog_write(
  *****************************************************************************
  */
 
+static void
+xlog_state_activate_iclog(
+	struct xlog_in_core	*iclog,
+	int			*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 (!*changed &&
+	    iclog->ic_header.h_num_logops == cpu_to_be32(XLOG_COVER_OPS)) {
+		*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.
+		 */
+		*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
@@ -2567,38 +2599,10 @@ xlog_state_clean_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 */
+		if (iclog->ic_state == XLOG_STATE_DIRTY)
+			xlog_state_activate_iclog(iclog, &changed);
+		else if (iclog->ic_state != XLOG_STATE_ACTIVE)
+			break;
 		iclog = iclog->ic_next;
 	} while (iclog != log->l_iclog);
 
-- 
2.24.1

Re: [PATCH 5/7] xfs: factor out a xlog_state_activate_iclog helper

[Brian Foster]

On Fri, Mar 06, 2020 at 07:31:35AM -0700, Christoph Hellwig wrote:
> Factor out the code to mark an iclog a active into a new helper.

"mark as active" or just "mark active"

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

Otherwise looks fine:

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

>  fs/xfs/xfs_log.c | 68 +++++++++++++++++++++++++-----------------------
>  1 file changed, 36 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 45f7a6eaddea..d1accad13af4 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2536,6 +2536,38 @@ xlog_write(
>   *****************************************************************************
>   */
>  
> +static void
> +xlog_state_activate_iclog(
> +	struct xlog_in_core	*iclog,
> +	int			*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 (!*changed &&
> +	    iclog->ic_header.h_num_logops == cpu_to_be32(XLOG_COVER_OPS)) {
> +		*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.
> +		 */
> +		*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
> @@ -2567,38 +2599,10 @@ xlog_state_clean_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 */
> +		if (iclog->ic_state == XLOG_STATE_DIRTY)
> +			xlog_state_activate_iclog(iclog, &changed);
> +		else if (iclog->ic_state != XLOG_STATE_ACTIVE)
> +			break;
>  		iclog = iclog->ic_next;
>  	} while (iclog != log->l_iclog);
>  
> -- 
> 2.24.1
> 

Re: [PATCH 5/7] xfs: factor out a xlog_state_activate_iclog helper

[Christoph Hellwig]

On Fri, Mar 06, 2020 at 12:12:13PM -0500, Brian Foster wrote:
> On Fri, Mar 06, 2020 at 07:31:35AM -0700, Christoph Hellwig wrote:
> > Factor out the code to mark an iclog a active into a new helper.
> 
> "mark as active" or just "mark active"

I've fixed this, but then replaced this patch with a bigger one
doing additional refactoring.

[PATCH 6/7] xfs: cleanup xlog_state_clean_iclog

[Christoph Hellwig]

Use the shutdown flag in the log to bypass the iclog processing
instead of looking at the ioerror flag, and slightly simplify the
while loop processing.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index d1accad13af4..fae5107099b1 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2582,30 +2582,29 @@ xlog_state_activate_iclog(
  *
  * Caller must hold the icloglock before calling us.
  *
- * State Change: !IOERROR -> DIRTY -> ACTIVE
+ * State Change: CALLBACK -> DIRTY -> ACTIVE
  */
 STATIC void
 xlog_state_clean_iclog(
 	struct xlog		*log,
 	struct xlog_in_core	*dirty_iclog)
 {
-	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;
+	if (!XLOG_FORCED_SHUTDOWN(log)) {
+		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)
-			xlog_state_activate_iclog(iclog, &changed);
-		else if (iclog->ic_state != XLOG_STATE_ACTIVE)
-			break;
-		iclog = iclog->ic_next;
-	} while (iclog != log->l_iclog);
+		/* Prepare the completed iclog. */
+		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
 
+		/* Walk all the iclogs to update the ordered active state. */
+		do {
+			if (iclog->ic_state == XLOG_STATE_DIRTY)
+				xlog_state_activate_iclog(iclog, &changed);
+			else if (iclog->ic_state != XLOG_STATE_ACTIVE)
+				break;
+		} while ((iclog = iclog->ic_next) != log->l_iclog);
+	}
 
 	/*
 	 * Wake up threads waiting in xfs_log_force() for the dirty iclog
-- 
2.24.1

Re: [PATCH 6/7] xfs: cleanup xlog_state_clean_iclog

[Brian Foster]

On Fri, Mar 06, 2020 at 07:31:36AM -0700, Christoph Hellwig wrote:
> Use the shutdown flag in the log to bypass the iclog processing
> instead of looking at the ioerror flag, and slightly simplify the
> while loop processing.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_log.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index d1accad13af4..fae5107099b1 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2582,30 +2582,29 @@ xlog_state_activate_iclog(
>   *
>   * Caller must hold the icloglock before calling us.
>   *
> - * State Change: !IOERROR -> DIRTY -> ACTIVE
> + * State Change: CALLBACK -> DIRTY -> ACTIVE
>   */
>  STATIC void
>  xlog_state_clean_iclog(
>  	struct xlog		*log,
>  	struct xlog_in_core	*dirty_iclog)
>  {
> -	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;
> +	if (!XLOG_FORCED_SHUTDOWN(log)) {
> +		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)
> -			xlog_state_activate_iclog(iclog, &changed);
> -		else if (iclog->ic_state != XLOG_STATE_ACTIVE)
> -			break;
> -		iclog = iclog->ic_next;
> -	} while (iclog != log->l_iclog);
> +		/* Prepare the completed iclog. */
> +		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>  
> +		/* Walk all the iclogs to update the ordered active state. */
> +		do {
> +			if (iclog->ic_state == XLOG_STATE_DIRTY)
> +				xlog_state_activate_iclog(iclog, &changed);
> +			else if (iclog->ic_state != XLOG_STATE_ACTIVE)
> +				break;
> +		} while ((iclog = iclog->ic_next) != log->l_iclog);
> +	}
>  
>  	/*
>  	 * Wake up threads waiting in xfs_log_force() for the dirty iclog
> -- 
> 2.24.1
> 

[PATCH 7/7] xfs: kill 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.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index fae5107099b1..1bcd5c735d6b 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -583,7 +583,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) &&
@@ -604,11 +604,11 @@ xfs_log_release_iclog(
 	struct xlog		*log = mp->m_log;
 	bool			sync;
 
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
+	if (XLOG_FORCED_SHUTDOWN(log))
 		goto error;
 
 	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
-		if (iclog->ic_state == XLOG_STATE_IOERROR) {
+		if (XLOG_FORCED_SHUTDOWN(log)) {
 			spin_unlock(&log->l_icloglock);
 			goto error;
 		}
@@ -914,7 +914,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)
@@ -1737,7 +1737,7 @@ xlog_write_iclog(
 	 * across the log IO to archieve that.
 	 */
 	down(&iclog->ic_sema);
-	if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) {
+	if (unlikely(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
@@ -2721,6 +2721,17 @@ xlog_state_iodone_process_iclog(
 	xfs_lsn_t		lowest_lsn;
 	xfs_lsn_t		header_lsn;
 
+	/*
+	 * Between marking a filesystem SHUTDOWN and stopping the log, we do
+	 * flush all iclogs to disk (if there wasn't a log I/O error).  So, we
+	 * do want things to go smoothly in case of just a SHUTDOWN w/o a
+	 * LOG_IO_ERROR.
+	 */
+	if (XLOG_FORCED_SHUTDOWN(log)) {
+		*ioerror = true;
+		return false;
+	}
+
 	switch (iclog->ic_state) {
 	case XLOG_STATE_ACTIVE:
 	case XLOG_STATE_DIRTY:
@@ -2728,15 +2739,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
@@ -2830,7 +2832,7 @@ xlog_state_do_callback(
 				break;
 
 			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
-			    iclog->ic_state != XLOG_STATE_IOERROR) {
+			    !XLOG_FORCED_SHUTDOWN(log)) {
 				iclog = iclog->ic_next;
 				continue;
 			}
@@ -2856,7 +2858,7 @@ xlog_state_do_callback(
 	} while (!ioerror && cycled_icloglock);
 
 	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
-	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
+	    XLOG_FORCED_SHUTDOWN(log))
 		wake_up_all(&log->l_flush_wait);
 
 	spin_unlock(&log->l_icloglock);
@@ -3202,7 +3204,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 ||
@@ -3259,11 +3261,11 @@ xfs_log_force(
 	if (!(flags & XFS_LOG_SYNC))
 		goto out_unlock;
 
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
+	if (XLOG_FORCED_SHUTDOWN(log))
 		goto out_error;
 	XFS_STATS_INC(mp, xs_log_force_sleep);
 	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
+	if (XLOG_FORCED_SHUTDOWN(log))
 		return -EIO;
 	return 0;
 
@@ -3288,7 +3290,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) {
@@ -3338,12 +3340,12 @@ __xfs_log_force_lsn(
 	     iclog->ic_state == XLOG_STATE_DIRTY))
 		goto out_unlock;
 
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
+	if (XLOG_FORCED_SHUTDOWN(log))
 		goto out_error;
 
 	XFS_STATS_INC(mp, xs_log_force_sleep);
 	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
-	if (iclog->ic_state == XLOG_STATE_IOERROR)
+	if (XLOG_FORCED_SHUTDOWN(log))
 		return -EIO;
 	return 0;
 
@@ -3407,7 +3409,7 @@ xlog_state_want_sync(
 		xlog_state_switch_iclogs(log, iclog, 0);
 	} else {
 		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
-		       iclog->ic_state == XLOG_STATE_IOERROR);
+		       XLOG_FORCED_SHUTDOWN(log));
 	}
 }
 
@@ -3774,34 +3776,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.
@@ -3823,10 +3797,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
@@ -3844,10 +3816,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
@@ -3869,11 +3839,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);
 
 	/*
@@ -3897,7 +3869,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 b5c4a45c208c..41a45d75a2d0 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -846,7 +846,7 @@ xlog_cil_push(
 		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 b192c5a9f9fd..fd4c913ee7e6 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 7/7] xfs: kill XLOG_STATE_IOERROR

[Brian Foster]

On Fri, Mar 06, 2020 at 07:31:37AM -0700, Christoph Hellwig wrote:
> Just check the shutdown flag in struct xlog, instead of replicating the
> information into each iclog and checking it there.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Most of the code seems Ok to me, but as I work through this series I'm
starting to have a harder time seeing the value of removing the error
state in the current code. Of course there's obvious value of less code
and whatnot, but the state code that's being removed is mostly busted
code or very simple (i.e. the shutdown helper). In contrast, all of
these checks that remain sprinkled throughout the log code change
specific iclog checks into broader state checks where we now have to
consider what new racing might or might not occur, etc.

In the end the fs is busted and we're shutting down, but at the same
time shutdown has consistently been riddled with subtle
race/locking/state bugs that are low impact but quite annoying to track
down. I do see value in simplifying the log error handling overall as
the previous patches start to do, but ISTM that should be the primary
goal here. IOW, to simplify the bigger picture logic first to the point
where this patch should be much, much smaller than it is.

If we were to first significantly reduce the number of error state
checks required throughout this code (i.e. reduced to the minimum
critical points necessary that ensure we don't do more log I/O or other
"bad things"), _then_ I see the value of a patch to kill off the error
state. Until we get to that point, this kind of strikes me as
rejiggering complexity around. For example, things like how
xlog_state_do_callback() passes ioerror to
xlog_state_iodone_process_iclog(), which assigns it based on shutdown
state, only for the caller to also check the shutdown state again are
indication that more cleanup is in order before killing off the state.

Brian

>  fs/xfs/xfs_log.c      | 95 +++++++++++++++----------------------------
>  fs/xfs/xfs_log_cil.c  |  2 +-
>  fs/xfs/xfs_log_priv.h |  1 -
>  3 files changed, 34 insertions(+), 64 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index fae5107099b1..1bcd5c735d6b 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -583,7 +583,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) &&
> @@ -604,11 +604,11 @@ xfs_log_release_iclog(
>  	struct xlog		*log = mp->m_log;
>  	bool			sync;
>  
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> +	if (XLOG_FORCED_SHUTDOWN(log))
>  		goto error;
>  
>  	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
> -		if (iclog->ic_state == XLOG_STATE_IOERROR) {
> +		if (XLOG_FORCED_SHUTDOWN(log)) {
>  			spin_unlock(&log->l_icloglock);
>  			goto error;
>  		}
> @@ -914,7 +914,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)
> @@ -1737,7 +1737,7 @@ xlog_write_iclog(
>  	 * across the log IO to archieve that.
>  	 */
>  	down(&iclog->ic_sema);
> -	if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) {
> +	if (unlikely(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
> @@ -2721,6 +2721,17 @@ xlog_state_iodone_process_iclog(
>  	xfs_lsn_t		lowest_lsn;
>  	xfs_lsn_t		header_lsn;
>  
> +	/*
> +	 * Between marking a filesystem SHUTDOWN and stopping the log, we do
> +	 * flush all iclogs to disk (if there wasn't a log I/O error).  So, we
> +	 * do want things to go smoothly in case of just a SHUTDOWN w/o a
> +	 * LOG_IO_ERROR.
> +	 */
> +	if (XLOG_FORCED_SHUTDOWN(log)) {
> +		*ioerror = true;
> +		return false;
> +	}
> +
>  	switch (iclog->ic_state) {
>  	case XLOG_STATE_ACTIVE:
>  	case XLOG_STATE_DIRTY:
> @@ -2728,15 +2739,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
> @@ -2830,7 +2832,7 @@ xlog_state_do_callback(
>  				break;
>  
>  			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
> -			    iclog->ic_state != XLOG_STATE_IOERROR) {
> +			    !XLOG_FORCED_SHUTDOWN(log)) {
>  				iclog = iclog->ic_next;
>  				continue;
>  			}
> @@ -2856,7 +2858,7 @@ xlog_state_do_callback(
>  	} while (!ioerror && cycled_icloglock);
>  
>  	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
> -	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
> +	    XLOG_FORCED_SHUTDOWN(log))
>  		wake_up_all(&log->l_flush_wait);
>  
>  	spin_unlock(&log->l_icloglock);
> @@ -3202,7 +3204,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 ||
> @@ -3259,11 +3261,11 @@ xfs_log_force(
>  	if (!(flags & XFS_LOG_SYNC))
>  		goto out_unlock;
>  
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> +	if (XLOG_FORCED_SHUTDOWN(log))
>  		goto out_error;
>  	XFS_STATS_INC(mp, xs_log_force_sleep);
>  	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> +	if (XLOG_FORCED_SHUTDOWN(log))
>  		return -EIO;
>  	return 0;
>  
> @@ -3288,7 +3290,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) {
> @@ -3338,12 +3340,12 @@ __xfs_log_force_lsn(
>  	     iclog->ic_state == XLOG_STATE_DIRTY))
>  		goto out_unlock;
>  
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> +	if (XLOG_FORCED_SHUTDOWN(log))
>  		goto out_error;
>  
>  	XFS_STATS_INC(mp, xs_log_force_sleep);
>  	xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -	if (iclog->ic_state == XLOG_STATE_IOERROR)
> +	if (XLOG_FORCED_SHUTDOWN(log))
>  		return -EIO;
>  	return 0;
>  
> @@ -3407,7 +3409,7 @@ xlog_state_want_sync(
>  		xlog_state_switch_iclogs(log, iclog, 0);
>  	} else {
>  		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -		       iclog->ic_state == XLOG_STATE_IOERROR);
> +		       XLOG_FORCED_SHUTDOWN(log));
>  	}
>  }
>  
> @@ -3774,34 +3776,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.
> @@ -3823,10 +3797,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
> @@ -3844,10 +3816,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
> @@ -3869,11 +3839,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);
>  
>  	/*
> @@ -3897,7 +3869,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 b5c4a45c208c..41a45d75a2d0 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -846,7 +846,7 @@ xlog_cil_push(
>  		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 b192c5a9f9fd..fd4c913ee7e6 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 7/7] xfs: kill XLOG_STATE_IOERROR

[Christoph Hellwig]

On Fri, Mar 06, 2020 at 12:15:45PM -0500, Brian Foster wrote:
> If we were to first significantly reduce the number of error state
> checks required throughout this code (i.e. reduced to the minimum
> critical points necessary that ensure we don't do more log I/O or other
> "bad things"), _then_ I see the value of a patch to kill off the error
> state. Until we get to that point, this kind of strikes me as
> rejiggering complexity around. For example, things like how
> xlog_state_do_callback() passes ioerror to
> xlog_state_iodone_process_iclog(), which assigns it based on shutdown
> state, only for the caller to also check the shutdown state again are
> indication that more cleanup is in order before killing off the state.

I've added a few more patches fixing up some low hanging fruit and
completely redoing the code structure around
xlog_state_iodone_process_iclog.  Although that means the series keeps
growing, so I might split it into multiple series with some easier
prep patches and then the bigger changes in a second round.