Re: [PATCH v4 4/4] virtio-blk: add iothread-vq-mapping parameter

[Kevin Wolf <kwolf@redhat.com>]

Am 20.12.2023 um 14:47 hat Stefan Hajnoczi geschrieben:
> Add the iothread-vq-mapping parameter to assign virtqueues to IOThreads.
> Store the vq:AioContext mapping in the new struct
> VirtIOBlockDataPlane->vq_aio_context[] field and refactor the code to
> use the per-vq AioContext instead of the BlockDriverState's AioContext.
> 
> Reimplement --device virtio-blk-pci,iothread= and non-IOThread mode by
> assigning all virtqueues to the IOThread and main loop's AioContext in
> vq_aio_context[], respectively.
> 
> The comment in struct VirtIOBlockDataPlane about EventNotifiers is
> stale. Remove it.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

> @@ -177,19 +238,13 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
>  
>      trace_virtio_blk_data_plane_start(s);
>  
> -    r = blk_set_aio_context(s->conf->conf.blk, s->ctx, &local_err);
> +    r = blk_set_aio_context(s->conf->conf.blk, s->vq_aio_context[0],
> +                            &local_err);
>      if (r < 0) {
>          error_report_err(local_err);
>          goto fail_aio_context;
>      }

This doesn't really have to be an error any more, we'll just submit I/O
from any thread we want no matter what the home AioContext of the
BlockBackend is.

So the only effect the blk_set_aio_context() has is that other users of
the image try to submit their requests from the same iothread as the
first virtqueue in the hope that this performs a bit better (maybe less
lock contention or whatever the idea was?)

> -    /* Kick right away to begin processing requests already in vring */
> -    for (i = 0; i < nvqs; i++) {
> -        VirtQueue *vq = virtio_get_queue(s->vdev, i);
> -
> -        event_notifier_set(virtio_queue_get_host_notifier(vq));
> -    }
> -
>      /*
>       * These fields must be visible to the IOThread when it processes the
>       * virtqueue, otherwise it will think dataplane has not started yet.
> @@ -206,8 +261,12 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
>      if (!blk_in_drain(s->conf->conf.blk)) {
>          for (i = 0; i < nvqs; i++) {
>              VirtQueue *vq = virtio_get_queue(s->vdev, i);
> +            AioContext *ctx = s->vq_aio_context[i];
>  
> -            virtio_queue_aio_attach_host_notifier(vq, s->ctx);
> +            /* Kick right away to begin processing requests already in vring */
> +            event_notifier_set(virtio_queue_get_host_notifier(vq));

The old code did this also for blk_in_drain() == true. Why don't we need
it there any more? Should the 'if' move inside the loop just around
attaching the notifier?

> +            virtio_queue_aio_attach_host_notifier(vq, ctx);
>          }
>      }
>      return 0;

Kevin