From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> To: Greg Kurz <groug@kaod.org> Cc: "Daniel P . Berrangé" <berrange@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, qemu-devel@nongnu.org, virtio-fs@redhat.com, "Stefan Hajnoczi" <stefanha@redhat.com>, "Vivek Goyal" <vgoyal@redhat.com> Subject: Re: [PATCH v2 7/7] virtiofsd: Release vu_dispatch_lock when stopping queue Date: Mon, 15 Mar 2021 14:57:17 +0000 [thread overview] Message-ID: <YE91zZZu6PlE4zhz@work-vm> (raw) In-Reply-To: <20210312092212.782255-8-groug@kaod.org> * Greg Kurz (groug@kaod.org) wrote: > QEMU can stop a virtqueue by sending a VHOST_USER_GET_VRING_BASE request > to virtiofsd. As with all other vhost-user protocol messages, the thread > that runs the main event loop in virtiofsd takes the vu_dispatch lock in > write mode. This ensures that no other thread can access virtqueues or > memory tables at the same time. > > In the case of VHOST_USER_GET_VRING_BASE, the main thread basically > notifies the queue thread that it should terminate and waits for its > termination: > > main() > virtio_loop() > vu_dispatch_wrlock() > vu_dispatch() > vu_process_message() > vu_get_vring_base_exec() > fv_queue_cleanup_thread() > pthread_join() > > Unfortunately, the queue thread ends up calling virtio_send_msg() > at some point, which itself needs to grab the lock: > > fv_queue_thread() > g_list_foreach() > fv_queue_worker() > fuse_session_process_buf_int() > do_release() > lo_release() > fuse_reply_err() > send_reply() > send_reply_iov() > fuse_send_reply_iov_nofree() > fuse_send_msg() > virtio_send_msg() > vu_dispatch_rdlock() <-- Deadlock ! > > Simply have the main thread to release the lock before going to > sleep and take it back afterwards. A very similar patch was already > sent by Vivek Goyal sometime back: > > https://listman.redhat.com/archives/virtio-fs/2021-January/msg00073.html > > The only difference here is that this done in fv_queue_set_started() > because fv_queue_cleanup_thread() can also be called from virtio_loop() > without the lock being held. > > Signed-off-by: Greg Kurz <groug@kaod.org> > Reviewed-by: Vivek Goyal <vgoyal@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> I've queued just this 7/7 in the virtiofsd pull I'm just making at the moment. I'll leave the rest to ride the vhost-user train. Dave > --- > tools/virtiofsd/fuse_virtio.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > index 523ee64fb7ae..3e13997406bf 100644 > --- a/tools/virtiofsd/fuse_virtio.c > +++ b/tools/virtiofsd/fuse_virtio.c > @@ -792,7 +792,13 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) > assert(0); > } > } else { > + /* > + * Temporarily drop write-lock taken in virtio_loop() so that > + * the queue thread doesn't block in virtio_send_msg(). > + */ > + vu_dispatch_unlock(vud); > fv_queue_cleanup_thread(vud, qidx); > + vu_dispatch_wrlock(vud); > } > } > > -- > 2.26.2 > -- 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: Greg Kurz <groug@kaod.org> Cc: "Daniel P . Berrangé" <berrange@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, qemu-devel@nongnu.org, virtio-fs@redhat.com, "Vivek Goyal" <vgoyal@redhat.com> Subject: Re: [Virtio-fs] [PATCH v2 7/7] virtiofsd: Release vu_dispatch_lock when stopping queue Date: Mon, 15 Mar 2021 14:57:17 +0000 [thread overview] Message-ID: <YE91zZZu6PlE4zhz@work-vm> (raw) In-Reply-To: <20210312092212.782255-8-groug@kaod.org> * Greg Kurz (groug@kaod.org) wrote: > QEMU can stop a virtqueue by sending a VHOST_USER_GET_VRING_BASE request > to virtiofsd. As with all other vhost-user protocol messages, the thread > that runs the main event loop in virtiofsd takes the vu_dispatch lock in > write mode. This ensures that no other thread can access virtqueues or > memory tables at the same time. > > In the case of VHOST_USER_GET_VRING_BASE, the main thread basically > notifies the queue thread that it should terminate and waits for its > termination: > > main() > virtio_loop() > vu_dispatch_wrlock() > vu_dispatch() > vu_process_message() > vu_get_vring_base_exec() > fv_queue_cleanup_thread() > pthread_join() > > Unfortunately, the queue thread ends up calling virtio_send_msg() > at some point, which itself needs to grab the lock: > > fv_queue_thread() > g_list_foreach() > fv_queue_worker() > fuse_session_process_buf_int() > do_release() > lo_release() > fuse_reply_err() > send_reply() > send_reply_iov() > fuse_send_reply_iov_nofree() > fuse_send_msg() > virtio_send_msg() > vu_dispatch_rdlock() <-- Deadlock ! > > Simply have the main thread to release the lock before going to > sleep and take it back afterwards. A very similar patch was already > sent by Vivek Goyal sometime back: > > https://listman.redhat.com/archives/virtio-fs/2021-January/msg00073.html > > The only difference here is that this done in fv_queue_set_started() > because fv_queue_cleanup_thread() can also be called from virtio_loop() > without the lock being held. > > Signed-off-by: Greg Kurz <groug@kaod.org> > Reviewed-by: Vivek Goyal <vgoyal@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> I've queued just this 7/7 in the virtiofsd pull I'm just making at the moment. I'll leave the rest to ride the vhost-user train. Dave > --- > tools/virtiofsd/fuse_virtio.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > index 523ee64fb7ae..3e13997406bf 100644 > --- a/tools/virtiofsd/fuse_virtio.c > +++ b/tools/virtiofsd/fuse_virtio.c > @@ -792,7 +792,13 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) > assert(0); > } > } else { > + /* > + * Temporarily drop write-lock taken in virtio_loop() so that > + * the queue thread doesn't block in virtio_send_msg(). > + */ > + vu_dispatch_unlock(vud); > fv_queue_cleanup_thread(vud, qidx); > + vu_dispatch_wrlock(vud); > } > } > > -- > 2.26.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2021-03-15 14:59 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-12 9:22 [PATCH v2 0/7] virtiofsd: Avoid potential deadlocks Greg Kurz 2021-03-12 9:22 ` [Virtio-fs] " Greg Kurz 2021-03-12 9:22 ` [PATCH v2 1/7] vhost-user: Drop misleading EAGAIN checks in slave_read() Greg Kurz 2021-03-12 9:22 ` [Virtio-fs] " Greg Kurz 2021-03-15 10:34 ` Stefan Hajnoczi 2021-03-15 10:34 ` [Virtio-fs] " Stefan Hajnoczi 2021-03-12 9:22 ` [PATCH v2 2/7] vhost-user: Fix double-close on slave_read() error path Greg Kurz 2021-03-12 9:22 ` [Virtio-fs] " Greg Kurz 2021-03-15 10:36 ` Stefan Hajnoczi 2021-03-15 10:36 ` [Virtio-fs] " Stefan Hajnoczi 2021-03-12 9:22 ` [PATCH v2 3/7] vhost-user: Factor out duplicated slave_fd teardown code Greg Kurz 2021-03-12 9:22 ` [Virtio-fs] " Greg Kurz 2021-03-15 10:36 ` Stefan Hajnoczi 2021-03-15 10:36 ` [Virtio-fs] " Stefan Hajnoczi 2021-03-12 9:22 ` [PATCH v2 4/7] vhost-user: Convert slave channel to QIOChannelSocket Greg Kurz 2021-03-12 9:22 ` [Virtio-fs] " Greg Kurz 2021-03-15 10:37 ` Stefan Hajnoczi 2021-03-15 10:37 ` [Virtio-fs] " Stefan Hajnoczi 2021-03-12 9:22 ` [PATCH v2 5/7] vhost-user: Introduce nested event loop in vhost_user_read() Greg Kurz 2021-03-12 9:22 ` [Virtio-fs] " Greg Kurz 2021-03-15 10:38 ` Stefan Hajnoczi 2021-03-15 10:38 ` [Virtio-fs] " Stefan Hajnoczi 2021-03-12 9:22 ` [PATCH v2 6/7] vhost-user: Monitor slave channel " Greg Kurz 2021-03-12 9:22 ` [Virtio-fs] " Greg Kurz 2021-03-15 12:20 ` Stefan Hajnoczi 2021-03-15 12:20 ` [Virtio-fs] " Stefan Hajnoczi 2021-03-12 9:22 ` [PATCH v2 7/7] virtiofsd: Release vu_dispatch_lock when stopping queue Greg Kurz 2021-03-12 9:22 ` [Virtio-fs] " Greg Kurz 2021-03-15 14:57 ` Dr. David Alan Gilbert [this message] 2021-03-15 14:57 ` Dr. David Alan Gilbert
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=YE91zZZu6PlE4zhz@work-vm \ --to=dgilbert@redhat.com \ --cc=berrange@redhat.com \ --cc=groug@kaod.org \ --cc=mst@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=stefanha@redhat.com \ --cc=vgoyal@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.