On Mon, Jul 26, 2021 at 04:07:11PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> We force iclogs in several places - we need them all to have the
> same cache flush semantics, so start by factoring out the iclog
> force into a common helper.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Looks like a pretty simple hoist.
I also wonder about the removal of the assertion in xlog_unmount_write,
but I /think/ that's ok because at that point we've turned off
everything else that could write the log and forced it, which means that
the log is clean, and therefore all iclogs ought to be in ACTIVE state?
If I got that right,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_log.c | 42 ++++++++++++++++++------------------------
> 1 file changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 1f8c00e40c53..7107cf542eda 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -781,6 +781,20 @@ xfs_log_mount_cancel(
> xfs_log_unmount(mp);
> }
>
> +/*
> + * Flush out the iclog to disk ensuring that device caches are flushed and
> + * the iclog hits stable storage before any completion waiters are woken.
> + */
> +static inline int
> +xlog_force_iclog(
> + struct xlog_in_core *iclog)
> +{
> + atomic_inc(&iclog->ic_refcnt);
> + if (iclog->ic_state == XLOG_STATE_ACTIVE)
> + xlog_state_switch_iclogs(iclog->ic_log, iclog, 0);
> + return xlog_state_release_iclog(iclog->ic_log, iclog, 0);
> +}
> +
> /*
> * Wait for the iclog and all prior iclogs to be written disk as required by the
> * log force state machine. Waiting on ic_force_wait ensures iclog completions
> @@ -866,18 +880,8 @@ xlog_unmount_write(
>
> spin_lock(&log->l_icloglock);
> iclog = log->l_iclog;
> - atomic_inc(&iclog->ic_refcnt);
> - if (iclog->ic_state == XLOG_STATE_ACTIVE)
> - xlog_state_switch_iclogs(log, iclog, 0);
> - else
> - ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> - iclog->ic_state == XLOG_STATE_IOERROR);
> - /*
> - * Ensure the journal is fully flushed and on stable storage once the
> - * iclog containing the unmount record is written.
> - */
> iclog->ic_flags |= (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA);
> - error = xlog_state_release_iclog(log, iclog, 0);
> + error = xlog_force_iclog(iclog);
> xlog_wait_on_iclog(iclog);
>
> if (tic) {
> @@ -3204,17 +3208,9 @@ xfs_log_force(
> iclog = iclog->ic_prev;
> } else if (iclog->ic_state == XLOG_STATE_ACTIVE) {
> if (atomic_read(&iclog->ic_refcnt) == 0) {
> - /*
> - * We are the only one with access to this iclog.
> - *
> - * Flush it out now. There should be a roundoff of zero
> - * to show that someone has already taken care of the
> - * roundoff from the previous sync.
> - */
> - atomic_inc(&iclog->ic_refcnt);
> + /* We have exclusive access to this iclog. */
> lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> - xlog_state_switch_iclogs(log, iclog, 0);
> - if (xlog_state_release_iclog(log, iclog, 0))
> + if (xlog_force_iclog(iclog))
> goto out_error;
>
> if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn)
> @@ -3292,9 +3288,7 @@ xlog_force_lsn(
> &log->l_icloglock);
> return -EAGAIN;
> }
> - atomic_inc(&iclog->ic_refcnt);
> - xlog_state_switch_iclogs(log, iclog, 0);
> - if (xlog_state_release_iclog(log, iclog, 0))
> + if (xlog_force_iclog(iclog))
> goto out_error;
> if (log_flushed)
> *log_flushed = 1;
> --
> 2.31.1
>
