mkfs.xfs: add configuration file parsing support using our own parser
diff mbox

Message ID 20180313205911.26082-1-mcgrof@kernel.org
State Superseded
Headers show

Commit Message

Luis Chamberlain March 13, 2018, 8:59 p.m. UTC
You may want to stick to specific set of configuration options when
creating filesystems with mkfs.xfs -- sometimes due to pure technical
reasons, but some other times to ensure systems remain compatible as
new features are introduced with older kernels, or if you always want
to take advantage of some new feature which would otherwise typically
be disruptive.

This adds support for parsing a configuration file to override defaults
parameters to be used for mkfs.xfs.

We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for
different types of configuration files, if none is specified we look for
the default type, /etc/mkfs.xfs.d/default, and you can override with -T.
For instance, if you specify:

	mkfs.xfs -T experimental -f /dev/loop0

The file /etc/mkfs.xfs.d/experimental will be used as your configuration
file. If you really need to override the full path of the configuration
file you may use the MKFS_XFS_CONFIG environment variable.

To use /etc/ be sure to configure xfsprogs with:

 ./configure --sysconfdir=/etc/

To verify what configuration file is used on a system use the typical:

  mkfs.xfs -N

There is only a subset of options allowed to be set on the configuration
file, and currently only 1 or 0 are acceptable values. The default
parameters you can override on a configuration file and their current
built-in default settings are:

[data]
noalign=0

[inode]
align=1
projid32bit=1
sparse=0

[log]
lazy-count=1

[metadata]
crc=1
finobt=1
rmapbt=0
reflink=0

[naming]
ftype=1

[rtdev]
noalign=0

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---

We have floated around enough bike shedding emails to have at least reached
a consensus on the fact that we'd be only supporting a mimimum set of default
parameters and would strive to simplify our parser as much as possible.

We never really decided on what parser to use though. To help with this I've
implemented what we have discussed so far with 3 different parsers options and
provide below URLs to both just a single patch needed for each on top today's
xfsprogs's origin/for-next and also provide a respective xfsprogs branch on
kernel.org:

  a) Our own parser (this patch) [0] [1]
  b) Using libini_config [2] [3]
  c) Using e2fsprogs profile parser [4] [5]

I decided to re-evaluate using our own parser as well again, despite the
original stated goal to not reinvent the wheel, since we decided to purposely
simplify the parser as much as possible. To this end I also limited the default
parameters we can override via the configuration file only to settings which we
can enable or disable, so default parameters for which we can only set their
values to 1 or 0. Final mkfs.xfs size evaluations also reveal carrying our own
simple parser may be worth considering for now, provided at the end of this
email.

Using our own parser is not a requirement, it just seems to make sense for now
given the simplicity of our current parsing needs. We can switch to a library
at any time. We can use either libini_config or the e2fsprogs profile parser,
if we are willing to depend on an external shared library or use a static
library.

Using a library libini_config proved to be the most robust and actively engaged
library to consider [6].

Using the e2fsprogs profile parser may make sense if we wanted to groom and
optimize a library for sizes *together* with the community. If one of our goals
is to use a *shared library*, and if one of the requirements for this library
is for it be as small as possible, we may want to consider grooming together
with the community a future *shared profile parser*. An independent e2fsprogs
profile parser today is about 14.86% the size of libini_config.so.5.2.0 [6],
as such those really counting bytes may like this approach.

However implementing our own simple parser still produces the smallest binary,
and dependencies today.

I believe that the size gains of using the e2fsprogs profile parser only makes
sense if we want to *reduce overall shared space* with other filesystem
configuration tools.  I've already taken the e2fsprogs profile parser and made
it a standalone library which we can use [7] and Ted is willing to help with
this, should we want to embrark on that route.

Current file size evaluations using the different parsing options reveal that
carrying our own parser produces the smallest binary. Using libini_config
actually has an extra penalty of 576 bytes over carrying our own parser.

Using e2fsprogs actually carries an added penalty of 79776 bytes over using
and carrying our own simple parser at this point.

Because of all this I've tried to simplify our own parser as much as possible,
and strived to keep the code as readible as possible.

Current size evaluations with the different parser options:

own-parser:
$ size mkfs/mkfs.xfs
   text    data     bss     dec     hex filename
 394164   38448     568  433180   69c1c mkfs/mkfs.xfs
$ du -b mkfs/mkfs.xfs
2552400 mkfs/mkfs.xfs

libini_config
$ size mkfs/mkfs.xfs
   text    data     bss     dec     hex filename
 395160   38568     568  434296   6a078 mkfs/mkfs.xfs
$ du -b mkfs/mkfs.xfs
2552976 mkfs/mkfs.xfs

e2fsprogs profile:
$ size mkfs/mkfs.xfs
   text    data     bss     dec     hex filename
 411913   38656     728  451297   6e2e1 mkfs/mkfs.xfs
$ du -b mkfs/mkfs.xfs
2632176 mkfs/mkfs.xfs

Let me know what color of the bike shed you'd like for this.

[0] http://drvbp1.linux-foundation.org/~mcgrof/patches/2018/03/13/own-parser.patch
[1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/xfsprogs-dev.git/log/?h=20180313-own-parser
[2] http://drvbp1.linux-foundation.org/~mcgrof/patches/2018/03/13/libini.patch
[3] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/xfsprogs-dev.git/log/?h=20180313-libini_config
[4] http://drvbp1.linux-foundation.org/~mcgrof/patches/2018/03/13/eprofile.patch
[5] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/xfsprogs-dev.git/log/?h=20180313-eprofile-parser
[6] https://gitlab.com/mcgrof/filesystem-configuration-paper/blob/master/paper.pdf
[7] https://gitlab.com/mcgrof/libekprofile

  Luis

 include/builddefs.in  |   2 +
 man/man5/mkfs.xfs.d.5 | 121 ++++++++++++++
 man/man8/mkfs.xfs.8   |  23 +++
 mkfs/xfs_mkfs.c       | 432 +++++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 559 insertions(+), 19 deletions(-)
 create mode 100644 man/man5/mkfs.xfs.d.5

Comments

Dave Chinner March 13, 2018, 9:39 p.m. UTC | #1
On Tue, Mar 13, 2018 at 01:59:11PM -0700, Luis R. Rodriguez wrote:
> You may want to stick to specific set of configuration options when
> creating filesystems with mkfs.xfs -- sometimes due to pure technical
> reasons, but some other times to ensure systems remain compatible as
> new features are introduced with older kernels, or if you always want
> to take advantage of some new feature which would otherwise typically
> be disruptive.
> 
> This adds support for parsing a configuration file to override defaults
> parameters to be used for mkfs.xfs.
> 
> We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for
> different types of configuration files, if none is specified we look for
> the default type, /etc/mkfs.xfs.d/default, and you can override with -T.
> For instance, if you specify:
> 
> 	mkfs.xfs -T experimental -f /dev/loop0

Just one obvious comment from a brief glance - you changed this from
"-t" to "-T", but ....

>  are equivalent.
>  .PP
> +An optional XFS configuration type file directory
> +.B mkfs.xfs.d (5)
> +exists to help fine tune default parameters which can be used when calling
> +.B mkfs.xfs (8), by default type will be used by default, /etc/mkfs.xfs.d/default.
> +Command line arguments directly passed to
> +.B mkfs.xfs (8)
> +will always override parameters set it the configuration type file.
> +You can override configuration file type on the
> +.B mkfs.xfs.d (5)
> +directory by using the -t parameter and secifying the type. Alternatively

Not here, or ....

> +you can set and use the MKFS_XFS_CONFIG environment variable to override
> +the default full path of the first file
> +.B mkfs.xfs (8)
> +looks for.
> +If you use -t the type configuration file must be present under

here, or ....

> +#define MKFS_XFS_CONF_DIR	ROOT_SYSCONFDIR "/mkfs.xfs.d/"
> +#define CONFIG_MAX_KEY		1024
> +#define CONFIG_MAX_VALUE	PATH_MAX
> +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> +#define PARAM_OPTS		"T:b:d:i:l:L:m:n:KNp:qr:s:CfV"

[ Please don't obfuscate parsing options like this ]

> @@ -3827,25 +4198,47 @@ main(
>  	textdomain(PACKAGE);
>  
>  	/*
> -	 * TODO: Sourcing defaults from a config file
> -	 *
>  	 * Before anything else, see if there's a config file with different
> -	 * defaults. If a file exists in <package location>, read in the new
> +	 * defaults. If a file exists in MKFS_XFS_CONF_DIR/default, read the new
>  	 * default values and overwrite them in the &dft structure. This way the
>  	 * new defaults will apply before we parse the CLI, and the CLI will
>  	 * still be able to override them. When more than one source is
>  	 * implemented, emit a message to indicate where the defaults being
>  	 * used came from.
> -	 *
> -	 * printf(_("Default configuration sourced from %s\n"), dft.source);
>  	 */
> +	tmp_config = getenv("MKFS_XFS_CONFIG");
> +	if (tmp_config != NULL) {
> +		dft.config_file = tmp_config;
> +		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
> +	}
> +
> +	c = getopt(argc, argv, PARAM_OPTS);
> +	if (c != EOF && c == 't') {

Here. Did you test this?

And, well, I have my doubts about this method of option parsing.
Where does it say in the getopt(3) man page that CLI options are
always parsed and returned in order of the option string rather than
the order the appear on the CLI?

I'll spend some more time looking at it, but my initial impression
is that there's a bit of work to be done yet...

Cheers,

Dave.
Luis Chamberlain March 13, 2018, 11:52 p.m. UTC | #2
On Wed, Mar 14, 2018 at 08:39:16AM +1100, Dave Chinner wrote:
> Just one obvious comment from a brief glance - you changed this from
> "-t" to "-T", but ....
> > +directory by using the -t parameter and secifying the type. Alternatively
> 
> Not here, or ....
>
> > +If you use -t the type configuration file must be present under
> 
> here, or ....
> 
> > +#define MKFS_XFS_CONF_DIR	ROOT_SYSCONFDIR "/mkfs.xfs.d/"
> > +#define CONFIG_MAX_KEY		1024
> > +#define CONFIG_MAX_VALUE	PATH_MAX
> > +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> > +#define PARAM_OPTS		"T:b:d:i:l:L:m:n:KNp:qr:s:CfV"
> 
> [ Please don't obfuscate parsing options like this ]

The reason was we use them twice.

> > @@ -3827,25 +4198,47 @@ main(
> >  	textdomain(PACKAGE);
> >  
> >  	/*
> > -	 * TODO: Sourcing defaults from a config file
> > -	 *
> >  	 * Before anything else, see if there's a config file with different
> > -	 * defaults. If a file exists in <package location>, read in the new
> > +	 * defaults. If a file exists in MKFS_XFS_CONF_DIR/default, read the new
> >  	 * default values and overwrite them in the &dft structure. This way the
> >  	 * new defaults will apply before we parse the CLI, and the CLI will
> >  	 * still be able to override them. When more than one source is
> >  	 * implemented, emit a message to indicate where the defaults being
> >  	 * used came from.
> > -	 *
> > -	 * printf(_("Default configuration sourced from %s\n"), dft.source);
> >  	 */
> > +	tmp_config = getenv("MKFS_XFS_CONFIG");
> > +	if (tmp_config != NULL) {
> > +		dft.config_file = tmp_config;
> > +		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
> > +	}
> > +
> > +	c = getopt(argc, argv, PARAM_OPTS);
> > +	if (c != EOF && c == 't') {
> 
> Here. Did you test this?

I forgot to test -T after we bike-shed from -t to -T, sorry. Will fix.
I had however tested -t for sure.

> And, well, I have my doubts about this method of option parsing.
> Where does it say in the getopt(3) man page that CLI options are
> always parsed and returned in order of the option string rather than
> the order the appear on the CLI?

You are right. Its rather unclear. If -t is passed at the end this may
not work.

> I'll spend some more time looking at it, but my initial impression
> is that there's a bit of work to be done yet...

You're right, we should decide if we want to allow for -T to be used
only in the beginning or if we want to allow for it anywhere on the
command line. If we allow for it only at the beginning of the command
line it may be easier and cleaner to support: we'd just check argv[1]
for -T manually and if passed just bump argv down two slots. So we
leave getopt() usage only for the other stuff.

Thoughts?

  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
Eric Sandeen March 14, 2018, 3:21 a.m. UTC | #3
On 3/13/18 4:59 PM, Luis R. Rodriguez wrote:
> You may want to stick to specific set of configuration options when
> creating filesystems with mkfs.xfs -- sometimes due to pure technical
> reasons, but some other times to ensure systems remain compatible as
> new features are introduced with older kernels, or if you always want
> to take advantage of some new feature which would otherwise typically
> be disruptive.
> 
> This adds support for parsing a configuration file to override defaults
> parameters to be used for mkfs.xfs.
> 
> We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for
> different types of configuration files, if none is specified we look for
> the default type, /etc/mkfs.xfs.d/default, and you can override with -T.
> For instance, if you specify:
> 
> 	mkfs.xfs -T experimental -f /dev/loop0

Ok, since you have to fix it all anyway, can we please use "-c" for config
instead of "-T" for type?  Because that's confusing w/ mkfs's '-t' for
the filesystem type, and mke2fs's -t for *cough* fs-type ... this isn't
a type (xfs,ext4,msdos,f2fs) and it's not an fs-type ala mke2fs
(PTSD from big/huge/largefile/largefile4 ... I really don't want
The Internet to start writing config files for a "webserver" type for
xfs.  Can we just call them 'configs'?)

> The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> file. If you really need to override the full path of the configuration
> file you may use the MKFS_XFS_CONFIG environment variable.
> 
> To use /etc/ be sure to configure xfsprogs with:
> 
>  ./configure --sysconfdir=/etc/

surely this should be made default?

> To verify what configuration file is used on a system use the typical:
> 
>   mkfs.xfs -N

Nitpick: -N by itself is not valid, it's treated as an error.
When it does get a config file, IMHO it should give its name, not just
"custom configuration file"

> There is only a subset of options allowed to be set on the configuration
> file, 

I think this is problematic - allowing only booleans is pretty
arbitrary.

> and currently only 1 or 0 are acceptable values. The default
> parameters you can override on a configuration file and their current
> built-in default settings are:
> 
> [data]
> noalign=0
> 
> [inode]
> align=1
> projid32bit=1
> sparse=0
> 
> [log]
> lazy-count=1
> 
> [metadata]
> crc=1
> finobt=1
> rmapbt=0
> reflink=0
> 
> [naming]
> ftype=1
> 
> [rtdev]
> noalign=0
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
> 
> We have floated around enough bike shedding emails to have at least reached
> a consensus on the fact that we'd be only supporting a mimimum set of default
> parameters and would strive to simplify our parser as much as possible.

See above - sorry for not chiming in sooner, but I don't think an arbitrary
restriction to the boolean options will be sufficient in the long run.

...
snip
...

> 
> diff --git a/include/builddefs.in b/include/builddefs.in
> index 7a2a62686717..231c96196ab9 100644
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -63,6 +63,7 @@ PKG_LIB_DIR	= @libdir@@libdirsuffix@
>  PKG_INC_DIR	= @includedir@/xfs
>  DK_INC_DIR	= @includedir@/disk
>  PKG_MAN_DIR	= @mandir@
> +PKG_ETC_DIR	= @sysconfdir@
>  PKG_DOC_DIR	= @datadir@/doc/@pkg_name@
>  PKG_LOCALE_DIR	= @datadir@/locale
>  
> @@ -194,6 +195,7 @@ endif
>  
>  GCFLAGS = $(DEBUG) \
>  	  -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\"  \
> +	  -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\"  \
>  	  -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs
>  
>  ifeq ($(ENABLE_GETTEXT),yes)
> diff --git a/man/man5/mkfs.xfs.d.5 b/man/man5/mkfs.xfs.d.5
> new file mode 100644
> index 000000000000..89fd28c79139
> --- /dev/null
> +++ b/man/man5/mkfs.xfs.d.5
> @@ -0,0 +1,121 @@
> +.TH mkfs.xfs.d 5
> +.SH NAME
> +mkfs.xfs.d \- mkfs.xfs configuration directory

IMHO this is all far too wordy, will put my editor hat on later.


<snip>

> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index 4b8c78c37806..18489b3a19ff 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -83,6 +83,23 @@ and
>  .B \-l internal \-l size=10m
>  are equivalent.
>  .PP
> +An optional XFS configuration type file directory
> +.B mkfs.xfs.d (5)
> +exists to help fine tune default parameters which can be used when calling
> +.B mkfs.xfs (8), by default type will be used by default, /etc/mkfs.xfs.d/default.

Optional mkfs.xfs configuration files may be stored in mkfs.xfs.d,
which may be used to override built-in mkfs.xfs default parameters.

> +Command line arguments directly passed to

drop "directly," commandline args are area always "directly" passed.

> +.B mkfs.xfs (8)
> +will always override parameters set it the configuration type file.

drop "type."

> +You can override configuration file type on the
> +.B mkfs.xfs.d (5)
> +directory by using the -t parameter and secifying the type.

A config file may be selected with the -c option as described below.

> Alternatively
> +you can set and use the MKFS_XFS_CONFIG environment variable to override
> +the default full path of the first file
> +.B mkfs.xfs (8)
> +looks for.

(How do you envision this env var being used?  How is this better than
simply allowing -t to specify a full path?)

> +If you use -t the type configuration file must be present under
> +.B mkfs.xfs.d (8).

Hm I'll try to wordsmith all this as well.

> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 1ca6a2d148c4..e66330a50b68 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -21,7 +21,11 @@
>  #include "xfs_multidisk.h"
>  #include "libxcmd.h"
>  
> -
> +#define MKFS_XFS_CONF_DIR	ROOT_SYSCONFDIR "/mkfs.xfs.d/"
> +#define CONFIG_MAX_KEY		1024
> +#define CONFIG_MAX_VALUE	PATH_MAX
> +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> +#define PARAM_OPTS		"T:b:d:i:l:L:m:n:KNp:qr:s:CfV"

what dave said ;)

also re: what dave said - functioning properly only if -t/-T/-c is given
first on the commandline is a little odd, though it makes some semantic
sense to specify a config file first then additional options so I can
live with it, I think.

But it really must then fail, as opposed to silently ignoring it,
if the option comes later.

<snip wow a whole lot of lines to parse the configfile>

Ok, so my configfile-containing-actual-commandlines was shot down,
but I'm a bit dismayed at the LOC required to re-parse the configfile,
especially when it's not even handling all options....

I'll have to take more time to review the bulk of the code, but the
behavioral issues above need to be addressed I think.

-Eric
--
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
Dave Chinner March 14, 2018, 3:55 a.m. UTC | #4
On Tue, Mar 13, 2018 at 11:52:27PM +0000, Luis R. Rodriguez wrote:
> On Wed, Mar 14, 2018 at 08:39:16AM +1100, Dave Chinner wrote:
> > Just one obvious comment from a brief glance - you changed this from
> > "-t" to "-T", but ....
> > > +directory by using the -t parameter and secifying the type. Alternatively
> > 
> > Not here, or ....
> >
> > > +If you use -t the type configuration file must be present under
> > 
> > here, or ....
> > 
> > > +#define MKFS_XFS_CONF_DIR	ROOT_SYSCONFDIR "/mkfs.xfs.d/"
> > > +#define CONFIG_MAX_KEY		1024
> > > +#define CONFIG_MAX_VALUE	PATH_MAX
> > > +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> > > +#define PARAM_OPTS		"T:b:d:i:l:L:m:n:KNp:qr:s:CfV"
> > 
> > [ Please don't obfuscate parsing options like this ]
> 
> The reason was we use them twice.

Which is not necessary. You don't need to change the option parsing
loop at all - the default config file can be parsed at any time as
a normal option. The default structure is not used for calculations
until after all the CLI options are parsed, so it can just be
treated as another CLI option in any location on the CLI.

> > I'll spend some more time looking at it, but my initial impression
> > is that there's a bit of work to be done yet...
> 
> You're right, we should decide if we want to allow for -T to be used
> only in the beginning or if we want to allow for it anywhere on the
> command line.

That's not what I meant. I meant the code needs a lot of work, not
that we needed to decide where on the CLI the config file option
should be placed.

Basically:

1. the default file parsing code needs to go into it's own file.
xfs_mkfs.c is too large and needs to be split into smaller files, so
lets not make it worse by shovelling another 500 lines of code into
it...

2. The default option should not share option table parsing with the
CLI options, because all that means is you add extra code to convert
config file sections to CLI sections before using the common parsing
code.

3. it needs to support more than just boolean options e.g. for
setting a default log size

4. it needs default values to be range checked for sanity. These
values can be different to the min/max values the can be specified
on CLI as defaults need to be more forgiving when combined with
other random options.

There will be other things when I look at the code, but those are
the first ones that come to mind....

Cheers,

Dave.
Luis Chamberlain March 14, 2018, 5:19 p.m. UTC | #5
On Wed, Mar 14, 2018 at 02:55:31PM +1100, Dave Chinner wrote:
> On Tue, Mar 13, 2018 at 11:52:27PM +0000, Luis R. Rodriguez wrote:
> > On Wed, Mar 14, 2018 at 08:39:16AM +1100, Dave Chinner wrote:
> > > Just one obvious comment from a brief glance - you changed this from
> > > "-t" to "-T", but ....
> > > > +directory by using the -t parameter and secifying the type. Alternatively
> > > 
> > > Not here, or ....
> > >
> > > > +If you use -t the type configuration file must be present under
> > > 
> > > here, or ....
> > > 
> > > > +#define MKFS_XFS_CONF_DIR	ROOT_SYSCONFDIR "/mkfs.xfs.d/"
> > > > +#define CONFIG_MAX_KEY		1024
> > > > +#define CONFIG_MAX_VALUE	PATH_MAX
> > > > +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> > > > +#define PARAM_OPTS		"T:b:d:i:l:L:m:n:KNp:qr:s:CfV"
> > > 
> > > [ Please don't obfuscate parsing options like this ]
> > 
> > The reason was we use them twice.
> 
> Which is not necessary. You don't need to change the option parsing
> loop at all - the default config file can be parsed at any time as
> a normal option. The default structure is not used for calculations
> until after all the CLI options are parsed, so it can just be
> treated as another CLI option in any location on the CLI.

Alright.

> > > I'll spend some more time looking at it, but my initial impression
> > > is that there's a bit of work to be done yet...
> > 
> > You're right, we should decide if we want to allow for -T to be used
> > only in the beginning or if we want to allow for it anywhere on the
> > command line.
> 
> That's not what I meant. I meant the code needs a lot of work, not
> that we needed to decide where on the CLI the config file option
> should be placed.
> 
> Basically:
> 
> 1. the default file parsing code needs to go into it's own file.
> xfs_mkfs.c is too large and needs to be split into smaller files, so
> lets not make it worse by shovelling another 500 lines of code into
> it...

Sure.

> 2. The default option should not share option table parsing with the
> CLI options, because all that means is you add extra code to convert
> config file sections to CLI sections before using the common parsing
> code.

Sure.

> 3. it needs to support more than just boolean options e.g. for
> setting a default log size

Log size is not part of struct mkfs_default_params, do we reaally
want to be able to set that via the configuration file?

sectorsize and blocksize are though.
Then struct mkfs_default_params has a struct sb_feat_args, of
which non-bools are (and their corresponding mapping):

	int log_version L_VERSION
	int attr_version I_ATTR
	int dir_version N_VERSION

So sure, I can add support for these, and sectorsize and blocksize.  It just
didn't seem worth it given most of the others were bool, so if we restrict the
parser to bool its much simpler.

BTW I see we have bool parent_pointers, and that maps to setting
XFS_SB_VERSION2_PARENTBIT, but we have no respective specific CLI
param, so of these:

[data]
[inode]
[log]
[metadata]
[naming]
[rtdev]

Where does parent_pointers map to? I'd only enable "parent_pointers=0|parent_pointers=1"
as its a bool.

> 4. it needs default values to be range checked for sanity. These
> values can be different to the min/max values the can be specified
> on CLI as defaults need to be more forgiving when combined with
> other random options.

Alright.

> There will be other things when I look at the code, but those are
> the first ones that come to mind....

I can address most of what you describe rather easily, the biggest
issue is ensuring that our goal here is only to modify struct
mkfs_default_params values, inclusive of what is in struct sb_feat_args.

Note that struct mkfs_default_params also has a struct fsxattr... do we
want to modify all those (this struct is defined per OS), for Linux we have:

struct fsxattr {                                                                
        __u32           fsx_xflags;     /* xflags field value (get/set) */      
        __u32           fsx_extsize;    /* extsize field value (get/set)*/      
        __u32           fsx_nextents;   /* nextents field value (get)   */      
        __u32           fsx_projid;     /* project identifier (get/set) */      
        __u32           fsx_cowextsize; /* cow extsize field value (get/set) */ 
        unsigned char   fsx_pad[8];                                             
};

I didn't enable parsing support for these, do we want to parse each of these
as well?

  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
Luis Chamberlain March 14, 2018, 6:41 p.m. UTC | #6
On Tue, Mar 13, 2018 at 11:21:48PM -0400, Eric Sandeen wrote:
> On 3/13/18 4:59 PM, Luis R. Rodriguez wrote:
> > You may want to stick to specific set of configuration options when
> > creating filesystems with mkfs.xfs -- sometimes due to pure technical
> > reasons, but some other times to ensure systems remain compatible as
> > new features are introduced with older kernels, or if you always want
> > to take advantage of some new feature which would otherwise typically
> > be disruptive.
> > 
> > This adds support for parsing a configuration file to override defaults
> > parameters to be used for mkfs.xfs.
> > 
> > We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for
> > different types of configuration files, if none is specified we look for
> > the default type, /etc/mkfs.xfs.d/default, and you can override with -T.
> > For instance, if you specify:
> > 
> > 	mkfs.xfs -T experimental -f /dev/loop0
> 
> Ok, since you have to fix it all anyway, can we please use "-c" for config
> instead of "-T" for type?

Sure. Its what I had originally used and I followed Darrick's bike-shed
email to use -t, and then -T after. I guess I'll revert back.

> Can we just call them 'configs'?)

My pleasure.

> > The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> > file. If you really need to override the full path of the configuration
> > file you may use the MKFS_XFS_CONFIG environment variable.
> > 
> > To use /etc/ be sure to configure xfsprogs with:
> > 
> >  ./configure --sysconfdir=/etc/
> 
> surely this should be made default?

As you wish.

> > To verify what configuration file is used on a system use the typical:
> > 
> >   mkfs.xfs -N
> 
> Nitpick: -N by itself is not valid, it's treated as an error.
> When it does get a config file, IMHO it should give its name, not just
> "custom configuration file"

Sure, will amend.

> > There is only a subset of options allowed to be set on the configuration
> > file, 
> 
> I think this is problematic - allowing only booleans is pretty
> arbitrary.

I was trying to simplify things considering how limited the number of
uints were on struct mkfs_default_params, and instead how most were just
bools. But sure, we can go ahead and parse the uints out of struct mkfs_default_params
as well.

> > We have floated around enough bike shedding emails to have at least reached
> > a consensus on the fact that we'd be only supporting a mimimum set of default
> > parameters and would strive to simplify our parser as much as possible.
> 
> See above - sorry for not chiming in sooner, but I don't think an arbitrary
> restriction to the boolean options will be sufficient in the long run.

Sure.

> > --- /dev/null
> > +++ b/man/man5/mkfs.xfs.d.5
> > @@ -0,0 +1,121 @@
> > +.TH mkfs.xfs.d 5
> > +.SH NAME
> > +mkfs.xfs.d \- mkfs.xfs configuration directory
> 
> IMHO this is all far too wordy, will put my editor hat on later.

Thanks, I appreciate the editorial feedback.

> > Alternatively
> > +you can set and use the MKFS_XFS_CONFIG environment variable to override
> > +the default full path of the first file
> > +.B mkfs.xfs (8)
> > +looks for.
> 
> (How do you envision this env var being used?  How is this better than
> simply allowing -t to specify a full path?)

-c can specify the full path if we want, its up to us if we want to allow
for that. Its simpler to support it, so fine by me to only accept that
and remove the environment variable.

> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 1ca6a2d148c4..e66330a50b68 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -21,7 +21,11 @@
> >  #include "xfs_multidisk.h"
> >  #include "libxcmd.h"
> >  
> > -
> > +#define MKFS_XFS_CONF_DIR	ROOT_SYSCONFDIR "/mkfs.xfs.d/"
> > +#define CONFIG_MAX_KEY		1024
> > +#define CONFIG_MAX_VALUE	PATH_MAX
> > +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> > +#define PARAM_OPTS		"T:b:d:i:l:L:m:n:KNp:qr:s:CfV"
> 
> what dave said ;)
> 
> also re: what dave said - functioning properly only if -t/-T/-c is given
> first on the commandline is a little odd, though it makes some semantic
> sense to specify a config file first then additional options so I can
> live with it, I think.
> 
> But it really must then fail, as opposed to silently ignoring it,
> if the option comes later.

I'll just enable -c to be parsed via the command line interface options
so it can be defined anywhere.

> <snip wow a whole lot of lines to parse the configfile>
> 
> Ok, so my configfile-containing-actual-commandlines was shot down,
> but I'm a bit dismayed at the LOC required to re-parse the configfile,
> especially when it's not even handling all options....

Adding uint shouldn't be too much extra code, it just means the callbacks
will be passing a uint and each option will have also implement a boundary
check. With the way Dave is suggesting this to be on its own file and set
of data structures or range checks, this shouldn't be too hard to do, its
just yet-more code.

> I'll have to take more time to review the bulk of the code, but the
> behavioral issues above need to be addressed I think.

Makes sense, thanks for the review.

  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
Dave Chinner March 14, 2018, 9:01 p.m. UTC | #7
On Wed, Mar 14, 2018 at 05:19:01PM +0000, Luis R. Rodriguez wrote:
> On Wed, Mar 14, 2018 at 02:55:31PM +1100, Dave Chinner wrote:
> > On Tue, Mar 13, 2018 at 11:52:27PM +0000, Luis R. Rodriguez wrote:
> > > On Wed, Mar 14, 2018 at 08:39:16AM +1100, Dave Chinner wrote:
> > > > Just one obvious comment from a brief glance - you changed this from
> > > > "-t" to "-T", but ....
> > > > > +directory by using the -t parameter and secifying the type. Alternatively
> > > > 
> > > > Not here, or ....
> > > >
> > > > > +If you use -t the type configuration file must be present under
> > > > 
> > > > here, or ....
> > > > 
> > > > > +#define MKFS_XFS_CONF_DIR	ROOT_SYSCONFDIR "/mkfs.xfs.d/"
> > > > > +#define CONFIG_MAX_KEY		1024
> > > > > +#define CONFIG_MAX_VALUE	PATH_MAX
> > > > > +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> > > > > +#define PARAM_OPTS		"T:b:d:i:l:L:m:n:KNp:qr:s:CfV"
> > > > 
> > > > [ Please don't obfuscate parsing options like this ]
> > > 
> > > The reason was we use them twice.
> > 
> > Which is not necessary. You don't need to change the option parsing
> > loop at all - the default config file can be parsed at any time as
> > a normal option. The default structure is not used for calculations
> > until after all the CLI options are parsed, so it can just be
> > treated as another CLI option in any location on the CLI.
> 
> Alright.
> 
> > > > I'll spend some more time looking at it, but my initial impression
> > > > is that there's a bit of work to be done yet...
> > > 
> > > You're right, we should decide if we want to allow for -T to be used
> > > only in the beginning or if we want to allow for it anywhere on the
> > > command line.
> > 
> > That's not what I meant. I meant the code needs a lot of work, not
> > that we needed to decide where on the CLI the config file option
> > should be placed.
> > 
> > Basically:
> > 
> > 1. the default file parsing code needs to go into it's own file.
> > xfs_mkfs.c is too large and needs to be split into smaller files, so
> > lets not make it worse by shovelling another 500 lines of code into
> > it...
> 
> Sure.
> 
> > 2. The default option should not share option table parsing with the
> > CLI options, because all that means is you add extra code to convert
> > config file sections to CLI sections before using the common parsing
> > code.
> 
> Sure.
> 
> > 3. it needs to support more than just boolean options e.g. for
> > setting a default log size
> 
> Log size is not part of struct mkfs_default_params, do we reaally
> want to be able to set that via the configuration file?

I think you're over-complicating/over-thinking this. setting a
default log size is an /example/, not something we /must do/. I
could have said "default directory blocksize", which is another
thing that is not currently in the defaults structure but is also
something we should also consider as a configurable default because
there are classes of users that want to use a different default
value.

> sectorsize and blocksize are though.
> Then struct mkfs_default_params has a struct sb_feat_args, of
> which non-bools are (and their corresponding mapping):
> 
> 	int log_version L_VERSION
> 	int attr_version I_ATTR
> 	int dir_version N_VERSION
> 
> So sure, I can add support for these, and sectorsize and blocksize.  It just
> didn't seem worth it given most of the others were bool, so if we restrict the
> parser to bool its much simpler.

Again, you're only looking at the current implementation to support
what it implements rather than implementing what we need to
arbitrary config file parameters.

e.g. Look at validate_dirblocksize() - it's default value is hard
coded into the function, rather than getting it from the defaults
structure.  Same goes for validate_inodesize(), and so on. Those are
going to need to be pulled up into the default value structure,
because they are things we're going to want to support changing
default values for.

Start by looking at all the types we parse on the CLI (bool, flag,
int, int64, unit suffices, etc) and implementing those, because they
are all going to be needed to set the value of some default
parameter....

> BTW I see we have bool parent_pointers, and that maps to setting
> XFS_SB_VERSION2_PARENTBIT, but we have no respective specific CLI
> param, so of these:

Again, you're looking at implementation, and asking questions about
a function that *isn't yet implemented* and so has no valid value
other than 0. When it's actually implemented, then we'll add it to
the CLI options and the default parsing into the appropriate
section. It's not the job of a "introduce config files for defaults"
patch to implement CLI/default parsing support for features that
aren't currently implemented.

> > There will be other things when I look at the code, but those are
> > the first ones that come to mind....
> 
> I can address most of what you describe rather easily, the biggest
> issue is ensuring that our goal here is only to modify struct
> mkfs_default_params values, inclusive of what is in struct sb_feat_args.
> 
> Note that struct mkfs_default_params also has a struct fsxattr... do we
> want to modify all those (this struct is defined per OS), for Linux we have:
> 
> struct fsxattr {                                                                
>         __u32           fsx_xflags;     /* xflags field value (get/set) */      
>         __u32           fsx_extsize;    /* extsize field value (get/set)*/      
>         __u32           fsx_nextents;   /* nextents field value (get)   */      
>         __u32           fsx_projid;     /* project identifier (get/set) */      
>         __u32           fsx_cowextsize; /* cow extsize field value (get/set) */ 
>         unsigned char   fsx_pad[8];                                             
> };
> 
> I didn't enable parsing support for these, do we want to parse each of these
> as well?

They are set by parsing existing CLI options, so if you enable those
options in the config file parsing, then this structure will need to
be updated, too.

Cheers,

Dave.
Luis Chamberlain March 14, 2018, 10:13 p.m. UTC | #8
On Wed, Mar 14, 2018 at 2:01 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Mar 14, 2018 at 05:19:01PM +0000, Luis R. Rodriguez wrote:
>> On Wed, Mar 14, 2018 at 02:55:31PM +1100, Dave Chinner wrote:
>> > 3. it needs to support more than just boolean options e.g. for
>> > setting a default log size
>>
>> Log size is not part of struct mkfs_default_params, do we reaally
>> want to be able to set that via the configuration file?
>
> I think you're over-complicating/over-thinking this. setting a
> default log size is an /example/, not something we /must do/. I
> could have said "default directory blocksize", which is another
> thing that is not currently in the defaults structure but is also
> something we should also consider as a configurable default because
> there are classes of users that want to use a different default
> value.

Just wanted to be sure.

>> sectorsize and blocksize are though.
>> Then struct mkfs_default_params has a struct sb_feat_args, of
>> which non-bools are (and their corresponding mapping):
>>
>>       int log_version L_VERSION
>>       int attr_version I_ATTR
>>       int dir_version N_VERSION
>>
>> So sure, I can add support for these, and sectorsize and blocksize.  It just
>> didn't seem worth it given most of the others were bool, so if we restrict the
>> parser to bool its much simpler.
>
> Again, you're only looking at the current implementation to support
> what it implements rather than implementing what we need to
> arbitrary config file parameters.

Sure, from my perspective *just* parsing the bools on struct
mkfs_default_params sufficed for my own needs to ensure I can drop in
a new xfsprogs on ancient systems. I understand what you mean now
though, in that the default structure may *very likely* grow later and
we may want to expand on it later. Got it.

> e.g. Look at validate_dirblocksize() - it's default value is hard
> coded into the function, rather than getting it from the defaults
> structure.

If nothing is set it depends if dirblocklog is set. And I guess if
nothing is set dirblocklog is set to XFS_MIN_REC_DIRSIZE, and
dirblocksize would be (1 << XFS_MIN_REC_DIRSIZE).

> Same goes for validate_inodesize(), and so on.

This default value will depends on if crcs_enabled which is already
part of the default struct. If we enable one to set this via the
default struct we'd just have to do similar sanity checks, which I'd
hope we can just share across both files then.

> Those are
> going to need to be pulled up into the default value structure,
> because they are things we're going to want to support changing
> default values for.

Alright...

> Start by looking at all the types we parse on the CLI (bool, flag,
> int, int64, unit suffices, etc) and implementing those, because they
> are all going to be needed to set the value of some default
> parameter....

Last I looked uint64 would suffice for all, modulo we had a few
"strings", an odd one was for N_VERSION where it may be a string with
"ci" in which case we'd set nci=true, otherwise we assume we're
updating the dir_version.

>> BTW I see we have bool parent_pointers, and that maps to setting
>> XFS_SB_VERSION2_PARENTBIT, but we have no respective specific CLI
>> param, so of these:
>
> Again, you're looking at implementation, and asking questions about
> a function that *isn't yet implemented* and so has no valid value
> other than 0. When it's actually implemented, then we'll add it to
> the CLI options and the default parsing into the appropriate
> section. It's not the job of a "introduce config files for defaults"
> patch to implement CLI/default parsing support for features that
> aren't currently implemented.

Great.

>> > There will be other things when I look at the code, but those are
>> > the first ones that come to mind....
>>
>> I can address most of what you describe rather easily, the biggest
>> issue is ensuring that our goal here is only to modify struct
>> mkfs_default_params values, inclusive of what is in struct sb_feat_args.
>>
>> Note that struct mkfs_default_params also has a struct fsxattr... do we
>> want to modify all those (this struct is defined per OS), for Linux we have:
>>
>> struct fsxattr {
>>         __u32           fsx_xflags;     /* xflags field value (get/set) */
>>         __u32           fsx_extsize;    /* extsize field value (get/set)*/
>>         __u32           fsx_nextents;   /* nextents field value (get)   */
>>         __u32           fsx_projid;     /* project identifier (get/set) */
>>         __u32           fsx_cowextsize; /* cow extsize field value (get/set) */
>>         unsigned char   fsx_pad[8];
>> };
>>
>> I didn't enable parsing support for these, do we want to parse each of these
>> as well?
>
> They are set by parsing existing CLI options, so if you enable those
> options in the config file parsing, then this structure will need to
> be updated, too.

I don't have a need for this to support old kernels. I'm perfectly
happy to support only what we have in the defaults structure for now.

 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
Luis Chamberlain April 26, 2018, 5:37 p.m. UTC | #9
On Thu, Mar 15, 2018 at 08:01:45AM +1100, Dave Chinner wrote:
> On Wed, Mar 14, 2018 at 05:19:01PM +0000, Luis R. Rodriguez wrote:
> > On Wed, Mar 14, 2018 at 02:55:31PM +1100, Dave Chinner wrote:
> > > On Tue, Mar 13, 2018 at 11:52:27PM +0000, Luis R. Rodriguez wrote:
> > > > On Wed, Mar 14, 2018 at 08:39:16AM +1100, Dave Chinner wrote:
> > > 
> > > Basically:
> > > 
> > > 1. the default file parsing code needs to go into it's own file.
> > > xfs_mkfs.c is too large and needs to be split into smaller files, so
> > > lets not make it worse by shovelling another 500 lines of code into
> > > it...
> > 
> > Sure.

Other than using its own file you had suggested to have the config stuff have
its own structure. At LSFMM you had also suggested it should have its own
defaults which may change later, however for now they can be the same as the
CLI defaults. I suspect likewise applies for maxval, minval.

What about the conflict mapping / checkers? Don't we want to share that? If so
it would make sense to share the enums used to describe the individual subopts
as well. If so I'll shove the enums into its own file. The conflict stuff
is already in its own structure, so if we want to *share* that, I could just
move all that into its own set of files and share that.

  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
Luis Chamberlain May 3, 2018, midnight UTC | #10
On Thu, Apr 26, 2018 at 05:37:39PM +0000, Luis R. Rodriguez wrote:
> On Thu, Mar 15, 2018 at 08:01:45AM +1100, Dave Chinner wrote:
> > On Wed, Mar 14, 2018 at 05:19:01PM +0000, Luis R. Rodriguez wrote:
> > > On Wed, Mar 14, 2018 at 02:55:31PM +1100, Dave Chinner wrote:
> > > > On Tue, Mar 13, 2018 at 11:52:27PM +0000, Luis R. Rodriguez wrote:
> > > > > On Wed, Mar 14, 2018 at 08:39:16AM +1100, Dave Chinner wrote:
> > > > 
> > > > Basically:
> > > > 
> > > > 1. the default file parsing code needs to go into it's own file.
> > > > xfs_mkfs.c is too large and needs to be split into smaller files, so
> > > > lets not make it worse by shovelling another 500 lines of code into
> > > > it...
> > > 
> > > Sure.
> 
> Other than using its own file you had suggested to have the config stuff have
> its own structure. At LSFMM you had also suggested it should have its own
> defaults which may change later, however for now they can be the same as the
> CLI defaults. I suspect likewise applies for maxval, minval.
> 
> What about the conflict mapping / checkers? Don't we want to share that? If so
> it would make sense to share the enums used to describe the individual subopts
> as well. If so I'll shove the enums into its own file. The conflict stuff
> is already in its own structure, so if we want to *share* that, I could just
> move all that into its own set of files and share that.

Re-poke.

  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
Eric Sandeen May 4, 2018, 9:31 p.m. UTC | #11
On 3/13/18 10:21 PM, Eric Sandeen wrote:
>> There is only a subset of options allowed to be set on the configuration
>> file, 

> I think this is problematic - allowing only booleans is pretty
> arbitrary.

...
 
>> and currently only 1 or 0 are acceptable values. The default
>> parameters you can override on a configuration file and their current
>> built-in default settings are:
>>
>> [data]
>> noalign=0

...

>> We have floated around enough bike shedding emails to have at least reached
>> a consensus on the fact that we'd be only supporting a mimimum set of default
>> parameters and would strive to simplify our parser as much as possible.

> See above - sorry for not chiming in sooner, but I don't think an arbitrary
> restriction to the boolean options will be sufficient in the long run.

Ok, I'm rethinking this concern now.

It seems like we may have two separate but related purposes for a mkfs config file:

1) Set system-wide defaults based on kernel and/or OS support.
2) Set specific-use defaults, such as "mkfs.xfs -c gluster" etc.

For the former, it may well only be the booleans that matter.  For the latter,
it may be more, like setting inode size, or directory block size, etc.

Is there anything we'd reasonably want to set system-wide that's /not/ a
boolean?  If not, the maybe that can be Phase 1 as long as the config file
format doesn't need to change; we can just add to the list of supported config
options at a later date, perhaps?

The system-wide, supported-features default config may be the most pressing
need at this point, the rest is just nice to have.

Thanks,
-Eric
--
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
Darrick J. Wong May 4, 2018, 9:36 p.m. UTC | #12
On Fri, May 04, 2018 at 04:31:56PM -0500, Eric Sandeen wrote:
> On 3/13/18 10:21 PM, Eric Sandeen wrote:
> >> There is only a subset of options allowed to be set on the configuration
> >> file, 
> 
> > I think this is problematic - allowing only booleans is pretty
> > arbitrary.
> 
> ...
>  
> >> and currently only 1 or 0 are acceptable values. The default
> >> parameters you can override on a configuration file and their current
> >> built-in default settings are:
> >>
> >> [data]
> >> noalign=0
> 
> ...
> 
> >> We have floated around enough bike shedding emails to have at least reached
> >> a consensus on the fact that we'd be only supporting a mimimum set of default
> >> parameters and would strive to simplify our parser as much as possible.
> 
> > See above - sorry for not chiming in sooner, but I don't think an arbitrary
> > restriction to the boolean options will be sufficient in the long run.
> 
> Ok, I'm rethinking this concern now.
> 
> It seems like we may have two separate but related purposes for a mkfs config file:
> 
> 1) Set system-wide defaults based on kernel and/or OS support.
> 2) Set specific-use defaults, such as "mkfs.xfs -c gluster" etc.
> 
> For the former, it may well only be the booleans that matter.  For the latter,
> it may be more, like setting inode size, or directory block size, etc.
> 
> Is there anything we'd reasonably want to set system-wide that's /not/ a
> boolean?  If not, the maybe that can be Phase 1 as long as the config file
> format doesn't need to change; we can just add to the list of supported config
> options at a later date, perhaps?

Looking at the *documented* mkfs options, I think it'd be useful for
cloud users to be able to set cowextsize and extsize from mkfs.

As I was saying on irc, I /think/ the options (Allison feel free to
chime in here) that we want are....

-m crc,finobt,rmapbt,reflink,projinherit,extszinherit,cowextsize,extsize
-i maxpct,sparse
-n ftype

--D

> The system-wide, supported-features default config may be the most pressing
> need at this point, the rest is just nice to have.
> 
> Thanks,
> -Eric
> --
> 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
--
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
Eric Sandeen May 4, 2018, 9:39 p.m. UTC | #13
On 5/4/18 4:36 PM, Darrick J. Wong wrote:
> On Fri, May 04, 2018 at 04:31:56PM -0500, Eric Sandeen wrote:
>> On 3/13/18 10:21 PM, Eric Sandeen wrote:
>>>> There is only a subset of options allowed to be set on the configuration
>>>> file, 
>>
>>> I think this is problematic - allowing only booleans is pretty
>>> arbitrary.
>>
>> ...
>>  
>>>> and currently only 1 or 0 are acceptable values. The default
>>>> parameters you can override on a configuration file and their current
>>>> built-in default settings are:
>>>>
>>>> [data]
>>>> noalign=0
>>
>> ...
>>
>>>> We have floated around enough bike shedding emails to have at least reached
>>>> a consensus on the fact that we'd be only supporting a mimimum set of default
>>>> parameters and would strive to simplify our parser as much as possible.
>>
>>> See above - sorry for not chiming in sooner, but I don't think an arbitrary
>>> restriction to the boolean options will be sufficient in the long run.
>>
>> Ok, I'm rethinking this concern now.
>>
>> It seems like we may have two separate but related purposes for a mkfs config file:
>>
>> 1) Set system-wide defaults based on kernel and/or OS support.
>> 2) Set specific-use defaults, such as "mkfs.xfs -c gluster" etc.
>>
>> For the former, it may well only be the booleans that matter.  For the latter,
>> it may be more, like setting inode size, or directory block size, etc.
>>
>> Is there anything we'd reasonably want to set system-wide that's /not/ a
>> boolean?  If not, the maybe that can be Phase 1 as long as the config file
>> format doesn't need to change; we can just add to the list of supported config
>> options at a later date, perhaps?
> 
> Looking at the *documented* mkfs options, I think it'd be useful for
> cloud users to be able to set cowextsize and extsize from mkfs.

Fair, but I guess my point is that that falls more into special-use
config optimization, vs. supported/not-supported defaults.

And as such could maybe come in Phase 2 of the grand plan.  ;)

-Eric

> As I was saying on irc, I /think/ the options (Allison feel free to
> chime in here) that we want are....
> 
> -m crc,finobt,rmapbt,reflink,projinherit,extszinherit,cowextsize,extsize
> -i maxpct,sparse
> -n ftype
> 
> --D


--
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
Luis Chamberlain May 11, 2018, 10:20 p.m. UTC | #14
On Thu, May 03, 2018 at 12:00:15AM +0000, Luis R. Rodriguez wrote:
> On Thu, Apr 26, 2018 at 05:37:39PM +0000, Luis R. Rodriguez wrote:
> > On Thu, Mar 15, 2018 at 08:01:45AM +1100, Dave Chinner wrote:
> > > On Wed, Mar 14, 2018 at 05:19:01PM +0000, Luis R. Rodriguez wrote:
> > > > On Wed, Mar 14, 2018 at 02:55:31PM +1100, Dave Chinner wrote:
> > > > > On Tue, Mar 13, 2018 at 11:52:27PM +0000, Luis R. Rodriguez wrote:
> > > > > > On Wed, Mar 14, 2018 at 08:39:16AM +1100, Dave Chinner wrote:
> > > > > 
> > > > > Basically:
> > > > > 
> > > > > 1. the default file parsing code needs to go into it's own file.
> > > > > xfs_mkfs.c is too large and needs to be split into smaller files, so
> > > > > lets not make it worse by shovelling another 500 lines of code into
> > > > > it...
> > > > 
> > > > Sure.
> > 
> > Other than using its own file you had suggested to have the config stuff have
> > its own structure. At LSFMM you had also suggested it should have its own
> > defaults which may change later, however for now they can be the same as the
> > CLI defaults. I suspect likewise applies for maxval, minval.
> > 
> > What about the conflict mapping / checkers? Don't we want to share that? If so
> > it would make sense to share the enums used to describe the individual subopts
> > as well. If so I'll shove the enums into its own file. The conflict stuff
> > is already in its own structure, so if we want to *share* that, I could just
> > move all that into its own set of files and share that.
> 
> Re-poke.

Alright... I'm just gonna do what I think is right :)

  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
Luis Chamberlain May 17, 2018, 6:47 p.m. UTC | #15
On Fri, May 04, 2018 at 02:36:46PM -0700, Darrick J. Wong wrote:
> On Fri, May 04, 2018 at 04:31:56PM -0500, Eric Sandeen wrote:
> > On 3/13/18 10:21 PM, Eric Sandeen wrote:
> > >> There is only a subset of options allowed to be set on the configuration
> > >> file, 
> > 
> > > I think this is problematic - allowing only booleans is pretty
> > > arbitrary.
> > 
> > ...
> >  
> > >> and currently only 1 or 0 are acceptable values. The default
> > >> parameters you can override on a configuration file and their current
> > >> built-in default settings are:
> > >>
> > >> [data]
> > >> noalign=0
> > 
> > ...
> > 
> > >> We have floated around enough bike shedding emails to have at least reached
> > >> a consensus on the fact that we'd be only supporting a mimimum set of default
> > >> parameters and would strive to simplify our parser as much as possible.
> > 
> > > See above - sorry for not chiming in sooner, but I don't think an arbitrary
> > > restriction to the boolean options will be sufficient in the long run.
> > 
> > Ok, I'm rethinking this concern now.
> > 
> > It seems like we may have two separate but related purposes for a mkfs config file:
> > 
> > 1) Set system-wide defaults based on kernel and/or OS support.
> > 2) Set specific-use defaults, such as "mkfs.xfs -c gluster" etc.
> > 
> > For the former, it may well only be the booleans that matter.  For the latter,
> > it may be more, like setting inode size, or directory block size, etc.
> > 
> > Is there anything we'd reasonably want to set system-wide that's /not/ a
> > boolean?  If not, the maybe that can be Phase 1 as long as the config file
> > format doesn't need to change; we can just add to the list of supported config
> > options at a later date, perhaps?
> 
> Looking at the *documented* mkfs options, I think it'd be useful for
> cloud users to be able to set cowextsize and extsize from mkfs.
> 
> As I was saying on irc, I /think/ the options (Allison feel free to
> chime in here) that we want are....
> 
> -m crc,finobt,rmapbt,reflink,projinherit,extszinherit,cowextsize,extsize
> -i maxpct,sparse
> -n ftype

Of these, the following would need respective defaults added:

-m extszinherit,cowextsize,extsize
-i maxpct,

Once defaults are added, the respective configuration entry can be added. The
rest would be supported from the start, and from what I have seen all that
would go in would enable support for use on older kernels.

  Luis


> 
> --D
> 
> > The system-wide, supported-features default config may be the most pressing
> > need at this point, the rest is just nice to have.
> > 
> > Thanks,
> > -Eric
> > --
> > 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
>

Patch
diff mbox

diff --git a/include/builddefs.in b/include/builddefs.in
index 7a2a62686717..231c96196ab9 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -63,6 +63,7 @@  PKG_LIB_DIR	= @libdir@@libdirsuffix@
 PKG_INC_DIR	= @includedir@/xfs
 DK_INC_DIR	= @includedir@/disk
 PKG_MAN_DIR	= @mandir@
+PKG_ETC_DIR	= @sysconfdir@
 PKG_DOC_DIR	= @datadir@/doc/@pkg_name@
 PKG_LOCALE_DIR	= @datadir@/locale
 
@@ -194,6 +195,7 @@  endif
 
 GCFLAGS = $(DEBUG) \
 	  -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\"  \
+	  -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\"  \
 	  -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs
 
 ifeq ($(ENABLE_GETTEXT),yes)
diff --git a/man/man5/mkfs.xfs.d.5 b/man/man5/mkfs.xfs.d.5
new file mode 100644
index 000000000000..89fd28c79139
--- /dev/null
+++ b/man/man5/mkfs.xfs.d.5
@@ -0,0 +1,121 @@ 
+.TH mkfs.xfs.d 5
+.SH NAME
+mkfs.xfs.d \- mkfs.xfs configuration directory
+.SH DESCRIPTION
+.B mkfs.xfs (8)
+uses a set of initial default parameters for configuration. These defaults
+are conservatively decided by the community as xfsprogs and features for XFS
+in the kernel advance. One can override these default on the
+.B mkfs.xfs (8)
+command line, but there are cases where it is desirable for sensible defaults
+to always be specified by the system where
+.B mkfs.xfs (8)
+runs on. This may desirable for example on systems with old kernels where the
+built-in default parameters on
+.B mkfs.xfs (8)
+may not be able to create a filesystem which the old kernel supports and it
+would be unclear what parameters are needed to produce a compatible filesystem.
+Overriding
+.B mkfs.xfs (8)
+built-in defaults may also be desirable if you have a series of systems with
+different kernels and want to be able to create filesystems which all systems
+are able to support properly.
+.PP
+The XFS configuration directory
+.B mkfs.xfs.d (5)
+can be used to define different configuration file types which can be used to
+override the built-in default parameters by
+.B mkfs.xfs (8).
+Different configuration file types are supported, the default
+configuration file type,
+.I /etc/mkfs.xfs.d/default
+, will be looked for first and if present will be used to override
+.B mkfs.xfs (8)
+built-in default parameters. You can override the configuration file type by
+specifying the type when using
+.B mkfs.xfs (8)
+by using the
+.B -T
+parameter. For example:
+.I mkfs.xfs -T experimental -f /dev/sda1
+will make
+.B mkfs.xfs (8)
+look for and use the configuration file type
+.I /etc/mkfs.xfs.d/experimental
+to override
+.B mkfs.xfs (8)
+default parameters. If you need to override the full path for a configuration
+file type you can use the
+.I MKFS_XFS_CONFIG
+environment variable prior to calling
+.B mkfs.xfs (8)
+to define the
+full path to the configuration file to be used. If you used the
+.B -T
+parameter or if you set the
+.I MKFS_XFS_CONFIG
+environment variable the configuration file must be present and should parse
+correctly.
+.PP
+Parameters passed to to the
+.B mkfs.xfs (8)
+command line always override any defaults set on the configuration file used.
+.PP
+.B mkfs.xfs (8)
+will always describe what configuration file was used, if any
+was used at all. To verify which configuration file would be used prior to
+execution of
+.B mkfs.xfs (8)
+you can use
+.I mkfs.xfs -N.
+.PP
+.SH DEFAULT PARAMETERS
+Default parameters for
+.B mkfs.xfs (8)
+consists of a small subset of the parameters one can set with on the command
+line. Default parameters can only be either enabled or disabled, you can set
+their value to 1 to enable or 0 to disable. Below we list the different
+supported default parameters which can be defined on configuration files, along
+with the current built-in setting.
+.PP
+.BI [data]
+.br
+.BI noalign=0
+.PP
+.BI [inode]
+.br
+.BI align=1
+.br
+.BI projid32bit=1
+.br
+.BI sparse=0
+.PP
+.BI [log]
+.br
+.BI lazy-count=1
+.PP
+.BI [metadata]
+.br
+.BI crc=1
+.br
+.BI finobt=1
+.br
+.BI rmapbt=0
+.br
+.BI reflink=0
+.PP
+.BI [naming]
+.br
+.BI ftype=1
+.PP
+.BI [rtdev]
+.br
+.BI noalign=0
+.PP
+.SH SEE ALSO
+.BR mkfs.xfs (8),
+.BR xfsctl (3),
+.BR xfs_info (8),
+.BR xfs_admin (8),
+.BR xfsdump (8),
+.BR xfsrestore (8).
diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 4b8c78c37806..18489b3a19ff 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -83,6 +83,23 @@  and
 .B \-l internal \-l size=10m
 are equivalent.
 .PP
+An optional XFS configuration type file directory
+.B mkfs.xfs.d (5)
+exists to help fine tune default parameters which can be used when calling
+.B mkfs.xfs (8), by default type will be used by default, /etc/mkfs.xfs.d/default.
+Command line arguments directly passed to
+.B mkfs.xfs (8)
+will always override parameters set it the configuration type file.
+You can override configuration file type on the
+.B mkfs.xfs.d (5)
+directory by using the -t parameter and secifying the type. Alternatively
+you can set and use the MKFS_XFS_CONFIG environment variable to override
+the default full path of the first file
+.B mkfs.xfs (8)
+looks for.
+If you use -t the type configuration file must be present under
+.B mkfs.xfs.d (8).
+.PP
 In the descriptions below, sizes are given in sectors, bytes, blocks,
 kilobytes, megabytes, gigabytes, etc.
 Sizes are treated as hexadecimal if prefixed by 0x or 0X,
@@ -123,6 +140,11 @@  Many feature options allow an optional argument of 0 or 1, to explicitly
 disable or enable the functionality.
 .SH OPTIONS
 .TP
+.BI \-t " configuration-type"
+Override the default type of the configuratio file under
+.B mkfs.xfs.d
+used.
+.TP
 .BI \-b " block_size_options"
 This option specifies the fundamental block size of the filesystem.
 The valid
@@ -923,6 +945,7 @@  Prints the version number and exits.
 .SH SEE ALSO
 .BR xfs (5),
 .BR mkfs (8),
+.BR mkfs.xfs.d (5),
 .BR mount (8),
 .BR xfs_info (8),
 .BR xfs_admin (8).
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 1ca6a2d148c4..e66330a50b68 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -21,7 +21,11 @@ 
 #include "xfs_multidisk.h"
 #include "libxcmd.h"
 
-
+#define MKFS_XFS_CONF_DIR	ROOT_SYSCONFDIR "/mkfs.xfs.d/"
+#define CONFIG_MAX_KEY		1024
+#define CONFIG_MAX_VALUE	PATH_MAX
+#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
+#define PARAM_OPTS		"T:b:d:i:l:L:m:n:KNp:qr:s:CfV"
 
 #define TERABYTES(count, blog)	((uint64_t)(count) << (40 - (blog)))
 #define GIGABYTES(count, blog)	((uint64_t)(count) << (30 - (blog)))
@@ -839,6 +843,43 @@  struct mkfs_params {
 	struct sb_feat_args	sb_feat;
 };
 
+/*
+ * File configuration type settings
+ *
+ * These are the different possibilities by which you can end up parsing
+ * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
+ * file parsed and we are using the built-in defaults on this code.
+ * DEFAULTS_CONFIG means the default configuration file was found and used.
+ * DEFAULTS_TYPE_CONFIG means the user asked for a custom configuration type
+ * and it was used, this means the default directory for mkfs.xfs
+ * configuration files was used to look for the type specified. If you need
+ * to override the default mkfs.xfs directory for configuration file you can
+ * use the MKFS_XFS_CONFIG environment variable to set the absolute path to
+ * your custom configuration file, when this is set the type is set to
+ * DEFAULTS_ENVIRONMENT_CONFIG.
+ */
+enum default_params_type {
+	DEFAULTS_BUILTIN = 0,
+	DEFAULTS_CONFIG,
+	DEFAULTS_ENVIRONMENT_CONFIG,
+	DEFAULTS_TYPE_CONFIG,
+};
+
+static const char *default_type_str(enum default_params_type type)
+{
+	switch (type) {
+	case DEFAULTS_BUILTIN:
+		return _("package build definitions");
+	case DEFAULTS_CONFIG:
+		return _("default configuration system file");
+	case DEFAULTS_ENVIRONMENT_CONFIG:
+		return _("custom configuration file set by environment");
+	case DEFAULTS_TYPE_CONFIG:
+		return _("custom configuration type file");
+	}
+	return _("Unkown\n");
+}
+
 /*
  * Default filesystem features and configuration values
  *
@@ -848,7 +889,8 @@  struct mkfs_params {
  * calculations.
  */
 struct mkfs_default_params {
-	char	*source;	/* where the defaults came from */
+	enum default_params_type type; /* where the defaults came from */
+	const char *config_file;
 
 	int	sectorsize;
 	int	blocksize;
@@ -1414,6 +1456,12 @@  block_opts_parser(
 	return 0;
 }
 
+static int block_config_parser(struct mkfs_default_params *dft, int subopt,
+			       bool value)
+{
+	return -EINVAL;
+}
+
 static int
 data_opts_parser(
 	struct opt_params	*opts,
@@ -1477,6 +1525,19 @@  data_opts_parser(
 	return 0;
 }
 
+static int data_config_parser(struct mkfs_default_params *dft, int subopt,
+			      bool value)
+{
+	switch (subopt) {
+	case D_NOALIGN:
+		dft->sb_feat.nodalign = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int
 inode_opts_parser(
 	struct opt_params	*opts,
@@ -1512,6 +1573,25 @@  inode_opts_parser(
 	return 0;
 }
 
+static int inode_config_parser(struct mkfs_default_params *dft, int subopt,
+			       bool value)
+{
+	switch (subopt) {
+	case I_ALIGN:
+		dft->sb_feat.inode_align = value;
+		break;
+	case I_PROJID32BIT:
+		dft->sb_feat.projid32bit = value;
+		break;
+	case I_SPINODES:
+		dft->sb_feat.spinodes = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int
 log_opts_parser(
 	struct opt_params	*opts,
@@ -1558,6 +1638,19 @@  log_opts_parser(
 	return 0;
 }
 
+static int log_config_parser(struct mkfs_default_params *dft, int subopt,
+			     bool value)
+{
+	switch (subopt) {
+	case L_LAZYSBCNTR:
+		dft->sb_feat.lazy_sb_counters = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int
 meta_opts_parser(
 	struct opt_params	*opts,
@@ -1592,6 +1685,30 @@  meta_opts_parser(
 	return 0;
 }
 
+static int meta_config_parser(struct mkfs_default_params *dft, int subopt,
+			      bool value)
+{
+	switch (subopt) {
+	case M_CRC:
+		dft->sb_feat.crcs_enabled = value;
+		if (dft->sb_feat.crcs_enabled)
+			dft->sb_feat.dirftype = true;
+		break;
+	case M_FINOBT:
+		dft->sb_feat.finobt = value;
+		break;
+	case M_RMAPBT:
+		dft->sb_feat.rmapbt = value;
+		break;
+	case M_REFLINK:
+		dft->sb_feat.reflink = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int
 naming_opts_parser(
 	struct opt_params	*opts,
@@ -1621,6 +1738,19 @@  naming_opts_parser(
 	return 0;
 }
 
+static int naming_config_parser(struct mkfs_default_params *dft, int subopt,
+				bool value)
+{
+	switch (subopt) {
+	case N_FTYPE:
+		dft->sb_feat.dirftype = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int
 rtdev_opts_parser(
 	struct opt_params	*opts,
@@ -1651,6 +1781,19 @@  rtdev_opts_parser(
 	return 0;
 }
 
+static int rtdev_config_parser(struct mkfs_default_params *dft, int subopt,
+			       bool value)
+{
+	switch (subopt) {
+	case R_NOALIGN:
+		dft->sb_feat.nortalign = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int
 sector_opts_parser(
 	struct opt_params	*opts,
@@ -1670,19 +1813,27 @@  sector_opts_parser(
 	return 0;
 }
 
+static int sector_config_parser(struct mkfs_default_params *dft, int subopt,
+				bool value)
+{
+	return -EINVAL;
+}
+
 struct subopts {
 	char		opt;
 	struct opt_params *opts;
 	int		(*parser)();
+	int		(*config_parser)(struct mkfs_default_params *dft,
+					 int subop, bool value);
 } subopt_tab[] = {
-	{ 'b', &bopts, block_opts_parser },
-	{ 'd', &dopts, data_opts_parser },
-	{ 'i', &iopts, inode_opts_parser },
-	{ 'l', &lopts, log_opts_parser },
-	{ 'm', &mopts, meta_opts_parser },
-	{ 'n', &nopts, naming_opts_parser },
-	{ 'r', &ropts, rtdev_opts_parser },
-	{ 's', &sopts, sector_opts_parser },
+	{ 'b', &bopts, block_opts_parser, block_config_parser },
+	{ 'd', &dopts, data_opts_parser, data_config_parser },
+	{ 'i', &iopts, inode_opts_parser, inode_config_parser },
+	{ 'l', &lopts, log_opts_parser, log_config_parser },
+	{ 'm', &mopts, meta_opts_parser, meta_config_parser },
+	{ 'n', &nopts, naming_opts_parser, naming_config_parser },
+	{ 'r', &ropts, rtdev_opts_parser, rtdev_config_parser },
+	{ 's', &sopts, sector_opts_parser, sector_config_parser },
 	{ '\0', NULL, NULL },
 };
 
@@ -1720,6 +1871,219 @@  parse_subopts(
 	}
 }
 
+static int
+parse_config_subopts(
+	char		opt,
+	bool            value,
+	char 		*line,
+	struct mkfs_default_params *dft)
+{
+	struct subopts	*sop = &subopt_tab[0];
+	char	**subopts = (char **)sop->opts->subopts;
+	int	subopt;
+	char *val;
+
+	while (sop->opts) {
+		if (sop->opt == opt)
+			break;
+		sop++;
+	}
+
+	/* should never happen */
+	if (!sop->opts)
+		return -EINVAL;
+
+	subopts = (char **)sop->opts->subopts;
+	subopt = getsubopt(&line, subopts, &val);
+	return (sop->config_parser)(dft, subopt, value);
+}
+
+static int get_opt_type(const char *buffer)
+{
+	if (strncmp("block", buffer, 5) == 0)
+		return 'b';
+	if (strncmp("data", buffer, 4) == 0)
+		return 'd';
+	if (strncmp("inode", buffer, 5) == 0)
+		return 'i';
+	if (strncmp("log", buffer, 3) == 0)
+		return 'l';
+	if (strncmp("metadata", buffer, 8) == 0)
+		return 'm';
+	if (strncmp("naming", buffer, 6) == 0)
+		return 'n';
+	if (strncmp("rtdev", buffer, 5) == 0)
+		return 'r';
+	if (strncmp("sector", buffer, 6) == 0)
+		return 'n';
+	return 0;
+}
+
+static int mkfs_check_config_file_limits(const char *filename,
+					 const struct stat *sb,
+					 unsigned int max_entries)
+{
+	unsigned long long size_limit;
+
+	size_limit = CONFIG_MAX_BUFFER * max_entries;
+
+	/*
+	 * Detect wrap around -- if max_entries * size_limit goves over
+	 * unsigned long long we detect that there. Also libiniconfig only
+	 * groks max INT_MAX entries anyway.
+	 */
+	if (size_limit < max_entries || max_entries > INT_MAX)
+		return -E2BIG;
+
+	if (sb->st_size > size_limit) {
+		fprintf(stderr,
+			"File %s is too big! File size limit: %llu\n",
+			filename, size_limit);
+		return -E2BIG;
+	}
+
+	return 0;
+}
+
+enum parse_line_type {
+	PARSE_COMMENT = 0,
+	PARSE_SECTION,
+	PARSE_TAG_VALUE,
+	PARSE_INVALID,
+	PARSE_EOF,
+};
+
+static int parse_line_section(const char *line, char *tag)
+{
+	return sscanf(line, " [%[^]]s]", tag);
+}
+
+static int parse_line_tag_value(const char *line, char *tag,
+				unsigned int *value)
+{
+	return sscanf(line, " %[^ \t=]"
+		      " = "
+		      "%u ",
+		      tag, value);
+}
+
+static enum parse_line_type parse_get_line_type(const char *line, char *tag,
+						bool *value)
+{
+	int ret;
+	unsigned int uint_value;
+
+	memset(tag, 0, 80);
+
+	if (strlen(line) < 2)
+		return PARSE_INVALID;
+
+	if (line[0] == '#')
+		return PARSE_COMMENT;
+
+	ret = parse_line_section(line, tag);
+	if (ret == 1)
+		return  PARSE_SECTION;
+
+	if (ret == EOF)
+		return PARSE_EOF;
+
+	ret = parse_line_tag_value(line, tag, &uint_value);
+	if (ret == 2) {
+		if (uint_value != 1 && uint_value != 0)
+			return -EINVAL;
+
+		*value = !!uint_value;
+
+		return PARSE_TAG_VALUE;
+	}
+
+	if (ret == EOF)
+		return PARSE_EOF;
+
+	return PARSE_INVALID;
+}
+
+static int parse_config_init(struct mkfs_default_params *dft)
+{
+	int ret;
+	FILE *fp;
+	struct stat sp;
+	unsigned int num_subopts_sections = sizeof(subopt_tab) /
+					    sizeof(struct subopts) - 1;
+	char *p;
+	char line[80], tag[80];
+	bool value;
+	enum parse_line_type parse_type;
+	int opt = 0;
+
+	fp = fopen(dft->config_file, "r");
+	if (!fp) {
+		if (dft->type != DEFAULTS_BUILTIN) {
+			fprintf(stderr, _("could not open config file: %s\n"),
+				dft->config_file);
+			ret = -ENOENT;
+		} else
+			ret = 0;
+		return ret;
+	}
+
+	/* If we found a file magically, it would be the default file */
+	if (dft->type == DEFAULTS_BUILTIN)
+		dft->type = DEFAULTS_CONFIG;
+
+	ret = stat(dft->config_file, &sp);
+	if (ret) {
+		if (dft->type != DEFAULTS_BUILTIN)
+			fprintf(stderr, _("could not stat() config file: %s: %s\n"),
+				dft->config_file, strerror(ret));
+		return ret;
+	}
+
+	if (!sp.st_size)
+		return 0;
+
+	ret = mkfs_check_config_file_limits(dft->config_file, &sp,
+					    num_subopts_sections);
+	if (ret)
+		return ret;
+
+	while (!feof(fp)) {
+		p = fgets(line, sizeof(line), fp);
+		if (!p)
+			continue;
+
+		parse_type = parse_get_line_type(line, tag, &value);
+
+		switch (parse_type) {
+		case PARSE_COMMENT:
+		case PARSE_INVALID:
+		case PARSE_EOF:
+			break;
+		case PARSE_SECTION:
+			opt = get_opt_type(tag);
+			if (!opt) {
+				fprintf(stderr, _("Invalid section: %s\n"),
+						tag);
+				return -EINVAL;
+			}
+			break;
+		case PARSE_TAG_VALUE:
+			/* Trims white spaces */
+			snprintf(line, sizeof(line), "%s=%u", tag, value);
+			ret = parse_config_subopts(opt, value, line, dft);
+			if (ret) {
+				fprintf(stderr, _("Error parsine line: %s\n"),
+						line);
+				return ret;
+			}
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static void
 validate_sectorsize(
 	struct mkfs_params	*cfg,
@@ -3177,11 +3541,13 @@  print_mkfs_cfg(
 	struct mkfs_params	*cfg,
 	char			*dfile,
 	char			*logfile,
-	char			*rtfile)
+	char			*rtfile,
+	struct mkfs_default_params *dft)
 {
 	struct sb_feat_args	*fp = &cfg->sb_feat;
 
 	printf(_(
+"config-file=%-22s\n"
 "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
 "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
 "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
@@ -3191,6 +3557,7 @@  print_mkfs_cfg(
 "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
 "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
 "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
+		dft->type != DEFAULTS_BUILTIN ? dft->config_file : "empty",
 		dfile, cfg->inodesize, (long long)cfg->agcount,
 			(long long)cfg->agsize,
 		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
@@ -3797,7 +4164,8 @@  main(
 
 	/* build time defaults */
 	struct mkfs_default_params	dft = {
-		.source = _("package build definitions"),
+		.type = DEFAULTS_BUILTIN,
+		.config_file = MKFS_XFS_CONF_DIR "default",
 		.sectorsize = XFS_MIN_SECTORSIZE,
 		.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
 		.sb_feat = {
@@ -3819,6 +4187,9 @@  main(
 			.nortalign = false,
 		},
 	};
+	char *tmp_config;
+	char custom_config[PATH_MAX];
+	int ret;
 
 	platform_uuid_generate(&cli.uuid);
 	progname = basename(argv[0]);
@@ -3827,25 +4198,47 @@  main(
 	textdomain(PACKAGE);
 
 	/*
-	 * TODO: Sourcing defaults from a config file
-	 *
 	 * Before anything else, see if there's a config file with different
-	 * defaults. If a file exists in <package location>, read in the new
+	 * defaults. If a file exists in MKFS_XFS_CONF_DIR/default, read the new
 	 * default values and overwrite them in the &dft structure. This way the
 	 * new defaults will apply before we parse the CLI, and the CLI will
 	 * still be able to override them. When more than one source is
 	 * implemented, emit a message to indicate where the defaults being
 	 * used came from.
-	 *
-	 * printf(_("Default configuration sourced from %s\n"), dft.source);
 	 */
+	tmp_config = getenv("MKFS_XFS_CONFIG");
+	if (tmp_config != NULL) {
+		dft.config_file = tmp_config;
+		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
+	}
+
+	c = getopt(argc, argv, PARAM_OPTS);
+	if (c != EOF && c == 't') {
+		memset(custom_config, 0, sizeof(custom_config));
+		snprintf(custom_config, sizeof(custom_config), "%s%s",
+			 MKFS_XFS_CONF_DIR, optarg);
+		dft.config_file = custom_config;
+		dft.type = DEFAULTS_TYPE_CONFIG;
+	}
+
+	ret = parse_config_init(&dft);
+
+	printf(_("Default configuration sourced from %s\n"),
+	       default_type_str(dft.type));
+
+	if (ret && dft.type != DEFAULTS_BUILTIN)
+		usage();
 
 	/* copy new defaults into CLI parsing structure */
 	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:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
+	while (true) {
+		if (c == EOF)
+			break;
 		switch (c) {
+		case 'T':
+			break;
 		case 'C':
 		case 'f':
 			force_overwrite = 1;
@@ -3885,6 +4278,7 @@  main(
 		case '?':
 			unknown(optopt, "");
 		}
+		c = getopt(argc, argv, PARAM_OPTS);
 	}
 	if (argc - optind > 1) {
 		fprintf(stderr, _("extra arguments\n"));
@@ -3968,7 +4362,7 @@  main(
 	calculate_log_size(&cfg, &cli, mp);
 
 	if (!quiet || dry_run) {
-		print_mkfs_cfg(&cfg, dfile, logfile, rtfile);
+		print_mkfs_cfg(&cfg, dfile, logfile, rtfile, &dft);
 		if (dry_run)
 			exit(0);
 	}