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

[Eric Sandeen <sandeen@sandeen.net>]

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.
>