[PATCH 00/16] xfs_repair: catch things that xfs_check misses

[PATCH 00/16] xfs_repair: catch things that xfs_check misses

[Darrick J. Wong]

Hi all,

A long-time goal of mine is to get rid of xfs_check from fstests,
because it is deprecated, adds quite a bit of runtime to the test suite,
and consumes memory like crazy.  We've not been able to do that for lack
of even a basic field-by-field corruption detection comparison between
check and repair, so I temporarily modified the dangerous_repair tests
to warn when check finds something but repair says clean.

The patches below teach xfs_repair to complain about things that it
previously did not catch but xfs_check did.  The one remaining gap is
the lack of quota counter checking, which will be sent in a separate
series once I've worked out all the bugs.

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

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

[PATCH 01/16] xfs_repair: fix missing dir buffer corruption checks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Both callers of da_read_buf do not adequately check for verifier
failures in the (salvage mode) buffer that gets returned.  This leads to
repair sometimes failing to complain about corrupt directories when run
with -n, which leads to an incorrect return value of 0 (all clean).

Found by running xfs/496 against lhdr.stale = middlebit.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/da_util.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)


diff --git a/repair/da_util.c b/repair/da_util.c
index 5061880f..92c04337 100644
--- a/repair/da_util.c
+++ b/repair/da_util.c
@@ -140,11 +140,24 @@ _("can't read %s block %u for inode %" PRIu64 "\n"),
 		if (whichfork == XFS_DATA_FORK &&
 		    (nodehdr.magic == XFS_DIR2_LEAFN_MAGIC ||
 		    nodehdr.magic == XFS_DIR3_LEAFN_MAGIC)) {
+			int bad = 0;
+
 			if (i != -1) {
 				do_warn(
 _("found non-root LEAFN node in inode %" PRIu64 " bno = %u\n"),
 					da_cursor->ino, bno);
+				bad++;
 			}
+
+			/* corrupt leafn node; rebuild the dir. */
+			if (!bad &&
+			    (bp->b_error == -EFSBADCRC ||
+			     bp->b_error == -EFSCORRUPTED)) {
+				do_warn(
+_("corrupt %s LEAFN block %u for inode %" PRIu64 "\n"),
+					FORKNAME(whichfork), bno, da_cursor->ino);
+			}
+
 			*rbno = 0;
 			libxfs_buf_relse(bp);
 			return 1;
@@ -599,6 +612,14 @@ _("bad level %d in %s block %u for inode %" PRIu64 "\n"),
 				dabno, cursor->ino);
 			bad++;
 		}
+		if (!bad &&
+		    (bp->b_error == -EFSCORRUPTED ||
+		     bp->b_error == -EFSBADCRC)) {
+			do_warn(
+_("corruption error reading %s block %u for inode %" PRIu64 "\n"),
+				FORKNAME(whichfork), dabno, cursor->ino);
+			bad++;
+		}
 		if (bad) {
 #ifdef XR_DIR_TRACE
 			fprintf(stderr, "verify_da_path returns 1 (bad) #4\n");

Re: [PATCH 01/16] xfs_repair: fix missing dir buffer corruption checks

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:29:58AM -0700, Darrick J. Wong wrote:
> @@ -140,11 +140,24 @@ _("can't read %s block %u for inode %" PRIu64 "\n"),
>  		if (whichfork == XFS_DATA_FORK &&
>  		    (nodehdr.magic == XFS_DIR2_LEAFN_MAGIC ||
>  		    nodehdr.magic == XFS_DIR3_LEAFN_MAGIC)) {
> +			int bad = 0;
> +
>  			if (i != -1) {
>  				do_warn(
>  _("found non-root LEAFN node in inode %" PRIu64 " bno = %u\n"),
>  					da_cursor->ino, bno);
> +				bad++;
>  			}
> +
> +			/* corrupt leafn node; rebuild the dir. */
> +			if (!bad &&
> +			    (bp->b_error == -EFSBADCRC ||
> +			     bp->b_error == -EFSCORRUPTED)) {
> +				do_warn(
> +_("corrupt %s LEAFN block %u for inode %" PRIu64 "\n"),
> +					FORKNAME(whichfork), bno, da_cursor->ino);
> +			}
> +

So this doesn't really change any return value, but just the error
message.  But looking at this code I wonder why we don't check
b_error first thing after reading the buffer, as checking the magic
for a corrupt buffer seems a little pointless.

Re: [PATCH 01/16] xfs_repair: fix missing dir buffer corruption checks

[Darrick J. Wong]

On Sat, May 09, 2020 at 10:08:50AM -0700, Christoph Hellwig wrote:
> On Sat, May 09, 2020 at 09:29:58AM -0700, Darrick J. Wong wrote:
> > @@ -140,11 +140,24 @@ _("can't read %s block %u for inode %" PRIu64 "\n"),
> >  		if (whichfork == XFS_DATA_FORK &&
> >  		    (nodehdr.magic == XFS_DIR2_LEAFN_MAGIC ||
> >  		    nodehdr.magic == XFS_DIR3_LEAFN_MAGIC)) {
> > +			int bad = 0;
> > +
> >  			if (i != -1) {
> >  				do_warn(
> >  _("found non-root LEAFN node in inode %" PRIu64 " bno = %u\n"),
> >  					da_cursor->ino, bno);
> > +				bad++;
> >  			}
> > +
> > +			/* corrupt leafn node; rebuild the dir. */
> > +			if (!bad &&
> > +			    (bp->b_error == -EFSBADCRC ||
> > +			     bp->b_error == -EFSCORRUPTED)) {
> > +				do_warn(
> > +_("corrupt %s LEAFN block %u for inode %" PRIu64 "\n"),
> > +					FORKNAME(whichfork), bno, da_cursor->ino);
> > +			}
> > +
> 
> So this doesn't really change any return value, but just the error
> message.  But looking at this code I wonder why we don't check
> b_error first thing after reading the buffer, as checking the magic
> for a corrupt buffer seems a little pointless.

<shrug> In the first hunk I was merely following what we did for DA_NODE
blocks (check magic, then check for corruption errors) but I guess I
could just pull that up in the file.  I'll have a look and see what
happens if I do that.

--D

Re: [PATCH 01/16] xfs_repair: fix missing dir buffer corruption checks

[Darrick J. Wong]

On Mon, May 11, 2020 at 09:44:21AM -0700, Darrick J. Wong wrote:
> On Sat, May 09, 2020 at 10:08:50AM -0700, Christoph Hellwig wrote:
> > On Sat, May 09, 2020 at 09:29:58AM -0700, Darrick J. Wong wrote:
> > > @@ -140,11 +140,24 @@ _("can't read %s block %u for inode %" PRIu64 "\n"),
> > >  		if (whichfork == XFS_DATA_FORK &&
> > >  		    (nodehdr.magic == XFS_DIR2_LEAFN_MAGIC ||
> > >  		    nodehdr.magic == XFS_DIR3_LEAFN_MAGIC)) {
> > > +			int bad = 0;
> > > +
> > >  			if (i != -1) {
> > >  				do_warn(
> > >  _("found non-root LEAFN node in inode %" PRIu64 " bno = %u\n"),
> > >  					da_cursor->ino, bno);
> > > +				bad++;
> > >  			}
> > > +
> > > +			/* corrupt leafn node; rebuild the dir. */
> > > +			if (!bad &&
> > > +			    (bp->b_error == -EFSBADCRC ||
> > > +			     bp->b_error == -EFSCORRUPTED)) {
> > > +				do_warn(
> > > +_("corrupt %s LEAFN block %u for inode %" PRIu64 "\n"),
> > > +					FORKNAME(whichfork), bno, da_cursor->ino);
> > > +			}
> > > +
> > 
> > So this doesn't really change any return value, but just the error
> > message.  But looking at this code I wonder why we don't check
> > b_error first thing after reading the buffer, as checking the magic
> > for a corrupt buffer seems a little pointless.
> 
> <shrug> In the first hunk I was merely following what we did for DA_NODE
> blocks (check magic, then check for corruption errors) but I guess I
> could just pull that up in the file.  I'll have a look and see what
> happens if I do that.

...and on re-examination, the other da_read_buf callers in repair/dir2.c
ought to look for EFSCORRUPTED (they already look for EFSBADCRC) and
complain about that.  Seeing as the directory salvage is done separately
in phase6.c, I guess there's no harm in jumping out early in phases 3/4.

--D

> --D

[PATCH v2 01/16] xfs_repair: fix missing dir buffer corruption checks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The da_read_buf() function operates in "salvage" mode, which means that
if the verifiers fail, it will return a buffer with b_error set.  The
callers of da_read_buf, however, do not adequately check for verifier
errors, which means that repair can fail to flag a corrupt filesystem.

Fix the callers to do this properly.  The dabtree block walker and the
dabtree path checker functions to complain any time the da node / leafn
verifiers fail.  Fix the directory block walking functions to complain
about EFSCORRUPTED, since they already dealt with EFSBADCRC.

Found by running xfs/496 against lhdr.stale = middlebit.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: check for corruption before checking magics; expand corruption
checks to other da_read_buf callers
---
 repair/da_util.c |   25 ++++++++++++++++---------
 repair/dir2.c    |   21 +++++++++++++++++++++
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/repair/da_util.c b/repair/da_util.c
index 5061880f..7239c2e2 100644
--- a/repair/da_util.c
+++ b/repair/da_util.c
@@ -134,6 +134,15 @@ _("can't read %s block %u for inode %" PRIu64 "\n"),
 			goto error_out;
 		}
 
+		/* corrupt leafn/node; rebuild the dir. */
+		if (bp->b_error == -EFSBADCRC || bp->b_error == -EFSCORRUPTED) {
+			do_warn(
+_("corrupt %s tree block %u for inode %" PRIu64 "\n"),
+				FORKNAME(whichfork), bno, da_cursor->ino);
+			libxfs_buf_relse(bp);
+			goto error_out;
+		}
+
 		node = bp->b_addr;
 		libxfs_da3_node_hdr_from_disk(mp, &nodehdr, node);
 
@@ -160,15 +169,6 @@ _("bad %s magic number 0x%x in inode %" PRIu64 " bno = %u\n"),
 			goto error_out;
 		}
 
-		/* corrupt node; rebuild the dir. */
-		if (bp->b_error == -EFSBADCRC || bp->b_error == -EFSCORRUPTED) {
-			libxfs_buf_relse(bp);
-			do_warn(
-_("corrupt %s tree block %u for inode %" PRIu64 "\n"),
-				FORKNAME(whichfork), bno, da_cursor->ino);
-			goto error_out;
-		}
-
 		if (nodehdr.count > geo->node_ents) {
 			do_warn(
 _("bad %s record count in inode %" PRIu64 ", count = %d, max = %d\n"),
@@ -562,6 +562,13 @@ _("can't read %s block %u for inode %" PRIu64 "\n"),
 				FORKNAME(whichfork), dabno, cursor->ino);
 			return 1;
 		}
+		if (bp->b_error == -EFSCORRUPTED || bp->b_error == -EFSBADCRC) {
+			do_warn(
+_("corrupt %s tree block %u for inode %" PRIu64 "\n"),
+				FORKNAME(whichfork), dabno, cursor->ino);
+			libxfs_buf_relse(bp);
+			return 1;
+		}
 
 		newnode = bp->b_addr;
 		libxfs_da3_node_hdr_from_disk(mp, &nodehdr, newnode);
diff --git a/repair/dir2.c b/repair/dir2.c
index cbbce601..b374bc7b 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -983,6 +983,13 @@ _("can't read block %u for directory inode %" PRIu64 "\n"),
 			mp->m_dir_geo->datablk, ino);
 		return 1;
 	}
+	if (bp->b_error == -EFSCORRUPTED) {
+		do_warn(
+_("corrupt directory block %u for inode %" PRIu64 "\n"),
+			mp->m_dir_geo->datablk, ino);
+		libxfs_buf_relse(bp);
+		return 1;
+	}
 	/*
 	 * Verify the block
 	 */
@@ -1122,6 +1129,13 @@ _("can't read file block %u for directory inode %" PRIu64 "\n"),
 				da_bno, ino);
 			goto error_out;
 		}
+		if (bp->b_error == -EFSCORRUPTED) {
+			do_warn(
+_("corrupt directory leafn block %u for inode %" PRIu64 "\n"),
+				da_bno, ino);
+			libxfs_buf_relse(bp);
+			goto error_out;
+		}
 		leaf = bp->b_addr;
 		libxfs_dir2_leaf_hdr_from_disk(mp, &leafhdr, leaf);
 		/*
@@ -1324,6 +1338,13 @@ _("can't read block %" PRIu64 " for directory inode %" PRIu64 "\n"),
 				dbno, ino);
 			continue;
 		}
+		if (bp->b_error == -EFSCORRUPTED) {
+			do_warn(
+_("corrupt directory data block %lu for inode %" PRIu64 "\n"),
+				dbno, ino);
+			libxfs_buf_relse(bp);
+			continue;
+		}
 		data = bp->b_addr;
 		if (!(be32_to_cpu(data->magic) == XFS_DIR2_DATA_MAGIC ||
 		      be32_to_cpu(data->magic) == XFS_DIR3_DATA_MAGIC))

Re: [PATCH v2 01/16] xfs_repair: fix missing dir buffer corruption checks

[Christoph Hellwig]

This looks ok, but it seems like da_read_buf should just return
the error instead of the b_error mess, at whih point we'd basically
have xfs_da_read_buf + the salvage flag.  But I guess we can apply
your patch first to let you make progress first and sort that
out later:

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

Re: [PATCH v2 01/16] xfs_repair: fix missing dir buffer corruption checks

[Darrick J. Wong]

On Tue, May 12, 2020 at 11:22:16PM -0700, Christoph Hellwig wrote:
> This looks ok, but it seems like da_read_buf should just return
> the error instead of the b_error mess, at whih point we'd basically
> have xfs_da_read_buf + the salvage flag.  But I guess we can apply
> your patch first to let you make progress first and sort that
> out later:

<nod> I /think/ the value in repair's use of salvage mode here is to be
able to pinpoint exactly which dirent in a directory data block went
bad, and then to log that for the sysadmin as maybe a clue for renaming
files out of lost+found.  We probably don't need salvage mode for
dabtree blocks since that's just indexing data that users never see
directly.

In theory we don't need salvage mode at all, but it's less exciting to
barf out the standard metadata verifier error and have nothing else to
say.

Anyway, thanks for the review.

--D

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

[PATCH 02/16] xfs_repair: warn when we would have rebuilt a directory

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

longform_dir2_entry_check should warn the user when we would have
rebuilt a directory had -n not been given on the command line.  The
missing warning results in repair returning 0 (all clean) when in fact
there were things that it would have fixed.

Found by running xfs/496 against lents[0].hashval = middlebit.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/phase6.c |    3 +++
 1 file changed, 3 insertions(+)


diff --git a/repair/phase6.c b/repair/phase6.c
index a938e802..75e273c8 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2426,6 +2426,9 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
 		*num_illegal = 0;
 		*need_dot = 0;
 	} else {
+		if (fixit || dotdot_update)
+			do_warn(
+	_("would rebuild directory inode %" PRIu64 "\n"), ino);
 		for (i = 0; i < num_bps; i++)
 			if (bplist[i])
 				libxfs_buf_relse(bplist[i]);

Re: [PATCH 02/16] xfs_repair: warn when we would have rebuilt a directory

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:30:05AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> longform_dir2_entry_check should warn the user when we would have
> rebuilt a directory had -n not been given on the command line.  The
> missing warning results in repair returning 0 (all clean) when in fact
> there were things that it would have fixed.
> 
> Found by running xfs/496 against lents[0].hashval = middlebit.

Looks good,

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

[PATCH 03/16] xfs_repair: check for AG btree records that would wrap around

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

For AG btree types, make sure that each record's length is not so huge
that integer wraparound would happen.

Found via xfs/358 fuzzing recs[1].blockcount = ones.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/scan.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 5c8d8b23..1ddb5763 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -684,7 +684,8 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
-			if (len == 0 || !verify_agbno(mp, agno, end - 1)) {
+			if (len == 0 || end <= b ||
+			    !verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -1066,7 +1067,8 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
-			if (len == 0 || !verify_agbno(mp, agno, end - 1)) {
+			if (len == 0 || end <= b ||
+			    !verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -1353,7 +1355,8 @@ _("leftover CoW extent has invalid startblock in record %u of %s btree block %u/
 					b, i, name, agno, bno);
 				continue;
 			}
-			if (len == 0 || !verify_agbno(mp, agno, end - 1)) {
+			if (len == 0 || end <= agb ||
+			    !verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);

Re: [PATCH 03/16] xfs_repair: check for AG btree records that would wrap around

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:30:11AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> For AG btree types, make sure that each record's length is not so huge
> that integer wraparound would happen.
> 
> Found via xfs/358 fuzzing recs[1].blockcount = ones.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 04/16] xfs_repair: fix bnobt and refcountbt record order checks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The bnobt and refcountbt scanners attempt to check that records are in
the correct order.  However, the lastblock variable in both functions
ought to be set to the end of the previous record (instead of the start)
because otherwise we fail to catch overlapping records, which are not
allowed in either btree type.

Found by running xfs/410 with recs[1].blockcount = middlebit.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/scan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 1ddb5763..2d156d64 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -699,7 +699,7 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 	"out-of-order bno btree record %d (%u %u) block %u/%u\n"),
 						i, b, len, agno, bno);
 				} else {
-					lastblock = b;
+					lastblock = end - 1;
 				}
 			} else {
 				agcnts->fdblocks += len;
@@ -1396,7 +1396,7 @@ _("extent (%u/%u) len %u claimed, state is %d\n"),
 	"out-of-order %s btree record %d (%u %u) block %u/%u\n"),
 					name, i, b, len, agno, bno);
 			} else {
-				lastblock = b;
+				lastblock = end - 1;
 			}
 
 			/* Is this record mergeable with the last one? */

Re: [PATCH 04/16] xfs_repair: fix bnobt and refcountbt record order checks

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:30:17AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The bnobt and refcountbt scanners attempt to check that records are in
> the correct order.  However, the lastblock variable in both functions
> ought to be set to the end of the previous record (instead of the start)
> because otherwise we fail to catch overlapping records, which are not
> allowed in either btree type.
> 
> Found by running xfs/410 with recs[1].blockcount = middlebit.

Looks good,

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

[PATCH 05/16] xfs_repair: check for out-of-order inobt records

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Make sure that the inode btree records are in order.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/scan.c |   10 ++++++++++
 1 file changed, 10 insertions(+)


diff --git a/repair/scan.c b/repair/scan.c
index 2d156d64..7c46ab89 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -1947,6 +1947,7 @@ scan_inobt(
 	const struct xfs_buf_ops *ops)
 {
 	struct aghdr_cnts	*agcnts = priv;
+	xfs_agino_t		lastino = 0;
 	int			i;
 	int			numrecs;
 	int			state;
@@ -2029,7 +2030,16 @@ _("inode btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 		 * the block.  skip processing of bogus records.
 		 */
 		for (i = 0; i < numrecs; i++) {
+			xfs_agino_t	startino;
+
 			freecount = inorec_get_freecount(mp, &rp[i]);
+			startino = be32_to_cpu(rp[i].ir_startino);
+			if (i > 0 && startino <= lastino)
+				do_warn(_(
+	"out-of-order ino btree record %d (%u) block %u/%u\n"),
+						i, startino, agno, bno);
+			else
+				lastino = startino + XFS_INODES_PER_CHUNK - 1;
 
 			if (magic == XFS_IBT_MAGIC ||
 			    magic == XFS_IBT_CRC_MAGIC) {

Re: [PATCH 05/16] xfs_repair: check for out-of-order inobt records

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:30:24AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure that the inode btree records are in order.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 06/16] xfs_repair: fix rmapbt record order check

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The rmapbt record order checks here don't quite work properly.  For
non-shared filesystems, we fail to check that the startblock of the nth
record comes entirely after the previous record.

However, for filesystems with shared blocks (reflink) we correctly check
that the startblock/owner/offset of the nth record comes after the
previous one.

Therefore, make the reflink fs checks use "laststartblock" to preserve
that functionality while making the non-reflink fs checks use
"lastblock" to fix the problem outlined above.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/scan.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 7c46ab89..7508f7e8 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -925,15 +925,15 @@ struct rmap_priv {
 static bool
 rmap_in_order(
 	xfs_agblock_t	b,
-	xfs_agblock_t	lastblock,
+	xfs_agblock_t	laststartblock,
 	uint64_t	owner,
 	uint64_t	lastowner,
 	uint64_t	offset,
 	uint64_t	lastoffset)
 {
-	if (b > lastblock)
+	if (b > laststartblock)
 		return true;
-	else if (b < lastblock)
+	else if (b < laststartblock)
 		return false;
 
 	if (owner > lastowner)
@@ -964,6 +964,7 @@ scan_rmapbt(
 	int			hdr_errors = 0;
 	int			numrecs;
 	int			state;
+	xfs_agblock_t		laststartblock = 0;
 	xfs_agblock_t		lastblock = 0;
 	uint64_t		lastowner = 0;
 	uint64_t		lastoffset = 0;
@@ -1101,14 +1102,15 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			/* Check for out of order records. */
 			if (i == 0) {
 advance:
-				lastblock = b;
+				laststartblock = b;
+				lastblock = end - 1;
 				lastowner = owner;
 				lastoffset = offset;
 			} else {
 				bool bad;
 
 				if (xfs_sb_version_hasreflink(&mp->m_sb))
-					bad = !rmap_in_order(b, lastblock,
+					bad = !rmap_in_order(b, laststartblock,
 							owner, lastowner,
 							offset, lastoffset);
 				else

Re: [PATCH 06/16] xfs_repair: fix rmapbt record order check

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:30:30AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The rmapbt record order checks here don't quite work properly.  For
> non-shared filesystems, we fail to check that the startblock of the nth
> record comes entirely after the previous record.
> 
> However, for filesystems with shared blocks (reflink) we correctly check
> that the startblock/owner/offset of the nth record comes after the
> previous one.
> 
> Therefore, make the reflink fs checks use "laststartblock" to preserve
> that functionality while making the non-reflink fs checks use
> "lastblock" to fix the problem outlined above.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/scan.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/repair/scan.c b/repair/scan.c
> index 7c46ab89..7508f7e8 100644
> --- a/repair/scan.c
> +++ b/repair/scan.c
> @@ -925,15 +925,15 @@ struct rmap_priv {
>  static bool
>  rmap_in_order(
>  	xfs_agblock_t	b,
> -	xfs_agblock_t	lastblock,
> +	xfs_agblock_t	laststartblock,
>  	uint64_t	owner,
>  	uint64_t	lastowner,
>  	uint64_t	offset,
>  	uint64_t	lastoffset)
>  {
> -	if (b > lastblock)
> +	if (b > laststartblock)
>  		return true;
> -	else if (b < lastblock)
> +	else if (b < laststartblock)
>  		return false;
>  
>  	if (owner > lastowner)

So this is just a variable rename and looks obviously fine.

> @@ -964,6 +964,7 @@ scan_rmapbt(
>  	int			hdr_errors = 0;
>  	int			numrecs;
>  	int			state;
> +	xfs_agblock_t		laststartblock = 0;
>  	xfs_agblock_t		lastblock = 0;
>  	uint64_t		lastowner = 0;
>  	uint64_t		lastoffset = 0;
> @@ -1101,14 +1102,15 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
>  			/* Check for out of order records. */
>  			if (i == 0) {
>  advance:
> -				lastblock = b;
> +				laststartblock = b;
> +				lastblock = end - 1;
>  				lastowner = owner;
>  				lastoffset = offset;
>  			} else {
>  				bool bad;
>  
>  				if (xfs_sb_version_hasreflink(&mp->m_sb))
> -					bad = !rmap_in_order(b, lastblock,
> +					bad = !rmap_in_order(b, laststartblock,
>  							owner, lastowner,
>  							offset, lastoffset);
>  				else

This looks correct, but really hard to read. I'll send a follow on
cleanup.

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

[PATCH 07/16] xfs_repair: tag inobt vs finobt errors properly

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Amend the generic inode btree block scanner function to tag correctly
which tree it's complaining about.  Previously, dubious finobt headers
would be attributed to the "inode btree", which is at best ambiguous
and misleading at worst.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/scan.c |   36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 7508f7e8..fff54ecf 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -1949,6 +1949,7 @@ scan_inobt(
 	const struct xfs_buf_ops *ops)
 {
 	struct aghdr_cnts	*agcnts = priv;
+	char			*name;
 	xfs_agino_t		lastino = 0;
 	int			i;
 	int			numrecs;
@@ -1961,17 +1962,32 @@ scan_inobt(
 
 	hdr_errors = 0;
 
+	switch (magic) {
+	case XFS_FIBT_MAGIC:
+	case XFS_FIBT_CRC_MAGIC:
+		name = "fino";
+		break;
+	case XFS_IBT_MAGIC:
+	case XFS_IBT_CRC_MAGIC:
+		name = "ino";
+		break;
+	default:
+		name = "(unknown)";
+		assert(0);
+		break;
+	}
+
 	if (be32_to_cpu(block->bb_magic) != magic) {
-		do_warn(_("bad magic # %#x in inobt block %d/%d\n"),
-			be32_to_cpu(block->bb_magic), agno, bno);
+		do_warn(_("bad magic # %#x in %sbt block %d/%d\n"),
+			be32_to_cpu(block->bb_magic), name, agno, bno);
 		hdr_errors++;
 		bad_ino_btree = 1;
 		if (suspect)
 			return;
 	}
 	if (be16_to_cpu(block->bb_level) != level) {
-		do_warn(_("expected level %d got %d in inobt block %d/%d\n"),
-			level, be16_to_cpu(block->bb_level), agno, bno);
+		do_warn(_("expected level %d got %d in %sbt block %d/%d\n"),
+			level, be16_to_cpu(block->bb_level), name, agno, bno);
 		hdr_errors++;
 		bad_ino_btree = 1;
 		if (suspect)
@@ -1993,8 +2009,8 @@ scan_inobt(
 	default:
 		set_bmap(agno, bno, XR_E_MULT);
 		do_warn(
-_("inode btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
-			state, agno, bno, suspect);
+_("%sbt btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
+			name, state, agno, bno, suspect);
 	}
 
 	numrecs = be16_to_cpu(block->bb_numrecs);
@@ -2016,8 +2032,8 @@ _("inode btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 
 		if (hdr_errors)  {
 			bad_ino_btree = 1;
-			do_warn(_("dubious inode btree block header %d/%d\n"),
-				agno, bno);
+			do_warn(_("dubious %sbt btree block header %d/%d\n"),
+				name, agno, bno);
 			suspect++;
 		}
 
@@ -2038,8 +2054,8 @@ _("inode btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			startino = be32_to_cpu(rp[i].ir_startino);
 			if (i > 0 && startino <= lastino)
 				do_warn(_(
-	"out-of-order ino btree record %d (%u) block %u/%u\n"),
-						i, startino, agno, bno);
+	"out-of-order %s btree record %d (%u) block %u/%u\n"),
+						name, i, startino, agno, bno);
 			else
 				lastino = startino + XFS_INODES_PER_CHUNK - 1;
 

Re: [PATCH 07/16] xfs_repair: tag inobt vs finobt errors properly

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:30:42AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Amend the generic inode btree block scanner function to tag correctly
> which tree it's complaining about.  Previously, dubious finobt headers
> would be attributed to the "inode btree", which is at best ambiguous
> and misleading at worst.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 08/16] xfs_repair: complain about bad interior btree pointers

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Actually complain about garbage btree node pointers, don't just silently
ignore them.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/scan.c            |   55 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 41 insertions(+), 15 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 4462036b..7b264ff2 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -180,6 +180,7 @@
 #define xfs_trans_roll_inode		libxfs_trans_roll_inode
 #define xfs_trans_roll			libxfs_trans_roll
 
+#define xfs_verify_agbno		libxfs_verify_agbno
 #define xfs_verify_cksum		libxfs_verify_cksum
 #define xfs_verify_dir_ino		libxfs_verify_dir_ino
 #define xfs_verify_ino			libxfs_verify_ino
diff --git a/repair/scan.c b/repair/scan.c
index fff54ecf..719ad035 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -779,6 +779,14 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 	for (i = 0; i < numrecs; i++)  {
 		xfs_agblock_t		agbno = be32_to_cpu(pp[i]);
 
+		if (!libxfs_verify_agbno(mp, agno, agbno)) {
+			do_warn(
+	_("bad btree pointer (%u) in %sbt block %u/%u\n"),
+				agbno, name, agno, bno);
+			suspect++;
+			return;
+		}
+
 		/*
 		 * XXX - put sibling detection right here.
 		 * we know our sibling chain is good.  So as we go,
@@ -788,10 +796,8 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 		 * pointer mismatch, try and extract as much data
 		 * as possible.
 		 */
-		if (agbno != 0 && verify_agbno(mp, agno, agbno)) {
-			scan_sbtree(agbno, level, agno, suspect, scan_allocbt,
-				    0, magic, priv, ops);
-		}
+		scan_sbtree(agbno, level, agno, suspect, scan_allocbt, 0,
+				magic, priv, ops);
 	}
 }
 
@@ -1234,10 +1240,16 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			continue;
 		}
 
-		if (agbno != 0 && verify_agbno(mp, agno, agbno)) {
-			scan_sbtree(agbno, level, agno, suspect, scan_rmapbt, 0,
-				    magic, priv, ops);
+		if (!libxfs_verify_agbno(mp, agno, agbno)) {
+			do_warn(
+	_("bad btree pointer (%u) in %sbt block %u/%u\n"),
+				agbno, name, agno, bno);
+			suspect++;
+			return;
 		}
+
+		scan_sbtree(agbno, level, agno, suspect, scan_rmapbt, 0, magic,
+				priv, ops);
 	}
 
 out:
@@ -1454,10 +1466,16 @@ _("extent (%u/%u) len %u claimed, state is %d\n"),
 	for (i = 0; i < numrecs; i++)  {
 		xfs_agblock_t		agbno = be32_to_cpu(pp[i]);
 
-		if (agbno != 0 && verify_agbno(mp, agno, agbno)) {
-			scan_sbtree(agbno, level, agno, suspect, scan_refcbt, 0,
-				    magic, priv, ops);
+		if (!libxfs_verify_agbno(mp, agno, agbno)) {
+			do_warn(
+	_("bad btree pointer (%u) in %sbt block %u/%u\n"),
+				agbno, name, agno, bno);
+			suspect++;
+			return;
 		}
+
+		scan_sbtree(agbno, level, agno, suspect, scan_refcbt, 0, magic,
+				priv, ops);
 	}
 out:
 	if (suspect)
@@ -2125,11 +2143,18 @@ _("%sbt btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 	}
 
 	for (i = 0; i < numrecs; i++)  {
-		if (be32_to_cpu(pp[i]) != 0 && verify_agbno(mp, agno,
-							be32_to_cpu(pp[i])))
-			scan_sbtree(be32_to_cpu(pp[i]), level, agno,
-					suspect, scan_inobt, 0, magic, priv,
-					ops);
+		xfs_agblock_t	agbno = be32_to_cpu(pp[i]);
+
+		if (!libxfs_verify_agbno(mp, agno, agbno)) {
+			do_warn(
+	_("bad btree pointer (%u) in %sbt block %u/%u\n"),
+				agbno, name, agno, bno);
+			suspect++;
+			return;
+		}
+
+		scan_sbtree(be32_to_cpu(pp[i]), level, agno, suspect,
+				scan_inobt, 0, magic, priv, ops);
 	}
 }
 

Re: [PATCH 08/16] xfs_repair: complain about bad interior btree pointers

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:30:48AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Actually complain about garbage btree node pointers, don't just silently
> ignore them.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 09/16] xfs_repair: convert to libxfs_verify_agbno

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert the homegrown verify_agbno callers to use the libxfs function,
as needed.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/dinode.c          |   11 -----------
 repair/dinode.h          |    5 -----
 repair/scan.c            |   36 +++++++++++++++++++++++-------------
 4 files changed, 24 insertions(+), 29 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 7b264ff2..c03f0efa 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -20,6 +20,7 @@
 #define xfs_agfl_walk			libxfs_agfl_walk
 
 #define xfs_ag_init_headers		libxfs_ag_init_headers
+#define xfs_ag_block_count		libxfs_ag_block_count
 
 #define xfs_alloc_ag_max_usable		libxfs_alloc_ag_max_usable
 #define xfs_allocbt_maxrecs		libxfs_allocbt_maxrecs
diff --git a/repair/dinode.c b/repair/dinode.c
index 1f1cc26b..b343534c 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -255,17 +255,6 @@ verify_dfsbno_range(xfs_mount_t	*mp,
 	return (XR_DFSBNORANGE_VALID);
 }
 
-int
-verify_agbno(xfs_mount_t	*mp,
-		xfs_agnumber_t	agno,
-		xfs_agblock_t	agbno)
-{
-	xfs_sb_t	*sbp = &mp->m_sb;;
-
-	/* range check ag #, ag block.  range-checking offset is pointless */
-	return verify_ag_bno(sbp, agno, agbno) == 0;
-}
-
 static int
 process_rt_rec(
 	xfs_mount_t		*mp,
diff --git a/repair/dinode.h b/repair/dinode.h
index 98238357..c8e563b5 100644
--- a/repair/dinode.h
+++ b/repair/dinode.h
@@ -9,11 +9,6 @@
 struct blkmap;
 struct prefetch_args;
 
-int
-verify_agbno(xfs_mount_t	*mp,
-		xfs_agnumber_t	agno,
-		xfs_agblock_t	agbno);
-
 int
 verify_dfsbno(xfs_mount_t	*mp,
 		xfs_fsblock_t	fsbno);
diff --git a/repair/scan.c b/repair/scan.c
index 719ad035..8e81c552 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -678,14 +678,14 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			len = be32_to_cpu(rp[i].ar_blockcount);
 			end = b + len;
 
-			if (b == 0 || !verify_agbno(mp, agno, b)) {
+			if (!libxfs_verify_agbno(mp, agno, b)) {
 				do_warn(
 	_("invalid start block %u in record %u of %s btree block %u/%u\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
 			if (len == 0 || end <= b ||
-			    !verify_agbno(mp, agno, end - 1)) {
+			    !libxfs_verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -950,6 +950,16 @@ rmap_in_order(
 	return offset > lastoffset;
 }
 
+static inline bool
+verify_rmap_agbno(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	xfs_agblock_t		agbno)
+{
+	return agbno < libxfs_ag_block_count(mp, agno);
+}
+
+
 static void
 scan_rmapbt(
 	struct xfs_btree_block	*block,
@@ -1068,14 +1078,14 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			end = key.rm_startblock + key.rm_blockcount;
 
 			/* Make sure agbno & len make sense. */
-			if (!verify_agbno(mp, agno, b)) {
+			if (!verify_rmap_agbno(mp, agno, b)) {
 				do_warn(
 	_("invalid start block %u in record %u of %s btree block %u/%u\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
 			if (len == 0 || end <= b ||
-			    !verify_agbno(mp, agno, end - 1)) {
+			    !verify_rmap_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -1363,14 +1373,14 @@ _("leftover CoW extent has invalid startblock in record %u of %s btree block %u/
 			}
 			end = agb + len;
 
-			if (!verify_agbno(mp, agno, agb)) {
+			if (!libxfs_verify_agbno(mp, agno, agb)) {
 				do_warn(
 	_("invalid start block %u in record %u of %s btree block %u/%u\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
 			if (len == 0 || end <= agb ||
-			    !verify_agbno(mp, agno, end - 1)) {
+			    !libxfs_verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -2171,7 +2181,7 @@ scan_agfl(
 {
 	struct agfl_state	*as = priv;
 
-	if (verify_agbno(mp, as->agno, bno))
+	if (libxfs_verify_agbno(mp, as->agno, bno))
 		set_bmap(as->agno, bno, XR_E_FREE);
 	else
 		do_warn(_("bad agbno %u in agfl, agno %d\n"),
@@ -2244,7 +2254,7 @@ validate_agf(
 	uint32_t		magic;
 
 	bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_BNO]);
-	if (bno != 0 && verify_agbno(mp, agno, bno)) {
+	if (libxfs_verify_agbno(mp, agno, bno)) {
 		magic = xfs_sb_version_hascrc(&mp->m_sb) ? XFS_ABTB_CRC_MAGIC
 							 : XFS_ABTB_MAGIC;
 		scan_sbtree(bno, be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]),
@@ -2256,7 +2266,7 @@ validate_agf(
 	}
 
 	bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_CNT]);
-	if (bno != 0 && verify_agbno(mp, agno, bno)) {
+	if (libxfs_verify_agbno(mp, agno, bno)) {
 		magic = xfs_sb_version_hascrc(&mp->m_sb) ? XFS_ABTC_CRC_MAGIC
 							 : XFS_ABTC_MAGIC;
 		scan_sbtree(bno, be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]),
@@ -2276,7 +2286,7 @@ validate_agf(
 		priv.last_rec.rm_owner = XFS_RMAP_OWN_UNKNOWN;
 		priv.nr_blocks = 0;
 		bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_RMAP]);
-		if (bno != 0 && verify_agbno(mp, agno, bno)) {
+		if (libxfs_verify_agbno(mp, agno, bno)) {
 			scan_sbtree(bno,
 				    be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]),
 				    agno, 0, scan_rmapbt, 1, XFS_RMAP_CRC_MAGIC,
@@ -2294,7 +2304,7 @@ validate_agf(
 
 	if (xfs_sb_version_hasreflink(&mp->m_sb)) {
 		bno = be32_to_cpu(agf->agf_refcount_root);
-		if (bno != 0 && verify_agbno(mp, agno, bno)) {
+		if (libxfs_verify_agbno(mp, agno, bno)) {
 			struct refc_priv	priv;
 
 			memset(&priv, 0, sizeof(priv));
@@ -2342,7 +2352,7 @@ validate_agi(
 	uint32_t		magic;
 
 	bno = be32_to_cpu(agi->agi_root);
-	if (bno != 0 && verify_agbno(mp, agno, bno)) {
+	if (libxfs_verify_agbno(mp, agno, bno)) {
 		magic = xfs_sb_version_hascrc(&mp->m_sb) ? XFS_IBT_CRC_MAGIC
 							 : XFS_IBT_MAGIC;
 		scan_sbtree(bno, be32_to_cpu(agi->agi_level),
@@ -2355,7 +2365,7 @@ validate_agi(
 
 	if (xfs_sb_version_hasfinobt(&mp->m_sb)) {
 		bno = be32_to_cpu(agi->agi_free_root);
-		if (bno != 0 && verify_agbno(mp, agno, bno)) {
+		if (libxfs_verify_agbno(mp, agno, bno)) {
 			magic = xfs_sb_version_hascrc(&mp->m_sb) ?
 					XFS_FIBT_CRC_MAGIC : XFS_FIBT_MAGIC;
 			scan_sbtree(bno, be32_to_cpu(agi->agi_free_level),

Re: [PATCH 09/16] xfs_repair: convert to libxfs_verify_agbno

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:30:54AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Convert the homegrown verify_agbno callers to use the libxfs function,
> as needed.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

This looks mostly good, but there is one thing I wonder about:

>  	bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_BNO]);
> -	if (bno != 0 && verify_agbno(mp, agno, bno)) {
> +	if (libxfs_verify_agbno(mp, agno, bno)) {

Various of these block is non-zero check are going away.  Did you
audit if they weren't used as intentional escapes in a few places?

Either way this should probably be documented in the changelog.

Re: [PATCH 09/16] xfs_repair: convert to libxfs_verify_agbno

[Darrick J. Wong]

On Sat, May 09, 2020 at 10:18:30AM -0700, Christoph Hellwig wrote:
> On Sat, May 09, 2020 at 09:30:54AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Convert the homegrown verify_agbno callers to use the libxfs function,
> > as needed.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> This looks mostly good, but there is one thing I wonder about:
> 
> >  	bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_BNO]);
> > -	if (bno != 0 && verify_agbno(mp, agno, bno)) {
> > +	if (libxfs_verify_agbno(mp, agno, bno)) {
> 
> Various of these block is non-zero check are going away.  Did you
> audit if they weren't used as intentional escapes in a few places?

Yes.  Each of those "bno != 0" checks occurs in the context of checking
an AG header's pointer to a btree root.  The roots should never be zero
if the corresponding feature is enabled, and we're careful to check the
feature bits first.

AFAICT that bno != 0 check is actually there to cover a deficiency in
the verify_agbno function, which is that it only checked that the
supplied argument didn't go past the end of the AG and did not check
that the pointer didn't point into the AG header block(s).

Checking for a nonzero value is also insufficient, since on a
blocksize < sectorsize * 4 filesystem, the AGFL can end up in a nonzero
agbno.  libxfs_verify_agbno covers all of these cases.

> Either way this should probably be documented in the changelog.

Ok, how about this for a commit message:

"Convert the homegrown verify_agbno callers to use the libxfs function,
as needed.  In some places we drop the "bno != 0" checks because those
conditionals are checking btree roots; btree roots should never be
zero if the corresponding feature bit is set; and repair skips the if
clause entirely if the feature bit is disabled.

"In effect, this strengthens repair to validate that AG btree pointers
neither point to the AG headers nor point past the end of the AG."

--D

Re: [PATCH 09/16] xfs_repair: convert to libxfs_verify_agbno

[Christoph Hellwig]

On Mon, May 11, 2020 at 09:22:03AM -0700, Darrick J. Wong wrote:
> Yes.  Each of those "bno != 0" checks occurs in the context of checking
> an AG header's pointer to a btree root.  The roots should never be zero
> if the corresponding feature is enabled, and we're careful to check the
> feature bits first.
> 
> AFAICT that bno != 0 check is actually there to cover a deficiency in
> the verify_agbno function, which is that it only checked that the
> supplied argument didn't go past the end of the AG and did not check
> that the pointer didn't point into the AG header block(s).
> 
> Checking for a nonzero value is also insufficient, since on a
> blocksize < sectorsize * 4 filesystem, the AGFL can end up in a nonzero
> agbno.  libxfs_verify_agbno covers all of these cases.
> 
> > Either way this should probably be documented in the changelog.
> 
> Ok, how about this for a commit message:
> 
> "Convert the homegrown verify_agbno callers to use the libxfs function,
> as needed.  In some places we drop the "bno != 0" checks because those
> conditionals are checking btree roots; btree roots should never be
> zero if the corresponding feature bit is set; and repair skips the if
> clause entirely if the feature bit is disabled.
> 
> "In effect, this strengthens repair to validate that AG btree pointers
> neither point to the AG headers nor point past the end of the AG."

With the additional explanation in the commit log:

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

[PATCH 10/16] xfs_repair: refactor verify_dfsbno_range

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor this function to use libxfs type checking helpers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/dinode.c          |   26 +++++++++-----------------
 2 files changed, 10 insertions(+), 17 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index c03f0efa..69f79a08 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -184,6 +184,7 @@
 #define xfs_verify_agbno		libxfs_verify_agbno
 #define xfs_verify_cksum		libxfs_verify_cksum
 #define xfs_verify_dir_ino		libxfs_verify_dir_ino
+#define xfs_verify_fsbno		libxfs_verify_fsbno
 #define xfs_verify_ino			libxfs_verify_ino
 #define xfs_verify_rtbno		libxfs_verify_rtbno
 #define xfs_zero_extent			libxfs_zero_extent
diff --git a/repair/dinode.c b/repair/dinode.c
index b343534c..135703d9 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -228,31 +228,23 @@ verify_dfsbno(xfs_mount_t	*mp,
 #define XR_DFSBNORANGE_OVERFLOW	3
 
 static __inline int
-verify_dfsbno_range(xfs_mount_t	*mp,
-		xfs_fsblock_t	fsbno,
-		xfs_filblks_t	count)
+verify_dfsbno_range(
+	struct xfs_mount	*mp,
+	xfs_fsblock_t		fsbno,
+	xfs_filblks_t		count)
 {
-	xfs_agnumber_t	agno;
-	xfs_agblock_t	agbno;
-	xfs_sb_t	*sbp = &mp->m_sb;;
-
 	/* the start and end blocks better be in the same allocation group */
-	agno = XFS_FSB_TO_AGNO(mp, fsbno);
-	if (agno != XFS_FSB_TO_AGNO(mp, fsbno + count - 1)) {
+	if (XFS_FSB_TO_AGNO(mp, fsbno) !=
+	    XFS_FSB_TO_AGNO(mp, fsbno + count - 1)) {
 		return XR_DFSBNORANGE_OVERFLOW;
 	}
 
-	agbno = XFS_FSB_TO_AGBNO(mp, fsbno);
-	if (verify_ag_bno(sbp, agno, agbno)) {
+	if (!libxfs_verify_fsbno(mp, fsbno))
 		return XR_DFSBNORANGE_BADSTART;
-	}
-
-	agbno = XFS_FSB_TO_AGBNO(mp, fsbno + count - 1);
-	if (verify_ag_bno(sbp, agno, agbno)) {
+	if (!libxfs_verify_fsbno(mp, fsbno + count - 1))
 		return XR_DFSBNORANGE_BADEND;
-	}
 
-	return (XR_DFSBNORANGE_VALID);
+	return XR_DFSBNORANGE_VALID;
 }
 
 static int

Re: [PATCH 10/16] xfs_repair: refactor verify_dfsbno_range

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:31:00AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Refactor this function to use libxfs type checking helpers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 11/16] xfs_repair: remove verify_dfsbno

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Replace this homegrown helper with its libxfs equivalent.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/attr_repair.c |    2 +-
 repair/dinode.c      |   21 +--------------------
 repair/dinode.h      |    4 ----
 repair/prefetch.c    |    9 +++++----
 repair/scan.c        |    2 +-
 5 files changed, 8 insertions(+), 30 deletions(-)


diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 5f884033..6cec0f70 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -1092,7 +1092,7 @@ process_longform_attr(
 	}
 
 	/* FIX FOR bug 653709 -- EKN */
-	if (!xfs_verify_fsbno(mp, bno)) {
+	if (!libxfs_verify_fsbno(mp, bno)) {
 		do_warn(
 	_("block in attribute fork of inode %" PRIu64 " is not valid\n"), ino);
 		return 1;
diff --git a/repair/dinode.c b/repair/dinode.c
index 135703d9..67adddd7 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -203,25 +203,6 @@ verify_aginum(xfs_mount_t	*mp,
 	return verify_ag_bno(sbp, agno, agbno);
 }
 
-/*
- * return 1 if block number is good, 0 if out of range
- */
-int
-verify_dfsbno(xfs_mount_t	*mp,
-		xfs_fsblock_t	fsbno)
-{
-	xfs_agnumber_t	agno;
-	xfs_agblock_t	agbno;
-	xfs_sb_t	*sbp = &mp->m_sb;;
-
-	/* range check ag #, ag block.  range-checking offset is pointless */
-
-	agno = XFS_FSB_TO_AGNO(mp, fsbno);
-	agbno = XFS_FSB_TO_AGBNO(mp, fsbno);
-
-	return verify_ag_bno(sbp, agno, agbno) == 0;
-}
-
 #define XR_DFSBNORANGE_VALID	0
 #define XR_DFSBNORANGE_BADSTART	1
 #define XR_DFSBNORANGE_BADEND	2
@@ -835,7 +816,7 @@ _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"),
 		 * btree, we'd do it right here.  For now, if there's a
 		 * problem, we'll bail out and presumably clear the inode.
 		 */
-		if (!verify_dfsbno(mp, get_unaligned_be64(&pp[i])))  {
+		if (!libxfs_verify_fsbno(mp, get_unaligned_be64(&pp[i])))  {
 			do_warn(
 _("bad bmap btree ptr 0x%" PRIx64 " in ino %" PRIu64 "\n"),
 				get_unaligned_be64(&pp[i]), lino);
diff --git a/repair/dinode.h b/repair/dinode.h
index c8e563b5..4bf7affd 100644
--- a/repair/dinode.h
+++ b/repair/dinode.h
@@ -9,10 +9,6 @@
 struct blkmap;
 struct prefetch_args;
 
-int
-verify_dfsbno(xfs_mount_t	*mp,
-		xfs_fsblock_t	fsbno);
-
 void
 convert_extent(
 	xfs_bmbt_rec_t		*rp,
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 3ac49db1..686bf7be 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -188,8 +188,9 @@ pf_read_bmbt_reclist(
 				(irec.br_startoff >= fs_max_file_offset))
 			goto out_free;
 
-		if (!verify_dfsbno(mp, irec.br_startblock) || !verify_dfsbno(mp,
-				irec.br_startblock + irec.br_blockcount - 1))
+		if (!libxfs_verify_fsbno(mp, irec.br_startblock) ||
+		    !libxfs_verify_fsbno(mp, irec.br_startblock +
+					     irec.br_blockcount - 1))
 			goto out_free;
 
 		if (!args->dirs_only && ((irec.br_startoff +
@@ -337,7 +338,7 @@ pf_scanfunc_bmap(
 
 	for (i = 0; i < numrecs; i++) {
 		dbno = get_unaligned_be64(&pp[i]);
-		if (!verify_dfsbno(mp, dbno))
+		if (!libxfs_verify_fsbno(mp, dbno))
 			return 0;
 		if (!pf_scan_lbtree(dbno, level, isadir, args, pf_scanfunc_bmap))
 			return 0;
@@ -379,7 +380,7 @@ pf_read_btinode(
 
 	for (i = 0; i < numrecs; i++) {
 		dbno = get_unaligned_be64(&pp[i]);
-		if (!verify_dfsbno(mp, dbno))
+		if (!libxfs_verify_fsbno(mp, dbno))
 			break;
 		if (!pf_scan_lbtree(dbno, level, isadir, args, pf_scanfunc_bmap))
 			break;
diff --git a/repair/scan.c b/repair/scan.c
index 8e81c552..dcd4864d 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -491,7 +491,7 @@ _("inode %" PRIu64 " bad # of bmap records (%u, min - %u, max - %u)\n"),
 		 * we'd do it right here.  For now, if there's a problem,
 		 * we'll bail out and presumably clear the inode.
 		 */
-		if (!verify_dfsbno(mp, be64_to_cpu(pp[i])))  {
+		if (!libxfs_verify_fsbno(mp, be64_to_cpu(pp[i])))  {
 			do_warn(
 _("bad bmap btree ptr 0x%llx in ino %" PRIu64 "\n"),
 			       (unsigned long long) be64_to_cpu(pp[i]), ino);

Re: [PATCH 11/16] xfs_repair: remove verify_dfsbno

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:31:07AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Replace this homegrown helper with its libxfs equivalent.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 12/16] xfs_repair: remove verify_aginum

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Replace this homegrown inode pointer verification function with the
libxfs checking helper.  This one is a little tricky because this
function (unlike all of its verify_* siblings) returned 1 for bad and 0
for good, so we must invert the checking logic.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/dino_chunks.c     |    6 +++--
 repair/dinode.c          |   51 ----------------------------------------------
 repair/dinode.h          |    5 -----
 repair/scan.c            |    4 ++--
 5 files changed, 6 insertions(+), 61 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 69f79a08..be06c763 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -182,6 +182,7 @@
 #define xfs_trans_roll			libxfs_trans_roll
 
 #define xfs_verify_agbno		libxfs_verify_agbno
+#define xfs_verify_agino		libxfs_verify_agino
 #define xfs_verify_cksum		libxfs_verify_cksum
 #define xfs_verify_dir_ino		libxfs_verify_dir_ino
 #define xfs_verify_fsbno		libxfs_verify_fsbno
diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 6685a4d2..399d4998 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -1124,7 +1124,7 @@ check_uncertain_aginodes(xfs_mount_t *mp, xfs_agnumber_t agno)
 
 			agino = i + irec->ino_startnum;
 
-			if (verify_aginum(mp, agno, agino))
+			if (!libxfs_verify_agino(mp, agno, agino))
 				continue;
 
 			if (nrec != NULL && nrec->ino_startnum <= agino &&
@@ -1133,7 +1133,7 @@ check_uncertain_aginodes(xfs_mount_t *mp, xfs_agnumber_t agno)
 				continue;
 
 			if ((nrec = find_inode_rec(mp, agno, agino)) == NULL)
-				if (!verify_aginum(mp, agno, agino))
+				if (libxfs_verify_agino(mp, agno, agino))
 					if (verify_aginode_chunk(mp, agno,
 							agino, &start))
 						got_some = 1;
@@ -1215,7 +1215,7 @@ process_uncertain_aginodes(xfs_mount_t *mp, xfs_agnumber_t agno)
 			 * good tree), bad inode numbers, and inode numbers
 			 * pointing to bogus inodes
 			 */
-			if (verify_aginum(mp, agno, agino))
+			if (!libxfs_verify_agino(mp, agno, agino))
 				continue;
 
 			if (nrec != NULL && nrec->ino_startnum <= agino &&
diff --git a/repair/dinode.c b/repair/dinode.c
index 67adddd7..526ecde3 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -152,57 +152,6 @@ clear_dinode(xfs_mount_t *mp, xfs_dinode_t *dino, xfs_ino_t ino_num)
  * misc. inode-related utility routines
  */
 
-/*
- * verify_ag_bno is heavily used. In the common case, it
- * performs just two number of compares
- * Returns 1 for bad ag/bno pair or 0 if it's valid.
- */
-static __inline int
-verify_ag_bno(xfs_sb_t *sbp,
-		xfs_agnumber_t agno,
-		xfs_agblock_t agbno)
-{
-	if (agno < (sbp->sb_agcount - 1))
-		return (agbno >= sbp->sb_agblocks);
-	if (agno == (sbp->sb_agcount - 1))
-		return (agbno >= (sbp->sb_dblocks -
-				((xfs_rfsblock_t)(sbp->sb_agcount - 1) *
-				 sbp->sb_agblocks)));
-	return 1;
-}
-
-/*
- * have a separate routine to ensure that we don't accidentally
- * lose illegally set bits in the agino by turning it into an FSINO
- * to feed to the above routine
- */
-int
-verify_aginum(xfs_mount_t	*mp,
-		xfs_agnumber_t	agno,
-		xfs_agino_t	agino)
-{
-	xfs_agblock_t	agbno;
-	xfs_sb_t	*sbp = &mp->m_sb;;
-
-	/* range check ag #, ag block.  range-checking offset is pointless */
-
-	if (agino == 0 || agino == NULLAGINO)
-		return(1);
-
-	/*
-	 * agino's can't be too close to NULLAGINO because the min blocksize
-	 * is 9 bits and at most 1 bit of that gets used for the inode offset
-	 * so if the agino gets shifted by the # of offset bits and compared
-	 * to the legal agbno values, a bogus agino will be too large.  there
-	 * will be extra bits set at the top that shouldn't be set.
-	 */
-	agbno = XFS_AGINO_TO_AGBNO(mp, agino);
-	if (agbno == 0)
-		return 1;
-
-	return verify_ag_bno(sbp, agno, agbno);
-}
-
 #define XR_DFSBNORANGE_VALID	0
 #define XR_DFSBNORANGE_BADSTART	1
 #define XR_DFSBNORANGE_BADEND	2
diff --git a/repair/dinode.h b/repair/dinode.h
index 4bf7affd..1bd0e0b7 100644
--- a/repair/dinode.h
+++ b/repair/dinode.h
@@ -68,11 +68,6 @@ verify_uncertain_dinode(xfs_mount_t *mp,
 		xfs_agnumber_t agno,
 		xfs_agino_t ino);
 
-int
-verify_aginum(xfs_mount_t	*mp,
-		xfs_agnumber_t	agno,
-		xfs_agino_t	agino);
-
 int
 process_uncertain_aginodes(xfs_mount_t		*mp,
 				xfs_agnumber_t	agno);
diff --git a/repair/scan.c b/repair/scan.c
index dcd4864d..76079247 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -1561,7 +1561,7 @@ verify_single_ino_chunk_align(
 	 * (NULLAGINO). if it gets closer, the agino number will be illegal as
 	 * the agbno will be too large.
 	 */
-	if (verify_aginum(mp, agno, ino)) {
+	if (!libxfs_verify_agino(mp, agno, ino)) {
 		do_warn(
 _("bad starting inode # (%" PRIu64 " (0x%x 0x%x)) in %s rec, skipping rec\n"),
 			lino, agno, ino, inobt_name);
@@ -1569,7 +1569,7 @@ _("bad starting inode # (%" PRIu64 " (0x%x 0x%x)) in %s rec, skipping rec\n"),
 		return ++suspect;
 	}
 
-	if (verify_aginum(mp, agno,
+	if (!libxfs_verify_agino(mp, agno,
 			ino + XFS_INODES_PER_CHUNK - 1)) {
 		do_warn(
 _("bad ending inode # (%" PRIu64 " (0x%x 0x%zx)) in %s rec, skipping rec\n"),

Re: [PATCH 12/16] xfs_repair: remove verify_aginum

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:31:14AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Replace this homegrown inode pointer verification function with the
> libxfs checking helper.  This one is a little tricky because this
> function (unlike all of its verify_* siblings) returned 1 for bad and 0
> for good, so we must invert the checking logic.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 13/16] xfs_repair: mark entire free space btree record as free1

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In scan_allocbt, we iterate each free space btree record (of both bnobt
and cntbt) in the hopes of pushing all the free space from UNKNOWN to
FREE1 to FREE.  Unfortunately, the first time we see a free space record
we only set the first block of that record to FREE1, which means that
the second time we see the record, the first block will get set to FREE,
but the rest of the free space will only make it to FREE1.  This is
incorrect state, so we need to fix that.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/scan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/repair/scan.c b/repair/scan.c
index 76079247..505cfc53 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -719,7 +719,7 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 				state = get_bmap_ext(agno, b, end, &blen);
 				switch (state) {
 				case XR_E_UNKNOWN:
-					set_bmap(agno, b, XR_E_FREE1);
+					set_bmap_ext(agno, b, blen, XR_E_FREE1);
 					break;
 				case XR_E_FREE1:
 					/*

Re: [PATCH 13/16] xfs_repair: mark entire free space btree record as free1

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:31:20AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In scan_allocbt, we iterate each free space btree record (of both bnobt
> and cntbt) in the hopes of pushing all the free space from UNKNOWN to
> FREE1 to FREE.  Unfortunately, the first time we see a free space record
> we only set the first block of that record to FREE1, which means that
> the second time we see the record, the first block will get set to FREE,
> but the rest of the free space will only make it to FREE1.  This is
> incorrect state, so we need to fix that.

That sounds pretty bad..

The fix looks good:

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

[PATCH 14/16] xfs_repair: complain about free space only seen by one btree

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

During the free space btree walk, scan_allocbt claims in a comment that
we'll catch FREE1 blocks (i.e. blocks that were seen by only one free
space btree) later.  This never happens, with the result that xfs_repair
in dry-run mode can return 0 on a filesystem with corrupt free space
btrees.

Found by fuzzing xfs/358 with numrecs = middlebit (or sub).

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/phase4.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


diff --git a/repair/phase4.c b/repair/phase4.c
index 8197db06..a43413c7 100644
--- a/repair/phase4.c
+++ b/repair/phase4.c
@@ -306,6 +306,12 @@ phase4(xfs_mount_t *mp)
 		for (j = ag_hdr_block; j < ag_end; j += blen)  {
 			bstate = get_bmap_ext(i, j, ag_end, &blen);
 			switch (bstate) {
+			case XR_E_FREE1:
+				if (no_modify)
+					do_warn(
+	_("free space (%u,%u-%u) only seen by one free space btree\n"),
+						i, j, j + blen - 1);
+				break;
 			case XR_E_BAD_STATE:
 			default:
 				do_warn(
@@ -313,7 +319,6 @@ phase4(xfs_mount_t *mp)
 					i, j);
 				/* fall through .. */
 			case XR_E_UNKNOWN:
-			case XR_E_FREE1:
 			case XR_E_FREE:
 			case XR_E_INUSE:
 			case XR_E_INUSE_FS:

Re: [PATCH 14/16] xfs_repair: complain about free space only seen by one btree

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:31:26AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> During the free space btree walk, scan_allocbt claims in a comment that
> we'll catch FREE1 blocks (i.e. blocks that were seen by only one free
> space btree) later.  This never happens, with the result that xfs_repair
> in dry-run mode can return 0 on a filesystem with corrupt free space
> btrees.
> 
> Found by fuzzing xfs/358 with numrecs = middlebit (or sub).
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 15/16] xfs_repair: complain about extents in unknown state

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

During phase 4, if we find any extents that are unaccounted for, report
the entire extent, not just the first block.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/phase4.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/repair/phase4.c b/repair/phase4.c
index a43413c7..191b4842 100644
--- a/repair/phase4.c
+++ b/repair/phase4.c
@@ -315,8 +315,8 @@ phase4(xfs_mount_t *mp)
 			case XR_E_BAD_STATE:
 			default:
 				do_warn(
-				_("unknown block state, ag %d, block %d\n"),
-					i, j);
+				_("unknown block state, ag %d, blocks %u-%u\n"),
+					i, j, j + blen - 1);
 				/* fall through .. */
 			case XR_E_UNKNOWN:
 			case XR_E_FREE:

Re: [PATCH 15/16] xfs_repair: complain about extents in unknown state

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:31:32AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> During phase 4, if we find any extents that are unaccounted for, report
> the entire extent, not just the first block.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 16/16] xfs_repair: complain about any nonzero inprogress value, not just 1

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Complain about the primary superblock having any non-zero sb_inprogress
value, not just 1.  This brings repair's behavior into alignment with
xfs_check and the kernel.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/sb.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


diff --git a/repair/sb.c b/repair/sb.c
index 91a36dd3..17ce43cc 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -369,8 +369,7 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
 		return(XR_BAD_VERSION);
 
 	/* does sb think mkfs really finished ? */
-
-	if (is_primary_sb && sb->sb_inprogress == 1)
+	if (is_primary_sb && sb->sb_inprogress)
 		return(XR_BAD_INPROGRESS);
 
 	/*

Re: [PATCH 16/16] xfs_repair: complain about any nonzero inprogress value, not just 1

[Christoph Hellwig]

On Sat, May 09, 2020 at 09:31:38AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Complain about the primary superblock having any non-zero sb_inprogress
> value, not just 1.  This brings repair's behavior into alignment with
> xfs_check and the kernel.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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