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

[Cornelia Huck]

On Sun, 14 Feb 2016 18:17:08 +0100
Paolo Bonzini <address@hidden> 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.

It seems a bit odd to me that ->started is the only state that is not
inside the dataplane struct... this approach saves a function call for
an accessor, though.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  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(-)
> 

> @@ -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;

I originally introduced ->disabled to cover (possibly temporary)
failures to set up notifiers, but I guess this doesn't hurt.

>      s->starting = false;
> +    vblk->dataplane_started = true;
>  }
> 
>  /* Context: QEMU global mutex held */

After spending some time wrapping my head around it again, this looks
sane to me.

Reviewed-by: Cornelia Huck <address@hidden>