From: Amir Goldstein <amir73il@gmail.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jan Kara <jack@suse.cz>,
linux-unionfs@vger.kernel.org,
Christian Brauner <brauner@kernel.org>
Subject: Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
Date: Fri, 8 Sep 2023 15:29:19 +0300 [thread overview]
Message-ID: <CAOQ4uxis1UR36rJ-sLsgfk4mHhTOn_uM3xhSGbR08G0auzbhxQ@mail.gmail.com> (raw)
In-Reply-To: <CAOQ4uxj+RAFeaqErOdE7xymUShawJka7L0noCopjzaeFY8ZQ-w@mail.gmail.com>
On Wed, Aug 16, 2023 at 6:02 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Aug 15, 2023 at 10:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 15 Aug 2023 at 17:59, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > > What occurs to me is why are we bothering with getting write access on
> > > > the internal upper mnt each time. Seems to me it's a historical thing
> > > > without a good reason. Upper mnt is never changed from R/W to R/O.
> > > >
> > > > So the only thing we need to do is grab the upper mount write access
> > > > on superblock creation and do the sb_start_write/end_write() thing
> > > > which can't fail. If upper mnt is read-only, we effectively have a
> > > > read-only filesystem, and can handle it that way (sb->s_flags |=
> > > > SB_RDONLY).
> > > >
> > > > There's still the possibility that we do some changes to upper even
> > > > for non-modify operations. But with careful review we can remove a
> > > > most (possibly all) error handling cases from ovl_want_write()
> > > > callsites when we do know that we have write access on upper. And
> > > > WARN_ON(__mnt_is_readonly(ovl_upper_mnt(ofs))) should ensure that we
> > > > catch any mistakes.
> > > >
> > > > Hmm?
> > > >
> > >
> > > I was thinking the same thing myself, before I went on this journey.
> > > I reached the conclusion that doing only sb_start_write() would not be
> > > safe against emergency remount rdonly of the upper sb.
> > >
> > > I guess if upper sb is emergency mounted rdonly, then overlayfs
> > > sb would also be emergency remounted rdonly, but for example
> > > ext4 sb can become rdonly on internal errors.
> > > But maybe that is not the responsibility of vfs or ovl to care about?
> >
> > Consider the case of a writable open file: the mount write access is
> > only checked on open. So not having fine grained mnt write access
> > checks is not without precedent.
> >
> > I'm not sure, but the number of added lines in this particular patch
> > makes me think that at least during copy-up we could separate the mnt
> > and the sb write locks.
> >
>
> The patch with separate locks during copy-up is not much smaller
> but it is a lot nicer IMO:
>
> https://github.com/amir73il/linux/commits/ovl_want_write-v3
>
> I shall post these shortly after tests are complete.
>
Hi Miklos,
Did you get a change to review v3 patches [1] with the split of
ovl_want_write() to ovl_get_mnt_write() and ovl_start_write()?
I would like to queue this lock ordering change for 6.7.
Thanks,
Amir.
[1] https://lore.kernel.org/linux-unionfs/20230816152334.924960-1-amir73il@gmail.com/
next prev parent reply other threads:[~2023-09-08 12:29 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-14 14:05 [PATCH v2 0/3] overlayfs lock ordering changes Amir Goldstein
2023-08-14 14:05 ` [PATCH v2 1/3] ovl: reorder ovl_want_write() after ovl_inode_lock() Amir Goldstein
2023-08-15 10:50 ` Miklos Szeredi
2023-08-15 15:12 ` Amir Goldstein
2023-08-14 14:05 ` [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held Amir Goldstein
2023-08-15 15:12 ` Miklos Szeredi
2023-08-15 15:59 ` Amir Goldstein
2023-08-15 19:07 ` Miklos Szeredi
2023-08-15 19:51 ` Amir Goldstein
2023-08-15 20:36 ` Miklos Szeredi
2023-08-16 10:28 ` Christian Brauner
2023-08-16 15:02 ` Amir Goldstein
2023-09-08 12:29 ` Amir Goldstein [this message]
2023-08-16 10:34 ` Christian Brauner
2023-08-14 14:05 ` [PATCH v2 3/3] ovl: do not encode lower fh " Amir Goldstein
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=CAOQ4uxis1UR36rJ-sLsgfk4mHhTOn_uM3xhSGbR08G0auzbhxQ@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=brauner@kernel.org \
--cc=jack@suse.cz \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
/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).