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

[Brian Foster <bfoster@redhat.com>]

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
>