[PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags

[PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags

[Eric Sandeen]

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,
 		},
 	},
 };

Re: [PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags

[Darrick J. Wong]

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,
>  		},
>  	},
>  };
> 

Re: [PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags

[Jan Tulak]

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

Re: [PATCH] mkfs.xfs: Assign proper defaults to rmapbt and reflink flags

[Dave Chinner]

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