Re: [PATCH 6/9] xfs: reowrk up xlog_state_do_callback

From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 6/9] xfs: reowrk up xlog_state_do_callback
Date: Thu, 8 Jul 2021 21:42:14 -0700	[thread overview]
Message-ID: <20210709044214.GY11588@locust> (raw)
In-Reply-To: <20210630063813.1751007-7-david@fromorbit.com>

On Wed, Jun 30, 2021 at 04:38:10PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Clean it up a bit by factoring and rearranging some of the code.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log.c | 99 ++++++++++++++++++++++++++----------------------
>  1 file changed, 53 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 6c7cfc052135..bb44dcfcae89 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2758,67 +2758,74 @@ 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.
> + */
> +static bool
> +xlog_state_do_iclog_callbacks(
> +	struct xlog		*log)
> +		__releases(&log->l_icloglock)
> +		__acquires(&log->l_icloglock)
> +{
> +	struct xlog_in_core	*iclog = log->l_iclog;
> +	struct xlog_in_core	*first_iclog = NULL;
> +	bool			ran_callback = false;
> +
> +	for (; iclog != first_iclog; iclog = iclog->ic_next) {
> +		LIST_HEAD(cb_list);
> +
> +		if (!first_iclog)
> +			first_iclog = iclog;
> +
> +		if (!xlog_is_shutdown(log)) {
> +			if (xlog_state_iodone_process_iclog(log, iclog))
> +				break;
> +			if (iclog->ic_state != XLOG_STATE_CALLBACK)
> +				continue;
> +		}
> +		list_splice_init(&iclog->ic_callbacks, &cb_list);
> +		spin_unlock(&log->l_icloglock);
> +
> +		trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> +		xlog_cil_process_committed(&cb_list);
> +		trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
> +		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);
> +	};

Aside from the unnecessary semicolon here...

> +	return ran_callback;
> +}
> +
> +
> +/*
> + * Loop running iclog completion callbacks until there are no more iclogs in a
> + * state that can run callbacks.
> + */
>  STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log)
>  {
> -	struct xlog_in_core	*iclog;
> -	struct xlog_in_core	*first_iclog;
> -	bool			cycled_icloglock;
>  	int			flushcnt = 0;
>  	int			repeats = 0;
>  
>  	spin_lock(&log->l_icloglock);
> -	do {
> -		repeats++;
> +	while (xlog_state_do_iclog_callbacks(log)) {
> +		if (xlog_is_shutdown(log))
> +			break;
>  
> -		/*
> -		 * 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.
> -		 */
> -		cycled_icloglock = false;
> -		first_iclog = NULL;
> -		for (iclog = log->l_iclog;
> -		     iclog != first_iclog;
> -		     iclog = iclog->ic_next) {
> -			LIST_HEAD(cb_list);
> -
> -			if (!first_iclog)
> -				first_iclog = iclog;
> -
> -			if (!xlog_is_shutdown(log)) {
> -				if (xlog_state_iodone_process_iclog(log, iclog))
> -					break;
> -				if (iclog->ic_state != XLOG_STATE_CALLBACK)
> -					continue;
> -			}
> -			list_splice_init(&iclog->ic_callbacks, &cb_list);
> -			spin_unlock(&log->l_icloglock);
> -
> -			trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> -			xlog_cil_process_committed(&cb_list);
> -			trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
> -			cycled_icloglock = 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);
> -		};
> -
> -		if (repeats > 5000) {
> +		if (repeats++ > 5000) {
>  			flushcnt += repeats;
>  			repeats = 0;
>  			xfs_warn(log->l_mp,
>  				"%s: possible infinite loop (%d iterations)",
>  				__func__, flushcnt);
>  		}
> -	} while (!xlog_is_shutdown(log) && cycled_icloglock);
> +	};

...and here, this looks like a simple hoist.

With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  
>  	if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE ||
>  	    xlog_is_shutdown(log))
> -- 
> 2.31.1
> 