On Thu, Jan 25, 2024 at 07:32:12PM +0100, Hanna Czenczek wrote: > On 25.01.24 19:18, Hanna Czenczek wrote: > > On 25.01.24 19:03, Stefan Hajnoczi wrote: > > > On Wed, Jan 24, 2024 at 06:38:30PM +0100, Hanna Czenczek wrote: > > [...] > > > > > @@ -3563,6 +3574,13 @@ void > > > > virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext > > > > *ctx) > > > > aio_set_event_notifier_poll(ctx, &vq->host_notifier, > > > > virtio_queue_host_notifier_aio_poll_begin, > > > > virtio_queue_host_notifier_aio_poll_end); > > > > + > > > > + /* > > > > + * We will have ignored notifications about new requests > > > > from the guest > > > > + * during the drain, so "kick" the virt queue to process > > > > those requests > > > > + * now. > > > > + */ > > > > + virtio_queue_notify(vq->vdev, vq->queue_index); > > > event_notifier_set(&vq->host_notifier) is easier to understand because > > > it doesn't contain a non-host_notifier code path that we must not take. > > > > > > Is there a reason why you used virtio_queue_notify() instead? > > > > Not a good one anyway! > > > > virtio_queue_notify() is just what seemed obvious to me (i.e. to notify > > the virtqueue). Before removal of the AioContext lock, calling > > handle_output seemed safe. But, yes, there was the discussion on the > > RFC that it really isn’t. I didn’t consider that means we must rely on > > virtio_queue_notify() calling event_notifier_set(), so we may as well > > call it explicitly here. > > > > I’ll fix it, thanks for pointing it out! > > (I think together with this change, I’ll also remove the > event_notifier_set() call from virtio_blk_data_plane_start(). It’d > obviously be a duplicate, and removing it shows why > virtio_queue_aio_attach_host_notifier() should always kick the queue.)
Yes, it can be removed from start(). Stefan
signature.asc
Description: PGP signature