On 20.09.23 04:40, Tyler Fanelli wrote:
Although DIRECT_IO_RELAX's initial usage is to allow shared mmap, its
description indicates a purpose of reducing memory footprint. This
may imply that it could be further used to relax other DIRECT_IO
operations in the future.
Replace it with a flag DIRECT_IO_ALLOW_MMAP which does only one thing,
allow shared mmap of DIRECT_IO files while still bypassing the cache
on regular reads and writes.
Thanks!
I prefer the definition to be narrow so that FUSE servers (virtiofsd,
specifically) can rely on what exact behavior this flag enables. As it
is, I think it’s hard to use the flag, because:
It is not clear what the flag does.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e78662e818f9
gives a goal for using it (in case you want to reduce memory footprint),
but doesn’t say what it will do. This makes it difficult for us (in
virtiofsd) to expose it, because we in turn can’t tell users in
documentation what it’ll do. For example, the commit correctly advises
“to make sure it doesn't break coherency in your use case”, but that
isn’t really possible when it isn’t well-defined what coherency
properties are changed.
Further, is implied that what the flag does may change in the future,
but how so is left unclear. The goal given is to reduce memory
footprint, but that’s actually done by using DIRECT_IO, not by using
DIRECT_IO_RELAX, so what restrictions that latter may relax is left
open. Allowing mmap specifically kind of increases memory footprint, so
it seems to me as if the combination of both flags is supposed to
optimize for memory usage under the hard restriction of allowing every
operation to work still, and mmap() is the one operation identified so
far. But if so, it should be possible to exhaustively identify all
other operations besides mmap() that are affected by DIRECT_IO, so that
they can all be enabled by the new flag, and exhaustively listed in its
documentation. (I assume mmap() is the only operation that’s affected,
though.) Without knowing what the flag will do in the future, any name
under which we (in virtiofsd) choose to expose this flag might be
outright wrong in the future.
Signed-off-by: Tyler Fanelli <tfanelli@xxxxxxxxxx>
---
fs/fuse/file.c | 6 +++---
fs/fuse/fuse_i.h | 4 ++--
fs/fuse/inode.c | 6 +++---
include/uapi/linux/fuse.h | 7 +++----
4 files changed, 11 insertions(+), 12 deletions(-)
[...]
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index db92a7202b34..f4e3c083aede 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -209,7 +209,7 @@
* - add FUSE_HAS_EXPIRE_ONLY
*
* 7.39
- * - add FUSE_DIRECT_IO_RELAX
+ * - add FUSE_DIRECT_IO_ALLOW_MMAP
* - add FUSE_STATX and related structures
*/
@@ -409,8 +409,7 @@ struct fuse_file_lock {
* FUSE_CREATE_SUPP_GROUP: add supplementary group info to create, mkdir,
* symlink and mknod (single group that matches parent)
* FUSE_HAS_EXPIRE_ONLY: kernel supports expiry-only entry invalidation
- * FUSE_DIRECT_IO_RELAX: relax restrictions in FOPEN_DIRECT_IO mode, for now
- * allow shared mmap
+ * FUSE_DIRECT_IO_ALLOW_MMAP: allow shared mmap in FOPEN_DIRECT_IO mode.
*/
#define FUSE_ASYNC_READ (1 << 0)
#define FUSE_POSIX_LOCKS (1 << 1)
@@ -449,7 +448,7 @@ struct fuse_file_lock {
#define FUSE_HAS_INODE_DAX (1ULL << 33)
#define FUSE_CREATE_SUPP_GROUP (1ULL << 34)
#define FUSE_HAS_EXPIRE_ONLY (1ULL << 35)
-#define FUSE_DIRECT_IO_RELAX (1ULL << 36)
+#define FUSE_DIRECT_IO_ALLOW_MMAP (1ULL << 36)
Is it allowed to remove FUSE_DIRECT_IO_RELAX now that it was already
present in a uapi header?
Personally, I don’t mind keeping the name for the flag and just change
the documentation. Or we might keep the old name as an alias for the
new one.
/**
* CUSE INIT request/reply flags
