diff mbox series

[2/2] mkfs: allow setting dax flag on root directory

Message ID 159716415037.2135493.12958426655000840394.stgit@magnolia
State Superseded
Headers show
Series xfsprogs: various small enhancements | expand

Commit Message

Darrick J. Wong Aug. 11, 2020, 4:42 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Teach mkfs to set the DAX flag on the root directory so that all new
files can be created in dax mode.  This is a complement to removing the
mount option.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man8/mkfs.xfs.8 |   11 +++++++++++
 mkfs/xfs_mkfs.c     |   14 ++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Eric Sandeen Aug. 11, 2020, 7:39 p.m. UTC | #1
On 8/11/20 9:42 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach mkfs to set the DAX flag on the root directory so that all new
> files can be created in dax mode.  This is a complement to removing the
> mount option.

So, a new -d option, "-d dax"

This is ~analogous to cowextsize, rtinherit, projinherit, and extszinherit
so there is certainly precedence for this.  (where only rtinherit is a boolean
like this, but they are all inheritable behaviors)

(I wonder if "daxinherit" would be more consistent, but won't bikeshed
that (much))

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  man/man8/mkfs.xfs.8 |   11 +++++++++++
>  mkfs/xfs_mkfs.c     |   14 ++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> 
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index 9d762a43011a..4b4fdd86b2f4 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -394,6 +394,17 @@ All inodes created by
>  will have this extent size hint applied.
>  The value must be provided in units of filesystem blocks.
>  Directories will pass on this hint to newly created children.
> +.TP
> +.BI dax= value
> +All inodes created by
> +.B mkfs.xfs
> +will have the DAX flag set.
> +This means that directories will pass the flag on to newly created files

let's call this "children" to match the other similar options?

(because technically it is passed on not only to regular files, right?)

> +and files will use the DAX IO paths when possible.
> +This value is either 1 to enable the use or 0 to disable.
> +By default,
> +.B mkfs.xfs
> +will not enable DAX mode.
>  .RE
>  .TP
>  .B \-f
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 2e6cd280e388..33507f6ea21c 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -60,6 +60,7 @@ enum {
>  	D_PROJINHERIT,
>  	D_EXTSZINHERIT,
>  	D_COWEXTSIZE,
> +	D_DAX,
>  	D_MAX_OPTS,
>  };
>  
> @@ -254,6 +255,7 @@ static struct opt_params dopts = {
>  		[D_PROJINHERIT] = "projinherit",
>  		[D_EXTSZINHERIT] = "extszinherit",
>  		[D_COWEXTSIZE] = "cowextsize",
> +		[D_DAX] = "dax",
>  	},
>  	.subopt_params = {
>  		{ .index = D_AGCOUNT,
> @@ -369,6 +371,12 @@ static struct opt_params dopts = {
>  		  .maxval = UINT_MAX,
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
> +		{ .index = D_DAX,
> +		  .conflicts = { { NULL, LAST_CONFLICT } },

er....  should we conflict with reflink ....  ?

> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 1,

Hm, interesting that this is a little different from rtinherit:

                { .index = D_RTINHERIT,
                  .conflicts = { { NULL, LAST_CONFLICT } },
                  .minval = 1,
                  .maxval = 1,
                  .defaultval = 1,
                },

I think this means that:

-d rtinherit
-d rtinherit=1

are valid, but

-d rtinherit=0 is not, but

-d dax
-d dax=1
-d dax=0

are all valid?

While the latter makes a bit more sense, I wonder if we should stay consistent w/ the rtinherit semantics.  Or do you envision some sort of automatic enabling of this based on device typethat we'd need to override in the future?

> +		},
>  	},
>  };
>  
> @@ -1434,6 +1442,12 @@ data_opts_parser(
>  		cli->fsx.fsx_cowextsize = getnum(value, opts, subopt);
>  		cli->fsx.fsx_xflags |= FS_XFLAG_COWEXTSIZE;
>  		break;
> +	case D_DAX:
> +		if (getnum(value, opts, subopt))
> +			cli->fsx.fsx_xflags |= FS_XFLAG_DAX;
> +		else
> +			cli->fsx.fsx_xflags &= ~FS_XFLAG_DAX;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
>
Darrick J. Wong Aug. 11, 2020, 7:54 p.m. UTC | #2
On Tue, Aug 11, 2020 at 02:39:01PM -0500, Eric Sandeen wrote:
> On 8/11/20 9:42 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Teach mkfs to set the DAX flag on the root directory so that all new
> > files can be created in dax mode.  This is a complement to removing the
> > mount option.
> 
> So, a new -d option, "-d dax"
> 
> This is ~analogous to cowextsize, rtinherit, projinherit, and extszinherit
> so there is certainly precedence for this.  (where only rtinherit is a boolean
> like this, but they are all inheritable behaviors)
> 
> (I wonder if "daxinherit" would be more consistent, but won't bikeshed
> that (much))

/me is indifferent either way.  But I guess some day we might want to
have a dax= flag to indicate something like "set the data device
geometry to optimize for DAX?

Nah, I think if we were ever going to do that, we'd have something more
like:

	-d usage=dax
	-d usage=ssd
	-d usage=floopy

Meh.  I'll change it to daxinherit, since that /is/ what it does.

> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  man/man8/mkfs.xfs.8 |   11 +++++++++++
> >  mkfs/xfs_mkfs.c     |   14 ++++++++++++++
> >  2 files changed, 25 insertions(+)
> > 
> > 
> > diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> > index 9d762a43011a..4b4fdd86b2f4 100644
> > --- a/man/man8/mkfs.xfs.8
> > +++ b/man/man8/mkfs.xfs.8
> > @@ -394,6 +394,17 @@ All inodes created by
> >  will have this extent size hint applied.
> >  The value must be provided in units of filesystem blocks.
> >  Directories will pass on this hint to newly created children.
> > +.TP
> > +.BI dax= value
> > +All inodes created by
> > +.B mkfs.xfs
> > +will have the DAX flag set.
> > +This means that directories will pass the flag on to newly created files
> 
> let's call this "children" to match the other similar options?
> 
> (because technically it is passed on not only to regular files, right?)

Directories and regular files, though not to other special files.
Maybe we should fix that.

> > +and files will use the DAX IO paths when possible.
> > +This value is either 1 to enable the use or 0 to disable.
> > +By default,
> > +.B mkfs.xfs
> > +will not enable DAX mode.
> >  .RE
> >  .TP
> >  .B \-f
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 2e6cd280e388..33507f6ea21c 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -60,6 +60,7 @@ enum {
> >  	D_PROJINHERIT,
> >  	D_EXTSZINHERIT,
> >  	D_COWEXTSIZE,
> > +	D_DAX,
> >  	D_MAX_OPTS,
> >  };
> >  
> > @@ -254,6 +255,7 @@ static struct opt_params dopts = {
> >  		[D_PROJINHERIT] = "projinherit",
> >  		[D_EXTSZINHERIT] = "extszinherit",
> >  		[D_COWEXTSIZE] = "cowextsize",
> > +		[D_DAX] = "dax",
> >  	},
> >  	.subopt_params = {
> >  		{ .index = D_AGCOUNT,
> > @@ -369,6 +371,12 @@ static struct opt_params dopts = {
> >  		  .maxval = UINT_MAX,
> >  		  .defaultval = SUBOPT_NEEDS_VAL,
> >  		},
> > +		{ .index = D_DAX,
> > +		  .conflicts = { { NULL, LAST_CONFLICT } },
> 
> er....  should we conflict with reflink ....  ?
> 
> > +		  .minval = 0,
> > +		  .maxval = 1,
> > +		  .defaultval = 1,
> 
> Hm, interesting that this is a little different from rtinherit:
> 
>                 { .index = D_RTINHERIT,
>                   .conflicts = { { NULL, LAST_CONFLICT } },
>                   .minval = 1,
>                   .maxval = 1,
>                   .defaultval = 1,
>                 },
> 
> I think this means that:
> 
> -d rtinherit
> -d rtinherit=1
> 
> are valid, but
> 
> -d rtinherit=0 is not, but
> 
> -d dax
> -d dax=1
> -d dax=0
> 
> are all valid?

TBH, I find it a little odd that you *can't* say "-d rtinherit=0" from a
completeness perspective, but...

> While the latter makes a bit more sense, I wonder if we should stay
> consistent w/ the rtinherit semantics.  Or do you envision some sort
> of automatic enabling of this based on device typethat we'd need to
> override in the future?

...the goal is to set this automatically once distros start shipping a
libblkid that has blkid_topology_get_dax().  At that point we'll
probably want a way to force it off.

Unless we want the ability to specify -ddax=0 the magic seekrit hook to
discover if (future) mkfs actually supports dax autodetection?  Hmm,
that alone sounds like sufficient justification.  Ok.

--D

> 
> > +		},
> >  	},
> >  };
> >  
> > @@ -1434,6 +1442,12 @@ data_opts_parser(
> >  		cli->fsx.fsx_cowextsize = getnum(value, opts, subopt);
> >  		cli->fsx.fsx_xflags |= FS_XFLAG_COWEXTSIZE;
> >  		break;
> > +	case D_DAX:
> > +		if (getnum(value, opts, subopt))
> > +			cli->fsx.fsx_xflags |= FS_XFLAG_DAX;
> > +		else
> > +			cli->fsx.fsx_xflags &= ~FS_XFLAG_DAX;
> > +		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> >
Eric Sandeen Aug. 11, 2020, 8:02 p.m. UTC | #3
On 8/11/20 12:54 PM, Darrick J. Wong wrote:
> On Tue, Aug 11, 2020 at 02:39:01PM -0500, Eric Sandeen wrote:
>> On 8/11/20 9:42 AM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Teach mkfs to set the DAX flag on the root directory so that all new
>>> files can be created in dax mode.  This is a complement to removing the
>>> mount option.
>>
>> So, a new -d option, "-d dax"
>>
>> This is ~analogous to cowextsize, rtinherit, projinherit, and extszinherit
>> so there is certainly precedence for this.  (where only rtinherit is a boolean
>> like this, but they are all inheritable behaviors)
>>
>> (I wonder if "daxinherit" would be more consistent, but won't bikeshed
>> that (much))
> 
> /me is indifferent either way.  But I guess some day we might want to
> have a dax= flag to indicate something like "set the data device
> geometry to optimize for DAX?
> 
> Nah, I think if we were ever going to do that, we'd have something more
> like:
> 
> 	-d usage=dax
> 	-d usage=ssd
> 	-d usage=floopy
> 
> Meh.  I'll change it to daxinherit, since that /is/ what it does.

Ok.  I'm really pretty indifferent as well.

>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>> ---
>>>  man/man8/mkfs.xfs.8 |   11 +++++++++++
>>>  mkfs/xfs_mkfs.c     |   14 ++++++++++++++
>>>  2 files changed, 25 insertions(+)
>>>
>>>
>>> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
>>> index 9d762a43011a..4b4fdd86b2f4 100644
>>> --- a/man/man8/mkfs.xfs.8
>>> +++ b/man/man8/mkfs.xfs.8
>>> @@ -394,6 +394,17 @@ All inodes created by
>>>  will have this extent size hint applied.
>>>  The value must be provided in units of filesystem blocks.
>>>  Directories will pass on this hint to newly created children.
>>> +.TP
>>> +.BI dax= value
>>> +All inodes created by
>>> +.B mkfs.xfs
>>> +will have the DAX flag set.
>>> +This means that directories will pass the flag on to newly created files
>>
>> let's call this "children" to match the other similar options?
>>
>> (because technically it is passed on not only to regular files, right?)
> 
> Directories and regular files, though not to other special files.
> Maybe we should fix that.

Ok so not all children.  But also not just files.  :P

"... pass the flag on to newly created files and directories, so that new
files will use the DAX IO paths when possible." ?

>>> +and files will use the DAX IO paths when possible.
>>> +This value is either 1 to enable the use or 0 to disable.

...

>>> @@ -369,6 +371,12 @@ static struct opt_params dopts = {
>>>  		  .maxval = UINT_MAX,
>>>  		  .defaultval = SUBOPT_NEEDS_VAL,
>>>  		},
>>> +		{ .index = D_DAX,
>>> +		  .conflicts = { { NULL, LAST_CONFLICT } },
>>
>> er....  should we conflict with reflink ....  ?

Thoughts? :)

>>> +		  .minval = 0,
>>> +		  .maxval = 1,
>>> +		  .defaultval = 1,
>>
>> Hm, interesting that this is a little different from rtinherit:
>>
>>                 { .index = D_RTINHERIT,
>>                   .conflicts = { { NULL, LAST_CONFLICT } },
>>                   .minval = 1,
>>                   .maxval = 1,
>>                   .defaultval = 1,
>>                 },
>>
>> I think this means that:
>>
>> -d rtinherit
>> -d rtinherit=1
>>
>> are valid, but
>>
>> -d rtinherit=0 is not, but
>>
>> -d dax
>> -d dax=1
>> -d dax=0
>>
>> are all valid?
> 
> TBH, I find it a little odd that you *can't* say "-d rtinherit=0" from a
> completeness perspective, but...

We could probably loosen it up and start allowing zero here too.
It wouldn't break any old scripts, right.

>> While the latter makes a bit more sense, I wonder if we should stay
>> consistent w/ the rtinherit semantics.  Or do you envision some sort
>> of automatic enabling of this based on device typethat we'd need to
>> override in the future?
> 
> ...the goal is to set this automatically once distros start shipping a
> libblkid that has blkid_topology_get_dax().  At that point we'll
> probably want a way to force it off.

*nod*

> Unless we want the ability to specify -ddax=0 the magic seekrit hook to
> discover if (future) mkfs actually supports dax autodetection?  Hmm,
> that alone sounds like sufficient justification.  Ok.

Not sure I followed that... :)

-Eric
Darrick J. Wong Aug. 11, 2020, 8:18 p.m. UTC | #4
On Tue, Aug 11, 2020 at 03:02:07PM -0500, Eric Sandeen wrote:
> On 8/11/20 12:54 PM, Darrick J. Wong wrote:
> > On Tue, Aug 11, 2020 at 02:39:01PM -0500, Eric Sandeen wrote:
> >> On 8/11/20 9:42 AM, Darrick J. Wong wrote:
> >>> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>>
> >>> Teach mkfs to set the DAX flag on the root directory so that all new
> >>> files can be created in dax mode.  This is a complement to removing the
> >>> mount option.
> >>
> >> So, a new -d option, "-d dax"
> >>
> >> This is ~analogous to cowextsize, rtinherit, projinherit, and extszinherit
> >> so there is certainly precedence for this.  (where only rtinherit is a boolean
> >> like this, but they are all inheritable behaviors)
> >>
> >> (I wonder if "daxinherit" would be more consistent, but won't bikeshed
> >> that (much))
> > 
> > /me is indifferent either way.  But I guess some day we might want to
> > have a dax= flag to indicate something like "set the data device
> > geometry to optimize for DAX?
> > 
> > Nah, I think if we were ever going to do that, we'd have something more
> > like:
> > 
> > 	-d usage=dax
> > 	-d usage=ssd
> > 	-d usage=floopy
> > 
> > Meh.  I'll change it to daxinherit, since that /is/ what it does.
> 
> Ok.  I'm really pretty indifferent as well.
> 
> >>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >>> ---
> >>>  man/man8/mkfs.xfs.8 |   11 +++++++++++
> >>>  mkfs/xfs_mkfs.c     |   14 ++++++++++++++
> >>>  2 files changed, 25 insertions(+)
> >>>
> >>>
> >>> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> >>> index 9d762a43011a..4b4fdd86b2f4 100644
> >>> --- a/man/man8/mkfs.xfs.8
> >>> +++ b/man/man8/mkfs.xfs.8
> >>> @@ -394,6 +394,17 @@ All inodes created by
> >>>  will have this extent size hint applied.
> >>>  The value must be provided in units of filesystem blocks.
> >>>  Directories will pass on this hint to newly created children.
> >>> +.TP
> >>> +.BI dax= value
> >>> +All inodes created by
> >>> +.B mkfs.xfs
> >>> +will have the DAX flag set.
> >>> +This means that directories will pass the flag on to newly created files
> >>
> >> let's call this "children" to match the other similar options?
> >>
> >> (because technically it is passed on not only to regular files, right?)
> > 
> > Directories and regular files, though not to other special files.
> > Maybe we should fix that.
> 
> Ok so not all children.  But also not just files.  :P
> 
> "... pass the flag on to newly created files and directories, so that new
> files will use the DAX IO paths when possible." ?
> 
> >>> +and files will use the DAX IO paths when possible.
> >>> +This value is either 1 to enable the use or 0 to disable.
> 
> ...
> 
> >>> @@ -369,6 +371,12 @@ static struct opt_params dopts = {
> >>>  		  .maxval = UINT_MAX,
> >>>  		  .defaultval = SUBOPT_NEEDS_VAL,
> >>>  		},
> >>> +		{ .index = D_DAX,
> >>> +		  .conflicts = { { NULL, LAST_CONFLICT } },
> >>
> >> er....  should we conflict with reflink ....  ?
> 
> Thoughts? :)

That only prevents the user from specifying a reflink cli option; it
doesn't prevent them from turning on daxinherit when reflink already
defaults to enabled.

If you want to prevent people from formatting with the two options, you
have to do it in validate_sb_features.  Hm, maybe we should at least
warn about that.

> >>> +		  .minval = 0,
> >>> +		  .maxval = 1,
> >>> +		  .defaultval = 1,
> >>
> >> Hm, interesting that this is a little different from rtinherit:
> >>
> >>                 { .index = D_RTINHERIT,
> >>                   .conflicts = { { NULL, LAST_CONFLICT } },
> >>                   .minval = 1,
> >>                   .maxval = 1,
> >>                   .defaultval = 1,
> >>                 },
> >>
> >> I think this means that:
> >>
> >> -d rtinherit
> >> -d rtinherit=1
> >>
> >> are valid, but
> >>
> >> -d rtinherit=0 is not, but
> >>
> >> -d dax
> >> -d dax=1
> >> -d dax=0
> >>
> >> are all valid?
> > 
> > TBH, I find it a little odd that you *can't* say "-d rtinherit=0" from a
> > completeness perspective, but...
> 
> We could probably loosen it up and start allowing zero here too.
> It wouldn't break any old scripts, right.

Right.

> >> While the latter makes a bit more sense, I wonder if we should stay
> >> consistent w/ the rtinherit semantics.  Or do you envision some sort
> >> of automatic enabling of this based on device typethat we'd need to
> >> override in the future?
> > 
> > ...the goal is to set this automatically once distros start shipping a
> > libblkid that has blkid_topology_get_dax().  At that point we'll
> > probably want a way to force it off.
> 
> *nod*
> 
> > Unless we want the ability to specify -ddax=0 the magic seekrit hook to
> > discover if (future) mkfs actually supports dax autodetection?  Hmm,
> > that alone sounds like sufficient justification.  Ok.
> 
> Not sure I followed that... :)

Never mind, I talked myself out of it since script authors aren't going
to be happy with an option that only sometimes works.

--D

> -Eric
>
diff mbox series

Patch

diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 9d762a43011a..4b4fdd86b2f4 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -394,6 +394,17 @@  All inodes created by
 will have this extent size hint applied.
 The value must be provided in units of filesystem blocks.
 Directories will pass on this hint to newly created children.
+.TP
+.BI dax= value
+All inodes created by
+.B mkfs.xfs
+will have the DAX flag set.
+This means that directories will pass the flag on to newly created files
+and files will use the DAX IO paths when possible.
+This value is either 1 to enable the use or 0 to disable.
+By default,
+.B mkfs.xfs
+will not enable DAX mode.
 .RE
 .TP
 .B \-f
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2e6cd280e388..33507f6ea21c 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -60,6 +60,7 @@  enum {
 	D_PROJINHERIT,
 	D_EXTSZINHERIT,
 	D_COWEXTSIZE,
+	D_DAX,
 	D_MAX_OPTS,
 };
 
@@ -254,6 +255,7 @@  static struct opt_params dopts = {
 		[D_PROJINHERIT] = "projinherit",
 		[D_EXTSZINHERIT] = "extszinherit",
 		[D_COWEXTSIZE] = "cowextsize",
+		[D_DAX] = "dax",
 	},
 	.subopt_params = {
 		{ .index = D_AGCOUNT,
@@ -369,6 +371,12 @@  static struct opt_params dopts = {
 		  .maxval = UINT_MAX,
 		  .defaultval = SUBOPT_NEEDS_VAL,
 		},
+		{ .index = D_DAX,
+		  .conflicts = { { NULL, LAST_CONFLICT } },
+		  .minval = 0,
+		  .maxval = 1,
+		  .defaultval = 1,
+		},
 	},
 };
 
@@ -1434,6 +1442,12 @@  data_opts_parser(
 		cli->fsx.fsx_cowextsize = getnum(value, opts, subopt);
 		cli->fsx.fsx_xflags |= FS_XFLAG_COWEXTSIZE;
 		break;
+	case D_DAX:
+		if (getnum(value, opts, subopt))
+			cli->fsx.fsx_xflags |= FS_XFLAG_DAX;
+		else
+			cli->fsx.fsx_xflags &= ~FS_XFLAG_DAX;
+		break;
 	default:
 		return -EINVAL;
 	}