[PATCHSET v6 0/7] xfsprogs: add the ability to flag a fs for repair

[PATCHSET v6 0/7] xfsprogs: add the ability to flag a fs for repair

[Darrick J. Wong]

Hi all,

This series adds userspace support for 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.
All of these patches are reviewed and (hopefully) already in Eric's
upstream tree.

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
v4: move all the upgrader code to xfs_repair per Eric suggestion
v5: various fixes suggested by reviewers, and document deprecated V4 options
v6: break this series into smaller pieces since this is all the reviewed stuff;
    and fix some comments

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              |   13 ++++++++++++
 db/xfs_admin.sh      |   13 ++++++------
 include/xfs_mount.h  |    1 +
 libxfs/init.c        |   20 ++++++++++++------
 man/man8/xfs_admin.8 |    8 +++++++
 repair/agheader.c    |   21 +++++++++++++++++++
 repair/xfs_repair.c  |   56 ++++++++++++++++++++++++++++++++++++++++++--------
 8 files changed, 115 insertions(+), 22 deletions(-)

[PATCH 1/7] 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>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.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 1/7] xfs_admin: clean up string quoting

[Allison Henderson]

On 2/22/21 8:00 PM, 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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Looks ok
Reviewed-by: Allison Henderson <allison.henderson@oracle.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
> 

[PATCH 2/7] xfs_admin: support filesystems with realtime devices

[Darrick J. Wong]

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

Add a -r option to xfs_admin so that we can pass the name of the
realtime device to xfs_repair.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 db/xfs_admin.sh      |   11 ++++++-----
 man/man8/xfs_admin.8 |    8 ++++++++
 2 files changed, 14 insertions(+), 5 deletions(-)


diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
index 71a9aa98..430872ef 100755
--- a/db/xfs_admin.sh
+++ b/db/xfs_admin.sh
@@ -7,9 +7,10 @@
 status=0
 DB_OPTS=""
 REPAIR_OPTS=""
-USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] device [logdev]"
+REPAIR_DEV_OPTS=""
+USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-r rtdev] [-U uuid] device [logdev]"
 
-while getopts "efjlpuc:L:U:V" c
+while getopts "c:efjlL:pr:uU:V" c
 do
 	case $c in
 	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
@@ -19,6 +20,7 @@ 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'";;
+	r)	REPAIR_DEV_OPTS=" -r '$OPTARG'";;
 	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
 	U)	DB_OPTS=$DB_OPTS" -c 'uuid "$OPTARG"'";;
 	V)	xfs_db -p xfs_admin -V
@@ -37,8 +39,7 @@ case $# in
 		# Pick up the log device, if present
 		if [ -n "$2" ]; then
 			DB_OPTS=$DB_OPTS" -l '$2'"
-			test -n "$REPAIR_OPTS" && \
-				REPAIR_OPTS=$REPAIR_OPTS" -l '$2'"
+			REPAIR_DEV_OPTS=$REPAIR_DEV_OPTS" -l '$2'"
 		fi
 
 		if [ -n "$DB_OPTS" ]
@@ -53,7 +54,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_DEV_OPTS $REPAIR_OPTS "$1" 2> /dev/null
 			status=`expr $? + $status`
 			if [ $status -ne 0 ]
 			then
diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
index 8afc873f..cccbb224 100644
--- a/man/man8/xfs_admin.8
+++ b/man/man8/xfs_admin.8
@@ -13,6 +13,9 @@ xfs_admin \- change parameters of an XFS filesystem
 ] [
 .B \-U
 .I uuid
+] [
+.B \-r
+.I rtdev
 ]
 .I device
 [
@@ -123,6 +126,11 @@ not be able to mount the filesystem.  To remove this incompatible flag, use
 which will restore the original UUID and remove the incompatible
 feature flag as needed.
 .TP
+.BI \-r " rtdev"
+Specifies the device special file where the filesystem's realtime section
+resides.
+Only for those filesystems which use a realtime section.
+.TP
 .B \-V
 Prints the version number and exits.
 .PP

Re: [PATCH 2/7] xfs_admin: support filesystems with realtime devices

[Allison Henderson]

On 2/22/21 8:00 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Add a -r option to xfs_admin so that we can pass the name of the
> realtime device to xfs_repair.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Looks ok to me
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>   db/xfs_admin.sh      |   11 ++++++-----
>   man/man8/xfs_admin.8 |    8 ++++++++
>   2 files changed, 14 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
> index 71a9aa98..430872ef 100755
> --- a/db/xfs_admin.sh
> +++ b/db/xfs_admin.sh
> @@ -7,9 +7,10 @@
>   status=0
>   DB_OPTS=""
>   REPAIR_OPTS=""
> -USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] device [logdev]"
> +REPAIR_DEV_OPTS=""
> +USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-r rtdev] [-U uuid] device [logdev]"
>   
> -while getopts "efjlpuc:L:U:V" c
> +while getopts "c:efjlL:pr:uU:V" c
>   do
>   	case $c in
>   	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
> @@ -19,6 +20,7 @@ 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'";;
> +	r)	REPAIR_DEV_OPTS=" -r '$OPTARG'";;
>   	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
>   	U)	DB_OPTS=$DB_OPTS" -c 'uuid "$OPTARG"'";;
>   	V)	xfs_db -p xfs_admin -V
> @@ -37,8 +39,7 @@ case $# in
>   		# Pick up the log device, if present
>   		if [ -n "$2" ]; then
>   			DB_OPTS=$DB_OPTS" -l '$2'"
> -			test -n "$REPAIR_OPTS" && \
> -				REPAIR_OPTS=$REPAIR_OPTS" -l '$2'"
> +			REPAIR_DEV_OPTS=$REPAIR_DEV_OPTS" -l '$2'"
>   		fi
>   
>   		if [ -n "$DB_OPTS" ]
> @@ -53,7 +54,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_DEV_OPTS $REPAIR_OPTS "$1" 2> /dev/null
>   			status=`expr $? + $status`
>   			if [ $status -ne 0 ]
>   			then
> diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
> index 8afc873f..cccbb224 100644
> --- a/man/man8/xfs_admin.8
> +++ b/man/man8/xfs_admin.8
> @@ -13,6 +13,9 @@ xfs_admin \- change parameters of an XFS filesystem
>   ] [
>   .B \-U
>   .I uuid
> +] [
> +.B \-r
> +.I rtdev
>   ]
>   .I device
>   [
> @@ -123,6 +126,11 @@ not be able to mount the filesystem.  To remove this incompatible flag, use
>   which will restore the original UUID and remove the incompatible
>   feature flag as needed.
>   .TP
> +.BI \-r " rtdev"
> +Specifies the device special file where the filesystem's realtime section
> +resides.
> +Only for those filesystems which use a realtime section.
> +.TP
>   .B \-V
>   Prints the version number and exits.
>   .PP
> 

[PATCH 3/7] xfs_db: report the needsrepair flag in check and version commands

[Darrick J. Wong]

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

Teach the version and check commands to report the presence of the
NEEDSREPAIR flag.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 db/check.c |    5 +++++
 db/sb.c    |    2 ++
 2 files changed, 7 insertions(+)


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 d09f653d..d7111e92 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -691,6 +691,8 @@ 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;
 }
 

Re: [PATCH 3/7] xfs_db: report the needsrepair flag in check and version commands

[Allison Henderson]

On 2/22/21 8:00 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Teach the version and check commands to report the presence of the
> NEEDSREPAIR flag.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Looks good
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>   db/check.c |    5 +++++
>   db/sb.c    |    2 ++
>   2 files changed, 7 insertions(+)
> 
> 
> 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 d09f653d..d7111e92 100644
> --- a/db/sb.c
> +++ b/db/sb.c
> @@ -691,6 +691,8 @@ 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;
>   }
>   
> 

Re: [PATCH 3/7] xfs_db: report the needsrepair flag in check and version commands

[Christoph Hellwig]

On Mon, Feb 22, 2021 at 07:00:35PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Teach the version and check commands to report the presence of the
> NEEDSREPAIR flag.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/7] xfs_db: don't allow label/uuid setting if the needsrepair flag is set

[Darrick J. Wong]

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

The NEEDSREPAIR flag can be set on filesystems where we /know/ that
there's something wrong with the metadata and want to force the sysadmin
to run xfs_repair before the next mount.  The goal here is to prevent
non-repair changes to a filesystem when we are confident of its
instability.  Normally we wouldn't bother with such safety checks for
the debugger, but the label and uuid functions can be called from
xfs_admin, so we should prevent these administrative tasks until the
filesystem can be repaired.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 db/sb.c |   11 +++++++++++
 1 file changed, 11 insertions(+)


diff --git a/db/sb.c b/db/sb.c
index d7111e92..cec7dce9 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) {

Re: [PATCH 4/7] xfs_db: don't allow label/uuid setting if the needsrepair flag is set

[Allison Henderson]

On 2/22/21 8:00 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The NEEDSREPAIR flag can be set on filesystems where we /know/ that
> there's something wrong with the metadata and want to force the sysadmin
> to run xfs_repair before the next mount.  The goal here is to prevent
> non-repair changes to a filesystem when we are confident of its
> instability.  Normally we wouldn't bother with such safety checks for
> the debugger, but the label and uuid functions can be called from
> xfs_admin, so we should prevent these administrative tasks until the
> filesystem can be repaired.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>   db/sb.c |   11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> 
> diff --git a/db/sb.c b/db/sb.c
> index d7111e92..cec7dce9 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) {
> 

Re: [PATCH 4/7] xfs_db: don't allow label/uuid setting if the needsrepair flag is set

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 5/7] 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.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);

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

[Allison Henderson]

On 2/22/21 8:00 PM, Darrick J. Wong wrote:
> 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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.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 6/7] xfs_repair: clear quota CHKD flags on the incore superblock too

[Darrick J. Wong]

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

At the end of a repair run, xfs_repair clears the superblock's quota
checked flags if it found mistakes in the quota accounting to force a
quotacheck at the next mount.  This is currently the last time repair
modifies the primary superblock, so it is sufficient to update the
ondisk buffer and not the incore mount structure.

However, we're about to introduce code to clear the needsrepair feature
at the very end of repair, after all metadata blocks have been written
to disk and all disk caches flush.  Since the convention everywhere else
in xfs is to update the incore superblock, call libxfs_sb_to_disk to
translate that into the ondisk buffer, and then write the buffer to
disk, switch the quota CHKD code to use this mechanism too.

(Get rid of dsb too, since the incore super should be in sync with the
ondisk super.)

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 repair/xfs_repair.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)


diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 9409f0d8..40352458 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -717,7 +717,6 @@ main(int argc, char **argv)
 {
 	xfs_mount_t	*temp_mp;
 	xfs_mount_t	*mp;
-	xfs_dsb_t	*dsb;
 	struct xfs_buf	*sbp;
 	xfs_mount_t	xfs_m;
 	struct xlog	log = {0};
@@ -1103,22 +1102,19 @@ _("Warning:  project quota information would be cleared.\n"
 	if (!sbp)
 		do_error(_("couldn't get superblock\n"));
 
-	dsb = sbp->b_addr;
-
 	if ((mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD) != quotacheck_results()) {
 		do_warn(_("Note - quota info will be regenerated on next "
 			"quota mount.\n"));
-		dsb->sb_qflags &= cpu_to_be16(~(XFS_UQUOTA_CHKD |
-						XFS_GQUOTA_CHKD |
-						XFS_PQUOTA_CHKD |
-						XFS_OQUOTA_CHKD));
+		mp->m_sb.sb_qflags &= ~(XFS_UQUOTA_CHKD | XFS_GQUOTA_CHKD |
+					XFS_PQUOTA_CHKD | XFS_OQUOTA_CHKD);
+		libxfs_sb_to_disk(sbp->b_addr, &mp->m_sb);
 	}
 
 	if (copied_sunit) {
 		do_warn(
 _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\n"
   "Please reset with mount -o sunit=<value>,swidth=<value> if necessary\n"),
-			be32_to_cpu(dsb->sb_unit), be32_to_cpu(dsb->sb_width));
+			mp->m_sb.sb_unit, mp->m_sb.sb_width);
 	}
 
 	libxfs_buf_mark_dirty(sbp);

Re: [PATCH 6/7] xfs_repair: clear quota CHKD flags on the incore superblock too

[Allison Henderson]

On 2/22/21 8:00 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> At the end of a repair run, xfs_repair clears the superblock's quota
> checked flags if it found mistakes in the quota accounting to force a
> quotacheck at the next mount.  This is currently the last time repair
> modifies the primary superblock, so it is sufficient to update the
> ondisk buffer and not the incore mount structure.
> 
> However, we're about to introduce code to clear the needsrepair feature
> at the very end of repair, after all metadata blocks have been written
> to disk and all disk caches flush.  Since the convention everywhere else
> in xfs is to update the incore superblock, call libxfs_sb_to_disk to
> translate that into the ondisk buffer, and then write the buffer to
> disk, switch the quota CHKD code to use this mechanism too.
> 
> (Get rid of dsb too, since the incore super should be in sync with the
> ondisk super.)
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>   repair/xfs_repair.c |   12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 9409f0d8..40352458 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -717,7 +717,6 @@ main(int argc, char **argv)
>   {
>   	xfs_mount_t	*temp_mp;
>   	xfs_mount_t	*mp;
> -	xfs_dsb_t	*dsb;
>   	struct xfs_buf	*sbp;
>   	xfs_mount_t	xfs_m;
>   	struct xlog	log = {0};
> @@ -1103,22 +1102,19 @@ _("Warning:  project quota information would be cleared.\n"
>   	if (!sbp)
>   		do_error(_("couldn't get superblock\n"));
>   
> -	dsb = sbp->b_addr;
> -
>   	if ((mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD) != quotacheck_results()) {
>   		do_warn(_("Note - quota info will be regenerated on next "
>   			"quota mount.\n"));
> -		dsb->sb_qflags &= cpu_to_be16(~(XFS_UQUOTA_CHKD |
> -						XFS_GQUOTA_CHKD |
> -						XFS_PQUOTA_CHKD |
> -						XFS_OQUOTA_CHKD));
> +		mp->m_sb.sb_qflags &= ~(XFS_UQUOTA_CHKD | XFS_GQUOTA_CHKD |
> +					XFS_PQUOTA_CHKD | XFS_OQUOTA_CHKD);
> +		libxfs_sb_to_disk(sbp->b_addr, &mp->m_sb);
>   	}
>   
>   	if (copied_sunit) {
>   		do_warn(
>   _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\n"
>     "Please reset with mount -o sunit=<value>,swidth=<value> if necessary\n"),
> -			be32_to_cpu(dsb->sb_unit), be32_to_cpu(dsb->sb_width));
> +			mp->m_sb.sb_unit, mp->m_sb.sb_width);
>   	}
>   
>   	libxfs_buf_mark_dirty(sbp);
> 

[PATCH 7/7] 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 include/xfs_mount.h |    1 +
 libxfs/init.c       |   20 +++++++++++++-------
 repair/agheader.c   |   21 +++++++++++++++++++++
 repair/xfs_repair.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 77 insertions(+), 7 deletions(-)


diff --git a/include/xfs_mount.h b/include/xfs_mount.h
index 36594643..75230ca5 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);
 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..8a8ce3c4 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -870,7 +870,7 @@ _("%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)
 {
@@ -878,13 +878,13 @@ libxfs_flush_mount(
 	int			err2;
 
 	/*
-	 * Purge the buffer cache to write all dirty buffers to disk and free
-	 * all incore buffers.  Buffers that fail write verification will cause
-	 * the CORRUPT_WRITE flag to be set in the buftarg.  Buffers that
-	 * cannot be written will cause the LOST_WRITE flag to be set in the
-	 * buftarg.
+	 * Flush the buffer cache to write all dirty buffers to disk.  Buffers
+	 * that fail write verification will cause the CORRUPT_WRITE flag to be
+	 * set in the buftarg.  Buffers that cannot be written will cause the
+	 * LOST_WRITE flag to be set in the buftarg.  Once that's done,
+	 * instruct the disks to persist their write caches.
 	 */
-	libxfs_bcache_purge();
+	libxfs_bcache_flush();
 
 	/* Flush all kernel and disk write caches, and report failures. */
 	if (mp->m_ddev_targp) {
@@ -923,6 +923,12 @@ libxfs_umount(
 
 	libxfs_rtmount_destroy(mp);
 
+	/*
+	 * Purge the buffer cache to write all dirty buffers to disk and free
+	 * all incore buffers, then pick up the outcome when we tell the disks
+	 * to persist their write caches.
+	 */
+	libxfs_bcache_purge();
 	error = libxfs_flush_mount(mp);
 
 	for (agno = 0; agno < mp->m_maxagi; 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 40352458..e2e99b21 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -712,6 +712,45 @@ 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.  Make sure any
+	 * dirty buffers are sent to disk and that the disks have persisted
+	 * writes to stable storage.  If that fails, leave NEEDSREPAIR in
+	 * place.
+	 */
+	error = -libxfs_flush_mount(mp);
+	if (error) {
+		do_warn(
+	_("Cannot clear needsrepair due to flush failure, err=%d.\n"),
+			error);
+		return;
+	}
+
+	/* Clear needsrepair from the superblock. */
+	bp = libxfs_getsb(mp);
+	if (!bp || bp->b_error) {
+		do_warn(
+	_("Cannot clear needsrepair from primary super, err=%d.\n"),
+			bp ? bp->b_error : ENOMEM);
+	} 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);
+	}
+	if (bp)
+		libxfs_buf_relse(bp);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -1128,6 +1167,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 7/7] xfs_repair: clear the needsrepair flag

[Allison Henderson]

On 2/22/21 8:00 PM, 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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>   include/xfs_mount.h |    1 +
>   libxfs/init.c       |   20 +++++++++++++-------
>   repair/agheader.c   |   21 +++++++++++++++++++++
>   repair/xfs_repair.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 77 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> index 36594643..75230ca5 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);
>   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..8a8ce3c4 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -870,7 +870,7 @@ _("%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)
>   {
> @@ -878,13 +878,13 @@ libxfs_flush_mount(
>   	int			err2;
>   
>   	/*
> -	 * Purge the buffer cache to write all dirty buffers to disk and free
> -	 * all incore buffers.  Buffers that fail write verification will cause
> -	 * the CORRUPT_WRITE flag to be set in the buftarg.  Buffers that
> -	 * cannot be written will cause the LOST_WRITE flag to be set in the
> -	 * buftarg.
> +	 * Flush the buffer cache to write all dirty buffers to disk.  Buffers
> +	 * that fail write verification will cause the CORRUPT_WRITE flag to be
> +	 * set in the buftarg.  Buffers that cannot be written will cause the
> +	 * LOST_WRITE flag to be set in the buftarg.  Once that's done,
> +	 * instruct the disks to persist their write caches.
>   	 */
> -	libxfs_bcache_purge();
> +	libxfs_bcache_flush();
>   
>   	/* Flush all kernel and disk write caches, and report failures. */
>   	if (mp->m_ddev_targp) {
> @@ -923,6 +923,12 @@ libxfs_umount(
>   
>   	libxfs_rtmount_destroy(mp);
>   
> +	/*
> +	 * Purge the buffer cache to write all dirty buffers to disk and free
> +	 * all incore buffers, then pick up the outcome when we tell the disks
> +	 * to persist their write caches.
> +	 */
> +	libxfs_bcache_purge();
>   	error = libxfs_flush_mount(mp);
>   
>   	for (agno = 0; agno < mp->m_maxagi; 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 40352458..e2e99b21 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -712,6 +712,45 @@ 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.  Make sure any
> +	 * dirty buffers are sent to disk and that the disks have persisted
> +	 * writes to stable storage.  If that fails, leave NEEDSREPAIR in
> +	 * place.
> +	 */
> +	error = -libxfs_flush_mount(mp);
> +	if (error) {
> +		do_warn(
> +	_("Cannot clear needsrepair due to flush failure, err=%d.\n"),
> +			error);
> +		return;
> +	}
> +
> +	/* Clear needsrepair from the superblock. */
> +	bp = libxfs_getsb(mp);
> +	if (!bp || bp->b_error) {
> +		do_warn(
> +	_("Cannot clear needsrepair from primary super, err=%d.\n"),
> +			bp ? bp->b_error : ENOMEM);
> +	} 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);
> +	}
> +	if (bp)
> +		libxfs_buf_relse(bp);
> +}
> +
>   int
>   main(int argc, char **argv)
>   {
> @@ -1128,6 +1167,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)
>