From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47723) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1atCzw-0001Tz-D7 for qemu-devel@nongnu.org; Thu, 21 Apr 2016 07:42:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1atCzv-0002fe-Bt for qemu-devel@nongnu.org; Thu, 21 Apr 2016 07:42:16 -0400 Date: Thu, 21 Apr 2016 13:42:07 +0200 From: Kevin Wolf Message-ID: <20160421114207.GD4193@noname.redhat.com> References: <1461204268-7643-1-git-send-email-famz@redhat.com> <20160421050513.GH24822@dhcp-14-234.nay.redhat.com> <20160421080707.GB4193@noname.redhat.com> <20160421085719.GA4197@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160421085719.GA4197@localhost.localdomain> Subject: Re: [Qemu-devel] [PATCH for-2.6] virtio: Register host notifier handler as external List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: qemu-devel@nongnu.org, Paolo Bonzini , "Michael S. Tsirkin" , Stefan Hajnoczi , Max Reitz , qemu-block@nongnu.org, qemu-stable@nongnu.org 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 > > > > --- > > > > 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