[PATCH 0/7] xfsprogs: 4.15 rollup pt. 5

[PATCH 0/7] xfsprogs: 4.15 rollup pt. 5

[Darrick J. Wong]

Hi all,

A bunch of scrub fixes here -- disable syslog if we're debugging the program,
tidying up the scrub manpage to avoid mentioning things that it doesn't yet
know how to do, removing preen mode to bring the command line usage in line
with xfs_repair, fixing complaints about the report that gets printed at the
end, and reclassifying how a lot of runtime errors get logged so that the
return status reflects this.

--D

[PATCH 1/7] xfs_scrub: log operational messages when interactive

[Darrick J. Wong]

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

Record the output of an interactive session in the system log so that
future support requests can get a better picture of what happened.

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


diff --git a/scrub/common.c b/scrub/common.c
index 17c3699..672f286 100644
--- a/scrub/common.c
+++ b/scrub/common.c
@@ -21,6 +21,7 @@
 #include <pthread.h>
 #include <stdbool.h>
 #include <sys/statvfs.h>
+#include <syslog.h>
 #include "platform_defs.h"
 #include "xfs.h"
 #include "xfs_fs.h"
@@ -29,6 +30,8 @@
 #include "common.h"
 #include "progress.h"
 
+extern char		*progname;
+
 /*
  * Reporting Status to the Console
  *
@@ -64,6 +67,12 @@ static const char *err_str[] = {
 	[S_PREEN]	= "Optimized",
 };
 
+static int log_level[] = {
+	[S_ERROR]	= LOG_ERR,
+	[S_WARN]	= LOG_WARNING,
+	[S_INFO]	= LOG_INFO,
+};
+
 /* If stream is a tty, clear to end of line to clean up progress bar. */
 static inline const char *stream_start(FILE *stream)
 {
@@ -73,6 +82,8 @@ static inline const char *stream_start(FILE *stream)
 }
 
 /* Print a warning string and some warning text. */
+#define LOG_BUFSZ	4096
+#define LOGNAME_BUFSZ	256
 void
 __str_out(
 	struct scrub_ctx	*ctx,
@@ -110,6 +121,32 @@ __str_out(
 		va_end(args);
 	}
 
+	/* If we're running interactively, log the message to syslog too. */
+	if (isatty(fileno(stdin)) && !debug) {
+		char	logname[LOGNAME_BUFSZ];
+
+		snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname,
+				ctx->mntpoint);
+		openlog(logname, LOG_PID, LOG_DAEMON);
+
+		if (error) {
+			syslog(LOG_ERR, "%s: %s: %s.",
+					_(err_str[level]), descr,
+					strerror_r(error, buf, DESCR_BUFSZ));
+		} else {
+			char	buf[LOG_BUFSZ];
+			int	sz;
+
+			sz = snprintf(buf, LOG_BUFSZ, "%s: %s: ",
+					_(err_str[level]), descr);
+			va_start(args, format);
+			vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args);
+			va_end(args);
+			syslog(log_level[level], "%s", buf);
+		}
+		closelog();
+	}
+
 	if (debug)
 		fprintf(stream, _(" (%s line %d)"), file, line);
 	fprintf(stream, "\n");

[PATCH 2/7] xfs_scrub: remove preen mode

[Darrick J. Wong]

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.

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);

[PATCH 3/7] xfs_scrub: classify lack of ioctl support as a runtime error

[Darrick J. Wong]

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

If the kernel doesn't have the SCRUB_METADATA ioctl that's a runtime
error, not a fs error.  Account it as such.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/phase1.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/scrub/phase1.c b/scrub/phase1.c
index af93d0f..82c8022 100644
--- a/scrub/phase1.c
+++ b/scrub/phase1.c
@@ -174,14 +174,14 @@ _("Does not appear to be an XFS filesystem!"));
 	    !xfs_can_scrub_bmap(ctx) || !xfs_can_scrub_dir(ctx) ||
 	    !xfs_can_scrub_attr(ctx) || !xfs_can_scrub_symlink(ctx) ||
 	    !xfs_can_scrub_parent(ctx)) {
-		str_error(ctx, ctx->mntpoint,
+		str_info(ctx, ctx->mntpoint,
 _("Kernel metadata scrubbing facility is not available."));
 		return false;
 	}
 
 	/* Do we need kernel-assisted metadata repair? */
 	if (ctx->mode != SCRUB_MODE_DRY_RUN && !xfs_can_repair(ctx)) {
-		str_error(ctx, ctx->mntpoint,
+		str_info(ctx, ctx->mntpoint,
 _("Kernel metadata repair facility is not available.  Use -n to scrub."));
 		return false;
 	}

[PATCH 4/7] xfs_scrub: reclassify runtime errors

[Darrick J. Wong]

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

If the program encounters runtime errors, these should be noted as
information.  Because these errors abort the execution flow (which is
counted as a runtime error), we need only call str_info to log the
event.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/fscounters.c  |    4 ++--
 scrub/inodes.c      |    4 ++--
 scrub/phase1.c      |    8 ++++----
 scrub/phase2.c      |    6 +++---
 scrub/phase3.c      |    2 +-
 scrub/phase6.c      |    2 +-
 scrub/read_verify.c |    2 +-
 scrub/scrub.c       |    8 ++++----
 scrub/spacemap.c    |    8 ++++----
 scrub/vfs.c         |    6 +++---
 10 files changed, 25 insertions(+), 25 deletions(-)


diff --git a/scrub/fscounters.c b/scrub/fscounters.c
index 4294bf3..ecdf4c6 100644
--- a/scrub/fscounters.c
+++ b/scrub/fscounters.c
@@ -139,14 +139,14 @@ xfs_count_all_inodes(
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
 		moveon = false;
-		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
 		goto out_free;
 	}
 	for (agno = 0; agno < ctx->geo.agcount; agno++) {
 		ret = workqueue_add(&wq, xfs_count_ag_inodes, agno, ci);
 		if (ret) {
 			moveon = false;
-			str_error(ctx, ctx->mntpoint,
+			str_info(ctx, ctx->mntpoint,
 _("Could not queue AG %u icount work."), agno);
 			break;
 		}
diff --git a/scrub/inodes.c b/scrub/inodes.c
index 23ec704..57b773e 100644
--- a/scrub/inodes.c
+++ b/scrub/inodes.c
@@ -266,7 +266,7 @@ xfs_scan_all_inodes(
 	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
-		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
 		return false;
 	}
 
@@ -274,7 +274,7 @@ xfs_scan_all_inodes(
 		ret = workqueue_add(&wq, xfs_scan_ag_inodes, agno, &si);
 		if (ret) {
 			si.moveon = false;
-			str_error(ctx, ctx->mntpoint,
+			str_info(ctx, ctx->mntpoint,
 _("Could not queue AG %u bulkstat work."), agno);
 			break;
 		}
diff --git a/scrub/phase1.c b/scrub/phase1.c
index 82c8022..6cd5442 100644
--- a/scrub/phase1.c
+++ b/scrub/phase1.c
@@ -134,7 +134,7 @@ _("Must be root to run scrub."));
 	}
 
 	if (!platform_test_xfs_fd(ctx->mnt_fd)) {
-		str_error(ctx, ctx->mntpoint,
+		str_info(ctx, ctx->mntpoint,
 _("Does not appear to be an XFS filesystem!"));
 		return false;
 	}
@@ -191,7 +191,7 @@ _("Kernel metadata repair facility is not available.  Use -n to scrub."));
 	errno = 0;
 	fsp = fs_table_lookup(ctx->mntpoint, FS_MOUNT_POINT);
 	if (!fsp) {
-		str_error(ctx, ctx->mntpoint,
+		str_info(ctx, ctx->mntpoint,
 _("Unable to find XFS information."));
 		return false;
 	}
@@ -199,12 +199,12 @@ _("Unable to find XFS information."));
 
 	/* Did we find the log and rt devices, if they're present? */
 	if (ctx->geo.logstart == 0 && ctx->fsinfo.fs_log == NULL) {
-		str_error(ctx, ctx->mntpoint,
+		str_info(ctx, ctx->mntpoint,
 _("Unable to find log device path."));
 		return false;
 	}
 	if (ctx->geo.rtblocks && ctx->fsinfo.fs_rt == NULL) {
-		str_error(ctx, ctx->mntpoint,
+		str_info(ctx, ctx->mntpoint,
 _("Unable to find realtime device path."));
 		return false;
 	}
diff --git a/scrub/phase2.c b/scrub/phase2.c
index 32e2752..edf66df 100644
--- a/scrub/phase2.c
+++ b/scrub/phase2.c
@@ -94,7 +94,7 @@ xfs_scan_metadata(
 	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
-		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
 		return false;
 	}
 
@@ -111,7 +111,7 @@ xfs_scan_metadata(
 		ret = workqueue_add(&wq, xfs_scan_ag_metadata, agno, &moveon);
 		if (ret) {
 			moveon = false;
-			str_error(ctx, ctx->mntpoint,
+			str_info(ctx, ctx->mntpoint,
 _("Could not queue AG %u scrub work."), agno);
 			goto out;
 		}
@@ -123,7 +123,7 @@ _("Could not queue AG %u scrub work."), agno);
 	ret = workqueue_add(&wq, xfs_scan_fs_metadata, 0, &moveon);
 	if (ret) {
 		moveon = false;
-		str_error(ctx, ctx->mntpoint,
+		str_info(ctx, ctx->mntpoint,
 _("Could not queue filesystem scrub work."));
 		goto out;
 	}
diff --git a/scrub/phase3.c b/scrub/phase3.c
index f4117b0..a0ee5d9 100644
--- a/scrub/phase3.c
+++ b/scrub/phase3.c
@@ -137,7 +137,7 @@ xfs_scan_inodes(
 	ictx.moveon = true;
 	ictx.icount = ptcounter_init(scrub_nproc(ctx));
 	if (!ictx.icount) {
-		str_error(ctx, ctx->mntpoint, _("Could not create counter."));
+		str_info(ctx, ctx->mntpoint, _("Could not create counter."));
 		return false;
 	}
 
diff --git a/scrub/phase6.c b/scrub/phase6.c
index 9795bf3..f985950 100644
--- a/scrub/phase6.c
+++ b/scrub/phase6.c
@@ -485,7 +485,7 @@ xfs_scan_blocks(
 			xfs_check_rmap_ioerr, disk_heads(ctx->datadev));
 	if (!ve.readverify) {
 		moveon = false;
-		str_error(ctx, ctx->mntpoint,
+		str_info(ctx, ctx->mntpoint,
 _("Could not create media verifier."));
 		goto out_rbad;
 	}
diff --git a/scrub/read_verify.c b/scrub/read_verify.c
index e816688..ae2e85f 100644
--- a/scrub/read_verify.c
+++ b/scrub/read_verify.c
@@ -187,7 +187,7 @@ read_verify_queue(
 
 	ret = workqueue_add(&rvp->wq, read_verify, 0, tmp);
 	if (ret) {
-		str_error(rvp->ctx, rvp->ctx->mntpoint,
+		str_info(rvp->ctx, rvp->ctx->mntpoint,
 _("Could not queue read-verify work."));
 		free(tmp);
 		return false;
diff --git a/scrub/scrub.c b/scrub/scrub.c
index bc0e2f0..ff5357c 100644
--- a/scrub/scrub.c
+++ b/scrub/scrub.c
@@ -219,7 +219,7 @@ xfs_check_metadata(
 			return CHECK_DONE;
 		case ESHUTDOWN:
 			/* FS already crashed, give up. */
-			str_error(ctx, buf,
+			str_info(ctx, buf,
 _("Filesystem is shut down, aborting."));
 			return CHECK_ABORT;
 		case EIO:
@@ -235,7 +235,7 @@ _("Filesystem is shut down, aborting."));
 			 * The first two should never escape the kernel,
 			 * and the other two should be reported via sm_flags.
 			 */
-			str_error(ctx, buf,
+			str_info(ctx, buf,
 _("Kernel bug!  errno=%d"), code);
 			/* fall through */
 		default:
@@ -568,7 +568,7 @@ __xfs_scrub_test(
 	if (debug_tweak_on("XFS_SCRUB_NO_KERNEL"))
 		return false;
 	if (debug_tweak_on("XFS_SCRUB_FORCE_REPAIR") && !injected) {
-		str_error(ctx, "XFS_SCRUB_FORCE_REPAIR", "Not supported.");
+		str_info(ctx, "XFS_SCRUB_FORCE_REPAIR", "Not supported.");
 		return false;
 	}
 
@@ -726,7 +726,7 @@ _("Filesystem is busy, deferring repair."));
 			return CHECK_RETRY;
 		case ESHUTDOWN:
 			/* Filesystem is already shut down, abort. */
-			str_error(ctx, buf,
+			str_info(ctx, buf,
 _("Filesystem is shut down, aborting."));
 			return CHECK_ABORT;
 		case ENOTTY:
diff --git a/scrub/spacemap.c b/scrub/spacemap.c
index 2dc6e2b..f631913 100644
--- a/scrub/spacemap.c
+++ b/scrub/spacemap.c
@@ -217,7 +217,7 @@ xfs_scan_all_spacemaps(
 	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
-		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
 		return false;
 	}
 	if (ctx->fsinfo.fs_rt) {
@@ -225,7 +225,7 @@ xfs_scan_all_spacemaps(
 				ctx->geo.agcount + 1, &sbx);
 		if (ret) {
 			sbx.moveon = false;
-			str_error(ctx, ctx->mntpoint,
+			str_info(ctx, ctx->mntpoint,
 _("Could not queue rtdev fsmap work."));
 			goto out;
 		}
@@ -235,7 +235,7 @@ _("Could not queue rtdev fsmap work."));
 				ctx->geo.agcount + 2, &sbx);
 		if (ret) {
 			sbx.moveon = false;
-			str_error(ctx, ctx->mntpoint,
+			str_info(ctx, ctx->mntpoint,
 _("Could not queue logdev fsmap work."));
 			goto out;
 		}
@@ -244,7 +244,7 @@ _("Could not queue logdev fsmap work."));
 		ret = workqueue_add(&wq, xfs_scan_ag_blocks, agno, &sbx);
 		if (ret) {
 			sbx.moveon = false;
-			str_error(ctx, ctx->mntpoint,
+			str_info(ctx, ctx->mntpoint,
 _("Could not queue AG %u fsmap work."), agno);
 			break;
 		}
diff --git a/scrub/vfs.c b/scrub/vfs.c
index 573a2d0..0c5b353 100644
--- a/scrub/vfs.c
+++ b/scrub/vfs.c
@@ -145,7 +145,7 @@ scan_fs_dir(
 			pthread_mutex_unlock(&sft->lock);
 			error = workqueue_add(wq, scan_fs_dir, 0, new_sftd);
 			if (error) {
-				str_error(ctx, ctx->mntpoint,
+				str_info(ctx, ctx->mntpoint,
 _("Could not queue subdirectory scan work."));
 				sft->moveon = false;
 				break;
@@ -203,12 +203,12 @@ scan_fs_tree(
 	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
-		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
 		goto out_free;
 	}
 	ret = workqueue_add(&wq, scan_fs_dir, 0, sftd);
 	if (ret) {
-		str_error(ctx, ctx->mntpoint,
+		str_info(ctx, ctx->mntpoint,
 _("Could not queue directory scan work."));
 		goto out_free;
 	}

[PATCH 5/7] xfs_scrub: reclassify some of the warning messages

[Darrick J. Wong]

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

Some of the warning messages are actually runtime errors in optional
components, so turn them into informational messages.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/inodes.c |    6 +++---
 scrub/phase6.c |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)


diff --git a/scrub/inodes.c b/scrub/inodes.c
index 57b773e..744b003 100644
--- a/scrub/inodes.c
+++ b/scrub/inodes.c
@@ -157,7 +157,7 @@ xfs_iterate_inodes_range(
 		bulkreq.icount = inogrp.xi_alloccount;
 		error = ioctl(ctx->mnt_fd, XFS_IOC_FSBULKSTAT, &bulkreq);
 		if (error)
-			str_warn(ctx, descr, "%s", strerror_r(errno,
+			str_info(ctx, descr, "%s", strerror_r(errno,
 						buf, DESCR_BUFSZ));
 
 		xfs_iterate_inodes_range_check(ctx, &inogrp, bstat);
@@ -181,8 +181,8 @@ xfs_iterate_inodes_range(
 				}
 				snprintf(idescr, DESCR_BUFSZ, "inode %"PRIu64,
 						(uint64_t)bs->bs_ino);
-				str_warn(ctx, idescr, "%s", strerror_r(error,
-						buf, DESCR_BUFSZ));
+				str_info(ctx, idescr,
+_("Changed too many times during scan; giving up."));
 				break;
 			case XFS_ITERATE_INODES_ABORT:
 				error = 0;
diff --git a/scrub/phase6.c b/scrub/phase6.c
index f985950..e255eef 100644
--- a/scrub/phase6.c
+++ b/scrub/phase6.c
@@ -188,7 +188,6 @@ xfs_report_verify_inode(
 	void				*arg)
 {
 	char				descr[DESCR_BUFSZ];
-	char				buf[DESCR_BUFSZ];
 	bool				moveon;
 	int				fd;
 	int				error;
@@ -209,7 +208,8 @@ xfs_report_verify_inode(
 		if (error == ESTALE)
 			return error;
 
-		str_warn(ctx, descr, "%s", strerror_r(error, buf, DESCR_BUFSZ));
+		str_info(ctx, descr,
+_("Disappeared during read error reporting."));
 		return error;
 	}
 

[PATCH 6/7] xfs_scrub: always init phase information

[Darrick J. Wong]

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

Make sure we initialize the overall phase state before we start
executing any code that can end up in the report-status-and-exit paths.
Otherwise if debugging is turned on we get garbage io/cpu stat reports.

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


diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
index 6efcf77..89b7fa0 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -618,6 +618,11 @@ main(
 	if (getenv("SERVICE_MODE"))
 		is_service = true;
 
+	/* Initialize overall phase stats. */
+	moveon = phase_start(&all_pi, 0, NULL);
+	if (!moveon)
+		return SCRUB_RET_OPERROR;
+
 	/* Find the mount record for the passed-in argument. */
 	if (stat(argv[optind], &ctx.mnt_sb) < 0) {
 		fprintf(stderr,
@@ -641,11 +646,6 @@ main(
 			mtab = _PATH_MOUNTED;
 	}
 
-	/* Initialize overall phase stats. */
-	moveon = phase_start(&all_pi, 0, NULL);
-	if (!moveon)
-		goto out;
-
 	ismnt = find_mountpoint(mtab, &ctx);
 	if (!ismnt) {
 		fprintf(stderr,

[PATCH 7/7] xfs_scrub: refactor outcome display into a separate helper

[Darrick J. Wong]

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

Move all the printing of the scrub outcome into a separate helper to
declutter the main function.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/xfs_scrub.c |   48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)


diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
index 89b7fa0..fdd35df 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -493,6 +493,34 @@ _("Scrub aborted after phase %d."),
 	return moveon;
 }
 
+static void
+report_outcome(
+	struct scrub_ctx	*ctx)
+{
+	unsigned long long	total_errors;
+
+	total_errors = ctx->errors_found + ctx->runtime_errors;
+
+	if (total_errors == 0 && ctx->warnings_found == 0)
+		return;
+
+	if (total_errors == 0)
+		fprintf(stderr, _("%s: warnings found: %llu."), ctx->mntpoint,
+				ctx->warnings_found);
+	else if (ctx->warnings_found == 0)
+		fprintf(stderr, _("%s: errors found: %llu."), ctx->mntpoint,
+				total_errors);
+	else
+		fprintf(stderr, _("%s: errors found: %llu; warnings found: %llu."),
+				ctx->mntpoint, total_errors,
+				ctx->warnings_found);
+
+	if (ctx->need_repair)
+		fprintf(stderr, "  %s\n", _("Unmount and run xfs_repair."));
+	else
+		fprintf(stderr, "\n");
+}
+
 int
 main(
 	int			argc,
@@ -501,9 +529,7 @@ main(
 	struct scrub_ctx	ctx = {0};
 	struct phase_rusage	all_pi;
 	char			*mtab = NULL;
-	char			*repairstr = "";
 	FILE			*progress_fp = NULL;
-	unsigned long long	total_errors;
 	bool			moveon = true;
 	bool			ismnt;
 	int			c;
@@ -692,22 +718,8 @@ _("%s: Not a XFS mount point or block device.\n"),
 		ctx.runtime_errors++;
 
 out:
-	total_errors = ctx.errors_found + ctx.runtime_errors;
-	if (ctx.need_repair)
-		repairstr = _("  Unmount and run xfs_repair.");
-	if (total_errors && ctx.warnings_found)
-		fprintf(stderr,
-_("%s: %llu errors and %llu warnings found.%s\n"),
-			ctx.mntpoint, total_errors, ctx.warnings_found,
-			repairstr);
-	else if (total_errors && ctx.warnings_found == 0)
-		fprintf(stderr,
-_("%s: %llu errors found.%s\n"),
-			ctx.mntpoint, total_errors, repairstr);
-	else if (total_errors == 0 && ctx.warnings_found)
-		fprintf(stderr,
-_("%s: %llu warnings found.\n"),
-			ctx.mntpoint, ctx.warnings_found);
+	report_outcome(&ctx);
+
 	if (ctx.errors_found) {
 		if (ctx.error_action == ERRORS_SHUTDOWN)
 			xfs_shutdown_fs(&ctx);

Re: [PATCH 1/7] xfs_scrub: log operational messages when interactive

[Eric Sandeen]

On 2/5/18 5:22 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Record the output of an interactive session in the system log so that
> future support requests can get a better picture of what happened.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I really want to log things, but I'm conflicted about spamming syslog.
I'm wondering if a generic /var/log/xfs.log would be good, and it could
eventually log all xfs-related administrative actions and outcomes with
a libfrog library helper... I don't know how non-syslog log files are
handled in general, does everybody just roll their own?

I'm inclined to leave this one out of 4.15 for now while I/we think
about the big picture here.

-Eric

> ---
>  scrub/common.c |   37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> 
> diff --git a/scrub/common.c b/scrub/common.c
> index 17c3699..672f286 100644
> --- a/scrub/common.c
> +++ b/scrub/common.c
> @@ -21,6 +21,7 @@
>  #include <pthread.h>
>  #include <stdbool.h>
>  #include <sys/statvfs.h>
> +#include <syslog.h>
>  #include "platform_defs.h"
>  #include "xfs.h"
>  #include "xfs_fs.h"
> @@ -29,6 +30,8 @@
>  #include "common.h"
>  #include "progress.h"
>  
> +extern char		*progname;
> +
>  /*
>   * Reporting Status to the Console
>   *
> @@ -64,6 +67,12 @@ static const char *err_str[] = {
>  	[S_PREEN]	= "Optimized",
>  };
>  
> +static int log_level[] = {
> +	[S_ERROR]	= LOG_ERR,
> +	[S_WARN]	= LOG_WARNING,
> +	[S_INFO]	= LOG_INFO,
> +};
> +
>  /* If stream is a tty, clear to end of line to clean up progress bar. */
>  static inline const char *stream_start(FILE *stream)
>  {
> @@ -73,6 +82,8 @@ static inline const char *stream_start(FILE *stream)
>  }
>  
>  /* Print a warning string and some warning text. */
> +#define LOG_BUFSZ	4096
> +#define LOGNAME_BUFSZ	256
>  void
>  __str_out(
>  	struct scrub_ctx	*ctx,
> @@ -110,6 +121,32 @@ __str_out(
>  		va_end(args);
>  	}
>  
> +	/* If we're running interactively, log the message to syslog too. */
> +	if (isatty(fileno(stdin)) && !debug) {
> +		char	logname[LOGNAME_BUFSZ];
> +
> +		snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname,
> +				ctx->mntpoint);
> +		openlog(logname, LOG_PID, LOG_DAEMON);
> +
> +		if (error) {
> +			syslog(LOG_ERR, "%s: %s: %s.",
> +					_(err_str[level]), descr,
> +					strerror_r(error, buf, DESCR_BUFSZ));
> +		} else {
> +			char	buf[LOG_BUFSZ];
> +			int	sz;
> +
> +			sz = snprintf(buf, LOG_BUFSZ, "%s: %s: ",
> +					_(err_str[level]), descr);
> +			va_start(args, format);
> +			vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args);
> +			va_end(args);
> +			syslog(log_level[level], "%s", buf);
> +		}
> +		closelog();
> +	}
> +
>  	if (debug)
>  		fprintf(stream, _(" (%s line %d)"), file, line);
>  	fprintf(stream, "\n");
> 
> --
> 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
> 

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

[Eric Sandeen]

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.

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
> 

Re: [PATCH 3/7] xfs_scrub: classify lack of ioctl support as a runtime error

[Eric Sandeen]

On 2/5/18 5:22 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If the kernel doesn't have the SCRUB_METADATA ioctl that's a runtime
> error, not a fs error.  Account it as such.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Yup that's better I think.  I wonder if eventually str_$FOO should
get renamed to make it crystal clear to the caller whether they are
registering a filesystem error or something else, but this fixes this
problem up for now.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  scrub/phase1.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/scrub/phase1.c b/scrub/phase1.c
> index af93d0f..82c8022 100644
> --- a/scrub/phase1.c
> +++ b/scrub/phase1.c
> @@ -174,14 +174,14 @@ _("Does not appear to be an XFS filesystem!"));
>  	    !xfs_can_scrub_bmap(ctx) || !xfs_can_scrub_dir(ctx) ||
>  	    !xfs_can_scrub_attr(ctx) || !xfs_can_scrub_symlink(ctx) ||
>  	    !xfs_can_scrub_parent(ctx)) {
> -		str_error(ctx, ctx->mntpoint,
> +		str_info(ctx, ctx->mntpoint,
>  _("Kernel metadata scrubbing facility is not available."));
>  		return false;
>  	}
>  
>  	/* Do we need kernel-assisted metadata repair? */
>  	if (ctx->mode != SCRUB_MODE_DRY_RUN && !xfs_can_repair(ctx)) {
> -		str_error(ctx, ctx->mntpoint,
> +		str_info(ctx, ctx->mntpoint,
>  _("Kernel metadata repair facility is not available.  Use -n to scrub."));
>  		return false;
>  	}
> 
> --
> 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
> 

Re: [PATCH 4/7] xfs_scrub: reclassify runtime errors

[Eric Sandeen]

On 2/5/18 5:22 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If the program encounters runtime errors, these should be noted as
> information.  Because these errors abort the execution flow (which is
> counted as a runtime error), we need only call str_info to log the
> event.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Ok.  Again I wonder if eventually str_error should turn into
scrub_log_fs_corruption() and str_info into scrub_log_runtime_error()
or something...

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  scrub/fscounters.c  |    4 ++--
>  scrub/inodes.c      |    4 ++--
>  scrub/phase1.c      |    8 ++++----
>  scrub/phase2.c      |    6 +++---
>  scrub/phase3.c      |    2 +-
>  scrub/phase6.c      |    2 +-
>  scrub/read_verify.c |    2 +-
>  scrub/scrub.c       |    8 ++++----
>  scrub/spacemap.c    |    8 ++++----
>  scrub/vfs.c         |    6 +++---
>  10 files changed, 25 insertions(+), 25 deletions(-)
> 
> 
> diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> index 4294bf3..ecdf4c6 100644
> --- a/scrub/fscounters.c
> +++ b/scrub/fscounters.c
> @@ -139,14 +139,14 @@ xfs_count_all_inodes(
>  			scrub_nproc_workqueue(ctx));
>  	if (ret) {
>  		moveon = false;
> -		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
> +		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>  		goto out_free;
>  	}
>  	for (agno = 0; agno < ctx->geo.agcount; agno++) {
>  		ret = workqueue_add(&wq, xfs_count_ag_inodes, agno, ci);
>  		if (ret) {
>  			moveon = false;
> -			str_error(ctx, ctx->mntpoint,
> +			str_info(ctx, ctx->mntpoint,
>  _("Could not queue AG %u icount work."), agno);
>  			break;
>  		}
> diff --git a/scrub/inodes.c b/scrub/inodes.c
> index 23ec704..57b773e 100644
> --- a/scrub/inodes.c
> +++ b/scrub/inodes.c
> @@ -266,7 +266,7 @@ xfs_scan_all_inodes(
>  	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
>  			scrub_nproc_workqueue(ctx));
>  	if (ret) {
> -		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
> +		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>  		return false;
>  	}
>  
> @@ -274,7 +274,7 @@ xfs_scan_all_inodes(
>  		ret = workqueue_add(&wq, xfs_scan_ag_inodes, agno, &si);
>  		if (ret) {
>  			si.moveon = false;
> -			str_error(ctx, ctx->mntpoint,
> +			str_info(ctx, ctx->mntpoint,
>  _("Could not queue AG %u bulkstat work."), agno);
>  			break;
>  		}
> diff --git a/scrub/phase1.c b/scrub/phase1.c
> index 82c8022..6cd5442 100644
> --- a/scrub/phase1.c
> +++ b/scrub/phase1.c
> @@ -134,7 +134,7 @@ _("Must be root to run scrub."));
>  	}
>  
>  	if (!platform_test_xfs_fd(ctx->mnt_fd)) {
> -		str_error(ctx, ctx->mntpoint,
> +		str_info(ctx, ctx->mntpoint,
>  _("Does not appear to be an XFS filesystem!"));
>  		return false;
>  	}
> @@ -191,7 +191,7 @@ _("Kernel metadata repair facility is not available.  Use -n to scrub."));
>  	errno = 0;
>  	fsp = fs_table_lookup(ctx->mntpoint, FS_MOUNT_POINT);
>  	if (!fsp) {
> -		str_error(ctx, ctx->mntpoint,
> +		str_info(ctx, ctx->mntpoint,
>  _("Unable to find XFS information."));
>  		return false;
>  	}
> @@ -199,12 +199,12 @@ _("Unable to find XFS information."));
>  
>  	/* Did we find the log and rt devices, if they're present? */
>  	if (ctx->geo.logstart == 0 && ctx->fsinfo.fs_log == NULL) {
> -		str_error(ctx, ctx->mntpoint,
> +		str_info(ctx, ctx->mntpoint,
>  _("Unable to find log device path."));
>  		return false;
>  	}
>  	if (ctx->geo.rtblocks && ctx->fsinfo.fs_rt == NULL) {
> -		str_error(ctx, ctx->mntpoint,
> +		str_info(ctx, ctx->mntpoint,
>  _("Unable to find realtime device path."));
>  		return false;
>  	}
> diff --git a/scrub/phase2.c b/scrub/phase2.c
> index 32e2752..edf66df 100644
> --- a/scrub/phase2.c
> +++ b/scrub/phase2.c
> @@ -94,7 +94,7 @@ xfs_scan_metadata(
>  	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
>  			scrub_nproc_workqueue(ctx));
>  	if (ret) {
> -		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
> +		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>  		return false;
>  	}
>  
> @@ -111,7 +111,7 @@ xfs_scan_metadata(
>  		ret = workqueue_add(&wq, xfs_scan_ag_metadata, agno, &moveon);
>  		if (ret) {
>  			moveon = false;
> -			str_error(ctx, ctx->mntpoint,
> +			str_info(ctx, ctx->mntpoint,
>  _("Could not queue AG %u scrub work."), agno);
>  			goto out;
>  		}
> @@ -123,7 +123,7 @@ _("Could not queue AG %u scrub work."), agno);
>  	ret = workqueue_add(&wq, xfs_scan_fs_metadata, 0, &moveon);
>  	if (ret) {
>  		moveon = false;
> -		str_error(ctx, ctx->mntpoint,
> +		str_info(ctx, ctx->mntpoint,
>  _("Could not queue filesystem scrub work."));
>  		goto out;
>  	}
> diff --git a/scrub/phase3.c b/scrub/phase3.c
> index f4117b0..a0ee5d9 100644
> --- a/scrub/phase3.c
> +++ b/scrub/phase3.c
> @@ -137,7 +137,7 @@ xfs_scan_inodes(
>  	ictx.moveon = true;
>  	ictx.icount = ptcounter_init(scrub_nproc(ctx));
>  	if (!ictx.icount) {
> -		str_error(ctx, ctx->mntpoint, _("Could not create counter."));
> +		str_info(ctx, ctx->mntpoint, _("Could not create counter."));
>  		return false;
>  	}
>  
> diff --git a/scrub/phase6.c b/scrub/phase6.c
> index 9795bf3..f985950 100644
> --- a/scrub/phase6.c
> +++ b/scrub/phase6.c
> @@ -485,7 +485,7 @@ xfs_scan_blocks(
>  			xfs_check_rmap_ioerr, disk_heads(ctx->datadev));
>  	if (!ve.readverify) {
>  		moveon = false;
> -		str_error(ctx, ctx->mntpoint,
> +		str_info(ctx, ctx->mntpoint,
>  _("Could not create media verifier."));
>  		goto out_rbad;
>  	}
> diff --git a/scrub/read_verify.c b/scrub/read_verify.c
> index e816688..ae2e85f 100644
> --- a/scrub/read_verify.c
> +++ b/scrub/read_verify.c
> @@ -187,7 +187,7 @@ read_verify_queue(
>  
>  	ret = workqueue_add(&rvp->wq, read_verify, 0, tmp);
>  	if (ret) {
> -		str_error(rvp->ctx, rvp->ctx->mntpoint,
> +		str_info(rvp->ctx, rvp->ctx->mntpoint,
>  _("Could not queue read-verify work."));
>  		free(tmp);
>  		return false;
> diff --git a/scrub/scrub.c b/scrub/scrub.c
> index bc0e2f0..ff5357c 100644
> --- a/scrub/scrub.c
> +++ b/scrub/scrub.c
> @@ -219,7 +219,7 @@ xfs_check_metadata(
>  			return CHECK_DONE;
>  		case ESHUTDOWN:
>  			/* FS already crashed, give up. */
> -			str_error(ctx, buf,
> +			str_info(ctx, buf,
>  _("Filesystem is shut down, aborting."));
>  			return CHECK_ABORT;
>  		case EIO:
> @@ -235,7 +235,7 @@ _("Filesystem is shut down, aborting."));
>  			 * The first two should never escape the kernel,
>  			 * and the other two should be reported via sm_flags.
>  			 */
> -			str_error(ctx, buf,
> +			str_info(ctx, buf,
>  _("Kernel bug!  errno=%d"), code);
>  			/* fall through */
>  		default:
> @@ -568,7 +568,7 @@ __xfs_scrub_test(
>  	if (debug_tweak_on("XFS_SCRUB_NO_KERNEL"))
>  		return false;
>  	if (debug_tweak_on("XFS_SCRUB_FORCE_REPAIR") && !injected) {
> -		str_error(ctx, "XFS_SCRUB_FORCE_REPAIR", "Not supported.");
> +		str_info(ctx, "XFS_SCRUB_FORCE_REPAIR", "Not supported.");
>  		return false;
>  	}
>  
> @@ -726,7 +726,7 @@ _("Filesystem is busy, deferring repair."));
>  			return CHECK_RETRY;
>  		case ESHUTDOWN:
>  			/* Filesystem is already shut down, abort. */
> -			str_error(ctx, buf,
> +			str_info(ctx, buf,
>  _("Filesystem is shut down, aborting."));
>  			return CHECK_ABORT;
>  		case ENOTTY:
> diff --git a/scrub/spacemap.c b/scrub/spacemap.c
> index 2dc6e2b..f631913 100644
> --- a/scrub/spacemap.c
> +++ b/scrub/spacemap.c
> @@ -217,7 +217,7 @@ xfs_scan_all_spacemaps(
>  	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
>  			scrub_nproc_workqueue(ctx));
>  	if (ret) {
> -		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
> +		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>  		return false;
>  	}
>  	if (ctx->fsinfo.fs_rt) {
> @@ -225,7 +225,7 @@ xfs_scan_all_spacemaps(
>  				ctx->geo.agcount + 1, &sbx);
>  		if (ret) {
>  			sbx.moveon = false;
> -			str_error(ctx, ctx->mntpoint,
> +			str_info(ctx, ctx->mntpoint,
>  _("Could not queue rtdev fsmap work."));
>  			goto out;
>  		}
> @@ -235,7 +235,7 @@ _("Could not queue rtdev fsmap work."));
>  				ctx->geo.agcount + 2, &sbx);
>  		if (ret) {
>  			sbx.moveon = false;
> -			str_error(ctx, ctx->mntpoint,
> +			str_info(ctx, ctx->mntpoint,
>  _("Could not queue logdev fsmap work."));
>  			goto out;
>  		}
> @@ -244,7 +244,7 @@ _("Could not queue logdev fsmap work."));
>  		ret = workqueue_add(&wq, xfs_scan_ag_blocks, agno, &sbx);
>  		if (ret) {
>  			sbx.moveon = false;
> -			str_error(ctx, ctx->mntpoint,
> +			str_info(ctx, ctx->mntpoint,
>  _("Could not queue AG %u fsmap work."), agno);
>  			break;
>  		}
> diff --git a/scrub/vfs.c b/scrub/vfs.c
> index 573a2d0..0c5b353 100644
> --- a/scrub/vfs.c
> +++ b/scrub/vfs.c
> @@ -145,7 +145,7 @@ scan_fs_dir(
>  			pthread_mutex_unlock(&sft->lock);
>  			error = workqueue_add(wq, scan_fs_dir, 0, new_sftd);
>  			if (error) {
> -				str_error(ctx, ctx->mntpoint,
> +				str_info(ctx, ctx->mntpoint,
>  _("Could not queue subdirectory scan work."));
>  				sft->moveon = false;
>  				break;
> @@ -203,12 +203,12 @@ scan_fs_tree(
>  	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
>  			scrub_nproc_workqueue(ctx));
>  	if (ret) {
> -		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
> +		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>  		goto out_free;
>  	}
>  	ret = workqueue_add(&wq, scan_fs_dir, 0, sftd);
>  	if (ret) {
> -		str_error(ctx, ctx->mntpoint,
> +		str_info(ctx, ctx->mntpoint,
>  _("Could not queue directory scan work."));
>  		goto out_free;
>  	}
> 
> --
> 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
> 

Re: [PATCH 5/7] xfs_scrub: reclassify some of the warning messages

[Eric Sandeen]

On 2/5/18 5:22 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Some of the warning messages are actually runtime errors in optional
> components, so turn them into informational messages.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  scrub/inodes.c |    6 +++---
>  scrub/phase6.c |    4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/scrub/inodes.c b/scrub/inodes.c
> index 57b773e..744b003 100644
> --- a/scrub/inodes.c
> +++ b/scrub/inodes.c
> @@ -157,7 +157,7 @@ xfs_iterate_inodes_range(
>  		bulkreq.icount = inogrp.xi_alloccount;
>  		error = ioctl(ctx->mnt_fd, XFS_IOC_FSBULKSTAT, &bulkreq);
>  		if (error)
> -			str_warn(ctx, descr, "%s", strerror_r(errno,
> +			str_info(ctx, descr, "%s", strerror_r(errno,
>  						buf, DESCR_BUFSZ));
>  
>  		xfs_iterate_inodes_range_check(ctx, &inogrp, bstat);
> @@ -181,8 +181,8 @@ xfs_iterate_inodes_range(
>  				}
>  				snprintf(idescr, DESCR_BUFSZ, "inode %"PRIu64,
>  						(uint64_t)bs->bs_ino);
> -				str_warn(ctx, idescr, "%s", strerror_r(error,
> -						buf, DESCR_BUFSZ));
> +				str_info(ctx, idescr,
> +_("Changed too many times during scan; giving up."));
>  				break;
>  			case XFS_ITERATE_INODES_ABORT:
>  				error = 0;
> diff --git a/scrub/phase6.c b/scrub/phase6.c
> index f985950..e255eef 100644
> --- a/scrub/phase6.c
> +++ b/scrub/phase6.c
> @@ -188,7 +188,6 @@ xfs_report_verify_inode(
>  	void				*arg)
>  {
>  	char				descr[DESCR_BUFSZ];
> -	char				buf[DESCR_BUFSZ];
>  	bool				moveon;
>  	int				fd;
>  	int				error;
> @@ -209,7 +208,8 @@ xfs_report_verify_inode(
>  		if (error == ESTALE)
>  			return error;
>  
> -		str_warn(ctx, descr, "%s", strerror_r(error, buf, DESCR_BUFSZ));
> +		str_info(ctx, descr,
> +_("Disappeared during read error reporting."));
>  		return error;
>  	}
>  
> 
> --
> 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
> 

Re: [PATCH 6/7] xfs_scrub: always init phase information

[Eric Sandeen]

On 2/5/18 5:23 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure we initialize the overall phase state before we start
> executing any code that can end up in the report-status-and-exit paths.
> Otherwise if debugging is turned on we get garbage io/cpu stat reports.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  scrub/xfs_scrub.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> index 6efcf77..89b7fa0 100644
> --- a/scrub/xfs_scrub.c
> +++ b/scrub/xfs_scrub.c
> @@ -618,6 +618,11 @@ main(
>  	if (getenv("SERVICE_MODE"))
>  		is_service = true;
>  
> +	/* Initialize overall phase stats. */
> +	moveon = phase_start(&all_pi, 0, NULL);
> +	if (!moveon)
> +		return SCRUB_RET_OPERROR;
> +
>  	/* Find the mount record for the passed-in argument. */
>  	if (stat(argv[optind], &ctx.mnt_sb) < 0) {
>  		fprintf(stderr,
> @@ -641,11 +646,6 @@ main(
>  			mtab = _PATH_MOUNTED;
>  	}
>  
> -	/* Initialize overall phase stats. */
> -	moveon = phase_start(&all_pi, 0, NULL);
> -	if (!moveon)
> -		goto out;
> -
>  	ismnt = find_mountpoint(mtab, &ctx);
>  	if (!ismnt) {
>  		fprintf(stderr,
> 
> --
> 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
> 

Re: [PATCH 7/7] xfs_scrub: refactor outcome display into a separate helper

[Eric Sandeen]

On 2/5/18 5:23 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move all the printing of the scrub outcome into a separate helper to
> declutter the main function.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>


This might get a bit long, no?

"/path/to/filesystem: errors found 16384; warnings found 16.  Unmount and run xfs_repair."

Can we put the "Unmount and run xfs_repair" on its own line?
That'd make it stand out more, as well.  </nitpick>


> ---
>  scrub/xfs_scrub.c |   48 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> 
> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> index 89b7fa0..fdd35df 100644
> --- a/scrub/xfs_scrub.c
> +++ b/scrub/xfs_scrub.c
> @@ -493,6 +493,34 @@ _("Scrub aborted after phase %d."),
>  	return moveon;
>  }
>  
> +static void
> +report_outcome(
> +	struct scrub_ctx	*ctx)
> +{
> +	unsigned long long	total_errors;
> +
> +	total_errors = ctx->errors_found + ctx->runtime_errors;
> +
> +	if (total_errors == 0 && ctx->warnings_found == 0)
> +		return;
> +
> +	if (total_errors == 0)
> +		fprintf(stderr, _("%s: warnings found: %llu."), ctx->mntpoint,
> +				ctx->warnings_found);
> +	else if (ctx->warnings_found == 0)
> +		fprintf(stderr, _("%s: errors found: %llu."), ctx->mntpoint,
> +				total_errors);
> +	else
> +		fprintf(stderr, _("%s: errors found: %llu; warnings found: %llu."),
> +				ctx->mntpoint, total_errors,
> +				ctx->warnings_found);
> +
> +	if (ctx->need_repair)
> +		fprintf(stderr, "  %s\n", _("Unmount and run xfs_repair."));
> +	else
> +		fprintf(stderr, "\n");
> +}
> +
>  int
>  main(
>  	int			argc,
> @@ -501,9 +529,7 @@ main(
>  	struct scrub_ctx	ctx = {0};
>  	struct phase_rusage	all_pi;
>  	char			*mtab = NULL;
> -	char			*repairstr = "";
>  	FILE			*progress_fp = NULL;
> -	unsigned long long	total_errors;
>  	bool			moveon = true;
>  	bool			ismnt;
>  	int			c;
> @@ -692,22 +718,8 @@ _("%s: Not a XFS mount point or block device.\n"),
>  		ctx.runtime_errors++;
>  
>  out:
> -	total_errors = ctx.errors_found + ctx.runtime_errors;
> -	if (ctx.need_repair)
> -		repairstr = _("  Unmount and run xfs_repair.");
> -	if (total_errors && ctx.warnings_found)
> -		fprintf(stderr,
> -_("%s: %llu errors and %llu warnings found.%s\n"),
> -			ctx.mntpoint, total_errors, ctx.warnings_found,
> -			repairstr);
> -	else if (total_errors && ctx.warnings_found == 0)
> -		fprintf(stderr,
> -_("%s: %llu errors found.%s\n"),
> -			ctx.mntpoint, total_errors, repairstr);
> -	else if (total_errors == 0 && ctx.warnings_found)
> -		fprintf(stderr,
> -_("%s: %llu warnings found.\n"),
> -			ctx.mntpoint, ctx.warnings_found);
> +	report_outcome(&ctx);
> +
>  	if (ctx.errors_found) {
>  		if (ctx.error_action == ERRORS_SHUTDOWN)
>  			xfs_shutdown_fs(&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
> 

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

[Darrick J. Wong]

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

Re: [PATCH 7/7] xfs_scrub: refactor outcome display into a separate helper

[Darrick J. Wong]

On Mon, Feb 12, 2018 at 03:06:58PM -0600, Eric Sandeen wrote:
> On 2/5/18 5:23 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Move all the printing of the scrub outcome into a separate helper to
> > declutter the main function.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 
> This might get a bit long, no?
> 
> "/path/to/filesystem: errors found 16384; warnings found 16.  Unmount and run xfs_repair."
> 
> Can we put the "Unmount and run xfs_repair" on its own line?
> That'd make it stand out more, as well.  </nitpick>

Sure, works for me.  Should I send a separate patch or will you just fix
it up on the way in?

--D

> 
> > ---
> >  scrub/xfs_scrub.c |   48 ++++++++++++++++++++++++++++++------------------
> >  1 file changed, 30 insertions(+), 18 deletions(-)
> > 
> > 
> > diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> > index 89b7fa0..fdd35df 100644
> > --- a/scrub/xfs_scrub.c
> > +++ b/scrub/xfs_scrub.c
> > @@ -493,6 +493,34 @@ _("Scrub aborted after phase %d."),
> >  	return moveon;
> >  }
> >  
> > +static void
> > +report_outcome(
> > +	struct scrub_ctx	*ctx)
> > +{
> > +	unsigned long long	total_errors;
> > +
> > +	total_errors = ctx->errors_found + ctx->runtime_errors;
> > +
> > +	if (total_errors == 0 && ctx->warnings_found == 0)
> > +		return;
> > +
> > +	if (total_errors == 0)
> > +		fprintf(stderr, _("%s: warnings found: %llu."), ctx->mntpoint,
> > +				ctx->warnings_found);
> > +	else if (ctx->warnings_found == 0)
> > +		fprintf(stderr, _("%s: errors found: %llu."), ctx->mntpoint,
> > +				total_errors);
> > +	else
> > +		fprintf(stderr, _("%s: errors found: %llu; warnings found: %llu."),
> > +				ctx->mntpoint, total_errors,
> > +				ctx->warnings_found);
> > +
> > +	if (ctx->need_repair)
> > +		fprintf(stderr, "  %s\n", _("Unmount and run xfs_repair."));
> > +	else
> > +		fprintf(stderr, "\n");
> > +}
> > +
> >  int
> >  main(
> >  	int			argc,
> > @@ -501,9 +529,7 @@ main(
> >  	struct scrub_ctx	ctx = {0};
> >  	struct phase_rusage	all_pi;
> >  	char			*mtab = NULL;
> > -	char			*repairstr = "";
> >  	FILE			*progress_fp = NULL;
> > -	unsigned long long	total_errors;
> >  	bool			moveon = true;
> >  	bool			ismnt;
> >  	int			c;
> > @@ -692,22 +718,8 @@ _("%s: Not a XFS mount point or block device.\n"),
> >  		ctx.runtime_errors++;
> >  
> >  out:
> > -	total_errors = ctx.errors_found + ctx.runtime_errors;
> > -	if (ctx.need_repair)
> > -		repairstr = _("  Unmount and run xfs_repair.");
> > -	if (total_errors && ctx.warnings_found)
> > -		fprintf(stderr,
> > -_("%s: %llu errors and %llu warnings found.%s\n"),
> > -			ctx.mntpoint, total_errors, ctx.warnings_found,
> > -			repairstr);
> > -	else if (total_errors && ctx.warnings_found == 0)
> > -		fprintf(stderr,
> > -_("%s: %llu errors found.%s\n"),
> > -			ctx.mntpoint, total_errors, repairstr);
> > -	else if (total_errors == 0 && ctx.warnings_found)
> > -		fprintf(stderr,
> > -_("%s: %llu warnings found.\n"),
> > -			ctx.mntpoint, ctx.warnings_found);
> > +	report_outcome(&ctx);
> > +
> >  	if (ctx.errors_found) {
> >  		if (ctx.error_action == ERRORS_SHUTDOWN)
> >  			xfs_shutdown_fs(&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

Re: [PATCH 1/7] xfs_scrub: log operational messages when interactive

[Darrick J. Wong]

On Mon, Feb 12, 2018 at 02:47:14PM -0600, Eric Sandeen wrote:
> On 2/5/18 5:22 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Record the output of an interactive session in the system log so that
> > future support requests can get a better picture of what happened.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I really want to log things, but I'm conflicted about spamming syslog.
> I'm wondering if a generic /var/log/xfs.log would be good, and it could
> eventually log all xfs-related administrative actions and outcomes with
> a libfrog library helper... I don't know how non-syslog log files are
> handled in general, does everybody just roll their own?

Evidently rsyslog actually can filter things out to a separate file.
For example, /etc/rsyslog.d/20-ufw.conf:

# Log kernel generated UFW log messages to file
:msg,contains,"[UFW " /var/log/ufw.log

# Uncomment the following to stop logging anything that matches the last
# rule.  Doing this will stop logging kernel generated UFW log messages
# to the file normally containing kern.* messages (eg,
# /var/log/kern.log)
#& stop

Therefore, it looks as though we actually /can/ use the regular syslog
calls from various xfs utilities and (if rsyslog is up) forward the
output to a place where support can capture it later.

--D

> 
> I'm inclined to leave this one out of 4.15 for now while I/we think
> about the big picture here.
> 
> -Eric
> 
> > ---
> >  scrub/common.c |   37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > 
> > diff --git a/scrub/common.c b/scrub/common.c
> > index 17c3699..672f286 100644
> > --- a/scrub/common.c
> > +++ b/scrub/common.c
> > @@ -21,6 +21,7 @@
> >  #include <pthread.h>
> >  #include <stdbool.h>
> >  #include <sys/statvfs.h>
> > +#include <syslog.h>
> >  #include "platform_defs.h"
> >  #include "xfs.h"
> >  #include "xfs_fs.h"
> > @@ -29,6 +30,8 @@
> >  #include "common.h"
> >  #include "progress.h"
> >  
> > +extern char		*progname;
> > +
> >  /*
> >   * Reporting Status to the Console
> >   *
> > @@ -64,6 +67,12 @@ static const char *err_str[] = {
> >  	[S_PREEN]	= "Optimized",
> >  };
> >  
> > +static int log_level[] = {
> > +	[S_ERROR]	= LOG_ERR,
> > +	[S_WARN]	= LOG_WARNING,
> > +	[S_INFO]	= LOG_INFO,
> > +};
> > +
> >  /* If stream is a tty, clear to end of line to clean up progress bar. */
> >  static inline const char *stream_start(FILE *stream)
> >  {
> > @@ -73,6 +82,8 @@ static inline const char *stream_start(FILE *stream)
> >  }
> >  
> >  /* Print a warning string and some warning text. */
> > +#define LOG_BUFSZ	4096
> > +#define LOGNAME_BUFSZ	256
> >  void
> >  __str_out(
> >  	struct scrub_ctx	*ctx,
> > @@ -110,6 +121,32 @@ __str_out(
> >  		va_end(args);
> >  	}
> >  
> > +	/* If we're running interactively, log the message to syslog too. */
> > +	if (isatty(fileno(stdin)) && !debug) {
> > +		char	logname[LOGNAME_BUFSZ];
> > +
> > +		snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname,
> > +				ctx->mntpoint);
> > +		openlog(logname, LOG_PID, LOG_DAEMON);
> > +
> > +		if (error) {
> > +			syslog(LOG_ERR, "%s: %s: %s.",
> > +					_(err_str[level]), descr,
> > +					strerror_r(error, buf, DESCR_BUFSZ));
> > +		} else {
> > +			char	buf[LOG_BUFSZ];
> > +			int	sz;
> > +
> > +			sz = snprintf(buf, LOG_BUFSZ, "%s: %s: ",
> > +					_(err_str[level]), descr);
> > +			va_start(args, format);
> > +			vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args);
> > +			va_end(args);
> > +			syslog(log_level[level], "%s", buf);
> > +		}
> > +		closelog();
> > +	}
> > +
> >  	if (debug)
> >  		fprintf(stream, _(" (%s line %d)"), file, line);
> >  	fprintf(stream, "\n");
> > 
> > --
> > 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

Re: [PATCH 1/7] xfs_scrub: log operational messages when interactive

[Jan Tulak]

On Wed, Feb 14, 2018 at 1:39 AM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Mon, Feb 12, 2018 at 02:47:14PM -0600, Eric Sandeen wrote:
>> On 2/5/18 5:22 PM, Darrick J. Wong wrote:
>> > From: Darrick J. Wong <darrick.wong@oracle.com>
>> >
>> > Record the output of an interactive session in the system log so that
>> > future support requests can get a better picture of what happened.
>> >
>> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> I really want to log things, but I'm conflicted about spamming syslog.
>> I'm wondering if a generic /var/log/xfs.log would be good, and it could
>> eventually log all xfs-related administrative actions and outcomes with
>> a libfrog library helper... I don't know how non-syslog log files are
>> handled in general, does everybody just roll their own?
>
> Evidently rsyslog actually can filter things out to a separate file.
> For example, /etc/rsyslog.d/20-ufw.conf:
>
> # Log kernel generated UFW log messages to file
> :msg,contains,"[UFW " /var/log/ufw.log
>
> # Uncomment the following to stop logging anything that matches the last
> # rule.  Doing this will stop logging kernel generated UFW log messages
> # to the file normally containing kern.* messages (eg,
> # /var/log/kern.log)
> #& stop
>
> Therefore, it looks as though we actually /can/ use the regular syslog
> calls from various xfs utilities and (if rsyslog is up) forward the
> output to a place where support can capture it later.
>

I wanted to join Eric in this, but I didn't know about the filter
rules, thanks for something new to learn. With such a rule, I'm all
for syslog, and I'm attaching my first humble reviewed-by. :-)

Cheers,
Jan

> --D
>
>>
>> I'm inclined to leave this one out of 4.15 for now while I/we think
>> about the big picture here.
>>
>> -Eric
>>
>> > ---
>> >  scrub/common.c |   37 +++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 37 insertions(+)
>> >
>> >
>> > diff --git a/scrub/common.c b/scrub/common.c
>> > index 17c3699..672f286 100644
>> > --- a/scrub/common.c
>> > +++ b/scrub/common.c
>> > @@ -21,6 +21,7 @@
>> >  #include <pthread.h>
>> >  #include <stdbool.h>
>> >  #include <sys/statvfs.h>
>> > +#include <syslog.h>
>> >  #include "platform_defs.h"
>> >  #include "xfs.h"
>> >  #include "xfs_fs.h"
>> > @@ -29,6 +30,8 @@
>> >  #include "common.h"
>> >  #include "progress.h"
>> >
>> > +extern char                *progname;
>> > +
>> >  /*
>> >   * Reporting Status to the Console
>> >   *
>> > @@ -64,6 +67,12 @@ static const char *err_str[] = {
>> >     [S_PREEN]       = "Optimized",
>> >  };
>> >
>> > +static int log_level[] = {
>> > +   [S_ERROR]       = LOG_ERR,
>> > +   [S_WARN]        = LOG_WARNING,
>> > +   [S_INFO]        = LOG_INFO,
>> > +};
>> > +
>> >  /* If stream is a tty, clear to end of line to clean up progress bar. */
>> >  static inline const char *stream_start(FILE *stream)
>> >  {
>> > @@ -73,6 +82,8 @@ static inline const char *stream_start(FILE *stream)
>> >  }
>> >
>> >  /* Print a warning string and some warning text. */
>> > +#define LOG_BUFSZ  4096
>> > +#define LOGNAME_BUFSZ      256
>> >  void
>> >  __str_out(
>> >     struct scrub_ctx        *ctx,
>> > @@ -110,6 +121,32 @@ __str_out(
>> >             va_end(args);
>> >     }
>> >
>> > +   /* If we're running interactively, log the message to syslog too. */
>> > +   if (isatty(fileno(stdin)) && !debug) {
>> > +           char    logname[LOGNAME_BUFSZ];
>> > +
>> > +           snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname,
>> > +                           ctx->mntpoint);
>> > +           openlog(logname, LOG_PID, LOG_DAEMON);
>> > +
>> > +           if (error) {
>> > +                   syslog(LOG_ERR, "%s: %s: %s.",
>> > +                                   _(err_str[level]), descr,
>> > +                                   strerror_r(error, buf, DESCR_BUFSZ));
>> > +           } else {
>> > +                   char    buf[LOG_BUFSZ];
>> > +                   int     sz;
>> > +
>> > +                   sz = snprintf(buf, LOG_BUFSZ, "%s: %s: ",
>> > +                                   _(err_str[level]), descr);
>> > +                   va_start(args, format);
>> > +                   vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args);
>> > +                   va_end(args);
>> > +                   syslog(log_level[level], "%s", buf);
>> > +           }
>> > +           closelog();
>> > +   }
>> > +
>> >     if (debug)
>> >             fprintf(stream, _(" (%s line %d)"), file, line);
>> >     fprintf(stream, "\n");
>> >
>> > --
>> > 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
> --
> 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

Re: [PATCH 1/7] xfs_scrub: log operational messages when interactive

[Jan Tulak]

On Tue, Feb 6, 2018 at 12:22 AM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Record the output of an interactive session in the system log so that
> future support requests can get a better picture of what happened.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Jan Tulak <jtulak@redhat.com>

> ---
>  scrub/common.c |   37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
>
> diff --git a/scrub/common.c b/scrub/common.c
> index 17c3699..672f286 100644
> --- a/scrub/common.c
> +++ b/scrub/common.c
> @@ -21,6 +21,7 @@
>  #include <pthread.h>
>  #include <stdbool.h>
>  #include <sys/statvfs.h>
> +#include <syslog.h>
>  #include "platform_defs.h"
>  #include "xfs.h"
>  #include "xfs_fs.h"
> @@ -29,6 +30,8 @@
>  #include "common.h"
>  #include "progress.h"
>
> +extern char            *progname;
> +
>  /*
>   * Reporting Status to the Console
>   *
> @@ -64,6 +67,12 @@ static const char *err_str[] = {
>         [S_PREEN]       = "Optimized",
>  };
>
> +static int log_level[] = {
> +       [S_ERROR]       = LOG_ERR,
> +       [S_WARN]        = LOG_WARNING,
> +       [S_INFO]        = LOG_INFO,
> +};
> +
>  /* If stream is a tty, clear to end of line to clean up progress bar. */
>  static inline const char *stream_start(FILE *stream)
>  {
> @@ -73,6 +82,8 @@ static inline const char *stream_start(FILE *stream)
>  }
>
>  /* Print a warning string and some warning text. */
> +#define LOG_BUFSZ      4096
> +#define LOGNAME_BUFSZ  256
>  void
>  __str_out(
>         struct scrub_ctx        *ctx,
> @@ -110,6 +121,32 @@ __str_out(
>                 va_end(args);
>         }
>
> +       /* If we're running interactively, log the message to syslog too. */
> +       if (isatty(fileno(stdin)) && !debug) {
> +               char    logname[LOGNAME_BUFSZ];
> +
> +               snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname,
> +                               ctx->mntpoint);
> +               openlog(logname, LOG_PID, LOG_DAEMON);
> +
> +               if (error) {
> +                       syslog(LOG_ERR, "%s: %s: %s.",
> +                                       _(err_str[level]), descr,
> +                                       strerror_r(error, buf, DESCR_BUFSZ));
> +               } else {
> +                       char    buf[LOG_BUFSZ];
> +                       int     sz;
> +
> +                       sz = snprintf(buf, LOG_BUFSZ, "%s: %s: ",
> +                                       _(err_str[level]), descr);
> +                       va_start(args, format);
> +                       vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args);
> +                       va_end(args);
> +                       syslog(log_level[level], "%s", buf);
> +               }
> +               closelog();
> +       }
> +
>         if (debug)
>                 fprintf(stream, _(" (%s line %d)"), file, line);
>         fprintf(stream, "\n");
>
> --
> 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

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

[Jan Tulak]

On Mon, Feb 12, 2018 at 9:49 PM, Eric Sandeen <sandeen@sandeen.net> 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.
>
> Anyway, for the changes here:
>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>

I read it as "do check, but do not repair". The previous "dry run"
description was unclear about that (at least from my understanding of
"dry run" phrase), but the changed version seems clear to me.

Reviewed-by: Jan Tulak <jtulak@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

Re: [PATCH 3/7] xfs_scrub: classify lack of ioctl support as a runtime error

[Jan Tulak]

On Tue, Feb 6, 2018 at 12:22 AM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> If the kernel doesn't have the SCRUB_METADATA ioctl that's a runtime
> error, not a fs error.  Account it as such.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Jan Tulak <jtulak@redhat.com>

> ---
>  scrub/phase1.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
>
> diff --git a/scrub/phase1.c b/scrub/phase1.c
> index af93d0f..82c8022 100644
> --- a/scrub/phase1.c
> +++ b/scrub/phase1.c
> @@ -174,14 +174,14 @@ _("Does not appear to be an XFS filesystem!"));
>             !xfs_can_scrub_bmap(ctx) || !xfs_can_scrub_dir(ctx) ||
>             !xfs_can_scrub_attr(ctx) || !xfs_can_scrub_symlink(ctx) ||
>             !xfs_can_scrub_parent(ctx)) {
> -               str_error(ctx, ctx->mntpoint,
> +               str_info(ctx, ctx->mntpoint,
>  _("Kernel metadata scrubbing facility is not available."));
>                 return false;
>         }
>
>         /* Do we need kernel-assisted metadata repair? */
>         if (ctx->mode != SCRUB_MODE_DRY_RUN && !xfs_can_repair(ctx)) {
> -               str_error(ctx, ctx->mntpoint,
> +               str_info(ctx, ctx->mntpoint,
>  _("Kernel metadata repair facility is not available.  Use -n to scrub."));
>                 return false;
>         }
>
> --
> 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

Re: [PATCH 4/7] xfs_scrub: reclassify runtime errors

[Jan Tulak]

On Mon, Feb 12, 2018 at 9:52 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>
>
> On 2/5/18 5:22 PM, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> If the program encounters runtime errors, these should be noted as
>> information.  Because these errors abort the execution flow (which is
>> counted as a runtime error), we need only call str_info to log the
>> event.
>>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> Ok.  Again I wonder if eventually str_error should turn into
> scrub_log_fs_corruption() and str_info into scrub_log_runtime_error()
> or something...
>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

This sounds like a good idea, I would not guess that this is the
meaning of the functions unless I dig in them (and who always checks
what exactly logging functions do?). But for this patch, it is ok.

Reviewed-by: Jan Tulak <jtulak@redhat.com>

>
>> ---
>>  scrub/fscounters.c  |    4 ++--
>>  scrub/inodes.c      |    4 ++--
>>  scrub/phase1.c      |    8 ++++----
>>  scrub/phase2.c      |    6 +++---
>>  scrub/phase3.c      |    2 +-
>>  scrub/phase6.c      |    2 +-
>>  scrub/read_verify.c |    2 +-
>>  scrub/scrub.c       |    8 ++++----
>>  scrub/spacemap.c    |    8 ++++----
>>  scrub/vfs.c         |    6 +++---
>>  10 files changed, 25 insertions(+), 25 deletions(-)
>>
>>
>> diff --git a/scrub/fscounters.c b/scrub/fscounters.c
>> index 4294bf3..ecdf4c6 100644
>> --- a/scrub/fscounters.c
>> +++ b/scrub/fscounters.c
>> @@ -139,14 +139,14 @@ xfs_count_all_inodes(
>>                       scrub_nproc_workqueue(ctx));
>>       if (ret) {
>>               moveon = false;
>> -             str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
>> +             str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>>               goto out_free;
>>       }
>>       for (agno = 0; agno < ctx->geo.agcount; agno++) {
>>               ret = workqueue_add(&wq, xfs_count_ag_inodes, agno, ci);
>>               if (ret) {
>>                       moveon = false;
>> -                     str_error(ctx, ctx->mntpoint,
>> +                     str_info(ctx, ctx->mntpoint,
>>  _("Could not queue AG %u icount work."), agno);
>>                       break;
>>               }
>> diff --git a/scrub/inodes.c b/scrub/inodes.c
>> index 23ec704..57b773e 100644
>> --- a/scrub/inodes.c
>> +++ b/scrub/inodes.c
>> @@ -266,7 +266,7 @@ xfs_scan_all_inodes(
>>       ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
>>                       scrub_nproc_workqueue(ctx));
>>       if (ret) {
>> -             str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
>> +             str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>>               return false;
>>       }
>>
>> @@ -274,7 +274,7 @@ xfs_scan_all_inodes(
>>               ret = workqueue_add(&wq, xfs_scan_ag_inodes, agno, &si);
>>               if (ret) {
>>                       si.moveon = false;
>> -                     str_error(ctx, ctx->mntpoint,
>> +                     str_info(ctx, ctx->mntpoint,
>>  _("Could not queue AG %u bulkstat work."), agno);
>>                       break;
>>               }
>> diff --git a/scrub/phase1.c b/scrub/phase1.c
>> index 82c8022..6cd5442 100644
>> --- a/scrub/phase1.c
>> +++ b/scrub/phase1.c
>> @@ -134,7 +134,7 @@ _("Must be root to run scrub."));
>>       }
>>
>>       if (!platform_test_xfs_fd(ctx->mnt_fd)) {
>> -             str_error(ctx, ctx->mntpoint,
>> +             str_info(ctx, ctx->mntpoint,
>>  _("Does not appear to be an XFS filesystem!"));
>>               return false;
>>       }
>> @@ -191,7 +191,7 @@ _("Kernel metadata repair facility is not available.  Use -n to scrub."));
>>       errno = 0;
>>       fsp = fs_table_lookup(ctx->mntpoint, FS_MOUNT_POINT);
>>       if (!fsp) {
>> -             str_error(ctx, ctx->mntpoint,
>> +             str_info(ctx, ctx->mntpoint,
>>  _("Unable to find XFS information."));
>>               return false;
>>       }
>> @@ -199,12 +199,12 @@ _("Unable to find XFS information."));
>>
>>       /* Did we find the log and rt devices, if they're present? */
>>       if (ctx->geo.logstart == 0 && ctx->fsinfo.fs_log == NULL) {
>> -             str_error(ctx, ctx->mntpoint,
>> +             str_info(ctx, ctx->mntpoint,
>>  _("Unable to find log device path."));
>>               return false;
>>       }
>>       if (ctx->geo.rtblocks && ctx->fsinfo.fs_rt == NULL) {
>> -             str_error(ctx, ctx->mntpoint,
>> +             str_info(ctx, ctx->mntpoint,
>>  _("Unable to find realtime device path."));
>>               return false;
>>       }
>> diff --git a/scrub/phase2.c b/scrub/phase2.c
>> index 32e2752..edf66df 100644
>> --- a/scrub/phase2.c
>> +++ b/scrub/phase2.c
>> @@ -94,7 +94,7 @@ xfs_scan_metadata(
>>       ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
>>                       scrub_nproc_workqueue(ctx));
>>       if (ret) {
>> -             str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
>> +             str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>>               return false;
>>       }
>>
>> @@ -111,7 +111,7 @@ xfs_scan_metadata(
>>               ret = workqueue_add(&wq, xfs_scan_ag_metadata, agno, &moveon);
>>               if (ret) {
>>                       moveon = false;
>> -                     str_error(ctx, ctx->mntpoint,
>> +                     str_info(ctx, ctx->mntpoint,
>>  _("Could not queue AG %u scrub work."), agno);
>>                       goto out;
>>               }
>> @@ -123,7 +123,7 @@ _("Could not queue AG %u scrub work."), agno);
>>       ret = workqueue_add(&wq, xfs_scan_fs_metadata, 0, &moveon);
>>       if (ret) {
>>               moveon = false;
>> -             str_error(ctx, ctx->mntpoint,
>> +             str_info(ctx, ctx->mntpoint,
>>  _("Could not queue filesystem scrub work."));
>>               goto out;
>>       }
>> diff --git a/scrub/phase3.c b/scrub/phase3.c
>> index f4117b0..a0ee5d9 100644
>> --- a/scrub/phase3.c
>> +++ b/scrub/phase3.c
>> @@ -137,7 +137,7 @@ xfs_scan_inodes(
>>       ictx.moveon = true;
>>       ictx.icount = ptcounter_init(scrub_nproc(ctx));
>>       if (!ictx.icount) {
>> -             str_error(ctx, ctx->mntpoint, _("Could not create counter."));
>> +             str_info(ctx, ctx->mntpoint, _("Could not create counter."));
>>               return false;
>>       }
>>
>> diff --git a/scrub/phase6.c b/scrub/phase6.c
>> index 9795bf3..f985950 100644
>> --- a/scrub/phase6.c
>> +++ b/scrub/phase6.c
>> @@ -485,7 +485,7 @@ xfs_scan_blocks(
>>                       xfs_check_rmap_ioerr, disk_heads(ctx->datadev));
>>       if (!ve.readverify) {
>>               moveon = false;
>> -             str_error(ctx, ctx->mntpoint,
>> +             str_info(ctx, ctx->mntpoint,
>>  _("Could not create media verifier."));
>>               goto out_rbad;
>>       }
>> diff --git a/scrub/read_verify.c b/scrub/read_verify.c
>> index e816688..ae2e85f 100644
>> --- a/scrub/read_verify.c
>> +++ b/scrub/read_verify.c
>> @@ -187,7 +187,7 @@ read_verify_queue(
>>
>>       ret = workqueue_add(&rvp->wq, read_verify, 0, tmp);
>>       if (ret) {
>> -             str_error(rvp->ctx, rvp->ctx->mntpoint,
>> +             str_info(rvp->ctx, rvp->ctx->mntpoint,
>>  _("Could not queue read-verify work."));
>>               free(tmp);
>>               return false;
>> diff --git a/scrub/scrub.c b/scrub/scrub.c
>> index bc0e2f0..ff5357c 100644
>> --- a/scrub/scrub.c
>> +++ b/scrub/scrub.c
>> @@ -219,7 +219,7 @@ xfs_check_metadata(
>>                       return CHECK_DONE;
>>               case ESHUTDOWN:
>>                       /* FS already crashed, give up. */
>> -                     str_error(ctx, buf,
>> +                     str_info(ctx, buf,
>>  _("Filesystem is shut down, aborting."));
>>                       return CHECK_ABORT;
>>               case EIO:
>> @@ -235,7 +235,7 @@ _("Filesystem is shut down, aborting."));
>>                        * The first two should never escape the kernel,
>>                        * and the other two should be reported via sm_flags.
>>                        */
>> -                     str_error(ctx, buf,
>> +                     str_info(ctx, buf,
>>  _("Kernel bug!  errno=%d"), code);
>>                       /* fall through */
>>               default:
>> @@ -568,7 +568,7 @@ __xfs_scrub_test(
>>       if (debug_tweak_on("XFS_SCRUB_NO_KERNEL"))
>>               return false;
>>       if (debug_tweak_on("XFS_SCRUB_FORCE_REPAIR") && !injected) {
>> -             str_error(ctx, "XFS_SCRUB_FORCE_REPAIR", "Not supported.");
>> +             str_info(ctx, "XFS_SCRUB_FORCE_REPAIR", "Not supported.");
>>               return false;
>>       }
>>
>> @@ -726,7 +726,7 @@ _("Filesystem is busy, deferring repair."));
>>                       return CHECK_RETRY;
>>               case ESHUTDOWN:
>>                       /* Filesystem is already shut down, abort. */
>> -                     str_error(ctx, buf,
>> +                     str_info(ctx, buf,
>>  _("Filesystem is shut down, aborting."));
>>                       return CHECK_ABORT;
>>               case ENOTTY:
>> diff --git a/scrub/spacemap.c b/scrub/spacemap.c
>> index 2dc6e2b..f631913 100644
>> --- a/scrub/spacemap.c
>> +++ b/scrub/spacemap.c
>> @@ -217,7 +217,7 @@ xfs_scan_all_spacemaps(
>>       ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
>>                       scrub_nproc_workqueue(ctx));
>>       if (ret) {
>> -             str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
>> +             str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>>               return false;
>>       }
>>       if (ctx->fsinfo.fs_rt) {
>> @@ -225,7 +225,7 @@ xfs_scan_all_spacemaps(
>>                               ctx->geo.agcount + 1, &sbx);
>>               if (ret) {
>>                       sbx.moveon = false;
>> -                     str_error(ctx, ctx->mntpoint,
>> +                     str_info(ctx, ctx->mntpoint,
>>  _("Could not queue rtdev fsmap work."));
>>                       goto out;
>>               }
>> @@ -235,7 +235,7 @@ _("Could not queue rtdev fsmap work."));
>>                               ctx->geo.agcount + 2, &sbx);
>>               if (ret) {
>>                       sbx.moveon = false;
>> -                     str_error(ctx, ctx->mntpoint,
>> +                     str_info(ctx, ctx->mntpoint,
>>  _("Could not queue logdev fsmap work."));
>>                       goto out;
>>               }
>> @@ -244,7 +244,7 @@ _("Could not queue logdev fsmap work."));
>>               ret = workqueue_add(&wq, xfs_scan_ag_blocks, agno, &sbx);
>>               if (ret) {
>>                       sbx.moveon = false;
>> -                     str_error(ctx, ctx->mntpoint,
>> +                     str_info(ctx, ctx->mntpoint,
>>  _("Could not queue AG %u fsmap work."), agno);
>>                       break;
>>               }
>> diff --git a/scrub/vfs.c b/scrub/vfs.c
>> index 573a2d0..0c5b353 100644
>> --- a/scrub/vfs.c
>> +++ b/scrub/vfs.c
>> @@ -145,7 +145,7 @@ scan_fs_dir(
>>                       pthread_mutex_unlock(&sft->lock);
>>                       error = workqueue_add(wq, scan_fs_dir, 0, new_sftd);
>>                       if (error) {
>> -                             str_error(ctx, ctx->mntpoint,
>> +                             str_info(ctx, ctx->mntpoint,
>>  _("Could not queue subdirectory scan work."));
>>                               sft->moveon = false;
>>                               break;
>> @@ -203,12 +203,12 @@ scan_fs_tree(
>>       ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
>>                       scrub_nproc_workqueue(ctx));
>>       if (ret) {
>> -             str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
>> +             str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
>>               goto out_free;
>>       }
>>       ret = workqueue_add(&wq, scan_fs_dir, 0, sftd);
>>       if (ret) {
>> -             str_error(ctx, ctx->mntpoint,
>> +             str_info(ctx, ctx->mntpoint,
>>  _("Could not queue directory scan work."));
>>               goto out_free;
>>       }
>>
>> --
>> 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

Re: [PATCH 5/7] xfs_scrub: reclassify some of the warning messages

[Jan Tulak]

On Mon, Feb 12, 2018 at 9:53 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>
>
> On 2/5/18 5:22 PM, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Some of the warning messages are actually runtime errors in optional
>> components, so turn them into informational messages.
>>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Reviewed-by: Jan Tulak <jtulak@redhat.com>
>
>> ---
>>  scrub/inodes.c |    6 +++---
>>  scrub/phase6.c |    4 ++--
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>>
>> diff --git a/scrub/inodes.c b/scrub/inodes.c
>> index 57b773e..744b003 100644
>> --- a/scrub/inodes.c
>> +++ b/scrub/inodes.c
>> @@ -157,7 +157,7 @@ xfs_iterate_inodes_range(
>>               bulkreq.icount = inogrp.xi_alloccount;
>>               error = ioctl(ctx->mnt_fd, XFS_IOC_FSBULKSTAT, &bulkreq);
>>               if (error)
>> -                     str_warn(ctx, descr, "%s", strerror_r(errno,
>> +                     str_info(ctx, descr, "%s", strerror_r(errno,
>>                                               buf, DESCR_BUFSZ));
>>
>>               xfs_iterate_inodes_range_check(ctx, &inogrp, bstat);
>> @@ -181,8 +181,8 @@ xfs_iterate_inodes_range(
>>                               }
>>                               snprintf(idescr, DESCR_BUFSZ, "inode %"PRIu64,
>>                                               (uint64_t)bs->bs_ino);
>> -                             str_warn(ctx, idescr, "%s", strerror_r(error,
>> -                                             buf, DESCR_BUFSZ));
>> +                             str_info(ctx, idescr,
>> +_("Changed too many times during scan; giving up."));
>>                               break;
>>                       case XFS_ITERATE_INODES_ABORT:
>>                               error = 0;
>> diff --git a/scrub/phase6.c b/scrub/phase6.c
>> index f985950..e255eef 100644
>> --- a/scrub/phase6.c
>> +++ b/scrub/phase6.c
>> @@ -188,7 +188,6 @@ xfs_report_verify_inode(
>>       void                            *arg)
>>  {
>>       char                            descr[DESCR_BUFSZ];
>> -     char                            buf[DESCR_BUFSZ];
>>       bool                            moveon;
>>       int                             fd;
>>       int                             error;
>> @@ -209,7 +208,8 @@ xfs_report_verify_inode(
>>               if (error == ESTALE)
>>                       return error;
>>
>> -             str_warn(ctx, descr, "%s", strerror_r(error, buf, DESCR_BUFSZ));
>> +             str_info(ctx, descr,
>> +_("Disappeared during read error reporting."));
>>               return error;
>>       }
>>
>>
>> --
>> 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

Re: [PATCH 6/7] xfs_scrub: always init phase information

[Jan Tulak]

On Mon, Feb 12, 2018 at 9:53 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>
>
> On 2/5/18 5:23 PM, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Make sure we initialize the overall phase state before we start
>> executing any code that can end up in the report-status-and-exit paths.
>> Otherwise if debugging is turned on we get garbage io/cpu stat reports.
>>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Reviewed-by: Jan Tulak <jtulak@redhat.com>
>
>> ---
>>  scrub/xfs_scrub.c |   10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>>
>> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
>> index 6efcf77..89b7fa0 100644
>> --- a/scrub/xfs_scrub.c
>> +++ b/scrub/xfs_scrub.c
>> @@ -618,6 +618,11 @@ main(
>>       if (getenv("SERVICE_MODE"))
>>               is_service = true;
>>
>> +     /* Initialize overall phase stats. */
>> +     moveon = phase_start(&all_pi, 0, NULL);
>> +     if (!moveon)
>> +             return SCRUB_RET_OPERROR;
>> +
>>       /* Find the mount record for the passed-in argument. */
>>       if (stat(argv[optind], &ctx.mnt_sb) < 0) {
>>               fprintf(stderr,
>> @@ -641,11 +646,6 @@ main(
>>                       mtab = _PATH_MOUNTED;
>>       }
>>
>> -     /* Initialize overall phase stats. */
>> -     moveon = phase_start(&all_pi, 0, NULL);
>> -     if (!moveon)
>> -             goto out;
>> -
>>       ismnt = find_mountpoint(mtab, &ctx);
>>       if (!ismnt) {
>>               fprintf(stderr,
>>
>> --
>> 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

Re: [PATCH 7/7] xfs_scrub: refactor outcome display into a separate helper

[Jan Tulak]

On Mon, Feb 12, 2018 at 10:06 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> On 2/5/18 5:23 PM, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Move all the printing of the scrub outcome into a separate helper to
>> declutter the main function.
>>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>
>
> This might get a bit long, no?
>
> "/path/to/filesystem: errors found 16384; warnings found 16.  Unmount and run xfs_repair."
>
> Can we put the "Unmount and run xfs_repair" on its own line?
> That'd make it stand out more, as well.  </nitpick>

I agree, but that's really just a detail.

Reviewed-by: Jan Tulak <jtulak@redhat.com>

>
>
>> ---
>>  scrub/xfs_scrub.c |   48 ++++++++++++++++++++++++++++++------------------
>>  1 file changed, 30 insertions(+), 18 deletions(-)
>>
>>
>> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
>> index 89b7fa0..fdd35df 100644
>> --- a/scrub/xfs_scrub.c
>> +++ b/scrub/xfs_scrub.c
>> @@ -493,6 +493,34 @@ _("Scrub aborted after phase %d."),
>>       return moveon;
>>  }
>>
>> +static void
>> +report_outcome(
>> +     struct scrub_ctx        *ctx)
>> +{
>> +     unsigned long long      total_errors;
>> +
>> +     total_errors = ctx->errors_found + ctx->runtime_errors;
>> +
>> +     if (total_errors == 0 && ctx->warnings_found == 0)
>> +             return;
>> +
>> +     if (total_errors == 0)
>> +             fprintf(stderr, _("%s: warnings found: %llu."), ctx->mntpoint,
>> +                             ctx->warnings_found);
>> +     else if (ctx->warnings_found == 0)
>> +             fprintf(stderr, _("%s: errors found: %llu."), ctx->mntpoint,
>> +                             total_errors);
>> +     else
>> +             fprintf(stderr, _("%s: errors found: %llu; warnings found: %llu."),
>> +                             ctx->mntpoint, total_errors,
>> +                             ctx->warnings_found);
>> +
>> +     if (ctx->need_repair)
>> +             fprintf(stderr, "  %s\n", _("Unmount and run xfs_repair."));
>> +     else
>> +             fprintf(stderr, "\n");
>> +}
>> +
>>  int
>>  main(
>>       int                     argc,
>> @@ -501,9 +529,7 @@ main(
>>       struct scrub_ctx        ctx = {0};
>>       struct phase_rusage     all_pi;
>>       char                    *mtab = NULL;
>> -     char                    *repairstr = "";
>>       FILE                    *progress_fp = NULL;
>> -     unsigned long long      total_errors;
>>       bool                    moveon = true;
>>       bool                    ismnt;
>>       int                     c;
>> @@ -692,22 +718,8 @@ _("%s: Not a XFS mount point or block device.\n"),
>>               ctx.runtime_errors++;
>>
>>  out:
>> -     total_errors = ctx.errors_found + ctx.runtime_errors;
>> -     if (ctx.need_repair)
>> -             repairstr = _("  Unmount and run xfs_repair.");
>> -     if (total_errors && ctx.warnings_found)
>> -             fprintf(stderr,
>> -_("%s: %llu errors and %llu warnings found.%s\n"),
>> -                     ctx.mntpoint, total_errors, ctx.warnings_found,
>> -                     repairstr);
>> -     else if (total_errors && ctx.warnings_found == 0)
>> -             fprintf(stderr,
>> -_("%s: %llu errors found.%s\n"),
>> -                     ctx.mntpoint, total_errors, repairstr);
>> -     else if (total_errors == 0 && ctx.warnings_found)
>> -             fprintf(stderr,
>> -_("%s: %llu warnings found.\n"),
>> -                     ctx.mntpoint, ctx.warnings_found);
>> +     report_outcome(&ctx);
>> +
>>       if (ctx.errors_found) {
>>               if (ctx.error_action == ERRORS_SHUTDOWN)
>>                       xfs_shutdown_fs(&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

Re: [PATCH 1/7] xfs_scrub: log operational messages when interactive

[Darrick J. Wong]

On Wed, Feb 14, 2018 at 08:48:09AM +0100, Jan Tulak wrote:
> On Wed, Feb 14, 2018 at 1:39 AM, Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
> > On Mon, Feb 12, 2018 at 02:47:14PM -0600, Eric Sandeen wrote:
> >> On 2/5/18 5:22 PM, Darrick J. Wong wrote:
> >> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >> >
> >> > Record the output of an interactive session in the system log so that
> >> > future support requests can get a better picture of what happened.
> >> >
> >> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >>
> >> I really want to log things, but I'm conflicted about spamming syslog.
> >> I'm wondering if a generic /var/log/xfs.log would be good, and it could
> >> eventually log all xfs-related administrative actions and outcomes with
> >> a libfrog library helper... I don't know how non-syslog log files are
> >> handled in general, does everybody just roll their own?
> >
> > Evidently rsyslog actually can filter things out to a separate file.
> > For example, /etc/rsyslog.d/20-ufw.conf:
> >
> > # Log kernel generated UFW log messages to file
> > :msg,contains,"[UFW " /var/log/ufw.log
> >
> > # Uncomment the following to stop logging anything that matches the last
> > # rule.  Doing this will stop logging kernel generated UFW log messages
> > # to the file normally containing kern.* messages (eg,
> > # /var/log/kern.log)
> > #& stop
> >
> > Therefore, it looks as though we actually /can/ use the regular syslog
> > calls from various xfs utilities and (if rsyslog is up) forward the
> > output to a place where support can capture it later.
> >
> 
> I wanted to join Eric in this, but I didn't know about the filter
> rules, thanks for something new to learn. With such a rule, I'm all
> for syslog, and I'm attaching my first humble reviewed-by. :-)

The downside is that it also doesn't work, at least on the ubuntu 16.04
rsyslog (which is curious, since that's also where I got the 20-ufw.conf
file) so someone with better rsyslog-conf-fu will have to help us out
here.

--D

> Cheers,
> Jan
> 
> > --D
> >
> >>
> >> I'm inclined to leave this one out of 4.15 for now while I/we think
> >> about the big picture here.
> >>
> >> -Eric
> >>
> >> > ---
> >> >  scrub/common.c |   37 +++++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 37 insertions(+)
> >> >
> >> >
> >> > diff --git a/scrub/common.c b/scrub/common.c
> >> > index 17c3699..672f286 100644
> >> > --- a/scrub/common.c
> >> > +++ b/scrub/common.c
> >> > @@ -21,6 +21,7 @@
> >> >  #include <pthread.h>
> >> >  #include <stdbool.h>
> >> >  #include <sys/statvfs.h>
> >> > +#include <syslog.h>
> >> >  #include "platform_defs.h"
> >> >  #include "xfs.h"
> >> >  #include "xfs_fs.h"
> >> > @@ -29,6 +30,8 @@
> >> >  #include "common.h"
> >> >  #include "progress.h"
> >> >
> >> > +extern char                *progname;
> >> > +
> >> >  /*
> >> >   * Reporting Status to the Console
> >> >   *
> >> > @@ -64,6 +67,12 @@ static const char *err_str[] = {
> >> >     [S_PREEN]       = "Optimized",
> >> >  };
> >> >
> >> > +static int log_level[] = {
> >> > +   [S_ERROR]       = LOG_ERR,
> >> > +   [S_WARN]        = LOG_WARNING,
> >> > +   [S_INFO]        = LOG_INFO,
> >> > +};
> >> > +
> >> >  /* If stream is a tty, clear to end of line to clean up progress bar. */
> >> >  static inline const char *stream_start(FILE *stream)
> >> >  {
> >> > @@ -73,6 +82,8 @@ static inline const char *stream_start(FILE *stream)
> >> >  }
> >> >
> >> >  /* Print a warning string and some warning text. */
> >> > +#define LOG_BUFSZ  4096
> >> > +#define LOGNAME_BUFSZ      256
> >> >  void
> >> >  __str_out(
> >> >     struct scrub_ctx        *ctx,
> >> > @@ -110,6 +121,32 @@ __str_out(
> >> >             va_end(args);
> >> >     }
> >> >
> >> > +   /* If we're running interactively, log the message to syslog too. */
> >> > +   if (isatty(fileno(stdin)) && !debug) {
> >> > +           char    logname[LOGNAME_BUFSZ];
> >> > +
> >> > +           snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname,
> >> > +                           ctx->mntpoint);
> >> > +           openlog(logname, LOG_PID, LOG_DAEMON);
> >> > +
> >> > +           if (error) {
> >> > +                   syslog(LOG_ERR, "%s: %s: %s.",
> >> > +                                   _(err_str[level]), descr,
> >> > +                                   strerror_r(error, buf, DESCR_BUFSZ));
> >> > +           } else {
> >> > +                   char    buf[LOG_BUFSZ];
> >> > +                   int     sz;
> >> > +
> >> > +                   sz = snprintf(buf, LOG_BUFSZ, "%s: %s: ",
> >> > +                                   _(err_str[level]), descr);
> >> > +                   va_start(args, format);
> >> > +                   vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args);
> >> > +                   va_end(args);
> >> > +                   syslog(log_level[level], "%s", buf);
> >> > +           }
> >> > +           closelog();
> >> > +   }
> >> > +
> >> >     if (debug)
> >> >             fprintf(stream, _(" (%s line %d)"), file, line);
> >> >     fprintf(stream, "\n");
> >> >
> >> > --
> >> > 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
> > --
> > 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

Re: [PATCH 1/7] xfs_scrub: log operational messages when interactive

[Darrick J. Wong]

On Mon, Feb 12, 2018 at 02:47:14PM -0600, Eric Sandeen wrote:
> On 2/5/18 5:22 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Record the output of an interactive session in the system log so that
> > future support requests can get a better picture of what happened.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I really want to log things, but I'm conflicted about spamming syslog.
> I'm wondering if a generic /var/log/xfs.log would be good, and it could
> eventually log all xfs-related administrative actions and outcomes with
> a libfrog library helper... I don't know how non-syslog log files are
> handled in general, does everybody just roll their own?
> 
> I'm inclined to leave this one out of 4.15 for now while I/we think
> about the big picture here.

New idea: we won't flood syslog with the gory details of everything that
happened.  Instead we syslog right after we've decided that yes we will
check this filesystem, and syslog again at the end to report what we
found.

--D

> -Eric
> 
> > ---
> >  scrub/common.c |   37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > 
> > diff --git a/scrub/common.c b/scrub/common.c
> > index 17c3699..672f286 100644
> > --- a/scrub/common.c
> > +++ b/scrub/common.c
> > @@ -21,6 +21,7 @@
> >  #include <pthread.h>
> >  #include <stdbool.h>
> >  #include <sys/statvfs.h>
> > +#include <syslog.h>
> >  #include "platform_defs.h"
> >  #include "xfs.h"
> >  #include "xfs_fs.h"
> > @@ -29,6 +30,8 @@
> >  #include "common.h"
> >  #include "progress.h"
> >  
> > +extern char		*progname;
> > +
> >  /*
> >   * Reporting Status to the Console
> >   *
> > @@ -64,6 +67,12 @@ static const char *err_str[] = {
> >  	[S_PREEN]	= "Optimized",
> >  };
> >  
> > +static int log_level[] = {
> > +	[S_ERROR]	= LOG_ERR,
> > +	[S_WARN]	= LOG_WARNING,
> > +	[S_INFO]	= LOG_INFO,
> > +};
> > +
> >  /* If stream is a tty, clear to end of line to clean up progress bar. */
> >  static inline const char *stream_start(FILE *stream)
> >  {
> > @@ -73,6 +82,8 @@ static inline const char *stream_start(FILE *stream)
> >  }
> >  
> >  /* Print a warning string and some warning text. */
> > +#define LOG_BUFSZ	4096
> > +#define LOGNAME_BUFSZ	256
> >  void
> >  __str_out(
> >  	struct scrub_ctx	*ctx,
> > @@ -110,6 +121,32 @@ __str_out(
> >  		va_end(args);
> >  	}
> >  
> > +	/* If we're running interactively, log the message to syslog too. */
> > +	if (isatty(fileno(stdin)) && !debug) {
> > +		char	logname[LOGNAME_BUFSZ];
> > +
> > +		snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname,
> > +				ctx->mntpoint);
> > +		openlog(logname, LOG_PID, LOG_DAEMON);
> > +
> > +		if (error) {
> > +			syslog(LOG_ERR, "%s: %s: %s.",
> > +					_(err_str[level]), descr,
> > +					strerror_r(error, buf, DESCR_BUFSZ));
> > +		} else {
> > +			char	buf[LOG_BUFSZ];
> > +			int	sz;
> > +
> > +			sz = snprintf(buf, LOG_BUFSZ, "%s: %s: ",
> > +					_(err_str[level]), descr);
> > +			va_start(args, format);
> > +			vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args);
> > +			va_end(args);
> > +			syslog(log_level[level], "%s", buf);
> > +		}
> > +		closelog();
> > +	}
> > +
> >  	if (debug)
> >  		fprintf(stream, _(" (%s line %d)"), file, line);
> >  	fprintf(stream, "\n");
> > 
> > --
> > 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

Re: [PATCH 1/7] xfs_scrub: log operational messages when interactive

[Eric Sandeen]

On 3/8/18 12:07 PM, Darrick J. Wong wrote:
> On Mon, Feb 12, 2018 at 02:47:14PM -0600, Eric Sandeen wrote:
>> On 2/5/18 5:22 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Record the output of an interactive session in the system log so that
>>> future support requests can get a better picture of what happened.
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> I really want to log things, but I'm conflicted about spamming syslog.
>> I'm wondering if a generic /var/log/xfs.log would be good, and it could
>> eventually log all xfs-related administrative actions and outcomes with
>> a libfrog library helper... I don't know how non-syslog log files are
>> handled in general, does everybody just roll their own?
>>
>> I'm inclined to leave this one out of 4.15 for now while I/we think
>> about the big picture here.
> 
> New idea: we won't flood syslog with the gory details of everything that
> happened.  Instead we syslog right after we've decided that yes we will
> check this filesystem, and syslog again at the end to report what we
> found.

Sounds good to me.

-Eric