From: Greg Kurz <groug@kaod.org> To: qemu-devel@nongnu.org Cc: "Daniel P . Berrangé" <berrange@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, "Greg Kurz" <groug@kaod.org>, virtio-fs@redhat.com, "Stefan Hajnoczi" <stefanha@redhat.com>, "Vivek Goyal" <vgoyal@redhat.com> Subject: [PATCH v2 5/7] vhost-user: Introduce nested event loop in vhost_user_read() Date: Fri, 12 Mar 2021 10:22:10 +0100 [thread overview] Message-ID: <20210312092212.782255-6-groug@kaod.org> (raw) In-Reply-To: <20210312092212.782255-1-groug@kaod.org> A deadlock condition potentially exists if a vhost-user process needs to request something to QEMU on the slave channel while processing a vhost-user message. This doesn't seem to affect any vhost-user implementation so far, but this is currently biting the upcoming enablement of DAX with virtio-fs. The issue is being observed when the guest does an emergency reboot while a mapping still exits in the DAX window, which is very easy to get with a busy enough workload (e.g. as simulated by blogbench [1]) : - QEMU sends VHOST_USER_GET_VRING_BASE to virtiofsd. - In order to complete the request, virtiofsd then asks QEMU to remove the mapping on the slave channel. All these dialogs are synchronous, hence the deadlock. As pointed out by Stefan Hajnoczi: When QEMU's vhost-user master implementation sends a vhost-user protocol message, vhost_user_read() does a "blocking" read during which slave_fd is not monitored by QEMU. The natural solution for this issue is an event loop. The main event loop cannot be nested though since we have no guarantees that its fd handlers are prepared for re-entrancy. Introduce a new event loop that only monitors the chardev I/O for now in vhost_user_read() and push the actual reading to a one-shot handler. A subsequent patch will teach the loop to monitor and process messages from the slave channel as well. [1] https://github.com/jedisct1/Blogbench Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Greg Kurz <groug@kaod.org> --- v2: - Document why a nested loop is needed in vhost_user_read() (Stefan) --- hw/virtio/vhost-user.c | 65 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 3c1e1611b087..00256fa318a6 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -296,15 +296,27 @@ static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg) return 0; } -static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) +struct vhost_user_read_cb_data { + struct vhost_dev *dev; + VhostUserMsg *msg; + GMainLoop *loop; + int ret; +}; + +static gboolean vhost_user_read_cb(GIOChannel *source, GIOCondition condition, + gpointer opaque) { + struct vhost_user_read_cb_data *data = opaque; + struct vhost_dev *dev = data->dev; + VhostUserMsg *msg = data->msg; struct vhost_user *u = dev->opaque; CharBackend *chr = u->user->chr; uint8_t *p = (uint8_t *) msg; int r, size; if (vhost_user_read_header(dev, msg) < 0) { - return -1; + data->ret = -1; + goto end; } /* validate message size is sane */ @@ -312,7 +324,8 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) error_report("Failed to read msg header." " Size %d exceeds the maximum %zu.", msg->hdr.size, VHOST_USER_PAYLOAD_SIZE); - return -1; + data->ret = -1; + goto end; } if (msg->hdr.size) { @@ -322,11 +335,53 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) if (r != size) { error_report("Failed to read msg payload." " Read %d instead of %d.", r, msg->hdr.size); - return -1; + data->ret = -1; + goto end; } } - return 0; +end: + g_main_loop_quit(data->loop); + return G_SOURCE_REMOVE; +} + +static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) +{ + struct vhost_user *u = dev->opaque; + CharBackend *chr = u->user->chr; + GMainContext *prev_ctxt = chr->chr->gcontext; + GMainContext *ctxt = g_main_context_new(); + GMainLoop *loop = g_main_loop_new(ctxt, FALSE); + struct vhost_user_read_cb_data data = { + .dev = dev, + .loop = loop, + .msg = msg, + .ret = 0 + }; + + /* + * We want to be able to monitor the slave channel fd while waiting + * for chr I/O. This requires an event loop, but we can't nest the + * one to which chr is currently attached : its fd handlers might not + * be prepared for re-entrancy. So we create a new one and switch chr + * to use it. + */ + qemu_chr_be_update_read_handlers(chr->chr, ctxt); + qemu_chr_fe_add_watch(chr, G_IO_IN | G_IO_HUP, vhost_user_read_cb, &data); + + g_main_loop_run(loop); + + /* + * Restore the previous event loop context. This also destroys/recreates + * event sources : this guarantees that all pending events in the original + * context that have been processed by the nested loop are purged. + */ + qemu_chr_be_update_read_handlers(chr->chr, prev_ctxt); + + g_main_loop_unref(loop); + g_main_context_unref(ctxt); + + return data.ret; } static int process_message_reply(struct vhost_dev *dev, -- 2.26.2
WARNING: multiple messages have this Message-ID (diff)
From: Greg Kurz <groug@kaod.org> To: qemu-devel@nongnu.org Cc: "Daniel P . Berrangé" <berrange@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, virtio-fs@redhat.com, "Vivek Goyal" <vgoyal@redhat.com> Subject: [Virtio-fs] [PATCH v2 5/7] vhost-user: Introduce nested event loop in vhost_user_read() Date: Fri, 12 Mar 2021 10:22:10 +0100 [thread overview] Message-ID: <20210312092212.782255-6-groug@kaod.org> (raw) In-Reply-To: <20210312092212.782255-1-groug@kaod.org> A deadlock condition potentially exists if a vhost-user process needs to request something to QEMU on the slave channel while processing a vhost-user message. This doesn't seem to affect any vhost-user implementation so far, but this is currently biting the upcoming enablement of DAX with virtio-fs. The issue is being observed when the guest does an emergency reboot while a mapping still exits in the DAX window, which is very easy to get with a busy enough workload (e.g. as simulated by blogbench [1]) : - QEMU sends VHOST_USER_GET_VRING_BASE to virtiofsd. - In order to complete the request, virtiofsd then asks QEMU to remove the mapping on the slave channel. All these dialogs are synchronous, hence the deadlock. As pointed out by Stefan Hajnoczi: When QEMU's vhost-user master implementation sends a vhost-user protocol message, vhost_user_read() does a "blocking" read during which slave_fd is not monitored by QEMU. The natural solution for this issue is an event loop. The main event loop cannot be nested though since we have no guarantees that its fd handlers are prepared for re-entrancy. Introduce a new event loop that only monitors the chardev I/O for now in vhost_user_read() and push the actual reading to a one-shot handler. A subsequent patch will teach the loop to monitor and process messages from the slave channel as well. [1] https://github.com/jedisct1/Blogbench Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Greg Kurz <groug@kaod.org> --- v2: - Document why a nested loop is needed in vhost_user_read() (Stefan) --- hw/virtio/vhost-user.c | 65 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 3c1e1611b087..00256fa318a6 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -296,15 +296,27 @@ static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg) return 0; } -static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) +struct vhost_user_read_cb_data { + struct vhost_dev *dev; + VhostUserMsg *msg; + GMainLoop *loop; + int ret; +}; + +static gboolean vhost_user_read_cb(GIOChannel *source, GIOCondition condition, + gpointer opaque) { + struct vhost_user_read_cb_data *data = opaque; + struct vhost_dev *dev = data->dev; + VhostUserMsg *msg = data->msg; struct vhost_user *u = dev->opaque; CharBackend *chr = u->user->chr; uint8_t *p = (uint8_t *) msg; int r, size; if (vhost_user_read_header(dev, msg) < 0) { - return -1; + data->ret = -1; + goto end; } /* validate message size is sane */ @@ -312,7 +324,8 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) error_report("Failed to read msg header." " Size %d exceeds the maximum %zu.", msg->hdr.size, VHOST_USER_PAYLOAD_SIZE); - return -1; + data->ret = -1; + goto end; } if (msg->hdr.size) { @@ -322,11 +335,53 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) if (r != size) { error_report("Failed to read msg payload." " Read %d instead of %d.", r, msg->hdr.size); - return -1; + data->ret = -1; + goto end; } } - return 0; +end: + g_main_loop_quit(data->loop); + return G_SOURCE_REMOVE; +} + +static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) +{ + struct vhost_user *u = dev->opaque; + CharBackend *chr = u->user->chr; + GMainContext *prev_ctxt = chr->chr->gcontext; + GMainContext *ctxt = g_main_context_new(); + GMainLoop *loop = g_main_loop_new(ctxt, FALSE); + struct vhost_user_read_cb_data data = { + .dev = dev, + .loop = loop, + .msg = msg, + .ret = 0 + }; + + /* + * We want to be able to monitor the slave channel fd while waiting + * for chr I/O. This requires an event loop, but we can't nest the + * one to which chr is currently attached : its fd handlers might not + * be prepared for re-entrancy. So we create a new one and switch chr + * to use it. + */ + qemu_chr_be_update_read_handlers(chr->chr, ctxt); + qemu_chr_fe_add_watch(chr, G_IO_IN | G_IO_HUP, vhost_user_read_cb, &data); + + g_main_loop_run(loop); + + /* + * Restore the previous event loop context. This also destroys/recreates + * event sources : this guarantees that all pending events in the original + * context that have been processed by the nested loop are purged. + */ + qemu_chr_be_update_read_handlers(chr->chr, prev_ctxt); + + g_main_loop_unref(loop); + g_main_context_unref(ctxt); + + return data.ret; } static int process_message_reply(struct vhost_dev *dev, -- 2.26.2
next prev parent reply other threads:[~2021-03-12 9:24 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 ` Greg Kurz [this message] 2021-03-12 9:22 ` [Virtio-fs] [PATCH v2 5/7] vhost-user: Introduce nested event loop in vhost_user_read() 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 2021-03-15 14:57 ` [Virtio-fs] " 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=20210312092212.782255-6-groug@kaod.org \ --to=groug@kaod.org \ --cc=berrange@redhat.com \ --cc=dgilbert@redhat.com \ --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.