Re: [PATCH 10/14] xfs: refactor xlog_state_iodone_process_iclog

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

On Mon, Mar 16, 2020 at 03:42:29PM +0100, Christoph Hellwig wrote:
> Move all state checks into the caller to make the loop flow more clear,
> and instead move the callback processing together with marking the iclog
> for callbacks.
> 
> This also allows to easily indicate when we actually dropped the
> icloglock instead of assuming we do so for any iclog processed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Ok, I think this is a pretty straightfoward hoisting of the the state
checksk, as promised...

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

--D

> ---
>  fs/xfs/xfs_log.c | 82 ++++++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 4efaa248a03d..a38d495b6e81 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2738,47 +2738,28 @@ xlog_state_do_iclog_callbacks(
>  	spin_unlock(&iclog->ic_callback_lock);
>  }
>  
> -/*
> - * Return true if we need to stop processing, false to continue to the next
> - * iclog. The caller will need to run callbacks if the iclog is returned in the
> - * XLOG_STATE_CALLBACK state.
> - */
>  static bool
>  xlog_state_iodone_process_iclog(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog)
>  {
> -	xfs_lsn_t		lowest_lsn;
> -	xfs_lsn_t		header_lsn;
> +	xfs_lsn_t		header_lsn, lowest_lsn;
>  
> -	switch (iclog->ic_state) {
> -	case XLOG_STATE_ACTIVE:
> -	case XLOG_STATE_DIRTY:
> -		/*
> -		 * Skip all iclogs in the ACTIVE & DIRTY states:
> -		 */
> -		return false;
> -	case XLOG_STATE_DONE_SYNC:
> -		/*
> -		 * Now that we have an iclog that is in the DONE_SYNC state, do
> -		 * one more check here to see if we have chased our tail around.
> -		 * If this is not the lowest lsn iclog, then we will leave it
> -		 * for another completion to process.
> -		 */
> -		header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> -		lowest_lsn = xlog_get_lowest_lsn(log);
> -		if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
> -			return false;
> -		xlog_state_set_callback(log, iclog, header_lsn);
> +	/*
> +	 * Now that we have an iclog that is in the DONE_SYNC state, do one more
> +	 * check here to see if we have chased our tail around.  If this is not
> +	 * the lowest lsn iclog, then we will leave it for another completion to
> +	 * process.
> +	 */
> +	header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> +	lowest_lsn = xlog_get_lowest_lsn(log);
> +	if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
>  		return false;
> -	default:
> -		/*
> -		 * Can only perform callbacks in order.  Since this iclog is not
> -		 * in the DONE_SYNC state, we skip the rest and just try to
> -		 * clean up.
> -		 */
> -		return true;
> -	}
> +
> +	xlog_state_set_callback(log, iclog, header_lsn);
> +	xlog_state_do_iclog_callbacks(log, iclog);
> +	xlog_state_clean_iclog(log, iclog);
> +	return true;
>  }
>  
>  STATIC void
> @@ -2795,10 +2776,6 @@ xlog_state_do_callback(
>  	 *
>  	 * 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 {
> @@ -2809,25 +2786,40 @@ xlog_state_do_callback(
>  		repeats++;
>  
>  		do {
> +			/*
> +			 * If the log has been shut down, still perform the
> +			 * callbacks to abort all log items to clean up any
> +			 * allocate resource, but don't bother to restart the
> +			 * loop after dropping the log as no new callbacks can
> +			 * be attached now.
> +			 */
>  			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)
> +			/*
> +			 * Skip all iclogs in the ACTIVE & DIRTY states:
> +			 */
> +			if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> +			    iclog->ic_state == XLOG_STATE_DIRTY)
>  				continue;
>  
> +			/*
> +			 * We can only perform callbacks in order.  If this
> +			 * iclog is not in the DONE_SYNC state, we skip the rest
> +			 * and just try to clean up.
> +			 */
> +			if (iclog->ic_state != XLOG_STATE_DONE_SYNC)
> +				break;
> +
>  			/*
>  			 * Running callbacks will drop the icloglock which means
>  			 * we'll have to run at least one more complete loop.
>  			 */
> -			cycled_icloglock = true;
> -			xlog_state_do_iclog_callbacks(log, iclog);
> -			xlog_state_clean_iclog(log, iclog);
> +			if (xlog_state_iodone_process_iclog(log, iclog))
> +				cycled_icloglock = true;
>  		} while ((iclog = iclog->ic_next) != first_iclog);
>  
>  		if (repeats > 5000) {
> -- 
> 2.24.1
>