Re: [Qemu-block] [PATCH] vl: pause vcpus before stopping iothreads

[Kevin Wolf]

Am 30.01.2018 um 16:38 hat Stefan Hajnoczi geschrieben:
> 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 <address@hidden>

Does pausing the vcpus actually make sure that the iothread isn't active
any more, or do we still have a small window where the vcpu is already
stopped, but the iothread is still processing requests?

Essentially, I think the bdrv_set_aio_context() in iothread_stop_all()
does either not have any effect, or if it does have an effect, it's
wrong. You can't just force an in-use BDS into a different AioContext
when the user that set the AioContext is still there.

At the very least, do we need a blk_drain_all() before stopping the
iothreads? It would still just be a hack, the proper way seens to be
getting the virtio device out of dataplane mode so that the iothread is
actually unused and doesn't just happen to not process something at the
moment.

Kevin

>  vl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index e517a8d995..011f22ae20 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4765,10 +4765,10 @@ int main(int argc, char **argv, char **envp)
>      os_setup_post();
>  
>      main_loop();
> +
>      replay_disable_events();
> +    pause_all_vcpus();
>      iothread_stop_all();
> -
> -    pause_all_vcpus();
>      bdrv_close_all();
>      res_free();
>  
> -- 
> 2.14.3
>