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@xxxxxxxxxx>
> ---
>
> 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
