Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
block-coroutine-wrapper: Take AioContext lock in no_co_wrappers
All of the functions that currently take a BlockDriverState, BdrvChild
or BlockBackend as their first parameter expect the associated
AioContext to be locked when they are called. In the case of
no_co_wrappers, they are called from bottom halves directly in the main
loop, so no other caller can be expected to take the lock for them. This
can result in assertion failures because a lock that isn't taken is
released in nested event loops.

Looking at the first parameter is already done by co_wrappers to decide
where the coroutine should run, so doing the same in no_co_wrappers is
only consistent. Take the lock in the generated bottom halves to fix the
problem.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20230525124713.401149-2-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
  • Loading branch information
Kevin Wolf committed May 30, 2023
1 parent aa9bbd8 commit dea97c1
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 11 deletions.
7 changes: 6 additions & 1 deletion block/block-backend.c
Expand Up @@ -2394,9 +2394,14 @@ void blk_op_unblock_all(BlockBackend *blk, Error *reason)

AioContext *blk_get_aio_context(BlockBackend *blk)
{
BlockDriverState *bs = blk_bs(blk);
BlockDriverState *bs;
IO_CODE();

if (!blk) {
return qemu_get_aio_context();
}

bs = blk_bs(blk);
if (bs) {
AioContext *ctx = bdrv_get_aio_context(blk_bs(blk));
assert(ctx == blk->ctx);
Expand Down
3 changes: 3 additions & 0 deletions include/block/block-common.h
Expand Up @@ -65,6 +65,9 @@
* scheduling a BH in the bottom half that runs the respective non-coroutine
* function. The coroutine yields after scheduling the BH and is reentered when
* the wrapped function returns.
*
* If the first parameter of the function is a BlockDriverState, BdrvChild or
* BlockBackend pointer, the AioContext lock for it is taken in the wrapper.
*/
#define no_co_wrapper

Expand Down
25 changes: 15 additions & 10 deletions scripts/block-coroutine-wrapper.py
Expand Up @@ -88,16 +88,7 @@ def __init__(self, wrapper_type: str, return_type: str, name: str,
raise ValueError(f"no_co function can't be rdlock: {self.name}")
self.target_name = f'{subsystem}_{subname}'

t = self.args[0].type
if t == 'BlockDriverState *':
ctx = 'bdrv_get_aio_context(bs)'
elif t == 'BdrvChild *':
ctx = 'bdrv_get_aio_context(child->bs)'
elif t == 'BlockBackend *':
ctx = 'blk_get_aio_context(blk)'
else:
ctx = 'qemu_get_aio_context()'
self.ctx = ctx
self.ctx = self.gen_ctx()

self.get_result = 's->ret = '
self.ret = 'return s.ret;'
Expand All @@ -109,6 +100,17 @@ def __init__(self, wrapper_type: str, return_type: str, name: str,
self.co_ret = ''
self.return_field = ''

def gen_ctx(self, prefix: str = '') -> str:
t = self.args[0].type
if t == 'BlockDriverState *':
return f'bdrv_get_aio_context({prefix}bs)'
elif t == 'BdrvChild *':
return f'bdrv_get_aio_context({prefix}child->bs)'
elif t == 'BlockBackend *':
return f'blk_get_aio_context({prefix}blk)'
else:
return 'qemu_get_aio_context()'

def gen_list(self, format: str) -> str:
return ', '.join(format.format_map(arg.__dict__) for arg in self.args)

Expand Down Expand Up @@ -262,8 +264,11 @@ def gen_no_co_wrapper(func: FuncDecl) -> str:
static void {name}_bh(void *opaque)
{{
{struct_name} *s = opaque;
AioContext *ctx = {func.gen_ctx('s->')};
aio_context_acquire(ctx);
{func.get_result}{name}({ func.gen_list('s->{name}') });
aio_context_release(ctx);
aio_co_wake(s->co);
}}
Expand Down

0 comments on commit dea97c1

Please sign in to comment.