[PATCHSET v4 00/10] xfs: add the ability to flag a fs for repair

[PATCHSET v4 00/10] 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
v4: move all the upgrader code to xfs_repair per Eric suggestion

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      |   15 ++++---
 include/xfs_mount.h  |    1 
 libxfs/init.c        |   25 ++++++------
 man/man8/xfs_admin.8 |   30 ++++++++++++++
 repair/agheader.c    |   21 ++++++++++
 repair/agheader.h    |    2 +
 repair/dir2.c        |    3 +
 repair/globals.c     |    3 +
 repair/globals.h     |    4 ++
 repair/phase1.c      |   28 +++++++++++++
 repair/phase6.c      |    7 +++
 repair/xfs_repair.c  |  107 ++++++++++++++++++++++++++++++++++++++++++++++++--
 14 files changed, 241 insertions(+), 23 deletions(-)

[PATCH 01/10] 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>
---
 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 01/10] xfs_admin: clean up string quoting

[Christoph Hellwig]

Looks good,

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

[PATCH 02/10] 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>
---
 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 02/10] xfs_admin: support filesystems with realtime devices

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:10:15PM -0800, 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.

Looks good,

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

Re: [PATCH 02/10] xfs_admin: support filesystems with realtime devices

[Brian Foster]

On Mon, Feb 08, 2021 at 08:10:15PM -0800, 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: Brian Foster <bfoster@redhat.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 03/10] 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    |   13 +++++++++++++
 2 files changed, 18 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..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) {
@@ -691,6 +702,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 03/10] xfs_db: support the needsrepair feature flag in the version command

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:10:21PM -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.

In the "version" command?

Re: [PATCH 03/10] xfs_db: support the needsrepair feature flag in the version command

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 09:09:40AM +0000, Christoph Hellwig wrote:
> On Mon, Feb 08, 2021 at 08:10:21PM -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.
> 
> In the "version" command?

Urk.  Yeah, this patch is a bit incoherent.  Two of the hunks merely
report the presence of needsrepair in check/version; and the middle two
are used to prevent the administrative changes that we allow via
xfs_admin (aka label/uuid) on a needsrepair fs.

Will split them up and repost with better commit messages.

Thanks for the review!

--D

Re: [PATCH 03/10] xfs_db: support the needsrepair feature flag in the version command

[Brian Foster]

On Mon, Feb 08, 2021 at 08:10:21PM -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>
> ---

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

>  db/check.c |    5 +++++
>  db/sb.c    |   13 +++++++++++++
>  2 files changed, 18 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..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) {
> @@ -691,6 +702,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;
>  }
>  
> 

[PATCH 04/10] 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);

Re: [PATCH 04/10] xfs_repair: fix unmount error message to have a newline

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:10:26PM -0800, 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>

Looks good,

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

[PATCH 05/10] 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.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/xfs_repair.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)


diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 9409f0d8..32755821 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -1108,10 +1108,9 @@ _("Warning:  project quota information would be cleared.\n"
 	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) {

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

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:10:32PM -0800, 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.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

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

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

[Brian Foster]

On Mon, Feb 08, 2021 at 08:10:32PM -0800, 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.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/xfs_repair.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 9409f0d8..32755821 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -1108,10 +1108,9 @@ _("Warning:  project quota information would be cleared.\n"
>  	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);

Nit: we can still use dsb here instead of open-coding the buffer
address. Otherwise:

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

>  	}
>  
>  	if (copied_sunit) {
> 

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

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 12:20:11PM -0500, Brian Foster wrote:
> On Mon, Feb 08, 2021 at 08:10:32PM -0800, 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.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  repair/xfs_repair.c |    7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 9409f0d8..32755821 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -1108,10 +1108,9 @@ _("Warning:  project quota information would be cleared.\n"
> >  	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);
> 
> Nit: we can still use dsb here instead of open-coding the buffer
> address. Otherwise:

Or I could remove dsb entirely, since the xfs_mount's sb_unit/sb_width
should reflect the values copied from the backup super during phase 1
(aka before we actually libxfs_mount) when we set copied_sunit=1.

--D

> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> >  	}
> >  
> >  	if (copied_sunit) {
> > 
> 

[PATCH 06/10] 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       |   25 +++++++++++++------------
 repair/agheader.c   |   21 +++++++++++++++++++++
 repair/xfs_repair.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 80 insertions(+), 12 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..98057b78 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -867,25 +867,17 @@ _("%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.
+ * Persist all disk write caches and report on writes that didn't make it to
+ * stable storage.  Callers should flush (or purge) the libxfs buffer caches
+ * before calling this function.
  */
-static int
+int
 libxfs_flush_mount(
 	struct xfs_mount	*mp)
 {
 	int			error = 0;
 	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.
-	 */
-	libxfs_bcache_purge();
-
 	/* Flush all kernel and disk write caches, and report failures. */
 	if (mp->m_ddev_targp) {
 		err2 = libxfs_flush_buftarg(mp->m_ddev_targp, _("data device"));
@@ -923,6 +915,15 @@ libxfs_umount(
 
 	libxfs_rtmount_destroy(mp);
 
+	/*
+	 * 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.  Once that's done, instruct 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 32755821..f607afcb 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -712,6 +712,48 @@ 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.
+	 */
+	libxfs_bcache_flush();
+	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)
 {
@@ -1131,6 +1173,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 06/10] xfs_repair: clear the needsrepair flag

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:10:38PM -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 good,

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

Re: [PATCH 06/10] xfs_repair: clear the needsrepair flag

[Brian Foster]

On Mon, Feb 08, 2021 at 08:10:38PM -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>
> ---
>  include/xfs_mount.h |    1 +
>  libxfs/init.c       |   25 +++++++++++++------------
>  repair/agheader.c   |   21 +++++++++++++++++++++
>  repair/xfs_repair.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 80 insertions(+), 12 deletions(-)
> 
> 
...
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 9fe13b8d..98057b78 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -867,25 +867,17 @@ _("%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.
> + * Persist all disk write caches and report on writes that didn't make it to
> + * stable storage.  Callers should flush (or purge) the libxfs buffer caches
> + * before calling this function.
>   */
> -static int
> +int
>  libxfs_flush_mount(
>  	struct xfs_mount	*mp)
>  {
>  	int			error = 0;
>  	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.
> -	 */
> -	libxfs_bcache_purge();
> -

FWIW, my comment on the previous version was that I think it's
reasonable to call libxfs_bcache_flush() here instead of the purge so
callers don't necessarily have to do anything special. The one caller
that does the purge is free to do so before calling
libxfs_flush_mount(), as that essentially supercedes the flush that
would otherwise occur here. Either way, the patch looks fine to me:

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

>  	/* Flush all kernel and disk write caches, and report failures. */
>  	if (mp->m_ddev_targp) {
>  		err2 = libxfs_flush_buftarg(mp->m_ddev_targp, _("data device"));
> @@ -923,6 +915,15 @@ libxfs_umount(
>  
>  	libxfs_rtmount_destroy(mp);
>  
> +	/*
> +	 * 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.  Once that's done, instruct 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 32755821..f607afcb 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -712,6 +712,48 @@ 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.
> +	 */
> +	libxfs_bcache_flush();
> +	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)
>  {
> @@ -1131,6 +1173,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 06/10] xfs_repair: clear the needsrepair flag

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 12:20:29PM -0500, Brian Foster wrote:
> On Mon, Feb 08, 2021 at 08:10:38PM -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>
> > ---
> >  include/xfs_mount.h |    1 +
> >  libxfs/init.c       |   25 +++++++++++++------------
> >  repair/agheader.c   |   21 +++++++++++++++++++++
> >  repair/xfs_repair.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 80 insertions(+), 12 deletions(-)
> > 
> > 
> ...
> > diff --git a/libxfs/init.c b/libxfs/init.c
> > index 9fe13b8d..98057b78 100644
> > --- a/libxfs/init.c
> > +++ b/libxfs/init.c
> > @@ -867,25 +867,17 @@ _("%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.
> > + * Persist all disk write caches and report on writes that didn't make it to
> > + * stable storage.  Callers should flush (or purge) the libxfs buffer caches
> > + * before calling this function.
> >   */
> > -static int
> > +int
> >  libxfs_flush_mount(
> >  	struct xfs_mount	*mp)
> >  {
> >  	int			error = 0;
> >  	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.
> > -	 */
> > -	libxfs_bcache_purge();
> > -
> 
> FWIW, my comment on the previous version was that I think it's
> reasonable to call libxfs_bcache_flush() here instead of the purge so
> callers don't necessarily have to do anything special. The one caller
> that does the purge is free to do so before calling
> libxfs_flush_mount(), as that essentially supercedes the flush that
> would otherwise occur here. Either way, the patch looks fine to me:

Ah, yeah.  That's a pretty simple change to make.  Thanks for the
review.

--D

> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> >  	/* Flush all kernel and disk write caches, and report failures. */
> >  	if (mp->m_ddev_targp) {
> >  		err2 = libxfs_flush_buftarg(mp->m_ddev_targp, _("data device"));
> > @@ -923,6 +915,15 @@ libxfs_umount(
> >  
> >  	libxfs_rtmount_destroy(mp);
> >  
> > +	/*
> > +	 * 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.  Once that's done, instruct 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 32755821..f607afcb 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -712,6 +712,48 @@ 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.
> > +	 */
> > +	libxfs_bcache_flush();
> > +	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)
> >  {
> > @@ -1131,6 +1173,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)
> > 
> 

[PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories

[Darrick J. Wong]

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

There are a few places in xfs_repair's directory checking code where we
deliberately corrupt a directory entry as a sentinel to trigger a
correction in later repair phase.  In the mean time, the filesystem is
inconsistent, so set the needsrepair flag to force a re-run of repair if
the system goes down.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/agheader.h   |    2 ++
 repair/dir2.c       |    3 +++
 repair/phase6.c     |    7 +++++++
 repair/xfs_repair.c |   37 +++++++++++++++++++++++++++++++++++++
 4 files changed, 49 insertions(+)


diff --git a/repair/agheader.h b/repair/agheader.h
index a63827c8..fa6fe596 100644
--- a/repair/agheader.h
+++ b/repair/agheader.h
@@ -82,3 +82,5 @@ typedef struct fs_geo_list  {
 #define XR_AG_AGF	0x2
 #define XR_AG_AGI	0x4
 #define XR_AG_SB_SEC	0x8
+
+void force_needsrepair(struct xfs_mount *mp);
diff --git a/repair/dir2.c b/repair/dir2.c
index eabdb4f2..922b8a3e 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -15,6 +15,7 @@
 #include "da_util.h"
 #include "prefetch.h"
 #include "progress.h"
+#include "agheader.h"
 
 /*
  * Known bad inode list.  These are seen when the leaf and node
@@ -774,6 +775,7 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
 				do_warn(
 _("\tclearing inode number in entry at offset %" PRIdPTR "...\n"),
 					(intptr_t)ptr - (intptr_t)d);
+				force_needsrepair(mp);
 				dep->name[0] = '/';
 				*dirty = 1;
 			} else {
@@ -914,6 +916,7 @@ _("entry \"%*.*s\" in directory inode %" PRIu64 " points to self: "),
 		 */
 		if (junkit) {
 			if (!no_modify) {
+				force_needsrepair(mp);
 				dep->name[0] = '/';
 				*dirty = 1;
 				do_warn(_("clearing entry\n"));
diff --git a/repair/phase6.c b/repair/phase6.c
index 14464bef..5ecbe9b2 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1649,6 +1649,7 @@ longform_dir2_entry_check_data(
 			if (entry_junked(
 	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ""),
 					fname, ip->i_ino, inum)) {
+				force_needsrepair(mp);
 				dep->name[0] = '/';
 				libxfs_dir2_data_log_entry(&da, bp, dep);
 			}
@@ -1666,6 +1667,7 @@ longform_dir2_entry_check_data(
 			if (entry_junked(
 	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64),
 					fname, ip->i_ino, inum)) {
+				force_needsrepair(mp);
 				dep->name[0] = '/';
 				libxfs_dir2_data_log_entry(&da, bp, dep);
 			}
@@ -1684,6 +1686,7 @@ longform_dir2_entry_check_data(
 				if (entry_junked(
 	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory"),
 						ORPHANAGE, inum, ip->i_ino)) {
+					force_needsrepair(mp);
 					dep->name[0] = '/';
 					libxfs_dir2_data_log_entry(&da, bp, dep);
 				}
@@ -1706,6 +1709,7 @@ longform_dir2_entry_check_data(
 			if (entry_junked(
 	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
 					fname, inum, ip->i_ino)) {
+				force_needsrepair(mp);
 				dep->name[0] = '/';
 				libxfs_dir2_data_log_entry(&da, bp, dep);
 			}
@@ -1737,6 +1741,7 @@ longform_dir2_entry_check_data(
 				if (entry_junked(
 	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block"), fname,
 						inum, ip->i_ino)) {
+					force_needsrepair(mp);
 					dep->name[0] = '/';
 					libxfs_dir2_data_log_entry(&da, bp, dep);
 				}
@@ -1764,6 +1769,7 @@ longform_dir2_entry_check_data(
 				if (entry_junked(
 	_("entry \"%s\" in dir %" PRIu64 " is not the first entry"),
 						fname, inum, ip->i_ino)) {
+					force_needsrepair(mp);
 					dep->name[0] = '/';
 					libxfs_dir2_data_log_entry(&da, bp, dep);
 				}
@@ -1852,6 +1858,7 @@ _("entry \"%s\" in dir inode %" PRIu64 " inconsistent with .. value (%" PRIu64 "
 				orphanage_ino = 0;
 			nbad++;
 			if (!no_modify)  {
+				force_needsrepair(mp);
 				dep->name[0] = '/';
 				libxfs_dir2_data_log_entry(&da, bp, dep);
 				if (verbose)
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index f607afcb..9dc73854 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -754,6 +754,43 @@ clear_needsrepair(
 		libxfs_buf_relse(bp);
 }
 
+/*
+ * Mark the filesystem as needing repair.  This should only be called by code
+ * that deliberately sets invalid sentinel values in the on-disk metadata to
+ * trigger a later reconstruction, and only after we've settled the primary
+ * super contents (i.e. after phase 1).
+ */
+void
+force_needsrepair(
+	struct xfs_mount	*mp)
+{
+	struct xfs_buf		*bp;
+	int			error;
+
+	if (!xfs_sb_version_hascrc(&mp->m_sb) ||
+	    xfs_sb_version_needsrepair(&mp->m_sb))
+		return;
+
+	bp = libxfs_getsb(mp);
+	if (!bp || bp->b_error) {
+		do_log(
+	_("couldn't get superblock to set needsrepair, err=%d\n"),
+				bp ? bp->b_error : ENOMEM);
+		return;
+	} else {
+		mp->m_sb.sb_features_incompat |=
+				XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+		libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
+
+		/* Force the primary super to disk immediately. */
+		error = -libxfs_bwrite(bp);
+		if (error)
+			do_log(_("couldn't force needsrepair, err=%d\n"), error);
+	}
+	if (bp)
+		libxfs_buf_relse(bp);
+}
+
 int
 main(int argc, char **argv)
 {

Re: [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:10:44PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> There are a few places in xfs_repair's directory checking code where we
> deliberately corrupt a directory entry as a sentinel to trigger a
> correction in later repair phase.  In the mean time, the filesystem is
> inconsistent, so set the needsrepair flag to force a re-run of repair if
> the system goes down.

I guess this is an improvement over what we have no, but I suspect an
in-core side band way to notify the later phases would be much better
than these corrupt sentinel values..

Re: [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 09:13:36AM +0000, Christoph Hellwig wrote:
> On Mon, Feb 08, 2021 at 08:10:44PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > There are a few places in xfs_repair's directory checking code where we
> > deliberately corrupt a directory entry as a sentinel to trigger a
> > correction in later repair phase.  In the mean time, the filesystem is
> > inconsistent, so set the needsrepair flag to force a re-run of repair if
> > the system goes down.
> 
> I guess this is an improvement over what we have no, but I suspect an
> in-core side band way to notify the later phases would be much better
> than these corrupt sentinel values..

It would be, but we'd still have to track which directory entries are
thee ones that we don't want to preserve.  We could rewrite the
directory entry to point to a plausible inode number that isn't
allocated, but that runs the risk that someone will come along and
allocate that inode for lost+found and then we're really in a mess.

Alternately, I suppose we could rewrite the directory entry to be a
DHT_WHITEOUT entry that doesn't look like any that the kernel would
produce ... insofar as I'm not even sure what the real ones were
supposed to look like, given the weird left turn that overlayfs took on
that. :(

--D

Re: [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories

[Brian Foster]

On Mon, Feb 08, 2021 at 08:10:44PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> There are a few places in xfs_repair's directory checking code where we
> deliberately corrupt a directory entry as a sentinel to trigger a
> correction in later repair phase.  In the mean time, the filesystem is
> inconsistent, so set the needsrepair flag to force a re-run of repair if
> the system goes down.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Hmm.. this seems orthogonal to the rest of the series. I'm sure we can
come up with various additional uses for the bit, but it seems a little
odd to me that repair might set it in some cases after a crash but not
others (if the filesystem happens to already be corrupt, for example).

Brian

>  repair/agheader.h   |    2 ++
>  repair/dir2.c       |    3 +++
>  repair/phase6.c     |    7 +++++++
>  repair/xfs_repair.c |   37 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 49 insertions(+)
> 
> 
> diff --git a/repair/agheader.h b/repair/agheader.h
> index a63827c8..fa6fe596 100644
> --- a/repair/agheader.h
> +++ b/repair/agheader.h
> @@ -82,3 +82,5 @@ typedef struct fs_geo_list  {
>  #define XR_AG_AGF	0x2
>  #define XR_AG_AGI	0x4
>  #define XR_AG_SB_SEC	0x8
> +
> +void force_needsrepair(struct xfs_mount *mp);
> diff --git a/repair/dir2.c b/repair/dir2.c
> index eabdb4f2..922b8a3e 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -15,6 +15,7 @@
>  #include "da_util.h"
>  #include "prefetch.h"
>  #include "progress.h"
> +#include "agheader.h"
>  
>  /*
>   * Known bad inode list.  These are seen when the leaf and node
> @@ -774,6 +775,7 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
>  				do_warn(
>  _("\tclearing inode number in entry at offset %" PRIdPTR "...\n"),
>  					(intptr_t)ptr - (intptr_t)d);
> +				force_needsrepair(mp);
>  				dep->name[0] = '/';
>  				*dirty = 1;
>  			} else {
> @@ -914,6 +916,7 @@ _("entry \"%*.*s\" in directory inode %" PRIu64 " points to self: "),
>  		 */
>  		if (junkit) {
>  			if (!no_modify) {
> +				force_needsrepair(mp);
>  				dep->name[0] = '/';
>  				*dirty = 1;
>  				do_warn(_("clearing entry\n"));
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 14464bef..5ecbe9b2 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1649,6 +1649,7 @@ longform_dir2_entry_check_data(
>  			if (entry_junked(
>  	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ""),
>  					fname, ip->i_ino, inum)) {
> +				force_needsrepair(mp);
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
>  			}
> @@ -1666,6 +1667,7 @@ longform_dir2_entry_check_data(
>  			if (entry_junked(
>  	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64),
>  					fname, ip->i_ino, inum)) {
> +				force_needsrepair(mp);
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
>  			}
> @@ -1684,6 +1686,7 @@ longform_dir2_entry_check_data(
>  				if (entry_junked(
>  	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory"),
>  						ORPHANAGE, inum, ip->i_ino)) {
> +					force_needsrepair(mp);
>  					dep->name[0] = '/';
>  					libxfs_dir2_data_log_entry(&da, bp, dep);
>  				}
> @@ -1706,6 +1709,7 @@ longform_dir2_entry_check_data(
>  			if (entry_junked(
>  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
>  					fname, inum, ip->i_ino)) {
> +				force_needsrepair(mp);
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
>  			}
> @@ -1737,6 +1741,7 @@ longform_dir2_entry_check_data(
>  				if (entry_junked(
>  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block"), fname,
>  						inum, ip->i_ino)) {
> +					force_needsrepair(mp);
>  					dep->name[0] = '/';
>  					libxfs_dir2_data_log_entry(&da, bp, dep);
>  				}
> @@ -1764,6 +1769,7 @@ longform_dir2_entry_check_data(
>  				if (entry_junked(
>  	_("entry \"%s\" in dir %" PRIu64 " is not the first entry"),
>  						fname, inum, ip->i_ino)) {
> +					force_needsrepair(mp);
>  					dep->name[0] = '/';
>  					libxfs_dir2_data_log_entry(&da, bp, dep);
>  				}
> @@ -1852,6 +1858,7 @@ _("entry \"%s\" in dir inode %" PRIu64 " inconsistent with .. value (%" PRIu64 "
>  				orphanage_ino = 0;
>  			nbad++;
>  			if (!no_modify)  {
> +				force_needsrepair(mp);
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
>  				if (verbose)
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index f607afcb..9dc73854 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -754,6 +754,43 @@ clear_needsrepair(
>  		libxfs_buf_relse(bp);
>  }
>  
> +/*
> + * Mark the filesystem as needing repair.  This should only be called by code
> + * that deliberately sets invalid sentinel values in the on-disk metadata to
> + * trigger a later reconstruction, and only after we've settled the primary
> + * super contents (i.e. after phase 1).
> + */
> +void
> +force_needsrepair(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_buf		*bp;
> +	int			error;
> +
> +	if (!xfs_sb_version_hascrc(&mp->m_sb) ||
> +	    xfs_sb_version_needsrepair(&mp->m_sb))
> +		return;
> +
> +	bp = libxfs_getsb(mp);
> +	if (!bp || bp->b_error) {
> +		do_log(
> +	_("couldn't get superblock to set needsrepair, err=%d\n"),
> +				bp ? bp->b_error : ENOMEM);
> +		return;
> +	} else {
> +		mp->m_sb.sb_features_incompat |=
> +				XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> +		libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> +
> +		/* Force the primary super to disk immediately. */
> +		error = -libxfs_bwrite(bp);
> +		if (error)
> +			do_log(_("couldn't force needsrepair, err=%d\n"), error);
> +	}
> +	if (bp)
> +		libxfs_buf_relse(bp);
> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> 

Re: [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 12:20:59PM -0500, Brian Foster wrote:
> On Mon, Feb 08, 2021 at 08:10:44PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > There are a few places in xfs_repair's directory checking code where we
> > deliberately corrupt a directory entry as a sentinel to trigger a
> > correction in later repair phase.  In the mean time, the filesystem is
> > inconsistent, so set the needsrepair flag to force a re-run of repair if
> > the system goes down.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> Hmm.. this seems orthogonal to the rest of the series. I'm sure we can
> come up with various additional uses for the bit, but it seems a little
> odd to me that repair might set it in some cases after a crash but not
> others (if the filesystem happens to already be corrupt, for example).

<nod> Another option I thought of is to add a hook to the buffer cache
so that the first time anyone tries to bwrite a buffer (either directly
or via a delwri list or normal buffer cache writeback) we'll also set
needsrepair on the ondisk primary super.  That would protect us against
other scenarios like crashing after writing a new AGF but before writing
the new AGI, where the fs is left in an indeterminate state.

Hmm, maybe I should pursue /that/ instead.

--D

> Brian
> 
> >  repair/agheader.h   |    2 ++
> >  repair/dir2.c       |    3 +++
> >  repair/phase6.c     |    7 +++++++
> >  repair/xfs_repair.c |   37 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 49 insertions(+)
> > 
> > 
> > diff --git a/repair/agheader.h b/repair/agheader.h
> > index a63827c8..fa6fe596 100644
> > --- a/repair/agheader.h
> > +++ b/repair/agheader.h
> > @@ -82,3 +82,5 @@ typedef struct fs_geo_list  {
> >  #define XR_AG_AGF	0x2
> >  #define XR_AG_AGI	0x4
> >  #define XR_AG_SB_SEC	0x8
> > +
> > +void force_needsrepair(struct xfs_mount *mp);
> > diff --git a/repair/dir2.c b/repair/dir2.c
> > index eabdb4f2..922b8a3e 100644
> > --- a/repair/dir2.c
> > +++ b/repair/dir2.c
> > @@ -15,6 +15,7 @@
> >  #include "da_util.h"
> >  #include "prefetch.h"
> >  #include "progress.h"
> > +#include "agheader.h"
> >  
> >  /*
> >   * Known bad inode list.  These are seen when the leaf and node
> > @@ -774,6 +775,7 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
> >  				do_warn(
> >  _("\tclearing inode number in entry at offset %" PRIdPTR "...\n"),
> >  					(intptr_t)ptr - (intptr_t)d);
> > +				force_needsrepair(mp);
> >  				dep->name[0] = '/';
> >  				*dirty = 1;
> >  			} else {
> > @@ -914,6 +916,7 @@ _("entry \"%*.*s\" in directory inode %" PRIu64 " points to self: "),
> >  		 */
> >  		if (junkit) {
> >  			if (!no_modify) {
> > +				force_needsrepair(mp);
> >  				dep->name[0] = '/';
> >  				*dirty = 1;
> >  				do_warn(_("clearing entry\n"));
> > diff --git a/repair/phase6.c b/repair/phase6.c
> > index 14464bef..5ecbe9b2 100644
> > --- a/repair/phase6.c
> > +++ b/repair/phase6.c
> > @@ -1649,6 +1649,7 @@ longform_dir2_entry_check_data(
> >  			if (entry_junked(
> >  	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ""),
> >  					fname, ip->i_ino, inum)) {
> > +				force_needsrepair(mp);
> >  				dep->name[0] = '/';
> >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> >  			}
> > @@ -1666,6 +1667,7 @@ longform_dir2_entry_check_data(
> >  			if (entry_junked(
> >  	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64),
> >  					fname, ip->i_ino, inum)) {
> > +				force_needsrepair(mp);
> >  				dep->name[0] = '/';
> >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> >  			}
> > @@ -1684,6 +1686,7 @@ longform_dir2_entry_check_data(
> >  				if (entry_junked(
> >  	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory"),
> >  						ORPHANAGE, inum, ip->i_ino)) {
> > +					force_needsrepair(mp);
> >  					dep->name[0] = '/';
> >  					libxfs_dir2_data_log_entry(&da, bp, dep);
> >  				}
> > @@ -1706,6 +1709,7 @@ longform_dir2_entry_check_data(
> >  			if (entry_junked(
> >  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
> >  					fname, inum, ip->i_ino)) {
> > +				force_needsrepair(mp);
> >  				dep->name[0] = '/';
> >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> >  			}
> > @@ -1737,6 +1741,7 @@ longform_dir2_entry_check_data(
> >  				if (entry_junked(
> >  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block"), fname,
> >  						inum, ip->i_ino)) {
> > +					force_needsrepair(mp);
> >  					dep->name[0] = '/';
> >  					libxfs_dir2_data_log_entry(&da, bp, dep);
> >  				}
> > @@ -1764,6 +1769,7 @@ longform_dir2_entry_check_data(
> >  				if (entry_junked(
> >  	_("entry \"%s\" in dir %" PRIu64 " is not the first entry"),
> >  						fname, inum, ip->i_ino)) {
> > +					force_needsrepair(mp);
> >  					dep->name[0] = '/';
> >  					libxfs_dir2_data_log_entry(&da, bp, dep);
> >  				}
> > @@ -1852,6 +1858,7 @@ _("entry \"%s\" in dir inode %" PRIu64 " inconsistent with .. value (%" PRIu64 "
> >  				orphanage_ino = 0;
> >  			nbad++;
> >  			if (!no_modify)  {
> > +				force_needsrepair(mp);
> >  				dep->name[0] = '/';
> >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> >  				if (verbose)
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index f607afcb..9dc73854 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -754,6 +754,43 @@ clear_needsrepair(
> >  		libxfs_buf_relse(bp);
> >  }
> >  
> > +/*
> > + * Mark the filesystem as needing repair.  This should only be called by code
> > + * that deliberately sets invalid sentinel values in the on-disk metadata to
> > + * trigger a later reconstruction, and only after we've settled the primary
> > + * super contents (i.e. after phase 1).
> > + */
> > +void
> > +force_needsrepair(
> > +	struct xfs_mount	*mp)
> > +{
> > +	struct xfs_buf		*bp;
> > +	int			error;
> > +
> > +	if (!xfs_sb_version_hascrc(&mp->m_sb) ||
> > +	    xfs_sb_version_needsrepair(&mp->m_sb))
> > +		return;
> > +
> > +	bp = libxfs_getsb(mp);
> > +	if (!bp || bp->b_error) {
> > +		do_log(
> > +	_("couldn't get superblock to set needsrepair, err=%d\n"),
> > +				bp ? bp->b_error : ENOMEM);
> > +		return;
> > +	} else {
> > +		mp->m_sb.sb_features_incompat |=
> > +				XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > +		libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > +
> > +		/* Force the primary super to disk immediately. */
> > +		error = -libxfs_bwrite(bp);
> > +		if (error)
> > +			do_log(_("couldn't force needsrepair, err=%d\n"), error);
> > +	}
> > +	if (bp)
> > +		libxfs_buf_relse(bp);
> > +}
> > +
> >  int
> >  main(int argc, char **argv)
> >  {
> > 
> 

Re: [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories

[Brian Foster]

On Tue, Feb 09, 2021 at 10:35:42AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 09, 2021 at 12:20:59PM -0500, Brian Foster wrote:
> > On Mon, Feb 08, 2021 at 08:10:44PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > There are a few places in xfs_repair's directory checking code where we
> > > deliberately corrupt a directory entry as a sentinel to trigger a
> > > correction in later repair phase.  In the mean time, the filesystem is
> > > inconsistent, so set the needsrepair flag to force a re-run of repair if
> > > the system goes down.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > 
> > Hmm.. this seems orthogonal to the rest of the series. I'm sure we can
> > come up with various additional uses for the bit, but it seems a little
> > odd to me that repair might set it in some cases after a crash but not
> > others (if the filesystem happens to already be corrupt, for example).
> 
> <nod> Another option I thought of is to add a hook to the buffer cache
> so that the first time anyone tries to bwrite a buffer (either directly
> or via a delwri list or normal buffer cache writeback) we'll also set
> needsrepair on the ondisk primary super.  That would protect us against
> other scenarios like crashing after writing a new AGF but before writing
> the new AGI, where the fs is left in an indeterminate state.
> 

Yeah, that _seems_ more appropriate to me. It's not immediately clear to
me what the implementation should look like, but in general behavior
that sets needsrepair on first modification and clears it as a final
step sounds more practical. Then the behavior can be easily explained as
"once repair starts on an fs, it must be completed before it is allowed
to mount." I do think this should be lifted off for a followon series so
we can make progress on the feature upgrade bits without growing more
requirements and complexity..

Brian

> Hmm, maybe I should pursue /that/ instead.
> 
> --D
> 
> > Brian
> > 
> > >  repair/agheader.h   |    2 ++
> > >  repair/dir2.c       |    3 +++
> > >  repair/phase6.c     |    7 +++++++
> > >  repair/xfs_repair.c |   37 +++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 49 insertions(+)
> > > 
> > > 
> > > diff --git a/repair/agheader.h b/repair/agheader.h
> > > index a63827c8..fa6fe596 100644
> > > --- a/repair/agheader.h
> > > +++ b/repair/agheader.h
> > > @@ -82,3 +82,5 @@ typedef struct fs_geo_list  {
> > >  #define XR_AG_AGF	0x2
> > >  #define XR_AG_AGI	0x4
> > >  #define XR_AG_SB_SEC	0x8
> > > +
> > > +void force_needsrepair(struct xfs_mount *mp);
> > > diff --git a/repair/dir2.c b/repair/dir2.c
> > > index eabdb4f2..922b8a3e 100644
> > > --- a/repair/dir2.c
> > > +++ b/repair/dir2.c
> > > @@ -15,6 +15,7 @@
> > >  #include "da_util.h"
> > >  #include "prefetch.h"
> > >  #include "progress.h"
> > > +#include "agheader.h"
> > >  
> > >  /*
> > >   * Known bad inode list.  These are seen when the leaf and node
> > > @@ -774,6 +775,7 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
> > >  				do_warn(
> > >  _("\tclearing inode number in entry at offset %" PRIdPTR "...\n"),
> > >  					(intptr_t)ptr - (intptr_t)d);
> > > +				force_needsrepair(mp);
> > >  				dep->name[0] = '/';
> > >  				*dirty = 1;
> > >  			} else {
> > > @@ -914,6 +916,7 @@ _("entry \"%*.*s\" in directory inode %" PRIu64 " points to self: "),
> > >  		 */
> > >  		if (junkit) {
> > >  			if (!no_modify) {
> > > +				force_needsrepair(mp);
> > >  				dep->name[0] = '/';
> > >  				*dirty = 1;
> > >  				do_warn(_("clearing entry\n"));
> > > diff --git a/repair/phase6.c b/repair/phase6.c
> > > index 14464bef..5ecbe9b2 100644
> > > --- a/repair/phase6.c
> > > +++ b/repair/phase6.c
> > > @@ -1649,6 +1649,7 @@ longform_dir2_entry_check_data(
> > >  			if (entry_junked(
> > >  	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ""),
> > >  					fname, ip->i_ino, inum)) {
> > > +				force_needsrepair(mp);
> > >  				dep->name[0] = '/';
> > >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> > >  			}
> > > @@ -1666,6 +1667,7 @@ longform_dir2_entry_check_data(
> > >  			if (entry_junked(
> > >  	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64),
> > >  					fname, ip->i_ino, inum)) {
> > > +				force_needsrepair(mp);
> > >  				dep->name[0] = '/';
> > >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> > >  			}
> > > @@ -1684,6 +1686,7 @@ longform_dir2_entry_check_data(
> > >  				if (entry_junked(
> > >  	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory"),
> > >  						ORPHANAGE, inum, ip->i_ino)) {
> > > +					force_needsrepair(mp);
> > >  					dep->name[0] = '/';
> > >  					libxfs_dir2_data_log_entry(&da, bp, dep);
> > >  				}
> > > @@ -1706,6 +1709,7 @@ longform_dir2_entry_check_data(
> > >  			if (entry_junked(
> > >  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
> > >  					fname, inum, ip->i_ino)) {
> > > +				force_needsrepair(mp);
> > >  				dep->name[0] = '/';
> > >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> > >  			}
> > > @@ -1737,6 +1741,7 @@ longform_dir2_entry_check_data(
> > >  				if (entry_junked(
> > >  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block"), fname,
> > >  						inum, ip->i_ino)) {
> > > +					force_needsrepair(mp);
> > >  					dep->name[0] = '/';
> > >  					libxfs_dir2_data_log_entry(&da, bp, dep);
> > >  				}
> > > @@ -1764,6 +1769,7 @@ longform_dir2_entry_check_data(
> > >  				if (entry_junked(
> > >  	_("entry \"%s\" in dir %" PRIu64 " is not the first entry"),
> > >  						fname, inum, ip->i_ino)) {
> > > +					force_needsrepair(mp);
> > >  					dep->name[0] = '/';
> > >  					libxfs_dir2_data_log_entry(&da, bp, dep);
> > >  				}
> > > @@ -1852,6 +1858,7 @@ _("entry \"%s\" in dir inode %" PRIu64 " inconsistent with .. value (%" PRIu64 "
> > >  				orphanage_ino = 0;
> > >  			nbad++;
> > >  			if (!no_modify)  {
> > > +				force_needsrepair(mp);
> > >  				dep->name[0] = '/';
> > >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> > >  				if (verbose)
> > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > index f607afcb..9dc73854 100644
> > > --- a/repair/xfs_repair.c
> > > +++ b/repair/xfs_repair.c
> > > @@ -754,6 +754,43 @@ clear_needsrepair(
> > >  		libxfs_buf_relse(bp);
> > >  }
> > >  
> > > +/*
> > > + * Mark the filesystem as needing repair.  This should only be called by code
> > > + * that deliberately sets invalid sentinel values in the on-disk metadata to
> > > + * trigger a later reconstruction, and only after we've settled the primary
> > > + * super contents (i.e. after phase 1).
> > > + */
> > > +void
> > > +force_needsrepair(
> > > +	struct xfs_mount	*mp)
> > > +{
> > > +	struct xfs_buf		*bp;
> > > +	int			error;
> > > +
> > > +	if (!xfs_sb_version_hascrc(&mp->m_sb) ||
> > > +	    xfs_sb_version_needsrepair(&mp->m_sb))
> > > +		return;
> > > +
> > > +	bp = libxfs_getsb(mp);
> > > +	if (!bp || bp->b_error) {
> > > +		do_log(
> > > +	_("couldn't get superblock to set needsrepair, err=%d\n"),
> > > +				bp ? bp->b_error : ENOMEM);
> > > +		return;
> > > +	} else {
> > > +		mp->m_sb.sb_features_incompat |=
> > > +				XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > > +		libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > > +
> > > +		/* Force the primary super to disk immediately. */
> > > +		error = -libxfs_bwrite(bp);
> > > +		if (error)
> > > +			do_log(_("couldn't force needsrepair, err=%d\n"), error);
> > > +	}
> > > +	if (bp)
> > > +		libxfs_buf_relse(bp);
> > > +}
> > > +
> > >  int
> > >  main(int argc, char **argv)
> > >  {
> > > 
> > 
> 

Re: [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 02:14:22PM -0500, Brian Foster wrote:
> On Tue, Feb 09, 2021 at 10:35:42AM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 09, 2021 at 12:20:59PM -0500, Brian Foster wrote:
> > > On Mon, Feb 08, 2021 at 08:10:44PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > There are a few places in xfs_repair's directory checking code where we
> > > > deliberately corrupt a directory entry as a sentinel to trigger a
> > > > correction in later repair phase.  In the mean time, the filesystem is
> > > > inconsistent, so set the needsrepair flag to force a re-run of repair if
> > > > the system goes down.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > 
> > > Hmm.. this seems orthogonal to the rest of the series. I'm sure we can
> > > come up with various additional uses for the bit, but it seems a little
> > > odd to me that repair might set it in some cases after a crash but not
> > > others (if the filesystem happens to already be corrupt, for example).
> > 
> > <nod> Another option I thought of is to add a hook to the buffer cache
> > so that the first time anyone tries to bwrite a buffer (either directly
> > or via a delwri list or normal buffer cache writeback) we'll also set
> > needsrepair on the ondisk primary super.  That would protect us against
> > other scenarios like crashing after writing a new AGF but before writing
> > the new AGI, where the fs is left in an indeterminate state.
> > 
> 
> Yeah, that _seems_ more appropriate to me. It's not immediately clear to
> me what the implementation should look like, but in general behavior
> that sets needsrepair on first modification and clears it as a final
> step sounds more practical. Then the behavior can be easily explained as
> "once repair starts on an fs, it must be completed before it is allowed
> to mount." I do think this should be lifted off for a followon series so
> we can make progress on the feature upgrade bits without growing more
> requirements and complexity..

Oh, definitely. I'll withdraw this patch for now in the interests of
getting everything else going for Eric. :)

--D

> Brian
> 
> > Hmm, maybe I should pursue /that/ instead.
> > 
> > --D
> > 
> > > Brian
> > > 
> > > >  repair/agheader.h   |    2 ++
> > > >  repair/dir2.c       |    3 +++
> > > >  repair/phase6.c     |    7 +++++++
> > > >  repair/xfs_repair.c |   37 +++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 49 insertions(+)
> > > > 
> > > > 
> > > > diff --git a/repair/agheader.h b/repair/agheader.h
> > > > index a63827c8..fa6fe596 100644
> > > > --- a/repair/agheader.h
> > > > +++ b/repair/agheader.h
> > > > @@ -82,3 +82,5 @@ typedef struct fs_geo_list  {
> > > >  #define XR_AG_AGF	0x2
> > > >  #define XR_AG_AGI	0x4
> > > >  #define XR_AG_SB_SEC	0x8
> > > > +
> > > > +void force_needsrepair(struct xfs_mount *mp);
> > > > diff --git a/repair/dir2.c b/repair/dir2.c
> > > > index eabdb4f2..922b8a3e 100644
> > > > --- a/repair/dir2.c
> > > > +++ b/repair/dir2.c
> > > > @@ -15,6 +15,7 @@
> > > >  #include "da_util.h"
> > > >  #include "prefetch.h"
> > > >  #include "progress.h"
> > > > +#include "agheader.h"
> > > >  
> > > >  /*
> > > >   * Known bad inode list.  These are seen when the leaf and node
> > > > @@ -774,6 +775,7 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
> > > >  				do_warn(
> > > >  _("\tclearing inode number in entry at offset %" PRIdPTR "...\n"),
> > > >  					(intptr_t)ptr - (intptr_t)d);
> > > > +				force_needsrepair(mp);
> > > >  				dep->name[0] = '/';
> > > >  				*dirty = 1;
> > > >  			} else {
> > > > @@ -914,6 +916,7 @@ _("entry \"%*.*s\" in directory inode %" PRIu64 " points to self: "),
> > > >  		 */
> > > >  		if (junkit) {
> > > >  			if (!no_modify) {
> > > > +				force_needsrepair(mp);
> > > >  				dep->name[0] = '/';
> > > >  				*dirty = 1;
> > > >  				do_warn(_("clearing entry\n"));
> > > > diff --git a/repair/phase6.c b/repair/phase6.c
> > > > index 14464bef..5ecbe9b2 100644
> > > > --- a/repair/phase6.c
> > > > +++ b/repair/phase6.c
> > > > @@ -1649,6 +1649,7 @@ longform_dir2_entry_check_data(
> > > >  			if (entry_junked(
> > > >  	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ""),
> > > >  					fname, ip->i_ino, inum)) {
> > > > +				force_needsrepair(mp);
> > > >  				dep->name[0] = '/';
> > > >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> > > >  			}
> > > > @@ -1666,6 +1667,7 @@ longform_dir2_entry_check_data(
> > > >  			if (entry_junked(
> > > >  	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64),
> > > >  					fname, ip->i_ino, inum)) {
> > > > +				force_needsrepair(mp);
> > > >  				dep->name[0] = '/';
> > > >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> > > >  			}
> > > > @@ -1684,6 +1686,7 @@ longform_dir2_entry_check_data(
> > > >  				if (entry_junked(
> > > >  	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory"),
> > > >  						ORPHANAGE, inum, ip->i_ino)) {
> > > > +					force_needsrepair(mp);
> > > >  					dep->name[0] = '/';
> > > >  					libxfs_dir2_data_log_entry(&da, bp, dep);
> > > >  				}
> > > > @@ -1706,6 +1709,7 @@ longform_dir2_entry_check_data(
> > > >  			if (entry_junked(
> > > >  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name"),
> > > >  					fname, inum, ip->i_ino)) {
> > > > +				force_needsrepair(mp);
> > > >  				dep->name[0] = '/';
> > > >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> > > >  			}
> > > > @@ -1737,6 +1741,7 @@ longform_dir2_entry_check_data(
> > > >  				if (entry_junked(
> > > >  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block"), fname,
> > > >  						inum, ip->i_ino)) {
> > > > +					force_needsrepair(mp);
> > > >  					dep->name[0] = '/';
> > > >  					libxfs_dir2_data_log_entry(&da, bp, dep);
> > > >  				}
> > > > @@ -1764,6 +1769,7 @@ longform_dir2_entry_check_data(
> > > >  				if (entry_junked(
> > > >  	_("entry \"%s\" in dir %" PRIu64 " is not the first entry"),
> > > >  						fname, inum, ip->i_ino)) {
> > > > +					force_needsrepair(mp);
> > > >  					dep->name[0] = '/';
> > > >  					libxfs_dir2_data_log_entry(&da, bp, dep);
> > > >  				}
> > > > @@ -1852,6 +1858,7 @@ _("entry \"%s\" in dir inode %" PRIu64 " inconsistent with .. value (%" PRIu64 "
> > > >  				orphanage_ino = 0;
> > > >  			nbad++;
> > > >  			if (!no_modify)  {
> > > > +				force_needsrepair(mp);
> > > >  				dep->name[0] = '/';
> > > >  				libxfs_dir2_data_log_entry(&da, bp, dep);
> > > >  				if (verbose)
> > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > > index f607afcb..9dc73854 100644
> > > > --- a/repair/xfs_repair.c
> > > > +++ b/repair/xfs_repair.c
> > > > @@ -754,6 +754,43 @@ clear_needsrepair(
> > > >  		libxfs_buf_relse(bp);
> > > >  }
> > > >  
> > > > +/*
> > > > + * Mark the filesystem as needing repair.  This should only be called by code
> > > > + * that deliberately sets invalid sentinel values in the on-disk metadata to
> > > > + * trigger a later reconstruction, and only after we've settled the primary
> > > > + * super contents (i.e. after phase 1).
> > > > + */
> > > > +void
> > > > +force_needsrepair(
> > > > +	struct xfs_mount	*mp)
> > > > +{
> > > > +	struct xfs_buf		*bp;
> > > > +	int			error;
> > > > +
> > > > +	if (!xfs_sb_version_hascrc(&mp->m_sb) ||
> > > > +	    xfs_sb_version_needsrepair(&mp->m_sb))
> > > > +		return;
> > > > +
> > > > +	bp = libxfs_getsb(mp);
> > > > +	if (!bp || bp->b_error) {
> > > > +		do_log(
> > > > +	_("couldn't get superblock to set needsrepair, err=%d\n"),
> > > > +				bp ? bp->b_error : ENOMEM);
> > > > +		return;
> > > > +	} else {
> > > > +		mp->m_sb.sb_features_incompat |=
> > > > +				XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > > > +		libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > > > +
> > > > +		/* Force the primary super to disk immediately. */
> > > > +		error = -libxfs_bwrite(bp);
> > > > +		if (error)
> > > > +			do_log(_("couldn't force needsrepair, err=%d\n"), error);
> > > > +	}
> > > > +	if (bp)
> > > > +		libxfs_buf_relse(bp);
> > > > +}
> > > > +
> > > >  int
> > > >  main(int argc, char **argv)
> > > >  {
> > > > 
> > > 
> > 
> 

Re: [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories

[Eric Sandeen]

On 2/9/21 1:43 PM, Darrick J. Wong wrote:
> On Tue, Feb 09, 2021 at 02:14:22PM -0500, Brian Foster wrote:
>> On Tue, Feb 09, 2021 at 10:35:42AM -0800, Darrick J. Wong wrote:
>>> On Tue, Feb 09, 2021 at 12:20:59PM -0500, Brian Foster wrote:
>>>> On Mon, Feb 08, 2021 at 08:10:44PM -0800, Darrick J. Wong wrote:
>>>>> From: Darrick J. Wong <djwong@kernel.org>
>>>>>
>>>>> There are a few places in xfs_repair's directory checking code where we
>>>>> deliberately corrupt a directory entry as a sentinel to trigger a
>>>>> correction in later repair phase.  In the mean time, the filesystem is
>>>>> inconsistent, so set the needsrepair flag to force a re-run of repair if
>>>>> the system goes down.
>>>>>
>>>>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>>>>> ---
>>>>
>>>> Hmm.. this seems orthogonal to the rest of the series. I'm sure we can
>>>> come up with various additional uses for the bit, but it seems a little
>>>> odd to me that repair might set it in some cases after a crash but not
>>>> others (if the filesystem happens to already be corrupt, for example).
>>>
>>> <nod> Another option I thought of is to add a hook to the buffer cache
>>> so that the first time anyone tries to bwrite a buffer (either directly
>>> or via a delwri list or normal buffer cache writeback) we'll also set
>>> needsrepair on the ondisk primary super.  That would protect us against
>>> other scenarios like crashing after writing a new AGF but before writing
>>> the new AGI, where the fs is left in an indeterminate state.
>>>
>>
>> Yeah, that _seems_ more appropriate to me. It's not immediately clear to
>> me what the implementation should look like, but in general behavior
>> that sets needsrepair on first modification and clears it as a final
>> step sounds more practical. Then the behavior can be easily explained as
>> "once repair starts on an fs, it must be completed before it is allowed
>> to mount." I do think this should be lifted off for a followon series so
>> we can make progress on the feature upgrade bits without growing more
>> requirements and complexity..
> 
> Oh, definitely. I'll withdraw this patch for now in the interests of
> getting everything else going for Eric. :)

Noted, I'll drop this one for now, thanks.

-Eric

[PATCH 08/10] xfs_repair: allow setting the needsrepair flag

[Darrick J. Wong]

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

Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at
program start and (presumably) clear it by the end of the run.  This
code isn't terribly useful to users; it's mainly here so that fstests
can exercise the functionality.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/globals.c    |    2 ++
 repair/globals.h    |    2 ++
 repair/phase1.c     |   23 +++++++++++++++++++++++
 repair/xfs_repair.c |    9 +++++++++
 4 files changed, 36 insertions(+)


diff --git a/repair/globals.c b/repair/globals.c
index 110d98b6..699a96ee 100644
--- a/repair/globals.c
+++ b/repair/globals.c
@@ -49,6 +49,8 @@ int	rt_spec;		/* Realtime dev specified as option */
 int	convert_lazy_count;	/* Convert lazy-count mode on/off */
 int	lazy_count;		/* What to set if to if converting */
 
+bool	add_needsrepair;	/* forcibly set needsrepair while repairing */
+
 /* misc status variables */
 
 int	primary_sb_modified;
diff --git a/repair/globals.h b/repair/globals.h
index 1d397b35..043b3e8e 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -90,6 +90,8 @@ extern int	rt_spec;		/* Realtime dev specified as option */
 extern int	convert_lazy_count;	/* Convert lazy-count mode on/off */
 extern int	lazy_count;		/* What to set if to if converting */
 
+extern bool	add_needsrepair;
+
 /* misc status variables */
 
 extern int		primary_sb_modified;
diff --git a/repair/phase1.c b/repair/phase1.c
index 00b98584..b26d25f8 100644
--- a/repair/phase1.c
+++ b/repair/phase1.c
@@ -30,6 +30,26 @@ alloc_ag_buf(int size)
 	return(bp);
 }
 
+static void
+set_needsrepair(
+	struct xfs_sb	*sb)
+{
+	if (!xfs_sb_version_hascrc(sb)) {
+		printf(
+	_("needsrepair flag only supported on V5 filesystems.\n"));
+		exit(0);
+	}
+
+	if (xfs_sb_version_needsrepair(sb)) {
+		printf(_("Filesystem already marked as needing repair.\n"));
+		return;
+	}
+
+	printf(_("Marking filesystem in need of repair.\n"));
+	primary_sb_modified = 1;
+	sb->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+}
+
 /*
  * this has got to be big enough to hold 4 sectors
  */
@@ -126,6 +146,9 @@ _("Cannot disable lazy-counters on V5 fs\n"));
 		}
 	}
 
+	if (add_needsrepair)
+		set_needsrepair(sb);
+
 	/* shared_vn should be zero */
 	if (sb->sb_shared_vn) {
 		do_warn(_("resetting shared_vn to zero\n"));
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 9dc73854..ee377e8a 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -65,11 +65,13 @@ static char *o_opts[] = {
  */
 enum c_opt_nums {
 	CONVERT_LAZY_COUNT = 0,
+	CONVERT_NEEDSREPAIR,
 	C_MAX_OPTS,
 };
 
 static char *c_opts[] = {
 	[CONVERT_LAZY_COUNT]	= "lazycount",
+	[CONVERT_NEEDSREPAIR]	= "needsrepair",
 	[C_MAX_OPTS]		= NULL,
 };
 
@@ -302,6 +304,13 @@ process_args(int argc, char **argv)
 					lazy_count = (int)strtol(val, NULL, 0);
 					convert_lazy_count = 1;
 					break;
+				case CONVERT_NEEDSREPAIR:
+					if (!val)
+						do_abort(
+		_("-c needsrepair requires a parameter\n"));
+					if (strtol(val, NULL, 0) == 1)
+						add_needsrepair = true;
+					break;
 				default:
 					unknown('c', val);
 					break;

Re: [PATCH 08/10] xfs_repair: allow setting the needsrepair flag

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at
> program start and (presumably) clear it by the end of the run.  This
> code isn't terribly useful to users; it's mainly here so that fstests
> can exercise the functionality.

What does the quietly above mean?

Otherwise this looks good:

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

Re: [PATCH 08/10] xfs_repair: allow setting the needsrepair flag

[Eric Sandeen]

On 2/9/21 3:15 AM, Christoph Hellwig wrote:
> On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote:
>> From: Darrick J. Wong <djwong@kernel.org>
>>
>> Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at
>> program start and (presumably) clear it by the end of the run.  This
>> code isn't terribly useful to users; it's mainly here so that fstests
>> can exercise the functionality.
> 
> What does the quietly above mean?

I think it means "don't document it in the man page, this is a secret
for XFS developers and testers"

> Otherwise this looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Re: [PATCH 08/10] xfs_repair: allow setting the needsrepair flag

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 08:41:34AM -0600, Eric Sandeen wrote:
> 
> 
> On 2/9/21 3:15 AM, Christoph Hellwig wrote:
> > On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote:
> >> From: Darrick J. Wong <djwong@kernel.org>
> >>
> >> Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at
> >> program start and (presumably) clear it by the end of the run.  This
> >> code isn't terribly useful to users; it's mainly here so that fstests
> >> can exercise the functionality.
> > 
> > What does the quietly above mean?
> 
> I think it means "don't document it in the man page, this is a secret
> for XFS developers and testers"

Yep.  I'll add the following to the commit message:

"We don't document this flag in the manual pages at all because repair
clears needsrepair at exit, which means the knobs only exist for fstests
to exercise the functionality."

--D

> 
> > Otherwise this looks good:
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 

Re: [PATCH 08/10] xfs_repair: allow setting the needsrepair flag

[Eric Sandeen]

On 2/9/21 10:47 AM, Darrick J. Wong wrote:
> "We don't document this flag in the manual pages at all because repair
> clears needsrepair at exit, which means the knobs only exist for fstests
> to exercise the functionality."

I can add that and save you a re-send.

-Eric

Re: [PATCH 08/10] xfs_repair: allow setting the needsrepair flag

[Brian Foster]

On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at
> program start and (presumably) clear it by the end of the run.  This
> code isn't terribly useful to users; it's mainly here so that fstests
> can exercise the functionality.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/globals.c    |    2 ++
>  repair/globals.h    |    2 ++
>  repair/phase1.c     |   23 +++++++++++++++++++++++
>  repair/xfs_repair.c |    9 +++++++++
>  4 files changed, 36 insertions(+)
> 
> 
...
> diff --git a/repair/phase1.c b/repair/phase1.c
> index 00b98584..b26d25f8 100644
> --- a/repair/phase1.c
> +++ b/repair/phase1.c
> @@ -30,6 +30,26 @@ alloc_ag_buf(int size)
>  	return(bp);
>  }
>  
> +static void
> +set_needsrepair(
> +	struct xfs_sb	*sb)
> +{
> +	if (!xfs_sb_version_hascrc(sb)) {
> +		printf(
> +	_("needsrepair flag only supported on V5 filesystems.\n"));
> +		exit(0);
> +	}
> +
> +	if (xfs_sb_version_needsrepair(sb)) {
> +		printf(_("Filesystem already marked as needing repair.\n"));
> +		return;
> +	}

Any reason this doesn't exit the repair like the lazy counter logic
does? Otherwise seems fine:

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

> +
> +	printf(_("Marking filesystem in need of repair.\n"));
> +	primary_sb_modified = 1;
> +	sb->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> +}
> +
>  /*
>   * this has got to be big enough to hold 4 sectors
>   */
> @@ -126,6 +146,9 @@ _("Cannot disable lazy-counters on V5 fs\n"));
>  		}
>  	}
>  
> +	if (add_needsrepair)
> +		set_needsrepair(sb);
> +
>  	/* shared_vn should be zero */
>  	if (sb->sb_shared_vn) {
>  		do_warn(_("resetting shared_vn to zero\n"));
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 9dc73854..ee377e8a 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -65,11 +65,13 @@ static char *o_opts[] = {
>   */
>  enum c_opt_nums {
>  	CONVERT_LAZY_COUNT = 0,
> +	CONVERT_NEEDSREPAIR,
>  	C_MAX_OPTS,
>  };
>  
>  static char *c_opts[] = {
>  	[CONVERT_LAZY_COUNT]	= "lazycount",
> +	[CONVERT_NEEDSREPAIR]	= "needsrepair",
>  	[C_MAX_OPTS]		= NULL,
>  };
>  
> @@ -302,6 +304,13 @@ process_args(int argc, char **argv)
>  					lazy_count = (int)strtol(val, NULL, 0);
>  					convert_lazy_count = 1;
>  					break;
> +				case CONVERT_NEEDSREPAIR:
> +					if (!val)
> +						do_abort(
> +		_("-c needsrepair requires a parameter\n"));
> +					if (strtol(val, NULL, 0) == 1)
> +						add_needsrepair = true;
> +					break;
>  				default:
>  					unknown('c', val);
>  					break;
> 

Re: [PATCH 08/10] xfs_repair: allow setting the needsrepair flag

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 12:21:21PM -0500, Brian Foster wrote:
> On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at
> > program start and (presumably) clear it by the end of the run.  This
> > code isn't terribly useful to users; it's mainly here so that fstests
> > can exercise the functionality.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  repair/globals.c    |    2 ++
> >  repair/globals.h    |    2 ++
> >  repair/phase1.c     |   23 +++++++++++++++++++++++
> >  repair/xfs_repair.c |    9 +++++++++
> >  4 files changed, 36 insertions(+)
> > 
> > 
> ...
> > diff --git a/repair/phase1.c b/repair/phase1.c
> > index 00b98584..b26d25f8 100644
> > --- a/repair/phase1.c
> > +++ b/repair/phase1.c
> > @@ -30,6 +30,26 @@ alloc_ag_buf(int size)
> >  	return(bp);
> >  }
> >  
> > +static void
> > +set_needsrepair(
> > +	struct xfs_sb	*sb)
> > +{
> > +	if (!xfs_sb_version_hascrc(sb)) {
> > +		printf(
> > +	_("needsrepair flag only supported on V5 filesystems.\n"));
> > +		exit(0);
> > +	}
> > +
> > +	if (xfs_sb_version_needsrepair(sb)) {
> > +		printf(_("Filesystem already marked as needing repair.\n"));
> > +		return;
> > +	}
> 
> Any reason this doesn't exit the repair like the lazy counter logic
> does? Otherwise seems fine:

No particular reason.  Will fix for consistency's sake.

> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > +
> > +	printf(_("Marking filesystem in need of repair.\n"));
> > +	primary_sb_modified = 1;
> > +	sb->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > +}
> > +
> >  /*
> >   * this has got to be big enough to hold 4 sectors
> >   */
> > @@ -126,6 +146,9 @@ _("Cannot disable lazy-counters on V5 fs\n"));
> >  		}
> >  	}
> >  
> > +	if (add_needsrepair)
> > +		set_needsrepair(sb);
> > +
> >  	/* shared_vn should be zero */
> >  	if (sb->sb_shared_vn) {
> >  		do_warn(_("resetting shared_vn to zero\n"));
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 9dc73854..ee377e8a 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -65,11 +65,13 @@ static char *o_opts[] = {
> >   */
> >  enum c_opt_nums {
> >  	CONVERT_LAZY_COUNT = 0,
> > +	CONVERT_NEEDSREPAIR,
> >  	C_MAX_OPTS,
> >  };
> >  
> >  static char *c_opts[] = {
> >  	[CONVERT_LAZY_COUNT]	= "lazycount",
> > +	[CONVERT_NEEDSREPAIR]	= "needsrepair",
> >  	[C_MAX_OPTS]		= NULL,
> >  };
> >  
> > @@ -302,6 +304,13 @@ process_args(int argc, char **argv)
> >  					lazy_count = (int)strtol(val, NULL, 0);
> >  					convert_lazy_count = 1;
> >  					break;
> > +				case CONVERT_NEEDSREPAIR:
> > +					if (!val)
> > +						do_abort(
> > +		_("-c needsrepair requires a parameter\n"));
> > +					if (strtol(val, NULL, 0) == 1)
> > +						add_needsrepair = true;
> > +					break;
> >  				default:
> >  					unknown('c', val);
> >  					break;
> > 
> 

Re: [PATCH 08/10] xfs_repair: allow setting the needsrepair flag

[Eric Sandeen]

On 2/9/21 12:10 PM, Darrick J. Wong wrote:
> On Tue, Feb 09, 2021 at 12:21:21PM -0500, Brian Foster wrote:
>> On Mon, Feb 08, 2021 at 08:10:49PM -0800, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <djwong@kernel.org>
>>>
>>> Quietly set up the ability to tell xfs_repair to set NEEDSREPAIR at
>>> program start and (presumably) clear it by the end of the run.  This
>>> code isn't terribly useful to users; it's mainly here so that fstests
>>> can exercise the functionality.
>>>
>>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>>> ---
>>>  repair/globals.c    |    2 ++
>>>  repair/globals.h    |    2 ++
>>>  repair/phase1.c     |   23 +++++++++++++++++++++++
>>>  repair/xfs_repair.c |    9 +++++++++
>>>  4 files changed, 36 insertions(+)
>>>
>>>
>> ...
>>> diff --git a/repair/phase1.c b/repair/phase1.c
>>> index 00b98584..b26d25f8 100644
>>> --- a/repair/phase1.c
>>> +++ b/repair/phase1.c
>>> @@ -30,6 +30,26 @@ alloc_ag_buf(int size)
>>>  	return(bp);
>>>  }
>>>  
>>> +static void
>>> +set_needsrepair(
>>> +	struct xfs_sb	*sb)
>>> +{
>>> +	if (!xfs_sb_version_hascrc(sb)) {
>>> +		printf(
>>> +	_("needsrepair flag only supported on V5 filesystems.\n"));
>>> +		exit(0);
>>> +	}
>>> +
>>> +	if (xfs_sb_version_needsrepair(sb)) {
>>> +		printf(_("Filesystem already marked as needing repair.\n"));
>>> +		return;
>>> +	}
>>
>> Any reason this doesn't exit the repair like the lazy counter logic
>> does? Otherwise seems fine:
> 
> No particular reason.  Will fix for consistency's sake.

I can swap the return; for an exit(0); on my end if you like and note it
in the commit log.

-Eric

[PATCH 09/10] xfs_repair: add a testing hook for NEEDSREPAIR

[Darrick J. Wong]

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

Simulate a crash when anyone calls force_needsrepair.  This is a debug
knob so that we can test that the kernel won't mount after setting
needsrepair and that a re-run of xfs_repair will clear the flag.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/globals.c    |    1 +
 repair/globals.h    |    2 ++
 repair/phase1.c     |    5 +++++
 repair/xfs_repair.c |    7 +++++++
 4 files changed, 15 insertions(+)


diff --git a/repair/globals.c b/repair/globals.c
index 699a96ee..b0e23864 100644
--- a/repair/globals.c
+++ b/repair/globals.c
@@ -40,6 +40,7 @@ int	dangerously;		/* live dangerously ... fix ro mount */
 int	isa_file;
 int	zap_log;
 int	dumpcore;		/* abort, not exit on fatal errs */
+bool	abort_after_force_needsrepair;
 int	force_geo;		/* can set geo on low confidence info */
 int	assume_xfs;		/* assume we have an xfs fs */
 char	*log_name;		/* Name of log device */
diff --git a/repair/globals.h b/repair/globals.h
index 043b3e8e..9fa73b2c 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -82,6 +82,8 @@ extern int	isa_file;
 extern int	zap_log;
 extern int	dumpcore;		/* abort, not exit on fatal errs */
 extern int	force_geo;		/* can set geo on low confidence info */
+/* Abort after forcing NEEDSREPAIR to test its functionality */
+extern bool	abort_after_force_needsrepair;
 extern int	assume_xfs;		/* assume we have an xfs fs */
 extern char	*log_name;		/* Name of log device */
 extern int	log_spec;		/* Log dev specified as option */
diff --git a/repair/phase1.c b/repair/phase1.c
index b26d25f8..57f72cd0 100644
--- a/repair/phase1.c
+++ b/repair/phase1.c
@@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n"));
 	 */
 	sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0;
 
+	/* Simulate a crash after setting needsrepair. */
+	if (primary_sb_modified && add_needsrepair &&
+	    abort_after_force_needsrepair)
+		exit(55);
+
 	free(sb);
 }
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index ee377e8a..ae7106a6 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -44,6 +44,7 @@ enum o_opt_nums {
 	BLOAD_LEAF_SLACK,
 	BLOAD_NODE_SLACK,
 	NOQUOTA,
+	FORCE_NEEDSREPAIR_ABORT,
 	O_MAX_OPTS,
 };
 
@@ -57,6 +58,7 @@ static char *o_opts[] = {
 	[BLOAD_LEAF_SLACK]	= "debug_bload_leaf_slack",
 	[BLOAD_NODE_SLACK]	= "debug_bload_node_slack",
 	[NOQUOTA]		= "noquota",
+	[FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort",
 	[O_MAX_OPTS]		= NULL,
 };
 
@@ -282,6 +284,9 @@ process_args(int argc, char **argv)
 		_("-o debug_bload_node_slack requires a parameter\n"));
 					bload_node_slack = (int)strtol(val, NULL, 0);
 					break;
+				case FORCE_NEEDSREPAIR_ABORT:
+					abort_after_force_needsrepair = true;
+					break;
 				case NOQUOTA:
 					quotacheck_skip();
 					break;
@@ -795,6 +800,8 @@ force_needsrepair(
 		error = -libxfs_bwrite(bp);
 		if (error)
 			do_log(_("couldn't force needsrepair, err=%d\n"), error);
+		if (abort_after_force_needsrepair)
+			exit(55);
 	}
 	if (bp)
 		libxfs_buf_relse(bp);

Re: [PATCH 09/10] xfs_repair: add a testing hook for NEEDSREPAIR

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Simulate a crash when anyone calls force_needsrepair.  This is a debug
> knob so that we can test that the kernel won't mount after setting
> needsrepair and that a re-run of xfs_repair will clear the flag.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

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

Re: [PATCH 09/10] xfs_repair: add a testing hook for NEEDSREPAIR

[Brian Foster]

On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Simulate a crash when anyone calls force_needsrepair.  This is a debug
> knob so that we can test that the kernel won't mount after setting
> needsrepair and that a re-run of xfs_repair will clear the flag.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Can't we just use db to manually set the bit on the superblock?

Brian

>  repair/globals.c    |    1 +
>  repair/globals.h    |    2 ++
>  repair/phase1.c     |    5 +++++
>  repair/xfs_repair.c |    7 +++++++
>  4 files changed, 15 insertions(+)
> 
> 
> diff --git a/repair/globals.c b/repair/globals.c
> index 699a96ee..b0e23864 100644
> --- a/repair/globals.c
> +++ b/repair/globals.c
> @@ -40,6 +40,7 @@ int	dangerously;		/* live dangerously ... fix ro mount */
>  int	isa_file;
>  int	zap_log;
>  int	dumpcore;		/* abort, not exit on fatal errs */
> +bool	abort_after_force_needsrepair;
>  int	force_geo;		/* can set geo on low confidence info */
>  int	assume_xfs;		/* assume we have an xfs fs */
>  char	*log_name;		/* Name of log device */
> diff --git a/repair/globals.h b/repair/globals.h
> index 043b3e8e..9fa73b2c 100644
> --- a/repair/globals.h
> +++ b/repair/globals.h
> @@ -82,6 +82,8 @@ extern int	isa_file;
>  extern int	zap_log;
>  extern int	dumpcore;		/* abort, not exit on fatal errs */
>  extern int	force_geo;		/* can set geo on low confidence info */
> +/* Abort after forcing NEEDSREPAIR to test its functionality */
> +extern bool	abort_after_force_needsrepair;
>  extern int	assume_xfs;		/* assume we have an xfs fs */
>  extern char	*log_name;		/* Name of log device */
>  extern int	log_spec;		/* Log dev specified as option */
> diff --git a/repair/phase1.c b/repair/phase1.c
> index b26d25f8..57f72cd0 100644
> --- a/repair/phase1.c
> +++ b/repair/phase1.c
> @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n"));
>  	 */
>  	sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0;
>  
> +	/* Simulate a crash after setting needsrepair. */
> +	if (primary_sb_modified && add_needsrepair &&
> +	    abort_after_force_needsrepair)
> +		exit(55);
> +
>  	free(sb);
>  }
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index ee377e8a..ae7106a6 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -44,6 +44,7 @@ enum o_opt_nums {
>  	BLOAD_LEAF_SLACK,
>  	BLOAD_NODE_SLACK,
>  	NOQUOTA,
> +	FORCE_NEEDSREPAIR_ABORT,
>  	O_MAX_OPTS,
>  };
>  
> @@ -57,6 +58,7 @@ static char *o_opts[] = {
>  	[BLOAD_LEAF_SLACK]	= "debug_bload_leaf_slack",
>  	[BLOAD_NODE_SLACK]	= "debug_bload_node_slack",
>  	[NOQUOTA]		= "noquota",
> +	[FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort",
>  	[O_MAX_OPTS]		= NULL,
>  };
>  
> @@ -282,6 +284,9 @@ process_args(int argc, char **argv)
>  		_("-o debug_bload_node_slack requires a parameter\n"));
>  					bload_node_slack = (int)strtol(val, NULL, 0);
>  					break;
> +				case FORCE_NEEDSREPAIR_ABORT:
> +					abort_after_force_needsrepair = true;
> +					break;
>  				case NOQUOTA:
>  					quotacheck_skip();
>  					break;
> @@ -795,6 +800,8 @@ force_needsrepair(
>  		error = -libxfs_bwrite(bp);
>  		if (error)
>  			do_log(_("couldn't force needsrepair, err=%d\n"), error);
> +		if (abort_after_force_needsrepair)
> +			exit(55);
>  	}
>  	if (bp)
>  		libxfs_buf_relse(bp);
> 

Re: [PATCH 09/10] xfs_repair: add a testing hook for NEEDSREPAIR

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 12:21:31PM -0500, Brian Foster wrote:
> On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Simulate a crash when anyone calls force_needsrepair.  This is a debug
> > knob so that we can test that the kernel won't mount after setting
> > needsrepair and that a re-run of xfs_repair will clear the flag.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> Can't we just use db to manually set the bit on the superblock?

No, because the fstest uses this debug knob to simulate the following:

1) sysadmin issues 'xfs_admin -O inobtcount /dev/sda1'
2) xfs_repair flips on INOBTCOUNT and NEEDSREPAIR
3) system goes down and repair never completes
4) verify that we can't mount
5) verify that repair clears NEEDSREPAIR and gives us a clean fs
6) verify that mount works again

and the other scenario is:

1) fuzz a directory entry in such a way that repair will decide to
   blow out the dirent and rebuild the directory later
2) sysadmin issues 'xfs_repair /dev/sda1'
2) xfs_repair flips on NEEDSREPAIR at the same time it corrupts the
   dirent to trigger the rebuild later
3) system goes down and repair never completes
4) verify that we can't mount
5) verify that repair clears NEEDSREPAIR and gives us a clean fs
6) verify that mount works again

Both cases reflect what I think are the most likely failure scenarios,
hence the knob needs to be in xfs_repair to prevent it from running to
completion.

(And yes, I've been recently very bad at sending fstests out for review
the past few months; I will get that done by this afternoon.)

--D

> Brian
> 
> >  repair/globals.c    |    1 +
> >  repair/globals.h    |    2 ++
> >  repair/phase1.c     |    5 +++++
> >  repair/xfs_repair.c |    7 +++++++
> >  4 files changed, 15 insertions(+)
> > 
> > 
> > diff --git a/repair/globals.c b/repair/globals.c
> > index 699a96ee..b0e23864 100644
> > --- a/repair/globals.c
> > +++ b/repair/globals.c
> > @@ -40,6 +40,7 @@ int	dangerously;		/* live dangerously ... fix ro mount */
> >  int	isa_file;
> >  int	zap_log;
> >  int	dumpcore;		/* abort, not exit on fatal errs */
> > +bool	abort_after_force_needsrepair;
> >  int	force_geo;		/* can set geo on low confidence info */
> >  int	assume_xfs;		/* assume we have an xfs fs */
> >  char	*log_name;		/* Name of log device */
> > diff --git a/repair/globals.h b/repair/globals.h
> > index 043b3e8e..9fa73b2c 100644
> > --- a/repair/globals.h
> > +++ b/repair/globals.h
> > @@ -82,6 +82,8 @@ extern int	isa_file;
> >  extern int	zap_log;
> >  extern int	dumpcore;		/* abort, not exit on fatal errs */
> >  extern int	force_geo;		/* can set geo on low confidence info */
> > +/* Abort after forcing NEEDSREPAIR to test its functionality */
> > +extern bool	abort_after_force_needsrepair;
> >  extern int	assume_xfs;		/* assume we have an xfs fs */
> >  extern char	*log_name;		/* Name of log device */
> >  extern int	log_spec;		/* Log dev specified as option */
> > diff --git a/repair/phase1.c b/repair/phase1.c
> > index b26d25f8..57f72cd0 100644
> > --- a/repair/phase1.c
> > +++ b/repair/phase1.c
> > @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n"));
> >  	 */
> >  	sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0;
> >  
> > +	/* Simulate a crash after setting needsrepair. */
> > +	if (primary_sb_modified && add_needsrepair &&
> > +	    abort_after_force_needsrepair)
> > +		exit(55);
> > +
> >  	free(sb);
> >  }
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index ee377e8a..ae7106a6 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -44,6 +44,7 @@ enum o_opt_nums {
> >  	BLOAD_LEAF_SLACK,
> >  	BLOAD_NODE_SLACK,
> >  	NOQUOTA,
> > +	FORCE_NEEDSREPAIR_ABORT,
> >  	O_MAX_OPTS,
> >  };
> >  
> > @@ -57,6 +58,7 @@ static char *o_opts[] = {
> >  	[BLOAD_LEAF_SLACK]	= "debug_bload_leaf_slack",
> >  	[BLOAD_NODE_SLACK]	= "debug_bload_node_slack",
> >  	[NOQUOTA]		= "noquota",
> > +	[FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort",
> >  	[O_MAX_OPTS]		= NULL,
> >  };
> >  
> > @@ -282,6 +284,9 @@ process_args(int argc, char **argv)
> >  		_("-o debug_bload_node_slack requires a parameter\n"));
> >  					bload_node_slack = (int)strtol(val, NULL, 0);
> >  					break;
> > +				case FORCE_NEEDSREPAIR_ABORT:
> > +					abort_after_force_needsrepair = true;
> > +					break;
> >  				case NOQUOTA:
> >  					quotacheck_skip();
> >  					break;
> > @@ -795,6 +800,8 @@ force_needsrepair(
> >  		error = -libxfs_bwrite(bp);
> >  		if (error)
> >  			do_log(_("couldn't force needsrepair, err=%d\n"), error);
> > +		if (abort_after_force_needsrepair)
> > +			exit(55);
> >  	}
> >  	if (bp)
> >  		libxfs_buf_relse(bp);
> > 
> 

Re: [PATCH 09/10] xfs_repair: add a testing hook for NEEDSREPAIR

[Brian Foster]

On Tue, Feb 09, 2021 at 10:17:38AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 09, 2021 at 12:21:31PM -0500, Brian Foster wrote:
> > On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Simulate a crash when anyone calls force_needsrepair.  This is a debug
> > > knob so that we can test that the kernel won't mount after setting
> > > needsrepair and that a re-run of xfs_repair will clear the flag.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > 
> > Can't we just use db to manually set the bit on the superblock?
> 
> No, because the fstest uses this debug knob to simulate the following:
> 
> 1) sysadmin issues 'xfs_admin -O inobtcount /dev/sda1'
> 2) xfs_repair flips on INOBTCOUNT and NEEDSREPAIR
> 3) system goes down and repair never completes
> 4) verify that we can't mount
> 5) verify that repair clears NEEDSREPAIR and gives us a clean fs
> 6) verify that mount works again
> 

Ok, but that seems like circular reasoning. It wouldn't be quite the
same as a simulated repair failure, but ISTM that if we set the bit
manually, we can still verify steps 4, 5 and 6 as is (with the caveat
that the repair invocation performs a feature upgrade). I'm not sure how
important it really is to verify that a feature upgrade sequence sets
the bit if it happens to fail provided we have independent tests that 1.
verify the needsrepair bit works as expected and 2. verify the feature
upgrades work appropriately, since that is the primary functionality.

I wanted to think about that a little more before replying, but I also
just realized something odd when digging into the debug code:

# ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch 
Phase 1 - find and verify superblock...
Marking filesystem in need of repair.
writing modified primary superblock
Phase 2 - using internal log
        - zero log...
ERROR: The filesystem has valuable metadata changes in a log which needs to
...
# mount /dev/test/scratch /mnt/
mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error.
#

It looks like we can set a feature upgrade bit on the superblock before
we've examined the log and potentially discovered that it's dirty (phase
2). If the log is recoverable, that puts the user in a bit of a bind..

Brian

> and the other scenario is:
> 
> 1) fuzz a directory entry in such a way that repair will decide to
>    blow out the dirent and rebuild the directory later
> 2) sysadmin issues 'xfs_repair /dev/sda1'
> 2) xfs_repair flips on NEEDSREPAIR at the same time it corrupts the
>    dirent to trigger the rebuild later
> 3) system goes down and repair never completes
> 4) verify that we can't mount
> 5) verify that repair clears NEEDSREPAIR and gives us a clean fs
> 6) verify that mount works again
> 
> Both cases reflect what I think are the most likely failure scenarios,
> hence the knob needs to be in xfs_repair to prevent it from running to
> completion.
> 
> (And yes, I've been recently very bad at sending fstests out for review
> the past few months; I will get that done by this afternoon.)
> 
> --D
> 
> > Brian
> > 
> > >  repair/globals.c    |    1 +
> > >  repair/globals.h    |    2 ++
> > >  repair/phase1.c     |    5 +++++
> > >  repair/xfs_repair.c |    7 +++++++
> > >  4 files changed, 15 insertions(+)
> > > 
> > > 
> > > diff --git a/repair/globals.c b/repair/globals.c
> > > index 699a96ee..b0e23864 100644
> > > --- a/repair/globals.c
> > > +++ b/repair/globals.c
> > > @@ -40,6 +40,7 @@ int	dangerously;		/* live dangerously ... fix ro mount */
> > >  int	isa_file;
> > >  int	zap_log;
> > >  int	dumpcore;		/* abort, not exit on fatal errs */
> > > +bool	abort_after_force_needsrepair;
> > >  int	force_geo;		/* can set geo on low confidence info */
> > >  int	assume_xfs;		/* assume we have an xfs fs */
> > >  char	*log_name;		/* Name of log device */
> > > diff --git a/repair/globals.h b/repair/globals.h
> > > index 043b3e8e..9fa73b2c 100644
> > > --- a/repair/globals.h
> > > +++ b/repair/globals.h
> > > @@ -82,6 +82,8 @@ extern int	isa_file;
> > >  extern int	zap_log;
> > >  extern int	dumpcore;		/* abort, not exit on fatal errs */
> > >  extern int	force_geo;		/* can set geo on low confidence info */
> > > +/* Abort after forcing NEEDSREPAIR to test its functionality */
> > > +extern bool	abort_after_force_needsrepair;
> > >  extern int	assume_xfs;		/* assume we have an xfs fs */
> > >  extern char	*log_name;		/* Name of log device */
> > >  extern int	log_spec;		/* Log dev specified as option */
> > > diff --git a/repair/phase1.c b/repair/phase1.c
> > > index b26d25f8..57f72cd0 100644
> > > --- a/repair/phase1.c
> > > +++ b/repair/phase1.c
> > > @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n"));
> > >  	 */
> > >  	sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0;
> > >  
> > > +	/* Simulate a crash after setting needsrepair. */
> > > +	if (primary_sb_modified && add_needsrepair &&
> > > +	    abort_after_force_needsrepair)
> > > +		exit(55);
> > > +
> > >  	free(sb);
> > >  }
> > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > index ee377e8a..ae7106a6 100644
> > > --- a/repair/xfs_repair.c
> > > +++ b/repair/xfs_repair.c
> > > @@ -44,6 +44,7 @@ enum o_opt_nums {
> > >  	BLOAD_LEAF_SLACK,
> > >  	BLOAD_NODE_SLACK,
> > >  	NOQUOTA,
> > > +	FORCE_NEEDSREPAIR_ABORT,
> > >  	O_MAX_OPTS,
> > >  };
> > >  
> > > @@ -57,6 +58,7 @@ static char *o_opts[] = {
> > >  	[BLOAD_LEAF_SLACK]	= "debug_bload_leaf_slack",
> > >  	[BLOAD_NODE_SLACK]	= "debug_bload_node_slack",
> > >  	[NOQUOTA]		= "noquota",
> > > +	[FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort",
> > >  	[O_MAX_OPTS]		= NULL,
> > >  };
> > >  
> > > @@ -282,6 +284,9 @@ process_args(int argc, char **argv)
> > >  		_("-o debug_bload_node_slack requires a parameter\n"));
> > >  					bload_node_slack = (int)strtol(val, NULL, 0);
> > >  					break;
> > > +				case FORCE_NEEDSREPAIR_ABORT:
> > > +					abort_after_force_needsrepair = true;
> > > +					break;
> > >  				case NOQUOTA:
> > >  					quotacheck_skip();
> > >  					break;
> > > @@ -795,6 +800,8 @@ force_needsrepair(
> > >  		error = -libxfs_bwrite(bp);
> > >  		if (error)
> > >  			do_log(_("couldn't force needsrepair, err=%d\n"), error);
> > > +		if (abort_after_force_needsrepair)
> > > +			exit(55);
> > >  	}
> > >  	if (bp)
> > >  		libxfs_buf_relse(bp);
> > > 
> > 
> 

Re: [PATCH 09/10] xfs_repair: add a testing hook for NEEDSREPAIR

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 01:59:39PM -0500, Brian Foster wrote:
> On Tue, Feb 09, 2021 at 10:17:38AM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 09, 2021 at 12:21:31PM -0500, Brian Foster wrote:
> > > On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Simulate a crash when anyone calls force_needsrepair.  This is a debug
> > > > knob so that we can test that the kernel won't mount after setting
> > > > needsrepair and that a re-run of xfs_repair will clear the flag.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > 
> > > Can't we just use db to manually set the bit on the superblock?
> > 
> > No, because the fstest uses this debug knob to simulate the following:
> > 
> > 1) sysadmin issues 'xfs_admin -O inobtcount /dev/sda1'
> > 2) xfs_repair flips on INOBTCOUNT and NEEDSREPAIR
> > 3) system goes down and repair never completes
> > 4) verify that we can't mount
> > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs
> > 6) verify that mount works again
> > 
> 
> Ok, but that seems like circular reasoning.

I'm sorry, but I don't see how this is circular logic?

The test needs to show that NEEDSREPAIR is turned on during phase 1 (or
2) when we apply an upgrade, and it needs to induce some kind of early
exit so that the needsrepair clearing code after phase 7 does not run.

If we set NEEDSREPAIR with xfs_db before running repair then we have no
way to detect if the inobtcount upgrade doesn't set needsrepair.

If we don't have a debugging knob to stop repair before it reaches phase
7, we're not really testing a genuine early-repair-exit scenario.  Yes,
we can use xfs_db to manually set the flag after repair returns, but
that doesn't fill the testing gap above.

> It wouldn't be quite the
> same as a simulated repair failure, but ISTM that if we set the bit
> manually, we can still verify steps 4, 5 and 6 as is (with the caveat
> that the repair invocation performs a feature upgrade). I'm not sure how
> important it really is to verify that a feature upgrade sequence sets
> the bit if it happens to fail provided we have independent tests that 1.
> verify the needsrepair bit works as expected and 2. verify the feature
> upgrades work appropriately, since that is the primary functionality.
> 
> I wanted to think about that a little more before replying, but I also
> just realized something odd when digging into the debug code:
> 
> # ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch 
> Phase 1 - find and verify superblock...
> Marking filesystem in need of repair.
> writing modified primary superblock
> Phase 2 - using internal log
>         - zero log...
> ERROR: The filesystem has valuable metadata changes in a log which needs to
> ...
> # mount /dev/test/scratch /mnt/
> mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error.
> #
> 
> It looks like we can set a feature upgrade bit on the superblock before
> we've examined the log and potentially discovered that it's dirty (phase
> 2). If the log is recoverable, that puts the user in a bit of a bind..

Heh, funny that I was thinking that the upgrades shouldn't really be
happening in phase 1 anyway--

I've (separately) started working on a patch to make it so that you can
add reflink and finobt to a filesystem.  Those upgrades require somewhat
more intensive checks of the filesystem (such as checking free space in
each AG), so I ended up dumping them into phase 2, since the xfs_mount
and buffer cache aren't fully initialized until after phase 1.

So, yeah, the upgrade code should move to phase2() after log zeroing and
before the AG scan.

--D

> Brian
> 
> > and the other scenario is:
> > 
> > 1) fuzz a directory entry in such a way that repair will decide to
> >    blow out the dirent and rebuild the directory later
> > 2) sysadmin issues 'xfs_repair /dev/sda1'
> > 2) xfs_repair flips on NEEDSREPAIR at the same time it corrupts the
> >    dirent to trigger the rebuild later
> > 3) system goes down and repair never completes
> > 4) verify that we can't mount
> > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs
> > 6) verify that mount works again
> > 
> > Both cases reflect what I think are the most likely failure scenarios,
> > hence the knob needs to be in xfs_repair to prevent it from running to
> > completion.
> > 
> > (And yes, I've been recently very bad at sending fstests out for review
> > the past few months; I will get that done by this afternoon.)
> > 
> > --D
> > 
> > > Brian
> > > 
> > > >  repair/globals.c    |    1 +
> > > >  repair/globals.h    |    2 ++
> > > >  repair/phase1.c     |    5 +++++
> > > >  repair/xfs_repair.c |    7 +++++++
> > > >  4 files changed, 15 insertions(+)
> > > > 
> > > > 
> > > > diff --git a/repair/globals.c b/repair/globals.c
> > > > index 699a96ee..b0e23864 100644
> > > > --- a/repair/globals.c
> > > > +++ b/repair/globals.c
> > > > @@ -40,6 +40,7 @@ int	dangerously;		/* live dangerously ... fix ro mount */
> > > >  int	isa_file;
> > > >  int	zap_log;
> > > >  int	dumpcore;		/* abort, not exit on fatal errs */
> > > > +bool	abort_after_force_needsrepair;
> > > >  int	force_geo;		/* can set geo on low confidence info */
> > > >  int	assume_xfs;		/* assume we have an xfs fs */
> > > >  char	*log_name;		/* Name of log device */
> > > > diff --git a/repair/globals.h b/repair/globals.h
> > > > index 043b3e8e..9fa73b2c 100644
> > > > --- a/repair/globals.h
> > > > +++ b/repair/globals.h
> > > > @@ -82,6 +82,8 @@ extern int	isa_file;
> > > >  extern int	zap_log;
> > > >  extern int	dumpcore;		/* abort, not exit on fatal errs */
> > > >  extern int	force_geo;		/* can set geo on low confidence info */
> > > > +/* Abort after forcing NEEDSREPAIR to test its functionality */
> > > > +extern bool	abort_after_force_needsrepair;
> > > >  extern int	assume_xfs;		/* assume we have an xfs fs */
> > > >  extern char	*log_name;		/* Name of log device */
> > > >  extern int	log_spec;		/* Log dev specified as option */
> > > > diff --git a/repair/phase1.c b/repair/phase1.c
> > > > index b26d25f8..57f72cd0 100644
> > > > --- a/repair/phase1.c
> > > > +++ b/repair/phase1.c
> > > > @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n"));
> > > >  	 */
> > > >  	sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0;
> > > >  
> > > > +	/* Simulate a crash after setting needsrepair. */
> > > > +	if (primary_sb_modified && add_needsrepair &&
> > > > +	    abort_after_force_needsrepair)
> > > > +		exit(55);
> > > > +
> > > >  	free(sb);
> > > >  }
> > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > > index ee377e8a..ae7106a6 100644
> > > > --- a/repair/xfs_repair.c
> > > > +++ b/repair/xfs_repair.c
> > > > @@ -44,6 +44,7 @@ enum o_opt_nums {
> > > >  	BLOAD_LEAF_SLACK,
> > > >  	BLOAD_NODE_SLACK,
> > > >  	NOQUOTA,
> > > > +	FORCE_NEEDSREPAIR_ABORT,
> > > >  	O_MAX_OPTS,
> > > >  };
> > > >  
> > > > @@ -57,6 +58,7 @@ static char *o_opts[] = {
> > > >  	[BLOAD_LEAF_SLACK]	= "debug_bload_leaf_slack",
> > > >  	[BLOAD_NODE_SLACK]	= "debug_bload_node_slack",
> > > >  	[NOQUOTA]		= "noquota",
> > > > +	[FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort",
> > > >  	[O_MAX_OPTS]		= NULL,
> > > >  };
> > > >  
> > > > @@ -282,6 +284,9 @@ process_args(int argc, char **argv)
> > > >  		_("-o debug_bload_node_slack requires a parameter\n"));
> > > >  					bload_node_slack = (int)strtol(val, NULL, 0);
> > > >  					break;
> > > > +				case FORCE_NEEDSREPAIR_ABORT:
> > > > +					abort_after_force_needsrepair = true;
> > > > +					break;
> > > >  				case NOQUOTA:
> > > >  					quotacheck_skip();
> > > >  					break;
> > > > @@ -795,6 +800,8 @@ force_needsrepair(
> > > >  		error = -libxfs_bwrite(bp);
> > > >  		if (error)
> > > >  			do_log(_("couldn't force needsrepair, err=%d\n"), error);
> > > > +		if (abort_after_force_needsrepair)
> > > > +			exit(55);
> > > >  	}
> > > >  	if (bp)
> > > >  		libxfs_buf_relse(bp);
> > > > 
> > > 
> > 
> 

Re: [PATCH 09/10] xfs_repair: add a testing hook for NEEDSREPAIR

[Brian Foster]

On Tue, Feb 09, 2021 at 11:59:20AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 09, 2021 at 01:59:39PM -0500, Brian Foster wrote:
> > On Tue, Feb 09, 2021 at 10:17:38AM -0800, Darrick J. Wong wrote:
> > > On Tue, Feb 09, 2021 at 12:21:31PM -0500, Brian Foster wrote:
> > > > On Mon, Feb 08, 2021 at 08:10:55PM -0800, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > > 
> > > > > Simulate a crash when anyone calls force_needsrepair.  This is a debug
> > > > > knob so that we can test that the kernel won't mount after setting
> > > > > needsrepair and that a re-run of xfs_repair will clear the flag.
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > > ---
> > > > 
> > > > Can't we just use db to manually set the bit on the superblock?
> > > 
> > > No, because the fstest uses this debug knob to simulate the following:
> > > 
> > > 1) sysadmin issues 'xfs_admin -O inobtcount /dev/sda1'
> > > 2) xfs_repair flips on INOBTCOUNT and NEEDSREPAIR
> > > 3) system goes down and repair never completes
> > > 4) verify that we can't mount
> > > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs
> > > 6) verify that mount works again
> > > 
> > 
> > Ok, but that seems like circular reasoning.
> 
> I'm sorry, but I don't see how this is circular logic?
> 

I was just referring to the insinuation that we have to maintain such
debug logic because a test exists that wants it. Clearly that's why it
exists. ;)

> The test needs to show that NEEDSREPAIR is turned on during phase 1 (or
> 2) when we apply an upgrade, and it needs to induce some kind of early
> exit so that the needsrepair clearing code after phase 7 does not run.
> 
> If we set NEEDSREPAIR with xfs_db before running repair then we have no
> way to detect if the inobtcount upgrade doesn't set needsrepair.
> 
> If we don't have a debugging knob to stop repair before it reaches phase
> 7, we're not really testing a genuine early-repair-exit scenario.  Yes,
> we can use xfs_db to manually set the flag after repair returns, but
> that doesn't fill the testing gap above.
> 

But is it worth maintaining test specific debug logic in an application
just to confirm that particular feature bit upgrades actually set the
bit? It seems sufficient to me to test that needsrepair functionality
works as expected and that individual feature upgrade works as well.

Given the discussion on patch 7, perhaps it makes more sense to at least
defer this sort of injection mechanism until we have a scheme for
generic needsrepair usage worked out for xfs_repair? I am wondering if
there's a way to make repair fail without requiring additional code, but
if not and we do require some sort of injection mode, I suspect we might
end up better served by something more generic (i.e. capable of failures
at random points) rather than defining a command line option
specifically for a particular fstest..

Brian

> > It wouldn't be quite the
> > same as a simulated repair failure, but ISTM that if we set the bit
> > manually, we can still verify steps 4, 5 and 6 as is (with the caveat
> > that the repair invocation performs a feature upgrade). I'm not sure how
> > important it really is to verify that a feature upgrade sequence sets
> > the bit if it happens to fail provided we have independent tests that 1.
> > verify the needsrepair bit works as expected and 2. verify the feature
> > upgrades work appropriately, since that is the primary functionality.
> > 
> > I wanted to think about that a little more before replying, but I also
> > just realized something odd when digging into the debug code:
> > 
> > # ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch 
> > Phase 1 - find and verify superblock...
> > Marking filesystem in need of repair.
> > writing modified primary superblock
> > Phase 2 - using internal log
> >         - zero log...
> > ERROR: The filesystem has valuable metadata changes in a log which needs to
> > ...
> > # mount /dev/test/scratch /mnt/
> > mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error.
> > #
> > 
> > It looks like we can set a feature upgrade bit on the superblock before
> > we've examined the log and potentially discovered that it's dirty (phase
> > 2). If the log is recoverable, that puts the user in a bit of a bind..
> 
> Heh, funny that I was thinking that the upgrades shouldn't really be
> happening in phase 1 anyway--
> 
> I've (separately) started working on a patch to make it so that you can
> add reflink and finobt to a filesystem.  Those upgrades require somewhat
> more intensive checks of the filesystem (such as checking free space in
> each AG), so I ended up dumping them into phase 2, since the xfs_mount
> and buffer cache aren't fully initialized until after phase 1.
> 
> So, yeah, the upgrade code should move to phase2() after log zeroing and
> before the AG scan.
> 
> --D
> 
> > Brian
> > 
> > > and the other scenario is:
> > > 
> > > 1) fuzz a directory entry in such a way that repair will decide to
> > >    blow out the dirent and rebuild the directory later
> > > 2) sysadmin issues 'xfs_repair /dev/sda1'
> > > 2) xfs_repair flips on NEEDSREPAIR at the same time it corrupts the
> > >    dirent to trigger the rebuild later
> > > 3) system goes down and repair never completes
> > > 4) verify that we can't mount
> > > 5) verify that repair clears NEEDSREPAIR and gives us a clean fs
> > > 6) verify that mount works again
> > > 
> > > Both cases reflect what I think are the most likely failure scenarios,
> > > hence the knob needs to be in xfs_repair to prevent it from running to
> > > completion.
> > > 
> > > (And yes, I've been recently very bad at sending fstests out for review
> > > the past few months; I will get that done by this afternoon.)
> > > 
> > > --D
> > > 
> > > > Brian
> > > > 
> > > > >  repair/globals.c    |    1 +
> > > > >  repair/globals.h    |    2 ++
> > > > >  repair/phase1.c     |    5 +++++
> > > > >  repair/xfs_repair.c |    7 +++++++
> > > > >  4 files changed, 15 insertions(+)
> > > > > 
> > > > > 
> > > > > diff --git a/repair/globals.c b/repair/globals.c
> > > > > index 699a96ee..b0e23864 100644
> > > > > --- a/repair/globals.c
> > > > > +++ b/repair/globals.c
> > > > > @@ -40,6 +40,7 @@ int	dangerously;		/* live dangerously ... fix ro mount */
> > > > >  int	isa_file;
> > > > >  int	zap_log;
> > > > >  int	dumpcore;		/* abort, not exit on fatal errs */
> > > > > +bool	abort_after_force_needsrepair;
> > > > >  int	force_geo;		/* can set geo on low confidence info */
> > > > >  int	assume_xfs;		/* assume we have an xfs fs */
> > > > >  char	*log_name;		/* Name of log device */
> > > > > diff --git a/repair/globals.h b/repair/globals.h
> > > > > index 043b3e8e..9fa73b2c 100644
> > > > > --- a/repair/globals.h
> > > > > +++ b/repair/globals.h
> > > > > @@ -82,6 +82,8 @@ extern int	isa_file;
> > > > >  extern int	zap_log;
> > > > >  extern int	dumpcore;		/* abort, not exit on fatal errs */
> > > > >  extern int	force_geo;		/* can set geo on low confidence info */
> > > > > +/* Abort after forcing NEEDSREPAIR to test its functionality */
> > > > > +extern bool	abort_after_force_needsrepair;
> > > > >  extern int	assume_xfs;		/* assume we have an xfs fs */
> > > > >  extern char	*log_name;		/* Name of log device */
> > > > >  extern int	log_spec;		/* Log dev specified as option */
> > > > > diff --git a/repair/phase1.c b/repair/phase1.c
> > > > > index b26d25f8..57f72cd0 100644
> > > > > --- a/repair/phase1.c
> > > > > +++ b/repair/phase1.c
> > > > > @@ -170,5 +170,10 @@ _("Cannot disable lazy-counters on V5 fs\n"));
> > > > >  	 */
> > > > >  	sb_ifree = sb_icount = sb_fdblocks = sb_frextents = 0;
> > > > >  
> > > > > +	/* Simulate a crash after setting needsrepair. */
> > > > > +	if (primary_sb_modified && add_needsrepair &&
> > > > > +	    abort_after_force_needsrepair)
> > > > > +		exit(55);
> > > > > +
> > > > >  	free(sb);
> > > > >  }
> > > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > > > index ee377e8a..ae7106a6 100644
> > > > > --- a/repair/xfs_repair.c
> > > > > +++ b/repair/xfs_repair.c
> > > > > @@ -44,6 +44,7 @@ enum o_opt_nums {
> > > > >  	BLOAD_LEAF_SLACK,
> > > > >  	BLOAD_NODE_SLACK,
> > > > >  	NOQUOTA,
> > > > > +	FORCE_NEEDSREPAIR_ABORT,
> > > > >  	O_MAX_OPTS,
> > > > >  };
> > > > >  
> > > > > @@ -57,6 +58,7 @@ static char *o_opts[] = {
> > > > >  	[BLOAD_LEAF_SLACK]	= "debug_bload_leaf_slack",
> > > > >  	[BLOAD_NODE_SLACK]	= "debug_bload_node_slack",
> > > > >  	[NOQUOTA]		= "noquota",
> > > > > +	[FORCE_NEEDSREPAIR_ABORT] = "debug_force_needsrepair_abort",
> > > > >  	[O_MAX_OPTS]		= NULL,
> > > > >  };
> > > > >  
> > > > > @@ -282,6 +284,9 @@ process_args(int argc, char **argv)
> > > > >  		_("-o debug_bload_node_slack requires a parameter\n"));
> > > > >  					bload_node_slack = (int)strtol(val, NULL, 0);
> > > > >  					break;
> > > > > +				case FORCE_NEEDSREPAIR_ABORT:
> > > > > +					abort_after_force_needsrepair = true;
> > > > > +					break;
> > > > >  				case NOQUOTA:
> > > > >  					quotacheck_skip();
> > > > >  					break;
> > > > > @@ -795,6 +800,8 @@ force_needsrepair(
> > > > >  		error = -libxfs_bwrite(bp);
> > > > >  		if (error)
> > > > >  			do_log(_("couldn't force needsrepair, err=%d\n"), error);
> > > > > +		if (abort_after_force_needsrepair)
> > > > > +			exit(55);
> > > > >  	}
> > > > >  	if (bp)
> > > > >  		libxfs_buf_relse(bp);
> > > > > 
> > > > 
> > > 
> > 
> 

Re: [PATCH 09/10] xfs_repair: add a testing hook for NEEDSREPAIR

[Eric Sandeen]

On 2/9/21 1:59 PM, Darrick J. Wong wrote:
>> # ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch 
>> Phase 1 - find and verify superblock...
>> Marking filesystem in need of repair.
>> writing modified primary superblock
>> Phase 2 - using internal log
>>         - zero log...
>> ERROR: The filesystem has valuable metadata changes in a log which needs to
>> ...
>> # mount /dev/test/scratch /mnt/
>> mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error.
>> #
>>
>> It looks like we can set a feature upgrade bit on the superblock before
>> we've examined the log and potentially discovered that it's dirty (phase
>> 2). If the log is recoverable, that puts the user in a bit of a bind..
> Heh, funny that I was thinking that the upgrades shouldn't really be
> happening in phase 1 anyway--
> 
> I've (separately) started working on a patch to make it so that you can
> add reflink and finobt to a filesystem.  Those upgrades require somewhat
> more intensive checks of the filesystem (such as checking free space in
> each AG), so I ended up dumping them into phase 2, since the xfs_mount
> and buffer cache aren't fully initialized until after phase 1.
> 
> So, yeah, the upgrade code should move to phase2() after log zeroing and
> before the AG scan.

Oh, whoops - 

based on this, I think that the prior patch
[PATCH 08/10] xfs_repair: allow setting the needsrepair flag
needs to be adjusted as well, right; we don't want to set it in phase 1
or we might set it then abort on a dirty log and the user is stuck.

So maybe I should merge up through 

[PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories

and hold off on the rest? Or should I just hold off on the series and let
you reassemble (again?)

(Am I right that the upgrade bits will need to be moved and the series resent?)

-Eric

Re: [PATCH 09/10] xfs_repair: add a testing hook for NEEDSREPAIR

[Darrick J. Wong]

On Wed, Feb 10, 2021 at 03:41:52PM -0600, Eric Sandeen wrote:
> On 2/9/21 1:59 PM, Darrick J. Wong wrote:
> >> # ./repair/xfs_repair -c needsrepair=1 /dev/test/scratch 
> >> Phase 1 - find and verify superblock...
> >> Marking filesystem in need of repair.
> >> writing modified primary superblock
> >> Phase 2 - using internal log
> >>         - zero log...
> >> ERROR: The filesystem has valuable metadata changes in a log which needs to
> >> ...
> >> # mount /dev/test/scratch /mnt/
> >> mount: /mnt: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch, missing codepage or helper program, or other error.
> >> #
> >>
> >> It looks like we can set a feature upgrade bit on the superblock before
> >> we've examined the log and potentially discovered that it's dirty (phase
> >> 2). If the log is recoverable, that puts the user in a bit of a bind..
> > Heh, funny that I was thinking that the upgrades shouldn't really be
> > happening in phase 1 anyway--
> > 
> > I've (separately) started working on a patch to make it so that you can
> > add reflink and finobt to a filesystem.  Those upgrades require somewhat
> > more intensive checks of the filesystem (such as checking free space in
> > each AG), so I ended up dumping them into phase 2, since the xfs_mount
> > and buffer cache aren't fully initialized until after phase 1.
> > 
> > So, yeah, the upgrade code should move to phase2() after log zeroing and
> > before the AG scan.
> 
> Oh, whoops - 
> 
> based on this, I think that the prior patch
> [PATCH 08/10] xfs_repair: allow setting the needsrepair flag
> needs to be adjusted as well, right; we don't want to set it in phase 1
> or we might set it then abort on a dirty log and the user is stuck.
> 
> So maybe I should merge up through 
> 
> [PATCH 07/10] xfs_repair: set NEEDSREPAIR when we deliberately corrupt directories
> 
> and hold off on the rest? Or should I just hold off on the series and let
> you reassemble (again?)

Yeah, I'll push out v5 tomorrow morning with all this cleaned up.

--D

> (Am I right that the upgrade bits will need to be moved and the series resent?)
> 
> -Eric

[PATCH 10/10] xfs_admin: support adding features to V5 filesystems

[Darrick J. Wong]

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

Teach the xfs_admin script how to add features to V5 filesystems.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 db/xfs_admin.sh      |    6 ++++--
 man/man8/xfs_admin.8 |   22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)


diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
index 430872ef..7a467dbe 100755
--- a/db/xfs_admin.sh
+++ b/db/xfs_admin.sh
@@ -8,9 +8,10 @@ status=0
 DB_OPTS=""
 REPAIR_OPTS=""
 REPAIR_DEV_OPTS=""
-USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-r rtdev] [-U uuid] device [logdev]"
+DB_LOG_OPTS=""
+USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-O v5_feature] [-r rtdev] [-U uuid] device [logdev]"
 
-while getopts "c:efjlL:pr:uU:V" c
+while getopts "c:efjlL:O:pr:uU:V" c
 do
 	case $c in
 	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
@@ -19,6 +20,7 @@ do
 	j)	DB_OPTS=$DB_OPTS" -c 'version log2'";;
 	l)	DB_OPTS=$DB_OPTS" -r -c label";;
 	L)	DB_OPTS=$DB_OPTS" -c 'label "$OPTARG"'";;
+	O)	REPAIR_OPTS=$REPAIR_OPTS" -c $OPTARG=1";;
 	p)	DB_OPTS=$DB_OPTS" -c 'version projid32bit'";;
 	r)	REPAIR_DEV_OPTS=" -r '$OPTARG'";;
 	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
index cccbb224..3f3aeea8 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 " featurelist"
+] [
 .BR "\-c 0" | 1
 ] [
 .B \-L
@@ -106,6 +108,26 @@ The filesystem label can be cleared using the special "\c
 " value for
 .IR label .
 .TP
+.BI \-O " feature1" = "status" , "feature2" = "status..."
+Add or remove features on an existing a V5 filesystem.
+The features should be specified as a comma-separated list.
+.I status
+should be either 0 to disable the feature or 1 to enable the feature.
+Note, however, that most features cannot be disabled.
+.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
+There are currently no feature options.
+.TP
 .BI \-U " uuid"
 Set the UUID of the filesystem to
 .IR uuid .

Re: [PATCH 10/10] xfs_admin: support adding features to V5 filesystems

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 08:11:00PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Teach the xfs_admin script how to add features to V5 filesystems.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

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

Re: [PATCH 10/10] xfs_admin: support adding features to V5 filesystems

[Brian Foster]

On Mon, Feb 08, 2021 at 08:11:00PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Teach the xfs_admin script how to add features to V5 filesystems.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  db/xfs_admin.sh      |    6 ++++--
>  man/man8/xfs_admin.8 |   22 ++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
> index 430872ef..7a467dbe 100755
> --- a/db/xfs_admin.sh
> +++ b/db/xfs_admin.sh
> @@ -8,9 +8,10 @@ status=0
>  DB_OPTS=""
>  REPAIR_OPTS=""
>  REPAIR_DEV_OPTS=""
> -USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-r rtdev] [-U uuid] device [logdev]"
> +DB_LOG_OPTS=""
> +USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-O v5_feature] [-r rtdev] [-U uuid] device [logdev]"

Technically this would pass through the lazy counter variant on a v4
super, right? I wonder if we should just call it "[-O feature]" here.

>  
> -while getopts "c:efjlL:pr:uU:V" c
> +while getopts "c:efjlL:O:pr:uU:V" c
>  do
>  	case $c in
>  	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
...
> diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
> index cccbb224..3f3aeea8 100644
> --- a/man/man8/xfs_admin.8
> +++ b/man/man8/xfs_admin.8
...
> @@ -106,6 +108,26 @@ The filesystem label can be cleared using the special "\c
>  " value for
>  .IR label .
>  .TP
> +.BI \-O " feature1" = "status" , "feature2" = "status..."
> +Add or remove features on an existing a V5 filesystem.

s/existing a/existing/

Also, similar question around the lazycount variant. If it works, should
it not be documented here?

Brian

> +The features should be specified as a comma-separated list.
> +.I status
> +should be either 0 to disable the feature or 1 to enable the feature.
> +Note, however, that most features cannot be disabled.
> +.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
> +There are currently no feature options.
> +.TP
>  .BI \-U " uuid"
>  Set the UUID of the filesystem to
>  .IR uuid .
> 

Re: [PATCH 10/10] xfs_admin: support adding features to V5 filesystems

[Darrick J. Wong]

On Tue, Feb 09, 2021 at 12:22:04PM -0500, Brian Foster wrote:
> On Mon, Feb 08, 2021 at 08:11:00PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Teach the xfs_admin script how to add features to V5 filesystems.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  db/xfs_admin.sh      |    6 ++++--
> >  man/man8/xfs_admin.8 |   22 ++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
> > index 430872ef..7a467dbe 100755
> > --- a/db/xfs_admin.sh
> > +++ b/db/xfs_admin.sh
> > @@ -8,9 +8,10 @@ status=0
> >  DB_OPTS=""
> >  REPAIR_OPTS=""
> >  REPAIR_DEV_OPTS=""
> > -USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-r rtdev] [-U uuid] device [logdev]"
> > +DB_LOG_OPTS=""
> > +USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-O v5_feature] [-r rtdev] [-U uuid] device [logdev]"
> 
> Technically this would pass through the lazy counter variant on a v4
> super, right? I wonder if we should just call it "[-O feature]" here.
> 
> >  
> > -while getopts "c:efjlL:pr:uU:V" c
> > +while getopts "c:efjlL:O:pr:uU:V" c
> >  do
> >  	case $c in
> >  	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
> ...
> > diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
> > index cccbb224..3f3aeea8 100644
> > --- a/man/man8/xfs_admin.8
> > +++ b/man/man8/xfs_admin.8
> ...
> > @@ -106,6 +108,26 @@ The filesystem label can be cleared using the special "\c
> >  " value for
> >  .IR label .
> >  .TP
> > +.BI \-O " feature1" = "status" , "feature2" = "status..."
> > +Add or remove features on an existing a V5 filesystem.
> 
> s/existing a/existing/

Fixed, thanks.

> Also, similar question around the lazycount variant. If it works, should
> it not be documented here?

It would work, but I'd rather stick to the existing narrative that
"xfs_admin -c 0|1" is how one changes the lazycount feature.  Mentioning
both avenues in the manpage introduces the potential for confusion as to
whether one is particularly better than the other.

--D

> Brian
> 
> > +The features should be specified as a comma-separated list.
> > +.I status
> > +should be either 0 to disable the feature or 1 to enable the feature.
> > +Note, however, that most features cannot be disabled.
> > +.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
> > +There are currently no feature options.
> > +.TP
> >  .BI \-U " uuid"
> >  Set the UUID of the filesystem to
> >  .IR uuid .
> > 
>