Re: [PATCH 01/26] xfs: define the format of rt groups

Darrick J. Wong
2024-08-23T18:12:51Z

On Thu, Aug 22, 2024 at 10:11:04PM -0700, Christoph Hellwig wrote:
> On Thu, Aug 22, 2024 at 05:21:25PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Define the ondisk format of realtime group metadata, and a superblock
> > for realtime volumes. rt supers are protected by a separate rocompat
> > bit so that we can leave them off if the rt device is zoned.
>
> We actually killed the flag again and just kept the separate helper
> to check for it.
>
> > Add a xfs_sb_version_hasrtgroups so that xfs_repair knows how to zero
> > the tail of superblocks.
>
> .. and merged the rtgroup and metadir flags, so while this helper
> still exists (and will get lots of use to make the code readable),
> that particular use case is gone now.

I'll just delete this sentence since xfs_sb_version_hasmetadir is in
another patch anyway.

> > -#define XFS_SB_FEAT_RO_COMPAT_FINOBT (1 << 0) /* free inode btree */
> > -#define XFS_SB_FEAT_RO_COMPAT_RMAPBT (1 << 1) /* reverse map btree */
> > -#define XFS_SB_FEAT_RO_COMPAT_REFLINK (1 << 2) /* reflinked files */
> > -#define XFS_SB_FEAT_RO_COMPAT_INOBTCNT (1 << 3) /* inobt block counts */
> > +#define XFS_SB_FEAT_RO_COMPAT_FINOBT (1 << 0) /* free inode btree */
> > +#define XFS_SB_FEAT_RO_COMPAT_RMAPBT (1 << 1) /* reverse map btree */
> > +#define XFS_SB_FEAT_RO_COMPAT_REFLINK (1 << 2) /* reflinked files */
> > +#define XFS_SB_FEAT_RO_COMPAT_INOBTCNT (1 << 3) /* inobt block counts */
>
> That also means the above is just a spurious unrelated cleanup now.
> Still useful, but maybe it should go into a sepaarate patch? Or just
> don't bother. Btw, one day we should clearly mark all our on-disk
> bitmaps as unsigned.

Eh, I'll drop it and make the next new rocompat feature do it.

> > + if (xfs_has_rtgroups(nmp))
> > + nmp->m_sb.sb_rgcount =
> > + howmany_64(nmp->m_sb.sb_rextents, nmp->m_sb.sb_rgextents);
>
> nmp->m_sb.sb_rgcount = howmany_64(nmp->m_sb.sb_rextents,
> nmp->m_sb.sb_rgextents);

Done.

> to avoid the overly long line.
>
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

--D