[Virtio-fs] [PATCH v8 0/3] virtiofsd: support per inode DAX

[Virtio-fs] [PATCH v8 0/3] virtiofsd: support per inode DAX

[Jeffle Xu]

This patchset aims for adding support for per-inode DAX for (C version)
virtiofsd. Sorry it is has been a long time since the last version, and
several developers have asked me how to test this feature. :( Sorry for
the inconvenience.

Besids I know the C version virtiofsd has been marked as deprecated,
while the mainline branch in qemu also has minimal support for DAX. Thus
I'm not sure what the community thinks of this patchset.

It is worth mentioning that, this patchset is developed based on
gitlab.com/virtio-fs/qemu.git virtio-fs-dev, whilst the following
commits also need to be cherry-picked.

939513863f virtiofsd: Extend size of fuse_conn_info->capable and ->want fields
99401173dd virtiofsd: Parse extended "struct fuse_init_in"
f1af2502ab virtiofsd: Fix breakage due to fuse_init_in size change
214617239b linux-headers: Update headers to v5.17-rc1
0f9c46c89b linux-headers: update to 5.16-rc1

It can ben tested on
https://github.com/lostjeffle/virtiofsd/commits/jingbo/dev-perfile-dax-v8


changes since v7:
- merge patches implementing various DAX policies into one patch (patch
  2); merge patches introducing "dax=" option and negotiation among
  FUSE_INIT phase into one patch (patch 3)
- patch 1: restrict the newly introduced .ioctl() to only querying
  and setting FS_DAX_FL/FS_XFLAG_DAX flag (Vivek Goyal [1])
- patch 2: besides "inode" and "filesize" DAX policy, also add "none"
  and "always" policy (Vivek Goyal [2])
- patch3: return error if any operation fails when handling the fuse
  request, rather than fallback to "dax=none" behavior (Vivek Goyal
  [3])
- As disccussed in [2], a default threshold of 32KB in "filesize" policy
  is fine for now. We can add new option "--dax-filesize-threshold" if
  it's needed later.

[1] https://www.mail-archive.com/virtio-fs@redhat.com/msg04460.html
[2] https://www.mail-archive.com/virtio-fs@redhat.com/msg04461.html
[3] https://www.mail-archive.com/virtio-fs@redhat.com/msg04462.html

Corresponding kernel patchset v8 (merged)
https://lore.kernel.org/all/YbeMvKVmzUwmrZhC@redhat.com/T/

virtiofsd patchset v7
https://www.mail-archive.com/virtio-fs@redhat.com/msg04349.html


Jeffle Xu (3):
  virtiofsd: add .ioctl() support
  virtiofsd: implement DAX policies for per inodex DAX mode
  virtiofsd: add 'dax=' option

 tools/virtiofsd/fuse_common.h         |   5 +
 tools/virtiofsd/fuse_lowlevel.c       |   6 +
 tools/virtiofsd/helper.c              |   6 +
 tools/virtiofsd/passthrough_ll.c      | 211 +++++++++++++++++++++++++-
 tools/virtiofsd/passthrough_seccomp.c |   1 +
 5 files changed, 226 insertions(+), 3 deletions(-)

-- 
2.27.0

[Virtio-fs] [PATCH v8 1/3] virtiofsd: add .ioctl() support

[Jeffle Xu]

Support FS_IOC_[G|S]ETFLAGS and FS_IOC_FS[G|S]ETXATTR ioctls in prep
for the following per-inode DAX feature.

Currently only quering and setting FS_DAX_FL/FS_XFLAG_DAX flag are
supported for security concern. The restriction can be relaxed if it
is needed later.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 tools/virtiofsd/passthrough_ll.c      | 121 ++++++++++++++++++++++++++
 tools/virtiofsd/passthrough_seccomp.c |   1 +
 2 files changed, 122 insertions(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index b7c1fa71b5..f52398f1f4 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -47,6 +47,7 @@
 #include <dirent.h>
 #include <pthread.h>
 #include <sys/file.h>
+#include <sys/ioctl.h>
 #include <sys/mount.h>
 #include <sys/prctl.h>
 #include <sys/resource.h>
@@ -54,6 +55,7 @@
 #include <sys/wait.h>
 #include <sys/xattr.h>
 #include <syslog.h>
+#include <linux/fs.h>
 
 #include "qemu/cutils.h"
 #include "passthrough_helpers.h"
@@ -2188,6 +2190,124 @@ out:
     fuse_reply_err(req, saverr);
 }
 
+static int lo_ioctl_write(int fd, unsigned int cmd, const void *in_buf)
+{
+    int res = 0;
+
+    if (cmd == FS_IOC_SETFLAGS) {
+        const unsigned int *in_flags = in_buf;
+        unsigned int flags;
+
+        res = ioctl(fd, FS_IOC_GETFLAGS, &flags);
+        if (res < 0) {
+            return errno;
+        }
+
+        if (*in_flags & FS_DAX_FL) {
+            flags |= FS_DAX_FL;
+        } else {
+            flags &= ~FS_DAX_FL;
+        }
+
+        res = ioctl(fd, cmd, &flags);
+    } else if (cmd == FS_IOC_FSSETXATTR) {
+        const struct fsxattr *in_xfa = in_buf;
+        struct fsxattr xfa;
+
+        res = ioctl(fd, FS_IOC_FSGETXATTR, &xfa);
+        if (res < 0) {
+            return errno;
+        }
+
+        if (in_xfa->fsx_xflags & FS_XFLAG_DAX) {
+            xfa.fsx_xflags |= FS_XFLAG_DAX;
+        } else {
+            xfa.fsx_xflags &= ~FS_XFLAG_DAX;
+        }
+
+        res = ioctl(fd, cmd, &xfa);
+    }
+
+    return res < 0 ? errno : 0;
+}
+
+static int lo_ioctl_read(int fd, unsigned int cmd, size_t out_bufsz,
+                         void **out_buf)
+{
+    int res;
+    void *buf = NULL;
+
+    buf = malloc(out_bufsz);
+    if (!buf) {
+        return errno;
+    }
+
+    res = ioctl(fd, cmd, buf);
+    if (res < 0) {
+        free(buf);
+        return errno;
+    }
+
+    if (cmd == FS_IOC_GETFLAGS) {
+        unsigned int *flags = buf;
+
+        *flags = *flags & FS_DAX_FL;
+    } else if (cmd == FS_IOC_FSGETXATTR) {
+        struct fsxattr *xfa = buf;
+
+        *xfa = (struct fsxattr) {
+            .fsx_xflags = xfa->fsx_xflags & FS_XFLAG_DAX,
+        };
+    }
+
+    *out_buf = buf;
+    return 0;
+}
+
+static void lo_ioctl(fuse_req_t req, fuse_ino_t ino, unsigned int cmd,
+                     void *arg, struct fuse_file_info *fi,
+                     unsigned flags, const void *in_buf,
+                     size_t in_bufsz, size_t out_bufsz)
+{
+    int fd = lo_fi_fd(req, fi);
+    int res = ENOSYS;
+    void *buf = NULL;
+    size_t size = 0;
+
+    fuse_log(FUSE_LOG_DEBUG, "lo_ioctl(ino=%" PRIu64 ", cmd=0x%x, flags=0x%x, "
+            "in_bufsz = %lu, out_bufsz = %lu)\n",
+            ino, cmd, flags, in_bufsz, out_bufsz);
+
+    /*
+     * Unrestricted ioctl is not supported yet.
+     * Currently only quering and setting FS_DAX_FL/FS_XFLAG_DAX flag are
+     * supported to enable per-inode DAX feature. The restriction can be
+     * relaxed if it's needed later.
+     */
+    if (flags & FUSE_IOCTL_UNRESTRICTED ||
+        !(cmd == FS_IOC_SETFLAGS || cmd == FS_IOC_GETFLAGS ||
+          cmd == FS_IOC_FSSETXATTR || cmd == FS_IOC_FSGETXATTR)) {
+        fuse_log(FUSE_LOG_DEBUG, "unsupported cmd %u, flags %x\n", cmd, flags);
+        goto err;
+    }
+
+    if (_IOC_DIR(cmd) == _IOC_READ) {
+        size = out_bufsz;
+        res = lo_ioctl_read(fd, cmd, out_bufsz, &buf);
+    } else if (_IOC_DIR(cmd) == _IOC_WRITE) {
+        res = lo_ioctl_write(fd, cmd, in_buf);
+    }
+    if (res) {
+        goto err;
+    }
+
+    fuse_reply_ioctl(req, 0, buf, size);
+    free(buf);
+    return;
+err:
+    fuse_reply_err(req, res);
+}
+
 static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
                         struct fuse_file_info *fi)
 {
@@ -3474,6 +3594,7 @@ static struct fuse_lowlevel_ops lo_oper = {
     .fsyncdir = lo_fsyncdir,
     .create = lo_create,
     .getlk = lo_getlk,
+    .ioctl = lo_ioctl,
     .setlk = lo_setlk,
     .open = lo_open,
     .release = lo_release,
diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
index f49ed94b5e..eaed5b151b 100644
--- a/tools/virtiofsd/passthrough_seccomp.c
+++ b/tools/virtiofsd/passthrough_seccomp.c
@@ -62,6 +62,7 @@ static const int syscall_allowlist[] = {
     SCMP_SYS(gettid),
     SCMP_SYS(gettimeofday),
     SCMP_SYS(getxattr),
+    SCMP_SYS(ioctl),
     SCMP_SYS(linkat),
     SCMP_SYS(listxattr),
     SCMP_SYS(lseek),
-- 
2.27.0

[Virtio-fs] [PATCH v8 2/3] virtiofsd: implement DAX policies for per inodex DAX mode

[Jeffle Xu]

Implement various dax policies when guest virtiofs is mounted in per
inode DAX mode. Currently there are four valid such policies, "none",
"always", "inode" and "filesize".

When "none" policy is specified, DAX is always disabled for all files;
Similarly, DAX is always enabled for all files when "always" policy is

When "inode" policy is specified, virtiofsd enables DAX or not depending
on the persistent FS_XFLAG_DAX inode flag.

When works in per inode DAX mode, ext4/xfs uses the persistent inode
flag, i.e. FS_DAX_FL/FS_XFLAG_DAX, to indicate if DAX shall be enabled
for this file or not. To keep the consistency of per inode DAX
semantics, virtiofs also supports enabling DAX depending on the
persistent inode flag, which may be set/cleared by users inside guest or
admin on host.

As for "filesize" policy, it is inspired by the scenario where DAX
window is run out and needs to be expanded to avoid the performance
fluctuation triggered by DAX window recaliming. Considering from the
perspective of memory overhead, it may not be wise to allocate DAX
window for files with size smaller than a specific threshold. The
threshold can be calculated from the hint that guest virtiofs manages
DAX mapping at 2MB granularity.

To maintain one DAX window chunk (2MB in size), 32KB (512 * 64 bytes)
memory footprint will be consumed for page descriptors inside guest.
Thus it'd better disable DAX for those files smaller than 32KB, to
reduce the demand for DAX window and thus avoid the unworthy memory
overhead. Thus only enable DAX for those with the file size greater
than 32 KB.

Besides, it is worth mentioning that both FUSE_LOOKUP and
FUSE_READDIRPLUS are affected, and will convey FUSE_ATTR_DAX flag to
the guest.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 tools/virtiofsd/passthrough_ll.c | 70 ++++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index f52398f1f4..00ec21bfc9 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -54,6 +54,7 @@
 #include <sys/syscall.h>
 #include <sys/wait.h>
 #include <sys/xattr.h>
+#include <sys/user.h>
 #include <syslog.h>
 #include <linux/fs.h>
 
@@ -61,6 +62,20 @@
 #include "passthrough_helpers.h"
 #include "passthrough_seccomp.h"
 
+/*
+ * One page descriptor (64 bytes in size) needs to be maintained for every page
+ * in the DAX window chunk, i.e., there is certain guest memory overhead when
+ * DAX is enabled. Thus disable DAX for those with file size smaller than this
+ * certain memory overhead if virtiofs is mounted in per inode DAX mode. In
+ * this case, the guest page cache will consume less memory than that when DAX
+ * is enabled.
+ */
+#define FUSE_DAX_SHIFT      21
+#define PAGE_DESC_SHIFT     6
+#define FUSE_INODE_DAX_SHIFT \
+    (FUSE_DAX_SHIFT - PAGE_SHIFT + PAGE_DESC_SHIFT)
+#define FUSE_INODE_DAX_THRESH  (1 << FUSE_INODE_DAX_SHIFT)
+
 /* Keep track of inode posix locks for each owner. */
 struct lo_inode_plock {
     uint64_t lock_owner;
@@ -138,6 +153,13 @@ enum {
     SANDBOX_CHROOT,
 };
 
+enum {
+    INODE_DAX_NONE,
+    INODE_DAX_ALWAYS,
+    INODE_DAX_INODE,
+    INODE_DAX_FILESIZE,
+};
+
 typedef struct xattr_map_entry {
     char *key;
     char *prepend;
@@ -163,6 +185,7 @@ struct lo_data {
     int readdirplus_clear;
     int allow_direct_io;
     int announce_submounts;
+    int dax;
     bool use_statx;
     struct lo_inode root;
     GHashTable *inodes; /* protected by lo->mutex */
@@ -1002,6 +1025,42 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
     return 0;
 }
 
+static int lo_should_enable_dax(struct lo_data *lo, struct lo_inode *inode,
+                                struct fuse_entry_param *e)
+{
+    if (lo->dax == INODE_DAX_NONE || !S_ISREG(e->attr.st_mode)) {
+        return 0;
+    }
+
+    if (lo->dax == INODE_DAX_ALWAYS) {
+        return 1;
+    }
+
+    if (lo->dax == INODE_DAX_INODE) {
+        int res, fd;
+        unsigned int attr;
+
+        fd = lo_inode_open(lo, inode, O_RDONLY);
+        if (fd == -1) {
+            return -1;
+        }
+
+        res = ioctl(fd, FS_IOC_GETFLAGS, &attr);
+        if (res == 0 && (attr & FS_DAX_FL)) {
+            res = 1;
+        }
+
+        close(fd);
+        return res;
+    }
+
+    if (lo->dax == INODE_DAX_FILESIZE) {
+        return e->attr.st_size > FUSE_INODE_DAX_THRESH;
+    }
+
+    return -1;
+}
+
 /*
  * Increments nlookup on the inode on success. unref_inode_lolocked() must be
  * called eventually to decrement nlookup again. If inodep is non-NULL, the
@@ -1049,6 +1108,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
     res = do_statx(lo, newfd, "", &e->attr, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW,
                    &mnt_id);
     if (res == -1) {
+        close(newfd);
         goto out_err;
     }
 
@@ -1092,6 +1152,13 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
     }
     e->ino = inode->fuse_ino;
 
+    res = lo_should_enable_dax(lo, inode, e);
+    if (res == -1) {
+        goto out_err;
+    } else if (res == 1) {
+        e->attr_flags |= FUSE_ATTR_DAX;
+    }
+
     /* Transfer ownership of inode pointer to caller or drop it */
     if (inodep) {
         *inodep = inode;
@@ -1108,9 +1175,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
 
 out_err:
     saverr = errno;
-    if (newfd != -1) {
-        close(newfd);
-    }
     lo_inode_put(lo, &inode);
     lo_inode_put(lo, &dir);
     return saverr;
-- 
2.27.0

[Virtio-fs] [PATCH v8 3/3] virtiofsd: add 'dax=' option

[Jeffle Xu]

Add "dax=[none|always|inode|filesize]" option to specify the policy of
constructing per-inode DAX attribute when guest virtiofs is mounted in
per inode DAX mode.

The default behavior is "none". That is, virtiofsd will always clear
per-inode DAX attribute and thus DAX is always disabled for all files.
It will default to "none" mode when "dax=" option is not specified.

Similarly, when it works in "always" mode, DAX is always enabled for all
files when guest virtiofs is mounted in per inode DAX mode.

Then it comes two policies regarding to per inode attributes.

When "dax=inode" is specified, virtiofsd will construct per-inode DAX
attribute denpending on the persistent inode flags, i.e.
FS_XFLAG_DAX/FS_DAX_FL of host files. With this option, admin could
select those files that should be DAX enabled and mark them with
persistent inode flags, or users could mark files as DAX enabled inside
guest.

When "dax=filesize" is specified, virtiofsd will construct per-inode DAX
attribute depending on the file size. In this case DAX will be disabled
for those with file size smaller than a specific threshold. Currently
the threshold is hardcoded as 32KB, which is calculated from a hint that
guest virtiofs manages DAX mapping at 2MB granularity. We can introduce
a new option, e.g. "--dax-filesize-threshold" to make the threshold
configurable if it's needed later.

Besides, negotiate per inode DAX feature in FUSE_INIT phase. It is worth
mentioning that, if no dax policy (other than "none" mode) is specified,
virtiofsd won't advertise support for per indoe DAX feature.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 tools/virtiofsd/fuse_common.h    |  5 +++++
 tools/virtiofsd/fuse_lowlevel.c  |  6 ++++++
 tools/virtiofsd/helper.c         |  6 ++++++
 tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++++++++
 4 files changed, 37 insertions(+)

diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
index df6202d174..760e30cdfa 100644
--- a/tools/virtiofsd/fuse_common.h
+++ b/tools/virtiofsd/fuse_common.h
@@ -377,6 +377,11 @@ struct fuse_file_info {
  */
 #define FUSE_CAP_SETXATTR_EXT (1 << 29)
 
+/**
+ * Indicates support for per inode DAX.
+ */
+#define FUSE_CAP_INODE_DAX (1ULL << 33)
+
 /**
  * Ioctl flags
  *
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 178c5dc968..3d448ba351 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -2093,6 +2093,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
     if (flags & FUSE_SETXATTR_EXT) {
         se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
     }
+    if (flags & FUSE_HAS_INODE_DAX) {
+        se->conn.capable |= FUSE_CAP_INODE_DAX;
+    }
 #ifdef HAVE_SPLICE
 #ifdef HAVE_VMSPLICE
     se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE;
@@ -2208,6 +2211,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
     if (se->conn.want & FUSE_CAP_POSIX_ACL) {
         outarg.flags |= FUSE_POSIX_ACL;
     }
+    if (se->conn.want & FUSE_CAP_INODE_DAX) {
+        outarg.flags2 |= FUSE_HAS_INODE_DAX >> 32;
+    }
     outarg.max_readahead = se->conn.max_readahead;
     outarg.max_write = se->conn.max_write;
     if (se->conn.max_background >= (1 << 16)) {
diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
index a8295d975a..81f0adea71 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -187,6 +187,12 @@ void fuse_cmdline_help(void)
            "                               default: no_allow_direct_io\n"
            "    -o announce_submounts      Announce sub-mount points to the guest\n"
            "    -o posix_acl/no_posix_acl  Enable/Disable posix_acl. (default: disabled)\n"
+           "    -o dax=<policy>            policies of constructing per-inode DAX attribute when\n"
+           "                               guest advertises per-inode DAX mode.\n"
+           "                               - none: default, disable DAX for all files\n"
+           "                               - always: enable DAX for all files\n"
+           "                               - inode: depending on persistent inode flags\n"
+           "                               - filesize: depending on file size\n"
            );
 }
 
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 00ec21bfc9..e6ebde138e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -185,6 +185,7 @@ struct lo_data {
     int readdirplus_clear;
     int allow_direct_io;
     int announce_submounts;
+    int user_dax;
     int dax;
     bool use_statx;
     struct lo_inode root;
@@ -235,6 +236,10 @@ static const struct fuse_opt lo_opts[] = {
     { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 },
     { "posix_acl", offsetof(struct lo_data, user_posix_acl), 1 },
     { "no_posix_acl", offsetof(struct lo_data, user_posix_acl), 0 },
+    { "dax=none", offsetof(struct lo_data, user_dax), INODE_DAX_NONE },
+    { "dax=always", offsetof(struct lo_data, user_dax), INODE_DAX_ALWAYS },
+    { "dax=inode", offsetof(struct lo_data, user_dax), INODE_DAX_INODE },
+    { "dax=filesize", offsetof(struct lo_data, user_dax), INODE_DAX_FILESIZE },
     FUSE_OPT_END
 };
 static bool use_syslog = false;
@@ -759,6 +764,21 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
         fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
         conn->want &= ~FUSE_CAP_POSIX_ACL;
     }
+
+    if (conn->capable & FUSE_CAP_INODE_DAX) {
+        lo->dax = lo->user_dax;
+    } else {
+        /* Set INODE_DAX_NONE explicitly to override the old value */
+        lo->dax = INODE_DAX_NONE;
+    }
+
+    /*
+     * If no dax policy is specified, then virtiofsd won't advertise support
+     * for per indoe DAX.
+     */
+    if (lo->dax != INODE_DAX_NONE) {
+        conn->want |= FUSE_CAP_INODE_DAX;
+    }
 }
 
 static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
-- 
2.27.0