| Message ID | 20190905084717.30308-5-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/8] xfs: push the AIL in xlog_grant_head_wake | expand |
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... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > + 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) ((void)0) > +#endif > + > STATIC void > xlog_state_do_callback( > struct xlog *log, > @@ -2802,41 +2844,8 @@ 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 > + if (funcdidcallbacks) > + xlog_state_callback_check_state(log); > > if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE|XLOG_STATE_IOERROR)) > wake_up_all(&log->l_flush_wait); > -- > 2.23.0.rc1 >
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.
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 || + 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) ((void)0) +#endif + STATIC void xlog_state_do_callback( struct xlog *log, @@ -2802,41 +2844,8 @@ 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 + if (funcdidcallbacks) + xlog_state_callback_check_state(log); if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE|XLOG_STATE_IOERROR)) wake_up_all(&log->l_flush_wait);