: Use GLib threadpool in 9pfs.

* Arun R Bharadwaj <[email protected]> [2011-03-15 16:04:53]:

Author: Arun R Bharadwaj <[email protected]>
Date:   Thu Mar 10 14:45:25 2011 +0530

   Move the paio_signal_handler to a generic location.

   The paio subsystem uses the signal, SIGUSR2. So move the signal handler to a more generic place such that other subsystems like 9pfs can also use it.

   TODO: I have moved the signal handler code to qemu-thread.c, which is NOT the right place. I need suggestions as to where is the right place to put it.

Quoted 11 lines · Arun R Bharadwaj

* Arun R Bharadwaj <[email protected]> [2011-03-15 16:04:53]:

Author: Arun R Bharadwaj <[email protected]>
Date:   Thu Mar 10 14:45:25 2011 +0530

   Move the paio_signal_handler to a generic location.

   The paio subsystem uses the signal, SIGUSR2. So move the signal handler to a more generic place such that other subsystems like 9pfs can also use it.

Quoted 3 lines

   TODO: I have moved the signal handler code to qemu-thread.c, which is NOT the right place. I need suggestions as to where is the right place to put it.

I think os-posix.c would be appropriate. Please check how this affects Windows host and linux-user builds.

posix-aio-compat.c static void *aio_thread(void *unused) · line 362

Quoted 5 lines

posix-aio-compat.c +1 -0

@@ -356,6 +359,15 @@ static void *aio_thread(void *unused)
         idle_threads++;
         mutex_unlock(&lock);
 
+        if (posix_aio_state) {

If we get here posix_aio_state must be non-NULL. Please remove the check.

qemu-thread.c void qemu_thread_exit(void *retval) · line 153

Quoted 1 line

qemu-thread.c +1 -0

+void sigusr2_signal_handler(int signum)

static void sigusr2_signal_handler(int signum)

Stefan

* Arun R Bharadwaj<[email protected]> [2011-03-15 16:04:53]:

Author: Arun R Bharadwaj<[email protected]>
Date:   Thu Mar 10 15:11:49 2011 +0530

Helper routines to use GLib threadpool infrastructure in 9pfs.

This patch creates helper routines to make use of the threadpool infrastructure provided by GLib. This is based on the prototype patch by Anthony which does a similar thing for posix-aio-compat.c

An example use case is provided in the next patch where one of the syscalls in 9pfs is converted into the threaded model using these helper routines.

Signed-off-by Arun R Bharadwaj<[email protected]>

Reviewed-by Aneesh Kumar K.V<[email protected]>

hw/9pfs/virtio-9p.c +62 -0

index dceefd5..cf61345 100644
@@ -18,6 +18,8 @@
  #include "fsdev/qemu-fsdev.h"
  #include "virtio-9p-debug.h"
  #include "virtio-9p-xattr.h"
+#include "signal.h"
+#include "qemu-thread.h"
 
  int debug_9p_pdu;
  static void v9fs_reclaim_fd(V9fsState *s);

@@ -36,6 +38,89 @@ enum {
      Oappend = 0x80,
  };
 
+typedef struct V9fsPool {
+    GThreadPool *pool;
+    GList *requests;
+    int rfd;
+    int wfd;
+} V9fsPool;
+
+static V9fsPool v9fs_pool;
+
+static void v9fs_qemu_submit_request(V9fsRequest *req)
+{
+    V9fsPool *p =&v9fs_pool;
+
+    p->requests = g_list_append(p->requests, req);
+    g_thread_pool_push(v9fs_pool.pool, req, NULL);
+}
+
+static void die2(int err, const char *what)
+{
+    fprintf(stderr, "%s failed: %s\n", what, strerror(err));
+    abort();
+}
+
+static void die(const char *what)
+{
+    die2(errno, what);
+}
+
+static void v9fs_qemu_process_post_ops(void *arg)
+{
+    struct V9fsPool *p =&v9fs_pool;
+    struct V9fsPostOp *post_op;
+    char byte;
+    ssize_t len;
+    GList *cur_req, *next_req;
+
+    do {
+        len = read(p->rfd,&byte, sizeof(byte));
+    } while (len == -1&&  errno == EINTR);
+
+    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
+        V9fsRequest *req = cur_req->data;
+        next_req = g_list_next(cur_req);
+
+        if (!req->done) {
+            continue;
+        }
+
+        post_op =&req->post_op;
+        post_op->func(post_op->arg);
+        p->requests = g_list_remove_link(p->requests, cur_req);
+        g_list_free(p->requests);
+    }
+}
+
+static inline void v9fs_thread_signal(void)
+{
+    struct V9fsPool *p =&v9fs_pool;
+    char byte = 0;
+    ssize_t ret;

hw/9pfs/virtio-9p.c +9 -1

+}
+
  static int omode_to_uflags(int8_t mode)
  {
      int ret = 0;

@@ -3850,7 +3935,8 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
      int i, len;
      struct stat stat;
      FsTypeEntry *fse;
-
+    int fds[2];
+    V9fsPool *p =&v9fs_pool;
 
      s = (V9fsState *)virtio_common_init("virtio-9p",
                                      VIRTIO_ID_9P,

@@ -3939,5 +4025,21 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
                          s->tag_len;
      s->vdev.get_config = virtio_9p_get_config;
 
+    if (qemu_pipe(fds) == -1) {
+        fprintf(stderr, "failed to create fd's for virtio-9p\n");
+        exit(1);
+    }
+

hw/9pfs/virtio-9p.c +1 -0

+
      return&s->vdev;
  }

hw/9pfs/virtio-9p.h +14 -0

index 10809ba..e7d2326 100644
@@ -124,6 +124,20 @@ struct V9fsPDU
      QLIST_ENTRY(V9fsPDU) next;
  };
 
+typedef struct V9fsPostOp {
+    /* Post Operation routine to execute after executing syscall */
+    void (*func)(void *arg);
+    void *arg;
+} V9fsPostOp;
+
+typedef struct V9fsRequest {
+    void (*func)(struct V9fsRequest *req);
+
+    /* Flag to indicate that request is satisfied, ready for post-processing */
+    int done;
+
+    V9fsPostOp post_op;
+} V9fsRequest;
 
  /* FIXME
   * 1) change user needs to set groups and stuff

Quoted 101 lines · Arun R Bharadwaj

* Arun R Bharadwaj<[email protected]> [2011-03-15 16:04:53]:

Author: Arun R Bharadwaj<[email protected]>
Date:   Thu Mar 10 15:11:49 2011 +0530

Helper routines to use GLib threadpool infrastructure in 9pfs.

This patch creates helper routines to make use of the threadpool infrastructure provided by GLib. This is based on the prototype patch by Anthony which does a similar thing for posix-aio-compat.c

An example use case is provided in the next patch where one of the syscalls in 9pfs is converted into the threaded model using these helper routines.

Signed-off-by Arun R Bharadwaj<[email protected]>

Reviewed-by Aneesh Kumar K.V<[email protected]>

hw/9pfs/virtio-9p.c +67 -0

index dceefd5..cf61345 100644
@@ -18,6 +18,8 @@
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-debug.h"
 #include "virtio-9p-xattr.h"
+#include "signal.h"
+#include "qemu-thread.h"
 
 int debug_9p_pdu;
 static void v9fs_reclaim_fd(V9fsState *s);

@@ -36,6 +38,89 @@ enum {
     Oappend = 0x80,
 };
 
+typedef struct V9fsPool {
+    GThreadPool *pool;
+    GList *requests;
+    int rfd;
+    int wfd;
+} V9fsPool;
+
+static V9fsPool v9fs_pool;
+
+static void v9fs_qemu_submit_request(V9fsRequest *req)
+{
+    V9fsPool *p =&v9fs_pool;
+
+    p->requests = g_list_append(p->requests, req);
+    g_thread_pool_push(v9fs_pool.pool, req, NULL);
+}
+
+static void die2(int err, const char *what)
+{
+    fprintf(stderr, "%s failed: %s\n", what, strerror(err));
+    abort();
+}
+
+static void die(const char *what)
+{
+    die2(errno, what);
+}
+
+static void v9fs_qemu_process_post_ops(void *arg)
+{
+    struct V9fsPool *p =&v9fs_pool;
+    struct V9fsPostOp *post_op;
+    char byte;
+    ssize_t len;
+    GList *cur_req, *next_req;
+
+    do {
+        len = read(p->rfd,&byte, sizeof(byte));
+    } while (len == -1&&  errno == EINTR);
+
+    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
+        V9fsRequest *req = cur_req->data;
+        next_req = g_list_next(cur_req);
+
+        if (!req->done) {
+            continue;
+        }
+
+        post_op =&req->post_op;
+        post_op->func(post_op->arg);
+        p->requests = g_list_remove_link(p->requests, cur_req);
+        g_list_free(p->requests);
+    }
+}
+
+static inline void v9fs_thread_signal(void)
+{
+    struct V9fsPool *p =&v9fs_pool;
+    char byte = 0;
+    ssize_t ret;
+
+    do {
+        ret = write(p->wfd,&byte, sizeof(byte));
+    } while (ret == -1&&  errno == EINTR);
+

hw/9pfs/virtio-9p.c enum { · line 110

Quoted 5 lines

hw/9pfs/virtio-9p.c +5 -0

+    if (ret<  0&&  errno != EAGAIN) {
+        die("write() in v9fs");
+    }
+
+    if (kill(getpid(), SIGUSR2)) {

Not sure whether using pthread_kill or qemu_thread_signal is better to go with?

hw/9pfs/virtio-9p.c enum { · line 121

Quoted 11 lines

hw/9pfs/virtio-9p.c +11 -0

+        die("kill failed");
+    }
+}
+
+static void v9fs_thread_routine(gpointer data, gpointer user_data)
+{
+    V9fsRequest *req = data;
+
+    req->func(req);
+    v9fs_thread_signal();
+    req->done = 1;

Shouldn't it be in reverse order, setting flag first and then signal:

    req->done = 1;
    v9fs_thread_signal();

Quoted 28 lines

hw/9pfs/virtio-9p.c +13 -1

+}
+
 static int omode_to_uflags(int8_t mode)
 {
     int ret = 0;

@@ -3850,7 +3935,8 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
     int i, len;
     struct stat stat;
     FsTypeEntry *fse;
-
+    int fds[2];
+    V9fsPool *p =&v9fs_pool;
 
     s = (V9fsState *)virtio_common_init("virtio-9p",
                                     VIRTIO_ID_9P,

@@ -3939,5 +4025,21 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
                         s->tag_len;
     s->vdev.get_config = virtio_9p_get_config;
 
+    if (qemu_pipe(fds) == -1) {
+        fprintf(stderr, "failed to create fd's for virtio-9p\n");
+        exit(1);
+    }
+
+    p->pool = g_thread_pool_new(v9fs_thread_routine, p, 8, FALSE, NULL);
+    p->rfd = fds[0];
+    p->wfd = fds[1];
+

hw/9pfs/virtio-9p.c VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf) · line 4042

Quoted 6 lines

hw/9pfs/virtio-9p.c +6 -0

+    fcntl(p->rfd, F_SETFL, O_NONBLOCK);
+    fcntl(p->wfd, F_SETFL, O_NONBLOCK);
+
+    qemu_set_fd_handler(p->rfd, v9fs_qemu_process_post_ops, NULL, NULL);
+
+    (void) v9fs_qemu_submit_request;

Do we really need it ^ ?

- Harsh

Quoted 30 lines

hw/9pfs/virtio-9p.c +1 -0

+
     return&s->vdev;
 }

hw/9pfs/virtio-9p.h +14 -0

index 10809ba..e7d2326 100644
@@ -124,6 +124,20 @@ struct V9fsPDU
     QLIST_ENTRY(V9fsPDU) next;
 };
 
+typedef struct V9fsPostOp {
+    /* Post Operation routine to execute after executing syscall */
+    void (*func)(void *arg);
+    void *arg;
+} V9fsPostOp;
+
+typedef struct V9fsRequest {
+    void (*func)(struct V9fsRequest *req);
+
+    /* Flag to indicate that request is satisfied, ready for post-processing */
+    int done;
+
+    V9fsPostOp post_op;
+} V9fsRequest;
 
 /* FIXME
  * 1) change user needs to set groups and stuff

* Arun R Bharadwaj<[email protected]> [2011-03-15 16:04:53]:

Author: Arun R Bharadwaj<[email protected]>
Date:   Thu Mar 10 15:11:49 2011 +0530

Helper routines to use GLib threadpool infrastructure in 9pfs.

This patch creates helper routines to make use of the threadpool infrastructure provided by GLib. This is based on the prototype patch by Anthony which does a similar thing for posix-aio-compat.c

An example use case is provided in the next patch where one of the syscalls in 9pfs is converted into the threaded model using these helper routines.

Signed-off-by Arun R Bharadwaj<[email protected]>

Reviewed-by Aneesh Kumar K.V<[email protected]>

hw/9pfs/virtio-9p.c +103 -1

index dceefd5..cf61345 100644
@@ -18,6 +18,8 @@
  #include "fsdev/qemu-fsdev.h"
  #include "virtio-9p-debug.h"
  #include "virtio-9p-xattr.h"
+#include "signal.h"
+#include "qemu-thread.h"
 
  int debug_9p_pdu;
  static void v9fs_reclaim_fd(V9fsState *s);

@@ -36,6 +38,89 @@ enum {
      Oappend = 0x80,
  };
 
+typedef struct V9fsPool {
+    GThreadPool *pool;
+    GList *requests;
+    int rfd;
+    int wfd;
+} V9fsPool;
+
+static V9fsPool v9fs_pool;
+
+static void v9fs_qemu_submit_request(V9fsRequest *req)
+{
+    V9fsPool *p =&v9fs_pool;
+
+    p->requests = g_list_append(p->requests, req);
+    g_thread_pool_push(v9fs_pool.pool, req, NULL);
+}
+
+static void die2(int err, const char *what)
+{
+    fprintf(stderr, "%s failed: %s\n", what, strerror(err));
+    abort();
+}
+
+static void die(const char *what)
+{
+    die2(errno, what);
+}
+
+static void v9fs_qemu_process_post_ops(void *arg)
+{
+    struct V9fsPool *p =&v9fs_pool;
+    struct V9fsPostOp *post_op;
+    char byte;
+    ssize_t len;
+    GList *cur_req, *next_req;
+
+    do {
+        len = read(p->rfd,&byte, sizeof(byte));
+    } while (len == -1&&  errno == EINTR);
+
+    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
+        V9fsRequest *req = cur_req->data;
+        next_req = g_list_next(cur_req);
+
+        if (!req->done) {
+            continue;
+        }
+
+        post_op =&req->post_op;
+        post_op->func(post_op->arg);
+        p->requests = g_list_remove_link(p->requests, cur_req);
+        g_list_free(p->requests);
+    }
+}
+
+static inline void v9fs_thread_signal(void)
+{
+    struct V9fsPool *p =&v9fs_pool;
+    char byte = 0;
+    ssize_t ret;
+
+    do {
+        ret = write(p->wfd,&byte, sizeof(byte));
+    } while (ret == -1&&  errno == EINTR);
+
+    if (ret<  0&&  errno != EAGAIN) {
+        die("write() in v9fs");
+    }
+
+    if (kill(getpid(), SIGUSR2)) {
+        die("kill failed");
+    }
+}
+
+static void v9fs_thread_routine(gpointer data, gpointer user_data)
+{
+    V9fsRequest *req = data;
+
+    req->func(req);
+    v9fs_thread_signal();
+    req->done = 1;
+}
+
  static int omode_to_uflags(int8_t mode)
  {
      int ret = 0;

@@ -3850,7 +3935,8 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
      int i, len;
      struct stat stat;
      FsTypeEntry *fse;
-
+    int fds[2];
+    V9fsPool *p =&v9fs_pool;
 
      s = (V9fsState *)virtio_common_init("virtio-9p",
                                      VIRTIO_ID_9P,

@@ -3939,5 +4025,21 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
                          s->tag_len;
      s->vdev.get_config = virtio_9p_get_config;
 
+    if (qemu_pipe(fds) == -1) {
+        fprintf(stderr, "failed to create fd's for virtio-9p\n");
+        exit(1);
+    }
+
+    p->pool = g_thread_pool_new(v9fs_thread_routine, p, 8, FALSE, NULL);
+    p->rfd = fds[0];
+    p->wfd = fds[1];
+
+    fcntl(p->rfd, F_SETFL, O_NONBLOCK);
+    fcntl(p->wfd, F_SETFL, O_NONBLOCK);
+
+    qemu_set_fd_handler(p->rfd, v9fs_qemu_process_post_ops, NULL, NULL);
+
+    (void) v9fs_qemu_submit_request;
+
      return&s->vdev;
  }

hw/9pfs/virtio-9p.h +14 -0

index 10809ba..e7d2326 100644
@@ -124,6 +124,20 @@ struct V9fsPDU
      QLIST_ENTRY(V9fsPDU) next;
  };
 
+typedef struct V9fsPostOp {
+    /* Post Operation routine to execute after executing syscall */
+    void (*func)(void *arg);
+    void *arg;
+} V9fsPostOp;
+
+typedef struct V9fsRequest {
+    void (*func)(struct V9fsRequest *req);
+
+    /* Flag to indicate that request is satisfied, ready for post-processing */
+    int done;
+
+    V9fsPostOp post_op;
+} V9fsRequest;
 
  /* FIXME
   * 1) change user needs to set groups and stuff

Quoted 12 lines · Arun R Bharadwaj

* Arun R Bharadwaj<[email protected]> [2011-03-15 16:04:53]:

Author: Arun R Bharadwaj<[email protected]>
Date:   Thu Mar 10 15:11:49 2011 +0530

Helper routines to use GLib threadpool infrastructure in 9pfs.

This patch creates helper routines to make use of the threadpool infrastructure provided by GLib. This is based on the prototype patch by Anthony which does a similar thing for posix-aio-compat.c

Quoted 6 lines

An example use case is provided in the next patch where one of the syscalls in 9pfs is converted into the threaded model using these helper routines.

Signed-off-by Arun R Bharadwaj<[email protected]>

Reviewed-by Aneesh Kumar K.V<[email protected]>

Why even bothering signaling for completion with the virtio-9p threadpool?

There's no sane guest that's going to poll on virtio-9p completion with interrupts disabled and no timer. Once we enable the I/O thread by default, it won't even be necessary for the paio layer.

Regards,

Anthony Liguori

Quoted 160 lines

hw/9pfs/virtio-9p.c +103 -1

index dceefd5..cf61345 100644
@@ -18,6 +18,8 @@
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-debug.h"
 #include "virtio-9p-xattr.h"
+#include "signal.h"
+#include "qemu-thread.h"
 
 int debug_9p_pdu;
 static void v9fs_reclaim_fd(V9fsState *s);

@@ -36,6 +38,89 @@ enum {
     Oappend = 0x80,
 };
 
+typedef struct V9fsPool {
+    GThreadPool *pool;
+    GList *requests;
+    int rfd;
+    int wfd;
+} V9fsPool;
+
+static V9fsPool v9fs_pool;
+
+static void v9fs_qemu_submit_request(V9fsRequest *req)
+{
+    V9fsPool *p =&v9fs_pool;
+
+    p->requests = g_list_append(p->requests, req);
+    g_thread_pool_push(v9fs_pool.pool, req, NULL);
+}
+
+static void die2(int err, const char *what)
+{
+    fprintf(stderr, "%s failed: %s\n", what, strerror(err));
+    abort();
+}
+
+static void die(const char *what)
+{
+    die2(errno, what);
+}
+
+static void v9fs_qemu_process_post_ops(void *arg)
+{
+    struct V9fsPool *p =&v9fs_pool;
+    struct V9fsPostOp *post_op;
+    char byte;
+    ssize_t len;
+    GList *cur_req, *next_req;
+
+    do {
+        len = read(p->rfd,&byte, sizeof(byte));
+    } while (len == -1&&  errno == EINTR);
+
+    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
+        V9fsRequest *req = cur_req->data;
+        next_req = g_list_next(cur_req);
+
+        if (!req->done) {
+            continue;
+        }
+
+        post_op =&req->post_op;
+        post_op->func(post_op->arg);
+        p->requests = g_list_remove_link(p->requests, cur_req);
+        g_list_free(p->requests);
+    }
+}
+
+static inline void v9fs_thread_signal(void)
+{
+    struct V9fsPool *p =&v9fs_pool;
+    char byte = 0;
+    ssize_t ret;
+
+    do {
+        ret = write(p->wfd,&byte, sizeof(byte));
+    } while (ret == -1&&  errno == EINTR);
+
+    if (ret<  0&&  errno != EAGAIN) {
+        die("write() in v9fs");
+    }
+
+    if (kill(getpid(), SIGUSR2)) {
+        die("kill failed");
+    }
+}
+
+static void v9fs_thread_routine(gpointer data, gpointer user_data)
+{
+    V9fsRequest *req = data;
+
+    req->func(req);
+    v9fs_thread_signal();
+    req->done = 1;
+}
+
 static int omode_to_uflags(int8_t mode)
 {
     int ret = 0;

@@ -3850,7 +3935,8 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
     int i, len;
     struct stat stat;
     FsTypeEntry *fse;
-
+    int fds[2];
+    V9fsPool *p =&v9fs_pool;
 
     s = (V9fsState *)virtio_common_init("virtio-9p",
                                     VIRTIO_ID_9P,

@@ -3939,5 +4025,21 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
                         s->tag_len;
     s->vdev.get_config = virtio_9p_get_config;
 
+    if (qemu_pipe(fds) == -1) {
+        fprintf(stderr, "failed to create fd's for virtio-9p\n");
+        exit(1);
+    }
+
+    p->pool = g_thread_pool_new(v9fs_thread_routine, p, 8, FALSE, NULL);
+    p->rfd = fds[0];
+    p->wfd = fds[1];
+
+    fcntl(p->rfd, F_SETFL, O_NONBLOCK);
+    fcntl(p->wfd, F_SETFL, O_NONBLOCK);
+
+    qemu_set_fd_handler(p->rfd, v9fs_qemu_process_post_ops, NULL, NULL);
+
+    (void) v9fs_qemu_submit_request;
+
     return&s->vdev;
 }

hw/9pfs/virtio-9p.h +14 -0

index 10809ba..e7d2326 100644
@@ -124,6 +124,20 @@ struct V9fsPDU
     QLIST_ENTRY(V9fsPDU) next;
 };
 
+typedef struct V9fsPostOp {
+    /* Post Operation routine to execute after executing syscall */
+    void (*func)(void *arg);
+    void *arg;
+} V9fsPostOp;
+
+typedef struct V9fsRequest {
+    void (*func)(struct V9fsRequest *req);
+
+    /* Flag to indicate that request is satisfied, ready for post-processing */
+    int done;
+
+    V9fsPostOp post_op;
+} V9fsRequest;
 
 /* FIXME
  * 1) change user needs to set groups and stuff

Why even bothering signaling for completion with the virtio-9p threadpool?

There's no sane guest that's going to poll on virtio-9p completion with interrupts disabled and no timer.  Once we enable the I/O thread by default, it won't even be necessary for the paio layer.

Quoted 5 lines · Anthony Liguori

Why even bothering signaling for completion with the virtio-9p threadpool?

There's no sane guest that's going to poll on virtio-9p completion with interrupts disabled and no timer. Once we enable the I/O thread by default, it won't even be necessary for the paio layer.

It's not just about preventing livelock under extreme cases. If you omit the signal then !CONFIG_IOTHREAD builds will see increased latency because virtfs request completion will piggy-back on other events that *do* interrupt the vcpu.

I'm no fan of !CONFIG_IOTHREAD
but skipping signals is currently bad practice and will continue to be until CONFIG_IOTHREAD is removed entirely.

The proper solution would be a thin abstraction for thread-safe notification that compiles out signals when CONFIG_IOTHREAD is used. Then we have one place in QEMU that does signalling correctly and we can optimize it or remove CONFIG_IOTHREAD completely without having the burden of duplicating this code in several places.

Stefan

Quoted 10 lines · Stefan Hajnoczi

Quoted 5 lines · Anthony Liguori

Why even bothering signaling for completion with the virtio-9p threadpool?

There's no sane guest that's going to poll on virtio-9p completion with interrupts disabled and no timer. Once we enable the I/O thread by default, it won't even be necessary for the paio layer.

It's not just about preventing livelock under extreme cases. If you omit the signal then !CONFIG_IOTHREAD builds will see increased latency because virtfs request completion will piggy-back on other events that *do* interrupt the vcpu.

But realistically, the timer is firing at a high enough frequency that I doubt you'd even observe the latency.

There's an easy solution here, just do some sniff testing to see if you can tell the difference. I bet you can't.

Quoted 9 lines

I'm no fan of !CONFIG_IOTHREAD
but skipping signals is currently bad practice and will continue to be until CONFIG_IOTHREAD is removed entirely.

The proper solution would be a thin abstraction for thread-safe notification that compiles out signals when CONFIG_IOTHREAD is used. Then we have one place in QEMU that does signalling correctly and we can optimize it or remove CONFIG_IOTHREAD completely without having the burden of duplicating this code in several places.

We have probably 5 different ways to wake up a CPU. I don't think we should add a new one just for this.

!CONFIG_IOTHREAD needs to go away in the very near future. I'd rather focus on that.

Regards,

Anthony Liguori

Quoted 1 line

Stefan

Quoted 31 lines · Anthony Liguori

Quoted 10 lines · Stefan Hajnoczi

Quoted 5 lines · Anthony Liguori

Why even bothering signaling for completion with the virtio-9p threadpool?

There's no sane guest that's going to poll on virtio-9p completion with interrupts disabled and no timer.  Once we enable the I/O thread by default, it won't even be necessary for the paio layer.

It's not just about preventing livelock under extreme cases.  If you omit the signal then !CONFIG_IOTHREAD builds will see increased latency because virtfs request completion will piggy-back on other events that *do* interrupt the vcpu.

But realistically, the timer is firing at a high enough frequency that I doubt you'd even observe the latency.

There's an easy solution here, just do some sniff testing to see if you can tell the difference.  I bet you can't.

Quoted 9 lines

   I'm no fan of !CONFIG_IOTHREAD
but skipping signals is currently bad practice and will continue to be until CONFIG_IOTHREAD is removed entirely.

The proper solution would be a thin abstraction for thread-safe notification that compiles out signals when CONFIG_IOTHREAD is used. Then we have one place in QEMU that does signalling correctly and we can optimize it or remove CONFIG_IOTHREAD completely without having the burden of duplicating this code in several places.

We have probably 5 different ways to wake up a CPU.  I don't think we should add a new one just for this.

Quoted 2 lines

!CONFIG_IOTHREAD needs to go away in the very near future.  I'd rather focus on that.

If that is the case, how about making VirtFS dependent on CONFIG_IOTHREAD during the configuration? This can help even if !CONFIG_IOTHREAD lingers around for some more time and would avoid any of the Stefan's concerns.

hw/9pfs/virtio-9p.c +1 -1

-static void v9fs_stat_post_lstat(V9fsState *s, V9fsStatState *vs, int err)
+static void v9fs_stat_post_lstat(void *opaque)

 {
-    if (err == -1) {
-        err = -errno;
+    V9fsStatState *vs = (V9fsStatState *)opaque;

+    if (vs->err == -1) {
+        vs->err = -(vs->v9fs_errno);

        goto out;
    }

-    err = stat_to_v9stat(s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);
-    if (err) {
+    vs->err = stat_to_v9stat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);

+    if (vs->err) {
        goto out;
    }
    vs->offset += pdu_marshal(vs->pdu, vs->offset, "wS", 0, &vs->v9stat);
-    err = vs->offset;
+    vs->err = vs->offset;

 out:
-    complete_pdu(s, vs->pdu, err);
+    complete_pdu(vs->s, vs->pdu, vs->err);
    v9fs_stat_free(&vs->v9stat);
    qemu_free(vs);
 }

+
+    vs->err = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf);

+    vs->v9fs_errno = errno;

hw/9pfs/virtio-9p.c +2 -0

+}
+

 static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) {
    int32_t fid;

hw/9pfs/virtio-9p.c +4 -0

@@ -1487,6 +1496,10 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
     vs = qemu_malloc(sizeof(*vs));
     vs->pdu = pdu;
     vs->offset = 7;
+    vs->s = s;
+    vs->request.func = v9fs_stat_do_lstat;
+    vs->request.post_op.func = v9fs_stat_post_lstat;
+    vs->request.post_op.arg = vs;
 
     memset(&vs->v9stat, 0, sizeof(vs->v9stat));
 

hw/9pfs/virtio-9p.c out: · line 1463

Quoted 6 lines · Arun R Bharadwaj

hw/9pfs/virtio-9p.c +2 -3

-static void v9fs_stat_post_lstat(V9fsState *s, V9fsStatState *vs, int err)
+static void v9fs_stat_post_lstat(void *opaque)
 {
-    if (err == -1) {
-        err = -errno;
+    V9fsStatState *vs = (V9fsStatState *)opaque;

No need to cast void* in C.

hw/9pfs/virtio-9p.c out: · line 1465

Quoted 2 lines

hw/9pfs/virtio-9p.c +2 -0

+    if (vs->err == -1) {
+        vs->err = -(vs->v9fs_errno);

How about the thread worker function puts the -errno into a vs->ret field:

static void v9fs_stat_do_lstat(V9fsRequest *request)

{
    V9fsStatState *vs = container_of(request, V9fsStatState, request);

    vs->ret = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf);
    if (vs->ret != 0) {
        vs->ret = -errno;
    }
}

Then v9fs_stat_post_lstat() can use vs->ret directly and does not need to juggle around the two fields, vs->err and vs->v9fs_errno.

hw/9pfs/virtio-9p.c out: · line 1469

Quoted 6 lines

        goto out;
    }

hw/9pfs/virtio-9p.c +1 -2

-    err = stat_to_v9stat(s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);
-    if (err) {
+    vs->err = stat_to_v9stat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);

This function can block in v9fs_do_readlink(). Needs to be done asynchronously to avoid blocking QEMU.

Quoted 7 lines

hw/9pfs/virtio-9p.c +2 -1

+    if (vs->err) {
        goto out;
    }
    vs->offset += pdu_marshal(vs->pdu, vs->offset, "wS", 0, &vs->v9stat);
-    err = vs->offset;
+    vs->err = vs->offset;
 

hw/9pfs/virtio-9p.c out: · line 1484

Quoted 10 lines

hw/9pfs/virtio-9p.c +4 -1

 out:
-    complete_pdu(s, vs->pdu, err);
+    complete_pdu(vs->s, vs->pdu, vs->err);
    v9fs_stat_free(&vs->v9stat);
    qemu_free(vs);
 }
 
+static void v9fs_stat_do_lstat(V9fsRequest *request)
+{
+    V9fsStatState *vs = container_of(request, V9fsStatState, request);

Nice. Could container_of() be used for v9fs_post_lstat() too? I'm suggesting making post op functions take the V9fsRequest* instead of a void* opaque pointer.

hw/9pfs/virtio-9p.c out: · line 1486

Quoted 2 lines

hw/9pfs/virtio-9p.c +2 -0

+
+    vs->err = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf);

This is not threadsafe since rpath still uses a static buffer in qemu.git. Please ensure that rpath() is thread-safe before pushing this patch.

Quoted 19 lines

hw/9pfs/virtio-9p.c +7 -0

+    vs->v9fs_errno = errno;
+}
+
 static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
 {
    int32_t fid;

@@ -1487,6 +1496,10 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
    vs = qemu_malloc(sizeof(*vs));
    vs->pdu = pdu;
    vs->offset = 7;
+    vs->s = s;
+    vs->request.func = v9fs_stat_do_lstat;
+    vs->request.post_op.func = v9fs_stat_post_lstat;
+    vs->request.post_op.arg = vs;
 
    memset(&vs->v9stat, 0, sizeof(vs->v9stat));
 

hw/9pfs/virtio-9p.c static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) · line 1517

Quoted 7 lines

hw/9pfs/virtio-9p.c +2 -0

@@ -1498,8 +1511,11 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
        goto out;
    }
 
+    /*
    err = v9fs_do_lstat(s, &vs->fidp->fsmap.path, &vs->stbuf);
    v9fs_stat_post_lstat(s, vs, err);
+    */

Please remove unused code, it quickly becomes out-of-date and confuses readers.

hw/9pfs/virtio-9p.c static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) · line 1518

Quoted 1 line

hw/9pfs/virtio-9p.c +1 -0

+    v9fs_qemu_submit_request(&vs->request);

What happens when another PDU is handled next that uses the same fid? The worst case is if the client sends TCLUNK and fid is freed while the worker thread and later the post op still access the memory. There needs to be some kind of guard (like a reference count) to prevent this.

Stefan

Quoted 61 lines · Stefan Hajnoczi

Quoted 6 lines · Arun R Bharadwaj

hw/9pfs/virtio-9p.c +1 -1

-static void v9fs_stat_post_lstat(V9fsState *s, V9fsStatState *vs, int err)
+static void v9fs_stat_post_lstat(void *opaque)

 {
-    if (err == -1) {
-        err = -errno;
+    V9fsStatState *vs = (V9fsStatState *)opaque;

No need to cast void* in C.

Quoted 2 lines

+    if (vs->err == -1) {
+        vs->err = -(vs->v9fs_errno);

How about the thread worker function puts the -errno into a vs->ret field:

static void v9fs_stat_do_lstat(V9fsRequest *request)
{
    V9fsStatState *vs = container_of(request, V9fsStatState, request);

    vs->ret = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf); if (vs->ret != 0) {
        vs->ret = -errno;
    }
}

Then v9fs_stat_post_lstat() can use vs->ret directly and does not need to juggle around the two fields, vs->err and vs->v9fs_errno.

Quoted 6 lines

        goto out;
    }

-    err = stat_to_v9stat(s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);
-    if (err) {
+    vs->err = stat_to_v9stat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);

This function can block in v9fs_do_readlink().  Needs to be done asynchronously to avoid blocking QEMU.

Quoted 14 lines

+    if (vs->err) {
        goto out;
    }
    vs->offset += pdu_marshal(vs->pdu, vs->offset, "wS", 0, &vs->v9stat);
-    err = vs->offset;
+    vs->err = vs->offset;

 out:
-    complete_pdu(s, vs->pdu, err);
+    complete_pdu(vs->s, vs->pdu, vs->err);
    v9fs_stat_free(&vs->v9stat);
    qemu_free(vs);
 }

hw/9pfs/virtio-9p.c out: · line 1484

Quoted 3 lines

hw/9pfs/virtio-9p.c +3 -0

+static void v9fs_stat_do_lstat(V9fsRequest *request)
+{
+    V9fsStatState *vs = container_of(request, V9fsStatState, request);

Nice.  Could container_of() be used for v9fs_post_lstat() too?  I'm suggesting making post op functions take the V9fsRequest* instead of a void* opaque pointer.

Quoted 6 lines

Quoted 2 lines

+
+    vs->err = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf);

This is not threadsafe since rpath still uses a static buffer in qemu.git.  Please ensure that rpath() is thread-safe before pushing this patch.

There is another patch on the internal list to make rpath thread safe.

Quoted 29 lines

Quoted 19 lines

+    vs->v9fs_errno = errno;

hw/9pfs/virtio-9p.c +2 -0

+}
+

 static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) {
    int32_t fid;

hw/9pfs/virtio-9p.c +4 -0

@@ -1487,6 +1496,10 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
     vs = qemu_malloc(sizeof(*vs));
     vs->pdu = pdu;
     vs->offset = 7;
+    vs->s = s;
+    vs->request.func = v9fs_stat_do_lstat;
+    vs->request.post_op.func = v9fs_stat_post_lstat;
+    vs->request.post_op.arg = vs;
 
     memset(&vs->v9stat, 0, sizeof(vs->v9stat));
 

hw/9pfs/virtio-9p.c static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) · line 1517

Quoted 7 lines

hw/9pfs/virtio-9p.c +2 -0

@@ -1498,8 +1511,11 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
         goto out;
     }
 
+    /*
     err = v9fs_do_lstat(s, &vs->fidp->fsmap.path, &vs->stbuf);
     v9fs_stat_post_lstat(s, vs, err);
+    */

Please remove unused code, it quickly becomes out-of-date and confuses readers.

Quoted 7 lines

hw/9pfs/virtio-9p.c static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) · line 1518

Quoted 1 line

hw/9pfs/virtio-9p.c +1 -0

+    v9fs_qemu_submit_request(&vs->request);

What happens when another PDU is handled next that uses the same fid? The worst case is if the client sends TCLUNK and fid is freed while the worker thread and later the post op still access the memory. There needs to be some kind of guard (like a reference count) to prevent this.

As per the protocol this should not happen. Client is the controls the fid, and the fid is created or destroyed per the directive of the client. It should not send clunk until the response is received on that fid based operation(if there is any).

Quoted 108 lines · Venkateswararao Jujjuri (JV)

Quoted 61 lines · Stefan Hajnoczi

hw/9pfs/virtio-9p.c out: · line 1463

Quoted 6 lines · Arun R Bharadwaj

hw/9pfs/virtio-9p.c +2 -3

-static void v9fs_stat_post_lstat(V9fsState *s, V9fsStatState *vs, int err)
+static void v9fs_stat_post_lstat(void *opaque)
 {
-    if (err == -1) {
-        err = -errno;
+    V9fsStatState *vs = (V9fsStatState *)opaque;

No need to cast void* in C.

hw/9pfs/virtio-9p.c out: · line 1465

Quoted 2 lines

hw/9pfs/virtio-9p.c +2 -0

+    if (vs->err == -1) {
+        vs->err = -(vs->v9fs_errno);

How about the thread worker function puts the -errno into a vs->ret field:

static void v9fs_stat_do_lstat(V9fsRequest *request)

{
    V9fsStatState *vs = container_of(request, V9fsStatState, request);

    vs->ret = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf);
    if (vs->ret != 0) {
        vs->ret = -errno;
    }
}

Then v9fs_stat_post_lstat() can use vs->ret directly and does not need to juggle around the two fields, vs->err and vs->v9fs_errno.

hw/9pfs/virtio-9p.c out: · line 1469

Quoted 6 lines

        goto out;
    }

hw/9pfs/virtio-9p.c +1 -2

-    err = stat_to_v9stat(s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);
-    if (err) {
+    vs->err = stat_to_v9stat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);

This function can block in v9fs_do_readlink(). Needs to be done asynchronously to avoid blocking QEMU.

Quoted 7 lines

hw/9pfs/virtio-9p.c +2 -1

+    if (vs->err) {
        goto out;
    }
    vs->offset += pdu_marshal(vs->pdu, vs->offset, "wS", 0, &vs->v9stat);
-    err = vs->offset;
+    vs->err = vs->offset;
 

hw/9pfs/virtio-9p.c out: · line 1484

Quoted 10 lines

hw/9pfs/virtio-9p.c +4 -1

 out:
-    complete_pdu(s, vs->pdu, err);
+    complete_pdu(vs->s, vs->pdu, vs->err);
    v9fs_stat_free(&vs->v9stat);
    qemu_free(vs);
 }
 
+static void v9fs_stat_do_lstat(V9fsRequest *request)
+{
+    V9fsStatState *vs = container_of(request, V9fsStatState, request);

Nice. Could container_of() be used for v9fs_post_lstat() too? I'm suggesting making post op functions take the V9fsRequest* instead of a void* opaque pointer.

Quoted 6 lines

hw/9pfs/virtio-9p.c out: · line 1486

Quoted 2 lines

hw/9pfs/virtio-9p.c +2 -0

+
+    vs->err = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf);

This is not threadsafe since rpath still uses a static buffer in qemu.git. Please ensure that rpath() is thread-safe before pushing this patch.

There is another patch on the internal list to make rpath thread safe.

Quoted 36 lines

Quoted 19 lines

hw/9pfs/virtio-9p.c +7 -0

+    vs->v9fs_errno = errno;
+}
+
 static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
 {
    int32_t fid;

@@ -1487,6 +1496,10 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
    vs = qemu_malloc(sizeof(*vs));
    vs->pdu = pdu;
    vs->offset = 7;
+    vs->s = s;
+    vs->request.func = v9fs_stat_do_lstat;
+    vs->request.post_op.func = v9fs_stat_post_lstat;
+    vs->request.post_op.arg = vs;
 
    memset(&vs->v9stat, 0, sizeof(vs->v9stat));
 

hw/9pfs/virtio-9p.c static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) · line 1517

Quoted 7 lines

hw/9pfs/virtio-9p.c +2 -0

@@ -1498,8 +1511,11 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
        goto out;
    }
 
+    /*
    err = v9fs_do_lstat(s, &vs->fidp->fsmap.path, &vs->stbuf);
    v9fs_stat_post_lstat(s, vs, err);
+    */

Please remove unused code, it quickly becomes out-of-date and confuses readers.

hw/9pfs/virtio-9p.c static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) · line 1518

Quoted 1 line

hw/9pfs/virtio-9p.c +1 -0

+    v9fs_qemu_submit_request(&vs->request);

What happens when another PDU is handled next that uses the same fid? The worst case is if the client sends TCLUNK and fid is freed while the worker thread and later the post op still access the memory. There needs to be some kind of guard (like a reference count) to prevent this.

Quoted 4 lines

As per the protocol this should not happen. Client is the controls the fid, and the fid is created or destroyed per the directive of the client. It should not send clunk until the response is received on that fid based operation(if there is any).

Unfortunately it is still possible for a guest to do it. The model for emulated hardware is that the guest is untrusted and we cannot allow things to crash.

It's really important for everyone to keep this in mind otherwise security vulnerabilities will be introduced for use cases like hosting and cloud where the guest really cannot be trusted.

An easy fix is to mark a fid busy and reject requests that mess with it before the existing request has been processed.

Stefan

Quoted 115 lines · Stefan Hajnoczi

Quoted 112 lines · Venkateswararao Jujjuri (JV)

Quoted 61 lines · Stefan Hajnoczi

Quoted 6 lines · Arun R Bharadwaj

hw/9pfs/virtio-9p.c +1 -1

-static void v9fs_stat_post_lstat(V9fsState *s, V9fsStatState *vs, int err)
+static void v9fs_stat_post_lstat(void *opaque)

 {
-    if (err == -1) {
-        err = -errno;
+    V9fsStatState *vs = (V9fsStatState *)opaque;

No need to cast void* in C.

Quoted 2 lines

+    if (vs->err == -1) {
+        vs->err = -(vs->v9fs_errno);

How about the thread worker function puts the -errno into a vs->ret field:

static void v9fs_stat_do_lstat(V9fsRequest *request)
{
    V9fsStatState *vs = container_of(request, V9fsStatState, request);

    vs->ret = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf); if (vs->ret != 0) {
        vs->ret = -errno;
    }
}

Then v9fs_stat_post_lstat() can use vs->ret directly and does not need to juggle around the two fields, vs->err and vs->v9fs_errno.

Quoted 6 lines

        goto out;
    }

-    err = stat_to_v9stat(s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);
-    if (err) {
+    vs->err = stat_to_v9stat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat);

This function can block in v9fs_do_readlink().  Needs to be done asynchronously to avoid blocking QEMU.

Quoted 14 lines

+    if (vs->err) {
        goto out;
    }
    vs->offset += pdu_marshal(vs->pdu, vs->offset, "wS", 0, &vs->v9stat);
-    err = vs->offset;
+    vs->err = vs->offset;

 out:
-    complete_pdu(s, vs->pdu, err);
+    complete_pdu(vs->s, vs->pdu, vs->err);
    v9fs_stat_free(&vs->v9stat);
    qemu_free(vs);
 }

hw/9pfs/virtio-9p.c out: · line 1484

Quoted 3 lines

hw/9pfs/virtio-9p.c +3 -0

+static void v9fs_stat_do_lstat(V9fsRequest *request)
+{
+    V9fsStatState *vs = container_of(request, V9fsStatState, request);

Nice.  Could container_of() be used for v9fs_post_lstat() too?  I'm suggesting making post op functions take the V9fsRequest* instead of a void* opaque pointer.

Quoted 6 lines

Quoted 2 lines

+
+    vs->err = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf);

This is not threadsafe since rpath still uses a static buffer in qemu.git.  Please ensure that rpath() is thread-safe before pushing this patch.

There is another patch on the internal list to make rpath thread safe.

Quoted 29 lines

Quoted 19 lines

+    vs->v9fs_errno = errno;

hw/9pfs/virtio-9p.c +2 -0

+}
+

 static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) {
    int32_t fid;

hw/9pfs/virtio-9p.c +4 -0

@@ -1487,6 +1496,10 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
     vs = qemu_malloc(sizeof(*vs));
     vs->pdu = pdu;
     vs->offset = 7;
+    vs->s = s;
+    vs->request.func = v9fs_stat_do_lstat;
+    vs->request.post_op.func = v9fs_stat_post_lstat;
+    vs->request.post_op.arg = vs;
 
     memset(&vs->v9stat, 0, sizeof(vs->v9stat));
 

hw/9pfs/virtio-9p.c static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) · line 1517

Quoted 7 lines

hw/9pfs/virtio-9p.c +2 -0

@@ -1498,8 +1511,11 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
         goto out;
     }
 
+    /*
     err = v9fs_do_lstat(s, &vs->fidp->fsmap.path, &vs->stbuf);
     v9fs_stat_post_lstat(s, vs, err);
+    */

Please remove unused code, it quickly becomes out-of-date and confuses readers.

Quoted 7 lines

hw/9pfs/virtio-9p.c static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) · line 1518

Quoted 1 line

hw/9pfs/virtio-9p.c +1 -0

+    v9fs_qemu_submit_request(&vs->request);

What happens when another PDU is handled next that uses the same fid? The worst case is if the client sends TCLUNK and fid is freed while the worker thread and later the post op still access the memory. There needs to be some kind of guard (like a reference count) to prevent this.

As per the protocol this should not happen. Client is the controls the fid, and the fid is created or destroyed per the directive of the client. It should not send clunk until the response is received on that fid based operation(if there is any).

Quoted 3 lines

Unfortunately it is still possible for a guest to do it.  The model for emulated hardware is that the guest is untrusted and we cannot allow things to crash.

Well, it can happen only if the guest OS is hacked...and the worst thing can happen is guest goes down. I am not sure how it is different from a bare metal system..