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?
