Re: [PATCH 04/10] xfs: fix ordering violation between cache flushes and tail updates

[Dave Chinner <david@fromorbit.com>]

On Mon, Jul 26, 2021 at 10:35:21AM -0700, Darrick J. Wong wrote:
> On Mon, Jul 26, 2021 at 04:07:10PM +1000, Dave Chinner wrote:
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -489,12 +489,20 @@ xfs_log_reserve(
> >  
> >  /*
> >   * Flush iclog to disk if this is the last reference to the given iclog and the
> > - * it is in the WANT_SYNC state.
> > + * it is in the WANT_SYNC state. If the caller passes in a non-zero
> 
> I've noticed that the log code isn't always consistent about special
> looking LSNs -- some places use NULLCOMMITLSN, some places opencode
> (xfs_lsn_t)-1, and other code uses zero.  Is there some historical
> reason for having these distinct values?  Or do they actually mean
> separate things?

If depends on the use case. I resisted converting the (xfs_lsn_t)-1s
in and around this patchset to NULLCOMMITLSN just to keep the noise
down. Where-ever we use -1 as a LSN, we should really use
NULLCOMMITLSN.

As for comparing to zero, that works because we the LSN is largely
unlikely to have 64 bit overflow in the lifetime of the journal. And
if it does overflow, it's unlikely that we'll overflow exactly to
(0,0) as a meaningful LSN.

However, I think we've probably got bigger problems around xfs_lsn_t
overflowing, such as it being defined as signed rather than
unsigned and I suspect that XFS_LSN_CMP() fails if we overflow LSNs
back to zero and compare high cycle (old) with low cycle (new) LSNs.

So, really, right now I've ostriched this issue because I'm still
trying to work throw g/482 failures and I suspect we need to change
the definition of a LSN to fix these issues...

> > + * @old_tail_lsn, then we need to check if the log tail is different to the
> > + * caller's value. If it is different, this indicates that the log tail has
> > + * moved since the caller sampled the log tail and issued a cache flush and so
> > + * there may be metadata on disk that we need to flush before this iclog is
> 
> "If the caller passes in a non-zero @old_tail_lsn and the current log
> tail does not match, there may be metadata on disk that must be
> persisted before this iclog is written.  To satisfy that requirement,
> set the XLOG_ICL_NEED_FLUSH flag as a condition for writing this iclog
> with the new log tail value." ?

Ok.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com