On Wed, Aug 10, 2022 at 09:03:48AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Whenever we write an iclog, we call xlog_assign_tail_lsn() to update
> the current tail before we write it into the iclog header. This
> means we have to take the AIL lock on every iclog write just to
> check if the tail of the log has moved.
>
> This doesn't avoid races with log tail updates - the log tail could
> move immediately after we assign the tail to the iclog header and
> hence by the time the iclog reaches stable storage the tail LSN has
> moved forward in memory. Hence the log tail LSN in the iclog header
> is really just a point in time snapshot of the current state of the
> AIL.
>
> With this in mind, if we simply update the in memory log->l_tail_lsn
> every time it changes in the AIL, there is no need to update the in
> memory value when we are writing it into an iclog - it will already
> be up-to-date in memory and checking the AIL again will not change
> this.
This is too subtle for me to understand -- does the codebase
already update l_tail_lsn? Does this patch make it do that?
--D
> Hence xlog_state_release_iclog() does not need to check the
> AIL to update the tail lsn and can just sample it directly without
> needing to take the AIL lock.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_log.c | 5 ++---
> fs/xfs/xfs_trans_ail.c | 17 +++++++++++++++--
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index c609c188bd8a..042744fe37b7 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -530,7 +530,6 @@ xlog_state_release_iclog(
> struct xlog_in_core *iclog,
> struct xlog_ticket *ticket)
> {
> - xfs_lsn_t tail_lsn;
> bool last_ref;
>
> lockdep_assert_held(&log->l_icloglock);
> @@ -545,8 +544,8 @@ xlog_state_release_iclog(
> if ((iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> (iclog->ic_flags & XLOG_ICL_NEED_FUA)) &&
> !iclog->ic_header.h_tail_lsn) {
> - tail_lsn = xlog_assign_tail_lsn(log->l_mp);
> - iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
> + iclog->ic_header.h_tail_lsn =
> + cpu_to_be64(atomic64_read(&log->l_tail_lsn));
> }
>
> last_ref = atomic_dec_and_test(&iclog->ic_refcnt);
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index d3dcb4942d6a..5f40509877f7 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -715,6 +715,13 @@ xfs_ail_push_all_sync(
> finish_wait(&ailp->ail_empty, &wait);
> }
>
> +/*
> + * Callers should pass the the original tail lsn so that we can detect if the
> + * tail has moved as a result of the operation that was performed. If the caller
> + * needs to force a tail LSN update, it should pass NULLCOMMITLSN to bypass the
> + * "did the tail LSN change?" checks. If the caller wants to avoid a tail update
> + * (e.g. it knows the tail did not change) it should pass an @old_lsn of 0.
> + */
> void
> xfs_ail_update_finish(
> struct xfs_ail *ailp,
> @@ -799,10 +806,16 @@ xfs_trans_ail_update_bulk(
>
> /*
> * If this is the first insert, wake up the push daemon so it can
> - * actively scan for items to push.
> + * actively scan for items to push. We also need to do a log tail
> + * LSN update to ensure that it is correctly tracked by the log, so
> + * set the tail_lsn to NULLCOMMITLSN so that xfs_ail_update_finish()
> + * will see that the tail lsn has changed and will update the tail
> + * appropriately.
> */
> - if (!mlip)
> + if (!mlip) {
> wake_up_process(ailp->ail_task);
> + tail_lsn = NULLCOMMITLSN;
> + }
>
> xfs_ail_update_finish(ailp, tail_lsn);
> }
> --
> 2.36.1
>
