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

[Emanuele Giuseppe Esposito]

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 <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).

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