Re: [PATCH 4/8] xfs: factor debug code out of xlog_state_do_callback()

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Sep 05, 2019 at 06:47:13PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Start making this function readable by lifting the debug code into
> a conditional function.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log.c | 79 +++++++++++++++++++++++++++---------------------
>  1 file changed, 44 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 8380ed065608..2904bf0d17f3 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2628,6 +2628,48 @@ xlog_get_lowest_lsn(
>  	return lowest_lsn;
>  }
>  
> +#ifdef DEBUG
> +/*
> + * Make one last gasp attempt to see if iclogs are being left in limbo.  If the
> + * above loop finds an iclog earlier than the current iclog and in one of the
> + * syncing states, the current iclog is put into DO_CALLBACK and the callbacks
> + * are deferred to the completion of the earlier iclog. Walk the iclogs in order
> + * and make sure that no iclog is in DO_CALLBACK unless an earlier iclog is in
> + * one of the syncing states.
> + *
> + * Note that SYNCING|IOERROR is a valid state so we cannot just check for
> + * ic_state == SYNCING.
> + */
> +static void
> +xlog_state_callback_check_state(
> +	struct xlog		*log)
> +{
> +	struct xlog_in_core	*first_iclog = log->l_iclog;
> +	struct xlog_in_core	*iclog = first_iclog;
> +
> +	do {
> +		ASSERT(iclog->ic_state != XLOG_STATE_DO_CALLBACK);
> +		/*
> +		 * Terminate the loop if iclogs are found in states
> +		 * which will cause other threads to clean up iclogs.
> +		 *
> +		 * SYNCING - i/o completion will go through logs
> +		 * DONE_SYNC - interrupt thread should be waiting for
> +		 *              l_icloglock
> +		 * IOERROR - give up hope all ye who enter here
> +		 */
> +		if (iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> +		    iclog->ic_state & XLOG_STATE_SYNCING ||

Code like this make my eyes twitch (Does ic_state track mutually
exclusive state?  Or is it state flags?  I think it's the second!),
but as this is simply refactoring debugging code...

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

--D

> +		    iclog->ic_state == XLOG_STATE_DONE_SYNC ||
> +		    iclog->ic_state == XLOG_STATE_IOERROR )
> +			break;
> +		iclog = iclog->ic_next;
> +	} while (first_iclog != iclog);
> +}
> +#else
> +#define xlog_state_callback_check_state(l)	((void)0)
> +#endif
> +
>  STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log,
> @@ -2802,41 +2844,8 @@ xlog_state_do_callback(
>  		}
>  	} while (!ioerrors && loopdidcallbacks);
>  
> -#ifdef DEBUG
> -	/*
> -	 * Make one last gasp attempt to see if iclogs are being left in limbo.
> -	 * If the above loop finds an iclog earlier than the current iclog and
> -	 * in one of the syncing states, the current iclog is put into
> -	 * DO_CALLBACK and the callbacks are deferred to the completion of the
> -	 * earlier iclog. Walk the iclogs in order and make sure that no iclog
> -	 * is in DO_CALLBACK unless an earlier iclog is in one of the syncing
> -	 * states.
> -	 *
> -	 * Note that SYNCING|IOABORT is a valid state so we cannot just check
> -	 * for ic_state == SYNCING.
> -	 */
> -	if (funcdidcallbacks) {
> -		first_iclog = iclog = log->l_iclog;
> -		do {
> -			ASSERT(iclog->ic_state != XLOG_STATE_DO_CALLBACK);
> -			/*
> -			 * Terminate the loop if iclogs are found in states
> -			 * which will cause other threads to clean up iclogs.
> -			 *
> -			 * SYNCING - i/o completion will go through logs
> -			 * DONE_SYNC - interrupt thread should be waiting for
> -			 *              l_icloglock
> -			 * IOERROR - give up hope all ye who enter here
> -			 */
> -			if (iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -			    iclog->ic_state & XLOG_STATE_SYNCING ||
> -			    iclog->ic_state == XLOG_STATE_DONE_SYNC ||
> -			    iclog->ic_state == XLOG_STATE_IOERROR )
> -				break;
> -			iclog = iclog->ic_next;
> -		} while (first_iclog != iclog);
> -	}
> -#endif
> +	if (funcdidcallbacks)
> +		xlog_state_callback_check_state(log);
>  
>  	if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE|XLOG_STATE_IOERROR))
>  		wake_up_all(&log->l_flush_wait);
> -- 
> 2.23.0.rc1
>