qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v1 0/7] vhost-user reconnect issues during vhost initialization
@ 2020-04-23 18:39 Dima Stepanov
  2020-04-23 18:39 ` [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate disconnect on init Dima Stepanov
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Dima Stepanov @ 2020-04-23 18:39 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, kwolf, yc-core, qemu-block, mst, jasowang, dgilbert, mreitz,
	arei.gonglei, stefanha, marcandre.lureau, pbonzini,
	raphael.norwitz

During vhost-user reconnect functionality we hit several issues, if
vhost-user-blk daemon is "crashed" or made disconnect during vhost
initialization. The general scenario is as follows:
  - vhost start routine is called
  - vhost write failed due to SIGPIPE
  - this call the disconnect routine and vhost_dev_cleanup routine
    which set to 0 all the field of the vhost_dev structure
  - return back to vhost start routine with the error
  - on the fail path vhost start routine tries to rollback the changes
    by using vhost_dev struct fields which were already reset
  - sometimes this leads to SIGSEGV, sometimes to SIGABRT
Before revising the vhost-user initialization code, we suggest adding
the sanity checks to be aware of the possible disconnect event and that
the vhost_dev structure can be in "uninitialized" state.

The vhost-user-blk daemon is updated with the additional
"--simulate-disconnect-stage=CASENUM" argument to simulate disconnect during
VHOST device initialization. For instance:
  1. $ ./vhost-user-blk -s ./vhost.sock -b test-img.raw --simulate-disconnect-stage=1
     This command will simulate disconnect in the SET_VRING_CALL handler.
     In this case the vhost device in QEMU is not set the started field to
     true.
  2. $ ./vhost-user-blk -s ./vhost.sock -b test-img.raw --simulate-disconnect-stage=2
     This command will simulate disconnect in the SET_VRING_NUM handler.
     In this case the started field is set to true.
These two cases test different QEMU parts. Also to trigger different code paths
disconnect should be simulated in two ways:
  - before any successful initialization
  - make successful initialization once and try to simulate disconnects
Also we catch SIGABRT on the migration start if vhost-user daemon disconnected
during vhost-user set log commands communication.

Dima Stepanov (7):
  contrib/vhost-user-blk: add option to simulate disconnect on init
  char-socket: return -1 in case of disconnect during tcp_chr_write
  char-socket: initialize reconnect timer only if close is emitted
  vhost: introduce wrappers to set guest notifiers for virtio device
  vhost-user-blk: add mechanism to track the guest notifiers init state
  vhost: check vring address before calling unmap
  vhost: add device started check in migration set log

 backends/cryptodev-vhost.c              |  26 +++++---
 backends/vhost-user.c                   |  16 ++---
 chardev/char-socket.c                   |  18 ++---
 contrib/libvhost-user/libvhost-user.c   |  30 +++++++++
 contrib/libvhost-user/libvhost-user.h   |  13 ++++
 contrib/vhost-user-blk/vhost-user-blk.c |  14 +++-
 hw/block/vhost-user-blk.c               |  23 +++----
 hw/net/vhost_net.c                      |  30 +++++----
 hw/scsi/vhost-scsi-common.c             |  15 ++---
 hw/virtio/vhost-user-fs.c               |  17 ++---
 hw/virtio/vhost-vsock.c                 |  18 +++--
 hw/virtio/vhost.c                       | 115 +++++++++++++++++++++++++++++---
 hw/virtio/virtio.c                      |  13 ++++
 include/hw/virtio/vhost.h               |   5 ++
 include/hw/virtio/virtio.h              |   1 +
 15 files changed, 256 insertions(+), 98 deletions(-)

-- 
2.7.4



^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [RFC PATCH v1 5/7] vhost-user-blk: add mechanism to track the guest notifiers init state
@ 2020-04-24  2:38 Raphael Norwitz
  0 siblings, 0 replies; 17+ messages in thread
From: Raphael Norwitz @ 2020-04-24  2:38 UTC (permalink / raw)
  To: dimastep; +Cc: qemu-devel

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

There are some problems with this patch. It doesn't apply cleanly.

Are you sure you’re developing on an up to date master branch?

On Thu, Apr 23, 2020 at 09:39:36PM +0300, Dima Stepanov wrote:
>
> In case of the vhost-user devices the daemon can be killed at any
> moment. Since QEMU supports the reconnet functionality the guest
> notifiers should be reset and disabled after "disconnect" event. The
> most issues were found if the "disconnect" event happened during vhost
> device initialization step.
> The disconnect event leads to the call of the vhost_dev_cleanup()
> routine. Which memset to 0 a vhost device structure. Because of this, if
> device was not started (dev.started == false) and the connection is
> broken, then the set_guest_notifier method will produce assertion error.
> Also connection can be broken after the dev.started field is set to
> true.
> A new notifiers_set field is added to the vhost_dev structure to track
> the state of the guest notifiers during the initialization process.
>
> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> ---
>  hw/block/vhost-user-blk.c |  8 ++++----
>  hw/virtio/vhost.c         | 11 +++++++++++
>  include/hw/virtio/vhost.h |  1 +
>  3 files changed, 16 insertions(+), 4 deletions(-)
> @@ -1449,6 +1456,10 @@ int vhost_dev_drop_guest_notifiers(struct
vhost_dev *hdev,

I can’t find the function vhost_dev_drop_guest_notifiers() in
/hw/virtio/vhost.c, or anywhere in the codebase.

Where does this code come from?

>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      int ret;
>
> +    if (!hdev->notifiers_set) {
> +        return 0;
> +    }
> +
>      ret = k->set_guest_notifiers(qbus->parent, nvqs, false);
>      if (ret < 0) {
>          error_report("Error reset guest notifier: %d", -ret);
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 4d0d2e2..e3711a7 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -90,6 +90,7 @@ struct vhost_dev {
>      QLIST_HEAD(, vhost_iommu) iommu_list;
>      IOMMUNotifier n;
>      const VhostDevConfigOps *config_ops;
> +    bool notifiers_set;
>  };
>
>  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> --
> 2.7.4
>
>

[-- Attachment #2: Type: text/html, Size: 6586 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-04-27  9:39 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-23 18:39 [RFC PATCH v1 0/7] vhost-user reconnect issues during vhost initialization Dima Stepanov
2020-04-23 18:39 ` [RFC PATCH v1 1/7] contrib/vhost-user-blk: add option to simulate disconnect on init Dima Stepanov
     [not found]   ` <20200422160206.GA30919@localhost.localdomain>
2020-04-24 10:17     ` Marc-André Lureau
2020-04-25  9:30       ` Dima Stepanov
2020-04-23 18:39 ` [RFC PATCH v1 2/7] char-socket: return -1 in case of disconnect during tcp_chr_write Dima Stepanov
2020-04-23 20:10   ` Marc-André Lureau
     [not found]   ` <ca921f6f56104bcbb664424f97558ec3@HE1PR08MB2650.eurprd08.prod.outlook.com>
2020-04-24  9:55     ` Anton Nefedov
2020-04-23 18:39 ` [RFC PATCH v1 3/7] char-socket: initialize reconnect timer only if close is emitted Dima Stepanov
2020-04-23 19:16   ` Marc-André Lureau
2020-04-26  7:26     ` Li Feng
2020-04-27  9:38       ` Dima Stepanov
2020-04-23 18:39 ` [RFC PATCH v1 4/7] vhost: introduce wrappers to set guest notifiers for virtio device Dima Stepanov
2020-04-23 18:39 ` [RFC PATCH v1 5/7] vhost-user-blk: add mechanism to track the guest notifiers init state Dima Stepanov
     [not found]   ` <20200422161750.GC31091@localhost.localdomain>
2020-04-24  9:17     ` Dima Stepanov
2020-04-23 18:39 ` [RFC PATCH v1 6/7] vhost: check vring address before calling unmap Dima Stepanov
2020-04-23 18:39 ` [RFC PATCH v1 7/7] vhost: add device started check in migration set log Dima Stepanov
  -- strict thread matches above, loose matches on Subject: below --
2020-04-24  2:38 [RFC PATCH v1 5/7] vhost-user-blk: add mechanism to track the guest notifiers init state Raphael Norwitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).