Re: [PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org, Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog
Date: Mon, 16 Mar 2020 14:02:57 -0700	[thread overview]
Message-ID: <20200316210257.GN256767@magnolia> (raw)
In-Reply-To: <20200316144233.900390-10-hch@lst.de>

On Mon, Mar 16, 2020 at 03:42:28PM +0100, Christoph Hellwig wrote:
> Move handling of a shut down log out of xlog_state_iodone_process_iclog
> and into xlog_state_do_callback so that it can be moved into an entirely
> separate branch.  While doing so switch to using XLOG_FORCED_SHUTDOWN to
> check the shutdown condition global to the log instead of the per-iclog
> flag, and make sure the comments match reality.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_log.c | 64 ++++++++++++++++++++----------------------------
>  1 file changed, 26 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index c534d7007aa3..4efaa248a03d 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2746,8 +2746,7 @@ xlog_state_do_iclog_callbacks(
>  static bool
>  xlog_state_iodone_process_iclog(
>  	struct xlog		*log,
> -	struct xlog_in_core	*iclog,
> -	bool			*ioerror)
> +	struct xlog_in_core	*iclog)
>  {
>  	xfs_lsn_t		lowest_lsn;
>  	xfs_lsn_t		header_lsn;
> @@ -2759,15 +2758,6 @@ xlog_state_iodone_process_iclog(
>  		 * Skip all iclogs in the ACTIVE & DIRTY states:
>  		 */
>  		return false;
> -	case XLOG_STATE_IOERROR:
> -		/*
> -		 * Between marking a filesystem SHUTDOWN and stopping the log,
> -		 * we do flush all iclogs to disk (if there wasn't a log I/O
> -		 * error). So, we do want things to go smoothly in case of just
> -		 * a SHUTDOWN w/o a LOG_IO_ERROR.
> -		 */
> -		*ioerror = true;
> -		return false;
>  	case XLOG_STATE_DONE_SYNC:
>  		/*
>  		 * Now that we have an iclog that is in the DONE_SYNC state, do
> @@ -2795,39 +2785,41 @@ STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log)
>  {
> -	struct xlog_in_core	*iclog;
> -	struct xlog_in_core	*first_iclog;
>  	bool			cycled_icloglock;
> -	bool			ioerror;
>  	int			flushcnt = 0;
>  	int			repeats = 0;
>  
> +	/*
> +	 * Scan all iclogs starting with the one pointed to by the log.  Reset
> +	 * this starting point each time the log is unlocked (during callbacks).
> +	 *
> +	 * Keep looping through iclogs until one full pass is made without
> +	 * running any callbacks.
> +	 *
> +	 * If the log has been shut down, still perform the callbacks once per
> +	 * iclog to abort all log items, but don't bother to restart the loop
> +	 * after dropping the log as no new callbacks can show up.
> +	 */
>  	spin_lock(&log->l_icloglock);
>  	do {
> -		/*
> -		 * Scan all iclogs starting with the one pointed to by the
> -		 * log.  Reset this starting point each time the log is
> -		 * unlocked (during callbacks).
> -		 *
> -		 * Keep looping through iclogs until one full pass is made
> -		 * without running any callbacks.
> -		 */
> -		first_iclog = log->l_iclog;
> -		iclog = log->l_iclog;
> +		struct xlog_in_core	*first_iclog = log->l_iclog;
> +		struct xlog_in_core	*iclog = first_iclog;
> +
>  		cycled_icloglock = false;
> -		ioerror = false;
>  		repeats++;
>  
>  		do {
> -			if (xlog_state_iodone_process_iclog(log, iclog,
> -							&ioerror))
> +			if (XLOG_FORCED_SHUTDOWN(log)) {
> +				xlog_state_do_iclog_callbacks(log, iclog);
> +				wake_up_all(&iclog->ic_force_wait);
> +				continue;
> +			}
> +
> +			if (xlog_state_iodone_process_iclog(log, iclog))
>  				break;
>  
> -			if (iclog->ic_state != XLOG_STATE_CALLBACK &&
> -			    iclog->ic_state != XLOG_STATE_IOERROR) {
> -				iclog = iclog->ic_next;
> +			if (iclog->ic_state != XLOG_STATE_CALLBACK)
>  				continue;
> -			}
>  
>  			/*
>  			 * Running callbacks will drop the icloglock which means
> @@ -2835,12 +2827,8 @@ xlog_state_do_callback(
>  			 */
>  			cycled_icloglock = true;
>  			xlog_state_do_iclog_callbacks(log, iclog);
> -			if (XLOG_FORCED_SHUTDOWN(log))
> -				wake_up_all(&iclog->ic_force_wait);
> -			else
> -				xlog_state_clean_iclog(log, iclog);
> -			iclog = iclog->ic_next;
> -		} while (first_iclog != iclog);
> +			xlog_state_clean_iclog(log, iclog);
> +		} while ((iclog = iclog->ic_next) != first_iclog);
>  
>  		if (repeats > 5000) {
>  			flushcnt += repeats;
> @@ -2849,7 +2837,7 @@ xlog_state_do_callback(
>  				"%s: possible infinite loop (%d iterations)",
>  				__func__, flushcnt);
>  		}
> -	} while (!ioerror && cycled_icloglock);
> +	} while (cycled_icloglock);
>  
>  	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
>  	    log->l_iclog->ic_state == XLOG_STATE_IOERROR)
> -- 
> 2.24.1
> 