Re: [PATCH 3/5] xfs: repair free space btrees

Darrick J. Wong
2023-11-28T01:05:57Z

On Fri, Nov 24, 2023 at 10:11:18PM -0800, Christoph Hellwig wrote:
> On Fri, Nov 24, 2023 at 03:50:33PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Rebuild the free space btrees from the gaps in the rmap btree.
>
> This commit message feels a bit sparse for the amount of code added,
> although I can't really offer a good idea of what to add.

"Refer to the design documentation for more details:

Link: https://docs.kernel.org/filesystems/xfs-online-fsck-design.html?highlight=xfs#case-study-rebuilding-the-free-space-indices"

?

> Otherwise just two comments on the interaction with the rest of the
> xfs code, I'll try to digest the new repair code a bit more in the
> meantime.
>
> > +#ifdef CONFIG_XFS_ONLINE_REPAIR
> > + /*
> > + * Alternate btree heights so that online repair won't trip the write
> > + * verifiers while rebuilding the AG btrees.
> > + */
> > + uint8_t pagf_alt_levels[XFS_BTNUM_AGF];
> > +#endif
>
> Alternate and the alt_ prefix doesn't feel very descriptive. As far as
> I can tell these are about an ongoign repair, so as a at lest somewhat
> better choice call it "pagf_repair_levels"?

Done.

> > +xfs_failaddr_t
> > +xfs_alloc_check_irec(
> > + struct xfs_btree_cur *cur,
> > + const struct xfs_alloc_rec_incore *irec)
> > +{
> > + return xfs_alloc_check_perag_irec(cur->bc_ag.pag, irec);
> > +}
>
> Is there much of a point in even keeping this wrapper vs just
> switching xfs_alloc_check_irec to pass the pag instead of the
> cursor?

Not really. I'll remove this from the next spin.

--D