[Qemu-devel] [PATCH 1/6] hw/9pfs: Add reference counting for fid

[Qemu-devel] [PATCH 1/6] hw/9pfs: Add reference counting for fid

[Aneesh Kumar K.V]

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p.c |  205 +++++++++++++++++++++++++++++++++++----------------
 hw/9pfs/virtio-9p.h |    7 ++
 2 files changed, 147 insertions(+), 65 deletions(-)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index e2aa863..03d8664 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -232,12 +232,13 @@ static size_t v9fs_string_size(V9fsString *str)
     return str->size;
 }
 
-static V9fsFidState *lookup_fid(V9fsState *s, int32_t fid)
+static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
 {
     V9fsFidState *f;
 
     for (f = s->fid_list; f; f = f->next) {
         if (f->fid == fid) {
+            f->ref++;
             return f;
         }
     }
@@ -249,16 +250,16 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
 {
     V9fsFidState *f;
 
-    f = lookup_fid(s, fid);
+    f = get_fid(s, fid);
     if (f) {
+        f->ref--;
         return NULL;
     }
 
     f = qemu_mallocz(sizeof(V9fsFidState));
-
     f->fid = fid;
     f->fid_type = P9_FID_NONE;
-
+    f->ref = 1;
     f->next = s->fid_list;
     s->fid_list = f;
 
@@ -1014,19 +1015,22 @@ static void v9fs_attach(void *opaque)
     fidp = alloc_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     fidp->uid = n_uname;
     v9fs_string_sprintf(&fidp->path, "%s", "/");
     err = fid_to_qid(s, fidp, &qid);
     if (err < 0) {
         err = -EINVAL;
+        put_fid(fidp);
         free_fid(s, fid);
-        goto out;
+        goto out_nofid;
     }
     offset += pdu_marshal(pdu, offset, "Q", &qid);
     err = offset;
-out:
+    put_fid(fidp);
+
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&uname);
     v9fs_string_free(&aname);
@@ -1045,10 +1049,10 @@ static void v9fs_stat(void *opaque)
 
     pdu_unmarshal(pdu, offset, "d", &fid);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     err = v9fs_co_lstat(s, &fidp->path, &stbuf);
     if (err < 0) {
@@ -1062,6 +1066,8 @@ static void v9fs_stat(void *opaque)
     err = offset;
     v9fs_stat_free(&v9stat);
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1079,10 +1085,10 @@ static void v9fs_getattr(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         retval = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     /*
      * Currently we only support BASIC fields in stat, so there is no
@@ -1096,6 +1102,8 @@ static void v9fs_getattr(void *opaque)
     retval = offset;
     retval += pdu_marshal(pdu, offset, "A", &v9stat_dotl);
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, retval);
 }
 
@@ -1123,10 +1131,10 @@ static void v9fs_setattr(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dI", &fid, &v9iattr);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     if (v9iattr.valid & ATTR_MODE) {
         err = v9fs_co_chmod(s, &fidp->path, v9iattr.mode);
@@ -1188,6 +1196,8 @@ static void v9fs_setattr(void *opaque)
     }
     err = offset;
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1214,7 +1224,7 @@ static void v9fs_walk(void *opaque)
     int32_t fid, newfid;
     V9fsString *wnames = NULL;
     V9fsFidState *fidp;
-    V9fsFidState *newfidp;
+    V9fsFidState *newfidp = NULL;;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
 
@@ -1231,12 +1241,12 @@ static void v9fs_walk(void *opaque)
 
     } else if (nwnames > P9_MAXWELEM) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     if (fid == newfid) {
         BUG_ON(fidp->fid_type != P9_FID_NONE);
@@ -1269,7 +1279,9 @@ static void v9fs_walk(void *opaque)
             v9fs_string_copy(&newfidp->path, &path);
             err = v9fs_co_lstat(s, &newfidp->path, &stbuf);
             if (err < 0) {
+                put_fid(newfidp);
                 free_fid(s, newfidp->fid);
+                newfidp = NULL;
                 v9fs_string_free(&path);
                 goto out;
             }
@@ -1279,6 +1291,11 @@ static void v9fs_walk(void *opaque)
     }
     err = v9fs_walk_marshal(pdu, nwnames, qids);
 out:
+    put_fid(fidp);
+    if (newfidp) {
+        put_fid(newfidp);
+    }
+out_nofid:
     complete_pdu(s, pdu, err);
     if (nwnames && nwnames <= P9_MAXWELEM) {
         for (name_idx = 0; name_idx < nwnames; name_idx++) {
@@ -1327,10 +1344,10 @@ static void v9fs_open(void *opaque)
     } else {
         pdu_unmarshal(pdu, offset, "db", &fid, &mode);
     }
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     BUG_ON(fidp->fid_type != P9_FID_NONE);
 
@@ -1366,6 +1383,8 @@ static void v9fs_open(void *opaque)
         err = offset;
     }
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1388,10 +1407,10 @@ static void v9fs_lcreate(void *opaque)
     pdu_unmarshal(pdu, offset, "dsddd", &dfid, &name, &flags,
                   &mode, &gid);
 
-    fidp = lookup_fid(pdu->s, dfid);
+    fidp = get_fid(pdu->s, dfid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
 
@@ -1418,6 +1437,8 @@ static void v9fs_lcreate(void *opaque)
     offset += pdu_marshal(pdu, offset, "Qd", &qid, iounit);
     err = offset;
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
     v9fs_string_free(&name);
     v9fs_string_free(&fullname);
@@ -1434,7 +1455,7 @@ static void v9fs_fsync(void *opaque)
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "dd", &fid, &datasync);
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
         goto out;
@@ -1444,6 +1465,7 @@ static void v9fs_fsync(void *opaque)
         err = offset;
     }
 out:
+    put_fid(fidp);
     complete_pdu(s, pdu, err);
 }
 
@@ -1561,10 +1583,10 @@ static void v9fs_read(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dqd", &fid, &off, &max_count);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     if (fidp->fid_type == P9_FID_DIR) {
 
@@ -1616,6 +1638,8 @@ static void v9fs_read(void *opaque)
         err = -EINVAL;
     }
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1700,8 +1724,12 @@ static void v9fs_readdir(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dqd", &fid, &initial_offset, &max_count);
 
-    fidp = lookup_fid(s, fid);
-    if (fidp == NULL || !fidp->fs.dir) {
+    fidp = get_fid(s, fid);
+    if (fidp == NULL) {
+        retval = -EINVAL;
+        goto out_nofid;
+    }
+    if (!fidp->fs.dir) {
         retval = -EINVAL;
         goto out;
     }
@@ -1719,6 +1747,8 @@ static void v9fs_readdir(void *opaque)
     retval += pdu_marshal(pdu, offset, "d", count);
     retval += count;
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, retval);
 }
 
@@ -1784,10 +1814,10 @@ static void v9fs_write(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dqdv", &fid, &off, &count, sg, &cnt);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     if (fidp->fid_type == P9_FID_FILE) {
         if (fidp->fs.fd == -1) {
@@ -1827,6 +1857,8 @@ static void v9fs_write(void *opaque)
     offset += pdu_marshal(pdu, offset, "d", total);
     err = offset;
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1851,10 +1883,10 @@ static void v9fs_create(void *opaque)
     pdu_unmarshal(pdu, offset, "dsdbs", &fid, &name,
                   &perm, &mode, &extension);
 
-    fidp = lookup_fid(pdu->s, fid);
+    fidp = get_fid(pdu->s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
 
     v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
@@ -1884,15 +1916,17 @@ static void v9fs_create(void *opaque)
         }
     } else if (perm & P9_STAT_MODE_LINK) {
         int32_t nfid = atoi(extension.data);
-        V9fsFidState *nfidp = lookup_fid(pdu->s, nfid);
+        V9fsFidState *nfidp = get_fid(pdu->s, nfid);
         if (nfidp == NULL) {
             err = -EINVAL;
             goto out;
         }
         err = v9fs_co_link(pdu->s, &nfidp->path, &fullname);
         if (err < 0) {
+            put_fid(nfidp);
             goto out;
         }
+        put_fid(nfidp);
     } else if (perm & P9_STAT_MODE_DEVICE) {
         char ctype;
         uint32_t major, minor;
@@ -1956,6 +1990,8 @@ static void v9fs_create(void *opaque)
     err = offset;
 
 out:
+    put_fid(fidp);
+out_nofid:
    complete_pdu(pdu->s, pdu, err);
    v9fs_string_free(&name);
    v9fs_string_free(&extension);
@@ -1980,10 +2016,10 @@ static void v9fs_symlink(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dssd", &dfid, &name, &symname, &gid);
 
-    dfidp = lookup_fid(pdu->s, dfid);
+    dfidp = get_fid(pdu->s, dfid);
     if (dfidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
 
     v9fs_string_sprintf(&fullname, "%s/%s", dfidp->path.data, name.data);
@@ -1999,6 +2035,8 @@ static void v9fs_symlink(void *opaque)
     offset += pdu_marshal(pdu, offset, "Q", &qid);
     err = offset;
 out:
+    put_fid(dfidp);
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
     v9fs_string_free(&name);
     v9fs_string_free(&symname);
@@ -2028,13 +2066,13 @@ static void v9fs_link(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dds", &dfid, &oldfid, &name);
 
-    dfidp = lookup_fid(s, dfid);
+    dfidp = get_fid(s, dfid);
     if (dfidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
 
-    oldfidp = lookup_fid(s, oldfid);
+    oldfidp = get_fid(s, oldfid);
     if (oldfidp == NULL) {
         err = -ENOENT;
         goto out;
@@ -2048,6 +2086,8 @@ static void v9fs_link(void *opaque)
     v9fs_string_free(&fullname);
 
 out:
+    put_fid(dfidp);
+out_nofid:
     v9fs_string_free(&name);
     complete_pdu(s, pdu, err);
 }
@@ -2062,10 +2102,10 @@ static void v9fs_remove(void *opaque)
 
     pdu_unmarshal(pdu, offset, "d", &fid);
 
-    fidp = lookup_fid(pdu->s, fid);
+    fidp = get_fid(pdu->s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     err = v9fs_co_remove(pdu->s, &fidp->path);
     if (!err) {
@@ -2073,8 +2113,9 @@ static void v9fs_remove(void *opaque)
     }
 
     /* For TREMOVE we need to clunk the fid even on failed remove */
+    put_fid(fidp);
     free_fid(pdu->s, fidp->fid);
-out:
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
 }
 
@@ -2083,14 +2124,14 @@ static int v9fs_complete_rename(V9fsState *s, V9fsFidState *fidp,
 {
     char *end;
     int err = 0;
+    V9fsFidState *dirfidp = NULL;
     char *old_name, *new_name;
 
     if (newdirfid != -1) {
-        V9fsFidState *dirfidp;
-        dirfidp = lookup_fid(s, newdirfid);
+        dirfidp = get_fid(s, newdirfid);
         if (dirfidp == NULL) {
             err = -ENOENT;
-            goto out;
+            goto out_nofid;
         }
         BUG_ON(dirfidp->fid_type != P9_FID_NONE);
 
@@ -2143,6 +2184,10 @@ static int v9fs_complete_rename(V9fsState *s, V9fsFidState *fidp,
         v9fs_string_copy(&fidp->path, name);
     }
 out:
+    if (dirfidp) {
+        put_fid(dirfidp);
+    }
+out_nofid:
     return err;
 }
 
@@ -2159,10 +2204,10 @@ static void v9fs_rename(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dds", &fid, &newdirfid, &name);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     BUG_ON(fidp->fid_type != P9_FID_NONE);
 
@@ -2171,6 +2216,8 @@ static void v9fs_rename(void *opaque)
         err = offset;
     }
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&name);
 }
@@ -2189,10 +2236,10 @@ static void v9fs_wstat(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dwS", &fid, &unused, &v9stat);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     /* do we need to sync the file? */
     if (donttouch_stat(&v9stat)) {
@@ -2258,6 +2305,8 @@ static void v9fs_wstat(void *opaque)
     }
     err = offset;
 out:
+    put_fid(fidp);
+out_nofid:
     v9fs_stat_free(&v9stat);
     complete_pdu(s, pdu, err);
 }
@@ -2318,10 +2367,10 @@ static void v9fs_statfs(void *opaque)
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "d", &fid);
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         retval = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     retval = v9fs_co_statfs(s, &fidp->path, &stbuf);
     if (retval < 0) {
@@ -2330,6 +2379,8 @@ static void v9fs_statfs(void *opaque)
     retval = offset;
     retval += v9fs_fill_statfs(s, pdu, &stbuf);
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, retval);
     return;
 }
@@ -2355,10 +2406,10 @@ static void v9fs_mknod(void *opaque)
     pdu_unmarshal(pdu, offset, "dsdddd", &fid, &name, &mode,
                   &major, &minor, &gid);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
     err = v9fs_co_mknod(s, &fullname, fidp->uid, gid,
@@ -2374,6 +2425,8 @@ static void v9fs_mknod(void *opaque)
     err = offset;
     err += pdu_marshal(pdu, offset, "Q", &qid);
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&fullname);
     v9fs_string_free(&name);
@@ -2407,12 +2460,12 @@ static void v9fs_lock(void *opaque)
     /* We support only block flag now (that too ignored currently) */
     if (flock->flags & ~P9_LOCK_FLAGS_BLOCK) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     err = v9fs_co_fstat(s, fidp->fs.fd, &stbuf);
     if (err < 0) {
@@ -2420,6 +2473,8 @@ static void v9fs_lock(void *opaque)
     }
     status = P9_LOCK_SUCCESS;
 out:
+    put_fid(fidp);
+out_nofid:
     err = offset;
     err += pdu_marshal(pdu, offset, "b", status);
     complete_pdu(s, pdu, err);
@@ -2445,10 +2500,10 @@ static void v9fs_getlock(void *opaque)
                   &glock->start, &glock->length, &glock->proc_id,
                   &glock->client_id);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     err = v9fs_co_fstat(s, fidp->fs.fd, &stbuf);
     if (err < 0) {
@@ -2460,6 +2515,8 @@ static void v9fs_getlock(void *opaque)
                           &glock->client_id);
     err = offset;
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
     qemu_free(glock);
 }
@@ -2480,10 +2537,10 @@ static void v9fs_mkdir(void *opaque)
     v9fs_string_init(&fullname);
     pdu_unmarshal(pdu, offset, "dsdd", &fid, &name, &mode, &gid);
 
-    fidp = lookup_fid(pdu->s, fid);
+    fidp = get_fid(pdu->s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
     err = v9fs_co_mkdir(pdu->s, fullname.data, mode, fidp->uid, gid);
@@ -2498,6 +2555,8 @@ static void v9fs_mkdir(void *opaque)
     offset += pdu_marshal(pdu, offset, "Q", &qid);
     err = offset;
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
     v9fs_string_free(&fullname);
     v9fs_string_free(&name);
@@ -2511,15 +2570,15 @@ static void v9fs_xattrwalk(void *opaque)
     size_t offset = 7;
     int32_t fid, newfid;
     V9fsFidState *file_fidp;
-    V9fsFidState *xattr_fidp;
+    V9fsFidState *xattr_fidp = NULL;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "dds", &fid, &newfid, &name);
-    file_fidp = lookup_fid(s, fid);
+    file_fidp = get_fid(s, fid);
     if (file_fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     xattr_fidp = alloc_fid(s, newfid);
     if (xattr_fidp == NULL) {
@@ -2534,7 +2593,9 @@ static void v9fs_xattrwalk(void *opaque)
         size = v9fs_co_llistxattr(s, &xattr_fidp->path, NULL, 0);
         if (size < 0) {
             err = size;
+            put_fid(xattr_fidp);
             free_fid(s, xattr_fidp->fid);
+            xattr_fidp = NULL;
             goto out;
         }
         /*
@@ -2549,7 +2610,9 @@ static void v9fs_xattrwalk(void *opaque)
                                      xattr_fidp->fs.xattr.value,
                                      xattr_fidp->fs.xattr.len);
             if (err < 0) {
+                put_fid(xattr_fidp);
                 free_fid(s, xattr_fidp->fid);
+                xattr_fidp = NULL;
                 goto out;
             }
         }
@@ -2564,7 +2627,9 @@ static void v9fs_xattrwalk(void *opaque)
                                  &name, NULL, 0);
         if (size < 0) {
             err = size;
+            put_fid(xattr_fidp);
             free_fid(s, xattr_fidp->fid);
+            xattr_fidp = NULL;
             goto out;
         }
         /*
@@ -2579,7 +2644,9 @@ static void v9fs_xattrwalk(void *opaque)
                                     &name, xattr_fidp->fs.xattr.value,
                                     xattr_fidp->fs.xattr.len);
             if (err < 0) {
+                put_fid(xattr_fidp);
                 free_fid(s, xattr_fidp->fid);
+                xattr_fidp = NULL;
                 goto out;
             }
         }
@@ -2587,6 +2654,11 @@ static void v9fs_xattrwalk(void *opaque)
         err = offset;
     }
 out:
+    put_fid(file_fidp);
+    if (xattr_fidp) {
+        put_fid(xattr_fidp);
+    }
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&name);
 }
@@ -2607,10 +2679,10 @@ static void v9fs_xattrcreate(void *opaque)
     pdu_unmarshal(pdu, offset, "dsqd",
                   &fid, &name, &size, &flags);
 
-    file_fidp = lookup_fid(s, fid);
+    file_fidp = get_fid(s, fid);
     if (file_fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     /* Make the file fid point to xattr */
     xattr_fidp = file_fidp;
@@ -2626,7 +2698,8 @@ static void v9fs_xattrcreate(void *opaque)
         xattr_fidp->fs.xattr.value = NULL;
     }
     err = offset;
-out:
+    put_fid(file_fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&name);
 }
@@ -2641,10 +2714,10 @@ static void v9fs_readlink(void *opaque)
     V9fsFidState *fidp;
 
     pdu_unmarshal(pdu, offset, "d", &fid);
-    fidp = lookup_fid(pdu->s, fid);
+    fidp = get_fid(pdu->s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
 
     v9fs_string_init(&target);
@@ -2656,6 +2729,8 @@ static void v9fs_readlink(void *opaque)
     err = offset;
     v9fs_string_free(&target);
 out:
+    put_fid(fidp);
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
 }
 
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 1d8c1b1..ce93c20 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -203,6 +203,7 @@ struct V9fsFidState
 	V9fsXattr xattr;
     } fs;
     uid_t uid;
+    int ref;
     V9fsFidState *next;
 };
 
@@ -361,5 +362,11 @@ static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count,
     return pdu_packunpack(dst, sg, sg_count, offset, size, 0);
 }
 
+static inline void put_fid(V9fsFidState *fidp)
+{
+    BUG_ON(!fidp->ref);
+    fidp->ref--;
+}
+
 extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH 2/6] hw/9pfs: Don't free fid in clunk

[Aneesh Kumar K.V]

On interrupted syscall on client we can end up having fid
being acted upon by glib pool but still get a clunk request on that

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p.c |  139 +++++++++++++++++++++++++++-----------------------
 hw/9pfs/virtio-9p.h |    7 +--
 2 files changed, 76 insertions(+), 70 deletions(-)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 03d8664..f3127a5 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -237,12 +237,11 @@ static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
     V9fsFidState *f;
 
     for (f = s->fid_list; f; f = f->next) {
-        if (f->fid == fid) {
+        if (f->fid == fid && !f->clunked) {
             f->ref++;
             return f;
         }
     }
-
     return NULL;
 }
 
@@ -250,12 +249,12 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
 {
     V9fsFidState *f;
 
-    f = get_fid(s, fid);
-    if (f) {
-        f->ref--;
-        return NULL;
+    for (f = s->fid_list; f; f = f->next) {
+        /* If fid is already there return NULL */
+        if (f->fid == fid && !f->clunked) {
+            return NULL;
+        }
     }
-
     f = qemu_mallocz(sizeof(V9fsFidState));
     f->fid = fid;
     f->fid_type = P9_FID_NONE;
@@ -300,9 +299,33 @@ free_value:
     return retval;
 }
 
-static int free_fid(V9fsState *s, int32_t fid)
+static int release_fid(V9fsState *s, V9fsFidState *fidp)
 {
     int retval = 0;
+
+    if (fidp->fid_type == P9_FID_FILE) {
+        retval = v9fs_co_close(s, fidp);
+    } else if (fidp->fid_type == P9_FID_DIR) {
+        retval = v9fs_co_closedir(s, fidp);
+    } else if (fidp->fid_type == P9_FID_XATTR) {
+        retval = v9fs_xattr_fid_clunk(s, fidp);
+    }
+    v9fs_string_free(&fidp->path);
+    qemu_free(fidp);
+    return retval;
+}
+
+static void put_fid(V9fsState *s, V9fsFidState *fidp)
+{
+    BUG_ON(!fidp->ref);
+    fidp->ref--;
+    if (!fidp->ref && fidp->clunked) {
+        release_fid(s, fidp);
+    }
+}
+
+static int free_fid(V9fsState *s, int32_t fid)
+{
     V9fsFidState **fidpp, *fidp;
 
     for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) {
@@ -314,20 +337,10 @@ static int free_fid(V9fsState *s, int32_t fid)
     if (*fidpp == NULL) {
         return -ENOENT;
     }
-
     fidp = *fidpp;
     *fidpp = fidp->next;
-
-    if (fidp->fid_type == P9_FID_FILE) {
-        retval = v9fs_co_close(s, fidp);
-    } else if (fidp->fid_type == P9_FID_DIR) {
-        retval = v9fs_co_closedir(s, fidp);
-    } else if (fidp->fid_type == P9_FID_XATTR) {
-        retval = v9fs_xattr_fid_clunk(s, fidp);
-    }
-    v9fs_string_free(&fidp->path);
-    qemu_free(fidp);
-    return retval;
+    fidp->clunked = 1;
+    return 0;
 }
 
 #define P9_QID_TYPE_DIR         0x80
@@ -1022,14 +1035,13 @@ static void v9fs_attach(void *opaque)
     err = fid_to_qid(s, fidp, &qid);
     if (err < 0) {
         err = -EINVAL;
-        put_fid(fidp);
         free_fid(s, fid);
-        goto out_nofid;
+        goto out;
     }
     offset += pdu_marshal(pdu, offset, "Q", &qid);
     err = offset;
-    put_fid(fidp);
-
+out:
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&uname);
@@ -1066,7 +1078,7 @@ static void v9fs_stat(void *opaque)
     err = offset;
     v9fs_stat_free(&v9stat);
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
 }
@@ -1102,7 +1114,7 @@ static void v9fs_getattr(void *opaque)
     retval = offset;
     retval += pdu_marshal(pdu, offset, "A", &v9stat_dotl);
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, retval);
 }
@@ -1196,7 +1208,7 @@ static void v9fs_setattr(void *opaque)
     }
     err = offset;
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
 }
@@ -1279,9 +1291,7 @@ static void v9fs_walk(void *opaque)
             v9fs_string_copy(&newfidp->path, &path);
             err = v9fs_co_lstat(s, &newfidp->path, &stbuf);
             if (err < 0) {
-                put_fid(newfidp);
                 free_fid(s, newfidp->fid);
-                newfidp = NULL;
                 v9fs_string_free(&path);
                 goto out;
             }
@@ -1291,9 +1301,9 @@ static void v9fs_walk(void *opaque)
     }
     err = v9fs_walk_marshal(pdu, nwnames, qids);
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
     if (newfidp) {
-        put_fid(newfidp);
+        put_fid(s, newfidp);
     }
 out_nofid:
     complete_pdu(s, pdu, err);
@@ -1383,7 +1393,7 @@ static void v9fs_open(void *opaque)
         err = offset;
     }
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
 }
@@ -1437,7 +1447,7 @@ static void v9fs_lcreate(void *opaque)
     offset += pdu_marshal(pdu, offset, "Qd", &qid, iounit);
     err = offset;
 out:
-    put_fid(fidp);
+    put_fid(pdu->s, fidp);
 out_nofid:
     complete_pdu(pdu->s, pdu, err);
     v9fs_string_free(&name);
@@ -1465,7 +1475,7 @@ static void v9fs_fsync(void *opaque)
         err = offset;
     }
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
     complete_pdu(s, pdu, err);
 }
 
@@ -1474,16 +1484,25 @@ static void v9fs_clunk(void *opaque)
     int err;
     int32_t fid;
     size_t offset = 7;
+    V9fsFidState *fidp;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "d", &fid);
-    err = free_fid(s, fid);
+
+    fidp = get_fid(s, fid);
+    if (fidp == NULL) {
+        err = -ENOENT;
+        goto out_nofid;
+    }
+    err = free_fid(s, fidp->fid);
     if (err < 0) {
         goto out;
     }
     err = offset;
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1638,7 +1657,7 @@ static void v9fs_read(void *opaque)
         err = -EINVAL;
     }
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
 }
@@ -1747,7 +1766,7 @@ static void v9fs_readdir(void *opaque)
     retval += pdu_marshal(pdu, offset, "d", count);
     retval += count;
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, retval);
 }
@@ -1857,7 +1876,7 @@ static void v9fs_write(void *opaque)
     offset += pdu_marshal(pdu, offset, "d", total);
     err = offset;
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
 }
@@ -1923,10 +1942,10 @@ static void v9fs_create(void *opaque)
         }
         err = v9fs_co_link(pdu->s, &nfidp->path, &fullname);
         if (err < 0) {
-            put_fid(nfidp);
+            put_fid(pdu->s, nfidp);
             goto out;
         }
-        put_fid(nfidp);
+        put_fid(pdu->s, nfidp);
     } else if (perm & P9_STAT_MODE_DEVICE) {
         char ctype;
         uint32_t major, minor;
@@ -1990,7 +2009,7 @@ static void v9fs_create(void *opaque)
     err = offset;
 
 out:
-    put_fid(fidp);
+    put_fid(pdu->s, fidp);
 out_nofid:
    complete_pdu(pdu->s, pdu, err);
    v9fs_string_free(&name);
@@ -2035,7 +2054,7 @@ static void v9fs_symlink(void *opaque)
     offset += pdu_marshal(pdu, offset, "Q", &qid);
     err = offset;
 out:
-    put_fid(dfidp);
+    put_fid(pdu->s, dfidp);
 out_nofid:
     complete_pdu(pdu->s, pdu, err);
     v9fs_string_free(&name);
@@ -2086,7 +2105,7 @@ static void v9fs_link(void *opaque)
     v9fs_string_free(&fullname);
 
 out:
-    put_fid(dfidp);
+    put_fid(s, dfidp);
 out_nofid:
     v9fs_string_free(&name);
     complete_pdu(s, pdu, err);
@@ -2113,8 +2132,8 @@ static void v9fs_remove(void *opaque)
     }
 
     /* For TREMOVE we need to clunk the fid even on failed remove */
-    put_fid(fidp);
     free_fid(pdu->s, fidp->fid);
+    put_fid(pdu->s, fidp);
 out_nofid:
     complete_pdu(pdu->s, pdu, err);
 }
@@ -2185,7 +2204,7 @@ static int v9fs_complete_rename(V9fsState *s, V9fsFidState *fidp,
     }
 out:
     if (dirfidp) {
-        put_fid(dirfidp);
+        put_fid(s, dirfidp);
     }
 out_nofid:
     return err;
@@ -2216,7 +2235,7 @@ static void v9fs_rename(void *opaque)
         err = offset;
     }
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&name);
@@ -2305,7 +2324,7 @@ static void v9fs_wstat(void *opaque)
     }
     err = offset;
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     v9fs_stat_free(&v9stat);
     complete_pdu(s, pdu, err);
@@ -2379,7 +2398,7 @@ static void v9fs_statfs(void *opaque)
     retval = offset;
     retval += v9fs_fill_statfs(s, pdu, &stbuf);
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, retval);
     return;
@@ -2425,7 +2444,7 @@ static void v9fs_mknod(void *opaque)
     err = offset;
     err += pdu_marshal(pdu, offset, "Q", &qid);
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&fullname);
@@ -2473,7 +2492,7 @@ static void v9fs_lock(void *opaque)
     }
     status = P9_LOCK_SUCCESS;
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     err = offset;
     err += pdu_marshal(pdu, offset, "b", status);
@@ -2515,7 +2534,7 @@ static void v9fs_getlock(void *opaque)
                           &glock->client_id);
     err = offset;
 out:
-    put_fid(fidp);
+    put_fid(s, fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
     qemu_free(glock);
@@ -2555,7 +2574,7 @@ static void v9fs_mkdir(void *opaque)
     offset += pdu_marshal(pdu, offset, "Q", &qid);
     err = offset;
 out:
-    put_fid(fidp);
+    put_fid(pdu->s, fidp);
 out_nofid:
     complete_pdu(pdu->s, pdu, err);
     v9fs_string_free(&fullname);
@@ -2593,9 +2612,7 @@ static void v9fs_xattrwalk(void *opaque)
         size = v9fs_co_llistxattr(s, &xattr_fidp->path, NULL, 0);
         if (size < 0) {
             err = size;
-            put_fid(xattr_fidp);
             free_fid(s, xattr_fidp->fid);
-            xattr_fidp = NULL;
             goto out;
         }
         /*
@@ -2610,9 +2627,7 @@ static void v9fs_xattrwalk(void *opaque)
                                      xattr_fidp->fs.xattr.value,
                                      xattr_fidp->fs.xattr.len);
             if (err < 0) {
-                put_fid(xattr_fidp);
                 free_fid(s, xattr_fidp->fid);
-                xattr_fidp = NULL;
                 goto out;
             }
         }
@@ -2627,9 +2642,7 @@ static void v9fs_xattrwalk(void *opaque)
                                  &name, NULL, 0);
         if (size < 0) {
             err = size;
-            put_fid(xattr_fidp);
             free_fid(s, xattr_fidp->fid);
-            xattr_fidp = NULL;
             goto out;
         }
         /*
@@ -2644,9 +2657,7 @@ static void v9fs_xattrwalk(void *opaque)
                                     &name, xattr_fidp->fs.xattr.value,
                                     xattr_fidp->fs.xattr.len);
             if (err < 0) {
-                put_fid(xattr_fidp);
                 free_fid(s, xattr_fidp->fid);
-                xattr_fidp = NULL;
                 goto out;
             }
         }
@@ -2654,9 +2665,9 @@ static void v9fs_xattrwalk(void *opaque)
         err = offset;
     }
 out:
-    put_fid(file_fidp);
+    put_fid(s, file_fidp);
     if (xattr_fidp) {
-        put_fid(xattr_fidp);
+        put_fid(s, xattr_fidp);
     }
 out_nofid:
     complete_pdu(s, pdu, err);
@@ -2698,7 +2709,7 @@ static void v9fs_xattrcreate(void *opaque)
         xattr_fidp->fs.xattr.value = NULL;
     }
     err = offset;
-    put_fid(file_fidp);
+    put_fid(s, file_fidp);
 out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&name);
@@ -2729,7 +2740,7 @@ static void v9fs_readlink(void *opaque)
     err = offset;
     v9fs_string_free(&target);
 out:
-    put_fid(fidp);
+    put_fid(pdu->s, fidp);
 out_nofid:
     complete_pdu(pdu->s, pdu, err);
 }
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index ce93c20..e16e5f4 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -204,6 +204,7 @@ struct V9fsFidState
     } fs;
     uid_t uid;
     int ref;
+    int clunked;
     V9fsFidState *next;
 };
 
@@ -362,11 +363,5 @@ static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count,
     return pdu_packunpack(dst, sg, sg_count, offset, size, 0);
 }
 
-static inline void put_fid(V9fsFidState *fidp)
-{
-    BUG_ON(!fidp->ref);
-    fidp->ref--;
-}
-
 extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
 #endif
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 2/6] hw/9pfs: Don't free fid in clunk

[Venkateswararao Jujjuri]

On 06/06/2011 10:16 AM, Aneesh Kumar K.V wrote:
> On interrupted syscall on client we can end up having fid
> being acted upon by glib pool but still get a clunk request on that

Couple of comments below.

- JV

> Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
> ---
>   hw/9pfs/virtio-9p.c |  139 +++++++++++++++++++++++++++-----------------------
>   hw/9pfs/virtio-9p.h |    7 +--
>   2 files changed, 76 insertions(+), 70 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 03d8664..f3127a5 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -237,12 +237,11 @@ static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
>       V9fsFidState *f;
>
>       for (f = s->fid_list; f; f = f->next) {
> -        if (f->fid == fid) {
> +        if (f->fid == fid&&  !f->clunked) {
>               f->ref++;
>               return f;
>           }
>       }
> -
>       return NULL;
>   }
>
> @@ -250,12 +249,12 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
>   {
>       V9fsFidState *f;
>
> -    f = get_fid(s, fid);
> -    if (f) {
> -        f->ref--;
> -        return NULL;
> +    for (f = s->fid_list; f; f = f->next) {
> +        /* If fid is already there return NULL */
> +        if (f->fid == fid&&  !f->clunked) {
> +            return NULL;
> +        }
>       }
> -
>       f = qemu_mallocz(sizeof(V9fsFidState));
Memory leak if we find a cluncked fid here?
More than that how do you handle this?
>       f->fid = fid;
>       f->fid_type = P9_FID_NONE;
> @@ -300,9 +299,33 @@ free_value:
>       return retval;
>   }
>
> -static int free_fid(V9fsState *s, int32_t fid)
> +static int release_fid(V9fsState *s, V9fsFidState *fidp)
>   {
>       int retval = 0;
> +
> +    if (fidp->fid_type == P9_FID_FILE) {
> +        retval = v9fs_co_close(s, fidp);
> +    } else if (fidp->fid_type == P9_FID_DIR) {
> +        retval = v9fs_co_closedir(s, fidp);
> +    } else if (fidp->fid_type == P9_FID_XATTR) {
> +        retval = v9fs_xattr_fid_clunk(s, fidp);
> +    }
> +    v9fs_string_free(&fidp->path);
> +    qemu_free(fidp);
> +    return retval;
> +}
> +
> +static void put_fid(V9fsState *s, V9fsFidState *fidp)
> +{
> +    BUG_ON(!fidp->ref);
> +    fidp->ref--;
> +    if (!fidp->ref&&  fidp->clunked) {
> +        release_fid(s, fidp);
> +    }
> +}
> +
> +static int free_fid(V9fsState *s, int32_t fid)
With the changed semantics; I would suggest you to swap the names of
release_fid() and free_fid()

Or even better
free_fid() -> clunk_fid()
release_fid() -> free_fid()
> +{
>       V9fsFidState **fidpp, *fidp;
>
>       for (fidpp =&s->fid_list; *fidpp; fidpp =&(*fidpp)->next) {
> @@ -314,20 +337,10 @@ static int free_fid(V9fsState *s, int32_t fid)
>       if (*fidpp == NULL) {
>           return -ENOENT;
>       }
> -
>       fidp = *fidpp;
>       *fidpp = fidp->next;
> -
> -    if (fidp->fid_type == P9_FID_FILE) {
> -        retval = v9fs_co_close(s, fidp);
> -    } else if (fidp->fid_type == P9_FID_DIR) {
> -        retval = v9fs_co_closedir(s, fidp);
> -    } else if (fidp->fid_type == P9_FID_XATTR) {
> -        retval = v9fs_xattr_fid_clunk(s, fidp);
> -    }
> -    v9fs_string_free(&fidp->path);
> -    qemu_free(fidp);
> -    return retval;
> +    fidp->clunked = 1;
> +    return 0;
>   }
>
>   #define P9_QID_TYPE_DIR         0x80
> @@ -1022,14 +1035,13 @@ static void v9fs_attach(void *opaque)
>       err = fid_to_qid(s, fidp,&qid);
>       if (err<  0) {
>           err = -EINVAL;
> -        put_fid(fidp);
>           free_fid(s, fid);
> -        goto out_nofid;
> +        goto out;
>       }
>       offset += pdu_marshal(pdu, offset, "Q",&qid);
>       err = offset;
> -    put_fid(fidp);
> -
> +out:
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>       v9fs_string_free(&uname);
> @@ -1066,7 +1078,7 @@ static void v9fs_stat(void *opaque)
>       err = offset;
>       v9fs_stat_free(&v9stat);
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>   }
> @@ -1102,7 +1114,7 @@ static void v9fs_getattr(void *opaque)
>       retval = offset;
>       retval += pdu_marshal(pdu, offset, "A",&v9stat_dotl);
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, retval);
>   }
> @@ -1196,7 +1208,7 @@ static void v9fs_setattr(void *opaque)
>       }
>       err = offset;
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>   }
> @@ -1279,9 +1291,7 @@ static void v9fs_walk(void *opaque)
>               v9fs_string_copy(&newfidp->path,&path);
>               err = v9fs_co_lstat(s,&newfidp->path,&stbuf);
>               if (err<  0) {
> -                put_fid(newfidp);
>                   free_fid(s, newfidp->fid);
> -                newfidp = NULL;
>                   v9fs_string_free(&path);
>                   goto out;
>               }
> @@ -1291,9 +1301,9 @@ static void v9fs_walk(void *opaque)
>       }
>       err = v9fs_walk_marshal(pdu, nwnames, qids);
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>       if (newfidp) {
> -        put_fid(newfidp);
> +        put_fid(s, newfidp);
>       }
>   out_nofid:
>       complete_pdu(s, pdu, err);
> @@ -1383,7 +1393,7 @@ static void v9fs_open(void *opaque)
>           err = offset;
>       }
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>   }
> @@ -1437,7 +1447,7 @@ static void v9fs_lcreate(void *opaque)
>       offset += pdu_marshal(pdu, offset, "Qd",&qid, iounit);
>       err = offset;
>   out:
> -    put_fid(fidp);
> +    put_fid(pdu->s, fidp);
>   out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>       v9fs_string_free(&name);
> @@ -1465,7 +1475,7 @@ static void v9fs_fsync(void *opaque)
>           err = offset;
>       }
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>       complete_pdu(s, pdu, err);
>   }
>
> @@ -1474,16 +1484,25 @@ static void v9fs_clunk(void *opaque)
>       int err;
>       int32_t fid;
>       size_t offset = 7;
> +    V9fsFidState *fidp;
>       V9fsPDU *pdu = opaque;
>       V9fsState *s = pdu->s;
>
>       pdu_unmarshal(pdu, offset, "d",&fid);
> -    err = free_fid(s, fid);
> +
> +    fidp = get_fid(s, fid);
> +    if (fidp == NULL) {
> +        err = -ENOENT;
> +        goto out_nofid;
> +    }
> +    err = free_fid(s, fidp->fid);
>       if (err<  0) {
>           goto out;
>       }
>       err = offset;
>   out:
> +    put_fid(s, fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>   }
>
> @@ -1638,7 +1657,7 @@ static void v9fs_read(void *opaque)
>           err = -EINVAL;
>       }
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>   }
> @@ -1747,7 +1766,7 @@ static void v9fs_readdir(void *opaque)
>       retval += pdu_marshal(pdu, offset, "d", count);
>       retval += count;
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, retval);
>   }
> @@ -1857,7 +1876,7 @@ static void v9fs_write(void *opaque)
>       offset += pdu_marshal(pdu, offset, "d", total);
>       err = offset;
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>   }
> @@ -1923,10 +1942,10 @@ static void v9fs_create(void *opaque)
>           }
>           err = v9fs_co_link(pdu->s,&nfidp->path,&fullname);
>           if (err<  0) {
> -            put_fid(nfidp);
> +            put_fid(pdu->s, nfidp);
>               goto out;
>           }
> -        put_fid(nfidp);
> +        put_fid(pdu->s, nfidp);
>       } else if (perm&  P9_STAT_MODE_DEVICE) {
>           char ctype;
>           uint32_t major, minor;
> @@ -1990,7 +2009,7 @@ static void v9fs_create(void *opaque)
>       err = offset;
>
>   out:
> -    put_fid(fidp);
> +    put_fid(pdu->s, fidp);
>   out_nofid:
>      complete_pdu(pdu->s, pdu, err);
>      v9fs_string_free(&name);
> @@ -2035,7 +2054,7 @@ static void v9fs_symlink(void *opaque)
>       offset += pdu_marshal(pdu, offset, "Q",&qid);
>       err = offset;
>   out:
> -    put_fid(dfidp);
> +    put_fid(pdu->s, dfidp);
>   out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>       v9fs_string_free(&name);
> @@ -2086,7 +2105,7 @@ static void v9fs_link(void *opaque)
>       v9fs_string_free(&fullname);
>
>   out:
> -    put_fid(dfidp);
> +    put_fid(s, dfidp);
>   out_nofid:
>       v9fs_string_free(&name);
>       complete_pdu(s, pdu, err);
> @@ -2113,8 +2132,8 @@ static void v9fs_remove(void *opaque)
>       }
>
>       /* For TREMOVE we need to clunk the fid even on failed remove */
> -    put_fid(fidp);
>       free_fid(pdu->s, fidp->fid);
> +    put_fid(pdu->s, fidp);
>   out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>   }
> @@ -2185,7 +2204,7 @@ static int v9fs_complete_rename(V9fsState *s, V9fsFidState *fidp,
>       }
>   out:
>       if (dirfidp) {
> -        put_fid(dirfidp);
> +        put_fid(s, dirfidp);
>       }
>   out_nofid:
>       return err;
> @@ -2216,7 +2235,7 @@ static void v9fs_rename(void *opaque)
>           err = offset;
>       }
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>       v9fs_string_free(&name);
> @@ -2305,7 +2324,7 @@ static void v9fs_wstat(void *opaque)
>       }
>       err = offset;
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       v9fs_stat_free(&v9stat);
>       complete_pdu(s, pdu, err);
> @@ -2379,7 +2398,7 @@ static void v9fs_statfs(void *opaque)
>       retval = offset;
>       retval += v9fs_fill_statfs(s, pdu,&stbuf);
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, retval);
>       return;
> @@ -2425,7 +2444,7 @@ static void v9fs_mknod(void *opaque)
>       err = offset;
>       err += pdu_marshal(pdu, offset, "Q",&qid);
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>       v9fs_string_free(&fullname);
> @@ -2473,7 +2492,7 @@ static void v9fs_lock(void *opaque)
>       }
>       status = P9_LOCK_SUCCESS;
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       err = offset;
>       err += pdu_marshal(pdu, offset, "b", status);
> @@ -2515,7 +2534,7 @@ static void v9fs_getlock(void *opaque)
>                             &glock->client_id);
>       err = offset;
>   out:
> -    put_fid(fidp);
> +    put_fid(s, fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>       qemu_free(glock);
> @@ -2555,7 +2574,7 @@ static void v9fs_mkdir(void *opaque)
>       offset += pdu_marshal(pdu, offset, "Q",&qid);
>       err = offset;
>   out:
> -    put_fid(fidp);
> +    put_fid(pdu->s, fidp);
>   out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>       v9fs_string_free(&fullname);
> @@ -2593,9 +2612,7 @@ static void v9fs_xattrwalk(void *opaque)
>           size = v9fs_co_llistxattr(s,&xattr_fidp->path, NULL, 0);
>           if (size<  0) {
>               err = size;
> -            put_fid(xattr_fidp);
>               free_fid(s, xattr_fidp->fid);
> -            xattr_fidp = NULL;
>               goto out;
>           }
>           /*
> @@ -2610,9 +2627,7 @@ static void v9fs_xattrwalk(void *opaque)
>                                        xattr_fidp->fs.xattr.value,
>                                        xattr_fidp->fs.xattr.len);
>               if (err<  0) {
> -                put_fid(xattr_fidp);
>                   free_fid(s, xattr_fidp->fid);
> -                xattr_fidp = NULL;
>                   goto out;
>               }
>           }
> @@ -2627,9 +2642,7 @@ static void v9fs_xattrwalk(void *opaque)
>                                    &name, NULL, 0);
>           if (size<  0) {
>               err = size;
> -            put_fid(xattr_fidp);
>               free_fid(s, xattr_fidp->fid);
> -            xattr_fidp = NULL;
>               goto out;
>           }
>           /*
> @@ -2644,9 +2657,7 @@ static void v9fs_xattrwalk(void *opaque)
>                                       &name, xattr_fidp->fs.xattr.value,
>                                       xattr_fidp->fs.xattr.len);
>               if (err<  0) {
> -                put_fid(xattr_fidp);
>                   free_fid(s, xattr_fidp->fid);
> -                xattr_fidp = NULL;
>                   goto out;
>               }
>           }
> @@ -2654,9 +2665,9 @@ static void v9fs_xattrwalk(void *opaque)
>           err = offset;
>       }
>   out:
> -    put_fid(file_fidp);
> +    put_fid(s, file_fidp);
>       if (xattr_fidp) {
> -        put_fid(xattr_fidp);
> +        put_fid(s, xattr_fidp);
>       }
>   out_nofid:
>       complete_pdu(s, pdu, err);
> @@ -2698,7 +2709,7 @@ static void v9fs_xattrcreate(void *opaque)
>           xattr_fidp->fs.xattr.value = NULL;
>       }
>       err = offset;
> -    put_fid(file_fidp);
> +    put_fid(s, file_fidp);
>   out_nofid:
>       complete_pdu(s, pdu, err);
>       v9fs_string_free(&name);
> @@ -2729,7 +2740,7 @@ static void v9fs_readlink(void *opaque)
>       err = offset;
>       v9fs_string_free(&target);
>   out:
> -    put_fid(fidp);
> +    put_fid(pdu->s, fidp);
>   out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>   }
> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> index ce93c20..e16e5f4 100644
> --- a/hw/9pfs/virtio-9p.h
> +++ b/hw/9pfs/virtio-9p.h
> @@ -204,6 +204,7 @@ struct V9fsFidState
>       } fs;
>       uid_t uid;
>       int ref;
> +    int clunked;
>       V9fsFidState *next;
>   };
>
> @@ -362,11 +363,5 @@ static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count,
>       return pdu_packunpack(dst, sg, sg_count, offset, size, 0);
>   }
>
> -static inline void put_fid(V9fsFidState *fidp)
> -{
> -    BUG_ON(!fidp->ref);
> -    fidp->ref--;
> -}
> -
>   extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
>   #endif

Re: [Qemu-devel] [PATCH 2/6] hw/9pfs: Don't free fid in clunk

[Aneesh Kumar K.V]

On Thu, 09 Jun 2011 17:27:00 -0700, Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com> wrote:
> On 06/06/2011 10:16 AM, Aneesh Kumar K.V wrote:
> > On interrupted syscall on client we can end up having fid
> > being acted upon by glib pool but still get a clunk request on that
> 
> Couple of comments below.
> 
> - JV
> 
> > Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
> > ---
> >   hw/9pfs/virtio-9p.c |  139 +++++++++++++++++++++++++++-----------------------
> >   hw/9pfs/virtio-9p.h |    7 +--
> >   2 files changed, 76 insertions(+), 70 deletions(-)
> >
> > diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> > index 03d8664..f3127a5 100644
> > --- a/hw/9pfs/virtio-9p.c
> > +++ b/hw/9pfs/virtio-9p.c
> > @@ -237,12 +237,11 @@ static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
> >       V9fsFidState *f;
> >
> >       for (f = s->fid_list; f; f = f->next) {
> > -        if (f->fid == fid) {
> > +        if (f->fid == fid&&  !f->clunked) {
> >               f->ref++;
> >               return f;
> >           }
> >       }
> > -
> >       return NULL;
> >   }
> >
> > @@ -250,12 +249,12 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
> >   {
> >       V9fsFidState *f;
> >
> > -    f = get_fid(s, fid);
> > -    if (f) {
> > -        f->ref--;
> > -        return NULL;
> > +    for (f = s->fid_list; f; f = f->next) {
> > +        /* If fid is already there return NULL */
> > +        if (f->fid == fid&&  !f->clunked) {
> > +            return NULL;
> > +        }
> >       }
> > -
> >       f = qemu_mallocz(sizeof(V9fsFidState));
> Memory leak if we find a cluncked fid here?
> More than that how do you handle this?

clunk request happening parallel to alloc request ? is that 
possible. Only when the alloc succeed the client will find the
fid initialized and only on initialized fid we send clunk request.

> >       f->fid = fid;
> >       f->fid_type = P9_FID_NONE;
> > @@ -300,9 +299,33 @@ free_value:
> >       return retval;
> >   }
> >
> > -static int free_fid(V9fsState *s, int32_t fid)
> > +static int release_fid(V9fsState *s, V9fsFidState *fidp)
> >   {
> >       int retval = 0;
> > +
> > +    if (fidp->fid_type == P9_FID_FILE) {
> > +        retval = v9fs_co_close(s, fidp);
> > +    } else if (fidp->fid_type == P9_FID_DIR) {
> > +        retval = v9fs_co_closedir(s, fidp);
> > +    } else if (fidp->fid_type == P9_FID_XATTR) {
> > +        retval = v9fs_xattr_fid_clunk(s, fidp);
> > +    }
> > +    v9fs_string_free(&fidp->path);
> > +    qemu_free(fidp);
> > +    return retval;
> > +}
> > +
> > +static void put_fid(V9fsState *s, V9fsFidState *fidp)
> > +{
> > +    BUG_ON(!fidp->ref);
> > +    fidp->ref--;
> > +    if (!fidp->ref&&  fidp->clunked) {
> > +        release_fid(s, fidp);
> > +    }
> > +}
> > +
> > +static int free_fid(V9fsState *s, int32_t fid)
> With the changed semantics; I would suggest you to swap the names of
> release_fid() and free_fid()
> 
> Or even better
> free_fid() -> clunk_fid()
> release_fid() -> free_fid()

ok 

-aneesh

[Qemu-devel] [PATCH 3/6] hw/9pfs: Add file descriptor reclaim support

[Aneesh Kumar K.V]

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/9pfs/codir.c            |    4 +-
 hw/9pfs/cofile.c           |   19 ++++-
 hw/9pfs/virtio-9p-coth.h   |    4 +-
 hw/9pfs/virtio-9p-device.c |    1 +
 hw/9pfs/virtio-9p.c        |  193 +++++++++++++++++++++++++++++++++++++++++++-
 hw/9pfs/virtio-9p.h        |   23 +++++-
 6 files changed, 229 insertions(+), 15 deletions(-)

diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 783d279..3347038 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -101,12 +101,10 @@ int v9fs_co_opendir(V9fsState *s, V9fsFidState *fidp)
     return err;
 }
 
-int v9fs_co_closedir(V9fsState *s, V9fsFidState *fidp)
+int v9fs_co_closedir(V9fsState *s, DIR *dir)
 {
     int err;
-    DIR *dir;
 
-    dir = fidp->fs.dir;
     v9fs_co_run_in_worker(
         {
             err = s->ops->closedir(&s->ctx, dir);
diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
index e388146..0caf1e3 100644
--- a/hw/9pfs/cofile.c
+++ b/hw/9pfs/cofile.c
@@ -58,6 +58,12 @@ int v9fs_co_open(V9fsState *s, V9fsFidState *fidp, int flags)
                 err = 0;
             }
         });
+    if (!err) {
+        total_open_fd++;
+        if (total_open_fd > open_fd_hw) {
+            v9fs_reclaim_fd(s);
+        }
+    }
     return err;
 }
 
@@ -79,15 +85,19 @@ int v9fs_co_open2(V9fsState *s, V9fsFidState *fidp, char *fullname, gid_t gid,
                 err = -errno;
             }
         });
+    if (!err) {
+        total_open_fd++;
+        if (total_open_fd > open_fd_hw) {
+            v9fs_reclaim_fd(s);
+        }
+    }
     return err;
 }
 
-int v9fs_co_close(V9fsState *s, V9fsFidState *fidp)
+int v9fs_co_close(V9fsState *s, int fd)
 {
-    int fd;
     int err;
 
-    fd = fidp->fs.fd;
     v9fs_co_run_in_worker(
         {
             err = s->ops->close(&s->ctx, fd);
@@ -95,6 +105,9 @@ int v9fs_co_close(V9fsState *s, V9fsFidState *fidp)
                 err = -errno;
             }
         });
+    if (!err) {
+        total_open_fd--;
+    }
     return err;
 }
 
diff --git a/hw/9pfs/virtio-9p-coth.h b/hw/9pfs/virtio-9p-coth.h
index 48defb7..b7be9b5 100644
--- a/hw/9pfs/virtio-9p-coth.h
+++ b/hw/9pfs/virtio-9p-coth.h
@@ -83,8 +83,8 @@ extern int v9fs_co_open2(V9fsState *, V9fsFidState *, char *, gid_t, int, int);
 extern int v9fs_co_lsetxattr(V9fsState *, V9fsString *, V9fsString *,
                              void *, size_t, int);
 extern int v9fs_co_lremovexattr(V9fsState *, V9fsString *, V9fsString *);
-extern int v9fs_co_closedir(V9fsState *, V9fsFidState *);
-extern int v9fs_co_close(V9fsState *, V9fsFidState *);
+extern int v9fs_co_closedir(V9fsState *, DIR *);
+extern int v9fs_co_close(V9fsState *, int);
 extern int v9fs_co_fsync(V9fsState *, V9fsFidState *, int);
 extern int v9fs_co_symlink(V9fsState *, V9fsFidState *, const char *,
                            const char *, gid_t);
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 7811103..70629b2 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -171,6 +171,7 @@ static PCIDeviceInfo virtio_9p_info = {
 static void virtio_9p_register_devices(void)
 {
     pci_qdev_register(&virtio_9p_info);
+    virtio_9p_set_fd_limit();
 }
 
 device_init(virtio_9p_register_devices)
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index f3127a5..21e07fb 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -22,6 +22,9 @@
 #include "virtio-9p-coth.h"
 
 int debug_9p_pdu;
+int open_fd_hw;
+int total_open_fd;
+static int open_fd_rc;
 
 enum {
     Oread   = 0x00,
@@ -234,11 +237,40 @@ static size_t v9fs_string_size(V9fsString *str)
 
 static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
 {
+    int err;
     V9fsFidState *f;
 
     for (f = s->fid_list; f; f = f->next) {
         if (f->fid == fid && !f->clunked) {
+            /*
+             * Update the fid ref upfront so that
+             * we don't get reclaimed when we yield
+             * in open later.
+             */
             f->ref++;
+
+            /*
+             * check whether we need to reopen the
+             * file. We might have closed the fd
+             * while trying to free up some file
+             * descriptors.
+             */
+            if (f->fid_type == P9_FID_FILE) {
+                if (f->fs.fd == -1) {
+                    do {
+                        err = v9fs_co_open(s, f, f->open_flags);
+                    } while (err == -EINTR);
+                    if (err < 0) {
+                        f->ref--;
+                        return NULL;
+                    }
+                }
+            }
+            /*
+             * Mark the fid as referenced so that the LRU
+             * reclaim won't close the file descriptor
+             */
+            f->flags |= FID_REFERENCED;
             return f;
         }
     }
@@ -259,6 +291,11 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
     f->fid = fid;
     f->fid_type = P9_FID_NONE;
     f->ref = 1;
+    /*
+     * Mark the fid as referenced so that the LRU
+     * reclaim won't close the file descriptor
+     */
+    f->flags |= FID_REFERENCED;
     f->next = s->fid_list;
     s->fid_list = f;
 
@@ -304,9 +341,12 @@ static int release_fid(V9fsState *s, V9fsFidState *fidp)
     int retval = 0;
 
     if (fidp->fid_type == P9_FID_FILE) {
-        retval = v9fs_co_close(s, fidp);
+        /* If we reclaimed the fd no need to close */
+        if (fidp->fs.fd != -1) {
+            retval = v9fs_co_close(s, fidp->fs.fd);
+        }
     } else if (fidp->fid_type == P9_FID_DIR) {
-        retval = v9fs_co_closedir(s, fidp);
+        retval = v9fs_co_closedir(s, fidp->fs.dir);
     } else if (fidp->fid_type == P9_FID_XATTR) {
         retval = v9fs_xattr_fid_clunk(s, fidp);
     }
@@ -319,7 +359,10 @@ static void put_fid(V9fsState *s, V9fsFidState *fidp)
 {
     BUG_ON(!fidp->ref);
     fidp->ref--;
-    if (!fidp->ref && fidp->clunked) {
+    /*
+     * Don't free the fid if it is in reclaim list
+     */
+    if (!fidp->ref && fidp->clunked && !fidp->rclm_lst) {
         release_fid(s, fidp);
     }
 }
@@ -343,6 +386,105 @@ static int free_fid(V9fsState *s, int32_t fid)
     return 0;
 }
 
+void v9fs_reclaim_fd(V9fsState *s)
+{
+    int reclaim_count = 0;
+    V9fsFidState *f, head_fid, *reclaim_list = NULL;
+
+    /*
+     * We don't want multiple coroutine to do reclaim
+     */
+    if (s->reclaim_in_prgs) {
+        return;
+    }
+    s->reclaim_in_prgs = 1;
+
+    head_fid.next = s->fid_list;
+    for (f = s->fid_list; f; f = f->next) {
+        /*
+         * Unlink fids cannot be reclaimed. Check
+         * for them and skip them. Also skip fids
+         * currently being operated on.
+         */
+        if (f->ref || f->flags & FID_NON_RECLAIMABLE) {
+            continue;
+        }
+        /*
+         * if it is a recently referenced fid
+         * we leave the fid untouched and clear the
+         * reference bit. We come back to it later
+         * in the next iteration. (a simple LRU without
+         * moving list elements around)
+         */
+        if (f->flags & FID_REFERENCED) {
+            f->flags &= ~FID_REFERENCED;
+            continue;
+        }
+        /*
+         * Add fids to reclaim list.
+         */
+        if (f->fid_type == P9_FID_FILE) {
+            if (f->fs.fd != -1) {
+                f->rclm_lst = reclaim_list;
+                reclaim_list = f;
+                f->fs_reclaim.fd = f->fs.fd;
+                f->fs.fd = -1;
+                reclaim_count++;
+            }
+        }
+        if (reclaim_count >= open_fd_rc) {
+            break;
+        }
+    }
+    /*
+     * Now close the fid in reclaim list. Free them if they
+     * are already clunked.
+     */
+    while (reclaim_list) {
+        f = reclaim_list;
+        reclaim_list = f->rclm_lst;
+        if (f->clunked) {
+            release_fid(s, f);
+        } else {
+            if (f->fid_type == P9_FID_FILE) {
+                v9fs_co_close(s, f->fs_reclaim.fd);
+            }
+            f->rclm_lst = NULL;
+        }
+    }
+    s->reclaim_in_prgs = 0;
+}
+
+static int v9fs_mark_fids_unreclaim(V9fsState *s, V9fsString *str)
+{
+    int err;
+    V9fsFidState *fidp, head_fid;
+
+    head_fid.next = s->fid_list;
+    for (fidp = s->fid_list; fidp; fidp = fidp->next) {
+        if (!strcmp(fidp->path.data, str->data)) {
+            /* Mark the fid non reclaimable. */
+            fidp->flags |= FID_NON_RECLAIMABLE;
+            /* reopen the file if already closed */
+            if (fidp->fs.fd == -1) {
+                do {
+                    err = v9fs_co_open(s, fidp, fidp->open_flags);
+                } while (err == -EINTR);
+                if (err < 0) {
+                    return -1;
+                }
+                /*
+                 * Go back to head of fid list because
+                 * the list could have got updated when
+                 * switched to the worker thread
+                 */
+                fidp = &head_fid;
+            }
+        }
+    }
+    return 0;
+}
+
 #define P9_QID_TYPE_DIR         0x80
 #define P9_QID_TYPE_SYMLINK     0x02
 
@@ -1388,6 +1530,14 @@ static void v9fs_open(void *opaque)
             goto out;
         }
         fidp->fid_type = P9_FID_FILE;
+        fidp->open_flags = flags;
+        if (flags & O_EXCL) {
+            /*
+             * We let the host file system do O_EXCL check
+             * We should not reclaim such fd
+             */
+            fidp->flags |= FID_NON_RECLAIMABLE;
+        }
         iounit = get_iounit(s, &fidp->path);
         offset += pdu_marshal(pdu, offset, "Qd", &qid, iounit);
         err = offset;
@@ -1432,6 +1582,14 @@ static void v9fs_lcreate(void *opaque)
         goto out;
     }
     fidp->fid_type = P9_FID_FILE;
+    fidp->open_flags = flags;
+    if (flags & O_EXCL) {
+        /*
+         * We let the host file system do O_EXCL check
+         * We should not reclaim such fd
+         */
+        fidp->flags |= FID_NON_RECLAIMABLE;
+    }
     iounit =  get_iounit(pdu->s, &fullname);
 
     err = v9fs_co_lstat(pdu->s, &fullname, &stbuf);
@@ -1993,6 +2151,14 @@ static void v9fs_create(void *opaque)
             goto out;
         }
         fidp->fid_type = P9_FID_FILE;
+        fidp->open_flags = omode_to_uflags(mode);
+        if (fidp->open_flags & O_EXCL) {
+            /*
+             * We let the host file system do O_EXCL check
+             * We should not reclaim such fd
+             */
+            fidp->flags |= FID_NON_RECLAIMABLE;
+        }
     }
     err = v9fs_co_lstat(pdu->s, &fullname, &stbuf);
     if (err < 0) {
@@ -2126,11 +2292,19 @@ static void v9fs_remove(void *opaque)
         err = -EINVAL;
         goto out_nofid;
     }
+    /*
+     * IF the file is unlinked, we cannot reopen
+     * the file later. So don't reclaim fd
+     */
+    err = v9fs_mark_fids_unreclaim(pdu->s, &fidp->path);
+    if (err < 0) {
+        goto out_err;
+    }
     err = v9fs_co_remove(pdu->s, &fidp->path);
     if (!err) {
         err = offset;
     }
-
+out_err:
     /* For TREMOVE we need to clunk the fid even on failed remove */
     free_fid(pdu->s, fidp->fid);
     put_fid(pdu->s, fidp);
@@ -2826,3 +3000,14 @@ void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
     }
     free_pdu(s, pdu);
 }
+
+void virtio_9p_set_fd_limit(void)
+{
+    struct rlimit rlim;
+    if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
+        fprintf(stderr, "Failed to get the resource limit\n");
+        exit(1);
+    }
+    open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur/3);
+    open_fd_rc = rlim.rlim_cur/2;
+}
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index e16e5f4..36aa4a5 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -5,6 +5,7 @@
 #include <dirent.h>
 #include <sys/time.h>
 #include <utime.h>
+#include <sys/resource.h>
 #include "hw/virtio.h"
 #include "fsdev/file-op-9p.h"
 
@@ -101,6 +102,9 @@ enum p9_proto_version {
 #define P9_NOTAG    (u16)(~0)
 #define P9_NOFID    (u32)(~0)
 #define P9_MAXWELEM 16
+
+#define FID_REFERENCED          0x1
+#define FID_NON_RECLAIMABLE     0x2
 static inline const char *rpath(FsContext *ctx, const char *path, char *buffer)
 {
     snprintf(buffer, PATH_MAX, "%s/%s", ctx->fs_root, path);
@@ -198,14 +202,21 @@ struct V9fsFidState
     int32_t fid;
     V9fsString path;
     union {
-	int fd;
-	DIR *dir;
-	V9fsXattr xattr;
+        int fd;
+        DIR *dir;
+        V9fsXattr xattr;
     } fs;
+    union {
+        int fd;
+        DIR *dir;
+    } fs_reclaim;
+    int flags;
+    int open_flags;
     uid_t uid;
     int ref;
     int clunked;
     V9fsFidState *next;
+    V9fsFidState *rclm_lst;
 };
 
 typedef struct V9fsState
@@ -222,6 +233,7 @@ typedef struct V9fsState
     size_t config_size;
     enum p9_proto_version proto_version;
     int32_t msize;
+    int reclaim_in_prgs;
 } V9fsState;
 
 typedef struct V9fsStatState {
@@ -354,6 +366,9 @@ typedef struct V9fsGetlock
     V9fsString client_id;
 } V9fsGetlock;
 
+extern int open_fd_hw;
+extern int total_open_fd;
+
 size_t pdu_packunpack(void *addr, struct iovec *sg, int sg_count,
                       size_t offset, size_t size, int pack);
 
@@ -364,4 +379,6 @@ static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count,
 }
 
 extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
+extern void virtio_9p_set_fd_limit(void);
+extern void v9fs_reclaim_fd(V9fsState *s);
 #endif
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 3/6] hw/9pfs: Add file descriptor reclaim support

[Venkateswararao Jujjuri]

On 06/06/2011 10:16 AM, Aneesh Kumar K.V wrote:
> Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
Minor comments below; I think this looks good.

Reviewed-by : Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>

> ---
>   hw/9pfs/codir.c            |    4 +-
>   hw/9pfs/cofile.c           |   19 ++++-
>   hw/9pfs/virtio-9p-coth.h   |    4 +-
>   hw/9pfs/virtio-9p-device.c |    1 +
>   hw/9pfs/virtio-9p.c        |  193 +++++++++++++++++++++++++++++++++++++++++++-
>   hw/9pfs/virtio-9p.h        |   23 +++++-
>   6 files changed, 229 insertions(+), 15 deletions(-)
>
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index 783d279..3347038 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -101,12 +101,10 @@ int v9fs_co_opendir(V9fsState *s, V9fsFidState *fidp)
>       return err;
>   }
>
> -int v9fs_co_closedir(V9fsState *s, V9fsFidState *fidp)
> +int v9fs_co_closedir(V9fsState *s, DIR *dir)
>   {
>       int err;
> -    DIR *dir;
>
> -    dir = fidp->fs.dir;
>       v9fs_co_run_in_worker(
>           {
>               err = s->ops->closedir(&s->ctx, dir);
> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
> index e388146..0caf1e3 100644
> --- a/hw/9pfs/cofile.c
> +++ b/hw/9pfs/cofile.c
> @@ -58,6 +58,12 @@ int v9fs_co_open(V9fsState *s, V9fsFidState *fidp, int flags)
>                   err = 0;
>               }
>           });
> +    if (!err) {
> +        total_open_fd++;
> +        if (total_open_fd>  open_fd_hw) {
> +            v9fs_reclaim_fd(s);
> +        }
> +    }
>       return err;
>   }
>
> @@ -79,15 +85,19 @@ int v9fs_co_open2(V9fsState *s, V9fsFidState *fidp, char *fullname, gid_t gid,
>                   err = -errno;
>               }
>           });
> +    if (!err) {
> +        total_open_fd++;
> +        if (total_open_fd>  open_fd_hw) {
> +            v9fs_reclaim_fd(s);
> +        }
> +    }
>       return err;
>   }
>
> -int v9fs_co_close(V9fsState *s, V9fsFidState *fidp)
> +int v9fs_co_close(V9fsState *s, int fd)
>   {
> -    int fd;
>       int err;
>
> -    fd = fidp->fs.fd;
>       v9fs_co_run_in_worker(
>           {
>               err = s->ops->close(&s->ctx, fd);
> @@ -95,6 +105,9 @@ int v9fs_co_close(V9fsState *s, V9fsFidState *fidp)
>                   err = -errno;
>               }
>           });
> +    if (!err) {
> +        total_open_fd--;
> +    }
>       return err;
>   }
>
> diff --git a/hw/9pfs/virtio-9p-coth.h b/hw/9pfs/virtio-9p-coth.h
> index 48defb7..b7be9b5 100644
> --- a/hw/9pfs/virtio-9p-coth.h
> +++ b/hw/9pfs/virtio-9p-coth.h
> @@ -83,8 +83,8 @@ extern int v9fs_co_open2(V9fsState *, V9fsFidState *, char *, gid_t, int, int);
>   extern int v9fs_co_lsetxattr(V9fsState *, V9fsString *, V9fsString *,
>                                void *, size_t, int);
>   extern int v9fs_co_lremovexattr(V9fsState *, V9fsString *, V9fsString *);
> -extern int v9fs_co_closedir(V9fsState *, V9fsFidState *);
> -extern int v9fs_co_close(V9fsState *, V9fsFidState *);
> +extern int v9fs_co_closedir(V9fsState *, DIR *);
> +extern int v9fs_co_close(V9fsState *, int);
>   extern int v9fs_co_fsync(V9fsState *, V9fsFidState *, int);
>   extern int v9fs_co_symlink(V9fsState *, V9fsFidState *, const char *,
>                              const char *, gid_t);
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 7811103..70629b2 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -171,6 +171,7 @@ static PCIDeviceInfo virtio_9p_info = {
>   static void virtio_9p_register_devices(void)
>   {
>       pci_qdev_register(&virtio_9p_info);
> +    virtio_9p_set_fd_limit();
>   }
>
>   device_init(virtio_9p_register_devices)
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index f3127a5..21e07fb 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -22,6 +22,9 @@
>   #include "virtio-9p-coth.h"
>
>   int debug_9p_pdu;
> +int open_fd_hw;
> +int total_open_fd;
> +static int open_fd_rc;
>
>   enum {
>       Oread   = 0x00,
> @@ -234,11 +237,40 @@ static size_t v9fs_string_size(V9fsString *str)
>
>   static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
>   {
> +    int err;
>       V9fsFidState *f;
>
>       for (f = s->fid_list; f; f = f->next) {
>           if (f->fid == fid&&  !f->clunked) {
> +            /*
> +             * Update the fid ref upfront so that
> +             * we don't get reclaimed when we yield
> +             * in open later.
> +             */
>               f->ref++;
> +
> +            /*
> +             * check whether we need to reopen the
> +             * file. We might have closed the fd
> +             * while trying to free up some file
> +             * descriptors.
> +             */
> +            if (f->fid_type == P9_FID_FILE) {
> +                if (f->fs.fd == -1) {
> +                    do {
> +                        err = v9fs_co_open(s, f, f->open_flags);
> +                    } while (err == -EINTR);
> +                    if (err<  0) {
> +                        f->ref--;
> +                        return NULL;
> +                    }
> +                }
> +            }
> +            /*
> +             * Mark the fid as referenced so that the LRU
> +             * reclaim won't close the file descriptor
> +             */
> +            f->flags |= FID_REFERENCED;
>               return f;
>           }
>       }
> @@ -259,6 +291,11 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
>       f->fid = fid;
>       f->fid_type = P9_FID_NONE;
>       f->ref = 1;
> +    /*
> +     * Mark the fid as referenced so that the LRU
> +     * reclaim won't close the file descriptor
> +     */
> +    f->flags |= FID_REFERENCED;
>       f->next = s->fid_list;
>       s->fid_list = f;
>
> @@ -304,9 +341,12 @@ static int release_fid(V9fsState *s, V9fsFidState *fidp)
>       int retval = 0;
>
>       if (fidp->fid_type == P9_FID_FILE) {
> -        retval = v9fs_co_close(s, fidp);
> +        /* If we reclaimed the fd no need to close */
> +        if (fidp->fs.fd != -1) {
> +            retval = v9fs_co_close(s, fidp->fs.fd);
> +        }
>       } else if (fidp->fid_type == P9_FID_DIR) {
> -        retval = v9fs_co_closedir(s, fidp);
> +        retval = v9fs_co_closedir(s, fidp->fs.dir);
>       } else if (fidp->fid_type == P9_FID_XATTR) {
>           retval = v9fs_xattr_fid_clunk(s, fidp);
>       }
> @@ -319,7 +359,10 @@ static void put_fid(V9fsState *s, V9fsFidState *fidp)
>   {
>       BUG_ON(!fidp->ref);
>       fidp->ref--;
> -    if (!fidp->ref&&  fidp->clunked) {
> +    /*
> +     * Don't free the fid if it is in reclaim list
> +     */
> +    if (!fidp->ref&&  fidp->clunked&&  !fidp->rclm_lst) {
>           release_fid(s, fidp);
>       }
>   }
> @@ -343,6 +386,105 @@ static int free_fid(V9fsState *s, int32_t fid)
>       return 0;
>   }
>
> +void v9fs_reclaim_fd(V9fsState *s)
> +{
> +    int reclaim_count = 0;
> +    V9fsFidState *f, head_fid, *reclaim_list = NULL;
> +
> +    /*
> +     * We don't want multiple coroutine to do reclaim
> +     */
> +    if (s->reclaim_in_prgs) {
> +        return;
> +    }
> +    s->reclaim_in_prgs = 1;
> +
> +    head_fid.next = s->fid_list;
> +    for (f = s->fid_list; f; f = f->next) {
> +        /*
> +         * Unlink fids cannot be reclaimed. Check
> +         * for them and skip them. Also skip fids
> +         * currently being operated on.
> +         */
> +        if (f->ref || f->flags&  FID_NON_RECLAIMABLE) {
> +            continue;
> +        }
> +        /*
> +         * if it is a recently referenced fid
> +         * we leave the fid untouched and clear the
> +         * reference bit. We come back to it later
> +         * in the next iteration. (a simple LRU without
> +         * moving list elements around)
> +         */
> +        if (f->flags&  FID_REFERENCED) {
> +            f->flags&= ~FID_REFERENCED;
> +            continue;
> +        }
> +        /*
> +         * Add fids to reclaim list.
> +         */
> +        if (f->fid_type == P9_FID_FILE) {
> +            if (f->fs.fd != -1) {
> +                f->rclm_lst = reclaim_list;
> +                reclaim_list = f;
> +                f->fs_reclaim.fd = f->fs.fd;
> +                f->fs.fd = -1;
> +                reclaim_count++;
> +            }
> +        }
> +        if (reclaim_count>= open_fd_rc) {
> +            break;
> +        }
> +    }
> +    /*
> +     * Now close the fid in reclaim list. Free them if they
> +     * are already clunked.
> +     */
> +    while (reclaim_list) {
> +        f = reclaim_list;
> +        reclaim_list = f->rclm_lst;
> +        if (f->clunked) {
> +            release_fid(s, f);
> +        } else {
> +            if (f->fid_type == P9_FID_FILE) {
> +                v9fs_co_close(s, f->fs_reclaim.fd);
> +            }
> +            f->rclm_lst = NULL;
> +        }
> +    }
> +    s->reclaim_in_prgs = 0;
> +}
> +
> +static int v9fs_mark_fids_unreclaim(V9fsState *s, V9fsString *str)
> +{
> +    int err;
> +    V9fsFidState *fidp, head_fid;
> +
> +    head_fid.next = s->fid_list;
> +    for (fidp = s->fid_list; fidp; fidp = fidp->next) {
> +        if (!strcmp(fidp->path.data, str->data)) {
&& !( fidp->flags && FID_NON_RECLAIMABLE)  To avoid unnecessarily 
resetting the flag.
> +            /* Mark the fid non reclaimable. */
> +            fidp->flags |= FID_NON_RECLAIMABLE;
> +            /* reopen the file if already closed */
> +            if (fidp->fs.fd == -1) {
> +                do {
> +                    err = v9fs_co_open(s, fidp, fidp->open_flags);
> +                } while (err == -EINTR);
> +                if (err<  0) {
> +                    return -1;
> +                }
> +                /*
> +                 * Go back to head of fid list because
> +                 * the list could have got updated when
> +                 * switched to the worker thread
> +                 */
> +                fidp =&head_fid;
> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
>   #define P9_QID_TYPE_DIR         0x80
>   #define P9_QID_TYPE_SYMLINK     0x02
>
> @@ -1388,6 +1530,14 @@ static void v9fs_open(void *opaque)
>               goto out;
>           }
>           fidp->fid_type = P9_FID_FILE;
> +        fidp->open_flags = flags;
> +        if (flags&  O_EXCL) {
> +            /*
> +             * We let the host file system do O_EXCL check
> +             * We should not reclaim such fd
> +             */
> +            fidp->flags |= FID_NON_RECLAIMABLE;
> +        }
>           iounit = get_iounit(s,&fidp->path);
>           offset += pdu_marshal(pdu, offset, "Qd",&qid, iounit);
>           err = offset;
> @@ -1432,6 +1582,14 @@ static void v9fs_lcreate(void *opaque)
>           goto out;
>       }
>       fidp->fid_type = P9_FID_FILE;
> +    fidp->open_flags = flags;
> +    if (flags&  O_EXCL) {
> +        /*
> +         * We let the host file system do O_EXCL check
> +         * We should not reclaim such fd
> +         */
> +        fidp->flags |= FID_NON_RECLAIMABLE;
> +    }
>       iounit =  get_iounit(pdu->s,&fullname);
>
>       err = v9fs_co_lstat(pdu->s,&fullname,&stbuf);
> @@ -1993,6 +2151,14 @@ static void v9fs_create(void *opaque)
>               goto out;
>           }
>           fidp->fid_type = P9_FID_FILE;
> +        fidp->open_flags = omode_to_uflags(mode);
> +        if (fidp->open_flags&  O_EXCL) {
> +            /*
> +             * We let the host file system do O_EXCL check
> +             * We should not reclaim such fd
> +             */
> +            fidp->flags |= FID_NON_RECLAIMABLE;
> +        }
>       }
>       err = v9fs_co_lstat(pdu->s,&fullname,&stbuf);
>       if (err<  0) {
> @@ -2126,11 +2292,19 @@ static void v9fs_remove(void *opaque)
>           err = -EINVAL;
>           goto out_nofid;
>       }
> +    /*
> +     * IF the file is unlinked, we cannot reopen
> +     * the file later. So don't reclaim fd
> +     */
> +    err = v9fs_mark_fids_unreclaim(pdu->s,&fidp->path);
> +    if (err<  0) {
> +        goto out_err;
> +    }
>       err = v9fs_co_remove(pdu->s,&fidp->path);
>       if (!err) {
>           err = offset;
>       }
> -
> +out_err:
>       /* For TREMOVE we need to clunk the fid even on failed remove */
>       free_fid(pdu->s, fidp->fid);
>       put_fid(pdu->s, fidp);
> @@ -2826,3 +3000,14 @@ void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
>       }
>       free_pdu(s, pdu);
>   }
> +
> +void virtio_9p_set_fd_limit(void)
> +{
> +    struct rlimit rlim;
> +    if (getrlimit(RLIMIT_NOFILE,&rlim)<  0) {
> +        fprintf(stderr, "Failed to get the resource limit\n");
> +        exit(1);
> +    }
> +    open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur/3);
> +    open_fd_rc = rlim.rlim_cur/2;
Why not call it LW?
> +}
> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> index e16e5f4..36aa4a5 100644
> --- a/hw/9pfs/virtio-9p.h
> +++ b/hw/9pfs/virtio-9p.h
> @@ -5,6 +5,7 @@
>   #include<dirent.h>
>   #include<sys/time.h>
>   #include<utime.h>
> +#include<sys/resource.h>
>   #include "hw/virtio.h"
>   #include "fsdev/file-op-9p.h"
>
> @@ -101,6 +102,9 @@ enum p9_proto_version {
>   #define P9_NOTAG    (u16)(~0)
>   #define P9_NOFID    (u32)(~0)
>   #define P9_MAXWELEM 16
> +
> +#define FID_REFERENCED          0x1
> +#define FID_NON_RECLAIMABLE     0x2
>   static inline const char *rpath(FsContext *ctx, const char *path, char *buffer)
>   {
>       snprintf(buffer, PATH_MAX, "%s/%s", ctx->fs_root, path);
> @@ -198,14 +202,21 @@ struct V9fsFidState
>       int32_t fid;
>       V9fsString path;
>       union {
> -	int fd;
> -	DIR *dir;
> -	V9fsXattr xattr;
> +        int fd;
> +        DIR *dir;
> +        V9fsXattr xattr;
>       } fs;
> +    union {
> +        int fd;
> +        DIR *dir;
> +    } fs_reclaim;
> +    int flags;
> +    int open_flags;
>       uid_t uid;
>       int ref;
>       int clunked;
>       V9fsFidState *next;
> +    V9fsFidState *rclm_lst;
>   };
>
>   typedef struct V9fsState
> @@ -222,6 +233,7 @@ typedef struct V9fsState
>       size_t config_size;
>       enum p9_proto_version proto_version;
>       int32_t msize;
> +    int reclaim_in_prgs;
>   } V9fsState;
>
>   typedef struct V9fsStatState {
> @@ -354,6 +366,9 @@ typedef struct V9fsGetlock
>       V9fsString client_id;
>   } V9fsGetlock;
>
> +extern int open_fd_hw;
> +extern int total_open_fd;
> +
>   size_t pdu_packunpack(void *addr, struct iovec *sg, int sg_count,
>                         size_t offset, size_t size, int pack);
>
> @@ -364,4 +379,6 @@ static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count,
>   }
>
>   extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
> +extern void virtio_9p_set_fd_limit(void);
> +extern void v9fs_reclaim_fd(V9fsState *s);
>   #endif

[Qemu-devel] [PATCH 4/6] hw/9pfs: init fid list properly

[Aneesh Kumar K.V]

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 70629b2..4bec8bf 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -130,6 +130,7 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
     s->config_size = sizeof(struct virtio_9p_config) +
                         s->tag_len;
     s->vdev.get_config = virtio_9p_get_config;
+    s->fid_list = NULL;
 
     if (v9fs_init_worker_threads() < 0) {
         fprintf(stderr, "worker thread initialization failed\n");
-- 
1.7.4.1

[Qemu-devel] [PATCH 5/6] hw/9pfs: Use v9fs_do_close instead of close

[Aneesh Kumar K.V]

we should use the local abstraction instead of
directly calling close.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 21e07fb..d322814 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -1596,7 +1596,7 @@ static void v9fs_lcreate(void *opaque)
     if (err < 0) {
         fidp->fid_type = P9_FID_NONE;
         if (fidp->fs.fd > 0) {
-            close(fidp->fs.fd);
+            v9fs_co_close(pdu->s, fidp->fs.fd);
         }
         goto out;
     }
@@ -2164,7 +2164,7 @@ static void v9fs_create(void *opaque)
     if (err < 0) {
         fidp->fid_type = P9_FID_NONE;
         if (fidp->fs.fd) {
-            close(fidp->fs.fd);
+            v9fs_co_close(pdu->s, fidp->fs.fd);
         }
         goto out;
     }
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 5/6] hw/9pfs: Use v9fs_do_close instead of close

[Venkateswararao Jujjuri]

On 06/06/2011 10:16 AM, Aneesh Kumar K.V wrote:
> we should use the local abstraction instead of
> directly calling close.
>
Let us fold this also into our coroutine patches.

- JV

> Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
> ---
>   hw/9pfs/virtio-9p.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 21e07fb..d322814 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -1596,7 +1596,7 @@ static void v9fs_lcreate(void *opaque)
>       if (err<  0) {
>           fidp->fid_type = P9_FID_NONE;
>           if (fidp->fs.fd>  0) {
> -            close(fidp->fs.fd);
> +            v9fs_co_close(pdu->s, fidp->fs.fd);
>           }
>           goto out;
>       }
> @@ -2164,7 +2164,7 @@ static void v9fs_create(void *opaque)
>       if (err<  0) {
>           fidp->fid_type = P9_FID_NONE;
>           if (fidp->fs.fd) {
> -            close(fidp->fs.fd);
> +            v9fs_co_close(pdu->s, fidp->fs.fd);
>           }
>           goto out;
>       }

[Qemu-devel] [PATCH 6/6] hw/9pfs: Add directory reclaim support

[Aneesh Kumar K.V]

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/9pfs/codir.c     |    9 +++++++++
 hw/9pfs/virtio-9p.c |   26 ++++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 3347038..2c26f72 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -98,6 +98,12 @@ int v9fs_co_opendir(V9fsState *s, V9fsFidState *fidp)
                 err = 0;
             }
         });
+    if (!err) {
+        total_open_fd++;
+        if (total_open_fd > open_fd_hw) {
+            v9fs_reclaim_fd(s);
+        }
+    }
     return err;
 }
 
@@ -112,5 +118,8 @@ int v9fs_co_closedir(V9fsState *s, DIR *dir)
                 err = -errno;
             }
         });
+    if (!err) {
+        total_open_fd--;
+    }
     return err;
 }
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index d322814..55aca93 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -265,7 +265,17 @@ static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
                         return NULL;
                     }
                 }
-            }
+            } else if (f->fid_type == P9_FID_DIR) {
+                if (f->fs.dir == NULL) {
+                    do {
+                        err = v9fs_co_opendir(s, f);
+                    } while (err == -EINTR);
+                    if (err < 0) {
+                        f->ref--;
+                        return NULL;
+                    }
+                }
+             }
             /*
              * Mark the fid as referenced so that the LRU
              * reclaim won't close the file descriptor
@@ -346,7 +356,9 @@ static int release_fid(V9fsState *s, V9fsFidState *fidp)
             retval = v9fs_co_close(s, fidp->fs.fd);
         }
     } else if (fidp->fid_type == P9_FID_DIR) {
-        retval = v9fs_co_closedir(s, fidp->fs.dir);
+        if (fidp->fs.dir != NULL) {
+            retval = v9fs_co_closedir(s, fidp->fs.dir);
+        }
     } else if (fidp->fid_type == P9_FID_XATTR) {
         retval = v9fs_xattr_fid_clunk(s, fidp);
     }
@@ -431,6 +443,14 @@ void v9fs_reclaim_fd(V9fsState *s)
                 f->fs.fd = -1;
                 reclaim_count++;
             }
+        } else if (f->fid_type == P9_FID_DIR) {
+            if (f->fs.dir != NULL) {
+                f->rclm_lst = reclaim_list;
+                reclaim_list = f;
+                f->fs_reclaim.dir = f->fs.dir;
+                f->fs.dir = NULL;
+                reclaim_count++;
+            }
         }
         if (reclaim_count >= open_fd_rc) {
             break;
@@ -448,6 +468,8 @@ void v9fs_reclaim_fd(V9fsState *s)
         } else {
             if (f->fid_type == P9_FID_FILE) {
                 v9fs_co_close(s, f->fs_reclaim.fd);
+            } else if (f->fid_type == P9_FID_DIR) {
+                v9fs_co_closedir(s, f->fs_reclaim.dir);
             }
             f->rclm_lst = NULL;
         }
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 6/6] hw/9pfs: Add directory reclaim support

[Venkateswararao Jujjuri]

On 06/06/2011 10:16 AM, Aneesh Kumar K.V wrote:
> Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
Reviewed-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>

> ---
>   hw/9pfs/codir.c     |    9 +++++++++
>   hw/9pfs/virtio-9p.c |   26 ++++++++++++++++++++++++--
>   2 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index 3347038..2c26f72 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -98,6 +98,12 @@ int v9fs_co_opendir(V9fsState *s, V9fsFidState *fidp)
>                   err = 0;
>               }
>           });
> +    if (!err) {
> +        total_open_fd++;
> +        if (total_open_fd>  open_fd_hw) {
> +            v9fs_reclaim_fd(s);
> +        }
> +    }
>       return err;
>   }
>
> @@ -112,5 +118,8 @@ int v9fs_co_closedir(V9fsState *s, DIR *dir)
>                   err = -errno;
>               }
>           });
> +    if (!err) {
> +        total_open_fd--;
> +    }
>       return err;
>   }
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index d322814..55aca93 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -265,7 +265,17 @@ static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
>                           return NULL;
>                       }
>                   }
> -            }
> +            } else if (f->fid_type == P9_FID_DIR) {
> +                if (f->fs.dir == NULL) {
> +                    do {
> +                        err = v9fs_co_opendir(s, f);
> +                    } while (err == -EINTR);
> +                    if (err<  0) {
> +                        f->ref--;
> +                        return NULL;
> +                    }
> +                }
> +             }
>               /*
>                * Mark the fid as referenced so that the LRU
>                * reclaim won't close the file descriptor
> @@ -346,7 +356,9 @@ static int release_fid(V9fsState *s, V9fsFidState *fidp)
>               retval = v9fs_co_close(s, fidp->fs.fd);
>           }
>       } else if (fidp->fid_type == P9_FID_DIR) {
> -        retval = v9fs_co_closedir(s, fidp->fs.dir);
> +        if (fidp->fs.dir != NULL) {
> +            retval = v9fs_co_closedir(s, fidp->fs.dir);
> +        }
>       } else if (fidp->fid_type == P9_FID_XATTR) {
>           retval = v9fs_xattr_fid_clunk(s, fidp);
>       }
> @@ -431,6 +443,14 @@ void v9fs_reclaim_fd(V9fsState *s)
>                   f->fs.fd = -1;
>                   reclaim_count++;
>               }
> +        } else if (f->fid_type == P9_FID_DIR) {
> +            if (f->fs.dir != NULL) {
> +                f->rclm_lst = reclaim_list;
> +                reclaim_list = f;
> +                f->fs_reclaim.dir = f->fs.dir;
> +                f->fs.dir = NULL;
> +                reclaim_count++;
> +            }
>           }
>           if (reclaim_count>= open_fd_rc) {
>               break;
> @@ -448,6 +468,8 @@ void v9fs_reclaim_fd(V9fsState *s)
>           } else {
>               if (f->fid_type == P9_FID_FILE) {
>                   v9fs_co_close(s, f->fs_reclaim.fd);
> +            } else if (f->fid_type == P9_FID_DIR) {
> +                v9fs_co_closedir(s, f->fs_reclaim.dir);
>               }
>               f->rclm_lst = NULL;
>           }

Re: [Qemu-devel] [PATCH 1/6] hw/9pfs: Add reference counting for fid

[Venkateswararao Jujjuri]

On 06/06/2011 10:16 AM, Aneesh Kumar K.V wrote:
> Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>

Just one minor issue below; otherwise

Reviewed-by: Venkateswararao Jujjuri "<jvrao@linux.vnet.ibm.com>

> ---
>   hw/9pfs/virtio-9p.c |  205 +++++++++++++++++++++++++++++++++++----------------
>   hw/9pfs/virtio-9p.h |    7 ++
>   2 files changed, 147 insertions(+), 65 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index e2aa863..03d8664 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -232,12 +232,13 @@ static size_t v9fs_string_size(V9fsString *str)
>       return str->size;
>   }
>
> -static V9fsFidState *lookup_fid(V9fsState *s, int32_t fid)
> +static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
>   {
>       V9fsFidState *f;
>
>       for (f = s->fid_list; f; f = f->next) {
>           if (f->fid == fid) {
> +            f->ref++;
>               return f;
>           }
>       }
> @@ -249,16 +250,16 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
>   {
>       V9fsFidState *f;
>
> -    f = lookup_fid(s, fid);
> +    f = get_fid(s, fid);
>       if (f) {
> +        f->ref--;
>           return NULL;
>       }
>
>       f = qemu_mallocz(sizeof(V9fsFidState));
> -
>       f->fid = fid;
>       f->fid_type = P9_FID_NONE;
> -
> +    f->ref = 1;
>       f->next = s->fid_list;
>       s->fid_list = f;
>
> @@ -1014,19 +1015,22 @@ static void v9fs_attach(void *opaque)
>       fidp = alloc_fid(s, fid);
>       if (fidp == NULL) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
>       fidp->uid = n_uname;
>       v9fs_string_sprintf(&fidp->path, "%s", "/");
>       err = fid_to_qid(s, fidp,&qid);
>       if (err<  0) {
>           err = -EINVAL;
> +        put_fid(fidp);
>           free_fid(s, fid);
> -        goto out;
> +        goto out_nofid;
>       }
>       offset += pdu_marshal(pdu, offset, "Q",&qid);
>       err = offset;
> -out:
> +    put_fid(fidp);
> +
> +out_nofid:
>       complete_pdu(s, pdu, err);
>       v9fs_string_free(&uname);
>       v9fs_string_free(&aname);
> @@ -1045,10 +1049,10 @@ static void v9fs_stat(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "d",&fid);
>
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       err = v9fs_co_lstat(s,&fidp->path,&stbuf);
>       if (err<  0) {
> @@ -1062,6 +1066,8 @@ static void v9fs_stat(void *opaque)
>       err = offset;
>       v9fs_stat_free(&v9stat);
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>   }
>
> @@ -1079,10 +1085,10 @@ static void v9fs_getattr(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "dq",&fid,&request_mask);
>
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           retval = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       /*
>        * Currently we only support BASIC fields in stat, so there is no
> @@ -1096,6 +1102,8 @@ static void v9fs_getattr(void *opaque)
>       retval = offset;
>       retval += pdu_marshal(pdu, offset, "A",&v9stat_dotl);
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, retval);
>   }
>
> @@ -1123,10 +1131,10 @@ static void v9fs_setattr(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "dI",&fid,&v9iattr);
>
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
>       if (v9iattr.valid&  ATTR_MODE) {
>           err = v9fs_co_chmod(s,&fidp->path, v9iattr.mode);
> @@ -1188,6 +1196,8 @@ static void v9fs_setattr(void *opaque)
>       }
>       err = offset;
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>   }
>
> @@ -1214,7 +1224,7 @@ static void v9fs_walk(void *opaque)
>       int32_t fid, newfid;
>       V9fsString *wnames = NULL;
>       V9fsFidState *fidp;
> -    V9fsFidState *newfidp;
> +    V9fsFidState *newfidp = NULL;;
>       V9fsPDU *pdu = opaque;
>       V9fsState *s = pdu->s;
>
> @@ -1231,12 +1241,12 @@ static void v9fs_walk(void *opaque)
>
>       } else if (nwnames>  P9_MAXWELEM) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       if (fid == newfid) {
>           BUG_ON(fidp->fid_type != P9_FID_NONE);
> @@ -1269,7 +1279,9 @@ static void v9fs_walk(void *opaque)
>               v9fs_string_copy(&newfidp->path,&path);
>               err = v9fs_co_lstat(s,&newfidp->path,&stbuf);
>               if (err<  0) {
> +                put_fid(newfidp);
>                   free_fid(s, newfidp->fid);
> +                newfidp = NULL;
>                   v9fs_string_free(&path);
>                   goto out;
>               }
> @@ -1279,6 +1291,11 @@ static void v9fs_walk(void *opaque)
>       }
>       err = v9fs_walk_marshal(pdu, nwnames, qids);
>   out:
> +    put_fid(fidp);
> +    if (newfidp) {
> +        put_fid(newfidp);
> +    }
> +out_nofid:
>       complete_pdu(s, pdu, err);
>       if (nwnames&&  nwnames<= P9_MAXWELEM) {
>           for (name_idx = 0; name_idx<  nwnames; name_idx++) {
> @@ -1327,10 +1344,10 @@ static void v9fs_open(void *opaque)
>       } else {
>           pdu_unmarshal(pdu, offset, "db",&fid,&mode);
>       }
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       BUG_ON(fidp->fid_type != P9_FID_NONE);
>
> @@ -1366,6 +1383,8 @@ static void v9fs_open(void *opaque)
>           err = offset;
>       }
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>   }
>
> @@ -1388,10 +1407,10 @@ static void v9fs_lcreate(void *opaque)
>       pdu_unmarshal(pdu, offset, "dsddd",&dfid,&name,&flags,
>                     &mode,&gid);
>
> -    fidp = lookup_fid(pdu->s, dfid);
> +    fidp = get_fid(pdu->s, dfid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
>
> @@ -1418,6 +1437,8 @@ static void v9fs_lcreate(void *opaque)
>       offset += pdu_marshal(pdu, offset, "Qd",&qid, iounit);
>       err = offset;
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>       v9fs_string_free(&name);
>       v9fs_string_free(&fullname);
> @@ -1434,7 +1455,7 @@ static void v9fs_fsync(void *opaque)
>       V9fsState *s = pdu->s;
>
>       pdu_unmarshal(pdu, offset, "dd",&fid,&datasync);
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
>           goto out;
> @@ -1444,6 +1465,7 @@ static void v9fs_fsync(void *opaque)
>           err = offset;
>       }
>   out:
> +    put_fid(fidp);
It should be

put_fid(fidp);

out_nofid:

>       complete_pdu(s, pdu, err);
>   }
>
> @@ -1561,10 +1583,10 @@ static void v9fs_read(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "dqd",&fid,&off,&max_count);
>
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
>       if (fidp->fid_type == P9_FID_DIR) {
>
> @@ -1616,6 +1638,8 @@ static void v9fs_read(void *opaque)
>           err = -EINVAL;
>       }
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>   }
>
> @@ -1700,8 +1724,12 @@ static void v9fs_readdir(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "dqd",&fid,&initial_offset,&max_count);
>
> -    fidp = lookup_fid(s, fid);
> -    if (fidp == NULL || !fidp->fs.dir) {
> +    fidp = get_fid(s, fid);
> +    if (fidp == NULL) {
> +        retval = -EINVAL;
> +        goto out_nofid;
> +    }
> +    if (!fidp->fs.dir) {
>           retval = -EINVAL;
>           goto out;
>       }
> @@ -1719,6 +1747,8 @@ static void v9fs_readdir(void *opaque)
>       retval += pdu_marshal(pdu, offset, "d", count);
>       retval += count;
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, retval);
>   }
>
> @@ -1784,10 +1814,10 @@ static void v9fs_write(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "dqdv",&fid,&off,&count, sg,&cnt);
>
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
>       if (fidp->fid_type == P9_FID_FILE) {
>           if (fidp->fs.fd == -1) {
> @@ -1827,6 +1857,8 @@ static void v9fs_write(void *opaque)
>       offset += pdu_marshal(pdu, offset, "d", total);
>       err = offset;
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>   }
>
> @@ -1851,10 +1883,10 @@ static void v9fs_create(void *opaque)
>       pdu_unmarshal(pdu, offset, "dsdbs",&fid,&name,
>                     &perm,&mode,&extension);
>
> -    fidp = lookup_fid(pdu->s, fid);
> +    fidp = get_fid(pdu->s, fid);
>       if (fidp == NULL) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
>
>       v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
> @@ -1884,15 +1916,17 @@ static void v9fs_create(void *opaque)
>           }
>       } else if (perm&  P9_STAT_MODE_LINK) {
>           int32_t nfid = atoi(extension.data);
> -        V9fsFidState *nfidp = lookup_fid(pdu->s, nfid);
> +        V9fsFidState *nfidp = get_fid(pdu->s, nfid);
>           if (nfidp == NULL) {
>               err = -EINVAL;
>               goto out;
>           }
>           err = v9fs_co_link(pdu->s,&nfidp->path,&fullname);
>           if (err<  0) {
> +            put_fid(nfidp);
>               goto out;
>           }
> +        put_fid(nfidp);
>       } else if (perm&  P9_STAT_MODE_DEVICE) {
>           char ctype;
>           uint32_t major, minor;
> @@ -1956,6 +1990,8 @@ static void v9fs_create(void *opaque)
>       err = offset;
>
>   out:
> +    put_fid(fidp);
> +out_nofid:
>      complete_pdu(pdu->s, pdu, err);
>      v9fs_string_free(&name);
>      v9fs_string_free(&extension);
> @@ -1980,10 +2016,10 @@ static void v9fs_symlink(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "dssd",&dfid,&name,&symname,&gid);
>
> -    dfidp = lookup_fid(pdu->s, dfid);
> +    dfidp = get_fid(pdu->s, dfid);
>       if (dfidp == NULL) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
>
>       v9fs_string_sprintf(&fullname, "%s/%s", dfidp->path.data, name.data);
> @@ -1999,6 +2035,8 @@ static void v9fs_symlink(void *opaque)
>       offset += pdu_marshal(pdu, offset, "Q",&qid);
>       err = offset;
>   out:
> +    put_fid(dfidp);
> +out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>       v9fs_string_free(&name);
>       v9fs_string_free(&symname);
> @@ -2028,13 +2066,13 @@ static void v9fs_link(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "dds",&dfid,&oldfid,&name);
>
> -    dfidp = lookup_fid(s, dfid);
> +    dfidp = get_fid(s, dfid);
>       if (dfidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>
> -    oldfidp = lookup_fid(s, oldfid);
> +    oldfidp = get_fid(s, oldfid);
>       if (oldfidp == NULL) {
>           err = -ENOENT;
>           goto out;
> @@ -2048,6 +2086,8 @@ static void v9fs_link(void *opaque)
>       v9fs_string_free(&fullname);
>
>   out:
> +    put_fid(dfidp);
> +out_nofid:
>       v9fs_string_free(&name);
>       complete_pdu(s, pdu, err);
>   }
> @@ -2062,10 +2102,10 @@ static void v9fs_remove(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "d",&fid);
>
> -    fidp = lookup_fid(pdu->s, fid);
> +    fidp = get_fid(pdu->s, fid);
>       if (fidp == NULL) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
>       err = v9fs_co_remove(pdu->s,&fidp->path);
>       if (!err) {
> @@ -2073,8 +2113,9 @@ static void v9fs_remove(void *opaque)
>       }
>
>       /* For TREMOVE we need to clunk the fid even on failed remove */
> +    put_fid(fidp);
>       free_fid(pdu->s, fidp->fid);
> -out:
> +out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>   }
>
> @@ -2083,14 +2124,14 @@ static int v9fs_complete_rename(V9fsState *s, V9fsFidState *fidp,
>   {
>       char *end;
>       int err = 0;
> +    V9fsFidState *dirfidp = NULL;
>       char *old_name, *new_name;
>
>       if (newdirfid != -1) {
> -        V9fsFidState *dirfidp;
> -        dirfidp = lookup_fid(s, newdirfid);
> +        dirfidp = get_fid(s, newdirfid);
>           if (dirfidp == NULL) {
>               err = -ENOENT;
> -            goto out;
> +            goto out_nofid;
>           }
>           BUG_ON(dirfidp->fid_type != P9_FID_NONE);
>
> @@ -2143,6 +2184,10 @@ static int v9fs_complete_rename(V9fsState *s, V9fsFidState *fidp,
>           v9fs_string_copy(&fidp->path, name);
>       }
>   out:
> +    if (dirfidp) {
> +        put_fid(dirfidp);
> +    }
> +out_nofid:
>       return err;
>   }
>
> @@ -2159,10 +2204,10 @@ static void v9fs_rename(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "dds",&fid,&newdirfid,&name);
>
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       BUG_ON(fidp->fid_type != P9_FID_NONE);
>
> @@ -2171,6 +2216,8 @@ static void v9fs_rename(void *opaque)
>           err = offset;
>       }
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>       v9fs_string_free(&name);
>   }
> @@ -2189,10 +2236,10 @@ static void v9fs_wstat(void *opaque)
>
>       pdu_unmarshal(pdu, offset, "dwS",&fid,&unused,&v9stat);
>
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
>       /* do we need to sync the file? */
>       if (donttouch_stat(&v9stat)) {
> @@ -2258,6 +2305,8 @@ static void v9fs_wstat(void *opaque)
>       }
>       err = offset;
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       v9fs_stat_free(&v9stat);
>       complete_pdu(s, pdu, err);
>   }
> @@ -2318,10 +2367,10 @@ static void v9fs_statfs(void *opaque)
>       V9fsState *s = pdu->s;
>
>       pdu_unmarshal(pdu, offset, "d",&fid);
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           retval = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       retval = v9fs_co_statfs(s,&fidp->path,&stbuf);
>       if (retval<  0) {
> @@ -2330,6 +2379,8 @@ static void v9fs_statfs(void *opaque)
>       retval = offset;
>       retval += v9fs_fill_statfs(s, pdu,&stbuf);
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, retval);
>       return;
>   }
> @@ -2355,10 +2406,10 @@ static void v9fs_mknod(void *opaque)
>       pdu_unmarshal(pdu, offset, "dsdddd",&fid,&name,&mode,
>                     &major,&minor,&gid);
>
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
>       err = v9fs_co_mknod(s,&fullname, fidp->uid, gid,
> @@ -2374,6 +2425,8 @@ static void v9fs_mknod(void *opaque)
>       err = offset;
>       err += pdu_marshal(pdu, offset, "Q",&qid);
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>       v9fs_string_free(&fullname);
>       v9fs_string_free(&name);
> @@ -2407,12 +2460,12 @@ static void v9fs_lock(void *opaque)
>       /* We support only block flag now (that too ignored currently) */
>       if (flock->flags&  ~P9_LOCK_FLAGS_BLOCK) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       err = v9fs_co_fstat(s, fidp->fs.fd,&stbuf);
>       if (err<  0) {
> @@ -2420,6 +2473,8 @@ static void v9fs_lock(void *opaque)
>       }
>       status = P9_LOCK_SUCCESS;
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       err = offset;
>       err += pdu_marshal(pdu, offset, "b", status);
>       complete_pdu(s, pdu, err);
> @@ -2445,10 +2500,10 @@ static void v9fs_getlock(void *opaque)
>                     &glock->start,&glock->length,&glock->proc_id,
>                     &glock->client_id);
>
> -    fidp = lookup_fid(s, fid);
> +    fidp = get_fid(s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       err = v9fs_co_fstat(s, fidp->fs.fd,&stbuf);
>       if (err<  0) {
> @@ -2460,6 +2515,8 @@ static void v9fs_getlock(void *opaque)
>                             &glock->client_id);
>       err = offset;
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>       qemu_free(glock);
>   }
> @@ -2480,10 +2537,10 @@ static void v9fs_mkdir(void *opaque)
>       v9fs_string_init(&fullname);
>       pdu_unmarshal(pdu, offset, "dsdd",&fid,&name,&mode,&gid);
>
> -    fidp = lookup_fid(pdu->s, fid);
> +    fidp = get_fid(pdu->s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
>       err = v9fs_co_mkdir(pdu->s, fullname.data, mode, fidp->uid, gid);
> @@ -2498,6 +2555,8 @@ static void v9fs_mkdir(void *opaque)
>       offset += pdu_marshal(pdu, offset, "Q",&qid);
>       err = offset;
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>       v9fs_string_free(&fullname);
>       v9fs_string_free(&name);
> @@ -2511,15 +2570,15 @@ static void v9fs_xattrwalk(void *opaque)
>       size_t offset = 7;
>       int32_t fid, newfid;
>       V9fsFidState *file_fidp;
> -    V9fsFidState *xattr_fidp;
> +    V9fsFidState *xattr_fidp = NULL;
>       V9fsPDU *pdu = opaque;
>       V9fsState *s = pdu->s;
>
>       pdu_unmarshal(pdu, offset, "dds",&fid,&newfid,&name);
> -    file_fidp = lookup_fid(s, fid);
> +    file_fidp = get_fid(s, fid);
>       if (file_fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>       xattr_fidp = alloc_fid(s, newfid);
>       if (xattr_fidp == NULL) {
> @@ -2534,7 +2593,9 @@ static void v9fs_xattrwalk(void *opaque)
>           size = v9fs_co_llistxattr(s,&xattr_fidp->path, NULL, 0);
>           if (size<  0) {
>               err = size;
> +            put_fid(xattr_fidp);
>               free_fid(s, xattr_fidp->fid);
> +            xattr_fidp = NULL;
>               goto out;
>           }
>           /*
> @@ -2549,7 +2610,9 @@ static void v9fs_xattrwalk(void *opaque)
>                                        xattr_fidp->fs.xattr.value,
>                                        xattr_fidp->fs.xattr.len);
>               if (err<  0) {
> +                put_fid(xattr_fidp);
>                   free_fid(s, xattr_fidp->fid);
> +                xattr_fidp = NULL;
>                   goto out;
>               }
>           }
> @@ -2564,7 +2627,9 @@ static void v9fs_xattrwalk(void *opaque)
>                                    &name, NULL, 0);
>           if (size<  0) {
>               err = size;
> +            put_fid(xattr_fidp);
>               free_fid(s, xattr_fidp->fid);
> +            xattr_fidp = NULL;
>               goto out;
>           }
>           /*
> @@ -2579,7 +2644,9 @@ static void v9fs_xattrwalk(void *opaque)
>                                       &name, xattr_fidp->fs.xattr.value,
>                                       xattr_fidp->fs.xattr.len);
>               if (err<  0) {
> +                put_fid(xattr_fidp);
>                   free_fid(s, xattr_fidp->fid);
> +                xattr_fidp = NULL;
>                   goto out;
>               }
>           }
> @@ -2587,6 +2654,11 @@ static void v9fs_xattrwalk(void *opaque)
>           err = offset;
>       }
>   out:
> +    put_fid(file_fidp);
> +    if (xattr_fidp) {
> +        put_fid(xattr_fidp);
> +    }
> +out_nofid:
>       complete_pdu(s, pdu, err);
>       v9fs_string_free(&name);
>   }
> @@ -2607,10 +2679,10 @@ static void v9fs_xattrcreate(void *opaque)
>       pdu_unmarshal(pdu, offset, "dsqd",
>                     &fid,&name,&size,&flags);
>
> -    file_fidp = lookup_fid(s, fid);
> +    file_fidp = get_fid(s, fid);
>       if (file_fidp == NULL) {
>           err = -EINVAL;
> -        goto out;
> +        goto out_nofid;
>       }
>       /* Make the file fid point to xattr */
>       xattr_fidp = file_fidp;
> @@ -2626,7 +2698,8 @@ static void v9fs_xattrcreate(void *opaque)
>           xattr_fidp->fs.xattr.value = NULL;
>       }
>       err = offset;
> -out:
> +    put_fid(file_fidp);
> +out_nofid:
>       complete_pdu(s, pdu, err);
>       v9fs_string_free(&name);
>   }
> @@ -2641,10 +2714,10 @@ static void v9fs_readlink(void *opaque)
>       V9fsFidState *fidp;
>
>       pdu_unmarshal(pdu, offset, "d",&fid);
> -    fidp = lookup_fid(pdu->s, fid);
> +    fidp = get_fid(pdu->s, fid);
>       if (fidp == NULL) {
>           err = -ENOENT;
> -        goto out;
> +        goto out_nofid;
>       }
>
>       v9fs_string_init(&target);
> @@ -2656,6 +2729,8 @@ static void v9fs_readlink(void *opaque)
>       err = offset;
>       v9fs_string_free(&target);
>   out:
> +    put_fid(fidp);
> +out_nofid:
>       complete_pdu(pdu->s, pdu, err);
>   }
>
> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> index 1d8c1b1..ce93c20 100644
> --- a/hw/9pfs/virtio-9p.h
> +++ b/hw/9pfs/virtio-9p.h
> @@ -203,6 +203,7 @@ struct V9fsFidState
>   	V9fsXattr xattr;
>       } fs;
>       uid_t uid;
> +    int ref;
>       V9fsFidState *next;
>   };
>
> @@ -361,5 +362,11 @@ static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count,
>       return pdu_packunpack(dst, sg, sg_count, offset, size, 0);
>   }
>
> +static inline void put_fid(V9fsFidState *fidp)
> +{
> +    BUG_ON(!fidp->ref);
> +    fidp->ref--;
> +}
> +
>   extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
>   #endif

Re: [Qemu-devel] [PATCH 1/6] hw/9pfs: Add reference counting for fid

[Aneesh Kumar K.V]

On Thu, 09 Jun 2011 16:10:39 -0700, Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com> wrote:
> On 06/06/2011 10:16 AM, Aneesh Kumar K.V wrote:
> > Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
> 
> Just one minor issue below; otherwise
> 
> Reviewed-by: Venkateswararao Jujjuri "<jvrao@linux.vnet.ibm.com>
> 
> > ---
....

> >   hw/9pfs/virtio-9p.c |  205 +++++++++++++++++++++++++++++++++++----------------
> >
> >       pdu_unmarshal(pdu, offset, "dd",&fid,&datasync);
> > -    fidp = lookup_fid(s, fid);
> > +    fidp = get_fid(s, fid);
> >       if (fidp == NULL) {
> >           err = -ENOENT;
> >           goto out;
> > @@ -1444,6 +1465,7 @@ static void v9fs_fsync(void *opaque)
> >           err = offset;
> >       }
> >   out:
> > +    put_fid(fidp);
> It should be
> 
> put_fid(fidp);
> 
> out_nofid:
> 

ok


> >       complete_pdu(s, pdu, err);
> >   }


-aneesh

[Qemu-devel] [PATCH 0/6] hw/9pfs: Implement file descriptor reclaim in VirtFS server

[Aneesh Kumar K.V]

Hi,

The patch series implement file descriptor reclaim support in
VirtFS server. VirtFS qemu server track open file descriptor
on the client using an open fid. This result in server returning
EMFILE error even though on the client side the process have not
reached maximum open file limit.

To fix this we reclaim file descriptor on the server when we
reach high watermark.

The following changes since commit 56a7a874e962e28522857fbf72eaefb1a07e2001:

  Merge remote-tracking branch 'stefanha/trivial-patches' into staging (2011-08-25 07:50:07 -0500)

are available in the git repository at:

  git://repo.or.cz/qemu/v9fs.git for-upstream-3

Aneesh Kumar K.V (6):
      hw/9pfs: Add reference counting for fid
      hw/9pfs: Add file descriptor reclaim support
      hw/9pfs: init fid list properly
      hw/9pfs: Use v9fs_do_close instead of close
      hw/9pfs: Add directory reclaim support
      hw/9pfs: mark directories also as un-reclaimable on unlink

 hw/9pfs/codir.c            |   13 +-
 hw/9pfs/cofile.c           |   19 ++-
 hw/9pfs/virtio-9p-coth.h   |    4 +-
 hw/9pfs/virtio-9p-device.c |    2 +
 hw/9pfs/virtio-9p.c        |  486 +++++++++++++++++++++++++++++++++++---------
 hw/9pfs/virtio-9p.h        |   24 ++-
 6 files changed, 445 insertions(+), 103 deletions(-)

[Qemu-devel] [PATCH 1/6] hw/9pfs: Add reference counting for fid

[Aneesh Kumar K.V]

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p.c |  273 ++++++++++++++++++++++++++++++++++-----------------
 hw/9pfs/virtio-9p.h |    2 +
 2 files changed, 184 insertions(+), 91 deletions(-)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index ad70768..555f456 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -232,16 +232,17 @@ static size_t v9fs_string_size(V9fsString *str)
     return str->size;
 }
 
-static V9fsFidState *lookup_fid(V9fsState *s, int32_t fid)
+static V9fsFidState *get_fid(V9fsState *s, int32_t fid)
 {
     V9fsFidState *f;
 
     for (f = s->fid_list; f; f = f->next) {
+        BUG_ON(f->clunked);
         if (f->fid == fid) {
+            f->ref++;
             return f;
         }
     }
-
     return NULL;
 }
 
@@ -249,16 +250,17 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
 {
     V9fsFidState *f;
 
-    f = lookup_fid(s, fid);
-    if (f) {
-        return NULL;
+    for (f = s->fid_list; f; f = f->next) {
+        /* If fid is already there return NULL */
+        BUG_ON(f->clunked);
+        if (f->fid == fid) {
+            return NULL;
+        }
     }
-
     f = g_malloc0(sizeof(V9fsFidState));
-
     f->fid = fid;
     f->fid_type = P9_FID_NONE;
-
+    f->ref = 1;
     f->next = s->fid_list;
     s->fid_list = f;
 
@@ -299,9 +301,33 @@ free_value:
     return retval;
 }
 
-static int free_fid(V9fsState *s, int32_t fid)
+static int free_fid(V9fsState *s, V9fsFidState *fidp)
 {
     int retval = 0;
+
+    if (fidp->fid_type == P9_FID_FILE) {
+        retval = v9fs_co_close(s, fidp);
+    } else if (fidp->fid_type == P9_FID_DIR) {
+        retval = v9fs_co_closedir(s, fidp);
+    } else if (fidp->fid_type == P9_FID_XATTR) {
+        retval = v9fs_xattr_fid_clunk(s, fidp);
+    }
+    v9fs_string_free(&fidp->path);
+    g_free(fidp);
+    return retval;
+}
+
+static void put_fid(V9fsState *s, V9fsFidState *fidp)
+{
+    BUG_ON(!fidp->ref);
+    fidp->ref--;
+    if (!fidp->ref && fidp->clunked) {
+        free_fid(s, fidp);
+    }
+}
+
+static int clunk_fid(V9fsState *s, int32_t fid)
+{
     V9fsFidState **fidpp, *fidp;
 
     for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) {
@@ -313,20 +339,10 @@ static int free_fid(V9fsState *s, int32_t fid)
     if (*fidpp == NULL) {
         return -ENOENT;
     }
-
     fidp = *fidpp;
     *fidpp = fidp->next;
-
-    if (fidp->fid_type == P9_FID_FILE) {
-        retval = v9fs_co_close(s, fidp);
-    } else if (fidp->fid_type == P9_FID_DIR) {
-        retval = v9fs_co_closedir(s, fidp);
-    } else if (fidp->fid_type == P9_FID_XATTR) {
-        retval = v9fs_xattr_fid_clunk(s, fidp);
-    }
-    v9fs_string_free(&fidp->path);
-    g_free(fidp);
-    return retval;
+    fidp->clunked = 1;
+    return 0;
 }
 
 #define P9_QID_TYPE_DIR         0x80
@@ -1014,19 +1030,21 @@ static void v9fs_attach(void *opaque)
     fidp = alloc_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     fidp->uid = n_uname;
     v9fs_string_sprintf(&fidp->path, "%s", "/");
     err = fid_to_qid(s, fidp, &qid);
     if (err < 0) {
         err = -EINVAL;
-        free_fid(s, fid);
+        clunk_fid(s, fid);
         goto out;
     }
     offset += pdu_marshal(pdu, offset, "Q", &qid);
     err = offset;
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&uname);
     v9fs_string_free(&aname);
@@ -1044,10 +1062,11 @@ static void v9fs_stat(void *opaque)
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "d", &fid);
-    fidp = lookup_fid(s, fid);
+
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     err = v9fs_co_lstat(s, &fidp->path, &stbuf);
     if (err < 0) {
@@ -1061,6 +1080,8 @@ static void v9fs_stat(void *opaque)
     err = offset;
     v9fs_stat_free(&v9stat);
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1078,10 +1099,10 @@ static void v9fs_getattr(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         retval = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     /*
      * Currently we only support BASIC fields in stat, so there is no
@@ -1095,6 +1116,8 @@ static void v9fs_getattr(void *opaque)
     retval = offset;
     retval += pdu_marshal(pdu, offset, "A", &v9stat_dotl);
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, retval);
 }
 
@@ -1122,10 +1145,10 @@ static void v9fs_setattr(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dI", &fid, &v9iattr);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     if (v9iattr.valid & ATTR_MODE) {
         err = v9fs_co_chmod(s, &fidp->path, v9iattr.mode);
@@ -1187,6 +1210,8 @@ static void v9fs_setattr(void *opaque)
     }
     err = offset;
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1213,7 +1238,7 @@ static void v9fs_walk(void *opaque)
     int32_t fid, newfid;
     V9fsString *wnames = NULL;
     V9fsFidState *fidp;
-    V9fsFidState *newfidp;
+    V9fsFidState *newfidp = NULL;;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
 
@@ -1229,12 +1254,12 @@ static void v9fs_walk(void *opaque)
 
     } else if (nwnames > P9_MAXWELEM) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     if (fid == newfid) {
         BUG_ON(fidp->fid_type != P9_FID_NONE);
@@ -1267,7 +1292,7 @@ static void v9fs_walk(void *opaque)
             v9fs_string_copy(&newfidp->path, &path);
             err = v9fs_co_lstat(s, &newfidp->path, &stbuf);
             if (err < 0) {
-                free_fid(s, newfidp->fid);
+                clunk_fid(s, newfidp->fid);
                 v9fs_string_free(&path);
                 goto out;
             }
@@ -1277,6 +1302,11 @@ static void v9fs_walk(void *opaque)
     }
     err = v9fs_walk_marshal(pdu, nwnames, qids);
 out:
+    put_fid(s, fidp);
+    if (newfidp) {
+        put_fid(s, newfidp);
+    }
+out_nofid:
     complete_pdu(s, pdu, err);
     if (nwnames && nwnames <= P9_MAXWELEM) {
         for (name_idx = 0; name_idx < nwnames; name_idx++) {
@@ -1325,10 +1355,10 @@ static void v9fs_open(void *opaque)
     } else {
         pdu_unmarshal(pdu, offset, "db", &fid, &mode);
     }
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     BUG_ON(fidp->fid_type != P9_FID_NONE);
 
@@ -1364,6 +1394,8 @@ static void v9fs_open(void *opaque)
         err = offset;
     }
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1385,10 +1417,10 @@ static void v9fs_lcreate(void *opaque)
     pdu_unmarshal(pdu, offset, "dsddd", &dfid, &name, &flags,
                   &mode, &gid);
 
-    fidp = lookup_fid(pdu->s, dfid);
+    fidp = get_fid(pdu->s, dfid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
 
@@ -1415,6 +1447,8 @@ static void v9fs_lcreate(void *opaque)
     offset += pdu_marshal(pdu, offset, "Qd", &qid, iounit);
     err = offset;
 out:
+    put_fid(pdu->s, fidp);
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
     v9fs_string_free(&name);
     v9fs_string_free(&fullname);
@@ -1431,16 +1465,17 @@ static void v9fs_fsync(void *opaque)
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "dd", &fid, &datasync);
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     err = v9fs_co_fsync(s, fidp, datasync);
     if (!err) {
         err = offset;
     }
-out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1449,16 +1484,25 @@ static void v9fs_clunk(void *opaque)
     int err;
     int32_t fid;
     size_t offset = 7;
+    V9fsFidState *fidp;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "d", &fid);
-    err = free_fid(s, fid);
+
+    fidp = get_fid(s, fid);
+    if (fidp == NULL) {
+        err = -ENOENT;
+        goto out_nofid;
+    }
+    err = clunk_fid(s, fidp->fid);
     if (err < 0) {
         goto out;
     }
     err = offset;
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1557,10 +1601,11 @@ static void v9fs_read(void *opaque)
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "dqd", &fid, &off, &max_count);
-    fidp = lookup_fid(s, fid);
+
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     if (fidp->fid_type == P9_FID_DIR) {
 
@@ -1612,6 +1657,8 @@ static void v9fs_read(void *opaque)
         err = -EINVAL;
     }
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1696,8 +1743,12 @@ static void v9fs_readdir(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dqd", &fid, &initial_offset, &max_count);
 
-    fidp = lookup_fid(s, fid);
-    if (fidp == NULL || !fidp->fs.dir) {
+    fidp = get_fid(s, fid);
+    if (fidp == NULL) {
+        retval = -EINVAL;
+        goto out_nofid;
+    }
+    if (!fidp->fs.dir) {
         retval = -EINVAL;
         goto out;
     }
@@ -1715,6 +1766,8 @@ static void v9fs_readdir(void *opaque)
     retval += pdu_marshal(pdu, offset, "d", count);
     retval += count;
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, retval);
 }
 
@@ -1779,10 +1832,11 @@ static void v9fs_write(void *opaque)
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "dqdv", &fid, &off, &count, sg, &cnt);
-    fidp = lookup_fid(s, fid);
+
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     if (fidp->fid_type == P9_FID_FILE) {
         if (fidp->fs.fd == -1) {
@@ -1822,6 +1876,8 @@ static void v9fs_write(void *opaque)
     offset += pdu_marshal(pdu, offset, "d", total);
     err = offset;
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
 }
 
@@ -1846,10 +1902,10 @@ static void v9fs_create(void *opaque)
     pdu_unmarshal(pdu, offset, "dsdbs", &fid, &name,
                   &perm, &mode, &extension);
 
-    fidp = lookup_fid(pdu->s, fid);
+    fidp = get_fid(pdu->s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
 
     v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
@@ -1879,15 +1935,17 @@ static void v9fs_create(void *opaque)
         }
     } else if (perm & P9_STAT_MODE_LINK) {
         int32_t nfid = atoi(extension.data);
-        V9fsFidState *nfidp = lookup_fid(pdu->s, nfid);
+        V9fsFidState *nfidp = get_fid(pdu->s, nfid);
         if (nfidp == NULL) {
             err = -EINVAL;
             goto out;
         }
         err = v9fs_co_link(pdu->s, &nfidp->path, &fullname);
         if (err < 0) {
+            put_fid(pdu->s, nfidp);
             goto out;
         }
+        put_fid(pdu->s, nfidp);
     } else if (perm & P9_STAT_MODE_DEVICE) {
         char ctype;
         uint32_t major, minor;
@@ -1950,6 +2008,8 @@ static void v9fs_create(void *opaque)
     offset += pdu_marshal(pdu, offset, "Qd", &qid, iounit);
     err = offset;
 out:
+    put_fid(pdu->s, fidp);
+out_nofid:
    complete_pdu(pdu->s, pdu, err);
    v9fs_string_free(&name);
    v9fs_string_free(&extension);
@@ -1973,10 +2033,10 @@ static void v9fs_symlink(void *opaque)
     v9fs_string_init(&fullname);
     pdu_unmarshal(pdu, offset, "dssd", &dfid, &name, &symname, &gid);
 
-    dfidp = lookup_fid(pdu->s, dfid);
+    dfidp = get_fid(pdu->s, dfid);
     if (dfidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
 
     v9fs_string_sprintf(&fullname, "%s/%s", dfidp->path.data, name.data);
@@ -1992,6 +2052,8 @@ static void v9fs_symlink(void *opaque)
     offset += pdu_marshal(pdu, offset, "Q", &qid);
     err = offset;
 out:
+    put_fid(pdu->s, dfidp);
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
     v9fs_string_free(&name);
     v9fs_string_free(&symname);
@@ -2021,13 +2083,13 @@ static void v9fs_link(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dds", &dfid, &oldfid, &name);
 
-    dfidp = lookup_fid(s, dfid);
+    dfidp = get_fid(s, dfid);
     if (dfidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
 
-    oldfidp = lookup_fid(s, oldfid);
+    oldfidp = get_fid(s, oldfid);
     if (oldfidp == NULL) {
         err = -ENOENT;
         goto out;
@@ -2041,6 +2103,8 @@ static void v9fs_link(void *opaque)
     v9fs_string_free(&fullname);
 
 out:
+    put_fid(s, dfidp);
+out_nofid:
     v9fs_string_free(&name);
     complete_pdu(s, pdu, err);
 }
@@ -2055,10 +2119,10 @@ static void v9fs_remove(void *opaque)
 
     pdu_unmarshal(pdu, offset, "d", &fid);
 
-    fidp = lookup_fid(pdu->s, fid);
+    fidp = get_fid(pdu->s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     err = v9fs_co_remove(pdu->s, &fidp->path);
     if (!err) {
@@ -2066,8 +2130,9 @@ static void v9fs_remove(void *opaque)
     }
 
     /* For TREMOVE we need to clunk the fid even on failed remove */
-    free_fid(pdu->s, fidp->fid);
-out:
+    clunk_fid(pdu->s, fidp->fid);
+    put_fid(pdu->s, fidp);
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
 }
 
@@ -2076,14 +2141,14 @@ static int v9fs_complete_rename(V9fsState *s, V9fsFidState *fidp,
 {
     char *end;
     int err = 0;
+    V9fsFidState *dirfidp = NULL;
     char *old_name, *new_name;
 
     if (newdirfid != -1) {
-        V9fsFidState *dirfidp;
-        dirfidp = lookup_fid(s, newdirfid);
+        dirfidp = get_fid(s, newdirfid);
         if (dirfidp == NULL) {
             err = -ENOENT;
-            goto out;
+            goto out_nofid;
         }
         BUG_ON(dirfidp->fid_type != P9_FID_NONE);
 
@@ -2136,6 +2201,10 @@ static int v9fs_complete_rename(V9fsState *s, V9fsFidState *fidp,
         v9fs_string_copy(&fidp->path, name);
     }
 out:
+    if (dirfidp) {
+        put_fid(s, dirfidp);
+    }
+out_nofid:
     return err;
 }
 
@@ -2152,10 +2221,10 @@ static void v9fs_rename(void *opaque)
 
     pdu_unmarshal(pdu, offset, "dds", &fid, &newdirfid, &name);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     BUG_ON(fidp->fid_type != P9_FID_NONE);
 
@@ -2163,7 +2232,8 @@ static void v9fs_rename(void *opaque)
     if (!err) {
         err = offset;
     }
-out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&name);
 }
@@ -2181,10 +2251,11 @@ static void v9fs_wstat(void *opaque)
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "dwS", &fid, &unused, &v9stat);
-    fidp = lookup_fid(s, fid);
+
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     /* do we need to sync the file? */
     if (donttouch_stat(&v9stat)) {
@@ -2250,6 +2321,8 @@ static void v9fs_wstat(void *opaque)
     }
     err = offset;
 out:
+    put_fid(s, fidp);
+out_nofid:
     v9fs_stat_free(&v9stat);
     complete_pdu(s, pdu, err);
 }
@@ -2310,10 +2383,10 @@ static void v9fs_statfs(void *opaque)
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "d", &fid);
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         retval = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     retval = v9fs_co_statfs(s, &fidp->path, &stbuf);
     if (retval < 0) {
@@ -2322,6 +2395,8 @@ static void v9fs_statfs(void *opaque)
     retval = offset;
     retval += v9fs_fill_statfs(s, pdu, &stbuf);
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, retval);
     return;
 }
@@ -2347,10 +2422,10 @@ static void v9fs_mknod(void *opaque)
     pdu_unmarshal(pdu, offset, "dsdddd", &fid, &name, &mode,
                   &major, &minor, &gid);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
     err = v9fs_co_mknod(s, &fullname, fidp->uid, gid,
@@ -2366,6 +2441,8 @@ static void v9fs_mknod(void *opaque)
     err = offset;
     err += pdu_marshal(pdu, offset, "Q", &qid);
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&fullname);
     v9fs_string_free(&name);
@@ -2399,12 +2476,12 @@ static void v9fs_lock(void *opaque)
     /* We support only block flag now (that too ignored currently) */
     if (flock->flags & ~P9_LOCK_FLAGS_BLOCK) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     err = v9fs_co_fstat(s, fidp->fs.fd, &stbuf);
     if (err < 0) {
@@ -2412,6 +2489,8 @@ static void v9fs_lock(void *opaque)
     }
     status = P9_LOCK_SUCCESS;
 out:
+    put_fid(s, fidp);
+out_nofid:
     err = offset;
     err += pdu_marshal(pdu, offset, "b", status);
     complete_pdu(s, pdu, err);
@@ -2437,10 +2516,10 @@ static void v9fs_getlock(void *opaque)
                   &glock->start, &glock->length, &glock->proc_id,
                   &glock->client_id);
 
-    fidp = lookup_fid(s, fid);
+    fidp = get_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     err = v9fs_co_fstat(s, fidp->fs.fd, &stbuf);
     if (err < 0) {
@@ -2452,6 +2531,8 @@ static void v9fs_getlock(void *opaque)
                           &glock->client_id);
     err = offset;
 out:
+    put_fid(s, fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
     g_free(glock);
 }
@@ -2472,10 +2553,10 @@ static void v9fs_mkdir(void *opaque)
     v9fs_string_init(&fullname);
     pdu_unmarshal(pdu, offset, "dsdd", &fid, &name, &mode, &gid);
 
-    fidp = lookup_fid(pdu->s, fid);
+    fidp = get_fid(pdu->s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     v9fs_string_sprintf(&fullname, "%s/%s", fidp->path.data, name.data);
     err = v9fs_co_mkdir(pdu->s, fullname.data, mode, fidp->uid, gid);
@@ -2490,6 +2571,8 @@ static void v9fs_mkdir(void *opaque)
     offset += pdu_marshal(pdu, offset, "Q", &qid);
     err = offset;
 out:
+    put_fid(pdu->s, fidp);
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
     v9fs_string_free(&fullname);
     v9fs_string_free(&name);
@@ -2503,15 +2586,15 @@ static void v9fs_xattrwalk(void *opaque)
     size_t offset = 7;
     int32_t fid, newfid;
     V9fsFidState *file_fidp;
-    V9fsFidState *xattr_fidp;
+    V9fsFidState *xattr_fidp = NULL;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
 
     pdu_unmarshal(pdu, offset, "dds", &fid, &newfid, &name);
-    file_fidp = lookup_fid(s, fid);
+    file_fidp = get_fid(s, fid);
     if (file_fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
     xattr_fidp = alloc_fid(s, newfid);
     if (xattr_fidp == NULL) {
@@ -2526,7 +2609,7 @@ static void v9fs_xattrwalk(void *opaque)
         size = v9fs_co_llistxattr(s, &xattr_fidp->path, NULL, 0);
         if (size < 0) {
             err = size;
-            free_fid(s, xattr_fidp->fid);
+            clunk_fid(s, xattr_fidp->fid);
             goto out;
         }
         /*
@@ -2541,7 +2624,7 @@ static void v9fs_xattrwalk(void *opaque)
                                      xattr_fidp->fs.xattr.value,
                                      xattr_fidp->fs.xattr.len);
             if (err < 0) {
-                free_fid(s, xattr_fidp->fid);
+                clunk_fid(s, xattr_fidp->fid);
                 goto out;
             }
         }
@@ -2556,7 +2639,7 @@ static void v9fs_xattrwalk(void *opaque)
                                  &name, NULL, 0);
         if (size < 0) {
             err = size;
-            free_fid(s, xattr_fidp->fid);
+            clunk_fid(s, xattr_fidp->fid);
             goto out;
         }
         /*
@@ -2571,7 +2654,7 @@ static void v9fs_xattrwalk(void *opaque)
                                     &name, xattr_fidp->fs.xattr.value,
                                     xattr_fidp->fs.xattr.len);
             if (err < 0) {
-                free_fid(s, xattr_fidp->fid);
+                clunk_fid(s, xattr_fidp->fid);
                 goto out;
             }
         }
@@ -2579,6 +2662,11 @@ static void v9fs_xattrwalk(void *opaque)
         err = offset;
     }
 out:
+    put_fid(s, file_fidp);
+    if (xattr_fidp) {
+        put_fid(s, xattr_fidp);
+    }
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&name);
 }
@@ -2599,10 +2687,10 @@ static void v9fs_xattrcreate(void *opaque)
     pdu_unmarshal(pdu, offset, "dsqd",
                   &fid, &name, &size, &flags);
 
-    file_fidp = lookup_fid(s, fid);
+    file_fidp = get_fid(s, fid);
     if (file_fidp == NULL) {
         err = -EINVAL;
-        goto out;
+        goto out_nofid;
     }
     /* Make the file fid point to xattr */
     xattr_fidp = file_fidp;
@@ -2618,7 +2706,8 @@ static void v9fs_xattrcreate(void *opaque)
         xattr_fidp->fs.xattr.value = NULL;
     }
     err = offset;
-out:
+    put_fid(s, file_fidp);
+out_nofid:
     complete_pdu(s, pdu, err);
     v9fs_string_free(&name);
 }
@@ -2633,10 +2722,10 @@ static void v9fs_readlink(void *opaque)
     V9fsFidState *fidp;
 
     pdu_unmarshal(pdu, offset, "d", &fid);
-    fidp = lookup_fid(pdu->s, fid);
+    fidp = get_fid(pdu->s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
-        goto out;
+        goto out_nofid;
     }
 
     v9fs_string_init(&target);
@@ -2648,6 +2737,8 @@ static void v9fs_readlink(void *opaque)
     err = offset;
     v9fs_string_free(&target);
 out:
+    put_fid(pdu->s, fidp);
+out_nofid:
     complete_pdu(pdu->s, pdu, err);
 }
 
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 1d8c1b1..e16e5f4 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -203,6 +203,8 @@ struct V9fsFidState
 	V9fsXattr xattr;
     } fs;
     uid_t uid;
+    int ref;
+    int clunked;
     V9fsFidState *next;
 };
 
-- 
1.7.4.1