On 8/3/22 18:28, Stefan Hajnoczi wrote:
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 8bb6e6acfc..a575c3f0cd 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -66,6 +66,21 @@ static int virtio_scsi_set_host_notifier(VirtIOSCSI *s, VirtQueue *vq, int n)
> return 0;
> }
>
> +/* Context: BH in IOThread */
> +static void virtio_scsi_dataplane_start_bh(void *opaque)
> +{
> + VirtIOSCSI *s = opaque;
> + VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> + int i;
> +
> + virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx);
> + virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx);
> +
> + for (i = 0; i < vs->conf.num_queues; i++) {
> + virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx);
> + }
> +}
> +
> /* Context: BH in IOThread */
> static void virtio_scsi_dataplane_stop_bh(void *opaque)
> {
> @@ -136,16 +151,18 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
>
> memory_region_transaction_commit();
>
> - aio_context_acquire(s->ctx);
> - virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx);
> - virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx);
> -
> - for (i = 0; i < vs->conf.num_queues; i++) {
> - virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx);
> - }
> -
> s->dataplane_starting = false;
> s->dataplane_started = true;
> +
> + /*
> + * Attach notifiers from within the IOThread. It's possible to attach
> + * notifiers from our thread directly but this approach has the advantages
> + * that virtio_scsi_dataplane_start_bh() is symmetric with
> + * virtio_scsi_dataplane_stop_bh() and the s->dataplane_started assignment
> + * above doesn't require explicit synchronization.
> + */
> + aio_context_acquire(s->ctx);
> + aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_start_bh, s);
> aio_context_release(s->ctx);
Taking the AioContext lock for code that is running in another thread
seems wrong. But really there is no need to take the lock: I think it
wanted to protect against the handler running before
s->dataplane_starting/started were set, but it's not needed now because
the iothread is busy running the bottom half.
The lock must be taken solely because aio_wait_bh_oneshot() requires that the caller holds it. Emanuele is working on removing the lock from AIO_WAIT_WHILE() but for now it's necessary to hold the lock.
Also, please do the same in virtio-blk as well.
Okay. There are a few other differences between virtio-scsi and virtio-blk dataplane code that I'll investigate. Maybe they can share common start/stop functions.
Stefan