[Qemu-devel] [PATCH 0/3] virtio-scsi-dataplane: graceful fail

[Qemu-devel] [PATCH 0/3] virtio-scsi-dataplane: graceful fail

[Cornelia Huck]

Hi,

these patches are practically a duplicate of the patches I did for
virtio-blk dataplane -- they don't convert to the error report
infrastructure, for example. The aim was to do the same changes
as for virtio-blk in order to avoid killing the whole guest if we
run e.g. out of file handles for the notifiers.

Untested (better get your kittens to a safe place before trying out),
but a git branch is available at

git://github.com/cohuck/qemu graceful-scsi-dataplane

Cornelia Huck (3):
  virtio-scsi: dataplane: print why starting failed
  virtio-scsi: dataplane: fail setup gracefully
  virtio-scsi: dataplane: stop trying on notifier error

 hw/scsi/virtio-scsi-dataplane.c | 92 +++++++++++++++++++++++++++++++++++------
 include/hw/virtio/virtio-scsi.h |  1 +
 2 files changed, 80 insertions(+), 13 deletions(-)

-- 
1.8.5.5

[Qemu-devel] [PATCH 1/3] virtio-scsi: dataplane: print why starting failed

[Cornelia Huck]

Setting up guest or host notifiers may fail, but the user will have
no idea why: Let's print the error returned by the callback.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index b778e05..e068297 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -45,10 +45,13 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOSCSIVring *r = g_slice_new(VirtIOSCSIVring);
+    int rc;
 
     /* Set up virtqueue notify */
-    if (k->set_host_notifier(qbus->parent, n, true) != 0) {
-        fprintf(stderr, "virtio-scsi: Failed to set host notifier\n");
+    rc = k->set_host_notifier(qbus->parent, n, true);
+    if (rc != 0) {
+        fprintf(stderr, "virtio-scsi: Failed to set host notifier (%d)\n",
+                rc);
         exit(1);
     }
     r->host_notifier = *virtio_queue_get_host_notifier(vq);
@@ -157,8 +160,8 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
     /* Set up guest notifier (irq) */
     rc = k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, true);
     if (rc != 0) {
-        fprintf(stderr, "virtio-scsi: Failed to set guest notifiers, "
-                "ensure -enable-kvm is set\n");
+        fprintf(stderr, "virtio-scsi: Failed to set guest notifiers (%d), "
+                "ensure -enable-kvm is set\n", rc);
         exit(1);
     }
 
-- 
1.8.5.5

[Qemu-devel] [PATCH 2/3] virtio-scsi: dataplane: fail setup gracefully

[Cornelia Huck]

The dataplane code is currently doing a hard exit on various setup
failures. In practice, this may mean that a guest suddenly dies after
a dataplane device failed to come up (e.g., when a file descriptor
limit is hit for the nth device).

Let's just try to unwind the setup instead and return.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 73 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 9 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index e068297..445219c 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -52,7 +52,7 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
     if (rc != 0) {
         fprintf(stderr, "virtio-scsi: Failed to set host notifier (%d)\n",
                 rc);
-        exit(1);
+        return NULL;
     }
     r->host_notifier = *virtio_queue_get_host_notifier(vq);
     r->guest_notifier = *virtio_queue_get_guest_notifier(vq);
@@ -62,9 +62,15 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
 
     if (!vring_setup(&r->vring, VIRTIO_DEVICE(s), n)) {
         fprintf(stderr, "virtio-scsi: VRing setup failed\n");
-        exit(1);
+        goto fail_vring;
     }
     return r;
+
+fail_vring:
+    aio_set_event_notifier(s->ctx, &r->host_notifier, NULL);
+    k->set_host_notifier(qbus->parent, n, false);
+    g_slice_free(VirtIOSCSIVring, r);
+    return NULL;
 }
 
 VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
@@ -140,6 +146,40 @@ static void virtio_scsi_iothread_handle_cmd(EventNotifier *notifier)
     }
 }
 
+/* assumes s->ctx held */
+static void virtio_scsi_clear_aio(VirtIOSCSI *s)
+{
+    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+    int i;
+
+    if (s->ctrl_vring) {
+        aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
+    }
+    if (s->event_vring) {
+        aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
+    }
+    for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
+        aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier, NULL);
+    }
+}
+
+static void virtio_scsi_vring_teardown(VirtIOSCSI *s)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+    int i;
+
+    if (s->ctrl_vring) {
+        vring_teardown(&s->ctrl_vring->vring, vdev, 0);
+    }
+    if (s->event_vring) {
+        vring_teardown(&s->event_vring->vring, vdev, 1);
+    }
+    for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
+        vring_teardown(&s->cmd_vrings[i]->vring, vdev, 2 + i);
+    }
+}
+
 /* Context: QEMU global mutex held */
 void virtio_scsi_dataplane_start(VirtIOSCSI *s)
 {
@@ -162,27 +202,47 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
     if (rc != 0) {
         fprintf(stderr, "virtio-scsi: Failed to set guest notifiers (%d), "
                 "ensure -enable-kvm is set\n", rc);
-        exit(1);
+        goto fail_guest_notifiers;
     }
 
     aio_context_acquire(s->ctx);
     s->ctrl_vring = virtio_scsi_vring_init(s, vs->ctrl_vq,
                                            virtio_scsi_iothread_handle_ctrl,
                                            0);
+    if (!s->ctrl_vring) {
+        goto fail_vrings;
+    }
     s->event_vring = virtio_scsi_vring_init(s, vs->event_vq,
                                             virtio_scsi_iothread_handle_event,
                                             1);
+    if (!s->event_vring) {
+        goto fail_vrings;
+    }
     s->cmd_vrings = g_malloc0(sizeof(VirtIOSCSIVring) * vs->conf.num_queues);
     for (i = 0; i < vs->conf.num_queues; i++) {
         s->cmd_vrings[i] =
             virtio_scsi_vring_init(s, vs->cmd_vqs[i],
                                    virtio_scsi_iothread_handle_cmd,
                                    i + 2);
+        if (!s->cmd_vrings[i]) {
+            goto fail_vrings;
+        }
     }
 
     aio_context_release(s->ctx);
     s->dataplane_starting = false;
     s->dataplane_started = true;
+
+fail_vrings:
+    virtio_scsi_clear_aio(s);
+    aio_context_release(s->ctx);
+    virtio_scsi_vring_teardown(s);
+    for (i = 0; i < vs->conf.num_queues + 2; i++) {
+        k->set_host_notifier(qbus->parent, i, false);
+    }
+    k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
+fail_guest_notifiers:
+    s->dataplane_starting = false;
 }
 
 /* Context: QEMU global mutex held */
@@ -190,7 +250,6 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
@@ -215,11 +274,7 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
     /* Sync vring state back to virtqueue so that non-dataplane request
      * processing can continue when we disable the host notifier below.
      */
-    vring_teardown(&s->ctrl_vring->vring, vdev, 0);
-    vring_teardown(&s->event_vring->vring, vdev, 1);
-    for (i = 0; i < vs->conf.num_queues; i++) {
-        vring_teardown(&s->cmd_vrings[i]->vring, vdev, 2 + i);
-    }
+    virtio_scsi_vring_teardown(s);
 
     for (i = 0; i < vs->conf.num_queues + 2; i++) {
         k->set_host_notifier(qbus->parent, i, false);
-- 
1.8.5.5

[Qemu-devel] [PATCH 3/3] virtio-scsi: dataplane: stop trying on notifier error

[Cornelia Huck]

There's no use to constantly trying to enable dataplane if we failed
to set up guest or host notifiers, so fence it off in that case.
We'll try again if the device is reinitialized.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 8 ++++++++
 include/hw/virtio/virtio-scsi.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 445219c..30366d9 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -52,6 +52,7 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
     if (rc != 0) {
         fprintf(stderr, "virtio-scsi: Failed to set host notifier (%d)\n",
                 rc);
+        s->dataplane_fenced = true;
         return NULL;
     }
     r->host_notifier = *virtio_queue_get_host_notifier(vq);
@@ -191,6 +192,7 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
 
     if (s->dataplane_started ||
         s->dataplane_starting ||
+        s->dataplane_fenced ||
         s->ctx != iothread_get_aio_context(vs->conf.iothread)) {
         return;
     }
@@ -202,6 +204,7 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
     if (rc != 0) {
         fprintf(stderr, "virtio-scsi: Failed to set guest notifiers (%d), "
                 "ensure -enable-kvm is set\n", rc);
+        s->dataplane_fenced = true;
         goto fail_guest_notifiers;
     }
 
@@ -253,6 +256,11 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
+    /* Better luck next time. */
+    if (s->dataplane_fenced) {
+        s->dataplane_fenced = false;
+        return;
+    }
     if (!s->dataplane_started || s->dataplane_stopping) {
         return;
     }
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index d6e5e79..916c320 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -195,6 +195,7 @@ typedef struct VirtIOSCSI {
     bool dataplane_starting;
     bool dataplane_stopping;
     bool dataplane_disabled;
+    bool dataplane_fenced;
     Notifier migration_state_notifier;
 } VirtIOSCSI;
 
-- 
1.8.5.5

Re: [Qemu-devel] [PATCH 0/3] virtio-scsi-dataplane: graceful fail

[Paolo Bonzini]

On 10/15/2014 03:15 PM, Cornelia Huck wrote:
> Hi,
>
> these patches are practically a duplicate of the patches I did for
> virtio-blk dataplane -- they don't convert to the error report
> infrastructure, for example. The aim was to do the same changes
> as for virtio-blk in order to avoid killing the whole guest if we
> run e.g. out of file handles for the notifiers.
>
> Untested (better get your kittens to a safe place before trying out),
> but a git branch is available at
>
> git://github.com/cohuck/qemu graceful-scsi-dataplane
>
> Cornelia Huck (3):
>    virtio-scsi: dataplane: print why starting failed
>    virtio-scsi: dataplane: fail setup gracefully
>    virtio-scsi: dataplane: stop trying on notifier error
>
>   hw/scsi/virtio-scsi-dataplane.c | 92 +++++++++++++++++++++++++++++++++++------
>   include/hw/virtio/virtio-scsi.h |  1 +
>   2 files changed, 80 insertions(+), 13 deletions(-)
>

Thanks.  I'll test and send a pull request.

Paolo