[PATCHSET v3 0/2] mkfs: stop allowing tiny filesystems

[PATCHSET v3 0/2] mkfs: stop allowing tiny filesystems

[Darrick J. Wong]

Hi all,

The maintainers have been besieged by a /lot/ of complaints recently
from people who format tiny filesystems and growfs them into huge ones,
and others who format small filesystems.  We don't really want people to
have filesystems with no backup superblocks, and there are myriad
performance problems on modern-day filesystems when the log gets too
small.

Empirical evidence shows that increasing the minimum log size to 64MB
eliminates most of the stalling problems and other unwanted behaviors,
so this series makes that change and then disables creation of small
filesystems, which are defined as single-AGs fses, fses with a log size
smaller than 64MB, and fses smaller than 300MB.

v2: rebase to 5.19
v3: disable automatic detection of raid stripes when the device is less
    than 1G to avoid formatting failures

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=mkfs-forbid-tiny-fs
---
 mkfs/xfs_mkfs.c |   96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 1 deletion(-)

[PATCH 1/2] mkfs: ignore data blockdev stripe geometry for small filesystems

[Darrick J. Wong]

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

As part of the process of removing support for tiny filesystems (defined
in the next patch to be anything under 300MB or 64M log size), we are
trying to eliminate all the edge case regressions for small filesystems
that the maintainer can find.

Eric pointed out that the use case of formatting a 510M on a RAID device
regresses once we start enforcing the 64M log size limit:

# modprobe scsi_debug opt_blks=256 opt_xferlen_exp=6 dev_size_mb=510
# mkfs.xfs /dev/sdg
Log size must be at least 64MB.

<hapless user reads manpage, adjusts log size>

# mkfs.xfs -l size=64m /dev/sdg
internal log size 16384 too large, must be less than 16301

Because the device reports a stripe geometry, mkfs tries to create 8 AGs
(instead of the usual 4) which are then very nearly 64M in size.  The
log itself cannot consume the entire AG, so its size is decreased, so
its size is rounded down to allow the creation of AG headers and btrees,
and then the log size is rounded down again to match the stripe unit.
This results in a log that is less than 64MB in size, causing the format
to fail.

There's not much point in formatting tiny AGs on a small filesystem,
even if it is on a RAID.  Doubling the AG count from 4 to 8 doubles the
metadata overhead, conflicts with our attempts to boost the log size,
and on 2022-era storage hardware gains us very little extra performance
since we're not limited by storage access times.

Therefore, disable automatic detection of stripe unit and width if the
data device is less than 1GB.  We would like to format with 128M AGs to
avoid constraining the size of the internal log, and since RAIDs smaller
than 8GB are formatted with 8 AGs by default, 128*8=1G was chosen as the
cutoff.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index a5e2df76..68d6bd18 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2583,6 +2583,20 @@ _("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\
 				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
 			ft->dsunit = 0;
 			ft->dswidth = 0;
+		} else if (cfg->dblocks < GIGABYTES(1, cfg->blocklog)) {
+			/*
+			 * Don't use automatic stripe detection if the device
+			 * size is less than 1GB because the performance gains
+			 * on such a small system are not worth the risk that
+			 * we'll end up with an undersized log.
+			 */
+			if (ft->dsunit || ft->dswidth)
+				fprintf(stderr,
+_("%s: small data volume, ignoring data volume stripe unit %d and stripe width %d\n"),
+						progname, ft->dsunit,
+						ft->dswidth);
+			ft->dsunit = 0;
+			ft->dswidth = 0;
 		} else {
 			dsunit = ft->dsunit;
 			dswidth = ft->dswidth;

Re: [PATCH 1/2] mkfs: ignore data blockdev stripe geometry for small filesystems

[Carlos Maiolino]

> ---
>  mkfs/xfs_mkfs.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index a5e2df76..68d6bd18 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2583,6 +2583,20 @@ _("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\
>  				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
>  			ft->dsunit = 0;
>  			ft->dswidth = 0;
> +		} else if (cfg->dblocks < GIGABYTES(1, cfg->blocklog)) {
> +			/*
> +			 * Don't use automatic stripe detection if the device
> +			 * size is less than 1GB because the performance gains
> +			 * on such a small system are not worth the risk that
> +			 * we'll end up with an undersized log.
> +			 */
> +			if (ft->dsunit || ft->dswidth)
> +				fprintf(stderr,
> +_("%s: small data volume, ignoring data volume stripe unit %d and stripe width %d\n"),
> +						progname, ft->dsunit,
> +						ft->dswidth);
> +			ft->dsunit = 0;
> +			ft->dswidth = 0;
>  		} else {
>  			dsunit = ft->dsunit;
>  			dswidth = ft->dswidth;
> 

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

-- 
Carlos Maiolino

[PATCH 2/2] mkfs: stop allowing tiny filesystems

[Darrick J. Wong]

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

Refuse to format a filesystem that are "too small", because these
configurations are known to have performance and redundancy problems
that are not present on the volume sizes that XFS is best at handling.

Specifically, this means that we won't allow logs smaller than 64MB, we
won't allow single-AG filesystems, and we won't allow volumes smaller
than 300MB.  There are two exceptions: the first is an undocumented CLI
option that can be used for crafting debug filesystems.

The second exception is that if fstests is detected, because there are a
lot of fstests that use tiny filesystems to perform targeted regression
and functional testing in a controlled environment.  Fixing the ~40 or
so tests to run more slowly with larger filesystems isn't worth the risk
of breaking the tests.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 1 deletion(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 68d6bd18..9dd0e79c 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -859,6 +859,7 @@ struct cli_params {
 	int64_t	logagno;
 	int	loginternal;
 	int	lsunit;
+	int	is_supported;
 
 	/* parameters where 0 is not a valid value */
 	int64_t	agcount;
@@ -2496,6 +2497,68 @@ _("illegal CoW extent size hint %lld, must be less than %u.\n"),
 	}
 }
 
+/* Complain if this filesystem is not a supported configuration. */
+static void
+validate_supported(
+	struct xfs_mount	*mp,
+	struct cli_params	*cli)
+{
+	/* Undocumented option to enable unsupported tiny filesystems. */
+	if (!cli->is_supported) {
+		printf(
+ _("Filesystems formatted with --unsupported are not supported!!\n"));
+		return;
+	}
+
+	/*
+	 * fstests has a large number of tests that create tiny filesystems to
+	 * perform specific regression and resource depletion tests in a
+	 * controlled environment.  Avoid breaking fstests by allowing
+	 * unsupported configurations if TEST_DIR, TEST_DEV, and QA_CHECK_FS
+	 * are all set.
+	 */
+	if (getenv("TEST_DIR") && getenv("TEST_DEV") && getenv("QA_CHECK_FS"))
+		return;
+
+	/*
+	 * We don't support filesystems smaller than 300MB anymore.  Tiny
+	 * filesystems have never been XFS' design target.  This limit has been
+	 * carefully calculated to prevent formatting with a log smaller than
+	 * the "realistic" size.
+	 *
+	 * If the realistic log size is 64MB, there are four AGs, and the log
+	 * AG should be at least 1/8 free after formatting, this gives us:
+	 *
+	 * 64MB * (8 / 7) * 4 = 293MB
+	 */
+	if (mp->m_sb.sb_dblocks < MEGABYTES(300, mp->m_sb.sb_blocklog)) {
+		fprintf(stderr,
+ _("Filesystem must be larger than 300MB.\n"));
+		usage();
+	}
+
+	/*
+	 * For best performance, we don't allow unrealistically small logs.
+	 * See the comment for XFS_MIN_REALISTIC_LOG_BLOCKS.
+	 */
+	if (mp->m_sb.sb_logblocks <
+			XFS_MIN_REALISTIC_LOG_BLOCKS(mp->m_sb.sb_blocklog)) {
+		fprintf(stderr,
+ _("Log size must be at least 64MB.\n"));
+		usage();
+	}
+
+	/*
+	 * Filesystems should not have fewer than two AGs, because we need to
+	 * have redundant superblocks.
+	 */
+	if (mp->m_sb.sb_agcount < 2) {
+		fprintf(stderr,
+ _("Filesystem must have at least 2 superblocks for redundancy!\n"));
+		usage();
+	}
+}
+
 /*
  * Validate the configured stripe geometry, or is none is specified, pull
  * the configuration from the underlying device.
@@ -3966,9 +4029,21 @@ main(
 	struct cli_params	cli = {
 		.xi = &xi,
 		.loginternal = 1,
+		.is_supported	= 1,
 	};
 	struct mkfs_params	cfg = {};
 
+	struct option		long_options[] = {
+	{
+		.name		= "unsupported",
+		.has_arg	= no_argument,
+		.flag		= &cli.is_supported,
+		.val		= 0,
+	},
+	{NULL, 0, NULL, 0 },
+	};
+	int			option_index = 0;
+
 	/* build time defaults */
 	struct mkfs_default_params	dft = {
 		.source = _("package build definitions"),
@@ -4028,8 +4103,11 @@ main(
 	memcpy(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat));
 	memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx));
 
-	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
+	while ((c = getopt_long(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV",
+					long_options, &option_index)) != EOF) {
 		switch (c) {
+		case 0:
+			break;
 		case 'C':
 		case 'f':
 			force_overwrite = 1;
@@ -4167,6 +4245,8 @@ main(
 	validate_extsize_hint(mp, &cli);
 	validate_cowextsize_hint(mp, &cli);
 
+	validate_supported(mp, &cli);
+
 	/* Print the intended geometry of the fs. */
 	if (!quiet || dry_run) {
 		struct xfs_fsop_geom	geo;

Re: [PATCH 2/2] mkfs: stop allowing tiny filesystems

[Carlos Maiolino]

> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 68d6bd18..9dd0e79c 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -859,6 +859,7 @@ struct cli_params {
>  	int64_t	logagno;
>  	int	loginternal;
>  	int	lsunit;
> +	int	is_supported;
> 
>  	/* parameters where 0 is not a valid value */
>  	int64_t	agcount;
> @@ -2496,6 +2497,68 @@ _("illegal CoW extent size hint %lld, must be less than %u.\n"),
>  	}
>  }
> 
> +/* Complain if this filesystem is not a supported configuration. */
> +static void
> +validate_supported(
> +	struct xfs_mount	*mp,
> +	struct cli_params	*cli)
> +{
> +	/* Undocumented option to enable unsupported tiny filesystems. */
> +	if (!cli->is_supported) {
> +		printf(
> + _("Filesystems formatted with --unsupported are not supported!!\n"));
> +		return;
> +	}
> +
> +	/*
> +	 * fstests has a large number of tests that create tiny filesystems to
> +	 * perform specific regression and resource depletion tests in a
> +	 * controlled environment.  Avoid breaking fstests by allowing
> +	 * unsupported configurations if TEST_DIR, TEST_DEV, and QA_CHECK_FS
> +	 * are all set.
> +	 */
> +	if (getenv("TEST_DIR") && getenv("TEST_DEV") && getenv("QA_CHECK_FS"))
> +		return;
> +
> +	/*
> +	 * We don't support filesystems smaller than 300MB anymore.  Tiny
> +	 * filesystems have never been XFS' design target.  This limit has been
> +	 * carefully calculated to prevent formatting with a log smaller than
> +	 * the "realistic" size.
> +	 *
> +	 * If the realistic log size is 64MB, there are four AGs, and the log
> +	 * AG should be at least 1/8 free after formatting, this gives us:
> +	 *
> +	 * 64MB * (8 / 7) * 4 = 293MB
> +	 */
> +	if (mp->m_sb.sb_dblocks < MEGABYTES(300, mp->m_sb.sb_blocklog)) {
> +		fprintf(stderr,
> + _("Filesystem must be larger than 300MB.\n"));
> +		usage();
> +	}
> +
> +	/*
> +	 * For best performance, we don't allow unrealistically small logs.
> +	 * See the comment for XFS_MIN_REALISTIC_LOG_BLOCKS.
> +	 */
> +	if (mp->m_sb.sb_logblocks <
> +			XFS_MIN_REALISTIC_LOG_BLOCKS(mp->m_sb.sb_blocklog)) {
> +		fprintf(stderr,
> + _("Log size must be at least 64MB.\n"));
> +		usage();
> +	}
> +
> +	/*
> +	 * Filesystems should not have fewer than two AGs, because we need to
> +	 * have redundant superblocks.
> +	 */
> +	if (mp->m_sb.sb_agcount < 2) {
> +		fprintf(stderr,
> + _("Filesystem must have at least 2 superblocks for redundancy!\n"));
> +		usage();
> +	}
> +}
> +
>  /*
>   * Validate the configured stripe geometry, or is none is specified, pull
>   * the configuration from the underlying device.
> @@ -3966,9 +4029,21 @@ main(
>  	struct cli_params	cli = {
>  		.xi = &xi,
>  		.loginternal = 1,
> +		.is_supported	= 1,
>  	};
>  	struct mkfs_params	cfg = {};
> 
> +	struct option		long_options[] = {
> +	{
> +		.name		= "unsupported",
> +		.has_arg	= no_argument,
> +		.flag		= &cli.is_supported,
> +		.val		= 0,
> +	},
> +	{NULL, 0, NULL, 0 },
> +	};
> +	int			option_index = 0;
> +
>  	/* build time defaults */
>  	struct mkfs_default_params	dft = {
>  		.source = _("package build definitions"),
> @@ -4028,8 +4103,11 @@ main(
>  	memcpy(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat));
>  	memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx));
> 
> -	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> +	while ((c = getopt_long(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV",
> +					long_options, &option_index)) != EOF) {
>  		switch (c) {
> +		case 0:
> +			break;
>  		case 'C':
>  		case 'f':
>  			force_overwrite = 1;
> @@ -4167,6 +4245,8 @@ main(
>  	validate_extsize_hint(mp, &cli);
>  	validate_cowextsize_hint(mp, &cli);
> 
> +	validate_supported(mp, &cli);
> +
>  	/* Print the intended geometry of the fs. */
>  	if (!quiet || dry_run) {
>  		struct xfs_fsop_geom	geo;
> 

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

-- 
Carlos Maiolino

Re: [PATCHSET v3 0/2] mkfs: stop allowing tiny filesystems

[Carlos Maiolino]

On Tue, Jul 19, 2022 at 02:44:58PM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> The maintainers have been besieged by a /lot/ of complaints recently
> from people who format tiny filesystems and growfs them into huge ones,
> and others who format small filesystems.  We don't really want people to
> have filesystems with no backup superblocks, and there are myriad
> performance problems on modern-day filesystems when the log gets too
> small.
> 
> Empirical evidence shows that increasing the minimum log size to 64MB
> eliminates most of the stalling problems and other unwanted behaviors,
> so this series makes that change and then disables creation of small
> filesystems, which are defined as single-AGs fses, fses with a log size
> smaller than 64MB, and fses smaller than 300MB.
> 
> v2: rebase to 5.19
> v3: disable automatic detection of raid stripes when the device is less
>     than 1G to avoid formatting failures
> 
> 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.
> 

Both changes looks good to me, but IMHO we really require it to be documented in
manpages otherwise we'll get (even more) questions about "why can't I create
small FS'es anymore?".
But anyway, I can help with the manpages once these patches hit for-next if you
are ok with it.

-- 
Carlos Maiolino

Re: [PATCHSET v3 0/2] mkfs: stop allowing tiny filesystems

[Darrick J. Wong]

On Mon, Jul 25, 2022 at 09:59:42AM +0200, Carlos Maiolino wrote:
> On Tue, Jul 19, 2022 at 02:44:58PM -0700, Darrick J. Wong wrote:
> > Hi all,
> > 
> > The maintainers have been besieged by a /lot/ of complaints recently
> > from people who format tiny filesystems and growfs them into huge ones,
> > and others who format small filesystems.  We don't really want people to
> > have filesystems with no backup superblocks, and there are myriad
> > performance problems on modern-day filesystems when the log gets too
> > small.
> > 
> > Empirical evidence shows that increasing the minimum log size to 64MB
> > eliminates most of the stalling problems and other unwanted behaviors,
> > so this series makes that change and then disables creation of small
> > filesystems, which are defined as single-AGs fses, fses with a log size
> > smaller than 64MB, and fses smaller than 300MB.
> > 
> > v2: rebase to 5.19
> > v3: disable automatic detection of raid stripes when the device is less
> >     than 1G to avoid formatting failures
> > 
> > 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.
> > 
> 
> Both changes looks good to me, but IMHO we really require it to be documented in
> manpages otherwise we'll get (even more) questions about "why can't I create
> small FS'es anymore?".
> But anyway, I can help with the manpages once these patches hit for-next if you
> are ok with it.

Oooh, good point, Eric and I have been too busy figuring out the weird
corner cases and forgot that.  I'll add some manpage updates and send
that out tomorrow.

--D

> 
> -- 
> Carlos Maiolino