Re: [Qemu-devel] [Qemu-stable] [PATCH v4] block: fix QEMU crash with scsi-hd and drive_del

[Michael Roth]

Quoting Kevin Wolf (2018-05-29 15:19:17)
> Am 28.05.2018 um 14:03 hat Greg Kurz geschrieben:
> > Removing a drive with drive_del while it is being used to run an I/O
> > intensive workload can cause QEMU to crash.
> > 
> > An AIO flush can yield at some point:
> > 
> > blk_aio_flush_entry()
> >  blk_co_flush(blk)
> >   bdrv_co_flush(blk->root->bs)
> >    ...
> >     qemu_coroutine_yield()
> > 
> > and let the HMP command to run, free blk->root and give control
> > back to the AIO flush:
> > 
> >     hmp_drive_del()
> >      blk_remove_bs()
> >       bdrv_root_unref_child(blk->root)
> >        child_bs = blk->root->bs
> >        bdrv_detach_child(blk->root)
> >         bdrv_replace_child(blk->root, NULL)
> >          blk->root->bs = NULL
> >         g_free(blk->root) <============== blk->root becomes stale
> >        bdrv_unref(child_bs)
> >         bdrv_delete(child_bs)
> >          bdrv_close()
> >           bdrv_drained_begin()
> >            bdrv_do_drained_begin()
> >             bdrv_drain_recurse()
> >              aio_poll()
> >               ...
> >               qemu_coroutine_switch()
> > 
> > and the AIO flush completion ends up dereferencing blk->root:
> > 
> >   blk_aio_complete()
> >    scsi_aio_complete()
> >     blk_get_aio_context(blk)
> >      bs = blk_bs(blk)
> >  ie, bs = blk->root ? blk->root->bs : NULL
> >             ^^^^^
> >             stale
> > 
> > The problem is that we should avoid making block driver graph
> > changes while we have in-flight requests. Let's drain all I/O
> > for this BB before calling bdrv_root_unref_child().
> > 
> > Signed-off-by: Greg Kurz <address@hidden>
> 
> Hmm... It sounded convincing, but 'make check-tests/test-replication'
> fails now. The good news is that with the drain fixes, for which I sent
> v2 today, it passes, so instead of staging it in my block branch, I'll
> put it at the end of my branch for the drain fixes.
> 
> Might take a bit longer than planned until it's in master, sorry.

I'm getting the below test-replication failure/trace trying to backport
this patch for 2.12.1 (using this tree:
https://github.com/mdroth/qemu/commits/stable-2.12-staging-f45280cbf)

Is this the same issue you saw, and if so, are the drain fixes
appropriate for 2.12.x? Are there other prereqs/follow-ups you're
aware of that would also be needed?

address@hidden:~/w/qemu-build4$ MALLOC_PERTURB_=1 gdb --args 
tests/test-replication
(gdb) run
Starting program: /home/mdroth/dev/kvm/qemu-build4/tests/test-replication 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff50bf700 (LWP 3916)]
[New Thread 0x7ffff48be700 (LWP 3917)]
/replication/primary/read: OK
/replication/primary/write: OK
/replication/primary/start: OK
/replication/primary/stop: OK
/replication/primary/do_checkpoint: OK
/replication/primary/get_error_all: OK
/replication/secondary/read: OK
/replication/secondary/write: OK
/replication/secondary/start: [New Thread 0x7fffea9f5700 (LWP 3918)]
[New Thread 0x7fffea1f4700 (LWP 3919)]
[New Thread 0x7fffe99f3700 (LWP 3920)]
[New Thread 0x7fffe91f2700 (LWP 3921)]
[New Thread 0x7fffe89f1700 (LWP 3922)]
[New Thread 0x7fffdbfff700 (LWP 3923)]
[New Thread 0x7fffdb7fe700 (LWP 3924)]
[New Thread 0x7fffda1ef700 (LWP 3925)]
[New Thread 0x7fffd99ee700 (LWP 3926)]
[New Thread 0x7fffd91ed700 (LWP 3927)]
[New Thread 0x7fffd89ec700 (LWP 3928)]
[New Thread 0x7fffbffff700 (LWP 3929)]
[New Thread 0x7fffbf7fe700 (LWP 3930)]
[New Thread 0x7fffbeffd700 (LWP 3931)]
[New Thread 0x7fffbe7fc700 (LWP 3932)]
[New Thread 0x7fffbdffb700 (LWP 3933)]
[New Thread 0x7fffbd7fa700 (LWP 3934)]
[New Thread 0x7fffbcff9700 (LWP 3935)]
[New Thread 0x7fff9bfff700 (LWP 3936)]
[New Thread 0x7fff9b7fe700 (LWP 3937)]
[New Thread 0x7fff9affd700 (LWP 3938)]
[New Thread 0x7fff9a7fc700 (LWP 3939)]
[New Thread 0x7fff99ffb700 (LWP 3940)]
OK
/replication/secondary/stop: 
Thread 1 "test-replicatio" received signal SIGSEGV, Segmentation fault.
qemu_mutex_unlock_impl (address@hidden, address@hidden 
"/home/mdroth/w/qemu4.git/util/async.c", address@hidden) at 
/home/mdroth/w/qemu4.git/util/qemu-thread-posix.c:94
94          assert(mutex->initialized);
(gdb) bt
#0  qemu_mutex_unlock_impl (address@hidden, address@hidden 
"/home/mdroth/w/qemu4.git/util/async.c", address@hidden)
    at /home/mdroth/w/qemu4.git/util/qemu-thread-posix.c:94
#1  0x00005555556231f5 in aio_context_release (address@hidden) at 
/home/mdroth/w/qemu4.git/util/async.c:507
#2  0x00005555555c6895 in bdrv_drain_recurse (address@hidden) at 
/home/mdroth/w/qemu4.git/block/io.c:197
#3  0x00005555555c6f0f in bdrv_do_drained_begin (bs=0x555555d06250, 
recursive=<optimized out>, parent=0x0) at 
/home/mdroth/w/qemu4.git/block/io.c:290
#4  0x00005555555c6ef0 in bdrv_parent_drained_begin (ignore=0x0, 
bs=0x555555d56fb0) at /home/mdroth/w/qemu4.git/block/io.c:53
#5  bdrv_do_drained_begin (bs=0x555555d56fb0, recursive=<optimized out>, 
parent=0x0) at /home/mdroth/w/qemu4.git/block/io.c:288
#6  0x00005555555c6ef0 in bdrv_parent_drained_begin (ignore=0x0, 
bs=0x555555d37210) at /home/mdroth/w/qemu4.git/block/io.c:53
#7  bdrv_do_drained_begin (bs=0x555555d37210, recursive=<optimized out>, 
parent=0x0) at /home/mdroth/w/qemu4.git/block/io.c:288
#8  0x00005555555c6ef0 in bdrv_parent_drained_begin (ignore=0x0, 
bs=0x555555d18450) at /home/mdroth/w/qemu4.git/block/io.c:53
#9  bdrv_do_drained_begin (bs=0x555555d18450, recursive=<optimized out>, 
parent=0x0) at /home/mdroth/w/qemu4.git/block/io.c:288
#10 0x00005555555ba6d9 in blk_drain (blk=0x555555d03a20) at 
/home/mdroth/w/qemu4.git/block/block-backend.c:1591
#11 0x00005555555bb12a in blk_remove_bs (address@hidden) at 
/home/mdroth/w/qemu4.git/block/block-backend.c:775
#12 0x00005555555bb366 in blk_delete (blk=0x555555d03a20) at 
/home/mdroth/w/qemu4.git/block/block-backend.c:401
#13 blk_unref (blk=0x555555d03a20) at 
/home/mdroth/w/qemu4.git/block/block-backend.c:450
#14 0x0000555555572515 in teardown_secondary () at 
/home/mdroth/w/qemu4.git/tests/test-replication.c:373
#15 0x0000555555572f2a in test_secondary_stop () at 
/home/mdroth/w/qemu4.git/tests/test-replication.c:477
#16 0x00007ffff77134aa in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#17 0x00007ffff77133db in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#18 0x00007ffff77133db in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#19 0x00007ffff7713682 in g_test_run_suite () from 
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#20 0x00007ffff77136a1 in g_test_run () from 
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#21 0x000055555557164f in main (argc=<optimized out>, argv=<optimized out>) at 
/home/mdroth/w/qemu4.git/tests/test-replication.c:580
(gdb)

> 
> Kevin
>