| Message ID | 20170406143426.19727-1-jtulak@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 -- 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
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, > }, > -- 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
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, >> }, >>
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.
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. > -- 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
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, },
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(-)