Re: [PATCH 09/10] xfs: scrub/repair should update filesystem metadata health

[Brian Foster <bfoster@redhat.com>]

On Fri, Apr 05, 2019 at 01:54:47PM -0700, Darrick J. Wong wrote:
> On Fri, Apr 05, 2019 at 09:07:39AM -0400, Brian Foster wrote:
> > On Thu, Apr 04, 2019 at 11:01:33AM -0700, Darrick J. Wong wrote:
> > > On Thu, Apr 04, 2019 at 07:50:11AM -0400, Brian Foster wrote:
> > > > On Mon, Apr 01, 2019 at 10:11:12AM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > 
> > > > > Now that we have the ability to track sick metadata in-core, make scrub
> > > > > and repair update those health assessments after doing work.
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > >  fs/xfs/Makefile       |    1 
> > > > >  fs/xfs/scrub/health.c |  180 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  fs/xfs/scrub/health.h |   12 +++
> > > > >  fs/xfs/scrub/scrub.c  |    8 ++
> > > > >  fs/xfs/scrub/scrub.h  |   11 +++
> > > > >  5 files changed, 212 insertions(+)
> > > > >  create mode 100644 fs/xfs/scrub/health.c
> > > > >  create mode 100644 fs/xfs/scrub/health.h
> > > > > 
> > > > > 
> > > > ...
> > > > > diff --git a/fs/xfs/scrub/health.c b/fs/xfs/scrub/health.c
> > > > > new file mode 100644
> > > > > index 000000000000..dd9986500801
> > > > > --- /dev/null
> > > > > +++ b/fs/xfs/scrub/health.c
> > > > > @@ -0,0 +1,180 @@
> > > > ...
> > > > > +/* Update filesystem health assessments based on what we found and did. */
> > > > > +void
> > > > > +xchk_update_health(
> > > > > +	struct xfs_scrub	*sc,
> > > > > +	bool			already_fixed)
> > > > > +{
> > > > > +	/*
> > > > > +	 * If the scrubber finds errors, we mark sick whatever's mentioned in
> > > > > +	 * sick_mask, no matter whether this is a first scan or an evaluation
> > > > > +	 * of repair effectiveness.
> > > > > +	 *
> > > > > +	 * If there is no direct corruption and we're called after a repair,
> > > > > +	 * clear whatever's in heal_mask because that's what we fixed.
> > > > > +	 *
> > > > > +	 * Otherwise, there's no direct corruption and we didn't repair
> > > > > +	 * anything, so mark whatever's in sick_mask as healthy.
> > > > > +	 */
> > > > > +	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> > > > > +		xchk_mark_sick(sc, sc->sick_mask);
> > > > > +	else if (already_fixed)
> > > > > +		xchk_mark_healthy(sc, sc->heal_mask);
> > > > > +	else
> > > > > +		xchk_mark_healthy(sc, sc->sick_mask);
> > > > > +}
> > > > 
> > > > Hmm, I think I follow what we're doing here but it's a bit confusing
> > > > without the additional context of where these bits will be set/cleared
> > > > at the lower scrub layers (or at least without an example). Some
> > > > questions on that below...
> > > > 
> > > > ...
> > > > > diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> > > > > index 1b2344d00525..b1519dfc5811 100644
> > > > > --- a/fs/xfs/scrub/scrub.c
> > > > > +++ b/fs/xfs/scrub/scrub.c
> > > > > @@ -40,6 +40,7 @@
> > > > >  #include "scrub/trace.h"
> > > > >  #include "scrub/btree.h"
> > > > >  #include "scrub/repair.h"
> > > > > +#include "scrub/health.h"
> > > > >  
> > > > >  /*
> > > > >   * Online Scrub and Repair
> > > > > @@ -468,6 +469,7 @@ xfs_scrub_metadata(
> > > > >  {
> > > > >  	struct xfs_scrub		sc;
> > > > >  	struct xfs_mount		*mp = ip->i_mount;
> > > > > +	unsigned int			heal_mask;
> > > > >  	bool				try_harder = false;
> > > > >  	bool				already_fixed = false;
> > > > >  	int				error = 0;
> > > > > @@ -488,6 +490,7 @@ xfs_scrub_metadata(
> > > > >  	error = xchk_validate_inputs(mp, sm);
> > > > >  	if (error)
> > > > >  		goto out;
> > > > > +	heal_mask = xchk_health_mask_for_scrub_type(sm->sm_type);
> > > > >  
> > > > >  	xchk_experimental_warning(mp);
> > > > >  
> > > > > @@ -499,6 +502,8 @@ xfs_scrub_metadata(
> > > > >  	sc.ops = &meta_scrub_ops[sm->sm_type];
> > > > >  	sc.try_harder = try_harder;
> > > > >  	sc.sa.agno = NULLAGNUMBER;
> > > > > +	sc.heal_mask = heal_mask;
> > > > > +	sc.sick_mask = xchk_health_mask_for_scrub_type(sm->sm_type);
> > > > 
> > > > Ok, so we initialize the heal/sick masks based on the scrub type that
> > > > was requested on the first pass through...
> > > > 
> > > > >  	error = sc.ops->setup(&sc, ip);
> > > > >  	if (error)
> > > > >  		goto out_teardown;
> > > > > @@ -519,6 +524,8 @@ xfs_scrub_metadata(
> > > > >  	} else if (error)
> > > > >  		goto out_teardown;
> > > > >  
> > > > > +	xchk_update_health(&sc, already_fixed);
> > > > > +
> > > > 
> > > > ... then update the in-core fs health state based on the sick mask. Is
> > > > it possible for the scrub operation to set more sick mask bits based on
> > > > what it finds?
> > > 
> > > Theoretically, yes, but in practice none of the current scrubbers need
> > > to touch sick_mask.
> > > 
> > > heal_mask, OTOH, will be adjusted by the free space / inode repair
> > > functions since they rebuild multiple structures.
> > > 
> > 
> > Ok..
> > 
> > > > More specifically, I'm wondering why the masks wouldn't start as zero
> > > > and toggle based on finding/fixing corruption(s).
> > > 
> > > sick_mask is also the mask we feed to xfs_*_mark_healthy if the scan
> > > returns clean, which is why we set the default value before dispatching
> > > the scrub.
> > > 
> > > > Or if the sick mask value is essentially fixed, whether we need to
> > > > store it in the xfs_scrub context...
> > > 
> > > We could probably get away with generating it in xchk_update_health at
> > > the end, but it feels weird to have heal_mask in the scrub context but
> > > sick_mask gets auto-generated.
> > > 
> > 
> > Ok.. hmm. Both feel a little weird to me, but this is really just an
> > aesthetic/factoring thing so I'll think about it a bit more and come
> > back to it.
> > 
> > > > 
> > > > >  	if ((sc.sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR) && !already_fixed) {
> > > > >  		bool needs_fix;
> > > > >  
> > > > > @@ -551,6 +558,7 @@ xfs_scrub_metadata(
> > > > >  				xrep_failure(mp);
> > > > >  				goto out;
> > > > >  			}
> > > > > +			heal_mask = sc.heal_mask;
> > > > 
> > > > And if we end up doing a repair, we presumably can repair multiple
> > > > things and so we track that separately and persist the heal mask across
> > > > a potential retry.
> > > 
> > > Right.
> > > 
> > > > What about the case where we don't retry, but scrub finds something
> > > > and then immediately repairs it?
> > > 
> > > The repair jumps back to retry_op if either (a) we couldn't get all the
> > > resources we needed and therefore sc.try_harder = true and we need to
> > > start over; or (b) repair thinks it fixed a thing, so we need to scrub
> > > the thing again to see if it's really fixed...
> > > 
> > > > Should we update the fs state after both detecting and clearing the
> > > > problem, or does that happen elsewhere?
> > > 
> > > ...so if scrub immediately repairs a thing, we preserve heal_mask, jump
> > > back to the scrub, and if the scrub says clean we'll mark heal mask
> > > healthy.
> > > 
> > > If the repair has to retry then the we'll call the repair function
> > > again, which (presumably) will set (again) the heal_mask appropriately,
> > > and then we have the same post-repair state updating as above.
> > > 
> > > Does that make sense? :)
> > > 
> > 
> > Ah, Ok. I didn't realize that a successful repair looped back to the
> > scrub code (and thus the health update). Yes, that makes more sense.
> > 
> > > > Also, if repair can potentially clear multiple bits, what's the
> > > > possibility of a repair clearing one failure and then failing on
> > > > another, causing the broader repair op to return an error or jump into
> > > > this retry?
> > > 
> > > Scrub doesn't touch the fs health state at all until after the ->scrub
> > > or ->repair function succeeds.  If the scrub or the repair functions
> > > fail for any non-retry reason, we back out to userspace without updating
> > > anything.  It's as if we'd never called the failed function.
> > > 
> > 
> > Right.. what I was getting at above is seeing whether we'd actually
> > update partial repair state in-core. E.g., suppose things A and B are
> > faulted in-core and it's one of these cases where repair can fix A and B
> > at the same time. If it fixes thing A and fails on thing B, it sounds
> > like we'd not clear the in-core fault state on A even though it's
> > technically repaired.
> 
> Hmm.  If the repair function returns a runtime error (having fixed A but
> not B) then yes, we won't clear the incore fault state on A (or B) even
> though we fixed A.  Something weird happened, so we shouldn't be too
> hasty to clear things.  A subsequent re-scrub of A will clear the fault
> on A, though.
> 

Ok. Indeed, it doesn't seem that unreasonable to me for an operational
error to fail to clear health state for something that was repaired.

> OTOH... if the A/B repair function returns 0 having fixed A but left B
> corrupt, the rescan will see that A is fine and (incorrectly) clear both
> A and B.  I would say that's a bug, so maybe I should rethink the need
> for sick_mask and heal_mask.
> 

That one sounds more dodgy. ;P

> That said, a normal xfs_scrub run will check (or have already checked) B
> and noticed that it was corrupt, so it will circle back and try to fix B
> separately, so in a sense we don't really need heal_mask at all.
> 

Ok..

> > > Maybe some worked examples will help?
> > > 
> > > Let's say both inode btrees are corrupt.  We run xfs_scrub -n,
> > > xchk_inobt will record the corruption, and (assuming it hits no runtime
> > > errors) once we return to xfs_scrub_metadata, it'll set
> > > XFS_SICK_AG_INOBT.  Presumably xfs_scrub will also call the finobt scrub
> > > and SICK_AG_FINOBT will also get set.
> > > 
> > > If we run xfs_scrub without the -n, xchk_inobt will record the
> > > corruption and set SICK_AG_INOBT per above.  Then it'll run xrep_inobt,
> > > which will set heal_mask to SICK_AG_INOBT | SICK_AG_FINOBT.  If the
> > > repair fails with a non-retry runtime error, we exit to userspace and
> > > ignore heal_mask.
> > > 
> > 
> > Ok, this sounds like the case I'm theorizing about above (where suppose
> > repair fixed the inobt and then failed on the finobt, but hasn't cleared
> > faults for either..).
> > 
> > > If instead the repair succeeds, we scan the inobt again.  If that comes
> > > up clear then we use heal_mask to clear SICK_AG_INOBT | SICK_AG_FINOBT.
> > > xfs_scrub will call again later to repair the finobt, but the initial
> > > finobt scan will see no errors in the finobt, clear SICK_AG_FINOBT
> > > (which isn't set) and exit.
> > > 
> > 
> > So it sounds like the state would have to be cleared by a subsequent
> > scrub request. The scan would find thing A healthy and mark it so
> > regardless, to clear any potential previous faults that might have
> > already been repaired. Right?
> 
> Right.
> 
> > > If the inobt repair function is buggy and says it repaired the inode
> > > btrees but leaves corruptions, then the rescan of the inobt will notice
> > > and set SICK_AG_INOBT (which is already set) and exit.  Similarly, when
> > > xfs_scrub calls back about the finobt, it will notice the corrupt
> > > finobt, try to set SICK_AG_FINOBT (also already set), try to fix it, and
> > > the rescan of the finobt will notice that the finobt is still corrupt
> > > and try to set SICK_AG_FINOBT (which is still set).
> > > 
> > > The end result (I think) is that we always set the sick bits if a scan
> > > shows problems, and we only clear the sick bits for things if we can
> > > prove that the things are no longer sick.  Does that help?
> > > 
> > 
> > Yes, thanks for the explanation. I think the confusion is mostly due to
> > not being able to fully see how these scrub states are managed,
> > particularly the bits that warranted the creation of separate masks in
> > the first place.
> 
> You've convinced me that this patch is too convoluted to understand, so
> I think I want to simplify it some more.  First, I'd rename the field
> to "sick_mask_update" and change the behavior so that we:
> 
>  1. Set sick_mask_update to the default XFS_SICK flag for this scrub
>     type (call it A).  (We already do this)
> 
>  2. If the scrubber returns an error code, we exit making no changes to
>     the incore sick state.
> 
>  3. If the scrubber finds that A is clean, clear the incore sick flags
>     that are set in s_m_u and exit.
> 
>  4. If the scrubber finds that A is corrupt, set the incore sick flags
>     that are set in s_m_u.
> 
>     a. If the user doesn't want to repair, then we exit, having
>        previously set incore sick flags.
> 
>  5. Now we know that A is corrupt and the user wants to repair.
>     If repair returns an error code, we exit with that error code, having
>     made no further changes to the incore sick state.
> 
>  6. If repair rebuilds both A & B correctly and the re-scrub of A is
>     clean, we'll clear the incore sick flags using s_m_u.  This should
>     clear A.
> 
>  7. If repair rebuilds both A & B and screws up A, the re-scrub will find
>     it corrupt and leave the sick flags as they are, which is to say that
>     A is marked sick.
> 
>  8. If repair rebuilds A correctly but leaves B corrupt, the re-scrub of
>     A will be clean and we'll clear the incore sick flags using s_m_u.
>     This should clear A, even though B is corrupt.
> 
>  9. No matter whether we encountered scenarios 6, 7, or 8, if xfs_scrub
>     previously scrubbed B and found it corrupt, it will call again to
>     repair B, which will set the incore sick state appropriately.  If
>     xfs_scrub has not yet scrubbed B then it will call later to scrub B,
>     which will set the incore sick state appropriately.
> 
> I hope that's easier to understand...
> 

It sounds like the primary difference here is trading off the ability to
clear both A and B flags at the same time during a scrub+repair of A,
and rather rely on the separate scrub of B to detect that B is no longer
corrupt.

That sounds much more straightforward to me provided it works well
enough with the userspace tool (i.e., xfs_scrub will eventually mark B
healthy before it returns either way). It simplifies the tracking and if
we consider the normal sequence for a corrupted thing should be scrub(A)
-> setcorrupt(A) -> repair(A) -> scrub(A) -> sethealthy(A), then
clearing in-core sick state of B at the end kind of violates the model
where we'd expect another scrub(B) to take place first.

Brian

> > This does still have me wondering if separate masks are necessary, if we
> > perhaps had more selective health update logic, for example. I think it
> > might be better to either bundle this patch with whatever other changes
> > actually make use of the separate masks, or alternatively to simplify
> > the current logic and just defer the separate mask thing until those
> > more complex repair algorithms come along..
> 
> --D
> 
> > Brian
> > 
> > > > ISTM that it might be possible to skip clearing one fail state bit so
> > > > long as the original thing remained corrupted, but I feel like I'm
> > > > still missing some context on the bigger picture scrub tracking...
> > > 
> > > Yeah, the state machine is pretty squirrely. :/
> > > 
> > > --D
> > > 
> > > > Brian
> > > > 
> > > > >  			goto retry_op;
> > > > >  		}
> > > > >  	}
> > > > > diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
> > > > > index 22f754fba8e5..05f1ad242a35 100644
> > > > > --- a/fs/xfs/scrub/scrub.h
> > > > > +++ b/fs/xfs/scrub/scrub.h
> > > > > @@ -62,6 +62,17 @@ struct xfs_scrub {
> > > > >  	struct xfs_inode		*ip;
> > > > >  	void				*buf;
> > > > >  	uint				ilock_flags;
> > > > > +
> > > > > +	/* Metadata to be marked sick if scrub finds errors. */
> > > > > +	unsigned int			sick_mask;
> > > > > +
> > > > > +	/*
> > > > > +	 * Metadata to be marked healthy if repair fixes errors.  Some repair
> > > > > +	 * functions can fix multiple data structures at once, so we have to
> > > > > +	 * treat sick and heal masks separately.
> > > > > +	 */
> > > > > +	unsigned int			heal_mask;
> > > > > +
> > > > >  	bool				try_harder;
> > > > >  	bool				has_quotaofflock;
> > > > >  
> > > > >