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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]

On Wed, Jan 31, 2018 at 03:31:27PM +0100, Kevin Wolf wrote:
> Am 31.01.2018 um 14:56 hat Stefan Hajnoczi geschrieben:
> > On Tue, Jan 30, 2018 at 05:54:56PM +0100, Kevin Wolf wrote:
> > > 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 <stefanha@redhat.com>
> > > 
> > > 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?
> > 
> > bdrv_set_aio_context() contains aio_disable_external() +
> > bdrv_parent_drained_begin() + bdrv_drain(bs).  This should complete all
> > requests, even those sitting in a descriptor ring that hasn't been
> > processed yet.
> 
> Ah, yes. Not very obvious, so I wouldn't mind a comment, but you can
> have my R-b either way then:
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks for the review!  I have sent v2 with a comment for you to review.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]