Re: [PATCH 1/4] vhost-user: Introduce nested event loop in vhost_user_read()

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

On Tue, 9 Mar 2021 15:02:48 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Mon, Mar 08, 2021 at 01:31:38PM +0100, Greg Kurz wrote:
> > 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.
> > 
> > As a preliminary step to address this, split vhost_user_read() into a
> > nested even loop and a one-shot callback that does the actual reading.
> 
> In case you respin:
> s/even/event/
> 

Fixed.

> > +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
> > +    };
> > +
> > +    /* Switch context and add a new watch to monitor chardev activity */
> > +    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);
> 
> This comment could be expanded to explain why the nested event loop is
> necessary. The goal is to monitor the slave_fd while waiting for chr
> I/O so we'll need an event loop. prev_ctxt cannot be run nested since
> its fd handlers may not be prepared (e.g. re-entrancy).

Ok, will do.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]