| Message ID | 172437089450.61495.17228908896759675474.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [1/6] xfs: refactor xfs_qm_destroy_quotainos | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Aug 22, 2024 at 05:29:15PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > 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@kernel.org> > --- > 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? 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.... -Dave.
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@kernel.org> > > > > 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@kernel.org> > > --- > > 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@fromorbit.com >
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@kernel.org> > > > > > > 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@kernel.org> > > > --- > > > 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.
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@kernel.org> > > > > > > > > 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@kernel.org> > > > > --- > > > > 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@fromorbit.com
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 (sb->sb_rextsize != cpu_to_be32(mp->m_sb.sb_rextsize)) xchk_block_set_corrupt(sc, bp); @@ -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); + } /* * Skip the quota flags since repair will force quotacheck. @@ -342,8 +344,10 @@ xchk_superblock( if (sb->sb_spino_align != cpu_to_be32(mp->m_sb.sb_spino_align)) xchk_block_set_corrupt(sc, bp); - if (sb->sb_pquotino != cpu_to_be64(mp->m_sb.sb_pquotino)) - xchk_block_set_preen(sc, bp); + if (!xfs_has_metadir(mp)) { + if (sb->sb_pquotino != cpu_to_be64(mp->m_sb.sb_pquotino)) + xchk_block_set_preen(sc, bp); + } /* Don't care about sb_lsn */ } @@ -354,6 +358,14 @@ xchk_superblock( xchk_block_set_corrupt(sc, bp); } + if (xfs_has_metadir(mp)) { + if (sb->sb_rgcount != cpu_to_be32(mp->m_sb.sb_rgcount)) + xchk_block_set_corrupt(sc, bp); + + if (sb->sb_rgextents != cpu_to_be32(mp->m_sb.sb_rgextents)) + xchk_block_set_corrupt(sc, bp); + } + /* Everything else must be zero. */ if (memchr_inv(sb + 1, 0, BBTOB(bp->b_length) - sizeof(struct xfs_dsb)))