Re: [PATCH V2] nilfs2: convert to use the new mount API

[Eric Sandeen <sandeen@redhat.com>]

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