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
