| Message ID | 173328106652.1145623.7325198732846866757.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace | expand |
On Tue, Dec 03, 2024 at 07:03:16PM -0800, Darrick J. Wong wrote: > +/* Calculate the ondisk superblock size in bytes */ > +STATIC size_t > +xchk_superblock_ondisk_size( > + struct xfs_mount *mp) > +{ > + if (xfs_has_metadir(mp)) > + return offsetofend(struct xfs_dsb, sb_pad); > + if (xfs_has_metauuid(mp)) > + return offsetofend(struct xfs_dsb, sb_meta_uuid); > + if (xfs_has_crc(mp)) > + return offsetofend(struct xfs_dsb, sb_lsn); > + if (xfs_sb_version_hasmorebits(&mp->m_sb)) > + return offsetofend(struct xfs_dsb, sb_bad_features2); > + if (xfs_has_logv2(mp)) > + return offsetofend(struct xfs_dsb, sb_logsunit); > + if (xfs_has_sector(mp)) > + return offsetofend(struct xfs_dsb, sb_logsectsize); > + /* only support dirv2 or more recent */ > + return offsetofend(struct xfs_dsb, sb_dirblklog); 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. > +} > /* Everything else must be zero. */ > - if (memchr_inv(sb + 1, 0, > - BBTOB(bp->b_length) - sizeof(struct xfs_dsb))) > + sblen = xchk_superblock_ondisk_size(mp); > + if (memchr_inv((char *)sb + sblen, 0, BBTOB(bp->b_length) - sblen)) This could be simplified to if (memchr_inv(bp->b_addr + sblen, 0, BBTOB(bp->b_length) - sblen)) Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Dec 04, 2024 at 12:27:38AM -0800, Christoph Hellwig wrote: > On Tue, Dec 03, 2024 at 07:03:16PM -0800, Darrick J. Wong wrote: > > +/* Calculate the ondisk superblock size in bytes */ > > +STATIC size_t > > +xchk_superblock_ondisk_size( > > + struct xfs_mount *mp) > > +{ > > + if (xfs_has_metadir(mp)) > > + return offsetofend(struct xfs_dsb, sb_pad); > > + if (xfs_has_metauuid(mp)) > > + return offsetofend(struct xfs_dsb, sb_meta_uuid); > > + if (xfs_has_crc(mp)) > > + return offsetofend(struct xfs_dsb, sb_lsn); > > + if (xfs_sb_version_hasmorebits(&mp->m_sb)) > > + return offsetofend(struct xfs_dsb, sb_bad_features2); > > + if (xfs_has_logv2(mp)) > > + return offsetofend(struct xfs_dsb, sb_logsunit); > > + if (xfs_has_sector(mp)) > > + return offsetofend(struct xfs_dsb, sb_logsectsize); > > + /* only support dirv2 or more recent */ > > + return offsetofend(struct xfs_dsb, sb_dirblklog); > > 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. 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. > > +} > > /* Everything else must be zero. */ > > - if (memchr_inv(sb + 1, 0, > > - BBTOB(bp->b_length) - sizeof(struct xfs_dsb))) > > + sblen = xchk_superblock_ondisk_size(mp); > > + if (memchr_inv((char *)sb + sblen, 0, BBTOB(bp->b_length) - sblen)) > > This could be simplified to > > if (memchr_inv(bp->b_addr + sblen, 0, BBTOB(bp->b_length) - sblen)) <nod> > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks! --D
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?
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
diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c index 88063d67cb5fd4..190d56f8134463 100644 --- a/fs/xfs/scrub/agheader.c +++ b/fs/xfs/scrub/agheader.c @@ -59,6 +59,27 @@ xchk_superblock_xref( /* scrub teardown will take care of sc->sa for us */ } +/* Calculate the ondisk superblock size in bytes */ +STATIC size_t +xchk_superblock_ondisk_size( + struct xfs_mount *mp) +{ + if (xfs_has_metadir(mp)) + return offsetofend(struct xfs_dsb, sb_pad); + if (xfs_has_metauuid(mp)) + return offsetofend(struct xfs_dsb, sb_meta_uuid); + if (xfs_has_crc(mp)) + return offsetofend(struct xfs_dsb, sb_lsn); + if (xfs_sb_version_hasmorebits(&mp->m_sb)) + return offsetofend(struct xfs_dsb, sb_bad_features2); + if (xfs_has_logv2(mp)) + return offsetofend(struct xfs_dsb, sb_logsunit); + if (xfs_has_sector(mp)) + return offsetofend(struct xfs_dsb, sb_logsectsize); + /* only support dirv2 or more recent */ + return offsetofend(struct xfs_dsb, sb_dirblklog); +} + /* * Scrub the filesystem superblock. * @@ -75,6 +96,7 @@ xchk_superblock( struct xfs_buf *bp; struct xfs_dsb *sb; struct xfs_perag *pag; + size_t sblen; xfs_agnumber_t agno; uint32_t v2_ok; __be32 features_mask; @@ -388,8 +410,8 @@ xchk_superblock( } /* Everything else must be zero. */ - if (memchr_inv(sb + 1, 0, - BBTOB(bp->b_length) - sizeof(struct xfs_dsb))) + sblen = xchk_superblock_ondisk_size(mp); + if (memchr_inv((char *)sb + sblen, 0, BBTOB(bp->b_length) - sblen)) xchk_block_set_corrupt(sc, bp); xchk_superblock_xref(sc, bp);