[PATCH v2 0/9] xfsprogs: fixes for 5.10

[PATCH v2 0/9] xfsprogs: fixes for 5.10

[Darrick J. Wong]

Hi all,

Fix a few bugs ahead of landing all the 5.10 stuff.

v2: Add a few more fixes and cleanups.

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=xfsprogs-5.10-fixes
---
 db/check.c          |   33 +++++++++
 man/man8/mkfs.xfs.8 |   17 ++++-
 mkfs/xfs_mkfs.c     |   14 +++-
 repair/dinode.c     |  180 ++++++++++++++++++++++++++++++++-------------------
 repair/rmap.c       |    4 +
 repair/scan.c       |   36 +++++++---
 scrub/fscounters.c  |    8 +-
 scrub/fscounters.h  |    2 -
 scrub/phase6.c      |    7 +-
 scrub/phase7.c      |    5 -
 tools/libxfs-apply  |   14 +++-
 11 files changed, 213 insertions(+), 107 deletions(-)

[PATCH 1/9] mkfs: allow users to specify rtinherit=0

[Darrick J. Wong]

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

mkfs has quite a few boolean options that can be specified in several
ways: "option=1" (turn it on), "option" (turn it on), or "option=0"
(turn it off).  For whatever reason, rtinherit sticks out as the only
mkfs parameter that doesn't behave that way.  Let's make it behave the
same as all the other boolean variables.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 mkfs/xfs_mkfs.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 8fe149d74b0a..908d520df909 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -349,7 +349,7 @@ static struct opt_params dopts = {
 		},
 		{ .index = D_RTINHERIT,
 		  .conflicts = { { NULL, LAST_CONFLICT } },
-		  .minval = 1,
+		  .minval = 0,
 		  .maxval = 1,
 		  .defaultval = 1,
 		},
@@ -1429,6 +1429,8 @@ data_opts_parser(
 	case D_RTINHERIT:
 		if (getnum(value, opts, subopt))
 			cli->fsx.fsx_xflags |= FS_XFLAG_RTINHERIT;
+		else
+			cli->fsx.fsx_xflags &= ~FS_XFLAG_RTINHERIT;
 		break;
 	case D_PROJINHERIT:
 		cli->fsx.fsx_projid = getnum(value, opts, subopt);

[PATCH 2/9] mkfs: clarify valid "inherit" option values

[Darrick J. Wong]

From: Eric Sandeen <sandeen@sandeen.net>

Clarify which values are valid for the various *inherit= mkfs
options.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
[darrick: fix a few nits]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man8/mkfs.xfs.8 |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)


diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 0a7858748457..692daf2a9050 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -378,31 +378,40 @@ without stripe geometry alignment even if the underlying storage device provides
 this information.
 .TP
 .BI rtinherit= value
-If set, all inodes created by
+If
+.I value
+is set to 1, all inodes created by
 .B mkfs.xfs
 will be created with the realtime flag set.
+The default is 0.
 Directories will pass on this flag to newly created regular files and
 directories.
 .TP
 .BI projinherit= value
 All inodes created by
 .B mkfs.xfs
-will be assigned this project quota id.
+will be assigned the project quota id provided in
+.I value.
 Directories will pass on the project id to newly created regular files and
 directories.
 .TP
 .BI extszinherit= value
 All inodes created by
 .B mkfs.xfs
-will have this extent size hint applied.
+will have this
+.I value
+extent size hint applied.
 The value must be provided in units of filesystem blocks.
 Directories will pass on this hint to newly created regular files and
 directories.
 .TP
 .BI daxinherit= value
-If set, all inodes created by
+If
+.I value
+is set to 1, all inodes created by
 .B mkfs.xfs
 will be created with the DAX flag set.
+The default is 0.
 Directories will pass on this flag to newly created regular files and
 directories.
 By default,

Re: [PATCH 2/9] mkfs: clarify valid "inherit" option values

[Christoph Hellwig]

On Tue, Nov 10, 2020 at 10:03:16AM -0800, Darrick J. Wong wrote:
> From: Eric Sandeen <sandeen@sandeen.net>
> 
> Clarify which values are valid for the various *inherit= mkfs
> options.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> [darrick: fix a few nits]
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 3/9] mkfs: don't pass on extent size inherit flags when extent size is zero

[Darrick J. Wong]

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

If the caller passes in an extent size hint of zero, clear the inherit
flags because a hint value of zero is treated as not a hint.

Otherwise, you get stupid stuff like:
$ mkfs.xfs -d cowextsize=0 /tmp/a.img -f
illegal CoW extent size hint 0, must be less than 9600.

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


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 908d520df909..9989cf57c295 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1438,11 +1438,17 @@ data_opts_parser(
 		break;
 	case D_EXTSZINHERIT:
 		cli->fsx.fsx_extsize = getnum(value, opts, subopt);
-		cli->fsx.fsx_xflags |= FS_XFLAG_EXTSZINHERIT;
+		if (cli->fsx.fsx_extsize)
+			cli->fsx.fsx_xflags |= FS_XFLAG_EXTSZINHERIT;
+		else
+			cli->fsx.fsx_xflags &= ~FS_XFLAG_EXTSZINHERIT;
 		break;
 	case D_COWEXTSIZE:
 		cli->fsx.fsx_cowextsize = getnum(value, opts, subopt);
-		cli->fsx.fsx_xflags |= FS_XFLAG_COWEXTSIZE;
+		if (cli->fsx.fsx_cowextsize)
+			cli->fsx.fsx_xflags |= FS_XFLAG_COWEXTSIZE;
+		else
+			cli->fsx.fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
 		break;
 	case D_DAXINHERIT:
 		if (getnum(value, opts, subopt))

Re: [PATCH 3/9] mkfs: don't pass on extent size inherit flags when extent size is zero

[Christoph Hellwig]

On Tue, Nov 10, 2020 at 10:03:22AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If the caller passes in an extent size hint of zero, clear the inherit
> flags because a hint value of zero is treated as not a hint.
> 
> Otherwise, you get stupid stuff like:
> $ mkfs.xfs -d cowextsize=0 /tmp/a.img -f
> illegal CoW extent size hint 0, must be less than 9600.

Looks good,

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

[PATCH 4/9] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks

[Darrick J. Wong]

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

The only caller that cares about the file counts uses it to compute the
number of files used, so return that and save a parameter.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 scrub/fscounters.c |    8 +++-----
 scrub/fscounters.h |    2 +-
 scrub/phase6.c     |    7 +++----
 scrub/phase7.c     |    5 +----
 4 files changed, 8 insertions(+), 14 deletions(-)


diff --git a/scrub/fscounters.c b/scrub/fscounters.c
index e9901fcdf6df..9a240d49477b 100644
--- a/scrub/fscounters.c
+++ b/scrub/fscounters.c
@@ -116,7 +116,7 @@ scrub_count_all_inodes(
 }
 
 /*
- * Estimate the number of blocks and inodes in the filesystem.  Returns 0
+ * Estimate the number of blocks and used inodes in the filesystem.  Returns 0
  * or a positive error number.
  */
 int
@@ -126,8 +126,7 @@ scrub_scan_estimate_blocks(
 	unsigned long long		*d_bfree,
 	unsigned long long		*r_blocks,
 	unsigned long long		*r_bfree,
-	unsigned long long		*f_files,
-	unsigned long long		*f_free)
+	unsigned long long		*f_files_used)
 {
 	struct xfs_fsop_counts		fc;
 	int				error;
@@ -141,8 +140,7 @@ scrub_scan_estimate_blocks(
 	*d_bfree = fc.freedata;
 	*r_blocks = ctx->mnt.fsgeom.rtblocks;
 	*r_bfree = fc.freertx;
-	*f_files = fc.allocino;
-	*f_free = fc.freeino;
+	*f_files_used = fc.allocino - fc.freeino;
 
 	return 0;
 }
diff --git a/scrub/fscounters.h b/scrub/fscounters.h
index 1fae58a6b287..13bd9967f004 100644
--- a/scrub/fscounters.h
+++ b/scrub/fscounters.h
@@ -9,7 +9,7 @@
 int scrub_scan_estimate_blocks(struct scrub_ctx *ctx,
 		unsigned long long *d_blocks, unsigned long long *d_bfree,
 		unsigned long long *r_blocks, unsigned long long *r_bfree,
-		unsigned long long *f_files, unsigned long long *f_free);
+		unsigned long long *f_files_used);
 int scrub_count_all_inodes(struct scrub_ctx *ctx, uint64_t *count);
 
 #endif /* XFS_SCRUB_FSCOUNTERS_H_ */
diff --git a/scrub/phase6.c b/scrub/phase6.c
index 8d976732d8e1..87828b60fbed 100644
--- a/scrub/phase6.c
+++ b/scrub/phase6.c
@@ -719,12 +719,11 @@ phase6_estimate(
 	unsigned long long	d_bfree;
 	unsigned long long	r_blocks;
 	unsigned long long	r_bfree;
-	unsigned long long	f_files;
-	unsigned long long	f_free;
+	unsigned long long	dontcare;
 	int			ret;
 
-	ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree,
-				&r_blocks, &r_bfree, &f_files, &f_free);
+	ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
+			&r_bfree, &dontcare);
 	if (ret) {
 		str_liberror(ctx, ret, _("estimating verify work"));
 		return ret;
diff --git a/scrub/phase7.c b/scrub/phase7.c
index 96876f7c0596..bc652ab6f44a 100644
--- a/scrub/phase7.c
+++ b/scrub/phase7.c
@@ -111,8 +111,6 @@ phase7_func(
 	unsigned long long	d_bfree;
 	unsigned long long	r_blocks;
 	unsigned long long	r_bfree;
-	unsigned long long	f_files;
-	unsigned long long	f_free;
 	bool			complain;
 	int			ip;
 	int			error;
@@ -160,7 +158,7 @@ phase7_func(
 	}
 
 	error = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
-			&r_bfree, &f_files, &f_free);
+			&r_bfree, &used_files);
 	if (error) {
 		str_liberror(ctx, error, _("estimating verify work"));
 		return error;
@@ -177,7 +175,6 @@ phase7_func(
 	/* Report on what we found. */
 	used_data = cvt_off_fsb_to_b(&ctx->mnt, d_blocks - d_bfree);
 	used_rt = cvt_off_fsb_to_b(&ctx->mnt, r_blocks - r_bfree);
-	used_files = f_files - f_free;
 	stat_data = totalcount.dbytes;
 	stat_rt = totalcount.rbytes;
 

[PATCH 5/9] xfs_db: report ranges of invalid rt blocks

[Darrick J. Wong]

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

Copy-pasta the block range reporting code from check_range into
check_rrange so that we don't flood stdout with a ton of low value
messages when a bit flips somewhere in rt metadata.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 db/check.c |   33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)


diff --git a/db/check.c b/db/check.c
index 553249dc9a41..5aede6cca15c 100644
--- a/db/check.c
+++ b/db/check.c
@@ -1569,19 +1569,46 @@ check_rootdir(void)
 	}
 }
 
+static inline void
+report_rrange(
+	xfs_rfsblock_t	low,
+	xfs_rfsblock_t	high)
+{
+	if (low == high)
+		dbprintf(_("rtblock %llu out of range\n"), low);
+	else
+		dbprintf(_("rtblocks %llu..%llu out of range\n"), low, high);
+}
+
 static int
 check_rrange(
 	xfs_rfsblock_t	bno,
 	xfs_extlen_t	len)
 {
 	xfs_extlen_t	i;
+	xfs_rfsblock_t	low = 0;
+	xfs_rfsblock_t	high = 0;
+	bool		valid_range = false;
+	int		cur, prev = 0;
 
 	if (bno + len - 1 >= mp->m_sb.sb_rblocks) {
 		for (i = 0; i < len; i++) {
-			if (!sflag || CHECK_BLIST(bno + i))
-				dbprintf(_("rtblock %llu out of range\n"),
-					bno + i);
+			cur = !sflag || CHECK_BLIST(bno + i) ? 1 : 0;
+			if (cur == 1 && prev == 0) {
+				low = high = bno + i;
+				valid_range = true;
+			} else if (cur == 0 && prev == 0) {
+				/* Do nothing */
+			} else if (cur == 0 && prev == 1) {
+				report_rrange(low, high);
+				valid_range = false;
+			} else if (cur == 1 && prev == 1) {
+				high = bno + i;
+			}
+			prev = cur;
 		}
+		if (valid_range)
+			report_rrange(low, high);
 		error++;
 		return 0;
 	}

[PATCH 6/9] xfs_repair: skip the rmap and refcount btree checks when the levels are garbage

[Darrick J. Wong]

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

In validate_ag[fi], we should check that the levels of the rmap and
refcount btrees are valid.  If they aren't, we need to tell phase4 to
skip the comparison between the existing and incore rmap and refcount
data.  The comparison routines use libxfs btree cursors, which assume
that the caller validated bc_nlevels and will corrupt memory if we load
a btree cursor with a garbage level count.

This was found by examing a core dump from a failed xfs/086 invocation.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/scan.c |   36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 42b299f75067..2a38ae5197c6 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -2279,23 +2279,31 @@ validate_agf(
 
 	if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
 		struct rmap_priv	priv;
+		unsigned int		levels;
 
 		memset(&priv.high_key, 0xFF, sizeof(priv.high_key));
 		priv.high_key.rm_blockcount = 0;
 		priv.agcnts = agcnts;
 		priv.last_rec.rm_owner = XFS_RMAP_OWN_UNKNOWN;
 		priv.nr_blocks = 0;
+
+		levels = be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]);
+		if (levels >= XFS_BTREE_MAXLEVELS) {
+			do_warn(_("bad levels %u for rmapbt root, agno %d\n"),
+				levels, agno);
+			rmap_avoid_check();
+		}
+
 		bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_RMAP]);
 		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,
-				    &priv, &xfs_rmapbt_buf_ops);
+			scan_sbtree(bno, levels, agno, 0, scan_rmapbt, 1,
+					XFS_RMAP_CRC_MAGIC, &priv,
+					&xfs_rmapbt_buf_ops);
 			if (be32_to_cpu(agf->agf_rmap_blocks) != priv.nr_blocks)
 				do_warn(_("bad rmapbt block count %u, saw %u\n"),
 					priv.nr_blocks,
 					be32_to_cpu(agf->agf_rmap_blocks));
-		} else  {
+		} else {
 			do_warn(_("bad agbno %u for rmapbt root, agno %d\n"),
 				bno, agno);
 			rmap_avoid_check();
@@ -2303,20 +2311,28 @@ validate_agf(
 	}
 
 	if (xfs_sb_version_hasreflink(&mp->m_sb)) {
+		unsigned int	levels;
+
+		levels = be32_to_cpu(agf->agf_refcount_level);
+		if (levels >= XFS_BTREE_MAXLEVELS) {
+			do_warn(_("bad levels %u for refcountbt root, agno %d\n"),
+				levels, agno);
+			refcount_avoid_check();
+		}
+
 		bno = be32_to_cpu(agf->agf_refcount_root);
 		if (libxfs_verify_agbno(mp, agno, bno)) {
 			struct refc_priv	priv;
 
 			memset(&priv, 0, sizeof(priv));
-			scan_sbtree(bno,
-				    be32_to_cpu(agf->agf_refcount_level),
-				    agno, 0, scan_refcbt, 1, XFS_REFC_CRC_MAGIC,
-				    &priv, &xfs_refcountbt_buf_ops);
+			scan_sbtree(bno, levels, agno, 0, scan_refcbt, 1,
+					XFS_REFC_CRC_MAGIC, &priv,
+					&xfs_refcountbt_buf_ops);
 			if (be32_to_cpu(agf->agf_refcount_blocks) != priv.nr_blocks)
 				do_warn(_("bad refcountbt block count %u, saw %u\n"),
 					priv.nr_blocks,
 					be32_to_cpu(agf->agf_refcount_blocks));
-		} else  {
+		} else {
 			do_warn(_("bad agbno %u for refcntbt root, agno %d\n"),
 				bno, agno);
 			refcount_avoid_check();

[PATCH 7/9] xfs_repair: correctly detect partially written extents

[Darrick J. Wong]

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

Recently, I was able to create a realtime file with a 16b extent size
and the following data fork mapping:

data offset 0 startblock 144 (0/144) count 3 flag 0
data offset 3 startblock 147 (0/147) count 3 flag 1
data offset 6 startblock 150 (0/150) count 10 flag 0

Notice how we have a written extent, then an unwritten extent, and then
another written extent.  The current code in process_rt_rec trips over
that third extent, because repair only knows not to complain about inuse
extents if the mapping was unwritten.

This loop logic is confusing, because it tries to do too many things.
Move the phase3 and phase4 code to separate helper functions, then
isolate the code that handles a mapping that starts in the middle of an
rt extent so that it's clearer what's going on.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/dinode.c |  180 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 112 insertions(+), 68 deletions(-)


diff --git a/repair/dinode.c b/repair/dinode.c
index c89f21e08373..028a23cd5c8c 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -176,76 +176,69 @@ verify_dfsbno_range(
 
 	return XR_DFSBNORANGE_VALID;
 }
+
 static int
-process_rt_rec(
+process_rt_rec_dups(
 	struct xfs_mount	*mp,
-	struct xfs_bmbt_irec	*irec,
 	xfs_ino_t		ino,
-	xfs_rfsblock_t		*tot,
-	int			check_dups)
+	struct xfs_bmbt_irec	*irec)
 {
-	xfs_fsblock_t		b, lastb;
+	xfs_fsblock_t		b;
 	xfs_rtblock_t		ext;
-	int			state;
-	int			pwe;		/* partially-written extent */
 
-	/*
-	 * check numeric validity of the extent
-	 */
-	if (!libxfs_verify_rtbno(mp, irec->br_startblock)) {
-		do_warn(
-_("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" PRIu64 "\n"),
-			ino,
-			irec->br_startblock,
-			irec->br_startoff);
-		return 1;
-	}
-
-	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,
-			lastb,
-			irec->br_startoff);
-		return 1;
-	}
-	if (lastb < irec->br_startblock) {
-		do_warn(
-_("inode %" PRIu64 " - bad rt extent overflows - start %" PRIu64 ", "
-  "end %" PRIu64 ", offset %" PRIu64 "\n"),
-			ino,
-			irec->br_startblock,
-			lastb,
-			irec->br_startoff);
-		return 1;
-	}
-
-	/*
-	 * set the appropriate number of extents
-	 * this iterates block by block, this can be optimised using extents
-	 */
-	for (b = irec->br_startblock; b < irec->br_startblock +
-			irec->br_blockcount; b += mp->m_sb.sb_rextsize)  {
+	for (b = rounddown(irec->br_startblock, mp->m_sb.sb_rextsize);
+	     b < irec->br_startblock + irec->br_blockcount;
+	     b += mp->m_sb.sb_rextsize) {
 		ext = (xfs_rtblock_t) b / mp->m_sb.sb_rextsize;
-		pwe = irec->br_state == XFS_EXT_UNWRITTEN &&
-				(b % mp->m_sb.sb_rextsize != 0);
-
-		if (check_dups == 1)  {
-			if (search_rt_dup_extent(mp, ext) && !pwe)  {
-				do_warn(
+		if (search_rt_dup_extent(mp, ext))  {
+			do_warn(
 _("data fork in rt ino %" PRIu64 " claims dup rt extent,"
-  "off - %" PRIu64 ", start - %" PRIu64 ", count %" PRIu64 "\n"),
-					ino,
-					irec->br_startoff,
-					irec->br_startblock,
-					irec->br_blockcount);
-				return 1;
-			}
-			continue;
+"off - %" PRIu64 ", start - %" PRIu64 ", count %" PRIu64 "\n"),
+				ino,
+				irec->br_startoff,
+				irec->br_startblock,
+				irec->br_blockcount);
+			return 1;
 		}
+	}
 
+	return 0;
+}
+
+static int
+process_rt_rec_state(
+	struct xfs_mount	*mp,
+	xfs_ino_t		ino,
+	struct xfs_bmbt_irec	*irec)
+{
+	xfs_fsblock_t		b = irec->br_startblock;
+	xfs_rtblock_t		ext;
+	int			state;
+
+	do {
+		ext = (xfs_rtblock_t)b / mp->m_sb.sb_rextsize;
 		state = get_rtbmap(ext);
+
+		if ((b % mp->m_sb.sb_rextsize) != 0) {
+			/*
+			 * We are midway through a partially written extent.
+			 * If we don't find the state that gets set in the
+			 * other clause of this loop body, then we have a
+			 * partially *mapped* rt extent and should complain.
+			 */
+			if (state != XR_E_INUSE)
+				do_error(
+_("data fork in rt inode %" PRIu64 " found invalid rt extent %"PRIu64" state %d at rt block %"PRIu64"\n"),
+					ino, ext, state, b);
+			b = roundup(b, mp->m_sb.sb_rextsize);
+			continue;
+		}
+
+		/*
+		 * This is the start of an rt extent.  Set the extent state if
+		 * nobody else has claimed the extent, or complain if there are
+		 * conflicting states.
+		 */
 		switch (state)  {
 		case XR_E_FREE:
 		case XR_E_UNKNOWN:
@@ -253,32 +246,83 @@ _("data fork in rt ino %" PRIu64 " claims dup rt extent,"
 			break;
 		case XR_E_BAD_STATE:
 			do_error(
-_("bad state in rt block map %" PRIu64 "\n"),
+_("bad state in rt extent map %" PRIu64 "\n"),
 				ext);
 		case XR_E_FS_MAP:
 		case XR_E_INO:
 		case XR_E_INUSE_FS:
 			do_error(
-_("data fork in rt inode %" PRIu64 " found metadata block %" PRIu64 " in rt bmap\n"),
+_("data fork in rt inode %" PRIu64 " found rt metadata extent %" PRIu64 " in rt bmap\n"),
 				ino, ext);
 		case XR_E_INUSE:
-			if (pwe)
-				break;
-			/* fall through */
 		case XR_E_MULT:
 			set_rtbmap(ext, XR_E_MULT);
 			do_warn(
-_("data fork in rt inode %" PRIu64 " claims used rt block %" PRIu64 "\n"),
-				ino, ext);
+_("data fork in rt inode %" PRIu64 " claims used rt extent %" PRIu64 "\n"),
+				ino, b);
 			return 1;
 		case XR_E_FREE1:
 		default:
 			do_error(
-_("illegal state %d in rt block map %" PRIu64 "\n"),
-				state, b);
+_("illegal state %d in rt extent %" PRIu64 "\n"),
+				state, ext);
 		}
+		b += mp->m_sb.sb_rextsize;
+	} while (b < irec->br_startblock + irec->br_blockcount);
+
+	return 0;
+}
+
+static int
+process_rt_rec(
+	struct xfs_mount	*mp,
+	struct xfs_bmbt_irec	*irec,
+	xfs_ino_t		ino,
+	xfs_rfsblock_t		*tot,
+	int			check_dups)
+{
+	xfs_fsblock_t		lastb;
+	int			bad;
+
+	/*
+	 * check numeric validity of the extent
+	 */
+	if (!libxfs_verify_rtbno(mp, irec->br_startblock)) {
+		do_warn(
+_("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" PRIu64 "\n"),
+			ino,
+			irec->br_startblock,
+			irec->br_startoff);
+		return 1;
 	}
 
+	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,
+			lastb,
+			irec->br_startoff);
+		return 1;
+	}
+	if (lastb < irec->br_startblock) {
+		do_warn(
+_("inode %" PRIu64 " - bad rt extent overflows - start %" PRIu64 ", "
+  "end %" PRIu64 ", offset %" PRIu64 "\n"),
+			ino,
+			irec->br_startblock,
+			lastb,
+			irec->br_startoff);
+		return 1;
+	}
+
+	if (check_dups)
+		bad = process_rt_rec_dups(mp, ino, irec);
+	else
+		bad = process_rt_rec_state(mp, ino, irec);
+	if (bad)
+		return bad;
+
 	/*
 	 * bump up the block counter
 	 */

[PATCH 8/9] xfs_repair: directly compare refcount records

[Darrick J. Wong]

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

Check that our observed refcount records have exact matches for what's
in the ondisk refcount btree, since they're supposed to match exactly.

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


diff --git a/repair/rmap.c b/repair/rmap.c
index a4cc6a4937c9..54451a7e262d 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -1402,8 +1402,8 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
 
 		/* Compare each refcount observation against the btree's */
 		if (tmp.rc_startblock != rl_rec->rc_startblock ||
-		    tmp.rc_blockcount < rl_rec->rc_blockcount ||
-		    tmp.rc_refcount < rl_rec->rc_refcount)
+		    tmp.rc_blockcount != rl_rec->rc_blockcount ||
+		    tmp.rc_refcount != rl_rec->rc_refcount)
 			do_warn(
 _("Incorrect reference count: saw (%u/%u) len %u nlinks %u; should be (%u/%u) len %u nlinks %u\n"),
 				agno, tmp.rc_startblock, tmp.rc_blockcount,

Re: [PATCH 8/9] xfs_repair: directly compare refcount records

[Christoph Hellwig]

On Tue, Nov 10, 2020 at 10:03:54AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check that our observed refcount records have exact matches for what's
> in the ondisk refcount btree, since they're supposed to match exactly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 9/9] libxfs-apply: don't add duplicate headers

[Darrick J. Wong]

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

When we're backporting patches from libxfs, don't add a S-o-b header if
there's already one in the patch being ported.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tools/libxfs-apply |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)


diff --git a/tools/libxfs-apply b/tools/libxfs-apply
index 3258272d6189..35cdb9c3449b 100755
--- a/tools/libxfs-apply
+++ b/tools/libxfs-apply
@@ -193,6 +193,14 @@ filter_xfsprogs_patch()
 	rm -f $_libxfs_files
 }
 
+add_header()
+{
+	local hdr="$1"
+	local hdrfile="$2"
+
+	grep -q "^${hdr}$" "$hdrfile" || echo "$hdr" >> "$hdrfile"
+}
+
 fixup_header_format()
 {
 	local _source=$1
@@ -280,13 +288,13 @@ fixup_header_format()
 	sed -i '${/^[[:space:]]*$/d;}' $_hdr.new
 
 	# Add Signed-off-by: header if specified
-	if [ ! -z ${SIGNED_OFF_BY+x} ]; then 
-		echo "Signed-off-by: $SIGNED_OFF_BY" >> $_hdr.new
+	if [ ! -z ${SIGNED_OFF_BY+x} ]; then
+		add_header "Signed-off-by: $SIGNED_OFF_BY" $_hdr.new
 	else	# get it from git config if present
 		SOB_NAME=`git config --get user.name`
 		SOB_EMAIL=`git config --get user.email`
 		if [ ! -z ${SOB_NAME+x} ]; then
-			echo "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" >> $_hdr.new
+			add_header "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" $_hdr.new
 		fi
 	fi
 

Re: [PATCH 9/9] libxfs-apply: don't add duplicate headers

[Christoph Hellwig]

On Tue, Nov 10, 2020 at 10:04:00AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're backporting patches from libxfs, don't add a S-o-b header if
> there's already one in the patch being ported.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

Re: [PATCH 9/9] libxfs-apply: don't add duplicate headers

[Eric Sandeen]

On 11/10/20 12:04 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're backporting patches from libxfs, don't add a S-o-b header if
> there's already one in the patch being ported.

I guess the goal here is to not add 2 identical sign offs in a row.

But when I do the libxfs-application, I do feel like it should add
my SOB as sort of a chain of custody record before I commit it to a
new tree/project, no?

So could this be modified to simply not add 2 identical SOBs in a row?

Maybe we can just run "uniq" on the $_hdr.new file?

Thanks,
-Eric


> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  tools/libxfs-apply |   14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/tools/libxfs-apply b/tools/libxfs-apply
> index 3258272d6189..35cdb9c3449b 100755
> --- a/tools/libxfs-apply
> +++ b/tools/libxfs-apply
> @@ -193,6 +193,14 @@ filter_xfsprogs_patch()
>  	rm -f $_libxfs_files
>  }
>  
> +add_header()
> +{
> +	local hdr="$1"
> +	local hdrfile="$2"
> +
> +	grep -q "^${hdr}$" "$hdrfile" || echo "$hdr" >> "$hdrfile"
> +}
> +
>  fixup_header_format()
>  {
>  	local _source=$1
> @@ -280,13 +288,13 @@ fixup_header_format()
>  	sed -i '${/^[[:space:]]*$/d;}' $_hdr.new
>  
>  	# Add Signed-off-by: header if specified
> -	if [ ! -z ${SIGNED_OFF_BY+x} ]; then 
> -		echo "Signed-off-by: $SIGNED_OFF_BY" >> $_hdr.new
> +	if [ ! -z ${SIGNED_OFF_BY+x} ]; then
> +		add_header "Signed-off-by: $SIGNED_OFF_BY" $_hdr.new
>  	else	# get it from git config if present
>  		SOB_NAME=`git config --get user.name`
>  		SOB_EMAIL=`git config --get user.email`
>  		if [ ! -z ${SOB_NAME+x} ]; then
> -			echo "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" >> $_hdr.new
> +			add_header "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" $_hdr.new
>  		fi
>  	fi
>  
> 

Re: [PATCH 9/9] libxfs-apply: don't add duplicate headers

[Darrick J. Wong]

On Mon, Nov 23, 2020 at 02:30:58PM -0600, Eric Sandeen wrote:
> On 11/10/20 12:04 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When we're backporting patches from libxfs, don't add a S-o-b header if
> > there's already one in the patch being ported.
> 
> I guess the goal here is to not add 2 identical sign offs in a row.
> 
> But when I do the libxfs-application, I do feel like it should add
> my SOB as sort of a chain of custody record before I commit it to a
> new tree/project, no?
> 
> So could this be modified to simply not add 2 identical SOBs in a row?
> 
> Maybe we can just run "uniq" on the $_hdr.new file?

Not sure how uniq gets us to "not add 2 identical signoffs in a row" but
changing the last line of add_header to:

tail -n 1 "$hdrfile" | grep -q "^${hdr}$" || echo "$hdr" >> "$hdrfile"

would do it.

--D

> Thanks,
> -Eric
> 
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  tools/libxfs-apply |   14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/tools/libxfs-apply b/tools/libxfs-apply
> > index 3258272d6189..35cdb9c3449b 100755
> > --- a/tools/libxfs-apply
> > +++ b/tools/libxfs-apply
> > @@ -193,6 +193,14 @@ filter_xfsprogs_patch()
> >  	rm -f $_libxfs_files
> >  }
> >  
> > +add_header()
> > +{
> > +	local hdr="$1"
> > +	local hdrfile="$2"
> > +
> > +	grep -q "^${hdr}$" "$hdrfile" || echo "$hdr" >> "$hdrfile"
> > +}
> > +
> >  fixup_header_format()
> >  {
> >  	local _source=$1
> > @@ -280,13 +288,13 @@ fixup_header_format()
> >  	sed -i '${/^[[:space:]]*$/d;}' $_hdr.new
> >  
> >  	# Add Signed-off-by: header if specified
> > -	if [ ! -z ${SIGNED_OFF_BY+x} ]; then 
> > -		echo "Signed-off-by: $SIGNED_OFF_BY" >> $_hdr.new
> > +	if [ ! -z ${SIGNED_OFF_BY+x} ]; then
> > +		add_header "Signed-off-by: $SIGNED_OFF_BY" $_hdr.new
> >  	else	# get it from git config if present
> >  		SOB_NAME=`git config --get user.name`
> >  		SOB_EMAIL=`git config --get user.email`
> >  		if [ ! -z ${SOB_NAME+x} ]; then
> > -			echo "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" >> $_hdr.new
> > +			add_header "Signed-off-by: $SOB_NAME <$SOB_EMAIL>" $_hdr.new
> >  		fi
> >  	fi
> >  
> >