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

[Stefan Hajnoczi]

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