[PATCHSET v3 0/5] xfs: add the ability to flag a fs for repair

[PATCHSET v3 0/5] xfs: add the ability to flag a fs for repair

[Darrick J. Wong]

Hi all,

This series adds a new incompat feature flag so that we can force a
sysadmin to run xfs_repair on a filesystem before mounting.  This will
be used in conjunction with v5 feature upgrades.

v2: tweak the "can't upgrade" behavior and repair messages
v3: improve documentation, define error codes for the upgrade process, and
    only force repair if NEEDSREPAIR is set

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

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

--D

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

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=needsrepair

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=needsrepair
---
 db/check.c           |    5 ++
 db/sb.c              |  160 ++++++++++++++++++++++++++++++++++++++++++++++++--
 db/xfs_admin.sh      |   32 +++++++++-
 include/xfs_mount.h  |    1 
 libxfs/init.c        |   12 +++-
 man/man8/xfs_admin.8 |   30 +++++++++
 man/man8/xfs_db.8    |   16 +++++
 repair/agheader.c    |   21 +++++++
 repair/xfs_repair.c  |   51 ++++++++++++++++
 9 files changed, 311 insertions(+), 17 deletions(-)

[PATCH 1/5] xfs_admin: clean up string quoting

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Clean up the string quoting in this script so that we don't trip over
users feeding us arguments like "/dev/sd ha ha ha lol".

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 db/xfs_admin.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
index bd325da2..71a9aa98 100755
--- a/db/xfs_admin.sh
+++ b/db/xfs_admin.sh
@@ -43,7 +43,7 @@ case $# in
 
 		if [ -n "$DB_OPTS" ]
 		then
-			eval xfs_db -x -p xfs_admin $DB_OPTS $1
+			eval xfs_db -x -p xfs_admin $DB_OPTS "$1"
 			status=$?
 		fi
 		if [ -n "$REPAIR_OPTS" ]
@@ -53,7 +53,7 @@ case $# in
 			# running xfs_admin.
 			# Ideally, we need to improve the output behaviour
 			# of repair for this purpose (say a "quiet" mode).
-			eval xfs_repair $REPAIR_OPTS $1 2> /dev/null
+			eval xfs_repair $REPAIR_OPTS "$1" 2> /dev/null
 			status=`expr $? + $status`
 			if [ $status -ne 0 ]
 			then

[PATCH 2/5] xfs_db: define some exit codes for fs feature upgrades

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Define some exit codes that xfs_db will return when a sysadmin uses it
(or more likely xfs_admin) to add a feature to the filesystem.  At the
moment we return zero for successful upgrades and 1 for fs errors,
though we also allow for returning 2 if the upgrade cannot be applied
because the fs cannot handle it.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 db/sb.c           |    6 +++++-
 db/xfs_admin.sh   |    3 +++
 man/man8/xfs_db.8 |    8 ++++++++
 3 files changed, 16 insertions(+), 1 deletion(-)


diff --git a/db/sb.c b/db/sb.c
index d09f653d..f306e939 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -617,6 +617,9 @@ do_version(xfs_agnumber_t agno, uint16_t version, uint32_t features)
 	tsb.sb_bad_features2 = features;
 	libxfs_sb_to_disk(iocur_top->data, &tsb);
 	write_cur();
+	if (!iocur_top->bp || iocur_top->bp->b_error)
+		return 0;
+
 	return 1;
 }
 
@@ -804,7 +807,8 @@ version_f(
 				if (!do_version(ag, version, features)) {
 					dbprintf(_("failed to set versionnum "
 						 "in AG %d\n"), ag);
-					break;
+					exitcode = 1;
+					return 1;
 				}
 			mp->m_sb.sb_versionnum = version;
 			mp->m_sb.sb_features2 = features;
diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
index 71a9aa98..5c57b461 100755
--- a/db/xfs_admin.sh
+++ b/db/xfs_admin.sh
@@ -46,6 +46,9 @@ case $# in
 			eval xfs_db -x -p xfs_admin $DB_OPTS "$1"
 			status=$?
 		fi
+		if [ $status -eq 1 ]; then
+			echo "Conversion failed due to filesystem errors; run xfs_repair."
+		fi
 		if [ -n "$REPAIR_OPTS" ]
 		then
 			# Hide normal repair output which is sent to stderr
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 58727495..ee57b03a 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -981,6 +981,14 @@ and
 .I features2
 bits respectively, and their string equivalent reported
 (but no modifications are made).
+.IP
+If the feature upgrade succeeds, the program will return 0.
+If the requested upgrade has already been applied to the filesystem, the
+program will also return 0.
+If the upgrade fails due to corruption or IO errors, the program will return
+1.
+If the requested upgrade is not appropriate for this filesystem, the program
+will return 2.
 .TP
 .BI "write [\-c|\-d] [" "field value" "] ..."
 Write a value to disk.

[PATCH 3/5] xfs_db: support the needsrepair feature flag in the version command

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Teach the xfs_db version command about the 'needsrepair' flag, which can
be used to force the system administrator to repair the filesystem with
xfs_repair.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 db/check.c           |    5 ++
 db/sb.c              |  154 ++++++++++++++++++++++++++++++++++++++++++++++++--
 db/xfs_admin.sh      |   25 +++++++-
 man/man8/xfs_admin.8 |   30 ++++++++++
 man/man8/xfs_db.8    |    8 +++
 5 files changed, 213 insertions(+), 9 deletions(-)


diff --git a/db/check.c b/db/check.c
index 33736e33..485e855e 100644
--- a/db/check.c
+++ b/db/check.c
@@ -3970,6 +3970,11 @@ scan_ag(
 			dbprintf(_("mkfs not completed successfully\n"));
 		error++;
 	}
+	if (xfs_sb_version_needsrepair(sb)) {
+		if (!sflag)
+			dbprintf(_("filesystem needs xfs_repair\n"));
+		error++;
+	}
 	set_dbmap(agno, XFS_SB_BLOCK(mp), 1, DBM_SB, agno, XFS_SB_BLOCK(mp));
 	if (sb->sb_logstart && XFS_FSB_TO_AGNO(mp, sb->sb_logstart) == agno)
 		set_dbmap(agno, XFS_FSB_TO_AGBNO(mp, sb->sb_logstart),
diff --git a/db/sb.c b/db/sb.c
index f306e939..223b84fe 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -379,6 +379,11 @@ uuid_f(
 				progname);
 			return 0;
 		}
+		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
+			dbprintf(_("%s: filesystem needs xfs_repair\n"),
+				progname);
+			return 0;
+		}
 
 		if (!strcasecmp(argv[1], "generate")) {
 			platform_uuid_generate(&uu);
@@ -543,6 +548,12 @@ label_f(
 			return 0;
 		}
 
+		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
+			dbprintf(_("%s: filesystem needs xfs_repair\n"),
+				progname);
+			return 0;
+		}
+
 		dbprintf(_("writing all SBs\n"));
 		for (ag = 0; ag < mp->m_sb.sb_agcount; ag++)
 			if ((p = do_label(ag, argv[1])) == NULL) {
@@ -584,6 +595,7 @@ version_help(void)
 " 'version attr1'    - enable v1 inline extended attributes\n"
 " 'version attr2'    - enable v2 inline extended attributes\n"
 " 'version log2'     - enable v2 log format\n"
+" 'version needsrepair' - flag filesystem as requiring repair\n"
 "\n"
 "The version function prints currently enabled features for a filesystem\n"
 "according to the version field of its primary superblock.\n"
@@ -623,6 +635,114 @@ do_version(xfs_agnumber_t agno, uint16_t version, uint32_t features)
 	return 1;
 }
 
+struct v5feat {
+	uint32_t		compat;
+	uint32_t		ro_compat;
+	uint32_t		incompat;
+	uint32_t		log_incompat;
+};
+
+static void
+get_v5_features(
+	struct v5feat		*feat,
+	const struct xfs_sb	*sbp)
+{
+	feat->compat = sbp->sb_features_compat;
+	feat->ro_compat = sbp->sb_features_ro_compat;
+	feat->incompat = sbp->sb_features_incompat;
+	feat->log_incompat = sbp->sb_features_log_incompat;
+}
+
+static void
+set_v5_features(
+	struct xfs_sb		*sbp,
+	const struct v5feat	*feat)
+{
+	sbp->sb_features_compat = feat->compat;
+	sbp->sb_features_ro_compat = feat->ro_compat;
+	sbp->sb_features_incompat = feat->incompat;
+	sbp->sb_features_log_incompat = feat->log_incompat;
+}
+
+static bool
+upgrade_v5_features(
+	struct xfs_mount	*mp,
+	const struct v5feat	*upgrade)
+{
+	struct xfs_sb		tsb;
+	struct v5feat		old;
+	xfs_agnumber_t		agno = 0;
+	xfs_agnumber_t		revert_agno = 0;
+
+	if (upgrade->compat == mp->m_sb.sb_features_compat &&
+	    upgrade->ro_compat == mp->m_sb.sb_features_ro_compat &&
+	    upgrade->incompat == mp->m_sb.sb_features_incompat &&
+	    upgrade->log_incompat == mp->m_sb.sb_features_log_incompat)
+		return true;
+
+	/* Upgrade primary superblock. */
+	if (!get_sb(agno, &tsb))
+		goto fail;
+
+	dbprintf(_("Upgrading V5 filesystem\n"));
+
+	/* Save the old primary super features in case we revert. */
+	get_v5_features(&old, &tsb);
+
+	/* Update features and force user to run repair before mounting. */
+	set_v5_features(&tsb, upgrade);
+	tsb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+
+	/* Write new primary superblock */
+	libxfs_sb_to_disk(iocur_top->data, &tsb);
+	write_cur();
+	if (!iocur_top->bp || iocur_top->bp->b_error)
+		goto fail;
+
+	/* Update the secondary superblocks, or revert. */
+	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
+		if (!get_sb(agno, &tsb)) {
+			agno--;
+			goto revert;
+		}
+
+		/* Set features and write secondary super. */
+		set_v5_features(&tsb, upgrade);
+		tsb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+		libxfs_sb_to_disk(iocur_top->data, &tsb);
+		write_cur();
+
+		/* Write or abort. */
+		if (!iocur_top->bp || iocur_top->bp->b_error)
+			goto revert;
+	}
+
+	/* All superblocks updated, update the incore values. */
+	set_v5_features(&mp->m_sb, upgrade);
+	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+	dbprintf(_("Upgraded V5 filesystem.  Please run xfs_repair.\n"));
+	return true;
+
+revert:
+	/*
+	 * Try to revert feature flag changes, and don't worry if we fail.
+	 * We're probably in a mess anyhow, and the admin will have to run
+	 * repair anyways.
+	 */
+	for (revert_agno = 0; revert_agno <= agno; revert_agno++) {
+		if (!get_sb(revert_agno, &tsb))
+			continue;
+
+		set_v5_features(&tsb, &old);
+		libxfs_sb_to_disk(iocur_top->data, &tsb);
+		write_cur();
+	}
+fail:
+	dbprintf(
+_("Failed to upgrade V5 filesystem at AG %d, please run xfs_repair.\n"), agno);
+	return false;
+}
+
 static char *
 version_string(
 	xfs_sb_t	*sbp)
@@ -694,15 +814,12 @@ version_string(
 		strcat(s, ",INOBTCNT");
 	if (xfs_sb_version_hasbigtime(sbp))
 		strcat(s, ",BIGTIME");
+	if (xfs_sb_version_needsrepair(sbp))
+		strcat(s, ",NEEDSREPAIR");
 	return s;
 }
 
-/*
- * XXX: this only supports reading and writing to version 4 superblock fields.
- * V5 superblocks always define certain V4 feature bits - they are blocked from
- * being changed if a V5 sb is detected, but otherwise v5 superblock features
- * are not handled here.
- */
+/* Upgrade a superblock to support a feature. */
 static int
 version_f(
 	int		argc,
@@ -713,6 +830,9 @@ version_f(
 	xfs_agnumber_t	ag;
 
 	if (argc == 2) {	/* WRITE VERSION */
+		struct v5feat	v5features;
+
+		get_v5_features(&v5features, &mp->m_sb);
 
 		if ((x.isreadonly & LIBXFS_ISREADONLY) || !expert_mode) {
 			dbprintf(_("%s: not in expert mode, writing disabled\n"),
@@ -720,8 +840,23 @@ version_f(
 			return 0;
 		}
 
+		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
+			dbprintf(_("%s: filesystem needs xfs_repair\n"),
+				progname);
+			return 0;
+		}
+
 		/* Logic here derived from the IRIX xfs_chver(1M) script. */
-		if (!strcasecmp(argv[1], "extflg")) {
+		if (!strcasecmp(argv[1], "needsrepair")) {
+			if (!xfs_sb_version_hascrc(&mp->m_sb)) {
+				dbprintf(
+		_("needsrepair flag cannot be enabled on pre-V5 filesystems\n"));
+				exitcode = 2;
+				return 1;
+			}
+
+			v5features.incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+		} else if (!strcasecmp(argv[1], "extflg")) {
 			switch (XFS_SB_VERSION_NUM(&mp->m_sb)) {
 			case XFS_SB_VERSION_1:
 				version = 0x0004 | XFS_SB_VERSION_EXTFLGBIT;
@@ -813,6 +948,11 @@ version_f(
 			mp->m_sb.sb_versionnum = version;
 			mp->m_sb.sb_features2 = features;
 		}
+
+		if (!upgrade_v5_features(mp, &v5features)) {
+			exitcode = 1;
+			return 1;
+		}
 	}
 
 	if (argc == 3) {	/* VERSIONNUM + FEATURES2 */
diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
index 5c57b461..f465bd43 100755
--- a/db/xfs_admin.sh
+++ b/db/xfs_admin.sh
@@ -6,10 +6,11 @@
 
 status=0
 DB_OPTS=""
+DASH_O_DB_OPTS=""
 REPAIR_OPTS=""
-USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] device [logdev]"
+USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] [-O v5_feature] device [logdev]"
 
-while getopts "efjlpuc:L:U:V" c
+while getopts "efjlpuc:L:O:U:V" c
 do
 	case $c in
 	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
@@ -19,6 +20,13 @@ do
 	l)	DB_OPTS=$DB_OPTS" -r -c label";;
 	L)	DB_OPTS=$DB_OPTS" -c 'label "$OPTARG"'";;
 	p)	DB_OPTS=$DB_OPTS" -c 'version projid32bit'";;
+	O)
+		if [ -n "$DASH_O_DB_OPTS" ]; then
+			echo "-O can only be specified once." 1>&2
+			exit 1
+		fi
+		DASH_O_DB_OPTS=" -c 'version "$OPTARG"'"
+		;;
 	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
 	U)	DB_OPTS=$DB_OPTS" -c 'uuid "$OPTARG"'";;
 	V)	xfs_db -p xfs_admin -V
@@ -30,6 +38,13 @@ do
 		;;
 	esac
 done
+if [ -n "$DASH_O_DB_OPTS" ]; then
+	if [ -n "$DB_OPTS" ]; then
+		echo "-O can only be used by itself." 1>&2
+		exit 1
+	fi
+	DB_OPTS="$DASH_O_DB_OPTS"
+fi
 set -- extra $@
 shift $OPTIND
 case $# in
@@ -48,6 +63,12 @@ case $# in
 		fi
 		if [ $status -eq 1 ]; then
 			echo "Conversion failed due to filesystem errors; run xfs_repair."
+		elif xfs_db -c 'version' "$1" | grep -q NEEDSREPAIR; then
+			# Upgrade required us to run repair, so force
+			# xfs_repair to run by adding a single space to
+			# REPAIR_OPTS.
+			echo "Running xfs_repair to complete the upgrade."
+			REPAIR_OPTS="$REPAIR_OPTS "
 		fi
 		if [ -n "$REPAIR_OPTS" ]
 		then
diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
index 8afc873f..d8a0125c 100644
--- a/man/man8/xfs_admin.8
+++ b/man/man8/xfs_admin.8
@@ -6,6 +6,8 @@ xfs_admin \- change parameters of an XFS filesystem
 [
 .B \-eflpu
 ] [
+.BI \-O " feature"
+] [
 .BR "\-c 0" | 1
 ] [
 .B \-L
@@ -103,6 +105,34 @@ The filesystem label can be cleared using the special "\c
 " value for
 .IR label .
 .TP
+.BI \-O " feature"
+Add a new feature to a V5 filesystem.
+Only one filesystem feature can be specified per invocation of xfs_admin.
+This option cannot be combined with any other
+.B xfs_admin
+option.
+.IP
+.B NOTE:
+Administrators must ensure the filesystem is clean by running
+.B xfs_repair -n
+to inspect the filesystem before performing the upgrade.
+If corruption is found, recovery procedures (e.g. reformat followed by
+restoration from backup; or running
+.B xfs_repair
+without the
+.BR -n )
+must be followed to clean the filesystem.
+.IP
+Features are as follows:
+.RS 0.7i
+.TP
+.B needsrepair
+Flag the filesystem as needing repairs.
+Until
+.BR xfs_repair (8)
+is run, the filesystem will not be mountable.
+.RE
+.TP
 .BI \-U " uuid"
 Set the UUID of the filesystem to
 .IR uuid .
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index ee57b03a..792d98c8 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -971,6 +971,11 @@ may toggle between
 and
 .B attr2
 at will (older kernels may not support the newer version).
+The filesystem can be flagged as requiring a run through
+.BR xfs_repair (8)
+if the
+.B needsrepair
+option is specified and the filesystem is formatted with the V5 format.
 .IP
 If no argument is given, the current version and feature bits are printed.
 With one argument, this command will write the updated version number
@@ -983,6 +988,9 @@ bits respectively, and their string equivalent reported
 (but no modifications are made).
 .IP
 If the feature upgrade succeeds, the program will return 0.
+The upgrade process may set the NEEDSREPAIR feature in the superblock to
+force the filesystem to be run through
+.BR xfs_repair (8).
 If the requested upgrade has already been applied to the filesystem, the
 program will also return 0.
 If the upgrade fails due to corruption or IO errors, the program will return

[PATCH 4/5] xfs_repair: fix unmount error message to have a newline

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Add a newline so that this is consistent with the other error messages.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 repair/xfs_repair.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 724661d8..9409f0d8 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -1136,7 +1136,7 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
 	error = -libxfs_umount(mp);
 	if (error)
 		do_error(
-	_("File system metadata writeout failed, err=%d.  Re-run xfs_repair."),
+	_("File system metadata writeout failed, err=%d.  Re-run xfs_repair.\n"),
 				error);
 
 	libxfs_destroy(&x);

[PATCH 5/5] xfs_repair: clear the needsrepair flag

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Clear the needsrepair flag, since it's used to prevent mounting of an
inconsistent filesystem.  We only do this if we make it to the end of
repair with a non-zero error code, and all the rebuilt indices and
corrected metadata are persisted correctly.

Note that we cannot combine clearing needsrepair with clearing the quota
checked flags because we need to clear the quota flags even if
reformatting the log fails, whereas we can't clear needsrepair if the
log reformat fails.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 include/xfs_mount.h |    1 +
 libxfs/init.c       |   12 ++++++++----
 repair/agheader.c   |   21 +++++++++++++++++++++
 repair/xfs_repair.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+), 4 deletions(-)


diff --git a/include/xfs_mount.h b/include/xfs_mount.h
index 36594643..77574045 100644
--- a/include/xfs_mount.h
+++ b/include/xfs_mount.h
@@ -181,6 +181,7 @@ xfs_perag_resv(
 
 extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
 				dev_t, dev_t, dev_t, int);
+int libxfs_flush_mount(struct xfs_mount *mp, bool purge);
 int		libxfs_umount(struct xfs_mount *mp);
 extern void	libxfs_rtmount_destroy (xfs_mount_t *);
 
diff --git a/libxfs/init.c b/libxfs/init.c
index 9fe13b8d..99b1f72a 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -870,9 +870,10 @@ _("%s: Flushing the %s failed, err=%d!\n"),
  * Flush all dirty buffers to stable storage and report on writes that didn't
  * make it to stable storage.
  */
-static int
+int
 libxfs_flush_mount(
-	struct xfs_mount	*mp)
+	struct xfs_mount	*mp,
+	bool			purge)
 {
 	int			error = 0;
 	int			err2;
@@ -884,7 +885,10 @@ libxfs_flush_mount(
 	 * cannot be written will cause the LOST_WRITE flag to be set in the
 	 * buftarg.
 	 */
-	libxfs_bcache_purge();
+	if (purge)
+		libxfs_bcache_purge();
+	else
+		libxfs_bcache_flush();
 
 	/* Flush all kernel and disk write caches, and report failures. */
 	if (mp->m_ddev_targp) {
@@ -923,7 +927,7 @@ libxfs_umount(
 
 	libxfs_rtmount_destroy(mp);
 
-	error = libxfs_flush_mount(mp);
+	error = libxfs_flush_mount(mp, true);
 
 	for (agno = 0; agno < mp->m_maxagi; agno++) {
 		pag = radix_tree_delete(&mp->m_perag_tree, agno);
diff --git a/repair/agheader.c b/repair/agheader.c
index 8bb99489..2af24106 100644
--- a/repair/agheader.c
+++ b/repair/agheader.c
@@ -452,6 +452,27 @@ secondary_sb_whack(
 			rval |= XR_AG_SB_SEC;
 	}
 
+	if (xfs_sb_version_needsrepair(sb)) {
+		if (i == 0) {
+			if (!no_modify)
+				do_warn(
+	_("clearing needsrepair flag and regenerating metadata\n"));
+			else
+				do_warn(
+	_("would clear needsrepair flag and regenerate metadata\n"));
+		} else {
+			/*
+			 * Quietly clear needsrepair on the secondary supers as
+			 * part of ensuring them.  If needsrepair is set on the
+			 * primary, it will be cleared at the end of repair
+			 * once we've flushed all other dirty blocks to disk.
+			 */
+			sb->sb_features_incompat &=
+					~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+			rval |= XR_AG_SB_SEC;
+		}
+	}
+
 	return(rval);
 }
 
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 9409f0d8..4ca4fe5a 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -712,6 +712,52 @@ check_fs_vs_host_sectsize(
 	}
 }
 
+/* Clear needsrepair after a successful repair run. */
+void
+clear_needsrepair(
+	struct xfs_mount	*mp)
+{
+	struct xfs_buf		*bp;
+	int			error;
+
+	/*
+	 * If we're going to clear NEEDSREPAIR, we need to make absolutely sure
+	 * that everything is ok with the ondisk filesystem.  At this point
+	 * we've flushed the filesystem metadata out of the buffer cache and
+	 * possibly rewrote the log, but we haven't forced the disks to persist
+	 * the writes to stable storage.  Do that now, and if anything goes
+	 * wrong, leave NEEDSREPAIR in place.  Don't purge the buffer cache
+	 * here since we're not done yet.
+	 */
+	error = -libxfs_flush_mount(mp, false);
+	if (error) {
+		do_warn(
+	_("Cannot clear needsrepair from primary super due to metadata checkpoint failure, err=%d.\n"),
+			error);
+		return;
+	}
+
+	/* Clear needsrepair from the superblock. */
+	bp = libxfs_getsb(mp);
+	if (!bp) {
+		do_warn(
+	_("Cannot clear needsrepair from primary super, out of memory.\n"));
+		return;
+	}
+	if (bp->b_error) {
+		do_warn(
+	_("Cannot clear needsrepair from primary super, IO err=%d.\n"),
+			bp->b_error);
+	} else {
+		mp->m_sb.sb_features_incompat &=
+				~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+		libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
+		libxfs_buf_mark_dirty(bp);
+	}
+	libxfs_buf_relse(bp);
+	return;
+}
+
 int
 main(int argc, char **argv)
 {
@@ -1132,6 +1178,9 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
 	libxfs_bcache_flush();
 	format_log_max_lsn(mp);
 
+	if (xfs_sb_version_needsrepair(&mp->m_sb))
+		clear_needsrepair(mp);
+
 	/* Report failure if anything failed to get written to our fs. */
 	error = -libxfs_umount(mp);
 	if (error)

Re: [PATCH 1/5] xfs_admin: clean up string quoting

[Brian Foster]

On Wed, Feb 03, 2021 at 11:43:17AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Clean up the string quoting in this script so that we don't trip over
> users feeding us arguments like "/dev/sd ha ha ha lol".
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

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

>  db/xfs_admin.sh |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
> index bd325da2..71a9aa98 100755
> --- a/db/xfs_admin.sh
> +++ b/db/xfs_admin.sh
> @@ -43,7 +43,7 @@ case $# in
>  
>  		if [ -n "$DB_OPTS" ]
>  		then
> -			eval xfs_db -x -p xfs_admin $DB_OPTS $1
> +			eval xfs_db -x -p xfs_admin $DB_OPTS "$1"
>  			status=$?
>  		fi
>  		if [ -n "$REPAIR_OPTS" ]
> @@ -53,7 +53,7 @@ case $# in
>  			# running xfs_admin.
>  			# Ideally, we need to improve the output behaviour
>  			# of repair for this purpose (say a "quiet" mode).
> -			eval xfs_repair $REPAIR_OPTS $1 2> /dev/null
> +			eval xfs_repair $REPAIR_OPTS "$1" 2> /dev/null
>  			status=`expr $? + $status`
>  			if [ $status -ne 0 ]
>  			then
> 

Re: [PATCH 3/5] xfs_db: support the needsrepair feature flag in the version command

[Brian Foster]

On Wed, Feb 03, 2021 at 11:43:29AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Teach the xfs_db version command about the 'needsrepair' flag, which can
> be used to force the system administrator to repair the filesystem with
> xfs_repair.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  db/check.c           |    5 ++
>  db/sb.c              |  154 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  db/xfs_admin.sh      |   25 +++++++-
>  man/man8/xfs_admin.8 |   30 ++++++++++
>  man/man8/xfs_db.8    |    8 +++
>  5 files changed, 213 insertions(+), 9 deletions(-)
> 
> 
...
> diff --git a/db/sb.c b/db/sb.c
> index f306e939..223b84fe 100644
> --- a/db/sb.c
> +++ b/db/sb.c
...
> @@ -720,8 +840,23 @@ version_f(
>  			return 0;
>  		}
>  
> +		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> +			dbprintf(_("%s: filesystem needs xfs_repair\n"),
> +				progname);
> +			return 0;
> +		}
> +
>  		/* Logic here derived from the IRIX xfs_chver(1M) script. */
> -		if (!strcasecmp(argv[1], "extflg")) {
> +		if (!strcasecmp(argv[1], "needsrepair")) {
> +			if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> +				dbprintf(
> +		_("needsrepair flag cannot be enabled on pre-V5 filesystems\n"));
> +				exitcode = 2;
> +				return 1;
> +			}

I still don't understand why we need this magic error code logic, here
and in xfs_admin.sh. Can we not just have xfs_db succeed or fail
(printing why if necessary) as it does today, and then let xfs_admin run
repair if $status == 0 && NEEDSREPAIR?

Brian

> +
> +			v5features.incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> +		} else if (!strcasecmp(argv[1], "extflg")) {
>  			switch (XFS_SB_VERSION_NUM(&mp->m_sb)) {
>  			case XFS_SB_VERSION_1:
>  				version = 0x0004 | XFS_SB_VERSION_EXTFLGBIT;
> @@ -813,6 +948,11 @@ version_f(
>  			mp->m_sb.sb_versionnum = version;
>  			mp->m_sb.sb_features2 = features;
>  		}
> +
> +		if (!upgrade_v5_features(mp, &v5features)) {
> +			exitcode = 1;
> +			return 1;
> +		}
>  	}
>  
>  	if (argc == 3) {	/* VERSIONNUM + FEATURES2 */
> diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
> index 5c57b461..f465bd43 100755
> --- a/db/xfs_admin.sh
> +++ b/db/xfs_admin.sh
> @@ -6,10 +6,11 @@
>  
>  status=0
>  DB_OPTS=""
> +DASH_O_DB_OPTS=""
>  REPAIR_OPTS=""
> -USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] device [logdev]"
> +USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] [-O v5_feature] device [logdev]"
>  
> -while getopts "efjlpuc:L:U:V" c
> +while getopts "efjlpuc:L:O:U:V" c
>  do
>  	case $c in
>  	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
> @@ -19,6 +20,13 @@ do
>  	l)	DB_OPTS=$DB_OPTS" -r -c label";;
>  	L)	DB_OPTS=$DB_OPTS" -c 'label "$OPTARG"'";;
>  	p)	DB_OPTS=$DB_OPTS" -c 'version projid32bit'";;
> +	O)
> +		if [ -n "$DASH_O_DB_OPTS" ]; then
> +			echo "-O can only be specified once." 1>&2
> +			exit 1
> +		fi
> +		DASH_O_DB_OPTS=" -c 'version "$OPTARG"'"
> +		;;
>  	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
>  	U)	DB_OPTS=$DB_OPTS" -c 'uuid "$OPTARG"'";;
>  	V)	xfs_db -p xfs_admin -V
> @@ -30,6 +38,13 @@ do
>  		;;
>  	esac
>  done
> +if [ -n "$DASH_O_DB_OPTS" ]; then
> +	if [ -n "$DB_OPTS" ]; then
> +		echo "-O can only be used by itself." 1>&2
> +		exit 1
> +	fi
> +	DB_OPTS="$DASH_O_DB_OPTS"
> +fi
>  set -- extra $@
>  shift $OPTIND
>  case $# in
> @@ -48,6 +63,12 @@ case $# in
>  		fi
>  		if [ $status -eq 1 ]; then
>  			echo "Conversion failed due to filesystem errors; run xfs_repair."
> +		elif xfs_db -c 'version' "$1" | grep -q NEEDSREPAIR; then
> +			# Upgrade required us to run repair, so force
> +			# xfs_repair to run by adding a single space to
> +			# REPAIR_OPTS.
> +			echo "Running xfs_repair to complete the upgrade."
> +			REPAIR_OPTS="$REPAIR_OPTS "
>  		fi
>  		if [ -n "$REPAIR_OPTS" ]
>  		then
> diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
> index 8afc873f..d8a0125c 100644
> --- a/man/man8/xfs_admin.8
> +++ b/man/man8/xfs_admin.8
> @@ -6,6 +6,8 @@ xfs_admin \- change parameters of an XFS filesystem
>  [
>  .B \-eflpu
>  ] [
> +.BI \-O " feature"
> +] [
>  .BR "\-c 0" | 1
>  ] [
>  .B \-L
> @@ -103,6 +105,34 @@ The filesystem label can be cleared using the special "\c
>  " value for
>  .IR label .
>  .TP
> +.BI \-O " feature"
> +Add a new feature to a V5 filesystem.
> +Only one filesystem feature can be specified per invocation of xfs_admin.
> +This option cannot be combined with any other
> +.B xfs_admin
> +option.
> +.IP
> +.B NOTE:
> +Administrators must ensure the filesystem is clean by running
> +.B xfs_repair -n
> +to inspect the filesystem before performing the upgrade.
> +If corruption is found, recovery procedures (e.g. reformat followed by
> +restoration from backup; or running
> +.B xfs_repair
> +without the
> +.BR -n )
> +must be followed to clean the filesystem.
> +.IP
> +Features are as follows:
> +.RS 0.7i
> +.TP
> +.B needsrepair
> +Flag the filesystem as needing repairs.
> +Until
> +.BR xfs_repair (8)
> +is run, the filesystem will not be mountable.
> +.RE
> +.TP
>  .BI \-U " uuid"
>  Set the UUID of the filesystem to
>  .IR uuid .
> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index ee57b03a..792d98c8 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -971,6 +971,11 @@ may toggle between
>  and
>  .B attr2
>  at will (older kernels may not support the newer version).
> +The filesystem can be flagged as requiring a run through
> +.BR xfs_repair (8)
> +if the
> +.B needsrepair
> +option is specified and the filesystem is formatted with the V5 format.
>  .IP
>  If no argument is given, the current version and feature bits are printed.
>  With one argument, this command will write the updated version number
> @@ -983,6 +988,9 @@ bits respectively, and their string equivalent reported
>  (but no modifications are made).
>  .IP
>  If the feature upgrade succeeds, the program will return 0.
> +The upgrade process may set the NEEDSREPAIR feature in the superblock to
> +force the filesystem to be run through
> +.BR xfs_repair (8).
>  If the requested upgrade has already been applied to the filesystem, the
>  program will also return 0.
>  If the upgrade fails due to corruption or IO errors, the program will return
> 

Re: [PATCH 5/5] xfs_repair: clear the needsrepair flag

[Brian Foster]

On Wed, Feb 03, 2021 at 11:43:40AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Clear the needsrepair flag, since it's used to prevent mounting of an
> inconsistent filesystem.  We only do this if we make it to the end of
> repair with a non-zero error code, and all the rebuilt indices and
> corrected metadata are persisted correctly.
> 
> Note that we cannot combine clearing needsrepair with clearing the quota
> checked flags because we need to clear the quota flags even if
> reformatting the log fails, whereas we can't clear needsrepair if the
> log reformat fails.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Looks sane, just some nits...

>  include/xfs_mount.h |    1 +
>  libxfs/init.c       |   12 ++++++++----
>  repair/agheader.c   |   21 +++++++++++++++++++++
>  repair/xfs_repair.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 79 insertions(+), 4 deletions(-)
> 
> 
...
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 9fe13b8d..99b1f72a 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -870,9 +870,10 @@ _("%s: Flushing the %s failed, err=%d!\n"),
>   * Flush all dirty buffers to stable storage and report on writes that didn't
>   * make it to stable storage.
>   */
> -static int
> +int
>  libxfs_flush_mount(
> -	struct xfs_mount	*mp)
> +	struct xfs_mount	*mp,
> +	bool			purge)
>  {
>  	int			error = 0;
>  	int			err2;
> @@ -884,7 +885,10 @@ libxfs_flush_mount(
>  	 * cannot be written will cause the LOST_WRITE flag to be set in the
>  	 * buftarg.
>  	 */
> -	libxfs_bcache_purge();
> +	if (purge)
> +		libxfs_bcache_purge();
> +	else
> +		libxfs_bcache_flush();

Instead of the parameter, could we just lift the purge into the call
that requires it and let libxfs_flush_mount() just do flushes? I'm
assuming the bcache would be empty in the umount case so the extra flush
should pretty much be a no-op.

>  
>  	/* Flush all kernel and disk write caches, and report failures. */
>  	if (mp->m_ddev_targp) {
> @@ -923,7 +927,7 @@ libxfs_umount(
>  
>  	libxfs_rtmount_destroy(mp);
>  
> -	error = libxfs_flush_mount(mp);
> +	error = libxfs_flush_mount(mp, true);
>  
>  	for (agno = 0; agno < mp->m_maxagi; agno++) {
>  		pag = radix_tree_delete(&mp->m_perag_tree, agno);
...
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 9409f0d8..4ca4fe5a 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -712,6 +712,52 @@ check_fs_vs_host_sectsize(
>  	}
>  }
>  
> +/* Clear needsrepair after a successful repair run. */
> +void
> +clear_needsrepair(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_buf		*bp;
> +	int			error;
> +
> +	/*
> +	 * If we're going to clear NEEDSREPAIR, we need to make absolutely sure
> +	 * that everything is ok with the ondisk filesystem.  At this point
> +	 * we've flushed the filesystem metadata out of the buffer cache and
> +	 * possibly rewrote the log, but we haven't forced the disks to persist
> +	 * the writes to stable storage.  Do that now, and if anything goes
> +	 * wrong, leave NEEDSREPAIR in place.  Don't purge the buffer cache
> +	 * here since we're not done yet.
> +	 */
> +	error = -libxfs_flush_mount(mp, false);
> +	if (error) {
> +		do_warn(
> +	_("Cannot clear needsrepair from primary super due to metadata checkpoint failure, err=%d.\n"),
> +			error);

Not sure what metadata checkpoint failure means.. maybe just say that a
flush failed?

> +		return;
> +	}
> +
> +	/* Clear needsrepair from the superblock. */
> +	bp = libxfs_getsb(mp);
> +	if (!bp) {
> +		do_warn(
> +	_("Cannot clear needsrepair from primary super, out of memory.\n"));
> +		return;
> +	}
> +	if (bp->b_error) {
> +		do_warn(
> +	_("Cannot clear needsrepair from primary super, IO err=%d.\n"),
> +			bp->b_error);
> +	} else {

Maybe try to condense this a bit to something like the following to
reduce the number of branches and strings to translate and whatnot:

	if (!bp || bp->b_error) {
		do_warn(
		"Failed to clear needsrepair from primary super, err=%d.\n",
			bp ? bp->b_error : -ENOMEM);
		goto out;
	}

	...
out:
	libxfs_buf_release(bp);
}

> +		mp->m_sb.sb_features_incompat &=
> +				~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> +		libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> +		libxfs_buf_mark_dirty(bp);
> +	}
> +	libxfs_buf_relse(bp);
> +	return;

No need for the return statement here.

Brian

> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> @@ -1132,6 +1178,9 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
>  	libxfs_bcache_flush();
>  	format_log_max_lsn(mp);
>  
> +	if (xfs_sb_version_needsrepair(&mp->m_sb))
> +		clear_needsrepair(mp);
> +
>  	/* Report failure if anything failed to get written to our fs. */
>  	error = -libxfs_umount(mp);
>  	if (error)
> 

Re: [PATCH 5/5] xfs_repair: clear the needsrepair flag

[Darrick J. Wong]

On Thu, Feb 04, 2021 at 12:55:12PM -0500, Brian Foster wrote:
> On Wed, Feb 03, 2021 at 11:43:40AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Clear the needsrepair flag, since it's used to prevent mounting of an
> > inconsistent filesystem.  We only do this if we make it to the end of
> > repair with a non-zero error code, and all the rebuilt indices and
> > corrected metadata are persisted correctly.
> > 
> > Note that we cannot combine clearing needsrepair with clearing the quota
> > checked flags because we need to clear the quota flags even if
> > reformatting the log fails, whereas we can't clear needsrepair if the
> > log reformat fails.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> Looks sane, just some nits...
> 
> >  include/xfs_mount.h |    1 +
> >  libxfs/init.c       |   12 ++++++++----
> >  repair/agheader.c   |   21 +++++++++++++++++++++
> >  repair/xfs_repair.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 79 insertions(+), 4 deletions(-)
> > 
> > 
> ...
> > diff --git a/libxfs/init.c b/libxfs/init.c
> > index 9fe13b8d..99b1f72a 100644
> > --- a/libxfs/init.c
> > +++ b/libxfs/init.c
> > @@ -870,9 +870,10 @@ _("%s: Flushing the %s failed, err=%d!\n"),
> >   * Flush all dirty buffers to stable storage and report on writes that didn't
> >   * make it to stable storage.
> >   */
> > -static int
> > +int
> >  libxfs_flush_mount(
> > -	struct xfs_mount	*mp)
> > +	struct xfs_mount	*mp,
> > +	bool			purge)
> >  {
> >  	int			error = 0;
> >  	int			err2;
> > @@ -884,7 +885,10 @@ libxfs_flush_mount(
> >  	 * cannot be written will cause the LOST_WRITE flag to be set in the
> >  	 * buftarg.
> >  	 */
> > -	libxfs_bcache_purge();
> > +	if (purge)
> > +		libxfs_bcache_purge();
> > +	else
> > +		libxfs_bcache_flush();
> 
> Instead of the parameter, could we just lift the purge into the call
> that requires it and let libxfs_flush_mount() just do flushes? I'm
> assuming the bcache would be empty in the umount case so the extra flush
> should pretty much be a no-op.

<nod> Will do.

> 
> >  
> >  	/* Flush all kernel and disk write caches, and report failures. */
> >  	if (mp->m_ddev_targp) {
> > @@ -923,7 +927,7 @@ libxfs_umount(
> >  
> >  	libxfs_rtmount_destroy(mp);
> >  
> > -	error = libxfs_flush_mount(mp);
> > +	error = libxfs_flush_mount(mp, true);
> >  
> >  	for (agno = 0; agno < mp->m_maxagi; agno++) {
> >  		pag = radix_tree_delete(&mp->m_perag_tree, agno);
> ...
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 9409f0d8..4ca4fe5a 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -712,6 +712,52 @@ check_fs_vs_host_sectsize(
> >  	}
> >  }
> >  
> > +/* Clear needsrepair after a successful repair run. */
> > +void
> > +clear_needsrepair(
> > +	struct xfs_mount	*mp)
> > +{
> > +	struct xfs_buf		*bp;
> > +	int			error;
> > +
> > +	/*
> > +	 * If we're going to clear NEEDSREPAIR, we need to make absolutely sure
> > +	 * that everything is ok with the ondisk filesystem.  At this point
> > +	 * we've flushed the filesystem metadata out of the buffer cache and
> > +	 * possibly rewrote the log, but we haven't forced the disks to persist
> > +	 * the writes to stable storage.  Do that now, and if anything goes
> > +	 * wrong, leave NEEDSREPAIR in place.  Don't purge the buffer cache
> > +	 * here since we're not done yet.
> > +	 */
> > +	error = -libxfs_flush_mount(mp, false);
> > +	if (error) {
> > +		do_warn(
> > +	_("Cannot clear needsrepair from primary super due to metadata checkpoint failure, err=%d.\n"),
> > +			error);
> 
> Not sure what metadata checkpoint failure means.. maybe just say that a
> flush failed?

Ok.

> > +		return;
> > +	}
> > +
> > +	/* Clear needsrepair from the superblock. */
> > +	bp = libxfs_getsb(mp);
> > +	if (!bp) {
> > +		do_warn(
> > +	_("Cannot clear needsrepair from primary super, out of memory.\n"));
> > +		return;
> > +	}
> > +	if (bp->b_error) {
> > +		do_warn(
> > +	_("Cannot clear needsrepair from primary super, IO err=%d.\n"),
> > +			bp->b_error);
> > +	} else {
> 
> Maybe try to condense this a bit to something like the following to
> reduce the number of branches and strings to translate and whatnot:
> 
> 	if (!bp || bp->b_error) {
> 		do_warn(
> 		"Failed to clear needsrepair from primary super, err=%d.\n",
> 			bp ? bp->b_error : -ENOMEM);
> 		goto out;
> 	}
> 
> 	...
> out:
> 	libxfs_buf_release(bp);

Ok.

> }
> 
> > +		mp->m_sb.sb_features_incompat &=
> > +				~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > +		libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > +		libxfs_buf_mark_dirty(bp);
> > +	}
> > +	libxfs_buf_relse(bp);
> > +	return;
> 
> No need for the return statement here.

Fixed, thanks for the nits. :)

--D

> Brian
> 
> > +}
> > +
> >  int
> >  main(int argc, char **argv)
> >  {
> > @@ -1132,6 +1178,9 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
> >  	libxfs_bcache_flush();
> >  	format_log_max_lsn(mp);
> >  
> > +	if (xfs_sb_version_needsrepair(&mp->m_sb))
> > +		clear_needsrepair(mp);
> > +
> >  	/* Report failure if anything failed to get written to our fs. */
> >  	error = -libxfs_umount(mp);
> >  	if (error)
> > 
> 

Re: [PATCH 3/5] xfs_db: support the needsrepair feature flag in the version command

[Darrick J. Wong]

On Thu, Feb 04, 2021 at 12:54:12PM -0500, Brian Foster wrote:
> On Wed, Feb 03, 2021 at 11:43:29AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Teach the xfs_db version command about the 'needsrepair' flag, which can
> > be used to force the system administrator to repair the filesystem with
> > xfs_repair.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  db/check.c           |    5 ++
> >  db/sb.c              |  154 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  db/xfs_admin.sh      |   25 +++++++-
> >  man/man8/xfs_admin.8 |   30 ++++++++++
> >  man/man8/xfs_db.8    |    8 +++
> >  5 files changed, 213 insertions(+), 9 deletions(-)
> > 
> > 
> ...
> > diff --git a/db/sb.c b/db/sb.c
> > index f306e939..223b84fe 100644
> > --- a/db/sb.c
> > +++ b/db/sb.c
> ...
> > @@ -720,8 +840,23 @@ version_f(
> >  			return 0;
> >  		}
> >  
> > +		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> > +			dbprintf(_("%s: filesystem needs xfs_repair\n"),
> > +				progname);
> > +			return 0;
> > +		}
> > +
> >  		/* Logic here derived from the IRIX xfs_chver(1M) script. */
> > -		if (!strcasecmp(argv[1], "extflg")) {
> > +		if (!strcasecmp(argv[1], "needsrepair")) {
> > +			if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> > +				dbprintf(
> > +		_("needsrepair flag cannot be enabled on pre-V5 filesystems\n"));
> > +				exitcode = 2;
> > +				return 1;
> > +			}
> 
> I still don't understand why we need this magic error code logic, here
> and in xfs_admin.sh. Can we not just have xfs_db succeed or fail
> (printing why if necessary) as it does today, and then let xfs_admin run
> repair if $status == 0 && NEEDSREPAIR?

I wanted to establish a new behavior that xfs_db would return an error
code for upgrade requests that aren't applicable to the filesystem.

But, seeing as the version command already exhibits the "return zero"
behavior if you try to "add" proji32bit to a V5 fs or feed version_f a
garbage parameter, I'll just keep doing things the way we do them now so
that I can at least get this part (and the long delayed fstests updates
for these three new features) going.

--D

> Brian
> 
> > +
> > +			v5features.incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > +		} else if (!strcasecmp(argv[1], "extflg")) {
> >  			switch (XFS_SB_VERSION_NUM(&mp->m_sb)) {
> >  			case XFS_SB_VERSION_1:
> >  				version = 0x0004 | XFS_SB_VERSION_EXTFLGBIT;
> > @@ -813,6 +948,11 @@ version_f(
> >  			mp->m_sb.sb_versionnum = version;
> >  			mp->m_sb.sb_features2 = features;
> >  		}
> > +
> > +		if (!upgrade_v5_features(mp, &v5features)) {
> > +			exitcode = 1;
> > +			return 1;
> > +		}
> >  	}
> >  
> >  	if (argc == 3) {	/* VERSIONNUM + FEATURES2 */
> > diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
> > index 5c57b461..f465bd43 100755
> > --- a/db/xfs_admin.sh
> > +++ b/db/xfs_admin.sh
> > @@ -6,10 +6,11 @@
> >  
> >  status=0
> >  DB_OPTS=""
> > +DASH_O_DB_OPTS=""
> >  REPAIR_OPTS=""
> > -USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] device [logdev]"
> > +USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] [-O v5_feature] device [logdev]"
> >  
> > -while getopts "efjlpuc:L:U:V" c
> > +while getopts "efjlpuc:L:O:U:V" c
> >  do
> >  	case $c in
> >  	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
> > @@ -19,6 +20,13 @@ do
> >  	l)	DB_OPTS=$DB_OPTS" -r -c label";;
> >  	L)	DB_OPTS=$DB_OPTS" -c 'label "$OPTARG"'";;
> >  	p)	DB_OPTS=$DB_OPTS" -c 'version projid32bit'";;
> > +	O)
> > +		if [ -n "$DASH_O_DB_OPTS" ]; then
> > +			echo "-O can only be specified once." 1>&2
> > +			exit 1
> > +		fi
> > +		DASH_O_DB_OPTS=" -c 'version "$OPTARG"'"
> > +		;;
> >  	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
> >  	U)	DB_OPTS=$DB_OPTS" -c 'uuid "$OPTARG"'";;
> >  	V)	xfs_db -p xfs_admin -V
> > @@ -30,6 +38,13 @@ do
> >  		;;
> >  	esac
> >  done
> > +if [ -n "$DASH_O_DB_OPTS" ]; then
> > +	if [ -n "$DB_OPTS" ]; then
> > +		echo "-O can only be used by itself." 1>&2
> > +		exit 1
> > +	fi
> > +	DB_OPTS="$DASH_O_DB_OPTS"
> > +fi
> >  set -- extra $@
> >  shift $OPTIND
> >  case $# in
> > @@ -48,6 +63,12 @@ case $# in
> >  		fi
> >  		if [ $status -eq 1 ]; then
> >  			echo "Conversion failed due to filesystem errors; run xfs_repair."
> > +		elif xfs_db -c 'version' "$1" | grep -q NEEDSREPAIR; then
> > +			# Upgrade required us to run repair, so force
> > +			# xfs_repair to run by adding a single space to
> > +			# REPAIR_OPTS.
> > +			echo "Running xfs_repair to complete the upgrade."
> > +			REPAIR_OPTS="$REPAIR_OPTS "
> >  		fi
> >  		if [ -n "$REPAIR_OPTS" ]
> >  		then
> > diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
> > index 8afc873f..d8a0125c 100644
> > --- a/man/man8/xfs_admin.8
> > +++ b/man/man8/xfs_admin.8
> > @@ -6,6 +6,8 @@ xfs_admin \- change parameters of an XFS filesystem
> >  [
> >  .B \-eflpu
> >  ] [
> > +.BI \-O " feature"
> > +] [
> >  .BR "\-c 0" | 1
> >  ] [
> >  .B \-L
> > @@ -103,6 +105,34 @@ The filesystem label can be cleared using the special "\c
> >  " value for
> >  .IR label .
> >  .TP
> > +.BI \-O " feature"
> > +Add a new feature to a V5 filesystem.
> > +Only one filesystem feature can be specified per invocation of xfs_admin.
> > +This option cannot be combined with any other
> > +.B xfs_admin
> > +option.
> > +.IP
> > +.B NOTE:
> > +Administrators must ensure the filesystem is clean by running
> > +.B xfs_repair -n
> > +to inspect the filesystem before performing the upgrade.
> > +If corruption is found, recovery procedures (e.g. reformat followed by
> > +restoration from backup; or running
> > +.B xfs_repair
> > +without the
> > +.BR -n )
> > +must be followed to clean the filesystem.
> > +.IP
> > +Features are as follows:
> > +.RS 0.7i
> > +.TP
> > +.B needsrepair
> > +Flag the filesystem as needing repairs.
> > +Until
> > +.BR xfs_repair (8)
> > +is run, the filesystem will not be mountable.
> > +.RE
> > +.TP
> >  .BI \-U " uuid"
> >  Set the UUID of the filesystem to
> >  .IR uuid .
> > diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> > index ee57b03a..792d98c8 100644
> > --- a/man/man8/xfs_db.8
> > +++ b/man/man8/xfs_db.8
> > @@ -971,6 +971,11 @@ may toggle between
> >  and
> >  .B attr2
> >  at will (older kernels may not support the newer version).
> > +The filesystem can be flagged as requiring a run through
> > +.BR xfs_repair (8)
> > +if the
> > +.B needsrepair
> > +option is specified and the filesystem is formatted with the V5 format.
> >  .IP
> >  If no argument is given, the current version and feature bits are printed.
> >  With one argument, this command will write the updated version number
> > @@ -983,6 +988,9 @@ bits respectively, and their string equivalent reported
> >  (but no modifications are made).
> >  .IP
> >  If the feature upgrade succeeds, the program will return 0.
> > +The upgrade process may set the NEEDSREPAIR feature in the superblock to
> > +force the filesystem to be run through
> > +.BR xfs_repair (8).
> >  If the requested upgrade has already been applied to the filesystem, the
> >  program will also return 0.
> >  If the upgrade fails due to corruption or IO errors, the program will return
> > 
>