[14/14] xfs: remove XLOG_STATE_IOERROR
diff mbox series

Message ID 20200316144233.900390-15-hch@lst.de
State New
Headers show
Series
  • [01/14] xfs: merge xlog_cil_push into xlog_cil_push_work
Related show

Commit Message

Christoph Hellwig March 16, 2020, 2:42 p.m. UTC
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(-)

Comments

Darrick J. Wong March 16, 2020, 9:25 p.m. UTC | #1
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
>
Christoph Hellwig March 18, 2020, 9:43 a.m. UTC | #2
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.

Patch
diff mbox series

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 */
 };
 
 /*