[PATCHSET] xfsprogs: more random bug fixes

[PATCHSET] xfsprogs: more random bug fixes

[Darrick J. Wong]

Hi all,

This is a grab bag of random fixes targetting xfsprogs 6.13.  The first three
patches are bug fixes.  The fourth patch tells gcc to initialize automatic
variables to zero, which should avoid stack content disclosure and reduce
unpredictable behavior due to uninitialized variables.  The fifth patch adds a
"-r concurrency=" option to mkfs.xfs so that we can try to format enough
rtgroups to minimize contention on rtgroup metadata.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

This has been running on the djcloud for months with no problems.  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=more-random-fixes
---
Commits in this patchset:
 * xfs_db: improve error message when unknown btree type given to btheight
 * mkfs: fix parsing of value-less -d/-l concurrency cli option
 * m4: fix statx override selection if /usr/include doesn't define it
 * build: initialize stack variables to zero by default
 * mkfs: allow sizing realtime allocation groups for concurrency
---
 configure.ac            |    1 
 db/btheight.c           |    6 ++
 include/builddefs.in    |    2 -
 m4/package_libcdev.m4   |    2 -
 m4/package_sanitizer.m4 |   14 +++++
 man/man8/mkfs.xfs.8.in  |   28 +++++++++
 mkfs/xfs_mkfs.c         |  144 +++++++++++++++++++++++++++++++++++++++++++++--
 7 files changed, 190 insertions(+), 7 deletions(-)

[PATCH 1/5] xfs_db: improve error message when unknown btree type given to btheight

[Darrick J. Wong]

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

I found accidentally that if you do this (note 'rmap', not 'rmapbt'):

xfs_db /dev/sda -c 'btheight -n 100 rmap'

The program spits back "Numerical result out of range".  That's the
result of it failing to match "rmap" against a known btree type, and
falling back to parsing the string as if it were a btree geometry
description.

Improve this a little by checking that there's at least one semicolon in
the string so that the error message improves to:

"rmap: expected a btree geometry specification"

Fixes: cb1e69c564c1e0 ("xfs_db: add a function to compute btree geometry")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 db/btheight.c |    6 ++++++
 1 file changed, 6 insertions(+)


diff --git a/db/btheight.c b/db/btheight.c
index 6643489c82c4c9..98165b522e4f6f 100644
--- a/db/btheight.c
+++ b/db/btheight.c
@@ -145,6 +145,12 @@ construct_records_per_block(
 		}
 	}
 
+	p = strchr(tag, ':');
+	if (!p) {
+		fprintf(stderr, _("%s: expected a btree geometry specification.\n"), tag);
+		return -1;
+	}
+
 	toktag = strdup(tag);
 	ret = -1;
 

Re: [PATCH 1/5] xfs_db: improve error message when unknown btree type given to btheight

[Christoph Hellwig]

Looks good:

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

[PATCH 2/5] mkfs: fix parsing of value-less -d/-l concurrency cli option

[Darrick J. Wong]

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

It's supposed to be possible to specify the -d concurrency option with
no value in order to get mkfs calculate the agcount from the number of
CPUs.  Unfortunately I forgot to handle that case (optarg is null) so
mkfs crashes instead.  Fix that.

Fixes: 9338bc8b1bf073 ("mkfs: allow sizing allocation groups for concurrency")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 956cc295489342..deaac2044b94dd 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1722,7 +1722,7 @@ set_data_concurrency(
 	 * "nr_cpus" or "1" means set the concurrency level to the CPU count.
 	 * If this cannot be determined, fall back to the default AG geometry.
 	 */
-	if (!strcmp(value, "nr_cpus"))
+	if (!value || !strcmp(value, "nr_cpus"))
 		optnum = 1;
 	else
 		optnum = getnum(value, opts, subopt);
@@ -1867,7 +1867,7 @@ set_log_concurrency(
 	 * "nr_cpus" or 1 means set the concurrency level to the CPU count.  If
 	 * this cannot be determined, fall back to the default computation.
 	 */
-	if (!strcmp(value, "nr_cpus"))
+	if (!value || !strcmp(value, "nr_cpus"))
 		optnum = 1;
 	else
 		optnum = getnum(value, opts, subopt);

Re: [PATCH 2/5] mkfs: fix parsing of value-less -d/-l concurrency cli option

[Christoph Hellwig]

Looks good:

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

[PATCH 3/5] m4: fix statx override selection if /usr/include doesn't define it

[Darrick J. Wong]

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

If the system headers (aka the ones in /usr/include) do not define
struct statx at all, we need to use our internal override.  The m4 code
doesn't handle this admittedly corner case, but let's fix it for anyone
trying to build new xfsprogs on a decade-old distribution.

Fixes: 409477af604f46 ("xfs_io: add support for atomic write statx fields")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 m4/package_libcdev.m4 |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
index 6db1177350b643..4ef7e8f67a3ba6 100644
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -112,7 +112,7 @@ AC_DEFUN([AC_NEED_INTERNAL_STATX],
           need_internal_statx=yes,
           [#include <linux/stat.h>]
         )
-      ],,
+      ],need_internal_statx=yes,
       [#include <linux/stat.h>]
     )
     AC_SUBST(need_internal_statx)

Re: [PATCH 3/5] m4: fix statx override selection if /usr/include doesn't define it

[Christoph Hellwig]

Looks good:

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

[PATCH 4/5] build: initialize stack variables to zero by default

[Darrick J. Wong]

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

Newer versions of gcc and clang can include the ability to zero stack
variables by default.  Let's enable it so that we (a) reduce the risk of
writing stack contents to disk somewhere and (b) try to reduce
unpredictable program behavior based on random stack contents.  The
kernel added this 6 years ago, so I think it's mature enough for
xfsprogs.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 configure.ac            |    1 +
 include/builddefs.in    |    2 +-
 m4/package_sanitizer.m4 |   14 ++++++++++++++
 3 files changed, 16 insertions(+), 1 deletion(-)


diff --git a/configure.ac b/configure.ac
index 224d1d3930bf2f..90ef7925a925d0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -177,6 +177,7 @@ AC_CONFIG_SYSTEMD_SYSTEM_UNIT_DIR
 AC_CONFIG_CROND_DIR
 AC_CONFIG_UDEV_RULE_DIR
 AC_HAVE_BLKID_TOPO
+AC_HAVE_TRIVIAL_AUTO_VAR_INIT
 
 if test "$enable_ubsan" = "yes" || test "$enable_ubsan" = "probe"; then
         AC_PACKAGE_CHECK_UBSAN
diff --git a/include/builddefs.in b/include/builddefs.in
index ac43b6412c8cbb..82840ec7fd3adb 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -146,7 +146,7 @@ ifeq ($(HAVE_LIBURCU_ATOMIC64),yes)
 PCFLAGS += -DHAVE_LIBURCU_ATOMIC64
 endif
 
-SANITIZER_CFLAGS += @addrsan_cflags@ @threadsan_cflags@ @ubsan_cflags@
+SANITIZER_CFLAGS += @addrsan_cflags@ @threadsan_cflags@ @ubsan_cflags@ @autovar_init_cflags@
 SANITIZER_LDFLAGS += @addrsan_ldflags@ @threadsan_ldflags@ @ubsan_ldflags@
 
 # Use special ar/ranlib wrappers if we have lto
diff --git a/m4/package_sanitizer.m4 b/m4/package_sanitizer.m4
index 41b729906a27ba..6488f7ebce2f50 100644
--- a/m4/package_sanitizer.m4
+++ b/m4/package_sanitizer.m4
@@ -57,3 +57,17 @@ AC_DEFUN([AC_PACKAGE_CHECK_THREADSAN],
     AC_SUBST(threadsan_cflags)
     AC_SUBST(threadsan_ldflags)
   ])
+
+# Check if we have -ftrivial-auto-var-init=zero
+AC_DEFUN([AC_HAVE_TRIVIAL_AUTO_VAR_INIT],
+  [ AC_MSG_CHECKING([if C compiler supports zeroing automatic vars])
+    OLD_CFLAGS="$CFLAGS"
+    TEST_CFLAGS="-ftrivial-auto-var-init=zero"
+    CFLAGS="$CFLAGS $TEST_CFLAGS"
+    AC_LINK_IFELSE([AC_LANG_PROGRAM([])],
+        [AC_MSG_RESULT([yes])]
+        [autovar_init_cflags=$TEST_CFLAGS],
+        [AC_MSG_RESULT([no])])
+    CFLAGS="${OLD_CFLAGS}"
+    AC_SUBST(autovar_init_cflags)
+  ])

Re: [PATCH 4/5] build: initialize stack variables to zero by default

[Christoph Hellwig]

On Tue, Jan 14, 2025 at 01:41:25PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Newer versions of gcc and clang can include the ability to zero stack
> variables by default.  Let's enable it so that we (a) reduce the risk of
> writing stack contents to disk somewhere and (b) try to reduce
> unpredictable program behavior based on random stack contents.  The
> kernel added this 6 years ago, so I think it's mature enough for
> xfsprogs.

Hmm, this tends to paper of bugs quite badly.  But I guess we'd better
paper over bugs in the same way as the kernel code.

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

Re: [PATCH 4/5] build: initialize stack variables to zero by default

[Darrick J. Wong]

On Wed, Jan 15, 2025 at 06:21:55AM +0100, Christoph Hellwig wrote:
> On Tue, Jan 14, 2025 at 01:41:25PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Newer versions of gcc and clang can include the ability to zero stack
> > variables by default.  Let's enable it so that we (a) reduce the risk of
> > writing stack contents to disk somewhere and (b) try to reduce
> > unpredictable program behavior based on random stack contents.  The
> > kernel added this 6 years ago, so I think it's mature enough for
> > xfsprogs.
> 
> Hmm, this tends to paper of bugs quite badly.  But I guess we'd better
> paper over bugs in the same way as the kernel code.

Yeah, I've been thinking about this one for a couple of weeks -- yeah,
it does paper over uninit variable bugs, but since the kernel does it
we had better do that too if we want to keep porting random kernel code
to support libxfs. :/

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

Thanks for reviewing!

--D

[PATCH 5/5] mkfs: allow sizing realtime allocation groups for concurrency

[Darrick J. Wong]

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

Add a -r concurrency= option to mkfs so that sysadmins can configure the
filesystem so that there are enough rtgroups that the specified number
of threads can (in theory) can find an uncontended rtgroup from which to
allocate space.  This has the exact same purpose as the -d concurrency
switch that was added for the data device.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 man/man8/mkfs.xfs.8.in |   28 ++++++++++
 mkfs/xfs_mkfs.c        |  140 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 165 insertions(+), 3 deletions(-)


diff --git a/man/man8/mkfs.xfs.8.in b/man/man8/mkfs.xfs.8.in
index 32361cf973fcf8..37e3a88e7ac777 100644
--- a/man/man8/mkfs.xfs.8.in
+++ b/man/man8/mkfs.xfs.8.in
@@ -1220,6 +1220,34 @@ .SH OPTIONS
 and
 .B rgsize
 suboptions are mutually exclusive.
+.TP
+.BI concurrency= value
+Create enough realtime allocation groups to handle the desired level of
+concurrency.
+The goal of this calculation scheme is to set the number of rtgroups to an
+integer multiple of the number of writer threads desired, to minimize
+contention of rtgroup locks.
+This scheme will neither create fewer rtgroups than would be created by the
+default configuration, nor will it create rtgroups smaller than 4GB.
+This option is not compatible with the
+.B rgcount
+or
+.B rgsize
+options.
+The magic value
+.I nr_cpus
+or
+.I 1
+or no value at all will set this parameter to the number of active processors
+in the system.
+If the kernel advertises that the realtime device is a non-mechanical storage
+device,
+.B mkfs.xfs
+will use this new geometry calculation scheme.
+The magic value of
+.I 0
+forces use of the older rtgroups geometry calculations that is used for
+mechanical storage.
 .RE
 .PP
 .PD 0
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index deaac2044b94dd..073e79ac58303c 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -134,6 +134,7 @@ enum {
 	R_NOALIGN,
 	R_RGCOUNT,
 	R_RGSIZE,
+	R_CONCURRENCY,
 	R_MAX_OPTS,
 };
 
@@ -737,6 +738,7 @@ static struct opt_params ropts = {
 		[R_NOALIGN] = "noalign",
 		[R_RGCOUNT] = "rgcount",
 		[R_RGSIZE] = "rgsize",
+		[R_CONCURRENCY] = "concurrency",
 		[R_MAX_OPTS] = NULL,
 	},
 	.subopt_params = {
@@ -778,6 +780,7 @@ static struct opt_params ropts = {
 		},
 		{ .index = R_RGCOUNT,
 		  .conflicts = { { &ropts, R_RGSIZE },
+				 { &ropts, R_CONCURRENCY },
 				 { NULL, LAST_CONFLICT } },
 		  .minval = 1,
 		  .maxval = XFS_MAX_RGNUMBER,
@@ -785,12 +788,22 @@ static struct opt_params ropts = {
 		},
 		{ .index = R_RGSIZE,
 		  .conflicts = { { &ropts, R_RGCOUNT },
+				 { &ropts, R_CONCURRENCY },
 				 { NULL, LAST_CONFLICT } },
 		  .convert = true,
 		  .minval = 0,
 		  .maxval = (unsigned long long)XFS_MAX_RGBLOCKS << XFS_MAX_BLOCKSIZE_LOG,
 		  .defaultval = SUBOPT_NEEDS_VAL,
 		},
+		{ .index = R_CONCURRENCY,
+		  .conflicts = { { &ropts, R_RGCOUNT },
+				 { &ropts, R_RGSIZE },
+				 { NULL, LAST_CONFLICT } },
+		  .convert = true,
+		  .minval = 0,
+		  .maxval = INT_MAX,
+		  .defaultval = 1,
+		},
 	},
 };
 
@@ -1034,6 +1047,7 @@ struct cli_params {
 	int	proto_slashes_are_spaces;
 	int	data_concurrency;
 	int	log_concurrency;
+	int	rtvol_concurrency;
 
 	/* parameters where 0 is not a valid value */
 	int64_t	agcount;
@@ -1157,7 +1171,8 @@ usage( void )
 /* no-op info only */	[-N]\n\
 /* prototype file */	[-p fname]\n\
 /* quiet */		[-q]\n\
-/* realtime subvol */	[-r extsize=num,size=num,rtdev=xxx,rgcount=n,rgsize=n]\n\
+/* realtime subvol */	[-r extsize=num,size=num,rtdev=xxx,rgcount=n,rgsize=n,\n\
+			    concurrency=num]\n\
 /* sectorsize */	[-s size=num]\n\
 /* version */		[-V]\n\
 			devicename\n\
@@ -2071,6 +2086,31 @@ proto_opts_parser(
 	return 0;
 }
 
+static void
+set_rtvol_concurrency(
+	struct opt_params	*opts,
+	int			subopt,
+	struct cli_params	*cli,
+	const char		*value)
+{
+	long long		optnum;
+
+	/*
+	 * "nr_cpus" or "1" means set the concurrency level to the CPU count.
+	 * If this cannot be determined, fall back to the default rtgroup
+	 * geometry.
+	 */
+	if (!value || !strcmp(value, "nr_cpus"))
+		optnum = 1;
+	else
+		optnum = getnum(value, opts, subopt);
+
+	if (optnum == 1)
+		cli->rtvol_concurrency = nr_cpus();
+	else
+		cli->rtvol_concurrency = optnum;
+}
+
 static int
 rtdev_opts_parser(
 	struct opt_params	*opts,
@@ -2101,6 +2141,9 @@ rtdev_opts_parser(
 	case R_RGSIZE:
 		cli->rgsize = getstr(value, opts, subopt);
 		break;
+	case R_CONCURRENCY:
+		set_rtvol_concurrency(opts, subopt, cli, value);
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -3740,10 +3783,97 @@ _("realtime group size (%llu) not at all congruent with extent size (%llu)\n"),
 	return 0;
 }
 
+static bool
+rtdev_is_solidstate(
+	struct libxfs_init	*xi)
+{
+	unsigned short		rotational = 1;
+	int			error;
+
+	error = ioctl(xi->rt.fd, BLKROTATIONAL, &rotational);
+	if (error)
+		return false;
+
+	return rotational == 0;
+}
+
+static void
+calc_concurrency_rtgroup_geometry(
+	struct mkfs_params	*cfg,
+	struct cli_params	*cli,
+	struct libxfs_init	*xi)
+{
+	uint64_t		try_rgsize;
+	uint64_t		def_rgsize;
+	uint64_t		def_rgcount;
+	int			nr_threads = cli->rtvol_concurrency;
+	int			try_threads;
+
+	if (is_power_of_2(cfg->rtextblocks))
+		def_rgsize = calc_rgsize_extsize_power(cfg);
+	else
+		def_rgsize = calc_rgsize_extsize_nonpower(cfg);
+	def_rgcount = howmany(cfg->rtblocks, def_rgsize);
+	try_rgsize = def_rgsize;
+
+	/*
+	 * If the caller doesn't have a particular concurrency level in mind,
+	 * set it to the number of CPUs in the system.
+	 */
+	if (nr_threads < 0)
+		nr_threads = nr_cpus();
+
+	/*
+	 * Don't create fewer rtgroups than what we would create with the
+	 * default geometry calculation.
+	 */
+	if (!nr_threads || nr_threads < def_rgcount)
+		goto out;
+
+	/*
+	 * Let's try matching the number of rtgroups to the number of CPUs.  If
+	 * the proposed geometry results in rtgroups smaller than 4GB, reduce
+	 * the rtgroup count until we have 4GB rtgroups.  Don't let the thread
+	 * count go below the default geometry calculation.
+	 */
+	try_threads = nr_threads;
+	try_rgsize = cfg->rtblocks / try_threads;
+	if (try_rgsize < GIGABYTES(4, cfg->blocklog)) {
+		do {
+			try_threads--;
+			if (try_threads <= def_rgcount) {
+				try_rgsize = def_rgsize;
+				goto out;
+			}
+
+			try_rgsize = cfg->rtblocks / try_threads;
+		} while (try_rgsize < GIGABYTES(4, cfg->blocklog));
+		goto out;
+	}
+
+	/*
+	 * For large filesystems we try to ensure that the rtgroup count is a
+	 * multiple of the desired thread count.  Specifically, if the proposed
+	 * rtgroup size is larger than both the maximum rtgroup size and the
+	 * rtgroup size we would have gotten with the defaults, add the thread
+	 * count to the rtgroup count until we get an rtgroup size below both
+	 * of those factors.
+	 */
+	while (try_rgsize > XFS_MAX_RGBLOCKS && try_rgsize > def_rgsize) {
+		try_threads += nr_threads;
+		try_rgsize = cfg->dblocks / try_threads;
+	}
+
+out:
+	cfg->rgsize = try_rgsize;
+	cfg->rgcount = howmany(cfg->rtblocks, cfg->rgsize);
+}
+
 static void
 calculate_rtgroup_geometry(
 	struct mkfs_params	*cfg,
-	struct cli_params	*cli)
+	struct cli_params	*cli,
+	struct libxfs_init	*xi)
 {
 	if (!cli->sb_feat.metadir) {
 		cfg->rgcount = 0;
@@ -3783,6 +3913,9 @@ _("rgsize (%s) not a multiple of fs blk size (%d)\n"),
 		/* too small even for a single group */
 		cfg->rgsize = cfg->rtblocks;
 		cfg->rgcount = 0;
+	} else if (cli->rtvol_concurrency > 0 ||
+		   (cli->data_concurrency == -1 && rtdev_is_solidstate(xi))) {
+		calc_concurrency_rtgroup_geometry(cfg, cli, xi);
 	} else if (is_power_of_2(cfg->rtextblocks)) {
 		cfg->rgsize = calc_rgsize_extsize_power(cfg);
 		cfg->rgcount = cfg->rtblocks / cfg->rgsize +
@@ -4890,6 +5023,7 @@ main(
 		.is_supported	= 1,
 		.data_concurrency = -1, /* auto detect non-mechanical storage */
 		.log_concurrency = -1, /* auto detect non-mechanical ddev */
+		.rtvol_concurrency = -1, /* auto detect non-mechanical rtdev */
 		.autofsck = FSPROP_AUTOFSCK_UNSET,
 	};
 	struct mkfs_params	cfg = {};
@@ -5077,7 +5211,7 @@ main(
 	 */
 	calculate_initial_ag_geometry(&cfg, &cli, &xi);
 	align_ag_geometry(&cfg);
-	calculate_rtgroup_geometry(&cfg, &cli);
+	calculate_rtgroup_geometry(&cfg, &cli, &xi);
 
 	calculate_imaxpct(&cfg, &cli);
 

Re: [PATCH 5/5] mkfs: allow sizing realtime allocation groups for concurrency

[Christoph Hellwig]

On Tue, Jan 14, 2025 at 01:41:40PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Add a -r concurrency= option to mkfs so that sysadmins can configure the
> filesystem so that there are enough rtgroups that the specified number
> of threads can (in theory) can find an uncontended rtgroup from which to
> allocate space.  This has the exact same purpose as the -d concurrency
> switch that was added for the data device.

Looks good:

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