[Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Christian Schoenebeck via Qemu-devel]

This is v7 of a proposed patch set for fixing file ID collisions with 9pfs.

v6->v7:

  * Rebased to https://github.com/gkurz/qemu/commits/9p-next
    (SHA1 7fc4c49e91).

  * Be pedantic and abort with error on wrong value for new command line
    argument 'multidevs'.

  * Adjusted patches to qemu code style guidelines.

  * Fixed potential crash in qp_table_destroy() on error path.

  * Use dedicated hash table init functions (qpd_table_init(),
    qpf_table_init(), qpp_table_init()):
    https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00144.html

  * Use warn_report_once() instead of error_report_once() for issues
    interpreted merely as being warnings, not errors.

  * Simplified hash table destruction (due to simplified error path
    introduced by SHA1 7fc4c49e91).

  * Dropped capturing root_ino for now:
    https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00146.html

  * Don't update proxy docs, since new command line argument 'multidevs' is
    limited to the local backend for now.

  * Mention in docs that readdir() is currently not blocked by
    'multidevs=forbid'.

  * Rename qid_path_prefixmap() -> qid_path_suffixmap() in patch 3
    (due to the semantic change of that function by that patch).

Christian Schoenebeck (3):
  9p: Added virtfs option 'multidevs=remap|forbid|warn'
  9p: stat_to_qid: implement slow path
  9p: Use variable length suffixes for inode remapping

 fsdev/file-op-9p.h      |   5 +
 fsdev/qemu-fsdev-opts.c |   7 +-
 fsdev/qemu-fsdev.c      |  17 ++
 hw/9pfs/9p.c            | 456 ++++++++++++++++++++++++++++++++++++++--
 hw/9pfs/9p.h            |  59 ++++++
 qemu-options.hx         |  26 ++-
 vl.c                    |   7 +-
 7 files changed, 552 insertions(+), 25 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v7 1/3] 9p: Added virtfs option 'multidevs=remap|forbid|warn'

[Christian Schoenebeck via Qemu-devel]

From: Christian Schoenebeck <qemu_oss@crudebyte.com>

'warn' (default): Only log an error message (once) on host if more than one
device is shared by same export, except of that just ignore this config
error though. This is the default behaviour for not breaking existing
installations implying that they really know what they are doing.

'forbid': Like 'warn', but except of just logging an error this
also denies access of guest to additional devices.

'remap': Allows to share more than one device per export by remapping
inodes from host to guest appropriately. To support multiple devices on the
9p share, and avoid qid path collisions we take the device id as input to
generate a unique QID path. The lowest 48 bits of the path will be set
equal to the file inode, and the top bits will be uniquely assigned based
on the top 16 bits of the inode and the device id.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
[CS: - Rebased to https://github.com/gkurz/qemu/commits/9p-next
       (SHA1 7fc4c49e91).
     - Added virtfs option 'multidevs', original patch simply did the inode
       remapping without being asked.
     - Updated hash calls to new xxhash API.
     - Updated docs for new option 'multidevs'.
     - Fixed v9fs_do_readdir() not having remapped inodes.
     - Log error message when running out of prefixes in
       qid_path_prefixmap().
     - Fixed definition of QPATH_INO_MASK.
     - Wrapped qpp_table initialization to dedicated qpp_table_init()
       function.
     - Dropped unnecessary parantheses in qpp_lookup_func().
     - Dropped unnecessary g_malloc0() result checks. ]
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 fsdev/file-op-9p.h      |   5 ++
 fsdev/qemu-fsdev-opts.c |   7 +-
 fsdev/qemu-fsdev.c      |  17 ++++
 hw/9pfs/9p.c            | 188 +++++++++++++++++++++++++++++++++++-----
 hw/9pfs/9p.h            |  12 +++
 qemu-options.hx         |  26 +++++-
 vl.c                    |   7 +-
 7 files changed, 237 insertions(+), 25 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index c757c8099f..f2f7772c86 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -59,6 +59,11 @@ typedef struct ExtendedOps {
 #define V9FS_RDONLY                 0x00000040
 #define V9FS_PROXY_SOCK_FD          0x00000080
 #define V9FS_PROXY_SOCK_NAME        0x00000100
+/*
+ * multidevs option (either one of the two applies exclusively)
+ */
+#define V9FS_REMAP_INODES           0x00000200
+#define V9FS_FORBID_MULTIDEVS       0x00000400
 
 #define V9FS_SEC_MASK               0x0000003C
 
diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
index 7c31ffffaf..07a18c6e48 100644
--- a/fsdev/qemu-fsdev-opts.c
+++ b/fsdev/qemu-fsdev-opts.c
@@ -31,7 +31,9 @@ static QemuOptsList qemu_fsdev_opts = {
         }, {
             .name = "readonly",
             .type = QEMU_OPT_BOOL,
-
+        }, {
+            .name = "multidevs",
+            .type = QEMU_OPT_STRING,
         }, {
             .name = "socket",
             .type = QEMU_OPT_STRING,
@@ -75,6 +77,9 @@ static QemuOptsList qemu_virtfs_opts = {
         }, {
             .name = "readonly",
             .type = QEMU_OPT_BOOL,
+        }, {
+            .name = "multidevs",
+            .type = QEMU_OPT_STRING,
         }, {
             .name = "socket",
             .type = QEMU_OPT_STRING,
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index 077a8c4e2b..e407716177 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -58,6 +58,7 @@ static FsDriverTable FsDrivers[] = {
             "writeout",
             "fmode",
             "dmode",
+            "multidevs",
             "throttling.bps-total",
             "throttling.bps-read",
             "throttling.bps-write",
@@ -121,6 +122,7 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
     const char *fsdev_id = qemu_opts_id(opts);
     const char *fsdriver = qemu_opt_get(opts, "fsdriver");
     const char *writeout = qemu_opt_get(opts, "writeout");
+    const char *multidevs = qemu_opt_get(opts, "multidevs");
     bool ro = qemu_opt_get_bool(opts, "readonly", 0);
 
     if (!fsdev_id) {
@@ -161,6 +163,21 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
     } else {
         fsle->fse.export_flags &= ~V9FS_RDONLY;
     }
+    if (multidevs) {
+        if (!strcmp(multidevs, "remap")) {
+            fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
+            fsle->fse.export_flags |= V9FS_REMAP_INODES;
+        } else if (!strcmp(multidevs, "forbid")) {
+            fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
+            fsle->fse.export_flags |= V9FS_FORBID_MULTIDEVS;
+        } else if (!strcmp(multidevs, "warn")) {
+            fsle->fse.export_flags &= ~V9FS_FORBID_MULTIDEVS;
+            fsle->fse.export_flags &= ~V9FS_REMAP_INODES;
+        } else {
+            error_setg(errp, "fsdev: '%s' is invalid for multidevs", multidevs);
+            return -1;
+        }
+    }
 
     if (fsle->fse.ops->parse_opts) {
         if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) {
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 5a895ae0bb..8eb89c5c7d 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -26,6 +26,7 @@
 #include "trace.h"
 #include "migration/blocker.h"
 #include "sysemu/qtest.h"
+#include "qemu/xxhash.h"
 
 int open_fd_hw;
 int total_open_fd;
@@ -572,23 +573,117 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
                                 P9_STAT_MODE_NAMED_PIPE |   \
                                 P9_STAT_MODE_SOCKET)
 
-/* This is the algorithm from ufs in spfs */
+/* creative abuse of tb_hash_func7, which is based on xxhash */
+static uint32_t qpp_hash(QppEntry e)
+{
+    return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
+}
+
+static bool qpp_lookup_func(const void *obj, const void *userp)
+{
+    const QppEntry *e1 = obj, *e2 = userp;
+    return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
+}
+
+static void qpp_table_remove(void *p, uint32_t h, void *up)
+{
+    g_free(p);
+}
+
+static void qpp_table_destroy(struct qht *ht)
+{
+    if (!ht || !ht->map) {
+        return;
+    }
+    qht_iter(ht, qpp_table_remove, NULL);
+    qht_destroy(ht);
+}
+
+static void qpp_table_init(struct qht *ht)
+{
+    qht_init(ht, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
+}
+
+/*
+ * stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
+ * to a unique QID path (64 bits). To avoid having to map and keep track
+ * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
+ * the device id to the 16 highest bits of the QID path. The 48 lowest bits
+ * of the QID path equal to the lowest bits of the inode number.
+ *
+ * This takes advantage of the fact that inode number are usually not
+ * random but allocated sequentially, so we have fewer items to keep
+ * track of.
+ */
+static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
+                              uint64_t *path)
+{
+    QppEntry lookup = {
+        .dev = stbuf->st_dev,
+        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
+    }, *val;
+    uint32_t hash = qpp_hash(lookup);
+
+    val = qht_lookup(&pdu->s->qpp_table, &lookup, hash);
+
+    if (!val) {
+        if (pdu->s->qp_prefix_next == 0) {
+            /* we ran out of prefixes */
+            error_report_once(
+                "9p: No more prefixes available for remapping inodes from "
+                "host to guest."
+            );
+            return -ENFILE;
+        }
+
+        val = g_malloc0(sizeof(QppEntry));
+        *val = lookup;
+
+        /* new unique inode prefix and device combo */
+        val->qp_prefix = pdu->s->qp_prefix_next++;
+        qht_insert(&pdu->s->qpp_table, val, hash, NULL);
+    }
+
+    *path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
+    return 0;
+}
+
 static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
 {
+    int err;
     size_t size;
 
-    if (pdu->s->dev_id != stbuf->st_dev) {
-        warn_report_once(
-            "9p: Multiple devices detected in same VirtFS export, "
-            "which might lead to file ID collisions and severe "
-            "misbehaviours on guest! You should use a separate "
-            "export for each device shared from host."
-        );
+    if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
+        /* map inode+device to qid path (fast path) */
+        err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+        if (err) {
+            return err;
+        }
+    } else {
+        if (pdu->s->dev_id != stbuf->st_dev) {
+            if (pdu->s->ctx.export_flags & V9FS_FORBID_MULTIDEVS) {
+                error_report_once(
+                    "9p: Multiple devices detected in same VirtFS export. "
+                    "Access of guest to additional devices is (partly) "
+                    "denied due to virtfs option 'multidevs=forbid' being "
+                    "effective."
+                );
+                return -ENODEV;
+            } else {
+                warn_report_once(
+                    "9p: Multiple devices detected in same VirtFS export, "
+                    "which might lead to file ID collisions and severe "
+                    "misbehaviours on guest! You should either use a "
+                    "separate export for each device shared from host or "
+                    "use virtfs option 'multidevs=remap'!"
+                );
+            }
+        }
+        memset(&qidp->path, 0, sizeof(qidp->path));
+        size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
+        memcpy(&qidp->path, &stbuf->st_ino, size);
     }
 
-    memset(&qidp->path, 0, sizeof(qidp->path));
-    size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
-    memcpy(&qidp->path, &stbuf->st_ino, size);
     qidp->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
     qidp->type = 0;
     if (S_ISDIR(stbuf->st_mode)) {
@@ -618,6 +713,30 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
     return 0;
 }
 
+static int coroutine_fn dirent_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
+                                      struct dirent *dent, V9fsQID *qidp)
+{
+    struct stat stbuf;
+    V9fsPath path;
+    int err;
+
+    v9fs_path_init(&path);
+
+    err = v9fs_co_name_to_path(pdu, &fidp->path, dent->d_name, &path);
+    if (err < 0) {
+        goto out;
+    }
+    err = v9fs_co_lstat(pdu, &path, &stbuf);
+    if (err < 0) {
+        goto out;
+    }
+    err = stat_to_qid(pdu, &stbuf, qidp);
+
+out:
+    v9fs_path_free(&path);
+    return err;
+}
+
 V9fsPDU *pdu_alloc(V9fsState *s)
 {
     V9fsPDU *pdu = NULL;
@@ -1966,16 +2085,39 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
             v9fs_string_free(&name);
             return count;
         }
-        /*
-         * Fill up just the path field of qid because the client uses
-         * only that. To fill the entire qid structure we will have
-         * to stat each dirent found, which is expensive
-         */
-        size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
-        memcpy(&qid.path, &dent->d_ino, size);
-        /* Fill the other fields with dummy values */
-        qid.type = 0;
-        qid.version = 0;
+
+        if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
+            /*
+             * dirent_to_qid() implies expensive stat call for each entry,
+             * we must do that here though since inode remapping requires
+             * the device id, which in turn might be different for
+             * different entries; we cannot make any assumption to avoid
+             * that here.
+             */
+            err = dirent_to_qid(pdu, fidp, dent, &qid);
+            if (err < 0) {
+                v9fs_readdir_unlock(&fidp->fs.dir);
+                v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
+                v9fs_string_free(&name);
+                return err;
+            }
+        } else {
+            /*
+             * Fill up just the path field of qid because the client uses
+             * only that. To fill the entire qid structure we will have
+             * to stat each dirent found, which is expensive. For the
+             * latter reason we don't call dirent_to_qid() here. Only drawback
+             * is that no multi-device export detection of stat_to_qid()
+             * would be done and provided as error to the user here. But
+             * user would get that error anyway when accessing those
+             * files/dirs through other ways.
+             */
+            size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
+            memcpy(&qid.path, &dent->d_ino, size);
+            /* Fill the other fields with dummy values */
+            qid.type = 0;
+            qid.version = 0;
+        }
 
         /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
         len = pdu_marshal(pdu, 11 + count, "Qqbs",
@@ -3676,6 +3818,9 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
 
     s->dev_id = stat.st_dev;
 
+    qpp_table_init(&s->qpp_table);
+    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
+
     s->ctx.fst = &fse->fst;
     fsdev_throttle_init(s->ctx.fst);
 
@@ -3697,6 +3842,7 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
         fsdev_throttle_cleanup(s->ctx.fst);
     }
     g_free(s->tag);
+    qpp_table_destroy(&s->qpp_table);
     g_free(s->ctx.fs_root);
 }
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 5e316178d5..7262fe80aa 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -8,6 +8,7 @@
 #include "fsdev/9p-iov-marshal.h"
 #include "qemu/thread.h"
 #include "qemu/coroutine.h"
+#include "qemu/qht.h"
 
 enum {
     P9_TLERROR = 6,
@@ -235,6 +236,15 @@ struct V9fsFidState
     V9fsFidState *rclm_lst;
 };
 
+#define QPATH_INO_MASK        ((1ULL << 48) - 1)
+
+/* QID path prefix entry, see stat_to_qid */
+typedef struct {
+    dev_t dev;
+    uint16_t ino_prefix;
+    uint16_t qp_prefix;
+} QppEntry;
+
 struct V9fsState
 {
     QLIST_HEAD(, V9fsPDU) free_list;
@@ -257,6 +267,8 @@ struct V9fsState
     V9fsConf fsconf;
     V9fsQID root_qid;
     dev_t dev_id;
+    struct qht qpp_table;
+    uint16_t qp_prefix_next;
 };
 
 /* 9p2000.L open flags */
diff --git a/qemu-options.hx b/qemu-options.hx
index ea0638e92d..97d22fc6d7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1335,7 +1335,7 @@ ETEXI
 
 DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
     "-virtfs local,path=path,mount_tag=tag,security_model=mapped-xattr|mapped-file|passthrough|none\n"
-    "        [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
+    "        [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode][,multidevs=remap|forbid|warn]\n"
     "-virtfs proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly]\n"
     "-virtfs proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonly]\n"
     "-virtfs synth,mount_tag=tag[,id=id][,readonly]\n",
@@ -1343,7 +1343,7 @@ DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
 
 STEXI
 
-@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag} ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly] [,fmode=@var{fmode}][,dmode=@var{dmode}]
+@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag} ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly] [,fmode=@var{fmode}][,dmode=@var{dmode}][,multidevs=@var{multidevs}]
 @itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
 @itemx -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
 @itemx -virtfs synth,mount_tag=@var{mount_tag}
@@ -1399,6 +1399,28 @@ Specifies the default mode for newly created directories on the host. Works
 only with security models "mapped-xattr" and "mapped-file".
 @item mount_tag=@var{mount_tag}
 Specifies the tag name to be used by the guest to mount this export point.
+@item multidevs=@var{multidevs}
+Specifies how to deal with multiple devices being shared with a 9p export.
+Supported behaviours are either "remap", "forbid" or "warn". The latter is
+the default behaviour on which virtfs 9p expects only one device to be
+shared with the same export, and if more than one device is shared and
+accessed via the same 9p export then only a warning message is logged
+(once) by qemu on host side. In order to avoid file ID collisions on guest
+you should either create a separate virtfs export for each device to be
+shared with guests (recommended way) or you might use "remap" instead which
+allows you to share multiple devices with only one export instead, which is
+achieved by remapping the original inode numbers from host to guest in a
+way that would prevent such collisions. Remapping inodes in such use cases
+is required because the original device IDs from host are never passed and
+exposed on guest. Instead all files of an export shared with virtfs always
+share the same device id on guest. So two files with identical inode
+numbers but from actually different devices on host would otherwise cause a
+file ID collision and hence potential misbehaviours on guest. "forbid" on
+the other hand assumes like "warn" that only one device is shared by the
+same export, however it will not only log a warning message but also
+deny access to additional devices on guest. Note though that "forbid" does
+currently not block all possible file access operations (e.g. readdir()
+would still return entries from other devices).
 @end table
 ETEXI
 
diff --git a/vl.c b/vl.c
index 630f5c5e9c..1986332ee7 100644
--- a/vl.c
+++ b/vl.c
@@ -3335,7 +3335,8 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_virtfs: {
                 QemuOpts *fsdev;
                 QemuOpts *device;
-                const char *writeout, *sock_fd, *socket, *path, *security_model;
+                const char *writeout, *sock_fd, *socket, *path, *security_model,
+                           *multidevs;
 
                 olist = qemu_find_opts("virtfs");
                 if (!olist) {
@@ -3395,6 +3396,10 @@ int main(int argc, char **argv, char **envp)
                 qemu_opt_set_bool(fsdev, "readonly",
                                   qemu_opt_get_bool(opts, "readonly", 0),
                                   &error_abort);
+                multidevs = qemu_opt_get(opts, "multidevs");
+                if (multidevs) {
+                    qemu_opt_set(fsdev, "multidevs", multidevs, &error_abort);
+                }
                 device = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
                                           &error_abort);
                 qemu_opt_set(device, "driver", "virtio-9p-pci", &error_abort);
-- 
2.20.1

[Qemu-devel] [PATCH v7 2/3] 9p: stat_to_qid: implement slow path

[Christian Schoenebeck via Qemu-devel]

From: Christian Schoenebeck <qemu_oss@crudebyte.com>

stat_to_qid attempts via qid_path_prefixmap to map unique files (which are
identified by 64 bit inode nr and 32 bit device id) to a 64 QID path value.
However this implementation makes some assumptions about inode number
generation on the host.

If qid_path_prefixmap fails, we still have 48 bits available in the QID
path to fall back to a less memory efficient full mapping.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
[CS: - Rebased to https://github.com/gkurz/qemu/commits/9p-next
       (SHA1 7fc4c49e91).
     - Updated hash calls to new xxhash API.
     - Removed unnecessary parantheses in qpf_lookup_func().
     - Removed unnecessary g_malloc0() result checks.
     - Log error message when running out of prefixes in
       qid_path_fullmap().
     - Log warning message about potential degraded performance in
       qid_path_prefixmap().
     - Wrapped qpf_table initialization to dedicated qpf_table_init()
       function.
     - Fixed typo in comment. ]
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/9pfs/9p.h |  9 +++++++
 2 files changed, 76 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 8eb89c5c7d..d9be2d45d3 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -579,23 +579,34 @@ static uint32_t qpp_hash(QppEntry e)
     return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
 }
 
+static uint32_t qpf_hash(QpfEntry e)
+{
+    return qemu_xxhash7(e.ino, e.dev, 0, 0, 0);
+}
+
 static bool qpp_lookup_func(const void *obj, const void *userp)
 {
     const QppEntry *e1 = obj, *e2 = userp;
     return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
 }
 
-static void qpp_table_remove(void *p, uint32_t h, void *up)
+static bool qpf_lookup_func(const void *obj, const void *userp)
+{
+    const QpfEntry *e1 = obj, *e2 = userp;
+    return e1->dev == e2->dev && e1->ino == e2->ino;
+}
+
+static void qp_table_remove(void *p, uint32_t h, void *up)
 {
     g_free(p);
 }
 
-static void qpp_table_destroy(struct qht *ht)
+static void qp_table_destroy(struct qht *ht)
 {
     if (!ht || !ht->map) {
         return;
     }
-    qht_iter(ht, qpp_table_remove, NULL);
+    qht_iter(ht, qp_table_remove, NULL);
     qht_destroy(ht);
 }
 
@@ -604,6 +615,50 @@ static void qpp_table_init(struct qht *ht)
     qht_init(ht, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
 }
 
+static void qpf_table_init(struct qht *ht)
+{
+    qht_init(ht, qpf_lookup_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
+}
+
+static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
+                            uint64_t *path)
+{
+    QpfEntry lookup = {
+        .dev = stbuf->st_dev,
+        .ino = stbuf->st_ino
+    }, *val;
+    uint32_t hash = qpf_hash(lookup);
+
+    /* most users won't need the fullmap, so init the table lazily */
+    if (!pdu->s->qpf_table.map) {
+        qpf_table_init(&pdu->s->qpf_table);
+    }
+
+    val = qht_lookup(&pdu->s->qpf_table, &lookup, hash);
+
+    if (!val) {
+        if (pdu->s->qp_fullpath_next == 0) {
+            /* no more files can be mapped :'( */
+            error_report_once(
+                "9p: No more prefixes available for remapping inodes from "
+                "host to guest."
+            );
+            return -ENFILE;
+        }
+
+        val = g_malloc0(sizeof(QppEntry));
+        *val = lookup;
+
+        /* new unique inode and device combo */
+        val->path = pdu->s->qp_fullpath_next++;
+        pdu->s->qp_fullpath_next &= QPATH_INO_MASK;
+        qht_insert(&pdu->s->qpf_table, val, hash, NULL);
+    }
+
+    *path = val->path;
+    return 0;
+}
+
 /*
  * stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
  * to a unique QID path (64 bits). To avoid having to map and keep track
@@ -629,9 +684,8 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
     if (!val) {
         if (pdu->s->qp_prefix_next == 0) {
             /* we ran out of prefixes */
-            error_report_once(
-                "9p: No more prefixes available for remapping inodes from "
-                "host to guest."
+            warn_report_once(
+                "9p: Potential degraded performance of inode remapping"
             );
             return -ENFILE;
         }
@@ -656,6 +710,10 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
     if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
         /* map inode+device to qid path (fast path) */
         err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+        if (err == -ENFILE) {
+            /* fast path didn't work, fall back to full map */
+            err = qid_path_fullmap(pdu, stbuf, &qidp->path);
+        }
         if (err) {
             return err;
         }
@@ -3820,6 +3878,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
 
     qpp_table_init(&s->qpp_table);
     s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
+    s->qp_fullpath_next = 1;
 
     s->ctx.fst = &fse->fst;
     fsdev_throttle_init(s->ctx.fst);
@@ -3842,7 +3901,8 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
         fsdev_throttle_cleanup(s->ctx.fst);
     }
     g_free(s->tag);
-    qpp_table_destroy(&s->qpp_table);
+    qp_table_destroy(&s->qpp_table);
+    qp_table_destroy(&s->qpf_table);
     g_free(s->ctx.fs_root);
 }
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 7262fe80aa..35a362c0d7 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -245,6 +245,13 @@ typedef struct {
     uint16_t qp_prefix;
 } QppEntry;
 
+/* QID path full entry, as above */
+typedef struct {
+    dev_t dev;
+    ino_t ino;
+    uint64_t path;
+} QpfEntry;
+
 struct V9fsState
 {
     QLIST_HEAD(, V9fsPDU) free_list;
@@ -268,7 +275,9 @@ struct V9fsState
     V9fsQID root_qid;
     dev_t dev_id;
     struct qht qpp_table;
+    struct qht qpf_table;
     uint16_t qp_prefix_next;
+    uint64_t qp_fullpath_next;
 };
 
 /* 9p2000.L open flags */
-- 
2.20.1

[Qemu-devel] [PATCH v7 3/3] 9p: Use variable length suffixes for inode remapping

[Christian Schoenebeck via Qemu-devel]

From: Christian Schoenebeck <qemu_oss@crudebyte.com>

Use variable length suffixes for inode remapping instead of the fixed
16 bit size prefixes before. With this change the inode numbers on guest
will typically be much smaller (e.g. around >2^1 .. >2^7 instead of >2^48
with the previous fixed size inode remapping.

Additionally this solution is more efficient, since inode numbers in
practice can take almost their entire 64 bit range on guest as well, so
there is less likely a need for generating and tracking additional suffixes,
which might also be beneficial for nested virtualization where each level of
virtualization would shift up the inode bits and increase the chance of
expensive remapping actions.

The "Exponential Golomb" algorithm is used as basis for generating the
variable length suffixes. The algorithm has a parameter k which controls the
distribution of bits on increasing indeces (minimum bits at low index vs.
maximum bits at high index). With k=0 the generated suffixes look like:

Index Dec/Bin -> Generated Suffix Bin
1 [1] -> [1] (1 bits)
2 [10] -> [010] (3 bits)
3 [11] -> [110] (3 bits)
4 [100] -> [00100] (5 bits)
5 [101] -> [10100] (5 bits)
6 [110] -> [01100] (5 bits)
7 [111] -> [11100] (5 bits)
8 [1000] -> [0001000] (7 bits)
9 [1001] -> [1001000] (7 bits)
10 [1010] -> [0101000] (7 bits)
11 [1011] -> [1101000] (7 bits)
12 [1100] -> [0011000] (7 bits)
...
65533 [1111111111111101] ->  [1011111111111111000000000000000] (31 bits)
65534 [1111111111111110] ->  [0111111111111111000000000000000] (31 bits)
65535 [1111111111111111] ->  [1111111111111111000000000000000] (31 bits)
Hence minBits=1 maxBits=31

And with k=5 they would look like:

Index Dec/Bin -> Generated Suffix Bin
1 [1] -> [000001] (6 bits)
2 [10] -> [100001] (6 bits)
3 [11] -> [010001] (6 bits)
4 [100] -> [110001] (6 bits)
5 [101] -> [001001] (6 bits)
6 [110] -> [101001] (6 bits)
7 [111] -> [011001] (6 bits)
8 [1000] -> [111001] (6 bits)
9 [1001] -> [000101] (6 bits)
10 [1010] -> [100101] (6 bits)
11 [1011] -> [010101] (6 bits)
12 [1100] -> [110101] (6 bits)
...
65533 [1111111111111101] -> [0011100000000000100000000000] (28 bits)
65534 [1111111111111110] -> [1011100000000000100000000000] (28 bits)
65535 [1111111111111111] -> [0111100000000000100000000000] (28 bits)
Hence minBits=6 maxBits=28

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 268 +++++++++++++++++++++++++++++++++++++++++++++------
 hw/9pfs/9p.h |  44 ++++++++-
 2 files changed, 279 insertions(+), 33 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d9be2d45d3..37abcdb71e 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -27,6 +27,7 @@
 #include "migration/blocker.h"
 #include "sysemu/qtest.h"
 #include "qemu/xxhash.h"
+#include <math.h>
 
 int open_fd_hw;
 int total_open_fd;
@@ -573,6 +574,116 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
                                 P9_STAT_MODE_NAMED_PIPE |   \
                                 P9_STAT_MODE_SOCKET)
 
+/* Mirrors all bits of a byte. So e.g. binary 10100000 would become 00000101. */
+static inline uint8_t mirror8bit(uint8_t byte)
+{
+    return (byte * 0x0202020202ULL & 0x010884422010ULL) % 1023;
+}
+
+/* Same as mirror8bit() just for a 64 bit data type instead for a byte. */
+static inline uint64_t mirror64bit(uint64_t value)
+{
+    return ((uint64_t)mirror8bit(value         & 0xff) << 56) |
+           ((uint64_t)mirror8bit((value >> 8)  & 0xff) << 48) |
+           ((uint64_t)mirror8bit((value >> 16) & 0xff) << 40) |
+           ((uint64_t)mirror8bit((value >> 24) & 0xff) << 32) |
+           ((uint64_t)mirror8bit((value >> 32) & 0xff) << 24) |
+           ((uint64_t)mirror8bit((value >> 40) & 0xff) << 16) |
+           ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8)  |
+           ((uint64_t)mirror8bit((value >> 56) & 0xff));
+}
+
+/**
+ * @brief Parameter k for the Exponential Golomb algorihm to be used.
+ *
+ * The smaller this value, the smaller the minimum bit count for the Exp.
+ * Golomb generated affixes will be (at lowest index) however for the
+ * price of having higher maximum bit count of generated affixes (at highest
+ * index). Likewise increasing this parameter yields in smaller maximum bit
+ * count for the price of having higher minimum bit count.
+ *
+ * In practice that means: a good value for k depends on the expected amount
+ * of devices to be exposed by one export. For a small amount of devices k
+ * should be small, for a large amount of devices k might be increased
+ * instead. The default of k=0 should be fine for most users though.
+ *
+ * @b IMPORTANT: In case this ever becomes a runtime parameter; the value of
+ * k should not change as long as guest is still running! Because that would
+ * cause completely different inode numbers to be generated on guest.
+ */
+#define EXP_GOLOMB_K    0
+
+/**
+ * @brief Exponential Golomb algorithm for arbitrary k (including k=0).
+ *
+ * The Exponential Golomb algorithm generates @b prefixes (@b not suffixes!)
+ * with growing length and with the mathematical property of being
+ * "prefix-free". The latter means the generated prefixes can be prepended
+ * in front of arbitrary numbers and the resulting concatenated numbers are
+ * guaranteed to be always unique.
+ *
+ * This is a minor adjustment to the original Exp. Golomb algorithm in the
+ * sense that lowest allowed index (@param n) starts with 1, not with zero.
+ *
+ * @param n - natural number (or index) of the prefix to be generated
+ *            (1, 2, 3, ...)
+ * @param k - parameter k of Exp. Golomb algorithm to be used
+ *            (see comment on EXP_GOLOMB_K macro for details about k)
+ */
+static VariLenAffix expGolombEncode(uint64_t n, int k)
+{
+    const uint64_t value = n + (1 << k) - 1;
+    const int bits = (int) log2(value) + 1;
+    return (VariLenAffix) {
+        .type = AffixType_Prefix,
+        .value = value,
+        .bits = bits + MAX((bits - 1 - k), 0)
+    };
+}
+
+/**
+ * @brief Converts a suffix into a prefix, or a prefix into a suffix.
+ *
+ * Simply mirror all bits of the affix value, for the purpose to preserve
+ * respectively the mathematical "prefix-free" or "suffix-free" property
+ * after the conversion.
+ *
+ * If a passed prefix is suitable to create unique numbers, then the
+ * returned suffix is suitable to create unique numbers as well (and vice
+ * versa).
+ */
+static VariLenAffix invertAffix(const VariLenAffix *affix)
+{
+    return (VariLenAffix) {
+        .type =
+            (affix->type == AffixType_Suffix) ?
+                AffixType_Prefix : AffixType_Suffix,
+        .value =
+            mirror64bit(affix->value) >>
+            ((sizeof(affix->value) * 8) - affix->bits),
+        .bits = affix->bits
+    };
+}
+
+/**
+ * @brief Generates suffix numbers with "suffix-free" property.
+ *
+ * This is just a wrapper function on top of the Exp. Golomb algorithm.
+ *
+ * Since the Exp. Golomb algorithm generates prefixes, but we need suffixes,
+ * this function converts the Exp. Golomb prefixes into appropriate suffixes
+ * which are still suitable for generating unique numbers.
+ *
+ * @param n - natural number (or index) of the suffix to be generated
+ *            (1, 2, 3, ...)
+ */
+static VariLenAffix affixForIndex(uint64_t index)
+{
+    VariLenAffix prefix;
+    prefix = expGolombEncode(index, EXP_GOLOMB_K);
+    return invertAffix(&prefix); /* convert prefix to suffix */
+}
+
 /* creative abuse of tb_hash_func7, which is based on xxhash */
 static uint32_t qpp_hash(QppEntry e)
 {
@@ -584,13 +695,19 @@ static uint32_t qpf_hash(QpfEntry e)
     return qemu_xxhash7(e.ino, e.dev, 0, 0, 0);
 }
 
-static bool qpp_lookup_func(const void *obj, const void *userp)
+static bool qpd_cmp_func(const void *obj, const void *userp)
+{
+    const QpdEntry *e1 = obj, *e2 = userp;
+    return e1->dev == e2->dev;
+}
+
+static bool qpp_cmp_func(const void *obj, const void *userp)
 {
     const QppEntry *e1 = obj, *e2 = userp;
     return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
 }
 
-static bool qpf_lookup_func(const void *obj, const void *userp)
+static bool qpf_cmp_func(const void *obj, const void *userp)
 {
     const QpfEntry *e1 = obj, *e2 = userp;
     return e1->dev == e2->dev && e1->ino == e2->ino;
@@ -610,16 +727,70 @@ static void qp_table_destroy(struct qht *ht)
     qht_destroy(ht);
 }
 
+static void qpd_table_init(struct qht *ht)
+{
+    qht_init(ht, qpd_cmp_func, 1, QHT_MODE_AUTO_RESIZE);
+}
+
 static void qpp_table_init(struct qht *ht)
 {
-    qht_init(ht, qpp_lookup_func, 1, QHT_MODE_AUTO_RESIZE);
+    qht_init(ht, qpp_cmp_func, 1, QHT_MODE_AUTO_RESIZE);
 }
 
 static void qpf_table_init(struct qht *ht)
 {
-    qht_init(ht, qpf_lookup_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
+    qht_init(ht, qpf_cmp_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
+}
+
+/*
+ * Returns how many (high end) bits of inode numbers of the passed fs
+ * device shall be used (in combination with the device number) to
+ * generate hash values for qpp_table entries.
+ *
+ * This function is required if variable length suffixes are used for inode
+ * number mapping on guest level. Since a device may end up having multiple
+ * entries in qpp_table, each entry most probably with a different suffix
+ * length, we thus need this function in conjunction with qpd_table to
+ * "agree" about a fix amount of bits (per device) to be always used for
+ * generating hash values for the purpose of accessing qpp_table in order
+ * get consistent behaviour when accessing qpp_table.
+ */
+static int qid_inode_prefix_hash_bits(V9fsPDU *pdu, dev_t dev)
+{
+    QpdEntry lookup = {
+        .dev = dev
+    }, *val;
+    uint32_t hash = dev;
+    VariLenAffix affix;
+
+    val = qht_lookup(&pdu->s->qpd_table, &lookup, hash);
+    if (!val) {
+        val = g_malloc0(sizeof(QpdEntry));
+        *val = lookup;
+        affix = affixForIndex(pdu->s->qp_affix_next);
+        val->prefix_bits = affix.bits;
+        qht_insert(&pdu->s->qpd_table, val, hash, NULL);
+        pdu->s->qp_ndevices++;
+    }
+    return val->prefix_bits;
 }
 
+/**
+ * @brief Slow / full mapping host inode nr -> guest inode nr.
+ *
+ * This function performs a slower and much more costly remapping of an
+ * original file inode number on host to an appropriate different inode
+ * number on guest. For every (dev, inode) combination on host a new
+ * sequential number is generated, cached and exposed as inode number on
+ * guest.
+ *
+ * This is just a "last resort" fallback solution if the much faster/cheaper
+ * qid_path_suffixmap() failed. In practice this slow / full mapping is not
+ * expected ever to be used at all though.
+ *
+ * @see qid_path_suffixmap() for details
+ *
+ */
 static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
                             uint64_t *path)
 {
@@ -628,11 +799,7 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
         .ino = stbuf->st_ino
     }, *val;
     uint32_t hash = qpf_hash(lookup);
-
-    /* most users won't need the fullmap, so init the table lazily */
-    if (!pdu->s->qpf_table.map) {
-        qpf_table_init(&pdu->s->qpf_table);
-    }
+    VariLenAffix affix;
 
     val = qht_lookup(&pdu->s->qpf_table, &lookup, hash);
 
@@ -650,8 +817,11 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
         *val = lookup;
 
         /* new unique inode and device combo */
-        val->path = pdu->s->qp_fullpath_next++;
-        pdu->s->qp_fullpath_next &= QPATH_INO_MASK;
+        affix = affixForIndex(
+            1ULL << (sizeof(pdu->s->qp_affix_next) * 8)
+        );
+        val->path = (pdu->s->qp_fullpath_next++ << affix.bits) | affix.value;
+        pdu->s->qp_fullpath_next &= ((1ULL << (64 - affix.bits)) - 1);
         qht_insert(&pdu->s->qpf_table, val, hash, NULL);
     }
 
@@ -659,31 +829,60 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
     return 0;
 }
 
-/*
- * stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
- * to a unique QID path (64 bits). To avoid having to map and keep track
- * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
- * the device id to the 16 highest bits of the QID path. The 48 lowest bits
- * of the QID path equal to the lowest bits of the inode number.
+/**
+ * @brief Quick mapping host inode nr -> guest inode nr.
  *
- * This takes advantage of the fact that inode number are usually not
- * random but allocated sequentially, so we have fewer items to keep
- * track of.
+ * This function performs quick remapping of an original file inode number
+ * on host to an appropriate different inode number on guest. This remapping
+ * of inodes is required to avoid inode nr collisions on guest which would
+ * happen if the 9p export contains more than 1 exported file system (or
+ * more than 1 file system data set), because unlike on host level where the
+ * files would have different device nrs, all files exported by 9p would
+ * share the same device nr on guest (the device nr of the virtual 9p device
+ * that is).
+ *
+ * Inode remapping is performed by chopping off high end bits of the original
+ * inode number from host, shifting the result upwards and then assigning a
+ * generated suffix number for the low end bits, where the same suffix number
+ * will be shared by all inodes with the same device id AND the same high end
+ * bits that have been chopped off. That approach utilizes the fact that inode
+ * numbers very likely share the same high end bits (i.e. due to their common
+ * sequential generation by file systems) and hence we only have to generate
+ * and track a very limited amount of suffixes in practice due to that.
+ *
+ * We generate variable size suffixes for that purpose. The 1st generated
+ * suffix will only have 1 bit and hence we only need to chop off 1 bit from
+ * the original inode number. The subsequent suffixes being generated will
+ * grow in (bit) size subsequently, i.e. the 2nd and 3rd suffix being
+ * generated will have 3 bits and hence we have to chop off 3 bits from their
+ * original inodes, and so on. That approach of using variable length suffixes
+ * (i.e. over fixed size ones) utilizes the fact that in practice only a very
+ * limited amount of devices are shared by the same export (e.g. typically
+ * less than 2 dozen devices per 9p export), so in practice we need to chop
+ * off less bits than with fixed size prefixes and yet are flexible to add
+ * new devices at runtime below host's export directory at any time without
+ * having to reboot guest nor requiring to reconfigure guest for that. And due
+ * to the very limited amount of original high end bits that we chop off that
+ * way, the total amount of suffixes we need to generate is less than by using
+ * fixed size prefixes and hence it also improves performance of the inode
+ * remapping algorithm, and finally has the nice side effect that the inode
+ * numbers on guest will be much smaller & human friendly. ;-)
  */
-static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
+static int qid_path_suffixmap(V9fsPDU *pdu, const struct stat *stbuf,
                               uint64_t *path)
 {
+    const int ino_hash_bits = qid_inode_prefix_hash_bits(pdu, stbuf->st_dev);
     QppEntry lookup = {
         .dev = stbuf->st_dev,
-        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
+        .ino_prefix = (uint16_t) (stbuf->st_ino >> (64 - ino_hash_bits))
     }, *val;
     uint32_t hash = qpp_hash(lookup);
 
     val = qht_lookup(&pdu->s->qpp_table, &lookup, hash);
 
     if (!val) {
-        if (pdu->s->qp_prefix_next == 0) {
-            /* we ran out of prefixes */
+        if (pdu->s->qp_affix_next == 0) {
+            /* we ran out of affixes */
             warn_report_once(
                 "9p: Potential degraded performance of inode remapping"
             );
@@ -693,12 +892,13 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
         val = g_malloc0(sizeof(QppEntry));
         *val = lookup;
 
-        /* new unique inode prefix and device combo */
-        val->qp_prefix = pdu->s->qp_prefix_next++;
+        /* new unique inode affix and device combo */
+        val->qp_affix_index = pdu->s->qp_affix_next++;
+        val->qp_affix = affixForIndex(val->qp_affix_index);
         qht_insert(&pdu->s->qpp_table, val, hash, NULL);
     }
-
-    *path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
+    /* assuming generated affix to be suffix type, not prefix */
+    *path = (stbuf->st_ino << val->qp_affix.bits) | val->qp_affix.value;
     return 0;
 }
 
@@ -709,7 +909,7 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
 
     if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
         /* map inode+device to qid path (fast path) */
-        err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+        err = qid_path_suffixmap(pdu, stbuf, &qidp->path);
         if (err == -ENFILE) {
             /* fast path didn't work, fall back to full map */
             err = qid_path_fullmap(pdu, stbuf, &qidp->path);
@@ -3876,8 +4076,15 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
 
     s->dev_id = stat.st_dev;
 
+    /* init inode remapping : */
+    /* hash table for variable length inode suffixes */
+    qpd_table_init(&s->qpd_table);
+    /* hash table for slow/full inode remapping (most users won't need it) */
+    qpf_table_init(&s->qpf_table);
+    /* hash table for quick inode remapping */
     qpp_table_init(&s->qpp_table);
-    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
+    s->qp_ndevices = 0;
+    s->qp_affix_next = 1; /* reserve 0 to detect overflow */
     s->qp_fullpath_next = 1;
 
     s->ctx.fst = &fse->fst;
@@ -3901,6 +4108,7 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
         fsdev_throttle_cleanup(s->ctx.fst);
     }
     g_free(s->tag);
+    qp_table_destroy(&s->qpd_table);
     qp_table_destroy(&s->qpp_table);
     qp_table_destroy(&s->qpf_table);
     g_free(s->ctx.fs_root);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 35a362c0d7..3904f82901 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -236,13 +236,49 @@ struct V9fsFidState
     V9fsFidState *rclm_lst;
 };
 
-#define QPATH_INO_MASK        ((1ULL << 48) - 1)
+typedef enum AffixType_t {
+    AffixType_Prefix,
+    AffixType_Suffix, /* A.k.a. postfix. */
+} AffixType_t;
+
+/**
+ * @brief Unique affix of variable length.
+ *
+ * An affix is (currently) either a suffix or a prefix, which is either
+ * going to be prepended (prefix) or appended (suffix) with some other
+ * number for the goal to generate unique numbers. Accordingly the
+ * suffixes (or prefixes) we generate @b must all have the mathematical
+ * property of being suffix-free (or prefix-free in case of prefixes)
+ * so that no matter what number we concatenate the affix with, that we
+ * always reliably get unique numbers as result after concatenation.
+ */
+typedef struct VariLenAffix {
+    AffixType_t type; /* Whether this affix is a suffix or a prefix. */
+    uint64_t value; /* Actual numerical value of this affix. */
+    /*
+     * Lenght of the affix, that is how many (of the lowest) bits of @c value
+     * must be used for appending/prepending this affix to its final resulting,
+     * unique number.
+     */
+    int bits;
+} VariLenAffix;
+
+/* See qid_inode_prefix_hash_bits(). */
+typedef struct {
+    dev_t dev; /* FS device on host. */
+    /*
+     * How many (high) bits of the original inode number shall be used for
+     * hashing.
+     */
+    int prefix_bits;
+} QpdEntry;
 
 /* QID path prefix entry, see stat_to_qid */
 typedef struct {
     dev_t dev;
     uint16_t ino_prefix;
-    uint16_t qp_prefix;
+    uint32_t qp_affix_index;
+    VariLenAffix qp_affix;
 } QppEntry;
 
 /* QID path full entry, as above */
@@ -274,9 +310,11 @@ struct V9fsState
     V9fsConf fsconf;
     V9fsQID root_qid;
     dev_t dev_id;
+    struct qht qpd_table;
     struct qht qpp_table;
     struct qht qpf_table;
-    uint16_t qp_prefix_next;
+    uint64_t qp_ndevices; /* Amount of entries in qpd_table. */
+    uint16_t qp_affix_next;
     uint64_t qp_fullpath_next;
 };
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Greg Kurz]

On Thu, 5 Sep 2019 12:42:01 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This is v7 of a proposed patch set for fixing file ID collisions with 9pfs.
> 

So I did some changes in 1/3 and pushed everything to 9p-next. I'll do some
more manual testing and issue a PR when I'm confident enough.

It would be nice to have some sort of automated test for that in 'make check'.
My first thought is to simulate a cross-device setup with the synth backend,
because it might be difficult to do this on a real filesystem without requiring
elevated privileges.

> v6->v7:
> 
>   * Rebased to https://github.com/gkurz/qemu/commits/9p-next
>     (SHA1 7fc4c49e91).
> 
>   * Be pedantic and abort with error on wrong value for new command line
>     argument 'multidevs'.
> 
>   * Adjusted patches to qemu code style guidelines.
> 
>   * Fixed potential crash in qp_table_destroy() on error path.
> 
>   * Use dedicated hash table init functions (qpd_table_init(),
>     qpf_table_init(), qpp_table_init()):
>     https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00144.html
> 
>   * Use warn_report_once() instead of error_report_once() for issues
>     interpreted merely as being warnings, not errors.
> 
>   * Simplified hash table destruction (due to simplified error path
>     introduced by SHA1 7fc4c49e91).
> 
>   * Dropped capturing root_ino for now:
>     https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00146.html
> 
>   * Don't update proxy docs, since new command line argument 'multidevs' is
>     limited to the local backend for now.
> 
>   * Mention in docs that readdir() is currently not blocked by
>     'multidevs=forbid'.
> 
>   * Rename qid_path_prefixmap() -> qid_path_suffixmap() in patch 3
>     (due to the semantic change of that function by that patch).
> 
> Christian Schoenebeck (3):
>   9p: Added virtfs option 'multidevs=remap|forbid|warn'
>   9p: stat_to_qid: implement slow path
>   9p: Use variable length suffixes for inode remapping
> 
>  fsdev/file-op-9p.h      |   5 +
>  fsdev/qemu-fsdev-opts.c |   7 +-
>  fsdev/qemu-fsdev.c      |  17 ++
>  hw/9pfs/9p.c            | 456 ++++++++++++++++++++++++++++++++++++++--
>  hw/9pfs/9p.h            |  59 ++++++
>  qemu-options.hx         |  26 ++-
>  vl.c                    |   7 +-
>  7 files changed, 552 insertions(+), 25 deletions(-)
> 

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Christian Schoenebeck via]

On Freitag, 13. September 2019 19:01:57 CEST Greg Kurz wrote:
> So I did some changes in 1/3 and pushed everything to 9p-next. 

I've reviewed your changes. Some notes:

Patch 1:
https://github.com/gkurz/qemu/commit/9295011c5a961603959b966c8aa6ad9840fe6db2

* Typo 1:

	error_append_hint(&local_err, "Valide options are: multidevs="

	Valide -> Valid

* Typo 2 in log comment:

	[groug: - Moved "multidevs" parsing the local backend.
	->
	[groug: - Moved "multidevs" parsing to the local backend.

> I'll do some
> more manual testing and issue a PR when I'm confident enough.

That would be highly appreciated! So far I am the only one ever having tested 
this patch set at all!

> It would be nice to have some sort of automated test for that in 'make
> check'. My first thought is to simulate a cross-device setup with the synth
> backend, because it might be difficult to do this on a real filesystem
> without requiring elevated privileges.

Hmm, since I neither haven't used the synth backend before, nor added qemu 
test cases so far, I am yet missing the complete picture here. My initial 
suggested approach would have been using loopback devices for simulating two 
file systems, but yes that's probably not viable due to required permissions. 
How would the synth backend help here? I mean you would need to simulate 
specific inode numbers and device numbers in some way for the test cases.

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Greg Kurz]

On Mon, 23 Sep 2019 11:50:46 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Freitag, 13. September 2019 19:01:57 CEST Greg Kurz wrote:
> > So I did some changes in 1/3 and pushed everything to 9p-next. 
> 
> I've reviewed your changes. Some notes:
> 
> Patch 1:
> https://github.com/gkurz/qemu/commit/9295011c5a961603959b966c8aa6ad9840fe6db2
> 
> * Typo 1:
> 
> 	error_append_hint(&local_err, "Valide options are: multidevs="
> 
> 	Valide -> Valid
> 
> * Typo 2 in log comment:
> 
> 	[groug: - Moved "multidevs" parsing the local backend.
> 	->
> 	[groug: - Moved "multidevs" parsing to the local backend.
> 

Fixed.

> > I'll do some
> > more manual testing and issue a PR when I'm confident enough.
> 
> That would be highly appreciated! So far I am the only one ever having tested 
> this patch set at all!
> 

Just to clarify, I won't thoroughly test it. My main concern is that it
doesn't break things. I usually rely on this:

https://www.tuxera.com/community/posix-test-suite/

> > It would be nice to have some sort of automated test for that in 'make
> > check'. My first thought is to simulate a cross-device setup with the synth
> > backend, because it might be difficult to do this on a real filesystem
> > without requiring elevated privileges.
> 
> Hmm, since I neither haven't used the synth backend before, nor added qemu 
> test cases so far, I am yet missing the complete picture here. My initial 
> suggested approach would have been using loopback devices for simulating two 
> file systems, but yes that's probably not viable due to required permissions. 
> How would the synth backend help here? I mean you would need to simulate 
> specific inode numbers and device numbers in some way for the test cases.
> 


The synth backend allows to simulate anything you want, provided you
code it of course :)

It is currently used to run some 9p protocol conformance tests. Have a
look at the backend code to get the idea.

hw/9pfs/9p-synth.h
hw/9pfs/9p-synth.c

and the test program:

tests/virtio-9p-test.c

It currently doesn't care for st_dev/st_ino at all, but I guess
it shouldn't be that hard to add the necessary bits.

> 
> 

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Christian Schoenebeck via]

On Montag, 23. September 2019 14:56:11 CEST Greg Kurz wrote:
> On Mon, 23 Sep 2019 11:50:46 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Freitag, 13. September 2019 19:01:57 CEST Greg Kurz wrote:
> > > So I did some changes in 1/3 and pushed everything to 9p-next.
> > 
> > I've reviewed your changes. Some notes:
> > 
> > Patch 1:
> > https://github.com/gkurz/qemu/commit/9295011c5a961603959b966c8aa6ad9840fe6
> > db2> 
> > * Typo 1:
> > 	error_append_hint(&local_err, "Valide options are: multidevs="
> > 	
> > 	Valide -> Valid
> > 
> > * Typo 2 in log comment:
> > 	[groug: - Moved "multidevs" parsing the local backend.
> > 	->
> > 	[groug: - Moved "multidevs" parsing to the local backend.
> 
> Fixed.

Thanks! Saw it, looks fine now.

> > > I'll do some
> > > more manual testing and issue a PR when I'm confident enough.
> > 
> > That would be highly appreciated! So far I am the only one ever having
> > tested this patch set at all!
> 
> Just to clarify, I won't thoroughly test it. My main concern is that it
> doesn't break things. 

So in other words you are only going to test the default behaviour
--multidevs=warn?

If yes, and since that would mean I was the only person ever having tested the 
actual fix, shouldn't --multidevs=remap|forbid better be marked as 
experimental (docs and runtime warning) for now? Maybe that would also 
anticipate receiving feedback from people actually using it later on.

> I usually rely on this:
> 
> https://www.tuxera.com/community/posix-test-suite/

Well, that website is far from being too verbose of what that test suite 
actually encompasses.

> > > It would be nice to have some sort of automated test for that in 'make
> > > check'. My first thought is to simulate a cross-device setup with the
> > > synth
> > > backend, because it might be difficult to do this on a real filesystem
> > > without requiring elevated privileges.
> > 
> > Hmm, since I neither haven't used the synth backend before, nor added qemu
> > test cases so far, I am yet missing the complete picture here. My initial
> > suggested approach would have been using loopback devices for simulating
> > two file systems, but yes that's probably not viable due to required
> > permissions. How would the synth backend help here? I mean you would need
> > to simulate specific inode numbers and device numbers in some way for the
> > test cases.
> The synth backend allows to simulate anything you want, provided you
> code it of course :)
> 
> It is currently used to run some 9p protocol conformance tests. Have a
> look at the backend code to get the idea.
> 
> hw/9pfs/9p-synth.h
> hw/9pfs/9p-synth.c
> 
> and the test program:
> 
> tests/virtio-9p-test.c
> 
> It currently doesn't care for st_dev/st_ino at all, but I guess
> it shouldn't be that hard to add the necessary bits.

I see. Well, I will look at it, but that will definitely not be one of my 
current high priority tasks that would happen in the next few weeks (simply 
due to my work load).

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Greg Kurz]

On Mon, 23 Sep 2019 16:06:13 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 23. September 2019 14:56:11 CEST Greg Kurz wrote:
> > On Mon, 23 Sep 2019 11:50:46 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > On Freitag, 13. September 2019 19:01:57 CEST Greg Kurz wrote:
> > > > So I did some changes in 1/3 and pushed everything to 9p-next.
> > > 
> > > I've reviewed your changes. Some notes:
> > > 
> > > Patch 1:
> > > https://github.com/gkurz/qemu/commit/9295011c5a961603959b966c8aa6ad9840fe6
> > > db2> 
> > > * Typo 1:
> > > 	error_append_hint(&local_err, "Valide options are: multidevs="
> > > 	
> > > 	Valide -> Valid
> > > 
> > > * Typo 2 in log comment:
> > > 	[groug: - Moved "multidevs" parsing the local backend.
> > > 	->
> > > 	[groug: - Moved "multidevs" parsing to the local backend.
> > 
> > Fixed.
> 
> Thanks! Saw it, looks fine now.
> 
> > > > I'll do some
> > > > more manual testing and issue a PR when I'm confident enough.
> > > 
> > > That would be highly appreciated! So far I am the only one ever having
> > > tested this patch set at all!
> > 
> > Just to clarify, I won't thoroughly test it. My main concern is that it
> > doesn't break things. 
> 
> So in other words you are only going to test the default behaviour
> --multidevs=warn?
> 

This I've already done, along with multidevs=forbid.

Now I plan to run the PJD test suite from Tuxera with a simple
cross-device setup and --multidevs=remap. And that's it.

> If yes, and since that would mean I was the only person ever having tested the 
> actual fix, shouldn't --multidevs=remap|forbid better be marked as 
> experimental (docs and runtime warning) for now? Maybe that would also 
> anticipate receiving feedback from people actually using it later on.
> 

Makes sense. I don't think it is worth having a runtime warning,
but I'll turn remap to x-remap and amend the docs.

> > I usually rely on this:
> > 
> > https://www.tuxera.com/community/posix-test-suite/
> 
> Well, that website is far from being too verbose of what that test suite 
> actually encompasses.
> 

Yeah... this does a variety of tests I've never had time to
audit, but it exercices many paths of the 9pfs code.

> > > > It would be nice to have some sort of automated test for that in 'make
> > > > check'. My first thought is to simulate a cross-device setup with the
> > > > synth
> > > > backend, because it might be difficult to do this on a real filesystem
> > > > without requiring elevated privileges.
> > > 
> > > Hmm, since I neither haven't used the synth backend before, nor added qemu
> > > test cases so far, I am yet missing the complete picture here. My initial
> > > suggested approach would have been using loopback devices for simulating
> > > two file systems, but yes that's probably not viable due to required
> > > permissions. How would the synth backend help here? I mean you would need
> > > to simulate specific inode numbers and device numbers in some way for the
> > > test cases.
> > The synth backend allows to simulate anything you want, provided you
> > code it of course :)
> > 
> > It is currently used to run some 9p protocol conformance tests. Have a
> > look at the backend code to get the idea.
> > 
> > hw/9pfs/9p-synth.h
> > hw/9pfs/9p-synth.c
> > 
> > and the test program:
> > 
> > tests/virtio-9p-test.c
> > 
> > It currently doesn't care for st_dev/st_ino at all, but I guess
> > it shouldn't be that hard to add the necessary bits.
> 
> I see. Well, I will look at it, but that will definitely not be one of my 
> current high priority tasks that would happen in the next few weeks (simply 
> due to my work load).
> 

Same here :)

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Christian Schoenebeck via]

On Montag, 23. September 2019 16:46:53 CEST Greg Kurz wrote:
> > > > > I'll do some
> > > > > more manual testing and issue a PR when I'm confident enough.
> > > > 
> > > > That would be highly appreciated! So far I am the only one ever having
> > > > tested this patch set at all!
> > > 
> > > Just to clarify, I won't thoroughly test it. My main concern is that it
> > > doesn't break things.
> > 
> > So in other words you are only going to test the default behaviour
> > --multidevs=warn?
> 
> This I've already done, along with multidevs=forbid.
> 
> Now I plan to run the PJD test suite from Tuxera with a simple
> cross-device setup and --multidevs=remap. And that's it.

Well, Ok then, however at least some simple, manual, final "ls -i" of the 
inode numbers on guest would not hurt though. ;-)

> > If yes, and since that would mean I was the only person ever having tested
> > the actual fix, shouldn't --multidevs=remap|forbid better be marked as
> > experimental (docs and runtime warning) for now? Maybe that would also
> > anticipate receiving feedback from people actually using it later on.
> Makes sense. I don't think it is worth having a runtime warning,
> but I'll turn remap to x-remap and amend the docs.

Mwa, I would like to veto against your "x-remap" plan though. Keep in mind I 
also have to send out a patch for libvirt for this fix. Even I would not have 
read "x" to stand for "experimental". So I would definitely favor a runtime 
warning instead of renaming that parameter.

I can send a patch on top for docs and warning if you want.

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Greg Kurz]

On Mon, 23 Sep 2019 17:03:23 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 23. September 2019 16:46:53 CEST Greg Kurz wrote:
> > > > > > I'll do some
> > > > > > more manual testing and issue a PR when I'm confident enough.
> > > > > 
> > > > > That would be highly appreciated! So far I am the only one ever having
> > > > > tested this patch set at all!
> > > > 
> > > > Just to clarify, I won't thoroughly test it. My main concern is that it
> > > > doesn't break things.
> > > 
> > > So in other words you are only going to test the default behaviour
> > > --multidevs=warn?
> > 
> > This I've already done, along with multidevs=forbid.
> > 
> > Now I plan to run the PJD test suite from Tuxera with a simple
> > cross-device setup and --multidevs=remap. And that's it.
> 
> Well, Ok then, however at least some simple, manual, final "ls -i" of the 
> inode numbers on guest would not hurt though. ;-)
> 
> > > If yes, and since that would mean I was the only person ever having tested
> > > the actual fix, shouldn't --multidevs=remap|forbid better be marked as
> > > experimental (docs and runtime warning) for now? Maybe that would also
> > > anticipate receiving feedback from people actually using it later on.
> > Makes sense. I don't think it is worth having a runtime warning,
> > but I'll turn remap to x-remap and amend the docs.
> 
> Mwa, I would like to veto against your "x-remap" plan though. Keep in mind I 
> also have to send out a patch for libvirt for this fix. Even I would not have 
> read "x" to stand for "experimental". So I would definitely favor a runtime 
> warning instead of renaming that parameter.
> 

Hmmm... I don't see the point in adding a warning for a feature that
is only active if the user explicitly asks for it. And, anyway, this
still is an experimental feature, right ? Not sure it is time to have
libvirt to support it yet.

Maybe Daniel can comment on libvirt adoption of new features ?

> I can send a patch on top for docs and warning if you want.
> 
> 

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Christian Schoenebeck via]

On Montag, 23. September 2019 18:50:12 CEST Greg Kurz wrote:
> > > > If yes, and since that would mean I was the only person ever having
> > > > tested
> > > > the actual fix, shouldn't --multidevs=remap|forbid better be marked as
> > > > experimental (docs and runtime warning) for now? Maybe that would also
> > > > anticipate receiving feedback from people actually using it later on.
> > > 
> > > Makes sense. I don't think it is worth having a runtime warning,
> > > but I'll turn remap to x-remap and amend the docs.
> > 
> > Mwa, I would like to veto against your "x-remap" plan though. Keep in mind
> > I also have to send out a patch for libvirt for this fix. Even I would
> > not have read "x" to stand for "experimental". So I would definitely
> > favor a runtime warning instead of renaming that parameter.
> 
> Hmmm... I don't see the point in adding a warning for a feature that
> is only active if the user explicitly asks for it. 

Because many people might be using this option without ever reading the docs, 
e.g. because of being suggested by copy & paste tutorials or any stack*.com 
forum.

> And, anyway, this
> still is an experimental feature, right ? 

No, it is not a feature. It is still a fix. :) I cannot use 9p without this 
fix at all, so it is not some optional "feature" for me.

x-remap would just make things unnecessarily more complicated without adding 
anything useful.

A warning/info log could be used to motivate people providing feedback and 
make them clearly aware about their current version still being an 
experimental fix in their specific qemu version. That warning/info is just a 
one line change that can easily be dropped at some point in future after this 
qid fix proofed to be reliable (i.e. from users' feedback and test cases).

> Not sure it is time to have
> libvirt to support it yet.

Most people are using libvirt xml configs instead of calling qemu directly 
with command line options. So of course I will suggest a libvirt patch as soon 
as this patch set is pulled on qemu side.

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Greg Kurz]

On Tue, 24 Sep 2019 11:31:06 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 23. September 2019 18:50:12 CEST Greg Kurz wrote:
> > > > > If yes, and since that would mean I was the only person ever having
> > > > > tested
> > > > > the actual fix, shouldn't --multidevs=remap|forbid better be marked as
> > > > > experimental (docs and runtime warning) for now? Maybe that would also
> > > > > anticipate receiving feedback from people actually using it later on.
> > > > 
> > > > Makes sense. I don't think it is worth having a runtime warning,
> > > > but I'll turn remap to x-remap and amend the docs.
> > > 
> > > Mwa, I would like to veto against your "x-remap" plan though. Keep in mind
> > > I also have to send out a patch for libvirt for this fix. Even I would
> > > not have read "x" to stand for "experimental". So I would definitely
> > > favor a runtime warning instead of renaming that parameter.
> > 
> > Hmmm... I don't see the point in adding a warning for a feature that
> > is only active if the user explicitly asks for it. 
> 
> Because many people might be using this option without ever reading the docs, 
> e.g. because of being suggested by copy & paste tutorials or any stack*.com 
> forum.
> 
> > And, anyway, this
> > still is an experimental feature, right ? 
> 
> No, it is not a feature. It is still a fix. :) I cannot use 9p without this 
> fix at all, so it is not some optional "feature" for me.
> 

I understand your need but this is still arguable. The 9p device has
a limitation with cross-device setups. The actual bug is to silently
cause inode number collisions in the guest. This is partly fixed by the
"9p: Treat multiple devices on one export as an error" patch. Thinking
again, it would even make sense to move "remap" from "9p: Added virtfs
option 'multidevs=remap|forbid|warn'" to its own patch. We could then
consider that the bug is fully fixed with "multidevs=forbid|warn".

Then comes the "remap" feature which is expected to lift the limitation
with cross-device setups, with a "not yet determined" performance cost
and light reviewing of the code.

> x-remap would just make things unnecessarily more complicated without adding 
> anything useful.
> 

Not really. This gives a crucial information to the user about the
level of confidence we have in this feature.

> A warning/info log could be used to motivate people providing feedback and 
> make them clearly aware about their current version still being an 
> experimental fix in their specific qemu version. That warning/info is just a 
> one line change that can easily be dropped at some point in future after this 
> qid fix proofed to be reliable (i.e. from users' feedback and test cases).
> 

The overwhelming majority of feedbacks I had on 9p the last few years
are CVEs. Antonios and you are the only users who ever seemed to care
for cross-device setups. So I don't expect much feedback on that area
and I don't buy the "motivate people" argument, especially since "remap"
won't be the default.

> > Not sure it is time to have
> > libvirt to support it yet.
> 
> Most people are using libvirt xml configs instead of calling qemu directly 
> with command line options. So of course I will suggest a libvirt patch as soon 
> as this patch set is pulled on qemu side.
> 

Yes and before a feature has a chance to be officially supported
in libvirt, people usually rely on the <qemu:commandline> domain
XML tag to pass extra arguments to QEMU.

https://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html

With the current fsdev implementation, we can only pass properties
to the -fsdev command line option. So this would require to not
use the <filesystem type='mount'> XML tag and manually _open-code_
the needed QEMU arguments:

<qemu:commandline>
  <qemu:arg value='-fsdev'/>
  <qemu:arg value='local,id=fsdev0,path=/var/tmp/virtfs,security_model=passthrough,multidevs=remap'/>
  <qemu:arg value='-device'/>
  <qemu:arg value='virtio-9p,id=virtio-9p0,mount_tag=host,fsdev=fsdev0'/>
</qemu:commandline>

And if fsdev is converted to be a proper QEMU device, it would as
easy as:

<qemu:commandline>
  <qemu:arg value='-set'/>
  <qemu:arg value='device.fsdev0.multidevs=remap'/>
</qemu:commandline>

This is unrelated but it would also allow to drop a lot
of code in fsdev that mimics what qdev would give us
for free. :)

> 
> 

Also, I strongly recommend you try out "virtio-fs" which is
going to be soon the production grade way of sharing files
between host and guest.

https://www.mail-archive.com/libvir-list@redhat.com/msg182457.html

Cheers,

--
Greg

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Christian Schoenebeck]

On Dienstag, 8. Oktober 2019 11:14:59 CEST Greg Kurz wrote:
> > No, it is not a feature. It is still a fix. :) I cannot use 9p without
> > this
> > fix at all, so it is not some optional "feature" for me.
> 
> I understand your need but this is still arguable. The 9p device has
> a limitation with cross-device setups. The actual bug is to silently
> cause inode number collisions in the guest. This is partly fixed by the
> "9p: Treat multiple devices on one export as an error" patch. Thinking
> again, it would even make sense to move "remap" from "9p: Added virtfs
> option 'multidevs=remap|forbid|warn'" to its own patch. We could then
> consider that the bug is fully fixed with "multidevs=forbid|warn".
> 
> Then comes the "remap" feature which is expected to lift the limitation
> with cross-device setups, with a "not yet determined" performance cost
> and light reviewing of the code.

Are these patch transfer requests addressed at me to be done?

> Also, I strongly recommend you try out "virtio-fs" which is
> going to be soon the production grade way of sharing files
> between host and guest.
> 
> https://www.mail-archive.com/libvir-list@redhat.com/msg182457.html

Yes I know, I am following the development of virtio-fs already of course. 
However for me it is far too early to actually use it in a production 
environment. It e.g. seems to require bleeding edge kernel versions. And the 
real argument for switching from 9p to virtio-fs would be a significant 
performance increase. However so far (correct me if I am wrong) I have not 
seen benchmarks that would show that this was already the case (yet).

I wonder though whether virtio-fs suffers from the same file ID collisions 
problem when sharing multiple file systems.

What is your long-term plan for 9p? Will it be dropped completely after 
virtio-fs became stable?

Best regards,
Christian Schoenebeck

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Greg Kurz]

On Tue, 08 Oct 2019 14:05:28 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 8. Oktober 2019 11:14:59 CEST Greg Kurz wrote:
> > > No, it is not a feature. It is still a fix. :) I cannot use 9p without
> > > this
> > > fix at all, so it is not some optional "feature" for me.
> > 
> > I understand your need but this is still arguable. The 9p device has
> > a limitation with cross-device setups. The actual bug is to silently
> > cause inode number collisions in the guest. This is partly fixed by the
> > "9p: Treat multiple devices on one export as an error" patch. Thinking
> > again, it would even make sense to move "remap" from "9p: Added virtfs
> > option 'multidevs=remap|forbid|warn'" to its own patch. We could then
> > consider that the bug is fully fixed with "multidevs=forbid|warn".
> > 
> > Then comes the "remap" feature which is expected to lift the limitation
> > with cross-device setups, with a "not yet determined" performance cost
> > and light reviewing of the code.
> 
> Are these patch transfer requests addressed at me to be done?
> 

It would certainly be appreciated :) and if it happens to be done
before 2019-10-29, it can even be shipped with QEMU 4.2.

> > Also, I strongly recommend you try out "virtio-fs" which is
> > going to be soon the production grade way of sharing files
> > between host and guest.
> > 
> > https://www.mail-archive.com/libvir-list@redhat.com/msg182457.html
> 
> Yes I know, I am following the development of virtio-fs already of course. 
> However for me it is far too early to actually use it in a production 
> environment. It e.g. seems to require bleeding edge kernel versions. And the 
> real argument for switching from 9p to virtio-fs would be a significant 
> performance increase. However so far (correct me if I am wrong) I have not 
> seen benchmarks that would show that this was already the case (yet).
> 
> I wonder though whether virtio-fs suffers from the same file ID collisions 
> problem when sharing multiple file systems.
> 

I don't know.

> What is your long-term plan for 9p? Will it be dropped completely after 
> virtio-fs became stable?
> 

No, 9p will survive. The local backend has an advantage despite its various
limitations: it is really easy to use. No extra command needed. I also want
to keep the synth backend around for testing, but I'll gladly drop the
proxy backend which is clearly superseded by virtio-fs.

Cheers,

--
Greg

> Best regards,
> Christian Schoenebeck
> 
> 

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Christian Schoenebeck]

On Dienstag, 8. Oktober 2019 15:47:29 CEST Greg Kurz wrote:
> On Tue, 08 Oct 2019 14:05:28 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Dienstag, 8. Oktober 2019 11:14:59 CEST Greg Kurz wrote:
> > > > No, it is not a feature. It is still a fix. :) I cannot use 9p without
> > > > this
> > > > fix at all, so it is not some optional "feature" for me.
> > > 
> > > I understand your need but this is still arguable. The 9p device has
> > > a limitation with cross-device setups. The actual bug is to silently
> > > cause inode number collisions in the guest. This is partly fixed by the
> > > "9p: Treat multiple devices on one export as an error" patch. Thinking
> > > again, it would even make sense to move "remap" from "9p: Added virtfs
> > > option 'multidevs=remap|forbid|warn'" to its own patch. We could then
> > > consider that the bug is fully fixed with "multidevs=forbid|warn".
> > > 
> > > Then comes the "remap" feature which is expected to lift the limitation
> > > with cross-device setups, with a "not yet determined" performance cost
> > > and light reviewing of the code.
> > 
> > Are these patch transfer requests addressed at me to be done?
> 
> It would certainly be appreciated :) and if it happens to be done
> before 2019-10-29, it can even be shipped with QEMU 4.2.

Just to avoid any misapprehension, since today's comments of yours made me 
sceptical: that would be

a) PR for QEMU 4.2 for *all* QID patches up to and including remapping with 
variable suffix (e.g. as -multidevs=x-remap)?

or is your current plan rather 

b) to ship the discussed 9p patches for QEMU 4.2 only up to a certain patch 
like multidevs=warn|forbid and all subsequent patches "might" then be merged 
to master somewhere in distant future?

Best regards,
Christian Schoenebeck

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Greg Kurz]

On Tue, 08 Oct 2019 16:25:48 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 8. Oktober 2019 15:47:29 CEST Greg Kurz wrote:
> > On Tue, 08 Oct 2019 14:05:28 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > On Dienstag, 8. Oktober 2019 11:14:59 CEST Greg Kurz wrote:
> > > > > No, it is not a feature. It is still a fix. :) I cannot use 9p without
> > > > > this
> > > > > fix at all, so it is not some optional "feature" for me.
> > > > 
> > > > I understand your need but this is still arguable. The 9p device has
> > > > a limitation with cross-device setups. The actual bug is to silently
> > > > cause inode number collisions in the guest. This is partly fixed by the
> > > > "9p: Treat multiple devices on one export as an error" patch. Thinking
> > > > again, it would even make sense to move "remap" from "9p: Added virtfs
> > > > option 'multidevs=remap|forbid|warn'" to its own patch. We could then
> > > > consider that the bug is fully fixed with "multidevs=forbid|warn".
> > > > 
> > > > Then comes the "remap" feature which is expected to lift the limitation
> > > > with cross-device setups, with a "not yet determined" performance cost
> > > > and light reviewing of the code.
> > > 
> > > Are these patch transfer requests addressed at me to be done?
> > 
> > It would certainly be appreciated :) and if it happens to be done
> > before 2019-10-29, it can even be shipped with QEMU 4.2.
> 
> Just to avoid any misapprehension, since today's comments of yours made me 
> sceptical: that would be
> 
> a) PR for QEMU 4.2 for *all* QID patches up to and including remapping with 
> variable suffix (e.g. as -multidevs=x-remap)?
> 
> or is your current plan rather 
> 
> b) to ship the discussed 9p patches for QEMU 4.2 only up to a certain patch 
> like multidevs=warn|forbid and all subsequent patches "might" then be merged 
> to master somewhere in distant future?
> 

Rather b) but I guess other patches could be merged as soon as the 4.3
development phase begins (mid December), and I'll continue to push patches
to my 9p-next branch during the freeze period anyway.

> Best regards,
> Christian Schoenebeck
> 
> 

Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Greg Kurz]

On Tue, 08 Oct 2019 14:05:28 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> 
> I wonder though whether virtio-fs suffers from the same file ID collisions 
> problem when sharing multiple file systems.
> 

I gave a try and it seems that virtio-fs might expose the inode numbers from
different devices in the host, unvirtualized AND with the same device in the
guest:

# mkdir -p /var/tmp/virtio-fs/proc
# mount --bind /proc /var/tmp/virtio-fs/proc
# virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/var/tmp/virtio-fs -o cache=always

and then started QEMU with:

-chardev socket,id=char0,path=/tmp/vhostqemu \
-device vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs \
-m 4G -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
-numa node,memdev=mem

In the host:

$ stat /var/tmp/virtio-fs
  File: /var/tmp/virtio-fs
  Size: 4096            Blocks: 8          IO Block: 4096   directory
Device: fd00h/64768d    Inode: 787796      Links: 4
Access: (0775/drwxrwxr-x)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
Context: unconfined_u:object_r:user_tmp_t:s0
Access: 2019-10-15 11:08:52.070080922 +0200
Modify: 2019-10-15 11:02:09.887404446 +0200
Change: 2019-10-15 11:02:09.887404446 +0200
 Birth: 2019-10-13 19:13:04.009699354 +0200
[greg@bahia ~]$ stat /var/tmp/virtio-fs/FOO
  File: /var/tmp/virtio-fs/FOO
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: fd00h/64768d    Inode: 790740      Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
Context: unconfined_u:object_r:user_tmp_t:s0
Access: 2019-10-15 11:02:09.888404448 +0200
Modify: 2019-10-15 11:02:09.888404448 +0200
Change: 2019-10-15 11:02:09.888404448 +0200
 Birth: 2019-10-15 11:02:09.887404446 +0200
[greg@bahia ~]$ stat /var/tmp/virtio-fs/proc/fs
  File: /var/tmp/virtio-fs/proc/fs
  Size: 0               Blocks: 0          IO Block: 1024   directory
Device: 4h/4d   Inode: 4026531845  Links: 5
Access: (0555/dr-xr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
Context: system_u:object_r:proc_t:s0
Access: 2019-10-01 14:50:09.223233901 +0200
Modify: 2019-10-01 14:50:09.223233901 +0200
Change: 2019-10-01 14:50:09.223233901 +0200
 Birth: -

In the guest:

[greg@localhost ~]$ stat /mnt
  File: /mnt
  Size: 4096            Blocks: 8          IO Block: 4096   directory
Device: 2dh/45d Inode: 787796      Links: 4
Access: (0775/drwxrwxr-x)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
Context: system_u:object_r:unlabeled_t:s0
Access: 2019-10-15 11:08:52.070080922 +0200
Modify: 2019-10-15 11:02:09.887404446 +0200
Change: 2019-10-15 11:02:09.887404446 +0200
 Birth: -
[greg@localhost ~]$ stat /mnt/FOO
  File: /mnt/FOO
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: 2dh/45d Inode: 790740      Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
Context: system_u:object_r:unlabeled_t:s0
Access: 2019-10-15 11:02:09.888404448 +0200
Modify: 2019-10-15 11:02:09.888404448 +0200
Change: 2019-10-15 11:02:09.888404448 +0200
 Birth: -
[greg@localhost ~]$ stat /mnt/proc/fs
  File: /mnt/proc/fs
  Size: 0               Blocks: 0          IO Block: 1024   directory
Device: 2dh/45d Inode: 4026531845  Links: 5
Access: (0555/dr-xr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
Context: system_u:object_r:unlabeled_t:s0
Access: 2019-10-01 14:50:09.223233901 +0200
Modify: 2019-10-01 14:50:09.223233901 +0200
Change: 2019-10-01 14:50:09.223233901 +0200
 Birth: -

Unless I'm missing something, it seems that "virtio-fs" has the same
issue we had on 9pfs before Christian's patches... :-\

--
Greg

Re: virtio-fs: Fix file ID collisions (was: 9p: Fix file ID collisions)

[Christian Schoenebeck]

On Dienstag, 15. Oktober 2019 11:20:39 CEST Greg Kurz wrote:
> On Tue, 08 Oct 2019 14:05:28 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > I wonder though whether virtio-fs suffers from the same file ID collisions
> > problem when sharing multiple file systems.
> 
> I gave a try and it seems that virtio-fs might expose the inode numbers from
> different devices in the host, unvirtualized AND with the same device in
> the guest:
> 
> # mkdir -p /var/tmp/virtio-fs/proc
> # mount --bind /proc /var/tmp/virtio-fs/proc
> # virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/var/tmp/virtio-fs
> -o cache=always
> 
> and then started QEMU with:
> 
> -chardev socket,id=char0,path=/tmp/vhostqemu \
> -device vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs \
> -m 4G -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on
> \ -numa node,memdev=mem
> 
> In the host:
> 
> $ stat /var/tmp/virtio-fs
>   File: /var/tmp/virtio-fs
>   Size: 4096            Blocks: 8          IO Block: 4096   directory
> Device: fd00h/64768d    Inode: 787796      Links: 4
> Access: (0775/drwxrwxr-x)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> Context: unconfined_u:object_r:user_tmp_t:s0
> Access: 2019-10-15 11:08:52.070080922 +0200
> Modify: 2019-10-15 11:02:09.887404446 +0200
> Change: 2019-10-15 11:02:09.887404446 +0200
>  Birth: 2019-10-13 19:13:04.009699354 +0200
> [greg@bahia ~]$ stat /var/tmp/virtio-fs/FOO
>   File: /var/tmp/virtio-fs/FOO
>   Size: 0               Blocks: 0          IO Block: 4096   regular empty
> file Device: fd00h/64768d    Inode: 790740      Links: 1
> Access: (0664/-rw-rw-r--)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> Context: unconfined_u:object_r:user_tmp_t:s0
> Access: 2019-10-15 11:02:09.888404448 +0200
> Modify: 2019-10-15 11:02:09.888404448 +0200
> Change: 2019-10-15 11:02:09.888404448 +0200
>  Birth: 2019-10-15 11:02:09.887404446 +0200
> [greg@bahia ~]$ stat /var/tmp/virtio-fs/proc/fs
>   File: /var/tmp/virtio-fs/proc/fs
>   Size: 0               Blocks: 0          IO Block: 1024   directory
> Device: 4h/4d   Inode: 4026531845  Links: 5
> Access: (0555/dr-xr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: system_u:object_r:proc_t:s0
> Access: 2019-10-01 14:50:09.223233901 +0200
> Modify: 2019-10-01 14:50:09.223233901 +0200
> Change: 2019-10-01 14:50:09.223233901 +0200
>  Birth: -
> 
> In the guest:
> 
> [greg@localhost ~]$ stat /mnt
>   File: /mnt
>   Size: 4096            Blocks: 8          IO Block: 4096   directory
> Device: 2dh/45d Inode: 787796      Links: 4
> Access: (0775/drwxrwxr-x)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> Context: system_u:object_r:unlabeled_t:s0
> Access: 2019-10-15 11:08:52.070080922 +0200
> Modify: 2019-10-15 11:02:09.887404446 +0200
> Change: 2019-10-15 11:02:09.887404446 +0200
>  Birth: -
> [greg@localhost ~]$ stat /mnt/FOO
>   File: /mnt/FOO
>   Size: 0               Blocks: 0          IO Block: 4096   regular empty
> file Device: 2dh/45d Inode: 790740      Links: 1
> Access: (0664/-rw-rw-r--)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> Context: system_u:object_r:unlabeled_t:s0
> Access: 2019-10-15 11:02:09.888404448 +0200
> Modify: 2019-10-15 11:02:09.888404448 +0200
> Change: 2019-10-15 11:02:09.888404448 +0200
>  Birth: -
> [greg@localhost ~]$ stat /mnt/proc/fs
>   File: /mnt/proc/fs
>   Size: 0               Blocks: 0          IO Block: 1024   directory
> Device: 2dh/45d Inode: 4026531845  Links: 5
> Access: (0555/dr-xr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: system_u:object_r:unlabeled_t:s0
> Access: 2019-10-01 14:50:09.223233901 +0200
> Modify: 2019-10-01 14:50:09.223233901 +0200
> Change: 2019-10-01 14:50:09.223233901 +0200
>  Birth: -
> 
> Unless I'm missing something, it seems that "virtio-fs" has the same
> issue we had on 9pfs before Christian's patches... :-\

Is a fix for this desired for virtio-fs?

Greg, did you have to update kernel version on either host or guest side to 
get virtio-fs running? Or were the discussed kernel changes just for optional 
acceleration purposes (i.e. DAX)?

Best regards,
Christian Schoenebeck

Re: virtio-fs: Fix file ID collisions (was: 9p: Fix file ID collisions)

[Dr. David Alan Gilbert]

* Christian Schoenebeck (qemu_oss@crudebyte.com) wrote:
> On Dienstag, 15. Oktober 2019 11:20:39 CEST Greg Kurz wrote:
> > On Tue, 08 Oct 2019 14:05:28 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > I wonder though whether virtio-fs suffers from the same file ID collisions
> > > problem when sharing multiple file systems.
> > 
> > I gave a try and it seems that virtio-fs might expose the inode numbers from
> > different devices in the host, unvirtualized AND with the same device in
> > the guest:
> > 
> > # mkdir -p /var/tmp/virtio-fs/proc
> > # mount --bind /proc /var/tmp/virtio-fs/proc
> > # virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/var/tmp/virtio-fs
> > -o cache=always
> > 
> > and then started QEMU with:
> > 
> > -chardev socket,id=char0,path=/tmp/vhostqemu \
> > -device vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs \
> > -m 4G -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on
> > \ -numa node,memdev=mem
> > 
> > In the host:
> > 
> > $ stat /var/tmp/virtio-fs
> >   File: /var/tmp/virtio-fs
> >   Size: 4096            Blocks: 8          IO Block: 4096   directory
> > Device: fd00h/64768d    Inode: 787796      Links: 4
> > Access: (0775/drwxrwxr-x)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> > Context: unconfined_u:object_r:user_tmp_t:s0
> > Access: 2019-10-15 11:08:52.070080922 +0200
> > Modify: 2019-10-15 11:02:09.887404446 +0200
> > Change: 2019-10-15 11:02:09.887404446 +0200
> >  Birth: 2019-10-13 19:13:04.009699354 +0200
> > [greg@bahia ~]$ stat /var/tmp/virtio-fs/FOO
> >   File: /var/tmp/virtio-fs/FOO
> >   Size: 0               Blocks: 0          IO Block: 4096   regular empty
> > file Device: fd00h/64768d    Inode: 790740      Links: 1
> > Access: (0664/-rw-rw-r--)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> > Context: unconfined_u:object_r:user_tmp_t:s0
> > Access: 2019-10-15 11:02:09.888404448 +0200
> > Modify: 2019-10-15 11:02:09.888404448 +0200
> > Change: 2019-10-15 11:02:09.888404448 +0200
> >  Birth: 2019-10-15 11:02:09.887404446 +0200
> > [greg@bahia ~]$ stat /var/tmp/virtio-fs/proc/fs
> >   File: /var/tmp/virtio-fs/proc/fs
> >   Size: 0               Blocks: 0          IO Block: 1024   directory
> > Device: 4h/4d   Inode: 4026531845  Links: 5
> > Access: (0555/dr-xr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
> > Context: system_u:object_r:proc_t:s0
> > Access: 2019-10-01 14:50:09.223233901 +0200
> > Modify: 2019-10-01 14:50:09.223233901 +0200
> > Change: 2019-10-01 14:50:09.223233901 +0200
> >  Birth: -
> > 
> > In the guest:
> > 
> > [greg@localhost ~]$ stat /mnt
> >   File: /mnt
> >   Size: 4096            Blocks: 8          IO Block: 4096   directory
> > Device: 2dh/45d Inode: 787796      Links: 4
> > Access: (0775/drwxrwxr-x)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> > Context: system_u:object_r:unlabeled_t:s0
> > Access: 2019-10-15 11:08:52.070080922 +0200
> > Modify: 2019-10-15 11:02:09.887404446 +0200
> > Change: 2019-10-15 11:02:09.887404446 +0200
> >  Birth: -
> > [greg@localhost ~]$ stat /mnt/FOO
> >   File: /mnt/FOO
> >   Size: 0               Blocks: 0          IO Block: 4096   regular empty
> > file Device: 2dh/45d Inode: 790740      Links: 1
> > Access: (0664/-rw-rw-r--)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> > Context: system_u:object_r:unlabeled_t:s0
> > Access: 2019-10-15 11:02:09.888404448 +0200
> > Modify: 2019-10-15 11:02:09.888404448 +0200
> > Change: 2019-10-15 11:02:09.888404448 +0200
> >  Birth: -
> > [greg@localhost ~]$ stat /mnt/proc/fs
> >   File: /mnt/proc/fs
> >   Size: 0               Blocks: 0          IO Block: 1024   directory
> > Device: 2dh/45d Inode: 4026531845  Links: 5
> > Access: (0555/dr-xr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
> > Context: system_u:object_r:unlabeled_t:s0
> > Access: 2019-10-01 14:50:09.223233901 +0200
> > Modify: 2019-10-01 14:50:09.223233901 +0200
> > Change: 2019-10-01 14:50:09.223233901 +0200
> >  Birth: -
> > 
> > Unless I'm missing something, it seems that "virtio-fs" has the same
> > issue we had on 9pfs before Christian's patches... :-\
> 
> Is a fix for this desired for virtio-fs?

Yes I think so;  we had originally thought we were hiding the host inode
numbers; but that's not true - since we pass both a device and inode
number in virtiofs, unlike 9p, it seems we can probably get away with
only remapping device IDs rather than inode numbers; but that requires
some understanding of how multiple block device IDs are supposed to look
like to the guest kernel.

Dave

> Greg, did you have to update kernel version on either host or guest side to 
> get virtio-fs running? Or were the discussed kernel changes just for optional 
> acceleration purposes (i.e. DAX)?
> 
> Best regards,
> Christian Schoenebeck
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: virtio-fs: Fix file ID collisions (was: 9p: Fix file ID collisions)

[Christian Schoenebeck]

On Mittwoch, 16. Oktober 2019 15:44:09 CEST Dr. David Alan Gilbert wrote:
> > > Unless I'm missing something, it seems that "virtio-fs" has the same
> > > issue we had on 9pfs before Christian's patches... :-\
> > 
> > Is a fix for this desired for virtio-fs?
> 
> Yes I think so;  we had originally thought we were hiding the host inode
> numbers; but that's not true - since we pass both a device and inode
> number in virtiofs, unlike 9p, it seems we can probably get away with
> only remapping device IDs rather than inode numbers; but that requires
> some understanding of how multiple block device IDs are supposed to look
> like to the guest kernel.

Postponed on my side then. My original idea was simply sharing the existing 
inode remapping code from 9p. But remapping/adding device ids on guest side 
like you suggested is cleaner; takes more time though to lookup the required 
kernel interfaces to achieve that.

Best regards,
Christian Schoenebeck

Re: virtio-fs: Fix file ID collisions (was: 9p: Fix file ID collisions)

[Greg Kurz]

On Wed, 16 Oct 2019 11:42:52 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 15. Oktober 2019 11:20:39 CEST Greg Kurz wrote:
> > On Tue, 08 Oct 2019 14:05:28 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > I wonder though whether virtio-fs suffers from the same file ID collisions
> > > problem when sharing multiple file systems.
> > 
> > I gave a try and it seems that virtio-fs might expose the inode numbers from
> > different devices in the host, unvirtualized AND with the same device in
> > the guest:
> > 
> > # mkdir -p /var/tmp/virtio-fs/proc
> > # mount --bind /proc /var/tmp/virtio-fs/proc
> > # virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/var/tmp/virtio-fs
> > -o cache=always
> > 
> > and then started QEMU with:
> > 
> > -chardev socket,id=char0,path=/tmp/vhostqemu \
> > -device vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs \
> > -m 4G -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on
> > \ -numa node,memdev=mem
> > 
> > In the host:
> > 
> > $ stat /var/tmp/virtio-fs
> >   File: /var/tmp/virtio-fs
> >   Size: 4096            Blocks: 8          IO Block: 4096   directory
> > Device: fd00h/64768d    Inode: 787796      Links: 4
> > Access: (0775/drwxrwxr-x)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> > Context: unconfined_u:object_r:user_tmp_t:s0
> > Access: 2019-10-15 11:08:52.070080922 +0200
> > Modify: 2019-10-15 11:02:09.887404446 +0200
> > Change: 2019-10-15 11:02:09.887404446 +0200
> >  Birth: 2019-10-13 19:13:04.009699354 +0200
> > [greg@bahia ~]$ stat /var/tmp/virtio-fs/FOO
> >   File: /var/tmp/virtio-fs/FOO
> >   Size: 0               Blocks: 0          IO Block: 4096   regular empty
> > file Device: fd00h/64768d    Inode: 790740      Links: 1
> > Access: (0664/-rw-rw-r--)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> > Context: unconfined_u:object_r:user_tmp_t:s0
> > Access: 2019-10-15 11:02:09.888404448 +0200
> > Modify: 2019-10-15 11:02:09.888404448 +0200
> > Change: 2019-10-15 11:02:09.888404448 +0200
> >  Birth: 2019-10-15 11:02:09.887404446 +0200
> > [greg@bahia ~]$ stat /var/tmp/virtio-fs/proc/fs
> >   File: /var/tmp/virtio-fs/proc/fs
> >   Size: 0               Blocks: 0          IO Block: 1024   directory
> > Device: 4h/4d   Inode: 4026531845  Links: 5
> > Access: (0555/dr-xr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
> > Context: system_u:object_r:proc_t:s0
> > Access: 2019-10-01 14:50:09.223233901 +0200
> > Modify: 2019-10-01 14:50:09.223233901 +0200
> > Change: 2019-10-01 14:50:09.223233901 +0200
> >  Birth: -
> > 
> > In the guest:
> > 
> > [greg@localhost ~]$ stat /mnt
> >   File: /mnt
> >   Size: 4096            Blocks: 8          IO Block: 4096   directory
> > Device: 2dh/45d Inode: 787796      Links: 4
> > Access: (0775/drwxrwxr-x)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> > Context: system_u:object_r:unlabeled_t:s0
> > Access: 2019-10-15 11:08:52.070080922 +0200
> > Modify: 2019-10-15 11:02:09.887404446 +0200
> > Change: 2019-10-15 11:02:09.887404446 +0200
> >  Birth: -
> > [greg@localhost ~]$ stat /mnt/FOO
> >   File: /mnt/FOO
> >   Size: 0               Blocks: 0          IO Block: 4096   regular empty
> > file Device: 2dh/45d Inode: 790740      Links: 1
> > Access: (0664/-rw-rw-r--)  Uid: ( 1000/    greg)   Gid: ( 1000/    greg)
> > Context: system_u:object_r:unlabeled_t:s0
> > Access: 2019-10-15 11:02:09.888404448 +0200
> > Modify: 2019-10-15 11:02:09.888404448 +0200
> > Change: 2019-10-15 11:02:09.888404448 +0200
> >  Birth: -
> > [greg@localhost ~]$ stat /mnt/proc/fs
> >   File: /mnt/proc/fs
> >   Size: 0               Blocks: 0          IO Block: 1024   directory
> > Device: 2dh/45d Inode: 4026531845  Links: 5
> > Access: (0555/dr-xr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
> > Context: system_u:object_r:unlabeled_t:s0
> > Access: 2019-10-01 14:50:09.223233901 +0200
> > Modify: 2019-10-01 14:50:09.223233901 +0200
> > Change: 2019-10-01 14:50:09.223233901 +0200
> >  Birth: -
> > 
> > Unless I'm missing something, it seems that "virtio-fs" has the same
> > issue we had on 9pfs before Christian's patches... :-\
> 
> Is a fix for this desired for virtio-fs?
> 

Probably. Dave and/or Stefan should know what is needed.

> Greg, did you have to update kernel version on either host or guest side to 

I didn't change the kernel on the host (5.2.17-200.fc30.x86_64) but the 
victim guest was running upstream kernel (SHA1 5bc52f64e8841).

> get virtio-fs running? Or were the discussed kernel changes just for optional 
> acceleration purposes (i.e. DAX)?
> 
> Best regards,
> Christian Schoenebeck
> 
>