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

[Ryusuke Konishi <konishi.ryusuke@gmail.com>]

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