* [PATCH] mkfs: rtinherit minval should be 0 @ 2017-04-06 14:34 Jan Tulak 2017-04-06 15:42 ` Darrick J. Wong 2017-04-06 16:58 ` Eric Sandeen 0 siblings, 2 replies; 6+ messages in thread From: Jan Tulak @ 2017-04-06 14:34 UTC (permalink / raw) To: linux-xfs; +Cc: 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mkfs: rtinherit minval should be 0 2017-04-06 14:34 [PATCH] mkfs: rtinherit minval should be 0 Jan Tulak @ 2017-04-06 15:42 ` Darrick J. Wong 2017-04-06 16:58 ` Eric Sandeen 1 sibling, 0 replies; 6+ messages in thread From: Darrick J. Wong @ 2017-04-06 15:42 UTC (permalink / raw) To: Jan Tulak; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mkfs: rtinherit minval should be 0 2017-04-06 14:34 [PATCH] mkfs: rtinherit minval should be 0 Jan Tulak 2017-04-06 15:42 ` Darrick J. Wong @ 2017-04-06 16:58 ` Eric Sandeen 2017-04-06 19:59 ` Jan Tulak 2017-04-07 1:22 ` Dave Chinner 1 sibling, 2 replies; 6+ messages in thread From: Eric Sandeen @ 2017-04-06 16:58 UTC (permalink / raw) To: Jan Tulak, linux-xfs 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, > }, > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mkfs: rtinherit minval should be 0 2017-04-06 16:58 ` Eric Sandeen @ 2017-04-06 19:59 ` Jan Tulak 2017-04-07 1:22 ` Dave Chinner 1 sibling, 0 replies; 6+ messages in thread From: Jan Tulak @ 2017-04-06 19:59 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mkfs: rtinherit minval should be 0 2017-04-06 16:58 ` Eric Sandeen 2017-04-06 19:59 ` Jan Tulak @ 2017-04-07 1:22 ` Dave Chinner 2017-04-07 2:46 ` Eric Sandeen 1 sibling, 1 reply; 6+ messages in thread From: Dave Chinner @ 2017-04-07 1:22 UTC (permalink / raw) To: Eric Sandeen; +Cc: Jan Tulak, linux-xfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mkfs: rtinherit minval should be 0 2017-04-07 1:22 ` Dave Chinner @ 2017-04-07 2:46 ` Eric Sandeen 0 siblings, 0 replies; 6+ messages in thread From: Eric Sandeen @ 2017-04-07 2:46 UTC (permalink / raw) To: Dave Chinner; +Cc: Jan Tulak, linux-xfs 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. > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-07 2:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-06 14:34 [PATCH] mkfs: rtinherit minval should be 0 Jan Tulak 2017-04-06 15:42 ` Darrick J. Wong 2017-04-06 16:58 ` Eric Sandeen 2017-04-06 19:59 ` Jan Tulak 2017-04-07 1:22 ` Dave Chinner 2017-04-07 2:46 ` Eric Sandeen
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.