| Message ID | 20201203172311.68232-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix block_resize deadlock with iothreads | expand |
03.12.2020 20:23, Kevin Wolf wrote: > The drain functions assume that we hold the AioContext lock of the > drained block node. Make sure to actually take the lock. > > Cc: qemu-stable@nongnu.org > Fixes: eb94b81a94bce112e6b206df846c1551aaf6cab6 > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > blockdev.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/blockdev.c b/blockdev.c > index 229d2cce1b..0535a8dc9e 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2481,13 +2481,16 @@ void coroutine_fn qmp_block_resize(bool has_device, const char *device, > return; > } > > + bdrv_co_lock(bs); > bdrv_drained_begin(bs); > + bdrv_co_unlock(bs); > + > old_ctx = bdrv_co_enter(bs); > blk_truncate(blk, size, false, PREALLOC_MODE_OFF, 0, errp); > bdrv_co_leave(bs, old_ctx); > - bdrv_drained_end(bs); > > bdrv_co_lock(bs); > + bdrv_drained_end(bs); > blk_unref(blk); > bdrv_co_unlock(bs); > } > Can't we just do old_ctx = bdrv_co_enter(bs); bdrv_drained_begin(bs); blk_truncate(blk, size, false, PREALLOC_MODE_OFF, 0, errp); bdrv_drained_end(bs); blk_unref(blk); bdrv_co_leave(bs, old_ctx); ? This way we have one acquire/release section instead of three in a row.. But then we probably need addition bdrv_ref/bdrv_unref, to not crash with final bdrv_co_leave after blk_unref. Also, preexisting, but it seems not good that coroutine_fn qmp_block_resize is called from non-coroutine hmp_block_resize() anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Am 08.12.2020 um 15:46 hat Vladimir Sementsov-Ogievskiy geschrieben: > 03.12.2020 20:23, Kevin Wolf wrote: > > The drain functions assume that we hold the AioContext lock of the > > drained block node. Make sure to actually take the lock. > > > > Cc: qemu-stable@nongnu.org > > Fixes: eb94b81a94bce112e6b206df846c1551aaf6cab6 > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > blockdev.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/blockdev.c b/blockdev.c > > index 229d2cce1b..0535a8dc9e 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -2481,13 +2481,16 @@ void coroutine_fn qmp_block_resize(bool has_device, const char *device, > > return; > > } > > + bdrv_co_lock(bs); > > bdrv_drained_begin(bs); > > + bdrv_co_unlock(bs); > > + > > old_ctx = bdrv_co_enter(bs); > > blk_truncate(blk, size, false, PREALLOC_MODE_OFF, 0, errp); > > bdrv_co_leave(bs, old_ctx); > > - bdrv_drained_end(bs); > > bdrv_co_lock(bs); > > + bdrv_drained_end(bs); > > blk_unref(blk); > > bdrv_co_unlock(bs); > > } > > > > Can't we just do > > old_ctx = bdrv_co_enter(bs); > > bdrv_drained_begin(bs); > blk_truncate(blk, size, false, PREALLOC_MODE_OFF, 0, errp); > bdrv_drained_end(bs); > blk_unref(blk); > > bdrv_co_leave(bs, old_ctx); > > > ? This way we have one acquire/release section instead of three in a > row.. But then we probably need addition bdrv_ref/bdrv_unref, to not > crash with final bdrv_co_leave after blk_unref. That was my first attempt, but bdrv_co_enter()/leave() increase bs->in_flight, so the drain would deadlock. > Also, preexisting, but it seems not good that coroutine_fn > qmp_block_resize is called from non-coroutine hmp_block_resize() hmp_block_resize() is actually in coroutine context, commit eb94b81a only forgot to add a coroutine_fn marker to it. > anyway: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Thanks! Kevin
diff --git a/blockdev.c b/blockdev.c index 229d2cce1b..0535a8dc9e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2481,13 +2481,16 @@ void coroutine_fn qmp_block_resize(bool has_device, const char *device, return; } + bdrv_co_lock(bs); bdrv_drained_begin(bs); + bdrv_co_unlock(bs); + old_ctx = bdrv_co_enter(bs); blk_truncate(blk, size, false, PREALLOC_MODE_OFF, 0, errp); bdrv_co_leave(bs, old_ctx); - bdrv_drained_end(bs); bdrv_co_lock(bs); + bdrv_drained_end(bs); blk_unref(blk); bdrv_co_unlock(bs); }
The drain functions assume that we hold the AioContext lock of the drained block node. Make sure to actually take the lock. Cc: qemu-stable@nongnu.org Fixes: eb94b81a94bce112e6b206df846c1551aaf6cab6 Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)