From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> To: Stefan Hajnoczi <stefanha@redhat.com> Cc: virtio-fs@redhat.com, Liu Bo <bo.liu@linux.alibaba.com>, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH 3/4] virtiofsd: fix lo_destroy() resource leaks Date: Mon, 5 Aug 2019 16:17:08 +0100 [thread overview] Message-ID: <20190805151708.GN13734@work-vm> (raw) In-Reply-To: <20190801165409.20121-4-stefanha@redhat.com> * Stefan Hajnoczi (stefanha@redhat.com) wrote: > Now that lo_destroy() is serialized we can call unref_inode() so that > all inode resources are freed. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > contrib/virtiofsd/passthrough_ll.c | 43 ++++++++++++++---------------- > 1 file changed, 20 insertions(+), 23 deletions(-) > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index a81c01d0d1..02a5e97326 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -1340,28 +1340,6 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) > } > } > > -static int unref_all_inodes_cb(gpointer key, gpointer value, > - gpointer user_data) > -{ > - struct lo_inode *inode = value; > - struct lo_data *lo = user_data; > - > - inode->nlookup = 0; > - lo_map_remove(&lo->ino_map, inode->fuse_ino); > - close(inode->fd); > - lo_inode_put(lo, &inode); /* Drop our refcount from lo_do_lookup() */ > - > - return TRUE; > -} > - > -static void unref_all_inodes(struct lo_data *lo) > -{ > - pthread_mutex_lock(&lo->mutex); > - g_hash_table_foreach_remove(lo->inodes, unref_all_inodes_cb, lo); > - pthread_mutex_unlock(&lo->mutex); > - > -} > - > static void lo_forget_one(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) > { > struct lo_data *lo = lo_data(req); > @@ -2462,6 +2440,18 @@ static void lo_removemapping(fuse_req_t req, struct fuse_session *se, > fuse_reply_err(req, ret); > } > > +static int destroy_inode_cb(gpointer key, gpointer value, gpointer user_data) > +{ > + struct lo_inode *inode = value; > + struct lo_data *lo = user_data; > + > + /* inode->nlookup is normally protected by lo->mutex but see the > + * comment in lo_destroy(). > + */ > + unref_inode(lo, inode, inode->nlookup); > + return TRUE; > +} > + > static void lo_destroy(void *userdata, struct fuse_session *se) > { > struct lo_data *lo = (struct lo_data*) userdata; > @@ -2475,7 +2465,14 @@ static void lo_destroy(void *userdata, struct fuse_session *se) > fuse_err("%s: unmap during destroy failed\n", __func__); > } > } > - unref_all_inodes(lo); > + > + /* Normally lo->mutex must be taken when traversing lo->inodes but > + * lo_destroy() is a serialized request so no races are possible here. > + * > + * In addition, we cannot acquire lo->mutex since destroy_inode_cb() takes it > + * too and this would result in a recursive lock. > + */ > + g_hash_table_foreach_remove(lo->inodes, destroy_inode_cb, lo); > } > > static struct fuse_lowlevel_ops lo_oper = { > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
WARNING: multiple messages have this Message-ID (diff)
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> To: Stefan Hajnoczi <stefanha@redhat.com> Cc: virtio-fs@redhat.com, qemu-devel@nongnu.org Subject: Re: [Virtio-fs] [PATCH 3/4] virtiofsd: fix lo_destroy() resource leaks Date: Mon, 5 Aug 2019 16:17:08 +0100 [thread overview] Message-ID: <20190805151708.GN13734@work-vm> (raw) In-Reply-To: <20190801165409.20121-4-stefanha@redhat.com> * Stefan Hajnoczi (stefanha@redhat.com) wrote: > Now that lo_destroy() is serialized we can call unref_inode() so that > all inode resources are freed. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > contrib/virtiofsd/passthrough_ll.c | 43 ++++++++++++++---------------- > 1 file changed, 20 insertions(+), 23 deletions(-) > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index a81c01d0d1..02a5e97326 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -1340,28 +1340,6 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) > } > } > > -static int unref_all_inodes_cb(gpointer key, gpointer value, > - gpointer user_data) > -{ > - struct lo_inode *inode = value; > - struct lo_data *lo = user_data; > - > - inode->nlookup = 0; > - lo_map_remove(&lo->ino_map, inode->fuse_ino); > - close(inode->fd); > - lo_inode_put(lo, &inode); /* Drop our refcount from lo_do_lookup() */ > - > - return TRUE; > -} > - > -static void unref_all_inodes(struct lo_data *lo) > -{ > - pthread_mutex_lock(&lo->mutex); > - g_hash_table_foreach_remove(lo->inodes, unref_all_inodes_cb, lo); > - pthread_mutex_unlock(&lo->mutex); > - > -} > - > static void lo_forget_one(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) > { > struct lo_data *lo = lo_data(req); > @@ -2462,6 +2440,18 @@ static void lo_removemapping(fuse_req_t req, struct fuse_session *se, > fuse_reply_err(req, ret); > } > > +static int destroy_inode_cb(gpointer key, gpointer value, gpointer user_data) > +{ > + struct lo_inode *inode = value; > + struct lo_data *lo = user_data; > + > + /* inode->nlookup is normally protected by lo->mutex but see the > + * comment in lo_destroy(). > + */ > + unref_inode(lo, inode, inode->nlookup); > + return TRUE; > +} > + > static void lo_destroy(void *userdata, struct fuse_session *se) > { > struct lo_data *lo = (struct lo_data*) userdata; > @@ -2475,7 +2465,14 @@ static void lo_destroy(void *userdata, struct fuse_session *se) > fuse_err("%s: unmap during destroy failed\n", __func__); > } > } > - unref_all_inodes(lo); > + > + /* Normally lo->mutex must be taken when traversing lo->inodes but > + * lo_destroy() is a serialized request so no races are possible here. > + * > + * In addition, we cannot acquire lo->mutex since destroy_inode_cb() takes it > + * too and this would result in a recursive lock. > + */ > + g_hash_table_foreach_remove(lo->inodes, destroy_inode_cb, lo); > } > > static struct fuse_lowlevel_ops lo_oper = { > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2019-08-05 15:18 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-01 16:54 [Qemu-devel] [PATCH 0/4] virtiofsd: multithreading preparation part 3 Stefan Hajnoczi 2019-08-01 16:54 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-01 16:54 ` [Qemu-devel] [PATCH 1/4] virtiofsd: process requests in a thread pool Stefan Hajnoczi 2019-08-01 16:54 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-05 12:02 ` [Qemu-devel] " Dr. David Alan Gilbert 2019-08-05 12:02 ` [Virtio-fs] " Dr. David Alan Gilbert 2019-08-07 9:35 ` [Qemu-devel] " Stefan Hajnoczi 2019-08-07 9:35 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-01 16:54 ` [Qemu-devel] [PATCH 2/4] virtiofsd: prevent FUSE_INIT/FUSE_DESTROY races Stefan Hajnoczi 2019-08-01 16:54 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-05 12:26 ` [Qemu-devel] " Dr. David Alan Gilbert 2019-08-05 12:26 ` [Virtio-fs] " Dr. David Alan Gilbert 2019-08-01 16:54 ` [Qemu-devel] [PATCH 3/4] virtiofsd: fix lo_destroy() resource leaks Stefan Hajnoczi 2019-08-01 16:54 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-05 15:17 ` Dr. David Alan Gilbert [this message] 2019-08-05 15:17 ` Dr. David Alan Gilbert 2019-08-05 18:57 ` [Qemu-devel] " Dr. David Alan Gilbert 2019-08-05 18:57 ` [Virtio-fs] " Dr. David Alan Gilbert 2019-08-06 18:58 ` [Qemu-devel] " Dr. David Alan Gilbert 2019-08-06 18:58 ` [Virtio-fs] " Dr. David Alan Gilbert 2019-08-07 9:41 ` [Qemu-devel] " Stefan Hajnoczi 2019-08-07 9:41 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-01 16:54 ` [Qemu-devel] [PATCH 4/4] virtiofsd: add --thread-pool-size=NUM option Stefan Hajnoczi 2019-08-01 16:54 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-05 2:52 ` [Qemu-devel] [Virtio-fs] [PATCH 0/4] virtiofsd: multithreading preparation part 3 piaojun 2019-08-05 2:52 ` piaojun 2019-08-05 8:01 ` [Qemu-devel] " Stefan Hajnoczi 2019-08-05 8:01 ` [Virtio-fs] [Qemu-devel] " Stefan Hajnoczi 2019-08-05 9:40 ` [Qemu-devel] [Virtio-fs] " piaojun 2019-08-05 9:40 ` [Virtio-fs] [Qemu-devel] " piaojun 2019-08-07 18:03 ` Stefan Hajnoczi 2019-08-07 18:03 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-07 20:57 ` [Qemu-devel] " Vivek Goyal 2019-08-07 20:57 ` Vivek Goyal 2019-08-08 9:02 ` [Qemu-devel] " Stefan Hajnoczi 2019-08-08 9:02 ` Stefan Hajnoczi 2019-08-08 9:53 ` [Qemu-devel] " Dr. David Alan Gilbert 2019-08-08 9:53 ` Dr. David Alan Gilbert 2019-08-08 12:53 ` [Qemu-devel] " Vivek Goyal 2019-08-08 12:53 ` Vivek Goyal 2019-08-09 8:23 ` [Qemu-devel] " Stefan Hajnoczi 2019-08-09 8:23 ` Stefan Hajnoczi 2019-08-10 21:35 ` [Qemu-devel] " Liu Bo 2019-08-10 21:35 ` Liu Bo 2019-08-09 8:21 ` [Qemu-devel] " Stefan Hajnoczi 2019-08-09 8:21 ` Stefan Hajnoczi 2019-08-10 21:34 ` [Qemu-devel] " Liu Bo 2019-08-10 21:34 ` Liu Bo 2019-08-11 2:26 ` [Qemu-devel] " piaojun 2019-08-11 2:26 ` piaojun 2019-08-12 10:05 ` [Qemu-devel] " Stefan Hajnoczi 2019-08-12 10:05 ` Stefan Hajnoczi 2019-08-12 11:58 ` [Qemu-devel] " piaojun 2019-08-12 11:58 ` piaojun 2019-08-12 12:51 ` [Qemu-devel] " Dr. David Alan Gilbert 2019-08-12 12:51 ` Dr. David Alan Gilbert 2019-08-08 8:10 ` [Qemu-devel] " piaojun 2019-08-08 8:10 ` piaojun 2019-08-08 9:53 ` [Qemu-devel] " Stefan Hajnoczi 2019-08-08 9:53 ` Stefan Hajnoczi
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=20190805151708.GN13734@work-vm \ --to=dgilbert@redhat.com \ --cc=bo.liu@linux.alibaba.com \ --cc=qemu-devel@nongnu.org \ --cc=stefanha@redhat.com \ --cc=virtio-fs@redhat.com \ /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: linkBe 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.