Re: [Qemu-devel] [PATCH 06/13] virtio-blk: always use dataplane path if ioeventfd is active

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:34 +0200
Paolo Bonzini <address@hidden> wrote:

> Override start_ioeventfd and stop_ioeventfd to start/stop the
> whole dataplane logic.  This has some positive side effects:
> 
> - no need anymore for virtio_add_queue_aio (i.e. a revert of
>   commit 0ff841f6d138904d514efa1d885bcaf54583852d)
> 
> - no need anymore to switch from generic ioeventfd handlers to
>   dataplane
> 
> It detects some errors better:
> 
>     $ qemu-system-x86_64 -object iothread,id=io \
>           -drive id=null,file=null-aio://,if=none,format=raw \
>           -device virtio-blk-pci,ioeventfd=off,iothread=io,drive=null
>     qemu-system-x86_64: -device 
> virtio-blk-pci,ioeventfd=off,iothread=io,drive=null:
>     ioeventfd is required for iothread
> 
> while previously it would have started just fine.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>         v1->v2: improvement in error message consistency
>         loosen virtio_blk_set_status assertion [Christian]
> 
>  hw/block/dataplane/virtio-blk.c | 74 
> +++++++++++++++++++++++++----------------
>  hw/block/dataplane/virtio-blk.h |  6 ++--
>  hw/block/virtio-blk.c           | 15 ++++-----
>  3 files changed, 55 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 9b268f4..d96f45c 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -83,28 +83,33 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
> VirtIOBlkConf *conf,
>                                    Error **errp)
>  {
>      VirtIOBlockDataPlane *s;
> -    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));

Unrelated change?

>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> 
>      *dataplane = NULL;
> 
> -    if (!conf->iothread) {
> -        return;
> -    }
> -
>      /* Don't try if transport does not support notifiers. */

I'd move this comment together with the next block, otherwise this is
confusing.

> -    if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
> -        error_setg(errp,
> -                   "device is incompatible with dataplane "
> -                   "(transport does not support notifiers)");
> -        return;
> -    }
> +    if (conf->iothread) {
> +        if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
> +            error_setg(errp,
> +                       "device is incompatible with iothread "
> +                       "(transport does not support notifiers)");
> +            return;
> +        }
> +        if (!virtio_device_ioeventfd_enabled(vdev)) {
> +            error_setg(errp, "ioeventfd is required for iothread");
> +            return;
> +        }
> 
> -    /* If dataplane is (re-)enabled while the guest is running there could be
> -     * block jobs that can conflict.
> -     */
> -    if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
> -        error_prepend(errp, "cannot start dataplane thread: ");
> +        /* If dataplane is (re-)enabled while the guest is running there 
> could
> +         * be block jobs that can conflict.
> +         */
> +        if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, 
> errp)) {
> +            error_prepend(errp, "cannot start virtio-blk dataplane: ");
> +            return;
> +        }
> +    }
> +    if (!virtio_device_ioeventfd_enabled(vdev)) {
>          return;
>      }
> 

(...)

> @@ -124,14 +133,18 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
> VirtIOBlkConf *conf,
>  /* Context: QEMU global mutex held */
>  void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>  {
> +    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> +
>      if (!s) {

You already dereferenced s above...

>          return;
>      }
> 
> -    virtio_blk_data_plane_stop(s);
> +    assert(!vblk->dataplane_started);
>      g_free(s->batch_notify_vqs);
>      qemu_bh_delete(s->bh);
> -    object_unref(OBJECT(s->iothread));
> +    if (s->iothread) {
> +        object_unref(OBJECT(s->iothread));
> +    }
>      g_free(s);
>  }
> 

In general, I think this looks good, but I'll have a look at the end
result as well.