Re: [PATCH 3/7] xfs: factor debug code out of xlog_state_do_callback()

From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 3/7] xfs: factor debug code out of xlog_state_do_callback()
Date: Tue, 3 Sep 2019 23:10:38 -0700	[thread overview]
Message-ID: <20190904061038.GC12591@infradead.org> (raw)
In-Reply-To: <20190904042451.9314-4-david@fromorbit.com>

> + * Note that SYNCING|IOABORT is a valid state so we cannot just check for
> + * ic_state == SYNCING.

I've removed the IOABORT flag recently, but it seems I forgot to remove
this comment.  That beeing said the IOERR flag is still a complete mess
as we sometimes use it as a flag and sometimes as a separate state.
I've been wanting to sort that out, but always got preempted.

> + */
> +static void
> +xlog_state_callback_check_state(
> +	struct xlog		*log,
> +	int			did_callbacks)
> +{
> +	struct xlog_in_core	*iclog;
> +	struct xlog_in_core	*first_iclog;
> +
> +	if (!did_callbacks)
> +		return;
> +
> +	first_iclog = iclog = log->l_iclog;

I'd keep the did_callbacks check in the caller.  For the non-debug case
it will be optimized away, but it saves an argument, and allows
initializing the iclog variables on the declaration line.

> +	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;

No new, but if we list the states we should not miss one..