On 10/02/2022 15:32, Stefan Hajnoczi wrote:
> 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 <eespo...@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).
This is an assumption done in all unit tests, so it's not worth changing
only this case.
If you want to fix an inconsistency then maybe fix
> that instead of removing the acquire around block_job_create()?
Your explanation above makes sense, I think I will drop this change here.
Thank you,
Emanuele
