[PATCH 0/7] xfs_repair: more fuzzer fixes

[PATCH 0/7] xfs_repair: more fuzzer fixes

[Darrick J. Wong]

Hi all,

I've had enough spare time to start another round of fixing things that
the fuzz fstests found.  All of these patches heal crashes that
xfs_repair experiences on fuzzed filesystems.

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=repair-fixes
---
 repair/attr_repair.c |    9 ++++++
 repair/dino_chunks.c |   54 ++++++++++++++++++++++++++++++++++++
 repair/dinode.c      |   74 +++++++++++++++++++++++++++++---------------------
 repair/dir2.c        |    2 +
 repair/rmap.c        |    1 +
 5 files changed, 107 insertions(+), 33 deletions(-)

[PATCH 1/7] xfs_repair: don't crash on partially sparse inode clusters

[Darrick J. Wong]

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

While running xfs/364 to fuzz the middle bit of recs[2].holemask, I
observed a crash in xfs_repair stemming from the fact that each sparse
bit accounts for 4 inodes, but inode cluster buffers can map to more
than four inodes.

When the first inode in an inode cluster is marked sparse,
process_inode_chunk won't try to load the inode cluster buffer.
Unfortunately, if the holemask indicates that there are inodes present
anywhere in the rest of the cluster buffer, repair will try to check the
corresponding cluster buffer, even if we didn't load it.  This leads to
a null pointer dereference, which crashes repair.

Avoid the null pointer dereference by marking the inode sparse and
moving on to the next inode.

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


diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 96e2c1708b94..50a2003614df 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -792,6 +792,25 @@ process_inode_chunk(
 		if (is_inode_sparse(ino_rec, irec_offset))
 			goto process_next;
 
+		/*
+		 * Repair skips reading the cluster buffer if the first inode
+		 * in the cluster is marked as sparse.  If subsequent inodes in
+		 * the cluster buffer are /not/ marked sparse, there won't be
+		 * a buffer, so we need to avoid the null pointer dereference.
+		 */
+		if (bplist[bp_index] == NULL) {
+			do_warn(
+	_("imap claims inode %" PRIu64 " is present, but inode cluster is sparse, "),
+						ino);
+			if (verbose || !no_modify)
+				do_warn(_("correcting imap\n"));
+			else
+				do_warn(_("would correct imap\n"));
+			set_inode_sparse(ino_rec, irec_offset);
+			set_inode_free(ino_rec, irec_offset);
+			goto process_next;
+		}
+
 		/* make inode pointer */
 		dino = xfs_make_iptr(mp, bplist[bp_index], cluster_offset);
 

[PATCH 2/7] xfs_repair: fix error in process_sf_dir2_fixi8

[Darrick J. Wong]

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

The goal of process_sf_dir2_fixi8 is to convert an i8 shortform
directory into a (shorter) i4 shortform directory.  It achieves this by
duplicating the old sf directory contents (as oldsfp), zeroing i8count
in the caller's directory buffer (i.e. newsfp/sfp), and reinitializing
the new directory with the old directory's entries.

Unfortunately, it copies the parent pointer from sfp (the buffer we've
already started changing), not oldsfp.  This leads to directory
corruption since at that point we zeroed i8count, which means that we
save only the upper four bytes from the parent pointer entry.

This was found by fuzzing u3.sfdir3.hdr.i8count = ones in xfs/384.

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


diff --git a/repair/dir2.c b/repair/dir2.c
index 95e8c2009d1f..eabdb4f2d497 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -84,7 +84,7 @@ process_sf_dir2_fixi8(
 	memmove(oldsfp, newsfp, oldsize);
 	newsfp->count = oldsfp->count;
 	newsfp->i8count = 0;
-	ino = libxfs_dir2_sf_get_parent_ino(sfp);
+	ino = libxfs_dir2_sf_get_parent_ino(oldsfp);
 	libxfs_dir2_sf_put_parent_ino(newsfp, ino);
 	oldsfep = xfs_dir2_sf_firstentry(oldsfp);
 	newsfep = xfs_dir2_sf_firstentry(newsfp);

[PATCH 3/7] xfs_repair: junk corrupt xattr root blocks

[Darrick J. Wong]

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

If reading the root block of an extended attribute structure fails due
to a corruption error, we should junk the block since we know it's bad.
There's no point in moving on to the (rather insufficient) checks in the
attr code.

Found by fuzzing hdr.freemap[1].base = ones in xfs/400.

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


diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 6cec0f7075d5..d92909e1c831 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -1107,6 +1107,15 @@ process_longform_attr(
 			ino);
 		return 1;
 	}
+
+	if (bp->b_error == -EFSCORRUPTED) {
+		do_warn(
+	_("corrupt block 0 of inode %" PRIu64 " attribute fork\n"),
+			ino);
+		libxfs_buf_relse(bp);
+		return 1;
+	}
+
 	if (bp->b_error == -EFSBADCRC)
 		(*repair)++;
 

[PATCH 4/7] xfs_repair: complain about unwritten extents when they're not appropriate

[Darrick J. Wong]

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

We don't allow unwritten extents in the attr fork, and we don't allow
them in the data fork except for regular files.  Check that this is the
case.

Found by manually fuzzing the extentflag field of an attr fork to one.

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


diff --git a/repair/dinode.c b/repair/dinode.c
index d552db2d5f1a..1fe68bd41117 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -347,6 +347,28 @@ _("bmap rec out of order, inode %" PRIu64" entry %d "
 		cp = irec.br_blockcount;
 		sp = irec.br_startblock;
 
+		if (irec.br_state != XFS_EXT_NORM) {
+			/* No unwritten extents in the attr fork */
+			if (whichfork == XFS_ATTR_FORK) {
+				do_warn(
+_("unwritten extent (off = %" PRIu64 ", fsbno = %" PRIu64 ") in ino %" PRIu64 " attr fork\n"),
+					irec.br_startoff,
+					irec.br_startblock,
+					ino);
+				goto done;
+			}
+
+			/* No unwritten extents in non-regular files */
+			if (type != XR_INO_DATA && type != XR_INO_RTDATA) {
+				do_warn(
+_("unwritten extent (off = %" PRIu64 ", fsbno = %" PRIu64 ") in non-regular file ino %" PRIu64 "\n"),
+					irec.br_startoff,
+					irec.br_startblock,
+					ino);
+				goto done;
+			}
+		}
+
 		/*
 		 * check numeric validity of the extent
 		 */

[PATCH 5/7] xfs_repair: fix handling of data blocks colliding with existing metadata

[Darrick J. Wong]

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

Prior to commit a406779bc8d8, any blocks in a data fork extent that
collided with existing blocks would cause the entire data fork extent to
be rejected.  Unfortunately, the patch to add data block sharing support
suppressed checking for any collision, including metadata.  What we
really wanted to do here during a check_dups==1 scan is to is check for
specific collisions and without updating the block mapping data.

So, move the check_dups test after the for-switch construction.  This
re-enables detecting collisions between data fork blocks and a
previously scanned chunk of metadata, and improves the specificity of
the error message that results.

This was found by fuzzing recs[2].free=zeroes in xfs/364, though this
patch alone does not solve all the problems that scenario presents.

Fixes: a406779bc8d8 ("xfs_repair: handle multiple owners of data blocks")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/dinode.c |   33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)


diff --git a/repair/dinode.c b/repair/dinode.c
index 1fe68bd41117..7577b50ffb2b 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -476,28 +476,6 @@ _("Fatal error: inode %" PRIu64 " - blkmap_set_ext(): %s\n"
 			locked_agno = agno;
 		}
 
-		if (check_dups) {
-			/*
-			 * if we're just checking the bmap for dups,
-			 * return if we find one, otherwise, continue
-			 * checking each entry without setting the
-			 * block bitmap
-			 */
-			if (!(type == XR_INO_DATA &&
-			    xfs_sb_version_hasreflink(&mp->m_sb)) &&
-			    search_dup_extent(agno, agbno, ebno)) {
-				do_warn(
-_("%s fork in ino %" PRIu64 " claims dup extent, "
-  "off - %" PRIu64 ", start - %" PRIu64 ", cnt %" PRIu64 "\n"),
-					forkname, ino, irec.br_startoff,
-					irec.br_startblock,
-					irec.br_blockcount);
-				goto done;
-			}
-			*tot += irec.br_blockcount;
-			continue;
-		}
-
 		for (b = irec.br_startblock;
 		     agbno < ebno;
 		     b += blen, agbno += blen) {
@@ -554,6 +532,17 @@ _("illegal state %d in block map %" PRIu64 "\n"),
 			}
 		}
 
+		if (check_dups) {
+			/*
+			 * If we're just checking the bmap for dups and we
+			 * didn't find any non-reflink collisions, update our
+			 * inode's block count and move on to the next extent.
+			 * We're not yet updating the block usage information.
+			 */
+			*tot += irec.br_blockcount;
+			continue;
+		}
+
 		/*
 		 * Update the internal extent map only after we've checked
 		 * every block in this extent.  The first time we reject this

[PATCH 6/7] xfs_repair: throw away totally bad clusters

[Darrick J. Wong]

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

If the filesystem supports sparse inodes, we detect that an entire
cluster buffer has no detectable inodes at all, and we can easily mark
that part of the inode chunk sparse, just drop the cluster buffer and
forget about it.  This makes repair less likely to go to great lengths
to try to save something that's totally unsalvageable.

This manifested in recs[2].free=zeroes in xfs/364, wherein the root
directory claimed to own block X and the inobt also claimed that X was
inodes; repair tried to create rmaps for both owners, and then the whole
mess blew up because the rmap code aborts on those kinds of anomalies.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/dino_chunks.c |   35 ++++++++++++++++++++++++++++++++++-
 repair/rmap.c        |    1 +
 2 files changed, 35 insertions(+), 1 deletion(-)


diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 50a2003614df..e4a95ff635c8 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -601,6 +601,7 @@ process_inode_chunk(
 	xfs_dinode_t		*dino;
 	int			icnt;
 	int			status;
+	int			bp_found;
 	int			is_used;
 	int			ino_dirty;
 	int			irec_offset;
@@ -614,6 +615,7 @@ process_inode_chunk(
 	int			bp_index;
 	int			cluster_offset;
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
+	bool			can_punch_sparse = false;
 	int			error;
 
 	ASSERT(first_irec != NULL);
@@ -626,6 +628,10 @@ process_inode_chunk(
 	if (cluster_count == 0)
 		cluster_count = 1;
 
+	if (xfs_sb_version_hassparseinodes(&mp->m_sb) &&
+	    M_IGEO(mp)->inodes_per_cluster >= XFS_INODES_PER_HOLEMASK_BIT)
+		can_punch_sparse = true;
+
 	/*
 	 * get all blocks required to read in this chunk (may wind up
 	 * having to process more chunks in a multi-chunk per block fs)
@@ -700,6 +706,7 @@ process_inode_chunk(
 	cluster_offset = 0;
 	icnt = 0;
 	status = 0;
+	bp_found = 0;
 	bp_index = 0;
 
 	/*
@@ -725,8 +732,10 @@ process_inode_chunk(
 						(agno == 0 &&
 						(mp->m_sb.sb_rootino == agino ||
 						 mp->m_sb.sb_rsumino == agino ||
-						 mp->m_sb.sb_rbmino == agino)))
+						 mp->m_sb.sb_rbmino == agino))) {
 					status++;
+					bp_found++;
+				}
 			}
 
 			irec_offset++;
@@ -749,11 +758,35 @@ process_inode_chunk(
 				irec_offset = 0;
 			}
 			if (cluster_offset == M_IGEO(mp)->inodes_per_cluster) {
+				if (can_punch_sparse &&
+				    bplist[bp_index] != NULL &&
+				    bp_found == 0) {
+					/*
+					 * We didn't find any good inodes in
+					 * this cluster, blow it away before
+					 * moving on to the next one.
+					 */
+					libxfs_buf_relse(bplist[bp_index]);
+					bplist[bp_index] = NULL;
+				}
 				bp_index++;
 				cluster_offset = 0;
+				bp_found = 0;
 			}
 		}
 
+		if (can_punch_sparse &&
+		    bp_index < cluster_count &&
+		    bplist[bp_index] != NULL &&
+		    bp_found == 0) {
+			/*
+			 * We didn't find any good inodes in this cluster, blow
+			 * it away.
+			 */
+			libxfs_buf_relse(bplist[bp_index]);
+			bplist[bp_index] = NULL;
+		}
+
 		/*
 		 * if chunk/block is bad, blow it off.  the inode records
 		 * will be deleted by the caller if appropriate.
diff --git a/repair/rmap.c b/repair/rmap.c
index a4cc6a4937c9..98d2f46fa047 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -399,6 +399,7 @@ rmap_add_fixed_ag_rec(
 			nr = 1;
 		agino = ino_rec->ino_startnum + startidx;
 		agbno = XFS_AGINO_TO_AGBNO(mp, agino);
+		ASSERT(get_bmap(agno, agbno) == XR_E_INO);
 		if (XFS_AGINO_TO_OFFSET(mp, agino) == 0) {
 			error = rmap_add_ag_rec(mp, agno, agbno, nr,
 					XFS_RMAP_OWN_INODES);

[PATCH 7/7] xfs_repair: use libxfs_verify_rtbno to verify rt extents

[Darrick J. Wong]

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

Use the existing realtime block validation function to check the first
and last block of an extent in a realtime file.

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


diff --git a/repair/dinode.c b/repair/dinode.c
index 7577b50ffb2b..24ad6f0f071e 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -176,16 +176,15 @@ verify_dfsbno_range(
 
 	return XR_DFSBNORANGE_VALID;
 }
-
 static int
 process_rt_rec(
-	xfs_mount_t		*mp,
-	xfs_bmbt_irec_t 	*irec,
+	struct xfs_mount	*mp,
+	struct xfs_bmbt_irec	*irec,
 	xfs_ino_t		ino,
 	xfs_rfsblock_t		*tot,
 	int			check_dups)
 {
-	xfs_fsblock_t		b;
+	xfs_fsblock_t		b, lastb;
 	xfs_rtblock_t		ext;
 	int			state;
 	int			pwe;		/* partially-written extent */
@@ -193,7 +192,7 @@ process_rt_rec(
 	/*
 	 * check numeric validity of the extent
 	 */
-	if (irec->br_startblock >= mp->m_sb.sb_rblocks) {
+	if (!libxfs_verify_rtbno(mp, irec->br_startblock)) {
 		do_warn(
 _("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" PRIu64 "\n"),
 			ino,
@@ -201,21 +200,23 @@ _("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" P
 			irec->br_startoff);
 		return 1;
 	}
-	if (irec->br_startblock + irec->br_blockcount - 1 >= mp->m_sb.sb_rblocks) {
+
+	lastb = irec->br_startblock + irec->br_blockcount - 1;
+	if (!libxfs_verify_rtbno(mp, lastb)) {
 		do_warn(
 _("inode %" PRIu64 " - bad rt extent last block number %" PRIu64 ", offset %" PRIu64 "\n"),
 			ino,
-			irec->br_startblock + irec->br_blockcount - 1,
+			lastb,
 			irec->br_startoff);
 		return 1;
 	}
-	if (irec->br_startblock + irec->br_blockcount - 1 < irec->br_startblock) {
+	if (lastb < irec->br_startblock) {
 		do_warn(
 _("inode %" PRIu64 " - bad rt extent overflows - start %" PRIu64 ", "
   "end %" PRIu64 ", offset %" PRIu64 "\n"),
 			ino,
 			irec->br_startblock,
-			irec->br_startblock + irec->br_blockcount - 1,
+			lastb,
 			irec->br_startoff);
 		return 1;
 	}

Re: [PATCH 1/7] xfs_repair: don't crash on partially sparse inode clusters

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 10:52:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> While running xfs/364 to fuzz the middle bit of recs[2].holemask, I
> observed a crash in xfs_repair stemming from the fact that each sparse
> bit accounts for 4 inodes, but inode cluster buffers can map to more
> than four inodes.
> 
> When the first inode in an inode cluster is marked sparse,
> process_inode_chunk won't try to load the inode cluster buffer.
> Unfortunately, if the holemask indicates that there are inodes present
> anywhere in the rest of the cluster buffer, repair will try to check the
> corresponding cluster buffer, even if we didn't load it.  This leads to
> a null pointer dereference, which crashes repair.
> 
> Avoid the null pointer dereference by marking the inode sparse and
> moving on to the next inode.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

Re: [PATCH 2/7] xfs_repair: fix error in process_sf_dir2_fixi8

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 10:52:10AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The goal of process_sf_dir2_fixi8 is to convert an i8 shortform
> directory into a (shorter) i4 shortform directory.  It achieves this by
> duplicating the old sf directory contents (as oldsfp), zeroing i8count
> in the caller's directory buffer (i.e. newsfp/sfp), and reinitializing
> the new directory with the old directory's entries.
> 
> Unfortunately, it copies the parent pointer from sfp (the buffer we've
> already started changing), not oldsfp.  This leads to directory
> corruption since at that point we zeroed i8count, which means that we
> save only the upper four bytes from the parent pointer entry.
> 
> This was found by fuzzing u3.sfdir3.hdr.i8count = ones in xfs/384.

Looks good,

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

Re: [PATCH 3/7] xfs_repair: junk corrupt xattr root blocks

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 10:52:16AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If reading the root block of an extended attribute structure fails due
> to a corruption error, we should junk the block since we know it's bad.
> There's no point in moving on to the (rather insufficient) checks in the
> attr code.
> 
> Found by fuzzing hdr.freemap[1].base = ones in xfs/400.

Looks good,

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

Re: [PATCH 4/7] xfs_repair: complain about unwritten extents when they're not appropriate

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 10:52:22AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We don't allow unwritten extents in the attr fork, and we don't allow
> them in the data fork except for regular files.  Check that this is the
> case.
> 
> Found by manually fuzzing the extentflag field of an attr fork to one.

Looks good:

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

Re: [PATCH 5/7] xfs_repair: fix handling of data blocks colliding with existing metadata

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 10:52:28AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Prior to commit a406779bc8d8, any blocks in a data fork extent that
> collided with existing blocks would cause the entire data fork extent to
> be rejected.  Unfortunately, the patch to add data block sharing support
> suppressed checking for any collision, including metadata.  What we
> really wanted to do here during a check_dups==1 scan is to is check for
> specific collisions and without updating the block mapping data.
> 
> So, move the check_dups test after the for-switch construction.  This
> re-enables detecting collisions between data fork blocks and a
> previously scanned chunk of metadata, and improves the specificity of
> the error message that results.
> 
> This was found by fuzzing recs[2].free=zeroes in xfs/364, though this
> patch alone does not solve all the problems that scenario presents.
> 
> Fixes: a406779bc8d8 ("xfs_repair: handle multiple owners of data blocks")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

Re: [PATCH 7/7] xfs_repair: use libxfs_verify_rtbno to verify rt extents

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 10:52:41AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use the existing realtime block validation function to check the first
> and last block of an extent in a realtime file.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

Re: [PATCH 6/7] xfs_repair: throw away totally bad clusters

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 10:52:35AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If the filesystem supports sparse inodes, we detect that an entire
> cluster buffer has no detectable inodes at all, and we can easily mark
> that part of the inode chunk sparse, just drop the cluster buffer and
> forget about it.  This makes repair less likely to go to great lengths
> to try to save something that's totally unsalvageable.
> 
> This manifested in recs[2].free=zeroes in xfs/364, wherein the root
> directory claimed to own block X and the inobt also claimed that X was
> inodes; repair tried to create rmaps for both owners, and then the whole
> mess blew up because the rmap code aborts on those kinds of anomalies.

How is the rmap.c chunk related to this?  The dino_chunks.c part looks
fine, and the rmap.c at least not bad, but I don't understand how it
fits here.

Re: [PATCH 6/7] xfs_repair: throw away totally bad clusters

[Darrick J. Wong]

On Tue, Sep 08, 2020 at 04:15:26PM +0100, Christoph Hellwig wrote:
> On Mon, Sep 07, 2020 at 10:52:35AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > If the filesystem supports sparse inodes, we detect that an entire
> > cluster buffer has no detectable inodes at all, and we can easily mark
> > that part of the inode chunk sparse, just drop the cluster buffer and
> > forget about it.  This makes repair less likely to go to great lengths
> > to try to save something that's totally unsalvageable.
> > 
> > This manifested in recs[2].free=zeroes in xfs/364, wherein the root
> > directory claimed to own block X and the inobt also claimed that X was
> > inodes; repair tried to create rmaps for both owners, and then the whole
> > mess blew up because the rmap code aborts on those kinds of anomalies.
> 
> How is the rmap.c chunk related to this?  The dino_chunks.c part looks
> fine, and the rmap.c at least not bad, but I don't understand how it
> fits here.

I think that's a stray paste error; the chunk can be dropped.

--D

[PATCH v2 6/7] xfs_repair: throw away totally bad clusters

[Darrick J. Wong]

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

If the filesystem supports sparse inodes, we detect that an entire
cluster buffer has no detectable inodes at all, and we can easily mark
that part of the inode chunk sparse, just drop the cluster buffer and
forget about it.  This makes repair less likely to go to great lengths
to try to save something that's totally unsalvageable.

This manifested in recs[2].free=zeroes in xfs/364, wherein the root
directory claimed to own block X and the inobt also claimed that X was
inodes; repair tried to create rmaps for both owners, and then the whole
mess blew up because the rmap code aborts on those kinds of anomalies.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: drop repair/rmap.c paste error
---
 repair/dino_chunks.c |   35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 50a2003614df..e4a95ff635c8 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -601,6 +601,7 @@ process_inode_chunk(
 	xfs_dinode_t		*dino;
 	int			icnt;
 	int			status;
+	int			bp_found;
 	int			is_used;
 	int			ino_dirty;
 	int			irec_offset;
@@ -614,6 +615,7 @@ process_inode_chunk(
 	int			bp_index;
 	int			cluster_offset;
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
+	bool			can_punch_sparse = false;
 	int			error;
 
 	ASSERT(first_irec != NULL);
@@ -626,6 +628,10 @@ process_inode_chunk(
 	if (cluster_count == 0)
 		cluster_count = 1;
 
+	if (xfs_sb_version_hassparseinodes(&mp->m_sb) &&
+	    M_IGEO(mp)->inodes_per_cluster >= XFS_INODES_PER_HOLEMASK_BIT)
+		can_punch_sparse = true;
+
 	/*
 	 * get all blocks required to read in this chunk (may wind up
 	 * having to process more chunks in a multi-chunk per block fs)
@@ -700,6 +706,7 @@ process_inode_chunk(
 	cluster_offset = 0;
 	icnt = 0;
 	status = 0;
+	bp_found = 0;
 	bp_index = 0;
 
 	/*
@@ -725,8 +732,10 @@ process_inode_chunk(
 						(agno == 0 &&
 						(mp->m_sb.sb_rootino == agino ||
 						 mp->m_sb.sb_rsumino == agino ||
-						 mp->m_sb.sb_rbmino == agino)))
+						 mp->m_sb.sb_rbmino == agino))) {
 					status++;
+					bp_found++;
+				}
 			}
 
 			irec_offset++;
@@ -749,11 +758,35 @@ process_inode_chunk(
 				irec_offset = 0;
 			}
 			if (cluster_offset == M_IGEO(mp)->inodes_per_cluster) {
+				if (can_punch_sparse &&
+				    bplist[bp_index] != NULL &&
+				    bp_found == 0) {
+					/*
+					 * We didn't find any good inodes in
+					 * this cluster, blow it away before
+					 * moving on to the next one.
+					 */
+					libxfs_buf_relse(bplist[bp_index]);
+					bplist[bp_index] = NULL;
+				}
 				bp_index++;
 				cluster_offset = 0;
+				bp_found = 0;
 			}
 		}
 
+		if (can_punch_sparse &&
+		    bp_index < cluster_count &&
+		    bplist[bp_index] != NULL &&
+		    bp_found == 0) {
+			/*
+			 * We didn't find any good inodes in this cluster, blow
+			 * it away.
+			 */
+			libxfs_buf_relse(bplist[bp_index]);
+			bplist[bp_index] = NULL;
+		}
+
 		/*
 		 * if chunk/block is bad, blow it off.  the inode records
 		 * will be deleted by the caller if appropriate.

Re: [PATCH v2 6/7] xfs_repair: throw away totally bad clusters

[Christoph Hellwig]

Looks good,

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