Re: [PATCH 4/6] xfs: fix zero byte checking in the superblock scrubber

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Dec 04, 2024 at 10:48:39PM -0800, Christoph Hellwig wrote:
> On Wed, Dec 04, 2024 at 09:54:51PM -0800, Darrick J. Wong wrote:
> > > This really should be libxfs so tht it can be shared with
> > > secondary_sb_whack in xfsrepair.  The comment at the end of
> > > the xfs_dsb definition should also be changed to point to this
> > > libxfs version.
> > 
> > The xfs_repair version of this is subtlely different -- given a
> > secondary ondisk superblock, it figures out the size of the ondisk
> > superblock given the features set *in that alleged superblock*.  From
> > there it validates the secondary superblock.  The featureset in the
> > alleged superblock doesn't even have to match the primary super, but
> > it'll go zero things all the same before copying the incore super back
> > to disk:
> > 
> > 	if (xfs_sb_version_hasmetadir(sb))
> > 		size = offsetofend(struct xfs_dsb, sb_pad);
> > 	else if (xfs_sb_version_hasmetauuid(sb))
> > 		size = offsetofend(struct xfs_dsb, sb_meta_uuid);
> > 
> > This version in online computes the size of the secondary ondisk
> > superblock object given the features set in the *primary* superblock
> > that we used to mount the filesystem.
> 
> Well, it considers the size for the passed in superblock.  Where the
> passed in one happens to be the primary one and the usage is for the
> second.
> 
> > Also if I did that we'd have to recopy the xfs_sb_version_hasXXXX
> > functions back into libxfs after ripping most of them out.  Or we'd have
> > to encode the logic manually.  But even then, the xfs_repair and
> > xfs_scrub functions are /not quite/ switching on the same thing.
> 
> We don't really need the helpers and could just check the flag vs
> the field directly.
> 
> I'd personally prefer to share this code, but I also don't want to
> hold off the fix for it.  So if you prefer to stick to this
> version maybe just clearly document why these two are different
> with a comment that has the above information?

Ok.  I was thinking this hoist is a reasonable cleanup for 6.14 anyway,
not a bugfix to apply to 6.13.

--D