[PATCH] mkfs: rtinherit minval should be 0

[PATCH] mkfs: rtinherit minval should be 0

[Jan Tulak]

As with any other option, rtinherit=[0|1], but minval was incorrectly
set to 1, so it was not possible to disable this option.

Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
 mkfs/xfs_mkfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 6fedc05c..5aac4d1b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -290,7 +290,7 @@ struct opt_params dopts = {
 		},
 		{ .index = D_RTINHERIT,
 		  .conflicts = { LAST_CONFLICT },
-		  .minval = 1,
+		  .minval = 0,
 		  .maxval = 1,
 		  .defaultval = 1,
 		},
-- 
2.12.1

Re: [PATCH] mkfs: rtinherit minval should be 0

[Darrick J. Wong]

On Thu, Apr 06, 2017 at 04:34:26PM +0200, Jan Tulak wrote:
> As with any other option, rtinherit=[0|1], but minval was incorrectly
> set to 1, so it was not possible to disable this option.
> 
> Signed-off-by: Jan Tulak <jtulak@redhat.com>

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  mkfs/xfs_mkfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 6fedc05c..5aac4d1b 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -290,7 +290,7 @@ struct opt_params dopts = {
>  		},
>  		{ .index = D_RTINHERIT,
>  		  .conflicts = { LAST_CONFLICT },
> -		  .minval = 1,
> +		  .minval = 0,
>  		  .maxval = 1,
>  		  .defaultval = 1,
>  		},
> -- 
> 2.12.1
> 
> --
> 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

Re: [PATCH] mkfs: rtinherit minval should be 0

[Eric Sandeen]

On 4/6/17 9:34 AM, Jan Tulak wrote:
> As with any other option, rtinherit=[0|1], but minval was incorrectly
> set to 1, so it was not possible to disable this option.

Long ago, when this option was added, it was simply a flag with
no option parsing, i.e. "-d rtinherit"

so I just want to double check ...

1) was it intentional that this turned into a "=0/=1" type option,
   i.e. an option which can be specified as disabled, essentially restating
   the default?  Is it the intent that every flag option must now take a
   value, and that it must take both "off" and "on" values?  Just checking
   that I haven't lost the thread, here.

IOWS: we used to have only "-d rtinherit"  But I think now we accept
-d rtinherit, -d rtinherit=0, and -d rtinherit=1.  Maybe it's water
under the bridge, I don't see the use in adding value parsing to
something that was just a simple flag before.  Can you enlighten
me?

2) really, this and projinherit and, um, extszinherit should
   probably all go away.  They were written for testing, nothing
   tests them, and they aren't documented.  Any volunteers for
   that?  It actually finds its way outside of pure mkfs code,
   so it's a little tricky to completely eradicate it, but it
   could be done in 2 steps I think.

thanks,
-Eric

> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 6fedc05c..5aac4d1b 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -290,7 +290,7 @@ struct opt_params dopts = {
>  		},
>  		{ .index = D_RTINHERIT,
>  		  .conflicts = { LAST_CONFLICT },
> -		  .minval = 1,
> +		  .minval = 0,
>  		  .maxval = 1,
>  		  .defaultval = 1,
>  		},
> 

Re: [PATCH] mkfs: rtinherit minval should be 0

[Jan Tulak]

On Thu, Apr 6, 2017 at 6:58 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> On 4/6/17 9:34 AM, Jan Tulak wrote:
>> As with any other option, rtinherit=[0|1], but minval was incorrectly
>> set to 1, so it was not possible to disable this option.
>
> Long ago, when this option was added, it was simply a flag with
> no option parsing, i.e. "-d rtinherit"
>
> so I just want to double check ...
>
> 1) was it intentional that this turned into a "=0/=1" type option,
>    i.e. an option which can be specified as disabled, essentially restating
>    the default?  Is it the intent that every flag option must now take a
>    value, and that it must take both "off" and "on" values?  Just checking
>    that I haven't lost the thread, here.
>
> IOWS: we used to have only "-d rtinherit"  But I think now we accept
> -d rtinherit, -d rtinherit=0, and -d rtinherit=1.  Maybe it's water
> under the bridge, I don't see the use in adding value parsing to
> something that was just a simple flag before.  Can you enlighten
> me?
>

(Sending again, mailing list refuses emails with html part outright
and it looks like gmail for ios can't sent plaintext only. Sorry if
you got this twice, Eric.)

It is using the same framework as all other options, so it got the 0/1
for free by simply being in the opts table (assuming the min/max
values are set to 0/1 respectively). We need this behaviour for
options like crc anyway, so why not standardize it, if all it costs
you is to set minval to 0?

This behaviour was added in the previous part of my/Dave's cleaning work.

Jan

> 2) really, this and projinherit and, um, extszinherit should
>    probably all go away.  They were written for testing, nothing
>    tests them, and they aren't documented.  Any volunteers for
>    that?  It actually finds its way outside of pure mkfs code,
>    so it's a little tricky to completely eradicate it, but it
>    could be done in 2 steps I think.
>
> thanks,
> -Eric
>
>> Signed-off-by: Jan Tulak <jtulak@redhat.com>
>> ---
>>  mkfs/xfs_mkfs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index 6fedc05c..5aac4d1b 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -290,7 +290,7 @@ struct opt_params dopts = {
>>               },
>>               { .index = D_RTINHERIT,
>>                 .conflicts = { LAST_CONFLICT },
>> -               .minval = 1,
>> +               .minval = 0,
>>                 .maxval = 1,
>>                 .defaultval = 1,
>>               },
>>



-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me

Re: [PATCH] mkfs: rtinherit minval should be 0

[Dave Chinner]

On Thu, Apr 06, 2017 at 11:58:45AM -0500, Eric Sandeen wrote:
> On 4/6/17 9:34 AM, Jan Tulak wrote:
> > As with any other option, rtinherit=[0|1], but minval was incorrectly
> > set to 1, so it was not possible to disable this option.
> 
> Long ago, when this option was added, it was simply a flag with
> no option parsing, i.e. "-d rtinherit"
> 
> so I just want to double check ...
> 
> 1) was it intentional that this turned into a "=0/=1" type option,
>    i.e. an option which can be specified as disabled, essentially restating
>    the default?  Is it the intent that every flag option must now take a
>    value, and that it must take both "off" and "on" values?  Just checking
>    that I haven't lost the thread, here.
> 
> IOWS: we used to have only "-d rtinherit"  But I think now we accept
> -d rtinherit, -d rtinherit=0, and -d rtinherit=1.  Maybe it's water
> under the bridge, I don't see the use in adding value parsing to
> something that was just a simple flag before.  Can you enlighten
> me?

That's pretty much my original thinking in setting this option up
this way. i.e. it'a an option that can only be turned on as nothing
will turn it on automatically and so there is no reason to have a
mechanism to turn it off. Hence minval = maxval to give only a
single valid value for the option....

> 2) really, this and projinherit and, um, extszinherit should
>    probably all go away.  They were written for testing, nothing
>    tests them, and they aren't documented.  Any volunteers for
>    that?  It actually finds its way outside of pure mkfs code,
>    so it's a little tricky to completely eradicate it, but it
>    could be done in 2 steps I think.

Actually, I've been wanting them to be enabled and documented as
first class mkfs options, along with all the other inheritable inode
options, such as DAX. This is so mkfs can be told that a filesystem
will, say, always use DAX to access files with an extent size hint
sized to trigger the huge page fault paths as the default
behaviour for the filesystem and so it sets the appropriate
attributes on the root inode when it is created...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] mkfs: rtinherit minval should be 0

[Eric Sandeen]

On 4/6/17 8:22 PM, Dave Chinner wrote:
> On Thu, Apr 06, 2017 at 11:58:45AM -0500, Eric Sandeen wrote:
>> On 4/6/17 9:34 AM, Jan Tulak wrote:
>>> As with any other option, rtinherit=[0|1], but minval was incorrectly
>>> set to 1, so it was not possible to disable this option.
>>
>> Long ago, when this option was added, it was simply a flag with
>> no option parsing, i.e. "-d rtinherit"
>>
>> so I just want to double check ...
>>
>> 1) was it intentional that this turned into a "=0/=1" type option,
>>    i.e. an option which can be specified as disabled, essentially restating
>>    the default?  Is it the intent that every flag option must now take a
>>    value, and that it must take both "off" and "on" values?  Just checking
>>    that I haven't lost the thread, here.
>>
>> IOWS: we used to have only "-d rtinherit"  But I think now we accept
>> -d rtinherit, -d rtinherit=0, and -d rtinherit=1.  Maybe it's water
>> under the bridge, I don't see the use in adding value parsing to
>> something that was just a simple flag before.  Can you enlighten
>> me?
> 
> That's pretty much my original thinking in setting this option up
> this way. i.e. it'a an option that can only be turned on as nothing
> will turn it on automatically and so there is no reason to have a
> mechanism to turn it off. Hence minval = maxval to give only a
> single valid value for the option....

Ok, yeah, that was my understanding, stated more succinctly.

So as with any patch, I'd ask: What problem does this solve?
How does it /improve/ mkfs.xfs?

>> 2) really, this and projinherit and, um, extszinherit should
>>    probably all go away.  They were written for testing, nothing
>>    tests them, and they aren't documented.  Any volunteers for
>>    that?  It actually finds its way outside of pure mkfs code,
>>    so it's a little tricky to completely eradicate it, but it
>>    could be done in 2 steps I think.
> 
> Actually, I've been wanting them to be enabled and documented as

hohum, ok ;)

> first class mkfs options, along with all the other inheritable inode
> options, such as DAX. This is so mkfs can be told that a filesystem
> will, say, always use DAX to access files with an extent size hint
> sized to trigger the huge page fault paths as the default
> behaviour for the filesystem and so it sets the appropriate
> attributes on the root inode when it is created...

But if you dig into this, it goes way beyond setting an inheritable
flag on the root inode, AFAICT... I guess maybe that's the
interaction with the protofile crud.  :/  I'll take another look
(9f064b7ee added it, for those playing along at home...) ;)

Thanks,
-Eric

> Cheers,
> 
> Dave.
>