Re: [PATCH 04/21] xfs: repair the AGF and AGFL

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Jun 28, 2018 at 09:37:20AM +1000, Dave Chinner wrote:
> On Wed, Jun 27, 2018 at 09:44:53AM -0700, Allison Henderson wrote:
> > On 06/26/2018 07:19 PM, Dave Chinner wrote:
> > >On Sun, Jun 24, 2018 at 12:23:54PM -0700, Darrick J. Wong wrote:
> > >>From: Darrick J. Wong <darrick.wong@oracle.com>
> > >>
> > >>Regenerate the AGF and AGFL from the rmap data.
> > >>
> > >>Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > >[...]
> 
> > >>+	/* Record all the OWN_AG blocks. */
> > >>+	if (rec->rm_owner == XFS_RMAP_OWN_AG) {
> > >>+		fsb = XFS_AGB_TO_FSB(cur->bc_mp, cur->bc_private.a.agno,
> > >>+				rec->rm_startblock);
> > >>+		error = xfs_repair_collect_btree_extent(ra->sc,
> > >>+				ra->freesp_list, fsb, rec->rm_blockcount);
> > >>+		if (error)
> > >>+			return error;
> > >>+	}
> > >>+
> > >>+	return xfs_repair_collect_btree_cur_blocks(ra->sc, cur,
> > >>+			xfs_repair_collect_btree_cur_blocks_in_extent_list,
> > >
> > >Urk. The function name lengths is getting out of hand. I'm very
> > >tempted to suggest we should shorten the namespace of all this
> > >like s/xfs_repair_/xr_/ and s/xfs_scrub_/xs_/, etc just to make them
> > >shorter and easier to read.
> > >
> > >Oh, wait, did I say that out loud? :P
> > >
> > >Something to think about, anyway.
> > >
> > Well they are sort of long, but TBH I think i still kind of
> > appreciate the extra verbiage.  I have seen other projects do things
> > like adopt a sort of 3 or 4 letter abbreviation (like maybe xfs_scrb
> > or xfs_repr). Helps to cut down on the verbosity while still not
> > loosing too much of what it is supposed to mean.  Just another idea
> > to consider. :-)
> 
> We've got that in places, too, like "xlog_" prefixes for all the log
> code, so that's not an unreasonable thing to suggest. After all, in
> many cases we're talking about a tradeoff between readabilty and the
> amount of typing necessary.

I propose(d on IRC) to shorten the prefixes to xrep_ and xchk_.

I'll also take a look at condensing the non-prefix parts of the names.
Agreed that they're too long now.

> However, IMO, function names so long they need a line of their own
> indicates we have a structural problem in our code, not a
> readability problem. We should not need names that long to document
> what the function does - it should be obvious from the context, the
> abstraction that is being used and a short name....
> 
> e.g. how many of these different "collect extent" operations could
> be abstracted into a common extent list structure and generic
> callbacks? It seems there's a lot of similarity in them, and we're
> really only differentiating them by adding more namespace and
> context specific information into the structure and function names.

I'd /really/ like to convert this into a proper incore bitmap and then
turn the operations into proper bitmasking operations, since
collect_btree_extents is merely setting ranges of bits in two bitmaps
and subtract_extents takes the union of them both.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html