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