* [PATCH] nilfs2: convert to use the new mount API
@ 2024-04-03 22:12 Eric Sandeen
2024-04-04 20:11 ` Ryusuke Konishi
0 siblings, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2024-04-03 22:12 UTC (permalink / raw)
To: linux-nilfs; +Cc: Ryusuke Konishi
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
Note: This one was relatively more complex than others, so I would
appreciate review and testing. Basic checks of mounts, various mount
options, and snapshot mounts do seem to work. I may well have missed
something though, as I am not very familiar with nilfs.
You may want to look closely at the handling of case Opt_err: which
no longer uses nilfs_write_opt() and open-codes the flag change, so
that I can use the enum. If you'd prefer to make 3 independent
Opt_err_XXXZ cases, that would be possible.
If any of the other changes here are unclear, or problematic, please
let me know.
Thanks!
-Eric
diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
index 98cffaf0ac12..46d8d628f739 100644
--- a/fs/nilfs2/nilfs.h
+++ b/fs/nilfs2/nilfs.h
@@ -334,8 +334,8 @@ void __nilfs_error(struct super_block *sb, const char *function,
extern struct nilfs_super_block *
nilfs_read_super_block(struct super_block *, u64, int, struct buffer_head **);
-extern int nilfs_store_magic_and_option(struct super_block *,
- struct nilfs_super_block *, char *);
+extern int nilfs_store_magic(struct super_block *,
+ struct nilfs_super_block *);
extern int nilfs_check_feature_compatibility(struct super_block *,
struct nilfs_super_block *);
extern void nilfs_set_log_cursor(struct nilfs_super_block *,
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index df8674173b22..deaca772d1c7 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -29,7 +29,8 @@
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/blkdev.h>
-#include <linux/parser.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
#include <linux/crc32.h>
#include <linux/vfs.h>
#include <linux/writeback.h>
@@ -61,7 +62,6 @@ struct kmem_cache *nilfs_segbuf_cachep;
struct kmem_cache *nilfs_btree_path_cache;
static int nilfs_setup_super(struct super_block *sb, int is_mount);
-static int nilfs_remount(struct super_block *sb, int *flags, char *data);
void __nilfs_msg(struct super_block *sb, const char *fmt, ...)
{
@@ -723,105 +723,95 @@ static const struct super_operations nilfs_sops = {
.freeze_fs = nilfs_freeze,
.unfreeze_fs = nilfs_unfreeze,
.statfs = nilfs_statfs,
- .remount_fs = nilfs_remount,
.show_options = nilfs_show_options
};
enum {
- Opt_err_cont, Opt_err_panic, Opt_err_ro,
- Opt_barrier, Opt_nobarrier, Opt_snapshot, Opt_order, Opt_norecovery,
- Opt_discard, Opt_nodiscard, Opt_err,
+ Opt_err, Opt_barrier, Opt_snapshot, Opt_order, Opt_norecovery,
+ Opt_discard,
};
-static match_table_t tokens = {
- {Opt_err_cont, "errors=continue"},
- {Opt_err_panic, "errors=panic"},
- {Opt_err_ro, "errors=remount-ro"},
- {Opt_barrier, "barrier"},
- {Opt_nobarrier, "nobarrier"},
- {Opt_snapshot, "cp=%u"},
- {Opt_order, "order=%s"},
- {Opt_norecovery, "norecovery"},
- {Opt_discard, "discard"},
- {Opt_nodiscard, "nodiscard"},
- {Opt_err, NULL}
+static const struct constant_table nilfs_param_err[] = {
+ {"continue", NILFS_MOUNT_ERRORS_CONT},
+ {"panic", NILFS_MOUNT_ERRORS_PANIC},
+ {"remount-ro", NILFS_MOUNT_ERRORS_RO},
+ {}
};
-static int parse_options(char *options, struct super_block *sb, int is_remount)
-{
- struct the_nilfs *nilfs = sb->s_fs_info;
- char *p;
- substring_t args[MAX_OPT_ARGS];
-
- if (!options)
- return 1;
-
- while ((p = strsep(&options, ",")) != NULL) {
- int token;
+const struct fs_parameter_spec nilfs_param_spec[] = {
+ fsparam_enum ("errors", Opt_err, nilfs_param_err),
+ fsparam_flag_no ("barrier", Opt_barrier),
+ fsparam_u32 ("cp", Opt_snapshot),
+ fsparam_string ("order", Opt_order),
+ fsparam_flag ("norecovery", Opt_norecovery),
+ fsparam_flag_no ("discard", Opt_discard),
+ {}
+};
- if (!*p)
- continue;
+struct nilfs_fs_context {
+ unsigned long ns_mount_opt;
+ __u64 cno;
+};
- token = match_token(p, tokens, args);
- switch (token) {
- case Opt_barrier:
- nilfs_set_opt(nilfs, BARRIER);
- break;
- case Opt_nobarrier:
+static int nilfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
+{
+ struct nilfs_fs_context *nilfs = fc->fs_private;
+ int is_remount = fc->purpose == FS_CONTEXT_FOR_RECONFIGURE;
+ struct fs_parse_result result;
+ int opt;
+
+ opt = fs_parse(fc, nilfs_param_spec, param, &result);
+ if (opt < 0)
+ return opt;
+
+ switch (opt) {
+ case Opt_barrier:
+ if (result.negated)
nilfs_clear_opt(nilfs, BARRIER);
- break;
- case Opt_order:
- if (strcmp(args[0].from, "relaxed") == 0)
- /* Ordered data semantics */
- nilfs_clear_opt(nilfs, STRICT_ORDER);
- else if (strcmp(args[0].from, "strict") == 0)
- /* Strict in-order semantics */
- nilfs_set_opt(nilfs, STRICT_ORDER);
- else
- return 0;
- break;
- case Opt_err_panic:
- nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_PANIC);
- break;
- case Opt_err_ro:
- nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_RO);
- break;
- case Opt_err_cont:
- nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_CONT);
- break;
- case Opt_snapshot:
- if (is_remount) {
- nilfs_err(sb,
- "\"%s\" option is invalid for remount",
- p);
- return 0;
- }
- break;
- case Opt_norecovery:
- nilfs_set_opt(nilfs, NORECOVERY);
- break;
- case Opt_discard:
- nilfs_set_opt(nilfs, DISCARD);
- break;
- case Opt_nodiscard:
- nilfs_clear_opt(nilfs, DISCARD);
- break;
- default:
- nilfs_err(sb, "unrecognized mount option \"%s\"", p);
- return 0;
+ else
+ nilfs_set_opt(nilfs, BARRIER);
+ break;
+ case Opt_order:
+ if (strcmp(param->string, "relaxed") == 0)
+ /* Ordered data semantics */
+ nilfs_clear_opt(nilfs, STRICT_ORDER);
+ else if (strcmp(param->string, "strict") == 0)
+ /* Strict in-order semantics */
+ nilfs_set_opt(nilfs, STRICT_ORDER);
+ else
+ return -EINVAL;
+ break;
+ case Opt_err:
+ nilfs->ns_mount_opt &= ~NILFS_MOUNT_ERROR_MODE;
+ nilfs->ns_mount_opt |= result.uint_32;
+ break;
+ case Opt_snapshot:
+ /* in old API was handled elsewhere */
+ if (is_remount) {
+ struct super_block *sb = fc->root->d_sb;
+ nilfs_err(sb,
+ "\"%s\" option is invalid for remount",
+ param->key);
+ return -EINVAL;
}
+ if (result.uint_64 == 0)
+ return -EINVAL;
+ nilfs->cno = result.uint_64;
+ break;
+ case Opt_norecovery:
+ nilfs_set_opt(nilfs, NORECOVERY);
+ break;
+ case Opt_discard:
+ if (result.negated)
+ nilfs_clear_opt(nilfs, DISCARD);
+ else
+ nilfs_set_opt(nilfs, DISCARD);
+ break;
+ default:
+ return -EINVAL;
}
- return 1;
-}
-static inline void
-nilfs_set_default_options(struct super_block *sb,
- struct nilfs_super_block *sbp)
-{
- struct the_nilfs *nilfs = sb->s_fs_info;
-
- nilfs->ns_mount_opt =
- NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
+ return 0;
}
static int nilfs_setup_super(struct super_block *sb, int is_mount)
@@ -878,9 +868,8 @@ struct nilfs_super_block *nilfs_read_super_block(struct super_block *sb,
return (struct nilfs_super_block *)((char *)(*pbh)->b_data + offset);
}
-int nilfs_store_magic_and_option(struct super_block *sb,
- struct nilfs_super_block *sbp,
- char *data)
+int nilfs_store_magic(struct super_block *sb,
+ struct nilfs_super_block *sbp)
{
struct the_nilfs *nilfs = sb->s_fs_info;
@@ -891,14 +880,12 @@ int nilfs_store_magic_and_option(struct super_block *sb,
sb->s_flags |= SB_NOATIME;
#endif
- nilfs_set_default_options(sb, sbp);
-
nilfs->ns_resuid = le16_to_cpu(sbp->s_def_resuid);
nilfs->ns_resgid = le16_to_cpu(sbp->s_def_resgid);
nilfs->ns_interval = le32_to_cpu(sbp->s_c_interval);
nilfs->ns_watermark = le32_to_cpu(sbp->s_c_block_max);
- return !parse_options(data, sb, 0) ? -EINVAL : 0;
+ return 0;
}
int nilfs_check_feature_compatibility(struct super_block *sb,
@@ -1063,10 +1050,11 @@ int nilfs_checkpoint_is_mounted(struct super_block *sb, __u64 cno)
* So, the recovery process is protected from other simultaneous mounts.
*/
static int
-nilfs_fill_super(struct super_block *sb, void *data, int silent)
+nilfs_fill_super(struct super_block *sb, struct fs_context *fc)
{
struct the_nilfs *nilfs;
struct nilfs_root *fsroot;
+ struct nilfs_fs_context *ctx = fc->fs_private;
__u64 cno;
int err;
@@ -1076,10 +1064,13 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
sb->s_fs_info = nilfs;
- err = init_nilfs(nilfs, sb, (char *)data);
+ err = init_nilfs(nilfs, sb);
if (err)
goto failed_nilfs;
+ /* Copy in parsed mount options */
+ nilfs->ns_mount_opt = ctx->ns_mount_opt;
+
sb->s_op = &nilfs_sops;
sb->s_export_op = &nilfs_export_ops;
sb->s_root = NULL;
@@ -1138,21 +1129,15 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
return err;
}
-static int nilfs_remount(struct super_block *sb, int *flags, char *data)
+static int nilfs_reconfigure(struct fs_context *fc)
{
+ struct nilfs_fs_context *ctx = fc->fs_private;
+ struct super_block *sb = fc->root->d_sb;
struct the_nilfs *nilfs = sb->s_fs_info;
- unsigned long old_sb_flags;
- unsigned long old_mount_opt;
int err;
sync_filesystem(sb);
- old_sb_flags = sb->s_flags;
- old_mount_opt = nilfs->ns_mount_opt;
- if (!parse_options(data, sb, 1)) {
- err = -EINVAL;
- goto restore_opts;
- }
sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
err = -EINVAL;
@@ -1160,12 +1145,14 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
if (!nilfs_valid_fs(nilfs)) {
nilfs_warn(sb,
"couldn't remount because the filesystem is in an incomplete recovery state");
- goto restore_opts;
+ goto ignore_opts;
}
- if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
+ /* readonly unchanged */
+ if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
goto out;
- if (*flags & SB_RDONLY) {
+
+ if (fc->sb_flags & SB_RDONLY) {
sb->s_flags |= SB_RDONLY;
/*
@@ -1193,7 +1180,7 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
"couldn't remount RDWR because of unsupported optional features (%llx)",
(unsigned long long)features);
err = -EROFS;
- goto restore_opts;
+ goto ignore_opts;
}
sb->s_flags &= ~SB_RDONLY;
@@ -1201,130 +1188,57 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
root = NILFS_I(d_inode(sb->s_root))->i_root;
err = nilfs_attach_log_writer(sb, root);
if (err)
- goto restore_opts;
+ goto ignore_opts;
down_write(&nilfs->ns_sem);
nilfs_setup_super(sb, true);
up_write(&nilfs->ns_sem);
}
out:
- return 0;
-
- restore_opts:
- sb->s_flags = old_sb_flags;
- nilfs->ns_mount_opt = old_mount_opt;
- return err;
-}
-
-struct nilfs_super_data {
- __u64 cno;
- int flags;
-};
-
-static int nilfs_parse_snapshot_option(const char *option,
- const substring_t *arg,
- struct nilfs_super_data *sd)
-{
- unsigned long long val;
- const char *msg = NULL;
- int err;
-
- if (!(sd->flags & SB_RDONLY)) {
- msg = "read-only option is not specified";
- goto parse_error;
- }
-
- err = kstrtoull(arg->from, 0, &val);
- if (err) {
- if (err == -ERANGE)
- msg = "too large checkpoint number";
- else
- msg = "malformed argument";
- goto parse_error;
- } else if (val == 0) {
- msg = "invalid checkpoint number 0";
- goto parse_error;
- }
- sd->cno = val;
- return 0;
-
-parse_error:
- nilfs_err(NULL, "invalid option \"%s\": %s", option, msg);
- return 1;
-}
-
-/**
- * nilfs_identify - pre-read mount options needed to identify mount instance
- * @data: mount options
- * @sd: nilfs_super_data
- */
-static int nilfs_identify(char *data, struct nilfs_super_data *sd)
-{
- char *p, *options = data;
- substring_t args[MAX_OPT_ARGS];
- int token;
- int ret = 0;
-
- do {
- p = strsep(&options, ",");
- if (p != NULL && *p) {
- token = match_token(p, tokens, args);
- if (token == Opt_snapshot)
- ret = nilfs_parse_snapshot_option(p, &args[0],
- sd);
- }
- if (!options)
- break;
- BUG_ON(options == data);
- *(options - 1) = ',';
- } while (!ret);
- return ret;
-}
+ sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
+ /* Copy over parsed remount options */
+ nilfs->ns_mount_opt = ctx->ns_mount_opt;
-static int nilfs_set_bdev_super(struct super_block *s, void *data)
-{
- s->s_dev = *(dev_t *)data;
return 0;
-}
-static int nilfs_test_bdev_super(struct super_block *s, void *data)
-{
- return !(s->s_iflags & SB_I_RETIRED) && s->s_dev == *(dev_t *)data;
+ ignore_opts:
+ return err;
}
-static struct dentry *
-nilfs_mount(struct file_system_type *fs_type, int flags,
- const char *dev_name, void *data)
+static int
+nilfs_get_tree(struct fs_context *fc)
{
- struct nilfs_super_data sd = { .flags = flags };
+ struct nilfs_fs_context *ctx = fc->fs_private;
struct super_block *s;
dev_t dev;
int err;
- if (nilfs_identify(data, &sd))
- return ERR_PTR(-EINVAL);
+ if (ctx->cno && !(fc->sb_flags & SB_RDONLY)) {
+ nilfs_err(s, "invalid option \"cn=%llu\", "
+ "read-only option is not specified",
+ ctx->cno);
+ return -EINVAL;
+ }
- err = lookup_bdev(dev_name, &dev);
+ err = lookup_bdev(fc->source, &dev);
if (err)
- return ERR_PTR(err);
+ return err;
- s = sget(fs_type, nilfs_test_bdev_super, nilfs_set_bdev_super, flags,
- &dev);
+ s = sget_dev(fc, dev);
if (IS_ERR(s))
- return ERR_CAST(s);
+ return PTR_ERR(s);
if (!s->s_root) {
- err = setup_bdev_super(s, flags, NULL);
+ err = setup_bdev_super(s, fc->sb_flags, fc);
if (!err)
- err = nilfs_fill_super(s, data,
- flags & SB_SILENT ? 1 : 0);
+ err = nilfs_fill_super(s, fc);
if (err)
goto failed_super;
s->s_flags |= SB_ACTIVE;
- } else if (!sd.cno) {
+ } else if (!ctx->cno) {
if (nilfs_tree_is_busy(s->s_root)) {
- if ((flags ^ s->s_flags) & SB_RDONLY) {
+ if ((fc->sb_flags ^ s->s_flags) & SB_RDONLY) {
nilfs_err(s,
"the device already has a %s mount.",
sb_rdonly(s) ? "read-only" : "read/write");
@@ -1336,34 +1250,64 @@ nilfs_mount(struct file_system_type *fs_type, int flags,
* Try remount to setup mount states if the current
* tree is not mounted and only snapshots use this sb.
*/
- err = nilfs_remount(s, &flags, data);
+ err = nilfs_reconfigure(fc);
if (err)
goto failed_super;
}
}
- if (sd.cno) {
+ if (ctx->cno) {
struct dentry *root_dentry;
- err = nilfs_attach_snapshot(s, sd.cno, &root_dentry);
+ err = nilfs_attach_snapshot(s, ctx->cno, &root_dentry);
if (err)
goto failed_super;
- return root_dentry;
+ fc->root = root_dentry;
+ return 0;
}
- return dget(s->s_root);
+ fc->root = dget(s->s_root);
+ return 0;
failed_super:
deactivate_locked_super(s);
- return ERR_PTR(err);
+ return err;
+}
+
+static void nilfs_free_fc(struct fs_context *fc)
+{
+ kfree(fc->fs_private);
+}
+
+static const struct fs_context_operations nilfs_context_ops = {
+ .parse_param = nilfs_parse_param,
+ .get_tree = nilfs_get_tree,
+ .reconfigure = nilfs_reconfigure,
+ .free = nilfs_free_fc,
+};
+
+static int nilfs_init_fs_context(struct fs_context *fc)
+{
+ struct nilfs_fs_context *ctx;
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->ns_mount_opt = NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
+ fc->fs_private = ctx;
+ fc->ops = &nilfs_context_ops;
+
+ return 0;
}
struct file_system_type nilfs_fs_type = {
.owner = THIS_MODULE,
.name = "nilfs2",
- .mount = nilfs_mount,
.kill_sb = kill_block_super,
.fs_flags = FS_REQUIRES_DEV,
+ .init_fs_context = nilfs_init_fs_context,
+ .parameters = nilfs_param_spec,
};
MODULE_ALIAS_FS("nilfs2");
diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 71400496ed36..894d9a513eed 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -668,7 +668,7 @@ static int nilfs_load_super_block(struct the_nilfs *nilfs,
* Return Value: On success, 0 is returned. On error, a negative error
* code is returned.
*/
-int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
+int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb)
{
struct nilfs_super_block *sbp;
int blocksize;
@@ -686,7 +686,7 @@ int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
if (err)
goto out;
- err = nilfs_store_magic_and_option(sb, sbp, data);
+ err = nilfs_store_magic(sb, sbp);
if (err)
goto failed_sbh;
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index cd4ae1b8ae16..85da0629415d 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -219,10 +219,6 @@ THE_NILFS_FNS(PURGING, purging)
#define nilfs_set_opt(nilfs, opt) \
((nilfs)->ns_mount_opt |= NILFS_MOUNT_##opt)
#define nilfs_test_opt(nilfs, opt) ((nilfs)->ns_mount_opt & NILFS_MOUNT_##opt)
-#define nilfs_write_opt(nilfs, mask, opt) \
- ((nilfs)->ns_mount_opt = \
- (((nilfs)->ns_mount_opt & ~NILFS_MOUNT_##mask) | \
- NILFS_MOUNT_##opt)) \
/**
* struct nilfs_root - nilfs root object
@@ -276,7 +272,7 @@ static inline int nilfs_sb_will_flip(struct the_nilfs *nilfs)
void nilfs_set_last_segment(struct the_nilfs *, sector_t, u64, __u64);
struct the_nilfs *alloc_nilfs(struct super_block *sb);
void destroy_nilfs(struct the_nilfs *nilfs);
-int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data);
+int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
int load_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
unsigned long nilfs_nrsvsegs(struct the_nilfs *nilfs, unsigned long nsegs);
void nilfs_set_nsegments(struct the_nilfs *nilfs, unsigned long nsegs);
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] nilfs2: convert to use the new mount API
2024-04-03 22:12 [PATCH] nilfs2: convert to use the new mount API Eric Sandeen
@ 2024-04-04 20:11 ` Ryusuke Konishi
2024-04-04 20:35 ` Eric Sandeen
0 siblings, 1 reply; 16+ messages in thread
From: Ryusuke Konishi @ 2024-04-04 20:11 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-nilfs
[-- Attachment #1: Type: text/plain, Size: 25090 bytes --]
On Thu, Apr 4, 2024 at 7:12 AM Eric Sandeen wrote:
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> Note: This one was relatively more complex than others, so I would
> appreciate review and testing. Basic checks of mounts, various mount
> options, and snapshot mounts do seem to work. I may well have missed
> something though, as I am not very familiar with nilfs.
>
> You may want to look closely at the handling of case Opt_err: which
> no longer uses nilfs_write_opt() and open-codes the flag change, so
> that I can use the enum. If you'd prefer to make 3 independent
> Opt_err_XXXZ cases, that would be possible.
>
> If any of the other changes here are unclear, or problematic, please
> let me know.
>
> Thanks!
> -Eric
Hi Eric,
Thank you! This is one of the modernizations that I thought I had to
do with nilfs2.
I'm planning on doing a full review later, but when I ran a mount
pattern test, the kernel restarted without any messages (probably
caused a panic), so I'll give you some quick feedback.
The mount pattern that caused the kernel to restart was a simultaneous
mount of the current tree and a snapshot, which occurred when the
snapshot was mounted and then the current tree was mounted. Something
like below:
$ sudo losetup /dev/loop0 ./nilfs.iso
$ sudo mount -t nilfs2 -o ro,cp=38866 /dev/loop0 /mnt/snapshot
$ sudo mount -t nilfs2 /dev/loop0 /mnt/test
--> panic
Here, 38866 is the snapshot number that can be created with the
nilfs-utils "mkcp -s" command or "chcp" command, and the number can be
checked with "lscp -s".
I have placed the mount test script I used in the following location:
https://github.com/konis/nilfs-test-tools/blob/main/test-nilfs-mount.sh
The panic occurred in test #17 of that script.
I'll also try to track what's going on.
Thanks,
Ryusuke Konishi
>
> diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
> index 98cffaf0ac12..46d8d628f739 100644
> --- a/fs/nilfs2/nilfs.h
> +++ b/fs/nilfs2/nilfs.h
> @@ -334,8 +334,8 @@ void __nilfs_error(struct super_block *sb, const char *function,
>
> extern struct nilfs_super_block *
> nilfs_read_super_block(struct super_block *, u64, int, struct buffer_head **);
> -extern int nilfs_store_magic_and_option(struct super_block *,
> - struct nilfs_super_block *, char *);
> +extern int nilfs_store_magic(struct super_block *,
> + struct nilfs_super_block *);
> extern int nilfs_check_feature_compatibility(struct super_block *,
> struct nilfs_super_block *);
> extern void nilfs_set_log_cursor(struct nilfs_super_block *,
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index df8674173b22..deaca772d1c7 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -29,7 +29,8 @@
> #include <linux/slab.h>
> #include <linux/init.h>
> #include <linux/blkdev.h>
> -#include <linux/parser.h>
> +#include <linux/fs_context.h>
> +#include <linux/fs_parser.h>
> #include <linux/crc32.h>
> #include <linux/vfs.h>
> #include <linux/writeback.h>
> @@ -61,7 +62,6 @@ struct kmem_cache *nilfs_segbuf_cachep;
> struct kmem_cache *nilfs_btree_path_cache;
>
> static int nilfs_setup_super(struct super_block *sb, int is_mount);
> -static int nilfs_remount(struct super_block *sb, int *flags, char *data);
>
> void __nilfs_msg(struct super_block *sb, const char *fmt, ...)
> {
> @@ -723,105 +723,95 @@ static const struct super_operations nilfs_sops = {
> .freeze_fs = nilfs_freeze,
> .unfreeze_fs = nilfs_unfreeze,
> .statfs = nilfs_statfs,
> - .remount_fs = nilfs_remount,
> .show_options = nilfs_show_options
> };
>
> enum {
> - Opt_err_cont, Opt_err_panic, Opt_err_ro,
> - Opt_barrier, Opt_nobarrier, Opt_snapshot, Opt_order, Opt_norecovery,
> - Opt_discard, Opt_nodiscard, Opt_err,
> + Opt_err, Opt_barrier, Opt_snapshot, Opt_order, Opt_norecovery,
> + Opt_discard,
> };
>
> -static match_table_t tokens = {
> - {Opt_err_cont, "errors=continue"},
> - {Opt_err_panic, "errors=panic"},
> - {Opt_err_ro, "errors=remount-ro"},
> - {Opt_barrier, "barrier"},
> - {Opt_nobarrier, "nobarrier"},
> - {Opt_snapshot, "cp=%u"},
> - {Opt_order, "order=%s"},
> - {Opt_norecovery, "norecovery"},
> - {Opt_discard, "discard"},
> - {Opt_nodiscard, "nodiscard"},
> - {Opt_err, NULL}
> +static const struct constant_table nilfs_param_err[] = {
> + {"continue", NILFS_MOUNT_ERRORS_CONT},
> + {"panic", NILFS_MOUNT_ERRORS_PANIC},
> + {"remount-ro", NILFS_MOUNT_ERRORS_RO},
> + {}
> };
>
> -static int parse_options(char *options, struct super_block *sb, int is_remount)
> -{
> - struct the_nilfs *nilfs = sb->s_fs_info;
> - char *p;
> - substring_t args[MAX_OPT_ARGS];
> -
> - if (!options)
> - return 1;
> -
> - while ((p = strsep(&options, ",")) != NULL) {
> - int token;
> +const struct fs_parameter_spec nilfs_param_spec[] = {
> + fsparam_enum ("errors", Opt_err, nilfs_param_err),
> + fsparam_flag_no ("barrier", Opt_barrier),
> + fsparam_u32 ("cp", Opt_snapshot),
> + fsparam_string ("order", Opt_order),
> + fsparam_flag ("norecovery", Opt_norecovery),
> + fsparam_flag_no ("discard", Opt_discard),
> + {}
> +};
>
> - if (!*p)
> - continue;
> +struct nilfs_fs_context {
> + unsigned long ns_mount_opt;
> + __u64 cno;
> +};
>
> - token = match_token(p, tokens, args);
> - switch (token) {
> - case Opt_barrier:
> - nilfs_set_opt(nilfs, BARRIER);
> - break;
> - case Opt_nobarrier:
> +static int nilfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> +{
> + struct nilfs_fs_context *nilfs = fc->fs_private;
> + int is_remount = fc->purpose == FS_CONTEXT_FOR_RECONFIGURE;
> + struct fs_parse_result result;
> + int opt;
> +
> + opt = fs_parse(fc, nilfs_param_spec, param, &result);
> + if (opt < 0)
> + return opt;
> +
> + switch (opt) {
> + case Opt_barrier:
> + if (result.negated)
> nilfs_clear_opt(nilfs, BARRIER);
> - break;
> - case Opt_order:
> - if (strcmp(args[0].from, "relaxed") == 0)
> - /* Ordered data semantics */
> - nilfs_clear_opt(nilfs, STRICT_ORDER);
> - else if (strcmp(args[0].from, "strict") == 0)
> - /* Strict in-order semantics */
> - nilfs_set_opt(nilfs, STRICT_ORDER);
> - else
> - return 0;
> - break;
> - case Opt_err_panic:
> - nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_PANIC);
> - break;
> - case Opt_err_ro:
> - nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_RO);
> - break;
> - case Opt_err_cont:
> - nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_CONT);
> - break;
> - case Opt_snapshot:
> - if (is_remount) {
> - nilfs_err(sb,
> - "\"%s\" option is invalid for remount",
> - p);
> - return 0;
> - }
> - break;
> - case Opt_norecovery:
> - nilfs_set_opt(nilfs, NORECOVERY);
> - break;
> - case Opt_discard:
> - nilfs_set_opt(nilfs, DISCARD);
> - break;
> - case Opt_nodiscard:
> - nilfs_clear_opt(nilfs, DISCARD);
> - break;
> - default:
> - nilfs_err(sb, "unrecognized mount option \"%s\"", p);
> - return 0;
> + else
> + nilfs_set_opt(nilfs, BARRIER);
> + break;
> + case Opt_order:
> + if (strcmp(param->string, "relaxed") == 0)
> + /* Ordered data semantics */
> + nilfs_clear_opt(nilfs, STRICT_ORDER);
> + else if (strcmp(param->string, "strict") == 0)
> + /* Strict in-order semantics */
> + nilfs_set_opt(nilfs, STRICT_ORDER);
> + else
> + return -EINVAL;
> + break;
> + case Opt_err:
> + nilfs->ns_mount_opt &= ~NILFS_MOUNT_ERROR_MODE;
> + nilfs->ns_mount_opt |= result.uint_32;
> + break;
> + case Opt_snapshot:
> + /* in old API was handled elsewhere */
> + if (is_remount) {
> + struct super_block *sb = fc->root->d_sb;
> + nilfs_err(sb,
> + "\"%s\" option is invalid for remount",
> + param->key);
> + return -EINVAL;
> }
> + if (result.uint_64 == 0)
> + return -EINVAL;
> + nilfs->cno = result.uint_64;
> + break;
> + case Opt_norecovery:
> + nilfs_set_opt(nilfs, NORECOVERY);
> + break;
> + case Opt_discard:
> + if (result.negated)
> + nilfs_clear_opt(nilfs, DISCARD);
> + else
> + nilfs_set_opt(nilfs, DISCARD);
> + break;
> + default:
> + return -EINVAL;
> }
> - return 1;
> -}
>
> -static inline void
> -nilfs_set_default_options(struct super_block *sb,
> - struct nilfs_super_block *sbp)
> -{
> - struct the_nilfs *nilfs = sb->s_fs_info;
> -
> - nilfs->ns_mount_opt =
> - NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
> + return 0;
> }
>
> static int nilfs_setup_super(struct super_block *sb, int is_mount)
> @@ -878,9 +868,8 @@ struct nilfs_super_block *nilfs_read_super_block(struct super_block *sb,
> return (struct nilfs_super_block *)((char *)(*pbh)->b_data + offset);
> }
>
> -int nilfs_store_magic_and_option(struct super_block *sb,
> - struct nilfs_super_block *sbp,
> - char *data)
> +int nilfs_store_magic(struct super_block *sb,
> + struct nilfs_super_block *sbp)
> {
> struct the_nilfs *nilfs = sb->s_fs_info;
>
> @@ -891,14 +880,12 @@ int nilfs_store_magic_and_option(struct super_block *sb,
> sb->s_flags |= SB_NOATIME;
> #endif
>
> - nilfs_set_default_options(sb, sbp);
> -
> nilfs->ns_resuid = le16_to_cpu(sbp->s_def_resuid);
> nilfs->ns_resgid = le16_to_cpu(sbp->s_def_resgid);
> nilfs->ns_interval = le32_to_cpu(sbp->s_c_interval);
> nilfs->ns_watermark = le32_to_cpu(sbp->s_c_block_max);
>
> - return !parse_options(data, sb, 0) ? -EINVAL : 0;
> + return 0;
> }
>
> int nilfs_check_feature_compatibility(struct super_block *sb,
> @@ -1063,10 +1050,11 @@ int nilfs_checkpoint_is_mounted(struct super_block *sb, __u64 cno)
> * So, the recovery process is protected from other simultaneous mounts.
> */
> static int
> -nilfs_fill_super(struct super_block *sb, void *data, int silent)
> +nilfs_fill_super(struct super_block *sb, struct fs_context *fc)
> {
> struct the_nilfs *nilfs;
> struct nilfs_root *fsroot;
> + struct nilfs_fs_context *ctx = fc->fs_private;
> __u64 cno;
> int err;
>
> @@ -1076,10 +1064,13 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
>
> sb->s_fs_info = nilfs;
>
> - err = init_nilfs(nilfs, sb, (char *)data);
> + err = init_nilfs(nilfs, sb);
> if (err)
> goto failed_nilfs;
>
> + /* Copy in parsed mount options */
> + nilfs->ns_mount_opt = ctx->ns_mount_opt;
> +
> sb->s_op = &nilfs_sops;
> sb->s_export_op = &nilfs_export_ops;
> sb->s_root = NULL;
> @@ -1138,21 +1129,15 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
> return err;
> }
>
> -static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> +static int nilfs_reconfigure(struct fs_context *fc)
> {
> + struct nilfs_fs_context *ctx = fc->fs_private;
> + struct super_block *sb = fc->root->d_sb;
> struct the_nilfs *nilfs = sb->s_fs_info;
> - unsigned long old_sb_flags;
> - unsigned long old_mount_opt;
> int err;
>
> sync_filesystem(sb);
> - old_sb_flags = sb->s_flags;
> - old_mount_opt = nilfs->ns_mount_opt;
>
> - if (!parse_options(data, sb, 1)) {
> - err = -EINVAL;
> - goto restore_opts;
> - }
> sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
>
> err = -EINVAL;
> @@ -1160,12 +1145,14 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> if (!nilfs_valid_fs(nilfs)) {
> nilfs_warn(sb,
> "couldn't remount because the filesystem is in an incomplete recovery state");
> - goto restore_opts;
> + goto ignore_opts;
> }
>
> - if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
> + /* readonly unchanged */
> + if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
> goto out;
> - if (*flags & SB_RDONLY) {
> +
> + if (fc->sb_flags & SB_RDONLY) {
> sb->s_flags |= SB_RDONLY;
>
> /*
> @@ -1193,7 +1180,7 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> "couldn't remount RDWR because of unsupported optional features (%llx)",
> (unsigned long long)features);
> err = -EROFS;
> - goto restore_opts;
> + goto ignore_opts;
> }
>
> sb->s_flags &= ~SB_RDONLY;
> @@ -1201,130 +1188,57 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> root = NILFS_I(d_inode(sb->s_root))->i_root;
> err = nilfs_attach_log_writer(sb, root);
> if (err)
> - goto restore_opts;
> + goto ignore_opts;
>
> down_write(&nilfs->ns_sem);
> nilfs_setup_super(sb, true);
> up_write(&nilfs->ns_sem);
> }
> out:
> - return 0;
> -
> - restore_opts:
> - sb->s_flags = old_sb_flags;
> - nilfs->ns_mount_opt = old_mount_opt;
> - return err;
> -}
> -
> -struct nilfs_super_data {
> - __u64 cno;
> - int flags;
> -};
> -
> -static int nilfs_parse_snapshot_option(const char *option,
> - const substring_t *arg,
> - struct nilfs_super_data *sd)
> -{
> - unsigned long long val;
> - const char *msg = NULL;
> - int err;
> -
> - if (!(sd->flags & SB_RDONLY)) {
> - msg = "read-only option is not specified";
> - goto parse_error;
> - }
> -
> - err = kstrtoull(arg->from, 0, &val);
> - if (err) {
> - if (err == -ERANGE)
> - msg = "too large checkpoint number";
> - else
> - msg = "malformed argument";
> - goto parse_error;
> - } else if (val == 0) {
> - msg = "invalid checkpoint number 0";
> - goto parse_error;
> - }
> - sd->cno = val;
> - return 0;
> -
> -parse_error:
> - nilfs_err(NULL, "invalid option \"%s\": %s", option, msg);
> - return 1;
> -}
> -
> -/**
> - * nilfs_identify - pre-read mount options needed to identify mount instance
> - * @data: mount options
> - * @sd: nilfs_super_data
> - */
> -static int nilfs_identify(char *data, struct nilfs_super_data *sd)
> -{
> - char *p, *options = data;
> - substring_t args[MAX_OPT_ARGS];
> - int token;
> - int ret = 0;
> -
> - do {
> - p = strsep(&options, ",");
> - if (p != NULL && *p) {
> - token = match_token(p, tokens, args);
> - if (token == Opt_snapshot)
> - ret = nilfs_parse_snapshot_option(p, &args[0],
> - sd);
> - }
> - if (!options)
> - break;
> - BUG_ON(options == data);
> - *(options - 1) = ',';
> - } while (!ret);
> - return ret;
> -}
> + sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
> + /* Copy over parsed remount options */
> + nilfs->ns_mount_opt = ctx->ns_mount_opt;
>
> -static int nilfs_set_bdev_super(struct super_block *s, void *data)
> -{
> - s->s_dev = *(dev_t *)data;
> return 0;
> -}
>
> -static int nilfs_test_bdev_super(struct super_block *s, void *data)
> -{
> - return !(s->s_iflags & SB_I_RETIRED) && s->s_dev == *(dev_t *)data;
> + ignore_opts:
> + return err;
> }
>
> -static struct dentry *
> -nilfs_mount(struct file_system_type *fs_type, int flags,
> - const char *dev_name, void *data)
> +static int
> +nilfs_get_tree(struct fs_context *fc)
> {
> - struct nilfs_super_data sd = { .flags = flags };
> + struct nilfs_fs_context *ctx = fc->fs_private;
> struct super_block *s;
> dev_t dev;
> int err;
>
> - if (nilfs_identify(data, &sd))
> - return ERR_PTR(-EINVAL);
> + if (ctx->cno && !(fc->sb_flags & SB_RDONLY)) {
> + nilfs_err(s, "invalid option \"cn=%llu\", "
> + "read-only option is not specified",
> + ctx->cno);
> + return -EINVAL;
> + }
>
> - err = lookup_bdev(dev_name, &dev);
> + err = lookup_bdev(fc->source, &dev);
> if (err)
> - return ERR_PTR(err);
> + return err;
>
> - s = sget(fs_type, nilfs_test_bdev_super, nilfs_set_bdev_super, flags,
> - &dev);
> + s = sget_dev(fc, dev);
> if (IS_ERR(s))
> - return ERR_CAST(s);
> + return PTR_ERR(s);
>
> if (!s->s_root) {
> - err = setup_bdev_super(s, flags, NULL);
> + err = setup_bdev_super(s, fc->sb_flags, fc);
> if (!err)
> - err = nilfs_fill_super(s, data,
> - flags & SB_SILENT ? 1 : 0);
> + err = nilfs_fill_super(s, fc);
> if (err)
> goto failed_super;
>
> s->s_flags |= SB_ACTIVE;
> - } else if (!sd.cno) {
> + } else if (!ctx->cno) {
> if (nilfs_tree_is_busy(s->s_root)) {
> - if ((flags ^ s->s_flags) & SB_RDONLY) {
> + if ((fc->sb_flags ^ s->s_flags) & SB_RDONLY) {
> nilfs_err(s,
> "the device already has a %s mount.",
> sb_rdonly(s) ? "read-only" : "read/write");
> @@ -1336,34 +1250,64 @@ nilfs_mount(struct file_system_type *fs_type, int flags,
> * Try remount to setup mount states if the current
> * tree is not mounted and only snapshots use this sb.
> */
> - err = nilfs_remount(s, &flags, data);
> + err = nilfs_reconfigure(fc);
> if (err)
> goto failed_super;
> }
> }
>
> - if (sd.cno) {
> + if (ctx->cno) {
> struct dentry *root_dentry;
>
> - err = nilfs_attach_snapshot(s, sd.cno, &root_dentry);
> + err = nilfs_attach_snapshot(s, ctx->cno, &root_dentry);
> if (err)
> goto failed_super;
> - return root_dentry;
> + fc->root = root_dentry;
> + return 0;
> }
>
> - return dget(s->s_root);
> + fc->root = dget(s->s_root);
> + return 0;
>
> failed_super:
> deactivate_locked_super(s);
> - return ERR_PTR(err);
> + return err;
> +}
> +
> +static void nilfs_free_fc(struct fs_context *fc)
> +{
> + kfree(fc->fs_private);
> +}
> +
> +static const struct fs_context_operations nilfs_context_ops = {
> + .parse_param = nilfs_parse_param,
> + .get_tree = nilfs_get_tree,
> + .reconfigure = nilfs_reconfigure,
> + .free = nilfs_free_fc,
> +};
> +
> +static int nilfs_init_fs_context(struct fs_context *fc)
> +{
> + struct nilfs_fs_context *ctx;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->ns_mount_opt = NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
> + fc->fs_private = ctx;
> + fc->ops = &nilfs_context_ops;
> +
> + return 0;
> }
>
> struct file_system_type nilfs_fs_type = {
> .owner = THIS_MODULE,
> .name = "nilfs2",
> - .mount = nilfs_mount,
> .kill_sb = kill_block_super,
> .fs_flags = FS_REQUIRES_DEV,
> + .init_fs_context = nilfs_init_fs_context,
> + .parameters = nilfs_param_spec,
> };
> MODULE_ALIAS_FS("nilfs2");
>
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index 71400496ed36..894d9a513eed 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -668,7 +668,7 @@ static int nilfs_load_super_block(struct the_nilfs *nilfs,
> * Return Value: On success, 0 is returned. On error, a negative error
> * code is returned.
> */
> -int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
> +int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb)
> {
> struct nilfs_super_block *sbp;
> int blocksize;
> @@ -686,7 +686,7 @@ int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
> if (err)
> goto out;
>
> - err = nilfs_store_magic_and_option(sb, sbp, data);
> + err = nilfs_store_magic(sb, sbp);
> if (err)
> goto failed_sbh;
>
> diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
> index cd4ae1b8ae16..85da0629415d 100644
> --- a/fs/nilfs2/the_nilfs.h
> +++ b/fs/nilfs2/the_nilfs.h
> @@ -219,10 +219,6 @@ THE_NILFS_FNS(PURGING, purging)
> #define nilfs_set_opt(nilfs, opt) \
> ((nilfs)->ns_mount_opt |= NILFS_MOUNT_##opt)
> #define nilfs_test_opt(nilfs, opt) ((nilfs)->ns_mount_opt & NILFS_MOUNT_##opt)
> -#define nilfs_write_opt(nilfs, mask, opt) \
> - ((nilfs)->ns_mount_opt = \
> - (((nilfs)->ns_mount_opt & ~NILFS_MOUNT_##mask) | \
> - NILFS_MOUNT_##opt)) \
>
> /**
> * struct nilfs_root - nilfs root object
> @@ -276,7 +272,7 @@ static inline int nilfs_sb_will_flip(struct the_nilfs *nilfs)
> void nilfs_set_last_segment(struct the_nilfs *, sector_t, u64, __u64);
> struct the_nilfs *alloc_nilfs(struct super_block *sb);
> void destroy_nilfs(struct the_nilfs *nilfs);
> -int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data);
> +int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
> int load_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
> unsigned long nilfs_nrsvsegs(struct the_nilfs *nilfs, unsigned long nsegs);
> void nilfs_set_nsegments(struct the_nilfs *nilfs, unsigned long nsegs);
>
[-- Attachment #2: kconfig.gz --]
[-- Type: application/x-gzip, Size: 44962 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] nilfs2: convert to use the new mount API
2024-04-04 20:11 ` Ryusuke Konishi
@ 2024-04-04 20:35 ` Eric Sandeen
2024-04-04 22:33 ` Ryusuke Konishi
0 siblings, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2024-04-04 20:35 UTC (permalink / raw)
To: Ryusuke Konishi; +Cc: linux-nilfs
On 4/4/24 3:11 PM, Ryusuke Konishi wrote:
> On Thu, Apr 4, 2024 at 7:12 AM Eric Sandeen wrote:
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> Note: This one was relatively more complex than others, so I would
>> appreciate review and testing. Basic checks of mounts, various mount
>> options, and snapshot mounts do seem to work. I may well have missed
>> something though, as I am not very familiar with nilfs.
>>
>> You may want to look closely at the handling of case Opt_err: which
>> no longer uses nilfs_write_opt() and open-codes the flag change, so
>> that I can use the enum. If you'd prefer to make 3 independent
>> Opt_err_XXXZ cases, that would be possible.
>>
>> If any of the other changes here are unclear, or problematic, please
>> let me know.
>>
>> Thanks!
>> -Eric
>
> Hi Eric,
>
> Thank you! This is one of the modernizations that I thought I had to
> do with nilfs2.
>
> I'm planning on doing a full review later, but when I ran a mount
> pattern test, the kernel restarted without any messages (probably
> caused a panic), so I'll give you some quick feedback.
>
> The mount pattern that caused the kernel to restart was a simultaneous
> mount of the current tree and a snapshot, which occurred when the
> snapshot was mounted and then the current tree was mounted. Something
> like below:
>
> $ sudo losetup /dev/loop0 ./nilfs.iso
> $ sudo mount -t nilfs2 -o ro,cp=38866 /dev/loop0 /mnt/snapshot
> $ sudo mount -t nilfs2 /dev/loop0 /mnt/test
> --> panic
>
> Here, 38866 is the snapshot number that can be created with the
> nilfs-utils "mkcp -s" command or "chcp" command, and the number can be
> checked with "lscp -s".
>
> I have placed the mount test script I used in the following location:
>
> https://github.com/konis/nilfs-test-tools/blob/main/test-nilfs-mount.sh
>
> The panic occurred in test #17 of that script.
>
> I'll also try to track what's going on.
Thanks, I'll look - I was hoping/expecting that you had better tests for
mount options than I did! ;)
Feel free to debug if you like, but it must be a bug in my patch so
I'll take ownership of trying to track down the problem and get it to
pass your test script.
-Eric
> Thanks,
> Ryusuke Konishi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] nilfs2: convert to use the new mount API
2024-04-04 20:35 ` Eric Sandeen
@ 2024-04-04 22:33 ` Ryusuke Konishi
2024-04-05 1:21 ` Eric Sandeen
0 siblings, 1 reply; 16+ messages in thread
From: Ryusuke Konishi @ 2024-04-04 22:33 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-nilfs
On Fri, Apr 5, 2024 at 5:35 AM Eric Sandeen wrote:
>
> On 4/4/24 3:11 PM, Ryusuke Konishi wrote:
> > On Thu, Apr 4, 2024 at 7:12 AM Eric Sandeen wrote:
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> Note: This one was relatively more complex than others, so I would
> >> appreciate review and testing. Basic checks of mounts, various mount
> >> options, and snapshot mounts do seem to work. I may well have missed
> >> something though, as I am not very familiar with nilfs.
> >>
> >> You may want to look closely at the handling of case Opt_err: which
> >> no longer uses nilfs_write_opt() and open-codes the flag change, so
> >> that I can use the enum. If you'd prefer to make 3 independent
> >> Opt_err_XXXZ cases, that would be possible.
> >>
> >> If any of the other changes here are unclear, or problematic, please
> >> let me know.
> >>
> >> Thanks!
> >> -Eric
> >
> > Hi Eric,
> >
> > Thank you! This is one of the modernizations that I thought I had to
> > do with nilfs2.
> >
> > I'm planning on doing a full review later, but when I ran a mount
> > pattern test, the kernel restarted without any messages (probably
> > caused a panic), so I'll give you some quick feedback.
> >
> > The mount pattern that caused the kernel to restart was a simultaneous
> > mount of the current tree and a snapshot, which occurred when the
> > snapshot was mounted and then the current tree was mounted. Something
> > like below:
> >
> > $ sudo losetup /dev/loop0 ./nilfs.iso
> > $ sudo mount -t nilfs2 -o ro,cp=38866 /dev/loop0 /mnt/snapshot
> > $ sudo mount -t nilfs2 /dev/loop0 /mnt/test
> > --> panic
> >
> > Here, 38866 is the snapshot number that can be created with the
> > nilfs-utils "mkcp -s" command or "chcp" command, and the number can be
> > checked with "lscp -s".
> >
> > I have placed the mount test script I used in the following location:
> >
> > https://github.com/konis/nilfs-test-tools/blob/main/test-nilfs-mount.sh
> >
> > The panic occurred in test #17 of that script.
> >
> > I'll also try to track what's going on.
>
> Thanks, I'll look - I was hoping/expecting that you had better tests for
> mount options than I did! ;)
>
> Feel free to debug if you like, but it must be a bug in my patch so
> I'll take ownership of trying to track down the problem and get it to
> pass your test script.
Got it!
So I'll try to understand the patch first.
This test script focuses on reproducing NILFS-specific mount sequences
(such as mounting a snapshot and current tree simultaneously) and
checking the state of user space such as the GC process and utab.
And, is not exhaustive for mount options.
Looking at the patch, if I come up with test patterns that would be
better to add, I will enhance the test script.
Thanks,
Ryusuke Konishi
>
> -Eric
>
> > Thanks,
> > Ryusuke Konishi
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] nilfs2: convert to use the new mount API
2024-04-04 22:33 ` Ryusuke Konishi
@ 2024-04-05 1:21 ` Eric Sandeen
2024-04-05 1:46 ` Eric Sandeen
0 siblings, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2024-04-05 1:21 UTC (permalink / raw)
To: Ryusuke Konishi; +Cc: linux-nilfs
On 4/4/24 5:33 PM, Ryusuke Konishi wrote:
> On Fri, Apr 5, 2024 at 5:35 AM Eric Sandeen wrote:
>>
>> On 4/4/24 3:11 PM, Ryusuke Konishi wrote:
>>> On Thu, Apr 4, 2024 at 7:12 AM Eric Sandeen wrote:
>>>>
>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>>> ---
>>>>
>>>> Note: This one was relatively more complex than others, so I would
>>>> appreciate review and testing. Basic checks of mounts, various mount
>>>> options, and snapshot mounts do seem to work. I may well have missed
>>>> something though, as I am not very familiar with nilfs.
>>>>
>>>> You may want to look closely at the handling of case Opt_err: which
>>>> no longer uses nilfs_write_opt() and open-codes the flag change, so
>>>> that I can use the enum. If you'd prefer to make 3 independent
>>>> Opt_err_XXXZ cases, that would be possible.
>>>>
>>>> If any of the other changes here are unclear, or problematic, please
>>>> let me know.
>>>>
>>>> Thanks!
>>>> -Eric
>>>
>>> Hi Eric,
>>>
>>> Thank you! This is one of the modernizations that I thought I had to
>>> do with nilfs2.
>>>
>>> I'm planning on doing a full review later, but when I ran a mount
>>> pattern test, the kernel restarted without any messages (probably
>>> caused a panic), so I'll give you some quick feedback.
>>>
>>> The mount pattern that caused the kernel to restart was a simultaneous
>>> mount of the current tree and a snapshot, which occurred when the
>>> snapshot was mounted and then the current tree was mounted. Something
>>> like below:
>>>
>>> $ sudo losetup /dev/loop0 ./nilfs.iso
>>> $ sudo mount -t nilfs2 -o ro,cp=38866 /dev/loop0 /mnt/snapshot
>>> $ sudo mount -t nilfs2 /dev/loop0 /mnt/test
>>> --> panic
>>>
>>> Here, 38866 is the snapshot number that can be created with the
>>> nilfs-utils "mkcp -s" command or "chcp" command, and the number can be
>>> checked with "lscp -s".
>>>
>>> I have placed the mount test script I used in the following location:
>>>
>>> https://github.com/konis/nilfs-test-tools/blob/main/test-nilfs-mount.sh
>>>
>>> The panic occurred in test #17 of that script.
>>>
>>> I'll also try to track what's going on.
>>
>> Thanks, I'll look - I was hoping/expecting that you had better tests for
>> mount options than I did! ;)
>>
>> Feel free to debug if you like, but it must be a bug in my patch so
>> I'll take ownership of trying to track down the problem and get it to
>> pass your test script.
>
> Got it!
>
> So I'll try to understand the patch first.
Sorry that it's not really possible to break it down into smaller changes.
> This test script focuses on reproducing NILFS-specific mount sequences
> (such as mounting a snapshot and current tree simultaneously) and
> checking the state of user space such as the GC process and utab.
> And, is not exhaustive for mount options.
>
> Looking at the patch, if I come up with test patterns that would be
> better to add, I will enhance the test script.
Sounds good.
So the oops you hit is when a snapshot is mounted first, then the main
fs is mounted.
My patch does this:
/*
* Try remount to setup mount states if the current
* tree is not mounted and only snapshots use this sb.
*/
err = nilfs_reconfigure(fc);
(in place of nilfs_remount()), and nilfs_reconfigure expects to have
fc->root set, which is normally only set up for an actual remount.
fc->root is NULL, so that's the oops. I'll see what I can work out.
Thanks,
-Eric
> Thanks,
> Ryusuke Konishi
>
>>
>> -Eric
>>
>>> Thanks,
>>> Ryusuke Konishi
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] nilfs2: convert to use the new mount API
2024-04-05 1:21 ` Eric Sandeen
@ 2024-04-05 1:46 ` Eric Sandeen
2024-04-05 3:00 ` [PATCH V2] " Eric Sandeen
0 siblings, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2024-04-05 1:46 UTC (permalink / raw)
To: Ryusuke Konishi; +Cc: linux-nilfs
On 4/4/24 8:21 PM, Eric Sandeen wrote:
> So the oops you hit is when a snapshot is mounted first, then the main
> fs is mounted.
>
> My patch does this:
>
> /*
> * Try remount to setup mount states if the current
> * tree is not mounted and only snapshots use this sb.
> */
> err = nilfs_reconfigure(fc);
>
> (in place of nilfs_remount()), and nilfs_reconfigure expects to have
> fc->root set, which is normally only set up for an actual remount.
>
> fc->root is NULL, so that's the oops. I'll see what I can work out.
Not sure if this is a terrible hack, but your test script passes with
this change on top of my original patch:
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 1cdc38db2612..4a9a8924068e 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -1232,6 +1232,7 @@ nilfs_get_tree(struct fs_context *fc)
* Try remount to setup mount states if the current
* tree is not mounted and only snapshots use this sb.
*/
+ fc->root = s->s_root;
err = nilfs_reconfigure(fc);
if (err)
goto failed_super;
-Eric
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V2] nilfs2: convert to use the new mount API
2024-04-05 1:46 ` Eric Sandeen
@ 2024-04-05 3:00 ` Eric Sandeen
2024-04-05 10:33 ` Ryusuke Konishi
2024-04-09 19:13 ` Ryusuke Konishi
0 siblings, 2 replies; 16+ messages in thread
From: Eric Sandeen @ 2024-04-05 3:00 UTC (permalink / raw)
To: Ryusuke Konishi; +Cc: linux-nilfs
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: Fix call to nilfs2_reconfigure() in nilfs_mount() to ensure
fc->root is set.
Clean up some extraneous comments and whitespace
This one passes your current test script.
Thanks,
-Eric
diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
index 2e29b98ba8ba..f4d367295d38 100644
--- a/fs/nilfs2/nilfs.h
+++ b/fs/nilfs2/nilfs.h
@@ -335,8 +335,8 @@ void __nilfs_error(struct super_block *sb, const char *function,
extern struct nilfs_super_block *
nilfs_read_super_block(struct super_block *, u64, int, struct buffer_head **);
-extern int nilfs_store_magic_and_option(struct super_block *,
- struct nilfs_super_block *, char *);
+extern int nilfs_store_magic(struct super_block *,
+ struct nilfs_super_block *);
extern int nilfs_check_feature_compatibility(struct super_block *,
struct nilfs_super_block *);
extern void nilfs_set_log_cursor(struct nilfs_super_block *,
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index ac24ed109ce9..0a87637e0bc2 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -29,7 +29,8 @@
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/blkdev.h>
-#include <linux/parser.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
#include <linux/crc32.h>
#include <linux/vfs.h>
#include <linux/writeback.h>
@@ -61,7 +62,6 @@ struct kmem_cache *nilfs_segbuf_cachep;
struct kmem_cache *nilfs_btree_path_cache;
static int nilfs_setup_super(struct super_block *sb, int is_mount);
-static int nilfs_remount(struct super_block *sb, int *flags, char *data);
void __nilfs_msg(struct super_block *sb, const char *fmt, ...)
{
@@ -702,105 +702,94 @@ static const struct super_operations nilfs_sops = {
.freeze_fs = nilfs_freeze,
.unfreeze_fs = nilfs_unfreeze,
.statfs = nilfs_statfs,
- .remount_fs = nilfs_remount,
.show_options = nilfs_show_options
};
enum {
- Opt_err_cont, Opt_err_panic, Opt_err_ro,
- Opt_barrier, Opt_nobarrier, Opt_snapshot, Opt_order, Opt_norecovery,
- Opt_discard, Opt_nodiscard, Opt_err,
+ Opt_err, Opt_barrier, Opt_snapshot, Opt_order, Opt_norecovery,
+ Opt_discard,
};
-static match_table_t tokens = {
- {Opt_err_cont, "errors=continue"},
- {Opt_err_panic, "errors=panic"},
- {Opt_err_ro, "errors=remount-ro"},
- {Opt_barrier, "barrier"},
- {Opt_nobarrier, "nobarrier"},
- {Opt_snapshot, "cp=%u"},
- {Opt_order, "order=%s"},
- {Opt_norecovery, "norecovery"},
- {Opt_discard, "discard"},
- {Opt_nodiscard, "nodiscard"},
- {Opt_err, NULL}
+static const struct constant_table nilfs_param_err[] = {
+ {"continue", NILFS_MOUNT_ERRORS_CONT},
+ {"panic", NILFS_MOUNT_ERRORS_PANIC},
+ {"remount-ro", NILFS_MOUNT_ERRORS_RO},
+ {}
};
-static int parse_options(char *options, struct super_block *sb, int is_remount)
-{
- struct the_nilfs *nilfs = sb->s_fs_info;
- char *p;
- substring_t args[MAX_OPT_ARGS];
-
- if (!options)
- return 1;
-
- while ((p = strsep(&options, ",")) != NULL) {
- int token;
+const struct fs_parameter_spec nilfs_param_spec[] = {
+ fsparam_enum ("errors", Opt_err, nilfs_param_err),
+ fsparam_flag_no ("barrier", Opt_barrier),
+ fsparam_u32 ("cp", Opt_snapshot),
+ fsparam_string ("order", Opt_order),
+ fsparam_flag ("norecovery", Opt_norecovery),
+ fsparam_flag_no ("discard", Opt_discard),
+ {}
+};
- if (!*p)
- continue;
+struct nilfs_fs_context {
+ unsigned long ns_mount_opt;
+ __u64 cno;
+};
- token = match_token(p, tokens, args);
- switch (token) {
- case Opt_barrier:
- nilfs_set_opt(nilfs, BARRIER);
- break;
- case Opt_nobarrier:
+static int nilfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
+{
+ struct nilfs_fs_context *nilfs = fc->fs_private;
+ int is_remount = fc->purpose == FS_CONTEXT_FOR_RECONFIGURE;
+ struct fs_parse_result result;
+ int opt;
+
+ opt = fs_parse(fc, nilfs_param_spec, param, &result);
+ if (opt < 0)
+ return opt;
+
+ switch (opt) {
+ case Opt_barrier:
+ if (result.negated)
nilfs_clear_opt(nilfs, BARRIER);
- break;
- case Opt_order:
- if (strcmp(args[0].from, "relaxed") == 0)
- /* Ordered data semantics */
- nilfs_clear_opt(nilfs, STRICT_ORDER);
- else if (strcmp(args[0].from, "strict") == 0)
- /* Strict in-order semantics */
- nilfs_set_opt(nilfs, STRICT_ORDER);
- else
- return 0;
- break;
- case Opt_err_panic:
- nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_PANIC);
- break;
- case Opt_err_ro:
- nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_RO);
- break;
- case Opt_err_cont:
- nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_CONT);
- break;
- case Opt_snapshot:
- if (is_remount) {
- nilfs_err(sb,
- "\"%s\" option is invalid for remount",
- p);
- return 0;
- }
- break;
- case Opt_norecovery:
- nilfs_set_opt(nilfs, NORECOVERY);
- break;
- case Opt_discard:
- nilfs_set_opt(nilfs, DISCARD);
- break;
- case Opt_nodiscard:
- nilfs_clear_opt(nilfs, DISCARD);
- break;
- default:
- nilfs_err(sb, "unrecognized mount option \"%s\"", p);
- return 0;
+ else
+ nilfs_set_opt(nilfs, BARRIER);
+ break;
+ case Opt_order:
+ if (strcmp(param->string, "relaxed") == 0)
+ /* Ordered data semantics */
+ nilfs_clear_opt(nilfs, STRICT_ORDER);
+ else if (strcmp(param->string, "strict") == 0)
+ /* Strict in-order semantics */
+ nilfs_set_opt(nilfs, STRICT_ORDER);
+ else
+ return -EINVAL;
+ break;
+ case Opt_err:
+ nilfs->ns_mount_opt &= ~NILFS_MOUNT_ERROR_MODE;
+ nilfs->ns_mount_opt |= result.uint_32;
+ break;
+ case Opt_snapshot:
+ if (is_remount) {
+ struct super_block *sb = fc->root->d_sb;
+ nilfs_err(sb,
+ "\"%s\" option is invalid for remount",
+ param->key);
+ return -EINVAL;
}
+ if (result.uint_64 == 0)
+ return -EINVAL;
+ nilfs->cno = result.uint_64;
+ break;
+ case Opt_norecovery:
+ nilfs_set_opt(nilfs, NORECOVERY);
+ break;
+ case Opt_discard:
+ if (result.negated)
+ nilfs_clear_opt(nilfs, DISCARD);
+ else
+ nilfs_set_opt(nilfs, DISCARD);
+ break;
+ default:
+ return -EINVAL;
}
- return 1;
-}
-static inline void
-nilfs_set_default_options(struct super_block *sb,
- struct nilfs_super_block *sbp)
-{
- struct the_nilfs *nilfs = sb->s_fs_info;
-
- nilfs->ns_mount_opt =
- NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
+ return 0;
}
static int nilfs_setup_super(struct super_block *sb, int is_mount)
@@ -857,9 +846,8 @@ struct nilfs_super_block *nilfs_read_super_block(struct super_block *sb,
return (struct nilfs_super_block *)((char *)(*pbh)->b_data + offset);
}
-int nilfs_store_magic_and_option(struct super_block *sb,
- struct nilfs_super_block *sbp,
- char *data)
+int nilfs_store_magic(struct super_block *sb,
+ struct nilfs_super_block *sbp)
{
struct the_nilfs *nilfs = sb->s_fs_info;
@@ -870,14 +858,12 @@ int nilfs_store_magic_and_option(struct super_block *sb,
sb->s_flags |= SB_NOATIME;
#endif
- nilfs_set_default_options(sb, sbp);
-
nilfs->ns_resuid = le16_to_cpu(sbp->s_def_resuid);
nilfs->ns_resgid = le16_to_cpu(sbp->s_def_resgid);
nilfs->ns_interval = le32_to_cpu(sbp->s_c_interval);
nilfs->ns_watermark = le32_to_cpu(sbp->s_c_block_max);
- return !parse_options(data, sb, 0) ? -EINVAL : 0;
+ return 0;
}
int nilfs_check_feature_compatibility(struct super_block *sb,
@@ -1042,10 +1028,11 @@ int nilfs_checkpoint_is_mounted(struct super_block *sb, __u64 cno)
* So, the recovery process is protected from other simultaneous mounts.
*/
static int
-nilfs_fill_super(struct super_block *sb, void *data, int silent)
+nilfs_fill_super(struct super_block *sb, struct fs_context *fc)
{
struct the_nilfs *nilfs;
struct nilfs_root *fsroot;
+ struct nilfs_fs_context *ctx = fc->fs_private;
__u64 cno;
int err;
@@ -1055,10 +1042,13 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
sb->s_fs_info = nilfs;
- err = init_nilfs(nilfs, sb, (char *)data);
+ err = init_nilfs(nilfs, sb);
if (err)
goto failed_nilfs;
+ /* Copy in parsed mount options */
+ nilfs->ns_mount_opt = ctx->ns_mount_opt;
+
sb->s_op = &nilfs_sops;
sb->s_export_op = &nilfs_export_ops;
sb->s_root = NULL;
@@ -1117,21 +1107,15 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
return err;
}
-static int nilfs_remount(struct super_block *sb, int *flags, char *data)
+static int nilfs_reconfigure(struct fs_context *fc)
{
+ struct nilfs_fs_context *ctx = fc->fs_private;
+ struct super_block *sb = fc->root->d_sb;
struct the_nilfs *nilfs = sb->s_fs_info;
- unsigned long old_sb_flags;
- unsigned long old_mount_opt;
int err;
sync_filesystem(sb);
- old_sb_flags = sb->s_flags;
- old_mount_opt = nilfs->ns_mount_opt;
- if (!parse_options(data, sb, 1)) {
- err = -EINVAL;
- goto restore_opts;
- }
sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
err = -EINVAL;
@@ -1139,12 +1123,11 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
if (!nilfs_valid_fs(nilfs)) {
nilfs_warn(sb,
"couldn't remount because the filesystem is in an incomplete recovery state");
- goto restore_opts;
+ goto ignore_opts;
}
-
- if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
+ if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
goto out;
- if (*flags & SB_RDONLY) {
+ if (fc->sb_flags & SB_RDONLY) {
sb->s_flags |= SB_RDONLY;
/*
@@ -1172,7 +1155,7 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
"couldn't remount RDWR because of unsupported optional features (%llx)",
(unsigned long long)features);
err = -EROFS;
- goto restore_opts;
+ goto ignore_opts;
}
sb->s_flags &= ~SB_RDONLY;
@@ -1180,130 +1163,57 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
root = NILFS_I(d_inode(sb->s_root))->i_root;
err = nilfs_attach_log_writer(sb, root);
if (err)
- goto restore_opts;
+ goto ignore_opts;
down_write(&nilfs->ns_sem);
nilfs_setup_super(sb, true);
up_write(&nilfs->ns_sem);
}
out:
- return 0;
-
- restore_opts:
- sb->s_flags = old_sb_flags;
- nilfs->ns_mount_opt = old_mount_opt;
- return err;
-}
-
-struct nilfs_super_data {
- __u64 cno;
- int flags;
-};
-
-static int nilfs_parse_snapshot_option(const char *option,
- const substring_t *arg,
- struct nilfs_super_data *sd)
-{
- unsigned long long val;
- const char *msg = NULL;
- int err;
-
- if (!(sd->flags & SB_RDONLY)) {
- msg = "read-only option is not specified";
- goto parse_error;
- }
-
- err = kstrtoull(arg->from, 0, &val);
- if (err) {
- if (err == -ERANGE)
- msg = "too large checkpoint number";
- else
- msg = "malformed argument";
- goto parse_error;
- } else if (val == 0) {
- msg = "invalid checkpoint number 0";
- goto parse_error;
- }
- sd->cno = val;
- return 0;
-
-parse_error:
- nilfs_err(NULL, "invalid option \"%s\": %s", option, msg);
- return 1;
-}
-
-/**
- * nilfs_identify - pre-read mount options needed to identify mount instance
- * @data: mount options
- * @sd: nilfs_super_data
- */
-static int nilfs_identify(char *data, struct nilfs_super_data *sd)
-{
- char *p, *options = data;
- substring_t args[MAX_OPT_ARGS];
- int token;
- int ret = 0;
-
- do {
- p = strsep(&options, ",");
- if (p != NULL && *p) {
- token = match_token(p, tokens, args);
- if (token == Opt_snapshot)
- ret = nilfs_parse_snapshot_option(p, &args[0],
- sd);
- }
- if (!options)
- break;
- BUG_ON(options == data);
- *(options - 1) = ',';
- } while (!ret);
- return ret;
-}
+ sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
+ /* Copy over parsed remount options */
+ nilfs->ns_mount_opt = ctx->ns_mount_opt;
-static int nilfs_set_bdev_super(struct super_block *s, void *data)
-{
- s->s_dev = *(dev_t *)data;
return 0;
-}
-static int nilfs_test_bdev_super(struct super_block *s, void *data)
-{
- return !(s->s_iflags & SB_I_RETIRED) && s->s_dev == *(dev_t *)data;
+ ignore_opts:
+ return err;
}
-static struct dentry *
-nilfs_mount(struct file_system_type *fs_type, int flags,
- const char *dev_name, void *data)
+static int
+nilfs_get_tree(struct fs_context *fc)
{
- struct nilfs_super_data sd = { .flags = flags };
+ struct nilfs_fs_context *ctx = fc->fs_private;
struct super_block *s;
dev_t dev;
int err;
- if (nilfs_identify(data, &sd))
- return ERR_PTR(-EINVAL);
+ if (ctx->cno && !(fc->sb_flags & SB_RDONLY)) {
+ nilfs_err(s, "invalid option \"cn=%llu\", "
+ "read-only option is not specified",
+ ctx->cno);
+ return -EINVAL;
+ }
- err = lookup_bdev(dev_name, &dev);
+ err = lookup_bdev(fc->source, &dev);
if (err)
- return ERR_PTR(err);
+ return err;
- s = sget(fs_type, nilfs_test_bdev_super, nilfs_set_bdev_super, flags,
- &dev);
+ s = sget_dev(fc, dev);
if (IS_ERR(s))
- return ERR_CAST(s);
+ return PTR_ERR(s);
if (!s->s_root) {
- err = setup_bdev_super(s, flags, NULL);
+ err = setup_bdev_super(s, fc->sb_flags, fc);
if (!err)
- err = nilfs_fill_super(s, data,
- flags & SB_SILENT ? 1 : 0);
+ err = nilfs_fill_super(s, fc);
if (err)
goto failed_super;
s->s_flags |= SB_ACTIVE;
- } else if (!sd.cno) {
+ } else if (!ctx->cno) {
if (nilfs_tree_is_busy(s->s_root)) {
- if ((flags ^ s->s_flags) & SB_RDONLY) {
+ if ((fc->sb_flags ^ s->s_flags) & SB_RDONLY) {
nilfs_err(s,
"the device already has a %s mount.",
sb_rdonly(s) ? "read-only" : "read/write");
@@ -1315,34 +1225,65 @@ nilfs_mount(struct file_system_type *fs_type, int flags,
* Try remount to setup mount states if the current
* tree is not mounted and only snapshots use this sb.
*/
- err = nilfs_remount(s, &flags, data);
+ fc->root = s->s_root;
+ err = nilfs_reconfigure(fc);
if (err)
goto failed_super;
}
}
- if (sd.cno) {
+ if (ctx->cno) {
struct dentry *root_dentry;
- err = nilfs_attach_snapshot(s, sd.cno, &root_dentry);
+ err = nilfs_attach_snapshot(s, ctx->cno, &root_dentry);
if (err)
goto failed_super;
- return root_dentry;
+ fc->root = root_dentry;
+ return 0;
}
- return dget(s->s_root);
+ fc->root = dget(s->s_root);
+ return 0;
failed_super:
deactivate_locked_super(s);
- return ERR_PTR(err);
+ return err;
+}
+
+static void nilfs_free_fc(struct fs_context *fc)
+{
+ kfree(fc->fs_private);
+}
+
+static const struct fs_context_operations nilfs_context_ops = {
+ .parse_param = nilfs_parse_param,
+ .get_tree = nilfs_get_tree,
+ .reconfigure = nilfs_reconfigure,
+ .free = nilfs_free_fc,
+};
+
+static int nilfs_init_fs_context(struct fs_context *fc)
+{
+ struct nilfs_fs_context *ctx;
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->ns_mount_opt = NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
+ fc->fs_private = ctx;
+ fc->ops = &nilfs_context_ops;
+
+ return 0;
}
struct file_system_type nilfs_fs_type = {
.owner = THIS_MODULE,
.name = "nilfs2",
- .mount = nilfs_mount,
.kill_sb = kill_block_super,
.fs_flags = FS_REQUIRES_DEV,
+ .init_fs_context = nilfs_init_fs_context,
+ .parameters = nilfs_param_spec,
};
MODULE_ALIAS_FS("nilfs2");
diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 2ae2c1bbf6d1..77fce1f509d1 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -668,7 +668,7 @@ static int nilfs_load_super_block(struct the_nilfs *nilfs,
* Return Value: On success, 0 is returned. On error, a negative error
* code is returned.
*/
-int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
+int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb)
{
struct nilfs_super_block *sbp;
int blocksize;
@@ -686,7 +686,7 @@ int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
if (err)
goto out;
- err = nilfs_store_magic_and_option(sb, sbp, data);
+ err = nilfs_store_magic(sb, sbp);
if (err)
goto failed_sbh;
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index cd4ae1b8ae16..85da0629415d 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -219,10 +219,6 @@ THE_NILFS_FNS(PURGING, purging)
#define nilfs_set_opt(nilfs, opt) \
((nilfs)->ns_mount_opt |= NILFS_MOUNT_##opt)
#define nilfs_test_opt(nilfs, opt) ((nilfs)->ns_mount_opt & NILFS_MOUNT_##opt)
-#define nilfs_write_opt(nilfs, mask, opt) \
- ((nilfs)->ns_mount_opt = \
- (((nilfs)->ns_mount_opt & ~NILFS_MOUNT_##mask) | \
- NILFS_MOUNT_##opt)) \
/**
* struct nilfs_root - nilfs root object
@@ -276,7 +272,7 @@ static inline int nilfs_sb_will_flip(struct the_nilfs *nilfs)
void nilfs_set_last_segment(struct the_nilfs *, sector_t, u64, __u64);
struct the_nilfs *alloc_nilfs(struct super_block *sb);
void destroy_nilfs(struct the_nilfs *nilfs);
-int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data);
+int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
int load_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
unsigned long nilfs_nrsvsegs(struct the_nilfs *nilfs, unsigned long nsegs);
void nilfs_set_nsegments(struct the_nilfs *nilfs, unsigned long nsegs);
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V2] nilfs2: convert to use the new mount API
2024-04-05 3:00 ` [PATCH V2] " Eric Sandeen
@ 2024-04-05 10:33 ` Ryusuke Konishi
2024-04-05 15:03 ` Eric Sandeen
2024-04-09 19:13 ` Ryusuke Konishi
1 sibling, 1 reply; 16+ messages in thread
From: Ryusuke Konishi @ 2024-04-05 10:33 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-nilfs
On Fri, Apr 5, 2024 at 12:00 PM Eric Sandeen wrote:
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> V2: Fix call to nilfs2_reconfigure() in nilfs_mount() to ensure
> fc->root is set.
>
> Clean up some extraneous comments and whitespace
>
> This one passes your current test script.
>
> Thanks,
> -Eric
Yeah, this v2 patch resolved the panic issue. It passed the test
script in multiple environments, as well as my manual checks for mount
options with and without using the constant table. Seems to be
working perfectly so far.
I'll get started on the full review, but I'd like to provide feedback
on style warnings detected by the checkpatch script at this point.
---
WARNING: Missing commit description - Add an appropriate one
WARNING: function definition argument 'struct super_block *' should
also have an identifier name
#146: FILE: fs/nilfs2/nilfs.h:338:
+extern int nilfs_store_magic(struct super_block *,
WARNING: function definition argument 'struct nilfs_super_block *'
should also have an identifier name
#146: FILE: fs/nilfs2/nilfs.h:338:
+extern int nilfs_store_magic(struct super_block *,
WARNING: space prohibited between function name and open parenthesis '('
#220: FILE: fs/nilfs2/super.c:721:
+ fsparam_enum ("errors", Opt_err, nilfs_param_err),
WARNING: space prohibited between function name and open parenthesis '('
#221: FILE: fs/nilfs2/super.c:722:
+ fsparam_flag_no ("barrier", Opt_barrier),
WARNING: space prohibited between function name and open parenthesis '('
#223: FILE: fs/nilfs2/super.c:724:
+ fsparam_string ("order", Opt_order),
WARNING: space prohibited between function name and open parenthesis '('
#224: FILE: fs/nilfs2/super.c:725:
+ fsparam_flag ("norecovery", Opt_norecovery),
WARNING: space prohibited between function name and open parenthesis '('
#225: FILE: fs/nilfs2/super.c:726:
+ fsparam_flag_no ("discard", Opt_discard),
WARNING: Missing a blank line after declarations
#317: FILE: fs/nilfs2/super.c:770:
+ struct super_block *sb = fc->root->d_sb;
+ nilfs_err(sb,
WARNING: quoted string split across lines
#576: FILE: fs/nilfs2/super.c:1193:
+ nilfs_err(s, "invalid option \"cn=%llu\", "
+ "read-only option is not specified",
total: 0 errors, 10 warnings, 563 lines checked
---
Of these, the warning for the function declaration of
nilfs_store_magic() is an existing issue, so it can be left as is.
Also, I feel like the warnings for fsparam_{enum,flag,flag_no,string}
can be ignored for the sake of appearance. (I will not omit them here
so as not to make any preconceptions).
Thanks,
Ryusuke Konishi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2] nilfs2: convert to use the new mount API
2024-04-05 10:33 ` Ryusuke Konishi
@ 2024-04-05 15:03 ` Eric Sandeen
2024-04-09 20:17 ` Ryusuke Konishi
0 siblings, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2024-04-05 15:03 UTC (permalink / raw)
To: Ryusuke Konishi; +Cc: linux-nilfs
On 4/5/24 5:33 AM, Ryusuke Konishi wrote:
> On Fri, Apr 5, 2024 at 12:00 PM Eric Sandeen wrote:
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> V2: Fix call to nilfs2_reconfigure() in nilfs_mount() to ensure
>> fc->root is set.
>>
>> Clean up some extraneous comments and whitespace
>>
>> This one passes your current test script.
>>
>> Thanks,
>> -Eric
>
> Yeah, this v2 patch resolved the panic issue. It passed the test
> script in multiple environments, as well as my manual checks for mount
> options with and without using the constant table. Seems to be
> working perfectly so far.
great!
> I'll get started on the full review, but I'd like to provide feedback
> on style warnings detected by the checkpatch script at this point.
> ---
> WARNING: Missing commit description - Add an appropriate one
Not a lot to say; I can note 'interesting' things about the conversion
if that'd be helpful.
> WARNING: function definition argument 'struct super_block *' should
> also have an identifier name
> #146: FILE: fs/nilfs2/nilfs.h:338:
> +extern int nilfs_store_magic(struct super_block *,
As you say this is pre-existing; I can change it if you like, I was just
following current style.
> WARNING: function definition argument 'struct nilfs_super_block *'
> should also have an identifier name
> #146: FILE: fs/nilfs2/nilfs.h:338:
> +extern int nilfs_store_magic(struct super_block *,
same
> WARNING: space prohibited between function name and open parenthesis '('
> #220: FILE: fs/nilfs2/super.c:721:
> + fsparam_enum ("errors", Opt_err, nilfs_param_err),
This seems to be the pattern for every filesystem using these calls ...
> WARNING: space prohibited between function name and open parenthesis '('
> #221: FILE: fs/nilfs2/super.c:722:
> + fsparam_flag_no ("barrier", Opt_barrier),
>
> WARNING: space prohibited between function name and open parenthesis '('
> #223: FILE: fs/nilfs2/super.c:724:
> + fsparam_string ("order", Opt_order),
>
> WARNING: space prohibited between function name and open parenthesis '('
> #224: FILE: fs/nilfs2/super.c:725:
> + fsparam_flag ("norecovery", Opt_norecovery),
>
> WARNING: space prohibited between function name and open parenthesis '('
> #225: FILE: fs/nilfs2/super.c:726:
> + fsparam_flag_no ("discard", Opt_discard),
>
> WARNING: Missing a blank line after declarations
> #317: FILE: fs/nilfs2/super.c:770:
> + struct super_block *sb = fc->root->d_sb;
> + nilfs_err(sb,
easy enough to fix.
> WARNING: quoted string split across lines
> #576: FILE: fs/nilfs2/super.c:1193:
> + nilfs_err(s, "invalid option \"cn=%llu\", "
> + "read-only option is not specified",
Just let me know your preference on long strings like this (out-dent?
go past col 80? leave it alone?)
I'll wait for more review before I send a minor update just for these.
(or, if you prefer, feel free to tweak small things on your end.)
thanks,
-Eric
> total: 0 errors, 10 warnings, 563 lines checked
> ---
>
> Of these, the warning for the function declaration of
> nilfs_store_magic() is an existing issue, so it can be left as is.
>
> Also, I feel like the warnings for fsparam_{enum,flag,flag_no,string}
> can be ignored for the sake of appearance. (I will not omit them here
> so as not to make any preconceptions).
>
> Thanks,
> Ryusuke Konishi
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2] nilfs2: convert to use the new mount API
2024-04-05 3:00 ` [PATCH V2] " Eric Sandeen
2024-04-05 10:33 ` Ryusuke Konishi
@ 2024-04-09 19:13 ` Ryusuke Konishi
2024-04-09 19:54 ` Eric Sandeen
1 sibling, 1 reply; 16+ messages in thread
From: Ryusuke Konishi @ 2024-04-09 19:13 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-nilfs
Thank you for waiting. I've finished the full review.
I'll comment below, inline.
First let me say that this patch is great and I don't see any points
that need major rewrites.
Regarding style warnings, I will reply to that email later.
On Fri, Apr 5, 2024 at 12:00 PM Eric Sandeen wrote:
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> V2: Fix call to nilfs2_reconfigure() in nilfs_mount() to ensure
> fc->root is set.
>
> Clean up some extraneous comments and whitespace
>
> This one passes your current test script.
>
> Thanks,
> -Eric
>
> diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
> index 2e29b98ba8ba..f4d367295d38 100644
> --- a/fs/nilfs2/nilfs.h
> +++ b/fs/nilfs2/nilfs.h
> @@ -335,8 +335,8 @@ void __nilfs_error(struct super_block *sb, const char *function,
>
> extern struct nilfs_super_block *
> nilfs_read_super_block(struct super_block *, u64, int, struct buffer_head **);
> -extern int nilfs_store_magic_and_option(struct super_block *,
> - struct nilfs_super_block *, char *);
> +extern int nilfs_store_magic(struct super_block *,
> + struct nilfs_super_block *);
> extern int nilfs_check_feature_compatibility(struct super_block *,
> struct nilfs_super_block *);
> extern void nilfs_set_log_cursor(struct nilfs_super_block *,
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index ac24ed109ce9..0a87637e0bc2 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -29,7 +29,8 @@
> #include <linux/slab.h>
> #include <linux/init.h>
> #include <linux/blkdev.h>
> -#include <linux/parser.h>
> +#include <linux/fs_context.h>
> +#include <linux/fs_parser.h>
> #include <linux/crc32.h>
> #include <linux/vfs.h>
> #include <linux/writeback.h>
> @@ -61,7 +62,6 @@ struct kmem_cache *nilfs_segbuf_cachep;
> struct kmem_cache *nilfs_btree_path_cache;
>
> static int nilfs_setup_super(struct super_block *sb, int is_mount);
> -static int nilfs_remount(struct super_block *sb, int *flags, char *data);
>
> void __nilfs_msg(struct super_block *sb, const char *fmt, ...)
> {
> @@ -702,105 +702,94 @@ static const struct super_operations nilfs_sops = {
> .freeze_fs = nilfs_freeze,
> .unfreeze_fs = nilfs_unfreeze,
> .statfs = nilfs_statfs,
> - .remount_fs = nilfs_remount,
> .show_options = nilfs_show_options
> };
>
> enum {
> - Opt_err_cont, Opt_err_panic, Opt_err_ro,
> - Opt_barrier, Opt_nobarrier, Opt_snapshot, Opt_order, Opt_norecovery,
> - Opt_discard, Opt_nodiscard, Opt_err,
> + Opt_err, Opt_barrier, Opt_snapshot, Opt_order, Opt_norecovery,
> + Opt_discard,
> };
>
> -static match_table_t tokens = {
> - {Opt_err_cont, "errors=continue"},
> - {Opt_err_panic, "errors=panic"},
> - {Opt_err_ro, "errors=remount-ro"},
> - {Opt_barrier, "barrier"},
> - {Opt_nobarrier, "nobarrier"},
> - {Opt_snapshot, "cp=%u"},
> - {Opt_order, "order=%s"},
> - {Opt_norecovery, "norecovery"},
> - {Opt_discard, "discard"},
> - {Opt_nodiscard, "nodiscard"},
> - {Opt_err, NULL}
> +static const struct constant_table nilfs_param_err[] = {
> + {"continue", NILFS_MOUNT_ERRORS_CONT},
> + {"panic", NILFS_MOUNT_ERRORS_PANIC},
> + {"remount-ro", NILFS_MOUNT_ERRORS_RO},
> + {}
> };
>
> -static int parse_options(char *options, struct super_block *sb, int is_remount)
> -{
> - struct the_nilfs *nilfs = sb->s_fs_info;
> - char *p;
> - substring_t args[MAX_OPT_ARGS];
> -
> - if (!options)
> - return 1;
> -
> - while ((p = strsep(&options, ",")) != NULL) {
> - int token;
> +const struct fs_parameter_spec nilfs_param_spec[] = {
> + fsparam_enum ("errors", Opt_err, nilfs_param_err),
> + fsparam_flag_no ("barrier", Opt_barrier),
> + fsparam_u32 ("cp", Opt_snapshot),
Checkpoint numbers require a 64-bit unsigned integer parser
(originally parsed by kstrtoull()), so I think you should use
fsparam_u64 here.
Since nilfs_parse_param() was written assuming fsparam_u64, I guess
this is a simple mistake.
Also, "nilfs_param_spec" is not declared with the "static" class
specifier, so could you please add it ?
> + fsparam_string ("order", Opt_order),
> + fsparam_flag ("norecovery", Opt_norecovery),
> + fsparam_flag_no ("discard", Opt_discard),
> + {}
> +};
>
> - if (!*p)
> - continue;
> +struct nilfs_fs_context {
> + unsigned long ns_mount_opt;
> + __u64 cno;
> +};
>
> - token = match_token(p, tokens, args);
> - switch (token) {
> - case Opt_barrier:
> - nilfs_set_opt(nilfs, BARRIER);
> - break;
> - case Opt_nobarrier:
> +static int nilfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> +{
> + struct nilfs_fs_context *nilfs = fc->fs_private;
> + int is_remount = fc->purpose == FS_CONTEXT_FOR_RECONFIGURE;
> + struct fs_parse_result result;
> + int opt;
> +
> + opt = fs_parse(fc, nilfs_param_spec, param, &result);
> + if (opt < 0)
> + return opt;
> +
> + switch (opt) {
> + case Opt_barrier:
> + if (result.negated)
> nilfs_clear_opt(nilfs, BARRIER);
> - break;
> - case Opt_order:
> - if (strcmp(args[0].from, "relaxed") == 0)
> - /* Ordered data semantics */
> - nilfs_clear_opt(nilfs, STRICT_ORDER);
> - else if (strcmp(args[0].from, "strict") == 0)
> - /* Strict in-order semantics */
> - nilfs_set_opt(nilfs, STRICT_ORDER);
> - else
> - return 0;
> - break;
> - case Opt_err_panic:
> - nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_PANIC);
> - break;
> - case Opt_err_ro:
> - nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_RO);
> - break;
> - case Opt_err_cont:
> - nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_CONT);
> - break;
> - case Opt_snapshot:
> - if (is_remount) {
> - nilfs_err(sb,
> - "\"%s\" option is invalid for remount",
> - p);
> - return 0;
> - }
> - break;
> - case Opt_norecovery:
> - nilfs_set_opt(nilfs, NORECOVERY);
> - break;
> - case Opt_discard:
> - nilfs_set_opt(nilfs, DISCARD);
> - break;
> - case Opt_nodiscard:
> - nilfs_clear_opt(nilfs, DISCARD);
> - break;
> - default:
> - nilfs_err(sb, "unrecognized mount option \"%s\"", p);
> - return 0;
> + else
> + nilfs_set_opt(nilfs, BARRIER);
> + break;
> + case Opt_order:
> + if (strcmp(param->string, "relaxed") == 0)
> + /* Ordered data semantics */
> + nilfs_clear_opt(nilfs, STRICT_ORDER);
> + else if (strcmp(param->string, "strict") == 0)
> + /* Strict in-order semantics */
> + nilfs_set_opt(nilfs, STRICT_ORDER);
> + else
> + return -EINVAL;
> + break;
> + case Opt_err:
> + nilfs->ns_mount_opt &= ~NILFS_MOUNT_ERROR_MODE;
> + nilfs->ns_mount_opt |= result.uint_32;
> + break;
> + case Opt_snapshot:
> + if (is_remount) {
> + struct super_block *sb = fc->root->d_sb;
> + nilfs_err(sb,
> + "\"%s\" option is invalid for remount",
> + param->key);
> + return -EINVAL;
> }
> + if (result.uint_64 == 0)
> + return -EINVAL;
For the case where the "cp=0" invalid option is specified, could you
please output the following error message with nilfs_err() as before ?
"invalid option \"cp=0\": invalid checkpoint number 0"
Other similar messages seem to overlap with the message output by
fs_parser routines, so I think just adding this one is sufficient.
> + nilfs->cno = result.uint_64;
> + break;
> + case Opt_norecovery:
> + nilfs_set_opt(nilfs, NORECOVERY);
> + break;
> + case Opt_discard:
> + if (result.negated)
> + nilfs_clear_opt(nilfs, DISCARD);
> + else
> + nilfs_set_opt(nilfs, DISCARD);
> + break;
> + default:
> + return -EINVAL;
> }
> - return 1;
> -}
>
> -static inline void
> -nilfs_set_default_options(struct super_block *sb,
> - struct nilfs_super_block *sbp)
> -{
> - struct the_nilfs *nilfs = sb->s_fs_info;
> -
> - nilfs->ns_mount_opt =
> - NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
> + return 0;
> }
>
> static int nilfs_setup_super(struct super_block *sb, int is_mount)
> @@ -857,9 +846,8 @@ struct nilfs_super_block *nilfs_read_super_block(struct super_block *sb,
> return (struct nilfs_super_block *)((char *)(*pbh)->b_data + offset);
> }
>
> -int nilfs_store_magic_and_option(struct super_block *sb,
> - struct nilfs_super_block *sbp,
> - char *data)
> +int nilfs_store_magic(struct super_block *sb,
> + struct nilfs_super_block *sbp)
> {
> struct the_nilfs *nilfs = sb->s_fs_info;
>
> @@ -870,14 +858,12 @@ int nilfs_store_magic_and_option(struct super_block *sb,
> sb->s_flags |= SB_NOATIME;
> #endif
>
> - nilfs_set_default_options(sb, sbp);
> -
> nilfs->ns_resuid = le16_to_cpu(sbp->s_def_resuid);
> nilfs->ns_resgid = le16_to_cpu(sbp->s_def_resgid);
> nilfs->ns_interval = le32_to_cpu(sbp->s_c_interval);
> nilfs->ns_watermark = le32_to_cpu(sbp->s_c_block_max);
>
> - return !parse_options(data, sb, 0) ? -EINVAL : 0;
> + return 0;
> }
>
> int nilfs_check_feature_compatibility(struct super_block *sb,
> @@ -1042,10 +1028,11 @@ int nilfs_checkpoint_is_mounted(struct super_block *sb, __u64 cno)
> * So, the recovery process is protected from other simultaneous mounts.
> */
> static int
> -nilfs_fill_super(struct super_block *sb, void *data, int silent)
> +nilfs_fill_super(struct super_block *sb, struct fs_context *fc)
> {
> struct the_nilfs *nilfs;
> struct nilfs_root *fsroot;
> + struct nilfs_fs_context *ctx = fc->fs_private;
> __u64 cno;
> int err;
>
> @@ -1055,10 +1042,13 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
>
> sb->s_fs_info = nilfs;
>
> - err = init_nilfs(nilfs, sb, (char *)data);
> + err = init_nilfs(nilfs, sb);
> if (err)
> goto failed_nilfs;
>
> + /* Copy in parsed mount options */
> + nilfs->ns_mount_opt = ctx->ns_mount_opt;
> +
> sb->s_op = &nilfs_sops;
> sb->s_export_op = &nilfs_export_ops;
> sb->s_root = NULL;
> @@ -1117,21 +1107,15 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
> return err;
> }
>
> -static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> +static int nilfs_reconfigure(struct fs_context *fc)
> {
> + struct nilfs_fs_context *ctx = fc->fs_private;
> + struct super_block *sb = fc->root->d_sb;
> struct the_nilfs *nilfs = sb->s_fs_info;
> - unsigned long old_sb_flags;
> - unsigned long old_mount_opt;
> int err;
>
> sync_filesystem(sb);
> - old_sb_flags = sb->s_flags;
> - old_mount_opt = nilfs->ns_mount_opt;
>
> - if (!parse_options(data, sb, 1)) {
> - err = -EINVAL;
> - goto restore_opts;
> - }
> sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
This "s_flags" adjustment overlaps with the flag adjustment just
before returning with normal status.
I think there is no problem with deleting this.
>
> err = -EINVAL;
> @@ -1139,12 +1123,11 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> if (!nilfs_valid_fs(nilfs)) {
> nilfs_warn(sb,
> "couldn't remount because the filesystem is in an incomplete recovery state");
> - goto restore_opts;
> + goto ignore_opts;
> }
> -
> - if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
> + if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
> goto out;
> - if (*flags & SB_RDONLY) {
> + if (fc->sb_flags & SB_RDONLY) {
> sb->s_flags |= SB_RDONLY;
>
> /*
> @@ -1172,7 +1155,7 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> "couldn't remount RDWR because of unsupported optional features (%llx)",
> (unsigned long long)features);
> err = -EROFS;
> - goto restore_opts;
> + goto ignore_opts;
> }
>
> sb->s_flags &= ~SB_RDONLY;
> @@ -1180,130 +1163,57 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> root = NILFS_I(d_inode(sb->s_root))->i_root;
> err = nilfs_attach_log_writer(sb, root);
> if (err)
> - goto restore_opts;
> + goto ignore_opts;
Here, if nilfs_attach_log_writer() fails, it will return via
"ignore_opts" without restoring the SB_RDONLY flag in sb->s_flags.
I think it is necessary to repair the flag only for this error path,
what do you think?
>
> down_write(&nilfs->ns_sem);
> nilfs_setup_super(sb, true);
> up_write(&nilfs->ns_sem);
> }
> out:
> - return 0;
> -
> - restore_opts:
> - sb->s_flags = old_sb_flags;
> - nilfs->ns_mount_opt = old_mount_opt;
> - return err;
> -}
> -
> -struct nilfs_super_data {
> - __u64 cno;
> - int flags;
> -};
> -
> -static int nilfs_parse_snapshot_option(const char *option,
> - const substring_t *arg,
> - struct nilfs_super_data *sd)
> -{
> - unsigned long long val;
> - const char *msg = NULL;
> - int err;
> -
> - if (!(sd->flags & SB_RDONLY)) {
> - msg = "read-only option is not specified";
> - goto parse_error;
> - }
> -
> - err = kstrtoull(arg->from, 0, &val);
> - if (err) {
> - if (err == -ERANGE)
> - msg = "too large checkpoint number";
> - else
> - msg = "malformed argument";
> - goto parse_error;
> - } else if (val == 0) {
> - msg = "invalid checkpoint number 0";
> - goto parse_error;
> - }
> - sd->cno = val;
> - return 0;
> -
> -parse_error:
> - nilfs_err(NULL, "invalid option \"%s\": %s", option, msg);
> - return 1;
> -}
> -
> -/**
> - * nilfs_identify - pre-read mount options needed to identify mount instance
> - * @data: mount options
> - * @sd: nilfs_super_data
> - */
> -static int nilfs_identify(char *data, struct nilfs_super_data *sd)
> -{
> - char *p, *options = data;
> - substring_t args[MAX_OPT_ARGS];
> - int token;
> - int ret = 0;
> -
> - do {
> - p = strsep(&options, ",");
> - if (p != NULL && *p) {
> - token = match_token(p, tokens, args);
> - if (token == Opt_snapshot)
> - ret = nilfs_parse_snapshot_option(p, &args[0],
> - sd);
> - }
> - if (!options)
> - break;
> - BUG_ON(options == data);
> - *(options - 1) = ',';
> - } while (!ret);
> - return ret;
> -}
> + sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
> + /* Copy over parsed remount options */
> + nilfs->ns_mount_opt = ctx->ns_mount_opt;
>
> -static int nilfs_set_bdev_super(struct super_block *s, void *data)
> -{
> - s->s_dev = *(dev_t *)data;
> return 0;
> -}
>
> -static int nilfs_test_bdev_super(struct super_block *s, void *data)
> -{
> - return !(s->s_iflags & SB_I_RETIRED) && s->s_dev == *(dev_t *)data;
> + ignore_opts:
> + return err;
> }
>
> -static struct dentry *
> -nilfs_mount(struct file_system_type *fs_type, int flags,
> - const char *dev_name, void *data)
> +static int
> +nilfs_get_tree(struct fs_context *fc)
> {
> - struct nilfs_super_data sd = { .flags = flags };
> + struct nilfs_fs_context *ctx = fc->fs_private;
> struct super_block *s;
> dev_t dev;
> int err;
>
> - if (nilfs_identify(data, &sd))
> - return ERR_PTR(-EINVAL);
> + if (ctx->cno && !(fc->sb_flags & SB_RDONLY)) {
> + nilfs_err(s, "invalid option \"cn=%llu\", "
> + "read-only option is not specified",
> + ctx->cno);
> + return -EINVAL;
> + }
This error check conversion (to check after the read-only flag is
determined) is ok.
But, the option name in the error message has changed, so please correct it.
The original message looks like
"invalid option \"cp=%llu\": read-only option is not specified"
The checkpoint number expression cannot be reproduced to be exactly
the same as the input (as in the case where the radix is specified
like "cp=0x123") but I think it's acceptable.
That's all for the non-style issue comments.
Thanks,
Ryusuke Konishi
>
> - err = lookup_bdev(dev_name, &dev);
> + err = lookup_bdev(fc->source, &dev);
> if (err)
> - return ERR_PTR(err);
> + return err;
>
> - s = sget(fs_type, nilfs_test_bdev_super, nilfs_set_bdev_super, flags,
> - &dev);
> + s = sget_dev(fc, dev);
> if (IS_ERR(s))
> - return ERR_CAST(s);
> + return PTR_ERR(s);
>
> if (!s->s_root) {
> - err = setup_bdev_super(s, flags, NULL);
> + err = setup_bdev_super(s, fc->sb_flags, fc);
> if (!err)
> - err = nilfs_fill_super(s, data,
> - flags & SB_SILENT ? 1 : 0);
> + err = nilfs_fill_super(s, fc);
> if (err)
> goto failed_super;
>
> s->s_flags |= SB_ACTIVE;
> - } else if (!sd.cno) {
> + } else if (!ctx->cno) {
> if (nilfs_tree_is_busy(s->s_root)) {
> - if ((flags ^ s->s_flags) & SB_RDONLY) {
> + if ((fc->sb_flags ^ s->s_flags) & SB_RDONLY) {
> nilfs_err(s,
> "the device already has a %s mount.",
> sb_rdonly(s) ? "read-only" : "read/write");
> @@ -1315,34 +1225,65 @@ nilfs_mount(struct file_system_type *fs_type, int flags,
> * Try remount to setup mount states if the current
> * tree is not mounted and only snapshots use this sb.
> */
> - err = nilfs_remount(s, &flags, data);
> + fc->root = s->s_root;
> + err = nilfs_reconfigure(fc);
> if (err)
> goto failed_super;
> }
> }
>
> - if (sd.cno) {
> + if (ctx->cno) {
> struct dentry *root_dentry;
>
> - err = nilfs_attach_snapshot(s, sd.cno, &root_dentry);
> + err = nilfs_attach_snapshot(s, ctx->cno, &root_dentry);
> if (err)
> goto failed_super;
> - return root_dentry;
> + fc->root = root_dentry;
> + return 0;
> }
>
> - return dget(s->s_root);
> + fc->root = dget(s->s_root);
> + return 0;
>
> failed_super:
> deactivate_locked_super(s);
> - return ERR_PTR(err);
> + return err;
> +}
> +
> +static void nilfs_free_fc(struct fs_context *fc)
> +{
> + kfree(fc->fs_private);
> +}
> +
> +static const struct fs_context_operations nilfs_context_ops = {
> + .parse_param = nilfs_parse_param,
> + .get_tree = nilfs_get_tree,
> + .reconfigure = nilfs_reconfigure,
> + .free = nilfs_free_fc,
> +};
> +
> +static int nilfs_init_fs_context(struct fs_context *fc)
> +{
> + struct nilfs_fs_context *ctx;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->ns_mount_opt = NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
> + fc->fs_private = ctx;
> + fc->ops = &nilfs_context_ops;
> +
> + return 0;
> }
>
> struct file_system_type nilfs_fs_type = {
> .owner = THIS_MODULE,
> .name = "nilfs2",
> - .mount = nilfs_mount,
> .kill_sb = kill_block_super,
> .fs_flags = FS_REQUIRES_DEV,
> + .init_fs_context = nilfs_init_fs_context,
> + .parameters = nilfs_param_spec,
> };
> MODULE_ALIAS_FS("nilfs2");
>
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index 2ae2c1bbf6d1..77fce1f509d1 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -668,7 +668,7 @@ static int nilfs_load_super_block(struct the_nilfs *nilfs,
> * Return Value: On success, 0 is returned. On error, a negative error
> * code is returned.
> */
> -int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
> +int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb)
> {
> struct nilfs_super_block *sbp;
> int blocksize;
> @@ -686,7 +686,7 @@ int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
> if (err)
> goto out;
>
> - err = nilfs_store_magic_and_option(sb, sbp, data);
> + err = nilfs_store_magic(sb, sbp);
> if (err)
> goto failed_sbh;
>
> diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
> index cd4ae1b8ae16..85da0629415d 100644
> --- a/fs/nilfs2/the_nilfs.h
> +++ b/fs/nilfs2/the_nilfs.h
> @@ -219,10 +219,6 @@ THE_NILFS_FNS(PURGING, purging)
> #define nilfs_set_opt(nilfs, opt) \
> ((nilfs)->ns_mount_opt |= NILFS_MOUNT_##opt)
> #define nilfs_test_opt(nilfs, opt) ((nilfs)->ns_mount_opt & NILFS_MOUNT_##opt)
> -#define nilfs_write_opt(nilfs, mask, opt) \
> - ((nilfs)->ns_mount_opt = \
> - (((nilfs)->ns_mount_opt & ~NILFS_MOUNT_##mask) | \
> - NILFS_MOUNT_##opt)) \
>
> /**
> * struct nilfs_root - nilfs root object
> @@ -276,7 +272,7 @@ static inline int nilfs_sb_will_flip(struct the_nilfs *nilfs)
> void nilfs_set_last_segment(struct the_nilfs *, sector_t, u64, __u64);
> struct the_nilfs *alloc_nilfs(struct super_block *sb);
> void destroy_nilfs(struct the_nilfs *nilfs);
> -int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data);
> +int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
> int load_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
> unsigned long nilfs_nrsvsegs(struct the_nilfs *nilfs, unsigned long nsegs);
> void nilfs_set_nsegments(struct the_nilfs *nilfs, unsigned long nsegs);
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2] nilfs2: convert to use the new mount API
2024-04-09 19:13 ` Ryusuke Konishi
@ 2024-04-09 19:54 ` Eric Sandeen
2024-04-09 20:36 ` Ryusuke Konishi
2024-04-10 12:47 ` Ryusuke Konishi
0 siblings, 2 replies; 16+ messages in thread
From: Eric Sandeen @ 2024-04-09 19:54 UTC (permalink / raw)
To: Ryusuke Konishi; +Cc: linux-nilfs
On 4/9/24 2:13 PM, Ryusuke Konishi wrote:
> Thank you for waiting. I've finished the full review.
>
> I'll comment below, inline.
> First let me say that this patch is great and I don't see any points
> that need major rewrites.
Thanks!
> Regarding style warnings, I will reply to that email later.
>
> On Fri, Apr 5, 2024 at 12:00 PM Eric Sandeen wrote:
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
...
>> -static int parse_options(char *options, struct super_block *sb, int is_remount)
>> -{
>> - struct the_nilfs *nilfs = sb->s_fs_info;
>> - char *p;
>> - substring_t args[MAX_OPT_ARGS];
>> -
>> - if (!options)
>> - return 1;
>> -
>> - while ((p = strsep(&options, ",")) != NULL) {
>> - int token;
>> +const struct fs_parameter_spec nilfs_param_spec[] = {
>> + fsparam_enum ("errors", Opt_err, nilfs_param_err),
>> + fsparam_flag_no ("barrier", Opt_barrier),
>
>> + fsparam_u32 ("cp", Opt_snapshot),
>
> Checkpoint numbers require a 64-bit unsigned integer parser
> (originally parsed by kstrtoull()), so I think you should use
> fsparam_u64 here.
> Since nilfs_parse_param() was written assuming fsparam_u64, I guess
> this is a simple mistake.
Yep, I think I mechanically changed anything like
{Opt_snapshot, "cp=%u"}
to _u32, and missed how the actual parsing works.
> Also, "nilfs_param_spec" is not declared with the "static" class
> specifier, so could you please add it ?
Sure thing
...
>> + if (is_remount) {
>> + struct super_block *sb = fc->root->d_sb;
>> + nilfs_err(sb,
>> + "\"%s\" option is invalid for remount",
>> + param->key);
>> + return -EINVAL;
>> }
>
>> + if (result.uint_64 == 0)
>> + return -EINVAL;
>
> For the case where the "cp=0" invalid option is specified, could you
> please output the following error message with nilfs_err() as before ?
>
> "invalid option \"cp=0\": invalid checkpoint number 0"
>
> Other similar messages seem to overlap with the message output by
> fs_parser routines, so I think just adding this one is sufficient.
*nod* good catch
...
>
>> sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
>
> This "s_flags" adjustment overlaps with the flag adjustment just
> before returning with normal status.
> I think there is no problem with deleting this.
Ah, I think you are right. Will make sure nothing depends on the sb
flags before then, though.
...
>> @@ -1180,130 +1163,57 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
>> root = NILFS_I(d_inode(sb->s_root))->i_root;
>
>> err = nilfs_attach_log_writer(sb, root);
>> if (err)
>> - goto restore_opts;
>> + goto ignore_opts;
>
> Here, if nilfs_attach_log_writer() fails, it will return via
> "ignore_opts" without restoring the SB_RDONLY flag in sb->s_flags.
> I think it is necessary to repair the flag only for this error path,
> what do you think?
Again, I think you are right, although maybe if the above flags copy is
moved to the end, it won't be a problem? I'll look more closely.
...
>
>> + if (ctx->cno && !(fc->sb_flags & SB_RDONLY)) {
>> + nilfs_err(s, "invalid option \"cn=%llu\", "
>> + "read-only option is not specified",
>> + ctx->cno);
>> + return -EINVAL;
>> + }
>
> This error check conversion (to check after the read-only flag is
> determined) is ok.
> But, the option name in the error message has changed, so please correct it.
> The original message looks like
>
> "invalid option \"cp=%llu\": read-only option is not specified"
Whoops, I think that's just a typo.
> The checkpoint number expression cannot be reproduced to be exactly
> the same as the input (as in the case where the radix is specified
> like "cp=0x123") but I think it's acceptable.
Yup - that's a difference w/ the new mount API, I think - all of the mount
options need to be parsed independently, and we can only look for invalid
combinations after that's all done and by then we onlly have the value, not
the original format or string.
If you would like to keep the original format of the option, we could store
it in the fs_context as a string (I think) and emit that in the error message.
Worth it?
Thanks,
-Eric
>
> That's all for the non-style issue comments.
>
> Thanks,
> Ryusuke Konishi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2] nilfs2: convert to use the new mount API
2024-04-05 15:03 ` Eric Sandeen
@ 2024-04-09 20:17 ` Ryusuke Konishi
0 siblings, 0 replies; 16+ messages in thread
From: Ryusuke Konishi @ 2024-04-09 20:17 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-nilfs
In this email, I will reply regarding style issues.
On Sat, Apr 6, 2024 at 12:04 AM Eric Sandeen wrote:
>
> On 4/5/24 5:33 AM, Ryusuke Konishi wrote:
> > On Fri, Apr 5, 2024 at 12:00 PM Eric Sandeen wrote:
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> V2: Fix call to nilfs2_reconfigure() in nilfs_mount() to ensure
> >> fc->root is set.
> >>
> >> Clean up some extraneous comments and whitespace
> >>
> >> This one passes your current test script.
> >>
> >> Thanks,
> >> -Eric
> >
> > Yeah, this v2 patch resolved the panic issue. It passed the test
> > script in multiple environments, as well as my manual checks for mount
> > options with and without using the constant table. Seems to be
> > working perfectly so far.
>
> great!
>
> > I'll get started on the full review, but I'd like to provide feedback
> > on style warnings detected by the checkpatch script at this point.
>
> > ---
> > WARNING: Missing commit description - Add an appropriate one
>
> Not a lot to say; I can note 'interesting' things about the conversion
> if that'd be helpful.
>
It's OK if you include a commit log with the patch that you think is
the final version.
I also don't think a long explanation is necessary for this, but I'll
leave it to you.
> > WARNING: function definition argument 'struct super_block *' should
> > also have an identifier name
> > #146: FILE: fs/nilfs2/nilfs.h:338:
> > +extern int nilfs_store_magic(struct super_block *,
>
> As you say this is pre-existing; I can change it if you like, I was just
> following current style.
>
> > WARNING: function definition argument 'struct nilfs_super_block *'
> > should also have an identifier name
> > #146: FILE: fs/nilfs2/nilfs.h:338:
> > +extern int nilfs_store_magic(struct super_block *,
>
> same
When modifying declarations, I take the opportunity to add missing
identifier names to their arguments and delete "extern" specifiers to
eliminate checkpatch warnings, so I would appreciate it if you could
correct it as shown below.
int nilfs_store_magic(struct super_block *sb, struct nilfs_super_block *sbp);
>
> > WARNING: space prohibited between function name and open parenthesis '('
> > #220: FILE: fs/nilfs2/super.c:721:
> > + fsparam_enum ("errors", Opt_err, nilfs_param_err),
>
> This seems to be the pattern for every filesystem using these calls ...
Yes, I think it looks better now, please ignore these warnings.
>
> > WARNING: space prohibited between function name and open parenthesis '('
> > #221: FILE: fs/nilfs2/super.c:722:
> > + fsparam_flag_no ("barrier", Opt_barrier),
> >
> > WARNING: space prohibited between function name and open parenthesis '('
> > #223: FILE: fs/nilfs2/super.c:724:
> > + fsparam_string ("order", Opt_order),
> >
> > WARNING: space prohibited between function name and open parenthesis '('
> > #224: FILE: fs/nilfs2/super.c:725:
> > + fsparam_flag ("norecovery", Opt_norecovery),
> >
> > WARNING: space prohibited between function name and open parenthesis '('
> > #225: FILE: fs/nilfs2/super.c:726:
> > + fsparam_flag_no ("discard", Opt_discard),
> >
> > WARNING: Missing a blank line after declarations
> > #317: FILE: fs/nilfs2/super.c:770:
> > + struct super_block *sb = fc->root->d_sb;
> > + nilfs_err(sb,
>
> easy enough to fix.
>
> > WARNING: quoted string split across lines
> > #576: FILE: fs/nilfs2/super.c:1193:
> > + nilfs_err(s, "invalid option \"cn=%llu\", "
> > + "read-only option is not specified",
>
> Just let me know your preference on long strings like this (out-dent?
> go past col 80? leave it alone?)
Nowadays, when adding a new message or modifying an existing message,
I prioritize its searchability and allow past col 80 only for message
strings.
(I used to split messages to adhere to the 80 characters constraint. )
So, if you don't mind, please fix this checkpatch warning.
>
> I'll wait for more review before I send a minor update just for these.
> (or, if you prefer, feel free to tweak small things on your end.)
>
> thanks,
> -Eric
>
> > total: 0 errors, 10 warnings, 563 lines checked
> > ---
> >
> > Of these, the warning for the function declaration of
> > nilfs_store_magic() is an existing issue, so it can be left as is.
> >
> > Also, I feel like the warnings for fsparam_{enum,flag,flag_no,string}
> > can be ignored for the sake of appearance. (I will not omit them here
> > so as not to make any preconceptions).
> >
> > Thanks,
> > Ryusuke Konishi
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2] nilfs2: convert to use the new mount API
2024-04-09 19:54 ` Eric Sandeen
@ 2024-04-09 20:36 ` Ryusuke Konishi
2024-04-10 12:47 ` Ryusuke Konishi
1 sibling, 0 replies; 16+ messages in thread
From: Ryusuke Konishi @ 2024-04-09 20:36 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-nilfs
On Wed, Apr 10, 2024 at 4:54 AM Eric Sandeen wrote:
>
> On 4/9/24 2:13 PM, Ryusuke Konishi wrote:
> > Thank you for waiting. I've finished the full review.
> >
> > I'll comment below, inline.
> > First let me say that this patch is great and I don't see any points
> > that need major rewrites.
>
> Thanks!
>
> > Regarding style warnings, I will reply to that email later.
> >
> > On Fri, Apr 5, 2024 at 12:00 PM Eric Sandeen wrote:
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
>
> ...
>
> >> -static int parse_options(char *options, struct super_block *sb, int is_remount)
> >> -{
> >> - struct the_nilfs *nilfs = sb->s_fs_info;
> >> - char *p;
> >> - substring_t args[MAX_OPT_ARGS];
> >> -
> >> - if (!options)
> >> - return 1;
> >> -
> >> - while ((p = strsep(&options, ",")) != NULL) {
> >> - int token;
> >> +const struct fs_parameter_spec nilfs_param_spec[] = {
> >> + fsparam_enum ("errors", Opt_err, nilfs_param_err),
> >> + fsparam_flag_no ("barrier", Opt_barrier),
> >
> >> + fsparam_u32 ("cp", Opt_snapshot),
> >
> > Checkpoint numbers require a 64-bit unsigned integer parser
> > (originally parsed by kstrtoull()), so I think you should use
> > fsparam_u64 here.
> > Since nilfs_parse_param() was written assuming fsparam_u64, I guess
> > this is a simple mistake.
>
> Yep, I think I mechanically changed anything like
>
> {Opt_snapshot, "cp=%u"}
>
> to _u32, and missed how the actual parsing works.
>
> > Also, "nilfs_param_spec" is not declared with the "static" class
> > specifier, so could you please add it ?
>
> Sure thing
>
> ...
>
> >> + if (is_remount) {
> >> + struct super_block *sb = fc->root->d_sb;
> >> + nilfs_err(sb,
> >> + "\"%s\" option is invalid for remount",
> >> + param->key);
> >> + return -EINVAL;
> >> }
> >
> >> + if (result.uint_64 == 0)
> >> + return -EINVAL;
> >
> > For the case where the "cp=0" invalid option is specified, could you
> > please output the following error message with nilfs_err() as before ?
> >
> > "invalid option \"cp=0\": invalid checkpoint number 0"
> >
> > Other similar messages seem to overlap with the message output by
> > fs_parser routines, so I think just adding this one is sufficient.
>
> *nod* good catch
>
> ...
>
> >
> >> sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
> >
> > This "s_flags" adjustment overlaps with the flag adjustment just
> > before returning with normal status.
> > I think there is no problem with deleting this.
>
> Ah, I think you are right. Will make sure nothing depends on the sb
> flags before then, though.
>
> ...
>
> >> @@ -1180,130 +1163,57 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> >> root = NILFS_I(d_inode(sb->s_root))->i_root;
> >
> >> err = nilfs_attach_log_writer(sb, root);
> >> if (err)
> >> - goto restore_opts;
> >> + goto ignore_opts;
> >
> > Here, if nilfs_attach_log_writer() fails, it will return via
> > "ignore_opts" without restoring the SB_RDONLY flag in sb->s_flags.
> > I think it is necessary to repair the flag only for this error path,
> > what do you think?
>
> Again, I think you are right, although maybe if the above flags copy is
> moved to the end, it won't be a problem? I'll look more closely.
>
> ...
>
> >
> >> + if (ctx->cno && !(fc->sb_flags & SB_RDONLY)) {
> >> + nilfs_err(s, "invalid option \"cn=%llu\", "
> >> + "read-only option is not specified",
> >> + ctx->cno);
> >> + return -EINVAL;
> >> + }
> >
> > This error check conversion (to check after the read-only flag is
> > determined) is ok.
> > But, the option name in the error message has changed, so please correct it.
> > The original message looks like
> >
> > "invalid option \"cp=%llu\": read-only option is not specified"
>
> Whoops, I think that's just a typo.
>
> > The checkpoint number expression cannot be reproduced to be exactly
> > the same as the input (as in the case where the radix is specified
> > like "cp=0x123") but I think it's acceptable.
>
> Yup - that's a difference w/ the new mount API, I think - all of the mount
> options need to be parsed independently, and we can only look for invalid
> combinations after that's all done and by then we onlly have the value, not
> the original format or string.
>
> If you would like to keep the original format of the option, we could store
> it in the fs_context as a string (I think) and emit that in the error message.
> Worth it?
No, I don't think it's worth it, especially unnecessary if you need to
change the common parser implementation.
Almost all users will probably specify checkpoint numbers as decimal
integers and will not notice this difference.
Thanks,
Ryusuke Konishi
>
> Thanks,
>
> -Eric
>
> >
> > That's all for the non-style issue comments.
> >
> > Thanks,
> > Ryusuke Konishi
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2] nilfs2: convert to use the new mount API
2024-04-09 19:54 ` Eric Sandeen
2024-04-09 20:36 ` Ryusuke Konishi
@ 2024-04-10 12:47 ` Ryusuke Konishi
1 sibling, 0 replies; 16+ messages in thread
From: Ryusuke Konishi @ 2024-04-10 12:47 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-nilfs
On Wed, Apr 10, 2024 at 4:54 AM Eric Sandeen wrote:
>
> On 4/9/24 2:13 PM, Ryusuke Konishi wrote:
> > Thank you for waiting. I've finished the full review.
> >
> > I'll comment below, inline.
> > First let me say that this patch is great and I don't see any points
> > that need major rewrites.
>
> Thanks!
...
> >> @@ -1180,130 +1163,57 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
> >> root = NILFS_I(d_inode(sb->s_root))->i_root;
> >
> >> err = nilfs_attach_log_writer(sb, root);
> >> if (err)
> >> - goto restore_opts;
> >> + goto ignore_opts;
> >
> > Here, if nilfs_attach_log_writer() fails, it will return via
> > "ignore_opts" without restoring the SB_RDONLY flag in sb->s_flags.
> > I think it is necessary to repair the flag only for this error path,
> > what do you think?
>
> Again, I think you are right, although maybe if the above flags copy is
> moved to the end, it won't be a problem? I'll look more closely.
I also dug into the code to see if we could move the flag manipulation
backwards.
nilfs_attach_log_writer() is responsible for starting the log writer
thread, and this itself can be executed without being affected by the
SB_RDONLY flag.
In fact, when I tested it with such a change, the read-write remount
completed without any problems.
However, since the behavior of the log writer thread is affected by
the SB_RDONLY flag, there is a risk of introducing potential
regressions.
In conclusion, it's probably okay (unless you want to avoid even the
slightest risk).
Below are the details.
The first possible side effect is that log writing may start for some
reason before the SB_RDONLY flag is unset, and it will fail due to the
SB_RDONLY flag. The call path for this is as follows.
nilfs_segctor_thread()
nilfs_segctor_thread_construct()
nilfs_segctor_construct()
nilfs_segctor_do_construct() --> sb_rdonly() check fails and
returns -EROFS
The log writer thread ignores the error and does not output any
messages, and the write request (if any) will fail, but this is
expected behavior at this stage before the transition to read/write
mode is complete, so it seems ok.
The second possible side effect is that the log writer thread calls
iput(), which in turn calls nilfs_evict() through iput_final(), which
causes inode metadata removal to be incomplete due to the detection of
the SB_RDONLY flag.
This should not normally occur since inode->i_nlink does not fall to 0
in the read-only state, and even if it does occur, it can be
interpreted as the intended behavior since the transition to
read/write mode has not yet been completed.
Even if there is a problem, only syzbot will probably be able to detect it.
And If that concern turns out to be true, I can deal with it.
Sorry for being roundabout, but the bottom line is that the change you
think is OK.
Thanks,
Ryusuke Konishi
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] nilfs2: convert to use the new mount API
@ 2024-04-24 18:27 Ryusuke Konishi
2024-04-25 8:58 ` Ryusuke Konishi
0 siblings, 1 reply; 16+ messages in thread
From: Ryusuke Konishi @ 2024-04-24 18:27 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-nilfs, linux-kernel, linux-fsdevel, Eric Sandeen
From: Eric Sandeen <sandeen@redhat.com>
Convert nilfs2 to use the new mount API.
[konishi.ryusuke: fixed missing SB_RDONLY flag repair in nilfs_reconfigure]
Link: https://lkml.kernel.org/r/33d078a7-9072-4d8e-a3a9-dec23d4191da@redhat.com
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
---
Hi Andrew, please add this to the queue for the next merge window.
As the title suggests, this patch transforms the implementation around
nilfs2 mount/remount, so I reviewed all changes and tested all mount
options and nilfs2-specific mount patterns (such as simultaneous
mounting of snapshots and current tree). This one passed those checks.
Thanks,
Ryusuke Konishi
fs/nilfs2/nilfs.h | 4 +-
fs/nilfs2/super.c | 374 ++++++++++++++++++------------------------
fs/nilfs2/the_nilfs.c | 4 +-
fs/nilfs2/the_nilfs.h | 6 +-
4 files changed, 164 insertions(+), 224 deletions(-)
diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
index 2e29b98ba8ba..728e90be3570 100644
--- a/fs/nilfs2/nilfs.h
+++ b/fs/nilfs2/nilfs.h
@@ -335,8 +335,8 @@ void __nilfs_error(struct super_block *sb, const char *function,
extern struct nilfs_super_block *
nilfs_read_super_block(struct super_block *, u64, int, struct buffer_head **);
-extern int nilfs_store_magic_and_option(struct super_block *,
- struct nilfs_super_block *, char *);
+extern int nilfs_store_magic(struct super_block *sb,
+ struct nilfs_super_block *sbp);
extern int nilfs_check_feature_compatibility(struct super_block *,
struct nilfs_super_block *);
extern void nilfs_set_log_cursor(struct nilfs_super_block *,
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index ac24ed109ce9..55667ab6706d 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -29,7 +29,8 @@
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/blkdev.h>
-#include <linux/parser.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
#include <linux/crc32.h>
#include <linux/vfs.h>
#include <linux/writeback.h>
@@ -61,7 +62,6 @@ struct kmem_cache *nilfs_segbuf_cachep;
struct kmem_cache *nilfs_btree_path_cache;
static int nilfs_setup_super(struct super_block *sb, int is_mount);
-static int nilfs_remount(struct super_block *sb, int *flags, char *data);
void __nilfs_msg(struct super_block *sb, const char *fmt, ...)
{
@@ -702,105 +702,98 @@ static const struct super_operations nilfs_sops = {
.freeze_fs = nilfs_freeze,
.unfreeze_fs = nilfs_unfreeze,
.statfs = nilfs_statfs,
- .remount_fs = nilfs_remount,
.show_options = nilfs_show_options
};
enum {
- Opt_err_cont, Opt_err_panic, Opt_err_ro,
- Opt_barrier, Opt_nobarrier, Opt_snapshot, Opt_order, Opt_norecovery,
- Opt_discard, Opt_nodiscard, Opt_err,
+ Opt_err, Opt_barrier, Opt_snapshot, Opt_order, Opt_norecovery,
+ Opt_discard,
};
-static match_table_t tokens = {
- {Opt_err_cont, "errors=continue"},
- {Opt_err_panic, "errors=panic"},
- {Opt_err_ro, "errors=remount-ro"},
- {Opt_barrier, "barrier"},
- {Opt_nobarrier, "nobarrier"},
- {Opt_snapshot, "cp=%u"},
- {Opt_order, "order=%s"},
- {Opt_norecovery, "norecovery"},
- {Opt_discard, "discard"},
- {Opt_nodiscard, "nodiscard"},
- {Opt_err, NULL}
+static const struct constant_table nilfs_param_err[] = {
+ {"continue", NILFS_MOUNT_ERRORS_CONT},
+ {"panic", NILFS_MOUNT_ERRORS_PANIC},
+ {"remount-ro", NILFS_MOUNT_ERRORS_RO},
+ {}
};
-static int parse_options(char *options, struct super_block *sb, int is_remount)
-{
- struct the_nilfs *nilfs = sb->s_fs_info;
- char *p;
- substring_t args[MAX_OPT_ARGS];
-
- if (!options)
- return 1;
-
- while ((p = strsep(&options, ",")) != NULL) {
- int token;
+static const struct fs_parameter_spec nilfs_param_spec[] = {
+ fsparam_enum ("errors", Opt_err, nilfs_param_err),
+ fsparam_flag_no ("barrier", Opt_barrier),
+ fsparam_u64 ("cp", Opt_snapshot),
+ fsparam_string ("order", Opt_order),
+ fsparam_flag ("norecovery", Opt_norecovery),
+ fsparam_flag_no ("discard", Opt_discard),
+ {}
+};
- if (!*p)
- continue;
+struct nilfs_fs_context {
+ unsigned long ns_mount_opt;
+ __u64 cno;
+};
- token = match_token(p, tokens, args);
- switch (token) {
- case Opt_barrier:
- nilfs_set_opt(nilfs, BARRIER);
- break;
- case Opt_nobarrier:
+static int nilfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
+{
+ struct nilfs_fs_context *nilfs = fc->fs_private;
+ int is_remount = fc->purpose == FS_CONTEXT_FOR_RECONFIGURE;
+ struct fs_parse_result result;
+ int opt;
+
+ opt = fs_parse(fc, nilfs_param_spec, param, &result);
+ if (opt < 0)
+ return opt;
+
+ switch (opt) {
+ case Opt_barrier:
+ if (result.negated)
nilfs_clear_opt(nilfs, BARRIER);
- break;
- case Opt_order:
- if (strcmp(args[0].from, "relaxed") == 0)
- /* Ordered data semantics */
- nilfs_clear_opt(nilfs, STRICT_ORDER);
- else if (strcmp(args[0].from, "strict") == 0)
- /* Strict in-order semantics */
- nilfs_set_opt(nilfs, STRICT_ORDER);
- else
- return 0;
- break;
- case Opt_err_panic:
- nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_PANIC);
- break;
- case Opt_err_ro:
- nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_RO);
- break;
- case Opt_err_cont:
- nilfs_write_opt(nilfs, ERROR_MODE, ERRORS_CONT);
- break;
- case Opt_snapshot:
- if (is_remount) {
- nilfs_err(sb,
- "\"%s\" option is invalid for remount",
- p);
- return 0;
- }
- break;
- case Opt_norecovery:
- nilfs_set_opt(nilfs, NORECOVERY);
- break;
- case Opt_discard:
- nilfs_set_opt(nilfs, DISCARD);
- break;
- case Opt_nodiscard:
- nilfs_clear_opt(nilfs, DISCARD);
- break;
- default:
- nilfs_err(sb, "unrecognized mount option \"%s\"", p);
- return 0;
+ else
+ nilfs_set_opt(nilfs, BARRIER);
+ break;
+ case Opt_order:
+ if (strcmp(param->string, "relaxed") == 0)
+ /* Ordered data semantics */
+ nilfs_clear_opt(nilfs, STRICT_ORDER);
+ else if (strcmp(param->string, "strict") == 0)
+ /* Strict in-order semantics */
+ nilfs_set_opt(nilfs, STRICT_ORDER);
+ else
+ return -EINVAL;
+ break;
+ case Opt_err:
+ nilfs->ns_mount_opt &= ~NILFS_MOUNT_ERROR_MODE;
+ nilfs->ns_mount_opt |= result.uint_32;
+ break;
+ case Opt_snapshot:
+ if (is_remount) {
+ struct super_block *sb = fc->root->d_sb;
+
+ nilfs_err(sb,
+ "\"%s\" option is invalid for remount",
+ param->key);
+ return -EINVAL;
+ }
+ if (result.uint_64 == 0) {
+ nilfs_err(NULL,
+ "invalid option \"cp=0\": invalid checkpoint number 0");
+ return -EINVAL;
}
+ nilfs->cno = result.uint_64;
+ break;
+ case Opt_norecovery:
+ nilfs_set_opt(nilfs, NORECOVERY);
+ break;
+ case Opt_discard:
+ if (result.negated)
+ nilfs_clear_opt(nilfs, DISCARD);
+ else
+ nilfs_set_opt(nilfs, DISCARD);
+ break;
+ default:
+ return -EINVAL;
}
- return 1;
-}
-static inline void
-nilfs_set_default_options(struct super_block *sb,
- struct nilfs_super_block *sbp)
-{
- struct the_nilfs *nilfs = sb->s_fs_info;
-
- nilfs->ns_mount_opt =
- NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
+ return 0;
}
static int nilfs_setup_super(struct super_block *sb, int is_mount)
@@ -857,9 +850,8 @@ struct nilfs_super_block *nilfs_read_super_block(struct super_block *sb,
return (struct nilfs_super_block *)((char *)(*pbh)->b_data + offset);
}
-int nilfs_store_magic_and_option(struct super_block *sb,
- struct nilfs_super_block *sbp,
- char *data)
+int nilfs_store_magic(struct super_block *sb,
+ struct nilfs_super_block *sbp)
{
struct the_nilfs *nilfs = sb->s_fs_info;
@@ -870,14 +862,12 @@ int nilfs_store_magic_and_option(struct super_block *sb,
sb->s_flags |= SB_NOATIME;
#endif
- nilfs_set_default_options(sb, sbp);
-
nilfs->ns_resuid = le16_to_cpu(sbp->s_def_resuid);
nilfs->ns_resgid = le16_to_cpu(sbp->s_def_resgid);
nilfs->ns_interval = le32_to_cpu(sbp->s_c_interval);
nilfs->ns_watermark = le32_to_cpu(sbp->s_c_block_max);
- return !parse_options(data, sb, 0) ? -EINVAL : 0;
+ return 0;
}
int nilfs_check_feature_compatibility(struct super_block *sb,
@@ -1042,10 +1032,11 @@ int nilfs_checkpoint_is_mounted(struct super_block *sb, __u64 cno)
* So, the recovery process is protected from other simultaneous mounts.
*/
static int
-nilfs_fill_super(struct super_block *sb, void *data, int silent)
+nilfs_fill_super(struct super_block *sb, struct fs_context *fc)
{
struct the_nilfs *nilfs;
struct nilfs_root *fsroot;
+ struct nilfs_fs_context *ctx = fc->fs_private;
__u64 cno;
int err;
@@ -1055,10 +1046,13 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
sb->s_fs_info = nilfs;
- err = init_nilfs(nilfs, sb, (char *)data);
+ err = init_nilfs(nilfs, sb);
if (err)
goto failed_nilfs;
+ /* Copy in parsed mount options */
+ nilfs->ns_mount_opt = ctx->ns_mount_opt;
+
sb->s_op = &nilfs_sops;
sb->s_export_op = &nilfs_export_ops;
sb->s_root = NULL;
@@ -1117,34 +1111,25 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent)
return err;
}
-static int nilfs_remount(struct super_block *sb, int *flags, char *data)
+static int nilfs_reconfigure(struct fs_context *fc)
{
+ struct nilfs_fs_context *ctx = fc->fs_private;
+ struct super_block *sb = fc->root->d_sb;
struct the_nilfs *nilfs = sb->s_fs_info;
- unsigned long old_sb_flags;
- unsigned long old_mount_opt;
int err;
sync_filesystem(sb);
- old_sb_flags = sb->s_flags;
- old_mount_opt = nilfs->ns_mount_opt;
-
- if (!parse_options(data, sb, 1)) {
- err = -EINVAL;
- goto restore_opts;
- }
- sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
err = -EINVAL;
if (!nilfs_valid_fs(nilfs)) {
nilfs_warn(sb,
"couldn't remount because the filesystem is in an incomplete recovery state");
- goto restore_opts;
+ goto ignore_opts;
}
-
- if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
+ if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
goto out;
- if (*flags & SB_RDONLY) {
+ if (fc->sb_flags & SB_RDONLY) {
sb->s_flags |= SB_RDONLY;
/*
@@ -1172,138 +1157,66 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
"couldn't remount RDWR because of unsupported optional features (%llx)",
(unsigned long long)features);
err = -EROFS;
- goto restore_opts;
+ goto ignore_opts;
}
sb->s_flags &= ~SB_RDONLY;
root = NILFS_I(d_inode(sb->s_root))->i_root;
err = nilfs_attach_log_writer(sb, root);
- if (err)
- goto restore_opts;
+ if (err) {
+ sb->s_flags |= SB_RDONLY;
+ goto ignore_opts;
+ }
down_write(&nilfs->ns_sem);
nilfs_setup_super(sb, true);
up_write(&nilfs->ns_sem);
}
out:
- return 0;
-
- restore_opts:
- sb->s_flags = old_sb_flags;
- nilfs->ns_mount_opt = old_mount_opt;
- return err;
-}
-
-struct nilfs_super_data {
- __u64 cno;
- int flags;
-};
-
-static int nilfs_parse_snapshot_option(const char *option,
- const substring_t *arg,
- struct nilfs_super_data *sd)
-{
- unsigned long long val;
- const char *msg = NULL;
- int err;
-
- if (!(sd->flags & SB_RDONLY)) {
- msg = "read-only option is not specified";
- goto parse_error;
- }
-
- err = kstrtoull(arg->from, 0, &val);
- if (err) {
- if (err == -ERANGE)
- msg = "too large checkpoint number";
- else
- msg = "malformed argument";
- goto parse_error;
- } else if (val == 0) {
- msg = "invalid checkpoint number 0";
- goto parse_error;
- }
- sd->cno = val;
- return 0;
-
-parse_error:
- nilfs_err(NULL, "invalid option \"%s\": %s", option, msg);
- return 1;
-}
-
-/**
- * nilfs_identify - pre-read mount options needed to identify mount instance
- * @data: mount options
- * @sd: nilfs_super_data
- */
-static int nilfs_identify(char *data, struct nilfs_super_data *sd)
-{
- char *p, *options = data;
- substring_t args[MAX_OPT_ARGS];
- int token;
- int ret = 0;
-
- do {
- p = strsep(&options, ",");
- if (p != NULL && *p) {
- token = match_token(p, tokens, args);
- if (token == Opt_snapshot)
- ret = nilfs_parse_snapshot_option(p, &args[0],
- sd);
- }
- if (!options)
- break;
- BUG_ON(options == data);
- *(options - 1) = ',';
- } while (!ret);
- return ret;
-}
+ sb->s_flags = (sb->s_flags & ~SB_POSIXACL);
+ /* Copy over parsed remount options */
+ nilfs->ns_mount_opt = ctx->ns_mount_opt;
-static int nilfs_set_bdev_super(struct super_block *s, void *data)
-{
- s->s_dev = *(dev_t *)data;
return 0;
-}
-static int nilfs_test_bdev_super(struct super_block *s, void *data)
-{
- return !(s->s_iflags & SB_I_RETIRED) && s->s_dev == *(dev_t *)data;
+ ignore_opts:
+ return err;
}
-static struct dentry *
-nilfs_mount(struct file_system_type *fs_type, int flags,
- const char *dev_name, void *data)
+static int
+nilfs_get_tree(struct fs_context *fc)
{
- struct nilfs_super_data sd = { .flags = flags };
+ struct nilfs_fs_context *ctx = fc->fs_private;
struct super_block *s;
dev_t dev;
int err;
- if (nilfs_identify(data, &sd))
- return ERR_PTR(-EINVAL);
+ if (ctx->cno && !(fc->sb_flags & SB_RDONLY)) {
+ nilfs_err(s, "invalid option \"cp=%llu\": read-only option is not specified",
+ ctx->cno);
+ return -EINVAL;
+ }
- err = lookup_bdev(dev_name, &dev);
+ err = lookup_bdev(fc->source, &dev);
if (err)
- return ERR_PTR(err);
+ return err;
- s = sget(fs_type, nilfs_test_bdev_super, nilfs_set_bdev_super, flags,
- &dev);
+ s = sget_dev(fc, dev);
if (IS_ERR(s))
- return ERR_CAST(s);
+ return PTR_ERR(s);
if (!s->s_root) {
- err = setup_bdev_super(s, flags, NULL);
+ err = setup_bdev_super(s, fc->sb_flags, fc);
if (!err)
- err = nilfs_fill_super(s, data,
- flags & SB_SILENT ? 1 : 0);
+ err = nilfs_fill_super(s, fc);
if (err)
goto failed_super;
s->s_flags |= SB_ACTIVE;
- } else if (!sd.cno) {
+ } else if (!ctx->cno) {
if (nilfs_tree_is_busy(s->s_root)) {
- if ((flags ^ s->s_flags) & SB_RDONLY) {
+ if ((fc->sb_flags ^ s->s_flags) & SB_RDONLY) {
nilfs_err(s,
"the device already has a %s mount.",
sb_rdonly(s) ? "read-only" : "read/write");
@@ -1315,34 +1228,65 @@ nilfs_mount(struct file_system_type *fs_type, int flags,
* Try remount to setup mount states if the current
* tree is not mounted and only snapshots use this sb.
*/
- err = nilfs_remount(s, &flags, data);
+ fc->root = s->s_root;
+ err = nilfs_reconfigure(fc);
if (err)
goto failed_super;
}
}
- if (sd.cno) {
+ if (ctx->cno) {
struct dentry *root_dentry;
- err = nilfs_attach_snapshot(s, sd.cno, &root_dentry);
+ err = nilfs_attach_snapshot(s, ctx->cno, &root_dentry);
if (err)
goto failed_super;
- return root_dentry;
+ fc->root = root_dentry;
+ return 0;
}
- return dget(s->s_root);
+ fc->root = dget(s->s_root);
+ return 0;
failed_super:
deactivate_locked_super(s);
- return ERR_PTR(err);
+ return err;
+}
+
+static void nilfs_free_fc(struct fs_context *fc)
+{
+ kfree(fc->fs_private);
+}
+
+static const struct fs_context_operations nilfs_context_ops = {
+ .parse_param = nilfs_parse_param,
+ .get_tree = nilfs_get_tree,
+ .reconfigure = nilfs_reconfigure,
+ .free = nilfs_free_fc,
+};
+
+static int nilfs_init_fs_context(struct fs_context *fc)
+{
+ struct nilfs_fs_context *ctx;
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->ns_mount_opt = NILFS_MOUNT_ERRORS_RO | NILFS_MOUNT_BARRIER;
+ fc->fs_private = ctx;
+ fc->ops = &nilfs_context_ops;
+
+ return 0;
}
struct file_system_type nilfs_fs_type = {
.owner = THIS_MODULE,
.name = "nilfs2",
- .mount = nilfs_mount,
.kill_sb = kill_block_super,
.fs_flags = FS_REQUIRES_DEV,
+ .init_fs_context = nilfs_init_fs_context,
+ .parameters = nilfs_param_spec,
};
MODULE_ALIAS_FS("nilfs2");
diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 2ae2c1bbf6d1..77fce1f509d1 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -668,7 +668,7 @@ static int nilfs_load_super_block(struct the_nilfs *nilfs,
* Return Value: On success, 0 is returned. On error, a negative error
* code is returned.
*/
-int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
+int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb)
{
struct nilfs_super_block *sbp;
int blocksize;
@@ -686,7 +686,7 @@ int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data)
if (err)
goto out;
- err = nilfs_store_magic_and_option(sb, sbp, data);
+ err = nilfs_store_magic(sb, sbp);
if (err)
goto failed_sbh;
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index cd4ae1b8ae16..85da0629415d 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -219,10 +219,6 @@ THE_NILFS_FNS(PURGING, purging)
#define nilfs_set_opt(nilfs, opt) \
((nilfs)->ns_mount_opt |= NILFS_MOUNT_##opt)
#define nilfs_test_opt(nilfs, opt) ((nilfs)->ns_mount_opt & NILFS_MOUNT_##opt)
-#define nilfs_write_opt(nilfs, mask, opt) \
- ((nilfs)->ns_mount_opt = \
- (((nilfs)->ns_mount_opt & ~NILFS_MOUNT_##mask) | \
- NILFS_MOUNT_##opt)) \
/**
* struct nilfs_root - nilfs root object
@@ -276,7 +272,7 @@ static inline int nilfs_sb_will_flip(struct the_nilfs *nilfs)
void nilfs_set_last_segment(struct the_nilfs *, sector_t, u64, __u64);
struct the_nilfs *alloc_nilfs(struct super_block *sb);
void destroy_nilfs(struct the_nilfs *nilfs);
-int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb, char *data);
+int init_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
int load_nilfs(struct the_nilfs *nilfs, struct super_block *sb);
unsigned long nilfs_nrsvsegs(struct the_nilfs *nilfs, unsigned long nsegs);
void nilfs_set_nsegments(struct the_nilfs *nilfs, unsigned long nsegs);
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] nilfs2: convert to use the new mount API
2024-04-24 18:27 [PATCH] " Ryusuke Konishi
@ 2024-04-25 8:58 ` Ryusuke Konishi
0 siblings, 0 replies; 16+ messages in thread
From: Ryusuke Konishi @ 2024-04-25 8:58 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Sandeen, linux-nilfs, linux-kernel, linux-fsdevel
On Thu, Apr 25, 2024 at 3:27 AM Ryusuke Konishi wrote:
>
> From: Eric Sandeen <sandeen@redhat.com>
>
> Convert nilfs2 to use the new mount API.
>
> [konishi.ryusuke: fixed missing SB_RDONLY flag repair in nilfs_reconfigure]
> Link: https://lkml.kernel.org/r/33d078a7-9072-4d8e-a3a9-dec23d4191da@redhat.com
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> ---
> Hi Andrew, please add this to the queue for the next merge window.
>
> As the title suggests, this patch transforms the implementation around
> nilfs2 mount/remount, so I reviewed all changes and tested all mount
> options and nilfs2-specific mount patterns (such as simultaneous
> mounting of snapshots and current tree). This one passed those checks.
>
> Thanks,
> Ryusuke Konishi
Hi Andrew, please once drop this patch from the -mm tree.
I found one bug causing a kernel bug in additional fault injection
testing, and also received issue reports from the 0-DAY CI Kernel Test
Service.
I will fix these issues and resend you a revised patch.
Thanks,
Ryusuke Konishi
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2024-04-25 8:59 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-03 22:12 [PATCH] nilfs2: convert to use the new mount API Eric Sandeen
2024-04-04 20:11 ` Ryusuke Konishi
2024-04-04 20:35 ` Eric Sandeen
2024-04-04 22:33 ` Ryusuke Konishi
2024-04-05 1:21 ` Eric Sandeen
2024-04-05 1:46 ` Eric Sandeen
2024-04-05 3:00 ` [PATCH V2] " Eric Sandeen
2024-04-05 10:33 ` Ryusuke Konishi
2024-04-05 15:03 ` Eric Sandeen
2024-04-09 20:17 ` Ryusuke Konishi
2024-04-09 19:13 ` Ryusuke Konishi
2024-04-09 19:54 ` Eric Sandeen
2024-04-09 20:36 ` Ryusuke Konishi
2024-04-10 12:47 ` Ryusuke Konishi
-- strict thread matches above, loose matches on Subject: below --
2024-04-24 18:27 [PATCH] " Ryusuke Konishi
2024-04-25 8:58 ` Ryusuke Konishi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).