From: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
To: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 2/6] hw/9pfs: Don't free fid in clunk
Date: Thu, 09 Jun 2011 17:27:00 -0700 [thread overview]
Message-ID: <4DF164D4.5070902@linux.vnet.ibm.com> (raw)
In-Reply-To: <1307380618-1963-2-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
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
next prev parent reply other threads:[~2011-06-10 0:27 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-06-06 17:16 [Qemu-devel] [PATCH 1/6] hw/9pfs: Add reference counting for fid Aneesh Kumar K.V
2011-06-06 17:16 ` [Qemu-devel] [PATCH 2/6] hw/9pfs: Don't free fid in clunk Aneesh Kumar K.V
2011-06-10 0:27 ` Venkateswararao Jujjuri [this message]
2011-06-10 6:19 ` Aneesh Kumar K.V
2011-06-06 17:16 ` [Qemu-devel] [PATCH 3/6] hw/9pfs: Add file descriptor reclaim support Aneesh Kumar K.V
2011-06-10 1:27 ` Venkateswararao Jujjuri
2011-06-06 17:16 ` [Qemu-devel] [PATCH 4/6] hw/9pfs: init fid list properly Aneesh Kumar K.V
2011-06-06 17:16 ` [Qemu-devel] [PATCH 5/6] hw/9pfs: Use v9fs_do_close instead of close Aneesh Kumar K.V
2011-06-10 1:32 ` Venkateswararao Jujjuri
2011-06-06 17:16 ` [Qemu-devel] [PATCH 6/6] hw/9pfs: Add directory reclaim support Aneesh Kumar K.V
2011-06-10 1:36 ` Venkateswararao Jujjuri
2011-06-09 23:10 ` [Qemu-devel] [PATCH 1/6] hw/9pfs: Add reference counting for fid Venkateswararao Jujjuri
2011-06-10 6:27 ` Aneesh Kumar K.V
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DF164D4.5070902@linux.vnet.ibm.com \
--to=jvrao@linux.vnet.ibm.com \
--cc=aliguori@us.ibm.com \
--cc=aneesh.kumar@linux.vnet.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.