Re: [PATCH 4/9] xfs: ensure log tail is always up to date

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Aug 10, 2022 at 09:03:48AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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@redhat.com>
> ---
>  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
>