On Tue, Aug 27, 2024 at 12:16:43PM +1000, Dave Chinner wrote:
> On Mon, Aug 26, 2024 at 11:07:47AM -0700, Darrick J. Wong wrote:
> > 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?
>
> Yeah, I think it's best to give them a solid definition, that way we
> don't bump up against "we can't tell if it has never been used
> before" problems.
>
> >
> > > 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.
>
> OK.
>
> Just to add to that (because I looked), mkfs.xfs does this to
> initialise rtino numbers before they are allocated:
>
> $ git grep NULLFSINO mkfs
> mkfs/xfs_mkfs.c: sbp->sb_rootino = sbp->sb_rbmino = sbp->sb_rsumino = NULLFSINO;
> $
>
> and repair does this for quota inodes when clearing the superblock
> inode fields:
>
> $ git grep NULLFSINO repair/dinode.c
> repair/dinode.c: mp->m_sb.sb_uquotino = NULLFSINO;
> repair/dinode.c: mp->m_sb.sb_gquotino = NULLFSINO;
> repair/dinode.c: mp->m_sb.sb_pquotino = NULLFSINO;
> $
>
> So the current code is typically using NULLFSINO instead of zero on
> disk for "inode does not exist".
<nod> Though I noticed that it writes out sb_[ugp]quotino = 0.
Christoph once remarked that those parts of the sb were at some point
unused, so they were zero, and they only become NULLFSINO once someone
turns on QUOTABIT in sb_versionnum.
Regardless, all 1s is ok by me.
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
