> > > > Thanks Amir, I'm going to look at it in detail in the morning. > > Btw, there is another bad direct_io_allow_mmap issue (part of it is > > invalidate_inode_pages2, which you already noticed, but not alone). > > Going to send out the patch once xfstests have passed > > https://github.com/bsbernd/linux/commit/3dae6b05854c4fe84302889a5625c7e5428cdd6c <https://github.com/bsbernd/linux/commit/3dae6b05854c4fe84302889a5625c7e5428cdd6c> > > > > > > Nice! > > But I think that invalidate pages issue is not restricted to shared mmap? > > So history for that is > > commit 3121bfe7631126d1b13064855ac2cfa164381bb0 > Author: Miklos Szeredi <mszeredi@xxxxxxx> > Date: Thu Apr 9 17:37:53 2009 +0200 > > fuse: fix "direct_io" private mmap > > MAP_PRIVATE mmap could return stale data from the cache for > "direct_io" files. Fix this by flushing the cache on mmap. > > Found with a slightly modified fsx-linux. > > Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxx> > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index 0946861b10b7..06f30e965676 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1298,6 +1298,8 @@ static int fuse_direct_mmap(struct file *file, struct vm_area_struct *vma) > if (vma->vm_flags & VM_MAYSHARE) > return -ENODEV; > > + invalidate_inode_pages2(file->f_mapping); > + > return generic_file_mmap(file, vma); > } > > > I don't have a strong opinion here - so idea of this patch is to avoid > exposing stale data from a previous mmap. I guess (and probably hard to achieve > semantics) would be to invalidate pages when the last mapping of that _area_ > is done? > So now with a shared map, data are supposed to be stored in files and > close-to-open consistency with FOPEN_KEEP_CACHE should handle the invalidation? > Nevermind, it was just my bad understanding of invalidate_inode_pages2(). I think it calls fuse_launder_folio() for dirty pages, so data loss is not a concern. > > > > I think that the mix of direct io file with private mmap is common and > > doesn't have issues, but the mix of direct io files and caching files on > > the same inode is probably not very common has the same issues as the > > direct_io_allow_mmap regression that you are fixing. > > Yeah. I also find it interesting that generic_file_mmap is not doing such > things for files opened with O_DIRECT - FOPEN_DIRECT_IO tries to do > strong coherency? > > > I'm going to send out the patch for now as it is, as that might become a longer > discussion - maybe Miklos could comment on it. > I think your patch should not be avoiding invalidate_inode_pages2() in the shared mmap case. You have done that part because of my comment which was wrong, not because it reproduced a bug. Thanks, Amir.
