Re: [PATCH 2/2] xfs_scrub: check summary counters

[Dave Chinner <david@fromorbit.com>]

On Mon, Aug 26, 2019 at 02:21:09PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach scrub to ask the kernel to check and repair summary counters
> during phase 7.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  scrub/phase4.c |   12 ++++++++++++
>  scrub/phase7.c |   14 ++++++++++++++
>  scrub/repair.c |    3 +++
>  scrub/scrub.c  |   13 +++++++++++++
>  scrub/scrub.h  |    2 ++
>  5 files changed, 44 insertions(+)
> 
> 
> diff --git a/scrub/phase4.c b/scrub/phase4.c
> index 49f00723..c4da4852 100644
> --- a/scrub/phase4.c
> +++ b/scrub/phase4.c
> @@ -107,6 +107,18 @@ bool
>  xfs_repair_fs(
>  	struct scrub_ctx		*ctx)
>  {
> +	bool				moveon;
> +
> +	/*
> +	 * Check the summary counters early.  Normally we do this during phase
> +	 * seven, but some of the cross-referencing requires fairly-accurate
> +	 * counters, so counter repairs have to be put on the list now so that
> +	 * they get fixed before we stop retrying unfixed metadata repairs.
> +	 */
> +	moveon = xfs_scrub_fs_summary(ctx, &ctx->action_lists[0]);
> +	if (!moveon)
> +		return false;

"moveon" doesn't really make sense to me here. i.e. I can't tell if
"moveon = true" meant it failed or not, so I hav eno idea what the
intent of the code here is, and the comment doesn't explain it at
all, either.

> +
>  	return xfs_process_action_items(ctx);
>  }
>  
> diff --git a/scrub/phase7.c b/scrub/phase7.c
> index 1c459dfc..b3156fdf 100644
> --- a/scrub/phase7.c
> +++ b/scrub/phase7.c
> @@ -7,12 +7,15 @@
>  #include <stdint.h>
>  #include <stdlib.h>
>  #include <sys/statvfs.h>
> +#include "list.h"
>  #include "path.h"
>  #include "ptvar.h"
>  #include "xfs_scrub.h"
>  #include "common.h"
> +#include "scrub.h"
>  #include "fscounters.h"
>  #include "spacemap.h"
> +#include "repair.h"
>  
>  /* Phase 7: Check summary counters. */
>  
> @@ -91,6 +94,7 @@ xfs_scan_summary(
>  	struct scrub_ctx	*ctx)
>  {
>  	struct summary_counts	totalcount = {0};
> +	struct xfs_action_list	alist;
>  	struct ptvar		*ptvar;
>  	unsigned long long	used_data;
>  	unsigned long long	used_rt;
> @@ -110,6 +114,16 @@ xfs_scan_summary(
>  	int			ip;
>  	int			error;
>  
> +	/* Check and fix the fs summary counters. */
> +	xfs_action_list_init(&alist);
> +	moveon = xfs_scrub_fs_summary(ctx, &alist);
> +	if (!moveon)
> +		return false;
> +	moveon = xfs_action_list_process(ctx, ctx->mnt.fd, &alist,
> +			ALP_COMPLAIN_IF_UNFIXED | ALP_NOPROGRESS);
> +	if (!moveon)
> +		return moveon;

same here - "moveon" doesn't tell me if we're returning because the
scrub failed or passed....

> +
>  	/* Flush everything out to disk before we start counting. */
>  	error = syncfs(ctx->mnt.fd);
>  	if (error) {
> diff --git a/scrub/repair.c b/scrub/repair.c
> index 45450d8c..54639752 100644
> --- a/scrub/repair.c
> +++ b/scrub/repair.c
> @@ -84,6 +84,9 @@ xfs_action_item_priority(
>  	case XFS_SCRUB_TYPE_GQUOTA:
>  	case XFS_SCRUB_TYPE_PQUOTA:
>  		return PRIO(aitem, XFS_SCRUB_TYPE_UQUOTA);
> +	case XFS_SCRUB_TYPE_FSCOUNTERS:
> +		/* This should always go after AG headers no matter what. */
> +		return PRIO(aitem, INT_MAX);
>  	}
>  	abort();
>  }
> diff --git a/scrub/scrub.c b/scrub/scrub.c
> index 136ed529..a428b524 100644
> --- a/scrub/scrub.c
> +++ b/scrub/scrub.c
> @@ -28,6 +28,7 @@ enum scrub_type {
>  	ST_PERAG,	/* per-AG metadata */
>  	ST_FS,		/* per-FS metadata */
>  	ST_INODE,	/* per-inode metadata */
> +	ST_SUMMARY,	/* summary counters (phase 7) */
>  };

Hmmm - the previous patch used ST_FS for the summary counters.

Oh, wait, io/scrub.c has a duplicate scrub_type enum defined, and
the table looks largely the same, too. Except now the summary type
is different.

/me looks a bit closer...

Oh, the enum scrub_type definitions shadow the kernel enum
xchk_type, but have different values for the same names. I'm
just confused now...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com