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