Re: [PATCH 4/6] test-bdrv-drain.c: adapt test to the coming subtree drains

[Stefan Hajnoczi]

On Tue, Feb 08, 2022 at 10:36:53AM -0500, Emanuele Giuseppe Esposito wrote:
> There will be 2 problems in this test when we will add
> subtree drains in bdrv_replace_child_noperm:
> 
> - First, the test is inconsistent about taking the AioContext lock when
>   calling bdrv_replace_child_noperm.  bdrv_replace_child_noperm is reached
>   in two places: from blk_insert_bs directly, and via block_job_create.
>   Only the second does it with the AioContext lock taken, and there seems
>   to be no reason why the lock is needed.
>   Move aio_context_acquire further down, to just protect block_job_add_bdrv()
> 
> - Second, test_detach_indirect is only interested in observing the first
>   call to .drained_begin. In the original test, there was only a single
>   subtree drain; however, with additional drains introduced in
>   bdrv_replace_child_noperm(), the test callback would be called too early
>   and/or multiple times.
>   Override the callback only when we actually want to use it, and put back
>   the original after it's been invoked.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  tests/unit/test-bdrv-drain.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
> index 4924ceb562..c52ba2db4e 100644
> --- a/tests/unit/test-bdrv-drain.c
> +++ b/tests/unit/test-bdrv-drain.c
> @@ -912,12 +912,12 @@ static void test_blockjob_common_drain_node(enum 
> drain_type drain_type,
>      blk_insert_bs(blk_target, target, &error_abort);
>      blk_set_allow_aio_context_change(blk_target, true);
>  
> -    aio_context_acquire(ctx);
>      tjob = block_job_create("job0", &test_job_driver, NULL, src,
>                              0, BLK_PERM_ALL,
>                              0, 0, NULL, NULL, &error_abort);
>      tjob->bs = src;
>      job = &tjob->common;
> +    aio_context_acquire(ctx);

block_job_create() uses src's AioContext. In the IOThread case the
AioContext is not qemu_aio_context. My expectation is that src's
AioContext must be acquired before block_job_create() starts using src.

blockdev.c QMP commands acquire the BDS's AioContext before calling the
function that creates the job. It seems strange to do it differently in
this test case.

You mentioned that blk_insert_bs() is called without acquiring an
AioContext. That may be because we know blk_target is in
qemu_aio_context and we assume our thread holds it (even if we don't
explicitly hold it). If you want to fix an inconsistency then maybe fix
that instead of removing the acquire around block_job_create()?

Stefan

signature.asc
Description: PGP signature