* [Qemu-devel] [PATCH v3 1/5] 9p: mitigates most QID path collisions
2019-05-06 14:37 [Qemu-devel] [PATCH v3 0/5] 9p: Fix file ID collisions Christian Schoenebeck via Qemu-devel
@ 2019-04-23 11:30 ` Christian Schoenebeck via Qemu-devel
2019-05-07 12:42 ` Daniel P. Berrangé
2019-04-23 11:35 ` [Qemu-devel] [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation Christian Schoenebeck via Qemu-devel
` (3 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-04-23 11:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
This first patch here is an updated version of Antonios Motakis'
original 4-patch set (using fixed length 16 bit prefixes), merged to one
patch:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02283.html
* Updated to latest git master, specifically to new qht interface.
* Merged the original 4 patches to this single patch.
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
fsdev/9p-marshal.h | 4 +-
hw/9pfs/9p.c | 200 ++++++++++++++++++++++++++++++++++++++++++++++++-----
hw/9pfs/9p.h | 21 ++++++
3 files changed, 204 insertions(+), 21 deletions(-)
diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
index c8823d878f..d1ad3645c4 100644
--- a/fsdev/9p-marshal.h
+++ b/fsdev/9p-marshal.h
@@ -10,8 +10,8 @@ typedef struct V9fsString
typedef struct V9fsQID
{
int8_t type;
- int32_t version;
- int64_t path;
+ uint32_t version;
+ uint64_t path;
} V9fsQID;
typedef struct V9fsStat
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 55821343e5..b9bbdcbaee 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -25,6 +25,7 @@
#include "trace.h"
#include "migration/blocker.h"
#include "sysemu/qtest.h"
+#include "qemu/xxhash.h"
int open_fd_hw;
int total_open_fd;
@@ -571,14 +572,135 @@ 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 */
-static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
+
+/* creative abuse of qemu_xxhash7, which is based on xxhash */
+static uint32_t qpp_hash(QppEntry e)
{
- size_t size;
+ 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_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_cmp_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 qp_table_destroy(struct qht *ht)
+{
+ qht_iter(ht, qp_table_remove, NULL);
+ qht_destroy(ht);
+}
+
+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) {
+ qht_init(&pdu->s->qpf_table, qpf_cmp_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
+ }
+
+ val = qht_lookup(&pdu->s->qpf_table, &lookup, hash);
+
+ if (!val) {
+ if (pdu->s->qp_fullpath_next == 0) {
+ /* no more files can be mapped :'( */
+ return -ENFILE;
+ }
+
+ val = g_malloc0(sizeof(QppEntry));
+ if (!val) {
+ return -ENOMEM;
+ }
+ *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
+ * 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 */
+ return -ENFILE;
+ }
+
+ val = g_malloc0(sizeof(QppEntry));
+ if (!val) {
+ return -ENOMEM;
+ }
+ *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;
+
+ /* map inode+device to qid path (fast path) */
+ err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+ if (err == -ENFILE) {
+ /* fast path didn't work, fal back to full map */
+ err = qid_path_fullmap(pdu, stbuf, &qidp->path);
+ }
+ if (err) {
+ return err;
+ }
- 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)) {
@@ -587,6 +709,8 @@ static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
if (S_ISLNK(stbuf->st_mode)) {
qidp->type |= P9_QID_TYPE_SYMLINK;
}
+
+ return 0;
}
static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
@@ -599,7 +723,10 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
if (err < 0) {
return err;
}
- stat_to_qid(&stbuf, qidp);
+ err = stat_to_qid(pdu, &stbuf, qidp);
+ if (err < 0) {
+ return err;
+ }
return 0;
}
@@ -830,7 +957,10 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
memset(v9stat, 0, sizeof(*v9stat));
- stat_to_qid(stbuf, &v9stat->qid);
+ err = stat_to_qid(pdu, stbuf, &v9stat->qid);
+ if (err < 0) {
+ return err;
+ }
v9stat->mode = stat_to_v9mode(stbuf);
v9stat->atime = stbuf->st_atime;
v9stat->mtime = stbuf->st_mtime;
@@ -891,7 +1021,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
#define P9_STATS_ALL 0x00003fffULL /* Mask for All fields above */
-static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
+static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
V9fsStatDotl *v9lstat)
{
memset(v9lstat, 0, sizeof(*v9lstat));
@@ -913,7 +1043,7 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
/* Currently we only support BASIC fields in stat */
v9lstat->st_result_mask = P9_STATS_BASIC;
- stat_to_qid(stbuf, &v9lstat->qid);
+ return stat_to_qid(pdu, stbuf, &v9lstat->qid);
}
static void print_sg(struct iovec *sg, int cnt)
@@ -1115,7 +1245,6 @@ static void coroutine_fn v9fs_getattr(void *opaque)
uint64_t request_mask;
V9fsStatDotl v9stat_dotl;
V9fsPDU *pdu = opaque;
- V9fsState *s = pdu->s;
retval = pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
if (retval < 0) {
@@ -1136,7 +1265,10 @@ static void coroutine_fn v9fs_getattr(void *opaque)
if (retval < 0) {
goto out;
}
- stat_to_v9stat_dotl(s, &stbuf, &v9stat_dotl);
+ retval = stat_to_v9stat_dotl(pdu, &stbuf, &v9stat_dotl);
+ if (retval < 0) {
+ goto out;
+ }
/* fill st_gen if requested and supported by underlying fs */
if (request_mask & P9_STATS_GEN) {
@@ -1381,7 +1513,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
if (err < 0) {
goto out;
}
- stat_to_qid(&stbuf, &qid);
+ err = stat_to_qid(pdu, &stbuf, &qid);
+ if (err < 0) {
+ goto out;
+ }
v9fs_path_copy(&dpath, &path);
}
memcpy(&qids[name_idx], &qid, sizeof(qid));
@@ -1483,7 +1618,10 @@ static void coroutine_fn v9fs_open(void *opaque)
if (err < 0) {
goto out;
}
- stat_to_qid(&stbuf, &qid);
+ err = stat_to_qid(pdu, &stbuf, &qid);
+ if (err < 0) {
+ goto out;
+ }
if (S_ISDIR(stbuf.st_mode)) {
err = v9fs_co_opendir(pdu, fidp);
if (err < 0) {
@@ -1593,7 +1731,10 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
fidp->flags |= FID_NON_RECLAIMABLE;
}
iounit = get_iounit(pdu, &fidp->path);
- stat_to_qid(&stbuf, &qid);
+ err = stat_to_qid(pdu, &stbuf, &qid);
+ if (err < 0) {
+ goto out;
+ }
err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
if (err < 0) {
goto out;
@@ -2327,7 +2468,10 @@ static void coroutine_fn v9fs_create(void *opaque)
}
}
iounit = get_iounit(pdu, &fidp->path);
- stat_to_qid(&stbuf, &qid);
+ err = stat_to_qid(pdu, &stbuf, &qid);
+ if (err < 0) {
+ goto out;
+ }
err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
if (err < 0) {
goto out;
@@ -2384,7 +2528,10 @@ static void coroutine_fn v9fs_symlink(void *opaque)
if (err < 0) {
goto out;
}
- stat_to_qid(&stbuf, &qid);
+ err = stat_to_qid(pdu, &stbuf, &qid);
+ if (err < 0) {
+ goto out;
+ }
err = pdu_marshal(pdu, offset, "Q", &qid);
if (err < 0) {
goto out;
@@ -3064,7 +3211,10 @@ static void coroutine_fn v9fs_mknod(void *opaque)
if (err < 0) {
goto out;
}
- stat_to_qid(&stbuf, &qid);
+ err = stat_to_qid(pdu, &stbuf, &qid);
+ if (err < 0) {
+ goto out;
+ }
err = pdu_marshal(pdu, offset, "Q", &qid);
if (err < 0) {
goto out;
@@ -3222,7 +3372,10 @@ static void coroutine_fn v9fs_mkdir(void *opaque)
if (err < 0) {
goto out;
}
- stat_to_qid(&stbuf, &qid);
+ err = stat_to_qid(pdu, &stbuf, &qid);
+ if (err < 0) {
+ goto out;
+ }
err = pdu_marshal(pdu, offset, "Q", &qid);
if (err < 0) {
goto out;
@@ -3633,6 +3786,11 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
goto out;
}
+ /* QID path hash table. 1 entry ought to be enough for anybody ;) */
+ qht_init(&s->qpp_table, qpp_cmp_func, 1, QHT_MODE_AUTO_RESIZE);
+ 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);
@@ -3646,6 +3804,8 @@ out:
}
g_free(s->tag);
g_free(s->ctx.fs_root);
+ qp_table_destroy(&s->qpp_table);
+ qp_table_destroy(&s->qpf_table);
v9fs_path_free(&path);
}
return rc;
@@ -3658,6 +3818,8 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
}
fsdev_throttle_cleanup(s->ctx.fst);
g_free(s->tag);
+ 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 8883761b2c..44112ea97f 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,22 @@ struct V9fsFidState
V9fsFidState *rclm_lst;
};
+#define QPATH_INO_MASK (((unsigned long)1 << 48) - 1)
+
+/* QID path prefix entry, see stat_to_qid */
+typedef struct {
+ dev_t dev;
+ uint16_t ino_prefix;
+ 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;
@@ -256,6 +273,10 @@ struct V9fsState
Error *migration_blocker;
V9fsConf fsconf;
V9fsQID root_qid;
+ struct qht qpp_table;
+ struct qht qpf_table;
+ uint16_t qp_prefix_next;
+ uint64_t qp_fullpath_next;
};
/* 9p2000.L open flags */
--
2.11.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation
2019-05-06 14:37 [Qemu-devel] [PATCH v3 0/5] 9p: Fix file ID collisions Christian Schoenebeck via Qemu-devel
2019-04-23 11:30 ` [Qemu-devel] [PATCH v3 1/5] 9p: mitigates most QID path collisions Christian Schoenebeck via Qemu-devel
@ 2019-04-23 11:35 ` Christian Schoenebeck via Qemu-devel
2019-05-07 12:43 ` Daniel P. Berrangé
2019-04-23 11:41 ` [Qemu-devel] [PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions Christian Schoenebeck via Qemu-devel
` (2 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-04-23 11:35 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
Addresses trivial changes regarding the previous patch as requested
on the mailing list a while ago.
* Removed unneccessary parantheses:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02661.html
* Removed unneccessary g_malloc() result checks:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02814.html
* Unsigned type changes:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02581.html
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
fsdev/9p-marshal.h | 2 +-
hw/9pfs/9p.c | 16 +++++-----------
hw/9pfs/trace-events | 14 +++++++-------
3 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
index d1ad3645c4..8f3babb60a 100644
--- a/fsdev/9p-marshal.h
+++ b/fsdev/9p-marshal.h
@@ -9,7 +9,7 @@ typedef struct V9fsString
typedef struct V9fsQID
{
- int8_t type;
+ uint8_t type;
uint32_t version;
uint64_t path;
} V9fsQID;
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index b9bbdcbaee..2b893e25a1 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -587,13 +587,13 @@ static uint32_t qpf_hash(QpfEntry e)
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);
+ return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
}
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);
+ return e1->dev == e2->dev && e1->ino == e2->ino;
}
static void qp_table_remove(void *p, uint32_t h, void *up)
@@ -630,9 +630,6 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
}
val = g_malloc0(sizeof(QppEntry));
- if (!val) {
- return -ENOMEM;
- }
*val = lookup;
/* new unique inode and device combo */
@@ -673,9 +670,6 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
}
val = g_malloc0(sizeof(QppEntry));
- if (!val) {
- return -ENOMEM;
- }
*val = lookup;
/* new unique inode prefix and device combo */
@@ -870,9 +864,9 @@ static int donttouch_stat(V9fsStat *stat)
{
if (stat->type == -1 &&
stat->dev == -1 &&
- stat->qid.type == -1 &&
- stat->qid.version == -1 &&
- stat->qid.path == -1 &&
+ stat->qid.type == 0xff &&
+ stat->qid.version == (uint32_t) -1 &&
+ stat->qid.path == (uint64_t) -1 &&
stat->mode == -1 &&
stat->atime == -1 &&
stat->mtime == -1 &&
diff --git a/hw/9pfs/trace-events b/hw/9pfs/trace-events
index c0a0a4ab5d..6964756922 100644
--- a/hw/9pfs/trace-events
+++ b/hw/9pfs/trace-events
@@ -6,7 +6,7 @@ v9fs_rerror(uint16_t tag, uint8_t id, int err) "tag %d id %d err %d"
v9fs_version(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag %d id %d msize %d version %s"
v9fs_version_return(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag %d id %d msize %d version %s"
v9fs_attach(uint16_t tag, uint8_t id, int32_t fid, int32_t afid, char* uname, char* aname) "tag %u id %u fid %d afid %d uname %s aname %s"
-v9fs_attach_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path) "tag %d id %d type %d version %d path %"PRId64
+v9fs_attach_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path) "tag %d id %d type %d version %d path %"PRId64
v9fs_stat(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
v9fs_stat_return(uint16_t tag, uint8_t id, int32_t mode, int32_t atime, int32_t mtime, int64_t length) "tag %d id %d stat={mode %d atime %d mtime %d length %"PRId64"}"
v9fs_getattr(uint16_t tag, uint8_t id, int32_t fid, uint64_t request_mask) "tag %d id %d fid %d request_mask %"PRIu64
@@ -14,9 +14,9 @@ v9fs_getattr_return(uint16_t tag, uint8_t id, uint64_t result_mask, uint32_t mod
v9fs_walk(uint16_t tag, uint8_t id, int32_t fid, int32_t newfid, uint16_t nwnames) "tag %d id %d fid %d newfid %d nwnames %d"
v9fs_walk_return(uint16_t tag, uint8_t id, uint16_t nwnames, void* qids) "tag %d id %d nwnames %d qids %p"
v9fs_open(uint16_t tag, uint8_t id, int32_t fid, int32_t mode) "tag %d id %d fid %d mode %d"
-v9fs_open_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
+v9fs_open_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
v9fs_lcreate(uint16_t tag, uint8_t id, int32_t dfid, int32_t flags, int32_t mode, uint32_t gid) "tag %d id %d dfid %d flags %d mode %d gid %u"
-v9fs_lcreate_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int32_t iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
+v9fs_lcreate_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int32_t iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
v9fs_fsync(uint16_t tag, uint8_t id, int32_t fid, int datasync) "tag %d id %d fid %d datasync %d"
v9fs_clunk(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
v9fs_read(uint16_t tag, uint8_t id, int32_t fid, uint64_t off, uint32_t max_count) "tag %d id %d fid %d off %"PRIu64" max_count %u"
@@ -26,21 +26,21 @@ v9fs_readdir_return(uint16_t tag, uint8_t id, uint32_t count, ssize_t retval) "t
v9fs_write(uint16_t tag, uint8_t id, int32_t fid, uint64_t off, uint32_t count, int cnt) "tag %d id %d fid %d off %"PRIu64" count %u cnt %d"
v9fs_write_return(uint16_t tag, uint8_t id, int32_t total, ssize_t err) "tag %d id %d total %d err %zd"
v9fs_create(uint16_t tag, uint8_t id, int32_t fid, char* name, int32_t perm, int8_t mode) "tag %d id %d fid %d name %s perm %d mode %d"
-v9fs_create_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
+v9fs_create_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
v9fs_symlink(uint16_t tag, uint8_t id, int32_t fid, char* name, char* symname, uint32_t gid) "tag %d id %d fid %d name %s symname %s gid %u"
-v9fs_symlink_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
+v9fs_symlink_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
v9fs_flush(uint16_t tag, uint8_t id, int16_t flush_tag) "tag %d id %d flush_tag %d"
v9fs_link(uint16_t tag, uint8_t id, int32_t dfid, int32_t oldfid, char* name) "tag %d id %d dfid %d oldfid %d name %s"
v9fs_remove(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
v9fs_wstat(uint16_t tag, uint8_t id, int32_t fid, int32_t mode, int32_t atime, int32_t mtime) "tag %u id %u fid %d stat={mode %d atime %d mtime %d}"
v9fs_mknod(uint16_t tag, uint8_t id, int32_t fid, int mode, int major, int minor) "tag %d id %d fid %d mode %d major %d minor %d"
-v9fs_mknod_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
+v9fs_mknod_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
v9fs_lock(uint16_t tag, uint8_t id, int32_t fid, uint8_t type, uint64_t start, uint64_t length) "tag %d id %d fid %d type %d start %"PRIu64" length %"PRIu64
v9fs_lock_return(uint16_t tag, uint8_t id, int8_t status) "tag %d id %d status %d"
v9fs_getlock(uint16_t tag, uint8_t id, int32_t fid, uint8_t type, uint64_t start, uint64_t length)"tag %d id %d fid %d type %d start %"PRIu64" length %"PRIu64
v9fs_getlock_return(uint16_t tag, uint8_t id, uint8_t type, uint64_t start, uint64_t length, uint32_t proc_id) "tag %d id %d type %d start %"PRIu64" length %"PRIu64" proc_id %u"
v9fs_mkdir(uint16_t tag, uint8_t id, int32_t fid, char* name, int mode, uint32_t gid) "tag %u id %u fid %d name %s mode %d gid %u"
-v9fs_mkdir_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int err) "tag %u id %u qid={type %d version %d path %"PRId64"} err %d"
+v9fs_mkdir_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int err) "tag %u id %u qid={type %d version %d path %"PRId64"} err %d"
v9fs_xattrwalk(uint16_t tag, uint8_t id, int32_t fid, int32_t newfid, char* name) "tag %d id %d fid %d newfid %d name %s"
v9fs_xattrwalk_return(uint16_t tag, uint8_t id, int64_t size) "tag %d id %d size %"PRId64
v9fs_xattrcreate(uint16_t tag, uint8_t id, int32_t fid, char* name, uint64_t size, int flags) "tag %d id %d fid %d name %s size %"PRIu64" flags %d"
--
2.11.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions
2019-05-06 14:37 [Qemu-devel] [PATCH v3 0/5] 9p: Fix file ID collisions Christian Schoenebeck via Qemu-devel
2019-04-23 11:30 ` [Qemu-devel] [PATCH v3 1/5] 9p: mitigates most QID path collisions Christian Schoenebeck via Qemu-devel
2019-04-23 11:35 ` [Qemu-devel] [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation Christian Schoenebeck via Qemu-devel
@ 2019-04-23 11:41 ` Christian Schoenebeck via Qemu-devel
2019-05-03 14:51 ` [Qemu-devel] [PATCH v3 4/5] 9p: use variable length suffixes for inode mapping Christian Schoenebeck via Qemu-devel
2019-05-05 21:41 ` [Qemu-devel] [PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter Christian Schoenebeck via Qemu-devel
4 siblings, 0 replies; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-04-23 11:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
This patch aims to keep QID path identical beyond the scope of reboots and
guest suspensions. With the 1st patch alone the QID path of the same files
might change after reboots / suspensions, since 9p would restart with
empty qpp_table and the resulting QID path depends on the precise sequence
of files being accessed on guest.
The first patch should already avoid the vast majority of potential QID
path collisions. However especially network services running on guest
would still be prone to QID path issues when just using the 1st patch.
For instance Samba is exporting file IDs to clients in the network and
SMB cliens in the network will use those IDs to access and request
changes on the file server. If guest is now interrupted in between, like
it commonly happens on maintenance, e.g. software updates on host, then
SMB clients in the network will continue working with old file IDs, which
in turn leads to data corruption and data loss on the file server.
Furthermore on SMB client side I also encountered severe misbehaviours in
this case, for instance Macs accessing the file server would either
start to hang or die with a kernel panic within seconds, since the smbx
implementation on macOS heavily relies on file IDs being unique (within
the context of a connection that is).
So this patch here mitigates the remaining problem described above by
storing the qpp_table persistently as extended attribute(s) on the
exported root of the file system and automatically tries to restore the
qpp_table i.e. after reboots / resumptions.
This patch is aimed at real world scenarios, in which qpp_table will only
ever get few dozens of entries (and none ever in qpf_table). So it is e.g.
intentionally limited to only store qpp_table, not qpf_table; and so far
I have not made optimizations, since in practice the qpf_table is really
just tiny.
Since there is currently no callback in qemu yet that would reliably be
called on guest shutdowns, the table is stored on every new insertion for
now.
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
hw/9pfs/9p.c | 315 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
hw/9pfs/9p.h | 33 +++++++
2 files changed, 343 insertions(+), 5 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 2b893e25a1..29c6dfc68a 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -26,6 +26,19 @@
#include "migration/blocker.h"
#include "sysemu/qtest.h"
#include "qemu/xxhash.h"
+#include "qemu/crc32c.h"
+#if defined(__linux__) /* TODO: This should probably go into osdep.h instead */
+# include <linux/limits.h> /* for XATTR_SIZE_MAX */
+#endif
+
+/*
+ * How many bytes may we store to fs per extended attribute value?
+ */
+#ifdef XATTR_SIZE_MAX
+# define ATTR_MAX_SIZE XATTR_SIZE_MAX /* Linux only: 64kB limit in kernel */
+#else
+# define ATTR_MAX_SIZE 65536 /* Most systems allow a bit more, so we take this as basis. */
+#endif
int open_fd_hw;
int total_open_fd;
@@ -642,6 +655,285 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
return 0;
}
+static inline bool is_ro_export(FsContext *ctx)
+{
+ return ctx->export_flags & V9FS_RDONLY;
+}
+
+/*
+ * Once qpp_table size exceeds this value, we no longer save
+ * the table persistently. See comment in v9fs_store_qpp_table()
+ */
+#define QPP_TABLE_PERSISTENCY_LIMIT 32768
+
+/* Remove all user.virtfs.system.qidp.* xattrs from export root. */
+static void remove_qidp_xattr(FsContext *ctx)
+{
+ V9fsString name;
+ int i;
+
+ /* just for a paranoid endless recursion sanity check */
+ const ssize_t max_size =
+ sizeof(QppSrlzHeader) +
+ QPP_TABLE_PERSISTENCY_LIMIT * sizeof(QppEntryS);
+
+ v9fs_string_init(&name);
+ for (i = 0; i * ATTR_MAX_SIZE < max_size; ++i) {
+ v9fs_string_sprintf(&name, "user.virtfs.system.qidp.%d", i);
+ if (lremovexattr(ctx->fs_root, name.data) < 0)
+ break;
+ }
+ v9fs_string_free(&name);
+}
+
+/* Used to convert qpp hash table into continuous stream. */
+static void qpp_table_serialize(void *p, uint32_t h, void *up)
+{
+ const QppEntry *entry = (const QppEntry*) p;
+ QppSerialize *ser = (QppSerialize*) up;
+
+ if (ser->error)
+ return;
+
+ /* safety first */
+ if (entry->qp_prefix - 1 >= ser->count) {
+ ser->error = -1;
+ return;
+ }
+
+ ser->elements[entry->qp_prefix - 1] = (QppEntryS) {
+ .dev = entry->dev,
+ .ino_prefix = entry->ino_prefix
+ };
+ ser->done++;
+}
+
+/*
+ * Tries to store the current qpp_table as extended attribute(s) on the
+ * exported file system root with the goal to preserve identical qids
+ * beyond the scope of reboots.
+ */
+static void v9fs_store_qpp_table(V9fsState *s)
+{
+ FsContext *ctx = &s->ctx;
+ V9fsString name;
+ int i, res;
+ size_t size;
+ QppSrlzStream* stream;
+ QppSerialize ser;
+
+ if (is_ro_export(ctx))
+ return;
+
+ /*
+ * Whenever we exceeded some certain (arbitrary) high qpp_table size we
+ * delete the stored table from the file system to get rid of old device
+ * ids / inodes that might no longer exist with the goal to potentially
+ * yield in a smaller table size after next reboot.
+ */
+ if (!s->qp_prefix_next || s->qp_prefix_next >= QPP_TABLE_PERSISTENCY_LIMIT) {
+ if (s->qp_prefix_next == QPP_TABLE_PERSISTENCY_LIMIT) {
+ remove_qidp_xattr(ctx);
+ }
+ return;
+ }
+
+ /* Convert qpp hash table into continuous array. */
+ size = sizeof(QppSrlzHeader) +
+ ( (s->qp_prefix_next - 1) /* qpp_table entry count */ * sizeof(QppEntryS) );
+ stream = g_malloc0(size);
+ ser = (QppSerialize) {
+ .elements = &stream->elements[0],
+ .count = s->qp_prefix_next - 1,
+ .done = 0,
+ .error = 0,
+ };
+ qht_iter(&s->qpp_table, qpp_table_serialize, &ser);
+ if (ser.error || ser.done != ser.count)
+ goto out;
+
+ /* initialize header and calculate CRC32 checksum */
+ stream->header = (QppSrlzHeader) {
+ .version = 1,
+ .reserved = 0,
+ .crc32 = crc32c(
+ 0xffffffff,
+ (const uint8_t*) &stream->elements[0],
+ (ser.count * sizeof(QppEntryS))
+ ),
+ };
+
+ /*
+ * Actually just required if the qpp_table size decreased, or if the
+ * previous xattr size limit increased on OS (kernel/fs) level.
+ */
+ remove_qidp_xattr(ctx);
+
+ /*
+ * Subdivide (if required) the data stream into individual xattrs
+ * to cope with the system's max. supported xattr value size.
+ */
+ v9fs_string_init(&name);
+ for (i = 0; size > (i * ATTR_MAX_SIZE); ++i) {
+ v9fs_string_sprintf(&name, "user.virtfs.system.qidp.%d", i);
+ res = lsetxattr(
+ ctx->fs_root,
+ name.data,
+ ((const uint8_t*)stream) + i * ATTR_MAX_SIZE,
+ MIN(ATTR_MAX_SIZE, size - i * ATTR_MAX_SIZE),
+ 0/*flags*/
+ );
+ if (res < 0) {
+ if (i > 0)
+ remove_qidp_xattr(ctx);
+ break;
+ }
+ }
+ v9fs_string_free(&name);
+out:
+ g_free(stream);
+}
+
+/* Frees the entire chain of passed nodes from memory. */
+static void destroy_xattr_nodes(XAttrNode **first)
+{
+ XAttrNode *prev;
+ if (!first)
+ return;
+ while (*first) {
+ if ((*first)->value)
+ g_free((*first)->value);
+ prev = *first;
+ *first = (*first)->next;
+ g_free(prev);
+ }
+}
+
+/*
+ * Loads all user.virtfs.system.qidp.* xattrs from exported fs root and
+ * returns a linked list with one node per xattr.
+ */
+static XAttrNode* v9fs_load_qidp_xattr_nodes(V9fsState *s)
+{
+ FsContext *ctx = &s->ctx;
+ XAttrNode *first = NULL, *current = NULL;
+ V9fsString name;
+ ssize_t size;
+ int i;
+
+ const ssize_t max_size =
+ sizeof(QppSrlzHeader) +
+ QPP_TABLE_PERSISTENCY_LIMIT * sizeof(QppEntryS);
+
+ v9fs_string_init(&name);
+
+ for (i = 0; i * ATTR_MAX_SIZE < max_size; ++i) {
+ v9fs_string_sprintf(&name, "user.virtfs.system.qidp.%d", i);
+ size = lgetxattr(ctx->fs_root, name.data, NULL, 0);
+ if (size <= 0)
+ break;
+ if (!first) {
+ first = current = g_malloc0(sizeof(XAttrNode));
+ } else {
+ current = current->next = g_malloc0(sizeof(XAttrNode));
+ }
+ current->value = g_malloc0(size);
+ current->length = lgetxattr(
+ ctx->fs_root, name.data, current->value, size
+ );
+ if (current->length <= 0) {
+ goto out_w_err;
+ }
+ }
+ goto out;
+
+out_w_err:
+ destroy_xattr_nodes(&first);
+out:
+ v9fs_string_free(&name);
+ return first;
+}
+
+/*
+ * Try to load previously stored qpp_table from file system. Calling this
+ * function assumes that qpp_table is yet empty.
+ *
+ * @see v9fs_store_qpp_table()
+ */
+static void v9fs_load_qpp_table(V9fsState *s)
+{
+ ssize_t size, count;
+ XAttrNode *current, *first;
+ QppSrlzStream* stream = NULL;
+ uint32_t crc32;
+ int i;
+ QppEntry *val;
+ uint32_t hash;
+
+ if (s->qp_prefix_next != 1)
+ return;
+
+ first = v9fs_load_qidp_xattr_nodes(s);
+ if (!first)
+ return;
+
+ /* convert nodes into continuous stream */
+ size = 0;
+ for (current = first; current; current = current->next) {
+ size += current->length;
+ }
+ if (size <= 0) {
+ goto out;
+ }
+ stream = g_malloc0(size);
+ size = 0;
+ for (current = first; current; current = current->next) {
+ memcpy(((uint8_t*)stream) + size, current->value, current->length);
+ size += current->length;
+ }
+
+ if (stream->header.version != 1) {
+ goto out;
+ }
+
+ count = (size - sizeof(QppSrlzHeader)) / sizeof(QppEntryS);
+ if (count <= 0) {
+ goto out;
+ }
+
+ /* verify CRC32 checksum of stream */
+ crc32 = crc32c(
+ 0xffffffff,
+ (const uint8_t*) &stream->elements[0],
+ (count * sizeof(QppEntryS))
+ );
+ if (crc32 != stream->header.crc32) {
+ goto out;
+ }
+
+ /* fill qpp_table with the retrieved elements */
+ for (i = 0; i < count; ++i) {
+ val = g_malloc0(sizeof(QppEntry));
+ *val = (QppEntry) {
+ .dev = stream->elements[i].dev,
+ .ino_prefix = stream->elements[i].ino_prefix,
+ };
+ hash = qpp_hash(*val);
+ if (qht_lookup(&s->qpp_table, val, hash)) {
+ /* should never happen: duplicate entry detected */
+ g_free(val);
+ goto out;
+ }
+ val->qp_prefix = s->qp_prefix_next++;
+ qht_insert(&s->qpp_table, val, hash, NULL);
+ }
+
+out:
+ destroy_xattr_nodes(&first);
+ if (stream)
+ g_free(stream);
+}
+
/* 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
@@ -675,6 +967,14 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
/* new unique inode prefix and device combo */
val->qp_prefix = pdu->s->qp_prefix_next++;
qht_insert(&pdu->s->qpp_table, val, hash, NULL);
+
+ /*
+ * Store qpp_table as extended attribute(s) to file system.
+ *
+ * TODO: This should better only be called from a guest shutdown and
+ * suspend handler.
+ */
+ v9fs_store_qpp_table(pdu->s);
}
*path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
@@ -1064,11 +1364,6 @@ static void v9fs_fix_path(V9fsPath *dst, V9fsPath *src, int len)
v9fs_path_free(&str);
}
-static inline bool is_ro_export(FsContext *ctx)
-{
- return ctx->export_flags & V9FS_RDONLY;
-}
-
static void coroutine_fn v9fs_version(void *opaque)
{
ssize_t err;
@@ -3784,6 +4079,8 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
qht_init(&s->qpp_table, qpp_cmp_func, 1, QHT_MODE_AUTO_RESIZE);
s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
s->qp_fullpath_next = 1;
+ /* try to load and restore previous qpp_table */
+ v9fs_load_qpp_table(s);
s->ctx.fst = &fse->fst;
fsdev_throttle_init(s->ctx.fst);
@@ -3807,6 +4104,14 @@ out:
void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
{
+ /*
+ * Store qpp_table as extended attribute(s) to file system.
+ *
+ * This was actually plan A, but unfortunately unserialize is not called
+ * reliably on guest shutdowns and suspensions.
+ */
+ v9fs_store_qpp_table(s);
+
if (s->ops->cleanup) {
s->ops->cleanup(&s->ctx);
}
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 44112ea97f..54ce039969 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -245,6 +245,13 @@ typedef struct {
uint16_t qp_prefix;
} QppEntry;
+/* Small version of QppEntry for serialization as xattr. */
+struct QppEntryS {
+ dev_t dev;
+ uint16_t ino_prefix;
+} __attribute__((packed));
+typedef struct QppEntryS QppEntryS;
+
/* QID path full entry, as above */
typedef struct {
dev_t dev;
@@ -252,6 +259,32 @@ typedef struct {
uint64_t path;
} QpfEntry;
+typedef struct {
+ QppEntryS *elements;
+ uint count; /* In: QppEntryS count in @a elements */
+ uint done; /* Out: how many QppEntryS did we actually fill in @a elements */
+ int error; /* Out: zero on success */
+} QppSerialize;
+
+struct QppSrlzHeader {
+ uint16_t version;
+ uint16_t reserved; /* might be used e.g. for flags in future */
+ uint32_t crc32;
+} __attribute__((packed));
+typedef struct QppSrlzHeader QppSrlzHeader;
+
+struct QppSrlzStream {
+ QppSrlzHeader header;
+ QppEntryS elements[0];
+} __attribute__((packed));
+typedef struct QppSrlzStream QppSrlzStream;
+
+typedef struct XAttrNode {
+ uint8_t* value;
+ ssize_t length;
+ struct XAttrNode* next;
+} XAttrNode;
+
struct V9fsState
{
QLIST_HEAD(, V9fsPDU) free_list;
--
2.11.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v3 4/5] 9p: use variable length suffixes for inode mapping
2019-05-06 14:37 [Qemu-devel] [PATCH v3 0/5] 9p: Fix file ID collisions Christian Schoenebeck via Qemu-devel
` (2 preceding siblings ...)
2019-04-23 11:41 ` [Qemu-devel] [PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions Christian Schoenebeck via Qemu-devel
@ 2019-05-03 14:51 ` Christian Schoenebeck via Qemu-devel
2019-05-05 21:41 ` [Qemu-devel] [PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter Christian Schoenebeck via Qemu-devel
4 siblings, 0 replies; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-03 14:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
This patch introduces variable length suffixes for being used for inode mapping
instead of the fixed 16 bit size prefixes of patch 1. With this patch the inode
numbers on guest will typically be much smaller (e.g. around >2^1 .. >2^7
instead of >2^48 with patch 1).
I preserved both solutions in this patch though, so you can switch between
them by adjusting P9_VARI_LENGTH_INODE_SUFFIXES, which probably also makes
code comparison between the two more easy for now.
Motivation: with patch 1 the inode numbers were so high that some network
services had issues. Especially SMB seems to be a problem again due to varying
implementations regarding SMB file IDs. I don't bother you with the details.
Additionally this solution should be more efficient, since inode numbers in
practice can take almost their entire 64 bit range on guest as well. Which
might also be beneficial for nested virtualization.
The "Exponential Golomb" algorithm is used as basis for generating the
variable length suffixes. The algorithm has a paramter 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
If somebody wants to play around with the details of the suffix generation, let
me know and I will send you a small C source for you to play with.
Additionally this patch disables persistency of file IDs (that I introduced
with patch 3) at compile time by default for now. You can still enable it by
setting QPP_TABLE_PERSISTENCY_LIMIT to some reasonable high value (e.g. 127).
Reason: I am still unresolved what to do with this feature. The original
motivation was to avoid file ID collisions with network services if the machine
was interrupted for a short period. But what I did not consider was that
device IDs might change on host, so after loading the tables from fs the qp
tables would grow with irrelevant entries.
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
hw/9pfs/9p.c | 744 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
hw/9pfs/9p.h | 121 +++++++++-
2 files changed, 805 insertions(+), 60 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 29c6dfc68a..003901a1e0 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -30,6 +30,10 @@
#if defined(__linux__) /* TODO: This should probably go into osdep.h instead */
# include <linux/limits.h> /* for XATTR_SIZE_MAX */
#endif
+#include <sys/types.h>
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <math.h>
/*
* How many bytes may we store to fs per extended attribute value?
@@ -585,6 +589,123 @@ 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) ) ;
+}
+
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+
+/* 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.
+ */
+#define EXP_GOLOMB_K 0
+
+# if !EXP_GOLOMB_K
+
+/** @brief Exponential Golomb algorithm limited to the case k=0.
+ *
+ * See expGolombEncode() below for details.
+ *
+ * @param n - natural number (or index) of the prefix to be generated
+ * (1, 2, 3, ...)
+ */
+static VariLenAffix expGolombEncodeK0(uint64_t n) {
+ const int bits = (int) log2(n) + 1;
+ return (VariLenAffix) {
+ .type = AffixType_Prefix,
+ .value = n,
+ .bits = bits + bits - 1
+ };
+}
+
+# else
+
+/** @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)
+ };
+}
+
+# endif /* !EXP_GOLOMB_K */
+
+/** @brief Converts a suffix into a prefix, or a prefix into a suffix.
+ *
+ * What this function does is actually mirroring all bits of the affix value,
+ * with the purpose to preserve respectively the mathematical "prefix-free"
+ * or "suffix-free" property after the conversion.
+ *
+ * In other words: 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;
+ #if EXP_GOLOMB_K
+ prefix = expGolombEncode(index, EXP_GOLOMB_K);
+ #else
+ prefix = expGolombEncodeK0(index);
+ #endif
+ return invertAffix(&prefix); /* convert prefix to suffix */
+}
+
+#endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
/* creative abuse of qemu_xxhash7, which is based on xxhash */
static uint32_t qpp_hash(QppEntry e)
@@ -597,6 +718,16 @@ static uint32_t qpf_hash(QpfEntry e)
return qemu_xxhash7(e.ino, e.dev, 0, 0, 0);
}
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+
+static bool qpd_cmp_func(const void *obj, const void *userp)
+{
+ const QpdEntry *e1 = obj, *e2 = userp;
+ return e1->dev == e2->dev;
+}
+
+#endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
static bool qpp_cmp_func(const void *obj, const void *userp)
{
const QppEntry *e1 = obj, *e2 = userp;
@@ -620,6 +751,72 @@ static void qp_table_destroy(struct qht *ht)
qht_destroy(ht);
}
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+
+# if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
+static int v9fs_store_qpd_table(V9fsState *s);
+# endif /* (QPP_TABLE_PERSISTENCY_LIMIT > 0) */
+
+/*
+ * 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++;
+
+ /*
+ * Store qpd_table as extended attribute(s) to file system.
+ *
+ * TODO: This should better only be called from a guest shutdown and
+ * suspend handler.
+ */
+ #if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
+ v9fs_store_qpd_table(pdu->s);
+ #endif
+ }
+ return val->prefix_bits;
+}
+
+#endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
+/** @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_prefixmap() failed. In practice this slow / full mapping is not
+ * expected ever to be used at all though.
+ *
+ * @see qid_path_prefixmap() for details
+ *
+ */
static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
uint64_t *path)
{
@@ -628,11 +825,9 @@ 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) {
- qht_init(&pdu->s->qpf_table, qpf_cmp_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
- }
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ VariLenAffix affix;
+#endif
val = qht_lookup(&pdu->s->qpf_table, &lookup, hash);
@@ -646,8 +841,16 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
*val = lookup;
/* new unique inode and device combo */
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ 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);
+#else
val->path = pdu->s->qp_fullpath_next++;
pdu->s->qp_fullpath_next &= QPATH_INO_MASK;
+#endif
qht_insert(&pdu->s->qpf_table, val, hash, NULL);
}
@@ -660,18 +863,17 @@ static inline bool is_ro_export(FsContext *ctx)
return ctx->export_flags & V9FS_RDONLY;
}
-/*
- * Once qpp_table size exceeds this value, we no longer save
- * the table persistently. See comment in v9fs_store_qpp_table()
- */
-#define QPP_TABLE_PERSISTENCY_LIMIT 32768
+#if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
-/* Remove all user.virtfs.system.qidp.* xattrs from export root. */
-static void remove_qidp_xattr(FsContext *ctx)
+/* Remove all xattrs from export root matching the supplied xattr name_pattern. */
+static void remove_seq_xattr(FsContext *ctx, const char* name_pattern)
{
V9fsString name;
int i;
+ if (is_ro_export(ctx))
+ return;
+
/* just for a paranoid endless recursion sanity check */
const ssize_t max_size =
sizeof(QppSrlzHeader) +
@@ -679,83 +881,254 @@ static void remove_qidp_xattr(FsContext *ctx)
v9fs_string_init(&name);
for (i = 0; i * ATTR_MAX_SIZE < max_size; ++i) {
- v9fs_string_sprintf(&name, "user.virtfs.system.qidp.%d", i);
+ v9fs_string_sprintf(&name, name_pattern, i);
if (lremovexattr(ctx->fs_root, name.data) < 0)
break;
}
v9fs_string_free(&name);
}
+# if P9_VARI_LENGTH_INODE_SUFFIXES
+
+/* Remove all user.virtfs.system.qidd.* xattrs from export root. */
+static void remove_qidd_xattr(FsContext *ctx)
+{
+ remove_seq_xattr(ctx, "user.virtfs.system.qidd.%d");
+}
+
+# endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
+/* Remove all user.virtfs.system.qidp.* xattrs from export root. */
+static void remove_qidp_xattr(FsContext *ctx)
+{
+ remove_seq_xattr(ctx, "user.virtfs.system.qidp.%d");
+}
+
+static void remove_qp_tables_xattrs(FsContext *ctx) {
+ #if P9_VARI_LENGTH_INODE_SUFFIXES
+ remove_qidd_xattr(ctx);
+ #endif
+ remove_qidp_xattr(ctx);
+}
+
+# if P9_VARI_LENGTH_INODE_SUFFIXES
+
+/* Used to convert qpd hash table into continuous stream. */
+static void qpd_table_serialize(struct qht *ht, void *p, uint32_t h, void *up)
+{
+ const QpdEntry *entry = (const QpdEntry*) p;
+ QpdSerialize *ser = (QpdSerialize*) up;
+
+ /* NOTE: Due to the simple incremental index solution we use here, this
+ * means the entries might be serialized in the final data stream with
+ * different order than the devices were originally inserted into the
+ * qpd_table. However specifically for qpd_table this is irrelevant ATM.
+ */
+ const uint32_t index = ser->done;
+
+ if (ser->error)
+ return;
+
+ /* safety first */
+ if (index >= ser->count) {
+ ser->error = -1;
+ return;
+ }
+
+ ser->elements[index] = (QpdEntryS) {
+ .dev = entry->dev,
+ .prefix_bits = entry->prefix_bits
+ };
+ ser->done++;
+}
+
+# endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
/* Used to convert qpp hash table into continuous stream. */
static void qpp_table_serialize(void *p, uint32_t h, void *up)
{
const QppEntry *entry = (const QppEntry*) p;
QppSerialize *ser = (QppSerialize*) up;
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ const uint32_t index = entry->qp_affix_index - 1;
+#else
+ const uint32_t index = entry->qp_affix - 1;
+#endif
if (ser->error)
return;
/* safety first */
- if (entry->qp_prefix - 1 >= ser->count) {
+ if (index >= ser->count) {
ser->error = -1;
return;
}
- ser->elements[entry->qp_prefix - 1] = (QppEntryS) {
+ ser->elements[index] = (QppEntryS) {
.dev = entry->dev,
.ino_prefix = entry->ino_prefix
};
ser->done++;
}
+/** @brief Returns true if we should still save qpp_table (and qpd_table).
+ *
+ * Whenever we exceeded some certain (arbitrary) high qpp_table size we
+ * delete the stored table(s) from the file system to get rid of old device
+ * ids / inodes that might no longer exist with the goal to potentially
+ * yield in a smaller table size after next reboot and smaller inode
+ * numbers on guest.
+ */
+static bool shouldStoreQpTables(V9fsState *s) {
+ return s->qp_affix_next && s->qp_affix_next < QPP_TABLE_PERSISTENCY_LIMIT;
+}
+
+/* See shouldStoreQpTables() above */
+static bool shouldDeleteQpTables(V9fsState *s) {
+ return s->qp_affix_next == QPP_TABLE_PERSISTENCY_LIMIT;
+}
+
+# if P9_VARI_LENGTH_INODE_SUFFIXES
+
+/*
+ * Tries to store the current qpd_table as extended attribute(s) on the
+ * exported file system root with the goal to preserve identical qids
+ * beyond the scope of reboots / suspends.
+ *
+ * @returns 0 on success
+ */
+static int v9fs_store_qpd_table(V9fsState *s)
+{
+ FsContext *ctx = &s->ctx;
+ V9fsString name;
+ int i, res, err = 0;
+ size_t size;
+ QpdSrlzStream* stream;
+ QpdSerialize ser;
+
+ if (is_ro_export(ctx))
+ return 0;
+
+ if (!shouldStoreQpTables(s)) {
+ if (shouldDeleteQpTables(s)) {
+ remove_qp_tables_xattrs(ctx);
+ }
+ return 0;
+ }
+
+ /* Convert qpd hash table into continuous array. */
+ size = sizeof(QppSrlzHeader) +
+ ( s->qp_ndevices /* qpd_table entry count */ * sizeof(QpdEntryS) );
+ stream = g_malloc0(size);
+ ser = (QpdSerialize) {
+ .elements = &stream->elements[0],
+ .count = s->qp_ndevices,
+ .done = 0,
+ .error = 0,
+ };
+ qht_iter(&s->qpd_table, qpd_table_serialize, &ser);
+ if (ser.error || ser.done != ser.count) {
+ err = -1;
+ goto out;
+ }
+
+ /* initialize header and calculate CRC32 checksum */
+ stream->header = (QppSrlzHeader) {
+ .version = 1,
+ .options = 0, /* not used for storing the qpd_table yet */
+ .crc32 = crc32c(
+ 0xffffffff,
+ (const uint8_t*) &stream->elements[0],
+ (ser.count * sizeof(QpdEntryS))
+ ),
+ };
+
+ /*
+ * Actually just required if the qpd_table size decreased, or if the
+ * previous xattr size limit increased on OS (kernel/fs) level.
+ */
+ remove_qidd_xattr(ctx);
+
+ /*
+ * Subdivide (if required) the data stream into individual xattrs
+ * to cope with the system's max. supported xattr value size.
+ */
+ v9fs_string_init(&name);
+ for (i = 0; size > (i * ATTR_MAX_SIZE); ++i) {
+ v9fs_string_sprintf(&name, "user.virtfs.system.qidd.%d", i);
+ res = lsetxattr(
+ ctx->fs_root,
+ name.data,
+ ((const uint8_t*)stream) + i * ATTR_MAX_SIZE,
+ MIN(ATTR_MAX_SIZE, size - i * ATTR_MAX_SIZE),
+ 0/*flags*/
+ );
+ if (res < 0) {
+ if (i > 0)
+ remove_qidd_xattr(ctx);
+ err = -1;
+ break;
+ }
+ }
+ v9fs_string_free(&name);
+out:
+ g_free(stream);
+
+ return err;
+}
+
+# endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
/*
* Tries to store the current qpp_table as extended attribute(s) on the
* exported file system root with the goal to preserve identical qids
- * beyond the scope of reboots.
+ * beyond the scope of reboots / suspends.
+ *
+ * @returns 0 on success
*/
-static void v9fs_store_qpp_table(V9fsState *s)
+static int v9fs_store_qpp_table(V9fsState *s)
{
FsContext *ctx = &s->ctx;
V9fsString name;
- int i, res;
+ int i, res, err = 0;
size_t size;
QppSrlzStream* stream;
QppSerialize ser;
if (is_ro_export(ctx))
- return;
+ return 0;
- /*
- * Whenever we exceeded some certain (arbitrary) high qpp_table size we
- * delete the stored table from the file system to get rid of old device
- * ids / inodes that might no longer exist with the goal to potentially
- * yield in a smaller table size after next reboot.
- */
- if (!s->qp_prefix_next || s->qp_prefix_next >= QPP_TABLE_PERSISTENCY_LIMIT) {
- if (s->qp_prefix_next == QPP_TABLE_PERSISTENCY_LIMIT) {
- remove_qidp_xattr(ctx);
+ if (!shouldStoreQpTables(s)) {
+ if (shouldDeleteQpTables(s)) {
+ remove_qp_tables_xattrs(ctx);
}
- return;
+ return 0;
}
/* Convert qpp hash table into continuous array. */
size = sizeof(QppSrlzHeader) +
- ( (s->qp_prefix_next - 1) /* qpp_table entry count */ * sizeof(QppEntryS) );
+ ( (s->qp_affix_next - 1) /* qpp_table entry count */ * sizeof(QppEntryS) );
stream = g_malloc0(size);
ser = (QppSerialize) {
.elements = &stream->elements[0],
- .count = s->qp_prefix_next - 1,
+ .count = s->qp_affix_next - 1,
.done = 0,
.error = 0,
};
qht_iter(&s->qpp_table, qpp_table_serialize, &ser);
- if (ser.error || ser.done != ser.count)
+ if (ser.error || ser.done != ser.count) {
+ err = -1;
goto out;
+ }
/* initialize header and calculate CRC32 checksum */
stream->header = (QppSrlzHeader) {
.version = 1,
- .reserved = 0,
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ .options = EXP_GOLOMB_K + 1,
+#else
+ .options = 0,
+#endif
.crc32 = crc32c(
0xffffffff,
(const uint8_t*) &stream->elements[0],
@@ -786,12 +1159,15 @@ static void v9fs_store_qpp_table(V9fsState *s)
if (res < 0) {
if (i > 0)
remove_qidp_xattr(ctx);
+ err = -1;
break;
}
}
v9fs_string_free(&name);
out:
g_free(stream);
+
+ return err;
}
/* Frees the entire chain of passed nodes from memory. */
@@ -809,6 +1185,55 @@ static void destroy_xattr_nodes(XAttrNode **first)
}
}
+# if P9_VARI_LENGTH_INODE_SUFFIXES
+
+/*
+ * Loads all user.virtfs.system.qidd.* xattrs from exported fs root and
+ * returns a linked list with one node per xattr.
+ */
+static XAttrNode* v9fs_load_qidd_xattr_nodes(V9fsState *s)
+{
+ FsContext *ctx = &s->ctx;
+ XAttrNode *first = NULL, *current = NULL;
+ V9fsString name;
+ ssize_t size;
+ int i;
+
+ const ssize_t max_size =
+ sizeof(QppSrlzHeader) +
+ QPP_TABLE_PERSISTENCY_LIMIT * sizeof(QpdEntryS);
+
+ v9fs_string_init(&name);
+
+ for (i = 0; i * ATTR_MAX_SIZE < max_size; ++i) {
+ v9fs_string_sprintf(&name, "user.virtfs.system.qidd.%d", i);
+ size = lgetxattr(ctx->fs_root, name.data, NULL, 0);
+ if (size <= 0)
+ break;
+ if (!first) {
+ first = current = g_malloc0(sizeof(XAttrNode));
+ } else {
+ current = current->next = g_malloc0(sizeof(XAttrNode));
+ }
+ current->value = g_malloc0(size);
+ current->length = lgetxattr(
+ ctx->fs_root, name.data, current->value, size
+ );
+ if (current->length <= 0) {
+ goto out_w_err;
+ }
+ }
+ goto out;
+
+out_w_err:
+ destroy_xattr_nodes(&first);
+out:
+ v9fs_string_free(&name);
+ return first;
+}
+
+# endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
/*
* Loads all user.virtfs.system.qidp.* xattrs from exported fs root and
* returns a linked list with one node per xattr.
@@ -854,28 +1279,121 @@ out:
return first;
}
+# if P9_VARI_LENGTH_INODE_SUFFIXES
+
+/*
+ * Try to load previously stored qpd_table from file system. Calling this
+ * function assumes that qpd_table is yet empty.
+ *
+ * @returns number of entries loaded on success or negative number on error
+ * @see v9fs_store_qpd_table()
+ */
+static int v9fs_load_qpd_table(V9fsState *s)
+{
+ ssize_t size, count = 0;
+ XAttrNode *current, *first;
+ QpdSrlzStream* stream = NULL;
+ uint32_t crc32;
+ int i, err = 0;
+ QpdEntry *val;
+ uint32_t hash;
+
+ if (s->qp_ndevices != 0)
+ return -1;
+
+ first = v9fs_load_qidd_xattr_nodes(s);
+ if (!first)
+ return -1;
+
+ /* convert nodes into continuous stream */
+ size = 0;
+ for (current = first; current; current = current->next) {
+ size += current->length;
+ }
+ if (size <= 0) {
+ err = -1;
+ goto out;
+ }
+ stream = g_malloc0(size);
+ size = 0;
+ for (current = first; current; current = current->next) {
+ memcpy(((uint8_t*)stream) + size, current->value, current->length);
+ size += current->length;
+ }
+
+ if (stream->header.version != 1) {
+ err = -1;
+ goto out;
+ }
+
+ count = (size - sizeof(QppSrlzHeader)) / sizeof(QpdEntryS);
+ if (count <= 0) {
+ err = -1;
+ goto out;
+ }
+
+ /* verify CRC32 checksum of stream */
+ crc32 = crc32c(
+ 0xffffffff,
+ (const uint8_t*) &stream->elements[0],
+ (count * sizeof(QpdEntryS))
+ );
+ if (crc32 != stream->header.crc32) {
+ err = -1;
+ goto out;
+ }
+
+ /* fill qpd_table with the retrieved elements */
+ for (i = 0; i < count; ++i) {
+ val = g_malloc0(sizeof(QpdEntry));
+ *val = (QpdEntry) {
+ .dev = stream->elements[i].dev,
+ .prefix_bits = stream->elements[i].prefix_bits,
+ };
+ hash = val->dev;
+ if (qht_lookup(&s->qpd_table, val, hash)) {
+ /* should never happen: duplicate entry detected */
+ g_free(val);
+ err = -1;
+ goto out;
+ }
+ qht_insert(&s->qpd_table, val, hash, NULL);
+ s->qp_ndevices++;
+ }
+
+out:
+ destroy_xattr_nodes(&first);
+ if (stream)
+ g_free(stream);
+
+ return err ? err : count;
+}
+
+# endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
/*
* Try to load previously stored qpp_table from file system. Calling this
* function assumes that qpp_table is yet empty.
*
+ * @returns number of entries loaded on success or negative number on error
* @see v9fs_store_qpp_table()
*/
-static void v9fs_load_qpp_table(V9fsState *s)
+static int v9fs_load_qpp_table(V9fsState *s)
{
- ssize_t size, count;
+ ssize_t size, count = 0;
XAttrNode *current, *first;
QppSrlzStream* stream = NULL;
uint32_t crc32;
- int i;
+ int i, err = 0;
QppEntry *val;
uint32_t hash;
- if (s->qp_prefix_next != 1)
- return;
+ if (s->qp_affix_next != 1)
+ return -1;
first = v9fs_load_qidp_xattr_nodes(s);
if (!first)
- return;
+ return -1;
/* convert nodes into continuous stream */
size = 0;
@@ -883,6 +1401,7 @@ static void v9fs_load_qpp_table(V9fsState *s)
size += current->length;
}
if (size <= 0) {
+ err = -1;
goto out;
}
stream = g_malloc0(size);
@@ -893,11 +1412,24 @@ static void v9fs_load_qpp_table(V9fsState *s)
}
if (stream->header.version != 1) {
+ err = -1;
+ goto out;
+ }
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ if ((stream->header.options - 1) != EXP_GOLOMB_K) {
+ err = -1;
goto out;
}
+#else
+ if (stream->header.options != 0) {
+ err = -1;
+ goto out;
+ }
+#endif
count = (size - sizeof(QppSrlzHeader)) / sizeof(QppEntryS);
if (count <= 0) {
+ err = -1;
goto out;
}
@@ -908,6 +1440,7 @@ static void v9fs_load_qpp_table(V9fsState *s)
(count * sizeof(QppEntryS))
);
if (crc32 != stream->header.crc32) {
+ err = -1;
goto out;
}
@@ -922,9 +1455,15 @@ static void v9fs_load_qpp_table(V9fsState *s)
if (qht_lookup(&s->qpp_table, val, hash)) {
/* should never happen: duplicate entry detected */
g_free(val);
+ err = -1;
goto out;
}
- val->qp_prefix = s->qp_prefix_next++;
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ val->qp_affix_index = s->qp_affix_next++;
+ val->qp_affix = affixForIndex(val->qp_affix_index);
+#else
+ val->qp_affix = s->qp_affix_next++;
+#endif
qht_insert(&s->qpp_table, val, hash, NULL);
}
@@ -932,40 +1471,109 @@ out:
destroy_xattr_nodes(&first);
if (stream)
g_free(stream);
+
+ return err ? err : count;
+}
+
+/* Tries to load qpp_table (and qpd_table) from xattrs being saved on export root. */
+static void v9fs_load_qp_tables(V9fsState *s)
+{
+ int res;
+
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ res = v9fs_load_qpd_table(s);
+ if (res > 0) {
+ res = v9fs_load_qpp_table(s);
+ }
+#else
+ res = v9fs_load_qpp_table(s);
+#endif
+
+ /* if there was any error, remove the xattrs */
+ if (res < 0) {
+ remove_qp_tables_xattrs(&s->ctx);
+ }
}
-/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
+/* Tries to store qpp_table (and qpd_table) as xattrs on export root. */
+static void v9fs_store_qp_tables(V9fsState *s)
+{
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ int res;
+
+ res = v9fs_store_qpd_table(s);
+ if (res >= 0) {
+ res = v9fs_store_qpp_table(s);
+ }
+
+ /* if there was any error, remove the xattrs immediately again */
+ if (res < 0) {
+ remove_qp_tables_xattrs(&s->ctx);
+ }
+#else
+ v9fs_store_qpp_table(s);
+#endif
+}
+
+#endif /* (QPP_TABLE_PERSISTENCY_LIMIT > 0) */
+
+/** @brief Quick mapping host inode nr -> guest inode nr.
+ *
+ * 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).
+ *
+ * if P9_VARI_LENGTH_INODE_SUFFIXES == 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.
*
- * 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.
+ * if P9_VARI_LENGTH_INODE_SUFFIXES == 1 :
+ * Instead of fixed size (16 bit) generated prefix, we use variable size
+ * suffixes instead. See comment on P9_VARI_LENGTH_INODE_SUFFIXES for
+ * details.
*/
static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
uint64_t *path)
{
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ const int ino_hash_bits = qid_inode_prefix_hash_bits(pdu, stbuf->st_dev);
+#endif
QppEntry lookup = {
.dev = stbuf->st_dev,
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ .ino_prefix = (uint16_t) (stbuf->st_ino >> (64-ino_hash_bits))
+#else
.ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
+#endif
}, *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 */
return -ENFILE;
}
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 */
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ val->qp_affix_index = pdu->s->qp_affix_next++;
+ val->qp_affix = affixForIndex(val->qp_affix_index);
+#else
+ val->qp_affix = pdu->s->qp_affix_next++;
+#endif
qht_insert(&pdu->s->qpp_table, val, hash, NULL);
/*
@@ -974,10 +1582,16 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
* TODO: This should better only be called from a guest shutdown and
* suspend handler.
*/
+ #if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
v9fs_store_qpp_table(pdu->s);
+ #endif
}
-
- *path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ /* assuming generated affix to be suffix type, not prefix */
+ *path = (stbuf->st_ino << val->qp_affix.bits) | val->qp_affix.value;
+#else
+ *path = ((uint64_t)val->qp_affix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
+#endif
return 0;
}
@@ -988,7 +1602,7 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
/* map inode+device to qid path (fast path) */
err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
if (err == -ENFILE) {
- /* fast path didn't work, fal back to full map */
+ /* fast path didn't work, fall back to full map */
err = qid_path_fullmap(pdu, stbuf, &qidp->path);
}
if (err) {
@@ -4075,12 +4689,22 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
goto out;
}
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ qht_init(&s->qpd_table, qpd_cmp_func, 1, QHT_MODE_AUTO_RESIZE);
+#endif
+ /* most users won't need the fullmap, so init the table lazily */
+ qht_init(&s->qpf_table, qpf_cmp_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
/* QID path hash table. 1 entry ought to be enough for anybody ;) */
qht_init(&s->qpp_table, qpp_cmp_func, 1, QHT_MODE_AUTO_RESIZE);
- s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ s->qp_ndevices = 0;
+#endif
+ s->qp_affix_next = 1; /* reserve 0 to detect overflow */
s->qp_fullpath_next = 1;
- /* try to load and restore previous qpp_table */
- v9fs_load_qpp_table(s);
+ /* try to load and restore previous qpd_table and qpp_table */
+#if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
+ v9fs_load_qp_tables(s);
+#endif
s->ctx.fst = &fse->fst;
fsdev_throttle_init(s->ctx.fst);
@@ -4095,6 +4719,9 @@ out:
}
g_free(s->tag);
g_free(s->ctx.fs_root);
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ qp_table_destroy(&s->qpd_table);
+#endif
qp_table_destroy(&s->qpp_table);
qp_table_destroy(&s->qpf_table);
v9fs_path_free(&path);
@@ -4105,18 +4732,23 @@ out:
void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
{
/*
- * Store qpp_table as extended attribute(s) to file system.
+ * Store qpd_table and qpp_table as extended attribute(s) to file system.
*
* This was actually plan A, but unfortunately unserialize is not called
* reliably on guest shutdowns and suspensions.
*/
- v9fs_store_qpp_table(s);
+#if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
+ v9fs_store_qp_tables(s);
+#endif
if (s->ops->cleanup) {
s->ops->cleanup(&s->ctx);
}
fsdev_throttle_cleanup(s->ctx.fst);
g_free(s->tag);
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ qp_table_destroy(&s->qpd_table);
+#endif
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 54ce039969..78c29fb916 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -236,15 +236,95 @@ struct V9fsFidState
V9fsFidState *rclm_lst;
};
-#define QPATH_INO_MASK (((unsigned long)1 << 48) - 1)
+/*
+ * Defines how inode numbers from host shall be remapped on guest.
+ *
+ * When this compile time option is disabled then fixed length (16 bit)
+ * prefix values are used for all inode numbers on guest level. Accordingly
+ * guest's inode numbers will be quite large (>2^48).
+ *
+ * If this option is enabled then variable length suffixes will be used for
+ * guest's inode numbers instead which usually yields in much smaller inode
+ * numbers on guest level (typically around >2^1 .. >2^7).
+ */
+#define P9_VARI_LENGTH_INODE_SUFFIXES 1
+
+/*
+ * If this value is higher than 0 then file inode remapping is tried to be
+ * retained consistent beyond reboots/suspends.
+ *
+ * Once qpp_table size exceeds this value, we no longer save
+ * the table persistently. See comment in v9fs_store_qpp_table()
+ *
+ * By setting this to 0, persistency of file ids will (beyond the scope of
+ * reboots/suspends) will be disabled completely at compile time.
+ */
+#define QPP_TABLE_PERSISTENCY_LIMIT 0
+
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+
+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. */
+ int bits; /* 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. */
+} VariLenAffix;
+
+#endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
+#if !P9_VARI_LENGTH_INODE_SUFFIXES
+# define QPATH_INO_MASK (((unsigned long)1 << 48) - 1)
+#endif
+
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+
+/* See qid_inode_prefix_hash_bits(). */
+typedef struct {
+ dev_t dev; /* FS device on host. */
+ int prefix_bits; /* How many (high) bits of the original inode number shall be used for hashing. */
+} QpdEntry;
+
+# if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
+
+/* Small version of QpdEntry for serialization as xattr. */
+struct QpdEntryS {
+ dev_t dev;
+ uint8_t prefix_bits;
+} __attribute__((packed));
+typedef struct QpdEntryS QpdEntryS;
+
+# endif /* (QPP_TABLE_PERSISTENCY_LIMIT > 0) */
+
+#endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
/* QID path prefix entry, see stat_to_qid */
typedef struct {
dev_t dev;
uint16_t ino_prefix;
- uint16_t qp_prefix;
+ #if P9_VARI_LENGTH_INODE_SUFFIXES
+ uint32_t qp_affix_index;
+ VariLenAffix qp_affix;
+ #else
+ uint16_t qp_affix;
+ #endif
} QppEntry;
+#if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
+
/* Small version of QppEntry for serialization as xattr. */
struct QppEntryS {
dev_t dev;
@@ -252,6 +332,8 @@ struct QppEntryS {
} __attribute__((packed));
typedef struct QppEntryS QppEntryS;
+#endif /* (QPP_TABLE_PERSISTENCY_LIMIT > 0) */
+
/* QID path full entry, as above */
typedef struct {
dev_t dev;
@@ -259,6 +341,19 @@ typedef struct {
uint64_t path;
} QpfEntry;
+#if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
+
+# if P9_VARI_LENGTH_INODE_SUFFIXES
+
+typedef struct {
+ QpdEntryS *elements;
+ uint count; /* In: QpdEntryS count in @a elements */
+ uint done; /* Out: how many QpdEntryS did we actually fill in @a elements */
+ int error; /* Out: zero on success */
+} QpdSerialize;
+
+# endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
typedef struct {
QppEntryS *elements;
uint count; /* In: QppEntryS count in @a elements */
@@ -268,7 +363,7 @@ typedef struct {
struct QppSrlzHeader {
uint16_t version;
- uint16_t reserved; /* might be used e.g. for flags in future */
+ uint16_t options;
uint32_t crc32;
} __attribute__((packed));
typedef struct QppSrlzHeader QppSrlzHeader;
@@ -279,12 +374,24 @@ struct QppSrlzStream {
} __attribute__((packed));
typedef struct QppSrlzStream QppSrlzStream;
+# if P9_VARI_LENGTH_INODE_SUFFIXES
+
+struct QpdSrlzStream {
+ QppSrlzHeader header;
+ QpdEntryS elements[0];
+} __attribute__((packed));
+typedef struct QpdSrlzStream QpdSrlzStream;
+
+# endif /* P9_VARI_LENGTH_INODE_SUFFIXES */
+
typedef struct XAttrNode {
uint8_t* value;
ssize_t length;
struct XAttrNode* next;
} XAttrNode;
+#endif /* (QPP_TABLE_PERSISTENCY_LIMIT > 0) */
+
struct V9fsState
{
QLIST_HEAD(, V9fsPDU) free_list;
@@ -306,9 +413,15 @@ struct V9fsState
Error *migration_blocker;
V9fsConf fsconf;
V9fsQID root_qid;
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ struct qht qpd_table;
+#endif
struct qht qpp_table;
struct qht qpf_table;
- uint16_t qp_prefix_next;
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+ uint64_t qp_ndevices; /* Amount of entries in qpd_table. */
+#endif
+ uint16_t qp_affix_next;
uint64_t qp_fullpath_next;
};
--
2.11.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter
2019-05-06 14:37 [Qemu-devel] [PATCH v3 0/5] 9p: Fix file ID collisions Christian Schoenebeck via Qemu-devel
` (3 preceding siblings ...)
2019-05-03 14:51 ` [Qemu-devel] [PATCH v3 4/5] 9p: use variable length suffixes for inode mapping Christian Schoenebeck via Qemu-devel
@ 2019-05-05 21:41 ` Christian Schoenebeck via Qemu-devel
2019-05-06 17:58 ` [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii' Christian Schoenebeck via Qemu-devel
4 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-05 21:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
This patch adds an optional qemu device parameter "vii" (very important
inode[s]), which accepts a colon separated list of pathes relative below
the 9p export root, which shall be given the smallest inode suffix (and
hence largest possible inode namespace on guest) according to the given
order. That way host admins do have a way to fine tune the automatic inode
remapping algorithm of 9pfs.
Example: consider a file server running on guest with 9p export root /vm/fs
on host and the mass storage file system being mounted on host on
/vm/fs/var/shares and further consider some very frequently accessed temp
file system being mounted as /vm/fs/tmp on host, then host admin could give
the mass storage file system the highest inode namespace and the temp file
system the 2nd highest inode namespace with qemu virtfs argument:
vii=/var/shares:/tmp
The root file system (/vm/fs on host, / on guest) would then hence get the
3rd highest inode namespace on guest automatically (since not mentioned in
vii list).
For completeness I will also send a (6th) patch against libvirt which
allows to conigure this feature via libvirt/virsh xml config instead.
Additionally this patch addresses:
* Removes unnecessary header includes in 9p.c
(that I used for debugging).
* Fixes a compiler error of previous patch in 9p.c if macro
QPP_TABLE_PERSISTENCY_LIMIT was enabled and latest git master head
version used.
This patch probably needs some cleanup. But I will wait for your coarse
feedback before doing anything else. For instance the device property that
I added in virtio-9p-device.c is actually not used ATM. My initial
assumption was this to be used by the qemu options visitors automatically
for command line arguments, then I noticed that these properties are rather
intended for passing parameters from guest instead. I am unresolved though
whether this parameter shall be configurable from guest or not, so I have
not finished that yet. Then the user manual changes need to be finished and
obviously you might have a better idea for an argument name than "vii" ;-)
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
fsdev/file-op-9p.h | 1 +
fsdev/qemu-fsdev-opts.c | 6 ++
fsdev/qemu-fsdev.c | 3 +
hw/9pfs/9p.c | 138 +++++++++++++++++++++++++++++++--------------
hw/9pfs/9p.h | 6 ++
hw/9pfs/virtio-9p-device.c | 1 +
qemu-options.hx | 5 +-
vl.c | 9 ++-
8 files changed, 124 insertions(+), 45 deletions(-)
diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 3fa062b39f..2645faa113 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -77,6 +77,7 @@ typedef struct FsDriverEntry {
FsThrottle fst;
mode_t fmode;
mode_t dmode;
+ char *vii;
} FsDriverEntry;
struct FsContext {
diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
index 7c31ffffaf..5ae02887a5 100644
--- a/fsdev/qemu-fsdev-opts.c
+++ b/fsdev/qemu-fsdev-opts.c
@@ -44,6 +44,9 @@ static QemuOptsList qemu_fsdev_opts = {
}, {
.name = "dmode",
.type = QEMU_OPT_NUMBER,
+ }, {
+ .name = "vii",
+ .type = QEMU_OPT_STRING,
},
THROTTLE_OPTS,
@@ -87,6 +90,9 @@ static QemuOptsList qemu_virtfs_opts = {
}, {
.name = "dmode",
.type = QEMU_OPT_NUMBER,
+ }, {
+ .name = "vii",
+ .type = QEMU_OPT_STRING,
},
{ /*End of list */ }
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index 54cb36a212..29133b4a15 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -34,6 +34,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 *vii = qemu_opt_get(opts, "vii");
bool ro = qemu_opt_get_bool(opts, "readonly", 0);
if (!fsdev_id) {
@@ -59,6 +60,7 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
fsle = g_malloc0(sizeof(*fsle));
fsle->fse.fsdev_id = g_strdup(fsdev_id);
+ fsle->fse.vii = g_strdup(vii);
fsle->fse.ops = FsDrivers[i].ops;
if (writeout) {
if (!strcmp(writeout, "immediate")) {
@@ -74,6 +76,7 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
if (fsle->fse.ops->parse_opts) {
if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) {
g_free(fsle->fse.fsdev_id);
+ g_free(fsle->fse.vii);
g_free(fsle);
return -1;
}
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 003901a1e0..b4c4c66a3b 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -30,10 +30,8 @@
#if defined(__linux__) /* TODO: This should probably go into osdep.h instead */
# include <linux/limits.h> /* for XATTR_SIZE_MAX */
#endif
-#include <sys/types.h>
-#include <unistd.h>
-#include <sys/syscall.h>
#include <math.h>
+#include "qemu/cutils.h"
/*
* How many bytes may we store to fs per extended attribute value?
@@ -589,6 +587,8 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
P9_STAT_MODE_NAMED_PIPE | \
P9_STAT_MODE_SOCKET)
+#if P9_VARI_LENGTH_INODE_SUFFIXES
+
/* 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;
@@ -606,8 +606,6 @@ static inline uint64_t mirror64bit(uint64_t value) {
((uint64_t)mirror8bit((value >> 56) & 0xff) ) ;
}
-#if P9_VARI_LENGTH_INODE_SUFFIXES
-
/* Parameter k for the Exponential Golomb algorihm to be used.
*
* The smaller this value, the smaller the minimum bit count for the Exp.
@@ -770,7 +768,7 @@ static int v9fs_store_qpd_table(V9fsState *s);
* 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)
+static int qid_inode_prefix_hash_bits(V9fsState *s, dev_t dev)
{
QpdEntry lookup = {
.dev = dev
@@ -778,14 +776,14 @@ static int qid_inode_prefix_hash_bits(V9fsPDU *pdu, dev_t dev)
uint32_t hash = dev;
VariLenAffix affix;
- val = qht_lookup(&pdu->s->qpd_table, &lookup, hash);
+ val = qht_lookup(&s->qpd_table, &lookup, hash);
if (!val) {
val = g_malloc0(sizeof(QpdEntry));
*val = lookup;
- affix = affixForIndex(pdu->s->qp_affix_next);
+ affix = affixForIndex(s->qp_affix_next);
val->prefix_bits = affix.bits;
- qht_insert(&pdu->s->qpd_table, val, hash, NULL);
- pdu->s->qp_ndevices++;
+ qht_insert(&s->qpd_table, val, hash, NULL);
+ s->qp_ndevices++;
/*
* Store qpd_table as extended attribute(s) to file system.
@@ -794,7 +792,7 @@ static int qid_inode_prefix_hash_bits(V9fsPDU *pdu, dev_t dev)
* suspend handler.
*/
#if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
- v9fs_store_qpd_table(pdu->s);
+ v9fs_store_qpd_table(s);
#endif
}
return val->prefix_bits;
@@ -817,7 +815,7 @@ static int qid_inode_prefix_hash_bits(V9fsPDU *pdu, dev_t dev)
* @see qid_path_prefixmap() for details
*
*/
-static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
+static int qid_path_fullmap(V9fsState *s, const struct stat *stbuf,
uint64_t *path)
{
QpfEntry lookup = {
@@ -829,10 +827,10 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
VariLenAffix affix;
#endif
- val = qht_lookup(&pdu->s->qpf_table, &lookup, hash);
+ val = qht_lookup(&s->qpf_table, &lookup, hash);
if (!val) {
- if (pdu->s->qp_fullpath_next == 0) {
+ if (s->qp_fullpath_next == 0) {
/* no more files can be mapped :'( */
return -ENFILE;
}
@@ -843,15 +841,15 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
/* new unique inode and device combo */
#if P9_VARI_LENGTH_INODE_SUFFIXES
affix = affixForIndex(
- 1ULL << (sizeof(pdu->s->qp_affix_next) * 8)
+ 1ULL << (sizeof(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);
+ val->path = (s->qp_fullpath_next++ << affix.bits) | affix.value;
+ s->qp_fullpath_next &= ((1ULL << (64 - affix.bits)) - 1);
#else
- val->path = pdu->s->qp_fullpath_next++;
- pdu->s->qp_fullpath_next &= QPATH_INO_MASK;
+ val->path = s->qp_fullpath_next++;
+ s->qp_fullpath_next &= QPATH_INO_MASK;
#endif
- qht_insert(&pdu->s->qpf_table, val, hash, NULL);
+ qht_insert(&s->qpf_table, val, hash, NULL);
}
*path = val->path;
@@ -914,7 +912,7 @@ static void remove_qp_tables_xattrs(FsContext *ctx) {
# if P9_VARI_LENGTH_INODE_SUFFIXES
/* Used to convert qpd hash table into continuous stream. */
-static void qpd_table_serialize(struct qht *ht, void *p, uint32_t h, void *up)
+static void qpd_table_serialize(void *p, uint32_t h, void *up)
{
const QpdEntry *entry = (const QpdEntry*) p;
QpdSerialize *ser = (QpdSerialize*) up;
@@ -1540,11 +1538,11 @@ static void v9fs_store_qp_tables(V9fsState *s)
* suffixes instead. See comment on P9_VARI_LENGTH_INODE_SUFFIXES for
* details.
*/
-static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
+static int qid_path_prefixmap(V9fsState *s, const struct stat *stbuf,
uint64_t *path)
{
#if P9_VARI_LENGTH_INODE_SUFFIXES
- const int ino_hash_bits = qid_inode_prefix_hash_bits(pdu, stbuf->st_dev);
+ const int ino_hash_bits = qid_inode_prefix_hash_bits(s, stbuf->st_dev);
#endif
QppEntry lookup = {
.dev = stbuf->st_dev,
@@ -1556,10 +1554,10 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
}, *val;
uint32_t hash = qpp_hash(lookup);
- val = qht_lookup(&pdu->s->qpp_table, &lookup, hash);
+ val = qht_lookup(&s->qpp_table, &lookup, hash);
if (!val) {
- if (pdu->s->qp_affix_next == 0) {
+ if (s->qp_affix_next == 0) {
/* we ran out of affixes */
return -ENFILE;
}
@@ -1569,12 +1567,12 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
/* new unique inode affix and device combo */
#if P9_VARI_LENGTH_INODE_SUFFIXES
- val->qp_affix_index = pdu->s->qp_affix_next++;
+ val->qp_affix_index = s->qp_affix_next++;
val->qp_affix = affixForIndex(val->qp_affix_index);
#else
- val->qp_affix = pdu->s->qp_affix_next++;
+ val->qp_affix = s->qp_affix_next++;
#endif
- qht_insert(&pdu->s->qpp_table, val, hash, NULL);
+ qht_insert(&s->qpp_table, val, hash, NULL);
/*
* Store qpp_table as extended attribute(s) to file system.
@@ -1583,7 +1581,7 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
* suspend handler.
*/
#if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
- v9fs_store_qpp_table(pdu->s);
+ v9fs_store_qpp_table(s);
#endif
}
#if P9_VARI_LENGTH_INODE_SUFFIXES
@@ -1595,15 +1593,15 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
return 0;
}
-static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
+static int stat_to_qid(V9fsState *s, const struct stat *stbuf, V9fsQID *qidp)
{
int err;
/* map inode+device to qid path (fast path) */
- err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+ err = qid_path_prefixmap(s, stbuf, &qidp->path);
if (err == -ENFILE) {
/* fast path didn't work, fall back to full map */
- err = qid_path_fullmap(pdu, stbuf, &qidp->path);
+ err = qid_path_fullmap(s, stbuf, &qidp->path);
}
if (err) {
return err;
@@ -1621,6 +1619,57 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
return 0;
}
+/** @brief Handle "vii" device config parameter.
+ *
+ * "vii" (very important inode[s]) is a colon separated list of pathes
+ * passed as optional virtfs command line argument "vii" to qemu, which
+ * shall have precedence in their given order when emitting inode nr
+ * remapping suffixes on guest. So the very first vii path will get the
+ * smallest inode nr prefix (i.e. 1 bit) and hence the largest inode
+ * namespace on guest, the next path in the list will get the next higher
+ * inode prefix (i.e. 3 bit), and so on.
+ *
+ * @see qid_path_prefixmap()
+ */
+static void map_important_inodes(FsDriverEntry *fse, V9fsState *s, V9fsPath *path) {
+#if !P9_VARI_LENGTH_INODE_SUFFIXES
+ error_printf("virtfs-9p: ignoring argument vii='%s': virtfs-9p compiled without variable length inode suffixes", fse->vii);
+#else
+ struct stat stbuf;
+ char *buf, *token;
+ V9fsPath vii;
+ V9fsQID qid;
+
+ if (!fse->vii)
+ return; /* no "vii" command line argument passed to qemu */
+
+ v9fs_path_init(&vii);
+
+ buf = g_strdup(fse->vii);
+ while ((token = qemu_strsep(&buf, ":"))) {
+ if (token[0] != '/') {
+ error_printf("virtfs-9p: ignoring vii path '%s': misses mandatory leading slash", token);
+ continue;
+ }
+ /* token+1 : slash must be ripped off to avoid double slash in
+ * in vii and its subsequent assertion fault in lstat() */
+ if (s->ops->name_to_path(&s->ctx, path, token+1, &vii) < 0) {
+ error_printf("virtfs-9p: ignoring vii path '%s': error converting name to path: %s",
+ token, strerror(errno));
+ continue;
+ }
+ if (s->ops->lstat(&s->ctx, &vii, &stbuf) < 0) {
+ error_printf("virtfs-9p: ignoring vii path '%s': lstat failed", token);
+ goto next;
+ }
+ stat_to_qid(s, &stbuf, &qid);
+ next:
+ v9fs_path_free(&vii);
+ }
+ g_free(buf);
+#endif
+}
+
static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
V9fsQID *qidp)
{
@@ -1631,7 +1680,7 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
if (err < 0) {
return err;
}
- err = stat_to_qid(pdu, &stbuf, qidp);
+ err = stat_to_qid(pdu->s, &stbuf, qidp);
if (err < 0) {
return err;
}
@@ -1865,7 +1914,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
memset(v9stat, 0, sizeof(*v9stat));
- err = stat_to_qid(pdu, stbuf, &v9stat->qid);
+ err = stat_to_qid(pdu->s, stbuf, &v9stat->qid);
if (err < 0) {
return err;
}
@@ -1951,7 +2000,7 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
/* Currently we only support BASIC fields in stat */
v9lstat->st_result_mask = P9_STATS_BASIC;
- return stat_to_qid(pdu, stbuf, &v9lstat->qid);
+ return stat_to_qid(pdu->s, stbuf, &v9lstat->qid);
}
static void print_sg(struct iovec *sg, int cnt)
@@ -2416,7 +2465,7 @@ static void coroutine_fn v9fs_walk(void *opaque)
if (err < 0) {
goto out;
}
- err = stat_to_qid(pdu, &stbuf, &qid);
+ err = stat_to_qid(pdu->s, &stbuf, &qid);
if (err < 0) {
goto out;
}
@@ -2521,7 +2570,7 @@ static void coroutine_fn v9fs_open(void *opaque)
if (err < 0) {
goto out;
}
- err = stat_to_qid(pdu, &stbuf, &qid);
+ err = stat_to_qid(pdu->s, &stbuf, &qid);
if (err < 0) {
goto out;
}
@@ -2634,7 +2683,7 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
fidp->flags |= FID_NON_RECLAIMABLE;
}
iounit = get_iounit(pdu, &fidp->path);
- err = stat_to_qid(pdu, &stbuf, &qid);
+ err = stat_to_qid(pdu->s, &stbuf, &qid);
if (err < 0) {
goto out;
}
@@ -3371,7 +3420,7 @@ static void coroutine_fn v9fs_create(void *opaque)
}
}
iounit = get_iounit(pdu, &fidp->path);
- err = stat_to_qid(pdu, &stbuf, &qid);
+ err = stat_to_qid(pdu->s, &stbuf, &qid);
if (err < 0) {
goto out;
}
@@ -3431,7 +3480,7 @@ static void coroutine_fn v9fs_symlink(void *opaque)
if (err < 0) {
goto out;
}
- err = stat_to_qid(pdu, &stbuf, &qid);
+ err = stat_to_qid(pdu->s, &stbuf, &qid);
if (err < 0) {
goto out;
}
@@ -4114,7 +4163,7 @@ static void coroutine_fn v9fs_mknod(void *opaque)
if (err < 0) {
goto out;
}
- err = stat_to_qid(pdu, &stbuf, &qid);
+ err = stat_to_qid(pdu->s, &stbuf, &qid);
if (err < 0) {
goto out;
}
@@ -4275,7 +4324,7 @@ static void coroutine_fn v9fs_mkdir(void *opaque)
if (err < 0) {
goto out;
}
- err = stat_to_qid(pdu, &stbuf, &qid);
+ err = stat_to_qid(pdu->s, &stbuf, &qid);
if (err < 0) {
goto out;
}
@@ -4701,8 +4750,13 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
#endif
s->qp_affix_next = 1; /* reserve 0 to detect overflow */
s->qp_fullpath_next = 1;
+
+ /* handle "very important inodes" */
+ map_important_inodes(fse, s, &path);
+
/* try to load and restore previous qpd_table and qpp_table */
#if (QPP_TABLE_PERSISTENCY_LIMIT > 0)
+ /*TODO: call will be ignored ATM if virfs "vii" argument was passed as well */
v9fs_load_qp_tables(s);
#endif
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 78c29fb916..d78e7eff49 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -168,6 +168,12 @@ typedef struct V9fsConf
/* tag name for the device */
char *tag;
char *fsdev_id;
+ /*
+ * very important inodes: colon separated list of pathes (relative to
+ * export root) which should be preferred for getting a larger inode
+ * namespace on guest
+ */
+ char *viis;
} V9fsConf;
/* 9p2000.L xattr flags (matches Linux values) */
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 775e8ff766..cdbe157274 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -232,6 +232,7 @@ static const VMStateDescription vmstate_virtio_9p = {
static Property virtio_9p_properties[] = {
DEFINE_PROP_STRING("mount_tag", V9fsVirtioState, state.fsconf.tag),
DEFINE_PROP_STRING("fsdev", V9fsVirtioState, state.fsconf.fsdev_id),
+ DEFINE_PROP_STRING("vii", V9fsVirtioState, state.fsconf.viis),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/qemu-options.hx b/qemu-options.hx
index 08749a3391..4fd56f367e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1306,12 +1306,13 @@ 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][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n",
+ " [,id=id][,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode]\n"
+ " [,dmode=dmode][,vii=vii-path]\n",
QEMU_ARCH_ALL)
STEXI
-@item -virtfs @var{fsdriver}[,path=@var{path}],mount_tag=@var{mount_tag}[,security_model=@var{security_model}][,writeout=@var{writeout}][,readonly][,socket=@var{socket}|sock_fd=@var{sock_fd}][,fmode=@var{fmode}][,dmode=@var{dmode}]
+@item -virtfs @var{fsdriver}[,path=@var{path}],mount_tag=@var{mount_tag}[,security_model=@var{security_model}][,writeout=@var{writeout}][,readonly][,socket=@var{socket}|sock_fd=@var{sock_fd}][,fmode=@var{fmode}][,dmode=@var{dmode}][,vii=@var{vii-path}]
@findex -virtfs
The general form of a Virtual File system pass-through options are:
diff --git a/vl.c b/vl.c
index c696ad2a13..55d3f079dd 100644
--- a/vl.c
+++ b/vl.c
@@ -3447,7 +3447,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,
+ *vii;
olist = qemu_find_opts("virtfs");
if (!olist) {
@@ -3503,6 +3504,10 @@ int main(int argc, char **argv, char **envp)
if (sock_fd) {
qemu_opt_set(fsdev, "sock_fd", sock_fd, &error_abort);
}
+ vii = qemu_opt_get(opts, "vii");
+ if (vii) {
+ qemu_opt_set(fsdev, "vii", vii, &error_abort);
+ }
qemu_opt_set_bool(fsdev, "readonly",
qemu_opt_get_bool(opts, "readonly", 0),
@@ -3514,6 +3519,8 @@ int main(int argc, char **argv, char **envp)
qemu_opts_id(fsdev), &error_abort);
qemu_opt_set(device, "mount_tag",
qemu_opt_get(opts, "mount_tag"), &error_abort);
+ qemu_opt_set(device, "vii",
+ qemu_opt_get(opts, "vii"), &error_abort);
break;
}
case QEMU_OPTION_virtfs_synth: {
--
2.11.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v3 0/5] 9p: Fix file ID collisions
@ 2019-05-06 14:37 Christian Schoenebeck via Qemu-devel
2019-04-23 11:30 ` [Qemu-devel] [PATCH v3 1/5] 9p: mitigates most QID path collisions Christian Schoenebeck via Qemu-devel
` (4 more replies)
0 siblings, 5 replies; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-06 14:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
This is v3 of a proposed patch set for fixing file ID collisions with 9pfs.
That's it from my side for now regarding this overall issue. I am waiting for
your comments on this patch set before doing anything else.
Patch 1 to 4 are identical to the previous version. New patch 5 adds an
optional qemu virtfs device parameter "vii" (very important inode[s]) , which
allows host admins to configure which fs device(s) should get the largest inode
namespaces on guest.
I will also send a (6th) patch against libvirt which allows to configure the
"vii" feature of patch 5 via xml config instead of qemu command line argument.
I am yet unresolved whether or not to use persistency for file IDs that is
introduced with patch 3. After applying the entire patch set, the
persistency feature is disabled by default at compile time, but you can
enable it with macro QPP_TABLE_PERSISTENCY_LIMIT.
Christian Schoenebeck (5):
9p: mitigates most QID path collisions
9P: trivial cleanup of QID path collision mitigation
9p: persistency of QID path beyond reboots / suspensions
9p: use variable length suffixes for inode mapping
9p: adds virtfs 'vii' device parameter
fsdev/9p-marshal.h | 6 +-
fsdev/file-op-9p.h | 1 +
fsdev/qemu-fsdev-opts.c | 6 +
fsdev/qemu-fsdev.c | 3 +
hw/9pfs/9p.c | 1199 +++++++++++++++++++++++++++++++++++++++++++-
hw/9pfs/9p.h | 173 +++++++
hw/9pfs/trace-events | 14 +-
hw/9pfs/virtio-9p-device.c | 1 +
qemu-options.hx | 5 +-
vl.c | 9 +-
10 files changed, 1378 insertions(+), 39 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-05 21:41 ` [Qemu-devel] [PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter Christian Schoenebeck via Qemu-devel
@ 2019-05-06 17:58 ` Christian Schoenebeck via Qemu-devel
2019-05-07 9:55 ` Greg Kurz
2019-05-07 12:57 ` Daniel P. Berrangé
0 siblings, 2 replies; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-06 17:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
This is the counter part patch against latest libvirt git master head to
support the 'vii' feature of patch 5, which introduces the XML config
XML tag "important" on libvirt side.
To stick with the previous example mentioned with patch 5, likewise
libvirt XML configuration might then look like this:
<domain type='kvm'>
...
<devices>
...
<filesystem type='mount' accessmode='mapped'>
<source dir='/vm/fs'/>
<target dir='root'/>
<important path='/var/shares'/>
<important path='/tmp'/>
</filesystem>
</devices>
</domain>
Like with the vii qemu virtfs command line argument, the order of the
"important" tag defines which one gets the highest inode namespace
(smallest generated suffix) on guest side.
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
docs/schemas/domaincommon.rng | 6 ++++++
src/conf/domain_conf.c | 30 ++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 3 +++
src/qemu/qemu_command.c | 10 ++++++++++
4 files changed, 49 insertions(+)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 111b85c36f..c75edfc4d3 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2515,6 +2515,12 @@
</choice>
</attribute>
</optional>
+ <zeroOrMore>
+ <element name='important'>
+ <attribute name="path"/>
+ <empty/>
+ </element>
+ </zeroOrMore>
<optional>
<element name='readonly'>
<empty/>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b4fb6cf981..cc75c6a7dd 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2294,6 +2294,8 @@ virDomainFSDefNew(void)
void virDomainFSDefFree(virDomainFSDefPtr def)
{
+ size_t i;
+
if (!def)
return;
@@ -2302,6 +2304,13 @@ void virDomainFSDefFree(virDomainFSDefPtr def)
virDomainDeviceInfoClear(&def->info);
VIR_FREE(def->virtio);
+ if (def->important) {
+ for (i = 0; i < def->nimportant; i++)
+ if (def->important[i])
+ VIR_FREE(def->important[i]);
+ }
+ VIR_FREE(def->important);
+
VIR_FREE(def);
}
@@ -10953,6 +10962,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_AUTOFREE(char *) usage = NULL;
VIR_AUTOFREE(char *) units = NULL;
VIR_AUTOFREE(char *) model = NULL;
+ long n;
ctxt->node = node;
@@ -11001,6 +11011,12 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
1, ULLONG_MAX, false) < 0)
goto error;
+ n = virXMLChildElementCount(node);
+ if (n > 0) {
+ if (VIR_ALLOC_N(def->important, n) < 0)
+ goto error;
+ }
+
cur = node->children;
while (cur != NULL) {
if (cur->type == XML_ELEMENT_NODE) {
@@ -11039,6 +11055,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0)
goto error;
+ } else if (virXMLNodeNameEqual(cur, "important")) {
+ def->important[def->nimportant++] = virXMLPropString(cur, "path");
}
}
cur = cur->next;
@@ -11107,6 +11125,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
cleanup:
+ if (def && def->important && !def->nimportant)
+ VIR_FREE(def->important);
return def;
error:
@@ -24601,6 +24621,7 @@ virDomainFSDefFormat(virBufferPtr buf,
const char *src = def->src->path;
VIR_AUTOCLEAN(virBuffer) driverBuf = VIR_BUFFER_INITIALIZER;
int ret = -1;
+ size_t i;
if (!type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -24689,6 +24710,15 @@ virDomainFSDefFormat(virBufferPtr buf,
if (def->readonly)
virBufferAddLit(buf, "<readonly/>\n");
+ if (def->important) {
+ for (i = 0; i < def->nimportant; ++i) {
+ if (!def->important[i]) continue;
+ virBufferAddLit(buf, "<important");
+ virBufferEscapeString(buf, " path='%s'", def->important[i]);
+ virBufferAddLit(buf, "/>\n");
+ }
+ }
+
if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
goto cleanup;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 01c22d8cc3..9bbd66d932 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -818,6 +818,9 @@ struct _virDomainFSDef {
unsigned long long space_soft_limit; /* in bytes */
bool symlinksResolved;
virDomainVirtioOptionsPtr virtio;
+
+ size_t nimportant;
+ char **important;
};
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 50b4205267..2005ccadf8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2732,6 +2732,7 @@ qemuBuildFSStr(virDomainFSDefPtr fs)
virBuffer opt = VIR_BUFFER_INITIALIZER;
const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver);
const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy);
+ size_t i;
if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -2775,6 +2776,15 @@ qemuBuildFSStr(virDomainFSDefPtr fs)
if (fs->readonly)
virBufferAddLit(&opt, ",readonly");
+ if (fs->important) {
+ for (i = 0; i < fs->nimportant; ++i) {
+ if (i == 0)
+ virBufferAsprintf(&opt, ",vii=%s", fs->important[i]);
+ else
+ virBufferAsprintf(&opt, ":%s", fs->important[i]);
+ }
+ }
+
if (virBufferCheckError(&opt) < 0)
goto error;
--
2.11.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-06 17:58 ` [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii' Christian Schoenebeck via Qemu-devel
@ 2019-05-07 9:55 ` Greg Kurz
2019-05-07 12:23 ` Christian Schoenebeck via Qemu-devel
2019-05-07 12:57 ` Daniel P. Berrangé
1 sibling, 1 reply; 26+ messages in thread
From: Greg Kurz @ 2019-05-07 9:55 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, Antonios Motakis
On Mon, 06 May 2019 19:58:28 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> This is the counter part patch against latest libvirt git master head to
Hmm... shouldn't this be Cc'd to libvir-list@redhat.com as well then ?
> support the 'vii' feature of patch 5, which introduces the XML config
What is patch 5 ?!? What is 'vii' ? I am a bit lost here...
> XML tag "important" on libvirt side.
>
> To stick with the previous example mentioned with patch 5, likewise
> libvirt XML configuration might then look like this:
>
> <domain type='kvm'>
> ...
> <devices>
> ...
> <filesystem type='mount' accessmode='mapped'>
> <source dir='/vm/fs'/>
> <target dir='root'/>
> <important path='/var/shares'/>
> <important path='/tmp'/>
> </filesystem>
> </devices>
> </domain>
>
> Like with the vii qemu virtfs command line argument, the order of the
> "important" tag defines which one gets the highest inode namespace
> (smallest generated suffix) on guest side.
>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
> docs/schemas/domaincommon.rng | 6 ++++++
> src/conf/domain_conf.c | 30 ++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 3 +++
> src/qemu/qemu_command.c | 10 ++++++++++
> 4 files changed, 49 insertions(+)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 111b85c36f..c75edfc4d3 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2515,6 +2515,12 @@
> </choice>
> </attribute>
> </optional>
> + <zeroOrMore>
> + <element name='important'>
> + <attribute name="path"/>
> + <empty/>
> + </element>
> + </zeroOrMore>
> <optional>
> <element name='readonly'>
> <empty/>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b4fb6cf981..cc75c6a7dd 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2294,6 +2294,8 @@ virDomainFSDefNew(void)
>
> void virDomainFSDefFree(virDomainFSDefPtr def)
> {
> + size_t i;
> +
> if (!def)
> return;
>
> @@ -2302,6 +2304,13 @@ void virDomainFSDefFree(virDomainFSDefPtr def)
> virDomainDeviceInfoClear(&def->info);
> VIR_FREE(def->virtio);
>
> + if (def->important) {
> + for (i = 0; i < def->nimportant; i++)
> + if (def->important[i])
> + VIR_FREE(def->important[i]);
> + }
> + VIR_FREE(def->important);
> +
> VIR_FREE(def);
> }
>
> @@ -10953,6 +10962,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
> VIR_AUTOFREE(char *) usage = NULL;
> VIR_AUTOFREE(char *) units = NULL;
> VIR_AUTOFREE(char *) model = NULL;
> + long n;
>
> ctxt->node = node;
>
> @@ -11001,6 +11011,12 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
> 1, ULLONG_MAX, false) < 0)
> goto error;
>
> + n = virXMLChildElementCount(node);
> + if (n > 0) {
> + if (VIR_ALLOC_N(def->important, n) < 0)
> + goto error;
> + }
> +
> cur = node->children;
> while (cur != NULL) {
> if (cur->type == XML_ELEMENT_NODE) {
> @@ -11039,6 +11055,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
>
> if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0)
> goto error;
> + } else if (virXMLNodeNameEqual(cur, "important")) {
> + def->important[def->nimportant++] = virXMLPropString(cur, "path");
> }
> }
> cur = cur->next;
> @@ -11107,6 +11125,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
> goto error;
>
> cleanup:
> + if (def && def->important && !def->nimportant)
> + VIR_FREE(def->important);
> return def;
>
> error:
> @@ -24601,6 +24621,7 @@ virDomainFSDefFormat(virBufferPtr buf,
> const char *src = def->src->path;
> VIR_AUTOCLEAN(virBuffer) driverBuf = VIR_BUFFER_INITIALIZER;
> int ret = -1;
> + size_t i;
>
> if (!type) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -24689,6 +24710,15 @@ virDomainFSDefFormat(virBufferPtr buf,
> if (def->readonly)
> virBufferAddLit(buf, "<readonly/>\n");
>
> + if (def->important) {
> + for (i = 0; i < def->nimportant; ++i) {
> + if (!def->important[i]) continue;
> + virBufferAddLit(buf, "<important");
> + virBufferEscapeString(buf, " path='%s'", def->important[i]);
> + virBufferAddLit(buf, "/>\n");
> + }
> + }
> +
> if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
> goto cleanup;
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 01c22d8cc3..9bbd66d932 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -818,6 +818,9 @@ struct _virDomainFSDef {
> unsigned long long space_soft_limit; /* in bytes */
> bool symlinksResolved;
> virDomainVirtioOptionsPtr virtio;
> +
> + size_t nimportant;
> + char **important;
> };
>
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 50b4205267..2005ccadf8 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2732,6 +2732,7 @@ qemuBuildFSStr(virDomainFSDefPtr fs)
> virBuffer opt = VIR_BUFFER_INITIALIZER;
> const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver);
> const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy);
> + size_t i;
>
> if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -2775,6 +2776,15 @@ qemuBuildFSStr(virDomainFSDefPtr fs)
> if (fs->readonly)
> virBufferAddLit(&opt, ",readonly");
>
> + if (fs->important) {
> + for (i = 0; i < fs->nimportant; ++i) {
> + if (i == 0)
> + virBufferAsprintf(&opt, ",vii=%s", fs->important[i]);
> + else
> + virBufferAsprintf(&opt, ":%s", fs->important[i]);
> + }
> + }
> +
> if (virBufferCheckError(&opt) < 0)
> goto error;
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-07 9:55 ` Greg Kurz
@ 2019-05-07 12:23 ` Christian Schoenebeck via Qemu-devel
2019-05-07 15:42 ` Greg Kurz
0 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-07 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
On Dienstag, 7. Mai 2019 11:55:56 CEST Greg Kurz wrote:
> > support the 'vii' feature of patch 5, which introduces the XML config
>
> What is patch 5 ?!? What is 'vii' ? I am a bit lost here...
Hi Greg,
Sorry that I caused a bit of confusion, You were actually commenting mostly on
v2 of the patch set, where my email client replaced the message IDs and hence
screwed threading.
This is v3 that I sent yesterday and which has correct threading:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
Please just have a glimpse on that v3 thread, and before I address the details
that you requested (I have reviewed them all already and will address them), I
would like you to ask you for a coarse feedback on design/features first.
Because there are some things where I am unresolved on design level yet:
1. Should I drop the "persistency" feature of patch 3 (same inode numbers
after reboots/suspends) completely from the patch set? It is disabled at
compile time by default for now after entire v3 patch set is applied. Or
should that persistency feature probably become a qemu command line option
instead?
2. If persistency feature should be preserved, shall I probably move out all
the inode remapping code into a separate C unit to avoid 9p.c getting bloated
too much (the amount of code for saving/loading the qp*_table hash tables is
quite large). If yes, any suggestion for an appropriate unit name?
3. Are you fine with the suggested variable length suffixes (patch 4) becoming
the default behaviour (instead of the fixed length 16 bit prefix solution by
Antonios)?
4. Do you have a better idea for a name instead of the suggested "vii" (patch
5) virtfs qemu command line option? And are you fine with the idea of that
"vii" feature anyway?
> > This is the counter part patch against latest libvirt git master head to
>
> Hmm... shouldn't this be Cc'd to libvir-list@redhat.com as well then ?
Well, for now I just provided that libvirt patch to give you an idea about how
imagined this "vii" feature to be used. Does it make sense to CC them already
even though this suggested "vii" command line option does not exist on qemu
side yet?
I know I piled up quite a bit of code on this patch set, so to speed up things
simply raise questions instead of spending too much time in reviewing
everything in detail already.
Thanks Greg!
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/5] 9p: mitigates most QID path collisions
2019-04-23 11:30 ` [Qemu-devel] [PATCH v3 1/5] 9p: mitigates most QID path collisions Christian Schoenebeck via Qemu-devel
@ 2019-05-07 12:42 ` Daniel P. Berrangé
2019-05-07 13:11 ` Christian Schoenebeck via Qemu-devel
0 siblings, 1 reply; 26+ messages in thread
From: Daniel P. Berrangé @ 2019-05-07 12:42 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, Antonios Motakis, Greg Kurz
On Tue, Apr 23, 2019 at 01:30:03PM +0200, Christian Schoenebeck via Qemu-devel wrote:
> This first patch here is an updated version of Antonios Motakis'
> original 4-patch set (using fixed length 16 bit prefixes), merged to one
> patch:
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02283.html
>
> * Updated to latest git master, specifically to new qht interface.
>
> * Merged the original 4 patches to this single patch.
Why did you merge them all into one ? The split patches were "best
practice" IMHO. The original patch authorship & S-o-B lines could
be preserved if you kept them split as before too.
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
> fsdev/9p-marshal.h | 4 +-
> hw/9pfs/9p.c | 200 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> hw/9pfs/9p.h | 21 ++++++
> 3 files changed, 204 insertions(+), 21 deletions(-)
>
> diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
> index c8823d878f..d1ad3645c4 100644
> --- a/fsdev/9p-marshal.h
> +++ b/fsdev/9p-marshal.h
> @@ -10,8 +10,8 @@ typedef struct V9fsString
> typedef struct V9fsQID
> {
> int8_t type;
> - int32_t version;
> - int64_t path;
> + uint32_t version;
> + uint64_t path;
> } V9fsQID;
>
> typedef struct V9fsStat
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 55821343e5..b9bbdcbaee 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -25,6 +25,7 @@
> #include "trace.h"
> #include "migration/blocker.h"
> #include "sysemu/qtest.h"
> +#include "qemu/xxhash.h"
>
> int open_fd_hw;
> int total_open_fd;
> @@ -571,14 +572,135 @@ 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 */
> -static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
> +
> +/* creative abuse of qemu_xxhash7, which is based on xxhash */
> +static uint32_t qpp_hash(QppEntry e)
> {
> - size_t size;
> + 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_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_cmp_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 qp_table_destroy(struct qht *ht)
> +{
> + qht_iter(ht, qp_table_remove, NULL);
> + qht_destroy(ht);
> +}
> +
> +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) {
> + qht_init(&pdu->s->qpf_table, qpf_cmp_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
> + }
> +
> + val = qht_lookup(&pdu->s->qpf_table, &lookup, hash);
> +
> + if (!val) {
> + if (pdu->s->qp_fullpath_next == 0) {
> + /* no more files can be mapped :'( */
> + return -ENFILE;
> + }
> +
> + val = g_malloc0(sizeof(QppEntry));
> + if (!val) {
> + return -ENOMEM;
> + }
> + *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
> + * 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 */
> + return -ENFILE;
> + }
> +
> + val = g_malloc0(sizeof(QppEntry));
> + if (!val) {
> + return -ENOMEM;
> + }
> + *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;
> +
> + /* map inode+device to qid path (fast path) */
> + err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
> + if (err == -ENFILE) {
> + /* fast path didn't work, fal back to full map */
> + err = qid_path_fullmap(pdu, stbuf, &qidp->path);
> + }
> + if (err) {
> + return err;
> + }
>
> - 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)) {
> @@ -587,6 +709,8 @@ static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
> if (S_ISLNK(stbuf->st_mode)) {
> qidp->type |= P9_QID_TYPE_SYMLINK;
> }
> +
> + return 0;
> }
>
> static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> @@ -599,7 +723,10 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> if (err < 0) {
> return err;
> }
> - stat_to_qid(&stbuf, qidp);
> + err = stat_to_qid(pdu, &stbuf, qidp);
> + if (err < 0) {
> + return err;
> + }
> return 0;
> }
>
> @@ -830,7 +957,10 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
>
> memset(v9stat, 0, sizeof(*v9stat));
>
> - stat_to_qid(stbuf, &v9stat->qid);
> + err = stat_to_qid(pdu, stbuf, &v9stat->qid);
> + if (err < 0) {
> + return err;
> + }
> v9stat->mode = stat_to_v9mode(stbuf);
> v9stat->atime = stbuf->st_atime;
> v9stat->mtime = stbuf->st_mtime;
> @@ -891,7 +1021,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
> #define P9_STATS_ALL 0x00003fffULL /* Mask for All fields above */
>
>
> -static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
> +static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> V9fsStatDotl *v9lstat)
> {
> memset(v9lstat, 0, sizeof(*v9lstat));
> @@ -913,7 +1043,7 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
> /* Currently we only support BASIC fields in stat */
> v9lstat->st_result_mask = P9_STATS_BASIC;
>
> - stat_to_qid(stbuf, &v9lstat->qid);
> + return stat_to_qid(pdu, stbuf, &v9lstat->qid);
> }
>
> static void print_sg(struct iovec *sg, int cnt)
> @@ -1115,7 +1245,6 @@ static void coroutine_fn v9fs_getattr(void *opaque)
> uint64_t request_mask;
> V9fsStatDotl v9stat_dotl;
> V9fsPDU *pdu = opaque;
> - V9fsState *s = pdu->s;
>
> retval = pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
> if (retval < 0) {
> @@ -1136,7 +1265,10 @@ static void coroutine_fn v9fs_getattr(void *opaque)
> if (retval < 0) {
> goto out;
> }
> - stat_to_v9stat_dotl(s, &stbuf, &v9stat_dotl);
> + retval = stat_to_v9stat_dotl(pdu, &stbuf, &v9stat_dotl);
> + if (retval < 0) {
> + goto out;
> + }
>
> /* fill st_gen if requested and supported by underlying fs */
> if (request_mask & P9_STATS_GEN) {
> @@ -1381,7 +1513,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
> if (err < 0) {
> goto out;
> }
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> v9fs_path_copy(&dpath, &path);
> }
> memcpy(&qids[name_idx], &qid, sizeof(qid));
> @@ -1483,7 +1618,10 @@ static void coroutine_fn v9fs_open(void *opaque)
> if (err < 0) {
> goto out;
> }
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> if (S_ISDIR(stbuf.st_mode)) {
> err = v9fs_co_opendir(pdu, fidp);
> if (err < 0) {
> @@ -1593,7 +1731,10 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
> fidp->flags |= FID_NON_RECLAIMABLE;
> }
> iounit = get_iounit(pdu, &fidp->path);
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
> if (err < 0) {
> goto out;
> @@ -2327,7 +2468,10 @@ static void coroutine_fn v9fs_create(void *opaque)
> }
> }
> iounit = get_iounit(pdu, &fidp->path);
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
> if (err < 0) {
> goto out;
> @@ -2384,7 +2528,10 @@ static void coroutine_fn v9fs_symlink(void *opaque)
> if (err < 0) {
> goto out;
> }
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> err = pdu_marshal(pdu, offset, "Q", &qid);
> if (err < 0) {
> goto out;
> @@ -3064,7 +3211,10 @@ static void coroutine_fn v9fs_mknod(void *opaque)
> if (err < 0) {
> goto out;
> }
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> err = pdu_marshal(pdu, offset, "Q", &qid);
> if (err < 0) {
> goto out;
> @@ -3222,7 +3372,10 @@ static void coroutine_fn v9fs_mkdir(void *opaque)
> if (err < 0) {
> goto out;
> }
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> err = pdu_marshal(pdu, offset, "Q", &qid);
> if (err < 0) {
> goto out;
> @@ -3633,6 +3786,11 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
> goto out;
> }
>
> + /* QID path hash table. 1 entry ought to be enough for anybody ;) */
> + qht_init(&s->qpp_table, qpp_cmp_func, 1, QHT_MODE_AUTO_RESIZE);
> + 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);
>
> @@ -3646,6 +3804,8 @@ out:
> }
> g_free(s->tag);
> g_free(s->ctx.fs_root);
> + qp_table_destroy(&s->qpp_table);
> + qp_table_destroy(&s->qpf_table);
> v9fs_path_free(&path);
> }
> return rc;
> @@ -3658,6 +3818,8 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
> }
> fsdev_throttle_cleanup(s->ctx.fst);
> g_free(s->tag);
> + 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 8883761b2c..44112ea97f 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,22 @@ struct V9fsFidState
> V9fsFidState *rclm_lst;
> };
>
> +#define QPATH_INO_MASK (((unsigned long)1 << 48) - 1)
> +
> +/* QID path prefix entry, see stat_to_qid */
> +typedef struct {
> + dev_t dev;
> + uint16_t ino_prefix;
> + 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;
> @@ -256,6 +273,10 @@ struct V9fsState
> Error *migration_blocker;
> V9fsConf fsconf;
> V9fsQID root_qid;
> + struct qht qpp_table;
> + struct qht qpf_table;
> + uint16_t qp_prefix_next;
> + uint64_t qp_fullpath_next;
> };
>
> /* 9p2000.L open flags */
> --
> 2.11.0
>
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation
2019-04-23 11:35 ` [Qemu-devel] [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation Christian Schoenebeck via Qemu-devel
@ 2019-05-07 12:43 ` Daniel P. Berrangé
0 siblings, 0 replies; 26+ messages in thread
From: Daniel P. Berrangé @ 2019-05-07 12:43 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, Antonios Motakis, Greg Kurz
On Tue, Apr 23, 2019 at 01:35:23PM +0200, Christian Schoenebeck via Qemu-devel wrote:
> Addresses trivial changes regarding the previous patch as requested
> on the mailing list a while ago.
These changes should just be made to the original 4 patches individually
rather than in a new patch.
>
> * Removed unneccessary parantheses:
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02661.html
>
> * Removed unneccessary g_malloc() result checks:
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02814.html
>
> * Unsigned type changes:
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02581.html
>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
> fsdev/9p-marshal.h | 2 +-
> hw/9pfs/9p.c | 16 +++++-----------
> hw/9pfs/trace-events | 14 +++++++-------
> 3 files changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
> index d1ad3645c4..8f3babb60a 100644
> --- a/fsdev/9p-marshal.h
> +++ b/fsdev/9p-marshal.h
> @@ -9,7 +9,7 @@ typedef struct V9fsString
>
> typedef struct V9fsQID
> {
> - int8_t type;
> + uint8_t type;
> uint32_t version;
> uint64_t path;
> } V9fsQID;
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index b9bbdcbaee..2b893e25a1 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -587,13 +587,13 @@ static uint32_t qpf_hash(QpfEntry e)
> 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);
> + return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
> }
>
> 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);
> + return e1->dev == e2->dev && e1->ino == e2->ino;
> }
>
> static void qp_table_remove(void *p, uint32_t h, void *up)
> @@ -630,9 +630,6 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
> }
>
> val = g_malloc0(sizeof(QppEntry));
> - if (!val) {
> - return -ENOMEM;
> - }
> *val = lookup;
>
> /* new unique inode and device combo */
> @@ -673,9 +670,6 @@ static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
> }
>
> val = g_malloc0(sizeof(QppEntry));
> - if (!val) {
> - return -ENOMEM;
> - }
> *val = lookup;
>
> /* new unique inode prefix and device combo */
> @@ -870,9 +864,9 @@ static int donttouch_stat(V9fsStat *stat)
> {
> if (stat->type == -1 &&
> stat->dev == -1 &&
> - stat->qid.type == -1 &&
> - stat->qid.version == -1 &&
> - stat->qid.path == -1 &&
> + stat->qid.type == 0xff &&
> + stat->qid.version == (uint32_t) -1 &&
> + stat->qid.path == (uint64_t) -1 &&
> stat->mode == -1 &&
> stat->atime == -1 &&
> stat->mtime == -1 &&
> diff --git a/hw/9pfs/trace-events b/hw/9pfs/trace-events
> index c0a0a4ab5d..6964756922 100644
> --- a/hw/9pfs/trace-events
> +++ b/hw/9pfs/trace-events
> @@ -6,7 +6,7 @@ v9fs_rerror(uint16_t tag, uint8_t id, int err) "tag %d id %d err %d"
> v9fs_version(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag %d id %d msize %d version %s"
> v9fs_version_return(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag %d id %d msize %d version %s"
> v9fs_attach(uint16_t tag, uint8_t id, int32_t fid, int32_t afid, char* uname, char* aname) "tag %u id %u fid %d afid %d uname %s aname %s"
> -v9fs_attach_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path) "tag %d id %d type %d version %d path %"PRId64
> +v9fs_attach_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path) "tag %d id %d type %d version %d path %"PRId64
> v9fs_stat(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
> v9fs_stat_return(uint16_t tag, uint8_t id, int32_t mode, int32_t atime, int32_t mtime, int64_t length) "tag %d id %d stat={mode %d atime %d mtime %d length %"PRId64"}"
> v9fs_getattr(uint16_t tag, uint8_t id, int32_t fid, uint64_t request_mask) "tag %d id %d fid %d request_mask %"PRIu64
> @@ -14,9 +14,9 @@ v9fs_getattr_return(uint16_t tag, uint8_t id, uint64_t result_mask, uint32_t mod
> v9fs_walk(uint16_t tag, uint8_t id, int32_t fid, int32_t newfid, uint16_t nwnames) "tag %d id %d fid %d newfid %d nwnames %d"
> v9fs_walk_return(uint16_t tag, uint8_t id, uint16_t nwnames, void* qids) "tag %d id %d nwnames %d qids %p"
> v9fs_open(uint16_t tag, uint8_t id, int32_t fid, int32_t mode) "tag %d id %d fid %d mode %d"
> -v9fs_open_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
> +v9fs_open_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
> v9fs_lcreate(uint16_t tag, uint8_t id, int32_t dfid, int32_t flags, int32_t mode, uint32_t gid) "tag %d id %d dfid %d flags %d mode %d gid %u"
> -v9fs_lcreate_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int32_t iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
> +v9fs_lcreate_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int32_t iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
> v9fs_fsync(uint16_t tag, uint8_t id, int32_t fid, int datasync) "tag %d id %d fid %d datasync %d"
> v9fs_clunk(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
> v9fs_read(uint16_t tag, uint8_t id, int32_t fid, uint64_t off, uint32_t max_count) "tag %d id %d fid %d off %"PRIu64" max_count %u"
> @@ -26,21 +26,21 @@ v9fs_readdir_return(uint16_t tag, uint8_t id, uint32_t count, ssize_t retval) "t
> v9fs_write(uint16_t tag, uint8_t id, int32_t fid, uint64_t off, uint32_t count, int cnt) "tag %d id %d fid %d off %"PRIu64" count %u cnt %d"
> v9fs_write_return(uint16_t tag, uint8_t id, int32_t total, ssize_t err) "tag %d id %d total %d err %zd"
> v9fs_create(uint16_t tag, uint8_t id, int32_t fid, char* name, int32_t perm, int8_t mode) "tag %d id %d fid %d name %s perm %d mode %d"
> -v9fs_create_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
> +v9fs_create_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
> v9fs_symlink(uint16_t tag, uint8_t id, int32_t fid, char* name, char* symname, uint32_t gid) "tag %d id %d fid %d name %s symname %s gid %u"
> -v9fs_symlink_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
> +v9fs_symlink_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
> v9fs_flush(uint16_t tag, uint8_t id, int16_t flush_tag) "tag %d id %d flush_tag %d"
> v9fs_link(uint16_t tag, uint8_t id, int32_t dfid, int32_t oldfid, char* name) "tag %d id %d dfid %d oldfid %d name %s"
> v9fs_remove(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
> v9fs_wstat(uint16_t tag, uint8_t id, int32_t fid, int32_t mode, int32_t atime, int32_t mtime) "tag %u id %u fid %d stat={mode %d atime %d mtime %d}"
> v9fs_mknod(uint16_t tag, uint8_t id, int32_t fid, int mode, int major, int minor) "tag %d id %d fid %d mode %d major %d minor %d"
> -v9fs_mknod_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
> +v9fs_mknod_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
> v9fs_lock(uint16_t tag, uint8_t id, int32_t fid, uint8_t type, uint64_t start, uint64_t length) "tag %d id %d fid %d type %d start %"PRIu64" length %"PRIu64
> v9fs_lock_return(uint16_t tag, uint8_t id, int8_t status) "tag %d id %d status %d"
> v9fs_getlock(uint16_t tag, uint8_t id, int32_t fid, uint8_t type, uint64_t start, uint64_t length)"tag %d id %d fid %d type %d start %"PRIu64" length %"PRIu64
> v9fs_getlock_return(uint16_t tag, uint8_t id, uint8_t type, uint64_t start, uint64_t length, uint32_t proc_id) "tag %d id %d type %d start %"PRIu64" length %"PRIu64" proc_id %u"
> v9fs_mkdir(uint16_t tag, uint8_t id, int32_t fid, char* name, int mode, uint32_t gid) "tag %u id %u fid %d name %s mode %d gid %u"
> -v9fs_mkdir_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int err) "tag %u id %u qid={type %d version %d path %"PRId64"} err %d"
> +v9fs_mkdir_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int err) "tag %u id %u qid={type %d version %d path %"PRId64"} err %d"
> v9fs_xattrwalk(uint16_t tag, uint8_t id, int32_t fid, int32_t newfid, char* name) "tag %d id %d fid %d newfid %d name %s"
> v9fs_xattrwalk_return(uint16_t tag, uint8_t id, int64_t size) "tag %d id %d size %"PRId64
> v9fs_xattrcreate(uint16_t tag, uint8_t id, int32_t fid, char* name, uint64_t size, int flags) "tag %d id %d fid %d name %s size %"PRIu64" flags %d"
> --
> 2.11.0
>
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-06 17:58 ` [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii' Christian Schoenebeck via Qemu-devel
2019-05-07 9:55 ` Greg Kurz
@ 2019-05-07 12:57 ` Daniel P. Berrangé
2019-05-07 13:48 ` Christian Schoenebeck via Qemu-devel
1 sibling, 1 reply; 26+ messages in thread
From: Daniel P. Berrangé @ 2019-05-07 12:57 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, Antonios Motakis, Greg Kurz
On Mon, May 06, 2019 at 07:58:28PM +0200, Christian Schoenebeck via Qemu-devel wrote:
> This is the counter part patch against latest libvirt git master head to
> support the 'vii' feature of patch 5, which introduces the XML config
> XML tag "important" on libvirt side.
>
> To stick with the previous example mentioned with patch 5, likewise
> libvirt XML configuration might then look like this:
>
> <domain type='kvm'>
> ...
> <devices>
> ...
> <filesystem type='mount' accessmode='mapped'>
> <source dir='/vm/fs'/>
> <target dir='root'/>
> <important path='/var/shares'/>
> <important path='/tmp'/>
> </filesystem>
> </devices>
> </domain>
>
> Like with the vii qemu virtfs command line argument, the order of the
> "important" tag defines which one gets the highest inode namespace
> (smallest generated suffix) on guest side.
Do we think anyone is likely to use this feature in the real world ?
I'm not really a fan of the representation, because this is affecting
guest ABI via a side effect of the ordering which is the kind of thing
that has got us in trouble before. If we need control over the IDs
used for each mount point, then I tend to think we need to represent
it explicitly such that the mgmt app sets the exact ID used.
<pathid dir="/var/shares" id="0x1"/>
<pathid dir="/tmp" id="0x3"/>
this ensures that the IDs are still stable when adding or removing
paths
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/5] 9p: mitigates most QID path collisions
2019-05-07 12:42 ` Daniel P. Berrangé
@ 2019-05-07 13:11 ` Christian Schoenebeck via Qemu-devel
2019-05-07 13:13 ` Daniel P. Berrangé
0 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-07 13:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
On Dienstag, 7. Mai 2019 13:42:47 CEST Daniel P. Berrangé wrote:
> > This first patch here is an updated version of Antonios Motakis'
> > original 4-patch set (using fixed length 16 bit prefixes), merged to one
> > patch:
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02283.html
> >
> > * Updated to latest git master, specifically to new qht interface.
> >
> > * Merged the original 4 patches to this single patch.
>
> Why did you merge them all into one ? The split patches were "best
> practice" IMHO. The original patch authorship & S-o-B lines could
> be preserved if you kept them split as before too.
Seems I misinterpreted an old comment of Greg that he would like to see them
to be merged into less patches. I think it was this one:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02590.html
No problem, I will restore Antonios' original individual 4 patches
appropriately. What about SOB then? Should I just place Antonio's SOB on those
4 patches or does it need his and mine SOB lines? (I mean I need to rebase
those 4 patches and address the old issues on them)
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/5] 9p: mitigates most QID path collisions
2019-05-07 13:11 ` Christian Schoenebeck via Qemu-devel
@ 2019-05-07 13:13 ` Daniel P. Berrangé
0 siblings, 0 replies; 26+ messages in thread
From: Daniel P. Berrangé @ 2019-05-07 13:13 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, Antonios Motakis, Greg Kurz
On Tue, May 07, 2019 at 03:11:26PM +0200, Christian Schoenebeck wrote:
> On Dienstag, 7. Mai 2019 13:42:47 CEST Daniel P. Berrangé wrote:
> > > This first patch here is an updated version of Antonios Motakis'
> > > original 4-patch set (using fixed length 16 bit prefixes), merged to one
> > > patch:
> > >
> > > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02283.html
> > >
> > > * Updated to latest git master, specifically to new qht interface.
> > >
> > > * Merged the original 4 patches to this single patch.
> >
> > Why did you merge them all into one ? The split patches were "best
> > practice" IMHO. The original patch authorship & S-o-B lines could
> > be preserved if you kept them split as before too.
>
> Seems I misinterpreted an old comment of Greg that he would like to see them
> to be merged into less patches. I think it was this one:
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02590.html
>
> No problem, I will restore Antonios' original individual 4 patches
> appropriately. What about SOB then? Should I just place Antonio's SOB on those
> 4 patches or does it need his and mine SOB lines? (I mean I need to rebase
> those 4 patches and address the old issues on them)
If the patch is unchanged from Antonio's original then it only needs to
have Antonio's SOB. If you have modified the patch yourself, then you
would add your own SOB, in addition to Antonio's original SOB.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-07 12:57 ` Daniel P. Berrangé
@ 2019-05-07 13:48 ` Christian Schoenebeck via Qemu-devel
0 siblings, 0 replies; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-07 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
On Dienstag, 7. Mai 2019 13:57:56 CEST Daniel P. Berrangé wrote:
> > ...
> > <filesystem type='mount' accessmode='mapped'>
> >
> > <source dir='/vm/fs'/>
> > <target dir='root'/>
> > <important path='/var/shares'/>
> > <important path='/tmp'/>
> >
> > </filesystem>
> >
> > </devices>
> >
> > </domain>
> >
> > Like with the vii qemu virtfs command line argument, the order of the
> > "important" tag defines which one gets the highest inode namespace
> > (smallest generated suffix) on guest side.
>
> Do we think anyone is likely to use this feature in the real world ?
I don't know if other people need it, that's one of the reasons why I am
asking for a coarse high level feedback of the current v3 patch set before
getting into the details.
The only thing I can say right now is that I must use this feature when
running Samba to avoid all kinds of serious problems. And I could imagine
inode namespace control to become more of an issue once nested virtualization
becomes more popular.
> I'm not really a fan of the representation, because this is affecting
> guest ABI via a side effect of the ordering which is the kind of thing
> that has got us in trouble before. If we need control over the IDs
> used for each mount point, then I tend to think we need to represent
> it explicitly such that the mgmt app sets the exact ID used.
>
> <pathid dir="/var/shares" id="0x1"/>
> <pathid dir="/tmp" id="0x3"/>
>
> this ensures that the IDs are still stable when adding or removing
> paths
Well, that would lead to the exact opposite of what you asked for. Because
allowing admins to configure an exact ID (which I think you mean should be used
as exact inode suffix by 9p then) would expose implementation details inside
9pfs to config space, which are subject to change, might collide with
implementation details, and requires implementation knowledge and extreme care
by admins so they would pick appropriate IDs with "suffix-free" property which
are guaranteed to create unique numbers in all cases:
https://en.wikipedia.org/wiki/Prefix_code
Also keep in mind that one fs device might end up having multiple suffixes.
Hence my suggestion was to only expose the bare minimum to config space
regarding this issue: Asking (if required at all) admins which ones are the
most critical pathes regarding inode namespace for their use cases, and 9p
would then automatically generate appropriate suffixes for those mentioned by
admin to achieve the highest inode namespace appropriately and in a safe way.
Plus for the "important path=" semantics I suggested you don't have have to
use mount points BTW. You can use subdirs and even individual files and 9pfs
would then automatically resolve the appropriate fs device of the given path.
So e.g. when using nested virtualization, an admin inside a lower level guest
does not even need to know the exact mount points on a higher level / host.
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-07 12:23 ` Christian Schoenebeck via Qemu-devel
@ 2019-05-07 15:42 ` Greg Kurz
2019-05-07 16:16 ` Christian Schoenebeck via Qemu-devel
0 siblings, 1 reply; 26+ messages in thread
From: Greg Kurz @ 2019-05-07 15:42 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, Antonios Motakis
On Tue, 07 May 2019 14:23:11 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> On Dienstag, 7. Mai 2019 11:55:56 CEST Greg Kurz wrote:
> > > support the 'vii' feature of patch 5, which introduces the XML config
> >
> > What is patch 5 ?!? What is 'vii' ? I am a bit lost here...
>
> Hi Greg,
>
> Sorry that I caused a bit of confusion, You were actually commenting mostly on
> v2 of the patch set, where my email client replaced the message IDs and hence
> screwed threading.
>
> This is v3 that I sent yesterday and which has correct threading:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
>
For a reason yet to be investigated, I haven't received it yet...
> Please just have a glimpse on that v3 thread, and before I address the details
> that you requested (I have reviewed them all already and will address them), I
> would like you to ask you for a coarse feedback on design/features first.
> Because there are some things where I am unresolved on design level yet:
>
I'll try but probably not before next week.
> 1. Should I drop the "persistency" feature of patch 3 (same inode numbers
> after reboots/suspends) completely from the patch set? It is disabled at
> compile time by default for now after entire v3 patch set is applied. Or
> should that persistency feature probably become a qemu command line option
> instead?
>
> 2. If persistency feature should be preserved, shall I probably move out all
> the inode remapping code into a separate C unit to avoid 9p.c getting bloated
> too much (the amount of code for saving/loading the qp*_table hash tables is
> quite large). If yes, any suggestion for an appropriate unit name?
>
> 3. Are you fine with the suggested variable length suffixes (patch 4) becoming
> the default behaviour (instead of the fixed length 16 bit prefix solution by
> Antonios)?
>
> 4. Do you have a better idea for a name instead of the suggested "vii" (patch
> 5) virtfs qemu command line option? And are you fine with the idea of that
> "vii" feature anyway?
>
> > > This is the counter part patch against latest libvirt git master head to
> >
> > Hmm... shouldn't this be Cc'd to libvir-list@redhat.com as well then ?
>
> Well, for now I just provided that libvirt patch to give you an idea about how
> imagined this "vii" feature to be used. Does it make sense to CC them already
> even though this suggested "vii" command line option does not exist on qemu
> side yet?
>
> I know I piled up quite a bit of code on this patch set, so to speed up things
> simply raise questions instead of spending too much time in reviewing
> everything in detail already.
>
> Thanks Greg!
>
> Best regards,
> Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-07 15:42 ` Greg Kurz
@ 2019-05-07 16:16 ` Christian Schoenebeck via Qemu-devel
2019-05-17 8:40 ` Christian Schoenebeck via Qemu-devel
0 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-07 16:16 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz, Antonios Motakis
On Dienstag, 7. Mai 2019 17:42:39 CEST Greg Kurz wrote:
> > Sorry that I caused a bit of confusion, You were actually commenting
> > mostly on v2 of the patch set, where my email client replaced the message
> > IDs and hence screwed threading.
> >
> > This is v3 that I sent yesterday and which has correct threading:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
>
> For a reason yet to be investigated, I haven't received it yet...
Here are the archive links for latest v3 patch set [5(+1) patches total]:
[PATCH v3 0/5] 9p: Fix file ID collisions:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
[PATCH v3 1/5] 9p: mitigates most QID path collisions:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01142.html
[PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01140.html
[PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01144.html
[PATCH v3 4/5] 9p: use variable length suffixes for inode mapping:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01141.html
[PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01138.html
And the optional libvirt patch:
[libvirt patch] qemu: adds support for virtfs 9p argument 'vii':
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01223.html
> > Please just have a glimpse on that v3 thread, and before I address the
> > details that you requested (I have reviewed them all already and will
> > address them), I would like you to ask you for a coarse feedback on
> > design/features first.
> > Because there are some things where I am unresolved on design level yet:
> I'll try but probably not before next week.
No problem, take your time!
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-07 16:16 ` Christian Schoenebeck via Qemu-devel
@ 2019-05-17 8:40 ` Christian Schoenebeck via Qemu-devel
2019-05-17 12:30 ` Greg Kurz
0 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-17 8:40 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé, Greg Kurz, Antonios Motakis
On Dienstag, 7. Mai 2019 18:16:08 CEST Christian Schoenebeck wrote:
> Here are the archive links for latest v3 patch set [5(+1) patches total]:
>
> [PATCH v3 0/5] 9p: Fix file ID collisions:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
>
> [PATCH v3 1/5] 9p: mitigates most QID path collisions:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01142.html
>
> [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01140.html
>
> [PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01144.html
>
> [PATCH v3 4/5] 9p: use variable length suffixes for inode mapping:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01141.html
>
> [PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01138.html
>
> And the optional libvirt patch:
>
> [libvirt patch] qemu: adds support for virtfs 9p argument 'vii':
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01223.html
>
> > > Please just have a glimpse on that v3 thread, and before I address the
> > > details that you requested (I have reviewed them all already and will
> > > address them), I would like you to ask you for a coarse feedback on
> > > design/features first.
> >
> > > Because there are some things where I am unresolved on design level yet:
> >
> > I'll try but probably not before next week.
Hi Greg, you have not forgotten about me, did you? ;-)
Or should I go ahead and provide a v4 next week addressing the issues
discussed so far?
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-17 8:40 ` Christian Schoenebeck via Qemu-devel
@ 2019-05-17 12:30 ` Greg Kurz
2019-05-17 13:23 ` Christian Schoenebeck via Qemu-devel
0 siblings, 1 reply; 26+ messages in thread
From: Greg Kurz @ 2019-05-17 12:30 UTC (permalink / raw)
To: Christian Schoenebeck
Cc: Daniel P. Berrangé, qemu-devel, Antonios Motakis
On Fri, 17 May 2019 10:40:48 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> On Dienstag, 7. Mai 2019 18:16:08 CEST Christian Schoenebeck wrote:
> > Here are the archive links for latest v3 patch set [5(+1) patches total]:
> >
> > [PATCH v3 0/5] 9p: Fix file ID collisions:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
> >
> > [PATCH v3 1/5] 9p: mitigates most QID path collisions:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01142.html
> >
> > [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01140.html
> >
> > [PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01144.html
> >
> > [PATCH v3 4/5] 9p: use variable length suffixes for inode mapping:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01141.html
> >
> > [PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01138.html
> >
> > And the optional libvirt patch:
> >
> > [libvirt patch] qemu: adds support for virtfs 9p argument 'vii':
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01223.html
> >
> > > > Please just have a glimpse on that v3 thread, and before I address the
> > > > details that you requested (I have reviewed them all already and will
> > > > address them), I would like you to ask you for a coarse feedback on
> > > > design/features first.
> > >
> > > > Because there are some things where I am unresolved on design level yet:
> > >
> > > I'll try but probably not before next week.
>
> Hi Greg, you have not forgotten about me, did you? ;-)
>
Hi Christian,
I have certainly not forgotten but I had (and still have) some more urgent
work to do and I couldn't find time for this... Sorry :)
> Or should I go ahead and provide a v4 next week addressing the issues
> discussed so far?
>
Thinking again about the initial issue raised by Antonios, I agree we
should at least detect collisions in the existing 9pfs code and
emit an error rather than silently returning duplicate QIDs to the
client. This would be patch 2 from Antonios's series: only allow
a single host device for a given fs device.
Then, we come to the bulk problem: how to handle the case where we
have multiple devices involved in a directory we want to share ?
Antonios's proposal is a clever way to address the collisions, but
your work proves it isn't enough... Before going forward, I'd like
to consider another approach.
What about:
1) de-compose the shared directory on a per-device basis,
ie. identify all mount points under the shared directory
2) expose found mount points separately, each with its onw 9p device
3) re-compose the directory tree within the guest using the same topology
as the host
ie. if you want to share /vm/fs and
/vm/fs on device A
/vm/fs/shares on device B
/vm/fs/tmp on device C
you would start QEMU with
-fsdev local,path=/vm/fs,id=fsdev0... \
-device virtio-9p,fsdev=fsdev0,mount_tag=tag0 \
-fsdev local,path=/vm/fs,id=fsdev1... \
-device virtio-9p,fsdev=fsdev1,mount_tag=tag1 \
-fsdev local,path=/vm/fs,id=fsdev2... \
-device virtio-9p,fsdev=fsdev2,mount_tag=tag2
and /etc/fstab in the guest:
tag0 / 9p nofail,trans=virtio,version=9p2000.L 0 0
tag1 /shares 9p nofail,trans=virtio,version=9p2000.L 0 0
tag2 /tmp 9p nofail,trans=virtio,version=9p2000.L 0 0
This involves some more work for the user but it doesn't require
any changes in QEMU.
Would this approach solve the issues you've been hitting with Samba ?
Cheers,
--
Greg
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-17 12:30 ` Greg Kurz
@ 2019-05-17 13:23 ` Christian Schoenebeck via Qemu-devel
2019-05-17 14:47 ` Greg Kurz
0 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-17 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Christian Schoenebeck, Greg Kurz, Antonios Motakis
On Freitag, 17. Mai 2019 14:30:29 CEST Greg Kurz wrote:
> Then, we come to the bulk problem: how to handle the case where we
> have multiple devices involved in a directory we want to share ?
> Antonios's proposal is a clever way to address the collisions, but
> your work proves it isn't enough...
With the patch set I have right now, things finally bahave smooth.
> Before going forward, I'd like
> to consider another approach.
>
> What about:
>
> 1) de-compose the shared directory on a per-device basis,
> ie. identify all mount points under the shared directory
>
> 2) expose found mount points separately, each with its onw 9p device
>
> 3) re-compose the directory tree within the guest using the same topology
> as the host
>
> ie. if you want to share /vm/fs and
>
> /vm/fs on device A
> /vm/fs/shares on device B
> /vm/fs/tmp on device C
>
> you would start QEMU with
>
> -fsdev local,path=/vm/fs,id=fsdev0... \
> -device virtio-9p,fsdev=fsdev0,mount_tag=tag0 \
> -fsdev local,path=/vm/fs,id=fsdev1... \
> -device virtio-9p,fsdev=fsdev1,mount_tag=tag1 \
> -fsdev local,path=/vm/fs,id=fsdev2... \
> -device virtio-9p,fsdev=fsdev2,mount_tag=tag2
>
> and /etc/fstab in the guest:
>
> tag0 / 9p nofail,trans=virtio,version=9p2000.L 0 0
> tag1 /shares 9p nofail,trans=virtio,version=9p2000.L 0 0
> tag2 /tmp 9p nofail,trans=virtio,version=9p2000.L 0 0
>
> This involves some more work for the user but it doesn't require
> any changes in QEMU.
So your suggestion is actually: don't fix it.
"Some" more work for the user is a quantity of how many guests you are
running, multiplied by the nested virtualization levels you might have =
potentially a lot of work for admins.
> Would this approach solve the issues you've been hitting with Samba ?
No, because that completely neglects runtime changes on a higher level (host),
plus it completely destroys the fundamental idea about 9p, which is about
transparency of the higher level(s).
May I ask, do you have concrete reasons why you want to abondon the entire
patch set? Because that's what it sounds to me.
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-17 13:23 ` Christian Schoenebeck via Qemu-devel
@ 2019-05-17 14:47 ` Greg Kurz
2019-05-17 20:53 ` Christian Schoenebeck via Qemu-devel
0 siblings, 1 reply; 26+ messages in thread
From: Greg Kurz @ 2019-05-17 14:47 UTC (permalink / raw)
To: Christian Schoenebeck
Cc: Daniel P. Berrangé, qemu-devel, Antonios Motakis
On Fri, 17 May 2019 15:23:01 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> On Freitag, 17. Mai 2019 14:30:29 CEST Greg Kurz wrote:
> > Then, we come to the bulk problem: how to handle the case where we
> > have multiple devices involved in a directory we want to share ?
> > Antonios's proposal is a clever way to address the collisions, but
> > your work proves it isn't enough...
>
> With the patch set I have right now, things finally bahave smooth.
>
> > Before going forward, I'd like
> > to consider another approach.
> >
> > What about:
> >
> > 1) de-compose the shared directory on a per-device basis,
> > ie. identify all mount points under the shared directory
> >
> > 2) expose found mount points separately, each with its onw 9p device
> >
> > 3) re-compose the directory tree within the guest using the same topology
> > as the host
> >
> > ie. if you want to share /vm/fs and
> >
> > /vm/fs on device A
> > /vm/fs/shares on device B
> > /vm/fs/tmp on device C
> >
> > you would start QEMU with
> >
> > -fsdev local,path=/vm/fs,id=fsdev0... \
> > -device virtio-9p,fsdev=fsdev0,mount_tag=tag0 \
> > -fsdev local,path=/vm/fs,id=fsdev1... \
> > -device virtio-9p,fsdev=fsdev1,mount_tag=tag1 \
> > -fsdev local,path=/vm/fs,id=fsdev2... \
> > -device virtio-9p,fsdev=fsdev2,mount_tag=tag2
> >
> > and /etc/fstab in the guest:
> >
> > tag0 / 9p nofail,trans=virtio,version=9p2000.L 0 0
> > tag1 /shares 9p nofail,trans=virtio,version=9p2000.L 0 0
> > tag2 /tmp 9p nofail,trans=virtio,version=9p2000.L 0 0
> >
> > This involves some more work for the user but it doesn't require
> > any changes in QEMU.
>
> So your suggestion is actually: don't fix it.
>
Potentially yes if another approach is satisfying enough, as I wouldn't
want to over-engineer too much around this 9p imposed limitation. The
right thing to do would be to come up with a new version of the protocol
with variable sized QIDs and call it a day.
> "Some" more work for the user is a quantity of how many guests you are
> running, multiplied by the nested virtualization levels you might have =
> potentially a lot of work for admins.
>
Maybe, I don't have enough feedback on 9p use cases to have a clear
picture...
> > Would this approach solve the issues you've been hitting with Samba ?
>
> No, because that completely neglects runtime changes on a higher level (host),
Unless I'm missing something, runtime changes would be neglected as well
with the "vii" property since it is static for the device lifetime.
> plus it completely destroys the fundamental idea about 9p, which is about
> transparency of the higher level(s).
>
That's a point indeed, even if again I'm not sure if this is a frequent
case to share a directory tree spanning over multiple devices.
> May I ask, do you have concrete reasons why you want to abondon the entire
> patch set? Because that's what it sounds to me.
>
I don't have that much time to spend on 9p maintainership, for both
reviewing and fixing bugs (CVEs most of the time). So, yes it may
sound like I want to drop the patchset, but it's just I need to be
convinced I won't regret having merged a huge change like this...
when I'll have to support it alone later ;-)
For the moment, I'm not convinced by the "vii" solution. It even
motivated my suggestion of having several devices actually, since
the paths you would put in there are known before starting QEMU.
It might take me some more rounds of discussion to decide. I understand
it is frustrating but bear with me :)
> Best regards,
> Christian Schoenebeck
Cheers,
--
Greg
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-17 14:47 ` Greg Kurz
@ 2019-05-17 20:53 ` Christian Schoenebeck via Qemu-devel
2019-05-20 14:05 ` Greg Kurz
0 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-17 20:53 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Christian Schoenebeck, Greg Kurz, Antonios Motakis
On Freitag, 17. Mai 2019 16:47:46 CEST Greg Kurz wrote:
> Potentially yes if another approach is satisfying enough, as I wouldn't
> want to over-engineer too much around this 9p imposed limitation. The
> right thing to do would be to come up with a new version of the protocol
> with variable sized QIDs and call it a day.
Yes, but that's the long-term solution which will still take a very long
while. This patch set is already a functional solution until that happens, and
this 9p issue is IMO quite severe (as minor as data corruption, data loss and
exists for several years already).
> > plus it completely destroys the fundamental idea about 9p, which is about
> > transparency of the higher level(s).
>
> That's a point indeed, even if again I'm not sure if this is a frequent
> case to share a directory tree spanning over multiple devices.
When the use case is mass storage then it is very likely that you will have
several devices. Times have changed. With modern file systems like ZFS it is
very common to create a large amount of "data sets" for all kinds of
individual purposes and mount points which is cheap to get. Each fs data set
is a separate "device" from OS (i.e. Linux) point of view, even if all those
FS data sets are in the same FS pool and even on the same physical IO device.
Background: The concept of FS "data sets" combines the benefits of classical
partitions (e.g. logical file space separation, independent fs configurations
like compression on/off/algorithm, data deduplication on/off, snapshot
isolation, snapshots on/off) without the disadvantages of classical real
partitions (physical space is dynamically shared, no space wasted on fixed
boundaries; physical device pool management is transparent for all data sets,
configuration options can be inherited from parent data sets).
> I don't have that much time to spend on 9p maintainership, for both
> reviewing and fixing bugs (CVEs most of the time). So, yes it may
> sound like I want to drop the patchset, but it's just I need to be
> convinced I won't regret having merged a huge change like this...
> when I'll have to support it alone later ;-)
Actually I already assumed this to be the actual root cause.
I see that you are currently the only maintainer, and my plan was not to just
provide a one time shot but eventually hang along helping with maintaining it
due my use of 9p and its huge potential I see (as universal and efficient root
file system for all guests, not just for exotically sharing a small tree
portion between guests). I also have plans for supporting native file forks
BTW. But if you are seriously suggesting to simply omit a fundamental issue in
9p, then my plans would obviously no longer make sense. :)
I mean I am completely tolerant to all kinds of religious views on bit level,
languages, code style, libs, precise implementation details, parameters,
source structure, etc.; but saying to simply leave a fundamental bug open for
years to come, that's where I have to drop out.
> For the moment, I'm not convinced by the "vii" solution. It even
> motivated my suggestion of having several devices actually, since
> the paths you would put in there are known before starting QEMU.
Well, that "vii" configuration and the QID persistency are 2 optional patches
on top of the core fixes. It is a huge difference to suggest dropping these 2
patches than saying to completely drop the entire patch set and to leave this
bug open.
The mandatory core fixes that I see (for the short/mid term) are at least
Antonios' patches plus my variable length prefix patch, the latter significantly
reduces the inode numbers on guest and significantly increases the inode name
space, and hence also significanlty reduces the propability that 9p might ever
need to kick in with any kind of expensive remapping actions (or even
something worse like stat fatally returning -ENFILE to the user).
About the "vii" configuration, even though you don't like the idea: there is
also a big difference giving the user the _optional_ possibility to define e.g.
one path (not device) on guest said to be sensitive regarding high inode
numbers on guest; and something completely different telling the user that he
_must_ configure every single device from host that is ever supposed to pop up
with 9p on guest and forcing the user to update that configuration whenever a
new device is added or removed on host. The "vii" configuration feature does
not require any knowledge of how many and which kinds of devices are actually
ever used on host (nor on any higher level host in case of nested
virtualization), nor does that "vii" config require any changes ever when host
device setup changes. So 9p's core transparency feature would not be touched
at all.
> It might take me some more rounds of discussion to decide. I understand
> it is frustrating but bear with me :)
Let me make a different suggestion: how about putting these fixes into a
separate C unit for now and making the default behaviour (if you really want)
to not use any of that code by default at all. So the user would just get an
error message in the qemu log files by default if he tries to export several
devices with one 9p device, suggesting him either to map them as separate 9p
devices (like you suggested) and informing the user about the alternative of
enabling support for the automatic inode remapping code (from that separate C
unit) instead by adding one convenient config option if he/she really wants.
That way we would have a fix now, not in years, people can decide to use the
automatic and hardware transparent solution, instead of being forced to write
dozens of config lines for each single guest, or they might decide to stick
with your "solution" ;-).
And once the long term solution for this issue with variable length QIDs is in
place, the inode remapping code can very simply be dropped from the code base
completley by just deleting the C unit and about a hand full of lines in 9p.c
or so, and hence this fix would not bloat the existing 9p units nor cause
maintenance nightmares of any kind.
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-17 20:53 ` Christian Schoenebeck via Qemu-devel
@ 2019-05-20 14:05 ` Greg Kurz
2019-05-22 16:03 ` Christian Schoenebeck via Qemu-devel
0 siblings, 1 reply; 26+ messages in thread
From: Greg Kurz @ 2019-05-20 14:05 UTC (permalink / raw)
To: Christian Schoenebeck
Cc: Daniel P. Berrangé, qemu-devel, Antonios Motakis
Hi Christian,
On Fri, 17 May 2019 22:53:41 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> On Freitag, 17. Mai 2019 16:47:46 CEST Greg Kurz wrote:
> > Potentially yes if another approach is satisfying enough, as I wouldn't
> > want to over-engineer too much around this 9p imposed limitation. The
> > right thing to do would be to come up with a new version of the protocol
> > with variable sized QIDs and call it a day.
>
> Yes, but that's the long-term solution which will still take a very long
> while. This patch set is already a functional solution until that happens, and
> this 9p issue is IMO quite severe (as minor as data corruption, data loss and
> exists for several years already).
>
On the other hand, I'm afraid that having a functional solution won't
motivate people to come up with a new spec... Anyway, I agree that the
data corruption/loss issues must be prevented, ie, the 9p server should
at least return an error to the client rather than returning a colliding
QID. A simple way to avoid that is to enforce a single device, ie. patch
2 in Antonios's series. Of course this may break some setups where
multiple devices are involved, but it is pure luck if they aren't already
broken with the current code base. I assume that this covers most 9p users,
based on the fact that Antonios and now yourself are the only people who
ever reported this issue. Then, I'm okay if we try to cover some more
scenarios, but it must take the maintainance burden into account.
> > > plus it completely destroys the fundamental idea about 9p, which is about
> > > transparency of the higher level(s).
> >
> > That's a point indeed, even if again I'm not sure if this is a frequent
> > case to share a directory tree spanning over multiple devices.
>
> When the use case is mass storage then it is very likely that you will have
> several devices. Times have changed. With modern file systems like ZFS it is
> very common to create a large amount of "data sets" for all kinds of
> individual purposes and mount points which is cheap to get. Each fs data set
> is a separate "device" from OS (i.e. Linux) point of view, even if all those
> FS data sets are in the same FS pool and even on the same physical IO device.
>
> Background: The concept of FS "data sets" combines the benefits of classical
> partitions (e.g. logical file space separation, independent fs configurations
> like compression on/off/algorithm, data deduplication on/off, snapshot
> isolation, snapshots on/off) without the disadvantages of classical real
> partitions (physical space is dynamically shared, no space wasted on fixed
> boundaries; physical device pool management is transparent for all data sets,
> configuration options can be inherited from parent data sets).
>
Ok. I must admit my ignorance around ZFS and "data sets"... so IIUC, even with
a single underlying physical device you might end up with lstat() returning
different st_dev values on the associated files, correct ?
I take your word for the likeliness of the issue to popup in such setups. :)
> > I don't have that much time to spend on 9p maintainership, for both
> > reviewing and fixing bugs (CVEs most of the time). So, yes it may
> > sound like I want to drop the patchset, but it's just I need to be
> > convinced I won't regret having merged a huge change like this...
> > when I'll have to support it alone later ;-)
>
> Actually I already assumed this to be the actual root cause.
>
> I see that you are currently the only maintainer, and my plan was not to just
> provide a one time shot but eventually hang along helping with maintaining it
> due my use of 9p and its huge potential I see (as universal and efficient root
> file system for all guests, not just for exotically sharing a small tree
> portion between guests). I also have plans for supporting native file forks
What are "native file forks" ?
> BTW. But if you are seriously suggesting to simply omit a fundamental issue in
> 9p, then my plans would obviously no longer make sense. :)
>
It would be very much appreciated to have someone else investing time in 9p
of course. :)
> I mean I am completely tolerant to all kinds of religious views on bit level,
> languages, code style, libs, precise implementation details, parameters,
> source structure, etc.; but saying to simply leave a fundamental bug open for
> years to come, that's where I have to drop out.
>
I guess I was unclear... As said above, I do want to avoid QID collisions,
at least by enforcing a single device per shared path. Then, supporting
multiple devices will depend on the benefice/expense ratio.
> > For the moment, I'm not convinced by the "vii" solution. It even
> > motivated my suggestion of having several devices actually, since
> > the paths you would put in there are known before starting QEMU.
>
> Well, that "vii" configuration and the QID persistency are 2 optional patches
> on top of the core fixes. It is a huge difference to suggest dropping these 2
> patches than saying to completely drop the entire patch set and to leave this
> bug open.
>
> The mandatory core fixes that I see (for the short/mid term) are at least
> Antonios' patches plus my variable length prefix patch, the latter significantly
> reduces the inode numbers on guest and significantly increases the inode name
> space, and hence also significanlty reduces the propability that 9p might ever
> need to kick in with any kind of expensive remapping actions (or even
> something worse like stat fatally returning -ENFILE to the user).
>
> About the "vii" configuration, even though you don't like the idea: there is
Hmm... I was objecting on the fact that "vii" would cope with runtime
changes on the host while the guest is running.
> also a big difference giving the user the _optional_ possibility to define e.g.
> one path (not device) on guest said to be sensitive regarding high inode
> numbers on guest; and something completely different telling the user that he
> _must_ configure every single device from host that is ever supposed to pop up
> with 9p on guest and forcing the user to update that configuration whenever a
> new device is added or removed on host. The "vii" configuration feature does
> not require any knowledge of how many and which kinds of devices are actually
> ever used on host (nor on any higher level host in case of nested
> virtualization), nor does that "vii" config require any changes ever when host
> device setup changes. So 9p's core transparency feature would not be touched
> at all.
>
I guess this all boils down to I finding some time to read/understand more :)
As usual, a series with smaller and simpler patches will be easier to
review, and more likely to be merged.
> > It might take me some more rounds of discussion to decide. I understand
> > it is frustrating but bear with me :)
>
> Let me make a different suggestion: how about putting these fixes into a
> separate C unit for now and making the default behaviour (if you really want)
> to not use any of that code by default at all. So the user would just get an
> error message in the qemu log files by default if he tries to export several
> devices with one 9p device, suggesting him either to map them as separate 9p
> devices (like you suggested) and informing the user about the alternative of
> enabling support for the automatic inode remapping code (from that separate C
> unit) instead by adding one convenient config option if he/she really wants.
>
It seems that we may be reaching some consensus here :)
I like the approach, provided this is configurable from the command line, off
by default and doesn't duplicate core 9p code. Not sure this needs to sit in
its own C unit though.
> That way we would have a fix now, not in years, people can decide to use the
> automatic and hardware transparent solution, instead of being forced to write
> dozens of config lines for each single guest, or they might decide to stick
> with your "solution" ;-).
>
> And once the long term solution for this issue with variable length QIDs is in
> place, the inode remapping code can very simply be dropped from the code base
> completley by just deleting the C unit and about a hand full of lines in 9p.c
> or so, and hence this fix would not bloat the existing 9p units nor cause
> maintenance nightmares of any kind.
>
The 9p code has a long history of CVEs and limitations that prevented it
to reach full production grade quality. Combined with the poor quality of
the code, this has scared off many experienced QEMU developpers, which
prefer to work on finding an alternative solution. Such alternative is
virtio-fs, which is being actively worked on:
https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02746.html
Note: I'm not telling "stay away from 9p" but maybe worth taking a look,
because if virtio-fs gets merged, it is likely to become the official
and better supported way to share files between host and guest.
> Best regards,
> Christian Schoenebeck
Please repost a series, possibly based on some of Antonios's patches that
allows to avoid the QID collision, returns an error to the client instead
and possibly printing out some useful messages in the QEMU log. Then, on
top of that, you can start introducing hashing and variable prefix length.
Cheers,
--
Greg
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-20 14:05 ` Greg Kurz
@ 2019-05-22 16:03 ` Christian Schoenebeck via Qemu-devel
2019-06-03 6:57 ` Greg Kurz
0 siblings, 1 reply; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-05-22 16:03 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Christian Schoenebeck, Greg Kurz, Antonios Motakis
On Montag, 20. Mai 2019 16:05:09 CEST Greg Kurz wrote:
> Hi Christian,
Hi Greg,
> On the other hand, I'm afraid that having a functional solution won't
> motivate people to come up with a new spec... Anyway, I agree that the
> data corruption/loss issues must be prevented, ie, the 9p server should
> at least return an error to the client rather than returning a colliding
> QID.
Ok, I will extend Antonios' patch to log that error on host. I thought about
limiting that error message to once per session (for not flooding the logs),
but it is probably not worth it, so if you don't veto then I will just log
that error simply on every file access.
> A simple way to avoid that is to enforce a single device, ie. patch
> 2 in Antonios's series. Of course this may break some setups where
> multiple devices are involved, but it is pure luck if they aren't already
> broken with the current code base.
Yes, the worst thing you can have is this collision silently being ignored,
like it is actually right now. Because you end up getting all sorts of
different misbehaviours on guests, and these are just symptoms that take a
while to debug and realise that the actual root cause is an inode collision.
So enforcing a single device is still better than undefined behaviour.
> > Background: The concept of FS "data sets" combines the benefits of
> > classical partitions (e.g. logical file space separation, independent fs
> > configurations like compression on/off/algorithm, data deduplication
> > on/off, snapshot isolation, snapshots on/off) without the disadvantages
> > of classical real partitions (physical space is dynamically shared, no
> > space wasted on fixed boundaries; physical device pool management is
> > transparent for all data sets, configuration options can be inherited
> > from parent data sets).
>
> Ok. I must admit my ignorance around ZFS and "data sets"... so IIUC, even
> with a single underlying physical device you might end up with lstat()
> returning different st_dev values on the associated files, correct ?
>
> I take your word for the likeliness of the issue to popup in such setups. :)
Yes, that is correct, you _always_ get a different stat::st_dev value for each
ZFS data set. Furthermore, each ZFS data set has its own inode number sequence
generator starting from one. So consider you create two new ZFS data sets,
then you create one file on each data set, then both files will have inode
number 1.
That probably makes it clear why you hit this ID collision bug very easily
when using the combination ZFS & 9p.
> > also a big difference giving the user the _optional_ possibility to define
> > e.g. one path (not device) on guest said to be sensitive regarding high
> > inode numbers on guest; and something completely different telling the
> > user that he _must_ configure every single device from host that is ever
> > supposed to pop up with 9p on guest and forcing the user to update that
> > configuration whenever a new device is added or removed on host. The
> > "vii" configuration feature does not require any knowledge of how many
> > and which kinds of devices are actually ever used on host (nor on any
> > higher level host in case of nested
> > virtualization), nor does that "vii" config require any changes ever when
> > host device setup changes. So 9p's core transparency feature would not be
> > touched at all.
>
> I guess this all boils down to I finding some time to read/understand more
> :)
Yes, that helps sometimes. :)
> As usual, a series with smaller and simpler patches will be easier to
> review, and more likely to be merged.
Of course.
In the next patch series I will completely drop a) the entire QID persistency
feature code and b) that disputed "vii" feature. But I will still suggest the
variable inode suffix length patch as last patch in that new patch series.
That should make the amount of changes manageable small.
> > Let me make a different suggestion: how about putting these fixes into a
> > separate C unit for now and making the default behaviour (if you really
> > want) to not use any of that code by default at all. So the user would
> > just get an error message in the qemu log files by default if he tries to
> > export several devices with one 9p device, suggesting him either to map
> > them as separate 9p devices (like you suggested) and informing the user
> > about the alternative of enabling support for the automatic inode
> > remapping code (from that separate C unit) instead by adding one
> > convenient config option if he/she really wants.
> It seems that we may be reaching some consensus here :)
>
> I like the approach, provided this is configurable from the command line,
> off by default and doesn't duplicate core 9p code. Not sure this needs to
> sit in its own C unit though.
Any preference for a command line argument name and/or libvirt XML config tag/
attribute for switching the inode remapping code on?
About that separate C unit: I leave that up to you to decide, it does not
matter to me. I just suggested it since you consider these patches as
temporary workaround until there are appropriate protocol changes. So clear
code separation for them might help to get rid of the entire code later on.
Plus for distribution maintainers it might be easiert to cherry pick them as
backports.
However since I will drop the persistency and "vii" feature in the next patch
series, it probably does not make a huge difference anyway. As you prefer.
> The 9p code has a long history of CVEs and limitations that prevented it
> to reach full production grade quality. Combined with the poor quality of
> the code, this has scared off many experienced QEMU developpers, which
> prefer to work on finding an alternative solution.
And I already wondered about the obvious low activity on this particular qemu
feature. I mean I don't find it contemporary still running guests to use their
own file system being emulated on a file ontop of yet another file system and
loosing essentially all benefits of the host's actual backend file system
features.
> Such alternative is virtio-fs, which is being actively worked on:
>
> https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02746.html
>
> Note: I'm not telling "stay away from 9p" but maybe worth taking a look,
> because if virtio-fs gets merged, it is likely to become the official
> and better supported way to share files between host and guest.
Ah, good to know! That's new to me, thanks!
Makes sense to me, especially its performance will certainly be much better.
> Please repost a series, possibly based on some of Antonios's patches that
> allows to avoid the QID collision, returns an error to the client instead
> and possibly printing out some useful messages in the QEMU log. Then, on
> top of that, you can start introducing hashing and variable prefix length.
So you want that as its own patch series first, or can I continue with my
suggestion to deliver the hash patch and variable suffix length patch as last
patches within the same series?
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-05-22 16:03 ` Christian Schoenebeck via Qemu-devel
@ 2019-06-03 6:57 ` Greg Kurz
2019-06-03 9:17 ` Christian Schoenebeck via Qemu-devel
0 siblings, 1 reply; 26+ messages in thread
From: Greg Kurz @ 2019-06-03 6:57 UTC (permalink / raw)
To: Christian Schoenebeck
Cc: Daniel P. Berrangé, qemu-devel, Antonios Motakis
On Wed, 22 May 2019 18:03:13 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> On Montag, 20. Mai 2019 16:05:09 CEST Greg Kurz wrote:
> > Hi Christian,
>
> Hi Greg,
>
> > On the other hand, I'm afraid that having a functional solution won't
> > motivate people to come up with a new spec... Anyway, I agree that the
> > data corruption/loss issues must be prevented, ie, the 9p server should
> > at least return an error to the client rather than returning a colliding
> > QID.
>
> Ok, I will extend Antonios' patch to log that error on host. I thought about
> limiting that error message to once per session (for not flooding the logs),
> but it is probably not worth it, so if you don't veto then I will just log
> that error simply on every file access.
>
Please use error_report_once().
> > A simple way to avoid that is to enforce a single device, ie. patch
> > 2 in Antonios's series. Of course this may break some setups where
> > multiple devices are involved, but it is pure luck if they aren't already
> > broken with the current code base.
>
> Yes, the worst thing you can have is this collision silently being ignored,
> like it is actually right now. Because you end up getting all sorts of
> different misbehaviours on guests, and these are just symptoms that take a
> while to debug and realise that the actual root cause is an inode collision.
> So enforcing a single device is still better than undefined behaviour.
>
> > > Background: The concept of FS "data sets" combines the benefits of
> > > classical partitions (e.g. logical file space separation, independent fs
> > > configurations like compression on/off/algorithm, data deduplication
> > > on/off, snapshot isolation, snapshots on/off) without the disadvantages
> > > of classical real partitions (physical space is dynamically shared, no
> > > space wasted on fixed boundaries; physical device pool management is
> > > transparent for all data sets, configuration options can be inherited
> > > from parent data sets).
> >
> > Ok. I must admit my ignorance around ZFS and "data sets"... so IIUC, even
> > with a single underlying physical device you might end up with lstat()
> > returning different st_dev values on the associated files, correct ?
> >
> > I take your word for the likeliness of the issue to popup in such setups. :)
>
> Yes, that is correct, you _always_ get a different stat::st_dev value for each
> ZFS data set. Furthermore, each ZFS data set has its own inode number sequence
> generator starting from one. So consider you create two new ZFS data sets,
> then you create one file on each data set, then both files will have inode
> number 1.
>
> That probably makes it clear why you hit this ID collision bug very easily
> when using the combination ZFS & 9p.
>
> > > also a big difference giving the user the _optional_ possibility to define
> > > e.g. one path (not device) on guest said to be sensitive regarding high
> > > inode numbers on guest; and something completely different telling the
> > > user that he _must_ configure every single device from host that is ever
> > > supposed to pop up with 9p on guest and forcing the user to update that
> > > configuration whenever a new device is added or removed on host. The
> > > "vii" configuration feature does not require any knowledge of how many
> > > and which kinds of devices are actually ever used on host (nor on any
> > > higher level host in case of nested
> > > virtualization), nor does that "vii" config require any changes ever when
> > > host device setup changes. So 9p's core transparency feature would not be
> > > touched at all.
> >
> > I guess this all boils down to I finding some time to read/understand more
> > :)
>
> Yes, that helps sometimes. :)
>
> > As usual, a series with smaller and simpler patches will be easier to
> > review, and more likely to be merged.
>
> Of course.
>
> In the next patch series I will completely drop a) the entire QID persistency
> feature code and b) that disputed "vii" feature. But I will still suggest the
> variable inode suffix length patch as last patch in that new patch series.
>
> That should make the amount of changes manageable small.
>
> > > Let me make a different suggestion: how about putting these fixes into a
> > > separate C unit for now and making the default behaviour (if you really
> > > want) to not use any of that code by default at all. So the user would
> > > just get an error message in the qemu log files by default if he tries to
> > > export several devices with one 9p device, suggesting him either to map
> > > them as separate 9p devices (like you suggested) and informing the user
> > > about the alternative of enabling support for the automatic inode
> > > remapping code (from that separate C unit) instead by adding one
> > > convenient config option if he/she really wants.
> > It seems that we may be reaching some consensus here :)
> >
> > I like the approach, provided this is configurable from the command line,
> > off by default and doesn't duplicate core 9p code. Not sure this needs to
> > sit in its own C unit though.
>
> Any preference for a command line argument name and/or libvirt XML config tag/
> attribute for switching the inode remapping code on?
>
> About that separate C unit: I leave that up to you to decide, it does not
> matter to me. I just suggested it since you consider these patches as
> temporary workaround until there are appropriate protocol changes. So clear
> code separation for them might help to get rid of the entire code later on.
> Plus for distribution maintainers it might be easiert to cherry pick them as
> backports.
>
> However since I will drop the persistency and "vii" feature in the next patch
> series, it probably does not make a huge difference anyway. As you prefer.
>
> > The 9p code has a long history of CVEs and limitations that prevented it
> > to reach full production grade quality. Combined with the poor quality of
> > the code, this has scared off many experienced QEMU developpers, which
> > prefer to work on finding an alternative solution.
>
> And I already wondered about the obvious low activity on this particular qemu
> feature. I mean I don't find it contemporary still running guests to use their
> own file system being emulated on a file ontop of yet another file system and
> loosing essentially all benefits of the host's actual backend file system
> features.
>
> > Such alternative is virtio-fs, which is being actively worked on:
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02746.html
> >
> > Note: I'm not telling "stay away from 9p" but maybe worth taking a look,
> > because if virtio-fs gets merged, it is likely to become the official
> > and better supported way to share files between host and guest.
>
> Ah, good to know! That's new to me, thanks!
>
> Makes sense to me, especially its performance will certainly be much better.
>
> > Please repost a series, possibly based on some of Antonios's patches that
> > allows to avoid the QID collision, returns an error to the client instead
> > and possibly printing out some useful messages in the QEMU log. Then, on
> > top of that, you can start introducing hashing and variable prefix length.
>
> So you want that as its own patch series first, or can I continue with my
> suggestion to deliver the hash patch and variable suffix length patch as last
> patches within the same series?
>
Same series is okay.
> Best regards,
> Christian Schoenebeck
Cheers,
--
Greg
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii'
2019-06-03 6:57 ` Greg Kurz
@ 2019-06-03 9:17 ` Christian Schoenebeck via Qemu-devel
0 siblings, 0 replies; 26+ messages in thread
From: Christian Schoenebeck via Qemu-devel @ 2019-06-03 9:17 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Christian Schoenebeck, Greg Kurz, Antonios Motakis
On Montag, 3. Juni 2019 08:57:15 CEST Greg Kurz wrote:
> > Ok, I will extend Antonios' patch to log that error on host. I thought
> > about limiting that error message to once per session (for not flooding
> > the logs), but it is probably not worth it, so if you don't veto then I
> > will just log that error simply on every file access.
>
> Please use error_report_once().
I will.
> > > Please repost a series, possibly based on some of Antonios's patches
> > > that
> > > allows to avoid the QID collision, returns an error to the client
> > > instead
> > > and possibly printing out some useful messages in the QEMU log. Then, on
> > > top of that, you can start introducing hashing and variable prefix
> > > length.
> >
> > So you want that as its own patch series first, or can I continue with my
> > suggestion to deliver the hash patch and variable suffix length patch as
> > last patches within the same series?
>
> Same series is okay.
Ok.
I'm currently busy with other work; I will probably send a new patch set
approximately next week.
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-06-03 9:29 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-06 14:37 [Qemu-devel] [PATCH v3 0/5] 9p: Fix file ID collisions Christian Schoenebeck via Qemu-devel
2019-04-23 11:30 ` [Qemu-devel] [PATCH v3 1/5] 9p: mitigates most QID path collisions Christian Schoenebeck via Qemu-devel
2019-05-07 12:42 ` Daniel P. Berrangé
2019-05-07 13:11 ` Christian Schoenebeck via Qemu-devel
2019-05-07 13:13 ` Daniel P. Berrangé
2019-04-23 11:35 ` [Qemu-devel] [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation Christian Schoenebeck via Qemu-devel
2019-05-07 12:43 ` Daniel P. Berrangé
2019-04-23 11:41 ` [Qemu-devel] [PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions Christian Schoenebeck via Qemu-devel
2019-05-03 14:51 ` [Qemu-devel] [PATCH v3 4/5] 9p: use variable length suffixes for inode mapping Christian Schoenebeck via Qemu-devel
2019-05-05 21:41 ` [Qemu-devel] [PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter Christian Schoenebeck via Qemu-devel
2019-05-06 17:58 ` [Qemu-devel] [libvirt patch] qemu: adds support for virtfs 9p argument 'vii' Christian Schoenebeck via Qemu-devel
2019-05-07 9:55 ` Greg Kurz
2019-05-07 12:23 ` Christian Schoenebeck via Qemu-devel
2019-05-07 15:42 ` Greg Kurz
2019-05-07 16:16 ` Christian Schoenebeck via Qemu-devel
2019-05-17 8:40 ` Christian Schoenebeck via Qemu-devel
2019-05-17 12:30 ` Greg Kurz
2019-05-17 13:23 ` Christian Schoenebeck via Qemu-devel
2019-05-17 14:47 ` Greg Kurz
2019-05-17 20:53 ` Christian Schoenebeck via Qemu-devel
2019-05-20 14:05 ` Greg Kurz
2019-05-22 16:03 ` Christian Schoenebeck via Qemu-devel
2019-06-03 6:57 ` Greg Kurz
2019-06-03 9:17 ` Christian Schoenebeck via Qemu-devel
2019-05-07 12:57 ` Daniel P. Berrangé
2019-05-07 13:48 ` Christian Schoenebeck via Qemu-devel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.