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.
Also, please do the same in virtio-blk as well. Thanks, Paolo
