Re: [PATCH 1/9] f2fs: Add fs parameter specifications for mount options

[Eric Sandeen <sandeen@sandeen.net>]

On 8/13/24 9:39 PM, Hongbo Li wrote:
> Use an array of `fs_parameter_spec` called f2fs_param_specs to
> hold the mount option specifications for the new mount api.
> 
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
>  fs/f2fs/super.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 3959fd137cc9..1bd923a73c1f 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -28,6 +28,7 @@
>  #include <linux/part_stat.h>
>  #include <linux/zstd.h>
>  #include <linux/lz4.h>
> +#include <linux/fs_parser.h>
>  
>  #include "f2fs.h"
>  #include "node.h"
> @@ -189,9 +190,87 @@ enum {
>  	Opt_memory_mode,
>  	Opt_age_extent_cache,
>  	Opt_errors,
> +	Opt_jqfmt,
> +	Opt_checkpoint,

If adding an opt_jqfmt to use with an enum, you can/should remove
Opt_jqfmt_vfsold Opt_jqfmt_vfsv0, and Opt_jqfmt_vfsv1, because they
are no longer used.

Similarly for Opt_checkpoint_disable_* symbols.

>  	Opt_err,
>  };
>  
> +static const struct constant_table f2fs_param_jqfmt[] = {
> +	{"vfsold",	QFMT_VFS_OLD},
> +	{"vfsv0",	QFMT_VFS_V0},
> +	{"vfsv1",	QFMT_VFS_V1},
> +	{}
> +};
> +
> +static const struct fs_parameter_spec f2fs_param_specs[] = {
> +	fsparam_string("background_gc", Opt_gc_background),
> +	fsparam_flag("disable_roll_forward", Opt_disable_roll_forward),
> +	fsparam_flag("norecovery", Opt_norecovery),

Many/most other filesystems tab-align the param_spec, like

...
+	fsparam_string	("background_gc",	Opt_gc_background),
+	fsparam_flag	("disable_roll_forward",Opt_disable_roll_forward),
+	fsparam_flag	("norecovery",		Opt_norecovery),
...

but that's just a style thing, up to you and the maintainers.

I'd also suggest making more use of enums (as you did for f2fs_param_jqfmt).
I think it can simplify parsing in the long run if you choose to. It avoids
the "if strcmp() else if strcmp() else if strcmp()... pattern, for example
you can do:

static const struct constant_table f2fs_param_background_gc[] = {
	{"on",		BGGC_MODE_ON},
	{"off",		BGGC_MODE_OFF},
	{"sync",	BGGC_MODE_SYNC},
	{}
};

...

	fsparam_enum	("background_gc",	Opt_gc_background, f2fs_param_background_gc),

...

and then parsing becomes simply:

	case Opt_gc_background:
		F2FS_CTX_INFO(ctx).bggc_mode = result.uint_32;
		ctx->spec_mask |= F2FS_SPEC_background_gc;
		break;

When I tried this I made a lot of use of enums, see
https://git.kernel.org/pub/scm/linux/kernel/git/sandeen/linux.git/tree/fs/f2fs/super.c?h=f2fs-mount-api#n182
and see what you think?

Thanks,
-Eric