Re: [Qemu-devel] [PATCH v7 6/7] vhost-user-blk: Add support to reconnect backend

[Daniel P . Berrangé]

On Tue, Mar 12, 2019 at 12:49:35PM -0400, Michael S. Tsirkin wrote:
> On Thu, Feb 28, 2019 at 04:53:54PM +0800, address@hidden wrote:
> > From: Xie Yongji <address@hidden>
> > 
> > Since we now support the message VHOST_USER_GET_INFLIGHT_FD
> > and VHOST_USER_SET_INFLIGHT_FD. The backend is able to restart
> > safely because it can track inflight I/O in shared memory.
> > This patch allows qemu to reconnect the backend after
> > connection closed.
> > 
> > Signed-off-by: Xie Yongji <address@hidden>
> > Signed-off-by: Ni Xun <address@hidden>
> > Signed-off-by: Zhang Yu <address@hidden>
> > ---
> >  hw/block/vhost-user-blk.c          | 205 +++++++++++++++++++++++------
> >  include/hw/virtio/vhost-user-blk.h |   4 +
> >  2 files changed, 167 insertions(+), 42 deletions(-)


> >  static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      VHostUserBlk *s = VHOST_USER_BLK(vdev);
> >      VhostUserState *user;
> > -    struct vhost_virtqueue *vqs = NULL;
> >      int i, ret;
> > +    Error *err = NULL;
> >  
> >      if (!s->chardev.chr) {
> >          error_setg(errp, "vhost-user-blk: chardev is mandatory");
> > @@ -312,27 +442,28 @@ static void vhost_user_blk_device_realize(DeviceState 
> > *dev, Error **errp)
> >      }
> >  
> >      s->inflight = g_new0(struct vhost_inflight, 1);
> > -
> > -    s->dev.nvqs = s->num_queues;
> > -    s->dev.vqs = g_new(struct vhost_virtqueue, s->dev.nvqs);
> > -    s->dev.vq_index = 0;
> > -    s->dev.backend_features = 0;
> > -    vqs = s->dev.vqs;
> > -
> > -    vhost_dev_set_config_notifier(&s->dev, &blk_ops);
> > -
> > -    ret = vhost_dev_init(&s->dev, s->vhost_user, VHOST_BACKEND_TYPE_USER, 
> > 0);
> > -    if (ret < 0) {
> > -        error_setg(errp, "vhost-user-blk: vhost initialization failed: %s",
> > -                   strerror(-ret));
> > -        goto virtio_err;
> > -    }
> > +    s->vqs = g_new(struct vhost_virtqueue, s->num_queues);
> > +    s->watch = 0;
> > +    s->should_start = false;
> > +    s->connected = false;
> > +
> > +    qemu_chr_fe_set_handlers(&s->chardev,  NULL, NULL, 
> > vhost_user_blk_event,
> > +                             NULL, (void *)dev, NULL, true);
> > +
> > +reconnect:
> > +    do {
> > +        if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) {
> > +            error_report_err(err);
> > +            err = NULL;
> > +            sleep(1);
> 
> Seems arbitrary. Is this basically waiting until backend will reconnect?
> Why not block until event on the fd triggers?
> 
> Also, it looks like this will just block forever with no monitor input
> and no way for user to figure out what is going on short of
> crashing QEMU.

FWIW, the current vhost-user-net device does exactly the same thing
with calling qemu_chr_fe_wait_connected during its realize() function.

Can't say I'm thrilled about the existing usage either, but I don't
see a particularly nice alternative if it isn't possible to realize
the device without having a backend available.

I don't think this an especially serious problem during cold startup
though since doesn't expect the monitor to become responsive until
the device model is fully realized & all backends connected.

The real concern is if you need to hotplug this kind of device
at runtime. In that case blocking the main loop / monitor is a
serious problem that will actively harm both the guest OS and
mgmt applications.

The vhost-user-net device will already suffer from that.

To solve the problem with hotplug, the mgmt app would need to
plug the chardev backend, then wait for it to become connected,
and only then plug the device frontend. In that case we would not
want this loop. We'd expect it to use the already connected
chardev, and fail the realize() function if the chardev has
lost its connection.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|