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. Hence I wonder if this change is necessary at all. If we can guarantee that anything adding a new log item type to the journal sets a LOG_INCOMPAT flag, then we don't need to change the RO_COMPAT handling in current kernels to avoid log recovery at all - the existing LOG_INCOMPAT flag handling will do that for us.... Yes, we can have a new feature that is RO_COMPAT + LOG_INCOMPAT; the reflink and rmap features should have been defined this way as that's where we went wrong. It's too late to set LOG_INCOMPAT for them, and so the only way to fix old supported kernels is to prevent log recovery when unknown RO_COMPAT bits are set. Hence I don't see this solution as necessary for any kernel recent enough to support rmap/reflink, nor do I see it necessary to protect against making the same mistake about RO_COMPAT features in the future. Everyone now knows that a log format change requires LOG_INCOMPAT, not RO_COMPAT, so we should not be making that mistake again..... Thoughts? -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
