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