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

[Hanna Czenczek]

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.)