diff mbox series

[3/8] xfs: ensure log tail is always up to date

Message ID 20220708015558.1134330-4-david@fromorbit.com (mailing list archive)
State Superseded
Headers show
Series xfs: byte-base grant head reservation tracking | expand

Commit Message

Dave Chinner July 8, 2022, 1:55 a.m. UTC 
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. 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       |  3 +--
 fs/xfs/xfs_trans_ail.c | 16 ++++++++++++++--
 2 files changed, 15 insertions(+), 4 deletions(-)

Comments

Christoph Hellwig July 11, 2022, 6:07 a.m. UTC | #1 
On Fri, Jul 08, 2022 at 11:55:53AM +1000, Dave Chinner wrote:
>  	lockdep_assert_held(&log->l_icloglock);
> @@ -544,7 +543,7 @@ 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);
> +		xfs_lsn_t tail_lsn = atomic64_read(&log->l_tail_lsn);
>  		iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);

Nit: I'd just do this as:

		iclog->ic_header.h_tail_lsn =
			cpu_to_be64(atomic64_read(&log->l_tail_lsn));

> +/*
> + * 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.
> + */

Should we also document the old_lsn == 0 case here?
Dave Chinner July 11, 2022, 11:42 p.m. UTC | #2 
On Sun, Jul 10, 2022 at 11:07:56PM -0700, Christoph Hellwig wrote:
> On Fri, Jul 08, 2022 at 11:55:53AM +1000, Dave Chinner wrote:
> >  	lockdep_assert_held(&log->l_icloglock);
> > @@ -544,7 +543,7 @@ 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);
> > +		xfs_lsn_t tail_lsn = atomic64_read(&log->l_tail_lsn);
> >  		iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
> 
> Nit: I'd just do this as:
> 
> 		iclog->ic_header.h_tail_lsn =
> 			cpu_to_be64(atomic64_read(&log->l_tail_lsn));
> 
> > +/*
> > + * 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.
> > + */
> 
> Should we also document the old_lsn == 0 case here?

I can, it's just the "tail did not change" value....

Cheers,

Dave.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 72bedcf9eefb..bef3c0fda5ff 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -529,7 +529,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);
@@ -544,7 +543,7 @@  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);
+		xfs_lsn_t tail_lsn = atomic64_read(&log->l_tail_lsn);
 		iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
 	}
 
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index e41951894b96..b123e8dec76c 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -712,6 +712,12 @@  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.
+ */
 void
 xfs_ail_update_finish(
 	struct xfs_ail		*ailp,
@@ -796,10 +802,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);
 }