Re: [Qemu-devel] [PATCH for-2.6] virtio: Register host notifier handler as external

[Kevin Wolf <kwolf@redhat.com>]

Am 21.04.2016 um 10:57 hat Fam Zheng geschrieben:
> On Thu, 04/21 10:07, Kevin Wolf wrote:
> > Am 21.04.2016 um 07:05 hat Fam Zheng geschrieben:
> > > On Thu, 04/21 10:04, Fam Zheng wrote:
> > > > This ensures the bdrv_drained_begin() in block layer is effective and
> > > > fixes launchpad bug #1570134.
> > > 
> > > Forgot to add to the subject, but this patch is for 2.6.
> > 
> > Also CCing qemu-stable (this affects 2.5) and qemu-block.
> > 
> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index f745c4a..002c2c6 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> > > >                                                 bool set_handler)
> > > >  {
> > > >      if (assign && set_handler) {
> > > > -        event_notifier_set_handler(&vq->host_notifier,
> > > > -                                   virtio_queue_host_notifier_read);
> > > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > > +                               true, virtio_queue_host_notifier_read);
> > > >      } else {
> > > > -        event_notifier_set_handler(&vq->host_notifier, NULL);
> > > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > > +                               true, NULL);
> > > >      }
> > 
> > This feels like fixing one special case of a more general problem. Are
> > we sure that event_notifier_set_handler() should ever result in an
> > internal handler? The default should probably be external, and block
> > drivers need to take care to use functions that have the is_external
> > parameter so they can mark themselves internal.
> > 
> 
> Looks like this is the only event_notifier_set_handler caller that
> should be fixed.  If we see problem in other devices they can be
> addressed case by case, I'm not sure marking event_notifier_set_handler
> as external is a better idea for 2.6. (The other two callers are
> hyperv.c, which seems never set the handler, and ccid-card-emulated.c,
> which device I have no idea about.)

As we don't have a lot of time for 2.6 any more, this local fix is
probably the right thing to do. It fixes what we know is broken and
has a very low risk of breaking other things.

But we should probably review all fd handlers during the 2.7 development
cycle. I didn't see anything obvious that could possibly call into the
block layer, but even unrelated and therefore harmless users should
probably be marked as external to avoid unnecessary traps for future
development.

Kevin