Patchwork [5/9] Convert wstat into 2nd threading model

login
register
mail settings
Submitter Arun Bharadwaj
Date Oct. 14, 2010, 12:24 p.m.
Message ID <20101014122438.2238.93872.stgit@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/67815/
State New
Headers show

Comments

Arun Bharadwaj - Oct. 14, 2010, 12:24 p.m.
From: Sripathi Kodi <sripathik@in.ibm.com>

In this model we hand over the vcpu thread only executes till
    the first blocking operation. It then hands over the call to
    the worker thread, which does everything needed to complete
    the call. It can make multiple blocking calls. It finally
    signals the IO thread to do complete_pdu().

    Simplification of code due to this thread model is obvious
    in this patch.

    Gautham suggested that I use a generic function to call
    complete_pdu, which could further simplify the post_op
    structure. However, some of the calls (stat included) need
    to free up some memory after calling complete_pdu. Handling
    this becomes messy if I use a common complete function.

    Review comments welcome. Review comment from Gautham is a
    necessity.

Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>
---
 hw/virtio-9p.c |  238 +++++++++++++++++++++-----------------------------------
 hw/virtio-9p.h |    4 +
 2 files changed, 94 insertions(+), 148 deletions(-)

Patch

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index d994293..1f2fd9f 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -2873,45 +2873,13 @@  out:
     qemu_free(vs);
 }
 
-static void v9fs_wstat_post_truncate(V9fsState *s, V9fsWstatState *vs, int err)
-{
-    if (err < 0) {
-        goto out;
-    }
-
-    err = vs->offset;
-
-out:
-    v9fs_stat_free(&vs->v9stat);
-    complete_pdu(s, vs->pdu, err);
-    qemu_free(vs);
-}
-
-static void v9fs_wstat_post_rename(V9fsState *s, V9fsWstatState *vs, int err)
-{
-    if (err < 0) {
-        goto out;
-    }
-    if (vs->v9stat.length != -1) {
-        if (v9fs_do_truncate(s, &vs->fidp->path, vs->v9stat.length) < 0) {
-            err = -errno;
-        }
-    }
-    v9fs_wstat_post_truncate(s, vs, err);
-    return;
-
-out:
-    v9fs_stat_free(&vs->v9stat);
-    complete_pdu(s, vs->pdu, err);
-    qemu_free(vs);
-}
-
 static int v9fs_complete_rename(V9fsState *s, V9fsRenameState *vs)
 {
     int err = 0;
     char *old_name, *new_name;
     char *end;
 
+    qemu_rwmutex_wrlock(&global_rename_lock);
     if (vs->newdirfid != -1) {
         V9fsFidState *dirfidp;
         dirfidp = lookup_fid(s, vs->newdirfid);
@@ -2955,6 +2923,9 @@  static int v9fs_complete_rename(V9fsState *s, V9fsRenameState *vs)
             * Fixup fid's pointing to the old name to
             * start pointing to the new name
             */
+            /* FIXME: Since we are holding the global rename
+             * lock in write mode there is no need to hold fid_list_lock
+             */
             qemu_rwmutex_rdlock(&s->fid_list_lock);
             for (fidp = s->fid_list; fidp; fidp = fidp->next) {
                 if (vs->fidp == fidp) {
@@ -2976,41 +2947,14 @@  static int v9fs_complete_rename(V9fsState *s, V9fsRenameState *vs)
         }
     }
 out:
+    qemu_rwmutex_unlock(&global_rename_lock);
     v9fs_string_free(&vs->name);
     return err;
 }
 
 static void v9fs_rename_post_rename(V9fsState *s, V9fsRenameState *vs, int err)
 {
-    complete_pdu(s, vs->pdu, err);
-    qemu_free(vs);
-}
-
-static void v9fs_wstat_post_chown(V9fsState *s, V9fsWstatState *vs, int err)
-{
-    if (err < 0) {
-        goto out;
-    }
-
-    if (vs->v9stat.name.size != 0) {
-        V9fsRenameState *vr;
-
-        vr = qemu_mallocz(sizeof(V9fsRenameState));
-        vr->newdirfid = -1;
-        vr->pdu = vs->pdu;
-        vr->fidp = vs->fidp;
-        vr->offset = vs->offset;
-        vr->name.size = vs->v9stat.name.size;
-        vr->name.data = qemu_strdup(vs->v9stat.name.data);
-
-        err = v9fs_complete_rename(s, vr);
-        qemu_free(vr);
-    }
-    v9fs_wstat_post_rename(s, vs, err);
-    return;
-
-out:
-    v9fs_stat_free(&vs->v9stat);
+/* FIXME: Needed?    v9fs_stat_free(&vs->v9stat); */
     complete_pdu(s, vs->pdu, err);
     qemu_free(vs);
 }
@@ -3043,31 +2987,55 @@  out:
     qemu_free(vs);
 }
 
-static void v9fs_wstat_post_utime(V9fsState *s, V9fsWstatState *vs, int err)
+static void v9fs_wstat_do_complete(void *opaque)
 {
-    if (err < 0) {
+    V9fsWstatState *vs = (V9fsWstatState *)opaque;
+    complete_pdu(vs->s, vs->pdu, vs->err);
+    v9fs_stat_free(&vs->v9stat);
+    qemu_free(vs);
+}
+
+static void v9fs_wstat_worker(ThreadletWork *work)
+{
+    uint32_t v9_mode;
+    V9fsWstatState *vs = container_of(work, V9fsWstatState, work);
+
+    vs->fidp = lookup_fid(vs->s, vs->fid);
+    if (vs->fidp == NULL) {
+        vs->err = -EINVAL;
         goto out;
     }
 
-    if (vs->v9stat.n_gid != -1 || vs->v9stat.n_uid != -1) {
-        if (v9fs_do_chown(s, &vs->fidp->path, vs->v9stat.n_uid,
-                    vs->v9stat.n_gid)) {
-            err = -errno;
+    /* do we need to sync the file? */
+    if (donttouch_stat(&vs->v9stat)) {
+        vs->err = v9fs_do_fsync(vs->s, vs->fidp->fs.fd);
+        if (vs->err == -1) {
+            vs->err = -errno;
+            goto out;
         }
     }
-    v9fs_wstat_post_chown(s, vs, err);
-    return;
 
-out:
-    v9fs_stat_free(&vs->v9stat);
-    complete_pdu(s, vs->pdu, err);
-    qemu_free(vs);
-}
+    if (vs->v9stat.mode != -1) {
+        qemu_rwmutex_rdlock(&global_rename_lock);
+        vs->err = v9fs_do_lstat(vs->s, &vs->fidp->path, &vs->stbuf);
+        if (vs->err == -1) {
+            vs->err = -errno;
+            goto out_unlock;
+        }
+        v9_mode = stat_to_v9mode(&vs->stbuf);
 
-static void v9fs_wstat_post_chmod(V9fsState *s, V9fsWstatState *vs, int err)
-{
-    if (err < 0) {
-        goto out;
+        if ((vs->v9stat.mode & P9_STAT_MODE_TYPE_BITS) !=
+            (v9_mode & P9_STAT_MODE_TYPE_BITS)) {
+                /* Attempting to change the type */
+                vs->err = -EIO;
+                goto out_unlock;
+        }
+        if (v9fs_do_chmod(vs->s, &vs->fidp->path, v9mode_to_mode(vs->v9stat.mode,
+                        &vs->v9stat.extension))) {
+                vs->err = -errno;
+                goto out_unlock;
+        }
+        qemu_rwmutex_unlock(&global_rename_lock);
     }
 
     if (vs->v9stat.mtime != -1 || vs->v9stat.atime != -1) {
@@ -3085,99 +3053,73 @@  static void v9fs_wstat_post_chmod(V9fsState *s, V9fsWstatState *vs, int err)
             times[1].tv_nsec = UTIME_OMIT;
         }
 
-        if (v9fs_do_utimensat(s, &vs->fidp->path, times)) {
-            err = -errno;
+        qemu_rwmutex_rdlock(&global_rename_lock);
+        if (v9fs_do_utimensat(vs->s, &vs->fidp->path, times)) {
+            vs->err = -errno;
+            goto out_unlock;
         }
+        qemu_rwmutex_unlock(&global_rename_lock);
     }
 
-    v9fs_wstat_post_utime(s, vs, err);
-    return;
-
-out:
-    v9fs_stat_free(&vs->v9stat);
-    complete_pdu(s, vs->pdu, err);
-    qemu_free(vs);
-}
-
-static void v9fs_wstat_post_fsync(V9fsState *s, V9fsWstatState *vs, int err)
-{
-    if (err == -1) {
-        err = -errno;
+    if (vs->v9stat.n_gid != -1 || vs->v9stat.n_uid != -1) {
+        qemu_rwmutex_rdlock(&global_rename_lock);
+        if (v9fs_do_chown(vs->s, &vs->fidp->path, vs->v9stat.n_uid,
+                    vs->v9stat.n_gid)) {
+            vs->err = -errno;
+            goto out_unlock;
+        }
+        qemu_rwmutex_unlock(&global_rename_lock);
     }
-    v9fs_stat_free(&vs->v9stat);
-    complete_pdu(s, vs->pdu, err);
-    qemu_free(vs);
-}
 
-static void v9fs_wstat_post_lstat(V9fsState *s, V9fsWstatState *vs, int err)
-{
-    uint32_t v9_mode;
-
-    if (err == -1) {
-        err = -errno;
-        goto out;
-    }
+    if (vs->v9stat.name.size != 0) {
+        V9fsRenameState *vr;
 
-    v9_mode = stat_to_v9mode(&vs->stbuf);
+        vr = qemu_mallocz(sizeof(V9fsRenameState));
+        vr->newdirfid= -1;
+        vr->pdu = vs->pdu;
+        vr->fidp = vs->fidp;
+        vr->offset = vs->offset;
+        vr->name.size = vs->v9stat.name.size;
+        vr->name.data = qemu_strdup(vs->v9stat.name.data);
 
-    if ((vs->v9stat.mode & P9_STAT_MODE_TYPE_BITS) !=
-        (v9_mode & P9_STAT_MODE_TYPE_BITS)) {
-            /* Attempting to change the type */
-            err = -EIO;
+        vs->err = v9fs_complete_rename(vs->s, vr);
+        qemu_free(vr);
+        if (vs->err < 0) {
             goto out;
+        }
     }
 
-    if (v9fs_do_chmod(s, &vs->fidp->path, v9mode_to_mode(vs->v9stat.mode,
-                    &vs->v9stat.extension))) {
-            err = -errno;
-     }
-    v9fs_wstat_post_chmod(s, vs, err);
-    return;
+    if (vs->v9stat.length != -1) {
+        qemu_rwmutex_rdlock(&global_rename_lock);
+        if (v9fs_do_truncate(vs->s, &vs->fidp->path, vs->v9stat.length) < 0) {
+            vs->err = -errno;
+            goto out_unlock;
+        }
+        qemu_rwmutex_unlock(&global_rename_lock);
+    }
+
+    goto out;
 
+out_unlock:
+    qemu_rwmutex_unlock(&global_rename_lock);
 out:
-    v9fs_stat_free(&vs->v9stat);
-    complete_pdu(s, vs->pdu, err);
-    qemu_free(vs);
+    v9fs_async_helper_done(v9fs_wstat_do_complete, vs);
 }
 
 static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
 {
-    int32_t fid;
     V9fsWstatState *vs;
-    int err = 0;
 
     vs = qemu_malloc(sizeof(*vs));
     vs->pdu = pdu;
     vs->offset = 7;
+    vs->s = s;
 
-    pdu_unmarshal(pdu, vs->offset, "dwS", &fid, &vs->unused, &vs->v9stat);
-
-    vs->fidp = lookup_fid(s, fid);
-    if (vs->fidp == NULL) {
-        err = -EINVAL;
-        goto out;
-    }
-
-    /* do we need to sync the file? */
-    if (donttouch_stat(&vs->v9stat)) {
-        err = v9fs_do_fsync(s, vs->fidp->fs.fd);
-        v9fs_wstat_post_fsync(s, vs, err);
-        return;
-    }
+    pdu_unmarshal(pdu, vs->offset, "dwS", &vs->fid, &vs->unused, &vs->v9stat);
 
-    if (vs->v9stat.mode != -1) {
-        err = v9fs_do_lstat(s, &vs->fidp->path, &vs->stbuf);
-        v9fs_wstat_post_lstat(s, vs, err);
-        return;
-    }
-
-    v9fs_wstat_post_chmod(s, vs, err);
+    vs->work.func = v9fs_wstat_worker;
+    submit_threadlet(&vs->work);
     return;
-
-out:
-    v9fs_stat_free(&vs->v9stat);
-    complete_pdu(s, vs->pdu, err);
-    qemu_free(vs);
 }
 
 static void v9fs_statfs_post_statfs(V9fsState *s, V9fsStatfsState *vs, int err)
diff --git a/hw/virtio-9p.h b/hw/virtio-9p.h
index 9ced508..59f7a4e 100644
--- a/hw/virtio-9p.h
+++ b/hw/virtio-9p.h
@@ -354,6 +354,10 @@  typedef struct V9fsWstatState
     V9fsStat v9stat;
     V9fsFidState *fidp;
     struct stat stbuf;
+    V9fsState *s;
+    int32_t fid;
+    int32_t err;
+    ThreadletWork work;
 } V9fsWstatState;
 
 typedef struct V9fsSymlinkState