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

[PATCHSET v5 00/11] 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
v5: various fixes suggested by reviewers, and document deprecated V4 options

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        |   20 +++++++++-----
 man/man8/mkfs.xfs.8  |   16 +++++++++++
 man/man8/xfs_admin.8 |   40 +++++++++++++++++++++++++++++
 repair/agheader.c    |   21 +++++++++++++++
 repair/globals.c     |    3 ++
 repair/globals.h     |    4 +++
 repair/phase2.c      |   67 ++++++++++++++++++++++++++++++++++++++++++++++++
 repair/xfs_repair.c  |   70 ++++++++++++++++++++++++++++++++++++++++++++------
 12 files changed, 253 insertions(+), 22 deletions(-)

[PATCH 01/11] xfs_admin: clean up string quoting

[Darrick J. Wong]

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

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

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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/11] xfs_admin: clean up string quoting

[Chaitanya Kulkarni]

On 2/11/21 15:00, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Clean up the string quoting in this script so that we don't trip over
> users feeding us arguments like "/dev/sd ha ha ha lol".
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

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

[Darrick J. Wong]

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

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

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 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/11] xfs_admin: support filesystems with realtime devices

[Chaitanya Kulkarni]

On 2/11/21 15:00, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Add a -r option to xfs_admin so that we can pass the name of the
> realtime device to xfs_repair.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[PATCH 03/11] xfs_db: report the needsrepair flag in check and version commands

[Darrick J. Wong]

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

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

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


diff --git a/db/check.c b/db/check.c
index 33736e33..485e855e 100644
--- a/db/check.c
+++ b/db/check.c
@@ -3970,6 +3970,11 @@ scan_ag(
 			dbprintf(_("mkfs not completed successfully\n"));
 		error++;
 	}
+	if (xfs_sb_version_needsrepair(sb)) {
+		if (!sflag)
+			dbprintf(_("filesystem needs xfs_repair\n"));
+		error++;
+	}
 	set_dbmap(agno, XFS_SB_BLOCK(mp), 1, DBM_SB, agno, XFS_SB_BLOCK(mp));
 	if (sb->sb_logstart && XFS_FSB_TO_AGNO(mp, sb->sb_logstart) == agno)
 		set_dbmap(agno, XFS_FSB_TO_AGBNO(mp, sb->sb_logstart),
diff --git a/db/sb.c b/db/sb.c
index d09f653d..d7111e92 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -691,6 +691,8 @@ version_string(
 		strcat(s, ",INOBTCNT");
 	if (xfs_sb_version_hasbigtime(sbp))
 		strcat(s, ",BIGTIME");
+	if (xfs_sb_version_needsrepair(sbp))
+		strcat(s, ",NEEDSREPAIR");
 	return s;
 }
 

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

[Darrick J. Wong]

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

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

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


diff --git a/db/sb.c b/db/sb.c
index d7111e92..cec7dce9 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -379,6 +379,11 @@ uuid_f(
 				progname);
 			return 0;
 		}
+		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
+			dbprintf(_("%s: filesystem needs xfs_repair\n"),
+				progname);
+			return 0;
+		}
 
 		if (!strcasecmp(argv[1], "generate")) {
 			platform_uuid_generate(&uu);
@@ -543,6 +548,12 @@ label_f(
 			return 0;
 		}
 
+		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
+			dbprintf(_("%s: filesystem needs xfs_repair\n"),
+				progname);
+			return 0;
+		}
+
 		dbprintf(_("writing all SBs\n"));
 		for (ag = 0; ag < mp->m_sb.sb_agcount; ag++)
 			if ((p = do_label(ag, argv[1])) == NULL) {

[PATCH 05/11] xfs_repair: fix unmount error message to have a newline

[Darrick J. Wong]

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

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

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 05/11] xfs_repair: fix unmount error message to have a newline

[Chaitanya Kulkarni]

On 2/11/21 15:00, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Add a newline so that this is consistent with the other error messages.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[PATCH 06/11] xfs_repair: clear quota CHKD flags on the incore superblock too

[Darrick J. Wong]

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

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

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

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

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


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

[PATCH 07/11] xfs_repair: clear the needsrepair flag

[Darrick J. Wong]

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

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

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

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 include/xfs_mount.h |    1 +
 libxfs/init.c       |   20 +++++++++++++-------
 repair/agheader.c   |   21 +++++++++++++++++++++
 repair/xfs_repair.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 77 insertions(+), 7 deletions(-)


diff --git a/include/xfs_mount.h b/include/xfs_mount.h
index 36594643..75230ca5 100644
--- a/include/xfs_mount.h
+++ b/include/xfs_mount.h
@@ -181,6 +181,7 @@ xfs_perag_resv(
 
 extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
 				dev_t, dev_t, dev_t, int);
+int libxfs_flush_mount(struct xfs_mount *mp);
 int		libxfs_umount(struct xfs_mount *mp);
 extern void	libxfs_rtmount_destroy (xfs_mount_t *);
 
diff --git a/libxfs/init.c b/libxfs/init.c
index 9fe13b8d..8a8ce3c4 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -870,7 +870,7 @@ _("%s: Flushing the %s failed, err=%d!\n"),
  * Flush all dirty buffers to stable storage and report on writes that didn't
  * make it to stable storage.
  */
-static int
+int
 libxfs_flush_mount(
 	struct xfs_mount	*mp)
 {
@@ -878,13 +878,13 @@ libxfs_flush_mount(
 	int			err2;
 
 	/*
-	 * Purge the buffer cache to write all dirty buffers to disk and free
-	 * all incore buffers.  Buffers that fail write verification will cause
-	 * the CORRUPT_WRITE flag to be set in the buftarg.  Buffers that
-	 * cannot be written will cause the LOST_WRITE flag to be set in the
-	 * buftarg.
+	 * Flush the buffer cache to write all dirty buffers to disk.  Buffers
+	 * that fail write verification will cause the CORRUPT_WRITE flag to be
+	 * set in the buftarg.  Buffers that cannot be written will cause the
+	 * LOST_WRITE flag to be set in the buftarg.  Once that's done,
+	 * instruct the disks to persist their write caches.
 	 */
-	libxfs_bcache_purge();
+	libxfs_bcache_flush();
 
 	/* Flush all kernel and disk write caches, and report failures. */
 	if (mp->m_ddev_targp) {
@@ -923,6 +923,12 @@ libxfs_umount(
 
 	libxfs_rtmount_destroy(mp);
 
+	/*
+	 * Purge the buffer cache to write all dirty buffers to disk and free
+	 * all incore buffers, then pick up the outcome when we tell the disks
+	 * to persist their write caches.
+	 */
+	libxfs_bcache_purge();
 	error = libxfs_flush_mount(mp);
 
 	for (agno = 0; agno < mp->m_maxagi; agno++) {
diff --git a/repair/agheader.c b/repair/agheader.c
index 8bb99489..2af24106 100644
--- a/repair/agheader.c
+++ b/repair/agheader.c
@@ -452,6 +452,27 @@ secondary_sb_whack(
 			rval |= XR_AG_SB_SEC;
 	}
 
+	if (xfs_sb_version_needsrepair(sb)) {
+		if (i == 0) {
+			if (!no_modify)
+				do_warn(
+	_("clearing needsrepair flag and regenerating metadata\n"));
+			else
+				do_warn(
+	_("would clear needsrepair flag and regenerate metadata\n"));
+		} else {
+			/*
+			 * Quietly clear needsrepair on the secondary supers as
+			 * part of ensuring them.  If needsrepair is set on the
+			 * primary, it will be cleared at the end of repair
+			 * once we've flushed all other dirty blocks to disk.
+			 */
+			sb->sb_features_incompat &=
+					~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+			rval |= XR_AG_SB_SEC;
+		}
+	}
+
 	return(rval);
 }
 
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 40352458..90d1a95a 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -712,6 +712,45 @@ check_fs_vs_host_sectsize(
 	}
 }
 
+/* Clear needsrepair after a successful repair run. */
+void
+clear_needsrepair(
+	struct xfs_mount	*mp)
+{
+	struct xfs_buf		*bp;
+	int			error;
+
+	/*
+	 * If we're going to clear NEEDSREPAIR, we need to make absolutely sure
+	 * that everything is ok with the ondisk filesystem.  Make sure any
+	 * dirty buffers are sent to disk and that the disks have persisted
+	 * writes to stable storage.  If that fails, leave NEEDSREPAIR in
+	 * place.  Don't purge the buffer cache here since we're not done yet.
+	 */
+	error = -libxfs_flush_mount(mp);
+	if (error) {
+		do_warn(
+	_("Cannot clear needsrepair due to flush failure, err=%d.\n"),
+			error);
+		return;
+	}
+
+	/* Clear needsrepair from the superblock. */
+	bp = libxfs_getsb(mp);
+	if (!bp || bp->b_error) {
+		do_warn(
+	_("Cannot clear needsrepair from primary super, err=%d.\n"),
+			bp ? bp->b_error : ENOMEM);
+	} else {
+		mp->m_sb.sb_features_incompat &=
+				~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+		libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
+		libxfs_buf_mark_dirty(bp);
+	}
+	if (bp)
+		libxfs_buf_relse(bp);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -1128,6 +1167,9 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
 	libxfs_bcache_flush();
 	format_log_max_lsn(mp);
 
+	if (xfs_sb_version_needsrepair(&mp->m_sb))
+		clear_needsrepair(mp);
+
 	/* Report failure if anything failed to get written to our fs. */
 	error = -libxfs_umount(mp);
 	if (error)

[PATCH 08/11] 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.  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.

Note that we can't do any of these upgrades until we've at least done a
preliminary scan of the primary super and the log.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 repair/globals.c    |    2 ++
 repair/globals.h    |    2 ++
 repair/phase2.c     |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 repair/xfs_repair.c |    9 +++++++
 4 files changed, 76 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/phase2.c b/repair/phase2.c
index 952ac4a5..9a8d42e1 100644
--- a/repair/phase2.c
+++ b/repair/phase2.c
@@ -131,6 +131,63 @@ zero_log(
 		libxfs_max_lsn = log->l_last_sync_lsn;
 }
 
+static bool
+set_needsrepair(
+	struct xfs_mount	*mp)
+{
+	if (!xfs_sb_version_hascrc(&mp->m_sb)) {
+		printf(
+	_("needsrepair flag only supported on V5 filesystems.\n"));
+		exit(0);
+	}
+
+	if (xfs_sb_version_needsrepair(&mp->m_sb)) {
+		printf(_("Filesystem already marked as needing repair.\n"));
+		exit(0);
+	}
+
+	printf(_("Marking filesystem in need of repair.\n"));
+	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+	return true;
+}
+
+/* Perform the user's requested upgrades on filesystem. */
+static void
+upgrade_filesystem(
+	struct xfs_mount	*mp)
+{
+	struct xfs_buf		*bp;
+	bool			dirty = false;
+	int			error;
+
+	if (add_needsrepair)
+		dirty |= set_needsrepair(mp);
+
+        if (no_modify || !dirty)
+                return;
+
+        bp = libxfs_getsb(mp);
+        if (!bp || bp->b_error) {
+                do_error(
+	_("couldn't get superblock for feature upgrade, err=%d\n"),
+                                bp ? bp->b_error : ENOMEM);
+        } else {
+                libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
+
+                /*
+		 * Write the primary super to disk immediately so that
+		 * needsrepair will be set if repair doesn't complete.
+		 */
+                error = -libxfs_bwrite(bp);
+                if (error)
+                        do_error(
+	_("filesystem feature upgrade failed, err=%d\n"),
+                                        error);
+        }
+        if (bp)
+                libxfs_buf_relse(bp);
+}
+
 /*
  * ok, at this point, the fs is mounted but the root inode may be
  * trashed and the ag headers haven't been checked.  So we have
@@ -235,4 +292,10 @@ phase2(
 				do_warn(_("would correct\n"));
 		}
 	}
+
+	/*
+	 * Upgrade the filesystem now that we've done a preliminary check of
+	 * the superblocks, the AGs, the log, and the metadata inodes.
+	 */
+	upgrade_filesystem(mp);
 }
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 90d1a95a..a613505f 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/11] xfs_repair: allow setting the needsrepair flag

[Eric Sandeen]

On 2/11/21 4:59 PM, 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.  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.
> 
> Note that we can't do any of these upgrades until we've at least done a
> preliminary scan of the primary super and the log.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>


I'm still a little on the fence about the cmdline option for crashing repair at a certain point from the POV that Brian kind of pointed out that this doesn't exactly scale as we need more hooks.

but

ehhhh it's a test-only undocumented option and I guess we could change it later if desired

we do have other debug options on the commandline already as well....


> ---
>  repair/globals.c    |    2 ++
>  repair/globals.h    |    2 ++
>  repair/phase2.c     |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  repair/xfs_repair.c |    9 +++++++
>  4 files changed, 76 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/phase2.c b/repair/phase2.c
> index 952ac4a5..9a8d42e1 100644
> --- a/repair/phase2.c
> +++ b/repair/phase2.c
> @@ -131,6 +131,63 @@ zero_log(
>  		libxfs_max_lsn = log->l_last_sync_lsn;
>  }
>  
> +static bool
> +set_needsrepair(
> +	struct xfs_mount	*mp)
> +{
> +	if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> +		printf(
> +	_("needsrepair flag only supported on V5 filesystems.\n"));
> +		exit(0);
> +	}
> +
> +	if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> +		printf(_("Filesystem already marked as needing repair.\n"));
> +		exit(0);
> +	}
> +
> +	printf(_("Marking filesystem in need of repair.\n"));
> +	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> +	return true;
> +}
> +
> +/* Perform the user's requested upgrades on filesystem. */
> +static void
> +upgrade_filesystem(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_buf		*bp;
> +	bool			dirty = false;
> +	int			error;
> +
> +	if (add_needsrepair)
> +		dirty |= set_needsrepair(mp);
> +
> +        if (no_modify || !dirty)
> +                return;
> +
> +        bp = libxfs_getsb(mp);
> +        if (!bp || bp->b_error) {
> +                do_error(
> +	_("couldn't get superblock for feature upgrade, err=%d\n"),
> +                                bp ? bp->b_error : ENOMEM);
> +        } else {
> +                libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> +
> +                /*
> +		 * Write the primary super to disk immediately so that
> +		 * needsrepair will be set if repair doesn't complete.
> +		 */
> +                error = -libxfs_bwrite(bp);
> +                if (error)
> +                        do_error(
> +	_("filesystem feature upgrade failed, err=%d\n"),
> +                                        error);
> +        }
> +        if (bp)
> +                libxfs_buf_relse(bp);
> +}
> +
>  /*
>   * ok, at this point, the fs is mounted but the root inode may be
>   * trashed and the ag headers haven't been checked.  So we have
> @@ -235,4 +292,10 @@ phase2(
>  				do_warn(_("would correct\n"));
>  		}
>  	}
> +
> +	/*
> +	 * Upgrade the filesystem now that we've done a preliminary check of
> +	 * the superblocks, the AGs, the log, and the metadata inodes.
> +	 */
> +	upgrade_filesystem(mp);
>  }
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 90d1a95a..a613505f 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/11] xfs_repair: allow setting the needsrepair flag

[Darrick J. Wong]

On Thu, Feb 11, 2021 at 05:29:05PM -0600, Eric Sandeen wrote:
> On 2/11/21 4:59 PM, 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.  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.
> > 
> > Note that we can't do any of these upgrades until we've at least done a
> > preliminary scan of the primary super and the log.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> 
> I'm still a little on the fence about the cmdline option for crashing
> repair at a certain point from the POV that Brian kind of pointed out
> that this doesn't exactly scale as we need more hooks.

(That's in the next patch.)

> but
> 
> ehhhh it's a test-only undocumented option and I guess we could change
> it later if desired
> 
> we do have other debug options on the commandline already as well....

I don't mind moving the debugging hooks to be seekrit environment
variables or something, but I don't think I've quite addressed some of
Brian's comments from last time:

[paste in stuff Brian said]

> But is it worth maintaining test specific debug logic in an
> application just to confirm that particular feature bit upgrades
> actually set the bit?

I argue that yes, this is important enough to burn a debugging knob.
The sequence that I think we should prevent through testing is the one
where we've set the new feature on the primary super but we haven't
finished generating whatever new metadata is needed to complete the
upgrade, the system crashes, and on remount the verifiers explode.

Chances are pretty good that we'll get an angry bug report on the
mailing list: "I upgraded my fs, the power went down, and the kernel
sprayed corruption everywhere!"  If we get a customer escalation like
this, I'd /much/ rather it be about not being able to mount right after
the reboot than a latent corruption that grows unseen until somebody's
filesystem loses data.

If a future patch to repair accidentally breaks the behavior where we
set NEEDSREPAIR at the same time as we set the new feature and flush the
super to disk, we cannot tell that there's been a regression in this
safety mechanism just by looking at the output of an otherwise
successful xfs_repair run...

> It seems sufficient to me to test that needsrepair functionality works
> as expected and that individual feature upgrade works as well.

...so in other words, we need some point to inject an error to make sure
that the upgrade interlock is correct.

> 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'm in the midst of prototyping what I said in the last thread --
hooking the buffe cache so that repair can catch the first time we
actually write anything to the filesystem, and using that to set
NEEDSREPAIR.  I've not run it through full fstests yet, but AFAICT I can
keep using the same tests and the same injection knobs I already wrote.

> 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..

Probably yes, but ... uh I don't want this to drag on into building a
generic error injection framework for userspace.

I would /really/ like to get inobtcount/bigtime tests into the kernel
without a giant detour they have nearly zero test coverage from the
wider community.

--D

> 
> > ---
> >  repair/globals.c    |    2 ++
> >  repair/globals.h    |    2 ++
> >  repair/phase2.c     |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  repair/xfs_repair.c |    9 +++++++
> >  4 files changed, 76 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/phase2.c b/repair/phase2.c
> > index 952ac4a5..9a8d42e1 100644
> > --- a/repair/phase2.c
> > +++ b/repair/phase2.c
> > @@ -131,6 +131,63 @@ zero_log(
> >  		libxfs_max_lsn = log->l_last_sync_lsn;
> >  }
> >  
> > +static bool
> > +set_needsrepair(
> > +	struct xfs_mount	*mp)
> > +{
> > +	if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> > +		printf(
> > +	_("needsrepair flag only supported on V5 filesystems.\n"));
> > +		exit(0);
> > +	}
> > +
> > +	if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> > +		printf(_("Filesystem already marked as needing repair.\n"));
> > +		exit(0);
> > +	}
> > +
> > +	printf(_("Marking filesystem in need of repair.\n"));
> > +	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > +	return true;
> > +}
> > +
> > +/* Perform the user's requested upgrades on filesystem. */
> > +static void
> > +upgrade_filesystem(
> > +	struct xfs_mount	*mp)
> > +{
> > +	struct xfs_buf		*bp;
> > +	bool			dirty = false;
> > +	int			error;
> > +
> > +	if (add_needsrepair)
> > +		dirty |= set_needsrepair(mp);
> > +
> > +        if (no_modify || !dirty)
> > +                return;
> > +
> > +        bp = libxfs_getsb(mp);
> > +        if (!bp || bp->b_error) {
> > +                do_error(
> > +	_("couldn't get superblock for feature upgrade, err=%d\n"),
> > +                                bp ? bp->b_error : ENOMEM);
> > +        } else {
> > +                libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > +
> > +                /*
> > +		 * Write the primary super to disk immediately so that
> > +		 * needsrepair will be set if repair doesn't complete.
> > +		 */
> > +                error = -libxfs_bwrite(bp);
> > +                if (error)
> > +                        do_error(
> > +	_("filesystem feature upgrade failed, err=%d\n"),
> > +                                        error);
> > +        }
> > +        if (bp)
> > +                libxfs_buf_relse(bp);
> > +}
> > +
> >  /*
> >   * ok, at this point, the fs is mounted but the root inode may be
> >   * trashed and the ag headers haven't been checked.  So we have
> > @@ -235,4 +292,10 @@ phase2(
> >  				do_warn(_("would correct\n"));
> >  		}
> >  	}
> > +
> > +	/*
> > +	 * Upgrade the filesystem now that we've done a preliminary check of
> > +	 * the superblocks, the AGs, the log, and the metadata inodes.
> > +	 */
> > +	upgrade_filesystem(mp);
> >  }
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 90d1a95a..a613505f 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/11] xfs_repair: allow setting the needsrepair flag

[Eric Sandeen]

On 2/11/21 6:17 PM, Darrick J. Wong wrote:
> On Thu, Feb 11, 2021 at 05:29:05PM -0600, Eric Sandeen wrote:
>> On 2/11/21 4:59 PM, 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.  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.
>>>
>>> Note that we can't do any of these upgrades until we've at least done a
>>> preliminary scan of the primary super and the log.
>>>
>>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>>
>>
>> I'm still a little on the fence about the cmdline option for crashing
>> repair at a certain point from the POV that Brian kind of pointed out
>> that this doesn't exactly scale as we need more hooks.
> 
> (That's in the next patch.)

I. Am. Awesome.
 
...

> Probably yes, but ... uh I don't want this to drag on into building a
> generic error injection framework for userspace.
> 
> I would /really/ like to get inobtcount/bigtime tests into the kernel
> without a giant detour they have nearly zero test coverage from the
> wider community.

Yeah, I dont' want that either.

this (er, next patch) is s3kr1t and if we have something better later we
can change it.  I'll just merge stuff as-is and move forward.

-Eric

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

[Darrick J. Wong]

On Thu, Feb 11, 2021 at 06:20:23PM -0600, Eric Sandeen wrote:
> 
> 
> On 2/11/21 6:17 PM, Darrick J. Wong wrote:
> > On Thu, Feb 11, 2021 at 05:29:05PM -0600, Eric Sandeen wrote:
> >> On 2/11/21 4:59 PM, 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.  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.
> >>>
> >>> Note that we can't do any of these upgrades until we've at least done a
> >>> preliminary scan of the primary super and the log.
> >>>
> >>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> >>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >>> Reviewed-by: Brian Foster <bfoster@redhat.com>
> >>
> >>
> >> I'm still a little on the fence about the cmdline option for crashing
> >> repair at a certain point from the POV that Brian kind of pointed out
> >> that this doesn't exactly scale as we need more hooks.
> > 
> > (That's in the next patch.)
> 
> I. Am. Awesome.
>  
> ...
> 
> > Probably yes, but ... uh I don't want this to drag on into building a
> > generic error injection framework for userspace.
> > 
> > I would /really/ like to get inobtcount/bigtime tests into the kernel
> > without a giant detour they have nearly zero test coverage from the
> > wider community.
> 
> Yeah, I dont' want that either.
> 
> this (er, next patch) is s3kr1t and if we have something better later we
> can change it.  I'll just merge stuff as-is and move forward.

Er... TBH I actually /did/ want to hear Brian's response...

--D

> -Eric

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

[Darrick J. Wong]

On Thu, Feb 11, 2021 at 04:17:31PM -0800, Darrick J. Wong wrote:
> On Thu, Feb 11, 2021 at 05:29:05PM -0600, Eric Sandeen wrote:
> > On 2/11/21 4:59 PM, 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.  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.
> > > 
> > > Note that we can't do any of these upgrades until we've at least done a
> > > preliminary scan of the primary super and the log.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > 
> > 
> > I'm still a little on the fence about the cmdline option for crashing
> > repair at a certain point from the POV that Brian kind of pointed out
> > that this doesn't exactly scale as we need more hooks.
> 
> (That's in the next patch.)
> 
> > but
> > 
> > ehhhh it's a test-only undocumented option and I guess we could change
> > it later if desired
> > 
> > we do have other debug options on the commandline already as well....
> 
> I don't mind moving the debugging hooks to be seekrit environment
> variables or something, but I don't think I've quite addressed some of
> Brian's comments from last time:
> 
> [paste in stuff Brian said]
> 
> > But is it worth maintaining test specific debug logic in an
> > application just to confirm that particular feature bit upgrades
> > actually set the bit?
> 
> I argue that yes, this is important enough to burn a debugging knob.
> The sequence that I think we should prevent through testing is the one
> where we've set the new feature on the primary super but we haven't
> finished generating whatever new metadata is needed to complete the
> upgrade, the system crashes, and on remount the verifiers explode.
> 
> Chances are pretty good that we'll get an angry bug report on the
> mailing list: "I upgraded my fs, the power went down, and the kernel
> sprayed corruption everywhere!"  If we get a customer escalation like
> this, I'd /much/ rather it be about not being able to mount right after
> the reboot than a latent corruption that grows unseen until somebody's
> filesystem loses data.
> 
> If a future patch to repair accidentally breaks the behavior where we
> set NEEDSREPAIR at the same time as we set the new feature and flush the
> super to disk, we cannot tell that there's been a regression in this
> safety mechanism just by looking at the output of an otherwise
> successful xfs_repair run...
> 
> > It seems sufficient to me to test that needsrepair functionality works
> > as expected and that individual feature upgrade works as well.
> 
> ...so in other words, we need some point to inject an error to make sure
> that the upgrade interlock is correct.
> 
> > 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'm in the midst of prototyping what I said in the last thread --
> hooking the buffe cache so that repair can catch the first time we
> actually write anything to the filesystem, and using that to set
> NEEDSREPAIR.  I've not run it through full fstests yet, but AFAICT I can
> keep using the same tests and the same injection knobs I already wrote.

Ok, so, the buffer cache hook works now.

I split the needsrepair functionality tests into two parts -- one
corrupts a filesystem and runs repair with the error injection armed so
that repair sets NEEDSREPAIR and exits when we try to fix the damaged
metadata.  The second test exercises xfs_admin -O inobtcount=1 in
various scenarios with external devices, and then arms the error
injector to make sure that a partially completed upgrade has to go back
through repair.

I removed -c needsrepair because it's no longer necessary.

I replaced -o debug_needsrepair_force-abort with a magic environment
variable XFS_REPAIR_DEBUG_NEEDSREPAIR so that we don't clutter up the
CLI.

So that at least gets rid of the complaints about CLI clutter.

--D


> 
> > 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..
> 
> Probably yes, but ... uh I don't want this to drag on into building a
> generic error injection framework for userspace.
> 
> I would /really/ like to get inobtcount/bigtime tests into the kernel
> without a giant detour they have nearly zero test coverage from the
> wider community.
> 
> --D
> 
> > 
> > > ---
> > >  repair/globals.c    |    2 ++
> > >  repair/globals.h    |    2 ++
> > >  repair/phase2.c     |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  repair/xfs_repair.c |    9 +++++++
> > >  4 files changed, 76 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/phase2.c b/repair/phase2.c
> > > index 952ac4a5..9a8d42e1 100644
> > > --- a/repair/phase2.c
> > > +++ b/repair/phase2.c
> > > @@ -131,6 +131,63 @@ zero_log(
> > >  		libxfs_max_lsn = log->l_last_sync_lsn;
> > >  }
> > >  
> > > +static bool
> > > +set_needsrepair(
> > > +	struct xfs_mount	*mp)
> > > +{
> > > +	if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> > > +		printf(
> > > +	_("needsrepair flag only supported on V5 filesystems.\n"));
> > > +		exit(0);
> > > +	}
> > > +
> > > +	if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> > > +		printf(_("Filesystem already marked as needing repair.\n"));
> > > +		exit(0);
> > > +	}
> > > +
> > > +	printf(_("Marking filesystem in need of repair.\n"));
> > > +	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > > +	return true;
> > > +}
> > > +
> > > +/* Perform the user's requested upgrades on filesystem. */
> > > +static void
> > > +upgrade_filesystem(
> > > +	struct xfs_mount	*mp)
> > > +{
> > > +	struct xfs_buf		*bp;
> > > +	bool			dirty = false;
> > > +	int			error;
> > > +
> > > +	if (add_needsrepair)
> > > +		dirty |= set_needsrepair(mp);
> > > +
> > > +        if (no_modify || !dirty)
> > > +                return;
> > > +
> > > +        bp = libxfs_getsb(mp);
> > > +        if (!bp || bp->b_error) {
> > > +                do_error(
> > > +	_("couldn't get superblock for feature upgrade, err=%d\n"),
> > > +                                bp ? bp->b_error : ENOMEM);
> > > +        } else {
> > > +                libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > > +
> > > +                /*
> > > +		 * Write the primary super to disk immediately so that
> > > +		 * needsrepair will be set if repair doesn't complete.
> > > +		 */
> > > +                error = -libxfs_bwrite(bp);
> > > +                if (error)
> > > +                        do_error(
> > > +	_("filesystem feature upgrade failed, err=%d\n"),
> > > +                                        error);
> > > +        }
> > > +        if (bp)
> > > +                libxfs_buf_relse(bp);
> > > +}
> > > +
> > >  /*
> > >   * ok, at this point, the fs is mounted but the root inode may be
> > >   * trashed and the ag headers haven't been checked.  So we have
> > > @@ -235,4 +292,10 @@ phase2(
> > >  				do_warn(_("would correct\n"));
> > >  		}
> > >  	}
> > > +
> > > +	/*
> > > +	 * Upgrade the filesystem now that we've done a preliminary check of
> > > +	 * the superblocks, the AGs, the log, and the metadata inodes.
> > > +	 */
> > > +	upgrade_filesystem(mp);
> > >  }
> > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > index 90d1a95a..a613505f 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/11] xfs_repair: allow setting the needsrepair flag

[Brian Foster]

On Thu, Feb 11, 2021 at 04:17:31PM -0800, Darrick J. Wong wrote:
> On Thu, Feb 11, 2021 at 05:29:05PM -0600, Eric Sandeen wrote:
> > On 2/11/21 4:59 PM, 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.  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.
> > > 
> > > Note that we can't do any of these upgrades until we've at least done a
> > > preliminary scan of the primary super and the log.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > 
> > 
> > I'm still a little on the fence about the cmdline option for crashing
> > repair at a certain point from the POV that Brian kind of pointed out
> > that this doesn't exactly scale as we need more hooks.
> 
> (That's in the next patch.)
> 
> > but
> > 
> > ehhhh it's a test-only undocumented option and I guess we could change
> > it later if desired
> > 
> > we do have other debug options on the commandline already as well....
> 
> I don't mind moving the debugging hooks to be seekrit environment
> variables or something, but I don't think I've quite addressed some of
> Brian's comments from last time:
> 
> [paste in stuff Brian said]
> 
> > But is it worth maintaining test specific debug logic in an
> > application just to confirm that particular feature bit upgrades
> > actually set the bit?
> 
> I argue that yes, this is important enough to burn a debugging knob.
> The sequence that I think we should prevent through testing is the one
> where we've set the new feature on the primary super but we haven't
> finished generating whatever new metadata is needed to complete the
> upgrade, the system crashes, and on remount the verifiers explode.
> 
> Chances are pretty good that we'll get an angry bug report on the
> mailing list: "I upgraded my fs, the power went down, and the kernel
> sprayed corruption everywhere!"  If we get a customer escalation like
> this, I'd /much/ rather it be about not being able to mount right after
> the reboot than a latent corruption that grows unseen until somebody's
> filesystem loses data.
> 
> If a future patch to repair accidentally breaks the behavior where we
> set NEEDSREPAIR at the same time as we set the new feature and flush the
> super to disk, we cannot tell that there's been a regression in this
> safety mechanism just by looking at the output of an otherwise
> successful xfs_repair run...
> 

So I think what urks me most about this is how specific it is to the
particular test. IMO, it would be _nice_ to be able to induce xfs_repair
aborts at random purely via external mechanism, but I don't view that as
a hard requirement and so don't necessarily oppose an injection
mechanism in general. I also don't think this particular mechanism is as
robust as suggested because it tests for one very particular failure
scenario (i.e. failure to set the bit) over and over. If somebody was so
misguided as to rewrite the superblock sometime later in repair without
the bit set (somehow and for who knows what reason), this test wouldn't
catch it.

Here are some handwavy random thoughts on approaches for inducing
failures that I think would be more preferable, yet wouldn't preclude
the specific test this mechanism intends to support:

- Define a custom signal handler to trigger an do_abort() and invoke it
  randomly via test (or just kill -9 randomly). Con: this might require
  a non-trivial test fs and some looping to provide adequate coverage.
- Rework the current hook into somewhere more generic that allows either
  a random or generally more configurable trigger:
	- I.e., randomly abort in the buffer I/O completion path based
	  on a percentage passed by the user.
	- Refactor the per-phase timestamp() calls into a helper and
	  wire in a per-phase injection point, then let the test produce
	  explicit failures at the end of each phase, 1-7. This is not
	  quite as random, but certainly more thorough than a single
	  specific failure point.

These would probably still require some command line option to enable,
but it becomes less of a "test that nobody screws up these few lines of
code we just added" regression test. IMO, those tests tend to fail more
rarely than the randomized stress/failure tests that have at least some
capability to produce unforeseen failure scenarios.

> > It seems sufficient to me to test that needsrepair functionality works
> > as expected and that individual feature upgrade works as well.
> 
> ...so in other words, we need some point to inject an error to make sure
> that the upgrade interlock is correct.
> 
> > 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'm in the midst of prototyping what I said in the last thread --
> hooking the buffe cache so that repair can catch the first time we
> actually write anything to the filesystem, and using that to set
> NEEDSREPAIR.  I've not run it through full fstests yet, but AFAICT I can
> keep using the same tests and the same injection knobs I already wrote.
> 
> > 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..
> 
> Probably yes, but ... uh I don't want this to drag on into building a
> generic error injection framework for userspace.
> 

That's certainly fair. That's partly why I suggested to kick this can
down the road just a bit. At the same time I don't see the suggestions
above as necessarily more complex or more involved than this patch. It
may require around the same amount of code either way, just with a bit
more generic of an implementation.

Brian

> I would /really/ like to get inobtcount/bigtime tests into the kernel
> without a giant detour they have nearly zero test coverage from the
> wider community.
> 
> --D
> 
> > 
> > > ---
> > >  repair/globals.c    |    2 ++
> > >  repair/globals.h    |    2 ++
> > >  repair/phase2.c     |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  repair/xfs_repair.c |    9 +++++++
> > >  4 files changed, 76 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/phase2.c b/repair/phase2.c
> > > index 952ac4a5..9a8d42e1 100644
> > > --- a/repair/phase2.c
> > > +++ b/repair/phase2.c
> > > @@ -131,6 +131,63 @@ zero_log(
> > >  		libxfs_max_lsn = log->l_last_sync_lsn;
> > >  }
> > >  
> > > +static bool
> > > +set_needsrepair(
> > > +	struct xfs_mount	*mp)
> > > +{
> > > +	if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> > > +		printf(
> > > +	_("needsrepair flag only supported on V5 filesystems.\n"));
> > > +		exit(0);
> > > +	}
> > > +
> > > +	if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> > > +		printf(_("Filesystem already marked as needing repair.\n"));
> > > +		exit(0);
> > > +	}
> > > +
> > > +	printf(_("Marking filesystem in need of repair.\n"));
> > > +	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > > +	return true;
> > > +}
> > > +
> > > +/* Perform the user's requested upgrades on filesystem. */
> > > +static void
> > > +upgrade_filesystem(
> > > +	struct xfs_mount	*mp)
> > > +{
> > > +	struct xfs_buf		*bp;
> > > +	bool			dirty = false;
> > > +	int			error;
> > > +
> > > +	if (add_needsrepair)
> > > +		dirty |= set_needsrepair(mp);
> > > +
> > > +        if (no_modify || !dirty)
> > > +                return;
> > > +
> > > +        bp = libxfs_getsb(mp);
> > > +        if (!bp || bp->b_error) {
> > > +                do_error(
> > > +	_("couldn't get superblock for feature upgrade, err=%d\n"),
> > > +                                bp ? bp->b_error : ENOMEM);
> > > +        } else {
> > > +                libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > > +
> > > +                /*
> > > +		 * Write the primary super to disk immediately so that
> > > +		 * needsrepair will be set if repair doesn't complete.
> > > +		 */
> > > +                error = -libxfs_bwrite(bp);
> > > +                if (error)
> > > +                        do_error(
> > > +	_("filesystem feature upgrade failed, err=%d\n"),
> > > +                                        error);
> > > +        }
> > > +        if (bp)
> > > +                libxfs_buf_relse(bp);
> > > +}
> > > +
> > >  /*
> > >   * ok, at this point, the fs is mounted but the root inode may be
> > >   * trashed and the ag headers haven't been checked.  So we have
> > > @@ -235,4 +292,10 @@ phase2(
> > >  				do_warn(_("would correct\n"));
> > >  		}
> > >  	}
> > > +
> > > +	/*
> > > +	 * Upgrade the filesystem now that we've done a preliminary check of
> > > +	 * the superblocks, the AGs, the log, and the metadata inodes.
> > > +	 */
> > > +	upgrade_filesystem(mp);
> > >  }
> > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > index 90d1a95a..a613505f 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/11] xfs_repair: allow setting the needsrepair flag

[Darrick J. Wong]

On Fri, Feb 12, 2021 at 08:35:03AM -0500, Brian Foster wrote:
> On Thu, Feb 11, 2021 at 04:17:31PM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 11, 2021 at 05:29:05PM -0600, Eric Sandeen wrote:
> > > On 2/11/21 4:59 PM, 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.  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.
> > > > 
> > > > Note that we can't do any of these upgrades until we've at least done a
> > > > preliminary scan of the primary super and the log.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > 
> > > 
> > > I'm still a little on the fence about the cmdline option for crashing
> > > repair at a certain point from the POV that Brian kind of pointed out
> > > that this doesn't exactly scale as we need more hooks.
> > 
> > (That's in the next patch.)
> > 
> > > but
> > > 
> > > ehhhh it's a test-only undocumented option and I guess we could change
> > > it later if desired
> > > 
> > > we do have other debug options on the commandline already as well....
> > 
> > I don't mind moving the debugging hooks to be seekrit environment
> > variables or something, but I don't think I've quite addressed some of
> > Brian's comments from last time:
> > 
> > [paste in stuff Brian said]
> > 
> > > But is it worth maintaining test specific debug logic in an
> > > application just to confirm that particular feature bit upgrades
> > > actually set the bit?
> > 
> > I argue that yes, this is important enough to burn a debugging knob.
> > The sequence that I think we should prevent through testing is the one
> > where we've set the new feature on the primary super but we haven't
> > finished generating whatever new metadata is needed to complete the
> > upgrade, the system crashes, and on remount the verifiers explode.
> > 
> > Chances are pretty good that we'll get an angry bug report on the
> > mailing list: "I upgraded my fs, the power went down, and the kernel
> > sprayed corruption everywhere!"  If we get a customer escalation like
> > this, I'd /much/ rather it be about not being able to mount right after
> > the reboot than a latent corruption that grows unseen until somebody's
> > filesystem loses data.
> > 
> > If a future patch to repair accidentally breaks the behavior where we
> > set NEEDSREPAIR at the same time as we set the new feature and flush the
> > super to disk, we cannot tell that there's been a regression in this
> > safety mechanism just by looking at the output of an otherwise
> > successful xfs_repair run...
> > 
> 
> So I think what urks me most about this is how specific it is to the
> particular test. IMO, it would be _nice_ to be able to induce xfs_repair
> aborts at random purely via external mechanism, but I don't view that as
> a hard requirement and so don't necessarily oppose an injection
> mechanism in general. I also don't think this particular mechanism is as
> robust as suggested because it tests for one very particular failure
> scenario (i.e. failure to set the bit) over and over. If somebody was so
> misguided as to rewrite the superblock sometime later in repair without
> the bit set (somehow and for who knows what reason), this test wouldn't
> catch it.
> 
> Here are some handwavy random thoughts on approaches for inducing
> failures that I think would be more preferable, yet wouldn't preclude
> the specific test this mechanism intends to support:
> 
> - Define a custom signal handler to trigger an do_abort() and invoke it
>   randomly via test (or just kill -9 randomly). Con: this might require
>   a non-trivial test fs and some looping to provide adequate coverage.

I don't think a randomly triggered abort is better than a targeted trip
point.  However...

> - Rework the current hook into somewhere more generic that allows either
>   a random or generally more configurable trigger:
> 	- I.e., randomly abort in the buffer I/O completion path based
> 	  on a percentage passed by the user.

...since we know that a given xfs_repair run will trigger a bunch of
disk writes between phase 2 and phase 6, I think I could build a
trigger that would abort() after N writes to a device.  From there it
wouldn't be hard to add a test that does (more or less):

for i in {0..1000..10}; do
	xfs_mdrestore <dumpfile> /dev/sda
	XFS_REPAIR_DEBUG_FAIL_WRITE=$i xfs_repair /dev/sda
	xfs_db -c version /dev/sda | grep NEEDSREPAIR || _fail
	xfs_repair /dev/sda
	xfs_db -c version /dev/sda | grep NEEDSREPAIR && _fail
done

> 	- Refactor the per-phase timestamp() calls into a helper and
> 	  wire in a per-phase injection point, then let the test produce
> 	  explicit failures at the end of each phase, 1-7. This is not
> 	  quite as random, but certainly more thorough than a single
> 	  specific failure point.

This sounds like a reasonable second trip point for the directory repair
checker, since we know that the sketchy directory repair bits happen in
phase 3 and/or phase 6:

<fuzz dirent>
XFS_REPAIR_DEBUG_FAIL_PHASE=6 xfs_repair /dev/sda
xfs_db -c version /dev/sda | grep NEEDSREPAIR || _fail
xfs_repair /dev/sda
xfs_db -c version /dev/sda | grep NEEDSREPAIR && _fail

This is a good starting point, thanks. :)

> These would probably still require some command line option to enable,
> but it becomes less of a "test that nobody screws up these few lines of
> code we just added" regression test. IMO, those tests tend to fail more
> rarely than the randomized stress/failure tests that have at least some
> capability to produce unforeseen failure scenarios.

Fair 'nuff.

> > > It seems sufficient to me to test that needsrepair functionality works
> > > as expected and that individual feature upgrade works as well.
> > 
> > ...so in other words, we need some point to inject an error to make sure
> > that the upgrade interlock is correct.
> > 
> > > 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'm in the midst of prototyping what I said in the last thread --
> > hooking the buffe cache so that repair can catch the first time we
> > actually write anything to the filesystem, and using that to set
> > NEEDSREPAIR.  I've not run it through full fstests yet, but AFAICT I can
> > keep using the same tests and the same injection knobs I already wrote.
> > 
> > > 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..
> > 
> > Probably yes, but ... uh I don't want this to drag on into building a
> > generic error injection framework for userspace.
> > 
> 
> That's certainly fair. That's partly why I suggested to kick this can
> down the road just a bit. At the same time I don't see the suggestions
> above as necessarily more complex or more involved than this patch. It
> may require around the same amount of code either way, just with a bit
> more generic of an implementation.

In the meantime I guess Eric can take the other 2 fully reviewed series
as well as patches 1-7 and 10 from this series since (AFAICT) those
pieces are fully reviewed.

--D

> Brian
> 
> > I would /really/ like to get inobtcount/bigtime tests into the kernel
> > without a giant detour they have nearly zero test coverage from the
> > wider community.
> > 
> > --D
> > 
> > > 
> > > > ---
> > > >  repair/globals.c    |    2 ++
> > > >  repair/globals.h    |    2 ++
> > > >  repair/phase2.c     |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  repair/xfs_repair.c |    9 +++++++
> > > >  4 files changed, 76 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/phase2.c b/repair/phase2.c
> > > > index 952ac4a5..9a8d42e1 100644
> > > > --- a/repair/phase2.c
> > > > +++ b/repair/phase2.c
> > > > @@ -131,6 +131,63 @@ zero_log(
> > > >  		libxfs_max_lsn = log->l_last_sync_lsn;
> > > >  }
> > > >  
> > > > +static bool
> > > > +set_needsrepair(
> > > > +	struct xfs_mount	*mp)
> > > > +{
> > > > +	if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> > > > +		printf(
> > > > +	_("needsrepair flag only supported on V5 filesystems.\n"));
> > > > +		exit(0);
> > > > +	}
> > > > +
> > > > +	if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> > > > +		printf(_("Filesystem already marked as needing repair.\n"));
> > > > +		exit(0);
> > > > +	}
> > > > +
> > > > +	printf(_("Marking filesystem in need of repair.\n"));
> > > > +	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > > > +	return true;
> > > > +}
> > > > +
> > > > +/* Perform the user's requested upgrades on filesystem. */
> > > > +static void
> > > > +upgrade_filesystem(
> > > > +	struct xfs_mount	*mp)
> > > > +{
> > > > +	struct xfs_buf		*bp;
> > > > +	bool			dirty = false;
> > > > +	int			error;
> > > > +
> > > > +	if (add_needsrepair)
> > > > +		dirty |= set_needsrepair(mp);
> > > > +
> > > > +        if (no_modify || !dirty)
> > > > +                return;
> > > > +
> > > > +        bp = libxfs_getsb(mp);
> > > > +        if (!bp || bp->b_error) {
> > > > +                do_error(
> > > > +	_("couldn't get superblock for feature upgrade, err=%d\n"),
> > > > +                                bp ? bp->b_error : ENOMEM);
> > > > +        } else {
> > > > +                libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > > > +
> > > > +                /*
> > > > +		 * Write the primary super to disk immediately so that
> > > > +		 * needsrepair will be set if repair doesn't complete.
> > > > +		 */
> > > > +                error = -libxfs_bwrite(bp);
> > > > +                if (error)
> > > > +                        do_error(
> > > > +	_("filesystem feature upgrade failed, err=%d\n"),
> > > > +                                        error);
> > > > +        }
> > > > +        if (bp)
> > > > +                libxfs_buf_relse(bp);
> > > > +}
> > > > +
> > > >  /*
> > > >   * ok, at this point, the fs is mounted but the root inode may be
> > > >   * trashed and the ag headers haven't been checked.  So we have
> > > > @@ -235,4 +292,10 @@ phase2(
> > > >  				do_warn(_("would correct\n"));
> > > >  		}
> > > >  	}
> > > > +
> > > > +	/*
> > > > +	 * Upgrade the filesystem now that we've done a preliminary check of
> > > > +	 * the superblocks, the AGs, the log, and the metadata inodes.
> > > > +	 */
> > > > +	upgrade_filesystem(mp);
> > > >  }
> > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > > index 90d1a95a..a613505f 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;
> > > > 
> > 
> 

[PATCH 09/11] 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/globals.c    |    1 +
 repair/globals.h    |    2 ++
 repair/phase2.c     |    4 ++++
 repair/xfs_repair.c |    5 +++++
 4 files changed, 12 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/phase2.c b/repair/phase2.c
index 9a8d42e1..177e0831 100644
--- a/repair/phase2.c
+++ b/repair/phase2.c
@@ -183,6 +183,10 @@ upgrade_filesystem(
                         do_error(
 	_("filesystem feature upgrade failed, err=%d\n"),
                                         error);
+
+		/* Simulate a crash after setting needsrepair. */
+		if (abort_after_force_needsrepair)
+			exit(55);
         }
         if (bp)
                 libxfs_buf_relse(bp);
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index a613505f..293d89b1 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;

[PATCH 10/11] 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.
Technically speaking we could add lazycount to the list, but that option
is only useful for the V4 format which is deprecated.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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..055a502a 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 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 .

[PATCH 11/11] man: mark all deprecated V4 format options

[Darrick J. Wong]

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

Update the manual pages for the most popular tools to note which options
are only useful with the V4 XFS format, and that the V4 format is
deprecated and will be removed no later than September 2030.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 man/man8/mkfs.xfs.8  |   16 ++++++++++++++++
 man/man8/xfs_admin.8 |   10 ++++++++++
 2 files changed, 26 insertions(+)


diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index fac82d74..df25abaa 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -223,6 +223,11 @@ of calculating and checking the CRCs is not noticeable in normal operation.
 By default,
 .B mkfs.xfs
 will enable metadata CRCs.
+.IP
+Formatting a filesystem without CRCs selects the V4 format, which is deprecated
+and will be removed from upstream in September 2030.
+Distributors may choose to withdraw support for the V4 format earlier than
+this date.
 .TP
 .BI finobt= value
 This option enables the use of a separate free inode btree index in each
@@ -592,6 +597,8 @@ This option can be used to turn off inode alignment when the
 filesystem needs to be mountable by a version of IRIX
 that does not have the inode alignment feature
 (any release of IRIX before 6.2, and IRIX 6.2 without XFS patches).
+.IP
+This option only applies to the deprecated V4 format.
 .TP
 .BI attr= value
 This is used to specify the version of extended attribute inline
@@ -602,6 +609,8 @@ between attribute and extent data.
 The previous version 1, which has fixed regions for attribute and
 extent data, is kept for backwards compatibility with kernels older
 than version 2.6.16.
+.IP
+This option only applies to the deprecated V4 format.
 .TP
 .BI projid32bit[= value ]
 This is used to enable 32bit quota project identifiers. The
@@ -609,6 +618,8 @@ This is used to enable 32bit quota project identifiers. The
 is either 0 or 1, with 1 signifying that 32bit projid are to be enabled.
 If the value is omitted, 1 is assumed.  (This default changed
 in release version 3.2.0.)
+.IP
+This option only applies to the deprecated V4 format.
 .TP
 .BI sparse[= value ]
 Enable sparse inode chunk allocation. The
@@ -690,6 +701,7 @@ stripe-aligned log writes (see the sunit and su options, below).
 The previous version 1, which is limited to 32k log buffers and does
 not support stripe-aligned writes, is kept for backwards compatibility
 with very old 2.4 kernels.
+This option only applies to the deprecated V4 format.
 .TP
 .BI sunit= value
 This specifies the alignment to be used for log writes. The
@@ -744,6 +756,8 @@ is 1 (on) so you must specify
 .B lazy-count=0
 if you want to disable this feature for older kernels which don't support
 it.
+.IP
+This option only applies to the deprecated V4 format.
 .RE
 .PP
 .PD 0
@@ -803,6 +817,8 @@ will be stored in the directory structure.  The default value is 1.
 When CRCs are enabled (the default), the ftype functionality is always
 enabled, and cannot be turned off.
 .IP
+This option only applies to the deprecated V4 format.
+.IP
 .RE
 .TP
 .BI \-p " protofile"
diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
index 055a502a..be5c189f 100644
--- a/man/man8/xfs_admin.8
+++ b/man/man8/xfs_admin.8
@@ -56,6 +56,8 @@ for a detailed description of the XFS log.
 Enables unwritten extent support on a filesystem that does not
 already have this enabled (for legacy filesystems, it can't be
 disabled anymore at mkfs time).
+.IP
+This option only applies to the deprecated V4 format.
 .TP
 .B \-f
 Specifies that the filesystem image to be processed is stored in a
@@ -69,12 +71,16 @@ option).
 .B \-j
 Enables version 2 log format (journal format supporting larger
 log buffers).
+.IP
+This option only applies to the deprecated V4 format.
 .TP
 .B \-l
 Print the current filesystem label.
 .TP
 .B \-p
 Enable 32bit project identifier support (PROJID32BIT feature).
+.IP
+This option only applies to the deprecated V4 format.
 .TP
 .B \-u
 Print the current filesystem UUID (Universally Unique IDentifier).
@@ -85,6 +91,8 @@ Enable (1) or disable (0) lazy-counters in the filesystem.
 Lazy-counters may not be disabled on Version 5 superblock filesystems
 (i.e. those with metadata CRCs enabled).
 .IP
+In other words, this option only applies to the deprecated V4 format.
+.IP
 This operation may take quite a bit of time on large filesystems as the
 entire filesystem needs to be scanned when this option is changed.
 .IP
@@ -94,6 +102,8 @@ information is kept in other parts of the filesystem to be able to
 maintain the counter values without needing to keep them in the
 superblock. This gives significant improvements in performance on some
 configurations and metadata intensive workloads.
+.IP
+This option only applies to the deprecated V4 format.
 .TP
 .BI \-L " label"
 Set the filesystem label to