[PATCH 00/16] xfsprogs: misc fixes, geometry refactoring

[PATCH 00/16] xfsprogs: misc fixes, geometry refactoring

[Darrick J. Wong]

Hi all,

Here's a bunch of patches for xfsprogs 4.16!

The first six patches fix compiler warnings, crasher bugs, and tweak
various things in xfs_scrub per early user and maintainer feedback.
Patch 7 creates a new xfs_db command to print transaction reservation
calculations so that they can be compared to the kernel's calculations.
Patch 8 enables repair to bypass inode fork verifiers when it's trying
to libxfs_iget an inode to reconnect it to lost+found, which should fix
some of the bug reports about repair failing to finish.  Patch 9 enables
LTO, which substantially reduces final executable sizes because we can
remove unnecessary code.

The seven patches at the end refactor geometry reporting in xfsprogs.
First we hoist the fsgeometry generation function to libxfs, then add
some libfrog functions to print fsgeometry structures.  Next we add a
command to xfs_db to print the geometry information of the loaded
filesystem.  Finally, we move online geometry reporting from growfs to
spaceman and refactor xfs_info to call spaceman or db depending on the
passed in command line argument.

This probably won't eat your data, and the branch[2] should apply
against for-next.

--D

[1] https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=djwong-devel

[PATCH 01/16] misc: fix gcc 7.3 warnings

[Darrick J. Wong]

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

Fix various compiler warnings that pop up in 7.3.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/bit.c        |    4 ++--
 repair/dinode.c |    1 +
 repair/scan.c   |    1 +
 3 files changed, 4 insertions(+), 2 deletions(-)


diff --git a/db/bit.c b/db/bit.c
index bf8d80e..a6d8c9f 100644
--- a/db/bit.c
+++ b/db/bit.c
@@ -111,11 +111,11 @@ getbitval(
 			/* handle endian swap here */
 #if __BYTE_ORDER == LITTLE_ENDIAN
 			if (i == 0 && signext && nbits < 64)
-				rval = -1LL << nbits;
+				rval = (~0ULL) << nbits;
 			rval |= 1ULL << (nbits - i - 1);
 #else
 			if ((i == (nbits - 1)) && signext && nbits < 64)
-				rval |= (-1LL << nbits);
+				rval |= ((~0ULL) << nbits);
 			rval |= 1ULL << (nbits - i - 1);
 #endif
 		}
diff --git a/repair/dinode.c b/repair/dinode.c
index 32cc769..07bcf80 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -525,6 +525,7 @@ _("data fork in rt inode %" PRIu64 " found metadata block %" PRIu64 " in rt bmap
 		case XR_E_INUSE:
 			if (pwe)
 				break;
+			/* fall through */
 		case XR_E_MULT:
 			set_rtbmap(ext, XR_E_MULT);
 			do_warn(
diff --git a/repair/scan.c b/repair/scan.c
index e4ac4a7..0fc41f2 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -705,6 +705,7 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 							     XR_E_FREE);
 						break;
 					}
+					/* fall through */
 				default:
 					do_warn(
 	_("block (%d,%d-%d) multiply claimed by %s space tree, state - %d\n"),

Re: [PATCH 01/16] misc: fix gcc 7.3 warnings

[Eric Sandeen]

On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix various compiler warnings that pop up in 7.3.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

(after long code archaeology session and reading to be sure
that last fallthrough really is correct, ugh, comments
would have been good when _cnt & _bno were merged...)

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

> ---
>  db/bit.c        |    4 ++--
>  repair/dinode.c |    1 +
>  repair/scan.c   |    1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/db/bit.c b/db/bit.c
> index bf8d80e..a6d8c9f 100644
> --- a/db/bit.c
> +++ b/db/bit.c
> @@ -111,11 +111,11 @@ getbitval(
>  			/* handle endian swap here */
>  #if __BYTE_ORDER == LITTLE_ENDIAN
>  			if (i == 0 && signext && nbits < 64)
> -				rval = -1LL << nbits;
> +				rval = (~0ULL) << nbits;
>  			rval |= 1ULL << (nbits - i - 1);
>  #else
>  			if ((i == (nbits - 1)) && signext && nbits < 64)
> -				rval |= (-1LL << nbits);
> +				rval |= ((~0ULL) << nbits);
>  			rval |= 1ULL << (nbits - i - 1);
>  #endif
>  		}
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 32cc769..07bcf80 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -525,6 +525,7 @@ _("data fork in rt inode %" PRIu64 " found metadata block %" PRIu64 " in rt bmap
>  		case XR_E_INUSE:
>  			if (pwe)
>  				break;
> +			/* fall through */
>  		case XR_E_MULT:
>  			set_rtbmap(ext, XR_E_MULT);
>  			do_warn(
> diff --git a/repair/scan.c b/repair/scan.c
> index e4ac4a7..0fc41f2 100644
> --- a/repair/scan.c
> +++ b/repair/scan.c
> @@ -705,6 +705,7 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
>  							     XR_E_FREE);
>  						break;
>  					}
> +					/* fall through */
>  				default:
>  					do_warn(
>  	_("block (%d,%d-%d) multiply claimed by %s space tree, state - %d\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
> 

[PATCH 02/16] xfs_db: don't crash in ablock if there's no inode

[Darrick J. Wong]

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

Make sure we actually have an inode selected before trying to unwrap its
attribute fork.  Found via a crash in xfs/288 with project quotas
enabled.

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


diff --git a/db/block.c b/db/block.c
index 5ecd687..174e29a 100644
--- a/db/block.c
+++ b/db/block.c
@@ -84,6 +84,11 @@ ablock_f(
 	}
 	push_cur();
 	set_cur_inode(iocur_top->ino);
+	if (!iocur_top->data) {
+		pop_cur();
+		dbprintf(_("no current inode\n"));
+		return 0;
+	}
 	haveattr = XFS_DFORK_Q((xfs_dinode_t *)iocur_top->data);
 	pop_cur();
 	if (!haveattr) {

[PATCH 03/16] 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 v2 03/16] xfs_scrub: log operational messages when interactive

[Darrick J. Wong]

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

Record the summary 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>
---
v2: only log beginning and results
---
 scrub/common.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 scrub/common.h    |   10 ++++++++++
 scrub/phase1.c    |    1 +
 scrub/xfs_scrub.c |   18 +++++++++++++-----
 scrub/xfs_scrub.h |    1 +
 5 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/scrub/common.c b/scrub/common.c
index 17c3699..4f26bf8 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)
 {
@@ -131,6 +140,46 @@ __str_out(
 	pthread_mutex_unlock(&ctx->lock);
 }
 
+/* Log a message to syslog. */
+#define LOG_BUFSZ	4096
+#define LOGNAME_BUFSZ	256
+void
+__str_log(
+	struct scrub_ctx	*ctx,
+	enum error_level	level,
+	const char		*format,
+	...)
+{
+	va_list			args;
+	char			logname[LOGNAME_BUFSZ];
+	char			buf[LOG_BUFSZ];
+	int			sz;
+
+	/* We only want to hear about optimizing when in debug/verbose mode. */
+	if (level == S_PREEN && !debug && !verbose)
+		return;
+
+	/*
+	 * Skip logging if we're being run as a service (presumably the
+	 * service will log stdout/stderr); if we're being run in a non
+	 * interactive manner (assume we're a service); or if we're in
+	 * debug mode.
+	 */
+	if (is_service || !isatty(fileno(stdin)) || debug)
+		return;
+
+	snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname, ctx->mntpoint);
+	openlog(logname, LOG_PID, LOG_DAEMON);
+
+	sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_str[level]));
+	va_start(args, format);
+	vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args);
+	va_end(args);
+	syslog(log_level[level], "%s", buf);
+
+	closelog();
+}
+
 double
 timeval_subtract(
 	struct timeval		*tv1,
diff --git a/scrub/common.h b/scrub/common.h
index 287bd4d..4f1f0cd 100644
--- a/scrub/common.h
+++ b/scrub/common.h
@@ -56,6 +56,16 @@ void __str_out(struct scrub_ctx *ctx, const char *descr, enum error_level level,
 #define dbg_printf(fmt, ...) \
 	do {if (debug > 1) {printf(fmt, __VA_ARGS__);}} while (0)
 
+void __str_log(struct scrub_ctx *ctx, enum error_level level,
+		const char *format, ...);
+
+#define log_info(ctx, ...) \
+	__str_log(ctx, S_INFO,	__VA_ARGS__)
+#define log_warn(ctx, ...) \
+	__str_log(ctx, S_WARN,	__VA_ARGS__)
+#define log_err(ctx, ...) \
+	__str_log(ctx, S_ERROR,	__VA_ARGS__)
+
 /* Is this debug tweak enabled? */
 static inline bool
 debug_tweak_on(
diff --git a/scrub/phase1.c b/scrub/phase1.c
index 6cd5442..b856a7f 100644
--- a/scrub/phase1.c
+++ b/scrub/phase1.c
@@ -236,6 +236,7 @@ _("Unable to find realtime device path."));
 	 * this point are most probably corruption errors (as opposed to
 	 * purely setup errors).
 	 */
+	log_info(ctx, _("Invoking online scrub."), ctx);
 	ctx->need_repair = true;
 	return true;
 }
diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
index ab26e63..7ab0c3e 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -162,7 +162,7 @@ bool				stdout_isatty;
  * If we are running as a service, we need to be careful about what
  * error codes we return to the calling process.
  */
-static bool			is_service;
+bool				is_service;
 
 #define SCRUB_RET_SUCCESS	(0)	/* no problems left behind */
 #define SCRUB_RET_CORRUPT	(1)	/* corruption remains on fs */
@@ -501,19 +501,27 @@ report_outcome(
 
 	total_errors = ctx->errors_found + ctx->runtime_errors;
 
-	if (total_errors == 0 && ctx->warnings_found == 0)
+	if (total_errors == 0 && ctx->warnings_found == 0) {
+		log_info(ctx, _("No errors found."));
 		return;
+	}
 
-	if (total_errors == 0)
+	if (total_errors == 0) {
 		fprintf(stderr, _("%s: warnings found: %llu\n"), ctx->mntpoint,
 				ctx->warnings_found);
-	else if (ctx->warnings_found == 0)
+		log_warn(ctx, _("warnings found: %llu"), ctx->warnings_found);
+	} else if (ctx->warnings_found == 0) {
 		fprintf(stderr, _("%s: errors found: %llu\n"), ctx->mntpoint,
 				total_errors);
-	else
+		log_err(ctx, _("errors found: %llu"), total_errors);
+	} else {
 		fprintf(stderr, _("%s: errors found: %llu; warnings found: %llu\n"),
 				ctx->mntpoint, total_errors,
 				ctx->warnings_found);
+		log_err(ctx, _("errors found: %llu; warnings found: %llu"),
+				total_errors, ctx->warnings_found);
+	}
+
 	if (ctx->need_repair)
 		fprintf(stderr, _("%s: Unmount and run xfs_repair.\n"),
 				ctx->mntpoint);
diff --git a/scrub/xfs_scrub.h b/scrub/xfs_scrub.h
index 89b46a4..b455747 100644
--- a/scrub/xfs_scrub.h
+++ b/scrub/xfs_scrub.h
@@ -31,6 +31,7 @@ extern long			page_size;
 extern bool			want_fstrim;
 extern bool			stderr_isatty;
 extern bool			stdout_isatty;
+extern bool			is_service;
 
 enum scrub_mode {
 	SCRUB_MODE_DRY_RUN,

Re: [PATCH v2 03/16] xfs_scrub: log operational messages when interactive

[Eric Sandeen]

On 3/8/18 1:35 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Record the summary 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>
> ---
> v2: only log beginning and results

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

> ---
>  scrub/common.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  scrub/common.h    |   10 ++++++++++
>  scrub/phase1.c    |    1 +
>  scrub/xfs_scrub.c |   18 +++++++++++++-----
>  scrub/xfs_scrub.h |    1 +
>  5 files changed, 74 insertions(+), 5 deletions(-)
> 
> diff --git a/scrub/common.c b/scrub/common.c
> index 17c3699..4f26bf8 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)
>  {
> @@ -131,6 +140,46 @@ __str_out(
>  	pthread_mutex_unlock(&ctx->lock);
>  }
>  
> +/* Log a message to syslog. */
> +#define LOG_BUFSZ	4096
> +#define LOGNAME_BUFSZ	256
> +void
> +__str_log(
> +	struct scrub_ctx	*ctx,
> +	enum error_level	level,
> +	const char		*format,
> +	...)
> +{
> +	va_list			args;
> +	char			logname[LOGNAME_BUFSZ];
> +	char			buf[LOG_BUFSZ];
> +	int			sz;
> +
> +	/* We only want to hear about optimizing when in debug/verbose mode. */
> +	if (level == S_PREEN && !debug && !verbose)
> +		return;
> +
> +	/*
> +	 * Skip logging if we're being run as a service (presumably the
> +	 * service will log stdout/stderr); if we're being run in a non
> +	 * interactive manner (assume we're a service); or if we're in
> +	 * debug mode.
> +	 */
> +	if (is_service || !isatty(fileno(stdin)) || debug)
> +		return;
> +
> +	snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname, ctx->mntpoint);
> +	openlog(logname, LOG_PID, LOG_DAEMON);
> +
> +	sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_str[level]));
> +	va_start(args, format);
> +	vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args);
> +	va_end(args);
> +	syslog(log_level[level], "%s", buf);
> +
> +	closelog();
> +}
> +
>  double
>  timeval_subtract(
>  	struct timeval		*tv1,
> diff --git a/scrub/common.h b/scrub/common.h
> index 287bd4d..4f1f0cd 100644
> --- a/scrub/common.h
> +++ b/scrub/common.h
> @@ -56,6 +56,16 @@ void __str_out(struct scrub_ctx *ctx, const char *descr, enum error_level level,
>  #define dbg_printf(fmt, ...) \
>  	do {if (debug > 1) {printf(fmt, __VA_ARGS__);}} while (0)
>  
> +void __str_log(struct scrub_ctx *ctx, enum error_level level,
> +		const char *format, ...);
> +
> +#define log_info(ctx, ...) \
> +	__str_log(ctx, S_INFO,	__VA_ARGS__)
> +#define log_warn(ctx, ...) \
> +	__str_log(ctx, S_WARN,	__VA_ARGS__)
> +#define log_err(ctx, ...) \
> +	__str_log(ctx, S_ERROR,	__VA_ARGS__)
> +
>  /* Is this debug tweak enabled? */
>  static inline bool
>  debug_tweak_on(
> diff --git a/scrub/phase1.c b/scrub/phase1.c
> index 6cd5442..b856a7f 100644
> --- a/scrub/phase1.c
> +++ b/scrub/phase1.c
> @@ -236,6 +236,7 @@ _("Unable to find realtime device path."));
>  	 * this point are most probably corruption errors (as opposed to
>  	 * purely setup errors).
>  	 */
> +	log_info(ctx, _("Invoking online scrub."), ctx);
>  	ctx->need_repair = true;
>  	return true;
>  }
> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> index ab26e63..7ab0c3e 100644
> --- a/scrub/xfs_scrub.c
> +++ b/scrub/xfs_scrub.c
> @@ -162,7 +162,7 @@ bool				stdout_isatty;
>   * If we are running as a service, we need to be careful about what
>   * error codes we return to the calling process.
>   */
> -static bool			is_service;
> +bool				is_service;
>  
>  #define SCRUB_RET_SUCCESS	(0)	/* no problems left behind */
>  #define SCRUB_RET_CORRUPT	(1)	/* corruption remains on fs */
> @@ -501,19 +501,27 @@ report_outcome(
>  
>  	total_errors = ctx->errors_found + ctx->runtime_errors;
>  
> -	if (total_errors == 0 && ctx->warnings_found == 0)
> +	if (total_errors == 0 && ctx->warnings_found == 0) {
> +		log_info(ctx, _("No errors found."));
>  		return;
> +	}
>  
> -	if (total_errors == 0)
> +	if (total_errors == 0) {
>  		fprintf(stderr, _("%s: warnings found: %llu\n"), ctx->mntpoint,
>  				ctx->warnings_found);
> -	else if (ctx->warnings_found == 0)
> +		log_warn(ctx, _("warnings found: %llu"), ctx->warnings_found);
> +	} else if (ctx->warnings_found == 0) {
>  		fprintf(stderr, _("%s: errors found: %llu\n"), ctx->mntpoint,
>  				total_errors);
> -	else
> +		log_err(ctx, _("errors found: %llu"), total_errors);
> +	} else {
>  		fprintf(stderr, _("%s: errors found: %llu; warnings found: %llu\n"),
>  				ctx->mntpoint, total_errors,
>  				ctx->warnings_found);
> +		log_err(ctx, _("errors found: %llu; warnings found: %llu"),
> +				total_errors, ctx->warnings_found);
> +	}
> +
>  	if (ctx->need_repair)
>  		fprintf(stderr, _("%s: Unmount and run xfs_repair.\n"),
>  				ctx->mntpoint);
> diff --git a/scrub/xfs_scrub.h b/scrub/xfs_scrub.h
> index 89b46a4..b455747 100644
> --- a/scrub/xfs_scrub.h
> +++ b/scrub/xfs_scrub.h
> @@ -31,6 +31,7 @@ extern long			page_size;
>  extern bool			want_fstrim;
>  extern bool			stderr_isatty;
>  extern bool			stdout_isatty;
> +extern bool			is_service;
>  
>  enum scrub_mode {
>  	SCRUB_MODE_DRY_RUN,
> 

[PATCH 04/16] xfs_scrub: don't ask user to run xfs_repair for only warnings

[Darrick J. Wong]

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

Don't advise the user to run xfs_repair on a filesystem that triggers
warnings but no errors; there's no corruption for it to fix.

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


diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
index ab26e63..53a105a 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -514,7 +514,7 @@ report_outcome(
 		fprintf(stderr, _("%s: errors found: %llu; warnings found: %llu\n"),
 				ctx->mntpoint, total_errors,
 				ctx->warnings_found);
-	if (ctx->need_repair)
+	if (ctx->need_repair && total_errors > 0)
 		fprintf(stderr, _("%s: Unmount and run xfs_repair.\n"),
 				ctx->mntpoint);
 }

Re: [PATCH 04/16] xfs_scrub: don't ask user to run xfs_repair for only warnings

[Eric Sandeen]

On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't advise the user to run xfs_repair on a filesystem that triggers
> warnings but no errors; there's no corruption for it to fix.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I went looking for why ->need_repair is set if repair isn't needed, and:

C symbol: need_repair

  File              Function	   Line
0 scrub/xfs_scrub.h <global>        98 bool need_repair;
1 scrub/phase1.c    xfs_setup_fs   239 ctx->need_repair = true;
2 scrub/xfs_scrub.c report_outcome 517 if (ctx->need_repair)

um, when is ->need_repair ever false?  What am I missing?

> ---
>  scrub/xfs_scrub.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> index ab26e63..53a105a 100644
> --- a/scrub/xfs_scrub.c
> +++ b/scrub/xfs_scrub.c
> @@ -514,7 +514,7 @@ report_outcome(
>  		fprintf(stderr, _("%s: errors found: %llu; warnings found: %llu\n"),
>  				ctx->mntpoint, total_errors,
>  				ctx->warnings_found);
> -	if (ctx->need_repair)
> +	if (ctx->need_repair && total_errors > 0)
>  		fprintf(stderr, _("%s: Unmount and run xfs_repair.\n"),
>  				ctx->mntpoint);
>  }
> 
> --
> 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 04/16] xfs_scrub: don't ask user to run xfs_repair for only warnings

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 11:16:50AM -0600, Eric Sandeen wrote:
> 
> 
> On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Don't advise the user to run xfs_repair on a filesystem that triggers
> > warnings but no errors; there's no corruption for it to fix.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I went looking for why ->need_repair is set if repair isn't needed, and:
> 
> C symbol: need_repair
> 
>   File              Function	   Line
> 0 scrub/xfs_scrub.h <global>        98 bool need_repair;
> 1 scrub/phase1.c    xfs_setup_fs   239 ctx->need_repair = true;
> 2 scrub/xfs_scrub.c report_outcome 517 if (ctx->need_repair)
> 
> um, when is ->need_repair ever false?  What am I missing?

In main():

struct scrub_ctx	ctx = {0};

ctx.need_repair is false from the start of the program until the end of
phase 1 when we've decided that yes we can check this xfs filesystem.

--D

> > ---
> >  scrub/xfs_scrub.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> > index ab26e63..53a105a 100644
> > --- a/scrub/xfs_scrub.c
> > +++ b/scrub/xfs_scrub.c
> > @@ -514,7 +514,7 @@ report_outcome(
> >  		fprintf(stderr, _("%s: errors found: %llu; warnings found: %llu\n"),
> >  				ctx->mntpoint, total_errors,
> >  				ctx->warnings_found);
> > -	if (ctx->need_repair)
> > +	if (ctx->need_repair && total_errors > 0)
> >  		fprintf(stderr, _("%s: Unmount and run xfs_repair.\n"),
> >  				ctx->mntpoint);
> >  }
> > 
> > --
> > 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 04/16] xfs_scrub: don't ask user to run xfs_repair for only warnings

[Eric Sandeen]

On 3/6/18 11:27 AM, Darrick J. Wong wrote:
> On Tue, Mar 06, 2018 at 11:16:50AM -0600, Eric Sandeen wrote:
>>
>>
>> On 3/1/18 1:13 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Don't advise the user to run xfs_repair on a filesystem that triggers
>>> warnings but no errors; there's no corruption for it to fix.
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> I went looking for why ->need_repair is set if repair isn't needed, and:
>>
>> C symbol: need_repair
>>
>>   File              Function	   Line
>> 0 scrub/xfs_scrub.h <global>        98 bool need_repair;
>> 1 scrub/phase1.c    xfs_setup_fs   239 ctx->need_repair = true;
>> 2 scrub/xfs_scrub.c report_outcome 517 if (ctx->need_repair)
>>
>> um, when is ->need_repair ever false?  What am I missing?
> 
> In main():
> 
> struct scrub_ctx	ctx = {0};
> 
> ctx.need_repair is false from the start of the program until the end of
> phase 1 when we've decided that yes we can check this xfs filesystem.

Ok so after more looking & discussion, what ->need_repair really means
is "we got far enough to run the scrub ioctl?"

If that's true, and errors remain for any reason (?), the user is told
to run repair.

So while I see that this patch improves the user experience, I wonder
if we shouldn't take this opportunity to improve the developer experience
by renaming ->need_repair to ->scrub_ran or something, because I think
that makes a bit more sense semantically:

if (scrub ioctl ran && errors remain)
	tell_user("run repair")

My other quibble is that if (scrub ioctl ran && errors remain) is true only
because "-n" was specified, it seems a little odd to instruct the user
to run repair, when the errors may remain only because of -n.  But that's
a separate issue, I guess.

-Eric

Re: [PATCH 04/16] xfs_scrub: don't ask user to run xfs_repair for only warnings

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 12:34:20PM -0600, Eric Sandeen wrote:
> 
> 
> On 3/6/18 11:27 AM, Darrick J. Wong wrote:
> > On Tue, Mar 06, 2018 at 11:16:50AM -0600, Eric Sandeen wrote:
> >>
> >>
> >> On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> >>> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>>
> >>> Don't advise the user to run xfs_repair on a filesystem that triggers
> >>> warnings but no errors; there's no corruption for it to fix.
> >>>
> >>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >>
> >> I went looking for why ->need_repair is set if repair isn't needed, and:
> >>
> >> C symbol: need_repair
> >>
> >>   File              Function	   Line
> >> 0 scrub/xfs_scrub.h <global>        98 bool need_repair;
> >> 1 scrub/phase1.c    xfs_setup_fs   239 ctx->need_repair = true;
> >> 2 scrub/xfs_scrub.c report_outcome 517 if (ctx->need_repair)
> >>
> >> um, when is ->need_repair ever false?  What am I missing?
> > 
> > In main():
> > 
> > struct scrub_ctx	ctx = {0};
> > 
> > ctx.need_repair is false from the start of the program until the end of
> > phase 1 when we've decided that yes we can check this xfs filesystem.
> 
> Ok so after more looking & discussion, what ->need_repair really means
> is "we got far enough to run the scrub ioctl?"
> 
> If that's true, and errors remain for any reason (?), the user is told
> to run repair.
> 
> So while I see that this patch improves the user experience, I wonder
> if we shouldn't take this opportunity to improve the developer experience
> by renaming ->need_repair to ->scrub_ran or something, because I think
> that makes a bit more sense semantically:
> 
> if (scrub ioctl ran && errors remain)
> 	tell_user("run repair")

Ok.  I'll update the name.

> My other quibble is that if (scrub ioctl ran && errors remain) is true only
> because "-n" was specified, it seems a little odd to instruct the user
> to run repair, when the errors may remain only because of -n.  But that's
> a separate issue, I guess.

My thought process here is that any time we leave errors behind on the
filesystem we should advise the caller to run xfs_repair, whether that's
because the caller told us to fix things and we failed, or because the
caller trusts xfs_scrub to find the errors but not to fix them and
therefore ran xfs_scrub -n.  Either way you have a broken fs and need to
repair it.

However, I wonder if you're thinking "the user told (scrub) they didn't
want to change anything, so why would we advise the user to run a
(repair) tool that changes things"?

--D

> -Eric
> --
> 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 04/16] xfs_scrub: don't ask user to run xfs_repair for only warnings

[Eric Sandeen]

On 3/6/18 12:53 PM, Darrick J. Wong wrote:
> On Tue, Mar 06, 2018 at 12:34:20PM -0600, Eric Sandeen wrote:

...

>> My other quibble is that if (scrub ioctl ran && errors remain) is true only
>> because "-n" was specified, it seems a little odd to instruct the user
>> to run repair, when the errors may remain only because of -n.  But that's
>> a separate issue, I guess.
> 
> My thought process here is that any time we leave errors behind on the
> filesystem we should advise the caller to run xfs_repair, whether that's
> because the caller told us to fix things and we failed, or because the
> caller trusts xfs_scrub to find the errors but not to fix them and
> therefore ran xfs_scrub -n.  Either way you have a broken fs and need to
> repair it.
> 
> However, I wonder if you're thinking "the user told (scrub) they didn't
> want to change anything, so why would we advise the user to run a
> (repair) tool that changes things"?

I guess my thinking is that in reality the user has two options and the
tool is issuing a specific instruction to use only one of them.  I don't
think we can guess what the user does or doesn't trust.

Perhaps just something along the lines of

        if (ctx->need_repair) {
                fprintf(stderr, _("%s: Unmount and run xfs_repair.\n"),
                                ctx->mntpoint);
                if (ctx->mode = SCRUB_MODE_DRY_RUN)
                        fprintf(stderr, _("%s: Or, re-run without '-n'.\n"),
                                        ctx->mntpoint);
        }

or whatever ordering/phrasing makes sense?

-Eric

Re: [PATCH 04/16] xfs_scrub: don't ask user to run xfs_repair for only warnings

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 01:00:47PM -0600, Eric Sandeen wrote:
> On 3/6/18 12:53 PM, Darrick J. Wong wrote:
> > On Tue, Mar 06, 2018 at 12:34:20PM -0600, Eric Sandeen wrote:
> 
> ...
> 
> >> My other quibble is that if (scrub ioctl ran && errors remain) is true only
> >> because "-n" was specified, it seems a little odd to instruct the user
> >> to run repair, when the errors may remain only because of -n.  But that's
> >> a separate issue, I guess.
> > 
> > My thought process here is that any time we leave errors behind on the
> > filesystem we should advise the caller to run xfs_repair, whether that's
> > because the caller told us to fix things and we failed, or because the
> > caller trusts xfs_scrub to find the errors but not to fix them and
> > therefore ran xfs_scrub -n.  Either way you have a broken fs and need to
> > repair it.
> > 
> > However, I wonder if you're thinking "the user told (scrub) they didn't
> > want to change anything, so why would we advise the user to run a
> > (repair) tool that changes things"?
> 
> I guess my thinking is that in reality the user has two options and the
> tool is issuing a specific instruction to use only one of them.  I don't
> think we can guess what the user does or doesn't trust.
> 
> Perhaps just something along the lines of
> 
>         if (ctx->need_repair) {
>                 fprintf(stderr, _("%s: Unmount and run xfs_repair.\n"),
>                                 ctx->mntpoint);

"need_repair" has been changed to "scrub_setup_succeeded".

>                 if (ctx->mode = SCRUB_MODE_DRY_RUN)
>                         fprintf(stderr, _("%s: Or, re-run without '-n'.\n"),
>                                         ctx->mntpoint);

I'll do that, but not until the patch that adds fs repair
functionality to xfs_scrub.

--D

>         }
> 
> or whatever ordering/phrasing makes sense?
> 
> -Eric
> --
> 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

[PATCH v2 04/16] xfs_scrub: don't ask user to run xfs_repair for only warnings

[Darrick J. Wong]

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

Don't advise the user to run xfs_repair on a filesystem that triggers
warnings but no errors; there's no corruption for it to fix.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: fix confusing variable name, add more comments
---
 scrub/phase1.c    |    2 +-
 scrub/xfs_scrub.c |    7 ++++++-
 scrub/xfs_scrub.h |    2 +-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/scrub/phase1.c b/scrub/phase1.c
index b856a7f..002c642 100644
--- a/scrub/phase1.c
+++ b/scrub/phase1.c
@@ -237,6 +237,6 @@ _("Unable to find realtime device path."));
 	 * purely setup errors).
 	 */
 	log_info(ctx, _("Invoking online scrub."), ctx);
-	ctx->need_repair = true;
+	ctx->scrub_setup_succeeded = true;
 	return true;
 }
diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
index 7ab0c3e..7ee02b6 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -522,7 +522,12 @@ report_outcome(
 				total_errors, ctx->warnings_found);
 	}
 
-	if (ctx->need_repair)
+	/*
+	 * Don't advise the user to run repair unless we were successful in
+	 * setting up the scrub and we actually saw corruptions.  Warnings
+	 * are not corruptions.
+	 */
+	if (ctx->scrub_setup_succeeded && total_errors > 0)
 		fprintf(stderr, _("%s: Unmount and run xfs_repair.\n"),
 				ctx->mntpoint);
 }
diff --git a/scrub/xfs_scrub.h b/scrub/xfs_scrub.h
index b455747..aa130a7 100644
--- a/scrub/xfs_scrub.h
+++ b/scrub/xfs_scrub.h
@@ -96,7 +96,7 @@ struct scrub_ctx {
 	unsigned long long	naming_warnings;
 	unsigned long long	repairs;
 	unsigned long long	preens;
-	bool			need_repair;
+	bool			scrub_setup_succeeded;
 	bool			preen_triggers[XFS_SCRUB_TYPE_NR];
 };
 

Re: [PATCH v2 04/16] xfs_scrub: don't ask user to run xfs_repair for only warnings

[Eric Sandeen]

On 3/8/18 1:36 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't advise the user to run xfs_repair on a filesystem that triggers
> warnings but no errors; there's no corruption for it to fix.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: fix confusing variable name, add more comments

thanks, that helps my poor brain.

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

> ---
>  scrub/phase1.c    |    2 +-
>  scrub/xfs_scrub.c |    7 ++++++-
>  scrub/xfs_scrub.h |    2 +-
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/scrub/phase1.c b/scrub/phase1.c
> index b856a7f..002c642 100644
> --- a/scrub/phase1.c
> +++ b/scrub/phase1.c
> @@ -237,6 +237,6 @@ _("Unable to find realtime device path."));
>  	 * purely setup errors).
>  	 */
>  	log_info(ctx, _("Invoking online scrub."), ctx);
> -	ctx->need_repair = true;
> +	ctx->scrub_setup_succeeded = true;
>  	return true;
>  }
> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> index 7ab0c3e..7ee02b6 100644
> --- a/scrub/xfs_scrub.c
> +++ b/scrub/xfs_scrub.c
> @@ -522,7 +522,12 @@ report_outcome(
>  				total_errors, ctx->warnings_found);
>  	}
>  
> -	if (ctx->need_repair)
> +	/*
> +	 * Don't advise the user to run repair unless we were successful in
> +	 * setting up the scrub and we actually saw corruptions.  Warnings
> +	 * are not corruptions.
> +	 */
> +	if (ctx->scrub_setup_succeeded && total_errors > 0)
>  		fprintf(stderr, _("%s: Unmount and run xfs_repair.\n"),
>  				ctx->mntpoint);
>  }
> diff --git a/scrub/xfs_scrub.h b/scrub/xfs_scrub.h
> index b455747..aa130a7 100644
> --- a/scrub/xfs_scrub.h
> +++ b/scrub/xfs_scrub.h
> @@ -96,7 +96,7 @@ struct scrub_ctx {
>  	unsigned long long	naming_warnings;
>  	unsigned long long	repairs;
>  	unsigned long long	preens;
> -	bool			need_repair;
> +	bool			scrub_setup_succeeded;
>  	bool			preen_triggers[XFS_SCRUB_TYPE_NR];
>  };
>  
> --
> 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
> 

[PATCH 05/16] xfs_scrub: fix #include ordering to avoid build failure

[Darrick J. Wong]

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

Fix the ordering of the header includes in all the scrub source.  We put
xfs.h first so that it will pull in include/linux.h which pulls in
linux/fs.h + whatever overrides are necessary (currently limited to
struct fsxattr) to make things work on this platform, and then we remove
the #includes for anything that will get pulled (directly or indirectly)
by xfs.h for cleanliness.  Without this, a user compiling new xfsprogs
on a system with a 4.7 kernel gets this:

Building scrub
    [CC]     disk.o
In file included from ../include/xfs.h:37:0,
                 from disk.c:40:
../include/xfs/linux.h:185:8: error: redefinition of 'struct fsxattr'
 struct fsxattr {
        ^~~~~~~
In file included from disk.c:31:0:
/usr/include/linux/fs.h:155:8: note: originally defined here
 struct fsxattr {
        ^~~~~~~
gmake[2]: *** [../include/buildrules:60: disk.o] Error 1
gmake[1]: *** [include/buildrules:36: scrub] Error 2
make: *** [Makefile:77: default] Error 2

Reported-by: Mikael Magnusson <mikachu@gmail.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/bitmap.c      |    5 +----
 scrub/common.c      |    5 +----
 scrub/counter.c     |    2 +-
 scrub/disk.c        |    9 +--------
 scrub/filemap.c     |    6 +-----
 scrub/fscounters.c  |    5 +----
 scrub/inodes.c      |    4 +---
 scrub/phase1.c      |   10 +---------
 scrub/phase2.c      |    5 +----
 scrub/phase3.c      |    5 +----
 scrub/phase4.c      |    6 +-----
 scrub/phase5.c      |    5 +----
 scrub/phase6.c      |    5 +----
 scrub/phase7.c      |    5 +----
 scrub/progress.c    |    4 +---
 scrub/read_verify.c |    5 +----
 scrub/scrub.c       |    6 +-----
 scrub/spacemap.c    |    5 +----
 scrub/unicrash.c    |    5 +----
 scrub/vfs.c         |    4 +---
 scrub/xfs_scrub.c   |    5 +----
 21 files changed, 21 insertions(+), 90 deletions(-)


diff --git a/scrub/bitmap.c b/scrub/bitmap.c
index a88fd0e..212c3b8 100644
--- a/scrub/bitmap.c
+++ b/scrub/bitmap.c
@@ -17,13 +17,10 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
-#include <stdbool.h>
+#include "xfs.h"
 #include <stdint.h>
 #include <stdlib.h>
-#include <errno.h>
 #include <assert.h>
-#include <inttypes.h>
 #include <pthread.h>
 #include "platform_defs.h"
 #include "avl64.h"
diff --git a/scrub/common.c b/scrub/common.c
index 672f286..32f7642 100644
--- a/scrub/common.c
+++ b/scrub/common.c
@@ -17,14 +17,11 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <pthread.h>
-#include <stdbool.h>
 #include <sys/statvfs.h>
 #include <syslog.h>
 #include "platform_defs.h"
-#include "xfs.h"
-#include "xfs_fs.h"
 #include "path.h"
 #include "xfs_scrub.h"
 #include "common.h"
diff --git a/scrub/counter.c b/scrub/counter.c
index ced3cf3..0e28f0f 100644
--- a/scrub/counter.c
+++ b/scrub/counter.c
@@ -17,9 +17,9 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
+#include "xfs.h"
 #include <stdint.h>
 #include <stdlib.h>
-#include <stdbool.h>
 #include <string.h>
 #include <assert.h>
 #include <pthread.h>
diff --git a/scrub/disk.c b/scrub/disk.c
index e12175c..798b4a0 100644
--- a/scrub/disk.c
+++ b/scrub/disk.c
@@ -17,18 +17,13 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
 #include <stdlib.h>
-#include <stdbool.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <sys/types.h>
-#include <sys/stat.h>
-#include <sys/ioctl.h>
 #include <sys/statvfs.h>
-#include <sys/vfs.h>
-#include <linux/fs.h>
 #ifdef HAVE_SG_IO
 # include <scsi/sg.h>
 #endif
@@ -37,9 +32,7 @@
 #endif
 #include "platform_defs.h"
 #include "libfrog.h"
-#include "xfs.h"
 #include "path.h"
-#include "xfs_fs.h"
 #include "xfs_scrub.h"
 #include "common.h"
 #include "disk.h"
diff --git a/scrub/filemap.c b/scrub/filemap.c
index f42a6ba..056b3b0 100644
--- a/scrub/filemap.c
+++ b/scrub/filemap.c
@@ -17,17 +17,13 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
-#include <stdbool.h>
+#include "xfs.h"
 #include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <sys/statvfs.h>
-#include "xfs.h"
-#include "xfs_fs.h"
 #include "path.h"
 #include "xfs_scrub.h"
 #include "common.h"
diff --git a/scrub/fscounters.c b/scrub/fscounters.c
index ecdf4c6..05bf95e 100644
--- a/scrub/fscounters.c
+++ b/scrub/fscounters.c
@@ -17,15 +17,12 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
-#include <stdbool.h>
+#include "xfs.h"
 #include <stdint.h>
 #include <stdlib.h>
 #include <sys/statvfs.h>
 #include "platform_defs.h"
-#include "xfs.h"
 #include "xfs_arch.h"
-#include "xfs_fs.h"
 #include "xfs_format.h"
 #include "path.h"
 #include "workqueue.h"
diff --git a/scrub/inodes.c b/scrub/inodes.c
index 744b003..ccfb9e0 100644
--- a/scrub/inodes.c
+++ b/scrub/inodes.c
@@ -17,14 +17,12 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
-#include <stdbool.h>
+#include "xfs.h"
 #include <stdint.h>
 #include <stdlib.h>
 #include <pthread.h>
 #include <sys/statvfs.h>
 #include "platform_defs.h"
-#include "xfs.h"
 #include "xfs_arch.h"
 #include "xfs_format.h"
 #include "handle.h"
diff --git a/scrub/phase1.c b/scrub/phase1.c
index 6cd5442..5ddce6e 100644
--- a/scrub/phase1.c
+++ b/scrub/phase1.c
@@ -17,30 +17,22 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
-#include <mntent.h>
+#include "xfs.h"
 #include <unistd.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <sys/time.h>
 #include <sys/resource.h>
 #include <sys/statvfs.h>
-#include <sys/vfs.h>
 #include <fcntl.h>
 #include <dirent.h>
 #include <stdint.h>
-#include <stdbool.h>
 #include <pthread.h>
-#include <errno.h>
-#include <linux/fs.h>
 #include "libfrog.h"
 #include "workqueue.h"
 #include "input.h"
 #include "path.h"
 #include "handle.h"
 #include "bitops.h"
-#include "xfs_arch.h"
-#include "xfs_format.h"
 #include "avl64.h"
 #include "list.h"
 #include "xfs_scrub.h"
diff --git a/scrub/phase2.c b/scrub/phase2.c
index edf66df..ad736bf 100644
--- a/scrub/phase2.c
+++ b/scrub/phase2.c
@@ -17,13 +17,10 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
-#include <stdbool.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <sys/statvfs.h>
-#include "xfs.h"
 #include "list.h"
 #include "path.h"
 #include "workqueue.h"
diff --git a/scrub/phase3.c b/scrub/phase3.c
index a0ee5d9..68c95e6 100644
--- a/scrub/phase3.c
+++ b/scrub/phase3.c
@@ -17,13 +17,10 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
-#include <stdbool.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <sys/statvfs.h>
-#include "xfs.h"
 #include "list.h"
 #include "path.h"
 #include "workqueue.h"
diff --git a/scrub/phase4.c b/scrub/phase4.c
index 1fb8da9..8573036 100644
--- a/scrub/phase4.c
+++ b/scrub/phase4.c
@@ -17,15 +17,11 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
-#include <stdbool.h>
 #include <dirent.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <sys/statvfs.h>
-#include "xfs.h"
-#include "xfs_fs.h"
 #include "list.h"
 #include "path.h"
 #include "workqueue.h"
diff --git a/scrub/phase5.c b/scrub/phase5.c
index 703b279..0ef6339 100644
--- a/scrub/phase5.c
+++ b/scrub/phase5.c
@@ -17,17 +17,14 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
-#include <stdbool.h>
 #include <dirent.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <sys/statvfs.h>
 #ifdef HAVE_LIBATTR
 # include <attr/attributes.h>
 #endif
-#include "xfs.h"
 #include "handle.h"
 #include "list.h"
 #include "path.h"
diff --git a/scrub/phase6.c b/scrub/phase6.c
index e255eef..b533cbb 100644
--- a/scrub/phase6.c
+++ b/scrub/phase6.c
@@ -17,13 +17,10 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
-#include <stdbool.h>
 #include <dirent.h>
 #include <sys/statvfs.h>
-#include "xfs.h"
-#include "xfs_fs.h"
 #include "handle.h"
 #include "path.h"
 #include "ptvar.h"
diff --git a/scrub/phase7.c b/scrub/phase7.c
index 460ca8a..50d04ae 100644
--- a/scrub/phase7.c
+++ b/scrub/phase7.c
@@ -17,13 +17,10 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
-#include <stdbool.h>
 #include <stdlib.h>
 #include <sys/statvfs.h>
-#include "xfs.h"
-#include "xfs_fs.h"
 #include "path.h"
 #include "ptvar.h"
 #include "xfs_scrub.h"
diff --git a/scrub/progress.c b/scrub/progress.c
index 61b9c60..e9e720b 100644
--- a/scrub/progress.c
+++ b/scrub/progress.c
@@ -17,13 +17,11 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include "libxfs.h"
-#include <stdio.h>
+#include "xfs.h"
 #include <dirent.h>
 #include <pthread.h>
 #include <sys/statvfs.h>
 #include <time.h>
-#include "../repair/threads.h"
 #include "path.h"
 #include "disk.h"
 #include "read_verify.h"
diff --git a/scrub/read_verify.c b/scrub/read_verify.c
index ae2e85f..d7bcc17 100644
--- a/scrub/read_verify.c
+++ b/scrub/read_verify.c
@@ -17,15 +17,12 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
-#include <stdbool.h>
 #include <stdlib.h>
 #include <sys/statvfs.h>
 #include "workqueue.h"
 #include "path.h"
-#include "xfs.h"
-#include "xfs_fs.h"
 #include "xfs_scrub.h"
 #include "common.h"
 #include "counter.h"
diff --git a/scrub/scrub.c b/scrub/scrub.c
index ff5357c..9e82675 100644
--- a/scrub/scrub.c
+++ b/scrub/scrub.c
@@ -17,17 +17,13 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
-#include <stdbool.h>
+#include "xfs.h"
 #include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <sys/statvfs.h>
-#include "xfs.h"
-#include "xfs_fs.h"
 #include "list.h"
 #include "path.h"
 #include "xfs_scrub.h"
diff --git a/scrub/spacemap.c b/scrub/spacemap.c
index f631913..3621035 100644
--- a/scrub/spacemap.c
+++ b/scrub/spacemap.c
@@ -17,15 +17,12 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
-#include <stdbool.h>
+#include "xfs.h"
 #include <stdint.h>
 #include <string.h>
 #include <pthread.h>
 #include <sys/statvfs.h>
 #include "workqueue.h"
-#include "xfs.h"
-#include "xfs_fs.h"
 #include "path.h"
 #include "xfs_scrub.h"
 #include "common.h"
diff --git a/scrub/unicrash.c b/scrub/unicrash.c
index ce3e7f9..0b5d1fa 100644
--- a/scrub/unicrash.c
+++ b/scrub/unicrash.c
@@ -17,17 +17,14 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
-#include <stdbool.h>
 #include <stdlib.h>
 #include <dirent.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <sys/statvfs.h>
 #include <unistr.h>
 #include <uninorm.h>
-#include "xfs.h"
 #include "path.h"
 #include "xfs_scrub.h"
 #include "common.h"
diff --git a/scrub/vfs.c b/scrub/vfs.c
index 0c5b353..cfb5878 100644
--- a/scrub/vfs.c
+++ b/scrub/vfs.c
@@ -17,13 +17,11 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <stdint.h>
-#include <stdbool.h>
 #include <dirent.h>
 #include <sys/types.h>
 #include <sys/statvfs.h>
-#include "xfs.h"
 #include "handle.h"
 #include "path.h"
 #include "workqueue.h"
diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
index 53a105a..df58db5 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -17,16 +17,13 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <stdio.h>
+#include "xfs.h"
 #include <pthread.h>
-#include <stdbool.h>
 #include <stdlib.h>
 #include <sys/time.h>
 #include <sys/resource.h>
 #include <sys/statvfs.h>
 #include "platform_defs.h"
-#include "xfs.h"
-#include "xfs_fs.h"
 #include "input.h"
 #include "path.h"
 #include "xfs_scrub.h"

Re: [PATCH 05/16] xfs_scrub: fix #include ordering to avoid build failure

[Eric Sandeen]

On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix the ordering of the header includes in all the scrub source.  We put
> xfs.h first so that it will pull in include/linux.h which pulls in
> linux/fs.h + whatever overrides are necessary (currently limited to
> struct fsxattr) to make things work on this platform, and then we remove
> the #includes for anything that will get pulled (directly or indirectly)
> by xfs.h for cleanliness.  Without this, a user compiling new xfsprogs
> on a system with a 4.7 kernel gets this:
> 
> Building scrub
>     [CC]     disk.o
> In file included from ../include/xfs.h:37:0,
>                  from disk.c:40:
> ../include/xfs/linux.h:185:8: error: redefinition of 'struct fsxattr'
>  struct fsxattr {
>         ^~~~~~~
> In file included from disk.c:31:0:
> /usr/include/linux/fs.h:155:8: note: originally defined here
>  struct fsxattr {
>         ^~~~~~~
> gmake[2]: *** [../include/buildrules:60: disk.o] Error 1
> gmake[1]: *** [include/buildrules:36: scrub] Error 2
> make: *** [Makefile:77: default] Error 2
> 
> Reported-by: Mikael Magnusson <mikachu@gmail.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

mmkay.  I guess the question of whether having our own redefined
fsxattr is wise in the first place is a different discussion.

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

[PATCH 06/16] xfs_scrub: don't try to scan xattrs if bstat says there aren't any

[Darrick J. Wong]

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

Only try to scan the extended attributes of a file if bstat says that
the file actually has any.  Surprisingly, this reduces the phase 5
runtime by 40% if most of the files don't have attrs.

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


diff --git a/scrub/phase5.c b/scrub/phase5.c
index 0ef6339..8e0a1be 100644
--- a/scrub/phase5.c
+++ b/scrub/phase5.c
@@ -256,9 +256,12 @@ xfs_scrub_connections(
 	background_sleep();
 
 	/* Warn about naming problems in xattrs. */
-	moveon = xfs_scrub_scan_fhandle_xattrs(ctx, descr, handle, bstat);
-	if (!moveon)
-		goto out;
+	if (bstat->bs_xflags & FS_XFLAG_HASATTR) {
+		moveon = xfs_scrub_scan_fhandle_xattrs(ctx, descr, handle,
+				bstat);
+		if (!moveon)
+			goto out;
+	}
 
 	/* Open the dir, let the kernel try to reconnect it to the root. */
 	if (S_ISDIR(bstat->bs_mode)) {

Re: [PATCH 06/16] xfs_scrub: don't try to scan xattrs if bstat says there aren't any

[Eric Sandeen]

On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Only try to scan the extended attributes of a file if bstat says that
> the file actually has any.  Surprisingly, this reduces the phase 5
> runtime by 40% if most of the files don't have attrs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

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

> ---
>  scrub/phase5.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/scrub/phase5.c b/scrub/phase5.c
> index 0ef6339..8e0a1be 100644
> --- a/scrub/phase5.c
> +++ b/scrub/phase5.c
> @@ -256,9 +256,12 @@ xfs_scrub_connections(
>  	background_sleep();
>  
>  	/* Warn about naming problems in xattrs. */
> -	moveon = xfs_scrub_scan_fhandle_xattrs(ctx, descr, handle, bstat);
> -	if (!moveon)
> -		goto out;
> +	if (bstat->bs_xflags & FS_XFLAG_HASATTR) {
> +		moveon = xfs_scrub_scan_fhandle_xattrs(ctx, descr, handle,
> +				bstat);
> +		if (!moveon)
> +			goto out;
> +	}
>  
>  	/* Open the dir, let the kernel try to reconnect it to the root. */
>  	if (S_ISDIR(bstat->bs_mode)) {
> 
> --
> 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
> 

[PATCH 07/16] xfs_db: print transaction reservation type information

[Darrick J. Wong]

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

Create a new xfs_db command to print the transaction reservation info for
a given filesystem.  This will make it easier to compare the calculations
made by the kernel and xfsprogs in case there is a discrepancy.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/command.c             |    1 +
 db/logformat.c           |   59 ++++++++++++++++++++++++++++++++++++++++++++++
 db/logformat.h           |    1 +
 libxfs/libxfs_api_defs.h |    2 +-
 man/man8/xfs_db.8        |    6 +++++
 5 files changed, 68 insertions(+), 1 deletion(-)


diff --git a/db/command.c b/db/command.c
index 5ff3c4f..12ae5b7 100644
--- a/db/command.c
+++ b/db/command.c
@@ -138,6 +138,7 @@ init_commands(void)
 	hash_init();
 	inode_init();
 	input_init();
+	logres_init();
 	logformat_init();
 	io_init();
 	metadump_init();
diff --git a/db/logformat.c b/db/logformat.c
index 70097bc..b290bd3 100644
--- a/db/logformat.c
+++ b/db/logformat.c
@@ -147,3 +147,62 @@ logformat_init(void)
 
 	add_command(&logformat_cmd);
 }
+
+static void
+print_logres(
+	int			i,
+	struct xfs_trans_res	*res)
+{
+	dbprintf(_("type %d logres %u logcount %d flags 0x%x\n"),
+		i, res->tr_logres, res->tr_logcount, res->tr_logflags);
+}
+
+int
+logres_f(
+	int			argc,
+	char			**argv)
+{
+	struct xfs_trans_res	resv;
+	struct xfs_trans_res	*res;
+	struct xfs_trans_res	*end_res;
+	int			i;
+
+	res = (struct xfs_trans_res *)M_RES(mp);
+	end_res = (struct xfs_trans_res *)(M_RES(mp) + 1);
+	for (i = 0; res < end_res; i++, res++)
+		print_logres(i, res);
+	libxfs_log_get_max_trans_res(mp, &resv);
+	print_logres(-1, &resv);
+
+	return 0;
+}
+
+static void
+logres_help(void)
+{
+	dbprintf(_(
+"\n"
+" The 'logres' command prints information about all log reservation types.\n"
+" This includes the reservation space, the intended transaction roll count,\n"
+" and the reservation flags, if any.\n"
+"\n"
+	));
+}
+
+static const struct cmdinfo logres_cmd = {
+	.name =		"logres",
+	.altname =	NULL,
+	.cfunc =	logres_f,
+	.argmin =	0,
+	.argmax =	0,
+	.canpush =	0,
+	.args =		NULL,
+	.oneline =	N_("dump log reservations"),
+	.help =		logres_help,
+};
+
+void
+logres_init(void)
+{
+	add_command(&logres_cmd);
+}
diff --git a/db/logformat.h b/db/logformat.h
index f9763ee..60396c0 100644
--- a/db/logformat.h
+++ b/db/logformat.h
@@ -17,3 +17,4 @@
  */
 
 void logformat_init(void);
+void logres_init(void);
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index d2ab02a..5d56340 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -60,7 +60,7 @@
 #define xfs_trans_roll			libxfs_trans_roll
 #define xfs_trans_get_buf_map		libxfs_trans_get_buf_map
 #define xfs_trans_resv_calc		libxfs_trans_resv_calc
-
+#define xfs_log_get_max_trans_res	libxfs_log_get_max_trans_res
 #define xfs_attr_get			libxfs_attr_get
 #define xfs_attr_set			libxfs_attr_set
 #define xfs_attr_remove			libxfs_attr_remove
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 37018a7..524b1ef 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -690,6 +690,12 @@ Start logging output to
 .IR filename ,
 stop logging, or print the current logging status.
 .TP
+.B logres
+Print transaction reservation size information for each transaction type.
+This makes it easier to find discrepancies in the reservation calculations
+between xfsprogs and the kernel, which will help when diagnosing minimum
+log size calculation errors.
+.TP
 .BI "metadump [\-egow] " filename
 Dumps metadata to a file. See
 .BR xfs_metadump (8)

Re: [PATCH 07/16] xfs_db: print transaction reservation type information

[Eric Sandeen]

On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a new xfs_db command to print the transaction reservation info for
> a given filesystem.  This will make it easier to compare the calculations
> made by the kernel and xfsprogs in case there is a discrepancy.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

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

> ---
>  db/command.c             |    1 +
>  db/logformat.c           |   59 ++++++++++++++++++++++++++++++++++++++++++++++
>  db/logformat.h           |    1 +
>  libxfs/libxfs_api_defs.h |    2 +-
>  man/man8/xfs_db.8        |    6 +++++
>  5 files changed, 68 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/db/command.c b/db/command.c
> index 5ff3c4f..12ae5b7 100644
> --- a/db/command.c
> +++ b/db/command.c
> @@ -138,6 +138,7 @@ init_commands(void)
>  	hash_init();
>  	inode_init();
>  	input_init();
> +	logres_init();
>  	logformat_init();
>  	io_init();
>  	metadump_init();
> diff --git a/db/logformat.c b/db/logformat.c
> index 70097bc..b290bd3 100644
> --- a/db/logformat.c
> +++ b/db/logformat.c
> @@ -147,3 +147,62 @@ logformat_init(void)
>  
>  	add_command(&logformat_cmd);
>  }
> +
> +static void
> +print_logres(
> +	int			i,
> +	struct xfs_trans_res	*res)
> +{
> +	dbprintf(_("type %d logres %u logcount %d flags 0x%x\n"),
> +		i, res->tr_logres, res->tr_logcount, res->tr_logflags);
> +}
> +
> +int
> +logres_f(
> +	int			argc,
> +	char			**argv)
> +{
> +	struct xfs_trans_res	resv;
> +	struct xfs_trans_res	*res;
> +	struct xfs_trans_res	*end_res;
> +	int			i;
> +
> +	res = (struct xfs_trans_res *)M_RES(mp);
> +	end_res = (struct xfs_trans_res *)(M_RES(mp) + 1);
> +	for (i = 0; res < end_res; i++, res++)
> +		print_logres(i, res);
> +	libxfs_log_get_max_trans_res(mp, &resv);
> +	print_logres(-1, &resv);
> +
> +	return 0;
> +}
> +
> +static void
> +logres_help(void)
> +{
> +	dbprintf(_(
> +"\n"
> +" The 'logres' command prints information about all log reservation types.\n"
> +" This includes the reservation space, the intended transaction roll count,\n"
> +" and the reservation flags, if any.\n"
> +"\n"
> +	));
> +}
> +
> +static const struct cmdinfo logres_cmd = {
> +	.name =		"logres",
> +	.altname =	NULL,
> +	.cfunc =	logres_f,
> +	.argmin =	0,
> +	.argmax =	0,
> +	.canpush =	0,
> +	.args =		NULL,
> +	.oneline =	N_("dump log reservations"),
> +	.help =		logres_help,
> +};
> +
> +void
> +logres_init(void)
> +{
> +	add_command(&logres_cmd);
> +}
> diff --git a/db/logformat.h b/db/logformat.h
> index f9763ee..60396c0 100644
> --- a/db/logformat.h
> +++ b/db/logformat.h
> @@ -17,3 +17,4 @@
>   */
>  
>  void logformat_init(void);
> +void logres_init(void);
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index d2ab02a..5d56340 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -60,7 +60,7 @@
>  #define xfs_trans_roll			libxfs_trans_roll
>  #define xfs_trans_get_buf_map		libxfs_trans_get_buf_map
>  #define xfs_trans_resv_calc		libxfs_trans_resv_calc
> -
> +#define xfs_log_get_max_trans_res	libxfs_log_get_max_trans_res
>  #define xfs_attr_get			libxfs_attr_get
>  #define xfs_attr_set			libxfs_attr_set
>  #define xfs_attr_remove			libxfs_attr_remove
> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index 37018a7..524b1ef 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -690,6 +690,12 @@ Start logging output to
>  .IR filename ,
>  stop logging, or print the current logging status.
>  .TP
> +.B logres
> +Print transaction reservation size information for each transaction type.
> +This makes it easier to find discrepancies in the reservation calculations
> +between xfsprogs and the kernel, which will help when diagnosing minimum
> +log size calculation errors.
> +.TP
>  .BI "metadump [\-egow] " filename
>  Dumps metadata to a file. See
>  .BR xfs_metadump (8)
> 
> --
> 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
> 

[PATCH 08/16] xfs_repair: don't fail directory repairs when grabbing inodes

[Darrick J. Wong]

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

There are a few places where xfs_repair needs to be able to load a
damaged directory inode to perform repairs.  Since inline data fork
verifiers can now be customized, refactor libxfs_iget to enable
repair to get at this so that we don't crash in phase 6.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/attrset.c        |    6 ++++--
 include/xfs_inode.h |    6 ++++--
 libxfs/rdwr.c       |   25 +++++++++++++++++--------
 libxfs/trans.c      |    6 ++++--
 libxfs/util.c       |    2 +-
 repair/phase6.c     |   16 +++++++++++-----
 6 files changed, 41 insertions(+), 20 deletions(-)


diff --git a/db/attrset.c b/db/attrset.c
index ad3c8f3..457317a 100644
--- a/db/attrset.c
+++ b/db/attrset.c
@@ -151,7 +151,8 @@ attr_set_f(
 		value = NULL;
 	}
 
-	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip)) {
+	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
+			&xfs_default_ifork_ops)) {
 		dbprintf(_("failed to iget inode %llu\n"),
 			(unsigned long long)iocur_top->ino);
 		goto out;
@@ -226,7 +227,8 @@ attr_remove_f(
 
 	name = argv[optind];
 
-	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip)) {
+	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
+			&xfs_default_ifork_ops)) {
 		dbprintf(_("failed to iget inode %llu\n"),
 			(unsigned long long)iocur_top->ino);
 		goto out;
diff --git a/include/xfs_inode.h b/include/xfs_inode.h
index 92829a2..f29f0f0 100644
--- a/include/xfs_inode.h
+++ b/include/xfs_inode.h
@@ -162,9 +162,11 @@ extern void	libxfs_trans_ichgtime(struct xfs_trans *,
 extern int	libxfs_iflush_int (struct xfs_inode *, struct xfs_buf *);
 
 /* Inode Cache Interfaces */
-extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip);
+extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip,
+				struct xfs_ifork_ops *);
 extern int	libxfs_iget(struct xfs_mount *, struct xfs_trans *, xfs_ino_t,
-				uint, struct xfs_inode **);
+				uint, struct xfs_inode **,
+				struct xfs_ifork_ops *);
 extern void	libxfs_iput(struct xfs_inode *);
 
 #define IRELE(ip) libxfs_iput(ip)
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 3c5def2..416e89b 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1342,12 +1342,16 @@ extern kmem_zone_t	*xfs_inode_zone;
  */
 bool
 libxfs_inode_verify_forks(
-	struct xfs_inode	*ip)
+	struct xfs_inode	*ip,
+	struct xfs_ifork_ops	*ops)
 {
 	struct xfs_ifork	*ifp;
 	xfs_failaddr_t		fa;
 
-	fa = xfs_ifork_verify_data(ip, &xfs_default_ifork_ops);
+	if (!ops)
+		return true;
+
+	fa = xfs_ifork_verify_data(ip, ops);
 	if (fa) {
 		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
 		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
@@ -1355,7 +1359,7 @@ libxfs_inode_verify_forks(
 		return false;
 	}
 
-	fa = xfs_ifork_verify_attr(ip, &xfs_default_ifork_ops);
+	fa = xfs_ifork_verify_attr(ip, ops);
 	if (fa) {
 		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
 		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
@@ -1367,11 +1371,16 @@ libxfs_inode_verify_forks(
 }
 
 int
-libxfs_iget(xfs_mount_t *mp, xfs_trans_t *tp, xfs_ino_t ino, uint lock_flags,
-		xfs_inode_t **ipp)
+libxfs_iget(
+	struct xfs_mount	*mp,
+	struct xfs_trans	*tp,
+	xfs_ino_t		ino,
+	uint			lock_flags,
+	struct xfs_inode	**ipp,
+	struct xfs_ifork_ops	*ifork_ops)
 {
-	xfs_inode_t	*ip;
-	int		error = 0;
+	struct xfs_inode	*ip;
+	int			error = 0;
 
 	ip = kmem_zone_zalloc(xfs_inode_zone, 0);
 	if (!ip)
@@ -1386,7 +1395,7 @@ libxfs_iget(xfs_mount_t *mp, xfs_trans_t *tp, xfs_ino_t ino, uint lock_flags,
 		return error;
 	}
 
-	if (!libxfs_inode_verify_forks(ip)) {
+	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
 		libxfs_iput(ip);
 		return -EFSCORRUPTED;
 	}
diff --git a/libxfs/trans.c b/libxfs/trans.c
index 0e7b7ae..67b1117 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -243,9 +243,11 @@ libxfs_trans_iget(
 	xfs_inode_log_item_t	*iip;
 
 	if (tp == NULL)
-		return libxfs_iget(mp, tp, ino, lock_flags, ipp);
+		return libxfs_iget(mp, tp, ino, lock_flags, ipp,
+				&xfs_default_ifork_ops);
 
-	error = libxfs_iget(mp, tp, ino, lock_flags, &ip);
+	error = libxfs_iget(mp, tp, ino, lock_flags, &ip,
+			&xfs_default_ifork_ops);
 	if (error)
 		return error;
 	ASSERT(ip != NULL);
diff --git a/libxfs/util.c b/libxfs/util.c
index aac558c..611ab9c 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -494,7 +494,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
 		VFS_I(ip)->i_version++;
 
 	/* Check the inline fork data before we write out. */
-	if (!libxfs_inode_verify_forks(ip))
+	if (!libxfs_inode_verify_forks(ip, &xfs_default_ifork_ops))
 		return -EFSCORRUPTED;
 
 	/*
diff --git a/repair/phase6.c b/repair/phase6.c
index 1a398aa..aff83bc 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -927,7 +927,9 @@ mk_orphanage(xfs_mount_t *mp)
 	 * would have been cleared in phase3 and phase4.
 	 */
 
-	if ((i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip)))
+	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
+			&xfs_default_ifork_ops);
+	if (i)
 		do_error(_("%d - couldn't iget root inode to obtain %s\n"),
 			i, ORPHANAGE);
 
@@ -951,7 +953,9 @@ mk_orphanage(xfs_mount_t *mp)
 	 * use iget/ijoin instead of trans_iget because the ialloc
 	 * wrapper can commit the transaction and start a new one
 	 */
-/*	if ((i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip)))
+/*	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
+			&xfs_default_ifork_ops);
+	if (i)
 		do_error(_("%d - couldn't iget root inode to make %s\n"),
 			i, ORPHANAGE);*/
 
@@ -1066,7 +1070,8 @@ mv_orphanage(
 	xname.len = snprintf((char *)fname, sizeof(fname), "%llu",
 				(unsigned long long)ino);
 
-	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip);
+	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip,
+			&xfs_default_ifork_ops);
 	if (err)
 		do_error(_("%d - couldn't iget orphanage inode\n"), err);
 	/*
@@ -1078,7 +1083,8 @@ mv_orphanage(
 		xname.len = snprintf((char *)fname, sizeof(fname), "%llu.%d",
 					(unsigned long long)ino, ++incr);
 
-	if ((err = -libxfs_iget(mp, NULL, ino, 0, &ino_p)))
+	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);
+	if (err)
 		do_error(_("%d - couldn't iget disconnected inode\n"), err);
 
 	xname.type = libxfs_mode_to_ftype(VFS_I(ino_p)->i_mode);
@@ -2827,7 +2833,7 @@ process_dir_inode(
 
 	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
 
-	error = -libxfs_iget(mp, NULL, ino, 0, &ip);
+	error = -libxfs_iget(mp, NULL, ino, 0, &ip, NULL);
 	if (error) {
 		if (!no_modify)
 			do_error(

Re: [PATCH 08/16] xfs_repair: don't fail directory repairs when grabbing inodes

[Eric Sandeen]

On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> There are a few places where xfs_repair needs to be able to load a
> damaged directory inode to perform repairs.  Since inline data fork
> verifiers can now be customized, refactor libxfs_iget to enable
> repair to get at this so that we don't crash in phase 6.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>


Hm but there /is/ no custom verifier here, right?  Just NULL or default?
In that case does it make sense to be passing ops around vs. just
adding a "bool validate" arg in place of the NULL ops dance you have now?

> ---
>  db/attrset.c        |    6 ++++--
>  include/xfs_inode.h |    6 ++++--
>  libxfs/rdwr.c       |   25 +++++++++++++++++--------
>  libxfs/trans.c      |    6 ++++--
>  libxfs/util.c       |    2 +-
>  repair/phase6.c     |   16 +++++++++++-----
>  6 files changed, 41 insertions(+), 20 deletions(-)
> 
> 
> diff --git a/db/attrset.c b/db/attrset.c
> index ad3c8f3..457317a 100644
> --- a/db/attrset.c
> +++ b/db/attrset.c
> @@ -151,7 +151,8 @@ attr_set_f(
>  		value = NULL;
>  	}
>  
> -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip)) {
> +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> +			&xfs_default_ifork_ops)) {
>  		dbprintf(_("failed to iget inode %llu\n"),
>  			(unsigned long long)iocur_top->ino);
>  		goto out;
> @@ -226,7 +227,8 @@ attr_remove_f(
>  
>  	name = argv[optind];
>  
> -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip)) {
> +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> +			&xfs_default_ifork_ops)) {
>  		dbprintf(_("failed to iget inode %llu\n"),
>  			(unsigned long long)iocur_top->ino);
>  		goto out;
> diff --git a/include/xfs_inode.h b/include/xfs_inode.h
> index 92829a2..f29f0f0 100644
> --- a/include/xfs_inode.h
> +++ b/include/xfs_inode.h
> @@ -162,9 +162,11 @@ extern void	libxfs_trans_ichgtime(struct xfs_trans *,
>  extern int	libxfs_iflush_int (struct xfs_inode *, struct xfs_buf *);
>  
>  /* Inode Cache Interfaces */
> -extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip);
> +extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip,
> +				struct xfs_ifork_ops *);
>  extern int	libxfs_iget(struct xfs_mount *, struct xfs_trans *, xfs_ino_t,
> -				uint, struct xfs_inode **);
> +				uint, struct xfs_inode **,
> +				struct xfs_ifork_ops *);
>  extern void	libxfs_iput(struct xfs_inode *);
>  
>  #define IRELE(ip) libxfs_iput(ip)
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 3c5def2..416e89b 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -1342,12 +1342,16 @@ extern kmem_zone_t	*xfs_inode_zone;
>   */
>  bool
>  libxfs_inode_verify_forks(
> -	struct xfs_inode	*ip)
> +	struct xfs_inode	*ip,
> +	struct xfs_ifork_ops	*ops)
>  {
>  	struct xfs_ifork	*ifp;
>  	xfs_failaddr_t		fa;
>  
> -	fa = xfs_ifork_verify_data(ip, &xfs_default_ifork_ops);
> +	if (!ops)
> +		return true;
> +
> +	fa = xfs_ifork_verify_data(ip, ops);
>  	if (fa) {
>  		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> @@ -1355,7 +1359,7 @@ libxfs_inode_verify_forks(
>  		return false;
>  	}
>  
> -	fa = xfs_ifork_verify_attr(ip, &xfs_default_ifork_ops);
> +	fa = xfs_ifork_verify_attr(ip, ops);
>  	if (fa) {
>  		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> @@ -1367,11 +1371,16 @@ libxfs_inode_verify_forks(
>  }
>  
>  int
> -libxfs_iget(xfs_mount_t *mp, xfs_trans_t *tp, xfs_ino_t ino, uint lock_flags,
> -		xfs_inode_t **ipp)
> +libxfs_iget(
> +	struct xfs_mount	*mp,
> +	struct xfs_trans	*tp,
> +	xfs_ino_t		ino,
> +	uint			lock_flags,
> +	struct xfs_inode	**ipp,
> +	struct xfs_ifork_ops	*ifork_ops)
>  {
> -	xfs_inode_t	*ip;
> -	int		error = 0;
> +	struct xfs_inode	*ip;
> +	int			error = 0;
>  
>  	ip = kmem_zone_zalloc(xfs_inode_zone, 0);
>  	if (!ip)
> @@ -1386,7 +1395,7 @@ libxfs_iget(xfs_mount_t *mp, xfs_trans_t *tp, xfs_ino_t ino, uint lock_flags,
>  		return error;
>  	}
>  
> -	if (!libxfs_inode_verify_forks(ip)) {
> +	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
>  		libxfs_iput(ip);
>  		return -EFSCORRUPTED;
>  	}
> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index 0e7b7ae..67b1117 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -243,9 +243,11 @@ libxfs_trans_iget(
>  	xfs_inode_log_item_t	*iip;
>  
>  	if (tp == NULL)
> -		return libxfs_iget(mp, tp, ino, lock_flags, ipp);
> +		return libxfs_iget(mp, tp, ino, lock_flags, ipp,
> +				&xfs_default_ifork_ops);
>  
> -	error = libxfs_iget(mp, tp, ino, lock_flags, &ip);
> +	error = libxfs_iget(mp, tp, ino, lock_flags, &ip,
> +			&xfs_default_ifork_ops);
>  	if (error)
>  		return error;
>  	ASSERT(ip != NULL);
> diff --git a/libxfs/util.c b/libxfs/util.c
> index aac558c..611ab9c 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -494,7 +494,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
>  		VFS_I(ip)->i_version++;
>  
>  	/* Check the inline fork data before we write out. */
> -	if (!libxfs_inode_verify_forks(ip))
> +	if (!libxfs_inode_verify_forks(ip, &xfs_default_ifork_ops))
>  		return -EFSCORRUPTED;
>  
>  	/*
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 1a398aa..aff83bc 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -927,7 +927,9 @@ mk_orphanage(xfs_mount_t *mp)
>  	 * would have been cleared in phase3 and phase4.
>  	 */
>  
> -	if ((i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip)))
> +	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
> +			&xfs_default_ifork_ops);
> +	if (i)
>  		do_error(_("%d - couldn't iget root inode to obtain %s\n"),
>  			i, ORPHANAGE);
>  
> @@ -951,7 +953,9 @@ mk_orphanage(xfs_mount_t *mp)
>  	 * use iget/ijoin instead of trans_iget because the ialloc
>  	 * wrapper can commit the transaction and start a new one
>  	 */
> -/*	if ((i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip)))
> +/*	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
> +			&xfs_default_ifork_ops);
> +	if (i)
>  		do_error(_("%d - couldn't iget root inode to make %s\n"),
>  			i, ORPHANAGE);*/
>  
> @@ -1066,7 +1070,8 @@ mv_orphanage(
>  	xname.len = snprintf((char *)fname, sizeof(fname), "%llu",
>  				(unsigned long long)ino);
>  
> -	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip);
> +	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip,
> +			&xfs_default_ifork_ops);
>  	if (err)
>  		do_error(_("%d - couldn't iget orphanage inode\n"), err);
>  	/*
> @@ -1078,7 +1083,8 @@ mv_orphanage(
>  		xname.len = snprintf((char *)fname, sizeof(fname), "%llu.%d",
>  					(unsigned long long)ino, ++incr);
>  
> -	if ((err = -libxfs_iget(mp, NULL, ino, 0, &ino_p)))
> +	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);
> +	if (err)
>  		do_error(_("%d - couldn't iget disconnected inode\n"), err);
>  
>  	xname.type = libxfs_mode_to_ftype(VFS_I(ino_p)->i_mode);
> @@ -2827,7 +2833,7 @@ process_dir_inode(
>  
>  	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
>  
> -	error = -libxfs_iget(mp, NULL, ino, 0, &ip);
> +	error = -libxfs_iget(mp, NULL, ino, 0, &ip, NULL);
>  	if (error) {
>  		if (!no_modify)
>  			do_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 08/16] xfs_repair: don't fail directory repairs when grabbing inodes

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 01:23:51PM -0600, Eric Sandeen wrote:
> On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > There are a few places where xfs_repair needs to be able to load a
> > damaged directory inode to perform repairs.  Since inline data fork
> > verifiers can now be customized, refactor libxfs_iget to enable
> > repair to get at this so that we don't crash in phase 6.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 
> Hm but there /is/ no custom verifier here, right?  Just NULL or default?
> In that case does it make sense to be passing ops around vs. just
> adding a "bool validate" arg in place of the NULL ops dance you have now?

Originally it was just an iget flag, but I think Dave wanted repair to
have the ability to pass in a custom ifork_ops.  Theoretically we could
have an ifork verifier that does the same things as the libxfs ones
*except* for the places where repair deliberately zeroes a field that it
intends to set in a future pass.  I don't really like the prospect of
maintaining code almost-duplication, but in the mean time this restores
xfs_repair's ability to fix directory .. entries which we lost in
4.10).

--D

> > ---
> >  db/attrset.c        |    6 ++++--
> >  include/xfs_inode.h |    6 ++++--
> >  libxfs/rdwr.c       |   25 +++++++++++++++++--------
> >  libxfs/trans.c      |    6 ++++--
> >  libxfs/util.c       |    2 +-
> >  repair/phase6.c     |   16 +++++++++++-----
> >  6 files changed, 41 insertions(+), 20 deletions(-)
> > 
> > 
> > diff --git a/db/attrset.c b/db/attrset.c
> > index ad3c8f3..457317a 100644
> > --- a/db/attrset.c
> > +++ b/db/attrset.c
> > @@ -151,7 +151,8 @@ attr_set_f(
> >  		value = NULL;
> >  	}
> >  
> > -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip)) {
> > +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> > +			&xfs_default_ifork_ops)) {
> >  		dbprintf(_("failed to iget inode %llu\n"),
> >  			(unsigned long long)iocur_top->ino);
> >  		goto out;
> > @@ -226,7 +227,8 @@ attr_remove_f(
> >  
> >  	name = argv[optind];
> >  
> > -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip)) {
> > +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> > +			&xfs_default_ifork_ops)) {
> >  		dbprintf(_("failed to iget inode %llu\n"),
> >  			(unsigned long long)iocur_top->ino);
> >  		goto out;
> > diff --git a/include/xfs_inode.h b/include/xfs_inode.h
> > index 92829a2..f29f0f0 100644
> > --- a/include/xfs_inode.h
> > +++ b/include/xfs_inode.h
> > @@ -162,9 +162,11 @@ extern void	libxfs_trans_ichgtime(struct xfs_trans *,
> >  extern int	libxfs_iflush_int (struct xfs_inode *, struct xfs_buf *);
> >  
> >  /* Inode Cache Interfaces */
> > -extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip);
> > +extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip,
> > +				struct xfs_ifork_ops *);
> >  extern int	libxfs_iget(struct xfs_mount *, struct xfs_trans *, xfs_ino_t,
> > -				uint, struct xfs_inode **);
> > +				uint, struct xfs_inode **,
> > +				struct xfs_ifork_ops *);
> >  extern void	libxfs_iput(struct xfs_inode *);
> >  
> >  #define IRELE(ip) libxfs_iput(ip)
> > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> > index 3c5def2..416e89b 100644
> > --- a/libxfs/rdwr.c
> > +++ b/libxfs/rdwr.c
> > @@ -1342,12 +1342,16 @@ extern kmem_zone_t	*xfs_inode_zone;
> >   */
> >  bool
> >  libxfs_inode_verify_forks(
> > -	struct xfs_inode	*ip)
> > +	struct xfs_inode	*ip,
> > +	struct xfs_ifork_ops	*ops)
> >  {
> >  	struct xfs_ifork	*ifp;
> >  	xfs_failaddr_t		fa;
> >  
> > -	fa = xfs_ifork_verify_data(ip, &xfs_default_ifork_ops);
> > +	if (!ops)
> > +		return true;
> > +
> > +	fa = xfs_ifork_verify_data(ip, ops);
> >  	if (fa) {
> >  		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> >  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> > @@ -1355,7 +1359,7 @@ libxfs_inode_verify_forks(
> >  		return false;
> >  	}
> >  
> > -	fa = xfs_ifork_verify_attr(ip, &xfs_default_ifork_ops);
> > +	fa = xfs_ifork_verify_attr(ip, ops);
> >  	if (fa) {
> >  		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> >  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> > @@ -1367,11 +1371,16 @@ libxfs_inode_verify_forks(
> >  }
> >  
> >  int
> > -libxfs_iget(xfs_mount_t *mp, xfs_trans_t *tp, xfs_ino_t ino, uint lock_flags,
> > -		xfs_inode_t **ipp)
> > +libxfs_iget(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_trans	*tp,
> > +	xfs_ino_t		ino,
> > +	uint			lock_flags,
> > +	struct xfs_inode	**ipp,
> > +	struct xfs_ifork_ops	*ifork_ops)
> >  {
> > -	xfs_inode_t	*ip;
> > -	int		error = 0;
> > +	struct xfs_inode	*ip;
> > +	int			error = 0;
> >  
> >  	ip = kmem_zone_zalloc(xfs_inode_zone, 0);
> >  	if (!ip)
> > @@ -1386,7 +1395,7 @@ libxfs_iget(xfs_mount_t *mp, xfs_trans_t *tp, xfs_ino_t ino, uint lock_flags,
> >  		return error;
> >  	}
> >  
> > -	if (!libxfs_inode_verify_forks(ip)) {
> > +	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
> >  		libxfs_iput(ip);
> >  		return -EFSCORRUPTED;
> >  	}
> > diff --git a/libxfs/trans.c b/libxfs/trans.c
> > index 0e7b7ae..67b1117 100644
> > --- a/libxfs/trans.c
> > +++ b/libxfs/trans.c
> > @@ -243,9 +243,11 @@ libxfs_trans_iget(
> >  	xfs_inode_log_item_t	*iip;
> >  
> >  	if (tp == NULL)
> > -		return libxfs_iget(mp, tp, ino, lock_flags, ipp);
> > +		return libxfs_iget(mp, tp, ino, lock_flags, ipp,
> > +				&xfs_default_ifork_ops);
> >  
> > -	error = libxfs_iget(mp, tp, ino, lock_flags, &ip);
> > +	error = libxfs_iget(mp, tp, ino, lock_flags, &ip,
> > +			&xfs_default_ifork_ops);
> >  	if (error)
> >  		return error;
> >  	ASSERT(ip != NULL);
> > diff --git a/libxfs/util.c b/libxfs/util.c
> > index aac558c..611ab9c 100644
> > --- a/libxfs/util.c
> > +++ b/libxfs/util.c
> > @@ -494,7 +494,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
> >  		VFS_I(ip)->i_version++;
> >  
> >  	/* Check the inline fork data before we write out. */
> > -	if (!libxfs_inode_verify_forks(ip))
> > +	if (!libxfs_inode_verify_forks(ip, &xfs_default_ifork_ops))
> >  		return -EFSCORRUPTED;
> >  
> >  	/*
> > diff --git a/repair/phase6.c b/repair/phase6.c
> > index 1a398aa..aff83bc 100644
> > --- a/repair/phase6.c
> > +++ b/repair/phase6.c
> > @@ -927,7 +927,9 @@ mk_orphanage(xfs_mount_t *mp)
> >  	 * would have been cleared in phase3 and phase4.
> >  	 */
> >  
> > -	if ((i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip)))
> > +	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
> > +			&xfs_default_ifork_ops);
> > +	if (i)
> >  		do_error(_("%d - couldn't iget root inode to obtain %s\n"),
> >  			i, ORPHANAGE);
> >  
> > @@ -951,7 +953,9 @@ mk_orphanage(xfs_mount_t *mp)
> >  	 * use iget/ijoin instead of trans_iget because the ialloc
> >  	 * wrapper can commit the transaction and start a new one
> >  	 */
> > -/*	if ((i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip)))
> > +/*	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
> > +			&xfs_default_ifork_ops);
> > +	if (i)
> >  		do_error(_("%d - couldn't iget root inode to make %s\n"),
> >  			i, ORPHANAGE);*/
> >  
> > @@ -1066,7 +1070,8 @@ mv_orphanage(
> >  	xname.len = snprintf((char *)fname, sizeof(fname), "%llu",
> >  				(unsigned long long)ino);
> >  
> > -	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip);
> > +	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip,
> > +			&xfs_default_ifork_ops);
> >  	if (err)
> >  		do_error(_("%d - couldn't iget orphanage inode\n"), err);
> >  	/*
> > @@ -1078,7 +1083,8 @@ mv_orphanage(
> >  		xname.len = snprintf((char *)fname, sizeof(fname), "%llu.%d",
> >  					(unsigned long long)ino, ++incr);
> >  
> > -	if ((err = -libxfs_iget(mp, NULL, ino, 0, &ino_p)))
> > +	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);
> > +	if (err)
> >  		do_error(_("%d - couldn't iget disconnected inode\n"), err);
> >  
> >  	xname.type = libxfs_mode_to_ftype(VFS_I(ino_p)->i_mode);
> > @@ -2827,7 +2833,7 @@ process_dir_inode(
> >  
> >  	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
> >  
> > -	error = -libxfs_iget(mp, NULL, ino, 0, &ip);
> > +	error = -libxfs_iget(mp, NULL, ino, 0, &ip, NULL);
> >  	if (error) {
> >  		if (!no_modify)
> >  			do_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 08/16] xfs_repair: don't fail directory repairs when grabbing inodes

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 11:43:29AM -0800, Darrick J. Wong wrote:
> On Tue, Mar 06, 2018 at 01:23:51PM -0600, Eric Sandeen wrote:
> > On 3/1/18 1:13 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > There are a few places where xfs_repair needs to be able to load a
> > > damaged directory inode to perform repairs.  Since inline data fork
> > > verifiers can now be customized, refactor libxfs_iget to enable
> > > repair to get at this so that we don't crash in phase 6.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > 
> > Hm but there /is/ no custom verifier here, right?  Just NULL or default?
> > In that case does it make sense to be passing ops around vs. just
> > adding a "bool validate" arg in place of the NULL ops dance you have now?
> 
> Originally it was just an iget flag, but I think Dave wanted repair to
> have the ability to pass in a custom ifork_ops.  Theoretically we could
> have an ifork verifier that does the same things as the libxfs ones
> *except* for the places where repair deliberately zeroes a field that it
> intends to set in a future pass.  I don't really like the prospect of
> maintaining code almost-duplication, but in the mean time this restores
> xfs_repair's ability to fix directory .. entries which we lost in
> 4.10).

As a more concrete example, a phase6 inline directory verifier might
look like this instead of the NULL ops we do now:

xfs_failaddr_t
phase6_dir2_sf_verify(
	struct xfs_inode		*ip)
{
	struct xfs_dir2_sf_hdr		*sfp;
	const struct xfs_dir_ops	*dops;
	struct xfs_ifork		*ifp;
	int				size;

	dops = xfs_dir_get_ops(mp, NULL);
	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
	sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
	size = ifp->if_bytes;

	/*
	 * Give up if the directory is way too short.
	 */
	if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) ||
	    size < xfs_dir2_sf_hdr_size(sfp->i8count))
		return __this_address;

	/*
	 * Phase 4 zeroes the .. entry if it's garbage and wants phase 6
	 * to fix it, so let that through.
	 */
	if (dops->sf_get_parent_ino(sfp) == 0)
		return NULL;

	/* Otherwise all regular libxfs checks apply. */
	return xfs_dir2_sf_verify(ip);
}

--D

> --D
> 
> > > ---
> > >  db/attrset.c        |    6 ++++--
> > >  include/xfs_inode.h |    6 ++++--
> > >  libxfs/rdwr.c       |   25 +++++++++++++++++--------
> > >  libxfs/trans.c      |    6 ++++--
> > >  libxfs/util.c       |    2 +-
> > >  repair/phase6.c     |   16 +++++++++++-----
> > >  6 files changed, 41 insertions(+), 20 deletions(-)
> > > 
> > > 
> > > diff --git a/db/attrset.c b/db/attrset.c
> > > index ad3c8f3..457317a 100644
> > > --- a/db/attrset.c
> > > +++ b/db/attrset.c
> > > @@ -151,7 +151,8 @@ attr_set_f(
> > >  		value = NULL;
> > >  	}
> > >  
> > > -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip)) {
> > > +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> > > +			&xfs_default_ifork_ops)) {
> > >  		dbprintf(_("failed to iget inode %llu\n"),
> > >  			(unsigned long long)iocur_top->ino);
> > >  		goto out;
> > > @@ -226,7 +227,8 @@ attr_remove_f(
> > >  
> > >  	name = argv[optind];
> > >  
> > > -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip)) {
> > > +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> > > +			&xfs_default_ifork_ops)) {
> > >  		dbprintf(_("failed to iget inode %llu\n"),
> > >  			(unsigned long long)iocur_top->ino);
> > >  		goto out;
> > > diff --git a/include/xfs_inode.h b/include/xfs_inode.h
> > > index 92829a2..f29f0f0 100644
> > > --- a/include/xfs_inode.h
> > > +++ b/include/xfs_inode.h
> > > @@ -162,9 +162,11 @@ extern void	libxfs_trans_ichgtime(struct xfs_trans *,
> > >  extern int	libxfs_iflush_int (struct xfs_inode *, struct xfs_buf *);
> > >  
> > >  /* Inode Cache Interfaces */
> > > -extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip);
> > > +extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip,
> > > +				struct xfs_ifork_ops *);
> > >  extern int	libxfs_iget(struct xfs_mount *, struct xfs_trans *, xfs_ino_t,
> > > -				uint, struct xfs_inode **);
> > > +				uint, struct xfs_inode **,
> > > +				struct xfs_ifork_ops *);
> > >  extern void	libxfs_iput(struct xfs_inode *);
> > >  
> > >  #define IRELE(ip) libxfs_iput(ip)
> > > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> > > index 3c5def2..416e89b 100644
> > > --- a/libxfs/rdwr.c
> > > +++ b/libxfs/rdwr.c
> > > @@ -1342,12 +1342,16 @@ extern kmem_zone_t	*xfs_inode_zone;
> > >   */
> > >  bool
> > >  libxfs_inode_verify_forks(
> > > -	struct xfs_inode	*ip)
> > > +	struct xfs_inode	*ip,
> > > +	struct xfs_ifork_ops	*ops)
> > >  {
> > >  	struct xfs_ifork	*ifp;
> > >  	xfs_failaddr_t		fa;
> > >  
> > > -	fa = xfs_ifork_verify_data(ip, &xfs_default_ifork_ops);
> > > +	if (!ops)
> > > +		return true;
> > > +
> > > +	fa = xfs_ifork_verify_data(ip, ops);
> > >  	if (fa) {
> > >  		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > >  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> > > @@ -1355,7 +1359,7 @@ libxfs_inode_verify_forks(
> > >  		return false;
> > >  	}
> > >  
> > > -	fa = xfs_ifork_verify_attr(ip, &xfs_default_ifork_ops);
> > > +	fa = xfs_ifork_verify_attr(ip, ops);
> > >  	if (fa) {
> > >  		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> > >  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> > > @@ -1367,11 +1371,16 @@ libxfs_inode_verify_forks(
> > >  }
> > >  
> > >  int
> > > -libxfs_iget(xfs_mount_t *mp, xfs_trans_t *tp, xfs_ino_t ino, uint lock_flags,
> > > -		xfs_inode_t **ipp)
> > > +libxfs_iget(
> > > +	struct xfs_mount	*mp,
> > > +	struct xfs_trans	*tp,
> > > +	xfs_ino_t		ino,
> > > +	uint			lock_flags,
> > > +	struct xfs_inode	**ipp,
> > > +	struct xfs_ifork_ops	*ifork_ops)
> > >  {
> > > -	xfs_inode_t	*ip;
> > > -	int		error = 0;
> > > +	struct xfs_inode	*ip;
> > > +	int			error = 0;
> > >  
> > >  	ip = kmem_zone_zalloc(xfs_inode_zone, 0);
> > >  	if (!ip)
> > > @@ -1386,7 +1395,7 @@ libxfs_iget(xfs_mount_t *mp, xfs_trans_t *tp, xfs_ino_t ino, uint lock_flags,
> > >  		return error;
> > >  	}
> > >  
> > > -	if (!libxfs_inode_verify_forks(ip)) {
> > > +	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
> > >  		libxfs_iput(ip);
> > >  		return -EFSCORRUPTED;
> > >  	}
> > > diff --git a/libxfs/trans.c b/libxfs/trans.c
> > > index 0e7b7ae..67b1117 100644
> > > --- a/libxfs/trans.c
> > > +++ b/libxfs/trans.c
> > > @@ -243,9 +243,11 @@ libxfs_trans_iget(
> > >  	xfs_inode_log_item_t	*iip;
> > >  
> > >  	if (tp == NULL)
> > > -		return libxfs_iget(mp, tp, ino, lock_flags, ipp);
> > > +		return libxfs_iget(mp, tp, ino, lock_flags, ipp,
> > > +				&xfs_default_ifork_ops);
> > >  
> > > -	error = libxfs_iget(mp, tp, ino, lock_flags, &ip);
> > > +	error = libxfs_iget(mp, tp, ino, lock_flags, &ip,
> > > +			&xfs_default_ifork_ops);
> > >  	if (error)
> > >  		return error;
> > >  	ASSERT(ip != NULL);
> > > diff --git a/libxfs/util.c b/libxfs/util.c
> > > index aac558c..611ab9c 100644
> > > --- a/libxfs/util.c
> > > +++ b/libxfs/util.c
> > > @@ -494,7 +494,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
> > >  		VFS_I(ip)->i_version++;
> > >  
> > >  	/* Check the inline fork data before we write out. */
> > > -	if (!libxfs_inode_verify_forks(ip))
> > > +	if (!libxfs_inode_verify_forks(ip, &xfs_default_ifork_ops))
> > >  		return -EFSCORRUPTED;
> > >  
> > >  	/*
> > > diff --git a/repair/phase6.c b/repair/phase6.c
> > > index 1a398aa..aff83bc 100644
> > > --- a/repair/phase6.c
> > > +++ b/repair/phase6.c
> > > @@ -927,7 +927,9 @@ mk_orphanage(xfs_mount_t *mp)
> > >  	 * would have been cleared in phase3 and phase4.
> > >  	 */
> > >  
> > > -	if ((i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip)))
> > > +	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
> > > +			&xfs_default_ifork_ops);
> > > +	if (i)
> > >  		do_error(_("%d - couldn't iget root inode to obtain %s\n"),
> > >  			i, ORPHANAGE);
> > >  
> > > @@ -951,7 +953,9 @@ mk_orphanage(xfs_mount_t *mp)
> > >  	 * use iget/ijoin instead of trans_iget because the ialloc
> > >  	 * wrapper can commit the transaction and start a new one
> > >  	 */
> > > -/*	if ((i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip)))
> > > +/*	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
> > > +			&xfs_default_ifork_ops);
> > > +	if (i)
> > >  		do_error(_("%d - couldn't iget root inode to make %s\n"),
> > >  			i, ORPHANAGE);*/
> > >  
> > > @@ -1066,7 +1070,8 @@ mv_orphanage(
> > >  	xname.len = snprintf((char *)fname, sizeof(fname), "%llu",
> > >  				(unsigned long long)ino);
> > >  
> > > -	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip);
> > > +	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip,
> > > +			&xfs_default_ifork_ops);
> > >  	if (err)
> > >  		do_error(_("%d - couldn't iget orphanage inode\n"), err);
> > >  	/*
> > > @@ -1078,7 +1083,8 @@ mv_orphanage(
> > >  		xname.len = snprintf((char *)fname, sizeof(fname), "%llu.%d",
> > >  					(unsigned long long)ino, ++incr);
> > >  
> > > -	if ((err = -libxfs_iget(mp, NULL, ino, 0, &ino_p)))
> > > +	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);
> > > +	if (err)
> > >  		do_error(_("%d - couldn't iget disconnected inode\n"), err);
> > >  
> > >  	xname.type = libxfs_mode_to_ftype(VFS_I(ino_p)->i_mode);
> > > @@ -2827,7 +2833,7 @@ process_dir_inode(
> > >  
> > >  	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
> > >  
> > > -	error = -libxfs_iget(mp, NULL, ino, 0, &ip);
> > > +	error = -libxfs_iget(mp, NULL, ino, 0, &ip, NULL);
> > >  	if (error) {
> > >  		if (!no_modify)
> > >  			do_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
> --
> 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

[PATCH 09/16] misc: enable link time optimization, if requested

[Darrick J. Wong]

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

Enable link time optimization (LTO) if the builder requests it.  The
extra link optimization results in smaller binaries.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 configure.ac          |   12 ++++++++++++
 debian/rules          |    4 ++--
 include/builddefs.in  |    9 +++++++++
 m4/package_libcdev.m4 |   26 ++++++++++++++++++++++++++
 4 files changed, 49 insertions(+), 2 deletions(-)


diff --git a/configure.ac b/configure.ac
index 3a0ab18..9cfc661 100644
--- a/configure.ac
+++ b/configure.ac
@@ -90,6 +90,11 @@ AC_ARG_ENABLE(threadsan,
 	enable_threadsan=no)
 AC_SUBST(enable_threadsan)
 
+AC_ARG_ENABLE(lto,
+[ --enable-lto=[yes/no]      Enable link time optimization (LTO) [default=probe]],,
+	enable_lto=probe)
+AC_SUBST(enable_lto)
+
 #
 # If the user specified a libdir ending in lib64 do not append another
 # 64 to the library names.
@@ -206,6 +211,13 @@ if test "$have_threadsan" = "yes" && test "$have_addrsan" = "yes"; then
         AC_MSG_WARN([ADDRSAN and THREADSAN are not known to work together.])
 fi
 
+if test "$enable_lto" = "yes" || test "$enable_lto" = "probe"; then
+        AC_PACKAGE_CHECK_LTO
+fi
+if test "$enable_lto" = "yes" && test "$have_lto" != "yes"; then
+        AC_MSG_ERROR([LTO not supported by compiler.])
+fi
+
 AC_CHECK_SIZEOF([long])
 AC_CHECK_SIZEOF([char *])
 AC_TYPE_UMODE_T
diff --git a/debian/rules b/debian/rules
index 4cba165..cb4fa22 100755
--- a/debian/rules
+++ b/debian/rules
@@ -20,9 +20,9 @@ stdenv = @GZIP=-q; export GZIP;
 
 options = export DEBUG=-DNDEBUG DISTRIBUTION=debian \
 	  INSTALL_USER=root INSTALL_GROUP=root \
-	  LOCAL_CONFIGURE_OPTIONS="--enable-readline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan" ;
+	  LOCAL_CONFIGURE_OPTIONS="--enable-readline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ;
 diopts  = $(options) \
-	  export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan" ;
+	  export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ;
 checkdir = test -f debian/rules
 
 build: build-arch build-indep
diff --git a/include/builddefs.in b/include/builddefs.in
index df76b2c..b0bf9f1 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -176,6 +176,15 @@ endif
 SANITIZER_CFLAGS += @addrsan_cflags@ @threadsan_cflags@ @ubsan_cflags@
 SANITIZER_LDFLAGS += @addrsan_ldflags@ @threadsan_ldflags@ @ubsan_ldflags@
 
+# Use special ar/ranlib wrappers if we have lto
+HAVE_LTO = @have_lto@
+ifeq ($(HAVE_LTO),yes)
+OPTIMIZER += @lto_cflags@
+LOADERFLAGS += @lto_ldflags@
+AR = @gcc_ar@
+RANLIB = @gcc_ranlib@
+endif
+
 GCFLAGS = $(DEBUG) \
 	  -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\"  \
 	  -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs
diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
index 9258c27..52ddac2 100644
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -420,3 +420,29 @@ AC_DEFUN([AC_HAVE_HDIO_GETGEO],
        AC_MSG_RESULT(no))
     AC_SUBST(have_hdio_getgeo)
   ])
+
+AC_DEFUN([AC_PACKAGE_CHECK_LTO],
+  [ AC_MSG_CHECKING([if C compiler supports LTO])
+    OLD_CFLAGS="$CFLAGS"
+    OLD_LDFLAGS="$LDFLAGS"
+    LTO_FLAGS="-flto -ffat-lto-objects"
+    CFLAGS="$CFLAGS $LTO_FLAGS"
+    LDFLAGS="$LDFLAGS $LTO_FLAGS"
+    AC_LINK_IFELSE([AC_LANG_PROGRAM([])],
+        [AC_MSG_RESULT([yes])]
+        [lto_cflags=$LTO_FLAGS]
+        [lto_ldflags=$LTO_FLAGS]
+        [AC_PATH_PROG(gcc_ar, gcc-ar,,)]
+        [AC_PATH_PROG(gcc_ranlib, gcc-ranlib,,)],
+        [AC_MSG_RESULT([no])])
+    if test -x "$gcc_ar" && test -x "$gcc_ranlib"; then
+        have_lto=yes
+    fi
+    CFLAGS="${OLD_CFLAGS}"
+    LDFLAGS="${OLD_LDFLAGS}"
+    AC_SUBST(gcc_ar)
+    AC_SUBST(gcc_ranlib)
+    AC_SUBST(have_lto)
+    AC_SUBST(lto_cflags)
+    AC_SUBST(lto_ldflags)
+  ])

Re: [PATCH 09/16] misc: enable link time optimization, if requested

[Eric Sandeen]

On 3/1/18 1:14 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Enable link time optimization (LTO) if the builder requests it.  The
> extra link optimization results in smaller binaries.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

ok

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

> ---
>  configure.ac          |   12 ++++++++++++
>  debian/rules          |    4 ++--
>  include/builddefs.in  |    9 +++++++++
>  m4/package_libcdev.m4 |   26 ++++++++++++++++++++++++++
>  4 files changed, 49 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/configure.ac b/configure.ac
> index 3a0ab18..9cfc661 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -90,6 +90,11 @@ AC_ARG_ENABLE(threadsan,
>  	enable_threadsan=no)
>  AC_SUBST(enable_threadsan)
>  
> +AC_ARG_ENABLE(lto,
> +[ --enable-lto=[yes/no]      Enable link time optimization (LTO) [default=probe]],,
> +	enable_lto=probe)
> +AC_SUBST(enable_lto)
> +
>  #
>  # If the user specified a libdir ending in lib64 do not append another
>  # 64 to the library names.
> @@ -206,6 +211,13 @@ if test "$have_threadsan" = "yes" && test "$have_addrsan" = "yes"; then
>          AC_MSG_WARN([ADDRSAN and THREADSAN are not known to work together.])
>  fi
>  
> +if test "$enable_lto" = "yes" || test "$enable_lto" = "probe"; then
> +        AC_PACKAGE_CHECK_LTO
> +fi
> +if test "$enable_lto" = "yes" && test "$have_lto" != "yes"; then
> +        AC_MSG_ERROR([LTO not supported by compiler.])
> +fi
> +
>  AC_CHECK_SIZEOF([long])
>  AC_CHECK_SIZEOF([char *])
>  AC_TYPE_UMODE_T
> diff --git a/debian/rules b/debian/rules
> index 4cba165..cb4fa22 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -20,9 +20,9 @@ stdenv = @GZIP=-q; export GZIP;
>  
>  options = export DEBUG=-DNDEBUG DISTRIBUTION=debian \
>  	  INSTALL_USER=root INSTALL_GROUP=root \
> -	  LOCAL_CONFIGURE_OPTIONS="--enable-readline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan" ;
> +	  LOCAL_CONFIGURE_OPTIONS="--enable-readline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ;
>  diopts  = $(options) \
> -	  export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan" ;
> +	  export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ;
>  checkdir = test -f debian/rules
>  
>  build: build-arch build-indep
> diff --git a/include/builddefs.in b/include/builddefs.in
> index df76b2c..b0bf9f1 100644
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -176,6 +176,15 @@ endif
>  SANITIZER_CFLAGS += @addrsan_cflags@ @threadsan_cflags@ @ubsan_cflags@
>  SANITIZER_LDFLAGS += @addrsan_ldflags@ @threadsan_ldflags@ @ubsan_ldflags@
>  
> +# Use special ar/ranlib wrappers if we have lto
> +HAVE_LTO = @have_lto@
> +ifeq ($(HAVE_LTO),yes)
> +OPTIMIZER += @lto_cflags@
> +LOADERFLAGS += @lto_ldflags@
> +AR = @gcc_ar@
> +RANLIB = @gcc_ranlib@
> +endif
> +
>  GCFLAGS = $(DEBUG) \
>  	  -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\"  \
>  	  -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs
> diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
> index 9258c27..52ddac2 100644
> --- a/m4/package_libcdev.m4
> +++ b/m4/package_libcdev.m4
> @@ -420,3 +420,29 @@ AC_DEFUN([AC_HAVE_HDIO_GETGEO],
>         AC_MSG_RESULT(no))
>      AC_SUBST(have_hdio_getgeo)
>    ])
> +
> +AC_DEFUN([AC_PACKAGE_CHECK_LTO],
> +  [ AC_MSG_CHECKING([if C compiler supports LTO])
> +    OLD_CFLAGS="$CFLAGS"
> +    OLD_LDFLAGS="$LDFLAGS"
> +    LTO_FLAGS="-flto -ffat-lto-objects"
> +    CFLAGS="$CFLAGS $LTO_FLAGS"
> +    LDFLAGS="$LDFLAGS $LTO_FLAGS"
> +    AC_LINK_IFELSE([AC_LANG_PROGRAM([])],
> +        [AC_MSG_RESULT([yes])]
> +        [lto_cflags=$LTO_FLAGS]
> +        [lto_ldflags=$LTO_FLAGS]
> +        [AC_PATH_PROG(gcc_ar, gcc-ar,,)]
> +        [AC_PATH_PROG(gcc_ranlib, gcc-ranlib,,)],
> +        [AC_MSG_RESULT([no])])
> +    if test -x "$gcc_ar" && test -x "$gcc_ranlib"; then
> +        have_lto=yes
> +    fi
> +    CFLAGS="${OLD_CFLAGS}"
> +    LDFLAGS="${OLD_LDFLAGS}"
> +    AC_SUBST(gcc_ar)
> +    AC_SUBST(gcc_ranlib)
> +    AC_SUBST(have_lto)
> +    AC_SUBST(lto_cflags)
> +    AC_SUBST(lto_ldflags)
> +  ])
> 
> --
> 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
> 

[PATCH 10/16] libfrog: refactor fs geometry printing function

[Darrick J. Wong]

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

Move the fs geometry printing function to libfrog so that mkfs and others
can share.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/fsgeom.h |   24 ++++++++++++++++
 libfrog/Makefile |    1 +
 libfrog/fsgeom.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)
 create mode 100644 include/fsgeom.h
 create mode 100644 libfrog/fsgeom.c


diff --git a/include/fsgeom.h b/include/fsgeom.h
new file mode 100644
index 0000000..acf11bf
--- /dev/null
+++ b/include/fsgeom.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2000-2005 Silicon Graphics, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+#ifndef _LIBFROG_FSGEOM_H_
+#define _LIBFROG_FSGEOM_H_
+
+void xfs_report_geom(struct xfs_fsop_geom *geo, const char *mntpoint,
+		const char *logname, const char *rtname);
+
+#endif /* _LIBFROG_FSGEOM_H_ */
diff --git a/libfrog/Makefile b/libfrog/Makefile
index 230b08f..e601c79 100644
--- a/libfrog/Makefile
+++ b/libfrog/Makefile
@@ -13,6 +13,7 @@ LT_AGE = 0
 CFILES = \
 avl64.c \
 convert.c \
+fsgeom.c \
 list_sort.c \
 paths.c \
 projects.c \
diff --git a/libfrog/fsgeom.c b/libfrog/fsgeom.c
new file mode 100644
index 0000000..cbe847a
--- /dev/null
+++ b/libfrog/fsgeom.c
@@ -0,0 +1,82 @@
+/*
+ * Copyright (c) 2000-2005 Silicon Graphics, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+#include "libxfs.h"
+#include "fsgeom.h"
+
+void
+xfs_report_geom(
+	struct xfs_fsop_geom	*geo,
+	const char		*mntpoint,
+	const char		*logname,
+	const char		*rtname)
+{
+	int			isint;
+	int			lazycount;
+	int			dirversion;
+	int			logversion;
+	int			attrversion;
+	int			projid32bit;
+	int			crcs_enabled;
+	int			cimode;
+	int			ftype_enabled;
+	int			finobt_enabled;
+	int			spinodes;
+	int			rmapbt_enabled;
+	int			reflink_enabled;
+
+	isint = geo->logstart > 0;
+	lazycount = geo->flags & XFS_FSOP_GEOM_FLAGS_LAZYSB ? 1 : 0;
+	dirversion = geo->flags & XFS_FSOP_GEOM_FLAGS_DIRV2 ? 2 : 1;
+	logversion = geo->flags & XFS_FSOP_GEOM_FLAGS_LOGV2 ? 2 : 1;
+	attrversion = geo->flags & XFS_FSOP_GEOM_FLAGS_ATTR2 ? 2 : \
+			(geo->flags & XFS_FSOP_GEOM_FLAGS_ATTR ? 1 : 0);
+	cimode = geo->flags & XFS_FSOP_GEOM_FLAGS_DIRV2CI ? 1 : 0;
+	projid32bit = geo->flags & XFS_FSOP_GEOM_FLAGS_PROJID32 ? 1 : 0;
+	crcs_enabled = geo->flags & XFS_FSOP_GEOM_FLAGS_V5SB ? 1 : 0;
+	ftype_enabled = geo->flags & XFS_FSOP_GEOM_FLAGS_FTYPE ? 1 : 0;
+	finobt_enabled = geo->flags & XFS_FSOP_GEOM_FLAGS_FINOBT ? 1 : 0;
+	spinodes = geo->flags & XFS_FSOP_GEOM_FLAGS_SPINODES ? 1 : 0;
+	rmapbt_enabled = geo->flags & XFS_FSOP_GEOM_FLAGS_RMAPBT ? 1 : 0;
+	reflink_enabled = geo->flags & XFS_FSOP_GEOM_FLAGS_REFLINK ? 1 : 0;
+
+	printf(_(
+"meta-data=%-22s isize=%-6d agcount=%u, agsize=%u blks\n"
+"         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
+"         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u\n"
+"         =%-22s reflink=%u\n"
+"data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
+"         =%-22s sunit=%-6u swidth=%u blks\n"
+"naming   =version %-14u bsize=%-6u ascii-ci=%d, ftype=%d\n"
+"log      =%-22s bsize=%-6d blocks=%u, version=%d\n"
+"         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
+"realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
+		mntpoint, geo->inodesize, geo->agcount, geo->agblocks,
+		"", geo->sectsize, attrversion, projid32bit,
+		"", crcs_enabled, finobt_enabled, spinodes, rmapbt_enabled,
+		"", reflink_enabled,
+		"", geo->blocksize, (unsigned long long)geo->datablocks,
+			geo->imaxpct,
+		"", geo->sunit, geo->swidth,
+		dirversion, geo->dirblocksize, cimode, ftype_enabled,
+		isint ? _("internal log") : logname ? logname : _("external"),
+			geo->blocksize, geo->logblocks, logversion,
+		"", geo->logsectsize, geo->logsunit / geo->blocksize, lazycount,
+		!geo->rtblocks ? _("none") : rtname ? rtname : _("external"),
+		geo->rtextsize * geo->blocksize, (unsigned long long)geo->rtblocks,
+			(unsigned long long)geo->rtextents);
+}

[PATCH 11/16] mkfs: use geometry generation / helper functions

[Darrick J. Wong]

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

Since libxfs now has a function to fill out the geometry structure
and libfrog has a function to pretty-print the geometry, have mkfs
use the two helpers instead of open-coding it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 mkfs/xfs_mkfs.c          |   54 ++++++++++++++--------------------------------
 2 files changed, 18 insertions(+), 37 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 5d56340..a23a28d 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -150,5 +150,6 @@
 #define xfs_rmap_lookup_le_range	libxfs_rmap_lookup_le_range
 #define xfs_refc_block			libxfs_refc_block
 #define xfs_rmap_compare		libxfs_rmap_compare
+#define xfs_fs_geometry			libxfs_fs_geometry
 
 #endif /* __LIBXFS_API_DEFS_H__ */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f973b6b..c234b93 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -20,7 +20,7 @@
 #include <ctype.h>
 #include "xfs_multidisk.h"
 #include "libxcmd.h"
-
+#include "fsgeom.h"
 
 
 #define TERABYTES(count, blog)	((uint64_t)(count) << (40 - (blog)))
@@ -3172,40 +3172,6 @@ initialise_mount(
 	mp->m_sectbb_log = sbp->sb_sectlog - BBSHIFT;
 }
 
-static void
-print_mkfs_cfg(
-	struct mkfs_params	*cfg,
-	char			*dfile,
-	char			*logfile,
-	char			*rtfile)
-{
-	struct sb_feat_args	*fp = &cfg->sb_feat;
-
-	printf(_(
-"meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
-"         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
-"         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
-"data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
-"         =%-22s sunit=%-6u swidth=%u blks\n"
-"naming   =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
-"log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
-"         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
-"realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
-		dfile, cfg->inodesize, (long long)cfg->agcount,
-			(long long)cfg->agsize,
-		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
-		"", fp->crcs_enabled, fp->finobt, fp->spinodes, fp->rmapbt,
-			fp->reflink,
-		"", cfg->blocksize, (long long)cfg->dblocks, cfg->imaxpct,
-		"", cfg->dsunit, cfg->dswidth,
-		fp->dir_version, cfg->dirblocksize, fp->nci, fp->dirftype,
-		logfile, cfg->blocksize, (long long)cfg->logblocks,
-			fp->log_version,
-		"", cfg->lsectorsize, cfg->lsunit, fp->lazy_sb_counters,
-		rtfile, (int)cfg->rtextblocks << cfg->blocklog,
-			(long long)cfg->rtblocks, (long long)cfg->rtextents);
-}
-
 /*
  * Format everything from the generated config into the superblock that
  * will be used to initialise the on-disk superblock. This is the in-memory
@@ -3967,12 +3933,26 @@ main(
 	 */
 	calculate_log_size(&cfg, &cli, mp);
 
+	finish_superblock_setup(&cfg, mp, sbp);
+
+	/* Print the intended geometry of the fs. */
 	if (!quiet || dry_run) {
-		print_mkfs_cfg(&cfg, dfile, logfile, rtfile);
+		struct xfs_fsop_geom	geo;
+		int			error;
+
+		error = -libxfs_fs_geometry(sbp, &geo,
+				XFS_FS_GEOM_MAX_STRUCT_VER);
+		if (error) {
+			fprintf(stderr,
+	_("%s: failed to generate filesystem geometry\n"),
+				progname);
+			exit(1);
+		}
+
+		xfs_report_geom(&geo, dfile, logfile, rtfile);
 		if (dry_run)
 			exit(0);
 	}
-	finish_superblock_setup(&cfg, mp, sbp);
 
 	/*
 	 * we need the libxfs buffer cache from here on in.

[PATCH 12/16] xfs_db: add a superblock info command

[Darrick J. Wong]

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

Add an 'info' command to pretty-print the superblock geometry.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/Makefile       |    2 +
 db/command.c      |    1 +
 db/command.h      |    1 +
 db/info.c         |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 man/man8/xfs_db.8 |    6 ++++
 5 files changed, 83 insertions(+), 1 deletion(-)
 create mode 100644 db/info.c


diff --git a/db/Makefile b/db/Makefile
index 6caa634..c73d7f2 100644
--- a/db/Makefile
+++ b/db/Makefile
@@ -14,7 +14,7 @@ HFILES = addr.h agf.h agfl.h agi.h attr.h attrshort.h bit.h block.h bmap.h \
 	io.h logformat.h malloc.h metadump.h output.h print.h quit.h sb.h \
 	sig.h strvec.h text.h type.h write.h attrset.h symlink.h fsmap.h \
 	fuzz.h
-CFILES = $(HFILES:.h=.c) btdump.c
+CFILES = $(HFILES:.h=.c) btdump.c info.c
 LSRCFILES = xfs_admin.sh xfs_ncheck.sh xfs_metadump.sh
 
 LLDLIBS	= $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
diff --git a/db/command.c b/db/command.c
index 12ae5b7..087955e 100644
--- a/db/command.c
+++ b/db/command.c
@@ -136,6 +136,7 @@ init_commands(void)
 	fsmap_init();
 	help_init();
 	hash_init();
+	info_init();
 	inode_init();
 	input_init();
 	logres_init();
diff --git a/db/command.h b/db/command.h
index 9b4ed2d..84cd0b1 100644
--- a/db/command.h
+++ b/db/command.h
@@ -41,3 +41,4 @@ extern const cmdinfo_t	*find_command(const char *cmd);
 extern void		init_commands(void);
 
 extern void		btdump_init(void);
+extern void		info_init(void);
diff --git a/db/info.c b/db/info.c
new file mode 100644
index 0000000..7e7e4db
--- /dev/null
+++ b/db/info.c
@@ -0,0 +1,74 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <darrick.wong@oracle.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#include "libxfs.h"
+#include "command.h"
+#include "init.h"
+#include "output.h"
+#include "fsgeom.h"
+
+static void
+info_help(void)
+{
+	dbprintf(_(
+"\n"
+" Pretty-prints the filesystem geometry as derived from the superblock.\n"
+" The output has the same format as mkfs.\n"
+"\n"
+));
+
+}
+
+static int
+info_f(
+	int			argc,
+	char			**argv)
+{
+	struct xfs_fsop_geom	geo;
+	int			error;
+
+	error = -libxfs_fs_geometry(&mp->m_sb, &geo,
+			XFS_FS_GEOM_MAX_STRUCT_VER);
+	if (error) {
+		dbprintf(_("could not obtain geometry\n"));
+		exitcode = 1;
+		return 0;
+	}
+
+	xfs_report_geom(&geo, fsdevice, x.logname, x.rtname);
+	return 0;
+}
+
+static const struct cmdinfo info_cmd = {
+	.name =		"info",
+	.altname =	"i",
+	.cfunc =	info_f,
+	.argmin =	0,
+	.argmax =	0,
+	.canpush =	0,
+	.args =		NULL,
+	.oneline =	N_("dump superblock info"),
+	.help =		info_help,
+};
+
+void
+info_init(void)
+{
+	add_command(&info_cmd);
+}
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 524b1ef..e29821e 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -670,6 +670,12 @@ using the hash function of the XFS directory and attribute implementation.
 .BI "help [" command ]
 Print help for one or all commands.
 .TP
+.B info
+Displays selected geometry information about the filesystem.
+The output will have the same format that
+.BR "mkfs.xfs" "(8)"
+prints when creating a filesystem.
+.TP
 .BI "inode [" inode# ]
 Set the current inode number. If no
 .I inode#

Re: [PATCH 12/16] xfs_db: add a superblock info command

[Eric Sandeen]

On 3/1/18 1:14 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add an 'info' command to pretty-print the superblock geometry.

I'm on the fence about this; xfs_db generally just dumps representations
of individual disk structures.  This seems like a bit of an expansion of
its charter.  What is the use case for this?  Convince me that it's good
to add this code?

Thanks,
-Eric

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  db/Makefile       |    2 +
>  db/command.c      |    1 +
>  db/command.h      |    1 +
>  db/info.c         |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  man/man8/xfs_db.8 |    6 ++++
>  5 files changed, 83 insertions(+), 1 deletion(-)
>  create mode 100644 db/info.c
> 
> 
> diff --git a/db/Makefile b/db/Makefile
> index 6caa634..c73d7f2 100644
> --- a/db/Makefile
> +++ b/db/Makefile
> @@ -14,7 +14,7 @@ HFILES = addr.h agf.h agfl.h agi.h attr.h attrshort.h bit.h block.h bmap.h \
>  	io.h logformat.h malloc.h metadump.h output.h print.h quit.h sb.h \
>  	sig.h strvec.h text.h type.h write.h attrset.h symlink.h fsmap.h \
>  	fuzz.h
> -CFILES = $(HFILES:.h=.c) btdump.c
> +CFILES = $(HFILES:.h=.c) btdump.c info.c
>  LSRCFILES = xfs_admin.sh xfs_ncheck.sh xfs_metadump.sh
>  
>  LLDLIBS	= $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
> diff --git a/db/command.c b/db/command.c
> index 12ae5b7..087955e 100644
> --- a/db/command.c
> +++ b/db/command.c
> @@ -136,6 +136,7 @@ init_commands(void)
>  	fsmap_init();
>  	help_init();
>  	hash_init();
> +	info_init();
>  	inode_init();
>  	input_init();
>  	logres_init();
> diff --git a/db/command.h b/db/command.h
> index 9b4ed2d..84cd0b1 100644
> --- a/db/command.h
> +++ b/db/command.h
> @@ -41,3 +41,4 @@ extern const cmdinfo_t	*find_command(const char *cmd);
>  extern void		init_commands(void);
>  
>  extern void		btdump_init(void);
> +extern void		info_init(void);
> diff --git a/db/info.c b/db/info.c
> new file mode 100644
> index 0000000..7e7e4db
> --- /dev/null
> +++ b/db/info.c
> @@ -0,0 +1,74 @@
> +/*
> + * Copyright (C) 2018 Oracle.  All Rights Reserved.
> + *
> + * Author: Darrick J. Wong <darrick.wong@oracle.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
> + */
> +#include "libxfs.h"
> +#include "command.h"
> +#include "init.h"
> +#include "output.h"
> +#include "fsgeom.h"
> +
> +static void
> +info_help(void)
> +{
> +	dbprintf(_(
> +"\n"
> +" Pretty-prints the filesystem geometry as derived from the superblock.\n"
> +" The output has the same format as mkfs.\n"
> +"\n"
> +));
> +
> +}
> +
> +static int
> +info_f(
> +	int			argc,
> +	char			**argv)
> +{
> +	struct xfs_fsop_geom	geo;
> +	int			error;
> +
> +	error = -libxfs_fs_geometry(&mp->m_sb, &geo,
> +			XFS_FS_GEOM_MAX_STRUCT_VER);
> +	if (error) {
> +		dbprintf(_("could not obtain geometry\n"));
> +		exitcode = 1;
> +		return 0;
> +	}
> +
> +	xfs_report_geom(&geo, fsdevice, x.logname, x.rtname);
> +	return 0;
> +}
> +
> +static const struct cmdinfo info_cmd = {
> +	.name =		"info",
> +	.altname =	"i",
> +	.cfunc =	info_f,
> +	.argmin =	0,
> +	.argmax =	0,
> +	.canpush =	0,
> +	.args =		NULL,
> +	.oneline =	N_("dump superblock info"),
> +	.help =		info_help,
> +};
> +
> +void
> +info_init(void)
> +{
> +	add_command(&info_cmd);
> +}
> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index 524b1ef..e29821e 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -670,6 +670,12 @@ using the hash function of the XFS directory and attribute implementation.
>  .BI "help [" command ]
>  Print help for one or all commands.
>  .TP
> +.B info
> +Displays selected geometry information about the filesystem.
> +The output will have the same format that
> +.BR "mkfs.xfs" "(8)"
> +prints when creating a filesystem.
> +.TP
>  .BI "inode [" inode# ]
>  Set the current inode number. If no
>  .I inode#
> 
> --
> 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 12/16] xfs_db: add a superblock info command

[Eric Sandeen]

On 3/6/18 1:32 PM, Eric Sandeen wrote:
> On 3/1/18 1:14 PM, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Add an 'info' command to pretty-print the superblock geometry.
> I'm on the fence about this; xfs_db generally just dumps representations
> of individual disk structures.  This seems like a bit of an expansion of
> its charter.  What is the use case for this?  Convince me that it's good
> to add this code?
> 
> Thanks,
> -Eric
> 

Sorry - of course later patches make xfs_info work offline, but still,
I'd like to see some rationale for why we need that capability.  I'm all
for the code reduction of the common printing routine but what's the big
picture rational for teaching all these other tools to print mkfs-like output?

Thanks,
-Eric

Re: [PATCH 12/16] xfs_db: add a superblock info command

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 01:34:42PM -0600, Eric Sandeen wrote:
> 
> 
> On 3/6/18 1:32 PM, Eric Sandeen wrote:
> > On 3/1/18 1:14 PM, Darrick J. Wong wrote:
> >> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>
> >> Add an 'info' command to pretty-print the superblock geometry.
> > I'm on the fence about this; xfs_db generally just dumps representations
> > of individual disk structures.  This seems like a bit of an expansion of
> > its charter.  What is the use case for this?  Convince me that it's good
> > to add this code?

I occasionally (well ok pretty much every time) wish that when I'm
debugging an umountable filesystem w/ xfs_db I could just have it spit
out the familiar xfs_info output to figure out which features of the
user-visible features are enabled.

For xfstests there have been a few times where I'm writing some test and
it would be helpful to be able to figure out if the formatted image has
some feature enabled without having to mount the filesystem, and
wouldn't it be nice if we could reuse the same code that turns mkfs
output into environment variables?

That's a pretty minor use case though since I've so far always found a
way to code around the lack of it. :P  My primary itch is the debugging
case.

> > 
> > Thanks,
> > -Eric
> > 
> 
> Sorry - of course later patches make xfs_info work offline, but still,
> I'd like to see some rationale for why we need that capability.  I'm all
> for the code reduction of the common printing routine but what's the big
> picture rational for teaching all these other tools to print mkfs-like output?

Growing the filesystem falls under the umbrella of space management, so
in the long run (like after I finish putting online repair in) I intend
to make growfs a part of xfs_spaceman, and then the xfs_growfs command
becomes a wrapper around that.  Looking forward towards Dave's subvolume
thingy, I think it makes sense to be able to manage subvolumes entirely
via spaceman commands.

--D

> Thanks,
> -Eric
> --
> 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 12/16] xfs_db: add a superblock info command

[Dave Chinner]

On Tue, Mar 06, 2018 at 12:49:04PM -0800, Darrick J. Wong wrote:
> On Tue, Mar 06, 2018 at 01:34:42PM -0600, Eric Sandeen wrote:
> > Sorry - of course later patches make xfs_info work offline, but still,
> > I'd like to see some rationale for why we need that capability.  I'm all
> > for the code reduction of the common printing routine but what's the big
> > picture rational for teaching all these other tools to print mkfs-like output?
> 
> Growing the filesystem falls under the umbrella of space management, so
> in the long run (like after I finish putting online repair in) I intend
> to make growfs a part of xfs_spaceman, and then the xfs_growfs command
> becomes a wrapper around that.  Looking forward towards Dave's subvolume
> thingy, I think it makes sense to be able to manage subvolumes entirely
> via spaceman commands.

Agreed with this, as this is where I've implemented all the subvol
management stuff.

Also, when we have lots of subvoils sitting around as files, it'd
bee really nice to be able to have utilities that can identify them
from accessing the backing file....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 13/16] xfs_spaceman: add a superblock info command

[Darrick J. Wong]

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

Add an 'info' command to pretty-print the superblock geometry.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man8/xfs_spaceman.8 |    6 +++
 spaceman/Makefile       |    2 +
 spaceman/info.c         |   90 +++++++++++++++++++++++++++++++++++++++++++++++
 spaceman/init.c         |    1 +
 spaceman/space.h        |    1 +
 5 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 spaceman/info.c


diff --git a/man/man8/xfs_spaceman.8 b/man/man8/xfs_spaceman.8
index e4a9137..95c241c 100644
--- a/man/man8/xfs_spaceman.8
+++ b/man/man8/xfs_spaceman.8
@@ -84,6 +84,12 @@ Display a summary of the free space information found.
 .PD
 .RE
 .TP
+.B info
+Displays selected geometry information about the filesystem.
+The output will have the same format that
+.BR "xfs_info" "(8)"
+prints when querying a filesystem.
+.TP
 .BR "help [ " command " ]"
 Display a brief description of one or all commands.
 .TP
diff --git a/spaceman/Makefile b/spaceman/Makefile
index 8b31030..c1d903b 100644
--- a/spaceman/Makefile
+++ b/spaceman/Makefile
@@ -7,7 +7,7 @@ include $(TOPDIR)/include/builddefs
 
 LTCOMMAND = xfs_spaceman
 HFILES = init.h space.h
-CFILES = init.c file.c prealloc.c trim.c
+CFILES = info.c init.c file.c prealloc.c trim.c
 
 LLDLIBS = $(LIBXCMD) $(LIBFROG)
 LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
diff --git a/spaceman/info.c b/spaceman/info.c
new file mode 100644
index 0000000..6557b54
--- /dev/null
+++ b/spaceman/info.c
@@ -0,0 +1,90 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <darrick.wong@oracle.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#include "libxfs.h"
+#include "command.h"
+#include "init.h"
+#include "path.h"
+#include "space.h"
+#include "fsgeom.h"
+
+static void
+info_help(void)
+{
+	printf(_(
+"\n"
+" Pretty-prints the filesystem geometry as derived from the superblock.\n"
+" The output has the same format as mkfs.\n"
+"\n"
+));
+
+}
+
+static int
+info_f(
+	int			argc,
+	char			**argv)
+{
+	struct xfs_fsop_geom	geo;
+	int			error;
+
+	/* get the current filesystem size & geometry */
+	error = ioctl(file->fd, XFS_IOC_FSGEOMETRY, &geo);
+	if (error) {
+		/*
+		 * OK, new xfsctl barfed - back off and try earlier version
+		 * as we're probably running an older kernel version.
+		 * Only field added in the v2 geometry xfsctl is "logsunit"
+		 * so we'll zero that out for later display (as zero).
+		 */
+		geo.logsunit = 0;
+		error = ioctl(file->fd, XFS_IOC_FSGEOMETRY_V1, &geo);
+		if (error) {
+			fprintf(stderr, _(
+				"%s: cannot determine geometry of filesystem"
+				" mounted at %s: %s\n"),
+				progname, file->name, strerror(errno));
+			exitcode = 1;
+			return 0;
+		}
+	}
+
+	xfs_report_geom(&geo, file->fs_path.fs_name, file->fs_path.fs_log,
+			file->fs_path.fs_rt);
+	return 0;
+}
+
+static const struct cmdinfo info_cmd = {
+	.name =		"info",
+	.altname =	"i",
+	.cfunc =	info_f,
+	.argmin =	0,
+	.argmax =	0,
+	.canpush =	0,
+	.args =		NULL,
+	.flags =	CMD_FLAG_ONESHOT,
+	.oneline =	N_("dump superblock info"),
+	.help =		info_help,
+};
+
+void
+info_init(void)
+{
+	add_command(&info_cmd);
+}
diff --git a/spaceman/init.c b/spaceman/init.c
index b3eface..895504f 100644
--- a/spaceman/init.c
+++ b/spaceman/init.c
@@ -40,6 +40,7 @@ init_commands(void)
 {
 	print_init();
 	help_init();
+	info_init();
 	prealloc_init();
 	quit_init();
 	trim_init();
diff --git a/spaceman/space.h b/spaceman/space.h
index 5f4a8a0..d2a2543 100644
--- a/spaceman/space.h
+++ b/spaceman/space.h
@@ -42,5 +42,6 @@ extern void	freesp_init(void);
 #else
 # define freesp_init()	do { } while (0)
 #endif
+extern void	info_init(void);
 
 #endif /* XFS_SPACEMAN_SPACE_H_ */

[PATCH 14/16] xfs_info: move to xfs_spaceman

[Darrick J. Wong]

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

Move xfs_info to be under spaceman so that we can remove growfs -N.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 growfs/Makefile      |    2 --
 growfs/xfs_info.sh   |   32 --------------------------------
 spaceman/Makefile    |    2 ++
 spaceman/init.c      |    5 ++++-
 spaceman/xfs_info.sh |   32 ++++++++++++++++++++++++++++++++
 5 files changed, 38 insertions(+), 35 deletions(-)
 delete mode 100755 growfs/xfs_info.sh
 create mode 100755 spaceman/xfs_info.sh


diff --git a/growfs/Makefile b/growfs/Makefile
index f0190e4..adcd84b 100644
--- a/growfs/Makefile
+++ b/growfs/Makefile
@@ -20,7 +20,6 @@ endif
 
 LTDEPENDENCIES = $(LIBXFS) $(LIBXCMD) $(LIBFROG)
 LLDFLAGS = -static-libtool-libs
-LSRCFILES = xfs_info.sh
 
 default: depend $(LTCOMMAND)
 
@@ -29,7 +28,6 @@ include $(BUILDRULES)
 install: default
 	$(INSTALL) -m 755 -d $(PKG_SBIN_DIR)
 	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_SBIN_DIR)
-	$(INSTALL) -m 755 xfs_info.sh $(PKG_SBIN_DIR)/xfs_info
 install-dev:
 
 -include .dep
diff --git a/growfs/xfs_info.sh b/growfs/xfs_info.sh
deleted file mode 100755
index b85f120..0000000
--- a/growfs/xfs_info.sh
+++ /dev/null
@@ -1,32 +0,0 @@
-#!/bin/sh -f
-#
-# Copyright (c) 2000-2001 Silicon Graphics, Inc.  All Rights Reserved.
-#
-
-OPTS=""
-USAGE="Usage: xfs_info [-V] [-t mtab] mountpoint"
-
-while getopts "t:V" c
-do
-	case $c in
-	t)	OPTS="-t $OPTARG" ;;
-	V)	xfs_growfs -p xfs_info -V
-		status=$?
-		exit $status
-		;;
-	*)	echo $USAGE 1>&2
-		exit 2
-		;;
-	esac
-done
-set -- extra "$@"
-shift $OPTIND
-case $# in
-	1)	xfs_growfs -p xfs_info -n $OPTS "$1"
-		status=$?
-		;;
-	*)	echo $USAGE 1>&2
-		exit 2
-		;;
-esac
-exit $status
diff --git a/spaceman/Makefile b/spaceman/Makefile
index c1d903b..0d5ae2d 100644
--- a/spaceman/Makefile
+++ b/spaceman/Makefile
@@ -8,6 +8,7 @@ include $(TOPDIR)/include/builddefs
 LTCOMMAND = xfs_spaceman
 HFILES = init.h space.h
 CFILES = info.c init.c file.c prealloc.c trim.c
+LSRCFILES = xfs_info.sh
 
 LLDLIBS = $(LIBXCMD) $(LIBFROG)
 LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
@@ -35,6 +36,7 @@ include $(BUILDRULES)
 install: default
 	$(INSTALL) -m 755 -d $(PKG_SBIN_DIR)
 	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_SBIN_DIR)
+	$(INSTALL) -m 755 xfs_info.sh $(PKG_SBIN_DIR)/xfs_info
 install-dev:
 
 -include .dep
diff --git a/spaceman/init.c b/spaceman/init.c
index 895504f..91c773f 100644
--- a/spaceman/init.c
+++ b/spaceman/init.c
@@ -81,11 +81,14 @@ init(
 	textdomain(PACKAGE);
 
 	fs_table_initialise(0, NULL, 0, NULL);
-	while ((c = getopt(argc, argv, "c:V")) != EOF) {
+	while ((c = getopt(argc, argv, "c:p:V")) != EOF) {
 		switch (c) {
 		case 'c':
 			add_user_command(optarg);
 			break;
+		case 'p':
+			progname = optarg;
+			break;
 		case 'V':
 			printf(_("%s version %s\n"), progname, VERSION);
 			exit(0);
diff --git a/spaceman/xfs_info.sh b/spaceman/xfs_info.sh
new file mode 100755
index 0000000..5df0a26
--- /dev/null
+++ b/spaceman/xfs_info.sh
@@ -0,0 +1,32 @@
+#!/bin/sh -f
+#
+# Copyright (c) 2000-2001 Silicon Graphics, Inc.  All Rights Reserved.
+#
+
+OPTS=""
+USAGE="Usage: xfs_info [-V] [-t mtab] mountpoint"
+
+while getopts "t:V" c
+do
+	case $c in
+	t)	OPTS="-t $OPTARG" ;;
+	V)	xfs_spaceman -p xfs_info -V
+		status=$?
+		exit $status
+		;;
+	*)	echo $USAGE 1>&2
+		exit 2
+		;;
+	esac
+done
+set -- extra "$@"
+shift $OPTIND
+case $# in
+	1)	xfs_spaceman -p xfs_info -c "info" $OPTS "$1"
+		status=$?
+		;;
+	*)	echo $USAGE 1>&2
+		exit 2
+		;;
+esac
+exit $status

Re: [PATCH 14/16] xfs_info: move to xfs_spaceman

[Eric Sandeen]

On 3/1/18 1:14 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move xfs_info to be under spaceman so that we can remove growfs -N.

A while back we made xfs_growfs/ xfs_info fail on not-mount-points:

[root@localhost sandeen]# touch file

file not on xfs:

[root@localhost sandeen]# xfs_info file
xfs_info: specified file ["/"] is not on an XFS filesystem

file on xfs,  not mounpoint:

[root@localhost sandeen]# xfs_info mnt/foo
xfs_info: mnt/foo is not a mounted XFS filesystem

now if we point at a file not on xfs:

# ./spaceman/xfs_spaceman -c info /tmp/foo 
XFS_IOC_FSGEOMETRY: Inappropriate ioctl for device
(we lost the friendly failure string)

if we point at a file that's on xfs but is not a mountpoint:

# ./spaceman/xfs_spaceman -c info foo
meta-data=/dev/sdc1              isize=256    agcount=4, agsize=61047598 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=0        finobt=0, sparse=0, rmapbt=0
...

we get the underlying geometry.

Given that we now have tools which /can/ deal directly with image files,
I think that spaceman itself should probably continue to reject such requests
for image files, lest the user get the underlying geometry instead of the
image info as might have been hoped for...

but we really need a better way to determine "is this an xfs mountpoint"

-Eric

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  growfs/Makefile      |    2 --
>  growfs/xfs_info.sh   |   32 --------------------------------
>  spaceman/Makefile    |    2 ++
>  spaceman/init.c      |    5 ++++-
>  spaceman/xfs_info.sh |   32 ++++++++++++++++++++++++++++++++
>  5 files changed, 38 insertions(+), 35 deletions(-)
>  delete mode 100755 growfs/xfs_info.sh
>  create mode 100755 spaceman/xfs_info.sh
> 
> 
> diff --git a/growfs/Makefile b/growfs/Makefile
> index f0190e4..adcd84b 100644
> --- a/growfs/Makefile
> +++ b/growfs/Makefile
> @@ -20,7 +20,6 @@ endif
>  
>  LTDEPENDENCIES = $(LIBXFS) $(LIBXCMD) $(LIBFROG)
>  LLDFLAGS = -static-libtool-libs
> -LSRCFILES = xfs_info.sh
>  
>  default: depend $(LTCOMMAND)
>  
> @@ -29,7 +28,6 @@ include $(BUILDRULES)
>  install: default
>  	$(INSTALL) -m 755 -d $(PKG_SBIN_DIR)
>  	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_SBIN_DIR)
> -	$(INSTALL) -m 755 xfs_info.sh $(PKG_SBIN_DIR)/xfs_info
>  install-dev:
>  
>  -include .dep
> diff --git a/growfs/xfs_info.sh b/growfs/xfs_info.sh
> deleted file mode 100755
> index b85f120..0000000
> --- a/growfs/xfs_info.sh
> +++ /dev/null
> @@ -1,32 +0,0 @@
> -#!/bin/sh -f
> -#
> -# Copyright (c) 2000-2001 Silicon Graphics, Inc.  All Rights Reserved.
> -#
> -
> -OPTS=""
> -USAGE="Usage: xfs_info [-V] [-t mtab] mountpoint"
> -
> -while getopts "t:V" c
> -do
> -	case $c in
> -	t)	OPTS="-t $OPTARG" ;;
> -	V)	xfs_growfs -p xfs_info -V
> -		status=$?
> -		exit $status
> -		;;
> -	*)	echo $USAGE 1>&2
> -		exit 2
> -		;;
> -	esac
> -done
> -set -- extra "$@"
> -shift $OPTIND
> -case $# in
> -	1)	xfs_growfs -p xfs_info -n $OPTS "$1"
> -		status=$?
> -		;;
> -	*)	echo $USAGE 1>&2
> -		exit 2
> -		;;
> -esac
> -exit $status
> diff --git a/spaceman/Makefile b/spaceman/Makefile
> index c1d903b..0d5ae2d 100644
> --- a/spaceman/Makefile
> +++ b/spaceman/Makefile
> @@ -8,6 +8,7 @@ include $(TOPDIR)/include/builddefs
>  LTCOMMAND = xfs_spaceman
>  HFILES = init.h space.h
>  CFILES = info.c init.c file.c prealloc.c trim.c
> +LSRCFILES = xfs_info.sh
>  
>  LLDLIBS = $(LIBXCMD) $(LIBFROG)
>  LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
> @@ -35,6 +36,7 @@ include $(BUILDRULES)
>  install: default
>  	$(INSTALL) -m 755 -d $(PKG_SBIN_DIR)
>  	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_SBIN_DIR)
> +	$(INSTALL) -m 755 xfs_info.sh $(PKG_SBIN_DIR)/xfs_info
>  install-dev:
>  
>  -include .dep
> diff --git a/spaceman/init.c b/spaceman/init.c
> index 895504f..91c773f 100644
> --- a/spaceman/init.c
> +++ b/spaceman/init.c
> @@ -81,11 +81,14 @@ init(
>  	textdomain(PACKAGE);
>  
>  	fs_table_initialise(0, NULL, 0, NULL);
> -	while ((c = getopt(argc, argv, "c:V")) != EOF) {
> +	while ((c = getopt(argc, argv, "c:p:V")) != EOF) {
>  		switch (c) {
>  		case 'c':
>  			add_user_command(optarg);
>  			break;
> +		case 'p':
> +			progname = optarg;
> +			break;
>  		case 'V':
>  			printf(_("%s version %s\n"), progname, VERSION);
>  			exit(0);
> diff --git a/spaceman/xfs_info.sh b/spaceman/xfs_info.sh
> new file mode 100755
> index 0000000..5df0a26
> --- /dev/null
> +++ b/spaceman/xfs_info.sh
> @@ -0,0 +1,32 @@
> +#!/bin/sh -f
> +#
> +# Copyright (c) 2000-2001 Silicon Graphics, Inc.  All Rights Reserved.
> +#
> +
> +OPTS=""
> +USAGE="Usage: xfs_info [-V] [-t mtab] mountpoint"
> +
> +while getopts "t:V" c
> +do
> +	case $c in
> +	t)	OPTS="-t $OPTARG" ;;
> +	V)	xfs_spaceman -p xfs_info -V
> +		status=$?
> +		exit $status
> +		;;
> +	*)	echo $USAGE 1>&2
> +		exit 2
> +		;;
> +	esac
> +done
> +set -- extra "$@"
> +shift $OPTIND
> +case $# in
> +	1)	xfs_spaceman -p xfs_info -c "info" $OPTS "$1"
> +		status=$?
> +		;;
> +	*)	echo $USAGE 1>&2
> +		exit 2
> +		;;
> +esac
> +exit $status
> 
> --
> 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 14/16] xfs_info: move to xfs_spaceman

[Darrick J. Wong]

On Tue, Mar 06, 2018 at 09:50:09PM -0600, Eric Sandeen wrote:
> On 3/1/18 1:14 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Move xfs_info to be under spaceman so that we can remove growfs -N.
> 
> A while back we made xfs_growfs/ xfs_info fail on not-mount-points:
> 
> [root@localhost sandeen]# touch file
> 
> file not on xfs:
> 
> [root@localhost sandeen]# xfs_info file
> xfs_info: specified file ["/"] is not on an XFS filesystem
> 
> file on xfs,  not mounpoint:
> 
> [root@localhost sandeen]# xfs_info mnt/foo
> xfs_info: mnt/foo is not a mounted XFS filesystem
> 
> now if we point at a file not on xfs:
> 
> # ./spaceman/xfs_spaceman -c info /tmp/foo 
> XFS_IOC_FSGEOMETRY: Inappropriate ioctl for device
> (we lost the friendly failure string)
> 
> if we point at a file that's on xfs but is not a mountpoint:
> 
> # ./spaceman/xfs_spaceman -c info foo

So this raises some theoretical issues.  Right now spaceman applies its
management to whatever fs backs the inode that we opened from the cli
arguments.  If this will be the only scope forever and ever then we
could require that the pathnames on the cli actually be mountpoints and
enforce with the same mntent parsing that scrub/fsr currently use.

Will we ever want to be able to perform space management on specific
files?  I don't know that we do, but I also don't think we want to trade
away that option.

I think I'd rather change the info command to require a mount point, and
maybe that will just be a quirk of the info command.

> meta-data=/dev/sdc1              isize=256    agcount=4, agsize=61047598 blks
>          =                       sectsz=512   attr=2, projid32bit=1
>          =                       crc=0        finobt=0, sparse=0, rmapbt=0
> ...
> 
> we get the underlying geometry.
> 
> Given that we now have tools which /can/ deal directly with image files,
> I think that spaceman itself should probably continue to reject such requests
> for image files, lest the user get the underlying geometry instead of the
> image info as might have been hoped for...
> 
> but we really need a better way to determine "is this an xfs mountpoint"

It occurs to me that the manpage for info and growfs both say "mount
point", not "root directory of an xfs filesystem".  Since we can bind
mount anything anywhere on Linux, what does that even mean?

If I do:

# mount /dev/sda1 /home
# mount /home/djwong/foo /opt --bind
# xfs_info /opt

Should that succeed?  The manpage would seem to say yes.  Maybe it's
just time to hoist the mountpoint check code to libfrog and rework
scrub/fsr/spaceman to use it.

--D

> -Eric
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  growfs/Makefile      |    2 --
> >  growfs/xfs_info.sh   |   32 --------------------------------
> >  spaceman/Makefile    |    2 ++
> >  spaceman/init.c      |    5 ++++-
> >  spaceman/xfs_info.sh |   32 ++++++++++++++++++++++++++++++++
> >  5 files changed, 38 insertions(+), 35 deletions(-)
> >  delete mode 100755 growfs/xfs_info.sh
> >  create mode 100755 spaceman/xfs_info.sh
> > 
> > 
> > diff --git a/growfs/Makefile b/growfs/Makefile
> > index f0190e4..adcd84b 100644
> > --- a/growfs/Makefile
> > +++ b/growfs/Makefile
> > @@ -20,7 +20,6 @@ endif
> >  
> >  LTDEPENDENCIES = $(LIBXFS) $(LIBXCMD) $(LIBFROG)
> >  LLDFLAGS = -static-libtool-libs
> > -LSRCFILES = xfs_info.sh
> >  
> >  default: depend $(LTCOMMAND)
> >  
> > @@ -29,7 +28,6 @@ include $(BUILDRULES)
> >  install: default
> >  	$(INSTALL) -m 755 -d $(PKG_SBIN_DIR)
> >  	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_SBIN_DIR)
> > -	$(INSTALL) -m 755 xfs_info.sh $(PKG_SBIN_DIR)/xfs_info
> >  install-dev:
> >  
> >  -include .dep
> > diff --git a/growfs/xfs_info.sh b/growfs/xfs_info.sh
> > deleted file mode 100755
> > index b85f120..0000000
> > --- a/growfs/xfs_info.sh
> > +++ /dev/null
> > @@ -1,32 +0,0 @@
> > -#!/bin/sh -f
> > -#
> > -# Copyright (c) 2000-2001 Silicon Graphics, Inc.  All Rights Reserved.
> > -#
> > -
> > -OPTS=""
> > -USAGE="Usage: xfs_info [-V] [-t mtab] mountpoint"
> > -
> > -while getopts "t:V" c
> > -do
> > -	case $c in
> > -	t)	OPTS="-t $OPTARG" ;;
> > -	V)	xfs_growfs -p xfs_info -V
> > -		status=$?
> > -		exit $status
> > -		;;
> > -	*)	echo $USAGE 1>&2
> > -		exit 2
> > -		;;
> > -	esac
> > -done
> > -set -- extra "$@"
> > -shift $OPTIND
> > -case $# in
> > -	1)	xfs_growfs -p xfs_info -n $OPTS "$1"
> > -		status=$?
> > -		;;
> > -	*)	echo $USAGE 1>&2
> > -		exit 2
> > -		;;
> > -esac
> > -exit $status
> > diff --git a/spaceman/Makefile b/spaceman/Makefile
> > index c1d903b..0d5ae2d 100644
> > --- a/spaceman/Makefile
> > +++ b/spaceman/Makefile
> > @@ -8,6 +8,7 @@ include $(TOPDIR)/include/builddefs
> >  LTCOMMAND = xfs_spaceman
> >  HFILES = init.h space.h
> >  CFILES = info.c init.c file.c prealloc.c trim.c
> > +LSRCFILES = xfs_info.sh
> >  
> >  LLDLIBS = $(LIBXCMD) $(LIBFROG)
> >  LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
> > @@ -35,6 +36,7 @@ include $(BUILDRULES)
> >  install: default
> >  	$(INSTALL) -m 755 -d $(PKG_SBIN_DIR)
> >  	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_SBIN_DIR)
> > +	$(INSTALL) -m 755 xfs_info.sh $(PKG_SBIN_DIR)/xfs_info
> >  install-dev:
> >  
> >  -include .dep
> > diff --git a/spaceman/init.c b/spaceman/init.c
> > index 895504f..91c773f 100644
> > --- a/spaceman/init.c
> > +++ b/spaceman/init.c
> > @@ -81,11 +81,14 @@ init(
> >  	textdomain(PACKAGE);
> >  
> >  	fs_table_initialise(0, NULL, 0, NULL);
> > -	while ((c = getopt(argc, argv, "c:V")) != EOF) {
> > +	while ((c = getopt(argc, argv, "c:p:V")) != EOF) {
> >  		switch (c) {
> >  		case 'c':
> >  			add_user_command(optarg);
> >  			break;
> > +		case 'p':
> > +			progname = optarg;
> > +			break;
> >  		case 'V':
> >  			printf(_("%s version %s\n"), progname, VERSION);
> >  			exit(0);
> > diff --git a/spaceman/xfs_info.sh b/spaceman/xfs_info.sh
> > new file mode 100755
> > index 0000000..5df0a26
> > --- /dev/null
> > +++ b/spaceman/xfs_info.sh
> > @@ -0,0 +1,32 @@
> > +#!/bin/sh -f
> > +#
> > +# Copyright (c) 2000-2001 Silicon Graphics, Inc.  All Rights Reserved.
> > +#
> > +
> > +OPTS=""
> > +USAGE="Usage: xfs_info [-V] [-t mtab] mountpoint"
> > +
> > +while getopts "t:V" c
> > +do
> > +	case $c in
> > +	t)	OPTS="-t $OPTARG" ;;
> > +	V)	xfs_spaceman -p xfs_info -V
> > +		status=$?
> > +		exit $status
> > +		;;
> > +	*)	echo $USAGE 1>&2
> > +		exit 2
> > +		;;
> > +	esac
> > +done
> > +set -- extra "$@"
> > +shift $OPTIND
> > +case $# in
> > +	1)	xfs_spaceman -p xfs_info -c "info" $OPTS "$1"
> > +		status=$?
> > +		;;
> > +	*)	echo $USAGE 1>&2
> > +		exit 2
> > +		;;
> > +esac
> > +exit $status
> > 
> > --
> > 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 14/16] xfs_info: move to xfs_spaceman

[Dave Chinner]

On Wed, Mar 07, 2018 at 12:33:50PM -0800, Darrick J. Wong wrote:
> On Tue, Mar 06, 2018 at 09:50:09PM -0600, Eric Sandeen wrote:
> > On 3/1/18 1:14 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Move xfs_info to be under spaceman so that we can remove growfs -N.
> > 
> > A while back we made xfs_growfs/ xfs_info fail on not-mount-points:
> > 
> > [root@localhost sandeen]# touch file
> > 
> > file not on xfs:
> > 
> > [root@localhost sandeen]# xfs_info file
> > xfs_info: specified file ["/"] is not on an XFS filesystem
> > 
> > file on xfs,  not mounpoint:
> > 
> > [root@localhost sandeen]# xfs_info mnt/foo
> > xfs_info: mnt/foo is not a mounted XFS filesystem
> > 
> > now if we point at a file not on xfs:
> > 
> > # ./spaceman/xfs_spaceman -c info /tmp/foo 
> > XFS_IOC_FSGEOMETRY: Inappropriate ioctl for device
> > (we lost the friendly failure string)
> > 
> > if we point at a file that's on xfs but is not a mountpoint:
> > 
> > # ./spaceman/xfs_spaceman -c info foo
> 
> So this raises some theoretical issues.  Right now spaceman applies its
> management to whatever fs backs the inode that we opened from the cli
> arguments.  If this will be the only scope forever and ever then we
> could require that the pathnames on the cli actually be mountpoints and
> enforce with the same mntent parsing that scrub/fsr currently use.
> 
> Will we ever want to be able to perform space management on specific
> files?  I don't know that we do, but I also don't think we want to trade
> away that option.

Yes, yes we will. subvolume backing stores fall into this category.

> It occurs to me that the manpage for info and growfs both say "mount
> point", not "root directory of an xfs filesystem".  Since we can bind
> mount anything anywhere on Linux, what does that even mean?
> 
> If I do:
> 
> # mount /dev/sda1 /home
> # mount /home/djwong/foo /opt --bind
> # xfs_info /opt
> 
> Should that succeed?  The manpage would seem to say yes. 

The manpage wording pre-dates bind mounts and this capability. We
should not assume that the man page is the canonical truth that must
be obeyed, especially when it doesn't make sense anymore...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 15/16] xfs_info: call xfs_db for offline filesystems

[Darrick J. Wong]

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

If the online filesystem geometry query doesn't work, try using xfs_db
to see if we can grab the information offline.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man8/xfs_growfs.8 |   47 +-----------------------
 man/man8/xfs_info.8   |   95 +++++++++++++++++++++++++++++++++++++++++++++++++
 spaceman/xfs_info.sh  |   12 +++++-
 3 files changed, 106 insertions(+), 48 deletions(-)
 create mode 100644 man/man8/xfs_info.8


diff --git a/man/man8/xfs_growfs.8 b/man/man8/xfs_growfs.8
index e23d30e..7e6a387 100644
--- a/man/man8/xfs_growfs.8
+++ b/man/man8/xfs_growfs.8
@@ -11,7 +11,7 @@
 
 .TH xfs_growfs 8
 .SH NAME
-xfs_growfs, xfs_info \- expand an XFS filesystem
+xfs_growfs \- expand an XFS filesystem
 .SH SYNOPSIS
 .B xfs_growfs
 [
@@ -38,16 +38,6 @@ xfs_growfs, xfs_info \- expand an XFS filesystem
 .I mount-point
 .br
 .B xfs_growfs \-V
-.PP
-.br
-.B xfs_info
-[
-.B \-t
-.I mtab
-]
-.I mount-point
-.br
-.B xfs_info \-V
 .SH DESCRIPTION
 .B xfs_growfs
 expands an existing XFS filesystem (see
@@ -59,13 +49,6 @@ is mounted. The filesystem must be mounted to be grown (see
 .BR mount (8)).
 The existing contents of the filesystem are undisturbed, and the added space
 becomes available for additional file storage.
-.PP
-.B xfs_info
-is equivalent to invoking
-.B xfs_growfs
-with the
-.B \-n
-option (see discussion below).
 .SH OPTIONS
 .TP
 .BI "\-d | \-D " size
@@ -169,35 +152,9 @@ reside. In order to grow a filesystem, it is necessary to provide added
 space for it to occupy. Therefore there must be at least one spare new
 disk partition available. Adding the space is often done through the use
 of a logical volume manager.
-.SH "EXAMPLES"
-
-Understanding xfs_info output.
-.PP
-Suppose one has the following "xfs_info /dev/sda" output:
-.PP
-.RS 2
-.Vb
-\&meta-data=/dev/sda      isize=256    agcount=32, agsize=16777184 blks
-\&         =              sectsz=512   attr=2
-\&data     =              bsize=4096   blocks=536869888, imaxpct=5
-\&         =              sunit=32     swidth=128 blks
-\&naming   =version 2     bsize=4096
-\&log      =internal      bsize=4096   blocks=32768, version=2
-\&         =              sectsz=512   sunit=32 blks, lazy-count=1
-\&realtime =none          extsz=524288 blocks=0, rtextents=0
-.Ve
-.RE
-.PP
-
-Here, the data section of the output indicates "bsize=4096",
-meaning the data block size for this filesystem is 4096 bytes.
-This section also shows "sunit=32 swidth=128 blks", which means
-the stripe unit is 32*4096 bytes = 128 kibibytes and the stripe
-width is 128*4096 bytes = 512 kibibytes.
-A single stripe of this filesystem therefore consists
-of four stripe units (128 blocks / 32 blocks per unit).
 .SH SEE ALSO
 .BR mkfs.xfs (8),
+.BR xfs_info (8),
 .BR md (4),
 .BR lvm (8),
 .BR mount (8).
diff --git a/man/man8/xfs_info.8 b/man/man8/xfs_info.8
new file mode 100644
index 0000000..c4c470d
--- /dev/null
+++ b/man/man8/xfs_info.8
@@ -0,0 +1,95 @@
+.\" Verbatim blocks taken from openssl req manpage content
+.de Vb \" Begin verbatim text
+.ft CW
+.nf
+.ne \\$1
+..
+.de Ve \" End verbatim text
+.ft R
+.fi
+..
+
+.TH xfs_info 8
+.SH NAME
+xfs_info, \- display XFS filesystem geometry information
+.SH SYNOPSIS
+.B xfs_info
+[
+.B \-t
+.I mtab
+]
+[
+.I mount-point
+|
+.I block-device
+|
+.I file-image
+]
+.br
+.B xfs_info \-V
+.SH DESCRIPTION
+.B xfs_info
+displays geometry information about an existing XFS filesystem.
+The
+.I mount-point
+argument is the pathname of a directory where the filesystem
+is mounted.
+The
+.I block-device
+or
+.I file-image
+contain a raw XFS filesystem.
+The existing contents of the filesystem are undisturbed.
+.SH OPTIONS
+.TP
+.B \-t
+Specifies an alternate mount table file (default is
+.I /proc/mounts
+if it exists, else
+.IR /etc/mtab ).
+This is used when working with filesystems mounted without writing to
+.I /etc/mtab
+file - refer to
+.BR mount (8)
+for further details.
+This option has no effect with the
+.IR block-device " or " file-image
+parameters.
+.TP
+.B \-V
+Prints the version number and exits. The
+.I mount-point
+argument is not required with
+.BR \-V .
+.SH "EXAMPLES"
+
+Understanding xfs_info output.
+.PP
+Suppose one has the following "xfs_info /dev/sda" output:
+.PP
+.RS 2
+.Vb
+\&meta-data=/dev/sda      isize=256    agcount=32, agsize=16777184 blks
+\&         =              sectsz=512   attr=2
+\&data     =              bsize=4096   blocks=536869888, imaxpct=5
+\&         =              sunit=32     swidth=128 blks
+\&naming   =version 2     bsize=4096
+\&log      =internal log  bsize=4096   blocks=32768, version=2
+\&         =              sectsz=512   sunit=32 blks, lazy-count=1
+\&realtime =none          extsz=524288 blocks=0, rtextents=0
+.Ve
+.RE
+.PP
+
+Here, the data section of the output indicates "bsize=4096",
+meaning the data block size for this filesystem is 4096 bytes.
+This section also shows "sunit=32 swidth=128 blks", which means
+the stripe unit is 32*4096 bytes = 128 kibibytes and the stripe
+width is 128*4096 bytes = 512 kibibytes.
+A single stripe of this filesystem therefore consists
+of four stripe units (128 blocks / 32 blocks per unit).
+.SH SEE ALSO
+.BR mkfs.xfs (8),
+.BR md (4),
+.BR lvm (8),
+.BR mount (8).
diff --git a/spaceman/xfs_info.sh b/spaceman/xfs_info.sh
index 5df0a26..2e17fd9 100755
--- a/spaceman/xfs_info.sh
+++ b/spaceman/xfs_info.sh
@@ -4,7 +4,7 @@
 #
 
 OPTS=""
-USAGE="Usage: xfs_info [-V] [-t mtab] mountpoint"
+USAGE="Usage: xfs_info [-V] [-t mtab] [mountpoint|device|file]"
 
 while getopts "t:V" c
 do
@@ -22,8 +22,14 @@ done
 set -- extra "$@"
 shift $OPTIND
 case $# in
-	1)	xfs_spaceman -p xfs_info -c "info" $OPTS "$1"
-		status=$?
+	1)
+		if [ -b "$1" ] || [ -f "$1" ]; then
+			xfs_db -p xfs_info -c "info" $OPTS "$1"
+			status=$?
+		else
+			xfs_spaceman -p xfs_info -c "info" $OPTS "$1"
+			status=$?
+		fi
 		;;
 	*)	echo $USAGE 1>&2
 		exit 2

[PATCH 16/16] xfs_growfs: refactor geometry reporting

[Darrick J. Wong]

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

Use the new geometry pretty-printing function.

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


diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
index 366176b..8ec445a 100644
--- a/growfs/xfs_growfs.c
+++ b/growfs/xfs_growfs.c
@@ -18,6 +18,7 @@
 
 #include "libxfs.h"
 #include "path.h"
+#include "fsgeom.h"
 
 static void
 usage(void)
@@ -42,54 +43,6 @@ Options:\n\
 	exit(2);
 }
 
-void
-report_info(
-	xfs_fsop_geom_t	geo,
-	char		*mntpoint,
-	int		isint,
-	char		*logname,
-	char		*rtname,
-	int		lazycount,
-	int		dirversion,
-	int		logversion,
-	int		attrversion,
-	int		projid32bit,
-	int		crcs_enabled,
-	int		cimode,
-	int		ftype_enabled,
-	int		finobt_enabled,
-	int		spinodes,
-	int		rmapbt_enabled,
-	int		reflink_enabled)
-{
-	printf(_(
-	    "meta-data=%-22s isize=%-6u agcount=%u, agsize=%u blks\n"
-	    "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
-	    "         =%-22s crc=%-8u finobt=%u spinodes=%u rmapbt=%u\n"
-	    "         =%-22s reflink=%u\n"
-	    "data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
-	    "         =%-22s sunit=%-6u swidth=%u blks\n"
-	    "naming   =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
-	    "log      =%-22s bsize=%-6u blocks=%u, version=%u\n"
-	    "         =%-22s sectsz=%-5u sunit=%u blks, lazy-count=%u\n"
-	    "realtime =%-22s extsz=%-6u blocks=%llu, rtextents=%llu\n"),
-
-		mntpoint, geo.inodesize, geo.agcount, geo.agblocks,
-		"", geo.sectsize, attrversion, projid32bit,
-		"", crcs_enabled, finobt_enabled, spinodes, rmapbt_enabled,
-		"", reflink_enabled,
-		"", geo.blocksize, (unsigned long long)geo.datablocks,
-			geo.imaxpct,
-		"", geo.sunit, geo.swidth,
-  		dirversion, geo.dirblocksize, cimode, ftype_enabled,
-		isint ? _("internal") : logname ? logname : _("external"),
-			geo.blocksize, geo.logblocks, logversion,
-		"", geo.logsectsize, geo.logsunit / geo.blocksize, lazycount,
-		!geo.rtblocks ? _("none") : rtname ? rtname : _("external"),
-		geo.rtextsize * geo.blocksize, (unsigned long long)geo.rtblocks,
-			(unsigned long long)geo.rtextents);
-}
-
 int
 main(int argc, char **argv)
 {
@@ -97,9 +50,6 @@ main(int argc, char **argv)
 	int			c;	/* current option character */
 	long long		ddsize;	/* device size in 512-byte blocks */
 	int			dflag;	/* -d flag */
-	int			attrversion;/* attribute version number */
-	int			dirversion; /* directory version number */
-	int			logversion; /* log version number */
 	long long		dlsize;	/* device size in 512-byte blocks */
 	long long		drsize;	/* device size in 512-byte blocks */
 	long long		dsize;	/* new data size in fs blocks */
@@ -117,8 +67,6 @@ main(int argc, char **argv)
 	xfs_fsop_geom_t		ngeo;	/* new fs geometry */
 	int			rflag;	/* -r flag */
 	long long		rsize;	/* new rt size in fs blocks */
-	int			ci;	/* ASCII case-insensitive fs */
-	int			lazycount; /* lazy superblock counters */
 	int			xflag;	/* -x flag */
 	char			*fname;	/* mount point name */
 	char			*datadev; /* data device name */
@@ -126,13 +74,6 @@ main(int argc, char **argv)
 	char			*rtdev;	/*   RT device name */
 	fs_path_t		*fs;	/* mount point information */
 	libxfs_init_t		xi;	/* libxfs structure */
-	int			projid32bit;
-	int			crcs_enabled;
-	int			ftype_enabled = 0;
-	int			finobt_enabled;	/* free inode btree */
-	int			spinodes;
-	int			rmapbt_enabled;
-	int			reflink_enabled;
 	char			rpath[PATH_MAX];
 
 	progname = basename(argv[0]);
@@ -253,27 +194,6 @@ main(int argc, char **argv)
 		}
 	}
 	isint = geo.logstart > 0;
-	lazycount = geo.flags & XFS_FSOP_GEOM_FLAGS_LAZYSB ? 1 : 0;
-	dirversion = geo.flags & XFS_FSOP_GEOM_FLAGS_DIRV2 ? 2 : 1;
-	logversion = geo.flags & XFS_FSOP_GEOM_FLAGS_LOGV2 ? 2 : 1;
-	attrversion = geo.flags & XFS_FSOP_GEOM_FLAGS_ATTR2 ? 2 : \
-			(geo.flags & XFS_FSOP_GEOM_FLAGS_ATTR ? 1 : 0);
-	ci = geo.flags & XFS_FSOP_GEOM_FLAGS_DIRV2CI ? 1 : 0;
-	projid32bit = geo.flags & XFS_FSOP_GEOM_FLAGS_PROJID32 ? 1 : 0;
-	crcs_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_V5SB ? 1 : 0;
-	ftype_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_FTYPE ? 1 : 0;
-	finobt_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_FINOBT ? 1 : 0;
-	spinodes = geo.flags & XFS_FSOP_GEOM_FLAGS_SPINODES ? 1 : 0;
-	rmapbt_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_RMAPBT ? 1 : 0;
-	reflink_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_REFLINK ? 1 : 0;
-	if (nflag) {
-		report_info(geo, datadev, isint, logdev, rtdev,
-				lazycount, dirversion, logversion,
-				attrversion, projid32bit, crcs_enabled, ci,
-				ftype_enabled, finobt_enabled, spinodes,
-				rmapbt_enabled, reflink_enabled);
-		exit(0);
-	}
 
 	/*
 	 * Need root access from here on (using raw devices)...
@@ -306,11 +226,7 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
-	report_info(geo, datadev, isint, logdev, rtdev,
-			lazycount, dirversion, logversion,
-			attrversion, projid32bit, crcs_enabled, ci, ftype_enabled,
-			finobt_enabled, spinodes, rmapbt_enabled,
-			reflink_enabled);
+	xfs_report_geom(&geo, datadev, logdev, rtdev);
 
 	ddsize = xi.dsize;
 	dlsize = ( xi.logBBsize? xi.logBBsize :

[RFC PATCH 17/16] xfs_spaceman: only produce info for root of mounted xfs

[Eric Sandeen]

Previously, xfs_info / xfs_growfs would only operate if
pointed at the root of a mounted xfs filesystem, and not a
file within that filesystem. (Although the ioctl will allow it,
the utility does not, to avoid errors and confusion).

When the info capability was moved to xfs_spaceman, this restriction
got lost.

And now that xfs_db can print xfs_info-like info about image files
and devices, the potential exists for misdirecting the request
and running the ioctl on a file and getting the underlying fs
info, rather than using libxfs to parse the image itself, if
the wrong tool is used to point at the file.

Add a new function to (ab)use XFS_IOC_FSINUMBERS to determine
whether the thing we've been pointed at is in fact the root
directory of a mounted xfs filesystem.  If not, don't proceed.

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

I need to look, perhaps this could simplify growfs as well.
I'm not sure I've put this in the right place (libfrog?) either,
but for the sake of discsussion ...

diff --git a/include/libfrog.h b/include/libfrog.h
index 2d8055b..f7cd00a 100644
--- a/include/libfrog.h
+++ b/include/libfrog.h
@@ -18,6 +18,9 @@
 #ifndef __LIBFROG_UTIL_H_
 #define __LIBFROG_UTIL_H_
 
+#include <stdbool.h>
+
 unsigned int	log2_roundup(unsigned int i);
+bool		fd_is_xfs_root(int fd);
 
 #endif /* __LIBFROG_UTIL_H_ */
diff --git a/libfrog/util.c b/libfrog/util.c
index 4896e4b..4913f38 100644
--- a/libfrog/util.c
+++ b/libfrog/util.c
@@ -16,6 +16,7 @@
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 #include "platform_defs.h"
+#include "libxfs.h"
 #include "libfrog.h"
 
 /*
@@ -34,3 +35,44 @@ log2_roundup(unsigned int i)
 	}
 	return rval;
 }
+
+/* Test whether a given fd is the root of a mounted xfs filesystem */
+bool
+fd_is_xfs_root(
+	int			fd)
+{
+	struct xfs_fsop_bulkreq	igrpreq = {0};
+	struct xfs_inogrp	inogrp;
+	__u64			igrp_ino = 0;
+	__s32			igrplen = 0;
+	__u64			first_ino;
+	struct stat		statbuf;
+
+	igrpreq.lastip  = &igrp_ino;
+	igrpreq.icount = 1;
+	igrpreq.ubuffer = &inogrp;
+	igrpreq.ocount  = &igrplen;
+
+	if (ioctl(fd, XFS_IOC_FSINUMBERS, &igrpreq))
+		return false;
+
+	/* find the first allocated inode */
+	while (!ioctl(fd, XFS_IOC_FSINUMBERS, &igrpreq)) {
+		if (inogrp.xi_alloccount)
+			break;
+        }
+
+	/* The ioctl failed ... */
+	if (!inogrp.xi_alloccount)
+		return false;
+
+	first_ino = inogrp.xi_startino + libxfs_lowbit64(inogrp.xi_allocmask);
+
+	if (fstat(fd, &statbuf))
+		return false;
+
+	if (first_ino != statbuf.st_ino)
+		return false;
+
+	return true;
+}
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index a23a28d..79a0bd7 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -25,6 +25,8 @@
  * it can be included in both the internal and external libxfs header files
  * without introducing any depenencies between the two.
  */
+#define xfs_lowbit32			libxfs_lowbit32
+#define xfs_lowbit64			libxfs_lowbit64
 #define xfs_highbit32			libxfs_highbit32
 #define xfs_highbit64			libxfs_highbit64
 
diff --git a/spaceman/info.c b/spaceman/info.c
index 6557b54..df088f4 100644
--- a/spaceman/info.c
+++ b/spaceman/info.c
@@ -44,6 +44,13 @@ info_f(
 	struct xfs_fsop_geom	geo;
 	int			error;
 
+	if (!fd_is_xfs_root(file->fd)) {
+		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
+			progname, file->name);
+		exitcode = 1;
+		return 0;
+	}
+
 	/* get the current filesystem size & geometry */
 	error = ioctl(file->fd, XFS_IOC_FSGEOMETRY, &geo);
 	if (error) {