Re: [PATCH v14 00/12] FUSE passthrough for file io

From: Amir Goldstein <amir73il@gmail.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Bernd Schubert <bernd.schubert@fastmail.fm>,
	Daniel Rosenberg <drosen@google.com>,
	 Paul Lawrence <paullawrence@google.com>,
	Alessio Balsini <balsini@android.com>,
	 Christian Brauner <brauner@kernel.org>,
	fuse-devel@lists.sourceforge.net,  linux-fsdevel@vger.kernel.org,
	Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH v14 00/12] FUSE passthrough for file io
Date: Tue, 31 Oct 2023 14:31:36 +0200	[thread overview]
Message-ID: <CAOQ4uxj+myANTk2C+_tk_YNLe748i2xA0HMZ7FKCuw7W5RUCuA@mail.gmail.com> (raw)
In-Reply-To: <CAJfpegufvtaBaK8p+Q3v=9Qoeob3WamWBye=1BwGniRsvO5HZg@mail.gmail.com>

On Tue, Oct 31, 2023 at 1:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 31 Oct 2023 at 11:28, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > These tests do not do msync.
> >
> > Specifically, test generic/080 encodes an expectation that
> > c/mtime will be changed for mmaped write following mmaped read.
> > Not sure where this expectation is coming from?
>
> Probably because "update on first store" is the de-facto standard on
> linux (filemap_page_mkwrite -> file_update_time).
>
> Thinking about it, the POSIX text is a bit sloppy, I think this is
> what it wanted to say:
>
> "The last data modification and last file status change timestamps of
> a file that is mapped with MAP_SHARED and PROT_WRITE shall be marked
> for update at some point in the interval between the _first_ write reference to
> the mapped region _since_the_last_msync()_ call and the next call to msync()
> ..."
>
> Otherwise the linux behavior would be non-conforming.
>
> > I was thinking about a cache coherency model for FUSE passthough:
> > At any given point, if we have access to a backing file, we can compare
> > the backing file's inode timestamps with those of the FUSE inode.
>
> Yes, that makes sense as long as there's a 1:1 mapping between backing
> files and backed files.
>
> It's not the case for e.g. a blockdev backed fs.   But current
> patchset doesn't support that mode yet, so I don't think we need to
> care now.
>
> > If the timestamps differ, we do not copy them over to FUSE inode,
> > as overlayfs does, but we invalidate the FUSE attribute cache.
> > We can perform this checkup on open() release() flush() fsync()
> > and at other points, such as munmap() instead of unconditionally
> > invalidating attribute cache.
>
> This check can be done directly in  fuse_update_get_attr(), no?
>

Current patch set does not implement "backing inode" for FUSE inode,
so this only works for fuse_update_attributes() (with a file).
We do not do cached read/write/readdir in fuse passthrough mode
that leaves only lseek/llseek(), so I assume it's not what you meant.

IOW, we need to conditionally call fuse_invalidate_attr_mask()
at occasions that we have a backing file/inode.

> > I already used tha model for atime update:
> >
> >        /* Mimic atime update policy of backing inode, not the actual value */
> >        if (!timespec64_equal(&backing_inode->i_atime, &inode->i_atime))
> >                fuse_invalidate_atime(inode);
> >
> > Do you think that can work?
> > Do you think that the server should be able to configure this behavior
> > or we can do it by default?
>
> I think this can be the default, and as we generalize the passthrough
> behavior we'll add the necessary controls.
>

OK, I will try to add this and see if that also solved the 2 failing tests.
It should solve the mmaped write test because the tests do
stat()/open()/mmap()/mem-write/close()/stat()

So it's the close/flush that is going to update mtime, even though that's
not the POSIX semantics, it is a bit like the NFS close-to-open (cto)
semantics regarding data cache coherency.

Thanks,
Amir.