Re: [PATCH 7/9] xfs: separate out log shutdown callback processing

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jul 14, 2021 at 01:19:56PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The iclog callback processing done during a forced log shutdown has
> different logic to normal runtime IO completion callback processing.
> Separate out eh shutdown callbacks into their own function and call

"..out the shutdown callbacks..."

> that from the shutdown code instead.
> 
> We don't need this shutdown specific logic in the normal runtime
> completion code - we'll always run the shutdown version on shutdown,
> and it will do what shutdown needs regardless of whether there are
> racing IO completion callbacks scheduled or in progress. Hence we
> can also simplify the normal IO completion callpath and only abort
> if shutdown occurred while we actively were processing callbacks.
> 
> Further, separating out the IO completion logic from the shutdown
> logic avoids callback race conditions from being triggered by log IO
> completion after a shutdown. IO completion will now only run
> callbacks on iclogs that are in the correct state for a callback to
> be run, avoiding the possibility of running callbacks on a
> referenced iclog that hasn't yet been submitted for IO.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

With the typo fixed, I like this cleanup. :)
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log.c | 53 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 302c1ce27974..4d72d9efed7c 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -487,6 +487,32 @@ xfs_log_reserve(
>  	return error;
>  }
>  
> +/*
> + * Run all the pending iclog callbacks and wake log force waiters and iclog
> + * space waiters so they can process the newly set shutdown state. We really
> + * don't care what order we process callbacks here because the log is shut down
> + * and so state cannot change on disk anymore.
> + */
> +static void
> +xlog_state_shutdown_callbacks(
> +	struct xlog		*log)
> +{
> +	struct xlog_in_core	*iclog;
> +	LIST_HEAD(cb_list);
> +
> +	spin_lock(&log->l_icloglock);
> +	iclog = log->l_iclog;
> +	do {
> +		list_splice_init(&iclog->ic_callbacks, &cb_list);
> +		wake_up_all(&iclog->ic_force_wait);
> +	} while ((iclog = iclog->ic_next) != log->l_iclog);
> +
> +	wake_up_all(&log->l_flush_wait);
> +	spin_unlock(&log->l_icloglock);
> +
> +	xlog_cil_process_committed(&cb_list);
> +}
> +
>  static bool
>  __xlog_state_release_iclog(
>  	struct xlog		*log,
> @@ -2762,7 +2788,10 @@ xlog_state_iodone_process_iclog(
>  
>  /*
>   * Loop over all the iclogs, running attached callbacks on them. Return true if
> - * we ran any callbacks, indicating that we dropped the icloglock.
> + * we ran any callbacks, indicating that we dropped the icloglock. We don't need
> + * to handle transient shutdown state here at all because
> + * xlog_state_shutdown_callbacks() will be run to do the necessary shutdown
> + * cleanup of the callbacks.
>   */
>  static bool
>  xlog_state_do_iclog_callbacks(
> @@ -2777,13 +2806,11 @@ xlog_state_do_iclog_callbacks(
>  	do {
>  		LIST_HEAD(cb_list);
>  
> -		if (!xlog_is_shutdown(log)) {
> -			if (xlog_state_iodone_process_iclog(log, iclog))
> -				break;
> -			if (iclog->ic_state != XLOG_STATE_CALLBACK) {
> -				iclog = iclog->ic_next;
> -				continue;
> -			}
> +		if (xlog_state_iodone_process_iclog(log, iclog))
> +			break;
> +		if (iclog->ic_state != XLOG_STATE_CALLBACK) {
> +			iclog = iclog->ic_next;
> +			continue;
>  		}
>  		list_splice_init(&iclog->ic_callbacks, &cb_list);
>  		spin_unlock(&log->l_icloglock);
> @@ -2794,10 +2821,7 @@ xlog_state_do_iclog_callbacks(
>  		ran_callback = true;
>  
>  		spin_lock(&log->l_icloglock);
> -		if (xlog_is_shutdown(log))
> -			wake_up_all(&iclog->ic_force_wait);
> -		else
> -			xlog_state_clean_iclog(log, iclog);
> +		xlog_state_clean_iclog(log, iclog);
>  		iclog = iclog->ic_next;
>  	} while (iclog != first_iclog);
>  
> @@ -2830,8 +2854,7 @@ xlog_state_do_callback(
>  		}
>  	}
>  
> -	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
> -	    xlog_is_shutdown(log))
> +	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE)
>  		wake_up_all(&log->l_flush_wait);
>  
>  	spin_unlock(&log->l_icloglock);
> @@ -3764,7 +3787,7 @@ xlog_force_shutdown(
>  	spin_lock(&log->l_cilp->xc_push_lock);
>  	wake_up_all(&log->l_cilp->xc_commit_wait);
>  	spin_unlock(&log->l_cilp->xc_push_lock);
> -	xlog_state_do_callback(log);
> +	xlog_state_shutdown_callbacks(log);
>  
>  	return log_error;
>  }
> -- 
> 2.31.1
>