MIME-Version: 1.0
References: <20211118112315.GD13047@quack2.suse.cz> <17d32ecf46e.124314f8f672.8832559275193368959@mykernel.net>
 <20211118164349.GB8267@quack2.suse.cz> <17d36d37022.1227b6f102736.1047689367927335302@mykernel.net>
 <20211130112206.GE7174@quack2.suse.cz> <17d719b79f9.d89bf95117881.5882353172682156775@mykernel.net>
 <CAOQ4uxidK-yDMZoZtoRwTZLgSTr1o2Mu2L55vJRNJDLV0-Sb1w@mail.gmail.com>
 <17d73da701b.e571c37220081.6904057835107693340@mykernel.net>
 <17d74b08dcd.c0e94e6320632.9167792887632811518@mykernel.net>
 <CAOQ4uxiCYFeeH8oUUNG+rDCru_1XcwB6fR2keS1C6=d_yD9XzA@mail.gmail.com>
 <20211201134610.GA1815@quack2.suse.cz> <17d76cf59ee.12f4517f122167.2687299278423224602@mykernel.net>
In-Reply-To: <17d76cf59ee.12f4517f122167.2687299278423224602@mykernel.net>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Thu, 2 Dec 2021 00:47:25 +0200
Message-ID: <CAOQ4uxiEjGms-sKhrVDtDHSEk97Wku5oPxnmy4vVB=6yRE_Hdg@mail.gmail.com>
Subject: Re: [RFC PATCH v5 06/10] ovl: implement overlayfs' ->write_inode operation
To:     Chengguang Xu <cgxu519@mykernel.net>
Cc:     Jan Kara <jack@suse.cz>, Miklos Szeredi <miklos@szeredi.hu>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        overlayfs <linux-unionfs@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        ronyjin <ronyjin@tencent.com>,
        charliecgxu <charliecgxu@tencent.com>,
        Vivek Goyal <vgoyal@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Dec 1, 2021 at 6:24 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- =E5=9C=A8 =E6=98=9F=E6=9C=9F=E4=B8=89, 2021-12-01 21:46:10 Jan Kara=
 <jack@suse.cz> =E6=92=B0=E5=86=99 ----
>  > On Wed 01-12-21 09:19:17, Amir Goldstein wrote:
>  > > On Wed, Dec 1, 2021 at 8:31 AM Chengguang Xu <cgxu519@mykernel.net> =
wrote:
>  > > > So the final solution to handle all the concerns looks like accura=
tely
>  > > > mark overlay inode diry on modification and re-mark dirty only for
>  > > > mmaped file in ->write_inode().
>  > > >
>  > > > Hi Miklos, Jan
>  > > >
>  > > > Will you agree with new proposal above?
>  > > >
>  > >
>  > > Maybe you can still pull off a simpler version by remarking dirty on=
ly
>  > > writably mmapped upper AND inode_is_open_for_write(upper)?
>  >
>  > Well, if inode is writeably mapped, it must be also open for write, do=
esn't
>  > it? The VMA of the mapping will hold file open. So remarking overlay i=
node
>  > dirty during writeback while inode_is_open_for_write(upper) looks like
>  > reasonably easy and presumably there won't be that many inodes open fo=
r
>  > writing for this to become big overhead?

I think it should be ok and a good tradeoff of complexity vs. performance.

>  >
>  > > If I am not mistaken, if you always mark overlay inode dirty on ovl_=
flush()
>  > > of FMODE_WRITE file, there is nothing that can make upper inode dirt=
y
>  > > after last close (if upper is not mmaped), so one more inode sync sh=
ould
>  > > be enough. No?
>  >
>  > But we still need to catch other dirtying events like timestamp update=
s,
>  > truncate(2) etc. to mark overlay inode dirty. Not sure how reliably th=
at
>  > can be done...
>  >

Oh yeh, we have those as well :)
All those cases should be covered by ovl_copyattr() that updates the
ovl inode ctime/mtime, so always dirty in ovl_copyattr() should be good.
I *think* the only case of ovl_copyattr() that should not dirty is in
ovl_inode_init(), so need some special helper there.

>
> To be honest I even don't fully understand what's the ->flush() logic in =
overlayfs.
> Why should we open new underlying file when calling ->flush()?
> Is it still correct in the case of opening lower layer first then copy-up=
ed case?
>

The semantics of flush() are far from being uniform across filesystems.
most local filesystems do nothing on close.
most network fs only flush dirty data when a writer closes a file
but not when a reader closes a file.
It is hard to imagine that applications rely on flush-on-close of
rdonly fd behavior and I agree that flushing only if original fd was upper
makes more sense, so I am not sure if it is really essential for
overlayfs to open an upper rdonly fd just to do whatever the upper fs
would have done on close of rdonly fd, but maybe there is no good
reason to change this behavior either.

Thanks,
Amir.