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

Darrick J. Wong
2024-12-05T07:17:05Z

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