On Wed, Nov 29, 2023 at 4:14 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Wed, 29 Nov 2023 at 08:25, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > My proposed solution is to change the semantics with the init flag > > FUSE_PASSTHROUGH to disallow mmap on FOPEN_DIRECT_IO > > files. > > Why? FOPEN_DIRECT_IO and FUSE_PASSTHROUGH should mix much more > readily than FOPEN_DIRECT_IO with page cache. > Am I misunderstanding how mmap works with FOPEN_DIRECT_IO file? My understanding is that mmap of FOPEN_DIRECT_IO reads/writes from page cache of fuse inode. To clarify, the plan is to never allow mixing open of passthrough and cached files on the same inode. It is allowed to open FOPEN_DIRECT_IO file for inode either in cached or passthrough mode, but it is NOT allowed to mmap a FOPEN_DIRECT_IO file for inode in passthrough mode. However, if inode only has file open in FOPEN_DIRECT_IO mode, then inode mode is neutral. If we allow mmap in this state then a later open in passthourgh mode and mmap in passthrough mode will collide with the direct mode mmap. Therefore, my proposal is than when filesystem is FUSE_PASSTHROUGH capable, only passthrough file and cached file may be mmaped, but never allow to mmap a FOPEN_DIRECT_IO file. Does that make sense? > > So with a filesystem that supports FUSE_PASSTHROUGH, > > FOPEN_DIRECT_IO files can only be used for direct read/write io and > > mmap maps either the fuse inode pages or the backing inode pages. > > > > This also means that FUSE_DIRECT_IO_RELAX conflicts with > > FUSE_PASSTHROUGH. > > > > Should we also deny mixing FUSE_HAS_INODE_DAX with > > FUSE_PASSTHROUGH? Anything else from virtiofs? > > Virtiofs/DAX and passthrough are similar features in totally different > environments. We just need to make sure the code paths don't > collide. > > > While I have your attention, I am trying to consolidate the validation > > of FOPEN_ mode vs inode state into fuse_finish_open(). > > > > What do you think about this partial patch that also moves > > fuse_finish_open() to after fuse_release_nowrite(). > > Is it legit? > > I suspect it won't work due to the i_size reset being in > fuse_finish_open(). But I feel there's not enough context to > understand why this is needed. > the re-order of fuse_finish_open() and fuse_release_nowrite() is not needed - it just makes the code a little cleaner. I will try without reorder. Thanks, Amir.
