On Tue, Nov 28, 2023 at 03:30:09PM -0800, Darrick J. Wong wrote:
> On Tue, Nov 28, 2023 at 06:05:48AM -0800, Christoph Hellwig wrote:
> > > + /*
> > > + * Now that we've locked the rtbitmap and rtsummary, we can't race with
> > > + * growfsrt trying to expand the summary or change the size of the rt
> > > + * volume. Hence it is safe to compute and check the geometry values.
> > > + */
> > > + rts->rextents = xfs_rtb_to_rtx(mp, mp->m_sb.sb_rblocks);
> > > + rts->rbmblocks = xfs_rtbitmap_blockcount(mp, rts->rextents);
> > > + rts->rsumlevels = rts->rextents ? xfs_highbit32(rts->rextents) + 1 : 0;
> > > + rsumblocks = xfs_rtsummary_blockcount(mp, rts->rsumlevels,
> > > + rts->rbmblocks);
> > > + rts->rsumsize = XFS_FSB_TO_B(mp, rsumblocks);
> >
> > Same nitpick as for the last patch.
>
> LOL so I just tried a 64k rt volume with a 1M rextsize and mkfs crashed.
> I guess I'll go sort out what's going on there...
Oh good, XFS has been broken since the beginning of git history for the
stupid corner case where the rtblocks < rextsize. In this case, mkfs
will set sb_rextents and sb_rextslog both to zero:
sbp->sb_rextslog =
(uint8_t)(rtextents ?
libxfs_highbit32((unsigned int)rtextents) : 0);
However, that's not the check that xfs_repair uses for nonzero rtblocks:
if (sb->sb_rextslog !=
libxfs_highbit32((unsigned int)sb->sb_rextents))
The difference here is that xfs_highbit32 returns -1 if its argument is
zero, which means that for a runt rt volume, repair thinks the "correct"
value of rextslog is -1, even though mkfs wrote it as 0 and flags a
freshly formatted filesystem as corrupt. Because mkfs has been writing
ondisk artifacts like this for decades, we have to accept that as
"correct". TBH, zero rextslog for zero rtextents makes more sense to me
anyway.
Regrettably, the superblock verifier checks created in commit copied
xfs_repair even though mkfs has been writing out such filesystems for
ages. In testing /that/ fix, I discovered that the logic above is
wrong -- rsumlevels is always rextslog + 1 when rblocks > 0, even if
rextents == 0.
IOWs, this logic needs to be:
/*
* Now that we've locked the rtbitmap and rtsummary, we can't race with
* growfsrt trying to expand the summary or change the size of the rt
* volume. Hence it is safe to compute and check the geometry values.
*/
if (mp->m_sb.sb_rblocks) {
xfs_filblks_t rsumblocks;
int rextslog = 0;
rts->rextents = xfs_rtb_to_rtx(mp, mp->m_sb.sb_rblocks);
if (rts->rextents)
rextslog = xfs_highbit32(rts->rextents);
rts->rsumlevels = rextslog + 1;
rts->rbmblocks = xfs_rtbitmap_blockcount(mp, rts->rextents);
rsumblocks = xfs_rtsummary_blockcount(mp, rts->rsumlevels,
rts->rbmblocks);
rts->rsumsize = XFS_FSB_TO_B(mp, rsumblocks);
}
Yay winning.
--D
> > Otherwise looks good:
> >
> > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>
> Thanks!
>
> --D
>
