| Message ID | 157062063684.32346.12253005903079702405.stgit@fedora-28 (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | xfs: mount API patch series | expand |
On Wed, Oct 09, 2019 at 07:30:36PM +0800, Ian Kent wrote: > The mount-api doesn't have a "human unit" parse type yet so > the options that have values like "10k" etc. still need to > be converted by the fs. Any reason you don't lift it to the the core mount api code?
On Wed, Oct 09, 2019 at 07:48:59AM -0700, Christoph Hellwig wrote: > On Wed, Oct 09, 2019 at 07:30:36PM +0800, Ian Kent wrote: > > The mount-api doesn't have a "human unit" parse type yet so > > the options that have values like "10k" etc. still need to > > be converted by the fs. > > Any reason you don't lift it to the the core mount api code? The garbage enum fs_parameter_type thing. Once we kill that (and I *will* kill it), sure - it's a sufficiently common case to have helpers for it around. But until then I'll veto any further additions to that trash pile. Look at fs_parse() - every sodding value added to that enum turns into an extra case in a large switch. It's unsustainable in that form and I *really* don't want to be in position of gatekeeper deciding what is and what isn't sufficiently useful to go there. What we need to do is to turn fs_parameter_type into a pointer to function. With fs_param_is_bool et.al. becoming instances of such, and fs_parse() switch from hell turning into err = p->type(p, param, result); That won't affect the existing macros or any filesystem code. If some filesystem wants to have helpers of its own - more power to it, just use __fsparam(my_bloody_helper, "foo", Opt_foo, 0) and be done with that. The thing is, "here's a commonly useful helper, let's take it to fs/*.c" is much easier than "add a new constant to that global enum and a case to that ever-growing switch in fs_parser.c".
On Wed, Oct 09, 2019 at 04:21:27PM +0100, Al Viro wrote: > What we need to do is to turn fs_parameter_type into a pointer > to function. With fs_param_is_bool et.al. becoming instances > of such, and fs_parse() switch from hell turning into > err = p->type(p, param, result); > > That won't affect the existing macros or any filesystem code. > If some filesystem wants to have helpers of its own - more > power to it, just use __fsparam(my_bloody_helper, "foo", Opt_foo, 0) > and be done with that. Actually, while we could keep the old macros around at least temporarily for existing users I think killing them actually would improve the file systems as well. This: static const struct fs_parameter_spec afs_param_specs[] = { { "autocell", Opt_autocell, fs_parse_flag }, { "dyn", Opt_dyn, fs_parse_flag }, { "flock", Opt_flock, fs_parse_enum }, { "source", Opt_source, fs_parse_string }, {} }; is a lot more obvious than: static const struct fs_parameter_spec afs_param_specs[] = { fsparam_flag ("autocell", Opt_autocell), fsparam_flag ("dyn", Opt_dyn), fsparam_enum ("flock", Opt_flock), fsparam_string("source", Opt_source), {} };
On Wed, Oct 09, 2019 at 08:29:11AM -0700, Christoph Hellwig wrote: > On Wed, Oct 09, 2019 at 04:21:27PM +0100, Al Viro wrote: > > What we need to do is to turn fs_parameter_type into a pointer > > to function. With fs_param_is_bool et.al. becoming instances > > of such, and fs_parse() switch from hell turning into > > err = p->type(p, param, result); > > > > That won't affect the existing macros or any filesystem code. > > If some filesystem wants to have helpers of its own - more > > power to it, just use __fsparam(my_bloody_helper, "foo", Opt_foo, 0) > > and be done with that. > > Actually, while we could keep the old macros around at least > temporarily for existing users I think killing them actually would > improve the file systems as well. > > This: > > static const struct fs_parameter_spec afs_param_specs[] = { > { "autocell", Opt_autocell, fs_parse_flag }, > { "dyn", Opt_dyn, fs_parse_flag }, > { "flock", Opt_flock, fs_parse_enum }, > { "source", Opt_source, fs_parse_string }, > {} > }; > > > is a lot more obvious than: > > static const struct fs_parameter_spec afs_param_specs[] = { > fsparam_flag ("autocell", Opt_autocell), > fsparam_flag ("dyn", Opt_dyn), > fsparam_enum ("flock", Opt_flock), > fsparam_string("source", Opt_source), > {} > }; Except that I want to be able to have something like - fsparam_enum ("errors", Opt_errors), + fsparam_enum ("errors", Opt_errors, gfs2_param_errors), with +static const struct fs_parameter_enum gfs2_param_errors[] = { + {"withdraw", Opt_errors_withdraw }, + {"panic", Opt_errors_panic }, + {} +}; instead of having them all squashed into one array, as in -static const struct fs_parameter_enum gfs2_param_enums[] = { - { Opt_quota, "off", Opt_quota_off }, - { Opt_quota, "account", Opt_quota_account }, - { Opt_quota, "on", Opt_quota_on }, - { Opt_data, "writeback", Opt_data_writeback }, - { Opt_data, "ordered", Opt_data_ordered }, - { Opt_errors, "withdraw", Opt_errors_withdraw }, - { Opt_errors, "panic", Opt_errors_panic }, ... const struct fs_parameter_description gfs2_fs_parameters = { .name = "gfs2", .specs = gfs2_param_specs, - .enums = gfs2_param_enums, }; IOW, I want to kill ->enums thing. And ->name is also trivial to kill, at which point we are left with just what used to be ->specs. Another thing is, struct fs_parameter_enum becomes pretty much identical to struct constant_table and can be folded into it. I have some experiments in that direction (very incomplete right now) in #work.mount-parser-later; next cycle fodder, I'm afraid. Another thing is, consider something like "it's an integer in range from 2 to 36". Fairly useful in many cases, and we could do helpers for that. Except that they need a pointer to helper-private data (the limits)... These macros somewhat isolate the filesystems until the things settle down. And one needs examples of conversions to see what's missing - inventing a grand scheme out of thin air doesn't work...
On Wed, Oct 09, 2019 at 05:03:10PM +0100, Al Viro wrote: > Except that I want to be able to have something like > - fsparam_enum ("errors", Opt_errors), > + fsparam_enum ("errors", Opt_errors, gfs2_param_errors), > with > +static const struct fs_parameter_enum gfs2_param_errors[] = { > + {"withdraw", Opt_errors_withdraw }, > + {"panic", Opt_errors_panic }, > + {} > +}; > instead of having them all squashed into one array, as in Makes total sense and still fits the above scheme. > IOW, I want to kill ->enums thing. And ->name is also trivial > to kill, at which point we are left with just what used to be > ->specs. Agreed. > I have some experiments in that direction (very incomplete right > now) in #work.mount-parser-later; next cycle fodder, I'm afraid. I like that a lot, and feel like we really shouldn't do more conversions until that ground work has been done
On Wed, Oct 09, 2019 at 11:01:02AM -0700, Christoph Hellwig wrote: > On Wed, Oct 09, 2019 at 05:03:10PM +0100, Al Viro wrote: > > Except that I want to be able to have something like > > - fsparam_enum ("errors", Opt_errors), > > + fsparam_enum ("errors", Opt_errors, gfs2_param_errors), > > with > > +static const struct fs_parameter_enum gfs2_param_errors[] = { > > + {"withdraw", Opt_errors_withdraw }, > > + {"panic", Opt_errors_panic }, > > + {} > > +}; > > instead of having them all squashed into one array, as in > > Makes total sense and still fits the above scheme. > > > IOW, I want to kill ->enums thing. And ->name is also trivial > > to kill, at which point we are left with just what used to be > > ->specs. > > Agreed. > > > I have some experiments in that direction (very incomplete right > > now) in #work.mount-parser-later; next cycle fodder, I'm afraid. > > I like that a lot, and feel like we really shouldn't do more > conversions until that ground work has been done I'm not sure - for one thing, it's pretty much orthogonal to the bulk of conversion; for another, I'm very sceptical about grand schemes invented out of thin air in hope to cover everything in the world, without finding out what _is_ there first. Massaging the parser data structures can be done on top of the other work just as well - the conflicts will be trivial to deal with. And I'm perfectly fine with having the parser stuff go in last, just prior to -rc1, so resolution will be my headache. Alternatively, I can do never-rebased short branch right now, with the parts of changes likely to cause conflicts, so that xfs, nfs, etc. work could pull it and be done with that. Note that e.g. conversion of fs_is_... from enum members to functions would require zero changes in filesystems. It would allow to simplify them later on, but I would very much prefer to do those extra helpers with converted codebase already on hand.
On Wed, 2019-10-09 at 07:48 -0700, Christoph Hellwig wrote: > On Wed, Oct 09, 2019 at 07:30:36PM +0800, Ian Kent wrote: > > The mount-api doesn't have a "human unit" parse type yet so > > the options that have values like "10k" etc. still need to > > be converted by the fs. > > Any reason you don't lift it to the the core mount api code? I talked to David about that and there were some patches but I didn't see them posted. But it seems there may have been a reason for that, based on Al's comments. I'll leave this as it is for now. Ian
On Wed, Oct 09, 2019 at 07:22:22PM +0100, Al Viro wrote: > Massaging the parser data structures can be done on top of the > other work just as well - the conflicts will be trivial to deal > with. And I'm perfectly fine with having the parser stuff > go in last, just prior to -rc1, so resolution will be my > headache. So, it is that time of the cycle now. Would you mind updating the branch and feeding it to Linus?
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 9c1ce3d70c08..6a16750b1314 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -160,13 +160,13 @@ static const struct fs_parameter_description xfs_fs_parameters = { }; STATIC int -suffix_kstrtoint(const substring_t *s, unsigned int base, int *res) +suffix_kstrtoint(const char *s, unsigned int base, int *res) { int last, shift_left_factor = 0, _res; char *value; int ret = 0; - value = match_strdup(s); + value = kstrdup(s, GFP_KERNEL); if (!value) return -ENOMEM; @@ -191,6 +191,20 @@ suffix_kstrtoint(const substring_t *s, unsigned int base, int *res) return ret; } +STATIC int +match_kstrtoint(const substring_t *s, unsigned int base, int *res) +{ + const char *value; + int ret; + + value = match_strdup(s); + if (!value) + return -ENOMEM; + ret = suffix_kstrtoint(value, base, res); + kfree(value); + return ret; +} + /* * This function fills in xfs_mount_t fields based on mount args. * Note: the superblock has _not_ yet been read in. @@ -262,7 +276,7 @@ xfs_parseargs( return -EINVAL; break; case Opt_logbsize: - if (suffix_kstrtoint(args, 10, &mp->m_logbsize)) + if (match_kstrtoint(args, 10, &mp->m_logbsize)) return -EINVAL; break; case Opt_logdev: @@ -278,7 +292,7 @@ xfs_parseargs( return -ENOMEM; break; case Opt_allocsize: - if (suffix_kstrtoint(args, 10, &iosize)) + if (match_kstrtoint(args, 10, &iosize)) return -EINVAL; iosizelog = ffs(iosize) - 1; break;