* [PATCH V3] nilfs2: convert to use the new mount API
@ 2024-04-18 16:45 Eric Sandeen
2024-04-19 20:12 ` Ryusuke Konishi
0 siblings, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2024-04-18 16:45 UTC (permalink / raw)
To: linux-nilfs; +Cc: Ryusuke Konishi
Convert nilfs2 to use the new mount API.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
Sorry for the delay on resend, other work got in the way...
This all paged out a bit but I think I addressed all of the review
items. Thanks!
V3:
- Add identifiers to nilfs_store_magic() prototype
- switch Opt_snapshot to use fsparam_u64 vs. fsparam_u32
- add error message for invalid cp=0 case
- remove early sb->s_flags change in nilfs2_reconfigure()
- un-split error message string in nilfs_get_tree()
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..a54fa43331f5 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,7 +1157,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 +1165,56 @@ 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 \"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 +1226,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] 4+ messages in thread
* Re: [PATCH V3] nilfs2: convert to use the new mount API
2024-04-18 16:45 [PATCH V3] nilfs2: convert to use the new mount API Eric Sandeen
@ 2024-04-19 20:12 ` Ryusuke Konishi
2024-04-19 22:06 ` Eric Sandeen
0 siblings, 1 reply; 4+ messages in thread
From: Ryusuke Konishi @ 2024-04-19 20:12 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-nilfs
On Fri, Apr 19, 2024 at 1:45 AM Eric Sandeen wrote:
>
> Convert nilfs2 to use the new mount API.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> Sorry for the delay on resend, other work got in the way...
> This all paged out a bit but I think I addressed all of the review
> items. Thanks!
>
> V3:
> - Add identifiers to nilfs_store_magic() prototype
> - switch Opt_snapshot to use fsparam_u64 vs. fsparam_u32
> - add error message for invalid cp=0 case
> - remove early sb->s_flags change in nilfs2_reconfigure()
> - un-split error message string in nilfs_get_tree()
>
Thank you for the revised patch!
...
> + 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;
At first glance, I wondered why the nilfs_err() super_block instance
argument was NULL here, but I see, it can only be used by remount..
...
> @@ -1172,7 +1157,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 +1165,56 @@ 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);
> }
There is still an issue where the SB_RDONLY flag on sb->s_flags is not
repaired in the error path of nilfs_attach_log_writer().
This seems to be the only essential issue remaining, so I can add the
following fix (safer one - drop the SB_RDONLY flag for
nilfs_attach_log_writer, call it, and repair the flag if it fails),
and send it upstream.
Is this okay? Please let me know if you have any opinions.
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index a54fa43331f5..a8f03c860e87 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -1164,8 +1164,10 @@ static int nilfs_reconfigure(struct fs_context *fc)
root = NILFS_I(d_inode(sb->s_root))->i_root;
err = nilfs_attach_log_writer(sb, root);
- if (err)
+ if (err) {
+ sb->s_flags |= SB_RDONLY;
goto ignore_opts;
+ }
down_write(&nilfs->ns_sem);
nilfs_setup_super(sb, true);
Thanks,
Ryusuke Konishi
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH V3] nilfs2: convert to use the new mount API
2024-04-19 20:12 ` Ryusuke Konishi
@ 2024-04-19 22:06 ` Eric Sandeen
2024-04-20 13:57 ` Ryusuke Konishi
0 siblings, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2024-04-19 22:06 UTC (permalink / raw)
To: Ryusuke Konishi; +Cc: linux-nilfs
On 4/19/24 3:12 PM, Ryusuke Konishi wrote:
> On Fri, Apr 19, 2024 at 1:45 AM Eric Sandeen wrote:
>>
>> Convert nilfs2 to use the new mount API.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> + if (result.uint_64 == 0) {
>> + nilfs_err(NULL,
>> + "invalid option \"cp=0\": invalid checkpoint number 0");
>> + return -EINVAL;
>
> At first glance, I wondered why the nilfs_err() super_block instance
> argument was NULL here, but I see, it can only be used by remount..
Yup that's right, no sb yet on initial parsing. Hopefully the message
is ok this way. There is also an option to emit mount option errors through
the API, but nothing is listening for that yet.
> ...
>> @@ -1172,7 +1157,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 +1165,56 @@ 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);
>> }
>
> There is still an issue where the SB_RDONLY flag on sb->s_flags is not
> repaired in the error path of nilfs_attach_log_writer().
>
> This seems to be the only essential issue remaining, so I can add the
> following fix (safer one - drop the SB_RDONLY flag for
> nilfs_attach_log_writer, call it, and repair the flag if it fails),
> and send it upstream.
> Is this okay? Please let me know if you have any opinions.
>
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index a54fa43331f5..a8f03c860e87 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -1164,8 +1164,10 @@ static int nilfs_reconfigure(struct fs_context *fc)
>
> root = NILFS_I(d_inode(sb->s_root))->i_root;
> err = nilfs_attach_log_writer(sb, root);
> - if (err)
> + if (err) {
> + sb->s_flags |= SB_RDONLY;
> goto ignore_opts;
> + }
>
> down_write(&nilfs->ns_sem);
> nilfs_setup_super(sb, true);
Oh, I'm sorry I missed that :( Yes, I think that looks fine. Thank you.
-Eric
>
> Thanks,
> Ryusuke Konishi
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V3] nilfs2: convert to use the new mount API
2024-04-19 22:06 ` Eric Sandeen
@ 2024-04-20 13:57 ` Ryusuke Konishi
0 siblings, 0 replies; 4+ messages in thread
From: Ryusuke Konishi @ 2024-04-20 13:57 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-nilfs
On Sat, Apr 20, 2024 at 7:06 AM Eric Sandeen wrote:
>
> On 4/19/24 3:12 PM, Ryusuke Konishi wrote:
> > On Fri, Apr 19, 2024 at 1:45 AM Eric Sandeen wrote:
> >>
> >> Convert nilfs2 to use the new mount API.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>
>
> >> + if (result.uint_64 == 0) {
> >> + nilfs_err(NULL,
> >> + "invalid option \"cp=0\": invalid checkpoint number 0");
> >> + return -EINVAL;
> >
> > At first glance, I wondered why the nilfs_err() super_block instance
> > argument was NULL here, but I see, it can only be used by remount..
>
> Yup that's right, no sb yet on initial parsing. Hopefully the message
> is ok this way. There is also an option to emit mount option errors through
> the API, but nothing is listening for that yet.
The above implementation is fine (the original
nilfs_parse_snapshot_option() also does not output the device name, so
nothing is lost). I just found it interesting.
>
> > ...
> >> @@ -1172,7 +1157,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 +1165,56 @@ 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);
> >> }
> >
> > There is still an issue where the SB_RDONLY flag on sb->s_flags is not
> > repaired in the error path of nilfs_attach_log_writer().
> >
> > This seems to be the only essential issue remaining, so I can add the
> > following fix (safer one - drop the SB_RDONLY flag for
> > nilfs_attach_log_writer, call it, and repair the flag if it fails),
> > and send it upstream.
> > Is this okay? Please let me know if you have any opinions.
> >
> > diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> > index a54fa43331f5..a8f03c860e87 100644
> > --- a/fs/nilfs2/super.c
> > +++ b/fs/nilfs2/super.c
> > @@ -1164,8 +1164,10 @@ static int nilfs_reconfigure(struct fs_context *fc)
> >
> > root = NILFS_I(d_inode(sb->s_root))->i_root;
> > err = nilfs_attach_log_writer(sb, root);
> > - if (err)
> > + if (err) {
> > + sb->s_flags |= SB_RDONLY;
> > goto ignore_opts;
> > + }
> >
> > down_write(&nilfs->ns_sem);
> > nilfs_setup_super(sb, true);
>
> Oh, I'm sorry I missed that :( Yes, I think that looks fine. Thank you.
>
> -Eric
Good.
Well, I'll continue testing, so I'll keep this patch for now and let
you know if anything happens.
I would like to send it to the -mm tree as soon as possible.
Thank you!
Ryusuke Konishi
>
> >
> > Thanks,
> > Ryusuke Konishi
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-04-20 13:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-18 16:45 [PATCH V3] nilfs2: convert to use the new mount API Eric Sandeen
2024-04-19 20:12 ` Ryusuke Konishi
2024-04-19 22:06 ` Eric Sandeen
2024-04-20 13:57 ` Ryusuke Konishi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.