From: Ryusuke Konishi <konishi.ryusuke@gmail.com>
To: Eric Sandeen <sandeen@redhat.com>
Cc: linux-nilfs@vger.kernel.org
Subject: Re: [PATCH V2] nilfs2: convert to use the new mount API
Date: Wed, 10 Apr 2024 21:47:47 +0900 [thread overview]
Message-ID: <CAKFNMo=4NP_3-YebtVO-OcxzxCNvjSNAW_PY-Uv-7wvdgvxp9w@mail.gmail.com> (raw)
In-Reply-To: <55663cac-42e0-4be6-9f3a-e3f9f3d1ab50@redhat.com>
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
prev parent reply other threads:[~2024-04-10 12:48 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-03 22:12 [PATCH] nilfs2: convert to use the new mount API Eric Sandeen
2024-04-04 20:11 ` Ryusuke Konishi
2024-04-04 20:35 ` Eric Sandeen
2024-04-04 22:33 ` Ryusuke Konishi
2024-04-05 1:21 ` Eric Sandeen
2024-04-05 1:46 ` Eric Sandeen
2024-04-05 3:00 ` [PATCH V2] " Eric Sandeen
2024-04-05 10:33 ` Ryusuke Konishi
2024-04-05 15:03 ` Eric Sandeen
2024-04-09 20:17 ` Ryusuke Konishi
2024-04-09 19:13 ` Ryusuke Konishi
2024-04-09 19:54 ` Eric Sandeen
2024-04-09 20:36 ` Ryusuke Konishi
2024-04-10 12:47 ` Ryusuke Konishi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKFNMo=4NP_3-YebtVO-OcxzxCNvjSNAW_PY-Uv-7wvdgvxp9w@mail.gmail.com' \
--to=konishi.ryusuke@gmail.com \
--cc=linux-nilfs@vger.kernel.org \
--cc=sandeen@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).