[Qemu-devel] [PATCH RFC 0/3] dataplane: dataplane: more graceful error handling

[Qemu-devel] [PATCH RFC 0/3] dataplane: dataplane: more graceful error handling

[Cornelia Huck]

Currently, qemu will take a hard exit if it fails to set up guest or
host notifiers, giving no real clue as to what went wrong (e.g., when
out of file descriptors).

This patchset tries to make this more manageable: Both by improving the
error message and by gracefully falling back to non-dataplane in case of
errors.

Patches are also available on

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

Thoughts?

Cornelia Huck (3):
  dataplane: print why starting failed
  dataplane: fail notifier setting gracefully
  dataplane: stop trying on notifier error

 hw/block/dataplane/virtio-blk.c |   39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH RFC 1/3] 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.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/block/dataplane/virtio-blk.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index d6ba65c..527a53c 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -218,6 +218,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
     VirtQueue *vq;
+    int r;
 
     if (s->started) {
         return;
@@ -236,16 +237,18 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     }
 
     /* Set up guest notifier (irq) */
-    if (k->set_guest_notifiers(qbus->parent, 1, true) != 0) {
-        fprintf(stderr, "virtio-blk failed to set guest notifier, "
-                "ensure -enable-kvm is set\n");
+    r = k->set_guest_notifiers(qbus->parent, 1, true);
+    if (r != 0) {
+        fprintf(stderr, "virtio-blk failed to set guest notifier (%d), "
+                "ensure -enable-kvm is set\n", r);
         exit(1);
     }
     s->guest_notifier = virtio_queue_get_guest_notifier(vq);
 
     /* Set up virtqueue notify */
-    if (k->set_host_notifier(qbus->parent, 0, true) != 0) {
-        fprintf(stderr, "virtio-blk failed to set host notifier\n");
+    r = k->set_host_notifier(qbus->parent, 0, true);
+    if (r != 0) {
+        fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
         exit(1);
     }
     s->host_notifier = *virtio_queue_get_host_notifier(vq);
-- 
1.7.9.5

[Qemu-devel] [PATCH RFC 2/3] dataplane: fail notifier setting gracefully

[Cornelia Huck]

The dataplane code is currently doing a hard exit if it fails to set
up either guest or host notifiers. 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 tne nth device).

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

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/block/dataplane/virtio-blk.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 527a53c..94e1a29 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -232,8 +232,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
     vq = virtio_get_queue(s->vdev, 0);
     if (!vring_setup(&s->vring, s->vdev, 0)) {
-        s->starting = false;
-        return;
+        goto fail_vring;
     }
 
     /* Set up guest notifier (irq) */
@@ -241,7 +240,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     if (r != 0) {
         fprintf(stderr, "virtio-blk failed to set guest notifier (%d), "
                 "ensure -enable-kvm is set\n", r);
-        exit(1);
+        goto fail_guest_notifiers;
     }
     s->guest_notifier = virtio_queue_get_guest_notifier(vq);
 
@@ -249,7 +248,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     r = k->set_host_notifier(qbus->parent, 0, true);
     if (r != 0) {
         fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
-        exit(1);
+        goto fail_host_notifier;
     }
     s->host_notifier = *virtio_queue_get_host_notifier(vq);
 
@@ -269,6 +268,14 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     aio_context_acquire(s->ctx);
     aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify);
     aio_context_release(s->ctx);
+    return;
+
+  fail_host_notifier:
+    k->set_guest_notifiers(qbus->parent, 1, false);
+  fail_guest_notifiers:
+    vring_teardown(&s->vring, s->vdev, 0);
+  fail_vring:
+    s->starting = false;
 }
 
 /* Context: QEMU global mutex held */
-- 
1.7.9.5

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

[Cornelia Huck]

If we fail to set up guest or host notifiers, there's no use trying again
every time the guest kicks, so disable dataplane in that case.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/block/dataplane/virtio-blk.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 94e1a29..24a6b71 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -28,6 +28,7 @@ struct VirtIOBlockDataPlane {
     bool started;
     bool starting;
     bool stopping;
+    bool disabled;
 
     VirtIOBlkConf *blk;
 
@@ -220,7 +221,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     VirtQueue *vq;
     int r;
 
-    if (s->started) {
+    if (s->started || s->disabled) {
         return;
     }
 
@@ -274,6 +275,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     k->set_guest_notifiers(qbus->parent, 1, false);
   fail_guest_notifiers:
     vring_teardown(&s->vring, s->vdev, 0);
+    s->disabled = true;
   fail_vring:
     s->starting = false;
 }
@@ -284,6 +286,13 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
+
+
+    /* Better luck next time. */
+    if (s->disabled) {
+        s->disabled = false;
+        return;
+    }
     if (!s->started || s->stopping) {
         return;
     }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH RFC 0/3] dataplane: dataplane: more graceful error handling

[Cornelia Huck]

On Fri, 25 Jul 2014 14:10:45 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> Currently, qemu will take a hard exit if it fails to set up guest or
> host notifiers, giving no real clue as to what went wrong (e.g., when
> out of file descriptors).
> 
> This patchset tries to make this more manageable: Both by improving the
> error message and by gracefully falling back to non-dataplane in case of
> errors.
> 
> Patches are also available on
> 
> git://github.com/cohuck/qemu dataplane-graceful-fail
> 
> Thoughts?
> 
> Cornelia Huck (3):
>   dataplane: print why starting failed
>   dataplane: fail notifier setting gracefully
>   dataplane: stop trying on notifier error
> 
>  hw/block/dataplane/virtio-blk.c |   39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 

Ping?

Re: [Qemu-devel] [PATCH RFC 0/3] dataplane: dataplane: more graceful error handling

[Kevin Wolf]

Am 25.07.2014 um 14:10 hat Cornelia Huck geschrieben:
> Currently, qemu will take a hard exit if it fails to set up guest or
> host notifiers, giving no real clue as to what went wrong (e.g., when
> out of file descriptors).
> 
> This patchset tries to make this more manageable: Both by improving the
> error message and by gracefully falling back to non-dataplane in case of
> errors.
> 
> Patches are also available on
> 
> git://github.com/cohuck/qemu dataplane-graceful-fail
> 
> Thoughts?

I think Stefan should comment on this, but I certainly welcome every
patch that fixes an exit(1) call.

I'm not entirely sure about the added fprintf(). It feels wrong, but of
course it's a lot less wrong than exiting. Ideally already adding the
device with dataplane enabled would fail so that we can return a proper
QMP error message instead of just dumping something on stderr. Not sure
if it's possible, though, I don't really know that code.

Nothing to stop this series anyway.

Kevin

Re: [Qemu-devel] [PATCH RFC 0/3] dataplane: dataplane: more graceful error handling

[Cornelia Huck]

On Thu, 7 Aug 2014 16:39:01 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 25.07.2014 um 14:10 hat Cornelia Huck geschrieben:
> > Currently, qemu will take a hard exit if it fails to set up guest or
> > host notifiers, giving no real clue as to what went wrong (e.g., when
> > out of file descriptors).
> > 
> > This patchset tries to make this more manageable: Both by improving the
> > error message and by gracefully falling back to non-dataplane in case of
> > errors.
> > 
> > Patches are also available on
> > 
> > git://github.com/cohuck/qemu dataplane-graceful-fail
> > 
> > Thoughts?
> 
> I think Stefan should comment on this, but I certainly welcome every
> patch that fixes an exit(1) call.
> 
> I'm not entirely sure about the added fprintf(). It feels wrong, but of
> course it's a lot less wrong than exiting. 

Well, I was only enhancing an existing message :) At least the admin
has a chance to find out now what went wrong.

> Ideally already adding the
> device with dataplane enabled would fail so that we can return a proper
> QMP error message instead of just dumping something on stderr. Not sure
> if it's possible, though, I don't really know that code.

The problem is that we won't fail until after we actually try to start
dataplane (i.e. when the guest is trying to use the device). Depending
on the guest, this may be when the guest has already been running for a
time (e.g. when the guest disables using some devices and the guest
admin enables them manually later).

> 
> Nothing to stop this series anyway.
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH RFC 0/3] dataplane: dataplane: more graceful error handling

[Stefan Hajnoczi]

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

On Fri, Jul 25, 2014 at 02:10:45PM +0200, Cornelia Huck wrote:
> Currently, qemu will take a hard exit if it fails to set up guest or
> host notifiers, giving no real clue as to what went wrong (e.g., when
> out of file descriptors).
> 
> This patchset tries to make this more manageable: Both by improving the
> error message and by gracefully falling back to non-dataplane in case of
> errors.
> 
> Patches are also available on
> 
> git://github.com/cohuck/qemu dataplane-graceful-fail
> 
> Thoughts?
> 
> Cornelia Huck (3):
>   dataplane: print why starting failed
>   dataplane: fail notifier setting gracefully
>   dataplane: stop trying on notifier error
> 
>  hw/block/dataplane/virtio-blk.c |   39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)

Sorry for the delay.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]