On Tue, Nov 28, 2023 at 07:57:20AM -0800, Christoph Hellwig wrote:
> 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:
I already fixed the nitpicks from yesterday. :)
> > + 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).
Ok.
> > +/* 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.
Yep. "Skip mappings that are not inode records.", sorry about that.
> 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;
Sounds good to me.
> > + /* 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;
>
> ?
Done.
> > +/* 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?
Yes! Thank you for the suggestion.
--D
>
>
