Re: [PATCH] zonefs: convert zonefs to use the new mount api

[Ian Kent <raven@xxxxxxxxxx>]

On 9/2/24 10:10, Damien Le Moal wrote:
> On 2/9/24 09:08, Bill O'Donnell wrote:
> > Convert the zonefs filesystem to use the new mount API.
> > Tested using the zonefs test suite from:
> > https://github.com/damien-lemoal/zonefs-tools
> >
> > Signed-off-by: Bill O'Donnell <bodonnel@xxxxxxxxxx>
> Thanks for doing this. I will run tests on this but I do have a few nits below.
>
> > ---
> >   fs/zonefs/super.c | 156 ++++++++++++++++++++++++++--------------------
> >   1 file changed, 90 insertions(+), 66 deletions(-)
> >
> > diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
> > index e6a75401677d..6b8ecd2e55b8 100644
> > --- a/fs/zonefs/super.c
> > +++ b/fs/zonefs/super.c
> > @@ -15,13 +15,13 @@
> >   #include <linux/writeback.h>
> >   #include <linux/quotaops.h>
> >   #include <linux/seq_file.h>
> > -#include <linux/parser.h>
> >   #include <linux/uio.h>
> >   #include <linux/mman.h>
> >   #include <linux/sched/mm.h>
> >   #include <linux/crc32.h>
> >   #include <linux/task_io_accounting_ops.h>
> > -
> > +#include <linux/fs_parser.h>
> > +#include <linux/fs_context.h>
> Please keep the whiteline here.
>
> >   #include "zonefs.h"
> >   
> >   #define CREATE_TRACE_POINTS
> > @@ -460,58 +460,47 @@ static int zonefs_statfs(struct dentry *dentry, struct kstatfs *buf)
> >   }
> >   
> >   enum {
> > -	Opt_errors_ro, Opt_errors_zro, Opt_errors_zol, Opt_errors_repair,
> > -	Opt_explicit_open, Opt_err,
> > +	Opt_errors, Opt_explicit_open,
> >   };
> >   
> > -static const match_table_t tokens = {
> > -	{ Opt_errors_ro,	"errors=remount-ro"},
> > -	{ Opt_errors_zro,	"errors=zone-ro"},
> > -	{ Opt_errors_zol,	"errors=zone-offline"},
> > -	{ Opt_errors_repair,	"errors=repair"},
> > -	{ Opt_explicit_open,	"explicit-open" },
> > -	{ Opt_err,		NULL}
> > +struct zonefs_context {
> > +	unsigned long s_mount_opts;
> >   };
> >   
> > -static int zonefs_parse_options(struct super_block *sb, char *options)
> > -{
> > -	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
> > -	substring_t args[MAX_OPT_ARGS];
> > -	char *p;
> > -
> > -	if (!options)
> > -		return 0;
> > -
> > -	while ((p = strsep(&options, ",")) != NULL) {
> > -		int token;
> > +static const struct constant_table zonefs_param_errors[] = {
> > +	{"remount-ro",		ZONEFS_MNTOPT_ERRORS_RO},
> > +	{"zone-ro",		ZONEFS_MNTOPT_ERRORS_ZRO},
> > +	{"zone-offline",	ZONEFS_MNTOPT_ERRORS_ZOL},
> > +	{"repair", 		ZONEFS_MNTOPT_ERRORS_REPAIR},
> > +	{}
> > +};
> >   
> > -		if (!*p)
> > -			continue;
> > +static const struct fs_parameter_spec zonefs_param_spec[] = {
> > +	fsparam_enum	("errors",		Opt_errors, zonefs_param_errors),
> > +	fsparam_flag	("explicit-open",	Opt_explicit_open),
> > +	{}
> > +};
> >   
> > -		token = match_token(p, tokens, args);
> > -		switch (token) {
> > -		case Opt_errors_ro:
> > -			sbi->s_mount_opts &= ~ZONEFS_MNTOPT_ERRORS_MASK;
> > -			sbi->s_mount_opts |= ZONEFS_MNTOPT_ERRORS_RO;
> > -			break;
> > -		case Opt_errors_zro:
> > -			sbi->s_mount_opts &= ~ZONEFS_MNTOPT_ERRORS_MASK;
> > -			sbi->s_mount_opts |= ZONEFS_MNTOPT_ERRORS_ZRO;
> > -			break;
> > -		case Opt_errors_zol:
> > -			sbi->s_mount_opts &= ~ZONEFS_MNTOPT_ERRORS_MASK;
> > -			sbi->s_mount_opts |= ZONEFS_MNTOPT_ERRORS_ZOL;
> > -			break;
> > -		case Opt_errors_repair:
> > -			sbi->s_mount_opts &= ~ZONEFS_MNTOPT_ERRORS_MASK;
> > -			sbi->s_mount_opts |= ZONEFS_MNTOPT_ERRORS_REPAIR;
> > -			break;
> > -		case Opt_explicit_open:
> > -			sbi->s_mount_opts |= ZONEFS_MNTOPT_EXPLICIT_OPEN;
> > -			break;
> > -		default:
> > -			return -EINVAL;
> > -		}
> > +static int zonefs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> > +{
> > +	struct zonefs_context *ctx = fc->fs_private;
> > +	struct fs_parse_result result;
> > +	int opt;
> > +
> > +	opt = fs_parse(fc, zonefs_param_spec, param, &result);
> > +	if (opt < 0)
> > +		return opt;
> > +
> > +	switch (opt) {
> > +	case Opt_errors:
> > +		ctx->s_mount_opts &= ~ZONEFS_MNTOPT_ERRORS_MASK;
> > +		ctx->s_mount_opts |= result.uint_32;
> > +		break;
> > +	case Opt_explicit_open:
> > +		ctx->s_mount_opts |= ZONEFS_MNTOPT_EXPLICIT_OPEN;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> >   	}
> >   
> >   	return 0;
> > @@ -533,11 +522,19 @@ static int zonefs_show_options(struct seq_file *seq, struct dentry *root)
> >   	return 0;
> >   }
> >   
> > -static int zonefs_remount(struct super_block *sb, int *flags, char *data)
> > +static int zonefs_get_tree(struct fs_context *fc);
> Why the forward definition ? It seems that you could define this function here
> directly.

Not here though, it looks like it could be defined after zonefs_fill_super()

and eliminate the forward declaration.


> > +
> > +static int zonefs_reconfigure(struct fs_context *fc)
> >   {
> > -	sync_filesystem(sb);
> > +	struct zonefs_context *ctx = fc->fs_private;
> > +	struct super_block *sb = fc->root->d_sb;
> > +	struct zonefs_sb_info *sbi = sb->s_fs_info;
> >   
> > -	return zonefs_parse_options(sb, data);
> > +	sync_filesystem(fc->root->d_sb);
> > +	/* Copy new options from ctx into sbi. */
> > +	sbi->s_mount_opts = ctx->s_mount_opts;
> > +
> > +	return 0;
> >   }
> >   
> >   static int zonefs_inode_setattr(struct mnt_idmap *idmap,
> > @@ -1197,7 +1194,6 @@ static const struct super_operations zonefs_sops = {
> >   	.alloc_inode	= zonefs_alloc_inode,
> >   	.free_inode	= zonefs_free_inode,
> >   	.statfs		= zonefs_statfs,
> > -	.remount_fs	= zonefs_remount,
> >   	.show_options	= zonefs_show_options,
> >   };
> >   
> > @@ -1242,9 +1238,10 @@ static void zonefs_release_zgroup_inodes(struct super_block *sb)
> >    * sub-directories and files according to the device zone configuration and
> >    * format options.
> >    */
> > -static int zonefs_fill_super(struct super_block *sb, void *data, int silent)
> > +static int zonefs_fill_super(struct super_block *sb, struct fs_context *fc)
> >   {
> >   	struct zonefs_sb_info *sbi;
> > +	struct zonefs_context *ctx = fc->fs_private;
> >   	struct inode *inode;
> >   	enum zonefs_ztype ztype;
> >   	int ret;
> > @@ -1281,21 +1278,17 @@ static int zonefs_fill_super(struct super_block *sb, void *data, int silent)
> >   	sbi->s_uid = GLOBAL_ROOT_UID;
> >   	sbi->s_gid = GLOBAL_ROOT_GID;
> >   	sbi->s_perm = 0640;
> > -	sbi->s_mount_opts = ZONEFS_MNTOPT_ERRORS_RO;
> > -
> > +	sbi->s_mount_opts = ctx->s_mount_opts;
> Please keep the white line here...
>
> >   	atomic_set(&sbi->s_wro_seq_files, 0);
> >   	sbi->s_max_wro_seq_files = bdev_max_open_zones(sb->s_bdev);
> >   	atomic_set(&sbi->s_active_seq_files, 0);
> > +
> ...and remove this one. The initializations here are "grouped" together byt
> "theme" (sbi standard stuff first and zone resource accounting in a second
> "paragraph". I like to keep it that way.
>
> >   	sbi->s_max_active_seq_files = bdev_max_active_zones(sb->s_bdev);
> >   
> >   	ret = zonefs_read_super(sb);
> >   	if (ret)
> >   		return ret;
> >   
> > -	ret = zonefs_parse_options(sb, data);
> > -	if (ret)
> > -		return ret;
> > -
> >   	zonefs_info(sb, "Mounting %u zones", bdev_nr_zones(sb->s_bdev));
> >   
> >   	if (!sbi->s_max_wro_seq_files &&
> > @@ -1356,12 +1349,6 @@ static int zonefs_fill_super(struct super_block *sb, void *data, int silent)
> >   	return ret;
> >   }
> >   
> > -static struct dentry *zonefs_mount(struct file_system_type *fs_type,
> > -				   int flags, const char *dev_name, void *data)
> > -{
> > -	return mount_bdev(fs_type, flags, dev_name, data, zonefs_fill_super);
> > -}
> > -
> >   static void zonefs_kill_super(struct super_block *sb)
> >   {
> >   	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
> > @@ -1376,17 +1363,54 @@ static void zonefs_kill_super(struct super_block *sb)
> >   	kfree(sbi);
> >   }
> >   
> > +static void zonefs_free_fc(struct fs_context *fc)
> > +{
> > +	struct zonefs_context *ctx = fc->fs_private;
> I do not think you need this variable.

That's a fair comment but it says fs_private contains the fs context

for the casual reader.

> > +
> > +	kfree(ctx);
> Is it safe to not set fc->fs_private to NULL ?

I think it's been safe to call kfree() with a NULL argument for ages.


This could be done but so far the convention with mount api code

appears to have been to add the local variable which I thought was for

descriptive purposes but it could just be the result of cut and pastes.


> > +}
> > +
> > +static const struct fs_context_operations zonefs_context_ops = {
> > +	.parse_param    = zonefs_parse_param,
> > +	.get_tree       = zonefs_get_tree,
> > +	.reconfigure	= zonefs_reconfigure,
> > +	.free           = zonefs_free_fc,
> > +};
> > +
> > +/*
> > + * Set up the filesystem mount context.
> > + */
> > +static int zonefs_init_fs_context(struct fs_context *fc)
> > +{
> > +	struct zonefs_context *ctx;
> > +
> > +	ctx = kzalloc(sizeof(struct zonefs_context), GFP_KERNEL);
> > +	if (!ctx)
> > +		return 0;
> return 0 ? Shouldn't this be "return -ENOMEM" ?

Good catch!


While having zonefs_parse_param() up with the declarations is fine

maybe it would be better to move it down to somewhere around

zonefs_fill_super() with the other mount api functions. Then, after

eliminating the forward declaration, only the declarations would

remain above and the mount api code itself would be in the same

place.


With the above changes it looks good to me.

Reviewed-by: Ian Kent <raven@xxxxxxxxxx>


Ian

> > +	ctx->s_mount_opts = ZONEFS_MNTOPT_ERRORS_RO;
> > +	fc->ops = &zonefs_context_ops;
> > +	fc->fs_private = ctx;
> > +
> > +	return 0;
> > +}
> > +
> >   /*
> >    * File system definition and registration.
> >    */
> >   static struct file_system_type zonefs_type = {
> >   	.owner		= THIS_MODULE,
> >   	.name		= "zonefs",
> > -	.mount		= zonefs_mount,
> >   	.kill_sb	= zonefs_kill_super,
> >   	.fs_flags	= FS_REQUIRES_DEV,
> > +	.init_fs_context	= zonefs_init_fs_context,
> > +	.parameters	= zonefs_param_spec,
> Please re-align everything together.
>
> >   };
> >   
> > +static int zonefs_get_tree(struct fs_context *fc)
> > +{
> > +	return get_tree_bdev(fc, zonefs_fill_super);
> > +}
> > +
> >   static int __init zonefs_init_inodecache(void)
> >   {
> >   	zonefs_inode_cachep = kmem_cache_create("zonefs_inode_cache",