diff mbox series

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

Message ID 20190904042451.9314-4-david@fromorbit.com (mailing list archive)
State Superseded
Headers show
Series xfs: log race fixes and cleanups | expand

Commit Message

Dave Chinner Sept. 4, 2019, 4:24 a.m. UTC 
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 | 83 ++++++++++++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 35 deletions(-)

Comments

Christoph Hellwig Sept. 4, 2019, 6:10 a.m. UTC | #1 
> + * 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..
Dave Chinner Sept. 4, 2019, 9:14 p.m. UTC | #2 
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,
diff mbox series

Patch

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 6f494f6369e8..b3d8dbb3b956 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2612,6 +2612,53 @@  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|IOABORT is a valid state so we cannot just check for
+ * ic_state == SYNCING.
+ */
+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;
+	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;
+	} while (first_iclog != iclog);
+}
+#else
+#define xlog_state_callback_check_state(l,d)	((void)0)
+#endif
+
 STATIC void
 xlog_state_do_callback(
 	struct xlog		*log,
@@ -2786,41 +2833,7 @@  xlog_state_do_callback(
 		}
 	} while (!ioerrors && loopdidcallbacks);
 
-#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|IOABORT is a valid state so we cannot just check
-	 * for ic_state == SYNCING.
-	 */
-	if (funcdidcallbacks) {
-		first_iclog = iclog = log->l_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 ||
-			    iclog->ic_state == XLOG_STATE_DONE_SYNC ||
-			    iclog->ic_state == XLOG_STATE_IOERROR )
-				break;
-			iclog = iclog->ic_next;
-		} while (first_iclog != iclog);
-	}
-#endif
+	xlog_state_callback_check_state(log, funcdidcallbacks);
 
 	if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE|XLOG_STATE_IOERROR))
 		wake_up_all(&log->l_flush_wait);