[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/7] vhost: recheck dev state in the vhost_migration_log r
From: |
Dima Stepanov |
Subject: |
Re: [PATCH v2 1/7] vhost: recheck dev state in the vhost_migration_log routine |
Date: |
Mon, 31 Aug 2020 11:24:09 +0300 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Thu, Aug 27, 2020 at 09:44:22PM -0400, Raphael Norwitz wrote:
> Generally I’m happy with this. Some comments:
>
> On Mon, Aug 24, 2020 at 4:40 AM Dima Stepanov <dimastep@yandex-team.ru> wrote:
> >
> > vhost-user devices can get a disconnect in the middle of the VHOST-USER
> > handshake on the migration start. If disconnect event happened right
> > before sending next VHOST-USER command, then the vhost_dev_set_log()
> > call in the vhost_migration_log() function will return error. This error
> > will lead to the assert() and close the QEMU migration source process.
> > For the vhost-user devices the disconnect event should not break the
> > migration process, because:
> > - the device will be in the stopped state, so it will not be changed
> > during migration
> > - if reconnect will be made the migration log will be reinitialized as
> > part of reconnect/init process:
> > #0 vhost_log_global_start (listener=0x563989cf7be0)
> > at hw/virtio/vhost.c:920
> > #1 0x000056398603d8bc in listener_add_address_space
> > (listener=0x563989cf7be0,
> > as=0x563986ea4340 <address_space_memory>)
> > at softmmu/memory.c:2664
> > #2 0x000056398603dd30 in memory_listener_register
> > (listener=0x563989cf7be0,
> > as=0x563986ea4340 <address_space_memory>)
> > at softmmu/memory.c:2740
> > #3 0x0000563985fd6956 in vhost_dev_init (hdev=0x563989cf7bd8,
> > opaque=0x563989cf7e30, backend_type=VHOST_BACKEND_TYPE_USER,
> > busyloop_timeout=0)
> > at hw/virtio/vhost.c:1385
> > #4 0x0000563985f7d0b8 in vhost_user_blk_connect (dev=0x563989cf7990)
> > at hw/block/vhost-user-blk.c:315
> > #5 0x0000563985f7d3f6 in vhost_user_blk_event (opaque=0x563989cf7990,
> > event=CHR_EVENT_OPENED)
> > at hw/block/vhost-user-blk.c:379
> > Update the vhost-user-blk device with the internal started field which
> > will be used for initialization and clean up. The disconnect event will
> > set the overall VHOST device to the stopped state, so it can be used by
> > the vhost_migration_log routine.
> > Such approach could be propogated to the other vhost-user devices, but
> > better idea is just to make the same connect/disconnect code for all the
> > vhost-user devices.
> >
> > This migration issue was slightly discussed earlier:
> > - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01509.html
> > - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg05241.html
> >
>
> What you’re doing here on a structural level is adding an additional
> flag “started” to VhostUserBlk to track whether the device really
> needs to be stopped and cleaned up on a vhost level on a disconnect.
> Can you make that more clear in the commit message as you have in the
> comments?
Sounds reasonable, will update the commit message.
>
>
> > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> > ---
> > hw/block/vhost-user-blk.c | 19 ++++++++++++++++---
> > hw/virtio/vhost.c | 27 ++++++++++++++++++++++++---
> > include/hw/virtio/vhost-user-blk.h | 10 ++++++++++
> > 3 files changed, 50 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index a00b854..5573e89 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -150,6 +150,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
> > error_report("Error starting vhost: %d", -ret);
> > goto err_guest_notifiers;
> > }
> > + s->started = true;
> >
> > /* guest_notifier_mask/pending not used yet, so just unmask
> > * everything here. virtio-pci will do the right thing by
> > @@ -175,6 +176,11 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
> > VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > int ret;
> >
> > + if (!s->started) {
> > + return;
> > + }
> > + s->started = false;
> > +
> > if (!k->set_guest_notifiers) {
> > return;
> > }
> > @@ -341,9 +347,7 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> > }
> > s->connected = false;
> >
> > - if (s->dev.started) {
> > - vhost_user_blk_stop(vdev);
> > - }
> > + vhost_user_blk_stop(vdev);
> >
> > vhost_dev_cleanup(&s->dev);
> > }
> > @@ -399,6 +403,15 @@ static void vhost_user_blk_event(void *opaque,
> > QEMUChrEvent event)
> > NULL, NULL, false);
> > aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh,
> > opaque);
> > }
> > +
> > + /*
> > + * Move vhost device to the stopped state. The vhost-user device
> > + * will be clean up and disconnected in BH. This can be useful in
> > + * the vhost migration code. If disconnect was caught there is an
> > + * option for the general vhost code to get the dev state without
> > + * knowing its type (in this case vhost-user).
> > + */
> > + s->dev.started = false;
> > break;
> > case CHR_EVENT_BREAK:
> > case CHR_EVENT_MUX_IN:
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 1a1384e..ffef7ab 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -861,21 +861,42 @@ static int vhost_migration_log(MemoryListener
> > *listener, bool enable)
> > dev->log_enabled = enable;
> > return 0;
> > }
> > +
> > + r = 0;
> > if (!enable) {
> > r = vhost_dev_set_log(dev, false);
> > if (r < 0) {
> > - return r;
> > + goto check_dev_state;
> > }
> > vhost_log_put(dev, false);
> > } else {
> > vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> > r = vhost_dev_set_log(dev, true);
> > if (r < 0) {
> > - return r;
> > + goto check_dev_state;
> > }
> > }
> > +
> > +check_dev_state:
> > dev->log_enabled = enable;
> > - return 0;
> > + /*
> > + * vhost-user-* devices could change their state during log
> > + * initialization due to disconnect. So check dev state after
> > + * vhost communication.
> > + */
> > + if (!dev->started) {
> > + /*
> > + * Since device is in the stopped state, it is okay for
> > + * migration. Return success.
> > + */
> > + r = 0;
> > + }
> > + if (r) {
> > + /* An error is occured. */
> > + dev->log_enabled = false;
> > + }
> > +
> > + return r;
> > }
> >
> > static void vhost_log_global_start(MemoryListener *listener)
> > diff --git a/include/hw/virtio/vhost-user-blk.h
> > b/include/hw/virtio/vhost-user-blk.h
> > index 34ad6f0..f4c0754 100644
> > --- a/include/hw/virtio/vhost-user-blk.h
> > +++ b/include/hw/virtio/vhost-user-blk.h
> > @@ -38,7 +38,17 @@ typedef struct VHostUserBlk {
> > VhostUserState vhost_user;
> > struct vhost_virtqueue *vhost_vqs;
> > VirtQueue **virtqs;
> > +
> > + /*
> > + * There are at least two steps of initialization of the
> > + * vhost-user device. The first is a "connect" step and
> > + * second is a "start" step. Make a separation between
> > + * those initialization phases by using two fields.
> > + */
> > + /* vhost_user_blk_connect/vhost_user_blk_disconnect */
> > bool connected;
> > + /* vhost_user_blk_start/vhost_user_blk_stop */
>
> I don’t like the name “started” for the vhost-user-blk flag because
> it’s easy to get confused with the vhost_dev "started" field, and they
> are used for very different things. Maybe call it “needs_cleanup” or
> “started_internal”, or something to that effect?
Good point. In this case i like the "started_internal" or "started_vu"
just to show that it is only vhost-user start/stop routine flag.
No other comments mixed in below.
>
>
> > + bool started;
> > } VHostUserBlk;
> >
> > #endif
> > --
> > 2.7.4
> >
> >
- [PATCH v2 0/7] vhost-user-blk: fix the migration issue and enhance qtests, Dima Stepanov, 2020/08/24
- [PATCH v2 3/7] tests/qtest/vhost-user-test: prepare the tests for adding new dev class, Dima Stepanov, 2020/08/24
- [PATCH v2 4/7] tests/qtest/libqos/virtio-blk: add support for vhost-user-blk, Dima Stepanov, 2020/08/24
- [PATCH v2 5/7] tests/qtest/vhost-user-test: add support for the vhost-user-blk device, Dima Stepanov, 2020/08/24
- [PATCH v2 6/7] tests/qtest/vhost-user-test: add migrate_reconnect test, Dima Stepanov, 2020/08/24
- [PATCH v2 7/7] tests/qtest/vhost-user-test: enable the reconnect tests, Dima Stepanov, 2020/08/24
- Re: [PATCH v2 0/7] vhost-user-blk: fix the migration issue and enhance qtests, no-reply, 2020/08/24