On Mon, Mar 16, 2020 at 03:42:29PM +0100, Christoph Hellwig wrote:
> Move all state checks into the caller to make the loop flow more clear,
> and instead move the callback processing together with marking the iclog
> for callbacks.
>
> This also allows to easily indicate when we actually dropped the
> icloglock instead of assuming we do so for any iclog processed.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Ok, I think this is a pretty straightfoward hoisting of the the state
checksk, as promised...
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_log.c | 82 ++++++++++++++++++++++--------------------------
> 1 file changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 4efaa248a03d..a38d495b6e81 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2738,47 +2738,28 @@ xlog_state_do_iclog_callbacks(
> spin_unlock(&iclog->ic_callback_lock);
> }
>
> -/*
> - * Return true if we need to stop processing, false to continue to the next
> - * iclog. The caller will need to run callbacks if the iclog is returned in the
> - * XLOG_STATE_CALLBACK state.
> - */
> static bool
> xlog_state_iodone_process_iclog(
> struct xlog *log,
> struct xlog_in_core *iclog)
> {
> - xfs_lsn_t lowest_lsn;
> - xfs_lsn_t header_lsn;
> + xfs_lsn_t header_lsn, lowest_lsn;
>
> - switch (iclog->ic_state) {
> - case XLOG_STATE_ACTIVE:
> - case XLOG_STATE_DIRTY:
> - /*
> - * Skip all iclogs in the ACTIVE & DIRTY states:
> - */
> - return false;
> - case XLOG_STATE_DONE_SYNC:
> - /*
> - * Now that we have an iclog that is in the DONE_SYNC state, do
> - * one more check here to see if we have chased our tail around.
> - * If this is not the lowest lsn iclog, then we will leave it
> - * for another completion to process.
> - */
> - header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> - lowest_lsn = xlog_get_lowest_lsn(log);
> - if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
> - return false;
> - xlog_state_set_callback(log, iclog, header_lsn);
> + /*
> + * Now that we have an iclog that is in the DONE_SYNC state, do one more
> + * check here to see if we have chased our tail around. If this is not
> + * the lowest lsn iclog, then we will leave it for another completion to
> + * process.
> + */
> + header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> + lowest_lsn = xlog_get_lowest_lsn(log);
> + if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
> return false;
> - default:
> - /*
> - * Can only perform callbacks in order. Since this iclog is not
> - * in the DONE_SYNC state, we skip the rest and just try to
> - * clean up.
> - */
> - return true;
> - }
> +
> + xlog_state_set_callback(log, iclog, header_lsn);
> + xlog_state_do_iclog_callbacks(log, iclog);
> + xlog_state_clean_iclog(log, iclog);
> + return true;
> }
>
> STATIC void
> @@ -2795,10 +2776,6 @@ xlog_state_do_callback(
> *
> * Keep looping through iclogs until one full pass is made without
> * running any callbacks.
> - *
> - * If the log has been shut down, still perform the callbacks once per
> - * iclog to abort all log items, but don't bother to restart the loop
> - * after dropping the log as no new callbacks can show up.
> */
> spin_lock(&log->l_icloglock);
> do {
> @@ -2809,25 +2786,40 @@ xlog_state_do_callback(
> repeats++;
>
> do {
> + /*
> + * If the log has been shut down, still perform the
> + * callbacks to abort all log items to clean up any
> + * allocate resource, but don't bother to restart the
> + * loop after dropping the log as no new callbacks can
> + * be attached now.
> + */
> if (XLOG_FORCED_SHUTDOWN(log)) {
> xlog_state_do_iclog_callbacks(log, iclog);
> wake_up_all(&iclog->ic_force_wait);
> continue;
> }
>
> - if (xlog_state_iodone_process_iclog(log, iclog))
> - break;
> -
> - if (iclog->ic_state != XLOG_STATE_CALLBACK)
> + /*
> + * Skip all iclogs in the ACTIVE & DIRTY states:
> + */
> + if (iclog->ic_state == XLOG_STATE_ACTIVE ||
> + iclog->ic_state == XLOG_STATE_DIRTY)
> continue;
>
> + /*
> + * We can only perform callbacks in order. If this
> + * iclog is not in the DONE_SYNC state, we skip the rest
> + * and just try to clean up.
> + */
> + if (iclog->ic_state != XLOG_STATE_DONE_SYNC)
> + break;
> +
> /*
> * Running callbacks will drop the icloglock which means
> * we'll have to run at least one more complete loop.
> */
> - cycled_icloglock = true;
> - xlog_state_do_iclog_callbacks(log, iclog);
> - xlog_state_clean_iclog(log, iclog);
> + if (xlog_state_iodone_process_iclog(log, iclog))
> + cycled_icloglock = true;
> } while ((iclog = iclog->ic_next) != first_iclog);
>
> if (repeats > 5000) {
> --
> 2.24.1
>
