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

[Brian Foster <bfoster@redhat.com>]

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
>