On Wed, Aug 16, 2023 at 6:02 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Tue, Aug 15, 2023 at 10:07 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > > > On Tue, 15 Aug 2023 at 17:59, Amir Goldstein <amir73il@xxxxxxxxx> 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@xxxxxxxxx/
