On Mon, Aug 28, 2023 at 12:08:22PM -0700, Darrick J. Wong wrote: > On Thu, Aug 24, 2023 at 09:04:17PM -0700, Darrick J. Wong wrote: > > On Fri, Aug 25, 2023 at 11:07:56AM +1000, Dave Chinner wrote: > > > On Thu, Aug 24, 2023 at 04:21:46PM -0700, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > > > > > > > Don't allow log recovery to proceed on a readonly mount if the primary > > > > superblock advertises unknown rocompat bits. We used to allow this, but > > > > due to a misunderstanding between Dave and Darrick back in 2016, we > > > > cannot do that anymore. The XFS_SB_FEAT_RO_COMPAT_RMAPBT feature (4.8) > > > > protects RUI log items, and the REFLINK feature (4.9) protects CUI/BUI > > > > log items, which is why we can't allow older kernels to recover them. > > > > > > Ok, this would work for kernels that don't know waht the > > > REFLINK/RMAP features are, but upstream kernels will never fail to > > > recover these items because these are known ROCOMPAT bits. > > > > > > The reason this problem exists is that we've accidentally > > > conflated RO_COMPAT with LOG_INCOMPAT. If RUI/CUI/BUI creation had > > > of set a log incompat bit whenever they are used (similar to the > > > new ATTRI stuff setting log incompat bits), then older kernels > > > would not have allow log recovery even if the reflink/rmap RO_COMPAT > > > features were set and they didn't understand them. > > > > > > However, we can't do that on current kernels because then older > > > kernels that understand reflink/rmap just fine would see an unknown > > > log incompat bit and refuse to replay the log. So it comes back to > > > how we handle unknown ROCOMPAT flags on older kernels, not current > > > upstream kernels. > > > > > > i.e. this patch needs to be backported to kernels that don't know > > > anything about RMAP/REFLINK to be useful to anyone. i.e. kernels > > > older than 4.9 that don't know what rmap/reflink are. I suspect > > > that there are very few supported kernels that old that this might > > > get backported to. > > Seeing as the oldest LTS kernel now is 4.14, I agree with you, let's > just forget this whole patch and try to remember not to hide LOG > INCOMPAT features behind RO COMPAT flags again. :) > > If we do that, then all we need to do is change xfs_validate_sb_write > not to complain about unknown rocompat features if the xfs is readonly, > and remove all the code that temporarily clears the readonly state > around the log mount calls. > > Log recovery then goes back to supporting recovery even in the presence > of unknown rocompat bits, and patch 3 becomes unnecessary... *nod* > ...though this below is still true. > > > Hmm. The most annoying thing about LOG_INCOMPAT features is that > > turning them on requires a synchronous write to the primary sb along > > with a transaction to log the sb that is immediately forced to disk. > > Every time the log cleans itself it clears the LOG_INCOMPAT features, > > and then we have to do that /again/. > > > > Parent pointers, since they require log intent items to guarantee the > > dirent and pptr update, cycle the logged xattr LOG_INCOMPAT feature on > > and off repeatedly. A couple of weeks ago I decided to elide all that > > LOG_INCOMPAT cycling if parent pointers are enabled, and fstests runtime > > went from 4.9 hours back down to 4.4. (Parent pointers, for whatever > > reason, got an INCOMPAT feature bit so it's ok). I was a little > > surprised that xfs_log_clean ran that much, but there we go. Sure, but that's only a problem for operations that are a pure log format change. With parent pointers, we have an INCOMPAT bit because we have a new attr filter bit that older kernels will flag as corruption, and that means we don't need a log incompat bit for the attr logging. IOWs, if xfs_has_parent_pointers() is true, then we don't need to set the ATTRI log incompat bit, ever, because the parent pointer incompat feature bit implies ATTRI log items are in use and all kernels that understand the PP incompat bit also understand the ATTRI log items.... > > A different way to solve the cycling problem could be to start a timer > > after the last caller drops l_incompat_xattrs and only clear the feature > > bit after 5 minutes of idleness or unmount, instead of the next time the > > log cleans itself. Well, we only clear it from the xfs_log_worker() if the log needs covering, so the AIL and iclogs need to be empty before the worker will clear the incompat bit. That's on a 30s timer already, so perhaps all we need to do is extend the log covering timeout if there are incompat log flags set.... > > Alternately, we drop this patch and declare an INCOMPAT_RMAPREFLINKV2 > > feature that wraps up all the other broken bits that we've found since > > 2016 (overly large log reservations, incorrect units in xattr block > > reservation calculations, etc.) It's tempting, but let's try to put that off until we really need to.... -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
