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

[Harsh Bora <harsh@linux.vnet.ibm.com>]

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