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

[Dave Chinner <david@fromorbit.com>]

On Thu, Sep 05, 2019 at 08:30:06AM -0700, Darrick J. Wong wrote:
> 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...

Yup, it's just another of those little things about this code that
makes eyes cross and heads explode.  Fixing this is what Christoph
and I were talking about in the past version of this series -
pulling the ioerror state out of the iclog state machine state
flags...

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

Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com