[Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[Jeffle Xu]

The corresponding kernel patch set:
https://lore.kernel.org/all/20211102052604.59462-1-jefflexu@linux.alibaba.com/

changes since v6:
- rebase to 'virtio-fs-dev' branch
- the added new option is now named as "-o dax=inode|filesize"
- virtiofsd won't advertise support for per inode DAX if no DAX policy
  specified

changes since v5:
- add back support for .ioctl()
- add back negotiation during FUSE_INIT
- add '-o dax=[server|attr]' option to control the policy used by
  virtiofsd to determine whether DAX shall be enabled or not for
  specific file. Please refer to the commit log of patch 4/5/6 for
  more detailed information.


changes since v4:
- decide whether DAX shall be enabled or not solely depending on file
  size (DAX is disabled for files smaller than 32KB)
- negotiation during FUSE_INIT is droped
- drop support for .ioctl() for passthrough

changes since v2/v3:
Patch 4 in v2 is incomplete by mistake and it will fail to be compiled.
I had ever sent a seperate patch 4 of v3. Now I send the whole complete
set in v4. Except for this, there's no other diferrence.

Jeffle Xu (6):
  virtiofsd: add .ioctl() support
  virtiofsd: support per inode DAX in fuse protocol
  virtiofsd: add 'dax=' option
  virtiofsd: negotiate per inode DAX in FUSE_INIT
  virtiofsd: implement xflag based dax policy
  virtiofsd: implement file size based dax policy

 include/standard-headers/linux/fuse.h |   2 +
 tools/virtiofsd/fuse_common.h         |   5 +
 tools/virtiofsd/fuse_lowlevel.c       |   6 ++
 tools/virtiofsd/helper.c              |   5 +
 tools/virtiofsd/passthrough_ll.c      | 141 ++++++++++++++++++++++++++
 tools/virtiofsd/passthrough_seccomp.c |   1 +
 6 files changed, 160 insertions(+)

-- 
2.27.0

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

[Jeffle Xu]

For passthrough, it passes corresponding ioctls to host directly.

Currently only these ioctls that handling persistent inode flags, i.e.,
FS_IOC_[G|S]ETFLAGS and FS_IOC_FS[G|S]ETXATTR are supported for security
concern, though it's implemented in a quite general way, so that we can
expand the scope of allowed ioctls if it is really needed later.

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index b7c1fa71b5..2768497be4 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,68 @@ out:
     fuse_reply_err(req, saverr);
 }
 
+
+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 res, dir;
+    int fd = lo_fi_fd(req, fi);
+    int saverr = 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);
+
+    if (!(cmd == FS_IOC_SETFLAGS || cmd == FS_IOC_GETFLAGS ||
+          cmd == FS_IOC_FSSETXATTR || cmd == FS_IOC_FSGETXATTR)) {
+        goto out;
+    }
+
+    /* unrestricted ioctl is not supported yet */
+    if (flags & FUSE_IOCTL_UNRESTRICTED) {
+        goto out;
+    }
+
+    dir = _IOC_DIR(cmd);
+
+    if (dir & _IOC_READ) {
+        size = out_bufsz;
+        buf = malloc(size);
+        if (!buf) {
+            goto out_err;
+        }
+
+        if (dir & _IOC_WRITE) {
+            memcpy(buf, in_buf, size);
+        }
+
+        res = ioctl(fd, cmd, buf);
+    } else if (dir & _IOC_WRITE) {
+        res = ioctl(fd, cmd, in_buf);
+    } else {
+        res = ioctl(fd, cmd, arg);
+    }
+
+    if (res < 0) {
+        goto out_err;
+    }
+
+    fuse_reply_ioctl(req, 0, buf, size);
+    free(buf);
+
+    return;
+
+out_err:
+    saverr = errno;
+out:
+    fuse_reply_err(req, saverr);
+    free(buf);
+}
+
 static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
                         struct fuse_file_info *fi)
 {
@@ -3474,6 +3538,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 v7 2/6] virtiofsd: support per inode DAX in fuse protocol

[Jeffle Xu]

... in prep for the following support for per inode DAX feature.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 include/standard-headers/linux/fuse.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/standard-headers/linux/fuse.h b/include/standard-headers/linux/fuse.h
index cce105bfba..b6d6cd90e6 100644
--- a/include/standard-headers/linux/fuse.h
+++ b/include/standard-headers/linux/fuse.h
@@ -360,6 +360,7 @@ struct fuse_file_lock {
 #define FUSE_SUBMOUNTS		(1 << 27)
 #define FUSE_HANDLE_KILLPRIV_V2	(1 << 28)
 #define FUSE_SETXATTR_EXT	(1 << 29)
+#define FUSE_HAS_INODE_DAX	(1 << 30)
 
 /**
  * CUSE INIT request/reply flags
@@ -444,6 +445,7 @@ struct fuse_file_lock {
  * FUSE_ATTR_SUBMOUNT: Object is a submount root
  */
 #define FUSE_ATTR_SUBMOUNT      (1 << 0)
+#define FUSE_ATTR_DAX		(1 << 1)
 
 /**
  * Open flags
-- 
2.27.0

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

[Jeffle Xu]

This option is used to specify the policy of constructing per-inode DAX
attribute when guest virtiofs is mounted with "-o dax=inode".

Currently there are two valid policies, "inode" and "filesize".

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 32KB.

The default behavior is "none". That is, virtiofsd will always clear
per-inode DAX attribute and thus DAX is always disabled for all files
when guest virtiofs is mounted with "-o dax=inode".

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 tools/virtiofsd/helper.c         |  5 +++++
 tools/virtiofsd/passthrough_ll.c | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
index a8295d975a..c613bebf2c 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -187,6 +187,11 @@ 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.\n"
+           "                               could be one of \"inode\", \"filesize\"\n"
+           "                               - inode: depending on persistent inode flags\n"
+           "                               - filesize: depending on file size\n"
+           "                               default: none\n"
            );
 }
 
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 2768497be4..13a632bddd 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -138,6 +138,12 @@ enum {
     SANDBOX_CHROOT,
 };
 
+enum {
+    INODE_DAX_NONE,
+    INODE_DAX_INODE,
+    INODE_DAX_FILESIZE,
+};
+
 typedef struct xattr_map_entry {
     char *key;
     char *prepend;
@@ -163,6 +169,8 @@ 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;
     GHashTable *inodes; /* protected by lo->mutex */
@@ -212,6 +220,8 @@ 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=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;
@@ -736,6 +746,8 @@ 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;
     }
+
+    lo->dax = lo->user_dax;
 }
 
 static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
-- 
2.27.0

[Virtio-fs] [PATCH v7 4/6] virtiofsd: negotiate per inode DAX in FUSE_INIT

[Jeffle Xu]

Among the FUSE_INIT phase, guest virtiofs will advertise per inode DAX
if it's mounted with "-o dax=inode". Then virtiofsd is aware that client
is in per inode DAX mode, and will construct per-inode DAX attribute
accordingly.

Virtiofsd shall also advertise support for per inode DAX. DAX policy
must be specified to enable this capability for per inode DAX. That is,
when no DAX policy is specified for virtiofsd, virtiofsd won't advertise
support for per inode DAX then.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 tools/virtiofsd/fuse_common.h    |  5 +++++
 tools/virtiofsd/fuse_lowlevel.c  |  6 ++++++
 tools/virtiofsd/passthrough_ll.c | 14 +++++++++++++-
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
index f0df821c6d..f1b4497448 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 (1 << 30)
+
 /**
  * Ioctl flags
  *
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index f11ed576e4..9efe01dc54 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -2075,6 +2075,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
     if (arg->flags & FUSE_SETXATTR_EXT) {
         se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
     }
+    if (arg->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;
@@ -2190,6 +2193,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.flags |= FUSE_HAS_INODE_DAX;
+    }
     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/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 13a632bddd..999c906da2 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -747,7 +747,19 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
         conn->want &= ~FUSE_CAP_POSIX_ACL;
     }
 
-    lo->dax = lo->user_dax;
+    if (conn->capable & FUSE_CAP_INODE_DAX) {
+        lo->dax = lo->user_dax;
+    } else {
+        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

[Virtio-fs] [PATCH v7 5/6] virtiofsd: implement xflag based dax policy

[Jeffle Xu]

The per inode DAX feature in 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 when filesystem is mounted in per inode DAX mode.

To keep compatible with this feature in ext4/xfs, virtiofs also supports
enabling DAX depending on the persistent inode flag, which may be
set/cleared by users inside guest or admin on host.

This policy is used when '-o dax=inode' option is specified for
virtiofsd.

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 999c906da2..dac5063594 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1026,6 +1026,35 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
     return 0;
 }
 
+static bool 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 false;
+    }
+
+    if (lo->dax == INODE_DAX_INODE) {
+        int res, fd;
+        int ret = false;
+        unsigned int attr;
+
+        fd = lo_inode_open(lo, inode, O_RDONLY);
+        if (fd == -1) {
+            return false;
+        }
+
+        res = ioctl(fd, FS_IOC_GETFLAGS, &attr);
+        if (!res && (attr & FS_DAX_FL)) {
+            ret = true;
+        }
+
+        close(fd);
+        return ret;
+    }
+
+    return false;
+}
+
 /*
  * Increments nlookup on the inode on success. unref_inode_lolocked() must be
  * called eventually to decrement nlookup again. If inodep is non-NULL, the
@@ -1116,6 +1145,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
     }
     e->ino = inode->fuse_ino;
 
+    if (lo_should_enable_dax(lo, inode, e)) {
+        e->attr_flags |= FUSE_ATTR_DAX;
+    }
+
     /* Transfer ownership of inode pointer to caller or drop it */
     if (inodep) {
         *inodep = inode;
-- 
2.27.0

[Virtio-fs] [PATCH v7 6/6] virtiofsd: implement file size based dax policy

[Jeffle Xu]

When DAX window is fully utilized and needs to be expanded to avoid the
performance fluctuation triggered by DAX window recaliming, it may not
be wise to allocate DAX window for files with size smaller than some
specific point, considering from the perspective of reducing memory
overhead.

To maintain one DAX window chunk (e.g., 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 flag the file with FUSE_ATTR_DAX when the file size is greater
than 32 KB. The guest will enable DAX only for those files flagged with
FUSE_ATTR_DAX, when virtiofs is mounted with '-o dax=inode'.

To be noted that both FUSE_LOOKUP and FUSE_READDIRPLUS are affected, and
will convey FUSE_ATTR_DAX flag to the guest.

This policy will be used when '-o dax=filesize' option is specified for
virtiofsd.

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index dac5063594..cbf81d1593 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;
@@ -1052,6 +1067,10 @@ static bool lo_should_enable_dax(struct lo_data *lo, struct lo_inode *inode,
         return ret;
     }
 
+    if (lo->dax == INODE_DAX_FILESIZE) {
+        return e->attr.st_size > FUSE_INODE_DAX_THRESH;
+    }
+
     return false;
 }
 
-- 
2.27.0

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[Vivek Goyal]

Hi Jeffle,

I noticed that you posted V8 of kernel patches. I was away from work
so could not look at it. Now I am back and want to review and test
those patches.

Is there a new version of patches for virtiofsd as well?

As per v6 changelog, I see that you have rebased virtiofsd patches to.

https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev

I am not sure if that's the branch where latest DAX patches are supposed
to be.

David, could you please let us know which branch should we use for
latest DAX patches you have. Or push your latest patches to virtio-fs-dev
so that we are on same page.

Thanks
Vivek

On Tue, Nov 02, 2021 at 01:56:40PM +0800, Jeffle Xu wrote:
> The corresponding kernel patch set:
> https://lore.kernel.org/all/20211102052604.59462-1-jefflexu@linux.alibaba.com/
> 
> changes since v6:
> - rebase to 'virtio-fs-dev' branch
> - the added new option is now named as "-o dax=inode|filesize"
> - virtiofsd won't advertise support for per inode DAX if no DAX policy
>   specified
> 
> changes since v5:
> - add back support for .ioctl()
> - add back negotiation during FUSE_INIT
> - add '-o dax=[server|attr]' option to control the policy used by
>   virtiofsd to determine whether DAX shall be enabled or not for
>   specific file. Please refer to the commit log of patch 4/5/6 for
>   more detailed information.
> 
> 
> changes since v4:
> - decide whether DAX shall be enabled or not solely depending on file
>   size (DAX is disabled for files smaller than 32KB)
> - negotiation during FUSE_INIT is droped
> - drop support for .ioctl() for passthrough
> 
> changes since v2/v3:
> Patch 4 in v2 is incomplete by mistake and it will fail to be compiled.
> I had ever sent a seperate patch 4 of v3. Now I send the whole complete
> set in v4. Except for this, there's no other diferrence.
> 
> Jeffle Xu (6):
>   virtiofsd: add .ioctl() support
>   virtiofsd: support per inode DAX in fuse protocol
>   virtiofsd: add 'dax=' option
>   virtiofsd: negotiate per inode DAX in FUSE_INIT
>   virtiofsd: implement xflag based dax policy
>   virtiofsd: implement file size based dax policy
> 
>  include/standard-headers/linux/fuse.h |   2 +
>  tools/virtiofsd/fuse_common.h         |   5 +
>  tools/virtiofsd/fuse_lowlevel.c       |   6 ++
>  tools/virtiofsd/helper.c              |   5 +
>  tools/virtiofsd/passthrough_ll.c      | 141 ++++++++++++++++++++++++++
>  tools/virtiofsd/passthrough_seccomp.c |   1 +
>  6 files changed, 160 insertions(+)
> 
> -- 
> 2.27.0
> 

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[JeffleXu]

On 12/7/21 10:42 PM, Vivek Goyal wrote:
> Hi Jeffle,
> 
> I noticed that you posted V8 of kernel patches. I was away from work
> so could not look at it. Now I am back and want to review and test
> those patches.

Thanks for reviewing and evaluating.

> 
> Is there a new version of patches for virtiofsd as well?

Nope. Patches for virtiofsd is not updated. v7 is the latest version.

> 
> As per v6 changelog, I see that you have rebased virtiofsd patches to.
> 
> https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev
> 
> I am not sure if that's the branch where latest DAX patches are supposed
> to be.
> 
> David, could you please let us know which branch should we use for
> latest DAX patches you have. Or push your latest patches to virtio-fs-dev
> so that we are on same page.
> 


-- 
Thanks,
Jeffle

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[Vivek Goyal]

On Wed, Dec 08, 2021 at 09:38:45AM +0800, JeffleXu wrote:
> 
> 
> On 12/7/21 10:42 PM, Vivek Goyal wrote:
> > Hi Jeffle,
> > 
> > I noticed that you posted V8 of kernel patches. I was away from work
> > so could not look at it. Now I am back and want to review and test
> > those patches.
> 
> Thanks for reviewing and evaluating.
> 
> > 
> > Is there a new version of patches for virtiofsd as well?
> 
> Nope. Patches for virtiofsd is not updated. v7 is the latest version.

Hi Jeffle,

I am testing these patches now. Using virtio-fs-dev branch and your
patches on top.

When I unmount virtiofs in guest, I see following qemu-kvm message
on console.

qemu-kvm: check_slave_message_entries: Short VhostUserFSSlaveMsg size, 8

Wondering have you noticed it. I have not debugged it. Wondering if
it is due to your patches or there is something in virtio-fs-dev
branch patches which triggers it.

Vivek

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[JeffleXu]

On 12/9/21 4:05 AM, Vivek Goyal wrote:
> On Wed, Dec 08, 2021 at 09:38:45AM +0800, JeffleXu wrote:
>>
>>
>> On 12/7/21 10:42 PM, Vivek Goyal wrote:
>>> Hi Jeffle,
>>>
>>> I noticed that you posted V8 of kernel patches. I was away from work
>>> so could not look at it. Now I am back and want to review and test
>>> those patches.
>>
>> Thanks for reviewing and evaluating.
>>
>>>
>>> Is there a new version of patches for virtiofsd as well?
>>
>> Nope. Patches for virtiofsd is not updated. v7 is the latest version.
> 
> Hi Jeffle,
> 
> I am testing these patches now. Using virtio-fs-dev branch and your
> patches on top.
> 
> When I unmount virtiofs in guest, I see following qemu-kvm message
> on console.
> 
> qemu-kvm: check_slave_message_entries: Short VhostUserFSSlaveMsg size, 8
> 
> Wondering have you noticed it. I have not debugged it. Wondering if
> it is due to your patches or there is something in virtio-fs-dev
> branch patches which triggers it.
> 

This warning can also be triggered in my test environment.

It seems that the warning will be triggered even with pure virtio-fs-dev
branch without my patches and latest kernel without my kernel patches.
Thus I didn't pay attention on it.

KVM warning:
    check_slave_message_entries: Short VhostUserFSSlaveMsg size, 8

virtiofsd warning:
    lo_destroy: unmap during destroy failed


-- 
Thanks,
Jeffle

Re: [Virtio-fs] [PATCH v7 1/6] virtiofsd: add .ioctl() support

[Vivek Goyal]

On Tue, Nov 02, 2021 at 01:56:41PM +0800, Jeffle Xu wrote:
> For passthrough, it passes corresponding ioctls to host directly.
> 
> Currently only these ioctls that handling persistent inode flags, i.e.,
> FS_IOC_[G|S]ETFLAGS and FS_IOC_FS[G|S]ETXATTR are supported for security
> concern, though it's implemented in a quite general way, so that we can
> expand the scope of allowed ioctls if it is really needed later.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  tools/virtiofsd/passthrough_ll.c      | 65 +++++++++++++++++++++++++++
>  tools/virtiofsd/passthrough_seccomp.c |  1 +
>  2 files changed, 66 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index b7c1fa71b5..2768497be4 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,68 @@ out:
>      fuse_reply_err(req, saverr);
>  }
>  
> +
> +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 res, dir;
> +    int fd = lo_fi_fd(req, fi);
> +    int saverr = 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);
> +
> +    if (!(cmd == FS_IOC_SETFLAGS || cmd == FS_IOC_GETFLAGS ||
> +          cmd == FS_IOC_FSSETXATTR || cmd == FS_IOC_FSGETXATTR)) {
> +        goto out;

Good that you allowed only two operations. Still I think people might
have concern about all the inode attrs and whether it is safe to
allow that. For example immutable flag. Now even host can't delete
that file without first clearing that flag. I think it is doable
just that involves extra step.

So we probably might have to block certain attrs if it becomes an
issue or make it configurable.

> +    }
> +
> +    /* unrestricted ioctl is not supported yet */
> +    if (flags & FUSE_IOCTL_UNRESTRICTED) {
> +        goto out;
> +    }
> +
> +    dir = _IOC_DIR(cmd);
> +
> +    if (dir & _IOC_READ) {
> +        size = out_bufsz;
> +        buf = malloc(size);
> +        if (!buf) {
> +            goto out_err;
> +        }
> +
> +        if (dir & _IOC_WRITE) {
> +            memcpy(buf, in_buf, size);
> +        }
> +
> +        res = ioctl(fd, cmd, buf);
> +    } else if (dir & _IOC_WRITE) {
> +        res = ioctl(fd, cmd, in_buf);
> +    } else {
> +        res = ioctl(fd, cmd, arg);
> +    }

I do not understand this if block. So if _IOC_READ and _IOC_WRITE
is not set, then we just send "arg" as third argument. Can you
shed some light on how this third argument works for file attr
flags. I am assuming that "lsattr" will use the _IOC_READ path,
while chattr will use "_IOC_WRITE" path? If that's the case,
as of now third condition is not even required. Until and unless
we are supporting more ioctls.

Thanks
Vivek
> +
> +    if (res < 0) {
> +        goto out_err;
> +    }
> +
> +    fuse_reply_ioctl(req, 0, buf, size);
> +    free(buf);
> +
> +    return;
> +
> +out_err:
> +    saverr = errno;
> +out:
> +    fuse_reply_err(req, saverr);
> +    free(buf);
> +}
> +
>  static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
>                          struct fuse_file_info *fi)
>  {
> @@ -3474,6 +3538,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
> 

Re: [Virtio-fs] [PATCH v7 3/6] virtiofsd: add 'dax=' option

[Vivek Goyal]

On Tue, Nov 02, 2021 at 01:56:43PM +0800, Jeffle Xu wrote:
> This option is used to specify the policy of constructing per-inode DAX
> attribute when guest virtiofs is mounted with "-o dax=inode".
> 
> Currently there are two valid policies, "inode" and "filesize".

General thoughts.

I think additional policies like "dax=always" and "dax=none" will
also make sense. "dax=always" will set ATTR_FLAG_DAX on all inodes.
"dax=none" is default as of now but if somebody wants to enforce
it, they can pass it explicitly, just in case default changes later
down the line.

It should be trivial to add these two policies as well now.

I am little concenred with "dax=filesize". It has implicit threshold
of 32KB. What if somebody wants threshold of 64KB or 1MB or even
higher? One option would be to add another option say,
--dax-filesize-threshold=<foo> and that will allow one to override
default. Or we could hardcode threshold in policy name say
filesize_32KB or filesize_1MB etc.

I guess adding a separate option to modify filesize threshold
is better because it will allow wide range of values. So "dax=filesize"
will just mean that there is an inbuilt threshold of 32KB to enable
dax and and user can override it by passing new option
--dax-filesize-threshold (TBD in future when somebody needs it).


Vivek
> 
> 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 32KB.
> 
> The default behavior is "none". That is, virtiofsd will always clear
> per-inode DAX attribute and thus DAX is always disabled for all files
> when guest virtiofs is mounted with "-o dax=inode".
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  tools/virtiofsd/helper.c         |  5 +++++
>  tools/virtiofsd/passthrough_ll.c | 12 ++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> index a8295d975a..c613bebf2c 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -187,6 +187,11 @@ 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.\n"
> +           "                               could be one of \"inode\", \"filesize\"\n"
> +           "                               - inode: depending on persistent inode flags\n"
> +           "                               - filesize: depending on file size\n"
> +           "                               default: none\n"
>             );
>  }
>  
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 2768497be4..13a632bddd 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -138,6 +138,12 @@ enum {
>      SANDBOX_CHROOT,
>  };
>  
> +enum {
> +    INODE_DAX_NONE,
> +    INODE_DAX_INODE,
> +    INODE_DAX_FILESIZE,
> +};
> +
>  typedef struct xattr_map_entry {
>      char *key;
>      char *prepend;
> @@ -163,6 +169,8 @@ 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;
>      GHashTable *inodes; /* protected by lo->mutex */
> @@ -212,6 +220,8 @@ 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=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;
> @@ -736,6 +746,8 @@ 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;
>      }
> +
> +    lo->dax = lo->user_dax;
>  }
>  
>  static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
> -- 
> 2.27.0
> 

Re: [Virtio-fs] [PATCH v7 5/6] virtiofsd: implement xflag based dax policy

[Vivek Goyal]

On Tue, Nov 02, 2021 at 01:56:45PM +0800, Jeffle Xu wrote:
> The per inode DAX feature in 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 when filesystem is mounted in per inode DAX mode.
> 
> To keep compatible with this feature in ext4/xfs, virtiofs also supports
> enabling DAX depending on the persistent inode flag, which may be
> set/cleared by users inside guest or admin on host.
> 
> This policy is used when '-o dax=inode' option is specified for
> virtiofsd.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 33 ++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 999c906da2..dac5063594 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1026,6 +1026,35 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
>      return 0;
>  }
>  
> +static bool 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 false;
> +    }
> +
> +    if (lo->dax == INODE_DAX_INODE) {
> +        int res, fd;
> +        int ret = false;
> +        unsigned int attr;
> +
> +        fd = lo_inode_open(lo, inode, O_RDONLY);
> +        if (fd == -1) {
> +            return false;
> +        }

So we can't call this ioctl() on an O_PATH fd, and that's why you
are opening the file O_RDONLY?

Concerned about error handling. If we fail to open file, you are
not returning error to client. Instead simply falling back to
not enabling DAX (and hiding an error in the process).

I am not sure about this fallback behavior. I would rather capture
error and return to client. A user has configured system to
enable DAX on inode and if we can't open that inode or can't
query the state of FS_XFLAG_DAX, then its an error and should
be reported back.

> +
> +        res = ioctl(fd, FS_IOC_GETFLAGS, &attr);
> +        if (!res && (attr & FS_DAX_FL)) {
> +            ret = true;
> +        }

Same here. What if ioctl() fails. Shouldn't we return error to
caller? That way if there is something wrong with configuration,
user can fix it. (Instead of silently not enabling DAX).

Vivek
> +
> +        close(fd);
> +        return ret;
> +    }
> +
> +    return false;
> +}
> +
>  /*
>   * Increments nlookup on the inode on success. unref_inode_lolocked() must be
>   * called eventually to decrement nlookup again. If inodep is non-NULL, the
> @@ -1116,6 +1145,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>      }
>      e->ino = inode->fuse_ino;
>  
> +    if (lo_should_enable_dax(lo, inode, e)) {
> +        e->attr_flags |= FUSE_ATTR_DAX;
> +    }
> +
>      /* Transfer ownership of inode pointer to caller or drop it */
>      if (inodep) {
>          *inodep = inode;
> -- 
> 2.27.0
> 

Re: [Virtio-fs] [PATCH v7 6/6] virtiofsd: implement file size based dax policy

[Vivek Goyal]

On Tue, Nov 02, 2021 at 01:56:46PM +0800, Jeffle Xu wrote:
> When DAX window is fully utilized and needs to be expanded to avoid the
> performance fluctuation triggered by DAX window recaliming, it may not
> be wise to allocate DAX window for files with size smaller than some
> specific point, considering from the perspective of reducing memory
> overhead.
> 
> To maintain one DAX window chunk (e.g., 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 flag the file with FUSE_ATTR_DAX when the file size is greater
> than 32 KB. The guest will enable DAX only for those files flagged with
> FUSE_ATTR_DAX, when virtiofs is mounted with '-o dax=inode'.
> 
> To be noted that both FUSE_LOOKUP and FUSE_READDIRPLUS are affected, and
> will convey FUSE_ATTR_DAX flag to the guest.
> 
> This policy will be used when '-o dax=filesize' option is specified for
> virtiofsd.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index dac5063594..cbf81d1593 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

Hmm.., this is hardcoded value. If we change chunk size down the
line then it will not be valid anymore.

I guess when we change chunk size, we can think of negotiating
this with server and that will help.

I guess for now it is ok.

Vivek

> +#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;
> @@ -1052,6 +1067,10 @@ static bool lo_should_enable_dax(struct lo_data *lo, struct lo_inode *inode,
>          return ret;
>      }
>  
> +    if (lo->dax == INODE_DAX_FILESIZE) {
> +        return e->attr.st_size > FUSE_INODE_DAX_THRESH;
> +    }
> +
>      return false;
>  }
>  
> -- 
> 2.27.0
> 

Re: [Virtio-fs] [PATCH v7 5/6] virtiofsd: implement xflag based dax policy

[Vivek Goyal]

On Tue, Nov 02, 2021 at 01:56:45PM +0800, Jeffle Xu wrote:
> The per inode DAX feature in 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 when filesystem is mounted in per inode DAX mode.
> 
> To keep compatible with this feature in ext4/xfs, virtiofs also supports
> enabling DAX depending on the persistent inode flag, which may be
> set/cleared by users inside guest or admin on host.
> 
> This policy is used when '-o dax=inode' option is specified for
> virtiofsd.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 33 ++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 999c906da2..dac5063594 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1026,6 +1026,35 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
>      return 0;
>  }
>  
> +static bool 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 false;
> +    }
> +
> +    if (lo->dax == INODE_DAX_INODE) {
> +        int res, fd;
> +        int ret = false;
> +        unsigned int attr;
> +
> +        fd = lo_inode_open(lo, inode, O_RDONLY);
> +        if (fd == -1) {
> +            return false;
> +        }
> +
> +        res = ioctl(fd, FS_IOC_GETFLAGS, &attr);
> +        if (!res && (attr & FS_DAX_FL)) {
> +            ret = true;
> +        }
> +
> +        close(fd);
> +        return ret;
> +    }
> +
> +    return false;
> +}
> +
>  /*
>   * Increments nlookup on the inode on success. unref_inode_lolocked() must be
>   * called eventually to decrement nlookup again. If inodep is non-NULL, the
> @@ -1116,6 +1145,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>      }
>      e->ino = inode->fuse_ino;
>  
> +    if (lo_should_enable_dax(lo, inode, e)) {
> +        e->attr_flags |= FUSE_ATTR_DAX;
> +    }
> +

If we were too strict then we should probably add a
lo_should_enable_dax() call in lo_getattr() as well?

I guess for now it is ok. Overhead of checking fs_xflag_dax seems
enough and not many might care about immediate chnage of dax
status in guest.

Vivek

>      /* Transfer ownership of inode pointer to caller or drop it */
>      if (inodep) {
>          *inodep = inode;
> -- 
> 2.27.0
> 

Re: [Virtio-fs] [PATCH v7 1/6] virtiofsd: add .ioctl() support

[JeffleXu]

On 12/10/21 3:33 AM, Vivek Goyal wrote:
> On Tue, Nov 02, 2021 at 01:56:41PM +0800, Jeffle Xu wrote:
>> For passthrough, it passes corresponding ioctls to host directly.
>>
>> Currently only these ioctls that handling persistent inode flags, i.e.,
>> FS_IOC_[G|S]ETFLAGS and FS_IOC_FS[G|S]ETXATTR are supported for security
>> concern, though it's implemented in a quite general way, so that we can
>> expand the scope of allowed ioctls if it is really needed later.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>> ---
>>  tools/virtiofsd/passthrough_ll.c      | 65 +++++++++++++++++++++++++++
>>  tools/virtiofsd/passthrough_seccomp.c |  1 +
>>  2 files changed, 66 insertions(+)
>>
>> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
>> index b7c1fa71b5..2768497be4 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,68 @@ out:
>>      fuse_reply_err(req, saverr);
>>  }
>>  
>> +
>> +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 res, dir;
>> +    int fd = lo_fi_fd(req, fi);
>> +    int saverr = 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);
>> +
>> +    if (!(cmd == FS_IOC_SETFLAGS || cmd == FS_IOC_GETFLAGS ||
>> +          cmd == FS_IOC_FSSETXATTR || cmd == FS_IOC_FSGETXATTR)) {
>> +        goto out;
> 
> Good that you allowed only two operations. Still I think people might
> have concern about all the inode attrs and whether it is safe to
> allow that. For example immutable flag. Now even host can't delete
> that file without first clearing that flag. 

Yes, this will be an issue.

> I think it is doable
> just that involves extra step.
> 
> So we probably might have to block certain attrs if it becomes an
> issue or make it configurable.

Currently fuse kernel module only requires that virtiofsd shall support
the set flag path (FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR), so that users
inside guest are able to set/clear FS_XFLAG_DAX flag. For the set flag
path, maybe we could support modifying FS_XFLAG_DAX flag first for the
security concern?

The get flag path(FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR) is not strongly
required currently, since the DAX flag is totally constructed by
virtiofsd now. The use case of the get flag path may be like, users
inside guest may want to query the FS_XFLAG_DAX flag after setting
FS_XFLAG_DAX flag previously. The get flag path may also expose the file
attr of the host file to the guest, and thus maybe we shall also only
support quering FS_XFLAG_DAX flag as the first step.


> 
>> +    }
>> +
>> +    /* unrestricted ioctl is not supported yet */
>> +    if (flags & FUSE_IOCTL_UNRESTRICTED) {
>> +        goto out;
>> +    }
>> +
>> +    dir = _IOC_DIR(cmd);
>> +
>> +    if (dir & _IOC_READ) {
>> +        size = out_bufsz;
>> +        buf = malloc(size);
>> +        if (!buf) {
>> +            goto out_err;
>> +        }
>> +
>> +        if (dir & _IOC_WRITE) {
>> +            memcpy(buf, in_buf, size);
>> +        }
>> +
>> +        res = ioctl(fd, cmd, buf);
>> +    } else if (dir & _IOC_WRITE) {
>> +        res = ioctl(fd, cmd, in_buf);
>> +    } else {
>> +        res = ioctl(fd, cmd, arg);
>> +    }
> 
> I do not understand this if block. So if _IOC_READ and _IOC_WRITE
> is not set, then we just send "arg" as third argument. Can you
> shed some light on how this third argument works for file attr
> flags. 

In theory ioctl could be neither _IOC_READ nor _IOC_WRITE. I implement
this if block just for the completeness of the logic for ioctl support.
It doesn't matter with the file attr though.


> I am assuming that "lsattr" will use the _IOC_READ path,
> while chattr will use "_IOC_WRITE" path? If that's the case,
> as of now third condition is not even required. Until and unless
> we are supporting more ioctls.
> 

Sure, I can remove this if block for now.

-- 
Thanks,
Jeffle

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[JeffleXu]

Hi Vivek,

Besides, I'd like to know that, to make the per inode DAX feature merged
into v5.17, shall the patches for virtiofsd also be ready during the
merge window?


On 12/9/21 4:05 AM, Vivek Goyal wrote:
> On Wed, Dec 08, 2021 at 09:38:45AM +0800, JeffleXu wrote:
>>
>>
>> On 12/7/21 10:42 PM, Vivek Goyal wrote:
>>> Hi Jeffle,
>>>
>>> I noticed that you posted V8 of kernel patches. I was away from work
>>> so could not look at it. Now I am back and want to review and test
>>> those patches.
>>
>> Thanks for reviewing and evaluating.
>>
>>>
>>> Is there a new version of patches for virtiofsd as well?
>>
>> Nope. Patches for virtiofsd is not updated. v7 is the latest version.
> 
> Hi Jeffle,
> 
> I am testing these patches now. Using virtio-fs-dev branch and your
> patches on top.
> 
> When I unmount virtiofs in guest, I see following qemu-kvm message
> on console.
> 
> qemu-kvm: check_slave_message_entries: Short VhostUserFSSlaveMsg size, 8
> 
> Wondering have you noticed it. I have not debugged it. Wondering if
> it is due to your patches or there is something in virtio-fs-dev
> branch patches which triggers it.
> 
> Vivek
> 

-- 
Thanks,
Jeffle

Re: [Virtio-fs] [PATCH v7 3/6] virtiofsd: add 'dax=' option

[JeffleXu]

On 12/10/21 4:00 AM, Vivek Goyal wrote:
> On Tue, Nov 02, 2021 at 01:56:43PM +0800, Jeffle Xu wrote:
>> This option is used to specify the policy of constructing per-inode DAX
>> attribute when guest virtiofs is mounted with "-o dax=inode".
>>
>> Currently there are two valid policies, "inode" and "filesize".
> 
> General thoughts.
> 
> I think additional policies like "dax=always" and "dax=none" will
> also make sense. "dax=always" will set ATTR_FLAG_DAX on all inodes.
> "dax=none" is default as of now but if somebody wants to enforce
> it, they can pass it explicitly, just in case default changes later
> down the line.
> 
> It should be trivial to add these two policies as well now.

Make sense. I will add "dax=always" and "dax=none" in the next version.

> 
> I am little concenred with "dax=filesize". It has implicit threshold
> of 32KB. What if somebody wants threshold of 64KB or 1MB or even
> higher? One option would be to add another option say,
> --dax-filesize-threshold=<foo> and that will allow one to override
> default. Or we could hardcode threshold in policy name say
> filesize_32KB or filesize_1MB etc.
> 
> I guess adding a separate option to modify filesize threshold
> is better because it will allow wide range of values. So "dax=filesize"
> will just mean that there is an inbuilt threshold of 32KB to enable
> dax and and user can override it by passing new option
> --dax-filesize-threshold (TBD in future when somebody needs it).
> 

Yes, at least for now, the "--dax-filesize-threshold" shall be more
flexible.

-- 
Thanks,
Jeffle

Re: [Virtio-fs] [PATCH v7 5/6] virtiofsd: implement xflag based dax policy

[JeffleXu]

On 12/10/21 4:16 AM, Vivek Goyal wrote:
> On Tue, Nov 02, 2021 at 01:56:45PM +0800, Jeffle Xu wrote:
>> The per inode DAX feature in 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 when filesystem is mounted in per inode DAX mode.
>>
>> To keep compatible with this feature in ext4/xfs, virtiofs also supports
>> enabling DAX depending on the persistent inode flag, which may be
>> set/cleared by users inside guest or admin on host.
>>
>> This policy is used when '-o dax=inode' option is specified for
>> virtiofsd.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>> ---
>>  tools/virtiofsd/passthrough_ll.c | 33 ++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
>> index 999c906da2..dac5063594 100644
>> --- a/tools/virtiofsd/passthrough_ll.c
>> +++ b/tools/virtiofsd/passthrough_ll.c
>> @@ -1026,6 +1026,35 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
>>      return 0;
>>  }
>>  
>> +static bool 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 false;
>> +    }
>> +
>> +    if (lo->dax == INODE_DAX_INODE) {
>> +        int res, fd;
>> +        int ret = false;
>> +        unsigned int attr;
>> +
>> +        fd = lo_inode_open(lo, inode, O_RDONLY);
>> +        if (fd == -1) {
>> +            return false;
>> +        }
> 
> So we can't call this ioctl() on an O_PATH fd, and that's why you
> are opening the file O_RDONLY?

Yes. We can't call ioctl() on O_PATH fd, so lo_inode_open() is needed;
we only need to query the file flag, thus O_RDONLY is adequate.

> 
> Concerned about error handling. If we fail to open file, you are
> not returning error to client. Instead simply falling back to
> not enabling DAX (and hiding an error in the process).
> 
> I am not sure about this fallback behavior. I would rather capture
> error and return to client. A user has configured system to
> enable DAX on inode and if we can't open that inode or can't
> query the state of FS_XFLAG_DAX, then its an error and should
> be reported back.

OK, just report an error to make the error handling consistent.

> 
>> +
>> +        res = ioctl(fd, FS_IOC_GETFLAGS, &attr);
>> +        if (!res && (attr & FS_DAX_FL)) {
>> +            ret = true;
>> +        }
> 
> Same here. What if ioctl() fails. Shouldn't we return error to
> caller? That way if there is something wrong with configuration,
> user can fix it. (Instead of silently not enabling DAX).
> 

-- 
Thanks,
Jeffle

Re: [Virtio-fs] [PATCH v7 5/6] virtiofsd: implement xflag based dax policy

[JeffleXu]

On 12/10/21 6:02 AM, Vivek Goyal wrote:
> On Tue, Nov 02, 2021 at 01:56:45PM +0800, Jeffle Xu wrote:
>> The per inode DAX feature in 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 when filesystem is mounted in per inode DAX mode.
>>
>> To keep compatible with this feature in ext4/xfs, virtiofs also supports
>> enabling DAX depending on the persistent inode flag, which may be
>> set/cleared by users inside guest or admin on host.
>>
>> This policy is used when '-o dax=inode' option is specified for
>> virtiofsd.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>> ---
>>  tools/virtiofsd/passthrough_ll.c | 33 ++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
>> index 999c906da2..dac5063594 100644
>> --- a/tools/virtiofsd/passthrough_ll.c
>> +++ b/tools/virtiofsd/passthrough_ll.c
>> @@ -1026,6 +1026,35 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
>>      return 0;
>>  }
>>  
>> +static bool 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 false;
>> +    }
>> +
>> +    if (lo->dax == INODE_DAX_INODE) {
>> +        int res, fd;
>> +        int ret = false;
>> +        unsigned int attr;
>> +
>> +        fd = lo_inode_open(lo, inode, O_RDONLY);
>> +        if (fd == -1) {
>> +            return false;
>> +        }
>> +
>> +        res = ioctl(fd, FS_IOC_GETFLAGS, &attr);
>> +        if (!res && (attr & FS_DAX_FL)) {
>> +            ret = true;
>> +        }
>> +
>> +        close(fd);
>> +        return ret;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>  /*
>>   * Increments nlookup on the inode on success. unref_inode_lolocked() must be
>>   * called eventually to decrement nlookup again. If inodep is non-NULL, the
>> @@ -1116,6 +1145,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>>      }
>>      e->ino = inode->fuse_ino;
>>  
>> +    if (lo_should_enable_dax(lo, inode, e)) {
>> +        e->attr_flags |= FUSE_ATTR_DAX;
>> +    }
>> +
> 
> If we were too strict then we should probably add a
> lo_should_enable_dax() call in lo_getattr() as well?

Good catch. FUSE_GETATTR could also reply the per inode DAX flag.

> 
> I guess for now it is ok. Overhead of checking fs_xflag_dax seems
> enough and not many might care about immediate chnage of dax
> status in guest.
> 

OK, we can easily add it if it's needed later.

-- 
Thanks,
Jeffle

Re: [Virtio-fs] [PATCH v7 6/6] virtiofsd: implement file size based dax policy

[JeffleXu]

On 12/10/21 5:59 AM, Vivek Goyal wrote:
> On Tue, Nov 02, 2021 at 01:56:46PM +0800, Jeffle Xu wrote:
>> When DAX window is fully utilized and needs to be expanded to avoid the
>> performance fluctuation triggered by DAX window recaliming, it may not
>> be wise to allocate DAX window for files with size smaller than some
>> specific point, considering from the perspective of reducing memory
>> overhead.
>>
>> To maintain one DAX window chunk (e.g., 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 flag the file with FUSE_ATTR_DAX when the file size is greater
>> than 32 KB. The guest will enable DAX only for those files flagged with
>> FUSE_ATTR_DAX, when virtiofs is mounted with '-o dax=inode'.
>>
>> To be noted that both FUSE_LOOKUP and FUSE_READDIRPLUS are affected, and
>> will convey FUSE_ATTR_DAX flag to the guest.
>>
>> This policy will be used when '-o dax=filesize' option is specified for
>> virtiofsd.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>> ---
>>  tools/virtiofsd/passthrough_ll.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
>> index dac5063594..cbf81d1593 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
> 
> Hmm.., this is hardcoded value. If we change chunk size down the
> line then it will not be valid anymore.
> 
> I guess when we change chunk size, we can think of negotiating
> this with server and that will help.

Yes.

Another way is that move the FUSE_DAX_SHIFT definition into
include/uapi/ so that virtiofsd could detect the chunk size through the
header file at compile time. But it doesn't work if the host kernel
version and the guest kernel version are not equal. Negotiation during
runtime is certainly more flexible...

> 
> I guess for now it is ok.
> 


-- 
Thanks,
Jeffle

Re: [Virtio-fs] [PATCH v7 1/6] virtiofsd: add .ioctl() support

[Vivek Goyal]

On Fri, Dec 10, 2021 at 10:51:45AM +0800, JeffleXu wrote:
> 
> 
> On 12/10/21 3:33 AM, Vivek Goyal wrote:
> > On Tue, Nov 02, 2021 at 01:56:41PM +0800, Jeffle Xu wrote:
> >> For passthrough, it passes corresponding ioctls to host directly.
> >>
> >> Currently only these ioctls that handling persistent inode flags, i.e.,
> >> FS_IOC_[G|S]ETFLAGS and FS_IOC_FS[G|S]ETXATTR are supported for security
> >> concern, though it's implemented in a quite general way, so that we can
> >> expand the scope of allowed ioctls if it is really needed later.
> >>
> >> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> >> ---
> >>  tools/virtiofsd/passthrough_ll.c      | 65 +++++++++++++++++++++++++++
> >>  tools/virtiofsd/passthrough_seccomp.c |  1 +
> >>  2 files changed, 66 insertions(+)
> >>
> >> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> >> index b7c1fa71b5..2768497be4 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,68 @@ out:
> >>      fuse_reply_err(req, saverr);
> >>  }
> >>  
> >> +
> >> +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 res, dir;
> >> +    int fd = lo_fi_fd(req, fi);
> >> +    int saverr = 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);
> >> +
> >> +    if (!(cmd == FS_IOC_SETFLAGS || cmd == FS_IOC_GETFLAGS ||
> >> +          cmd == FS_IOC_FSSETXATTR || cmd == FS_IOC_FSGETXATTR)) {
> >> +        goto out;
> > 
> > Good that you allowed only two operations. Still I think people might
> > have concern about all the inode attrs and whether it is safe to
> > allow that. For example immutable flag. Now even host can't delete
> > that file without first clearing that flag. 
> 
> Yes, this will be an issue.
> 
> > I think it is doable
> > just that involves extra step.
> > 
> > So we probably might have to block certain attrs if it becomes an
> > issue or make it configurable.
> 
> Currently fuse kernel module only requires that virtiofsd shall support
> the set flag path (FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR), so that users
> inside guest are able to set/clear FS_XFLAG_DAX flag. For the set flag
> path, maybe we could support modifying FS_XFLAG_DAX flag first for the
> security concern?

I think right now limiting ioctl operations to FS_XFLAG_DAX is not
a bad idea. That's what we need for now. Others can be opened when
people need it.

> 
> The get flag path(FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR) is not strongly
> required currently, since the DAX flag is totally constructed by
> virtiofsd now. The use case of the get flag path may be like, users
> inside guest may want to query the FS_XFLAG_DAX flag after setting
> FS_XFLAG_DAX flag previously. The get flag path may also expose the file
> attr of the host file to the guest, and thus maybe we shall also only
> support quering FS_XFLAG_DAX flag as the first step.

Querying FS_XFLAG_DAX is fine. And I agreed that it is needed because
guest might want to query it.

So let us support both setting and querying FS_XFLAG_DAX and block other
operations for now.

> 
> 
> > 
> >> +    }
> >> +
> >> +    /* unrestricted ioctl is not supported yet */
> >> +    if (flags & FUSE_IOCTL_UNRESTRICTED) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    dir = _IOC_DIR(cmd);
> >> +
> >> +    if (dir & _IOC_READ) {
> >> +        size = out_bufsz;
> >> +        buf = malloc(size);
> >> +        if (!buf) {
> >> +            goto out_err;
> >> +        }
> >> +
> >> +        if (dir & _IOC_WRITE) {
> >> +            memcpy(buf, in_buf, size);
> >> +        }
> >> +
> >> +        res = ioctl(fd, cmd, buf);
> >> +    } else if (dir & _IOC_WRITE) {
> >> +        res = ioctl(fd, cmd, in_buf);
> >> +    } else {
> >> +        res = ioctl(fd, cmd, arg);
> >> +    }
> > 
> > I do not understand this if block. So if _IOC_READ and _IOC_WRITE
> > is not set, then we just send "arg" as third argument. Can you
> > shed some light on how this third argument works for file attr
> > flags. 
> 
> In theory ioctl could be neither _IOC_READ nor _IOC_WRITE. I implement
> this if block just for the completeness of the logic for ioctl support.
> It doesn't matter with the file attr though.
> 
> 
> > I am assuming that "lsattr" will use the _IOC_READ path,
> > while chattr will use "_IOC_WRITE" path? If that's the case,
> > as of now third condition is not even required. Until and unless
> > we are supporting more ioctls.
> > 
> 
> Sure, I can remove this if block for now.

Sounds good. Let somebody else add it later when need be. For now, we 
can probably focus on FS_XFLAG_DAX attr only. And put a comment in code
explaining this restriction.

Vivek

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[Vivek Goyal]

On Fri, Dec 10, 2021 at 10:54:20AM +0800, JeffleXu wrote:
> Hi Vivek,
> 
> Besides, I'd like to know that, to make the per inode DAX feature merged
> into v5.17, shall the patches for virtiofsd also be ready during the
> merge window?

I think having virtiofsd patches ready is not a strict requirement to
merge kernel patches. I will provide my ack for the kernel patches.
After that it is up to the Miklos to accept or reject those patches.

Thanks
Vivek

> 
> 
> On 12/9/21 4:05 AM, Vivek Goyal wrote:
> > On Wed, Dec 08, 2021 at 09:38:45AM +0800, JeffleXu wrote:
> >>
> >>
> >> On 12/7/21 10:42 PM, Vivek Goyal wrote:
> >>> Hi Jeffle,
> >>>
> >>> I noticed that you posted V8 of kernel patches. I was away from work
> >>> so could not look at it. Now I am back and want to review and test
> >>> those patches.
> >>
> >> Thanks for reviewing and evaluating.
> >>
> >>>
> >>> Is there a new version of patches for virtiofsd as well?
> >>
> >> Nope. Patches for virtiofsd is not updated. v7 is the latest version.
> > 
> > Hi Jeffle,
> > 
> > I am testing these patches now. Using virtio-fs-dev branch and your
> > patches on top.
> > 
> > When I unmount virtiofs in guest, I see following qemu-kvm message
> > on console.
> > 
> > qemu-kvm: check_slave_message_entries: Short VhostUserFSSlaveMsg size, 8
> > 
> > Wondering have you noticed it. I have not debugged it. Wondering if
> > it is due to your patches or there is something in virtio-fs-dev
> > branch patches which triggers it.
> > 
> > Vivek
> > 
> 
> -- 
> Thanks,
> Jeffle
> 

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[Miklos Szeredi]

On Mon, 13 Dec 2021 at 19:03, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Dec 10, 2021 at 10:54:20AM +0800, JeffleXu wrote:
> > Hi Vivek,
> >
> > Besides, I'd like to know that, to make the per inode DAX feature merged
> > into v5.17, shall the patches for virtiofsd also be ready during the
> > merge window?
>
> I think having virtiofsd patches ready is not a strict requirement to
> merge kernel patches. I will provide my ack for the kernel patches.
> After that it is up to the Miklos to accept or reject those patches.

Pushed to fuse.git#for-next.

The user API needed porting, the userspace part will have to take that
into account.  The only other change I did was replace the !!
construct with a (bool) cast.

Thanks,
Miklos

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[JeffleXu]

On 12/14/21 6:17 PM, Miklos Szeredi wrote:
> On Mon, 13 Dec 2021 at 19:03, Vivek Goyal <vgoyal@redhat.com> wrote:
>>
>> On Fri, Dec 10, 2021 at 10:54:20AM +0800, JeffleXu wrote:
>>> Hi Vivek,
>>>
>>> Besides, I'd like to know that, to make the per inode DAX feature merged
>>> into v5.17, shall the patches for virtiofsd also be ready during the
>>> merge window?
>>
>> I think having virtiofsd patches ready is not a strict requirement to
>> merge kernel patches. I will provide my ack for the kernel patches.
>> After that it is up to the Miklos to accept or reject those patches.
> 
> Pushed to fuse.git#for-next.
> 
> The user API needed porting, the userspace part will have to take that
> into account.  The only other change I did was replace the !!
> construct with a (bool) cast.


Hi, what do you mean by 'The user API needed porting'? Do you mean this
patch set for virtiofsd?

-- 
Thanks,
Jeffle

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[Miklos Szeredi]

On Tue, 14 Dec 2021 at 16:51, JeffleXu <jefflexu@linux.alibaba.com> wrote:
>
>
>
> On 12/14/21 6:17 PM, Miklos Szeredi wrote:
> > On Mon, 13 Dec 2021 at 19:03, Vivek Goyal <vgoyal@redhat.com> wrote:
> >>
> >> On Fri, Dec 10, 2021 at 10:54:20AM +0800, JeffleXu wrote:
> >>> Hi Vivek,
> >>>
> >>> Besides, I'd like to know that, to make the per inode DAX feature merged
> >>> into v5.17, shall the patches for virtiofsd also be ready during the
> >>> merge window?
> >>
> >> I think having virtiofsd patches ready is not a strict requirement to
> >> merge kernel patches. I will provide my ack for the kernel patches.
> >> After that it is up to the Miklos to accept or reject those patches.
> >
> > Pushed to fuse.git#for-next.
> >
> > The user API needed porting, the userspace part will have to take that
> > into account.  The only other change I did was replace the !!
> > construct with a (bool) cast.
>
>
> Hi, what do you mean by 'The user API needed porting'? Do you mean this
> patch set for virtiofsd?

What I meant is that the  FUSE_HAS_INODE_DAX value had to be changed
due to already pushed changes.   So any userspace code relying on the
old value in the original patchset would no longer work.

Thanks,
Miklos

Re: [Virtio-fs] [PATCH v7 0/6] virtiofsd: support per inode DAX

[JeffleXu]

On 12/15/21 12:10 AM, Miklos Szeredi wrote:
> On Tue, 14 Dec 2021 at 16:51, JeffleXu <jefflexu@linux.alibaba.com> wrote:
>>
>>
>>
>> On 12/14/21 6:17 PM, Miklos Szeredi wrote:
>>> On Mon, 13 Dec 2021 at 19:03, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>>
>>>> On Fri, Dec 10, 2021 at 10:54:20AM +0800, JeffleXu wrote:
>>>>> Hi Vivek,
>>>>>
>>>>> Besides, I'd like to know that, to make the per inode DAX feature merged
>>>>> into v5.17, shall the patches for virtiofsd also be ready during the
>>>>> merge window?
>>>>
>>>> I think having virtiofsd patches ready is not a strict requirement to
>>>> merge kernel patches. I will provide my ack for the kernel patches.
>>>> After that it is up to the Miklos to accept or reject those patches.
>>>
>>> Pushed to fuse.git#for-next.
>>>
>>> The user API needed porting, the userspace part will have to take that
>>> into account.  The only other change I did was replace the !!
>>> construct with a (bool) cast.
>>
>>
>> Hi, what do you mean by 'The user API needed porting'? Do you mean this
>> patch set for virtiofsd?
> 
> What I meant is that the  FUSE_HAS_INODE_DAX value had to be changed
> due to already pushed changes.   So any userspace code relying on the
> old value in the original patchset would no longer work.
> 

Got it. Thanks.

-- 
Thanks,
Jeffle