misc log cleanups

misc log cleanups

[Christoph Hellwig]

Hi all,

this series contains the simple and uncontroversial patches from the
"cleanup log I/O error handling", plus a few other trivial cleanups I
found while refactoring that series.

[PATCH 1/5] xfs: mark XLOG_FORCED_SHUTDOWN as unlikely

[Christoph Hellwig]

A shutdown log is a slow failure path.  Add an unlikely annotation to
it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log_priv.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index b192c5a9f9fd..e400170ff4af 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -402,7 +402,8 @@ struct xlog {
 #define XLOG_BUF_CANCEL_BUCKET(log, blkno) \
 	((log)->l_buf_cancel_table + ((uint64_t)blkno % XLOG_BC_TABLE_SIZE))
 
-#define XLOG_FORCED_SHUTDOWN(log)	((log)->l_flags & XLOG_IO_ERROR)
+#define XLOG_FORCED_SHUTDOWN(log) \
+	(unlikely((log)->l_flags & XLOG_IO_ERROR))
 
 /* common routines */
 extern int
-- 
2.24.1

Re: [PATCH 1/5] xfs: mark XLOG_FORCED_SHUTDOWN as unlikely

[Darrick J. Wong]

On Thu, Mar 12, 2020 at 03:39:55PM +0100, Christoph Hellwig wrote:
> A shutdown log is a slow failure path.  Add an unlikely annotation to
> it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log_priv.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index b192c5a9f9fd..e400170ff4af 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -402,7 +402,8 @@ struct xlog {
>  #define XLOG_BUF_CANCEL_BUCKET(log, blkno) \
>  	((log)->l_buf_cancel_table + ((uint64_t)blkno % XLOG_BC_TABLE_SIZE))
>  
> -#define XLOG_FORCED_SHUTDOWN(log)	((log)->l_flags & XLOG_IO_ERROR)
> +#define XLOG_FORCED_SHUTDOWN(log) \
> +	(unlikely((log)->l_flags & XLOG_IO_ERROR))
>  
>  /* common routines */
>  extern int
> -- 
> 2.24.1
> 

[PATCH 2/5] xfs: remove the unused XLOG_UNMOUNT_REC_TYPE define

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log_priv.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index e400170ff4af..2b0aec37e73e 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -525,12 +525,6 @@ xlog_cil_force(struct xlog *log)
 	xlog_cil_force_lsn(log, log->l_cilp->xc_current_sequence);
 }
 
-/*
- * Unmount record type is used as a pseudo transaction type for the ticket.
- * It's value must be outside the range of XFS_TRANS_* values.
- */
-#define XLOG_UNMOUNT_REC_TYPE	(-1U)
-
 /*
  * Wrapper function for waiting on a wait queue serialised against wakeups
  * by a spinlock. This matches the semantics of all the wait queues used in the
-- 
2.24.1

Re: [PATCH 2/5] xfs: remove the unused XLOG_UNMOUNT_REC_TYPE define

[Darrick J. Wong]

On Thu, Mar 12, 2020 at 03:39:56PM +0100, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log_priv.h | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index e400170ff4af..2b0aec37e73e 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -525,12 +525,6 @@ xlog_cil_force(struct xlog *log)
>  	xlog_cil_force_lsn(log, log->l_cilp->xc_current_sequence);
>  }
>  
> -/*
> - * Unmount record type is used as a pseudo transaction type for the ticket.
> - * It's value must be outside the range of XFS_TRANS_* values.
> - */
> -#define XLOG_UNMOUNT_REC_TYPE	(-1U)
> -
>  /*
>   * Wrapper function for waiting on a wait queue serialised against wakeups
>   * by a spinlock. This matches the semantics of all the wait queues used in the
> -- 
> 2.24.1
> 

[PATCH 3/5] 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>
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 3/5] xfs: remove the unused return value from xfs_log_unmount_write

[Darrick J. Wong]

On Thu, Mar 12, 2020 at 03:39:57PM +0100, 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>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

AFAICT the lack of error returning is acceptable because the vfs doesn't
care what failures we encounter while unmounting and xfs will log all of
its complaints as it crashes out of the kernel?

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

--D

> ---
>  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 3/5] xfs: remove the unused return value from xfs_log_unmount_write

[Christoph Hellwig]

On Thu, Mar 12, 2020 at 04:49:17PM -0700, Darrick J. Wong wrote:
> On Thu, Mar 12, 2020 at 03:39:57PM +0100, 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>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> AFAICT the lack of error returning is acceptable because the vfs doesn't
> care what failures we encounter while unmounting and xfs will log all of
> its complaints as it crashes out of the kernel?

Well, the only "errors" we get here are for the fact that the log has
been shut down.  Which aren't very helpful errors when you try to unmount
a file system..

[PATCH 4/5] 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>
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

Re: [PATCH 4/5] xfs: remove dead code from xfs_log_unmount_write

[Darrick J. Wong]

On Thu, Mar 12, 2020 at 03:39:58PM +0100, 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>

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

--D

> ---
>  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 5/5] 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>
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..0986983ef6b5 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 5/5] xfs: cleanup xfs_log_unmount_write

[Darrick J. Wong]

On Thu, Mar 12, 2020 at 03:39:59PM +0100, 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>

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

--D

> ---
>  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..0986983ef6b5 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
>