On 2018-05-28 14:03, Greg Kurz wrote: > 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 <gr...@kaod.org> > --- > v4: - call blk_drain() in blk_remove_bs() (Kevin) > > v3: - start drained section before modifying the graph (Stefan) > > v2: - drain I/O requests when detaching the BDS (Stefan, Paolo) > --- > block/block-backend.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 89f47b00ea24..bee1f0e41461 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -768,6 +768,11 @@ void blk_remove_bs(BlockBackend *blk) > > blk_update_root_state(blk); > > + /* bdrv_root_unref_child() will cause blk->root to become stale and may > + * switch to a completion coroutine later on. Let's drain all I/O here > + * to avoid that and a potential QEMU crash. > + */ > + blk_drain(blk); > bdrv_root_unref_child(blk->root); > blk->root = NULL; > }
For some reason, this patch breaks iotest 083 (with -nbd) on tmpfs for me. Only on tmpfs, though, so it's probably not going to be just a simple reference output fix. Max
signature.asc
Description: OpenPGP digital signature
