On Wed, Apr 10, 2024 at 4:54 AM Eric Sandeen wrote:
>
> 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!
...
> >> @@ -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.
I also dug into the code to see if we could move the flag manipulation
backwards.
nilfs_attach_log_writer() is responsible for starting the log writer
thread, and this itself can be executed without being affected by the
SB_RDONLY flag.
In fact, when I tested it with such a change, the read-write remount
completed without any problems.
However, since the behavior of the log writer thread is affected by
the SB_RDONLY flag, there is a risk of introducing potential
regressions.
In conclusion, it's probably okay (unless you want to avoid even the
slightest risk).
Below are the details.
The first possible side effect is that log writing may start for some
reason before the SB_RDONLY flag is unset, and it will fail due to the
SB_RDONLY flag. The call path for this is as follows.
nilfs_segctor_thread()
nilfs_segctor_thread_construct()
nilfs_segctor_construct()
nilfs_segctor_do_construct() --> sb_rdonly() check fails and
returns -EROFS
The log writer thread ignores the error and does not output any
messages, and the write request (if any) will fail, but this is
expected behavior at this stage before the transition to read/write
mode is complete, so it seems ok.
The second possible side effect is that the log writer thread calls
iput(), which in turn calls nilfs_evict() through iput_final(), which
causes inode metadata removal to be incomplete due to the detection of
the SB_RDONLY flag.
This should not normally occur since inode->i_nlink does not fall to 0
in the read-only state, and even if it does occur, it can be
interpreted as the intended behavior since the transition to
read/write mode has not yet been completed.
Even if there is a problem, only syzbot will probably be able to detect it.
And If that concern turns out to be true, I can deal with it.
Sorry for being roundabout, but the bottom line is that the change you
think is OK.
Thanks,
Ryusuke Konishi
