| Message ID | 20220118162738.1366281-6-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Removal of Aiocontext lock through drains: protect bdrv_replace_child_noperm. | expand |
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?] 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. Paolo
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
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 5a35dc391d..f6af206748 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_add_bdrv(job, "target", target, 0, BLK_PERM_ALL, &error_abort); switch (result) { @@ -1322,15 +1322,18 @@ static void detach_by_parent_aio_cb(void *opaque, int ret) } } +static BdrvChildClass detach_by_driver_cb_class; + static void detach_by_driver_cb_drained_begin(BdrvChild *child) { + /* restore .drained_begin cb, we don't need it anymore. */ + detach_by_driver_cb_class.drained_begin = child_of_bds.drained_begin; + aio_bh_schedule_oneshot(qemu_get_current_aio_context(), detach_indirect_bh, &detach_by_parent_data); child_of_bds.drained_begin(child); } -static BdrvChildClass detach_by_driver_cb_class; - /* * Initial graph: * @@ -1362,8 +1365,6 @@ static void test_detach_indirect(bool by_parent_cb) if (!by_parent_cb) { detach_by_driver_cb_class = child_of_bds; - detach_by_driver_cb_class.drained_begin = - detach_by_driver_cb_drained_begin; } /* Create all involved nodes */ @@ -1421,6 +1422,12 @@ static void test_detach_indirect(bool by_parent_cb) acb = blk_aio_preadv(blk, 0, &qiov, 0, detach_by_parent_aio_cb, NULL); g_assert(acb != NULL); + if (!by_parent_cb) { + /* set .drained_begin cb to run only in the following drain. */ + detach_by_driver_cb_class.drained_begin = + detach_by_driver_cb_drained_begin; + } + /* Drain and check the expected result */ bdrv_subtree_drained_begin(parent_b);
There will be 2 problems in this test when we will add subtree drains in bdrv_replace_child_noperm: - 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. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- tests/unit/test-bdrv-drain.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)