* [PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags
@ 2017-04-26 14:02 Eric Sandeen
2017-04-26 15:10 ` Darrick J. Wong
0 siblings, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2017-04-26 14:02 UTC (permalink / raw)
To: linux-xfs; +Cc: Darrick J. Wong, Jan Tulak
The "defaultval" field in the options structure was a bit confusing,
so when the rmapbt & reflink options got added, the desire was
to keep them off by default, and "defaultval = 0" got set.
However, the purpose of this field is to define the default value
when the flag is specified with no associated value, i.e.
-m rmapbt vs. -m rmapbt=0 or -m rmapbt=1
Today, the resulting behavior is unexpected, and different from any
other mkfs flags; specifying "-m rmapbt,reflink" results in a
filesystem /without/ those features.
Fix these to be consistent with every other boolean flag in the
mkfs options, so that specifying the flag with no value will
enable the feature.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
I want to get this fixed up for 4.11, so just sending it separately
ahead of any other change Jan may make in this area.
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index affa405..10858e4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -697,13 +697,13 @@ struct opt_params mopts = {
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 0,
+ .defaultval = 1,
},
{ .index = M_REFLINK,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 0,
+ .defaultval = 1,
},
},
};
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags
2017-04-26 14:02 [PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags Eric Sandeen
@ 2017-04-26 15:10 ` Darrick J. Wong
2017-04-26 15:16 ` Jan Tulak
2017-04-26 22:50 ` Dave Chinner
0 siblings, 2 replies; 4+ messages in thread
From: Darrick J. Wong @ 2017-04-26 15:10 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs, Jan Tulak
On Wed, Apr 26, 2017 at 09:02:44AM -0500, Eric Sandeen wrote:
> The "defaultval" field in the options structure was a bit confusing,
> so when the rmapbt & reflink options got added, the desire was
> to keep them off by default, and "defaultval = 0" got set.
>
> However, the purpose of this field is to define the default value
> when the flag is specified with no associated value, i.e.
>
> -m rmapbt vs. -m rmapbt=0 or -m rmapbt=1
>
> Today, the resulting behavior is unexpected, and different from any
> other mkfs flags; specifying "-m rmapbt,reflink" results in a
> filesystem /without/ those features.
>
> Fix these to be consistent with every other boolean flag in the
> mkfs options, so that specifying the flag with no value will
> enable the feature.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Someone should document that 'defaultval' is not the way to say
'disabled by default' in whatever the mkfs option processing code
turns into.
(Granted maybe it does and I just can't read :P)
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
>
> I want to get this fixed up for 4.11, so just sending it separately
> ahead of any other change Jan may make in this area.
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index affa405..10858e4 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -697,13 +697,13 @@ struct opt_params mopts = {
> .conflicts = { LAST_CONFLICT },
> .minval = 0,
> .maxval = 1,
> - .defaultval = 0,
> + .defaultval = 1,
> },
> { .index = M_REFLINK,
> .conflicts = { LAST_CONFLICT },
> .minval = 0,
> .maxval = 1,
> - .defaultval = 0,
> + .defaultval = 1,
> },
> },
> };
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags
2017-04-26 15:10 ` Darrick J. Wong
@ 2017-04-26 15:16 ` Jan Tulak
2017-04-26 22:50 ` Dave Chinner
1 sibling, 0 replies; 4+ messages in thread
From: Jan Tulak @ 2017-04-26 15:16 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Eric Sandeen, linux-xfs
On Wed, Apr 26, 2017 at 5:10 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Wed, Apr 26, 2017 at 09:02:44AM -0500, Eric Sandeen wrote:
>> The "defaultval" field in the options structure was a bit confusing,
>> so when the rmapbt & reflink options got added, the desire was
>> to keep them off by default, and "defaultval = 0" got set.
>>
>> However, the purpose of this field is to define the default value
>> when the flag is specified with no associated value, i.e.
>>
>> -m rmapbt vs. -m rmapbt=0 or -m rmapbt=1
>>
>> Today, the resulting behavior is unexpected, and different from any
>> other mkfs flags; specifying "-m rmapbt,reflink" results in a
>> filesystem /without/ those features.
>>
>> Fix these to be consistent with every other boolean flag in the
>> mkfs options, so that specifying the flag with no value will
>> enable the feature.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>
> Someone should document that 'defaultval' is not the way to say
> 'disabled by default' in whatever the mkfs option processing code
> turns into.
>
> (Granted maybe it does and I just can't read :P)
The current documentation is confusing about this too, which is my
fault. I already have a patch in my current set that renames it to
flagval and makes it clear what it does. And maybe I will remove it
altogether in exchange for some way how to explicitly say "This is a
flag, it has only 0 and 1. Thus, no value means 'enable it.'" in
another patchset.
Cheers,
Jan
>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> --D
>
>> ---
>>
>> I want to get this fixed up for 4.11, so just sending it separately
>> ahead of any other change Jan may make in this area.
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index affa405..10858e4 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -697,13 +697,13 @@ struct opt_params mopts = {
>> .conflicts = { LAST_CONFLICT },
>> .minval = 0,
>> .maxval = 1,
>> - .defaultval = 0,
>> + .defaultval = 1,
>> },
>> { .index = M_REFLINK,
>> .conflicts = { LAST_CONFLICT },
>> .minval = 0,
>> .maxval = 1,
>> - .defaultval = 0,
>> + .defaultval = 1,
>> },
>> },
>> };
>>
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags
2017-04-26 15:10 ` Darrick J. Wong
2017-04-26 15:16 ` Jan Tulak
@ 2017-04-26 22:50 ` Dave Chinner
1 sibling, 0 replies; 4+ messages in thread
From: Dave Chinner @ 2017-04-26 22:50 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Eric Sandeen, linux-xfs, Jan Tulak
On Wed, Apr 26, 2017 at 08:10:59AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 26, 2017 at 09:02:44AM -0500, Eric Sandeen wrote:
> > The "defaultval" field in the options structure was a bit confusing,
> > so when the rmapbt & reflink options got added, the desire was
> > to keep them off by default, and "defaultval = 0" got set.
> >
> > However, the purpose of this field is to define the default value
> > when the flag is specified with no associated value, i.e.
> >
> > -m rmapbt vs. -m rmapbt=0 or -m rmapbt=1
> >
> > Today, the resulting behavior is unexpected, and different from any
> > other mkfs flags; specifying "-m rmapbt,reflink" results in a
> > filesystem /without/ those features.
> >
> > Fix these to be consistent with every other boolean flag in the
> > mkfs options, so that specifying the flag with no value will
> > enable the feature.
> >
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>
> Someone should document that 'defaultval' is not the way to say
> 'disabled by default' in whatever the mkfs option processing code
> turns into.
....
* defaultval MANDATORY
* The value used if user specifies the subopt, but no value.
* If the subopt accepts some values (-d file=[1|0]), then this
* sets what is used with simple specifying the subopt (-d file).
* A special SUBOPT_NEEDS_VAL can be used to require a user-given
* value in any case.
*/
....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-26 22:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-26 14:02 [PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags Eric Sandeen
2017-04-26 15:10 ` Darrick J. Wong
2017-04-26 15:16 ` Jan Tulak
2017-04-26 22:50 ` Dave Chinner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.