[PATCHSET 0/1] mkfs: stop allowing tiny filesystems

[PATCHSET 0/1] 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.

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 |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 1 deletion(-)

[PATCH 1/1] 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 db322b3a..728a001a 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -847,6 +847,7 @@ struct cli_params {
 	int64_t	logagno;
 	int	loginternal;
 	int	lsunit;
+	int	has_warranty;
 
 	/* parameters where 0 is not a valid value */
 	int64_t	agcount;
@@ -2484,6 +2485,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_warranty(
+	struct xfs_mount	*mp,
+	struct cli_params	*cli)
+{
+	/* Undocumented option to enable unsupported tiny filesystems. */
+	if (!cli->has_warranty) {
+		printf(
+ _("Filesystems formatted with --yes-i-know-what-i-am-doing 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 redundant superblocks!\n"));
+		usage();
+	}
+}
+
 /*
  * Validate the configured stripe geometry, or is none is specified, pull
  * the configuration from the underlying device.
@@ -3933,9 +3996,21 @@ main(
 	struct cli_params	cli = {
 		.xi = &xi,
 		.loginternal = 1,
+		.has_warranty	= 1,
 	};
 	struct mkfs_params	cfg = {};
 
+	struct option		long_options[] = {
+	{
+		.name		= "yes-i-know-what-i-am-doing",
+		.has_arg	= no_argument,
+		.flag		= &cli.has_warranty,
+		.val		= 0,
+	},
+	{NULL, 0, NULL, 0 },
+	};
+	int			option_index = 0;
+
 	/* build time defaults */
 	struct mkfs_default_params	dft = {
 		.source = _("package build definitions"),
@@ -3995,8 +4070,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;
@@ -4134,6 +4212,8 @@ main(
 	validate_extsize_hint(mp, &cli);
 	validate_cowextsize_hint(mp, &cli);
 
+	validate_warranty(mp, &cli);
+
 	/* Print the intended geometry of the fs. */
 	if (!quiet || dry_run) {
 		struct xfs_fsop_geom	geo;

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

[Eric Sandeen]

On 6/28/22 3:50 PM, Darrick J. Wong wrote:
> 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.

This bugs me, because we're now explicitly testing filesystems that nobody
will be allowed to use in real life. Just seems odd. But so be it, I guess.
I understand why, it's just bleah.  If I care enough, I could try to whittle
away at those tests and remove this hack some day.

> 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 db322b3a..728a001a 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -847,6 +847,7 @@ struct cli_params {
>  	int64_t	logagno;
>  	int	loginternal;
>  	int	lsunit;
> +	int	has_warranty;
>  
>  	/* parameters where 0 is not a valid value */
>  	int64_t	agcount;
> @@ -2484,6 +2485,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_warranty(
> +	struct xfs_mount	*mp,
> +	struct cli_params	*cli)
> +{
> +	/* Undocumented option to enable unsupported tiny filesystems. */
> +	if (!cli->has_warranty) {
> +		printf(
> + _("Filesystems formatted with --yes-i-know-what-i-am-doing are not supported!!\n"));

maybe we can just make this "--unsupported" to be concise and self-documenting.

> +		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();
> +	}

So in practice, on striped storage this will require the filesystem to be a
bit over 500M to satisfy this constraint.  I worry about this constraint a
little.

# mkfs.xfs -dfile,name=fsfile,size=510m,su=32k,sw=4
Log size must be at least 64MB.

<hapless user reads manpage, adjusts log size>

# mkfs.xfs -dfile,name=fsfile,size=510m,su=32k,sw=4 -l size=64m
internal log size 16384 too large, must be less than 16301

So the log must be both "at least 64MB" and also "less than 64MB"

In reality, the problem is the filesystem size on this type of storage,
not the log size.

Let me think more about this. I understand and agree with the goal, I want
to do it in a way that doesn't cause user confusion...

> +	/*
> +	 * 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 redundant superblocks!\n"));

I think we should say "at least 2 AGs" because that's what can be directly
specified by the user. They won't know what it means to have redundant
supers.

> +		usage();	
> +	}
> +}
> +
>  /*
>   * Validate the configured stripe geometry, or is none is specified, pull
>   * the configuration from the underlying device.
> @@ -3933,9 +3996,21 @@ main(
>  	struct cli_params	cli = {
>  		.xi = &xi,
>  		.loginternal = 1,
> +		.has_warranty	= 1,
>  	};
>  	struct mkfs_params	cfg = {};
>  
> +	struct option		long_options[] = {
> +	{
> +		.name		= "yes-i-know-what-i-am-doing",
> +		.has_arg	= no_argument,
> +		.flag		= &cli.has_warranty,
> +		.val		= 0,
> +	},
> +	{NULL, 0, NULL, 0 },
> +	};
> +	int			option_index = 0;
> +
>  	/* build time defaults */
>  	struct mkfs_default_params	dft = {
>  		.source = _("package build definitions"),
> @@ -3995,8 +4070,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;
> @@ -4134,6 +4212,8 @@ main(
>  	validate_extsize_hint(mp, &cli);
>  	validate_cowextsize_hint(mp, &cli);
>  
> +	validate_warranty(mp, &cli);
> +
>  	/* Print the intended geometry of the fs. */
>  	if (!quiet || dry_run) {
>  		struct xfs_fsop_geom	geo;
> 

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

[Darrick J. Wong]

On Wed, Jul 13, 2022 at 08:09:24PM -0500, Eric Sandeen wrote:
> On 6/28/22 3:50 PM, Darrick J. Wong wrote:
> > 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.
> 
> This bugs me, because we're now explicitly testing filesystems that nobody
> will be allowed to use in real life. Just seems odd. But so be it, I guess.
> I understand why, it's just bleah.  If I care enough, I could try to whittle
> away at those tests and remove this hack some day.

Well now the nrext64=1 log size increases have caused breakage in
fstests.  This motivates me to get rid of all those tiny filesystems
that it creates, so this won't be around much longer.

> > 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 db322b3a..728a001a 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -847,6 +847,7 @@ struct cli_params {
> >  	int64_t	logagno;
> >  	int	loginternal;
> >  	int	lsunit;
> > +	int	has_warranty;
> >  
> >  	/* parameters where 0 is not a valid value */
> >  	int64_t	agcount;
> > @@ -2484,6 +2485,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_warranty(
> > +	struct xfs_mount	*mp,
> > +	struct cli_params	*cli)
> > +{
> > +	/* Undocumented option to enable unsupported tiny filesystems. */
> > +	if (!cli->has_warranty) {
> > +		printf(
> > + _("Filesystems formatted with --yes-i-know-what-i-am-doing are not supported!!\n"));
> 
> maybe we can just make this "--unsupported" to be concise and self-documenting.

Ok.  Though I was channelling my inner Jörg Schilling when I made that
fugly long option name.

> > +		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();
> > +	}
> 
> So in practice, on striped storage this will require the filesystem to be a
> bit over 500M to satisfy this constraint.  I worry about this constraint a
> little.
> 
> # mkfs.xfs -dfile,name=fsfile,size=510m,su=32k,sw=4
> Log size must be at least 64MB.
> 
> <hapless user reads manpage, adjusts log size>
> 
> # mkfs.xfs -dfile,name=fsfile,size=510m,su=32k,sw=4 -l size=64m
> internal log size 16384 too large, must be less than 16301
> 
> So the log must be both "at least 64MB" and also "less than 64MB"
> 
> In reality, the problem is the filesystem size on this type of storage,
> not the log size.
> 
> Let me think more about this. I understand and agree with the goal, I want
> to do it in a way that doesn't cause user confusion...

1GB? :D

> > +	/*
> > +	 * 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 redundant superblocks!\n"));
> 
> I think we should say "at least 2 AGs" because that's what can be directly
> specified by the user. They won't know what it means to have redundant
> supers.

Ok.

--D

> 
> > +		usage();	
> > +	}
> > +}
> > +
> >  /*
> >   * Validate the configured stripe geometry, or is none is specified, pull
> >   * the configuration from the underlying device.
> > @@ -3933,9 +3996,21 @@ main(
> >  	struct cli_params	cli = {
> >  		.xi = &xi,
> >  		.loginternal = 1,
> > +		.has_warranty	= 1,
> >  	};
> >  	struct mkfs_params	cfg = {};
> >  
> > +	struct option		long_options[] = {
> > +	{
> > +		.name		= "yes-i-know-what-i-am-doing",
> > +		.has_arg	= no_argument,
> > +		.flag		= &cli.has_warranty,
> > +		.val		= 0,
> > +	},
> > +	{NULL, 0, NULL, 0 },
> > +	};
> > +	int			option_index = 0;
> > +
> >  	/* build time defaults */
> >  	struct mkfs_default_params	dft = {
> >  		.source = _("package build definitions"),
> > @@ -3995,8 +4070,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;
> > @@ -4134,6 +4212,8 @@ main(
> >  	validate_extsize_hint(mp, &cli);
> >  	validate_cowextsize_hint(mp, &cli);
> >  
> > +	validate_warranty(mp, &cli);
> > +
> >  	/* Print the intended geometry of the fs. */
> >  	if (!quiet || dry_run) {
> >  		struct xfs_fsop_geom	geo;
> >