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

Christoph Hellwig
2019-09-04T06:10:39Z

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