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@xxxxxxxxxxxxx
