Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: Dave Chinner <david@fromorbit.com>,
	Eric Sandeen <sandeen@sandeen.net>,
	sandeen@redhat.com, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements
Date: Thu, 14 Jun 2018 10:59:09 -0700	[thread overview]
Message-ID: <20180614175909.GB8128@magnolia> (raw)
In-Reply-To: <20180614174640.GE7508@wotan.suse.de>

On Thu, Jun 14, 2018 at 07:46:40PM +0200, Luis R. Rodriguez wrote:
> On Wed, Jun 13, 2018 at 11:29:49PM -0700, Darrick J. Wong wrote:
> > On Thu, Jun 14, 2018 at 03:08:44PM +1000, Dave Chinner wrote:
> > > On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote:
> > > > On 6/13/18 11:06 PM, Dave Chinner wrote:
> > > > > With this change, we'll have code to maintain it to ensure that the
> > > > > file gets updated properly, and it will probably take more time and
> > > > > effort to validate that the generated file is correct (and debug if
> > > > > it's not!) compared to the 30s it will take to hand edit the
> > > > > template file to change or add a new default...
> 
> If we had an xfstest to *test* that same generated file, this would not
> be an issue and from what I gather we need quite a bit of work to get there.
> 
> I'm working on a test for config stuff but that will just test for now
> (and this reveals some future work needed):
> 
> a) a set of config files we know should work and ensure they produce the
>    same filesystem as if we had used CLI params. We can use xfs_db -c version
>    against both filesystems and check that each differences. Since this would
>    use the same xfsprogs for the results of a config based filesystem and
>    the CLI based filesystem the diff would only generate if there really
>    was a change between both runs, and you can use any xfsprogs version
>    for it.

Hmm, just out of curiosity, are there any mkfs cli/config options that
do /not/ show up in the output of mkfs and/or 'xfs_db -c info'?

>    It does however leave actual expected results on the filesystem up to
>    a separate test, it assumes that xfs/191-input-validation is doing its
>    job, but word is that needs some love.
> 
> b) a set of invalid config files and ensure they never work

Agreed, I have some twisty ones of my own, though fixing them requires
some amount of sscanf format string tweaking. :)

> c) test to ensure cli can override config params

<nod>

> To test that a self generated config file works would be a next step (d),
> and I think we can validate it by also making sure it yields the same
> filesystem as if we just ran mkfs.xfs with no options, so same strategy
> as in a).

Ok, good.  I'd also argue for a test that tries every file in
/etc/xfs/mkfs/* to see if mkfs will format the filesystem described in
the config file, that way we can pick up all the distro-packaged files.

> > Counterpoint: the mkfs config file template in mkfs.xfs.8 is already
> > incorrect; sparse inodes was enabled by default in 4.16 but the manpage
> > cfg file says it isn't.  This is exactly what happens when we rely on
> > ourselves to hand edit the template file.
> > 
> > Put the defaults in *one* place in the source code tree and generate
> > everything else off of that, instead of keeping multiple copies that
> > will then get out of sync.  We clearly suck at this maintenance.
> 
> I personally prefer this automation, so with this is mind I think this
> is a change in the right direction.

:)

> > > > Well, yeah.  /if/ we need a template, as well as text in a man page,
> > > > then this gets it down to editing 1 file instead of two, I guess.
> > > > But it really seems like we need to rethink these structures to get it
> > > > all tied together, not requiring 2 or 3 manual updates across several files.
> > > > It's bound to get out of sync.  I guess that can wait, but right now this
> > > > dispersal isn't good.
> > > 
> > > So let's get the basic conig file stuff in first, then cosolidate,
> > > then add all the bells and whistles.
> > 
> > That's where I started from -- fix the existing mess with the manpage,
> 
> That should be a separate patch first.

> > then fix the problems in the parsing code, then move all the CLI parsing and
> > validation code out of xfs_mkfs.c because it's just too long.
> 
> Yay!
> 
> > Granted, I haven't gotten all the way there yet, so perhaps it was a bad
> > idea to post just the first of those three parts.
> > 
> > > Too many cooks trying to add all their own bells and whistles before
> > > the core behaviour, infrastructure and implementation was nailed
> > > down was pretty much what lead all the tablised CLI option parsing
> > > code. And we're doing it again with this config file stuff...
> > 
> > Yeah.  TBH I feel at a loss for what exactly your vision is for how mkfs
> > option parsing is supposed to work in the end.  How close is this:
> 
> BTW it seems we may need to document this vision as otherwise others may
> run into issues later as well.

Yes.  This should go in xfs_mkfs.c after moving all the other stuff out.

> > 1. First we have the mkfs_default_params, which are the builtin
> > defaults.
> > 
> > 2. Configuration file parameters are supposed to change the
> > mkfs_default_params, yes?
> > 
> > 3. The mkfs_default_params are used to seed the cli_params.
> > 
> > 4. The getopt loop pulls the CLI option data into the cli_params.
> > 
> > 5. At this point we've melded together the builtin options
> > with whatever the config file told us about, then folded in the CLI
> > options, right?
> > 
> > 6. Now we start calling the validate_* and calculate_* functions.  These
> > functions check the full set of configuration data and copy them into
> > the mkfs_params; and they calculate whatever other bits are needed to
> > finish the mkfs_params.
> > 
> > 7. Finally the validated mkfs_params are fed to the initialisation
> > functions to set up the actual filesystem.
> 
> FWIW this at least matches my own expectations so far.
> 
> > So AFAICT what we have now is a big collection of ops_params that
> > contain all the cli parsing options and a bunch of functions that take
> > the cli strings and fill out the appropriate parts of cli_params.
> > 
> > Luis built a confopts_tab(le) that mapped cfgfile options to parsing
> > functions that know how to take the cfgfile strings and fill out the
> > appropriate parts of mkfs_default_params.  I understand now that from
> > these first five patches it sure looks like I'm building yet a third
> > structure, but this isn't the endgame -- I want a data structure that
> > maps cfgfile options directly to the mkfs default params.  I'm doing
> > this by building the new direct-mapped structure on the side and
> > migrating the parser code to use that.
> > 
> > Ås for "how do I check that the distro provided default template even
> > works?", I'm working on building an xfstest to make sure that the
> > default templates we ship (and whatever might be in
> > /etc/xfs/mkfs/default) actually produce a functioning filesystem.
> 
> With proper testing in place for regular configs stuff (the tests
> I have to write) and this I see your changes as desirable but I'm
> wondering if this should just wait until the next cycle so we at
> least have some effort on the testing started already?

Agreed.  I'm scurrying back under the rocks so that Eric can get
xfsprogs 4.17 out the door.

(I'm actually going to go debug some hardware and see if I can get
fscounter scrub working.)

--D

>   Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html