Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 02/14/2016 06:17 PM, Paolo Bonzini wrote:
> In disabled mode, virtio-blk dataplane seems to be enabled, but flow
> actually goes through the normal virtio path.  This patch simplifies a bit
> the handling of disabled mode.  In disabled mode, virtio_blk_handle_output
> might be called even if s->dataplane is not NULL.
> 
> This is a bit tricky, because the current check for s->dataplane will
> always trigger, causing a continuous stream of calls to
> virtio_blk_data_plane_start.  Unfortunately, these calls will not
> do anything.  To fix this, set the "started" flag even in disabled
> mode, and skip virtio_blk_data_plane_start if the started flag is true.
> The resulting changes also prepare the code for the next patch, were
> virtio-blk dataplane will reuse the same virtio_blk_handle_output function
> as "regular" virtio-blk.
> 
> Because struct VirtIOBlockDataPlane is opaque in virtio-blk.c, we have
> to move s->dataplane->started inside struct VirtIOBlock.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I have some random crashes at startup 
                
                Stack trace of thread 48326:
                #0  0x000002aa2e0cce46 bdrv_co_do_rw (qemu-system-s390x)
                #1  0x000002aa2e159e8e coroutine_trampoline (qemu-system-s390x)
                #2  0x000003ffbc35150a __makecontext_ret (libc.so.6)


that I was able to bisect.
commit 2906cddfecff21af20eedab43288b485a679f9ac does crash regularly, 
2906cddfecff21af20eedab43288b485a679f9ac^ does not.

I will try to find somebody that looks into that - unless you have an idea.

Christian

> ---

>  hw/block/dataplane/virtio-blk.c | 21 +++++++++------------
>  hw/block/virtio-blk.c           |  2 +-
>  include/hw/virtio/virtio-blk.h  |  1 +
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 03b81bc..cc521c1 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -28,7 +28,6 @@
>  #include "qom/object_interfaces.h"
> 
>  struct VirtIOBlockDataPlane {
> -    bool started;
>      bool starting;
>      bool stopping;
>      bool disabled;
> @@ -264,11 +263,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      VirtQueue *vq;
>      int r;
> 
> -    if (s->started || s->disabled) {
> -        return;
> -    }
> -
> -    if (s->starting) {
> +    if (vblk->dataplane_started || s->starting) {
>          return;
>      }
> 
> @@ -300,7 +295,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      vblk->complete_request = complete_request_vring;
> 
>      s->starting = false;
> -    s->started = true;
> +    vblk->dataplane_started = true;
>      trace_virtio_blk_data_plane_start(s);
> 
>      blk_set_aio_context(s->conf->conf.blk, s->ctx);
> @@ -319,9 +314,10 @@ 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->disabled = true;
>      s->starting = false;
> +    vblk->dataplane_started = true;
>  }
> 
>  /* Context: QEMU global mutex held */
> @@ -331,13 +327,14 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> 
> +    if (!vblk->dataplane_started || s->stopping) {
> +        return;
> +    }
> 
>      /* Better luck next time. */
>      if (s->disabled) {
>          s->disabled = false;
> -        return;
> -    }
> -    if (!s->started || s->stopping) {
> +        vblk->dataplane_started = false;
>          return;
>      }
>      s->stopping = true;
> @@ -364,6 +361,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>      /* Clean up guest notifier (irq) */
>      k->set_guest_notifiers(qbus->parent, 1, false);
> 
> -    s->started = false;
> +    vblk->dataplane_started = false;
>      s->stopping = false;
>  }
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index c427698..e04c8f5 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -589,7 +589,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>      /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
>       * dataplane here instead of waiting for .set_status().
>       */
> -    if (s->dataplane) {
> +    if (s->dataplane && !s->dataplane_started) {
>          virtio_blk_data_plane_start(s->dataplane);
>          return;
>      }
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 199bb0e..781969d 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -56,6 +56,7 @@ typedef struct VirtIOBlock {
>      /* Function to push to vq and notify guest */
>      void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
>      Notifier migration_state_notifier;
> +    bool dataplane_started;
>      struct VirtIOBlockDataPlane *dataplane;
>  } VirtIOBlock;
>