| Message ID | 1461204268-7643-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. Fam > > 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); > } > if (!assign) { > /* Test and clear notifier before after disabling event, > -- > 2.8.0 > >
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. Kevin
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.) Fam
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
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 <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. Completely agreed. I'm adding this to my todo list for 2.7. Fam
On Thu, Apr 21, 2016 at 10:04:28AM +0800, Fam Zheng wrote: > This ensures the bdrv_drained_begin() in block layer is effective and > fixes launchpad bug #1570134. > > 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); > } > if (!assign) { > /* Test and clear notifier before after disabling event, Is this the right thing to do in all cases? E.g. it doesn't look like event_notifier_set_handler previously called aio_set_event_notifier on win32. Was there a reason for this? > -- > 2.8.0
On Thu, 04/21 15:20, Michael S. Tsirkin wrote: > On Thu, Apr 21, 2016 at 10:04:28AM +0800, Fam Zheng wrote: > > This ensures the bdrv_drained_begin() in block layer is effective and > > fixes launchpad bug #1570134. > > > > 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); > > } > > if (!assign) { > > /* Test and clear notifier before after disabling event, > > Is this the right thing to do in all cases? > E.g. it doesn't look like event_notifier_set_handler previously called > aio_set_event_notifier on win32. Was there a reason for this? It's a good catch. I'm not sure whether/how win32 goes with ioeventfd, but it seems all the existing host notifier handling (fd handler) in vhost and data plane mismatch with the event notifier implementation (WaitObject) in win32. This is a subject understood best by Paolo but since he's on PTO I'll see if I can figure out the answer by testing this code with wine. Fam
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. > > 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); > } > if (!assign) { > /* Test and clear notifier before after disabling event, This isn't the right fix, at least not in the intended way. The crash is gone because in this patch the handler is moved from iohandler's AioContext to the main loop AioContext, on which the mirror BH is scheduled, and consequently the mirror BH handler always runs first than the VQ handler. For 2.6 I think what we could do is reverting some changes in 5a7e7a0bad1 for non-dataplane case, i.e. call mirror_exit directly in mirror_run. It's too late today and I'll look into this tomorrow. The fix will probably be left for -rc4. Fam
On 21/04/2016 16:28, Fam Zheng wrote: >> > Is this the right thing to do in all cases? >> > E.g. it doesn't look like event_notifier_set_handler previously called >> > aio_set_event_notifier on win32. Was there a reason for this? > It's a good catch. I'm not sure whether/how win32 goes with ioeventfd Fam's patch is okay. While POSIX has only file descriptors, Windows has socket descriptors and handles. EventNotifier is a handle, so it must use aio_set_event_notifier or qemu_add_wait_object. Fam changed qemu_add_wait_object to the corresponding AioContext-based function. qemu_add_wait_object and qemu_del_wait_object probably should go away, replaced by a new function aio_set_wait_handler. Windows's aio_set_event_notifier then can simply become aio_set_wait_handler(ctx, event_notifier_get_handle(e), is_external, io_notify, e); similar to how POSIX implements it with aio_set_fd_handler(ctx, event_notifier_get_fd(e), is_external, (IOHandler *)io_read, NULL, e); Thanks, Paolo > , but it > seems all the existing host notifier handling (fd handler) in vhost and data > plane mismatch with the event notifier implementation (WaitObject) in win32. > This is a subject understood best by Paolo but since he's on PTO I'll see if I > can figure out the answer by testing this code with wine.
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); } if (!assign) { /* Test and clear notifier before after disabling event,
This ensures the bdrv_drained_begin() in block layer is effective and fixes launchpad bug #1570134. Signed-off-by: Fam Zheng <famz@redhat.com> --- hw/virtio/virtio.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)