Re: [PATCH 2/3] xfs: don't allow log recovery when unknown rocompat bits are set

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 24, 2023 at 04:21:46PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> 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@fromorbit.com