Re: [PATCH 2/4] repair: don't double check dir2 sf parent in phase 4

[Brian Foster <bfoster@redhat.com>]

On Wed, Jul 15, 2020 at 04:54:14PM -0700, Darrick J. Wong wrote:
> On Wed, Jul 15, 2020 at 07:43:50PM +0100, Christoph Hellwig wrote:
> > On Wed, Jul 15, 2020 at 10:08:34AM -0400, Brian Foster wrote:
> > > The shortform parent ino verification code runs once in phase 3
> > > (ino_discovery == true) and once in phase 4 (ino_discovery ==
> > > false). This is unnecessary and leads to duplicate error messages if
> > > repair replaces an invalid parent value with zero because zero is
> > > still an invalid value. Skip the check in phase 4.
> > 
> > This looks good,
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> > As far as the existing code is concerned:  Does anyone else find the
> > ino_discovery booleand passed as int as annoying as I do?  An
> > "enum repair_phase phase" would be much more descriptive in my opinion.
> 
> I can never remember what "ino_discovery" actually means.  true means
> phase2 (looking at inodes for the first time) and false means phase4
> (looking for crosslinked data and whatnot)?
> 

Same.. I agree with Christoph on not just ino_discovery, but the various
boolean parameters to some of the common scanning functions that are
reused across multiple phases. It's confusing to track when reading the
code as well as to identify intent and whether certain hunks of
idempotent code are running multiple times, etc.

That said, I'm not necessarily convinced that replacing the booleans
with a phase enum is a huge benefit. That just changes the interface so
it's easier to determine what phase we're in vs. why certain bits of
logic are executed. I.e., 'if (ino_discovery) { do_discovery_stuff(); }'
confuses what phase we're in when reading the lower level code, but 'if
(phase == PHASE_3) { do_discovery_stuff() }' doesn't really clarify how
do_discovery_stuff() might interact with other behaviors that are part
of the same phase.

I'm not sure what the right answer is but if we're going to look at
refactoring things, I'd rather see us start with considering something
more fundamental. For example, could we perhaps factor out the the phase
specific functionality into phase specific functions via an approach
like Darrick's recent log recovery rework?  That way it might be easier
to read through each phase and understand the core repair logic vs.
having to troll through scanning infrastructure multiple times trying to
keep track of where you are in the grand scheme of things. Of course if
that is too much of a mess then perhaps the phase enum thing makes more
sense..

Brian

> --D
>