[PATCH 0/2] xfs: health evaluations for repair

[PATCH 0/2] xfs: health evaluations for repair

[Darrick J. Wong]

Hi all,

Lay the ground work for tying the online repair functions into the new
health reporting functions.  First, repair functions need to be able to
proivde a revalidation function that will check everything that the
repair function might have rebuilt.  This will be necessary for the free
space and inode btree repair functions, which rebuild both btrees at
once.

Second, we must be careful that the health reporting query code doesn't
get in the way of post-repair revalidation of all rebuilt metadata
structures.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=repair-health

[PATCH 1/2] xfs: add a repair revalidation function pointer

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Allow repair functions to set a separate function pointer to validate
the metadata that they've rebuilt.  This prevents us from exiting from a
repair function that rebuilds both A and B without checking that both A
and B can pass a scrub test.  We'll need this for the free space and
inode btree repair strategies.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/scrub.c |    5 ++++-
 fs/xfs/scrub/scrub.h |    8 ++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 15c8c5f3f688..0f0b64d7164b 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -495,7 +495,10 @@ xfs_scrub_metadata(
 		goto out_teardown;
 
 	/* Scrub for errors. */
-	error = sc.ops->scrub(&sc);
+	if ((sc.flags & XREP_ALREADY_FIXED) && sc.ops->repair_eval != NULL)
+		error = sc.ops->repair_eval(&sc);
+	else
+		error = sc.ops->scrub(&sc);
 	if (!(sc.flags & XCHK_TRY_HARDER) && error == -EDEADLOCK) {
 		/*
 		 * Scrubbers return -EDEADLOCK to mean 'try harder'.
diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
index ad1ceb44a628..94a30637a127 100644
--- a/fs/xfs/scrub/scrub.h
+++ b/fs/xfs/scrub/scrub.h
@@ -27,6 +27,14 @@ struct xchk_meta_ops {
 	/* Repair or optimize the metadata. */
 	int		(*repair)(struct xfs_scrub *);
 
+	/*
+	 * Re-scrub the metadata we repaired, in case there's extra work that
+	 * we need to do to check our repair work.  If this is NULL, we'll use
+	 * the ->scrub function pointer, assuming that the regular scrub is
+	 * sufficient.
+	 */
+	int		(*repair_eval)(struct xfs_scrub *sc);
+
 	/* Decide if we even have this piece of metadata. */
 	bool		(*has)(struct xfs_sb *);
 

Re: [PATCH 1/2] xfs: add a repair revalidation function pointer

[Brian Foster]

On Wed, Oct 09, 2019 at 09:49:39AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Allow repair functions to set a separate function pointer to validate
> the metadata that they've rebuilt.  This prevents us from exiting from a
> repair function that rebuilds both A and B without checking that both A
> and B can pass a scrub test.  We'll need this for the free space and
> inode btree repair strategies.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Seems reasonable, but perhaps should be placed along with the first
patch that uses the callback..

Brian

>  fs/xfs/scrub/scrub.c |    5 ++++-
>  fs/xfs/scrub/scrub.h |    8 ++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index 15c8c5f3f688..0f0b64d7164b 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -495,7 +495,10 @@ xfs_scrub_metadata(
>  		goto out_teardown;
>  
>  	/* Scrub for errors. */
> -	error = sc.ops->scrub(&sc);
> +	if ((sc.flags & XREP_ALREADY_FIXED) && sc.ops->repair_eval != NULL)
> +		error = sc.ops->repair_eval(&sc);
> +	else
> +		error = sc.ops->scrub(&sc);
>  	if (!(sc.flags & XCHK_TRY_HARDER) && error == -EDEADLOCK) {
>  		/*
>  		 * Scrubbers return -EDEADLOCK to mean 'try harder'.
> diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
> index ad1ceb44a628..94a30637a127 100644
> --- a/fs/xfs/scrub/scrub.h
> +++ b/fs/xfs/scrub/scrub.h
> @@ -27,6 +27,14 @@ struct xchk_meta_ops {
>  	/* Repair or optimize the metadata. */
>  	int		(*repair)(struct xfs_scrub *);
>  
> +	/*
> +	 * Re-scrub the metadata we repaired, in case there's extra work that
> +	 * we need to do to check our repair work.  If this is NULL, we'll use
> +	 * the ->scrub function pointer, assuming that the regular scrub is
> +	 * sufficient.
> +	 */
> +	int		(*repair_eval)(struct xfs_scrub *sc);
> +
>  	/* Decide if we even have this piece of metadata. */
>  	bool		(*has)(struct xfs_sb *);
>  
> 

[PATCH 2/2] xfs: always rescan allegedly healthy per-ag metadata after repair

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

After an online repair function runs for a per-AG metadata structure,
sc->sick_mask is supposed to reflect the per-AG metadata that the repair
function fixed.  Our next move is to re-check the metadata to assess
the completeness of our repair, so we don't want the rebuilt structure
to be excluded from the rescan just because the health system previously
logged a problem with the data structure.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/health.c |   10 ++++++++++
 1 file changed, 10 insertions(+)


diff --git a/fs/xfs/scrub/health.c b/fs/xfs/scrub/health.c
index b2f602811e9d..4865b2180e22 100644
--- a/fs/xfs/scrub/health.c
+++ b/fs/xfs/scrub/health.c
@@ -220,6 +220,16 @@ xchk_ag_btree_healthy_enough(
 		return true;
 	}
 
+	/*
+	 * If we just repaired some AG metadata, sc->sick_mask will reflect all
+	 * the per-AG metadata types that were repaired.  Exclude these from
+	 * the filesystem health query because we have not yet updated the
+	 * health status and we want everything to be scanned.
+	 */
+	if ((sc->flags & XREP_ALREADY_FIXED) &&
+	    type_to_health_flag[sc->sm->sm_type].group == XHG_AG)
+		mask &= ~sc->sick_mask;
+
 	if (xfs_ag_has_sickness(pag, mask)) {
 		sc->sm->sm_flags |= XFS_SCRUB_OFLAG_XFAIL;
 		return false;

Re: [PATCH 2/2] xfs: always rescan allegedly healthy per-ag metadata after repair

[Brian Foster]

On Wed, Oct 09, 2019 at 09:49:45AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> After an online repair function runs for a per-AG metadata structure,
> sc->sick_mask is supposed to reflect the per-AG metadata that the repair
> function fixed.  Our next move is to re-check the metadata to assess
> the completeness of our repair, so we don't want the rebuilt structure
> to be excluded from the rescan just because the health system previously
> logged a problem with the data structure.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/scrub/health.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/health.c b/fs/xfs/scrub/health.c
> index b2f602811e9d..4865b2180e22 100644
> --- a/fs/xfs/scrub/health.c
> +++ b/fs/xfs/scrub/health.c
> @@ -220,6 +220,16 @@ xchk_ag_btree_healthy_enough(
>  		return true;
>  	}
>  
> +	/*
> +	 * If we just repaired some AG metadata, sc->sick_mask will reflect all
> +	 * the per-AG metadata types that were repaired.  Exclude these from
> +	 * the filesystem health query because we have not yet updated the
> +	 * health status and we want everything to be scanned.
> +	 */
> +	if ((sc->flags & XREP_ALREADY_FIXED) &&
> +	    type_to_health_flag[sc->sm->sm_type].group == XHG_AG)
> +		mask &= ~sc->sick_mask;
> +
>  	if (xfs_ag_has_sickness(pag, mask)) {
>  		sc->sm->sm_flags |= XFS_SCRUB_OFLAG_XFAIL;
>  		return false;
>