But why would you need to call fuse_file_io_mmap() for O_DIRECT?
If a file was opened without FOPEN_DIRECT_IO, we already set inode to
caching mode on open.
Does your O_DIRECT patch to mmap solve an actual reproducible bug?
Yeah it does, in my fuse-dio-v5 branch, which adds in shared locks for
O_DIRECT writes without FOPEN_DIRECT_IO.
Ah. right, because open(O_DIRECT) does not enter io cache mode
in your branch. I missed that.
But still, I think that a better fix for fuse_io_mode would be to treat
mmap of O_DIRECT exactly the same as mmap of FOPEN_DIRECT_IO,
including invalidate page cache and require FUSE_DIRECT_IO_ALLOW_MMAP.
I know this could be a change of behavior of applications doing mmap()
on an fd that was opened with O_DIRECT, but I doubt that such applications
exist, even if this really works with upstream code.
Something like this (pushed to my fuse_io_mode branch)?
+static bool fuse_file_is_direct_io(struct file *file)
+{
+ struct fuse_file *ff = file->private_data;
+
+ return ff->open_flags & FOPEN_DIRECT_IO || file->f_flags & O_DIRECT;
+}
+
/* Request access to submit new io to inode via open file */
static bool fuse_file_io_open(struct file *file, struct inode *inode)
{
@@ -116,7 +121,7 @@ static bool fuse_file_io_open(struct file *file,
struct inode *inode)
return true;
/* Set explicit FOPEN_CACHE_IO flag for file open in caching mode */
- if (!(ff->open_flags & FOPEN_DIRECT_IO) && !(file->f_flags & O_DIRECT))
+ if (!fuse_file_is_direct_io(file))
ff->open_flags |= FOPEN_CACHE_IO;
spin_lock(&fi->lock);
@@ -2622,8 +2627,9 @@ static int fuse_file_mmap(struct file *file,
struct vm_area_struct *vma)
if (FUSE_IS_DAX(file_inode(file)))
return fuse_dax_mmap(file, vma);
- if (ff->open_flags & FOPEN_DIRECT_IO) {
- /* Can't provide the coherency needed for MAP_SHARED
+ if (fuse_file_is_direct_io(file)) {
+ /*
+ * Can't provide the coherency needed for MAP_SHARED
* if FUSE_DIRECT_IO_ALLOW_MMAP isn't set.
*/
I cut off the rest of the discussion as this is from my point of view a
rather major change.
I'm not sure about this. If an application opens with O_DIRECT and then
does mmap - sure, weird options, but then none of the other network file
systems requires a special setting for that? And none of the other file
systems has pages invalidations? Why is it needed for fuse' O_DIRECT?
The initial invalidation was for MAP_PRIVATE and FOPEN_DIRECT_IO, but
now gets extended - I get really worried about this special fuse
handling that none of the other file system has.
Also, NFS and smb/cifs do not have the same coherency guarantees, but
still allow mmap on O_DIRECT?
And assuming an application does this on an existing fuse file system,
the application would now need to ask the fuse file system developer to
set this flag with a new kernel?
At a minimum I wouldn't like to have this without its own change log
entry and with the risk that everything needs to be reverted, in case a
regression is reported.
Thanks,
Bernd
