Re: [Qemu-devel] [PATCH v4 4/5] 9p: stat_to_qid: implement slow path
On Freitag, 28. Juni 2019 12:21:20 CEST Greg Kurz wrote:
> > +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_lookup_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 :'( */
>
> This would be the place to put the error_report_once() suggested
> in the previous patch actually.
I will add the suggested error message to qid_path_prefixmap() in patch 3 and
then will move over that error message to qid_path_fullmap() in patch 4.
Or if you want I can also leave an error_report_once() in qid_path_prefixmap()
in patch 4 about potential degraded performance.
> > @@ -3779,7 +3831,8 @@ void v9fs_device_unrealize_common(V9fsState *s,
> > Error **errp)>
> > }
> > fsdev_throttle_cleanup(s->ctx.fst);
> > g_free(s->tag);
> >
> > -qpp_table_destroy(&s->qpp_table);
> > +qp_table_destroy(&s->qpp_table);
> > +qp_table_destroy(&s->qpf_table);
>
> I'm starting to think v9fs_device_unrealize_common() should be made
> idempotent, so that it can be used to handle rollback of a partially
> realized device, and thus avoid the code duplication. But this is
> out-of-scope for this series.
Well, I can also make that e.g.:
if (s->qpf_table.map)
qp_table_destroy(&s->qpf_table);
if you prefer the occurrence amount to be reduced.
Best regards,
Christian Schoenebeck
Re: [Qemu-devel] [PATCH v4 4/5] 9p: stat_to_qid: implement slow path
On Wed, 26 Jun 2019 20:46:24 +0200
Christian Schoenebeck via Qemu-devel wrote:
> stat_to_qid attempts via qid_path_prefixmap to map unique files (which are
> identified by 64 bit inode nr and 32 bit device id) to a 64 QID path value.
> However this implementation makes some assumptions about inode number
> generation on the host.
>
> If qid_path_prefixmap fails, we still have 48 bits available in the QID
> path to fall back to a less memory efficient full mapping.
>
> Signed-off-by: Antonios Motakis
> Signed-off-by: Christian Schoenebeck
> ---
> hw/9pfs/9p.c | 63
> +++-
> hw/9pfs/9p.h | 9 +
> 2 files changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 7ccc68a829..e6e410972f 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -579,23 +579,69 @@ static uint32_t qpp_hash(QppEntry e)
> return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
> }
>
> +static uint32_t qpf_hash(QpfEntry e)
> +{
> +return qemu_xxhash7(e.ino, e.dev, 0, 0, 0);
> +}
> +
> static bool qpp_lookup_func(const void *obj, const void *userp)
> {
> const QppEntry *e1 = obj, *e2 = userp;
> return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
> }
>
> -static void qpp_table_remove(void *p, uint32_t h, void *up)
> +static bool qpf_lookup_func(const void *obj, const void *userp)
> +{
> +const QpfEntry *e1 = obj, *e2 = userp;
> +return e1->dev == e2->dev && e1->ino == e2->ino;
> +}
> +
> +static void qp_table_remove(void *p, uint32_t h, void *up)
> {
> g_free(p);
> }
>
> -static void qpp_table_destroy(struct qht *ht)
> +static void qp_table_destroy(struct qht *ht)
> {
> -qht_iter(ht, qpp_table_remove, NULL);
> +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_lookup_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 :'( */
This would be the place to put the error_report_once() suggested
in the previous patch actually.
> +return -ENFILE;
> +}
> +
> +val = g_malloc0(sizeof(QppEntry));
> +*val = lookup;
> +
> +/* new unique inode and device combo */
> +val->path = pdu->s->qp_fullpath_next++;
> +pdu->s->qp_fullpath_next &= QPATH_INO_MASK;
> +qht_insert(&pdu->s->qpf_table, val, hash, NULL);
> +}
> +
> +*path = val->path;
> +return 0;
> +}
> +
> /* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
> * to a unique QID path (64 bits). To avoid having to map and keep track
> * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
> @@ -642,6 +688,10 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat
> *stbuf, V9fsQID *qidp)
> if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
> /* map inode+device to qid path (fast path) */
> err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
> +if (err == -ENFILE) {
> +/* fast path didn't work, fall back to full map */
> +err = qid_path_fullmap(pdu, stbuf, &qidp->path);
> +}
> if (err) {
> return err;
> }
> @@ -3752,6 +3802,7 @@ int v9fs_device_realize_common(V9fsState *s, const
> V9fsTransport *t,
> /* QID path hash table. 1 entry ought to be enough for anybody ;) */
> qht_init(&s->qpp_table, qpp_lookup_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);
> @@ -3766,7 +3817,8 @@ out:
> }
> g_free(s->tag);
> g_free(s->ctx.fs_root);
> -qpp_table_destroy(&s->qpp_table);
> +qp_table_destroy(&s->qpp_table);
> +qp_table_destroy(&s->qpf_table);
> v9fs_path_free(&path);
> }
> return rc;
> @@ -3779,7 +3831,8 @@ void v9fs_device_unrealize_common(V9fsState *s, Error
> **errp)
> }
> fsdev_throttle_cleanup(s->ctx.fst);
> g_free(s->tag);
> -qpp_table_destroy(&s->qpp_table);
> +qp_table_destroy(&s->qpp_table);
> +qp_table_destroy(&s->qpf_table);
I'm starting to think v9fs_device_unrealize_common() should be made
idempotent, so that it can be used to handle rollback of a partially
realized device, and thus avoid the code duplication. But this is
out-of-scope fo
[Qemu-devel] [PATCH v4 4/5] 9p: stat_to_qid: implement slow path
stat_to_qid attempts via qid_path_prefixmap to map unique files (which are
identified by 64 bit inode nr and 32 bit device id) to a 64 QID path value.
However this implementation makes some assumptions about inode number
generation on the host.
If qid_path_prefixmap fails, we still have 48 bits available in the QID
path to fall back to a less memory efficient full mapping.
Signed-off-by: Antonios Motakis
Signed-off-by: Christian Schoenebeck
---
hw/9pfs/9p.c | 63 +++-
hw/9pfs/9p.h | 9 +
2 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 7ccc68a829..e6e410972f 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -579,23 +579,69 @@ static uint32_t qpp_hash(QppEntry e)
return qemu_xxhash7(e.ino_prefix, e.dev, 0, 0, 0);
}
+static uint32_t qpf_hash(QpfEntry e)
+{
+return qemu_xxhash7(e.ino, e.dev, 0, 0, 0);
+}
+
static bool qpp_lookup_func(const void *obj, const void *userp)
{
const QppEntry *e1 = obj, *e2 = userp;
return e1->dev == e2->dev && e1->ino_prefix == e2->ino_prefix;
}
-static void qpp_table_remove(void *p, uint32_t h, void *up)
+static bool qpf_lookup_func(const void *obj, const void *userp)
+{
+const QpfEntry *e1 = obj, *e2 = userp;
+return e1->dev == e2->dev && e1->ino == e2->ino;
+}
+
+static void qp_table_remove(void *p, uint32_t h, void *up)
{
g_free(p);
}
-static void qpp_table_destroy(struct qht *ht)
+static void qp_table_destroy(struct qht *ht)
{
-qht_iter(ht, qpp_table_remove, NULL);
+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_lookup_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));
+*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
@@ -642,6 +688,10 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat
*stbuf, V9fsQID *qidp)
if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
/* map inode+device to qid path (fast path) */
err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+if (err == -ENFILE) {
+/* fast path didn't work, fall back to full map */
+err = qid_path_fullmap(pdu, stbuf, &qidp->path);
+}
if (err) {
return err;
}
@@ -3752,6 +3802,7 @@ int v9fs_device_realize_common(V9fsState *s, const
V9fsTransport *t,
/* QID path hash table. 1 entry ought to be enough for anybody ;) */
qht_init(&s->qpp_table, qpp_lookup_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);
@@ -3766,7 +3817,8 @@ out:
}
g_free(s->tag);
g_free(s->ctx.fs_root);
-qpp_table_destroy(&s->qpp_table);
+qp_table_destroy(&s->qpp_table);
+qp_table_destroy(&s->qpf_table);
v9fs_path_free(&path);
}
return rc;
@@ -3779,7 +3831,8 @@ void v9fs_device_unrealize_common(V9fsState *s, Error
**errp)
}
fsdev_throttle_cleanup(s->ctx.fst);
g_free(s->tag);
-qpp_table_destroy(&s->qpp_table);
+qp_table_destroy(&s->qpp_table);
+qp_table_destroy(&s->qpf_table);
g_free(s->ctx.fs_root);
}
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 0200e04176..2b74561030 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -245,6 +245,13 @@ typedef struct {
uint16_t qp_prefix;
} QppEntry;
+/* QID path full entry, as above */
+typedef struct {
+dev_t dev;
+ino_t ino;
+uint64_t path;
+} QpfEntry;
+
struct V9fsState
{
QLIST_HEAD(, V9fsPDU) free_list;
@@ -268,7 +275,9 @@ struct V9fsState
V9fsQID root_qid;
dev_t dev_id;
struct qht qpp_table;
+struct qht qpf_table;
uint16_t qp_prefix_next;
+uint64_t qp_fullpath_next;
};
/* 9p2000.L open flags */
--
2.11.0
