Re: [PATCH 4/5] xfs: repair inode btrees

Christoph Hellwig
2023-11-28T15:57:22Z

This generally looks good to me.

A bunch of my superficial comments to the previous patch apply
here as well, but I'm not going to repeat them, but I have a bunch of
new just as nitpicky ones:

> + uint64_t realfree;
>
> + if (!xfs_inobt_issparse(irec->ir_holemask))
> + realfree = irec->ir_free;
> + else
> + realfree = irec->ir_free & xfs_inobt_irec_to_allocmask(irec);

Nit:

I'd write this as:

uint64_t realfree = irec->ir_free;

if (xfs_inobt_issparse(irec->ir_holemask))
realfree &= xfs_inobt_irec_to_allocmask(irec);
return hweight64(realfree);

to simplify the logic a bit (and yes, I see the sniplet was just copied
out of an existing function).

> +/* Record extents that belong to inode btrees. */
> +STATIC int
> +xrep_ibt_walk_rmap(
> + struct xfs_btree_cur *cur,
> + const struct xfs_rmap_irec *rec,
> + void *priv)
> +{
> + struct xrep_ibt *ri = priv;
> + struct xfs_mount *mp = cur->bc_mp;
> + struct xfs_ino_geometry *igeo = M_IGEO(mp);
> + xfs_agblock_t cluster_base;
> + int error = 0;
> +
> + if (xchk_should_terminate(ri->sc, &error))
> + return error;
> +
> + if (rec->rm_owner == XFS_RMAP_OWN_INOBT)
> + return xrep_ibt_record_old_btree_blocks(ri, rec);
> +
> + /* Skip extents which are not owned by this inode and fork. */
> + if (rec->rm_owner != XFS_RMAP_OWN_INODES)
> + return 0;

The "Skip extents.." comment is clearly wrong and looks like it got
here by accident. And may ocaml-trained ind screams for a switch
statement and another helper for the rest of the functin body here:

switch (rec->rm_owner) {
case XFS_RMAP_OWN_INOBT:
return xrep_ibt_record_old_btree_blocks(ri, rec);
case XFS_RMAP_OWN_INODES:
return xrep_ibt_record_inode_blocks(mp, ri, rec);
default:
return 0;

> + /* If we have a record ready to go, add it to the array. */
> + if (ri->rie.ir_startino == NULLAGINO)
> + return 0;
> +
> + return xrep_ibt_stash(ri);
> +}

Superficial, but having the logic inverted from the comment makes
my brain a little dizzy. Anything again:

if (ri->rie.ir_startino != NULLAGINO)
error = xrep_ibt_stash(ri);

return error;

?

> +/* Make sure the records do not overlap in inumber address space. */
> +STATIC int
> +xrep_ibt_check_startino(

Would xrep_ibt_check_overlap be a better name here?