From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58672) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biC0e-00053z-Bn for qemu-devel@nongnu.org; Thu, 08 Sep 2016 22:57:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1biC0a-0000Sr-7F for qemu-devel@nongnu.org; Thu, 08 Sep 2016 22:57:43 -0400 Date: Fri, 9 Sep 2016 10:57:36 +0800 From: Fam Zheng Message-ID: <20160909025736.GC3360@lemon> References: <1473326931-9699-1-git-send-email-famz@redhat.com> <6706f8e0-8142-b1cf-4b89-f8df3e85c5be@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6706f8e0-8142-b1cf-4b89-f8df3e85c5be@redhat.com> Subject: Re: [Qemu-devel] [PATCH] iothread: Stop threads before main() quits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, qemu-stable@nongnu.org, stefanha@redhat.com On Thu, 09/08 11:34, Paolo Bonzini wrote: > > > On 08/09/2016 11:28, Fam Zheng wrote: > > Right after main_loop ends, we release various things but keep iothread > > alive. The latter is not prepared to the sudden change of resources. > > > > Specifically, after bdrv_close_all(), virtio-scsi dataplane get a > > surprise at the empty BlockBackend: > > > > (gdb) bt > > at /usr/src/debug/qemu-2.6.0/hw/scsi/virtio-scsi.c:543 > > at /usr/src/debug/qemu-2.6.0/hw/scsi/virtio-scsi.c:577 > > > > It is because the d->conf.blk->root is set to NULL, then > > blk_get_aio_context() returns qemu_aio_context, whereas s->ctx is still > > pointing to the iothread: > > > > hw/scsi/virtio-scsi.c:543: > > > > if (s->dataplane_started) { > > assert(blk_get_aio_context(d->conf.blk) == s->ctx); > > } > > > > To fix this, let's stop iothreads before doing bdrv_close_all(). > > > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Fam Zheng > > --- > > include/sysemu/iothread.h | 1 + > > iothread.c | 24 ++++++++++++++++++++---- > > vl.c | 2 ++ > > 3 files changed, 23 insertions(+), 4 deletions(-) > > > > diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h > > index 2eefea1..68ac2de 100644 > > --- a/include/sysemu/iothread.h > > +++ b/include/sysemu/iothread.h > > @@ -35,5 +35,6 @@ typedef struct { > > > > char *iothread_get_id(IOThread *iothread); > > AioContext *iothread_get_aio_context(IOThread *iothread); > > +void iothread_stop_all(void); > > > > #endif /* IOTHREAD_H */ > > diff --git a/iothread.c b/iothread.c > > index f183d38..fb08a60 100644 > > --- a/iothread.c > > +++ b/iothread.c > > @@ -54,16 +54,25 @@ static void *iothread_run(void *opaque) > > return NULL; > > } > > > > -static void iothread_instance_finalize(Object *obj) > > +static int iothread_stop(Object *object, void *opaque) > > { > > - IOThread *iothread = IOTHREAD(obj); > > + IOThread *iothread; > > > > - if (!iothread->ctx) { > > - return; > > + iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD); > > + if (!iothread || !iothread->ctx) { > > + return 0; > > } > > iothread->stopping = true; > > aio_notify(iothread->ctx); > > qemu_thread_join(&iothread->thread); > > + return 0; > > +} > > + > > +static void iothread_instance_finalize(Object *obj) > > +{ > > + IOThread *iothread = IOTHREAD(obj); > > + > > + iothread_stop(obj, NULL); > > qemu_cond_destroy(&iothread->init_done_cond); > > qemu_mutex_destroy(&iothread->init_done_lock); > > aio_context_unref(iothread->ctx); > > @@ -174,3 +183,10 @@ IOThreadInfoList *qmp_query_iothreads(Error **errp) > > object_child_foreach(container, query_one_iothread, &prev); > > return head; > > } > > + > > +void iothread_stop_all(void) > > +{ > > + Object *container = object_get_objects_root(); > > + > > + object_child_foreach(container, iothread_stop, NULL); > > +} > > diff --git a/vl.c b/vl.c > > index ee557a1..6a218ce 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -121,6 +121,7 @@ int main(int argc, char **argv) > > #include "crypto/init.h" > > #include "sysemu/replay.h" > > #include "qapi/qmp/qerror.h" > > +#include "sysemu/iothread.h" > > > > #define MAX_VIRTIO_CONSOLES 1 > > #define MAX_SCLP_CONSOLES 1 > > @@ -4616,6 +4617,7 @@ int main(int argc, char **argv, char **envp) > > trace_init_vcpu_events(); > > main_loop(); > > replay_disable_events(); > > + iothread_stop_all(); > > > > bdrv_close_all(); > > pause_all_vcpus(); > > > > Reviewed-by: Paolo Bonzini Thanks! Is there a coming PULL from you that this patch could sneak in? :) Fam