Re: [PATCH 2/2] virtio: Keep notifications disabled during drain

[Hanna Czenczek <hreitz@redhat.com>]

On 25.01.24 19:03, Stefan Hajnoczi wrote:
> On Wed, Jan 24, 2024 at 06:38:30PM +0100, Hanna Czenczek wrote:
>> During drain, we do not care about virtqueue notifications, which is why
>> we remove the handlers on it.  When removing those handlers, whether vq
>> notifications are enabled or not depends on whether we were in polling
>> mode or not; if not, they are enabled (by default); if so, they have
>> been disabled by the io_poll_start callback.
>>
>> Because we do not care about those notifications after removing the
>> handlers, this is fine.  However, we have to explicitly ensure they are
>> enabled when re-attaching the handlers, so we will resume receiving
>> notifications.  We do this in virtio_queue_aio_attach_host_notifier*().
>> If such a function is called while we are in a polling section,
>> attaching the notifiers will then invoke the io_poll_start callback,
>> re-disabling notifications.
>>
>> Because we will always miss virtqueue updates in the drained section, we
>> also need to poll the virtqueue once after attaching the notifiers.
>>
>> Buglink: https://issues.redhat.com/browse/RHEL-3934
>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>> ---
>>   include/block/aio.h |  7 ++++++-
>>   hw/virtio/virtio.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/block/aio.h b/include/block/aio.h
>> index 5d0a114988..8378553eb9 100644
>> --- a/include/block/aio.h
>> +++ b/include/block/aio.h
>> @@ -480,9 +480,14 @@ void aio_set_event_notifier(AioContext *ctx,
>>                               AioPollFn *io_poll,
>>                               EventNotifierHandler *io_poll_ready);
>>   
>> -/* Set polling begin/end callbacks for an event notifier that has already been
>> +/*
>> + * Set polling begin/end callbacks for an event notifier that has already been
>>    * registered with aio_set_event_notifier.  Do nothing if the event notifier is
>>    * not registered.
>> + *
>> + * Note that if the io_poll_end() callback (or the entire notifier) is removed
>> + * during polling, it will not be called, so an io_poll_begin() is not
>> + * necessarily always followed by an io_poll_end().
>>    */
>>   void aio_set_event_notifier_poll(AioContext *ctx,
>>                                    EventNotifier *notifier,
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 7549094154..4166da9e97 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -3556,6 +3556,17 @@ static void virtio_queue_host_notifier_aio_poll_end(EventNotifier *n)
>>   
>>   void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
>>   {
>> +    /*
>> +     * virtio_queue_aio_detach_host_notifier() can leave notifications disabled.
>> +     * Re-enable them.  (And if detach has not been used before, notifications
>> +     * being enabled is still the default state while a notifier is attached;
>> +     * see virtio_queue_host_notifier_aio_poll_end(), which will always leave
>> +     * notifications enabled once the polling section is left.)
>> +     */
>> +    if (!virtio_queue_get_notification(vq)) {
>> +        virtio_queue_set_notification(vq, 1);
>> +    }
>> +
>>       aio_set_event_notifier(ctx, &vq->host_notifier,
>>                              virtio_queue_host_notifier_read,
>>                              virtio_queue_host_notifier_aio_poll,
>> @@ -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!

> Otherwise:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks!

Hanna