On Mon, Aug 26, 2024 at 07:52:43PM +1000, Dave Chinner wrote:
> On Thu, Aug 22, 2024 at 05:29:15PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > When metadir is enabled, we want to check the two new rtgroups fields,
> > and we don't want to check the old inumbers that are now in the metadir.
> >
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> > fs/xfs/scrub/agheader.c | 36 ++++++++++++++++++++++++------------
> > 1 file changed, 24 insertions(+), 12 deletions(-)
> >
> >
> > diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> > index cad997f38a424..0d22d70950a5c 100644
> > --- a/fs/xfs/scrub/agheader.c
> > +++ b/fs/xfs/scrub/agheader.c
> > @@ -147,14 +147,14 @@ xchk_superblock(
> > if (xfs_has_metadir(sc->mp)) {
> > if (sb->sb_metadirino != cpu_to_be64(mp->m_sb.sb_metadirino))
> > xchk_block_set_preen(sc, bp);
> > + } else {
> > + if (sb->sb_rbmino != cpu_to_be64(mp->m_sb.sb_rbmino))
> > + xchk_block_set_preen(sc, bp);
> > +
> > + if (sb->sb_rsumino != cpu_to_be64(mp->m_sb.sb_rsumino))
> > + xchk_block_set_preen(sc, bp);
> > }
> >
> > - if (sb->sb_rbmino != cpu_to_be64(mp->m_sb.sb_rbmino))
> > - xchk_block_set_preen(sc, bp);
> > -
> > - if (sb->sb_rsumino != cpu_to_be64(mp->m_sb.sb_rsumino))
> > - xchk_block_set_preen(sc, bp);
> > -
>
> If metadir is enabled, then shouldn't sb->sb_rbmino/sb_rsumino both
> be NULLFSINO to indicate they aren't valid?
The ondisk sb values aren't defined anymore and we set the incore values
to NULLFSINO (and never write that back out) so there's not much to
check anymore. I guess we could check that they're all zero or
something, which is what mkfs writes out, though my intent here was to
leave them as undefined bits, figuring that if we ever want to reuse
those fields we're going to define a new incompat bit anyway.
OTOH now would be the time to define what the field contents are
supposed to be -- zero or NULLFSINO?
> Given the rt inodes should have a well defined value even when
> metadir is enabled, I would say the current code that is validating
> the values are consistent with the primary across all secondary
> superblocks is correct and this change is unnecessary....
>
>
> > @@ -229,11 +229,13 @@ xchk_superblock(
> > * sb_icount, sb_ifree, sb_fdblocks, sb_frexents
> > */
> >
> > - if (sb->sb_uquotino != cpu_to_be64(mp->m_sb.sb_uquotino))
> > - xchk_block_set_preen(sc, bp);
> > + if (!xfs_has_metadir(mp)) {
> > + if (sb->sb_uquotino != cpu_to_be64(mp->m_sb.sb_uquotino))
> > + xchk_block_set_preen(sc, bp);
> >
> > - if (sb->sb_gquotino != cpu_to_be64(mp->m_sb.sb_gquotino))
> > - xchk_block_set_preen(sc, bp);
> > + if (sb->sb_gquotino != cpu_to_be64(mp->m_sb.sb_gquotino))
> > + xchk_block_set_preen(sc, bp);
> > + }
>
> Same - if metadir is in use and quota inodes are in the metadir,
> then the superblock quota inodes should be NULLFSINO....
Ok, I'll go with NULLFSINO ondisk and in memory.
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
