* [Qemu-devel] [PATCH v2] vl: pause vcpus before stopping iothreads
@ 2018-02-01 11:07 Stefan Hajnoczi
2018-02-07 15:52 ` Stefan Hajnoczi
0 siblings, 1 reply; 2+ messages in thread
From: Stefan Hajnoczi @ 2018-02-01 11:07 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, qemu-block, Fam Zheng, Stefan Hajnoczi
Commit dce8921b2baaf95974af8176406881872067adfa ("iothread: Stop threads
before main() quits") introduced iothread_stop_all() to avoid the
following virtio-scsi assertion failure:
assert(blk_get_aio_context(d->conf.blk) == s->ctx);
Back then the assertion failed because when bdrv_close_all() made
d->conf.blk NULL, blk_get_aio_context() returned the global AioContext
instead of s->ctx.
The same assertion can still fail today when vcpus submit new I/O
requests after iothread_stop_all() has moved the BDS to the global
AioContext.
This patch hardens the iothread_stop_all() approach by pausing vcpus
before calling iothread_stop_all().
Note that the assertion failure is a race condition. It is not possible
to reproduce it reliably.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
v2:
* Add comment explaining rationale for this ordering and why it's safe.
[Kevin]
vl.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/vl.c b/vl.c
index e517a8d995..b72a9fc6df 100644
--- a/vl.c
+++ b/vl.c
@@ -4766,10 +4766,18 @@ int main(int argc, char **argv, char **envp)
main_loop();
replay_disable_events();
+
+ /* The ordering of the following is delicate. Stop vcpus to prevent new
+ * I/O requests being queued by the guest. Then stop IOThreads (this
+ * includes a drain operation and completes all request processing). At
+ * this point emulated devices are still associated with their IOThreads
+ * (if any) but no longer have any work to do. Only then can we close
+ * block devices safely because we know there is no more I/O coming.
+ */
+ pause_all_vcpus();
iothread_stop_all();
-
- pause_all_vcpus();
bdrv_close_all();
+
res_free();
/* vhost-user must be cleaned up before chardevs. */
--
2.14.3
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH v2] vl: pause vcpus before stopping iothreads
2018-02-01 11:07 [Qemu-devel] [PATCH v2] vl: pause vcpus before stopping iothreads Stefan Hajnoczi
@ 2018-02-07 15:52 ` Stefan Hajnoczi
0 siblings, 0 replies; 2+ messages in thread
From: Stefan Hajnoczi @ 2018-02-07 15:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, qemu-block, Fam Zheng
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
On Thu, Feb 01, 2018 at 11:07:08AM +0000, Stefan Hajnoczi wrote:
> Commit dce8921b2baaf95974af8176406881872067adfa ("iothread: Stop threads
> before main() quits") introduced iothread_stop_all() to avoid the
> following virtio-scsi assertion failure:
>
> assert(blk_get_aio_context(d->conf.blk) == s->ctx);
>
> Back then the assertion failed because when bdrv_close_all() made
> d->conf.blk NULL, blk_get_aio_context() returned the global AioContext
> instead of s->ctx.
>
> The same assertion can still fail today when vcpus submit new I/O
> requests after iothread_stop_all() has moved the BDS to the global
> AioContext.
>
> This patch hardens the iothread_stop_all() approach by pausing vcpus
> before calling iothread_stop_all().
>
> Note that the assertion failure is a race condition. It is not possible
> to reproduce it reliably.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> v2:
> * Add comment explaining rationale for this ordering and why it's safe.
> [Kevin]
>
> vl.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-02-07 15:52 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-01 11:07 [Qemu-devel] [PATCH v2] vl: pause vcpus before stopping iothreads Stefan Hajnoczi
2018-02-07 15:52 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.