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

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

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