[Qemu-devel] [v1 PATCH 0/3]: Use GLib threadpool in 9pfs.

[Qemu-devel] [v1 PATCH 0/3]: Use GLib threadpool in 9pfs.

[Arun R Bharadwaj]

Hi,

This patchset enables the use of GLib threadpool infrastructure in
9pfs. It contains the following patches:

1/3 - Move the paio_signal_handler to a generic location.
2/3 - Helper routines to use GLib threadpool infrastructure in 9pfs.
3/3 - Convert v9fs_stat to threaded model.

As a prerequisite, before these patches are applied, we need to apply
Anthony's patch "Add support for glib based threading and convert qemu
thread to use it" found at
http://www.mail-archive.com/qemu-devel@nongnu.org/msg52791.html


Testing carried out:

This patchset has been tested by running the following autotest suites
successfully:
* Dbench
* Fsstress
* Connecthon
* Tuxera POSIX.

Please let me know your comments.


-arun

[Qemu-devel] [v1 PATCH 1/3]: Move the paio_signal_handler to a generic location.

[Arun R Bharadwaj]

* Arun R Bharadwaj <arun@linux.vnet.ibm.com> [2011-03-15 16:04:53]:

Author: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
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.
    
    Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
    Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index fa5494d..df001d6 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -28,6 +28,7 @@
 #include "qemu-common.h"
 #include "trace.h"
 #include "block_int.h"
+#include "qemu-thread.h"
 
 #include "block/raw-posix-aio.h"
 
@@ -302,6 +303,8 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
     return nbytes;
 }
 
+static PosixAioState *posix_aio_state;
+
 static void *aio_thread(void *unused)
 {
     pid_t pid;
@@ -356,6 +359,15 @@ static void *aio_thread(void *unused)
         idle_threads++;
         mutex_unlock(&lock);
 
+        if (posix_aio_state) {
+            char byte = 0;
+            ssize_t ret;
+
+            ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
+            if (ret < 0 && errno != EAGAIN)
+                die("write()");
+        }
+
         if (kill(pid, aiocb->ev_signo)) die("kill failed");
     }
 
@@ -497,22 +509,6 @@ static int posix_aio_flush(void *opaque)
     return !!s->first_aio;
 }
 
-static PosixAioState *posix_aio_state;
-
-static void aio_signal_handler(int signum)
-{
-    if (posix_aio_state) {
-        char byte = 0;
-        ssize_t ret;
-
-        ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
-        if (ret < 0 && errno != EAGAIN)
-            die("write()");
-    }
-
-    qemu_service_io();
-}
-
 static void paio_remove(struct qemu_paiocb *acb)
 {
     struct qemu_paiocb **pacb;
@@ -616,7 +612,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd,
 
 int paio_init(void)
 {
-    struct sigaction act;
     PosixAioState *s;
     int fds[2];
     int ret;
@@ -626,11 +621,6 @@ int paio_init(void)
 
     s = qemu_malloc(sizeof(PosixAioState));
 
-    sigfillset(&act.sa_mask);
-    act.sa_flags = 0; /* do not restart syscalls to interrupt select() */
-    act.sa_handler = aio_signal_handler;
-    sigaction(SIGUSR2, &act, NULL);
-
     s->first_aio = NULL;
     if (qemu_pipe(fds) == -1) {
         fprintf(stderr, "failed to create pipe\n");
diff --git a/qemu-thread.c b/qemu-thread.c
index 2c521ab..bed1b60 100644
--- a/qemu-thread.c
+++ b/qemu-thread.c
@@ -149,3 +149,18 @@ void qemu_thread_exit(void *retval)
 {
     g_thread_exit(retval);
 }
+
+void sigusr2_signal_handler(int signum)
+{
+    qemu_service_io();
+}
+
+void register_sigusr2_signal_handler(void)
+{
+    struct sigaction act;
+
+    sigfillset(&act.sa_mask);
+    act.sa_flags = 0; /* do not restart syscalls to interrupt select() */
+    act.sa_handler = sigusr2_signal_handler;
+    sigaction(SIGUSR2, &act, NULL);
+}
diff --git a/qemu-thread.h b/qemu-thread.h
index dc22a60..0f1cbe8 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -41,5 +41,7 @@ void qemu_thread_signal(QemuThread *thread, int sig);
 void qemu_thread_self(QemuThread *thread);
 int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2);
 void qemu_thread_exit(void *retval);
+void sigusr2_signal_handler(int signum);
+void register_sigusr2_signal_handler(void);
 
 #endif
diff --git a/vl.c b/vl.c
index 3225b1d..a8dc4fc 100644
--- a/vl.c
+++ b/vl.c
@@ -148,6 +148,7 @@ int main(int argc, char **argv)
 #include "qemu-config.h"
 #include "qemu-objects.h"
 #include "qemu-options.h"
+#include "qemu-thread.h"
 #ifdef CONFIG_VIRTFS
 #include "fsdev/qemu-fsdev.h"
 #endif
@@ -3003,6 +3004,8 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
+    register_sigusr2_signal_handler();
+
     if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
         exit(1);
     }

[Qemu-devel] [v1 PATCH 2/3]: Helper routines to use GLib threadpool infrastructure in 9pfs.

[Arun R Bharadwaj]

* Arun R Bharadwaj <arun@linux.vnet.ibm.com> [2011-03-15 16:04:53]:

Author: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
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 <arun@linux.vnet.ibm.com>
    Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index dceefd5..cf61345 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -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;
 }
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 10809ba..e7d2326 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -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

[Qemu-devel] [v1 PATCH 3/3]: Convert v9fs_stat to threaded model.

[Arun R Bharadwaj]

* Arun R Bharadwaj <arun@linux.vnet.ibm.com> [2011-03-15 16:04:53]:

Author: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
Date:   Mon Mar 14 13:55:37 2011 +0530

    Convert v9fs_stat to threaded model.
    
    This patch converts v9fs_stat syscall of 9pfs to threaded
    model by making use of the helper routines provided
    created by the previous patch.
    
    Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
    Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com>

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index cf61345..a328e97 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -1458,26 +1458,35 @@ out:
     v9fs_string_free(&aname);
 }
 
-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);
 }
 
+static void v9fs_stat_do_lstat(V9fsRequest *request)
+{
+    V9fsStatState *vs = container_of(request, V9fsStatState, request);
+
+    vs->err = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf);
+    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));
 
@@ -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);
+    */
+    v9fs_qemu_submit_request(&vs->request);
     return;
 
 out:
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index e7d2326..1d6c17c 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -271,6 +271,10 @@ typedef struct V9fsStatState {
     V9fsStat v9stat;
     V9fsFidState *fidp;
     struct stat stbuf;
+    V9fsState *s;
+    int err;
+    int v9fs_errno;
+    V9fsRequest request;
 } V9fsStatState;
 
 typedef struct V9fsStatDotl {

[Qemu-devel] Re: [v1 PATCH 1/3]: Move the paio_signal_handler to a generic location.

[Stefan Hajnoczi]

On Tue, Mar 15, 2011 at 10:36 AM, Arun R Bharadwaj
<arun@linux.vnet.ibm.com> wrote:
> * Arun R Bharadwaj <arun@linux.vnet.ibm.com> [2011-03-15 16:04:53]:
>
> Author: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
> 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.

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

> @@ -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.

> +void sigusr2_signal_handler(int signum)

static void sigusr2_signal_handler(int signum)

Stefan

Re: [Qemu-devel] [v1 PATCH 2/3]: Helper routines to use GLib threadpool infrastructure in 9pfs.

[Harsh Bora]

On 03/15/2011 04:08 PM, Arun R Bharadwaj wrote:
> * Arun R Bharadwaj<arun@linux.vnet.ibm.com>  [2011-03-15 16:04:53]:
>
> Author: Arun R Bharadwaj<arun@linux.vnet.ibm.com>
> 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<arun@linux.vnet.ibm.com>
>      Reviewed-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
>
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index dceefd5..cf61345 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -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)) {

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

> +        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();

> +}
> +
>   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;

Do we really need it ^ ?

- Harsh

> +
>       return&s->vdev;
>   }
> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> index 10809ba..e7d2326 100644
> --- a/hw/9pfs/virtio-9p.h
> +++ b/hw/9pfs/virtio-9p.h
> @@ -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
>
>

[Qemu-devel] Re: [v1 PATCH 2/3]: Helper routines to use GLib threadpool infrastructure in 9pfs.

[Anthony Liguori]

On 03/15/2011 05:38 AM, Arun R Bharadwaj wrote:
> * Arun R Bharadwaj<arun@linux.vnet.ibm.com>  [2011-03-15 16:04:53]:
>
> Author: Arun R Bharadwaj<arun@linux.vnet.ibm.com>
> 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<arun@linux.vnet.ibm.com>
>      Reviewed-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>

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

> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index dceefd5..cf61345 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -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;
>   }
> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> index 10809ba..e7d2326 100644
> --- a/hw/9pfs/virtio-9p.h
> +++ b/hw/9pfs/virtio-9p.h
> @@ -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

[Qemu-devel] Re: [v1 PATCH 1/3]: Move the paio_signal_handler to a generic location.

[Arun R Bharadwaj]

* Stefan Hajnoczi <stefanha@gmail.com> [2011-03-15 11:38:03]:

> On Tue, Mar 15, 2011 at 10:36 AM, Arun R Bharadwaj
> <arun@linux.vnet.ibm.com> wrote:
> > * Arun R Bharadwaj <arun@linux.vnet.ibm.com> [2011-03-15 16:04:53]:
> >
> > Author: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
> > 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.
> 
> I think os-posix.c would be appropriate.  Please check how this
> affects Windows host and linux-user builds.
> 

Anthony's reply negates this point. I will remove the signalling code
and also check the behaviour of Windows host and linux-user builds.

> > @@ -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.
> 

Yes, will remove this.

> > +void sigusr2_signal_handler(int signum)
> 
> static void sigusr2_signal_handler(int signum)
> 

Thanks for noticing this.

> Stefan

Re: [Qemu-devel] [v1 PATCH 2/3]: Helper routines to use GLib threadpool infrastructure in 9pfs.

[Arun R Bharadwaj]

* Harsh Bora <harsh@linux.vnet.ibm.com> [2011-03-15 17:15:48]:

> On 03/15/2011 04:08 PM, Arun R Bharadwaj wrote:
> >* Arun R Bharadwaj<arun@linux.vnet.ibm.com>  [2011-03-15 16:04:53]:
> >
> >Author: Arun R Bharadwaj<arun@linux.vnet.ibm.com>
> >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<arun@linux.vnet.ibm.com>
> >     Reviewed-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
> >
> >diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> >index dceefd5..cf61345 100644
> >--- a/hw/9pfs/virtio-9p.c
> >+++ b/hw/9pfs/virtio-9p.c
> >@@ -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)) {
> 
> Not sure whether using pthread_kill or qemu_thread_signal is better
> to go with?
> 

Please check the other replies. Looks like we don't really need
signalling.

> >+        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();
> 
> >+}
> >+
> >  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;
> 
> Do we really need it ^ ?
> 

Yeah. Because till we make use of v9fs_qemu_submit_request() in the
next patch, we will get a "v9fs_qemu_submit_request defined but not
used" compile error.

> - Harsh
> 
> >+
> >      return&s->vdev;
> >  }
> >diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> >index 10809ba..e7d2326 100644
> >--- a/hw/9pfs/virtio-9p.h
> >+++ b/hw/9pfs/virtio-9p.h
> >@@ -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
> >
> >
> 

[Qemu-devel] Re: [v1 PATCH 2/3]: Helper routines to use GLib threadpool infrastructure in 9pfs.

[Arun R Bharadwaj]

* Anthony Liguori <aliguori@us.ibm.com> [2011-03-15 08:13:19]:

> On 03/15/2011 05:38 AM, Arun R Bharadwaj wrote:
> >* Arun R Bharadwaj<arun@linux.vnet.ibm.com>  [2011-03-15 16:04:53]:
> >
> >Author: Arun R Bharadwaj<arun@linux.vnet.ibm.com>
> >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<arun@linux.vnet.ibm.com>
> >     Reviewed-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
> 
> 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
> 

Thanks for the information. I will remove the signalling part,
test the code again and re-post.

> >diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> >index dceefd5..cf61345 100644
> >--- a/hw/9pfs/virtio-9p.c
> >+++ b/hw/9pfs/virtio-9p.c
> >@@ -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;
> >  }
> >diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> >index 10809ba..e7d2326 100644
> >--- a/hw/9pfs/virtio-9p.h
> >+++ b/hw/9pfs/virtio-9p.h
> >@@ -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
> 

[Qemu-devel] Re: [v1 PATCH 2/3]: Helper routines to use GLib threadpool infrastructure in 9pfs.

[Stefan Hajnoczi]

On Tue, Mar 15, 2011 at 1:13 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> 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

[Qemu-devel] Re: [v1 PATCH 3/3]: Convert v9fs_stat to threaded model.

[Stefan Hajnoczi]

On Tue, Mar 15, 2011 at 10:39 AM, Arun R Bharadwaj
<arun@linux.vnet.ibm.com> wrote:
> -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.

> +    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.

>         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.

> +    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);
>  }
>
> +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.

> +
> +    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.

> +    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));
>
> @@ -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.

> +    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

[Qemu-devel] Re: [v1 PATCH 2/3]: Helper routines to use GLib threadpool infrastructure in 9pfs.

[Anthony Liguori]

On 03/16/2011 04:20 AM, Stefan Hajnoczi wrote:
> On Tue, Mar 15, 2011 at 1:13 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> 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.

>    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

> Stefan

[Qemu-devel] Re: [v1 PATCH 2/3]: Helper routines to use GLib threadpool infrastructure in 9pfs.

[Venkateswararao Jujjuri (JV)]

On 3/16/2011 6:10 AM, Anthony Liguori wrote:
> On 03/16/2011 04:20 AM, Stefan Hajnoczi wrote:
>> On Tue, Mar 15, 2011 at 1:13 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>> 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.
> 
>>    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.

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.

- JV

> 
> Regards,
> 
> Anthony Liguori
> 
>> Stefan
> 

[Qemu-devel] Re: [v1 PATCH 3/3]: Convert v9fs_stat to threaded model.

[Venkateswararao Jujjuri (JV)]

On 3/16/2011 3:23 AM, Stefan Hajnoczi wrote:
> On Tue, Mar 15, 2011 at 10:39 AM, Arun R Bharadwaj
> <arun@linux.vnet.ibm.com> wrote:
>> -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.
> 
>> +    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.
> 
>>         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.
> 
>> +    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);
>>  }
>>
>> +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.
> 
>> +
>> +    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.

> 
>> +    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));
>>
>> @@ -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.
> 
>> +    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).

- JV
> 
> Stefan

[Qemu-devel] Re: [v1 PATCH 2/3]: Helper routines to use GLib threadpool infrastructure in 9pfs.

[Stefan Hajnoczi]

On Wed, Mar 16, 2011 at 2:20 PM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> On 3/16/2011 6:10 AM, Anthony Liguori wrote:
>> On 03/16/2011 04:20 AM, Stefan Hajnoczi wrote:
>>> On Tue, Mar 15, 2011 at 1:13 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>>> 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.
>>
>>>    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.
>
> 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.

Sounds fair.

Stefan

[Qemu-devel] Re: [v1 PATCH 3/3]: Convert v9fs_stat to threaded model.

[Stefan Hajnoczi]

On Wed, Mar 16, 2011 at 2:33 PM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> On 3/16/2011 3:23 AM, Stefan Hajnoczi wrote:
>> On Tue, Mar 15, 2011 at 10:39 AM, Arun R Bharadwaj
>> <arun@linux.vnet.ibm.com> wrote:
>>> -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.
>>
>>> +    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.
>>
>>>         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.
>>
>>> +    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);
>>>  }
>>>
>>> +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.
>>
>>> +
>>> +    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.
>
>>
>>> +    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));
>>>
>>> @@ -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.
>>
>>> +    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).

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

[Qemu-devel] Re: [v1 PATCH 3/3]: Convert v9fs_stat to threaded model.

[Venkateswararao Jujjuri (JV)]

On 3/16/2011 10:10 AM, Stefan Hajnoczi wrote:
> On Wed, Mar 16, 2011 at 2:33 PM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com> wrote:
>> On 3/16/2011 3:23 AM, Stefan Hajnoczi wrote:
>>> On Tue, Mar 15, 2011 at 10:39 AM, Arun R Bharadwaj
>>> <arun@linux.vnet.ibm.com> wrote:
>>>> -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.
>>>
>>>> +    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.
>>>
>>>>         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.
>>>
>>>> +    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);
>>>>  }
>>>>
>>>> +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.
>>>
>>>> +
>>>> +    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.
>>
>>>
>>>> +    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));
>>>>
>>>> @@ -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.
>>>
>>>> +    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).
> 
> 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..
> 
> 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

[Qemu-devel] Re: [v1 PATCH 3/3]: Convert v9fs_stat to threaded model.

[Stefan Hajnoczi]

On Thu, Mar 17, 2011 at 4:26 AM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> On 3/16/2011 10:10 AM, Stefan Hajnoczi wrote:
>> On Wed, Mar 16, 2011 at 2:33 PM, Venkateswararao Jujjuri (JV)
>> <jvrao@linux.vnet.ibm.com> wrote:
>>> On 3/16/2011 3:23 AM, Stefan Hajnoczi wrote:
>>>> On Tue, Mar 15, 2011 at 10:39 AM, Arun R Bharadwaj
>>>> <arun@linux.vnet.ibm.com> wrote:
>>>>> -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.
>>>>
>>>>> +    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.
>>>>
>>>>>         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.
>>>>
>>>>> +    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);
>>>>>  }
>>>>>
>>>>> +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.
>>>>
>>>>> +
>>>>> +    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.
>>>
>>>>
>>>>> +    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));
>>>>>
>>>>> @@ -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.
>>>>
>>>>> +    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).
>>
>> 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..

No, use after free can lead to arbitrary code execution or other
effects more severe than the guest going down:
http://en.wikipedia.org/wiki/Malloc#Use_after_free

For example if the same memory is handed out by malloc and used to
store a function pointer, then the function pointer can be corrupted
and cause a jump to an arbitrary place in memory.

Hardware emulation is like implementing system calls in an operating
system.  You cannot crash in the kernel and you cannot allow undefined
things to happen.  Every state needs to be handled, whether a
reasonable process would do it or not.

Also, allowing hardware emulation to take down the guest is not an
option going forward.  People have been working on nested
virtualization.  In that scenario a virtio-9p-pci device can be passed
through to a nested guest.  If that guest is able to take down QEMU
then it can kill its sibling nested guests and its parent guest, which
it should not be able to do.

Stefan