On Wed, Dec 09, 2020 at 03:19:50PM +1100, Dave Chinner wrote: > On Tue, Dec 08, 2020 at 07:26:24PM -0800, Darrick J. Wong wrote: > > On Tue, Dec 08, 2020 at 02:19:13PM -0500, Brian Foster wrote: > > > On Tue, Dec 08, 2020 at 10:10:27AM -0800, Darrick J. Wong wrote: > > > > On Tue, Dec 08, 2020 at 06:19:06AM -0500, Brian Foster wrote: > > > > > On Mon, Dec 07, 2020 at 04:40:28PM -0800, Darrick J. Wong wrote: > > > > > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > > > > > > > > > > Log incompat feature flags in the superblock exist for one purpose: to > > > > > > protect the contents of a dirty log from replay on a kernel that isn't > > > > > > prepared to handle those dirty contents. This means that they can be > > > > > > cleared if (a) we know the log is clean and (b) we know that there > > > > > > aren't any other threads in the system that might be setting or relying > > > > > > upon a log incompat flag. > > > > > > > > > > > > Therefore, clear the log incompat flags when we've finished recovering > > > > > > the log, when we're unmounting cleanly, remounting read-only, or > > > > > > freezing; and provide a function so that subsequent patches can start > > > > > > using this. > > > > > > > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > > > > --- > > > > > > Note: I wrote this so that we could turn on log incompat flags for > > > > > > atomic extent swapping and Allison could probably use it for the delayed > > > > > > logged xattr patchset. Not gonna try to land this in 5.11, FWIW... > > > > > > --- > .... > > > > > unmount, we ensure a full/sync AIL push completes (and moves the in-core > > > > > tail) before we log the feature bit change. I do wonder if it's worth > > > > > complicating the log quiesce path to clear feature bits at all, but I > > > > > suppose it could be a little inconsistent to clean the log on freeze yet > > > > > leave an incompat bit around. Perhaps we should push the clear bit > > > > > sequence down into the log quiesce path between completing the AIL push > > > > > and writing the unmount record. We may have to commit a sync transaction > > > > > and then push the AIL again, but that would cover the unmount and freeze > > > > > cases and I think we could probably do away with the post-recovery bit > > > > > clearing case entirely. A current/recovered mount should clear the > > > > > associated bits on the next log quiesce anyways. Hm? > > > > > > > > Hm. You know how xfs_log_quiesce takes and releases the superblock > > > > buffer lock after pushing the AIL but before writing the unmount record? > > > > What if we did the log_incompat feature clearing + bwrite right after > > > > that? > > > > > > > > > > Yeah, that's where I was thinking for the unmount side. I'm not sure we > > > want to just bwrite there vs. log+push though. Otherwise, I think > > > there's still a window of potential inconsistency between when the > > > superblock write completes and the unmount record lands on disk. > > The only thing the unmount record really does is ensure that the > head and tail of the log point to the unmount record. that's what > marks the log clean, not the unmount record itself. We use a special > record here because we're not actually modifying anything - it's not > a transaction, just a marker to get the log head + tail to point to > the same LSN in the log. > Right.. > Log covering does the same thing via logging and flushing the > superblock multiple times - it makes sure that the only item in the > commit at the head of the journal has a tail pointer that also > points to the same checkpoint. The fact that it's got a "dirty" > unmodified superblock in rather than being an unmount record is > largely irrelevant - the rest of the log has been marked as clean by > this checkpoint and the replay of the SB from this checkpoint is > effectively a no-op. > That's close to what I was trying to accomplish above (independent of whether the approach was actually correct ;). The difference is that I wasn't trying to mark the log clean generically, but guarantee it's "clean of incompat items" by the time the superblock buffer that clears the incompat feature bit hits the log. That way a recovery would update the superblock on current kernels and old kernels would either see a dirty log or the a clean log with the updated super. > In fact, I think it's a "no-op" we can take advantage of..... > > > > I > > > _think_ the whole log force -> AIL push (i.e. move log tail) -> update > > > super -> log force -> AIL push sequence ensures that if an older kernel > > > saw the updated super, the log would technically be dirty but we'd at > > > least be sure that all incompat log items are behind the tail. > > It's a bit more complex than that - if the log is not dirty, then > the second log force does nothing, and the tail of the log does not > get updated. Hence you have to log, commit and writeback the > superblock twice to ensure that the log tail has been updated in the > journal to after the point in time where the superblock was *first > logged*. > I'm a little confused by your statement. The second log force would come after we've explicitly logged the superblock (having cleared the feature bit), so the log would have to be dirty at that point. Do you mean if the log is not dirty from the start, something else, or just misread? > The log covering state machine handles this all correctly, and it > does it using the superblock as the mechanism that moves the tail of > the log forward. I suspect that we could use the second logging of > the superblock in that state machine to clear/set log incompat > flags, as that is the commit that marks the log "empty". If we crash > before the sb write at this point, log recovery will only see the > superblock to recover, so it doesn't matter that there's a change in > log incompat bits here because after recovery we're going to clear > them, right? > Hmm.. but if we update the superblock in the same checkpoint that marks the log empty and then fail before the superblock write, the superblock update is lost since recovery would never happen (because the log is empty). I think the more significant problem with that is we can now present a filesystem with a clean log to an older kernel with the bit set. This is why I was trying to basically log the superblock update in a checkpoint that also ensured the tail moved forward past incompat items but didn't necessarily clean the log. I wonder if we can fit that state into the existing log covering mechanism by logging the update earlier (or maybe via an intermediate state..)? I'll need to go stare at that code a bit.. > I'd like to avoid re-inventing the wheel here if we can :) > Agreed. The idle log covering mechanism crossed my mind but I didn't look further into it because I thought it was an isolated mechanism (and hadn't grok'd the details of it). I.e., we'd still have to handle freeze and unmount separately so it made more sense to grok those cases first. If you think there's a possibility to rework it so the same covering mechanism can be reused across idle, quiesce, unmount, and that we can conditionally fit a superblock log incompat bit clear into that sequence, that sounds like a nice overall approach to me. Brian > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx >
