Re: [PATCH 02/17] ext4: Add fs parameter description

[Lukas Czerner <lczerner@redhat.com>]

On Tue, Dec 17, 2019 at 03:20:12PM +0000, Al Viro wrote:
> On Tue, Dec 17, 2019 at 01:19:56PM +0100, Lukas Czerner wrote:
> > On Tue, Dec 17, 2019 at 12:44:19AM +0000, Al Viro wrote:
> > > On Wed, Nov 06, 2019 at 11:14:42AM +0100, Lukas Czerner wrote:
> > > > +	fsparam_string_empty
> > > > +			("usrjquota",		Opt_usrjquota),
> > > > +	fsparam_string_empty
> > > > +			("grpjquota",		Opt_grpjquota),
> > > 
> > > Umm...  That makes ...,usrjquota,... equivalent to ...,usrjquota=,...
> > > unless I'm misreading the series.  Different from mainline, right?
> > 
> > Unfortunatelly yes, I do not think this is a problem, but if you have a
> > solution within the new mount api framework I am happy to use it.
> 
> Er...  Dump the fsparam_string_empty() use and instead of your
> +       if (token == Opt_usrjquota) {
> +               if (result.negated)
> +                       return clear_qf_name(sb, USRQUOTA);
> +               else
> +                       return set_qf_name(sb, USRQUOTA, param);
> do
> +       if (token == Opt_usrjquota) {
> +               if (!param->string[0])
> +                       return clear_qf_name(sb, USRQUOTA);
> +               else
> +                       return set_qf_name(sb, USRQUOTA, param);
> with the same for grpjquota?

Ah right, it's been a while I guess I didn't realize that it will accept
usrjquota as well as usrjquota=

It makes sense to check the string directly, thanks.

> 
> > > > +	fsparam_bool	("barrier",		Opt_barrier),
> > > > +	fsparam_flag	("nobarrier",		Opt_nobarrier),
> > > 
> > > That's even more interesting.  Current mainline:
> > > 		barrier		OK, sets EXT4_MOUNT_BARRIER
> > > 		barrier=0	OK, sets EXT4_MOUNT_BARRIER
> > > 		barrier=42	OK, sets EXT4_MOUNT_BARRIER
> > > 		barrier=yes	error
> > > 		barrier=no	error
> > > 		nobarrier	OK, clears EXT4_MOUNT_BARRIER
> > > Unless I'm misreading your series, you get
> > > 		barrier		error
> > 
> > Not really, this seems to be working as expected. Assuming that this
> > didn't change since 5.4.0-rc6. I does make sense to me that specifying
> > bool type parameter without any options would express "true".
> > 
> > 
> > > 		barrier=0	OK, sets EXT4_MOUNT_BARRIER
> > 
> > 
> > > 		barrier=42	error
> > > 		barrier=yes	OK, sets EXT4_MOUNT_BARRIER
> > > 		barrier=no	OK, sets EXT4_MOUNT_BARRIER
> > 
> > Those three are different, just because of how param_book() work. I do
> > not really see a problem with it, but if we want to keep it exactly the
> > same as current mainline it would be difficult with how the current api
> > works. Any suggestions ?
> 
> If fsparam_bool() doesn't do the right thing, perhaps it shouldn't be
> used in the first place?  Or changed, for that matter - it's not as if
> we had many users of that thing and the only in-tree one is definitely
> breaking userland ABI.
> 
> In case of ext4, after rereading that code (and getting some sleep) the
> current behaviour is, AFAICS to accept barrier | nobarrier | barrier=<number>
> with the last case being equialent to nobarrier when number is 0 and barrier
> in all other cases.  Is that an accurate description?

Yeah, the documentation says
barrier=0 / barrier=1 / barrier / nobarrier

but we do accept any number from 0 to 2147483647

> 
> If so, I would prefer
> 	fsparam_flag_no("barrier", Opt_barrier),	// barrier | nobarrier
> 	fsparam_u32("barrier", Opt_barrier),		// barrier=<number>
> as the solution, with fs_parse() having been taught to allow argument-bearing
> and argument-less options with the same name, picking the right one.  That
> way Opt_nobarrier gets removed as well...
> 
> I'll push a branch with that stuff later today; will post when it's out...

That would be great, thanks.

-Lukas