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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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;
> >
