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@xxxxxxxxxxxxx
