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

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

On Tue, Jul 27, 2021 at 07:44:49AM +1000, Dave Chinner wrote:
> 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.

<nod> The -1 conversion is a pretty trivial cleanup that can go on the
end...

> 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.

Yes, I think so.

> 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...

<nod> Ok.  I figured that this 0 vs. -1 stuff was most likely just
inconsistent usage, but thought I should ask just in case there was
some real functional reason that I wasn't aware of.

--D

> > > + * @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