Re: [PULL 1/1] test-bdrv-drain: fix iothread_join() hang

[Stefan Hajnoczi]

On Mon, Oct 14, 2019 at 01:11:41PM +0200, Paolo Bonzini wrote:
> On 14/10/19 10:52, Stefan Hajnoczi wrote:
> > tests/test-bdrv-drain can hang in tests/iothread.c:iothread_run():
> > 
> >   while (!atomic_read(&iothread->stopping)) {
> >       aio_poll(iothread->ctx, true);
> >   }
> > 
> > The iothread_join() function works as follows:
> > 
> >   void iothread_join(IOThread *iothread)
> >   {
> >       iothread->stopping = true;
> >       aio_notify(iothread->ctx);
> >       qemu_thread_join(&iothread->thread);
> > 
> > If iothread_run() checks iothread->stopping before the iothread_join()
> > thread sets stopping to true, then aio_notify() may be optimized away
> > and iothread_run() hangs forever in aio_poll().
> > 
> > The correct way to change iothread->stopping is from a BH that executes
> > within iothread_run().  This ensures that iothread->stopping is checked
> > after we set it to true.
> > 
> > This was already fixed for ./iothread.c (note this is a different source
> > file!) by commit 2362a28ea11c145e1a13ae79342d76dc118a72a6 ("iothread:
> > fix iothread_stop() race condition"), but not for tests/iothread.c.
> 
> Aha, I did have some kind of dejavu when sending the patch I have just
> sent; let's see if this also fixes the test-aio-multithread assertion
> failure.
> 
> Note that with this change the atomic read of iothread->stopping can go
> away; I can send a separate patch later.

Yes, I thought about the atomic_read() later as well.

Stefan

signature.asc
Description: PGP signature