[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 05/12] test-bdrv-drain.c: adapt test to the coming subtree dr
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH 05/12] test-bdrv-drain.c: adapt test to the coming subtree drains |
Date: |
Thu, 3 Feb 2022 12:41:12 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
On 19/01/2022 10:18, Paolo Bonzini wrote:
> On 1/18/22 17:27, Emanuele Giuseppe Esposito wrote:
>> - First of all, inconsistency between block_job_create under
>> aiocontext lock that internally calls blk_insert_bs and therefore
>> bdrv_replace_child_noperm, and blk_insert_bs that is called two lines
>> above in the same test without aiocontext. There seems to be no
>> reason on why we need the lock there, so move the aiocontext lock further
>> down.
>>
>> - test_detach_indirect: here it is simply a matter of wrong callbacks
>> used. In the original test, there was only a subtree drain, so
>> overriding .drained_begin was not a problem. Now that we want to have
>> additional subtree drains, we risk to call the test callback
>> to early, or multiple times. We do not want that, so override
>> the callback only when we actually want to use it.
>
> The language here is a bit overcomplicated. Don't think that you're
> writing Italian, instead use simple sentences.
>
> 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. [Any reason why you don't move it even further down, to
> cover only job_start?]
The lock is left just to cover block_job_add_bdrv, since it internally
releases and then acquires the lock.
Fixing that is a future TODO.
job_start did not and does not need the AioContext lock :)
>
> 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.
>
> This could also be split in two commits.
>
Will update the commit message, thank you!
Emanuele
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH 05/12] test-bdrv-drain.c: adapt test to the coming subtree drains,
Emanuele Giuseppe Esposito <=