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

From: "Darrick J. Wong" <djwong@kernel.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: Dave Chinner <dchinner@redhat.com>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 4/5] xfs: repair inode btrees
Date: Tue, 28 Nov 2023 13:37:23 -0800	[thread overview]
Message-ID: <20231128213723.GC4167244@frogsfrogsfrogs> (raw)
In-Reply-To: <ZWYN4MvhQDhFWqHO@infradead.org>

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

> 
> 