[06/11] xfs: move xlog_state_ioerror()
diff mbox series

Message ID 20200304075401.21558-7-david@fromorbit.com
State New
Headers show
Series
  • xfs: clean up log tickets and record writes
Related show

Commit Message

Dave Chinner March 4, 2020, 7:53 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

To clean up unmount record writing error handling, we need to move
xlog_state_ioerror() higher up in the file. Also move the setting of
the XLOG_IO_ERROR state to inside the function.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 59 ++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

Comments

Christoph Hellwig March 4, 2020, 3:51 p.m. UTC | #1
On Wed, Mar 04, 2020 at 06:53:56PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> To clean up unmount record writing error handling, we need to move
> xlog_state_ioerror() higher up in the file. Also move the setting of
> the XLOG_IO_ERROR state to inside the function.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

FYI, I have a pending series that kills xlog_state_ioerror and
XLOG_IO_ERROR.  Let me send that out now that Brians fix is in for-next.
Dave Chinner March 4, 2020, 9:41 p.m. UTC | #2
On Wed, Mar 04, 2020 at 07:51:40AM -0800, Christoph Hellwig wrote:
> On Wed, Mar 04, 2020 at 06:53:56PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > To clean up unmount record writing error handling, we need to move
> > xlog_state_ioerror() higher up in the file. Also move the setting of
> > the XLOG_IO_ERROR state to inside the function.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> FYI, I have a pending series that kills xlog_state_ioerror and
> XLOG_IO_ERROR.  Let me send that out now that Brians fix is in for-next.

Can you rebase that on top of this? removing IOERROR is a much more
invasive and riskier set of state machine changes compared to what
this patchset does. This patchset simplifies some of the error
handling that handles IOERROR, too...

Cheers,

Dave.
Christoph Hellwig March 5, 2020, 3:21 p.m. UTC | #3
On Thu, Mar 05, 2020 at 08:41:15AM +1100, Dave Chinner wrote:
> On Wed, Mar 04, 2020 at 07:51:40AM -0800, Christoph Hellwig wrote:
> > On Wed, Mar 04, 2020 at 06:53:56PM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > To clean up unmount record writing error handling, we need to move
> > > xlog_state_ioerror() higher up in the file. Also move the setting of
> > > the XLOG_IO_ERROR state to inside the function.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > 
> > FYI, I have a pending series that kills xlog_state_ioerror and
> > XLOG_IO_ERROR.  Let me send that out now that Brians fix is in for-next.
> 
> Can you rebase that on top of this? removing IOERROR is a much more
> invasive and riskier set of state machine changes compared to what
> this patchset does. This patchset simplifies some of the error
> handling that handles IOERROR, too...

I find this lipstick on the pig here a little pointless, especially
moving things around that are planned to be removed..
Brian Foster March 5, 2020, 6:07 p.m. UTC | #4
On Wed, Mar 04, 2020 at 06:53:56PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> To clean up unmount record writing error handling, we need to move
> xlog_state_ioerror() higher up in the file. Also move the setting of
> the XLOG_IO_ERROR state to inside the function.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log.c | 59 ++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 2e9f3baa7cc8..0de3c32d42b6 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -874,6 +874,36 @@ xfs_log_mount_cancel(
>  	xfs_log_unmount(mp);
>  }
>  
> +/*
> + * 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;

Might as well kill off the typedef usage, though it sounds like
Christoph has code to remove this. Eh, that discussion aside this looks
fine to me:

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

> +
> +	log->l_flags |= XLOG_IO_ERROR;
> +
> +	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;
> +}
> +
>  /*
>   * Mark the filesystem clean by writing an unmount record to the head of the
>   * log.
> @@ -3770,34 +3800,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.
> @@ -3868,7 +3870,6 @@ xfs_log_force_umount(
>  	 * Mark the log and the iclogs with IO error flags to prevent any
>  	 * further log IO from being issued or completed.
>  	 */
> -	log->l_flags |= XLOG_IO_ERROR;
>  	retval = xlog_state_ioerror(log);
>  	spin_unlock(&log->l_icloglock);
>  
> -- 
> 2.24.0.rc0
>

Patch
diff mbox series

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 2e9f3baa7cc8..0de3c32d42b6 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -874,6 +874,36 @@  xfs_log_mount_cancel(
 	xfs_log_unmount(mp);
 }
 
+/*
+ * 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;
+
+	log->l_flags |= XLOG_IO_ERROR;
+
+	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;
+}
+
 /*
  * Mark the filesystem clean by writing an unmount record to the head of the
  * log.
@@ -3770,34 +3800,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.
@@ -3868,7 +3870,6 @@  xfs_log_force_umount(
 	 * Mark the log and the iclogs with IO error flags to prevent any
 	 * further log IO from being issued or completed.
 	 */
-	log->l_flags |= XLOG_IO_ERROR;
 	retval = xlog_state_ioerror(log);
 	spin_unlock(&log->l_icloglock);