[PATCH 0/3] block: Fix block_resize deadlock with iothreads

[PATCH 0/3] block: Fix block_resize deadlock with iothreads

[Kevin Wolf]

The conversion of qmp_block_resize() to coroutines exposed a preexisting
locking bug in the drain implementation that can cause deadlocks.

As it happens, fixing this bug reveals in turn that the locking in
qmp_block_resize() itself is incomplete, too.

Kevin Wolf (3):
  block: Simplify qmp_block_resize() error paths
  block: Fix locking in qmp_block_resize()
  block: Fix deadlock in bdrv_co_yield_to_drain()

 block/io.c | 41 ++++++++++++++++++++++++-----------------
 blockdev.c | 12 +++++++-----
 2 files changed, 31 insertions(+), 22 deletions(-)

-- 
2.28.0

[PATCH 1/3] block: Simplify qmp_block_resize() error paths

[Kevin Wolf]

The only thing that happens after the 'out:' label is blk_unref(blk).
However, blk = NULL in all of the error cases, so instead of jumping to
'out:', we can just return directly.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index fe6fb5dc1d..229d2cce1b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2468,17 +2468,17 @@ void coroutine_fn qmp_block_resize(bool has_device, const char *device,
 
     if (size < 0) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
-        goto out;
+        return;
     }
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
         error_setg(errp, QERR_DEVICE_IN_USE, device);
-        goto out;
+        return;
     }
 
     blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
     if (!blk) {
-        goto out;
+        return;
     }
 
     bdrv_drained_begin(bs);
@@ -2487,7 +2487,6 @@ void coroutine_fn qmp_block_resize(bool has_device, const char *device,
     bdrv_co_leave(bs, old_ctx);
     bdrv_drained_end(bs);
 
-out:
     bdrv_co_lock(bs);
     blk_unref(blk);
     bdrv_co_unlock(bs);
-- 
2.28.0

Re: [PATCH 1/3] block: Simplify qmp_block_resize() error paths

[Vladimir Sementsov-Ogievskiy]

03.12.2020 20:23, Kevin Wolf wrote:
> The only thing that happens after the 'out:' label is blk_unref(blk).
> However, blk = NULL in all of the error cases, so instead of jumping to
> 'out:', we can just return directly.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   blockdev.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index fe6fb5dc1d..229d2cce1b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2468,17 +2468,17 @@ void coroutine_fn qmp_block_resize(bool has_device, const char *device,
>   
>       if (size < 0) {
>           error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
> -        goto out;
> +        return;
>       }
>   
>       if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
>           error_setg(errp, QERR_DEVICE_IN_USE, device);
> -        goto out;
> +        return;
>       }
>   
>       blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
>       if (!blk) {
> -        goto out;
> +        return;
>       }
>   
>       bdrv_drained_begin(bs);
> @@ -2487,7 +2487,6 @@ void coroutine_fn qmp_block_resize(bool has_device, const char *device,
>       bdrv_co_leave(bs, old_ctx);
>       bdrv_drained_end(bs);
>   
> -out:
>       bdrv_co_lock(bs);
>       blk_unref(blk);
>       bdrv_co_unlock(bs);
> 

Initialization of blk to NULL becomes redundant with this patch, so may be dropped too. Anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 1/3] block: Simplify qmp_block_resize() error paths

[Kevin Wolf]

Am 08.12.2020 um 15:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 03.12.2020 20:23, Kevin Wolf wrote:
> > The only thing that happens after the 'out:' label is blk_unref(blk).
> > However, blk = NULL in all of the error cases, so instead of jumping to
> > 'out:', we can just return directly.
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   blockdev.c | 7 +++----
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index fe6fb5dc1d..229d2cce1b 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2468,17 +2468,17 @@ void coroutine_fn qmp_block_resize(bool has_device, const char *device,
> >       if (size < 0) {
> >           error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
> > -        goto out;
> > +        return;
> >       }
> >       if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
> >           error_setg(errp, QERR_DEVICE_IN_USE, device);
> > -        goto out;
> > +        return;
> >       }
> >       blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
> >       if (!blk) {
> > -        goto out;
> > +        return;
> >       }
> >       bdrv_drained_begin(bs);
> > @@ -2487,7 +2487,6 @@ void coroutine_fn qmp_block_resize(bool has_device, const char *device,
> >       bdrv_co_leave(bs, old_ctx);
> >       bdrv_drained_end(bs);
> > -out:
> >       bdrv_co_lock(bs);
> >       blk_unref(blk);
> >       bdrv_co_unlock(bs);
> > 
> 
> Initialization of blk to NULL becomes redundant with this patch, so
> may be dropped too.

Good catch, I'll change this while applying.

Kevin

[PATCH 2/3] block: Fix locking in qmp_block_resize()

[Kevin Wolf]

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);
 }
-- 
2.28.0

Re: [PATCH 2/3] block: Fix locking in qmp_block_resize()

[Vladimir Sementsov-Ogievskiy]

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>

-- 
Best regards,
Vladimir

Re: [PATCH 2/3] block: Fix locking in qmp_block_resize()

[Kevin Wolf]

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

[PATCH 3/3] block: Fix deadlock in bdrv_co_yield_to_drain()

[Kevin Wolf]

If bdrv_co_yield_to_drain() is called for draining a block node that
runs in a different AioContext, it keeps that AioContext locked while it
yields and schedules a BH in the AioContext to do the actual drain.

As long as executing the BH is the very next thing the event loop of the
node's AioContext, this actually happens to work, but when it tries to
execute something else that wants to take the AioContext lock, it will
deadlock. (In the bug report, this other thing is a virtio-scsi device
running virtio_scsi_data_plane_handle_cmd().)

Instead, always drop the AioContext lock across the yield and reacquire
it only when the coroutine is reentered. The BH needs to unconditionally
take the lock for itself now.

This fixes the 'block_resize' QMP command on a block node that runs in
an iothread.

Cc: qemu-stable@nongnu.org
Fixes: eb94b81a94bce112e6b206df846c1551aaf6cab6
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1903511
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/block/io.c b/block/io.c
index ec5e152bb7..a9f56a9ab1 100644
--- a/block/io.c
+++ b/block/io.c
@@ -306,17 +306,7 @@ static void bdrv_co_drain_bh_cb(void *opaque)
 
     if (bs) {
         AioContext *ctx = bdrv_get_aio_context(bs);
-        AioContext *co_ctx = qemu_coroutine_get_aio_context(co);
-
-        /*
-         * When the coroutine yielded, the lock for its home context was
-         * released, so we need to re-acquire it here. If it explicitly
-         * acquired a different context, the lock is still held and we don't
-         * want to lock it a second time (or AIO_WAIT_WHILE() would hang).
-         */
-        if (ctx == co_ctx) {
-            aio_context_acquire(ctx);
-        }
+        aio_context_acquire(ctx);
         bdrv_dec_in_flight(bs);
         if (data->begin) {
             assert(!data->drained_end_counter);
@@ -328,9 +318,7 @@ static void bdrv_co_drain_bh_cb(void *opaque)
                                 data->ignore_bds_parents,
                                 data->drained_end_counter);
         }
-        if (ctx == co_ctx) {
-            aio_context_release(ctx);
-        }
+        aio_context_release(ctx);
     } else {
         assert(data->begin);
         bdrv_drain_all_begin();
@@ -348,13 +336,16 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
                                                 int *drained_end_counter)
 {
     BdrvCoDrainData data;
+    Coroutine *self = qemu_coroutine_self();
+    AioContext *ctx = bdrv_get_aio_context(bs);
+    AioContext *co_ctx = qemu_coroutine_get_aio_context(self);
 
     /* Calling bdrv_drain() from a BH ensures the current coroutine yields and
      * other coroutines run if they were queued by aio_co_enter(). */
 
     assert(qemu_in_coroutine());
     data = (BdrvCoDrainData) {
-        .co = qemu_coroutine_self(),
+        .co = self,
         .bs = bs,
         .done = false,
         .begin = begin,
@@ -368,13 +359,29 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
     if (bs) {
         bdrv_inc_in_flight(bs);
     }
-    replay_bh_schedule_oneshot_event(bdrv_get_aio_context(bs),
-                                     bdrv_co_drain_bh_cb, &data);
+
+    /*
+     * Temporarily drop the lock across yield or we would get deadlocks.
+     * bdrv_co_drain_bh_cb() reaquires the lock as needed.
+     *
+     * When we yield below, the lock for the current context will be
+     * released, so if this is actually the lock that protects bs, don't drop
+     * it a second time.
+     */
+    if (ctx != co_ctx) {
+        aio_context_release(ctx);
+    }
+    replay_bh_schedule_oneshot_event(ctx, bdrv_co_drain_bh_cb, &data);
 
     qemu_coroutine_yield();
     /* If we are resumed from some other event (such as an aio completion or a
      * timer callback), it is a bug in the caller that should be fixed. */
     assert(data.done);
+
+    /* Reaquire the AioContext of bs if we dropped it */
+    if (ctx != co_ctx) {
+        aio_context_acquire(ctx);
+    }
 }
 
 void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
-- 
2.28.0

Re: [PATCH 3/3] block: Fix deadlock in bdrv_co_yield_to_drain()

[Vladimir Sementsov-Ogievskiy]

03.12.2020 20:23, Kevin Wolf wrote:
> If bdrv_co_yield_to_drain() is called for draining a block node that
> runs in a different AioContext, it keeps that AioContext locked while it
> yields and schedules a BH in the AioContext to do the actual drain.
> 
> As long as executing the BH is the very next thing the event loop of the

s/thing the event/thing in the event/

(I've reread several times to understand :)

> node's AioContext, this actually happens to work, but when it tries to
> execute something else that wants to take the AioContext lock, it will
> deadlock. (In the bug report, this other thing is a virtio-scsi device
> running virtio_scsi_data_plane_handle_cmd().)
> 
> Instead, always drop the AioContext lock across the yield and reacquire
> it only when the coroutine is reentered. The BH needs to unconditionally
> take the lock for itself now.
> 
> This fixes the 'block_resize' QMP command on a block node that runs in
> an iothread.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: eb94b81a94bce112e6b206df846c1551aaf6cab6
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1903511
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

I don't feel myself good enough in aio contexts acquiring and switching, to see any side effects. At least I don't see any obvious mistakes, so my weak:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Note, I looked through the callers:

bdrv_do_drained_begin/end should be ok, as their normal usage is to start/end drained section under acquired aio context, so it seems correct to temporary release the context. Still I didn't check all drained sections in the code.

bdrv_drain_all_begin seems OK too (we just wait until everything is drained, not bad to temporary release the lock). Still I don't see any call of it from coroutine context.

-- 
Best regards,
Vladimir

Re: [PATCH 3/3] block: Fix deadlock in bdrv_co_yield_to_drain()

[Kevin Wolf]

Am 08.12.2020 um 16:33 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 03.12.2020 20:23, Kevin Wolf wrote:
> > If bdrv_co_yield_to_drain() is called for draining a block node that
> > runs in a different AioContext, it keeps that AioContext locked while it
> > yields and schedules a BH in the AioContext to do the actual drain.
> > 
> > As long as executing the BH is the very next thing the event loop of the
> 
> s/thing the event/thing in the event/
> 
> (I've reread several times to understand :)

Oops, thanks.

"...the next thing that the event loop _does_" is actually what I had in
mind.

> > node's AioContext, this actually happens to work, but when it tries to
> > execute something else that wants to take the AioContext lock, it will
> > deadlock. (In the bug report, this other thing is a virtio-scsi device
> > running virtio_scsi_data_plane_handle_cmd().)
> > 
> > Instead, always drop the AioContext lock across the yield and reacquire
> > it only when the coroutine is reentered. The BH needs to unconditionally
> > take the lock for itself now.
> > 
> > This fixes the 'block_resize' QMP command on a block node that runs in
> > an iothread.
> > 
> > Cc: qemu-stable@nongnu.org
> > Fixes: eb94b81a94bce112e6b206df846c1551aaf6cab6
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1903511
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> I don't feel myself good enough in aio contexts acquiring and
> switching, to see any side effects. At least I don't see any obvious
> mistakes, so my weak:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> Note, I looked through the callers:
> 
> bdrv_do_drained_begin/end should be ok, as their normal usage is to
> start/end drained section under acquired aio context, so it seems
> correct to temporary release the context. Still I didn't check all
> drained sections in the code.
> 
> bdrv_drain_all_begin seems OK too (we just wait until everything is
> drained, not bad to temporary release the lock). Still I don't see any
> call of it from coroutine context.

The good thing there is that BDRV_POLL_WHILE() drops the lock anyway, so
at least for all callers of bdrv_do_drained_begin() that pass poll=true,
we know that they are fine with releasing the lock temporarily.

There are two callers that pass false: The recursive call inside the
function itself, and bdrv_drain_all_begin(). We know that both will poll
later, so they always release the lock at least once.

For ending the drain section, there is bdrv_drained_end_no_poll(), which
is only called in bdrv_child_cb_drained_end(), i.e. an implementation of
BdrvChildClass.drained_end. This is only called recursively in the
context of a polling drain_end, which already drops the lock.

So I think we don't introduce any cases of dropping the lock where this
wouldn't have happened before.

Kevin