On Tue, Oct 31, 2023 at 1:17 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Tue, 31 Oct 2023 at 11:28, Amir Goldstein <amir73il@xxxxxxxxx> 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.
