From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50459) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1atDF3-0003c8-GG for qemu-devel@nongnu.org; Thu, 21 Apr 2016 07:57:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1atDF2-0006sH-Ej for qemu-devel@nongnu.org; Thu, 21 Apr 2016 07:57:53 -0400 Date: Thu, 21 Apr 2016 19:57:51 +0800 From: Fam Zheng Message-ID: <20160421115751.GA2220@ad.usersys.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> <20160421114207.GD4193@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160421114207.GD4193@noname.redhat.com> 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: Kevin Wolf Cc: qemu-devel@nongnu.org, Paolo Bonzini , "Michael S. Tsirkin" , Stefan Hajnoczi , Max Reitz , qemu-block@nongnu.org, qemu-stable@nongnu.org On Thu, 04/21 13:42, Kevin Wolf wrote: > 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. Completely agreed. I'm adding this to my todo list for 2.7. Fam