From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2130.oracle.com ([156.151.31.86]:50352 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752332AbeF2POc (ORCPT ); Fri, 29 Jun 2018 11:14:32 -0400 Date: Fri, 29 Jun 2018 08:14:27 -0700 From: "Darrick J. Wong" Subject: Re: [PATCH 04/21] xfs: repair the AGF and AGFL Message-ID: <20180629151427.GO5711@magnolia> References: <152986820984.3155.16417868536016544528.stgit@magnolia> <152986823481.3155.13034509035761369722.stgit@magnolia> <20180627021908.GD19934@dastard> <20180627233720.GA2234@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180627233720.GA2234@dastard> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Dave Chinner Cc: Allison Henderson , linux-xfs@vger.kernel.org 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 > > >> > > >>Regenerate the AGF and AGFL from the rmap data. > > >> > > >>Signed-off-by: Darrick J. Wong > > > > > >[...] > > > >>+ /* 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