diff mbox series

[v4,4/5] 9p: stat_to_qid: implement slow path

Message ID fa636e323b8644469f9b03619758b6c1c6d1d4f2.1561575449.git.qemu_oss@crudebyte.com (mailing list archive)
State New, archived
Headers show
Series 9p: Fix file ID collisions | expand

Commit Message

Denis V. Lunev" via June 26, 2019, 6:46 p.m. UTC 
stat_to_qid attempts via qid_path_prefixmap to map unique files (which are
identified by 64 bit inode nr and 32 bit device id) to a 64 QID path value.
However this implementation makes some assumptions about inode number
generation on the host.

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

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/9pfs/9p.h |  9 +++++++++
 2 files changed, 67 insertions(+), 5 deletions(-)

Comments

Greg Kurz June 28, 2019, 10:21 a.m. UTC | #1 
On Wed, 26 Jun 2019 20:46:24 +0200
Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> 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 <antonios.motakis@huawei.com>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  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 for this series.

LGTM.

>      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 */
Denis V. Lunev" via June 28, 2019, 2:03 p.m. UTC | #2 
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
diff mbox series

Patch

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 */