diff mbox series

[09/11] xfs: merge unmount record write iclog cleanup.

Message ID 20200304075401.21558-10-david@fromorbit.com (mailing list archive)
State New, archived
Headers show
Series xfs: clean up log tickets and record writes | expand

Commit Message

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

The unmount iclog handling is duplicated in both
xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
need one copy of it in xfs_log_unmount_write() because that is the
only function that calls xfs_log_write_unmount_record().

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

Comments

Christoph Hellwig March 4, 2020, 3:53 p.m. UTC | #1 
On Wed, Mar 04, 2020 at 06:53:59PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The unmount iclog handling is duplicated in both
> xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
> need one copy of it in xfs_log_unmount_write() because that is the
> only function that calls xfs_log_write_unmount_record().

The copy in xfs_log_unmount_write actually is dead code.  It only
is called in the XLOG_FORCED_SHUTDOWN case, in which case all iclogs
are marked as STATE_IOERROR, and thus xlog_state_release_iclog is
a no-op.  I really need to send the series out to clean this up
ASAP..
Dave Chinner March 4, 2020, 9:38 p.m. UTC | #2 
On Wed, Mar 04, 2020 at 07:53:32AM -0800, Christoph Hellwig wrote:
> On Wed, Mar 04, 2020 at 06:53:59PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The unmount iclog handling is duplicated in both
> > xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
> > need one copy of it in xfs_log_unmount_write() because that is the
> > only function that calls xfs_log_write_unmount_record().
> 
> The copy in xfs_log_unmount_write actually is dead code.  It only
> is called in the XLOG_FORCED_SHUTDOWN case, in which case all iclogs
> are marked as STATE_IOERROR, and thus xlog_state_release_iclog is
> a no-op.  I really need to send the series out to clean this up
> ASAP..

Well, this patch pretty much solves that "dead code" problem in that
it now handles already shut down, error in unmount record write and
successful unmount record write now. i.e. we run the same code in
all cases now, so you'll only need to fix the IOERROR handling in
one place :P

Cheers,

Dave.
Christoph Hellwig March 5, 2020, 3:27 p.m. UTC | #3 
On Thu, Mar 05, 2020 at 08:38:54AM +1100, Dave Chinner wrote:
> On Wed, Mar 04, 2020 at 07:53:32AM -0800, Christoph Hellwig wrote:
> > On Wed, Mar 04, 2020 at 06:53:59PM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > The unmount iclog handling is duplicated in both
> > > xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
> > > need one copy of it in xfs_log_unmount_write() because that is the
> > > only function that calls xfs_log_write_unmount_record().
> > 
> > The copy in xfs_log_unmount_write actually is dead code.  It only
> > is called in the XLOG_FORCED_SHUTDOWN case, in which case all iclogs
> > are marked as STATE_IOERROR, and thus xlog_state_release_iclog is
> > a no-op.  I really need to send the series out to clean this up
> > ASAP..
> 
> Well, this patch pretty much solves that "dead code" problem in that
> it now handles already shut down, error in unmount record write and
> successful unmount record write now. i.e. we run the same code in
> all cases now, so you'll only need to fix the IOERROR handling in
> one place :P

It doesn't really.  We still end up with more convoluted logic after
your series, that goes through a whole bunch of steps that don't make
any sense when the log is already shut down.  Just like everywhere
else we should just return early with a shutdown log / iclog and just
delete this code instead of rearranging the chairs.
Brian Foster March 5, 2020, 6:08 p.m. UTC | #4 
On Wed, Mar 04, 2020 at 06:53:59PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The unmount iclog handling is duplicated in both
> xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
> need one copy of it in xfs_log_unmount_write() because that is the
> only function that calls xfs_log_write_unmount_record().
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Any reason for the period at the end of the patch subject? I just
noticed it now, but the previous patch has one as well.

>  fs/xfs/xfs_log.c | 130 +++++++++++++++++------------------------------
>  1 file changed, 48 insertions(+), 82 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index bdf604d31d8c..a687c20dd77d 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
...
> @@ -931,61 +926,41 @@ xlog_unmount_write(
>  	}
>  
>  	error = xlog_write_unmount_record(log, tic, &lsn, flags);
> -	/*
> -	 * At this point, we're umounting anyway, so there's no point in
> -	 * transitioning log state to IOERROR. Just continue...
> -	 */
> -out_err:
> -	if (error)
> -		xfs_alert(mp, "%s: unmount record failed", __func__);
> -
> -	spin_lock(&log->l_icloglock);
> -	iclog = log->l_iclog;
> -	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:
> +	if (error) {
> +		/* A full shutdown is unnecessary at this point of unmount */
> +		spin_lock(&log->l_icloglock);
> +		log->l_flags |= XLOG_IO_ERROR;

A previous patch added this to xlog_state_ioerror(). Nits aside, the
rest looks fine to me:

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

> +		xlog_state_ioerror(log);
>  		spin_unlock(&log->l_icloglock);
> -		break;
>  	}
>  
> -	if (tic) {
> -		trace_xfs_log_umount_write(log, tic);
> -		xlog_ungrant_log_space(log, tic);
> -		xfs_log_ticket_put(tic);
> -	}
> +	trace_xfs_log_umount_write(log, tic);
> +	xlog_ungrant_log_space(log, tic);
> +	xfs_log_ticket_put(tic);
> +out_err:
> +	if (error)
> +		xfs_alert(mp, "%s: unmount record failed", __func__);
> +	return error;
>  }
>  
>  /*
> - * Unmount record used to have a string "Unmount filesystem--" in the
> - * data section where the "Un" was really a magic number (XLOG_UNMOUNT_TYPE).
> - * We just write the magic number now since that particular field isn't
> - * currently architecture converted and "Unmount" is a bit foo.
> - * As far as I know, there weren't any dependencies on the old behaviour.
> + * Finalise the unmount by writing the unmount record to the log. This is the
> + * mark that the filesystem was cleanly unmounted.
> + *
> + * Avoid writing the unmount record on no-recovery mounts, ro-devices, or when
> + * the log has already been shut down.
>   */
> -
>  static int
> -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;
> +	struct xlog		*log = mp->m_log;
> +	struct xlog_in_core	*iclog;
>  #ifdef DEBUG
> -	xlog_in_core_t	 *first_iclog;
> +	struct xlog_in_core	*first_iclog;
>  #endif
> -	int		 error;
> +	int			error;
>  
> -	/*
> -	 * Don't write out unmount record on norecovery mounts or ro devices.
> -	 * Or, if we are doing a forced umount (typically because of IO errors).
> -	 */
>  	if (mp->m_flags & XFS_MOUNT_NORECOVERY ||
>  	    xfs_readonly_buftarg(log->l_targ)) {
>  		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
> @@ -1005,41 +980,32 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		iclog = iclog->ic_next;
>  	} while (iclog != first_iclog);
>  #endif
> -	if (! (XLOG_FORCED_SHUTDOWN(log))) {
> -		xlog_unmount_write(log);
> -	} 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);
> -		error =  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))
> +		error = xlog_unmount_write(log);
> +
> +	/*
> +	 * Sync and release the current iclog so the unmount record gets to
> +	 * disk. If we are in a shutdown state, no IO will be done, but we still
> +	 * we need to wait for other log I/Os that may already be in progress.
> +	 */
> +	spin_lock(&log->l_icloglock);
> +	iclog = log->l_iclog;
> +	atomic_inc(&iclog->ic_refcnt);
> +	xlog_state_want_sync(log, iclog);
> +	error =  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;
> +	}
>  	return error;
> -}	/* xfs_log_unmount_write */
> +}
>  
>  /*
>   * Empty the log for unmount/freeze.
> -- 
> 2.24.0.rc0
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index bdf604d31d8c..a687c20dd77d 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -898,16 +898,11 @@  xlog_state_ioerror(
 	return 0;
 }
 
-/*
- * Mark the filesystem clean by writing an unmount record to the head of the
- * log.
- */
-static void
+static int
 xlog_unmount_write(
 	struct xlog		*log)
 {
 	struct xfs_mount	*mp = log->l_mp;
-	struct xlog_in_core	*iclog;
 	struct xlog_ticket	*tic = NULL;
 	xfs_lsn_t		lsn;
 	uint			flags = XLOG_UNMOUNT_TRANS;
@@ -931,61 +926,41 @@  xlog_unmount_write(
 	}
 
 	error = xlog_write_unmount_record(log, tic, &lsn, flags);
-	/*
-	 * At this point, we're umounting anyway, so there's no point in
-	 * transitioning log state to IOERROR. Just continue...
-	 */
-out_err:
-	if (error)
-		xfs_alert(mp, "%s: unmount record failed", __func__);
-
-	spin_lock(&log->l_icloglock);
-	iclog = log->l_iclog;
-	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:
+	if (error) {
+		/* A full shutdown is unnecessary at this point of unmount */
+		spin_lock(&log->l_icloglock);
+		log->l_flags |= XLOG_IO_ERROR;
+		xlog_state_ioerror(log);
 		spin_unlock(&log->l_icloglock);
-		break;
 	}
 
-	if (tic) {
-		trace_xfs_log_umount_write(log, tic);
-		xlog_ungrant_log_space(log, tic);
-		xfs_log_ticket_put(tic);
-	}
+	trace_xfs_log_umount_write(log, tic);
+	xlog_ungrant_log_space(log, tic);
+	xfs_log_ticket_put(tic);
+out_err:
+	if (error)
+		xfs_alert(mp, "%s: unmount record failed", __func__);
+	return error;
 }
 
 /*
- * Unmount record used to have a string "Unmount filesystem--" in the
- * data section where the "Un" was really a magic number (XLOG_UNMOUNT_TYPE).
- * We just write the magic number now since that particular field isn't
- * currently architecture converted and "Unmount" is a bit foo.
- * As far as I know, there weren't any dependencies on the old behaviour.
+ * Finalise the unmount by writing the unmount record to the log. This is the
+ * mark that the filesystem was cleanly unmounted.
+ *
+ * Avoid writing the unmount record on no-recovery mounts, ro-devices, or when
+ * the log has already been shut down.
  */
-
 static int
-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;
+	struct xlog		*log = mp->m_log;
+	struct xlog_in_core	*iclog;
 #ifdef DEBUG
-	xlog_in_core_t	 *first_iclog;
+	struct xlog_in_core	*first_iclog;
 #endif
-	int		 error;
+	int			error;
 
-	/*
-	 * Don't write out unmount record on norecovery mounts or ro devices.
-	 * Or, if we are doing a forced umount (typically because of IO errors).
-	 */
 	if (mp->m_flags & XFS_MOUNT_NORECOVERY ||
 	    xfs_readonly_buftarg(log->l_targ)) {
 		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
@@ -1005,41 +980,32 @@  xfs_log_unmount_write(xfs_mount_t *mp)
 		iclog = iclog->ic_next;
 	} while (iclog != first_iclog);
 #endif
-	if (! (XLOG_FORCED_SHUTDOWN(log))) {
-		xlog_unmount_write(log);
-	} 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);
-		error =  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))
+		error = xlog_unmount_write(log);
+
+	/*
+	 * Sync and release the current iclog so the unmount record gets to
+	 * disk. If we are in a shutdown state, no IO will be done, but we still
+	 * we need to wait for other log I/Os that may already be in progress.
+	 */
+	spin_lock(&log->l_icloglock);
+	iclog = log->l_iclog;
+	atomic_inc(&iclog->ic_refcnt);
+	xlog_state_want_sync(log, iclog);
+	error =  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;
+	}
 	return error;
-}	/* xfs_log_unmount_write */
+}
 
 /*
  * Empty the log for unmount/freeze.