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

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 3/7] xfs: factor debug code out of xlog_state_do_callback()
Date: Thu, 5 Sep 2019 07:14:54 +1000	[thread overview]
Message-ID: <20190904211454.GF1119@dread.disaster.area> (raw)
In-Reply-To: <20190904061038.GC12591@infradead.org>

On Tue, Sep 03, 2019 at 11:10:38PM -0700, Christoph Hellwig wrote:
> > + * 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. 

I think the comment is still relevant for SYNCING|IOERROR state,
so I s/ABORT/ERROR/

> 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.

Yes, I started cleaning that up (eg. using XLOG_FORCED_SHUTDOWN
instead of checking the IOERROR state of the first iclog for
shutdown) but it's not that simple unfortunately - it's a bit of a
mess - and so I dropped that from the series to get the fixes out.
I have an idea of what is wrong, but it's not ready yet.

> > + */
> > +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.

Fixed.

FWIW, I started cleaning that up to with xlog_for_each_iclog()
for all the places where we iterate like this. But that's also
dependent on the IOERROR cleanup I shelved for the moment....

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

Which one if missing?

Cheers,
-- 
Dave Chinner
david@fromorbit.com