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

[Christoph Hellwig <hch@infradead.org>]

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?