| Message ID | 20240209000857.21040-1-bodonnel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | zonefs: convert zonefs to use the new mount api | expand |
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@redhat.com> 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. > + > +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. > + > + kfree(ctx); Is it safe to not set fc->fs_private to NULL ? > +} > + > +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" ? > + 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",
On Fri, Feb 09, 2024 at 11:10:00AM +0900, 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@redhat.com> > > Thanks for doing this. I will run tests on this but I do have a few nits below. I will provide a v2 with the changes you suggest, with one exception (please see my note within). > > > --- > > 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. > > > + > > +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. > > > + > > + kfree(ctx); > > Is it safe to not set fc->fs_private to NULL ? I agree that ctx is not needed, and instead kfree(fc->fs_private) is sufficient. However, since other fs conversions do not simply set fc->fs_private to NULL, kfree(fc_fs_private) is preferred here. > > > +} > > + > > +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" ? > > > + 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", > > -- > Damien Le Moal > Western Digital Research >
On 10/2/24 00:55, Bill O'Donnell wrote: > On Fri, Feb 09, 2024 at 11:10:00AM +0900, 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@redhat.com> >> Thanks for doing this. I will run tests on this but I do have a few nits below. > I will provide a v2 with the changes you suggest, with one exception (please > see my note within). First sorry I haven't had time to devote to looking at this just lately. I'll review the patch here as soon as I can but ... One thing that I wanted to add about the mount api conversions, not just this conversion and I'm also guilty of it, is the handling of error returns. One of the things the mount api is meant to do is provide a way for user space to retrieve error strings from the kernel mount process. Within the current mount api this is done via macros such as invalfc(), warnfc() et. al. (see fs_context.h). But we have seen that using them can also increase the kernel log noise and can cause surprises for users. On one occasion an NFS user was, it seemed, unwilling to accept that it was the logging in the NFS mount api change that caused a new log message and the cause had been there all along. It was this that caused me to think we will probably need to revisit this after the conversions because they may need some changes. Bottom line is I'm not sure we need to care about using these macros, and they haven't been used here, but maybe they should be to save on further work later. It's not a simple decision but is worth some thought. It's probably worth casting a eye over those macros and the log functions they lead to but please don't worry too much as I expect these will need some more thought anyway. Ian > >>> --- >>> 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. >> >>> + >>> +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. >> >>> + >>> + kfree(ctx); >> Is it safe to not set fc->fs_private to NULL ? > I agree that ctx is not needed, and instead kfree(fc->fs_private) is > sufficient. However, since other fs conversions do not simply set > fc->fs_private to NULL, kfree(fc_fs_private) is preferred here. > >>> +} >>> + >>> +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" ? >> >>> + 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", >> -- >> Damien Le Moal >> Western Digital Research >> >
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@redhat.com> > 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@themaw.net> 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",
On 2/11/24 12:36, Ian Kent wrote: >>> +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. That I know, which is why I asked if *not* setting fc->fs_private to NULL after the kfree is safe. Because if another call to kfree for that pointer is done, we will endup with a double free oops. But as long as the mount API guarantees that it will not happen, then OK. > > > 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. Keeping the variable is fine. After all, that is not the fast path :)
On 12/2/24 09:13, Damien Le Moal wrote: > On 2/11/24 12:36, Ian Kent wrote: >>>> +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. > That I know, which is why I asked if *not* setting fc->fs_private to NULL after > the kfree is safe. Because if another call to kfree for that pointer is done, we > will endup with a double free oops. But as long as the mount API guarantees that > it will not happen, then OK. Interesting point, TBH I hadn't thought about it. Given that, as far as I have seen, VFS struct private fields leave the setting and freeing of them to the file system so I assumed that, seeing this done in other mount api implementations, including ones written by the mount api author, it was the same as other VFS cases. But it's not too hard to check. Ian > >> >> 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. > Keeping the variable is fine. After all, that is not the fast path :) > >
On 12/2/24 20:12, Ian Kent wrote: > On 12/2/24 09:13, Damien Le Moal wrote: >> On 2/11/24 12:36, Ian Kent wrote: >>>>> +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. >> That I know, which is why I asked if *not* setting fc->fs_private to >> NULL after >> the kfree is safe. Because if another call to kfree for that pointer >> is done, we >> will endup with a double free oops. But as long as the mount API >> guarantees that >> it will not happen, then OK. > > Interesting point, TBH I hadn't thought about it. > > > Given that, as far as I have seen, VFS struct private fields leave the > > setting and freeing of them to the file system so I assumed that, seeing > > this done in other mount api implementations, including ones written by > > the mount api author, it was the same as other VFS cases. > > > But it's not too hard to check. As I thought, the context private data field is delegated to the file system. The usage here is as expected by the VFS. Ian > > > Ian > >> >>> >>> 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. >> Keeping the variable is fine. After all, that is not the fast path :) >> >> >
On Wed, Feb 14, 2024 at 07:31:09AM +0800, Ian Kent wrote: > On 12/2/24 20:12, Ian Kent wrote: > > On 12/2/24 09:13, Damien Le Moal wrote: > > > On 2/11/24 12:36, Ian Kent wrote: > > > > > > +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. > > > That I know, which is why I asked if *not* setting fc->fs_private to > > > NULL after > > > the kfree is safe. Because if another call to kfree for that pointer > > > is done, we > > > will endup with a double free oops. But as long as the mount API > > > guarantees that > > > it will not happen, then OK. > > > > Interesting point, TBH I hadn't thought about it. > > > > > > Given that, as far as I have seen, VFS struct private fields leave the > > > > setting and freeing of them to the file system so I assumed that, seeing > > > > this done in other mount api implementations, including ones written by > > > > the mount api author, it was the same as other VFS cases. > > > > > > But it's not too hard to check. > > As I thought, the context private data field is delegated to the file > system. > > The usage here is as expected by the VFS. Thanks for the reviews. I submitted a v2 patch. Cheers- Bill > > > Ian > > > > > > > Ian > > > > > > > > > > > > > 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. > > > Keeping the variable is fine. After all, that is not the fast path :) > > > > > > > > >
On 2/14/24 08:37, Bill O'Donnell wrote: > On Wed, Feb 14, 2024 at 07:31:09AM +0800, Ian Kent wrote: >> On 12/2/24 20:12, Ian Kent wrote: >>> On 12/2/24 09:13, Damien Le Moal wrote: >>>> On 2/11/24 12:36, Ian Kent wrote: >>>>>>> +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. >>>> That I know, which is why I asked if *not* setting fc->fs_private to >>>> NULL after >>>> the kfree is safe. Because if another call to kfree for that pointer >>>> is done, we >>>> will endup with a double free oops. But as long as the mount API >>>> guarantees that >>>> it will not happen, then OK. >>> >>> Interesting point, TBH I hadn't thought about it. >>> >>> >>> Given that, as far as I have seen, VFS struct private fields leave the >>> >>> setting and freeing of them to the file system so I assumed that, seeing >>> >>> this done in other mount api implementations, including ones written by >>> >>> the mount api author, it was the same as other VFS cases. >>> >>> >>> But it's not too hard to check. >> >> As I thought, the context private data field is delegated to the file >> system. >> >> The usage here is as expected by the VFS. > > Thanks for the reviews. I submitted a v2 patch. > Cheers- > Bill I will run tests today. Thanks.
On 2/14/24 08:37, Bill O'Donnell wrote: > On Wed, Feb 14, 2024 at 07:31:09AM +0800, Ian Kent wrote: >> On 12/2/24 20:12, Ian Kent wrote: >>> On 12/2/24 09:13, Damien Le Moal wrote: >>>> On 2/11/24 12:36, Ian Kent wrote: >>>>>>> +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. >>>> That I know, which is why I asked if *not* setting fc->fs_private to >>>> NULL after >>>> the kfree is safe. Because if another call to kfree for that pointer >>>> is done, we >>>> will endup with a double free oops. But as long as the mount API >>>> guarantees that >>>> it will not happen, then OK. >>> >>> Interesting point, TBH I hadn't thought about it. >>> >>> >>> Given that, as far as I have seen, VFS struct private fields leave the >>> >>> setting and freeing of them to the file system so I assumed that, seeing >>> >>> this done in other mount api implementations, including ones written by >>> >>> the mount api author, it was the same as other VFS cases. >>> >>> >>> But it's not too hard to check. >> >> As I thought, the context private data field is delegated to the file >> system. >> >> The usage here is as expected by the VFS. > > Thanks for the reviews. I submitted a v2 patch. > Cheers- > Bill Testing was all good, so I applied this to for-6.9 branch. I did tweak the patch to re-add the local ctx variable in zonefs_free_fc() since that seems to be the accepted pattern. Thanks !
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> #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); + +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; 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); + 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; + + kfree(ctx); +} + +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; + 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, }; +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",
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@redhat.com> --- fs/zonefs/super.c | 156 ++++++++++++++++++++++++++-------------------- 1 file changed, 90 insertions(+), 66 deletions(-)