Re: [PATCH 2/7] xfs_scrub: remove preen mode

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Feb 12, 2018 at 02:49:01PM -0600, Eric Sandeen wrote:
> 
> 
> On 2/5/18 5:22 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > While it's true that the kernel can tell us whether something needs
> > repairs or it needs optimizing, from the admin's perspective there's
> > no point in having an optimize-only mode -- either fix everything, or
> > don't.  This is what xfs_repair does w.r.t. -n, so let's do the same
> > thing too.
> 
> <ok and a few other changes but sure> :)
> 
> nitpick, does -n skip the "read all the data blocks" pass?  I'm not sure
> the manpage really addresses the data block scrubbing yet.

-n will not skip the data block scrub, it only prevents metadata repairs.

--D

> Anyway, for the changes here:
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  man/man8/xfs_scrub.8 |   51 +++++++++++++++++++++++---------------------------
> >  scrub/phase1.c       |    6 +-----
> >  scrub/phase4.c       |   19 -------------------
> >  scrub/scrub.c        |    2 +-
> >  scrub/xfs_scrub.c    |   33 +++++++-------------------------
> >  scrub/xfs_scrub.h    |    3 ---
> >  6 files changed, 32 insertions(+), 82 deletions(-)
> > 
> > 
> > diff --git a/man/man8/xfs_scrub.8 b/man/man8/xfs_scrub.8
> > index 4c394a5..77fed92 100644
> > --- a/man/man8/xfs_scrub.8
> > +++ b/man/man8/xfs_scrub.8
> > @@ -1,10 +1,10 @@
> >  .TH xfs_scrub 8
> >  .SH NAME
> > -xfs_scrub \- scrub the contents of an XFS filesystem
> > +xfs_scrub \- check the contents of a mounted XFS filesystem
> >  .SH SYNOPSIS
> >  .B xfs_scrub
> >  [
> > -.B \-abCemnTvxy
> > +.B \-abCemnTvx
> >  ]
> >  .RI "[" mount-point " | " block-device "]"
> >  .br
> > @@ -13,6 +13,12 @@ xfs_scrub \- scrub the contents of an XFS filesystem
> >  .B xfs_scrub
> >  attempts to check and repair all metadata in a mounted XFS filesystem.
> >  .PP
> > +.B WARNING!
> > +This program is
> > +.BR EXPERIMENTAL ","
> > +which means that its behavior and interface
> > +could change at any time!
> > +.PP
> >  .B xfs_scrub
> >  asks the kernel to scrub all metadata objects in the filesystem.
> >  Metadata records are scanned for obviously bad values and then
> > @@ -28,19 +34,17 @@ the standard error stream.
> >  Enabling verbose mode will increase the amount of status information
> >  sent to the output.
> >  .PP
> > -This utility does not know how to correct all errors.
> > -If the tool cannot fix the detected errors, you must unmount the
> > -filesystem and run
> > +If the kernel scrub reports that metadata needs repairs or optimizations and
> > +the user does not pass
> > +.B -n
> > +on the command line, this program will ask the kernel to make the repairs and
> > +to perform the optimizations.
> > +See the sections about optimizations and repairs for a list of optimizations
> > +and repairs known to this program.
> > +The kernel may not support repairing or optimizing the filesystem.
> > +If this is the case, the filesystem must be unmounted and
> >  .BR xfs_repair (8)
> > -to fix the problems.
> > -If this tool is not run with either of the
> > -.B \-n
> > -or
> > -.B \-y
> > -options, then it will optimize the filesystem when possible,
> > -but it will not try to fix errors.
> > -See the optimizations section below for a list of optimizations
> > -supported by this program.
> > +run on the filesystem to fix the problems.
> >  .SH OPTIONS
> >  .TP
> >  .BI \-a " errors"
> > @@ -73,14 +77,14 @@ is given, no action is taken if errors are found; this is the default
> >  behavior.
> >  .TP
> >  .B \-k
> > -Do not call FITRIM on the free space.
> > +Do not call TRIM on the free space.
> >  .TP
> >  .BI \-m " file"
> >  Search this file for mounted filesystems instead of /etc/mtab.
> >  .TP
> >  .B \-n
> > -Dry run, do not modify anything in the filesystem.
> > -This disables all optimization and repair behaviors.
> > +Only check filesystem metadata.
> > +Do not repair or optimize anything.
> >  .TP
> >  .BI \-T
> >  Print timing and memory usage information for each phase.
> > @@ -98,20 +102,11 @@ will issue O_DIRECT reads to the block device directly.
> >  If the block device is a SCSI disk, it will instead issue READ VERIFY commands
> >  directly to the disk.
> >  These actions will confirm that all file data blocks can be read from storage.
> > -.TP
> > -.B \-y
> > -Try to repair all filesystem errors.
> > -If the errors cannot be fixed online, then the filesystem must be taken
> > -offline for repair.
> >  .SH OPTIMIZATIONS
> > -Optimizations supported by this program include:
> > +Optimizations supported by this program include, but are not limited to:
> >  .IP \[bu] 2
> > -Updating secondary superblocks to match the primary superblock.
> > -.IP \[bu]
> > -Turning off shared block write checks for files that no longer share blocks.
> > -.IP \[bu]
> >  Instructing the underlying storage to discard unused extents via the
> > -.B FITRIM
> > +.B TRIM
> >  ioctl.
> >  .SH REPAIRS
> >  This program currently does not support making any repairs.
> > diff --git a/scrub/phase1.c b/scrub/phase1.c
> > index 75da296..af93d0f 100644
> > --- a/scrub/phase1.c
> > +++ b/scrub/phase1.c
> > @@ -181,11 +181,7 @@ _("Kernel metadata scrubbing facility is not available."));
> >  
> >  	/* Do we need kernel-assisted metadata repair? */
> >  	if (ctx->mode != SCRUB_MODE_DRY_RUN && !xfs_can_repair(ctx)) {
> > -		if (ctx->mode == SCRUB_MODE_PREEN)
> > -			str_error(ctx, ctx->mntpoint,
> > -_("Kernel metadata optimization facility is not available.  Use -n to scrub."));
> > -		else
> > -			str_error(ctx, ctx->mntpoint,
> > +		str_error(ctx, ctx->mntpoint,
> >  _("Kernel metadata repair facility is not available.  Use -n to scrub."));
> >  		return false;
> >  	}
> > diff --git a/scrub/phase4.c b/scrub/phase4.c
> > index 3100d75..1fb8da9 100644
> > --- a/scrub/phase4.c
> > +++ b/scrub/phase4.c
> > @@ -62,25 +62,6 @@ xfs_repair_fs(
> >  	return xfs_process_action_items(ctx);
> >  }
> >  
> > -/* Run the optimize-only phase if there are no errors. */
> > -bool
> > -xfs_optimize_fs(
> > -	struct scrub_ctx	*ctx)
> > -{
> > -	/*
> > -	 * In preen mode, corruptions are immediately recorded as errors,
> > -	 * so if there are any corruptions on the filesystem errors_found
> > -	 * will be non-zero and we won't do anything.
> > -	 */
> > -	if (ctx->errors_found) {
> > -		str_info(ctx, ctx->mntpoint,
> > -_("Errors found, please re-run with -y."));
> > -		return true;
> > -	}
> > -
> > -	return xfs_process_action_items(ctx);
> > -}
> > -
> >  /* Estimate how much work we're going to do. */
> >  bool
> >  xfs_estimate_repair_work(
> > diff --git a/scrub/scrub.c b/scrub/scrub.c
> > index 6abca2a..bc0e2f0 100644
> > --- a/scrub/scrub.c
> > +++ b/scrub/scrub.c
> > @@ -279,7 +279,7 @@ _("Repairs are required."));
> >  	 * otherwise complain.
> >  	 */
> >  	if (is_unoptimized(meta)) {
> > -		if (ctx->mode < SCRUB_MODE_PREEN) {
> > +		if (ctx->mode != SCRUB_MODE_REPAIR) {
> >  			if (!is_inode) {
> >  				/* AG or FS metadata, always warn. */
> >  				str_info(ctx, buf,
> > diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> > index 5ab557d..6efcf77 100644
> > --- a/scrub/xfs_scrub.c
> > +++ b/scrub/xfs_scrub.c
> > @@ -187,7 +187,6 @@ usage(void)
> >  	fprintf(stderr, _("  -v           Verbose output.\n"));
> >  	fprintf(stderr, _("  -V           Print version.\n"));
> >  	fprintf(stderr, _("  -x           Scrub file data too.\n"));
> > -	fprintf(stderr, _("  -y           Repair all errors.\n"));
> >  
> >  	exit(SCRUB_RET_SYNTAX);
> >  }
> > @@ -441,16 +440,11 @@ run_scrub_phases(
> >  		/* Turn on certain phases if user said to. */
> >  		if (sp->fn == DATASCAN_DUMMY_FN && scrub_data) {
> >  			sp->fn = xfs_scan_blocks;
> > -		} else if (sp->fn == REPAIR_DUMMY_FN) {
> > -			if (ctx->mode == SCRUB_MODE_PREEN) {
> > -				sp->descr = _("Optimize filesystem.");
> > -				sp->fn = xfs_optimize_fs;
> > -				sp->must_run = true;
> > -			} else if (ctx->mode == SCRUB_MODE_REPAIR) {
> > -				sp->descr = _("Repair filesystem.");
> > -				sp->fn = xfs_repair_fs;
> > -				sp->must_run = true;
> > -			}
> > +		} else if (sp->fn == REPAIR_DUMMY_FN &&
> > +			   ctx->mode == SCRUB_MODE_REPAIR) {
> > +			sp->descr = _("Repair filesystem.");
> > +			sp->fn = xfs_repair_fs;
> > +			sp->must_run = true;
> >  		}
> >  
> >  		/* Skip certain phases unless they're turned on. */
> > @@ -524,9 +518,9 @@ main(
> >  	textdomain(PACKAGE);
> >  
> >  	pthread_mutex_init(&ctx.lock, NULL);
> > -	ctx.mode = SCRUB_MODE_DEFAULT;
> > +	ctx.mode = SCRUB_MODE_REPAIR;
> >  	ctx.error_action = ERRORS_CONTINUE;
> > -	while ((c = getopt(argc, argv, "a:bC:de:km:nTvxVy")) != EOF) {
> > +	while ((c = getopt(argc, argv, "a:bC:de:km:nTvxV")) != EOF) {
> >  		switch (c) {
> >  		case 'a':
> >  			ctx.max_errors = cvt_u64(optarg, 10);
> > @@ -574,11 +568,6 @@ main(
> >  			mtab = optarg;
> >  			break;
> >  		case 'n':
> > -			if (ctx.mode != SCRUB_MODE_DEFAULT) {
> > -				fprintf(stderr,
> > -_("Only one of the options -n or -y may be specified.\n"));
> > -				usage();
> > -			}
> >  			ctx.mode = SCRUB_MODE_DRY_RUN;
> >  			break;
> >  		case 'T':
> > @@ -595,14 +584,6 @@ _("Only one of the options -n or -y may be specified.\n"));
> >  		case 'x':
> >  			scrub_data = true;
> >  			break;
> > -		case 'y':
> > -			if (ctx.mode != SCRUB_MODE_DEFAULT) {
> > -				fprintf(stderr,
> > -_("Only one of the options -n or -y may be specified.\n"));
> > -				usage();
> > -			}
> > -			ctx.mode = SCRUB_MODE_REPAIR;
> > -			break;
> >  		case '?':
> >  			/* fall through */
> >  		default:
> > diff --git a/scrub/xfs_scrub.h b/scrub/xfs_scrub.h
> > index 8407885..89b46a4 100644
> > --- a/scrub/xfs_scrub.h
> > +++ b/scrub/xfs_scrub.h
> > @@ -34,10 +34,8 @@ extern bool			stdout_isatty;
> >  
> >  enum scrub_mode {
> >  	SCRUB_MODE_DRY_RUN,
> > -	SCRUB_MODE_PREEN,
> >  	SCRUB_MODE_REPAIR,
> >  };
> > -#define SCRUB_MODE_DEFAULT			SCRUB_MODE_PREEN
> >  
> >  enum error_action {
> >  	ERRORS_CONTINUE,
> > @@ -111,7 +109,6 @@ bool xfs_scan_connections(struct scrub_ctx *ctx);
> >  bool xfs_scan_blocks(struct scrub_ctx *ctx);
> >  bool xfs_scan_summary(struct scrub_ctx *ctx);
> >  bool xfs_repair_fs(struct scrub_ctx *ctx);
> > -bool xfs_optimize_fs(struct scrub_ctx *ctx);
> >  
> >  /* Progress estimator functions */
> >  uint64_t xfs_estimate_inodes(struct scrub_ctx *ctx);
> > 
> > --
> > 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
> > 
> --
> 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