| Message ID | 20210630063813.1751007-7-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: shutdown is a racy mess | expand |
s/reowrk/rework/ in the subject.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Jun 30, 2021 at 04:38:10PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Clean it up a bit by factoring and rearranging some of the code. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_log.c | 99 ++++++++++++++++++++++++++---------------------- > 1 file changed, 53 insertions(+), 46 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 6c7cfc052135..bb44dcfcae89 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -2758,67 +2758,74 @@ xlog_state_iodone_process_iclog( > } > } > > +/* > + * Loop over all the iclogs, running attached callbacks on them. Return true if > + * we ran any callbacks, indicating that we dropped the icloglock. > + */ > +static bool > +xlog_state_do_iclog_callbacks( > + struct xlog *log) > + __releases(&log->l_icloglock) > + __acquires(&log->l_icloglock) > +{ > + struct xlog_in_core *iclog = log->l_iclog; > + struct xlog_in_core *first_iclog = NULL; > + bool ran_callback = false; > + > + for (; iclog != first_iclog; iclog = iclog->ic_next) { > + LIST_HEAD(cb_list); > + > + if (!first_iclog) > + first_iclog = iclog; > + > + if (!xlog_is_shutdown(log)) { > + if (xlog_state_iodone_process_iclog(log, iclog)) > + break; > + if (iclog->ic_state != XLOG_STATE_CALLBACK) > + continue; > + } > + list_splice_init(&iclog->ic_callbacks, &cb_list); > + spin_unlock(&log->l_icloglock); > + > + trace_xlog_iclog_callbacks_start(iclog, _RET_IP_); > + xlog_cil_process_committed(&cb_list); > + trace_xlog_iclog_callbacks_done(iclog, _RET_IP_); > + ran_callback = true; > + > + spin_lock(&log->l_icloglock); > + if (xlog_is_shutdown(log)) > + wake_up_all(&iclog->ic_force_wait); > + else > + xlog_state_clean_iclog(log, iclog); > + }; Aside from the unnecessary semicolon here... > + return ran_callback; > +} > + > + > +/* > + * Loop running iclog completion callbacks until there are no more iclogs in a > + * state that can run callbacks. > + */ > STATIC void > xlog_state_do_callback( > struct xlog *log) > { > - struct xlog_in_core *iclog; > - struct xlog_in_core *first_iclog; > - bool cycled_icloglock; > int flushcnt = 0; > int repeats = 0; > > spin_lock(&log->l_icloglock); > - do { > - repeats++; > + while (xlog_state_do_iclog_callbacks(log)) { > + if (xlog_is_shutdown(log)) > + break; > > - /* > - * Scan all iclogs starting with the one pointed to by the > - * log. Reset this starting point each time the log is > - * unlocked (during callbacks). > - * > - * Keep looping through iclogs until one full pass is made > - * without running any callbacks. > - */ > - cycled_icloglock = false; > - first_iclog = NULL; > - for (iclog = log->l_iclog; > - iclog != first_iclog; > - iclog = iclog->ic_next) { > - LIST_HEAD(cb_list); > - > - if (!first_iclog) > - first_iclog = iclog; > - > - if (!xlog_is_shutdown(log)) { > - if (xlog_state_iodone_process_iclog(log, iclog)) > - break; > - if (iclog->ic_state != XLOG_STATE_CALLBACK) > - continue; > - } > - list_splice_init(&iclog->ic_callbacks, &cb_list); > - spin_unlock(&log->l_icloglock); > - > - trace_xlog_iclog_callbacks_start(iclog, _RET_IP_); > - xlog_cil_process_committed(&cb_list); > - trace_xlog_iclog_callbacks_done(iclog, _RET_IP_); > - cycled_icloglock = true; > - > - spin_lock(&log->l_icloglock); > - if (xlog_is_shutdown(log)) > - wake_up_all(&iclog->ic_force_wait); > - else > - xlog_state_clean_iclog(log, iclog); > - }; > - > - if (repeats > 5000) { > + if (repeats++ > 5000) { > flushcnt += repeats; > repeats = 0; > xfs_warn(log->l_mp, > "%s: possible infinite loop (%d iterations)", > __func__, flushcnt); > } > - } while (!xlog_is_shutdown(log) && cycled_icloglock); > + }; ...and here, this looks like a simple hoist. With that fixed, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE || > xlog_is_shutdown(log)) > -- > 2.31.1 >
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 6c7cfc052135..bb44dcfcae89 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -2758,67 +2758,74 @@ xlog_state_iodone_process_iclog( } } +/* + * Loop over all the iclogs, running attached callbacks on them. Return true if + * we ran any callbacks, indicating that we dropped the icloglock. + */ +static bool +xlog_state_do_iclog_callbacks( + struct xlog *log) + __releases(&log->l_icloglock) + __acquires(&log->l_icloglock) +{ + struct xlog_in_core *iclog = log->l_iclog; + struct xlog_in_core *first_iclog = NULL; + bool ran_callback = false; + + for (; iclog != first_iclog; iclog = iclog->ic_next) { + LIST_HEAD(cb_list); + + if (!first_iclog) + first_iclog = iclog; + + if (!xlog_is_shutdown(log)) { + if (xlog_state_iodone_process_iclog(log, iclog)) + break; + if (iclog->ic_state != XLOG_STATE_CALLBACK) + continue; + } + list_splice_init(&iclog->ic_callbacks, &cb_list); + spin_unlock(&log->l_icloglock); + + trace_xlog_iclog_callbacks_start(iclog, _RET_IP_); + xlog_cil_process_committed(&cb_list); + trace_xlog_iclog_callbacks_done(iclog, _RET_IP_); + ran_callback = true; + + spin_lock(&log->l_icloglock); + if (xlog_is_shutdown(log)) + wake_up_all(&iclog->ic_force_wait); + else + xlog_state_clean_iclog(log, iclog); + }; + return ran_callback; +} + + +/* + * Loop running iclog completion callbacks until there are no more iclogs in a + * state that can run callbacks. + */ STATIC void xlog_state_do_callback( struct xlog *log) { - struct xlog_in_core *iclog; - struct xlog_in_core *first_iclog; - bool cycled_icloglock; int flushcnt = 0; int repeats = 0; spin_lock(&log->l_icloglock); - do { - repeats++; + while (xlog_state_do_iclog_callbacks(log)) { + if (xlog_is_shutdown(log)) + break; - /* - * Scan all iclogs starting with the one pointed to by the - * log. Reset this starting point each time the log is - * unlocked (during callbacks). - * - * Keep looping through iclogs until one full pass is made - * without running any callbacks. - */ - cycled_icloglock = false; - first_iclog = NULL; - for (iclog = log->l_iclog; - iclog != first_iclog; - iclog = iclog->ic_next) { - LIST_HEAD(cb_list); - - if (!first_iclog) - first_iclog = iclog; - - if (!xlog_is_shutdown(log)) { - if (xlog_state_iodone_process_iclog(log, iclog)) - break; - if (iclog->ic_state != XLOG_STATE_CALLBACK) - continue; - } - list_splice_init(&iclog->ic_callbacks, &cb_list); - spin_unlock(&log->l_icloglock); - - trace_xlog_iclog_callbacks_start(iclog, _RET_IP_); - xlog_cil_process_committed(&cb_list); - trace_xlog_iclog_callbacks_done(iclog, _RET_IP_); - cycled_icloglock = true; - - spin_lock(&log->l_icloglock); - if (xlog_is_shutdown(log)) - wake_up_all(&iclog->ic_force_wait); - else - xlog_state_clean_iclog(log, iclog); - }; - - if (repeats > 5000) { + if (repeats++ > 5000) { flushcnt += repeats; repeats = 0; xfs_warn(log->l_mp, "%s: possible infinite loop (%d iterations)", __func__, flushcnt); } - } while (!xlog_is_shutdown(log) && cycled_icloglock); + }; if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE || xlog_is_shutdown(log))