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".
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
