Re: [PATCH] xfs: check v5 superblock features early

On Wed, Jul 29, 2026 at 08:35:06AM +0200, Christoph Hellwig wrote:
> On Tue, Jul 28, 2026 at 08:47:30AM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 28, 2026 at 05:42:07PM +0200, Christoph Hellwig wrote:
> > > On Tue, Jul 28, 2026 at 08:26:28AM -0700, Darrick J. Wong wrote:
> > > > On Tue, Jul 28, 2026 at 10:04:35AM +0200, Christoph Hellwig wrote:
> > > > > When working on a new features that reuses the existing pad in the
> > > > > superblock, I noticed that mounting such a file system on an old kernel
> > > > > logs a rather confusing warning:
> > > > > 
> > > > >     XFS (vdc): Metadir superblock padding fields must be zero.
> > > > > 
> > > > > This is because we only validate the various feature fields in v5
> > > > > superblocks after the common superblock validation helper is called.
> > > > > 
> > > > > Fix this by calling the feature validation first.  To make this more
> > > > > obvious, rename xfs_validate_sb_read to xfs_validate_sb_features and only
> > > > > call it for v5 file systems.
> > > > 
> > > > Shouldn't it be called xfs_validate_v5_sb_features then?
> > > > 
> > > > Oh.  There's already a xfs_sb_validate_v5_features function that checks
> > > > that the v4 feature bits are set correctly for a v5 filesystem, and it
> > > > would be confusing to have both.
> > > 
> > > Or just stick to the old name :)
> > > 
> > > > Could we move the code in
> > > > xfs_validate_sb_read into xfs_sb_validate_v5_features instead?
> > > 
> > > But yes, that does look sensible as well.
> > > 
> > > > If you do that, then xfs_sb_good_version will validate the feature bit
> > > > recognition, which it doesn't do now.  I'm not sure what weird side
> > > > effects might result from that though.
> > > 
> > > None in the kernel, but userspace has a lot more callers that need a
> > > careful look.
> > 
> > I /think/ the only visible change to userspace is that the code in
> > repair that checks v5 feature flag recognition now becomes defunct
> > because libxfs_sb_good_version will catch it first.  But the
> > xfs_validate_sb_read code will already log (more or less) the same
> > complaint so I don't think it will have any real effect.
> 
> Yeah.  OTOH we now also add it to writes.

<shrug> xfs_validate_sb_write already checks the feature bits, so adding
it to the common function is no big deal.

> I think I'll reduce scope for this patch and drop the renaming
> and just do the check earlier for a trivial backportable fix.
> 
> Then I'll add your above idea to my started work of refactoring the
> sb verifier for reuse in xfs_repair for the next merge window or
> whenver I get to it.

OK.

--D