Re: [PATCH 07/21] block-coroutine-wrapper: Allow arbitrary parameter names
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > Don't assume specific parameter names like 'bs' or 'blk' in the > generated code, but use the actual name. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 14/21] block: Mark bdrv_get_cumulative_perm() and callers GRAPH_RDLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > The function reads the parents list, so it needs to hold the graph lock. > > This happens to result in BlockDriver.bdrv_set_perm() to be called with > the graph lock held. For consistency, make it the same for all of the > BlockDriver callbacks for updating permissions and annotate the function > pointers with GRAPH_RDLOCK_PTR. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 15/21] block: Mark bdrv_child_perm() GRAPH_RDLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > This adds GRAPH_RDLOCK annotations to declare that callers of > bdrv_child_perm() need to hold a reader lock for the graph because > some implementations access the children list of a node. > > The callers of bdrv_child_perm() conveniently already hold the lock. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 18/21] block: Take graph rdlock in bdrv_change_aio_context()
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > The function reads the parents list, so it needs to hold the graph lock. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 13/21] block: Mark bdrv_parent_perms_conflict() and callers GRAPH_RDLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > The function reads the parents list, so it needs to hold the graph lock. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 19/21] block: Mark bdrv_root_unref_child() GRAPH_WRLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > Instead of taking the writer lock internally, require callers to already > hold it when calling bdrv_root_unref_child(). These callers will > typically already hold the graph lock once the locking work is > completed, which means that they can't call functions that take it > internally. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 06/21] block-coroutine-wrapper: Add no_co_wrapper_bdrv_wrlock functions
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > Add a new wrapper type for GRAPH_WRLOCK functions that should be called > from coroutine context. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 04/21] block: Take AioContext lock for bdrv_append() more consistently
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > The documentation for bdrv_append() says that the caller must hold the > AioContext lock for bs_top. Change all callers to actually adhere to the > contract. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 21/21] block: Mark bdrv_add/del_child() and caller GRAPH_WRLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > The functions read the parents list in the generic block layer, so we > need to hold the graph lock already there. The BlockDriver > implementations actually modify the graph, so it has to be a writer > lock. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 20/21] block: Mark bdrv_unref_child() GRAPH_WRLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > Instead of taking the writer lock internally, require callers to already > hold it when calling bdrv_unref_child(). These callers will typically > already hold the graph lock once the locking work is completed, which > means that they can't call functions that take it internally. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 08/21] block: Mark bdrv_replace_child_noperm() GRAPH_WRLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > Instead of taking the writer lock internally, require callers to already > hold it when calling bdrv_replace_child_noperm(). These callers will > typically already hold the graph lock once the locking work is > completed, which means that they can't call functions that take it > internally. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 17/21] block: Take graph rdlock in bdrv_drop_intermediate()
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > The function reads the parents list, so it needs to hold the graph lock. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 16/21] block: Mark bdrv_parent_cb_change_media() GRAPH_RDLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > The function reads the parents list, so it needs to hold the graph lock. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 12/21] block: Mark bdrv_attach_child() GRAPH_WRLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > Instead of taking the writer lock internally, require callers to already > hold it when calling bdrv_attach_child_common(). These callers will > typically already hold the graph lock once the locking work is > completed, which means that they can't call functions that take it > internally. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 09/21] block: Mark bdrv_replace_child_tran() GRAPH_WRLOCK
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > Instead of taking the writer lock internally, require callers to already > hold it when calling bdrv_replace_child_tran(). These callers will > typically already hold the graph lock once the locking work is > completed, which means that they can't call functions that take it > internally. > > While a graph lock is held, polling is not allowed. Therefore draining > the necessary nodes can no longer be done in bdrv_remove_child() and > bdrv_replace_node_noperm(), but the callers must already make sure that > they are drained. > > Note that the transaction callbacks still take the lock internally, so > tran_finalize() must be called without the lock held. This is because > bdrv_append() also calls bdrv_attach_child_noperm(), which currently > requires to be called unlocked. Once it changes, the transaction > callbacks can be changed, too. > > Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 01/21] block: Remove unused BlockReopenQueueEntry.perms_checked
Am 17/08/2023 um 14:50 schrieb Kevin Wolf:
> This field has been unused since commit 72373e40fbc ('block:
> bdrv_reopen_multiple: refresh permissions on updated graph').
> Remove it.
>
> Signed-off-by: Kevin Wolf
Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 02/21] preallocate: Factor out preallocate_truncate_to_real_size()
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > It's essentially the same code in preallocate_check_perm() and > preallocate_close(), except that the latter ignores errors. > > Signed-off-by: Kevin Wolf > --- Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH 0/2] target/i386: add support for cpu FLUSH_L1D feature and FB_CLEAR capability
Ping? Am 01/02/2023 um 14:57 schrieb Emanuele Giuseppe Esposito: > QEMU should be able to show the guest the above feature/capability, > otherwise we risk to have false vulnerability reports in the guest like in > /sys/devices/system/cpu/vulnerabilities/mmio_stale_data > because the mitigation is present only if the guest supports > (FLUSH_L1D and MD_CLEAR) or FB_CLEAR. > > Emanuele > > Emanuele Giuseppe Esposito (2): > target/i386: add support for FLUSH_L1D feature > target/i386: add support for FB_CLEAR feature > > target/i386/cpu.h | 3 +++ > target/i386/cpu.c | 4 ++-- > 2 files changed, 5 insertions(+), 2 deletions(-) >
[PATCH v2] tests/unit/test-blockjob: Re-enable complete_in_standby test
Pause the job while draining so that pause_count will be
increased and bdrv_drain_all_end() won't reset it to 0, so the
job will not continue.
With this fix, the test is not flaky anymore.
Additionally remove useless aiocontext lock around bdrv_drain_all_end()
in test_complete_in_standby().
Fixes: b6903cbe3a2 "tests/unit/test-blockjob: Disable
complete_in_standby test"
Suggested-by: Hanna Czenczek
Signed-off-by: Emanuele Giuseppe Esposito
---
tests/unit/test-blockjob.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c
index a130f6fefb..7c03958feb 100644
--- a/tests/unit/test-blockjob.c
+++ b/tests/unit/test-blockjob.c
@@ -488,11 +488,14 @@ static void test_complete_in_standby(void)
bdrv_drain_all_begin();
assert_job_status_is(job, JOB_STATUS_STANDBY);
-/* Lock the IO thread to prevent the job from being run */
-aio_context_acquire(ctx);
+/*
+ * Increase pause_count so that the counter is
+ * unbalanced and job won't resume
+ */
+job_pause(job);
+
/* This will schedule the job to resume it */
bdrv_drain_all_end();
-aio_context_release(ctx);
WITH_JOB_LOCK_GUARD() {
/* But the job cannot run, so it will remain on standby */
@@ -531,13 +534,6 @@ int main(int argc, char **argv)
g_test_add_func("/blockjob/cancel/standby", test_cancel_standby);
g_test_add_func("/blockjob/cancel/pending", test_cancel_pending);
g_test_add_func("/blockjob/cancel/concluded", test_cancel_concluded);
-
-/*
- * This test is flaky and sometimes fails in CI and otherwise:
- * don't run unless user opts in via environment variable.
- */
-if (getenv("QEMU_TEST_FLAKY_TESTS")) {
-g_test_add_func("/blockjob/complete_in_standby",
test_complete_in_standby);
-}
+g_test_add_func("/blockjob/complete_in_standby", test_complete_in_standby);
return g_test_run();
}
--
2.39.1
Re: [PATCH] tests/unit/test-blockjob: Re-enable complete_in_standby test
Am 26/04/2023 um 10:45 schrieb Thomas Huth: > On 26/04/2023 10.16, Emanuele Giuseppe Esposito wrote: >> Pause the job while draining so that pause_count will be >> increased and bdrv_drain_all_end() won't reset it to 0, so the >> job will not continue. >> >> With this fix, the test is not flaky anymore. >> >> Additionally remove useless aiocontext lock around bdrv_drain_all_end() >> in test_complete_in_standby(). >> >> Fixes: b6903cbe3a2 "tests/unit/test-blockjob: Disable >> complete_in_standby test" >> Suggested-by: Hanna Czenczek >> Signed-off-by: Emanuele Giuseppe Esposito >> --- >> tests/unit/test-blockjob.c | 17 +++-- >> 1 file changed, 7 insertions(+), 10 deletions(-) >> >> diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c >> index a130f6fefb..46d720aeee 100644 >> --- a/tests/unit/test-blockjob.c >> +++ b/tests/unit/test-blockjob.c >> @@ -488,11 +488,15 @@ static void test_complete_in_standby(void) >> bdrv_drain_all_begin(); >> assert_job_status_is(job, JOB_STATUS_STANDBY); >> + /* >> + * Increase pause_count so that the counter is >> + * unbalanced and job won't resume >> + */ >> + job_pause(job); >> + >> /* Lock the IO thread to prevent the job from being run */ > > I guess the above comment should now be removed, too? > >> - aio_context_acquire(ctx); >> /* This will schedule the job to resume it */ >> bdrv_drain_all_end(); >> - aio_context_release(ctx); > > Thomas > Makes sense, resending Emanuele
Re: test-blockjob: intermittent CI failures in msys2-64bit job
Am 26/04/2023 um 10:03 schrieb Hanna Czenczek: > On 26.04.23 09:38, Emanuele Giuseppe Esposito wrote: >> >> Am 25/04/2023 um 18:48 schrieb Hanna Czenczek: >>> On 24.04.23 20:32, Vladimir Sementsov-Ogievskiy wrote: >>>> On 24.04.23 16:36, Emanuele Giuseppe Esposito wrote: >>>>> >>>>> Am 21/04/2023 um 12:13 schrieb Vladimir Sementsov-Ogievskiy: >>>>>> On 17.03.23 15:35, Thomas Huth wrote: >>>>>>> On 17/03/2023 11.17, Peter Maydell wrote: >>>>>>>> On Mon, 6 Mar 2023 at 11:16, Peter Maydell >>>>>>>> >>>>>>>> wrote: >>>>>>>>> On Fri, 3 Mar 2023 at 18:36, Peter Maydell >>>>>>>>> wrote: >>>>>>>>>> I've noticed that test-blockjob seems to fail intermittently >>>>>>>>>> on the msys2-64bit job: >>>>>>>>>> >>>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/3872508803 >>>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/3871061024 >>>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/3865312440 >>>>>>>>>> >>>>>>>>>> Sample output: >>>>>>>>>> | 53/89 >>>>>>>>>> ERROR:../tests/unit/test-blockjob.c:499:test_complete_in_standby: >>>>>>>>>> assertion failed: (job->status == JOB_STATUS_STANDBY) ERROR >>>>>>>>>> 53/89 qemu:unit / test-blockjob ERROR 0.08s exit status 3 >>>>>>>>> Here's an intermittent failure from my macos x86 machine: >>>>>>>>> >>>>>>>>> 172/621 qemu:unit / test-blockjob >>>>>>>>> ERROR 0.26s killed by signal 6 SIGABRT >>>>>>>> And an intermittent on the freebsd 13 CI job: >>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/3950667240 >>>>>>>> >>>>>>>>>>> MALLOC_PERTURB_=197 >>>>>>>>>>> G_TEST_BUILDDIR=/tmp/cirrus-ci-build/build/tests/unit >>>>>>>>>>> G_TEST_SRCDIR=/tmp/cirrus-ci-build/tests/unit >>>>>>>>>>> /tmp/cirrus-ci-build/build/tests/unit/test-blockjob --tap -k >>>>>>>> ▶ 178/650 /blockjob/ids >>>>>>>> OK >>>>>>>> 178/650 qemu:unit / test-blockjob >>>>>>>> ERROR 0.31s killed by signal 6 SIGABRT >>>>>>>> ― ✀ >>>>>>>> ― >>>>>>>> stderr: >>>>>>>> Assertion failed: (job->status == JOB_STATUS_STANDBY), function >>>>>>>> test_complete_in_standby, file ../tests/unit/test-blockjob.c, line >>>>>>>> 499. >>>>>>>> >>>>>>>> >>>>>>>> TAP parsing error: Too few tests run (expected 9, got 1) >>>>>>>> (test program exited with status code -6) >>>>>>>> ―― >>>>>>>> >>>>>>>> Anybody in the block team looking at these, or shall we just >>>>>>>> disable this test entirely ? >>>>>>> I ran into this issue today, too: >>>>>>> >>>>>>> https://gitlab.com/thuth/qemu/-/jobs/3954367101 >>>>>>> >>>>>>> ... if nobody is interested in fixing this test, I think we should >>>>>>> disable it... >>>>>>> >>>>>>> Thomas >>>>>>> >>>>>> I'm looking at this now, and seems that it's broken since >>>>>> 6f592e5aca1a27fe1c1f6 "job.c: enable job lock/unlock and remove >>>>>> Aiocontext locks", as it stops critical section by new >>>>>> aio_context_release() call exactly after bdrv_drain_all_and(), so >>>>>> it's >>>>>> not a surprise that job may start at that moment and the following >>>>>> assertion fires. >>>>>> >>>>>> Emanuele could please look at it? >>>>>> >>>>> Well if I underst
[PATCH] tests/unit/test-blockjob: Re-enable complete_in_standby test
Pause the job while draining so that pause_count will be
increased and bdrv_drain_all_end() won't reset it to 0, so the
job will not continue.
With this fix, the test is not flaky anymore.
Additionally remove useless aiocontext lock around bdrv_drain_all_end()
in test_complete_in_standby().
Fixes: b6903cbe3a2 "tests/unit/test-blockjob: Disable
complete_in_standby test"
Suggested-by: Hanna Czenczek
Signed-off-by: Emanuele Giuseppe Esposito
---
tests/unit/test-blockjob.c | 17 +++--
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c
index a130f6fefb..46d720aeee 100644
--- a/tests/unit/test-blockjob.c
+++ b/tests/unit/test-blockjob.c
@@ -488,11 +488,15 @@ static void test_complete_in_standby(void)
bdrv_drain_all_begin();
assert_job_status_is(job, JOB_STATUS_STANDBY);
+/*
+ * Increase pause_count so that the counter is
+ * unbalanced and job won't resume
+ */
+job_pause(job);
+
/* Lock the IO thread to prevent the job from being run */
-aio_context_acquire(ctx);
/* This will schedule the job to resume it */
bdrv_drain_all_end();
-aio_context_release(ctx);
WITH_JOB_LOCK_GUARD() {
/* But the job cannot run, so it will remain on standby */
@@ -531,13 +535,6 @@ int main(int argc, char **argv)
g_test_add_func("/blockjob/cancel/standby", test_cancel_standby);
g_test_add_func("/blockjob/cancel/pending", test_cancel_pending);
g_test_add_func("/blockjob/cancel/concluded", test_cancel_concluded);
-
-/*
- * This test is flaky and sometimes fails in CI and otherwise:
- * don't run unless user opts in via environment variable.
- */
-if (getenv("QEMU_TEST_FLAKY_TESTS")) {
-g_test_add_func("/blockjob/complete_in_standby",
test_complete_in_standby);
-}
+g_test_add_func("/blockjob/complete_in_standby", test_complete_in_standby);
return g_test_run();
}
--
2.39.1
Re: test-blockjob: intermittent CI failures in msys2-64bit job
Am 25/04/2023 um 18:48 schrieb Hanna Czenczek: > On 24.04.23 20:32, Vladimir Sementsov-Ogievskiy wrote: >> On 24.04.23 16:36, Emanuele Giuseppe Esposito wrote: >>> >>> >>> Am 21/04/2023 um 12:13 schrieb Vladimir Sementsov-Ogievskiy: >>>> On 17.03.23 15:35, Thomas Huth wrote: >>>>> On 17/03/2023 11.17, Peter Maydell wrote: >>>>>> On Mon, 6 Mar 2023 at 11:16, Peter Maydell >>>>>> wrote: >>>>>>> >>>>>>> On Fri, 3 Mar 2023 at 18:36, Peter Maydell >>>>>>> wrote: >>>>>>>> >>>>>>>> I've noticed that test-blockjob seems to fail intermittently >>>>>>>> on the msys2-64bit job: >>>>>>>> >>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/3872508803 >>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/3871061024 >>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/3865312440 >>>>>>>> >>>>>>>> Sample output: >>>>>>>> | 53/89 >>>>>>>> ERROR:../tests/unit/test-blockjob.c:499:test_complete_in_standby: >>>>>>>> assertion failed: (job->status == JOB_STATUS_STANDBY) ERROR >>>>>>>> 53/89 qemu:unit / test-blockjob ERROR 0.08s exit status 3 >>>>>> >>>>>>> Here's an intermittent failure from my macos x86 machine: >>>>>>> >>>>>>> 172/621 qemu:unit / test-blockjob >>>>>>> ERROR 0.26s killed by signal 6 SIGABRT >>>>>> >>>>>> And an intermittent on the freebsd 13 CI job: >>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/3950667240 >>>>>> >>>>>>>>> MALLOC_PERTURB_=197 >>>>>>>>> G_TEST_BUILDDIR=/tmp/cirrus-ci-build/build/tests/unit >>>>>>>>> G_TEST_SRCDIR=/tmp/cirrus-ci-build/tests/unit >>>>>>>>> /tmp/cirrus-ci-build/build/tests/unit/test-blockjob --tap -k >>>>>> ▶ 178/650 /blockjob/ids >>>>>> OK >>>>>> 178/650 qemu:unit / test-blockjob >>>>>> ERROR 0.31s killed by signal 6 SIGABRT >>>>>> ― ✀ >>>>>> ― >>>>>> stderr: >>>>>> Assertion failed: (job->status == JOB_STATUS_STANDBY), function >>>>>> test_complete_in_standby, file ../tests/unit/test-blockjob.c, line >>>>>> 499. >>>>>> >>>>>> >>>>>> TAP parsing error: Too few tests run (expected 9, got 1) >>>>>> (test program exited with status code -6) >>>>>> ―― >>>>>> >>>>>> Anybody in the block team looking at these, or shall we just >>>>>> disable this test entirely ? >>>>> >>>>> I ran into this issue today, too: >>>>> >>>>> https://gitlab.com/thuth/qemu/-/jobs/3954367101 >>>>> >>>>> ... if nobody is interested in fixing this test, I think we should >>>>> disable it... >>>>> >>>>> Thomas >>>>> >>>> >>>> I'm looking at this now, and seems that it's broken since >>>> 6f592e5aca1a27fe1c1f6 "job.c: enable job lock/unlock and remove >>>> Aiocontext locks", as it stops critical section by new >>>> aio_context_release() call exactly after bdrv_drain_all_and(), so it's >>>> not a surprise that job may start at that moment and the following >>>> assertion fires. >>>> >>>> Emanuele could please look at it? >>>> >>> Well if I understood correctly, the only thing that was preventing the >>> job from continuing was the aiocontext lock held. >>> >>> The failing assertion even mentions that: >>> /* Lock the IO thread to prevent the job from being run */ >>> [...] >>> /* But the job cannot run, so it will remain on standby */ >>> assert(job->status == JOB_STATUS_STANDBY); >>> >>> Essentially bdrv_drain_all_end() would wake up the job coroutine, but it >>> would anyways block somewhere after since the aiocontext lock was taken >>> by th
Re: test-blockjob: intermittent CI failures in msys2-64bit job
Am 21/04/2023 um 12:13 schrieb Vladimir Sementsov-Ogievskiy: > On 17.03.23 15:35, Thomas Huth wrote: >> On 17/03/2023 11.17, Peter Maydell wrote: >>> On Mon, 6 Mar 2023 at 11:16, Peter Maydell >>> wrote: On Fri, 3 Mar 2023 at 18:36, Peter Maydell wrote: > > I've noticed that test-blockjob seems to fail intermittently > on the msys2-64bit job: > > https://gitlab.com/qemu-project/qemu/-/jobs/3872508803 > https://gitlab.com/qemu-project/qemu/-/jobs/3871061024 > https://gitlab.com/qemu-project/qemu/-/jobs/3865312440 > > Sample output: > | 53/89 > ERROR:../tests/unit/test-blockjob.c:499:test_complete_in_standby: > assertion failed: (job->status == JOB_STATUS_STANDBY) ERROR > 53/89 qemu:unit / test-blockjob ERROR 0.08s exit status 3 >>> Here's an intermittent failure from my macos x86 machine: 172/621 qemu:unit / test-blockjob ERROR 0.26s killed by signal 6 SIGABRT >>> >>> And an intermittent on the freebsd 13 CI job: >>> https://gitlab.com/qemu-project/qemu/-/jobs/3950667240 >>> >> MALLOC_PERTURB_=197 >> G_TEST_BUILDDIR=/tmp/cirrus-ci-build/build/tests/unit >> G_TEST_SRCDIR=/tmp/cirrus-ci-build/tests/unit >> /tmp/cirrus-ci-build/build/tests/unit/test-blockjob --tap -k >>> ▶ 178/650 /blockjob/ids >>> OK >>> 178/650 qemu:unit / test-blockjob >>> ERROR 0.31s killed by signal 6 SIGABRT >>> ― ✀ >>> ― >>> stderr: >>> Assertion failed: (job->status == JOB_STATUS_STANDBY), function >>> test_complete_in_standby, file ../tests/unit/test-blockjob.c, line >>> 499. >>> >>> >>> TAP parsing error: Too few tests run (expected 9, got 1) >>> (test program exited with status code -6) >>> ―― >>> >>> Anybody in the block team looking at these, or shall we just >>> disable this test entirely ? >> >> I ran into this issue today, too: >> >> https://gitlab.com/thuth/qemu/-/jobs/3954367101 >> >> ... if nobody is interested in fixing this test, I think we should >> disable it... >> >> Thomas >> > > I'm looking at this now, and seems that it's broken since > 6f592e5aca1a27fe1c1f6 "job.c: enable job lock/unlock and remove > Aiocontext locks", as it stops critical section by new > aio_context_release() call exactly after bdrv_drain_all_and(), so it's > not a surprise that job may start at that moment and the following > assertion fires. > > Emanuele could please look at it? > Well if I understood correctly, the only thing that was preventing the job from continuing was the aiocontext lock held. The failing assertion even mentions that: /* Lock the IO thread to prevent the job from being run */ [...] /* But the job cannot run, so it will remain on standby */ assert(job->status == JOB_STATUS_STANDBY); Essentially bdrv_drain_all_end() would wake up the job coroutine, but it would anyways block somewhere after since the aiocontext lock was taken by the test. Now that we got rid of aiocontext lock in job code, nothing prevents the test from resuming. Mixing job lock and aiocontext acquire/release is not a good idea, and it would anyways block job_resume() called by bdrv_drain_all_end(), so the test itself would deadlock. So unless @Kevin has a better idea, this seems to be just an "hack" to test stuff that is not possible to do now anymore. What I would suggest is to get rid of that test, or at least of that assert function. I unfortunately cannot reproduce the failure, but I think the remaining functions called by the test should run as expected. Thank you, Emanuele
Re: [PATCH v2] block/monitor/block-hmp-cmds.c: Fix crash when execute hmp_commit
Am 24/04/2023 um 12:39 schrieb wanglian...@126.com: > From: Wang Liang > > hmp_commit() calls blk_is_available() from a non-coroutine context (and in > the main loop). blk_is_available() is a co_wrapper_mixed_bdrv_rdlock > function, and in the non-coroutine context it calls AIO_WAIT_WHILE(), > which crashes if the aio_context lock is not taken before. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1615 > Signed-off-by: Wang Liang > Thanks! Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH] block/monitor/block-hmp-cmds.c: Fix crash when execute hmp_commit
Am 24/04/2023 um 10:15 schrieb Emanuele Giuseppe Esposito: > > > Am 23/04/2023 um 13:02 schrieb wanglian...@126.com: >> From: Wang Liang >> >> We need to get the aio_context before calling the blk_is_available. >> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1615 >> Signed-off-by: Wang Liang >> > > > Reviewed-by: Emanuele Giuseppe Esposito > Sorry I forgot, if you need to resend can you add the cause of this issue in the commit message? Something along the lines of: "hmp_commit() calls blk_is_available() from a non-coroutine context (and in the main loop). Since this is a co_wrapper_mixed_bdrv_rdlock function, in this case it calls AIO_WAIT_WHILE(), which crashes if the aio_context lock is not taken before" Thank you, Emanuele
Re: [PATCH] block/monitor/block-hmp-cmds.c: Fix crash when execute hmp_commit
Am 23/04/2023 um 13:02 schrieb wanglian...@126.com: > From: Wang Liang > > We need to get the aio_context before calling the blk_is_available. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1615 > Signed-off-by: Wang Liang > Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH] aio-wait: avoid AioContext lock in aio_wait_bh_oneshot()
Am 04/04/2023 um 17:33 schrieb Stefan Hajnoczi: > There is no need for the AioContext lock in aio_wait_bh_oneshot(). > It's easy to remove the lock from existing callers and then switch from > AIO_WAIT_WHILE() to AIO_WAIT_WHILE_UNLOCKED() in aio_wait_bh_oneshot(). > > Document that the AioContext lock should not be held across > aio_wait_bh_oneshot(). Holding a lock across aio_poll() can cause > deadlock so we don't want callers to do that. > > This is a step towards getting rid of the AioContext lock. > > Cc: Paolo Bonzini > Signed-off-by: Stefan Hajnoczi > Reviewed-by: Emanuele Giuseppe Esposito
Re: [PATCH for-8.0] block/export: Fix graph locking in blk_get_geometry() call
Am 27/03/2023 um 13:39 schrieb Kevin Wolf:
> blk_get_geometry() eventually calls bdrv_nb_sectors(), which is a
> co_wrapper_mixed_bdrv_rdlock. This means that when it is called from
> coroutine context, it already assume to have the graph locked.
>
> However, virtio_blk_sect_range_ok() in block/export/virtio-blk-handler.c
> (used by vhost-user-blk and VDUSE exports) runs in a coroutine, but
> doesn't take the graph lock - blk_*() functions are generally expected
> to do that internally. This causes an assertion failure when accessing
> an export for the first time if it runs in an iothread.
>
> This is an example of the crash:
>
> $ ./storage-daemon/qemu-storage-daemon --object iothread,id=th0 --blockdev
> file,filename=/home/kwolf/images/hd.img,node-name=disk --export
> vhost-user-blk,addr.type=unix,addr.path=/tmp/vhost.sock,node-name=disk,id=exp0,iothread=th0
> qemu-storage-daemon: ../block/graph-lock.c:268: void
> assert_bdrv_graph_readable(void): Assertion `qemu_in_main_thread() ||
> reader_count()' failed.
>
> (gdb) bt
>
> Fix this by creating a new blk_co_get_geometry() that takes the lock,
> and changing blk_get_geometry() to be a co_wrapper_mixed around it.
>
> To make the resulting code cleaner, virtio-blk-handler.c can directly
> call the coroutine version now (though that wouldn't be necessary for
> fixing the bug, taking the lock in blk_co_get_geometry() is what fixes
> it).
>
> Fixes: 8ab8140a04cf771d63e9754d6ba6c1e676bfe507
> Reported-by: Lukáš Doktor
> Signed-off-by: Kevin Wolf
Reviewed-by: Emanuele Giuseppe Esposito
> ---
> include/block/block-io.h | 4 +++-
> include/sysemu/block-backend-io.h | 5 -
> block.c | 5 +++--
> block/block-backend.c | 7 +--
> block/export/virtio-blk-handler.c | 7 ---
> 5 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/include/block/block-io.h b/include/block/block-io.h
> index 5da99d4d60..dbc034b728 100644
> --- a/include/block/block-io.h
> +++ b/include/block/block-io.h
> @@ -89,7 +89,9 @@ int64_t co_wrapper
> bdrv_get_allocated_file_size(BlockDriverState *bs);
>
> BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
> BlockDriverState *in_bs, Error **errp);
> -void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
> +
> +void coroutine_fn GRAPH_RDLOCK
> +bdrv_co_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
>
> int coroutine_fn GRAPH_RDLOCK
> bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
> diff --git a/include/sysemu/block-backend-io.h
> b/include/sysemu/block-backend-io.h
> index 40ab178719..c672b77247 100644
> --- a/include/sysemu/block-backend-io.h
> +++ b/include/sysemu/block-backend-io.h
> @@ -70,7 +70,10 @@ void co_wrapper blk_eject(BlockBackend *blk, bool
> eject_flag);
> int64_t coroutine_fn blk_co_getlength(BlockBackend *blk);
> int64_t co_wrapper_mixed blk_getlength(BlockBackend *blk);
>
> -void blk_get_geometry(BlockBackend *blk, uint64_t *nb_sectors_ptr);
> +void coroutine_fn blk_co_get_geometry(BlockBackend *blk,
> + uint64_t *nb_sectors_ptr);
> +void co_wrapper_mixed blk_get_geometry(BlockBackend *blk,
> + uint64_t *nb_sectors_ptr);
>
> int64_t coroutine_fn blk_co_nb_sectors(BlockBackend *blk);
> int64_t co_wrapper_mixed blk_nb_sectors(BlockBackend *blk);
> diff --git a/block.c b/block.c
> index 0dd604d0f6..e0c6c648b1 100644
> --- a/block.c
> +++ b/block.c
> @@ -5879,9 +5879,10 @@ int64_t coroutine_fn
> bdrv_co_getlength(BlockDriverState *bs)
> }
>
> /* return 0 as number of sectors if no device present or error */
> -void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
> +void coroutine_fn bdrv_co_get_geometry(BlockDriverState *bs,
> + uint64_t *nb_sectors_ptr)
> {
> -int64_t nb_sectors = bdrv_nb_sectors(bs);
> +int64_t nb_sectors = bdrv_co_nb_sectors(bs);
> IO_CODE();
>
> *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 278b04ce69..2ee39229e4 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1615,13 +1615,16 @@ int64_t coroutine_fn blk_co_getlength(BlockBackend
> *blk)
> return bdrv_co_getlength(blk_bs(blk));
> }
>
> -void blk_get_geometry(BlockBackend *blk, uint64_t *nb_sectors_ptr)
> +void coroutine_fn blk_co_get_geometry(BlockBackend *blk,
> + uint64_t *nb_sectors_ptr)
> {
> IO_CODE();
> +GRAPH_RDLOCK_GUARD();
Re: [PATCH 00/23] block: Lock the graph, part 2 (BlockDriver callbacks)
Am 03/02/2023 um 16:21 schrieb Kevin Wolf: > After introducing the graph lock in a previous series, this series > actually starts making widespread use of it. > > Most of the BlockDriver callbacks access the children list in some way, > so you need to hold the graph lock to call them. The patches in this > series add the corresponding GRAPH_RDLOCK annotations and take the lock > in places where it doesn't happen yet - all of the bdrv_*() co_wrappers > are already covered, but in particular BlockBackend coroutine_fns still > need it. > > There is no particularly good reason why exactly these patches and not > others are included in the series. I couldn't find a self-contained part > that could reasonable be addressed in a single series. So these just > happen to be patches that are somewhat related (centered around the > BlockDriver callback theme), are ready and their number looks > manageable. You will still see some FIXMEs at the end of the series > that will only be addressed in future patches. Reviewed-by: Emanuele Giuseppe Esposito > > Emanuele Giuseppe Esposito (5): > block/qed: add missing graph rdlock in qed_need_check_timer_entry > block: Mark bdrv_co_flush() and callers GRAPH_RDLOCK > block: Mark bdrv_co_pdiscard() and callers GRAPH_RDLOCK > block: Mark bdrv_co_copy_range() GRAPH_RDLOCK > block: Mark bdrv_co_is_inserted() and callers GRAPH_RDLOCK > > Kevin Wolf (18): > block: Make bdrv_can_set_read_only() static > mirror: Fix access of uninitialised fields during start > block: Mark bdrv_co_truncate() and callers GRAPH_RDLOCK > block: Mark bdrv_co_block_status() and callers GRAPH_RDLOCK > block: Mark bdrv_co_ioctl() and callers GRAPH_RDLOCK > block: Mark bdrv_co_pwrite_zeroes() and callers GRAPH_RDLOCK > block: Mark read/write in block/io.c GRAPH_RDLOCK > block: Mark public read/write functions GRAPH_RDLOCK > block: Mark bdrv_co_pwrite_sync() and callers GRAPH_RDLOCK > block: Mark bdrv_co_do_pwrite_zeroes() GRAPH_RDLOCK > block: Mark preadv_snapshot/snapshot_block_status GRAPH_RDLOCK > block: Mark bdrv_co_create() and callers GRAPH_RDLOCK > block: Mark bdrv_co_io_(un)plug() and callers GRAPH_RDLOCK > block: Mark bdrv_co_eject/lock_medium() and callers GRAPH_RDLOCK > block: Mark bdrv_(un)register_buf() GRAPH_RDLOCK > block: Mark bdrv_co_delete_file() and callers GRAPH_RDLOCK > block: Mark bdrv_*_dirty_bitmap() and callers GRAPH_RDLOCK > block: Mark bdrv_co_refresh_total_sectors() and callers GRAPH_RDLOCK > > block/coroutines.h | 2 +- > block/qcow2.h | 27 +++-- > block/qed.h| 45 > include/block/block-copy.h | 6 +- > include/block/block-global-state.h | 14 ++- > include/block/block-io.h | 110 +- > include/block/block_int-common.h | 173 - > include/block/block_int-io.h | 53 - > include/block/dirty-bitmap.h | 12 +- > include/sysemu/block-backend-io.h | 7 +- > block.c| 12 +- > block/backup.c | 3 + > block/blkdebug.c | 19 ++-- > block/blklogwrites.c | 35 +++--- > block/blkreplay.c | 24 ++-- > block/blkverify.c | 5 +- > block/block-backend.c | 39 +-- > block/block-copy.c | 32 +++--- > block/bochs.c | 2 +- > block/commit.c | 5 +- > block/copy-before-write.c | 33 +++--- > block/copy-on-read.c | 44 > block/create.c | 9 +- > block/crypto.c | 16 +-- > block/dirty-bitmap.c | 2 + > block/file-posix.c | 27 ++--- > block/file-win32.c | 7 +- > block/filter-compress.c| 36 +++--- > block/io.c | 108 +++--- > block/iscsi.c | 28 ++--- > block/mirror.c | 59 ++ > block/parallels.c | 33 +++--- > block/preallocate.c| 38 --- > block/qcow.c | 46 > block/qcow2-cluster.c | 17 ++- > block/qcow2.c | 136 --- > block/qed-check.c | 3 +- > block/qed-table.c | 10 +- > block/qed.c| 101 + > block/quorum.c | 62 ++- > block/raw-format.c | 76 ++--- > block/replication.c| 18 ++- > block/snapshot-access
[PATCH] block/file-posix: don't use functions calling AIO_WAIT_WHILE in worker threads
When calling bdrv_getlength() in handle_aiocb_write_zeroes(), the
function creates a new coroutine and then waits that it finishes using
AIO_WAIT_WHILE.
The problem is that this function could also run in a worker thread,
that has a different AioContext from main loop and iothreads, therefore
in AIO_WAIT_WHILE we will have in_aio_context_home_thread(ctx) == false
and therefore
assert(qemu_get_current_aio_context() == qemu_get_aio_context());
in the else branch will fail, crashing QEMU.
Aside from that, bdrv_getlength() is wrong also conceptually, because
it reads the BDS graph from another thread and is not protected by
any lock.
Replace it with raw_co_getlength, that doesn't create a coroutine and
doesn't read the BDS graph.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/file-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index d3073a7caa..9a99111f45 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1738,7 +1738,7 @@ static int handle_aiocb_write_zeroes(void *opaque)
#ifdef CONFIG_FALLOCATE
/* Last resort: we are trying to extend the file with zeroed data. This
* can be done via fallocate(fd, 0) */
-len = bdrv_getlength(aiocb->bs);
+len = raw_co_getlength(aiocb->bs);
if (s->has_fallocate && len >= 0 && aiocb->aio_offset >= len) {
int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
if (ret == 0 || ret != -ENOTSUP) {
--
2.39.1
Re: rainier-bmc machine dumping core with latest qemu
Adding Kevin too.
I can't reproduce your issue. I tried the exact steps showed in your
mail, both with BLD_VERSION=20230205025034 (not anymore latest compose)
and BLD_VERSION=20230209025037 (currently latest), and the VM boots till
the login screen.
Talking with Kevin it looks like handle_aiocb_write_zeroes() should not
use bdrv_getlength() but rather raw_co_getlength().
Could you try replacing it in block/file-posix.c line 1741 and rebuild
and see if it works?
Thank you,
Emanuele
Am 08/02/2023 um 15:56 schrieb Philippe Mathieu-Daudé:
> Cc'ing Emanuele.
>
> On 8/2/23 15:26, Ninad Palsule wrote:
>> Hello,
>>
>> I am hitting core dump while running qemu with rainier-bmc machine. I
>> started
>> hitting it after I rebased to latest qemu from master.
>> Can someone please help?
>>
>> /home/abc/dev/qemu/build/qemu-system-arm -M rainier-bmc -nographic \
>> -kernel
>> ./fitImage-linux.bin--${LINUX_VERSION}-r0-p10bmc-${BLD_VERSION}.bin \
>> -dtb
>> ./aspeed-bmc-ibm-rainier--${LINUX_VERSION}-r0-p10bmc-${BLD_VERSION}.dtb \
>> -initrd
>> ./obmc-phosphor-initramfs-p10bmc-${BLD_VERSION}.rootfs.cpio.xz \
>> -drive
>> file=./obmc-phosphor-image-p10bmc-${BLD_VERSION}.rootfs.wic.qcow2,if=sd,index=2
>> \
>> -append 'rootwait console=ttyS4,115200n8 root=PARTLABEL=rofs-a'
>>
>>
>>
>> [ OK ] Started Journal Service.
>> Starting Flush Journal to Persistent Storage...
>> [ 45.873184] systemd-journald[156]: Received client request to flush
>> runtime journal.
>> [ 45.932231] systemd-journald[156]: File
>> /var/log/journal/97cd620eaa284caf980533438c7355c4/system.journal
>> corrupted or uncleanly shut down, renaming and replacing.
>> qemu-system-arm: /home/abc/dev/qemu/block/block-gen.h:43:
>> bdrv_poll_co: Assertion `qemu_get_current_aio_context() ==
>> qemu_get_aio_context()' failed.
>>
>> Thread 3 "qemu-system-arm" received signal SIGABRT, Aborted.
>> [Switching to Thread 0x76896640 (LWP 3898452)]
>> __pthread_kill_implementation (no_tid=0, signo=6,
>> threadid=140737329587776) at ./nptl/pthread_kill.c:44
>> 44 ./nptl/pthread_kill.c: No such file or directory.
>>
>>
>> #6 0x777e8e96 in __GI___assert_fail
>> (assertion=assertion@entry=0x5603e798
>> "qemu_get_current_aio_context() ==
>> qemu_get_aio_context()", file=file@entry=0x560aef28
>> "/home/abc/dev/qemu/block/block-gen.h", line=line@entry=43,
>> function=function@entry=0x560af080 <__PRETTY_FUNCTION__.9>
>> "bdrv_poll_co") at ./assert/assert.c:101
>> #7 0x55da91f9 in bdrv_poll_co (s=0x768957f0) at
>> /home/abc/dev/qemu/block/block-gen.h:43
>> #8 bdrv_poll_co (s=0x768957f0) at
>> /home/abc/dev/qemu/block/block-gen.h:38
>> #9 bdrv_getlength (bs=) at block/block-gen.c:310
>> #10 0x55e3558e in handle_aiocb_write_zeroes
>> (opaque=0x7fff419965f0) at ../block/file-posix.c:1741
>> #11 0x55ef80fd in worker_thread
>> (opaque=opaque@entry=0x569e2300) at ../util/thread-pool.c:110
>> #12 0x55ee3901 in qemu_thread_start (args=) at
>> ../util/qemu-thread-posix.c:505
>> #13 0x77843b43 in start_thread (arg=) at
>> ./nptl/pthread_create.c:442
>> #14 0x778d5a00 in clone3 () at
>> ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
>>
>> (gdb) p qemu_aio_context
>> $1 = (AioContext *) 0x568b13d0
>>
>> ---
>>
>>
>> Reproduction steps:
>>
>> 1. Clone and build qemu from https://github.com/qemu/qemu
>> 2. Download following eBMC images
>>
>> BLD_VERSION=20230205025034
>> LINUX_VERSION="6.0.19+git0+67c9407e1f"
>> wget
>> https://jenkins.openbmc.org/view/latest/job/latest-master/label=docker-builder,target=p10bmc/lastStableBuild/artifact/openbmc/build/tmp/deploy/images/p10bmc/obmc-phosphor-initramfs-p10bmc-${BLD_VERSION}.rootfs.cpio.xz
>>
>> wget
>> https://jenkins.openbmc.org/view/latest/job/latest-master/label=docker-builder,target=p10bmc/lastStableBuild/artifact/openbmc/build/tmp/deploy/images/p10bmc/aspeed-bmc-ibm-rainier--${LINUX_VERSION}-r0-p10bmc-${BLD_VERSION}.dtb
>>
>> wget
>> https://jenkins.openbmc.org/view/latest/job/latest-master/label=docker-builder,target=p10bmc/lastStableBuild/artifact/openbmc/build/tmp/deploy/images/p10bmc/fitImage-linux.bin--${LINUX_VERSION}-r0-p10bmc-${BLD_VERSION}.bin
>>
>> wget
>> https://jenkins.openbmc.org/view/latest/job/latest-master/label=docker-builder,target=p10bmc/lastStableBuild/artifact/openbmc/build/tmp/deploy/images/p10bmc/obmc-phosphor-image-p10bmc-${BLD_VERSION}.rootfs.wic.qcow2
>> qemu-img resize
>> obmc-phosphor-image-p10bmc-${BLD_VERSION}.rootfs.wic.qcow2 16G
>>
>> 3. Run the qemu command as show at the start
>>
>>
>>
>
[PATCH] virtio-blk: add missing AioContext lock
virtio_blk_update_config() calls blk_get_geometry and blk_getlength,
and both functions eventually end up calling bdrv_poll_co when not
running in a coroutine:
- blk_getlength is a co_wrapper_mixed function
- blk_get_geometry calls bdrv_get_geometry -> bdrv_nb_sectors, a
co_wrapper_mixed function too
Since we are not running in a coroutine, we need to take s->blk
AioContext lock, otherwise bdrv_poll_co will inevitably call
AIO_WAIT_WHILE and therefore try to un unlock() an AioContext lock
that was never acquired.
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2167838
Steps to reproduce the issue: simply boot a VM with
-object '{"qom-type":"iothread","id":"iothread1"}' \
-blockdev
'{"driver":"file","filename":"$QCOW2","aio":"native","node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}'
\
-blockdev
'{"node-name":"libvirt-1-format","read-only":false,"cache":{"direct":true,"no-flush":false},"driver":"qcow2","file":"libvirt-1-storage"}'
\
-device
virtio-blk-pci,iothread=iothread1,drive=libvirt-1-format,id=virtio-disk0,bootindex=1,write-cache=on
and observe that it will fail not manage to boot with "qemu_mutex_unlock_impl:
Operation not permitted"
Signed-off-by: Emanuele Giuseppe Esposito
---
hw/block/virtio-blk.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 1762517878..cefca93b31 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -894,6 +894,10 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
uint8_t *config)
uint64_t capacity;
int64_t length;
int blk_size = conf->logical_block_size;
+AioContext *ctx;
+
+ctx = blk_get_aio_context(s->blk);
+aio_context_acquire(ctx);
blk_get_geometry(s->blk, );
memset(, 0, sizeof(blkcfg));
@@ -917,6 +921,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
uint8_t *config)
* per track (cylinder).
*/
length = blk_getlength(s->blk);
+aio_context_release(ctx);
if (length > 0 && length / conf->heads / conf->secs % blk_size) {
blkcfg.geometry.sectors = conf->secs & ~s->sector_mask;
} else {
--
2.39.1
[PATCH v5 1/4] linux-aio: use LinuxAioState from the running thread
Remove usage of aio_context_acquire by always submitting asynchronous
AIO to the current thread's LinuxAioState.
In order to prevent mistakes from the caller side, avoid passing LinuxAioState
in laio_io_{plug/unplug} and laio_co_submit, and document the functions
to make clear that they work in the current thread's AioContext.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/aio.h | 4
include/block/raw-aio.h | 18 --
include/sysemu/block-backend-io.h | 6 ++
block/file-posix.c| 10 +++---
block/linux-aio.c | 29 +
5 files changed, 38 insertions(+), 29 deletions(-)
diff --git a/include/block/aio.h b/include/block/aio.h
index 8fba6a3584..b6b396cfcb 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -208,10 +208,6 @@ struct AioContext {
struct ThreadPool *thread_pool;
#ifdef CONFIG_LINUX_AIO
-/*
- * State for native Linux AIO. Uses aio_context_acquire/release for
- * locking.
- */
struct LinuxAioState *linux_aio;
#endif
#ifdef CONFIG_LINUX_IO_URING
diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index f8cda9df91..db614472e6 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -49,14 +49,20 @@
typedef struct LinuxAioState LinuxAioState;
LinuxAioState *laio_init(Error **errp);
void laio_cleanup(LinuxAioState *s);
-int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
-uint64_t offset, QEMUIOVector *qiov, int type,
-uint64_t dev_max_batch);
+
+/* laio_co_submit: submit I/O requests in the thread's current AioContext. */
+int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov,
+int type, uint64_t dev_max_batch);
+
void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
-void laio_io_plug(BlockDriverState *bs, LinuxAioState *s);
-void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
-uint64_t dev_max_batch);
+
+/*
+ * laio_io_plug/unplug work in the thread's current AioContext, therefore the
+ * caller must ensure that they are paired in the same IOThread.
+ */
+void laio_io_plug(void);
+void laio_io_unplug(uint64_t dev_max_batch);
#endif
/* io_uring.c - Linux io_uring implementation */
#ifdef CONFIG_LINUX_IO_URING
diff --git a/include/sysemu/block-backend-io.h
b/include/sysemu/block-backend-io.h
index 031a27ba10..d41698ccc5 100644
--- a/include/sysemu/block-backend-io.h
+++ b/include/sysemu/block-backend-io.h
@@ -74,8 +74,14 @@ void blk_iostatus_set_err(BlockBackend *blk, int error);
int blk_get_max_iov(BlockBackend *blk);
int blk_get_max_hw_iov(BlockBackend *blk);
+/*
+ * blk_io_plug/unplug are thread-local operations. This means that multiple
+ * IOThreads can simultaneously call plug/unplug, but the caller must ensure
+ * that each unplug() is called in the same IOThread of the matching plug().
+ */
void blk_io_plug(BlockBackend *blk);
void blk_io_unplug(BlockBackend *blk);
+
AioContext *blk_get_aio_context(BlockBackend *blk);
BlockAcctStats *blk_get_stats(BlockBackend *blk);
void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
diff --git a/block/file-posix.c b/block/file-posix.c
index fa227d9d14..fa99d1c25a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2095,10 +2095,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs,
uint64_t offset,
#endif
#ifdef CONFIG_LINUX_AIO
} else if (s->use_linux_aio) {
-LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
assert(qiov->size == bytes);
-return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
- s->aio_max_batch);
+return laio_co_submit(s->fd, offset, qiov, type, s->aio_max_batch);
#endif
}
@@ -2137,8 +2135,7 @@ static void raw_aio_plug(BlockDriverState *bs)
BDRVRawState __attribute__((unused)) *s = bs->opaque;
#ifdef CONFIG_LINUX_AIO
if (s->use_linux_aio) {
-LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
-laio_io_plug(bs, aio);
+laio_io_plug();
}
#endif
#ifdef CONFIG_LINUX_IO_URING
@@ -2154,8 +2151,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
BDRVRawState __attribute__((unused)) *s = bs->opaque;
#ifdef CONFIG_LINUX_AIO
if (s->use_linux_aio) {
-LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
-laio_io_unplug(bs, aio, s->aio_max_batch);
+laio_io_unplug(s->aio_max_batch);
}
#endif
#ifdef CONFIG_LINUX_IO_URING
diff --git a/block/linux-aio.c b/block/linux-aio.c
index d2cfb7f523..fc50cdd1bf 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -16,6 +16,9 @@
#include "qemu/coroutine.h"
#include
[PATCH v5 3/4] thread-pool: use ThreadPool from the running thread
Use qemu_get_current_aio_context() where possible, since we always
submit work to the current thread anyways.
We want to also be sure that the thread submitting the work is
the same as the one processing the pool, to avoid adding
synchronization to the pool list.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/thread-pool.h | 5 +
block/file-posix.c | 21 ++---
block/file-win32.c | 2 +-
block/qcow2-threads.c | 2 +-
util/thread-pool.c | 9 -
5 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/include/block/thread-pool.h b/include/block/thread-pool.h
index 95ff2b0bdb..c408bde74c 100644
--- a/include/block/thread-pool.h
+++ b/include/block/thread-pool.h
@@ -29,12 +29,17 @@ typedef struct ThreadPool ThreadPool;
ThreadPool *thread_pool_new(struct AioContext *ctx);
void thread_pool_free(ThreadPool *pool);
+/*
+ * thread_pool_submit* API: submit I/O requests in the thread's
+ * current AioContext.
+ */
BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
ThreadPoolFunc *func, void *arg,
BlockCompletionFunc *cb, void *opaque);
int coroutine_fn thread_pool_submit_co(ThreadPool *pool,
ThreadPoolFunc *func, void *arg);
void thread_pool_submit(ThreadPool *pool, ThreadPoolFunc *func, void *arg);
+
void thread_pool_update_params(ThreadPool *pool, struct AioContext *ctx);
#endif
diff --git a/block/file-posix.c b/block/file-posix.c
index b8ee58201c..f7d88fa857 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2040,11 +2040,10 @@ out:
return result;
}
-static int coroutine_fn raw_thread_pool_submit(BlockDriverState *bs,
- ThreadPoolFunc func, void *arg)
+static int coroutine_fn raw_thread_pool_submit(ThreadPoolFunc func, void *arg)
{
/* @bs can be NULL, bdrv_get_aio_context() returns the main context then */
-ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+ThreadPool *pool = aio_get_thread_pool(qemu_get_current_aio_context());
return thread_pool_submit_co(pool, func, arg);
}
@@ -2112,7 +2111,7 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs,
uint64_t offset,
};
assert(qiov->size == bytes);
-return raw_thread_pool_submit(bs, handle_aiocb_rw, );
+return raw_thread_pool_submit(handle_aiocb_rw, );
}
static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset,
@@ -2181,7 +2180,7 @@ static int coroutine_fn
raw_co_flush_to_disk(BlockDriverState *bs)
return luring_co_submit(bs, s->fd, 0, NULL, QEMU_AIO_FLUSH);
}
#endif
-return raw_thread_pool_submit(bs, handle_aiocb_flush, );
+return raw_thread_pool_submit(handle_aiocb_flush, );
}
static void raw_aio_attach_aio_context(BlockDriverState *bs,
@@ -2243,7 +2242,7 @@ raw_regular_truncate(BlockDriverState *bs, int fd,
int64_t offset,
},
};
-return raw_thread_pool_submit(bs, handle_aiocb_truncate, );
+return raw_thread_pool_submit(handle_aiocb_truncate, );
}
static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
@@ -2993,7 +2992,7 @@ raw_do_pdiscard(BlockDriverState *bs, int64_t offset,
int64_t bytes,
acb.aio_type |= QEMU_AIO_BLKDEV;
}
-ret = raw_thread_pool_submit(bs, handle_aiocb_discard, );
+ret = raw_thread_pool_submit(handle_aiocb_discard, );
raw_account_discard(s, bytes, ret);
return ret;
}
@@ -3068,7 +3067,7 @@ raw_do_pwrite_zeroes(BlockDriverState *bs, int64_t
offset, int64_t bytes,
handler = handle_aiocb_write_zeroes;
}
-return raw_thread_pool_submit(bs, handler, );
+return raw_thread_pool_submit(handler, );
}
static int coroutine_fn raw_co_pwrite_zeroes(
@@ -3279,7 +3278,7 @@ static int coroutine_fn
raw_co_copy_range_to(BlockDriverState *bs,
},
};
-return raw_thread_pool_submit(bs, handle_aiocb_copy_range, );
+return raw_thread_pool_submit(handle_aiocb_copy_range, );
}
BlockDriver bdrv_file = {
@@ -3609,7 +3608,7 @@ hdev_co_ioctl(BlockDriverState *bs, unsigned long int
req, void *buf)
struct sg_io_hdr *io_hdr = buf;
if (io_hdr->cmdp[0] == PERSISTENT_RESERVE_OUT ||
io_hdr->cmdp[0] == PERSISTENT_RESERVE_IN) {
-return pr_manager_execute(s->pr_mgr, bdrv_get_aio_context(bs),
+return pr_manager_execute(s->pr_mgr,
qemu_get_current_aio_context(),
s->fd, io_hdr);
}
}
@@ -3625,7 +3624,7 @@ hdev_co_ioctl(BlockDriverState *bs, unsigned long int
req, void *buf)
},
};
-return raw_thread_pool_submit(bs, handle_aiocb_ioctl, );
+return raw_thread_pool_submit(handle_aiocb_ioctl, );
}
#endif /* linux */
diff --git a/block/file-win32.c b/block/file-win32.c
index 12be9c3d0f..1af6d3c810 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -168,7 +168,7 @@ static Bl
[PATCH v5 2/4] io_uring: use LuringState from the running thread
Remove usage of aio_context_acquire by always submitting asynchronous
AIO to the current thread's LuringState.
In order to prevent mistakes from the caller side, avoid passing LuringState
in luring_io_{plug/unplug} and luring_co_submit, and document the functions
to make clear that they work in the current thread's AioContext.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/aio.h | 4
include/block/raw-aio.h | 15 +++
block/file-posix.c | 12
block/io_uring.c| 23 +++
4 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/include/block/aio.h b/include/block/aio.h
index b6b396cfcb..3b7634bef4 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -211,10 +211,6 @@ struct AioContext {
struct LinuxAioState *linux_aio;
#endif
#ifdef CONFIG_LINUX_IO_URING
-/*
- * State for Linux io_uring. Uses aio_context_acquire/release for
- * locking.
- */
struct LuringState *linux_io_uring;
/* State for file descriptor monitoring using Linux io_uring */
diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index db614472e6..e46a29c3f0 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -69,12 +69,19 @@ void laio_io_unplug(uint64_t dev_max_batch);
typedef struct LuringState LuringState;
LuringState *luring_init(Error **errp);
void luring_cleanup(LuringState *s);
-int coroutine_fn luring_co_submit(BlockDriverState *bs, LuringState *s, int fd,
-uint64_t offset, QEMUIOVector *qiov, int type);
+
+/* luring_co_submit: submit I/O requests in the thread's current AioContext. */
+int coroutine_fn luring_co_submit(BlockDriverState *bs, int fd, uint64_t
offset,
+ QEMUIOVector *qiov, int type);
void luring_detach_aio_context(LuringState *s, AioContext *old_context);
void luring_attach_aio_context(LuringState *s, AioContext *new_context);
-void luring_io_plug(BlockDriverState *bs, LuringState *s);
-void luring_io_unplug(BlockDriverState *bs, LuringState *s);
+
+/*
+ * luring_io_plug/unplug work in the thread's current AioContext, therefore the
+ * caller must ensure that they are paired in the same IOThread.
+ */
+void luring_io_plug(void);
+void luring_io_unplug(void);
#endif
#ifdef _WIN32
diff --git a/block/file-posix.c b/block/file-posix.c
index fa99d1c25a..b8ee58201c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2089,9 +2089,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs,
uint64_t offset,
type |= QEMU_AIO_MISALIGNED;
#ifdef CONFIG_LINUX_IO_URING
} else if (s->use_linux_io_uring) {
-LuringState *aio = aio_get_linux_io_uring(bdrv_get_aio_context(bs));
assert(qiov->size == bytes);
-return luring_co_submit(bs, aio, s->fd, offset, qiov, type);
+return luring_co_submit(bs, s->fd, offset, qiov, type);
#endif
#ifdef CONFIG_LINUX_AIO
} else if (s->use_linux_aio) {
@@ -2140,8 +2139,7 @@ static void raw_aio_plug(BlockDriverState *bs)
#endif
#ifdef CONFIG_LINUX_IO_URING
if (s->use_linux_io_uring) {
-LuringState *aio = aio_get_linux_io_uring(bdrv_get_aio_context(bs));
-luring_io_plug(bs, aio);
+luring_io_plug();
}
#endif
}
@@ -2156,8 +2154,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
#endif
#ifdef CONFIG_LINUX_IO_URING
if (s->use_linux_io_uring) {
-LuringState *aio = aio_get_linux_io_uring(bdrv_get_aio_context(bs));
-luring_io_unplug(bs, aio);
+luring_io_unplug();
}
#endif
}
@@ -2181,8 +2178,7 @@ static int coroutine_fn
raw_co_flush_to_disk(BlockDriverState *bs)
#ifdef CONFIG_LINUX_IO_URING
if (s->use_linux_io_uring) {
-LuringState *aio = aio_get_linux_io_uring(bdrv_get_aio_context(bs));
-return luring_co_submit(bs, aio, s->fd, 0, NULL, QEMU_AIO_FLUSH);
+return luring_co_submit(bs, s->fd, 0, NULL, QEMU_AIO_FLUSH);
}
#endif
return raw_thread_pool_submit(bs, handle_aiocb_flush, );
diff --git a/block/io_uring.c b/block/io_uring.c
index 973e15d876..220fb72ae6 100644
--- a/block/io_uring.c
+++ b/block/io_uring.c
@@ -18,6 +18,9 @@
#include "qapi/error.h"
#include "trace.h"
+/* Only used for assertions. */
+#include "qemu/coroutine_int.h"
+
/* io_uring ring size */
#define MAX_ENTRIES 128
@@ -50,10 +53,9 @@ typedef struct LuringState {
struct io_uring ring;
-/* io queue for submit at batch. Protected by AioContext lock. */
+/* No locking required, only accessed from AioContext home thread */
LuringQueue io_q;
-/* I/O completion processing. Only runs in I/O thread. */
QEMUBH *completion_bh;
} LuringState;
@@ -209,6 +211,7 @@ end:
* eventually runs later. Coroutines cannot be entered recursively
* so avoid doing that!
*/
+assert(luringcb->co->ctx == luringc
[PATCH v5 0/4] AioContext removal: LinuxAioState and ThreadPool
Just remove some AioContext lock in LinuxAioState and ThreadPool. Not related to anything specific, so I decided to send it as a separate patch. These patches are taken from Paolo's old draft series. --- v5: * apply Stefan comments, add patch 4 to remove ThreadPool * param from thread_pool_submit* * document that functions run in current IOThread v4: * add missing aio_context removal, and fix typo v3: * remove qemu_coroutine_enter_if_inactive v2: * assertion in thread_pool * remove useless BlockDriverState * param in patch 1 and 2 * io_uring cleaned too Emanuele Giuseppe Esposito (4): linux-aio: use LinuxAioState from the running thread io_uring: use LuringState from the running thread thread-pool: use ThreadPool from the running thread thread-pool: avoid passing the pool parameter every time include/block/aio.h | 8 -- include/block/raw-aio.h | 33 --- include/block/thread-pool.h | 15 ++- include/sysemu/block-backend-io.h | 6 + backends/tpm/tpm_backend.c| 4 +-- block/file-posix.c| 45 --- block/file-win32.c| 4 +-- block/io_uring.c | 23 ++-- block/linux-aio.c | 29 +++- block/qcow2-threads.c | 3 +-- hw/9pfs/coth.c| 3 +-- hw/ppc/spapr_nvdimm.c | 6 ++--- hw/virtio/virtio-pmem.c | 3 +-- scsi/pr-manager.c | 3 +-- scsi/qemu-pr-helper.c | 3 +-- tests/unit/test-thread-pool.c | 12 - util/thread-pool.c| 25 + 17 files changed, 113 insertions(+), 112 deletions(-) -- 2.39.1
[PATCH v5 4/4] thread-pool: avoid passing the pool parameter every time
thread_pool_submit_aio() is always called on a pool taken from
qemu_get_current_aio_context(), and that is the only intended
use: each pool runs only in the same thread that is submitting
work to it, it can't run anywhere else.
Therefore simplify the thread_pool_submit* API and remove the
ThreadPool function parameter.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/thread-pool.h | 10 --
backends/tpm/tpm_backend.c| 4 +---
block/file-posix.c| 4 +---
block/file-win32.c| 4 +---
block/qcow2-threads.c | 3 +--
hw/9pfs/coth.c| 3 +--
hw/ppc/spapr_nvdimm.c | 6 ++
hw/virtio/virtio-pmem.c | 3 +--
scsi/pr-manager.c | 3 +--
scsi/qemu-pr-helper.c | 3 +--
tests/unit/test-thread-pool.c | 12 +---
util/thread-pool.c| 16
12 files changed, 27 insertions(+), 44 deletions(-)
diff --git a/include/block/thread-pool.h b/include/block/thread-pool.h
index c408bde74c..948ff5f30c 100644
--- a/include/block/thread-pool.h
+++ b/include/block/thread-pool.h
@@ -33,12 +33,10 @@ void thread_pool_free(ThreadPool *pool);
* thread_pool_submit* API: submit I/O requests in the thread's
* current AioContext.
*/
-BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
-ThreadPoolFunc *func, void *arg,
-BlockCompletionFunc *cb, void *opaque);
-int coroutine_fn thread_pool_submit_co(ThreadPool *pool,
-ThreadPoolFunc *func, void *arg);
-void thread_pool_submit(ThreadPool *pool, ThreadPoolFunc *func, void *arg);
+BlockAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
+ BlockCompletionFunc *cb, void *opaque);
+int coroutine_fn thread_pool_submit_co(ThreadPoolFunc *func, void *arg);
+void thread_pool_submit(ThreadPoolFunc *func, void *arg);
void thread_pool_update_params(ThreadPool *pool, struct AioContext *ctx);
diff --git a/backends/tpm/tpm_backend.c b/backends/tpm/tpm_backend.c
index 375587e743..485a20b9e0 100644
--- a/backends/tpm/tpm_backend.c
+++ b/backends/tpm/tpm_backend.c
@@ -100,8 +100,6 @@ bool tpm_backend_had_startup_error(TPMBackend *s)
void tpm_backend_deliver_request(TPMBackend *s, TPMBackendCmd *cmd)
{
-ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
-
if (s->cmd != NULL) {
error_report("There is a TPM request pending");
return;
@@ -109,7 +107,7 @@ void tpm_backend_deliver_request(TPMBackend *s,
TPMBackendCmd *cmd)
s->cmd = cmd;
object_ref(OBJECT(s));
-thread_pool_submit_aio(pool, tpm_backend_worker_thread, s,
+thread_pool_submit_aio(tpm_backend_worker_thread, s,
tpm_backend_request_completed, s);
}
diff --git a/block/file-posix.c b/block/file-posix.c
index f7d88fa857..e4c433d071 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2042,9 +2042,7 @@ out:
static int coroutine_fn raw_thread_pool_submit(ThreadPoolFunc func, void *arg)
{
-/* @bs can be NULL, bdrv_get_aio_context() returns the main context then */
-ThreadPool *pool = aio_get_thread_pool(qemu_get_current_aio_context());
-return thread_pool_submit_co(pool, func, arg);
+return thread_pool_submit_co(func, arg);
}
/*
diff --git a/block/file-win32.c b/block/file-win32.c
index 1af6d3c810..c4c9c985c8 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -153,7 +153,6 @@ static BlockAIOCB *paio_submit(BlockDriverState *bs, HANDLE
hfile,
BlockCompletionFunc *cb, void *opaque, int type)
{
RawWin32AIOData *acb = g_new(RawWin32AIOData, 1);
-ThreadPool *pool;
acb->bs = bs;
acb->hfile = hfile;
@@ -168,8 +167,7 @@ static BlockAIOCB *paio_submit(BlockDriverState *bs, HANDLE
hfile,
acb->aio_offset = offset;
trace_file_paio_submit(acb, opaque, offset, count, type);
-pool = aio_get_thread_pool(qemu_get_current_aio_context());
-return thread_pool_submit_aio(pool, aio_worker, acb, cb, opaque);
+return thread_pool_submit_aio(aio_worker, acb, cb, opaque);
}
int qemu_ftruncate64(int fd, int64_t length)
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 6d2e6b7bf4..d6071a1eae 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -43,7 +43,6 @@ qcow2_co_process(BlockDriverState *bs, ThreadPoolFunc *func,
void *arg)
{
int ret;
BDRVQcow2State *s = bs->opaque;
-ThreadPool *pool = aio_get_thread_pool(qemu_get_current_aio_context());
qemu_co_mutex_lock(>lock);
while (s->nb_threads >= QCOW2_MAX_THREADS) {
@@ -52,7 +51,7 @@ qcow2_co_process(BlockDriverState *bs, ThreadPoolFunc *func,
void *arg)
s->nb_threads++;
qemu_co_mutex_unlock(>lock);
-ret = thread_pool_submit_co(pool, func, arg);
+ret = thread_pool_submit_co(func, arg);
qemu_co_mutex_lock(>lock);
s->nb_threads--;
diff --git a/hw/9pfs/coth.c b/hw/9pfs/coth.c
index 2802d41cce
[PATCH 1/2] target/i386: add support for FLUSH_L1D feature
As reported by Intel's doc:
"L1D_FLUSH: Writeback and invalidate the L1 data cache"
If this cpu feature is present in host, allow QEMU to choose whether to
show it to the guest too.
One disadvantage of not exposing it is that the guest will report
a non existing vulnerability in
/sys/devices/system/cpu/vulnerabilities/mmio_stale_data
because the mitigation is present only when the cpu has
(FLUSH_L1D and MD_CLEAR) or FB_CLEAR
features enabled.
Signed-off-by: Emanuele Giuseppe Esposito
---
target/i386/cpu.h | 2 ++
target/i386/cpu.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index d4bc19577a..4948130900 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -889,6 +889,8 @@ uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
#define CPUID_7_0_EDX_SPEC_CTRL (1U << 26)
/* Single Thread Indirect Branch Predictors */
#define CPUID_7_0_EDX_STIBP (1U << 27)
+/* Flush L1D cache */
+#define CPUID_7_0_EDX_FLUSH_L1D (1U << 28)
/* Arch Capabilities */
#define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)
/* Core Capability */
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4d2b8d0444..390120cad8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -858,7 +858,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
"tsx-ldtrk", NULL, NULL /* pconfig */, "arch-lbr",
NULL, NULL, "amx-bf16", "avx512-fp16",
"amx-tile", "amx-int8", "spec-ctrl", "stibp",
-NULL, "arch-capabilities", "core-capability", "ssbd",
+"flush-l1d", "arch-capabilities", "core-capability", "ssbd",
},
.cpuid = {
.eax = 7,
--
2.39.1
[PATCH 0/2] target/i386: add support for cpu FLUSH_L1D feature and FB_CLEAR capability
QEMU should be able to show the guest the above feature/capability, otherwise we risk to have false vulnerability reports in the guest like in /sys/devices/system/cpu/vulnerabilities/mmio_stale_data because the mitigation is present only if the guest supports (FLUSH_L1D and MD_CLEAR) or FB_CLEAR. Emanuele Emanuele Giuseppe Esposito (2): target/i386: add support for FLUSH_L1D feature target/i386: add support for FB_CLEAR feature target/i386/cpu.h | 3 +++ target/i386/cpu.c | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) -- 2.39.1
[PATCH 2/2] target/i386: add support for FB_CLEAR feature
As reported by the Intel's doc:
"FB_CLEAR: The processor will overwrite fill buffer values as part of
MD_CLEAR operations with the VERW instruction.
On these processors, L1D_FLUSH does not overwrite fill buffer values."
If this cpu feature is present in host, allow QEMU to choose whether to
show it to the guest too.
One disadvantage of not exposing it is that the guest will report
a non existing vulnerability in
/sys/devices/system/cpu/vulnerabilities/mmio_stale_data
because the mitigation is present only when the cpu has
(FLUSH_L1D and MD_CLEAR) or FB_CLEAR
features enabled.
Signed-off-by: Emanuele Giuseppe Esposito
---
target/i386/cpu.h | 1 +
target/i386/cpu.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 4948130900..68a6ded0d7 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -975,6 +975,7 @@ uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
#define MSR_ARCH_CAP_PSCHANGE_MC_NO (1U << 6)
#define MSR_ARCH_CAP_TSX_CTRL_MSR (1U << 7)
#define MSR_ARCH_CAP_TAA_NO (1U << 8)
+#define MSR_ARCH_CAP_FB_CLEAR (1U << 17)
#define MSR_CORE_CAP_SPLIT_LOCK_DETECT (1U << 5)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 390120cad8..1405eb42d6 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1010,7 +1010,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
"ssb-no", "mds-no", "pschange-mc-no", "tsx-ctrl",
"taa-no", NULL, NULL, NULL,
NULL, NULL, NULL, NULL,
-NULL, NULL, NULL, NULL,
+NULL, "fb-clear", NULL, NULL,
NULL, NULL, NULL, NULL,
NULL, NULL, NULL, NULL,
NULL, NULL, NULL, NULL,
--
2.39.1
Re: [PATCH 00/13] block: Fix bdrv_open*() calls from coroutine context
Am 26/01/2023 um 18:24 schrieb Kevin Wolf: > bdrv_open*() must not be called from coroutine context, amongst others > because it modifies the block graph. However, some functions - in > particular all .bdrv_co_create* implementations of image formats - do > call it from coroutine context. This is already wrong today, but when we > add locking, it actually becomes visible. > > This series adds no_co_wrapper functions, which are automatically > generated wrappers that run in coroutine context and use a BH to call > the wrapped function outside of coroutine context. It then uses these > wrappers to fix the problematic bdrv_open*() calls. > > Kevin Wolf (13): > block-coroutine-wrapper: Introduce no_co_wrapper > block: Create no_co_wrappers for open functions > luks: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper > parallels: Fix .bdrv_co_create(_opts) to open images with > no_co_wrapper > qcow: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper > qcow2: Fix open/create to open images with no_co_wrapper > qed: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper > vdi: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper > vhdx: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper > vmdk: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper > vpc: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper > block: Fix bdrv_co_create_opts_simple() to open images with > no_co_wrapper > block: Assert non-coroutine context for bdrv_open_inherit() > Apart from a small nitpick in patch 3 where the functions are not marked as coroutine_fn (but I think this is because BDS callbacks usually don't have such annotations), looks good to me. Reviewed-by: Emanuele Giuseppe Esposito > include/block/block-common.h| 14 > include/block/block-global-state.h | 35 ++--- > include/sysemu/block-backend-global-state.h | 21 +- > block.c | 17 ++--- > block/crypto.c | 10 +-- > block/parallels.c | 10 +-- > block/qcow.c| 10 +-- > block/qcow2.c | 43 +-- > block/qed.c | 10 +-- > block/vdi.c | 10 +-- > block/vhdx.c| 10 +-- > block/vmdk.c| 22 +++--- > block/vpc.c | 10 +-- > scripts/block-coroutine-wrapper.py | 83 ++--- > block/meson.build | 1 + > 15 files changed, 207 insertions(+), 99 deletions(-) >
Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings
Am 17/01/2023 um 18:17 schrieb Kevin Wolf: > Am 17.01.2023 um 17:43 hat Warner Losh geschrieben: >> On Tue, Jan 17, 2023 at 9:25 AM Kevin Wolf wrote: >> >>> Am 17.01.2023 um 17:16 hat Warner Losh geschrieben: >>>> On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito < >>>> eespo...@redhat.com> wrote: >>>> >>>>> QEMU does not compile when enabling clang's thread safety analysis >>>>> (TSA), >>>>> because some functions create wrappers for pthread mutexes but do >>>>> not use any TSA macro. Therefore the compiler fails. >>>>> >>>>> In order to make the compiler happy and avoid adding all the >>>>> necessary macros to all callers (lock functions should use >>>>> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of >>>>> pthread_mutex_lock/pthread_mutex_unlock), >>>>> simply use TSA_NO_TSA to supppress such warnings. >>>> >>>> I'm not sure I understand this quite right. Maybe a clarifying question >>>> will help me understand: Why is this needed for bsd-user but not >>>> linux-user? How are they different here? >>> >>> FreeBSD's pthread headers include TSA annotations for some functions >>> that force us to do something about them (for now: suppress the warnings >>> in their callers) before we can enable -Wthread-safety for the purposes >>> where we really want it. Without this, calling functions like >>> pthread_mutex_lock() would cause compiler errors. >>> >>> glibc's headers don't contain such annotations, so the same is not >>> necessary on Linux >>> >> >> Thanks Kevin. With that explanation, these patches and their explanation >> make perfect sense now. Often when there's a patch to bsd-user but not >> linux-user, it's because bsd-user needs to do more in some way (which I try >> to keep up on). >> >> In this case, it's because FreeBSD's libc is a bit ahead of the curve. So I >> understand why it's needed, and what I need to do next (though I think that >> I may have to wait for the rest of qemu to be annotated)... > > I assume that the bsd-user part is actually sufficiently independent > that you could do proper annotations there if you want. > > However, be aware that TSA has some serious limitations with C, so you > can't express certain things, and it isn't as strict as it could be (in > particular, function pointers bypass it). As long as you have global > locks (as opposed to locks in structs), it kind of works, though. > Certainly better than nothing. > > But it probably means that some of the rest of QEMU may never get the > annotations. Also, our primary goal is protecting the block layer, so > someone else would have to work on other locks. With checks disabled on > individual functions like in this series, it should at least be possible > to work on it incrementally. > >> It might be better, though, to put some of this information in the commit >> message so it isn't just on the mailing list. > > Yes, I agree. We can tweak the commit messages before merging it. New proposed commit message: bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings in FreeBSD FreeBSD implements pthread headers using TSA (thread safety analysis) annotations, therefore when an application is compiled with -Wthread-safety there are some locking/annotation requirements that the user of the pthread API has to follow. This will also be the case in QEMU, since bsd-user/mmap.c uses the pthread API. Therefore when building it with -Wthread-safety the compiler will throw warnings because the functions are not properly annotated. We need TSA to be enabled because it ensures that the critical sections of an annotated variable are properly locked. In order to make the compiler happy and avoid adding all the necessary macros to all callers (lock functions should use TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all users of pthread_mutex_lock/pthread_mutex_unlock), simply use TSA_NO_TSA to supppress such warnings. Signed-off-by: Emanuele Giuseppe Esposito Same message could be applied to patch 1, substituting bsd-user/mmap with util/qemu-thread-posix. Thank you, Emanuele > >> Just a suggestion: >> >> Reviewed-by: Warner Losh > > Thanks! > > Kevin >
Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings
Am 17/01/2023 um 17:16 schrieb Warner Losh: > > > On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito > mailto:eespo...@redhat.com>> wrote: > > QEMU does not compile when enabling clang's thread safety analysis > (TSA), > because some functions create wrappers for pthread mutexes but do > not use any TSA macro. Therefore the compiler fails. > > In order to make the compiler happy and avoid adding all the > necessary macros to all callers (lock functions should use > TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers > of pthread_mutex_lock/pthread_mutex_unlock), > simply use TSA_NO_TSA to supppress such warnings. > > > I'm not sure I understand this quite right. Maybe a clarifying question > will help me understand: Why is this needed for bsd-user but not > linux-user? How are they different here? > Honestly I just went and fix the warnings that the compiler was throwing at me. On FreeBSD it complains on bsd-user/mmap.c, but apparently the CI or even compiling locally in linux doesn't create any issue with linux-user. Weird. But I guess it won't hurt to add TSA_NO_TSA there too. Emanuele > Warner > > > Signed-off-by: Emanuele Giuseppe Esposito <mailto:eespo...@redhat.com>> > --- > bsd-user/qemu.h | 5 +++-- > include/exec/exec-all.h | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h > index be6105385e..711fdd1b64 100644 > --- a/bsd-user/qemu.h > +++ b/bsd-user/qemu.h > @@ -37,6 +37,7 @@ extern char **environ; > #include "target_os_signal.h" > #include "target.h" > #include "exec/gdbstub.h" > +#include "qemu/clang-tsa.h" > > /* > * This struct is used to hold certain information about the > image. Basically, > @@ -235,8 +236,8 @@ int target_msync(abi_ulong start, abi_ulong len, > int flags); > extern unsigned long last_brk; > extern abi_ulong mmap_next_start; > abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size); > -void mmap_fork_start(void); > -void mmap_fork_end(int child); > +void TSA_NO_TSA mmap_fork_start(void); > +void TSA_NO_TSA mmap_fork_end(int child); > > /* main.c */ > extern char qemu_proc_pathname[]; > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 25e11b0a8d..4f0c0559ac 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -25,6 +25,7 @@ > #include "exec/cpu_ldst.h" > #endif > #include "qemu/interval-tree.h" > +#include "qemu/clang-tsa.h" > > /* allow to see translation results - the slowdown should be > negligible, so we leave it */ > #define DEBUG_DISAS > @@ -758,8 +759,8 @@ static inline tb_page_addr_t > get_page_addr_code(CPUArchState *env, > } > > #if defined(CONFIG_USER_ONLY) > -void mmap_lock(void); > -void mmap_unlock(void); > +void TSA_NO_TSA mmap_lock(void); > +void TSA_NO_TSA mmap_unlock(void); > bool have_mmap_lock(void); > > /** > -- > 2.39.0 >
Re: [PATCH 1/3] util/qemu-thread-posix: use TSA_NO_TSA to suppress clang TSA warnings
Am 17/01/2023 um 15:33 schrieb Philippe Mathieu-Daudé: > On 17/1/23 14:52, Emanuele Giuseppe Esposito wrote: >> QEMU does not compile when enabling clang's thread safety analysis >> (TSA), >> because some functions create wrappers for pthread mutexes but do >> not use any TSA macro. Therefore the compiler fails. >> >> In order to make the compiler happy and avoid adding all the >> necessary macros to all callers (lock functions should use >> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all >> users of pthread_mutex_lock/pthread_mutex_unlock), >> simply use TSA_NO_TSA to supppress such warnings. >> >> Signed-off-by: Emanuele Giuseppe Esposito >> --- >> include/qemu/thread.h | 14 +- >> util/qemu-thread-posix.c | 2 +- >> 2 files changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/include/qemu/thread.h b/include/qemu/thread.h >> index 7c6703bce3..81ec9fc144 100644 >> --- a/include/qemu/thread.h >> +++ b/include/qemu/thread.h >> @@ -3,6 +3,7 @@ >> #include "qemu/processor.h" >> #include "qemu/atomic.h" >> +#include "qemu/clang-tsa.h" > > Missing file? ^^ > ? Forgot to pull latest changes? I see clang-tsa.h in master
Re: [PATCH 3/3] configure: Enable -Wthread-safety if present
Am 17/01/2023 um 15:02 schrieb Daniel P. Berrangé: > On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote: >> From: Kevin Wolf >> >> This enables clang's thread safety analysis (TSA), which we'll use to >> statically check the block graph locking. >> >> Signed-off-by: Kevin Wolf >> Message-Id: <20221207131838.239125-9-kw...@redhat.com> >> Reviewed-by: Emanuele Giuseppe Esposito >> Signed-off-by: Kevin Wolf >> --- >> configure | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/configure b/configure >> index 2281892657..14668e6269 100755 >> --- a/configure >> +++ b/configure >> @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs >> add_to warn_flags -Wendif-labels >> add_to warn_flags -Wexpansion-to-defined >> add_to warn_flags -Wimplicit-fallthrough=2 >> +add_to warn_flags -Wthread-safety > > Does this thread safety analysis have any kind of measurable > impact on compilation speed ? > > Our CI jobs are quite sensitive to any increase in build > time. >From a quick run in my machine (Dell PowerEdge R440 with Intel(R) Xeon(R) Gold 5120 CPU @ 2.20GHz, 28 cpus): without clang: real2m46.729s user19m24.122s sys 2m58.643s with clang: real2m45.204s user19m52.096s sys 2m9.036s So there should be no significative impact. I also forgot to mention that this serie fixes the CI failure seen in: https://gitlab.com/qemu-project/qemu/-/jobs/3479763741 https://gitlab.com/qemu-project/qemu/-/jobs/3479763746 Thank you, Emanuele > > > With regards, > Daniel
[PATCH 3/3] configure: Enable -Wthread-safety if present
From: Kevin Wolf This enables clang's thread safety analysis (TSA), which we'll use to statically check the block graph locking. Signed-off-by: Kevin Wolf Message-Id: <20221207131838.239125-9-kw...@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf --- configure | 1 + 1 file changed, 1 insertion(+) diff --git a/configure b/configure index 2281892657..14668e6269 100755 --- a/configure +++ b/configure @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs add_to warn_flags -Wendif-labels add_to warn_flags -Wexpansion-to-defined add_to warn_flags -Wimplicit-fallthrough=2 +add_to warn_flags -Wthread-safety nowarn_flags= add_to nowarn_flags -Wno-initializer-overrides -- 2.39.0
[PATCH 1/3] util/qemu-thread-posix: use TSA_NO_TSA to suppress clang TSA warnings
QEMU does not compile when enabling clang's thread safety analysis
(TSA),
because some functions create wrappers for pthread mutexes but do
not use any TSA macro. Therefore the compiler fails.
In order to make the compiler happy and avoid adding all the
necessary macros to all callers (lock functions should use
TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all
users of pthread_mutex_lock/pthread_mutex_unlock),
simply use TSA_NO_TSA to supppress such warnings.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/qemu/thread.h| 14 +-
util/qemu-thread-posix.c | 2 +-
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 7c6703bce3..81ec9fc144 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -3,6 +3,7 @@
#include "qemu/processor.h"
#include "qemu/atomic.h"
+#include "qemu/clang-tsa.h"
typedef struct QemuCond QemuCond;
typedef struct QemuSemaphore QemuSemaphore;
@@ -24,9 +25,12 @@ typedef struct QemuThread QemuThread;
void qemu_mutex_init(QemuMutex *mutex);
void qemu_mutex_destroy(QemuMutex *mutex);
-int qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file, const int
line);
-void qemu_mutex_lock_impl(QemuMutex *mutex, const char *file, const int line);
-void qemu_mutex_unlock_impl(QemuMutex *mutex, const char *file, const int
line);
+int TSA_NO_TSA qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file,
+ const int line);
+void TSA_NO_TSA qemu_mutex_lock_impl(QemuMutex *mutex, const char *file,
+ const int line);
+void TSA_NO_TSA qemu_mutex_unlock_impl(QemuMutex *mutex, const char *file,
+ const int line);
void qemu_rec_mutex_init(QemuRecMutex *mutex);
void qemu_rec_mutex_destroy(QemuRecMutex *mutex);
@@ -153,8 +157,8 @@ void qemu_cond_destroy(QemuCond *cond);
*/
void qemu_cond_signal(QemuCond *cond);
void qemu_cond_broadcast(QemuCond *cond);
-void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex,
- const char *file, const int line);
+void TSA_NO_TSA qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex,
+const char *file, const int line);
bool qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
const char *file, const int line);
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index bae938c670..2dd1069cd3 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -223,7 +223,7 @@ void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex,
const char *file, con
error_exit(err, __func__);
}
-static bool
+static bool TSA_NO_TSA
qemu_cond_timedwait_ts(QemuCond *cond, QemuMutex *mutex, struct timespec *ts,
const char *file, const int line)
{
--
2.39.0
[PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings
QEMU does not compile when enabling clang's thread safety analysis (TSA), because some functions create wrappers for pthread mutexes but do not use any TSA macro. Therefore the compiler fails. In order to make the compiler happy and avoid adding all the necessary macros to all callers (lock functions should use TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of pthread_mutex_lock/pthread_mutex_unlock), simply use TSA_NO_TSA to supppress such warnings. Signed-off-by: Emanuele Giuseppe Esposito --- bsd-user/qemu.h | 5 +++-- include/exec/exec-all.h | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h index be6105385e..711fdd1b64 100644 --- a/bsd-user/qemu.h +++ b/bsd-user/qemu.h @@ -37,6 +37,7 @@ extern char **environ; #include "target_os_signal.h" #include "target.h" #include "exec/gdbstub.h" +#include "qemu/clang-tsa.h" /* * This struct is used to hold certain information about the image. Basically, @@ -235,8 +236,8 @@ int target_msync(abi_ulong start, abi_ulong len, int flags); extern unsigned long last_brk; extern abi_ulong mmap_next_start; abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size); -void mmap_fork_start(void); -void mmap_fork_end(int child); +void TSA_NO_TSA mmap_fork_start(void); +void TSA_NO_TSA mmap_fork_end(int child); /* main.c */ extern char qemu_proc_pathname[]; diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 25e11b0a8d..4f0c0559ac 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -25,6 +25,7 @@ #include "exec/cpu_ldst.h" #endif #include "qemu/interval-tree.h" +#include "qemu/clang-tsa.h" /* allow to see translation results - the slowdown should be negligible, so we leave it */ #define DEBUG_DISAS @@ -758,8 +759,8 @@ static inline tb_page_addr_t get_page_addr_code(CPUArchState *env, } #if defined(CONFIG_USER_ONLY) -void mmap_lock(void); -void mmap_unlock(void); +void TSA_NO_TSA mmap_lock(void); +void TSA_NO_TSA mmap_unlock(void); bool have_mmap_lock(void); /** -- 2.39.0
[PATCH 0/3] TSA: make sure QEMU compiles when using clang TSA
This serie aims to enable clang Thread Safety Analysis (TSA) in QEMU. The goal is to use it for our multiqueue project aiming to replace the block layer AioContext lock with a rwlock and make sure the lock is taken correctly everywhere [1]. By default, TSA covers pthread mutexes, therefore when added in QEMU it immediately detects some wrappers using pthread_mutex_lock/unlock without using the proper TSA macros. Since adding such macro requires scanning all possible callers of the affected wrapper, simply use TSA_NO_TSA to suppress the warnings. [1] = https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg00903.html Emanuele Giuseppe Esposito (2): util/qemu-thread-posix: use TSA_NO_TSA to suppress clang TSA warnings bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings Kevin Wolf (1): configure: Enable -Wthread-safety if present configure| 1 + bsd-user/qemu.h | 5 +++-- include/exec/exec-all.h | 5 +++-- include/qemu/thread.h| 14 +- util/qemu-thread-posix.c | 2 +- 5 files changed, 17 insertions(+), 10 deletions(-) -- 2.39.0
Re: [PATCH v2 00/14] block: Move more functions to coroutines
Am 13/01/2023 um 21:41 schrieb Kevin Wolf: > This series converts some IO_CODE() functions to coroutine_fn because > they access the graph and will need to hold the graph lock in the > future. IO_CODE() functions can be called from iothreads, so taking the > graph lock requires the function to run in coroutine context. > > Pretty much all of the changes in this series were posted by Emanuele > before as part of "Protect the block layer with a rwlock: part 3". The > major difference is that in the old version, the patches did two things > at once: Converting functions to coroutine_fn, and adding the locking to > them. This series does only the coroutine conversion. The locking part > will be in another series which now comes with TSA annotations and makes > the locking related changes big enough to have separate patches. > > v2: > - In each patch converting a BlockDriver callback to be called in > coroutine, also immediately rename it and its implementation to > include co_ in its name, as well as mark the implementations > coroutine_fn [Vladimir] > - Moved bdrv_is_inserted() earlier in the series because > raw_is_inserted() calls raw_getlength(), so it needs to be converted > first to avoid calling a coroutine_fn from a non-coroutine_fn in an > intermediate state. > - The final patch only renames bdrv_load/save_vmstate() any more, which > was already converted to coroutine_fn earlier. > - Since pretty much every patch was touched in this, I refrained from > picking up any Reviewed-by for v1 > Reviewed-by: Emanuele Giuseppe Esposito > Emanuele Giuseppe Esposito (14): > block-coroutine-wrapper: support void functions > block: Convert bdrv_io_plug() to co_wrapper > block: Convert bdrv_io_unplug() to co_wrapper > block: Convert bdrv_is_inserted() to co_wrapper > block: Rename refresh_total_sectors to bdrv_refresh_total_sectors > block: Convert bdrv_refresh_total_sectors() to co_wrapper_mixed > block-backend: use bdrv_getlength instead of blk_getlength > block: use bdrv_co_refresh_total_sectors when possible > block: Convert bdrv_get_allocated_file_size() to co_wrapper > block: Convert bdrv_get_info() to co_wrapper_mixed > block: Convert bdrv_eject() to co_wrapper > block: Convert bdrv_lock_medium() to co_wrapper > block: Convert bdrv_debug_event() to co_wrapper_mixed > block: Rename bdrv_load/save_vmstate() to bdrv_co_load/save_vmstate() > > include/block/block-io.h | 36 +++--- > include/block/block_int-common.h | 26 --- > include/block/block_int-io.h | 5 +- > include/sysemu/block-backend-io.h | 31 +++-- > block.c| 82 +- > block/blkdebug.c | 11 +-- > block/blkio.c | 15 ++-- > block/blklogwrites.c | 6 +- > block/blkreplay.c | 6 +- > block/blkverify.c | 6 +- > block/block-backend.c | 36 +- > block/commit.c | 4 +- > block/copy-on-read.c | 18 ++--- > block/crypto.c | 14 ++-- > block/curl.c | 10 +-- > block/file-posix.c | 107 ++--- > block/file-win32.c | 18 +++-- > block/filter-compress.c| 20 +++--- > block/gluster.c| 23 --- > block/io.c | 76 ++-- > block/iscsi.c | 17 ++--- > block/mirror.c | 6 +- > block/nbd.c| 8 +-- > block/nfs.c| 4 +- > block/null.c | 13 ++-- > block/nvme.c | 14 ++-- > block/preallocate.c| 16 ++--- > block/qcow.c | 5 +- > block/qcow2-refcount.c | 2 +- > block/qcow2.c | 17 ++--- > block/qed.c| 11 +-- > block/quorum.c | 8 +-- > block/raw-format.c | 25 +++ > block/rbd.c| 9 +-- > block/replication.c| 6 +- > block/ssh.c| 4 +- > block/throttle.c | 6 +- > block/vdi.c| 7 +- > block/vhdx.c | 5 +- > block/vmdk.c | 14 ++-- > block/vpc.c| 5 +- > blockdev.c | 8 ++- > hw/scsi/scsi-disk.c| 5 ++ > tests/unit/test-block-iothread.c | 3 + > scripts/block-coroutine-wrapper.py | 20 -- > block/meson.build | 1 + > 46 files changed, 443 insertions(+), 346 deletions(-) >
Re: [PATCH 06/14] block-backend: use bdrv_getlength instead of blk_getlength
Am 16/12/2022 um 18:22 schrieb Vladimir Sementsov-Ogievskiy:
> On 12/13/22 11:53, Kevin Wolf wrote:
>> From: Emanuele Giuseppe Esposito
>>
>> The only difference is that blk_ checks if the block is available,
>> but this check is already performed above in blk_check_byte_request().
>>
>> This is in preparation for the graph rdlock, which will be taken
>> by both the callers of blk_check_byte_request() and blk_getlength().
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> Signed-off-by: Kevin Wolf
>> ---
>> block/block-backend.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 0194d86113..5b8da86772 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -1253,7 +1253,7 @@ static int blk_check_byte_request(BlockBackend
>> *blk, int64_t offset,
>> }
>> if (!blk->allow_write_beyond_eof) {
>> - len = blk_getlength(blk);
>> + len = bdrv_getlength(blk_bs(blk));
>
> I understand the reasoning, but the change looks like a degradation..
> bdrv_* functions becomes kind of *_locked() ? If we are going to
> introduce a lot of such changes, that's not good. But this one is not a
> problem of course.
Nope, that's the only one (you can check my previous series "part 1",
"part 2" and "part 3" to have an idea on what is coming).
Thank you,
Emanuele
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
>> if (len < 0) {
>> return len;
>> }
>
Re: [PATCH 02/14] block: Convert bdrv_io_plug() to co_wrapper
Am 16/12/2022 um 17:12 schrieb Vladimir Sementsov-Ogievskiy:
> On 12/13/22 11:53, Kevin Wolf wrote:
>> --- a/include/block/block_int-common.h
>> +++ b/include/block/block_int-common.h
>> @@ -729,7 +729,7 @@ struct BlockDriver {
>> void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent
>> event);
>> /* io queue for linux-aio */
>> - void (*bdrv_io_plug)(BlockDriverState *bs);
>> + void coroutine_fn (*bdrv_io_plug)(BlockDriverState *bs);
>
> But you didn't add coroutine_fn to realizations of this callback. Seems,
> it should be done
>
This has to be done by Paolo & Alberto Faria work to add missing
coroutine_fn and various annotations in a systematic way. It has to be
done using tools like vrc, and not manually because it's not feasible.
Not covered by this serie.
Thank you,
Emanuele
Re: [PATCH 02/14] block: Convert bdrv_io_plug() to co_wrapper
Am 16/12/2022 um 15:26 schrieb Vladimir Sementsov-Ogievskiy:
> On 12/13/22 11:53, Kevin Wolf wrote:
>> From: Emanuele Giuseppe Esposito
>>
>> BlockDriver->bdrv_io_plug is categorized as IO callback, and it
>> currently doesn't run in a coroutine. We should let it take a graph
>> rdlock since the callback traverses the block nodes graph, which however
>> is only possible in a coroutine.
>>
>> The only caller of this function is blk_io_plug(), therefore make
>> blk_io_plug() a co_wrapper, so that we're always running in a coroutine
>> where the lock can be taken.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> Signed-off-by: Kevin Wolf
>> ---
>
> [..]
>
>> --- a/include/block/block_int-common.h
>> +++ b/include/block/block_int-common.h
>> @@ -729,7 +729,7 @@ struct BlockDriver {
>> void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent
>> event);
>> /* io queue for linux-aio */
>> - void (*bdrv_io_plug)(BlockDriverState *bs);
>> + void coroutine_fn (*bdrv_io_plug)(BlockDriverState *bs);
>
> don't we want to rename it to _co_ too?
I think you realized this is done in patch 14
Thanks,
Emanuele
>
> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
>
>
Re: [PATCH 00/14] block: Move more functions to coroutines
Am 13/12/2022 um 09:53 schrieb Kevin Wolf: > This series converts some IO_CODE() functions to coroutine_fn because > they access the graph and will need to hold the graph lock in the > future. IO_CODE() functions can be called from iothreads, so taking the > graph lock requires the function to run in coroutine context. > > Pretty much all of the changes in this series were posted by Emanuele > before as part of "Protect the block layer with a rwlock: part 3". The > major difference is that in the old version, the patches did two things > at once: Converting functions to coroutine_fn, and adding the locking to > them. This series does only the coroutine conversion. The locking part > will be in another series which now comes with TSA annotations and makes > the locking related changes big enough to have separate patches. > Reviewed-by: Emanuele Giuseppe Esposito > Emanuele Giuseppe Esposito (14): > block-coroutine-wrapper: support void functions > block: Convert bdrv_io_plug() to co_wrapper > block: Convert bdrv_io_unplug() to co_wrapper > block: Rename refresh_total_sectors to bdrv_refresh_total_sectors > block: Convert bdrv_refresh_total_sectors() to co_wrapper_mixed > block-backend: use bdrv_getlength instead of blk_getlength > block: use bdrv_co_refresh_total_sectors when possible > block: Convert bdrv_get_allocated_file_size() to co_wrapper > block: Convert bdrv_get_info() to co_wrapper_mixed > block: Convert bdrv_is_inserted() to co_wrapper > block: Convert bdrv_eject() to co_wrapper > block: convert bdrv_lock_medium in co_wrapper > block: Convert bdrv_debug_event to co_wrapper_mixed > block: Rename newly converted BlockDriver IO coroutine functions > > include/block/block-io.h | 36 + > include/block/block_int-common.h | 26 ++ > include/block/block_int-io.h | 5 +- > include/sysemu/block-backend-io.h | 31 --- > block.c| 82 ++ > block/blkdebug.c | 4 +- > block/blkio.c | 6 +-- > block/blklogwrites.c | 2 +- > block/blkreplay.c | 2 +- > block/blkverify.c | 2 +- > block/block-backend.c | 36 ++--- > block/commit.c | 4 +- > block/copy-on-read.c | 12 ++--- > block/crypto.c | 6 +-- > block/curl.c | 8 +-- > block/file-posix.c | 48 - > block/file-win32.c | 12 ++--- > block/filter-compress.c| 10 ++-- > block/gluster.c| 16 +++--- > block/io.c | 76 +-- > block/iscsi.c | 8 +-- > block/mirror.c | 6 +-- > block/nbd.c| 6 +-- > block/nfs.c| 2 +- > block/null.c | 8 +-- > block/nvme.c | 6 +-- > block/preallocate.c| 2 +- > block/qcow.c | 2 +- > block/qcow2-refcount.c | 2 +- > block/qcow2.c | 6 +-- > block/qed.c| 4 +- > block/quorum.c | 2 +- > block/raw-format.c | 14 ++--- > block/rbd.c| 4 +- > block/replication.c| 2 +- > block/ssh.c| 2 +- > block/throttle.c | 2 +- > block/vdi.c| 2 +- > block/vhdx.c | 2 +- > block/vmdk.c | 4 +- > block/vpc.c| 2 +- > blockdev.c | 8 ++- > hw/scsi/scsi-disk.c| 5 ++ > tests/unit/test-block-iothread.c | 3 ++ > scripts/block-coroutine-wrapper.py | 20 ++-- > block/meson.build | 1 + > 46 files changed, 316 insertions(+), 233 deletions(-) >
Re: [RFC PATCH] test-bdrv-drain: keep graph manipulations out of coroutines
Am 09/12/2022 um 13:18 schrieb Emanuele Giuseppe Esposito:
>
>
> Am 05/12/2022 um 14:01 schrieb Kevin Wolf:
>> Am 02.12.2022 um 18:22 hat Paolo Bonzini geschrieben:
>>> On 12/2/22 14:42, Emanuele Giuseppe Esposito wrote:
>>>>
>>>>
>>>> Am 02/12/2022 um 14:27 schrieb Paolo Bonzini:
>>>>> Changes to the BlockDriverState graph will have to take the
>>>>> corresponding lock for writing, and therefore cannot be done
>>>>> inside a coroutine. Move them outside the test body.
>>>>>
>>>>> Signed-off-by: Paolo Bonzini
>>>>> ---
>>>>> tests/unit/test-bdrv-drain.c | 63 ++--
>>>>> 1 file changed, 46 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
>>>>> index 6ae44116fe79..d85083dd4f9e 100644
>>>>> --- a/tests/unit/test-bdrv-drain.c
>>>>> +++ b/tests/unit/test-bdrv-drain.c
>>>>> @@ -199,25 +199,40 @@ static void do_drain_end_unlocked(enum drain_type
>>>>> drain_type, BlockDriverState *
>>>>> }
>>>>> }
>>>>> +static BlockBackend *blk;
>>>>> +static BlockDriverState *bs, *backing;
>>>>> +
>>>>> +static void test_drv_cb_init(void)
>>>>> +{
>>>>> +blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
>>>>> +bs = bdrv_new_open_driver(_test, "test-node", BDRV_O_RDWR,
>>>>> + _abort);
>>>>> +blk_insert_bs(blk, bs, _abort);
>>>>> +
>>>>> +backing = bdrv_new_open_driver(_test, "backing", 0,
>>>>> _abort);
>>>>> +bdrv_set_backing_hd(bs, backing, _abort);
>>>>> +}
>>>>> +
>>>>> +static void test_drv_cb_fini(void)
>>>>
>>>> fini stands for "finito"? :)
>>>
>>> No, for finish :)
>>> http://ftp.math.utah.edu/u/ma/hohn/linux/misc/elf/node3.html
>>>
>>>> Anyways, an alternative solution for this is also here (probably coming
>>>> from you too):
>>>> https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03517.html
>>>
>>> Much better. At least patches 7-8 from that series have to be salvaged,
>>> possibly 10 as well.
>>
>> I wonder if we need a more general solution for this because this test
>> is not the only place that calls this kind of functions in a coroutine.
>> The one I'm aware of in particular is all the .bdrv_co_create
>> implementations, but I'm almost sure there are more.
>>
>> Can we use a yield_to_drain()-like mechanism for these functions? Maybe
>> even something like the opposite of co_wrapper, a no_co_wrapper that
>> generates a foo_co() variant that drops out of coroutine context before
>> calling foo()?
>>
>
> I implemented something like yield_to_drain as you suggested, but when
> thinking about it aren't we making a fix that will cost us even more
> work in the future? If we use a yield_to_drain-like function, we are
> doing something similar to g_c_w, and losing track of whether the caller
> is a coroutine or not. And the function could then be used potentially
> everywhere. Then we will realize "oh we need to get rid of this and
> split the functions differentiating the coroutine context" and
> eventually go through ALL the callers again to figure what is doing
> what, and implement the same fix of this patch or my series once again.
>
> Instead, even though this is just a test, we have a clear separation and
> one less case to worry about in the future.
>
At least the above is valid if the change you are proposing is the
following (tested already, works)
diff --git a/block.c b/block.c
index 6191ac1f44..8d28c1daa4 100644
--- a/block.c
+++ b/block.c
@@ -42,6 +42,7 @@
#include "qapi/qobject-output-visitor.h"
#include "qapi/qapi-visit-block-core.h"
#include "sysemu/block-backend.h"
+#include "sysemu/replay.h"
#include "qemu/notify.h"
#include "qemu/option.h"
#include "qemu/coroutine.h"
@@ -2831,6 +2832,94 @@ uint64_t
bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
return permissions[qapi_perm];
}
+typedef struct {
+Coroutine *co;
+BlockDriverState *new_bs;
+BdrvChild *child;
+AioContext *ctx;
+bool done;
+} BdrvCoGraphModData;
+
+static void bdrv_co_graph_mod_bh_cb(void *opaque)
+{
+BdrvCo
Re: [RFC PATCH] test-bdrv-drain: keep graph manipulations out of coroutines
Am 05/12/2022 um 14:01 schrieb Kevin Wolf:
> Am 02.12.2022 um 18:22 hat Paolo Bonzini geschrieben:
>> On 12/2/22 14:42, Emanuele Giuseppe Esposito wrote:
>>>
>>>
>>> Am 02/12/2022 um 14:27 schrieb Paolo Bonzini:
>>>> Changes to the BlockDriverState graph will have to take the
>>>> corresponding lock for writing, and therefore cannot be done
>>>> inside a coroutine. Move them outside the test body.
>>>>
>>>> Signed-off-by: Paolo Bonzini
>>>> ---
>>>> tests/unit/test-bdrv-drain.c | 63 ++--
>>>> 1 file changed, 46 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
>>>> index 6ae44116fe79..d85083dd4f9e 100644
>>>> --- a/tests/unit/test-bdrv-drain.c
>>>> +++ b/tests/unit/test-bdrv-drain.c
>>>> @@ -199,25 +199,40 @@ static void do_drain_end_unlocked(enum drain_type
>>>> drain_type, BlockDriverState *
>>>> }
>>>> }
>>>> +static BlockBackend *blk;
>>>> +static BlockDriverState *bs, *backing;
>>>> +
>>>> +static void test_drv_cb_init(void)
>>>> +{
>>>> +blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
>>>> +bs = bdrv_new_open_driver(_test, "test-node", BDRV_O_RDWR,
>>>> + _abort);
>>>> +blk_insert_bs(blk, bs, _abort);
>>>> +
>>>> +backing = bdrv_new_open_driver(_test, "backing", 0,
>>>> _abort);
>>>> +bdrv_set_backing_hd(bs, backing, _abort);
>>>> +}
>>>> +
>>>> +static void test_drv_cb_fini(void)
>>>
>>> fini stands for "finito"? :)
>>
>> No, for finish :)
>> http://ftp.math.utah.edu/u/ma/hohn/linux/misc/elf/node3.html
>>
>>> Anyways, an alternative solution for this is also here (probably coming
>>> from you too):
>>> https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03517.html
>>
>> Much better. At least patches 7-8 from that series have to be salvaged,
>> possibly 10 as well.
>
> I wonder if we need a more general solution for this because this test
> is not the only place that calls this kind of functions in a coroutine.
> The one I'm aware of in particular is all the .bdrv_co_create
> implementations, but I'm almost sure there are more.
>
> Can we use a yield_to_drain()-like mechanism for these functions? Maybe
> even something like the opposite of co_wrapper, a no_co_wrapper that
> generates a foo_co() variant that drops out of coroutine context before
> calling foo()?
>
I implemented something like yield_to_drain as you suggested, but when
thinking about it aren't we making a fix that will cost us even more
work in the future? If we use a yield_to_drain-like function, we are
doing something similar to g_c_w, and losing track of whether the caller
is a coroutine or not. And the function could then be used potentially
everywhere. Then we will realize "oh we need to get rid of this and
split the functions differentiating the coroutine context" and
eventually go through ALL the callers again to figure what is doing
what, and implement the same fix of this patch or my series once again.
Instead, even though this is just a test, we have a clear separation and
one less case to worry about in the future.
Thank you,
Emanuele
Re: [PATCH 00/18] block: Introduce a block graph rwlock
Am 07/12/2022 um 14:18 schrieb Kevin Wolf: > This series supersedes the first half of Emanuele's "Protect the block > layer with a rwlock: part 1". It introduces the basic infrastructure for > protecting the block graph (specifically parent/child links) with a > rwlock. Actually taking the reader lock in all necessary places is left > for future series. > > Compared to Emanuele's series, this one adds patches to make use of > clang's Thread Safety Analysis (TSA) feature in order to statically > check at compile time that the places where we assert that we hold the > lock actually do hold it. Once we cover all relevant places, the check > can be extended to verify that all accesses of bs->children and > bs->parents hold the lock. > > For reference, here is the more detailed version of our plan in > Emanuele's words from his series: > > The aim is to replace the current AioContext lock with much > fine-grained locks, aimed to protect only specific data. Currently > the AioContext lock is used pretty much everywhere, and it's not > even clear what it is protecting exactly. > > The aim of the rwlock is to cover graph modifications: more > precisely, when a BlockDriverState parent or child list is modified > or read, since it can be concurrently accessed by the main loop and > iothreads. > > The main assumption is that the main loop is the only one allowed to > perform graph modifications, and so far this has always been held by > the current code. > > The rwlock is inspired from cpus-common.c implementation, and aims > to reduce cacheline bouncing by having per-aiocontext counter of > readers. All details and implementation of the lock are in patch 2. > > We distinguish between writer (main loop, under BQL) that modifies > the graph, and readers (all other coroutines running in various > AioContext), that go through the graph edges, reading ->parents > and->children. The writer (main loop) has an "exclusive" access, > so it first waits for current read to finish, and then prevents > incoming ones from entering while it has the exclusive access. The > readers (coroutines in multiple AioContext) are free to access the > graph as long the writer is not modifying the graph. In case it is, > they go in a CoQueue and sleep until the writer is done. > > In this and following series, we try to follow the following locking > pattern: > > - bdrv_co_* functions that call BlockDriver callbacks always expect > the lock to be taken, therefore they assert. > > - blk_co_* functions are called from external code outside the block > layer, which should not have to care about the block layer's > internal locking. Usually they also call blk_wait_while_drained(). > Therefore they take the lock internally. > > The long term goal of this series is to eventually replace the > AioContext lock, so that we can get rid of it once and for all. > > Emanuele Giuseppe Esposito (7): > graph-lock: Implement guard macros > async: Register/unregister aiocontext in graph lock list > block: wrlock in bdrv_replace_child_noperm > block: remove unnecessary assert_bdrv_graph_writable() > block: assert that graph read and writes are performed correctly > block-coroutine-wrapper.py: introduce annotations that take the graph > rdlock > block: use co_wrapper_mixed_bdrv_rdlock in functions taking the rdlock > > Kevin Wolf (10): > block: Factor out bdrv_drain_all_begin_nopoll() > Import clang-tsa.h > clang-tsa: Add TSA_ASSERT() macro > clang-tsa: Add macros for shared locks > configure: Enable -Wthread-safety if present > test-bdrv-drain: Fix incorrrect drain assumptions > block: Fix locking in external_snapshot_prepare() > graph-lock: TSA annotations for lock/unlock functions > Mark assert_bdrv_graph_readable/writable() GRAPH_RD/WRLOCK > block: GRAPH_RDLOCK for functions only called by co_wrappers > > Paolo Bonzini (1): > graph-lock: Introduce a lock to protect block graph operations > Reviewed-by: Emanuele Giuseppe Esposito ^ I am curious to see if I am allowed to have my r-b also on my patches :) > configure | 1 + > block/coroutines.h | 19 +- > include/block/aio.h| 9 + > include/block/block-common.h | 9 +- > include/block/block-global-state.h | 1 + > include/block/block-io.h | 53 +++-- > include/block/block_int-common.h | 24 +-- > include/block/block_int-global-state.h | 17 -- > include/block/block_int.h | 1 + > include/
Re: [RFC PATCH] test-bdrv-drain: keep graph manipulations out of coroutines
Am 02/12/2022 um 18:22 schrieb Paolo Bonzini:
> On 12/2/22 14:42, Emanuele Giuseppe Esposito wrote:
>>
>>
>> Am 02/12/2022 um 14:27 schrieb Paolo Bonzini:
>>> Changes to the BlockDriverState graph will have to take the
>>> corresponding lock for writing, and therefore cannot be done
>>> inside a coroutine. Move them outside the test body.
>>>
>>> Signed-off-by: Paolo Bonzini
>>> ---
>>> tests/unit/test-bdrv-drain.c | 63 ++--
>>> 1 file changed, 46 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
>>> index 6ae44116fe79..d85083dd4f9e 100644
>>> --- a/tests/unit/test-bdrv-drain.c
>>> +++ b/tests/unit/test-bdrv-drain.c
>>> @@ -199,25 +199,40 @@ static void do_drain_end_unlocked(enum
>>> drain_type drain_type, BlockDriverState *
>>> }
>>> }
>>> +static BlockBackend *blk;
>>> +static BlockDriverState *bs, *backing;
>>> +
>>> +static void test_drv_cb_init(void)
>>> +{
>>> + blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
>>> + bs = bdrv_new_open_driver(_test, "test-node", BDRV_O_RDWR,
>>> + _abort);
>>> + blk_insert_bs(blk, bs, _abort);
>>> +
>>> + backing = bdrv_new_open_driver(_test, "backing", 0,
>>> _abort);
>>> + bdrv_set_backing_hd(bs, backing, _abort);
>>> +}
>>> +
>>> +static void test_drv_cb_fini(void)
>>
>> fini stands for "finito"? :)
>
> No, for finish :)
> http://ftp.math.utah.edu/u/ma/hohn/linux/misc/elf/node3.html
Cool :)
>
>> Anyways, an alternative solution for this is also here (probably coming
>> from you too):
>> https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03517.html
>
> Much better. At least patches 7-8 from that series have to be salvaged,
> possibly 10 as well.
Serie sent:
https://patchew.org/QEMU/20221205121029.1089209-1-eespo...@redhat.com/
Yes theoretically also patch 9, but I think there's no need to respin
them. If someone is interested they are there.
Thank you,
Emanuele
>
> Paolo
>
[PATCH 1/2] test-bdrv-drain.c: remove test_detach_by_parent_cb()
This test uses a callback of an I/O function (blk_aio_preadv)
to modify the graph, using bdrv_attach_child.
This is simply not allowed anymore. I/O cannot change the graph.
The problem in this test is in:
acb = blk_aio_preadv(blk, 0, , 0, detach_by_parent_aio_cb, NULL);
/* Drain and check the expected result */
bdrv_subtree_drained_begin(parent_b);
because the detach_by_parent_aio_cb calls detach_indirect_bh(), that
modifies the graph and is invoked during bdrv_subtree_drained_begin().
The call stack is the following:
1. blk_aio_preadv() creates a coroutine, increments in_flight counter
and enters the coroutine running blk_aio_read_entry()
2. blk_aio_read_entry() performs the read and then schedules a bh to
complete (blk_aio_complete)
3. at this point, subtree_drained_begin() kicks in and waits for all
in_flight requests, polling
4. polling allows the bh to be scheduled, so blk_aio_complete runs
5. blk_aio_complete *first* invokes the callback
(detach_by_parent_aio_cb) and then decrements the in_flight counter
6. Here is the problem: detach_by_parent_aio_cb modifies the graph,
so both bdrv_unref_child() and bdrv_attach_child() will have
subtree_drains inside. And this causes a deadlock, because the
nested drain will wait for in_flight counter to go to zero, which
is only happening once the drain itself finishes.
Different story is test_detach_by_driver_cb(): in this case,
detach_by_parent_aio_cb() does not call detach_indirect_bh(),
but it is instead called as a bh running in the main loop by
detach_by_driver_cb_drained_begin(), the callback for
.drained_begin().
This test was added in 231281ab42 and part of the series
"Drain fixes and cleanups, part 3"
https://lists.nongnu.org/archive/html/qemu-block/2018-05/msg01132.html
but as explained above I believe that it is not valid anymore, and
can be discarded.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
tests/unit/test-bdrv-drain.c | 41
1 file changed, 9 insertions(+), 32 deletions(-)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 09dc4a4891..4ce159250e 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1316,7 +1316,6 @@ struct detach_by_parent_data {
BdrvChild *child_b;
BlockDriverState *c;
BdrvChild *child_c;
-bool by_parent_cb;
};
static struct detach_by_parent_data detach_by_parent_data;
@@ -1334,12 +1333,7 @@ static void detach_indirect_bh(void *opaque)
static void detach_by_parent_aio_cb(void *opaque, int ret)
{
-struct detach_by_parent_data *data = _by_parent_data;
-
g_assert_cmpint(ret, ==, 0);
-if (data->by_parent_cb) {
-detach_indirect_bh(data);
-}
}
static void detach_by_driver_cb_drained_begin(BdrvChild *child)
@@ -1358,33 +1352,25 @@ static BdrvChildClass detach_by_driver_cb_class;
*\ / \
* A B C
*
- * by_parent_cb == true: Test that parent callbacks don't poll
- *
- * PA has a pending write request whose callback changes the child nodes of
- * PB: It removes B and adds C instead. The subtree of PB is drained, which
- * will indirectly drain the write request, too.
- *
- * by_parent_cb == false: Test that bdrv_drain_invoke() doesn't poll
+ * Test that bdrv_drain_invoke() doesn't poll
*
* PA's BdrvChildClass has a .drained_begin callback that schedules a BH
* that does the same graph change. If bdrv_drain_invoke() calls it, the
* state is messed up, but if it is only polled in the single
* BDRV_POLL_WHILE() at the end of the drain, this should work fine.
*/
-static void test_detach_indirect(bool by_parent_cb)
+static void test_detach_indirect(void)
{
BlockBackend *blk;
BlockDriverState *parent_a, *parent_b, *a, *b, *c;
BdrvChild *child_a, *child_b;
BlockAIOCB *acb;
+BDRVTestState *s;
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
-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;
-}
+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 */
parent_a = bdrv_new_open_driver(_test, "parent-a", BDRV_O_RDWR,
@@ -1403,10 +1389,8 @@ static void test_detach_indirect(bool by_parent_cb)
/* If we want to get bdrv_drain_invoke() to call aio_poll(), the driver
* callback must not return immediately. */
-if (!by_parent_cb) {
-BDRVTestState *s = parent_a->opaque;
-s->sleep_in_drain_begin = true;
-}
+s = parent_a->opaque;
+s->sleep_in_drain_begin = true;
/* Set child relationships */
bdrv_ref(b);
@@ -1418,7 +1402,7 @@ static void test_detach_indirect(bool by_parent_cb)
bdrv_ref(a);
[PATCH 2/2] tests/unit/test-bdrv-drain.c: graph setup functions can't run in coroutines
Graph initialization functions like blk_new(), bdrv_new() and so on
should not run in a coroutine. In fact, they might invoke a drain
(for example blk_insert_bs eventually calls bdrv_replace_child_noperm)
that in turn can invoke callbacks like bdrv_do_drained_begin_quiesce(),
that asserts exactly that we are not in a coroutine.
Move the initialization phase of test_drv_cb and test_quiesce_common
outside the coroutine logic.
Signed-off-by: Emanuele Giuseppe Esposito
---
tests/unit/test-bdrv-drain.c | 118 ++-
1 file changed, 73 insertions(+), 45 deletions(-)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 4ce159250e..8b379727c9 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -116,7 +116,8 @@ static void aio_ret_cb(void *opaque, int ret)
}
typedef struct CallInCoroutineData {
-void (*entry)(void);
+void (*entry)(void *);
+void *arg;
bool done;
} CallInCoroutineData;
@@ -124,15 +125,16 @@ static coroutine_fn void call_in_coroutine_entry(void
*opaque)
{
CallInCoroutineData *data = opaque;
-data->entry();
+data->entry(data->arg);
data->done = true;
}
-static void call_in_coroutine(void (*entry)(void))
+static void call_in_coroutine(void (*entry)(void *), void *arg)
{
Coroutine *co;
CallInCoroutineData data = {
.entry = entry,
+.arg= arg,
.done = false,
};
@@ -192,26 +194,28 @@ static void do_drain_end_unlocked(enum drain_type
drain_type, BlockDriverState *
}
}
-static void test_drv_cb_common(enum drain_type drain_type, bool recursive)
-{
+typedef struct TestDriverCBData {
+enum drain_type drain_type;
+bool recursive;
BlockBackend *blk;
BlockDriverState *bs, *backing;
-BDRVTestState *s, *backing_s;
+} TestDriverCBData;
+
+static void test_drv_cb_common(void *arg)
+{
+TestDriverCBData *data = arg;
+BlockBackend *blk = data->blk;
+BlockDriverState *bs = data->bs;
+BlockDriverState *backing = data->backing;
+enum drain_type drain_type = data->drain_type;
+bool recursive = data->recursive;
+BDRVTestState *s = bs->opaque;
+BDRVTestState *backing_s = backing->opaque;
BlockAIOCB *acb;
int aio_ret;
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
-blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
-bs = bdrv_new_open_driver(_test, "test-node", BDRV_O_RDWR,
- _abort);
-s = bs->opaque;
-blk_insert_bs(blk, bs, _abort);
-
-backing = bdrv_new_open_driver(_test, "backing", 0, _abort);
-backing_s = backing->opaque;
-bdrv_set_backing_hd(bs, backing, _abort);
-
/* Simple bdrv_drain_all_begin/end pair, check that CBs are called */
g_assert_cmpint(s->drain_count, ==, 0);
g_assert_cmpint(backing_s->drain_count, ==, 0);
@@ -245,54 +249,77 @@ static void test_drv_cb_common(enum drain_type
drain_type, bool recursive)
g_assert_cmpint(s->drain_count, ==, 0);
g_assert_cmpint(backing_s->drain_count, ==, 0);
+}
-bdrv_unref(backing);
-bdrv_unref(bs);
-blk_unref(blk);
+static void test_common_cb(enum drain_type drain_type, bool in_coroutine,
+ void (*cb)(void *))
+{
+TestDriverCBData data;
+
+data.drain_type = drain_type;
+data.recursive = (drain_type != BDRV_DRAIN);
+
+data.blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
+data.bs = bdrv_new_open_driver(_test, "test-node", BDRV_O_RDWR,
+ _abort);
+blk_insert_bs(data.blk, data.bs, _abort);
+
+data.backing = bdrv_new_open_driver(_test, "backing", 0,
_abort);
+bdrv_set_backing_hd(data.bs, data.backing, _abort);
+
+if (in_coroutine) {
+call_in_coroutine(cb, );
+} else {
+cb();
+}
+
+bdrv_unref(data.backing);
+bdrv_unref(data.bs);
+blk_unref(data.blk);
+}
+
+static void test_drv_cb(enum drain_type drain_type, bool in_coroutine)
+{
+test_common_cb(drain_type, in_coroutine, test_drv_cb_common);
}
static void test_drv_cb_drain_all(void)
{
-test_drv_cb_common(BDRV_DRAIN_ALL, true);
+test_drv_cb(BDRV_DRAIN_ALL, false);
}
static void test_drv_cb_drain(void)
{
-test_drv_cb_common(BDRV_DRAIN, false);
+test_drv_cb(BDRV_DRAIN, false);
}
static void test_drv_cb_drain_subtree(void)
{
-test_drv_cb_common(BDRV_SUBTREE_DRAIN, true);
+test_drv_cb(BDRV_SUBTREE_DRAIN, false);
}
static void test_drv_cb_co_drain_all(void)
{
-call_in_coroutine(test_drv_cb_drain_all);
+test_drv_cb(BDRV_DRAIN_ALL, true);
}
static void test_drv_cb_co_drain(void)
{
-call_in_coroutine(test_drv_cb_drain);
+test_drv_cb(BDRV_DRAIN, true);
}
static void test_drv_cb_co_drain_subtree(void)
{
-call_in_coroutine(test_drv_cb_drain_subtr
[PATCH 0/2] Fixes to test-bdrv-drain unit test
This test performs graph modification while being in IO_CODE. This is not allowed anymore. This serie is taken from the forgotten (and partially invalid anymore) serie: "[PATCH v2 00/10] block: bug fixes in preparation of AioContext removal" (actually one could also use patch 9 and 10 from that serie, so if you're interested take a look). https://patchew.org/QEMU/20220314131854.2202651-1-eespo...@redhat.com/20220314131854.2202651-7-eespo...@redhat.com/ This serie substitutes Paolo's recent patch: "[RFC PATCH] test-bdrv-drain: keep graph manipulations out of coroutines" Thank you, Emanuele Emanuele Giuseppe Esposito (2): test-bdrv-drain.c: remove test_detach_by_parent_cb() tests/unit/test-bdrv-drain.c: graph setup functions can't run in coroutines tests/unit/test-bdrv-drain.c | 159 ++- 1 file changed, 82 insertions(+), 77 deletions(-) -- 2.31.1
Re: [PATCH v3 2/3] KVM: keep track of running ioctls
Am 02/12/2022 um 14:32 schrieb Robert Hoo: > On Fri, 2022-12-02 at 13:03 +0100, Emanuele Giuseppe Esposito wrote: > ... >>>> @@ -3032,7 +3035,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, >>>> ...) >>>> va_end(ap); >>>> >>>> trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg); >>>> +accel_cpu_ioctl_begin(cpu); >>> >>> Does this mean that kvm_region_commit() can inhibit any other vcpus >>> doing any ioctls? >> >> Yes, because we must prevent any vcpu from reading memslots while we >> are >> updating them. >> > But do most other vm/vcpu ioctls contend with memslot operations? > I think this is the simplest way. I agree not all ioctls contend with memslot operations, but there are also not so many memslot operations too. Instead of going one by one in all possible ioctls, covering all of them is the simplest way and it covers also the case of a new ioctl reading memslots that could be added in the future (alternatively we would be always updating the list of ioctls to block).
Re: [RFC PATCH] test-bdrv-drain: keep graph manipulations out of coroutines
Am 02/12/2022 um 14:27 schrieb Paolo Bonzini:
> Changes to the BlockDriverState graph will have to take the
> corresponding lock for writing, and therefore cannot be done
> inside a coroutine. Move them outside the test body.
>
> Signed-off-by: Paolo Bonzini
> ---
> tests/unit/test-bdrv-drain.c | 63 ++--
> 1 file changed, 46 insertions(+), 17 deletions(-)
>
> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
> index 6ae44116fe79..d85083dd4f9e 100644
> --- a/tests/unit/test-bdrv-drain.c
> +++ b/tests/unit/test-bdrv-drain.c
> @@ -199,25 +199,40 @@ static void do_drain_end_unlocked(enum drain_type
> drain_type, BlockDriverState *
> }
> }
>
> +static BlockBackend *blk;
> +static BlockDriverState *bs, *backing;
> +
> +static void test_drv_cb_init(void)
> +{
> +blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
> +bs = bdrv_new_open_driver(_test, "test-node", BDRV_O_RDWR,
> + _abort);
> +blk_insert_bs(blk, bs, _abort);
> +
> +backing = bdrv_new_open_driver(_test, "backing", 0, _abort);
> +bdrv_set_backing_hd(bs, backing, _abort);
> +}
> +
> +static void test_drv_cb_fini(void)
fini stands for "finito"? :)
Anyways, an alternative solution for this is also here (probably coming
from you too):
https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03517.html
Thank you,
Emanuele
> +{
> +bdrv_unref(backing);
> +bdrv_unref(bs);
> +blk_unref(blk);
> +backing = NULL;
> +bs = NULL;
> +blk = NULL;
> +}
> +
> static void test_drv_cb_common(enum drain_type drain_type, bool recursive)
> {
> -BlockBackend *blk;
> -BlockDriverState *bs, *backing;
> BDRVTestState *s, *backing_s;
> BlockAIOCB *acb;
> int aio_ret;
>
> QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
>
> -blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
> -bs = bdrv_new_open_driver(_test, "test-node", BDRV_O_RDWR,
> - _abort);
> s = bs->opaque;
> -blk_insert_bs(blk, bs, _abort);
> -
> -backing = bdrv_new_open_driver(_test, "backing", 0, _abort);
> backing_s = backing->opaque;
> -bdrv_set_backing_hd(bs, backing, _abort);
>
> /* Simple bdrv_drain_all_begin/end pair, check that CBs are called */
> g_assert_cmpint(s->drain_count, ==, 0);
> @@ -252,30 +267,44 @@ static void test_drv_cb_common(enum drain_type
> drain_type, bool recursive)
>
> g_assert_cmpint(s->drain_count, ==, 0);
> g_assert_cmpint(backing_s->drain_count, ==, 0);
> -
> -bdrv_unref(backing);
> -bdrv_unref(bs);
> -blk_unref(blk);
> }
>
> -static void test_drv_cb_drain_all(void)
> +static void test_drv_cb_do_drain_all(void)
> {
> test_drv_cb_common(BDRV_DRAIN_ALL, true);
> }
>
> -static void test_drv_cb_drain(void)
> +static void test_drv_cb_do_drain(void)
> {
> test_drv_cb_common(BDRV_DRAIN, false);
> }
>
> +static void test_drv_cb_drain_all(void)
> +{
> +test_drv_cb_init();
> +test_drv_cb_do_drain_all();
> +test_drv_cb_fini();
> +}
> +
> +static void test_drv_cb_drain(void)
> +{
> +test_drv_cb_init();
> +test_drv_cb_do_drain();
> +test_drv_cb_fini();
> +}
> +
> static void test_drv_cb_co_drain_all(void)
> {
> -call_in_coroutine(test_drv_cb_drain_all);
> +test_drv_cb_init();
> +call_in_coroutine(test_drv_cb_do_drain_all);
> +test_drv_cb_fini();
> }
>
> static void test_drv_cb_co_drain(void)
> {
> -call_in_coroutine(test_drv_cb_drain);
> +test_drv_cb_init();
> +call_in_coroutine(test_drv_cb_do_drain);
> +test_drv_cb_fini();
> }
>
> static void test_quiesce_common(enum drain_type drain_type, bool recursive)
>
Re: [PATCH v3 2/3] KVM: keep track of running ioctls
Am 02/12/2022 um 07:54 schrieb Robert Hoo:
> On Fri, 2022-11-11 at 10:47 -0500, Emanuele Giuseppe Esposito wrote:
>> Using the new accel-blocker API, mark where ioctls are being called
>> in KVM. Next, we will implement the critical section that will take
>> care of performing memslots modifications atomically, therefore
>> preventing any new ioctl from running and allowing the running ones
>> to finish.
>>
>> Signed-off-by: David Hildenbrand
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> accel/kvm/kvm-all.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index f99b0becd8..ff660fd469 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -2310,6 +2310,7 @@ static int kvm_init(MachineState *ms)
>> assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size());
>>
>> s->sigmask_len = 8;
>> +accel_blocker_init();
>>
>> #ifdef KVM_CAP_SET_GUEST_DEBUG
>> QTAILQ_INIT(>kvm_sw_breakpoints);
>> @@ -3014,7 +3015,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
>> va_end(ap);
>>
>> trace_kvm_vm_ioctl(type, arg);
>> +accel_ioctl_begin();
>> ret = ioctl(s->vmfd, type, arg);
>> +accel_ioctl_end();
>> if (ret == -1) {
>> ret = -errno;
>> }
>> @@ -3032,7 +3035,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type,
>> ...)
>> va_end(ap);
>>
>> trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
>> +accel_cpu_ioctl_begin(cpu);
>
> Does this mean that kvm_region_commit() can inhibit any other vcpus
> doing any ioctls?
Yes, because we must prevent any vcpu from reading memslots while we are
updating them.
>
>> ret = ioctl(cpu->kvm_fd, type, arg);
>> +accel_cpu_ioctl_end(cpu);
>> if (ret == -1) {
>> ret = -errno;
>> }
>> @@ -3050,7 +3055,9 @@ int kvm_device_ioctl(int fd, int type, ...)
>> va_end(ap);
>>
>> trace_kvm_device_ioctl(fd, type, arg);
>> +accel_ioctl_begin();
>> ret = ioctl(fd, type, arg);
>> +accel_ioctl_end();
>> if (ret == -1) {
>> ret = -errno;
>> }
>
[PATCH v7 13/14] block: convert bdrv_create to co_wrapper
This function is never called in coroutine context, therefore
instead of manually creating a new coroutine, delegate it to the
block-coroutine-wrapper script, defining it as co_wrapper.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/block/block-global-state.h | 8 --
block.c| 41 ++
2 files changed, 8 insertions(+), 41 deletions(-)
diff --git a/include/block/block-global-state.h
b/include/block/block-global-state.h
index 387a7cbb2e..1f8b54f2df 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -55,8 +55,12 @@ BlockDriver *bdrv_find_protocol(const char *filename,
bool allow_protocol_prefix,
Error **errp);
BlockDriver *bdrv_find_format(const char *format_name);
-int bdrv_create(BlockDriver *drv, const char* filename,
-QemuOpts *opts, Error **errp);
+
+int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+QemuOpts *opts, Error **errp);
+int co_wrapper bdrv_create(BlockDriver *drv, const char *filename,
+ QemuOpts *opts, Error **errp);
+
int coroutine_fn bdrv_co_create_file(const char *filename, QemuOpts *opts,
Error **errp);
diff --git a/block.c b/block.c
index 20a5d7e8cf..8683d24553 100644
--- a/block.c
+++ b/block.c
@@ -528,8 +528,8 @@ typedef struct CreateCo {
Error *err;
} CreateCo;
-static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
- QemuOpts *opts, Error **errp)
+int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+QemuOpts *opts, Error **errp)
{
int ret;
GLOBAL_STATE_CODE();
@@ -549,43 +549,6 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv,
const char *filename,
return ret;
}
-static void coroutine_fn bdrv_create_co_entry(void *opaque)
-{
-CreateCo *cco = opaque;
-GLOBAL_STATE_CODE();
-
-cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, >err);
-aio_wait_kick();
-}
-
-int bdrv_create(BlockDriver *drv, const char* filename,
-QemuOpts *opts, Error **errp)
-{
-GLOBAL_STATE_CODE();
-
-if (qemu_in_coroutine()) {
-/* Fast-path if already in coroutine context */
-return bdrv_co_create(drv, filename, opts, errp);
-} else {
-Coroutine *co;
-CreateCo cco = {
-.drv = drv,
-.filename = filename,
-.opts = opts,
-.ret = NOT_DONE,
-.err = NULL,
-};
-
-co = qemu_coroutine_create(bdrv_create_co_entry, );
-qemu_coroutine_enter(co);
-while (cco.ret == NOT_DONE) {
-aio_poll(qemu_get_aio_context(), true);
-}
-error_propagate(errp, cco.err);
-return cco.ret;
-}
-}
-
/**
* Helper function for bdrv_create_file_fallback(): Resize @blk to at
* least the given @minimum_size.
--
2.31.1
[PATCH v7 09/14] block: rename generated_co_wrapper in co_wrapper_mixed
In preparation to the incoming new function specifiers,
rename g_c_w with a more meaningful name and document it.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
docs/devel/block-coroutine-wrapper.rst | 6 +--
block/coroutines.h | 4 +-
include/block/block-common.h | 11 +++--
include/block/block-io.h | 44 -
include/sysemu/block-backend-io.h | 68 +-
scripts/block-coroutine-wrapper.py | 6 +--
6 files changed, 71 insertions(+), 68 deletions(-)
diff --git a/docs/devel/block-coroutine-wrapper.rst
b/docs/devel/block-coroutine-wrapper.rst
index 412851986b..64acc8d65d 100644
--- a/docs/devel/block-coroutine-wrapper.rst
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -26,12 +26,12 @@ called ``bdrv_foo()``. In this case the script
can help. To
trigger the generation:
1. You need ``bdrv_foo`` declaration somewhere (for example, in
- ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
+ ``block/coroutines.h``) with the ``co_wrapper_mixed`` mark,
like this:
.. code-block:: c
-int generated_co_wrapper bdrv_foo();
+int co_wrapper_mixed bdrv_foo();
2. You need to feed this declaration to block-coroutine-wrapper script.
For this, add the .h (or .c) file with the declaration to the
@@ -46,7 +46,7 @@ Links
1. The script location is ``scripts/block-coroutine-wrapper.py``.
-2. Generic place for private ``generated_co_wrapper`` declarations is
+2. Generic place for private ``co_wrapper_mixed`` declarations is
``block/coroutines.h``, for public declarations:
``include/block/block.h``
diff --git a/block/coroutines.h b/block/coroutines.h
index 3a2bad564f..17da4db963 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -71,7 +71,7 @@ nbd_co_do_establish_connection(BlockDriverState *bs, bool
blocking,
* the "I/O or GS" API.
*/
-int generated_co_wrapper
+int co_wrapper_mixed
bdrv_common_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
bool include_base,
@@ -82,7 +82,7 @@ bdrv_common_block_status_above(BlockDriverState *bs,
int64_t *map,
BlockDriverState **file,
int *depth);
-int generated_co_wrapper
+int co_wrapper_mixed
nbd_do_establish_connection(BlockDriverState *bs, bool blocking, Error **errp);
#endif /* BLOCK_COROUTINES_H */
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 297704c1e9..ec2309055b 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -35,14 +35,17 @@
#include "qemu/transactions.h"
/*
- * generated_co_wrapper
+ * co_wrapper{*}: Function specifiers used by block-coroutine-wrapper.py
*
- * Function specifier, which does nothing but mark functions to be
+ * Function specifiers, which do nothing but mark functions to be
* generated by scripts/block-coroutine-wrapper.py
*
- * Read more in docs/devel/block-coroutine-wrapper.rst
+ * Usage: read docs/devel/block-coroutine-wrapper.rst
+ *
+ * co_wrapper_mixed functions can be called by both coroutine and
+ * non-coroutine context.
*/
-#define generated_co_wrapper
+#define co_wrapper_mixed
/* block.c */
typedef struct BlockDriver BlockDriver;
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 72919254cd..72cf45975b 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -39,19 +39,19 @@
* to catch when they are accidentally called by the wrong API.
*/
-int generated_co_wrapper bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
-int64_t bytes,
-BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
+int64_t bytes,
+BdrvRequestFlags flags);
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
-int generated_co_wrapper bdrv_pread(BdrvChild *child, int64_t offset,
-int64_t bytes, void *buf,
-BdrvRequestFlags flags);
-int generated_co_wrapper bdrv_pwrite(BdrvChild *child, int64_t offset,
- int64_t bytes, const void *buf,
- BdrvRequestFlags flags);
-int generated_co_wrapper bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
- int64_t bytes, const void *buf,
- BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pread(BdrvChild *child, int64_t offset,
+int64_t bytes, void *buf,
+BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pwrite(BdrvChild *c
[PATCH v7 12/14] block-coroutine-wrapper.py: support also basic return types
Extend the regex to cover also return type, pointers included.
This implies that the value returned by the function cannot be
a simple "int" anymore, but the custom return type.
Therefore remove poll_state->ret and instead use a per-function
custom "ret" field.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/block-gen.h | 5 +
scripts/block-coroutine-wrapper.py | 19 +++
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/block/block-gen.h b/block/block-gen.h
index 08d977f493..89b7daaa1f 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -32,18 +32,15 @@
typedef struct BdrvPollCo {
AioContext *ctx;
bool in_progress;
-int ret;
Coroutine *co; /* Keep pointer here for debugging */
} BdrvPollCo;
-static inline int bdrv_poll_co(BdrvPollCo *s)
+static inline void bdrv_poll_co(BdrvPollCo *s)
{
assert(!qemu_in_coroutine());
aio_co_enter(s->ctx, s->co);
AIO_WAIT_WHILE(s->ctx, s->in_progress);
-
-return s->ret;
}
#endif /* BLOCK_BLOCK_GEN_H */
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index f540003af1..71a06e917a 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -92,7 +92,8 @@ def gen_block(self, format: str) -> str:
# Match wrappers declared with a co_wrapper mark
-func_decl_re = re.compile(r'^int\s*co_wrapper'
+func_decl_re = re.compile(r'^(?P[a-zA-Z][a-zA-Z0-9_]* [\*]?)'
+ r'\s*co_wrapper'
r'(?P(_[a-z][a-z0-9_]*)?)\s*'
r'(?P[a-z][a-z0-9_]*)'
r'\((?P[^)]*)\);$', re.MULTILINE)
@@ -100,7 +101,7 @@ def gen_block(self, format: str) -> str:
def func_decl_iter(text: str) -> Iterator:
for m in func_decl_re.finditer(text):
-yield FuncDecl(return_type='int',
+yield FuncDecl(return_type=m.group('return_type'),
name=m.group('wrapper_name'),
args=m.group('args'),
variant=m.group('variant'))
@@ -123,7 +124,7 @@ def create_mixed_wrapper(func: FuncDecl) -> str:
name = func.co_name
struct_name = func.struct_name
return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
{{
if (qemu_in_coroutine()) {{
return {name}({ func.gen_list('{name}') });
@@ -137,7 +138,8 @@ def create_mixed_wrapper(func: FuncDecl) -> str:
s.poll_state.co = qemu_coroutine_create({name}_entry, );
-return bdrv_poll_co(_state);
+bdrv_poll_co(_state);
+return s.ret;
}}
}}"""
@@ -149,7 +151,7 @@ def create_co_wrapper(func: FuncDecl) -> str:
name = func.co_name
struct_name = func.struct_name
return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
{{
{struct_name} s = {{
.poll_state.ctx = {func.ctx},
@@ -161,13 +163,13 @@ def create_co_wrapper(func: FuncDecl) -> str:
s.poll_state.co = qemu_coroutine_create({name}_entry, );
-return bdrv_poll_co(_state);
+bdrv_poll_co(_state);
+return s.ret;
}}"""
def gen_wrapper(func: FuncDecl) -> str:
assert not '_co_' in func.name
-assert func.return_type == 'int'
name = func.co_name
struct_name = func.struct_name
@@ -183,6 +185,7 @@ def gen_wrapper(func: FuncDecl) -> str:
typedef struct {struct_name} {{
BdrvPollCo poll_state;
+{func.return_type} ret;
{ func.gen_block('{decl};') }
}} {struct_name};
@@ -190,7 +193,7 @@ def gen_wrapper(func: FuncDecl) -> str:
{{
{struct_name} *s = opaque;
-s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
+s->ret = {name}({ func.gen_list('s->{name}') });
s->poll_state.in_progress = false;
aio_wait_kick();
--
2.31.1
[PATCH v7 08/14] block: bdrv_create_file is a coroutine_fn
It is always called in coroutine_fn callbacks, therefore
it can directly call bdrv_co_create().
Rename it to bdrv_co_create_file too.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/block/block-global-state.h | 3 ++-
block.c| 5 +++--
block/crypto.c | 2 +-
block/parallels.c | 2 +-
block/qcow.c | 2 +-
block/qcow2.c | 4 ++--
block/qed.c| 2 +-
block/raw-format.c | 2 +-
block/vdi.c| 2 +-
block/vhdx.c | 2 +-
block/vmdk.c | 2 +-
block/vpc.c| 2 +-
12 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/include/block/block-global-state.h
b/include/block/block-global-state.h
index 00e0cf8aea..387a7cbb2e 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -57,7 +57,8 @@ BlockDriver *bdrv_find_protocol(const char *filename,
BlockDriver *bdrv_find_format(const char *format_name);
int bdrv_create(BlockDriver *drv, const char* filename,
QemuOpts *opts, Error **errp);
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
+int coroutine_fn bdrv_co_create_file(const char *filename, QemuOpts *opts,
+ Error **errp);
BlockDriverState *bdrv_new(void);
int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
diff --git a/block.c b/block.c
index eb273dd2e3..20a5d7e8cf 100644
--- a/block.c
+++ b/block.c
@@ -721,7 +721,8 @@ out:
return ret;
}
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+int coroutine_fn bdrv_co_create_file(const char *filename, QemuOpts *opts,
+ Error **errp)
{
QemuOpts *protocol_opts;
BlockDriver *drv;
@@ -762,7 +763,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts,
Error **errp)
goto out;
}
-ret = bdrv_create(drv, filename, protocol_opts, errp);
+ret = bdrv_co_create(drv, filename, protocol_opts, errp);
out:
qemu_opts_del(protocol_opts);
qobject_unref(qdict);
diff --git a/block/crypto.c b/block/crypto.c
index 2fb8add458..bbeb9f437c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -703,7 +703,7 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(BlockDriver *drv,
}
/* Create protocol layer */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/parallels.c b/block/parallels.c
index fa08c1104b..bbea2f2221 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -646,7 +646,7 @@ static int coroutine_fn
parallels_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto done;
}
diff --git a/block/qcow.c b/block/qcow.c
index daa38839ab..18e17a5b12 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -973,7 +973,7 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver
*drv,
}
/* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 4dd3ff..7cc49a3a6c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3871,7 +3871,7 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver
*drv,
}
/* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto finish;
}
@@ -3886,7 +3886,7 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver
*drv,
/* Create and open an external data file (protocol layer) */
val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
if (val) {
-ret = bdrv_create_file(val, opts, errp);
+ret = bdrv_co_create_file(val, opts, errp);
if (ret < 0) {
goto finish;
}
diff --git a/block/qed.c b/block/qed.c
index c2691a85b1..9d54c8eec5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -778,7 +778,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(BlockDriver
*drv,
}
/* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/raw-format.c b/block/raw-format.c
index a68014ef0b..28905b09ee 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -433,7 +433,7 @@ static int coroutine_fn raw_co_create_opt
[PATCH v7 04/14] block-backend: replace bdrv_*_above with blk_*_above
Avoid mixing bdrv_* functions with blk_*, so create blk_* counterparts
for bdrv_block_status_above and bdrv_is_allocated_above.
Note that since blk_co_block_status_above only calls the g_c_w function
bdrv_common_block_status_above and is marked as coroutine_fn, call
directly bdrv_co_common_block_status_above() to avoid using a g_c_w.
Same applies to blk_co_is_allocated_above.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/sysemu/block-backend-io.h | 9 +
block/block-backend.c | 21
block/commit.c| 4 ++--
nbd/server.c | 32 +++
4 files changed, 48 insertions(+), 18 deletions(-)
diff --git a/include/sysemu/block-backend-io.h
b/include/sysemu/block-backend-io.h
index 50f5aa2e07..ee3eb12610 100644
--- a/include/sysemu/block-backend-io.h
+++ b/include/sysemu/block-backend-io.h
@@ -92,6 +92,15 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in,
int64_t off_in,
int64_t bytes, BdrvRequestFlags read_flags,
BdrvRequestFlags write_flags);
+int coroutine_fn blk_co_block_status_above(BlockBackend *blk,
+ BlockDriverState *base,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file);
+int coroutine_fn blk_co_is_allocated_above(BlockBackend *blk,
+ BlockDriverState *base,
+ bool include_base, int64_t offset,
+ int64_t bytes, int64_t *pnum);
/*
* "I/O or GS" API functions. These functions can run without
diff --git a/block/block-backend.c b/block/block-backend.c
index 742efa7955..1d2d8526ef 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1424,6 +1424,27 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk,
int64_t offset,
return blk_co_pwritev_part(blk, offset, bytes, qiov, 0, flags);
}
+int coroutine_fn blk_co_block_status_above(BlockBackend *blk,
+ BlockDriverState *base,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
+{
+IO_CODE();
+return bdrv_co_block_status_above(blk_bs(blk), base, offset, bytes, pnum,
+ map, file);
+}
+
+int coroutine_fn blk_co_is_allocated_above(BlockBackend *blk,
+ BlockDriverState *base,
+ bool include_base, int64_t offset,
+ int64_t bytes, int64_t *pnum)
+{
+IO_CODE();
+return bdrv_co_is_allocated_above(blk_bs(blk), base, include_base, offset,
+ bytes, pnum);
+}
+
typedef struct BlkRwCo {
BlockBackend *blk;
int64_t offset;
diff --git a/block/commit.c b/block/commit.c
index 0029b31944..b346341767 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -155,8 +155,8 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
break;
}
/* Copy if allocated above the base */
-ret = bdrv_is_allocated_above(blk_bs(s->top), s->base_overlay, true,
- offset, COMMIT_BUFFER_SIZE, );
+ret = blk_co_is_allocated_above(s->top, s->base_overlay, true,
+offset, COMMIT_BUFFER_SIZE, );
copy = (ret > 0);
trace_commit_one_iteration(s, offset, n, ret);
if (copy) {
diff --git a/nbd/server.c b/nbd/server.c
index 4af5c793a7..c53c39560e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1991,7 +1991,7 @@ static int coroutine_fn
nbd_co_send_structured_error(NBDClient *client,
}
/* Do a sparse read and send the structured reply to the client.
- * Returns -errno if sending fails. bdrv_block_status_above() failure is
+ * Returns -errno if sending fails. blk_co_block_status_above() failure is
* reported to the client, at which point this function succeeds.
*/
static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
@@ -2007,10 +2007,10 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
while (progress < size) {
int64_t pnum;
-int status = bdrv_block_status_above(blk_bs(exp->common.blk), NULL,
- offset + progress,
- size - progress, , NULL,
- NULL);
+int status = blk_co_block_sta
[PATCH v7 02/14] block-copy: add coroutine_fn annotations
These functions end up calling bdrv_common_block_status_above(), a
generated_co_wrapper function.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we can mark such functions coroutine_fn too.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Paolo Bonzini
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/block/block-copy.h | 5 +++--
block/block-copy.c | 21 -
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index ba0b425d78..8cea4f9b90 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -36,8 +36,9 @@ void block_copy_set_progress_meter(BlockCopyState *s,
ProgressMeter *pm);
void block_copy_state_free(BlockCopyState *s);
void block_copy_reset(BlockCopyState *s, int64_t offset, int64_t bytes);
-int64_t block_copy_reset_unallocated(BlockCopyState *s,
- int64_t offset, int64_t *count);
+int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
+ int64_t offset,
+ int64_t *count);
int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
bool ignore_ratelimit, uint64_t timeout_ns,
diff --git a/block/block-copy.c b/block/block-copy.c
index bb947afdda..5e59d6262f 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -577,8 +577,9 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
return ret;
}
-static int block_copy_block_status(BlockCopyState *s, int64_t offset,
- int64_t bytes, int64_t *pnum)
+static coroutine_fn int block_copy_block_status(BlockCopyState *s,
+int64_t offset,
+int64_t bytes, int64_t *pnum)
{
int64_t num;
BlockDriverState *base;
@@ -590,8 +591,8 @@ static int block_copy_block_status(BlockCopyState *s,
int64_t offset,
base = NULL;
}
-ret = bdrv_block_status_above(s->source->bs, base, offset, bytes, ,
- NULL, NULL);
+ret = bdrv_co_block_status_above(s->source->bs, base, offset, bytes, ,
+ NULL, NULL);
if (ret < 0 || num < s->cluster_size) {
/*
* On error or if failed to obtain large enough chunk just fallback to
@@ -613,8 +614,9 @@ static int block_copy_block_status(BlockCopyState *s,
int64_t offset,
* Check if the cluster starting at offset is allocated or not.
* return via pnum the number of contiguous clusters sharing this allocation.
*/
-static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t offset,
- int64_t *pnum)
+static int coroutine_fn block_copy_is_cluster_allocated(BlockCopyState *s,
+int64_t offset,
+int64_t *pnum)
{
BlockDriverState *bs = s->source->bs;
int64_t count, total_count = 0;
@@ -624,7 +626,7 @@ static int block_copy_is_cluster_allocated(BlockCopyState
*s, int64_t offset,
assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
while (true) {
-ret = bdrv_is_allocated(bs, offset, bytes, );
+ret = bdrv_co_is_allocated(bs, offset, bytes, );
if (ret < 0) {
return ret;
}
@@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t offset,
int64_t bytes)
* @return 0 when the cluster at @offset was unallocated,
* 1 otherwise, and -ret on error.
*/
-int64_t block_copy_reset_unallocated(BlockCopyState *s,
- int64_t offset, int64_t *count)
+int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
+ int64_t offset,
+ int64_t *count)
{
int ret;
int64_t clusters, bytes;
--
2.31.1
[PATCH v7 14/14] block/dirty-bitmap: convert coroutine-only functions to co_wrapper
bdrv_can_store_new_dirty_bitmap and bdrv_remove_persistent_dirty_bitmap
check if they are running in a coroutine, directly calling the
coroutine callback if it's the case.
Except that no coroutine calls such functions, therefore that check
can be removed, and function creation can be offloaded to
c_w.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/block/block-common.h | 5 +-
include/block/block-io.h | 10 +++-
include/block/dirty-bitmap.h | 10 +++-
block/dirty-bitmap.c | 88 +---
block/meson.build| 1 +
5 files changed, 22 insertions(+), 92 deletions(-)
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 847e4d4626..6cf603ab06 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -29,8 +29,6 @@
#include "qemu/iov.h"
#include "qemu/coroutine.h"
#include "block/accounting.h"
-#include "block/dirty-bitmap.h"
-#include "block/blockjob.h"
#include "qemu/hbitmap.h"
#include "qemu/transactions.h"
@@ -51,6 +49,9 @@
#define co_wrapper
#define co_wrapper_mixed
+#include "block/dirty-bitmap.h"
+#include "block/blockjob.h"
+
/* block.c */
typedef struct BlockDriver BlockDriver;
typedef struct BdrvChild BdrvChild;
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 72cf45975b..52869ea08e 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -215,8 +215,14 @@ AioContext *child_of_bds_get_parent_aio_context(BdrvChild
*c);
void bdrv_io_plug(BlockDriverState *bs);
void bdrv_io_unplug(BlockDriverState *bs);
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
- uint32_t granularity, Error **errp);
+bool coroutine_fn bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ uint32_t granularity,
+ Error **errp);
+bool co_wrapper bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs,
+const char *name,
+uint32_t granularity,
+Error **errp);
/**
*
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 6528336c4c..c3700cec76 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -34,8 +34,14 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap,
uint32_t flags,
Error **errp);
void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
-int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
-Error **errp);
+
+int coroutine_fn bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+const char *name,
+Error **errp);
+int co_wrapper bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp);
+
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bf3dc0512a..21cf592889 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -388,7 +388,7 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
* not fail.
* This function doesn't release corresponding BdrvDirtyBitmap.
*/
-static int coroutine_fn
+int coroutine_fn
bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
Error **errp)
{
@@ -399,45 +399,6 @@ bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState
*bs, const char *name,
return 0;
}
-typedef struct BdrvRemovePersistentDirtyBitmapCo {
-BlockDriverState *bs;
-const char *name;
-Error **errp;
-int ret;
-} BdrvRemovePersistentDirtyBitmapCo;
-
-static void coroutine_fn
-bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
-{
-BdrvRemovePersistentDirtyBitmapCo *s = opaque;
-
-s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp);
-aio_wait_kick();
-}
-
-int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
-Error **errp)
-{
-if (qemu_in_coroutine()) {
-return bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
-} else {
-Cor
[PATCH v7 05/14] block/vmdk: add coroutine_fn annotations
These functions end up calling bdrv_create() implemented as generated_co_wrapper
functions.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we can mark such functions coroutine_fn too.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Paolo Bonzini
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/vmdk.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 26376352b9..0c32bf2e83 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2285,10 +2285,11 @@ exit:
return ret;
}
-static int vmdk_create_extent(const char *filename, int64_t filesize,
- bool flat, bool compress, bool zeroed_grain,
- BlockBackend **pbb,
- QemuOpts *opts, Error **errp)
+static int coroutine_fn vmdk_create_extent(const char *filename,
+ int64_t filesize, bool flat,
+ bool compress, bool zeroed_grain,
+ BlockBackend **pbb,
+ QemuOpts *opts, Error **errp)
{
int ret;
BlockBackend *blk = NULL;
@@ -2366,14 +2367,14 @@ static int filename_decompose(const char *filename,
char *path, char *prefix,
* non-split format.
* idx >= 1: get the n-th extent if in a split subformat
*/
-typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
- int idx,
- bool flat,
- bool split,
- bool compress,
- bool zeroed_grain,
- void *opaque,
- Error **errp);
+typedef BlockBackend * coroutine_fn (*vmdk_create_extent_fn)(int64_t size,
+ int idx,
+ bool flat,
+ bool split,
+ bool compress,
+ bool zeroed_grain,
+ void *opaque,
+ Error **errp);
static void vmdk_desc_add_extent(GString *desc,
const char *extent_line_fmt,
@@ -2616,7 +2617,7 @@ typedef struct {
QemuOpts *opts;
} VMDKCreateOptsData;
-static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
+static BlockBackend * coroutine_fn vmdk_co_create_opts_cb(int64_t size, int
idx,
bool flat, bool split, bool
compress,
bool zeroed_grain, void *opaque,
Error **errp)
@@ -2768,10 +2769,11 @@ exit:
return ret;
}
-static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
- bool flat, bool split, bool compress,
- bool zeroed_grain, void *opaque,
- Error **errp)
+static BlockBackend * coroutine_fn vmdk_co_create_cb(int64_t size, int idx,
+ bool flat, bool split,
+ bool compress,
+ bool zeroed_grain,
+ void *opaque, Error
**errp)
{
int ret;
BlockDriverState *bs;
--
2.31.1
[PATCH v7 10/14] block-coroutine-wrapper.py: introduce co_wrapper
This new annotation starts just a function wrapper that creates
a new coroutine. It assumes the caller is not a coroutine.
It will be the default annotation to be used in the future.
This is much better as c_w_mixed, because it is clear if the caller
is a coroutine or not, and provides the advantage of automating
the code creation. In the future all c_w_mixed functions will be
substituted by co_wrapper.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
docs/devel/block-coroutine-wrapper.rst | 6 +-
include/block/block-common.h | 8 +-
scripts/block-coroutine-wrapper.py | 110 +
3 files changed, 86 insertions(+), 38 deletions(-)
diff --git a/docs/devel/block-coroutine-wrapper.rst
b/docs/devel/block-coroutine-wrapper.rst
index 64acc8d65d..6dd2cdcab3 100644
--- a/docs/devel/block-coroutine-wrapper.rst
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -26,12 +26,12 @@ called ``bdrv_foo()``. In this case the script
can help. To
trigger the generation:
1. You need ``bdrv_foo`` declaration somewhere (for example, in
- ``block/coroutines.h``) with the ``co_wrapper_mixed`` mark,
+ ``block/coroutines.h``) with the ``co_wrapper`` mark,
like this:
.. code-block:: c
-int co_wrapper_mixed bdrv_foo();
+int co_wrapper bdrv_foo();
2. You need to feed this declaration to block-coroutine-wrapper script.
For this, add the .h (or .c) file with the declaration to the
@@ -46,7 +46,7 @@ Links
1. The script location is ``scripts/block-coroutine-wrapper.py``.
-2. Generic place for private ``co_wrapper_mixed`` declarations is
+2. Generic place for private ``co_wrapper`` declarations is
``block/coroutines.h``, for public declarations:
``include/block/block.h``
diff --git a/include/block/block-common.h b/include/block/block-common.h
index ec2309055b..847e4d4626 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -42,9 +42,13 @@
*
* Usage: read docs/devel/block-coroutine-wrapper.rst
*
- * co_wrapper_mixed functions can be called by both coroutine and
- * non-coroutine context.
+ * There are 2 kind of specifiers:
+ * - co_wrapper functions can be called by only non-coroutine context, because
+ * they always generate a new coroutine.
+ * - co_wrapper_mixed functions can be called by both coroutine and
+ * non-coroutine context.
*/
+#define co_wrapper
#define co_wrapper_mixed
/* block.c */
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index 56e6425356..2090c3bf73 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -2,7 +2,7 @@
"""Generate coroutine wrappers for block subsystem.
The program parses one or several concatenated c files from stdin,
-searches for functions with the 'co_wrapper_mixed' specifier
+searches for functions with the 'co_wrapper' specifier
and generates corresponding wrappers on stdout.
Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
@@ -62,10 +62,25 @@ def __init__(self, param_decl: str) -> None:
class FuncDecl:
-def __init__(self, return_type: str, name: str, args: str) -> None:
+def __init__(self, return_type: str, name: str, args: str,
+ variant: str) -> None:
self.return_type = return_type.strip()
self.name = name.strip()
+self.struct_name = snake_to_camel(self.name)
self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
+self.create_only_co = 'mixed' not in variant
+
+subsystem, subname = self.name.split('_', 1)
+self.co_name = f'{subsystem}_co_{subname}'
+
+t = self.args[0].type
+if t == 'BlockDriverState *':
+bs = 'bs'
+elif t == 'BdrvChild *':
+bs = 'child->bs'
+else:
+bs = 'blk_bs(blk)'
+self.bs = bs
def gen_list(self, format: str) -> str:
return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
@@ -74,8 +89,9 @@ def gen_block(self, format: str) -> str:
return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
-# Match wrappers declared with a co_wrapper_mixed mark
-func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*'
+# Match wrappers declared with a co_wrapper mark
+func_decl_re = re.compile(r'^int\s*co_wrapper'
+ r'(?P(_[a-z][a-z0-9_]*)?)\s*'
r'(?P[a-z][a-z0-9_]*)'
r'\((?P[^)]*)\);$', re.MULTILINE)
@@ -84,7 +100,8 @@ def func_decl_iter(text: str) -> Iterator:
for m in func_decl_re.finditer(text):
yield FuncDecl(return_type='int',
name=m.group('wrapper_name'),
- args=m.group('args'))
+ args=m.group('args'),
+ variant=m.group('variant'))
def snake_to_camel(func_name: s
[PATCH v7 03/14] nbd/server.c: add coroutine_fn annotations
These functions end up calling bdrv_*() implemented as generated_co_wrapper
functions.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we can mark such functions coroutine_fn too.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Paolo Bonzini
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
nbd/server.c | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index ada16089f3..4af5c793a7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2141,14 +2141,15 @@ static int nbd_extent_array_add(NBDExtentArray *ea,
return 0;
}
-static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, NBDExtentArray *ea)
+static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ NBDExtentArray *ea)
{
while (bytes) {
uint32_t flags;
int64_t num;
-int ret = bdrv_block_status_above(bs, NULL, offset, bytes, ,
- NULL, NULL);
+int ret = bdrv_co_block_status_above(bs, NULL, offset, bytes, ,
+ NULL, NULL);
if (ret < 0) {
return ret;
@@ -2168,13 +2169,14 @@ static int blockstatus_to_extents(BlockDriverState *bs,
uint64_t offset,
return 0;
}
-static int blockalloc_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, NBDExtentArray *ea)
+static int coroutine_fn blockalloc_to_extents(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ NBDExtentArray *ea)
{
while (bytes) {
int64_t num;
-int ret = bdrv_is_allocated_above(bs, NULL, false, offset, bytes,
- );
+int ret = bdrv_co_is_allocated_above(bs, NULL, false, offset, bytes,
+ );
if (ret < 0) {
return ret;
@@ -2220,11 +,12 @@ static int nbd_co_send_extents(NBDClient *client,
uint64_t handle,
}
/* Get block status from the exported device and send it to the client */
-static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
-BlockDriverState *bs, uint64_t offset,
-uint32_t length, bool dont_fragment,
-bool last, uint32_t context_id,
-Error **errp)
+static int
+coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle,
+ BlockDriverState *bs, uint64_t offset,
+ uint32_t length, bool dont_fragment,
+ bool last, uint32_t context_id,
+ Error **errp)
{
int ret;
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
--
2.31.1
[PATCH v7 11/14] block-coroutine-wrapper.py: support functions without bs arg
Right now, we take the first parameter of the function to get the
BlockDriverState to pass to bdrv_poll_co(), that internally calls
functions that figure in which aiocontext the coroutine should run.
However, it is useless to pass a bs just to get its own AioContext,
so instead pass it directly, and default to the main loop if no
BlockDriverState is passed as parameter.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/block-gen.h | 6 +++---
scripts/block-coroutine-wrapper.py | 16
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/block/block-gen.h b/block/block-gen.h
index f80cf4897d..08d977f493 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -30,7 +30,7 @@
/* Base structure for argument packing structures */
typedef struct BdrvPollCo {
-BlockDriverState *bs;
+AioContext *ctx;
bool in_progress;
int ret;
Coroutine *co; /* Keep pointer here for debugging */
@@ -40,8 +40,8 @@ static inline int bdrv_poll_co(BdrvPollCo *s)
{
assert(!qemu_in_coroutine());
-bdrv_coroutine_enter(s->bs, s->co);
-BDRV_POLL_WHILE(s->bs, s->in_progress);
+aio_co_enter(s->ctx, s->co);
+AIO_WAIT_WHILE(s->ctx, s->in_progress);
return s->ret;
}
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index 2090c3bf73..f540003af1 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -75,12 +75,14 @@ def __init__(self, return_type: str, name: str, args: str,
t = self.args[0].type
if t == 'BlockDriverState *':
-bs = 'bs'
+ctx = 'bdrv_get_aio_context(bs)'
elif t == 'BdrvChild *':
-bs = 'child->bs'
+ctx = 'bdrv_get_aio_context(child->bs)'
+elif t == 'BlockBackend *':
+ctx = 'blk_get_aio_context(blk)'
else:
-bs = 'blk_bs(blk)'
-self.bs = bs
+ctx = 'qemu_get_aio_context()'
+self.ctx = ctx
def gen_list(self, format: str) -> str:
return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
@@ -127,7 +129,7 @@ def create_mixed_wrapper(func: FuncDecl) -> str:
return {name}({ func.gen_list('{name}') });
}} else {{
{struct_name} s = {{
-.poll_state.bs = {func.bs},
+.poll_state.ctx = {func.ctx},
.poll_state.in_progress = true,
{ func.gen_block('.{name} = {name},') }
@@ -150,7 +152,7 @@ def create_co_wrapper(func: FuncDecl) -> str:
int {func.name}({ func.gen_list('{decl}') })
{{
{struct_name} s = {{
-.poll_state.bs = {func.bs},
+.poll_state.ctx = {func.ctx},
.poll_state.in_progress = true,
{ func.gen_block('.{name} = {name},') }
@@ -166,8 +168,6 @@ def create_co_wrapper(func: FuncDecl) -> str:
def gen_wrapper(func: FuncDecl) -> str:
assert not '_co_' in func.name
assert func.return_type == 'int'
-assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *',
- 'BlockBackend *']
name = func.co_name
struct_name = func.struct_name
--
2.31.1
[PATCH v7 01/14] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers
bdrv_common_block_status_above() is a g_c_w, and it is being called by
many "wrapper" functions like bdrv_is_allocated(),
bdrv_is_allocated_above() and bdrv_block_status_above().
Because we want to eventually split the coroutine from non-coroutine
case in g_c_w, create duplicate wrappers that take care of directly
calling the same coroutine functions called in the g_c_w.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/block/block-io.h | 15 +++
block/io.c | 58 +---
2 files changed, 70 insertions(+), 3 deletions(-)
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 92aaa7c1e9..72919254cd 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -94,14 +94,29 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
int bdrv_block_status(BlockDriverState *bs, int64_t offset,
int64_t bytes, int64_t *pnum, int64_t *map,
BlockDriverState **file);
+
+int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
+BlockDriverState *base,
+int64_t offset, int64_t bytes,
+int64_t *pnum, int64_t *map,
+BlockDriverState **file);
int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
int64_t offset, int64_t bytes, int64_t *pnum,
int64_t *map, BlockDriverState **file);
+
+int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, int64_t *pnum);
int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
int64_t *pnum);
+
+int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
+BlockDriverState *base,
+bool include_base, int64_t offset,
+int64_t bytes, int64_t *pnum);
int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
bool include_base, int64_t offset, int64_t bytes,
int64_t *pnum);
+
int coroutine_fn bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset,
int64_t bytes);
diff --git a/block/io.c b/block/io.c
index 38e57d1f67..fb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2533,6 +2533,17 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
return ret;
}
+int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
+BlockDriverState *base,
+int64_t offset, int64_t bytes,
+int64_t *pnum, int64_t *map,
+BlockDriverState **file)
+{
+IO_CODE();
+return bdrv_co_common_block_status_above(bs, base, false, true, offset,
+ bytes, pnum, map, file, NULL);
+}
+
int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
int64_t offset, int64_t bytes, int64_t *pnum,
int64_t *map, BlockDriverState **file)
@@ -2578,6 +2589,22 @@ int coroutine_fn bdrv_co_is_zero_fast(BlockDriverState
*bs, int64_t offset,
return (pnum == bytes) && (ret & BDRV_BLOCK_ZERO);
}
+int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, int64_t *pnum)
+{
+int ret;
+int64_t dummy;
+IO_CODE();
+
+ret = bdrv_co_common_block_status_above(bs, bs, true, false, offset,
+bytes, pnum ? pnum : , NULL,
+NULL, NULL);
+if (ret < 0) {
+return ret;
+}
+return !!(ret & BDRV_BLOCK_ALLOCATED);
+}
+
int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
int64_t *pnum)
{
@@ -2594,6 +2621,29 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t
offset, int64_t bytes,
return !!(ret & BDRV_BLOCK_ALLOCATED);
}
+/* See bdrv_is_allocated_above for documentation */
+int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
+BlockDriverState *base,
+bool include_base, int64_t offset,
+int64_t bytes, int64_t *pnum)
+{
+int depth;
+int ret;
+IO_CODE();
+
+ret = bdrv_co_common_block_status_above(top, base, include_base, false,
+
[PATCH v7 07/14] block: distinguish between bdrv_create running in coroutine and not
Call two different functions depending on whether bdrv_create
is in coroutine or not, following the same pattern as
generated_co_wrapper functions.
This allows to also call the coroutine function directly,
without using CreateCo or relying in bdrv_create().
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block.c | 69 -
1 file changed, 34 insertions(+), 35 deletions(-)
diff --git a/block.c b/block.c
index 9d51e7b6e5..eb273dd2e3 100644
--- a/block.c
+++ b/block.c
@@ -528,63 +528,62 @@ typedef struct CreateCo {
Error *err;
} CreateCo;
-static void coroutine_fn bdrv_create_co_entry(void *opaque)
+static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+ QemuOpts *opts, Error **errp)
{
-Error *local_err = NULL;
int ret;
+GLOBAL_STATE_CODE();
+ERRP_GUARD();
+if (!drv->bdrv_co_create_opts) {
+error_setg(errp, "Driver '%s' does not support image creation",
+ drv->format_name);
+return -ENOTSUP;
+}
+
+ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
+if (ret < 0 && !*errp) {
+error_setg_errno(errp, -ret, "Could not create image");
+}
+
+return ret;
+}
+
+static void coroutine_fn bdrv_create_co_entry(void *opaque)
+{
CreateCo *cco = opaque;
-assert(cco->drv);
GLOBAL_STATE_CODE();
-ret = cco->drv->bdrv_co_create_opts(cco->drv,
-cco->filename, cco->opts, _err);
-error_propagate(>err, local_err);
-cco->ret = ret;
+cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, >err);
+aio_wait_kick();
}
int bdrv_create(BlockDriver *drv, const char* filename,
QemuOpts *opts, Error **errp)
{
-int ret;
-
GLOBAL_STATE_CODE();
-Coroutine *co;
-CreateCo cco = {
-.drv = drv,
-.filename = filename,
-.opts = opts,
-.ret = NOT_DONE,
-.err = NULL,
-};
-
-if (!drv->bdrv_co_create_opts) {
-error_setg(errp, "Driver '%s' does not support image creation",
drv->format_name);
-return -ENOTSUP;
-}
-
if (qemu_in_coroutine()) {
/* Fast-path if already in coroutine context */
-bdrv_create_co_entry();
+return bdrv_co_create(drv, filename, opts, errp);
} else {
+Coroutine *co;
+CreateCo cco = {
+.drv = drv,
+.filename = filename,
+.opts = opts,
+.ret = NOT_DONE,
+.err = NULL,
+};
+
co = qemu_coroutine_create(bdrv_create_co_entry, );
qemu_coroutine_enter(co);
while (cco.ret == NOT_DONE) {
aio_poll(qemu_get_aio_context(), true);
}
+error_propagate(errp, cco.err);
+return cco.ret;
}
-
-ret = cco.ret;
-if (ret < 0) {
-if (cco.err) {
-error_propagate(errp, cco.err);
-} else {
-error_setg_errno(errp, -ret, "Could not create image");
-}
-}
-
-return ret;
}
/**
--
2.31.1
[PATCH v7 06/14] block: avoid duplicating filename string in bdrv_create
We know that the string will stay around until the function
returns, and the parameter of drv->bdrv_co_create_opts is const char*,
so it must not be modified either.
Suggested-by: Kevin Wolf
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 8c9f4ee37c..9d51e7b6e5 100644
--- a/block.c
+++ b/block.c
@@ -553,7 +553,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
Coroutine *co;
CreateCo cco = {
.drv = drv,
-.filename = g_strdup(filename),
+.filename = filename,
.opts = opts,
.ret = NOT_DONE,
.err = NULL,
@@ -561,8 +561,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
if (!drv->bdrv_co_create_opts) {
error_setg(errp, "Driver '%s' does not support image creation",
drv->format_name);
-ret = -ENOTSUP;
-goto out;
+return -ENOTSUP;
}
if (qemu_in_coroutine()) {
@@ -585,8 +584,6 @@ int bdrv_create(BlockDriver *drv, const char* filename,
}
}
-out:
-g_free(cco.filename);
return ret;
}
--
2.31.1
[PATCH v7 00/14] Still more coroutine and various fixes in block layer
This is a dump of all minor coroutine-related fixes found while looking around and testing various things in the QEMU block layer. Patches aim to: - add missing coroutine_fn annotation to the functions - simplify to avoid the typical "if in coroutine: fn() // else create_coroutine(fn)" already present in generated_co_wraper functions. - make sure that if a BlockDriver callback is defined as coroutine_fn, then it is always running in a coroutine. This serie is based on Kevin Wolf's series "block: Simplify drain". Based-on: <20221108123738.530873-1-kw...@redhat.com> Emanuele --- v6: * use different naming for block-coroutine-wrapper annotations * fix minor typos and patch odering v5: * add missing reviewed-by from Paolo * minor indentation fixes * use when possible _co_, but do not create new g_c_w. It will be done in future series * introduce QEMU_IN_COROUTINE * reorder patches * rebase on kevin block branch + v2 from "block: Simplify drain" v4: * use v2 commit messages * introduce coroutine_wrapper to simplify patches v3: * Remove patch 1, base on kevin "drain semplification serie" v2: * clarified commit message in patches 2/3/6 on why we add coroutine_fn Emanuele Giuseppe Esposito (14): block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers block-copy: add coroutine_fn annotations nbd/server.c: add coroutine_fn annotations block-backend: replace bdrv_*_above with blk_*_above block/vmdk: add coroutine_fn annotations block: avoid duplicating filename string in bdrv_create block: distinguish between bdrv_create running in coroutine and not block: bdrv_create_file is a coroutine_fn block: rename generated_co_wrapper in co_wrapper_mixed block-coroutine-wrapper.py: introduce co_wrapper block-coroutine-wrapper.py: support functions without bs arg block-coroutine-wrapper.py: support also basic return types block: convert bdrv_create to co_wrapper block/dirty-bitmap: convert coroutine-only functions to co_wrapper docs/devel/block-coroutine-wrapper.rst | 6 +- block/block-gen.h | 11 +-- block/coroutines.h | 4 +- include/block/block-common.h | 20 ++-- include/block/block-copy.h | 5 +- include/block/block-global-state.h | 11 ++- include/block/block-io.h | 69 +- include/block/dirty-bitmap.h | 10 +- include/sysemu/block-backend-io.h | 77 +--- block.c| 64 +++-- block/block-backend.c | 21 + block/block-copy.c | 21 +++-- block/commit.c | 4 +- block/crypto.c | 2 +- block/dirty-bitmap.c | 88 +- block/io.c | 58 +++- block/parallels.c | 2 +- block/qcow.c | 2 +- block/qcow2.c | 4 +- block/qed.c| 2 +- block/raw-format.c | 2 +- block/vdi.c| 2 +- block/vhdx.c | 2 +- block/vmdk.c | 38 block/vpc.c| 2 +- nbd/server.c | 47 +- block/meson.build | 1 + scripts/block-coroutine-wrapper.py | 123 + 28 files changed, 375 insertions(+), 323 deletions(-) -- 2.31.1
Re: [PATCH v6 10/14] block-coroutine-wrapper.py: introduce co_wrapper
Am 25/11/2022 um 21:32 schrieb Vladimir Sementsov-Ogievskiy:
>
>> class FuncDecl:
>> - def __init__(self, return_type: str, name: str, args: str) -> None:
>> + def __init__(self, return_type: str, name: str, args: str,
>> + variant: str) -> None:
>
> I'd prefer mixed: bool parameter instead, to be more strict.
>
>> self.return_type = return_type.strip()
>> self.name = name.strip()
>> + self.struct_name = snake_to_camel(self.name)
>> self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
>> + self.create_only_co = True
>> +
>> + if 'mixed' in variant:
>> + self.create_only_co = False
>
> hmm, just
>
> self.create_only_co = 'mixed' not in variant
>
> ? And even better with boolean argument.
>
>> +
>> + subsystem, subname = self.name.split('_', 1)
>> + self.co_name = f'{subsystem}_co_{subname}'
>> +
>> + t = self.args[0].type
>> + if t == 'BlockDriverState *':
>> + bs = 'bs'
>> + elif t == 'BdrvChild *':
>> + bs = 'child->bs'
>> + else:
>> + bs = 'blk_bs(blk)'
>> + self.bs = bs
>> def gen_list(self, format: str) -> str:
>> return ', '.join(format.format_map(arg.__dict__) for arg in
>> self.args)
>> @@ -74,8 +92,9 @@ def gen_block(self, format: str) -> str:
>> return '\n'.join(format.format_map(arg.__dict__) for arg in
>> self.args)
>> -# Match wrappers declared with a co_wrapper_mixed mark
>> -func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*'
>> +# Match wrappers declared with a co_wrapper mark
>> +func_decl_re = re.compile(r'^int\s*co_wrapper'
>> + r'(?P(_[a-z][a-z0-9_]*)?)\s*'
>
> Why you allow everything here?
> I'd write just
>
> (?P(_mixed)?)
>
> or
>
> (?Pco_wrapper(_mixed)?)
Ok you couldn't possibly know that, but we are also adding other type of
"variants":
co_wrapper
co_wrapper_mixed
co_wrapper_bdrv_rdlock
co_wrapper_mixed_bdrv_rdlock
Therefore I need to keep variant : str and the regex as it is, but maybe
get rid of the if else condition. I'll change the docstring of course.
If you want to know more, see the thread in
"[PATCH 00/15] Protect the block layer with a rwlock: part 3"
Thank you,
Emanuele
Re: [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not
Am 25/11/2022 um 19:03 schrieb Vladimir Sementsov-Ogievskiy:
> On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
>> Call two different functions depending on whether bdrv_create
>> is in coroutine or not, following the same pattern as
>> generated_co_wrapper functions.
>>
>> This allows to also call the coroutine function directly,
>> without using CreateCo or relying in bdrv_create().
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> Reviewed-by: Kevin Wolf
>> ---
>> block.c | 71 +
>> 1 file changed, 36 insertions(+), 35 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 9d51e7b6e5..2cf50b37c4 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -528,63 +528,64 @@ typedef struct CreateCo {
>> Error *err;
>> } CreateCo;
>> -static void coroutine_fn bdrv_create_co_entry(void *opaque)
>> +static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char
>> *filename,
>> + QemuOpts *opts, Error **errp)
>> {
>> - Error *local_err = NULL;
>> int ret;
>> + GLOBAL_STATE_CODE();
>> + ERRP_GUARD();
>> + assert(*errp == NULL);
>> + assert(drv);
>
> Why we need these two assertions? These are general assumptions, and we
> don't assert it in all functions. Dereference of NULL will crash not
> worse than assertion. I'd drop them.
>
>> +
>> + if (!drv->bdrv_co_create_opts) {
>> + error_setg(errp, "Driver '%s' does not support image creation",
>> + drv->format_name);
>> + return -ENOTSUP;
>> + }
>> +
>> + ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
>>
>
> and this empty line, looks accidental.
>
> Offtopic: hope one day we fix *open* functions to always set errp on
> error paths.
>
>> + if (ret < 0 && !*errp) {
>> + error_setg_errno(errp, -ret, "Could not create image");
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void coroutine_fn bdrv_create_co_entry(void *opaque)
>> +{
>> CreateCo *cco = opaque;
>> - assert(cco->drv);
>> GLOBAL_STATE_CODE();
>> - ret = cco->drv->bdrv_co_create_opts(cco->drv,
>> - cco->filename, cco->opts,
>> _err);
>> - error_propagate(>err, local_err);
>> - cco->ret = ret;
>> + cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts,
>> >err);
>
> We need aio_wait_kick() call here, like in other co_entry() functions.
> Otherwise we may stuck in aio_poll()
>
> with it:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
> Hmm actually, we can simply merge this patch into patch 13 (and move 08
> to be after 13). Why to refactor bdrv_create twice?
>
I think I'll leave the patches separate, otherwise I am worried that it
will be difficult to understand what is happending and why when we merge
these 2 operations (move all logic to _co_ and using co_wrapper) together.
I agree with the rest.
Thank you,
Emanuele
[PATCH v6 06/14] block: avoid duplicating filename string in bdrv_create
We know that the string will stay around until the function
returns, and the parameter of drv->bdrv_co_create_opts is const char*,
so it must not be modified either.
Suggested-by: Kevin Wolf
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
---
block.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 8c9f4ee37c..9d51e7b6e5 100644
--- a/block.c
+++ b/block.c
@@ -553,7 +553,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
Coroutine *co;
CreateCo cco = {
.drv = drv,
-.filename = g_strdup(filename),
+.filename = filename,
.opts = opts,
.ret = NOT_DONE,
.err = NULL,
@@ -561,8 +561,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
if (!drv->bdrv_co_create_opts) {
error_setg(errp, "Driver '%s' does not support image creation",
drv->format_name);
-ret = -ENOTSUP;
-goto out;
+return -ENOTSUP;
}
if (qemu_in_coroutine()) {
@@ -585,8 +584,6 @@ int bdrv_create(BlockDriver *drv, const char* filename,
}
}
-out:
-g_free(cco.filename);
return ret;
}
--
2.31.1
[PATCH v6 10/14] block-coroutine-wrapper.py: introduce co_wrapper
This new annotation creates just a function wrapper that creates
a new coroutine. It assumes the caller is not a coroutine.
It will be the default annotation to be used in the future.
This is much better as c_w_mixed, because it is clear if the caller
is a coroutine or not, and provides the advantage of automating
the code creation. In the future all c_w_mixed functions will be
substituted by co_wrapper.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
---
docs/devel/block-coroutine-wrapper.rst | 6 +-
include/block/block-common.h | 8 +-
scripts/block-coroutine-wrapper.py | 109 +
3 files changed, 85 insertions(+), 38 deletions(-)
diff --git a/docs/devel/block-coroutine-wrapper.rst
b/docs/devel/block-coroutine-wrapper.rst
index 64acc8d65d..6dd2cdcab3 100644
--- a/docs/devel/block-coroutine-wrapper.rst
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -26,12 +26,12 @@ called ``bdrv_foo()``. In this case the script
can help. To
trigger the generation:
1. You need ``bdrv_foo`` declaration somewhere (for example, in
- ``block/coroutines.h``) with the ``co_wrapper_mixed`` mark,
+ ``block/coroutines.h``) with the ``co_wrapper`` mark,
like this:
.. code-block:: c
-int co_wrapper_mixed bdrv_foo();
+int co_wrapper bdrv_foo();
2. You need to feed this declaration to block-coroutine-wrapper script.
For this, add the .h (or .c) file with the declaration to the
@@ -46,7 +46,7 @@ Links
1. The script location is ``scripts/block-coroutine-wrapper.py``.
-2. Generic place for private ``co_wrapper_mixed`` declarations is
+2. Generic place for private ``co_wrapper`` declarations is
``block/coroutines.h``, for public declarations:
``include/block/block.h``
diff --git a/include/block/block-common.h b/include/block/block-common.h
index ec2309055b..847e4d4626 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -42,9 +42,13 @@
*
* Usage: read docs/devel/block-coroutine-wrapper.rst
*
- * co_wrapper_mixed functions can be called by both coroutine and
- * non-coroutine context.
+ * There are 2 kind of specifiers:
+ * - co_wrapper functions can be called by only non-coroutine context, because
+ * they always generate a new coroutine.
+ * - co_wrapper_mixed functions can be called by both coroutine and
+ * non-coroutine context.
*/
+#define co_wrapper
#define co_wrapper_mixed
/* block.c */
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index 56e6425356..7972d3fe01 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -2,7 +2,7 @@
"""Generate coroutine wrappers for block subsystem.
The program parses one or several concatenated c files from stdin,
-searches for functions with the 'co_wrapper_mixed' specifier
+searches for functions with the 'co_wrapper' specifier
and generates corresponding wrappers on stdout.
Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
@@ -62,10 +62,28 @@ def __init__(self, param_decl: str) -> None:
class FuncDecl:
-def __init__(self, return_type: str, name: str, args: str) -> None:
+def __init__(self, return_type: str, name: str, args: str,
+ variant: str) -> None:
self.return_type = return_type.strip()
self.name = name.strip()
+self.struct_name = snake_to_camel(self.name)
self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
+self.create_only_co = True
+
+if 'mixed' in variant:
+self.create_only_co = False
+
+subsystem, subname = self.name.split('_', 1)
+self.co_name = f'{subsystem}_co_{subname}'
+
+t = self.args[0].type
+if t == 'BlockDriverState *':
+bs = 'bs'
+elif t == 'BdrvChild *':
+bs = 'child->bs'
+else:
+bs = 'blk_bs(blk)'
+self.bs = bs
def gen_list(self, format: str) -> str:
return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
@@ -74,8 +92,9 @@ def gen_block(self, format: str) -> str:
return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
-# Match wrappers declared with a co_wrapper_mixed mark
-func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*'
+# Match wrappers declared with a co_wrapper mark
+func_decl_re = re.compile(r'^int\s*co_wrapper'
+ r'(?P(_[a-z][a-z0-9_]*)?)\s*'
r'(?P[a-z][a-z0-9_]*)'
r'\((?P[^)]*)\);$', re.MULTILINE)
@@ -84,7 +103,8 @@ def func_decl_iter(text: str) -> Iterator:
for m in func_decl_re.finditer(text):
yield FuncDecl(return_type='int',
name=m.group('wrapper_name'),
- args=m.group('args'))
+ args=m.group('args'),
+ variant=m.group('variant'))
def s
[PATCH v6 14/14] block/dirty-bitmap: convert coroutine-only functions to co_wrapper
bdrv_can_store_new_dirty_bitmap and bdrv_remove_persistent_dirty_bitmap
check if they are running in a coroutine, directly calling the
coroutine callback if it's the case.
Except that no coroutine calls such functions, therefore that check
can be removed, and function creation can be offloaded to
c_w.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
---
block/dirty-bitmap.c | 88 +---
block/meson.build| 1 +
include/block/block-common.h | 5 +-
include/block/block-io.h | 10 +++-
include/block/dirty-bitmap.h | 10 +++-
5 files changed, 22 insertions(+), 92 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bf3dc0512a..21cf592889 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -388,7 +388,7 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
* not fail.
* This function doesn't release corresponding BdrvDirtyBitmap.
*/
-static int coroutine_fn
+int coroutine_fn
bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
Error **errp)
{
@@ -399,45 +399,6 @@ bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState
*bs, const char *name,
return 0;
}
-typedef struct BdrvRemovePersistentDirtyBitmapCo {
-BlockDriverState *bs;
-const char *name;
-Error **errp;
-int ret;
-} BdrvRemovePersistentDirtyBitmapCo;
-
-static void coroutine_fn
-bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
-{
-BdrvRemovePersistentDirtyBitmapCo *s = opaque;
-
-s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp);
-aio_wait_kick();
-}
-
-int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
-Error **errp)
-{
-if (qemu_in_coroutine()) {
-return bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
-} else {
-Coroutine *co;
-BdrvRemovePersistentDirtyBitmapCo s = {
-.bs = bs,
-.name = name,
-.errp = errp,
-.ret = -EINPROGRESS,
-};
-
-co =
qemu_coroutine_create(bdrv_co_remove_persistent_dirty_bitmap_entry,
- );
-bdrv_coroutine_enter(bs, co);
-BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS);
-
-return s.ret;
-}
-}
-
bool
bdrv_supports_persistent_dirty_bitmap(BlockDriverState *bs)
{
@@ -447,7 +408,7 @@ bdrv_supports_persistent_dirty_bitmap(BlockDriverState *bs)
return false;
}
-static bool coroutine_fn
+bool coroutine_fn
bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
uint32_t granularity, Error **errp)
{
@@ -470,51 +431,6 @@ bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
const char *name,
return drv->bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity,
errp);
}
-typedef struct BdrvCanStoreNewDirtyBitmapCo {
-BlockDriverState *bs;
-const char *name;
-uint32_t granularity;
-Error **errp;
-bool ret;
-
-bool in_progress;
-} BdrvCanStoreNewDirtyBitmapCo;
-
-static void coroutine_fn bdrv_co_can_store_new_dirty_bitmap_entry(void *opaque)
-{
-BdrvCanStoreNewDirtyBitmapCo *s = opaque;
-
-s->ret = bdrv_co_can_store_new_dirty_bitmap(s->bs, s->name, s->granularity,
-s->errp);
-s->in_progress = false;
-aio_wait_kick();
-}
-
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
- uint32_t granularity, Error **errp)
-{
-IO_CODE();
-if (qemu_in_coroutine()) {
-return bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
-} else {
-Coroutine *co;
-BdrvCanStoreNewDirtyBitmapCo s = {
-.bs = bs,
-.name = name,
-.granularity = granularity,
-.errp = errp,
-.in_progress = true,
-};
-
-co = qemu_coroutine_create(bdrv_co_can_store_new_dirty_bitmap_entry,
- );
-bdrv_coroutine_enter(bs, co);
-BDRV_POLL_WHILE(bs, s.in_progress);
-
-return s.ret;
-}
-}
-
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
bdrv_dirty_bitmaps_lock(bitmap->bs);
diff --git a/block/meson.build b/block/meson.build
index b7c68b83a3..c48a59571e 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -137,6 +137,7 @@ block_gen_c = custom_target('block-gen.c',
output: 'block-gen.c',
input: files(
'../include/block/block-io.h',
+ '../include/block/dirty-bitmap.h',
'../include/block/block-global-state.h',
'
[PATCH v6 00/14] Still more coroutine and various fixes in block layer
This is a dump of all minor coroutine-related fixes found while looking around and testing various things in the QEMU block layer. Patches aim to: - add missing coroutine_fn annotation to the functions - simplify to avoid the typical "if in coroutine: fn() // else create_coroutine(fn)" already present in generated_co_wraper functions. - make sure that if a BlockDriver callback is defined as coroutine_fn, then it is always running in a coroutine. This serie is based on Kevin Wolf's series "block: Simplify drain". Based-on: <20221108123738.530873-1-kw...@redhat.com> Emanuele --- v6: * use different naming for block-coroutine-wrapper annotations * fix minor typos and patch odering v5: * add missing reviewed-by from Paolo * minor indentation fixes * use when possible _co_, but do not create new g_c_w. It will be done in future series * introduce QEMU_IN_COROUTINE * reorder patches * rebase on kevin block branch + v2 from "block: Simplify drain" v4: * use v2 commit messages * introduce coroutine_wrapper to simplify patches v3: * Remove patch 1, base on kevin "drain semplification serie" v2: * clarified commit message in patches 2/3/6 on why we add coroutine_fn Emanuele Giuseppe Esposito (14): block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers block-copy: add missing coroutine_fn annotations nbd/server.c: add missing coroutine_fn annotations block-backend: replace bdrv_*_above with blk_*_above block/vmdk: add missing coroutine_fn annotations block: avoid duplicating filename string in bdrv_create block: distinguish between bdrv_create running in coroutine and not block: bdrv_create_file is a coroutine_fn block: rename generated_co_wrapper in co_wrapper_mixed block-coroutine-wrapper.py: introduce co_wrapper block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter block-coroutine-wrapper.py: support also basic return types block: convert bdrv_create to co_wrapper block/dirty-bitmap: convert coroutine-only functions to co_wrapper block.c| 65 +++-- block/block-backend.c | 21 + block/block-copy.c | 21 +++-- block/block-gen.h | 11 +-- block/commit.c | 4 +- block/coroutines.h | 4 +- block/crypto.c | 2 +- block/dirty-bitmap.c | 88 +- block/io.c | 58 +++- block/meson.build | 1 + block/parallels.c | 2 +- block/qcow.c | 2 +- block/qcow2.c | 4 +- block/qed.c| 2 +- block/raw-format.c | 2 +- block/vdi.c| 2 +- block/vhdx.c | 2 +- block/vmdk.c | 38 block/vpc.c| 2 +- docs/devel/block-coroutine-wrapper.rst | 6 +- include/block/block-common.h | 20 ++-- include/block/block-copy.h | 5 +- include/block/block-global-state.h | 11 ++- include/block/block-io.h | 69 +- include/block/dirty-bitmap.h | 10 +- include/sysemu/block-backend-io.h | 77 +--- nbd/server.c | 47 +- scripts/block-coroutine-wrapper.py | 122 + 28 files changed, 376 insertions(+), 322 deletions(-) -- 2.31.1
[PATCH v6 01/14] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers
bdrv_common_block_status_above() is a g_c_w, and it is being called by
many "wrapper" functions like bdrv_is_allocated(),
bdrv_is_allocated_above() and bdrv_block_status_above().
Because we want to eventually split the coroutine from non-coroutine
case in g_c_w, create duplicate wrappers that take care of directly
calling the same coroutine functions called in the g_c_w.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
---
block/io.c | 58 +---
include/block/block-io.h | 15 +++
2 files changed, 70 insertions(+), 3 deletions(-)
diff --git a/block/io.c b/block/io.c
index 38e57d1f67..fb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2533,6 +2533,17 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
return ret;
}
+int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
+BlockDriverState *base,
+int64_t offset, int64_t bytes,
+int64_t *pnum, int64_t *map,
+BlockDriverState **file)
+{
+IO_CODE();
+return bdrv_co_common_block_status_above(bs, base, false, true, offset,
+ bytes, pnum, map, file, NULL);
+}
+
int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
int64_t offset, int64_t bytes, int64_t *pnum,
int64_t *map, BlockDriverState **file)
@@ -2578,6 +2589,22 @@ int coroutine_fn bdrv_co_is_zero_fast(BlockDriverState
*bs, int64_t offset,
return (pnum == bytes) && (ret & BDRV_BLOCK_ZERO);
}
+int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, int64_t *pnum)
+{
+int ret;
+int64_t dummy;
+IO_CODE();
+
+ret = bdrv_co_common_block_status_above(bs, bs, true, false, offset,
+bytes, pnum ? pnum : , NULL,
+NULL, NULL);
+if (ret < 0) {
+return ret;
+}
+return !!(ret & BDRV_BLOCK_ALLOCATED);
+}
+
int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
int64_t *pnum)
{
@@ -2594,6 +2621,29 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t
offset, int64_t bytes,
return !!(ret & BDRV_BLOCK_ALLOCATED);
}
+/* See bdrv_is_allocated_above for documentation */
+int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
+BlockDriverState *base,
+bool include_base, int64_t offset,
+int64_t bytes, int64_t *pnum)
+{
+int depth;
+int ret;
+IO_CODE();
+
+ret = bdrv_co_common_block_status_above(top, base, include_base, false,
+offset, bytes, pnum, NULL, NULL,
+);
+if (ret < 0) {
+return ret;
+}
+
+if (ret & BDRV_BLOCK_ALLOCATED) {
+return depth;
+}
+return 0;
+}
+
/*
* Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP]
*
@@ -2617,10 +2667,12 @@ int bdrv_is_allocated_above(BlockDriverState *top,
int64_t bytes, int64_t *pnum)
{
int depth;
-int ret = bdrv_common_block_status_above(top, base, include_base, false,
- offset, bytes, pnum, NULL, NULL,
- );
+int ret;
IO_CODE();
+
+ret = bdrv_common_block_status_above(top, base, include_base, false,
+ offset, bytes, pnum, NULL, NULL,
+ );
if (ret < 0) {
return ret;
}
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 92aaa7c1e9..72919254cd 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -94,14 +94,29 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
int bdrv_block_status(BlockDriverState *bs, int64_t offset,
int64_t bytes, int64_t *pnum, int64_t *map,
BlockDriverState **file);
+
+int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
+BlockDriverState *base,
+int64_t offset, int64_t bytes,
+int64_t *pnum, int64_t *map,
+BlockDriverState **file);
int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
int64_t offset, int64_t bytes, int64_t *pnum,
int64_t *map, BlockDriver
[PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not
Call two different functions depending on whether bdrv_create
is in coroutine or not, following the same pattern as
generated_co_wrapper functions.
This allows to also call the coroutine function directly,
without using CreateCo or relying in bdrv_create().
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
---
block.c | 71 +
1 file changed, 36 insertions(+), 35 deletions(-)
diff --git a/block.c b/block.c
index 9d51e7b6e5..2cf50b37c4 100644
--- a/block.c
+++ b/block.c
@@ -528,63 +528,64 @@ typedef struct CreateCo {
Error *err;
} CreateCo;
-static void coroutine_fn bdrv_create_co_entry(void *opaque)
+static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+ QemuOpts *opts, Error **errp)
{
-Error *local_err = NULL;
int ret;
+GLOBAL_STATE_CODE();
+ERRP_GUARD();
+assert(*errp == NULL);
+assert(drv);
+
+if (!drv->bdrv_co_create_opts) {
+error_setg(errp, "Driver '%s' does not support image creation",
+ drv->format_name);
+return -ENOTSUP;
+}
+
+ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
+if (ret < 0 && !*errp) {
+error_setg_errno(errp, -ret, "Could not create image");
+}
+
+return ret;
+}
+
+static void coroutine_fn bdrv_create_co_entry(void *opaque)
+{
CreateCo *cco = opaque;
-assert(cco->drv);
GLOBAL_STATE_CODE();
-ret = cco->drv->bdrv_co_create_opts(cco->drv,
-cco->filename, cco->opts, _err);
-error_propagate(>err, local_err);
-cco->ret = ret;
+cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, >err);
}
int bdrv_create(BlockDriver *drv, const char* filename,
QemuOpts *opts, Error **errp)
{
-int ret;
-
GLOBAL_STATE_CODE();
-Coroutine *co;
-CreateCo cco = {
-.drv = drv,
-.filename = filename,
-.opts = opts,
-.ret = NOT_DONE,
-.err = NULL,
-};
-
-if (!drv->bdrv_co_create_opts) {
-error_setg(errp, "Driver '%s' does not support image creation",
drv->format_name);
-return -ENOTSUP;
-}
-
if (qemu_in_coroutine()) {
/* Fast-path if already in coroutine context */
-bdrv_create_co_entry();
+return bdrv_co_create(drv, filename, opts, errp);
} else {
+Coroutine *co;
+CreateCo cco = {
+.drv = drv,
+.filename = filename,
+.opts = opts,
+.ret = NOT_DONE,
+.err = NULL,
+};
+
co = qemu_coroutine_create(bdrv_create_co_entry, );
qemu_coroutine_enter(co);
while (cco.ret == NOT_DONE) {
aio_poll(qemu_get_aio_context(), true);
}
+error_propagate(errp, cco.err);
+return cco.ret;
}
-
-ret = cco.ret;
-if (ret < 0) {
-if (cco.err) {
-error_propagate(errp, cco.err);
-} else {
-error_setg_errno(errp, -ret, "Could not create image");
-}
-}
-
-return ret;
}
/**
--
2.31.1
[PATCH v6 11/14] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
Right now, we take the first parameter of the function to get the
BlockDriverState to pass to bdrv_poll_co(), that internally calls
functions that figure in which aiocontext the coroutine should run.
However, it is useless to pass a bs just to get its own AioContext,
so instead pass it directly, and default to the main loop if no
BlockDriverState is passed as parameter.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/block-gen.h | 6 +++---
scripts/block-coroutine-wrapper.py | 16
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/block/block-gen.h b/block/block-gen.h
index f80cf4897d..08d977f493 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -30,7 +30,7 @@
/* Base structure for argument packing structures */
typedef struct BdrvPollCo {
-BlockDriverState *bs;
+AioContext *ctx;
bool in_progress;
int ret;
Coroutine *co; /* Keep pointer here for debugging */
@@ -40,8 +40,8 @@ static inline int bdrv_poll_co(BdrvPollCo *s)
{
assert(!qemu_in_coroutine());
-bdrv_coroutine_enter(s->bs, s->co);
-BDRV_POLL_WHILE(s->bs, s->in_progress);
+aio_co_enter(s->ctx, s->co);
+AIO_WAIT_WHILE(s->ctx, s->in_progress);
return s->ret;
}
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index 7972d3fe01..61a718ea2f 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -78,12 +78,14 @@ def __init__(self, return_type: str, name: str, args: str,
t = self.args[0].type
if t == 'BlockDriverState *':
-bs = 'bs'
+ctx = 'bdrv_get_aio_context(bs)'
elif t == 'BdrvChild *':
-bs = 'child->bs'
+ctx = 'bdrv_get_aio_context(child->bs)'
+elif t == 'BlockBackend *':
+ctx = 'blk_get_aio_context(blk)'
else:
-bs = 'blk_bs(blk)'
-self.bs = bs
+ctx = 'qemu_get_aio_context()'
+self.ctx = ctx
def gen_list(self, format: str) -> str:
return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
@@ -128,7 +130,7 @@ def create_g_c_w(func: FuncDecl) -> str:
return {name}({ func.gen_list('{name}') });
}} else {{
{struct_name} s = {{
-.poll_state.bs = {func.bs},
+.poll_state.ctx = {func.ctx},
.poll_state.in_progress = true,
{ func.gen_block('.{name} = {name},') }
@@ -149,7 +151,7 @@ def create_coroutine_only(func: FuncDecl) -> str:
int {func.name}({ func.gen_list('{decl}') })
{{
{struct_name} s = {{
-.poll_state.bs = {func.bs},
+.poll_state.ctx = {func.ctx},
.poll_state.in_progress = true,
{ func.gen_block('.{name} = {name},') }
@@ -165,8 +167,6 @@ def create_coroutine_only(func: FuncDecl) -> str:
def gen_wrapper(func: FuncDecl) -> str:
assert not '_co_' in func.name
assert func.return_type == 'int'
-assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *',
- 'BlockBackend *']
name = func.co_name
struct_name = func.struct_name
--
2.31.1
[PATCH v6 08/14] block: bdrv_create_file is a coroutine_fn
It is always called in coroutine_fn callbacks, therefore
it can directly call bdrv_co_create().
Rename it to bdrv_co_create_file too.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
---
block.c| 5 +++--
block/crypto.c | 2 +-
block/parallels.c | 2 +-
block/qcow.c | 2 +-
block/qcow2.c | 4 ++--
block/qed.c| 2 +-
block/raw-format.c | 2 +-
block/vdi.c| 2 +-
block/vhdx.c | 2 +-
block/vmdk.c | 2 +-
block/vpc.c| 2 +-
include/block/block-global-state.h | 3 ++-
12 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/block.c b/block.c
index 2cf50b37c4..772df02d54 100644
--- a/block.c
+++ b/block.c
@@ -723,7 +723,8 @@ out:
return ret;
}
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+int coroutine_fn bdrv_co_create_file(const char *filename, QemuOpts *opts,
+ Error **errp)
{
QemuOpts *protocol_opts;
BlockDriver *drv;
@@ -764,7 +765,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts,
Error **errp)
goto out;
}
-ret = bdrv_create(drv, filename, protocol_opts, errp);
+ret = bdrv_co_create(drv, filename, protocol_opts, errp);
out:
qemu_opts_del(protocol_opts);
qobject_unref(qdict);
diff --git a/block/crypto.c b/block/crypto.c
index 2fb8add458..bbeb9f437c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -703,7 +703,7 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(BlockDriver *drv,
}
/* Create protocol layer */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/parallels.c b/block/parallels.c
index fa08c1104b..bbea2f2221 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -646,7 +646,7 @@ static int coroutine_fn
parallels_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto done;
}
diff --git a/block/qcow.c b/block/qcow.c
index daa38839ab..18e17a5b12 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -973,7 +973,7 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver
*drv,
}
/* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 4dd3ff..7cc49a3a6c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3871,7 +3871,7 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver
*drv,
}
/* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto finish;
}
@@ -3886,7 +3886,7 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver
*drv,
/* Create and open an external data file (protocol layer) */
val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
if (val) {
-ret = bdrv_create_file(val, opts, errp);
+ret = bdrv_co_create_file(val, opts, errp);
if (ret < 0) {
goto finish;
}
diff --git a/block/qed.c b/block/qed.c
index c2691a85b1..9d54c8eec5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -778,7 +778,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(BlockDriver
*drv,
}
/* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/raw-format.c b/block/raw-format.c
index a68014ef0b..28905b09ee 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -433,7 +433,7 @@ static int coroutine_fn raw_co_create_opts(BlockDriver *drv,
QemuOpts *opts,
Error **errp)
{
-return bdrv_create_file(filename, opts, errp);
+return bdrv_co_create_file(filename, opts, errp);
}
static int raw_open(BlockDriverState *bs, QDict *options, int flags,
diff --git a/block/vdi.c b/block/vdi.c
index c0c111c4b9..479bcfe820 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -934,7 +934,7 @@ static int coroutine_fn vdi_co_create_opts(BlockDriver *drv,
qdict = qemu_opts_to_qdict_filtered(opts, NULL, _create_opts, true);
/* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, errp);
+ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto done;
[PATCH v6 04/14] block-backend: replace bdrv_*_above with blk_*_above
Avoid mixing bdrv_* functions with blk_*, so create blk_* counterparts
for bdrv_block_status_above and bdrv_is_allocated_above.
Note that since blk_co_block_status_above only calls the g_c_w function
bdrv_common_block_status_above and is marked as coroutine_fn, call
directly bdrv_co_common_block_status_above() to avoid using a g_c_w.
Same applies to blk_co_is_allocated_above.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/block-backend.c | 21
block/commit.c| 4 ++--
include/sysemu/block-backend-io.h | 9 +
nbd/server.c | 32 +++
4 files changed, 48 insertions(+), 18 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 742efa7955..1d2d8526ef 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1424,6 +1424,27 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk,
int64_t offset,
return blk_co_pwritev_part(blk, offset, bytes, qiov, 0, flags);
}
+int coroutine_fn blk_co_block_status_above(BlockBackend *blk,
+ BlockDriverState *base,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
+{
+IO_CODE();
+return bdrv_co_block_status_above(blk_bs(blk), base, offset, bytes, pnum,
+ map, file);
+}
+
+int coroutine_fn blk_co_is_allocated_above(BlockBackend *blk,
+ BlockDriverState *base,
+ bool include_base, int64_t offset,
+ int64_t bytes, int64_t *pnum)
+{
+IO_CODE();
+return bdrv_co_is_allocated_above(blk_bs(blk), base, include_base, offset,
+ bytes, pnum);
+}
+
typedef struct BlkRwCo {
BlockBackend *blk;
int64_t offset;
diff --git a/block/commit.c b/block/commit.c
index 0029b31944..b346341767 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -155,8 +155,8 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
break;
}
/* Copy if allocated above the base */
-ret = bdrv_is_allocated_above(blk_bs(s->top), s->base_overlay, true,
- offset, COMMIT_BUFFER_SIZE, );
+ret = blk_co_is_allocated_above(s->top, s->base_overlay, true,
+offset, COMMIT_BUFFER_SIZE, );
copy = (ret > 0);
trace_commit_one_iteration(s, offset, n, ret);
if (copy) {
diff --git a/include/sysemu/block-backend-io.h
b/include/sysemu/block-backend-io.h
index 50f5aa2e07..ee3eb12610 100644
--- a/include/sysemu/block-backend-io.h
+++ b/include/sysemu/block-backend-io.h
@@ -92,6 +92,15 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in,
int64_t off_in,
int64_t bytes, BdrvRequestFlags read_flags,
BdrvRequestFlags write_flags);
+int coroutine_fn blk_co_block_status_above(BlockBackend *blk,
+ BlockDriverState *base,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file);
+int coroutine_fn blk_co_is_allocated_above(BlockBackend *blk,
+ BlockDriverState *base,
+ bool include_base, int64_t offset,
+ int64_t bytes, int64_t *pnum);
/*
* "I/O or GS" API functions. These functions can run without
diff --git a/nbd/server.c b/nbd/server.c
index 4af5c793a7..c53c39560e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1991,7 +1991,7 @@ static int coroutine_fn
nbd_co_send_structured_error(NBDClient *client,
}
/* Do a sparse read and send the structured reply to the client.
- * Returns -errno if sending fails. bdrv_block_status_above() failure is
+ * Returns -errno if sending fails. blk_co_block_status_above() failure is
* reported to the client, at which point this function succeeds.
*/
static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
@@ -2007,10 +2007,10 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
while (progress < size) {
int64_t pnum;
-int status = bdrv_block_status_above(blk_bs(exp->common.blk), NULL,
- offset + progress,
- size - progress, , NULL,
- NULL);
+int status = blk_co_block_status_above(exp->common.blk, NULL,
+
[PATCH v6 03/14] nbd/server.c: add missing coroutine_fn annotations
These functions end up calling bdrv_*() implemented as generated_co_wrapper
functions.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we need to mark such functions coroutine_fn too.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Paolo Bonzini
---
nbd/server.c | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index ada16089f3..4af5c793a7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2141,14 +2141,15 @@ static int nbd_extent_array_add(NBDExtentArray *ea,
return 0;
}
-static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, NBDExtentArray *ea)
+static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ NBDExtentArray *ea)
{
while (bytes) {
uint32_t flags;
int64_t num;
-int ret = bdrv_block_status_above(bs, NULL, offset, bytes, ,
- NULL, NULL);
+int ret = bdrv_co_block_status_above(bs, NULL, offset, bytes, ,
+ NULL, NULL);
if (ret < 0) {
return ret;
@@ -2168,13 +2169,14 @@ static int blockstatus_to_extents(BlockDriverState *bs,
uint64_t offset,
return 0;
}
-static int blockalloc_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, NBDExtentArray *ea)
+static int coroutine_fn blockalloc_to_extents(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ NBDExtentArray *ea)
{
while (bytes) {
int64_t num;
-int ret = bdrv_is_allocated_above(bs, NULL, false, offset, bytes,
- );
+int ret = bdrv_co_is_allocated_above(bs, NULL, false, offset, bytes,
+ );
if (ret < 0) {
return ret;
@@ -2220,11 +,12 @@ static int nbd_co_send_extents(NBDClient *client,
uint64_t handle,
}
/* Get block status from the exported device and send it to the client */
-static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
-BlockDriverState *bs, uint64_t offset,
-uint32_t length, bool dont_fragment,
-bool last, uint32_t context_id,
-Error **errp)
+static int
+coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle,
+ BlockDriverState *bs, uint64_t offset,
+ uint32_t length, bool dont_fragment,
+ bool last, uint32_t context_id,
+ Error **errp)
{
int ret;
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
--
2.31.1
[PATCH v6 05/14] block/vmdk: add missing coroutine_fn annotations
These functions end up calling bdrv_create() implemented as generated_co_wrapper
functions.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we need to mark such functions coroutine_fn too.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Paolo Bonzini
Reviewed-by: Kevin Wolf
---
block/vmdk.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 26376352b9..0c32bf2e83 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2285,10 +2285,11 @@ exit:
return ret;
}
-static int vmdk_create_extent(const char *filename, int64_t filesize,
- bool flat, bool compress, bool zeroed_grain,
- BlockBackend **pbb,
- QemuOpts *opts, Error **errp)
+static int coroutine_fn vmdk_create_extent(const char *filename,
+ int64_t filesize, bool flat,
+ bool compress, bool zeroed_grain,
+ BlockBackend **pbb,
+ QemuOpts *opts, Error **errp)
{
int ret;
BlockBackend *blk = NULL;
@@ -2366,14 +2367,14 @@ static int filename_decompose(const char *filename,
char *path, char *prefix,
* non-split format.
* idx >= 1: get the n-th extent if in a split subformat
*/
-typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
- int idx,
- bool flat,
- bool split,
- bool compress,
- bool zeroed_grain,
- void *opaque,
- Error **errp);
+typedef BlockBackend * coroutine_fn (*vmdk_create_extent_fn)(int64_t size,
+ int idx,
+ bool flat,
+ bool split,
+ bool compress,
+ bool zeroed_grain,
+ void *opaque,
+ Error **errp);
static void vmdk_desc_add_extent(GString *desc,
const char *extent_line_fmt,
@@ -2616,7 +2617,7 @@ typedef struct {
QemuOpts *opts;
} VMDKCreateOptsData;
-static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
+static BlockBackend * coroutine_fn vmdk_co_create_opts_cb(int64_t size, int
idx,
bool flat, bool split, bool
compress,
bool zeroed_grain, void *opaque,
Error **errp)
@@ -2768,10 +2769,11 @@ exit:
return ret;
}
-static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
- bool flat, bool split, bool compress,
- bool zeroed_grain, void *opaque,
- Error **errp)
+static BlockBackend * coroutine_fn vmdk_co_create_cb(int64_t size, int idx,
+ bool flat, bool split,
+ bool compress,
+ bool zeroed_grain,
+ void *opaque, Error
**errp)
{
int ret;
BlockDriverState *bs;
--
2.31.1
[PATCH v6 12/14] block-coroutine-wrapper.py: support also basic return types
Extend the regex to cover also return type, pointers included.
This implies that the value returned by the function cannot be
a simple "int" anymore, but the custom return type.
Therefore remove poll_state->ret and instead use a per-function
custom "ret" field.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
---
block/block-gen.h | 5 +
scripts/block-coroutine-wrapper.py | 19 +++
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/block/block-gen.h b/block/block-gen.h
index 08d977f493..89b7daaa1f 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -32,18 +32,15 @@
typedef struct BdrvPollCo {
AioContext *ctx;
bool in_progress;
-int ret;
Coroutine *co; /* Keep pointer here for debugging */
} BdrvPollCo;
-static inline int bdrv_poll_co(BdrvPollCo *s)
+static inline void bdrv_poll_co(BdrvPollCo *s)
{
assert(!qemu_in_coroutine());
aio_co_enter(s->ctx, s->co);
AIO_WAIT_WHILE(s->ctx, s->in_progress);
-
-return s->ret;
}
#endif /* BLOCK_BLOCK_GEN_H */
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index 61a718ea2f..9d5d70a7dd 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -95,7 +95,8 @@ def gen_block(self, format: str) -> str:
# Match wrappers declared with a co_wrapper mark
-func_decl_re = re.compile(r'^int\s*co_wrapper'
+func_decl_re = re.compile(r'^(?P[a-zA-Z][a-zA-Z0-9_]* [*]?)'
+ r'\s*co_wrapper'
r'(?P(_[a-z][a-z0-9_]*)?)\s*'
r'(?P[a-z][a-z0-9_]*)'
r'\((?P[^)]*)\);$', re.MULTILINE)
@@ -103,7 +104,7 @@ def gen_block(self, format: str) -> str:
def func_decl_iter(text: str) -> Iterator:
for m in func_decl_re.finditer(text):
-yield FuncDecl(return_type='int',
+yield FuncDecl(return_type=m.group('return_type'),
name=m.group('wrapper_name'),
args=m.group('args'),
variant=m.group('variant'))
@@ -124,7 +125,7 @@ def create_g_c_w(func: FuncDecl) -> str:
name = func.co_name
struct_name = func.struct_name
return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
{{
if (qemu_in_coroutine()) {{
return {name}({ func.gen_list('{name}') });
@@ -138,7 +139,8 @@ def create_g_c_w(func: FuncDecl) -> str:
s.poll_state.co = qemu_coroutine_create({name}_entry, );
-return bdrv_poll_co(_state);
+bdrv_poll_co(_state);
+return s.ret;
}}
}}"""
@@ -148,7 +150,7 @@ def create_coroutine_only(func: FuncDecl) -> str:
name = func.co_name
struct_name = func.struct_name
return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
{{
{struct_name} s = {{
.poll_state.ctx = {func.ctx},
@@ -160,13 +162,13 @@ def create_coroutine_only(func: FuncDecl) -> str:
s.poll_state.co = qemu_coroutine_create({name}_entry, );
-return bdrv_poll_co(_state);
+bdrv_poll_co(_state);
+return s.ret;
}}"""
def gen_wrapper(func: FuncDecl) -> str:
assert not '_co_' in func.name
-assert func.return_type == 'int'
name = func.co_name
struct_name = func.struct_name
@@ -182,6 +184,7 @@ def gen_wrapper(func: FuncDecl) -> str:
typedef struct {struct_name} {{
BdrvPollCo poll_state;
+{func.return_type} ret;
{ func.gen_block('{decl};') }
}} {struct_name};
@@ -189,7 +192,7 @@ def gen_wrapper(func: FuncDecl) -> str:
{{
{struct_name} *s = opaque;
-s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
+s->ret = {name}({ func.gen_list('s->{name}') });
s->poll_state.in_progress = false;
aio_wait_kick();
--
2.31.1
[PATCH v6 09/14] block: rename generated_co_wrapper in co_wrapper_mixed
In preparation to the incoming new function specifiers,
rename g_c_w with a more meaningful name and document it.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/coroutines.h | 4 +-
docs/devel/block-coroutine-wrapper.rst | 6 +--
include/block/block-common.h | 11 +++--
include/block/block-io.h | 44 -
include/sysemu/block-backend-io.h | 68 +-
scripts/block-coroutine-wrapper.py | 6 +--
6 files changed, 71 insertions(+), 68 deletions(-)
diff --git a/block/coroutines.h b/block/coroutines.h
index 3a2bad564f..17da4db963 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -71,7 +71,7 @@ nbd_co_do_establish_connection(BlockDriverState *bs, bool
blocking,
* the "I/O or GS" API.
*/
-int generated_co_wrapper
+int co_wrapper_mixed
bdrv_common_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
bool include_base,
@@ -82,7 +82,7 @@ bdrv_common_block_status_above(BlockDriverState *bs,
int64_t *map,
BlockDriverState **file,
int *depth);
-int generated_co_wrapper
+int co_wrapper_mixed
nbd_do_establish_connection(BlockDriverState *bs, bool blocking, Error **errp);
#endif /* BLOCK_COROUTINES_H */
diff --git a/docs/devel/block-coroutine-wrapper.rst
b/docs/devel/block-coroutine-wrapper.rst
index 412851986b..64acc8d65d 100644
--- a/docs/devel/block-coroutine-wrapper.rst
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -26,12 +26,12 @@ called ``bdrv_foo()``. In this case the script
can help. To
trigger the generation:
1. You need ``bdrv_foo`` declaration somewhere (for example, in
- ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
+ ``block/coroutines.h``) with the ``co_wrapper_mixed`` mark,
like this:
.. code-block:: c
-int generated_co_wrapper bdrv_foo();
+int co_wrapper_mixed bdrv_foo();
2. You need to feed this declaration to block-coroutine-wrapper script.
For this, add the .h (or .c) file with the declaration to the
@@ -46,7 +46,7 @@ Links
1. The script location is ``scripts/block-coroutine-wrapper.py``.
-2. Generic place for private ``generated_co_wrapper`` declarations is
+2. Generic place for private ``co_wrapper_mixed`` declarations is
``block/coroutines.h``, for public declarations:
``include/block/block.h``
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 297704c1e9..ec2309055b 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -35,14 +35,17 @@
#include "qemu/transactions.h"
/*
- * generated_co_wrapper
+ * co_wrapper{*}: Function specifiers used by block-coroutine-wrapper.py
*
- * Function specifier, which does nothing but mark functions to be
+ * Function specifiers, which do nothing but mark functions to be
* generated by scripts/block-coroutine-wrapper.py
*
- * Read more in docs/devel/block-coroutine-wrapper.rst
+ * Usage: read docs/devel/block-coroutine-wrapper.rst
+ *
+ * co_wrapper_mixed functions can be called by both coroutine and
+ * non-coroutine context.
*/
-#define generated_co_wrapper
+#define co_wrapper_mixed
/* block.c */
typedef struct BlockDriver BlockDriver;
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 72919254cd..72cf45975b 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -39,19 +39,19 @@
* to catch when they are accidentally called by the wrong API.
*/
-int generated_co_wrapper bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
-int64_t bytes,
-BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
+int64_t bytes,
+BdrvRequestFlags flags);
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
-int generated_co_wrapper bdrv_pread(BdrvChild *child, int64_t offset,
-int64_t bytes, void *buf,
-BdrvRequestFlags flags);
-int generated_co_wrapper bdrv_pwrite(BdrvChild *child, int64_t offset,
- int64_t bytes, const void *buf,
- BdrvRequestFlags flags);
-int generated_co_wrapper bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
- int64_t bytes, const void *buf,
- BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pread(BdrvChild *child, int64_t offset,
+int64_t bytes, void *buf,
+BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pwrite(BdrvChild *child, int64_t offset,
+
[PATCH v6 13/14] block: convert bdrv_create to co_wrapper
This function is never called in coroutine context, therefore
instead of manually creating a new coroutine, delegate it to the
block-coroutine-wrapper script, defining it as co_wrapper.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
---
block.c| 40 ++
include/block/block-global-state.h | 8 --
2 files changed, 8 insertions(+), 40 deletions(-)
diff --git a/block.c b/block.c
index 772df02d54..a6465bac74 100644
--- a/block.c
+++ b/block.c
@@ -528,8 +528,8 @@ typedef struct CreateCo {
Error *err;
} CreateCo;
-static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
- QemuOpts *opts, Error **errp)
+int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+QemuOpts *opts, Error **errp)
{
int ret;
GLOBAL_STATE_CODE();
@@ -552,42 +552,6 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv,
const char *filename,
return ret;
}
-static void coroutine_fn bdrv_create_co_entry(void *opaque)
-{
-CreateCo *cco = opaque;
-GLOBAL_STATE_CODE();
-
-cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, >err);
-}
-
-int bdrv_create(BlockDriver *drv, const char* filename,
-QemuOpts *opts, Error **errp)
-{
-GLOBAL_STATE_CODE();
-
-if (qemu_in_coroutine()) {
-/* Fast-path if already in coroutine context */
-return bdrv_co_create(drv, filename, opts, errp);
-} else {
-Coroutine *co;
-CreateCo cco = {
-.drv = drv,
-.filename = filename,
-.opts = opts,
-.ret = NOT_DONE,
-.err = NULL,
-};
-
-co = qemu_coroutine_create(bdrv_create_co_entry, );
-qemu_coroutine_enter(co);
-while (cco.ret == NOT_DONE) {
-aio_poll(qemu_get_aio_context(), true);
-}
-error_propagate(errp, cco.err);
-return cco.ret;
-}
-}
-
/**
* Helper function for bdrv_create_file_fallback(): Resize @blk to at
* least the given @minimum_size.
diff --git a/include/block/block-global-state.h
b/include/block/block-global-state.h
index 387a7cbb2e..1f8b54f2df 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -55,8 +55,12 @@ BlockDriver *bdrv_find_protocol(const char *filename,
bool allow_protocol_prefix,
Error **errp);
BlockDriver *bdrv_find_format(const char *format_name);
-int bdrv_create(BlockDriver *drv, const char* filename,
-QemuOpts *opts, Error **errp);
+
+int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+QemuOpts *opts, Error **errp);
+int co_wrapper bdrv_create(BlockDriver *drv, const char *filename,
+ QemuOpts *opts, Error **errp);
+
int coroutine_fn bdrv_co_create_file(const char *filename, QemuOpts *opts,
Error **errp);
--
2.31.1
[PATCH v6 02/14] block-copy: add missing coroutine_fn annotations
These functions end up calling bdrv_common_block_status_above(), a
generated_co_wrapper function.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we need to mark such functions coroutine_fn too.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Paolo Bonzini
Reviewed-by: Kevin Wolf
---
block/block-copy.c | 21 -
include/block/block-copy.h | 5 +++--
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index bb947afdda..5e59d6262f 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -577,8 +577,9 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
return ret;
}
-static int block_copy_block_status(BlockCopyState *s, int64_t offset,
- int64_t bytes, int64_t *pnum)
+static coroutine_fn int block_copy_block_status(BlockCopyState *s,
+int64_t offset,
+int64_t bytes, int64_t *pnum)
{
int64_t num;
BlockDriverState *base;
@@ -590,8 +591,8 @@ static int block_copy_block_status(BlockCopyState *s,
int64_t offset,
base = NULL;
}
-ret = bdrv_block_status_above(s->source->bs, base, offset, bytes, ,
- NULL, NULL);
+ret = bdrv_co_block_status_above(s->source->bs, base, offset, bytes, ,
+ NULL, NULL);
if (ret < 0 || num < s->cluster_size) {
/*
* On error or if failed to obtain large enough chunk just fallback to
@@ -613,8 +614,9 @@ static int block_copy_block_status(BlockCopyState *s,
int64_t offset,
* Check if the cluster starting at offset is allocated or not.
* return via pnum the number of contiguous clusters sharing this allocation.
*/
-static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t offset,
- int64_t *pnum)
+static int coroutine_fn block_copy_is_cluster_allocated(BlockCopyState *s,
+int64_t offset,
+int64_t *pnum)
{
BlockDriverState *bs = s->source->bs;
int64_t count, total_count = 0;
@@ -624,7 +626,7 @@ static int block_copy_is_cluster_allocated(BlockCopyState
*s, int64_t offset,
assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
while (true) {
-ret = bdrv_is_allocated(bs, offset, bytes, );
+ret = bdrv_co_is_allocated(bs, offset, bytes, );
if (ret < 0) {
return ret;
}
@@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t offset,
int64_t bytes)
* @return 0 when the cluster at @offset was unallocated,
* 1 otherwise, and -ret on error.
*/
-int64_t block_copy_reset_unallocated(BlockCopyState *s,
- int64_t offset, int64_t *count)
+int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
+ int64_t offset,
+ int64_t *count)
{
int ret;
int64_t clusters, bytes;
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index ba0b425d78..8cea4f9b90 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -36,8 +36,9 @@ void block_copy_set_progress_meter(BlockCopyState *s,
ProgressMeter *pm);
void block_copy_state_free(BlockCopyState *s);
void block_copy_reset(BlockCopyState *s, int64_t offset, int64_t bytes);
-int64_t block_copy_reset_unallocated(BlockCopyState *s,
- int64_t offset, int64_t *count);
+int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
+ int64_t offset,
+ int64_t *count);
int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
bool ignore_ratelimit, uint64_t timeout_ns,
--
2.31.1
Re: [PATCH 00/15] Protect the block layer with a rwlock: part 3
Am 18/11/2022 um 11:57 schrieb Paolo Bonzini: > On 11/16/22 15:07, Emanuele Giuseppe Esposito wrote: >> Here we introduce generated_co_wrapper_simple, a simplification of >> g_c_w that >> only considers the case where the caller is not in a coroutine. >> This simplifies and clarifies a lot when the caller is a coroutine or >> not, and >> in the future will hopefully replace g_c_w. > > This is a good idea! > > Just one thing, though it belongs more in the two series which > introduced generated_co_wrapper_simple and generated_co_wrapper_blk - I > would make this the "official" wrapper. So perhaps: > > - generated_co_wrapper_simple -> coroutine_wrapper > - generated_co_wrapper_blk -> coroutine_wrapper_mixed > - generated_co_wrapper -> coroutine_wrapper_mixed_bdrv Ah damn I forgot about this, and of course I just sent v5 for "Still more coroutine and various fixes in block layer". To me it sounds good, but before I do a massive edit and then someone asks to revert it, @Kevin and the others do you agree? Thank you, Emanuele > > ? It is not clear to me yet if you will have bdrv_* functions that take > the rdlock as well - in which case however coroutine_wrapper_bdrv would > not be hard to add. > > Even without looking at the lock, the three series are going in the > right direction of ultimately having more "simple" coroutine wrappers at > the blk_* level and more coroutine functions (ultimately less wrappers, > too) at the bdrv_* level. > > Paolo >
[PATCH v5 12/15] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
Right now, we take the first parameter of the function to get the
BlockDriverState to pass to bdrv_poll_co(), that internally calls
functions that figure in which aiocontext the coroutine should run.
However, it is useless to pass a bs just to get its own AioContext,
so instead pass it directly, and default to the main loop if no
BlockDriverState is passed as parameter.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/block-gen.h | 6 +++---
scripts/block-coroutine-wrapper.py | 14 +++---
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/block/block-gen.h b/block/block-gen.h
index f80cf4897d..08d977f493 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -30,7 +30,7 @@
/* Base structure for argument packing structures */
typedef struct BdrvPollCo {
-BlockDriverState *bs;
+AioContext *ctx;
bool in_progress;
int ret;
Coroutine *co; /* Keep pointer here for debugging */
@@ -40,8 +40,8 @@ static inline int bdrv_poll_co(BdrvPollCo *s)
{
assert(!qemu_in_coroutine());
-bdrv_coroutine_enter(s->bs, s->co);
-BDRV_POLL_WHILE(s->bs, s->in_progress);
+aio_co_enter(s->ctx, s->co);
+AIO_WAIT_WHILE(s->ctx, s->in_progress);
return s->ret;
}
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index 7e8f2da84b..1d552cb734 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -78,14 +78,14 @@ def __init__(self, return_type: str, name: str, args: str,
t = self.args[0].type
if t == 'BlockDriverState *':
-bs = 'bs'
+ctx = 'bdrv_get_aio_context(bs)'
elif t == 'BdrvChild *':
-bs = 'child->bs'
+ctx = 'bdrv_get_aio_context(child->bs)'
elif t == 'BlockBackend *':
-bs = 'blk_bs(blk)'
+ctx = 'bdrv_get_aio_context(blk_bs(blk))'
else:
-bs = 'NULL'
-self.bs = bs
+ctx = 'qemu_get_aio_context()'
+self.ctx = ctx
def gen_list(self, format: str) -> str:
return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
@@ -130,7 +130,7 @@ def create_g_c_w(func: FuncDecl) -> str:
return {name}({ func.gen_list('{name}') });
}} else {{
{struct_name} s = {{
-.poll_state.bs = {func.bs},
+.poll_state.ctx = {func.ctx},
.poll_state.in_progress = true,
{ func.gen_block('.{name} = {name},') }
@@ -151,7 +151,7 @@ def create_coroutine_only(func: FuncDecl) -> str:
int {func.name}({ func.gen_list('{decl}') })
{{
{struct_name} s = {{
-.poll_state.bs = {func.bs},
+.poll_state.ctx = {func.ctx},
.poll_state.in_progress = true,
{ func.gen_block('.{name} = {name},') }
--
2.31.1
[PATCH v5 13/15] block-coroutine-wrapper.py: support also basic return types
Extend the regex to cover also return type, pointers included.
This implies that the value returned by the function cannot be
a simple "int" anymore, but the custom return type.
Therefore remove poll_state->ret and instead use a per-function
custom "ret" field.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/block-gen.h | 5 +
scripts/block-coroutine-wrapper.py | 19 +++
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/block/block-gen.h b/block/block-gen.h
index 08d977f493..89b7daaa1f 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -32,18 +32,15 @@
typedef struct BdrvPollCo {
AioContext *ctx;
bool in_progress;
-int ret;
Coroutine *co; /* Keep pointer here for debugging */
} BdrvPollCo;
-static inline int bdrv_poll_co(BdrvPollCo *s)
+static inline void bdrv_poll_co(BdrvPollCo *s)
{
assert(!qemu_in_coroutine());
aio_co_enter(s->ctx, s->co);
AIO_WAIT_WHILE(s->ctx, s->in_progress);
-
-return s->ret;
}
#endif /* BLOCK_BLOCK_GEN_H */
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index 1d552cb734..ef19b71c2a 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -95,7 +95,8 @@ def gen_block(self, format: str) -> str:
# Match wrappers declared with a generated_co_wrapper mark
-func_decl_re = re.compile(r'^int\s*generated_co_wrapper'
+func_decl_re = re.compile(r'^(?P[a-zA-Z][a-zA-Z0-9_]* [*]?)'
+ r'\s*generated_co_wrapper'
r'(?P(_[a-z][a-z0-9_]*)?)\s*'
r'(?P[a-z][a-z0-9_]*)'
r'\((?P[^)]*)\);$', re.MULTILINE)
@@ -103,7 +104,7 @@ def gen_block(self, format: str) -> str:
def func_decl_iter(text: str) -> Iterator:
for m in func_decl_re.finditer(text):
-yield FuncDecl(return_type='int',
+yield FuncDecl(return_type=m.group('return_type'),
name=m.group('wrapper_name'),
args=m.group('args'),
variant=m.group('variant'))
@@ -124,7 +125,7 @@ def create_g_c_w(func: FuncDecl) -> str:
name = func.co_name
struct_name = func.struct_name
return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
{{
if (qemu_in_coroutine()) {{
return {name}({ func.gen_list('{name}') });
@@ -138,7 +139,8 @@ def create_g_c_w(func: FuncDecl) -> str:
s.poll_state.co = qemu_coroutine_create({name}_entry, );
-return bdrv_poll_co(_state);
+bdrv_poll_co(_state);
+return s.ret;
}}
}}"""
@@ -148,7 +150,7 @@ def create_coroutine_only(func: FuncDecl) -> str:
name = func.co_name
struct_name = func.struct_name
return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
{{
{struct_name} s = {{
.poll_state.ctx = {func.ctx},
@@ -160,13 +162,13 @@ def create_coroutine_only(func: FuncDecl) -> str:
s.poll_state.co = qemu_coroutine_create({name}_entry, );
-return bdrv_poll_co(_state);
+bdrv_poll_co(_state);
+return s.ret;
}}"""
def gen_wrapper(func: FuncDecl) -> str:
assert not '_co_' in func.name
-assert func.return_type == 'int'
name = func.co_name
struct_name = func.struct_name
@@ -182,6 +184,7 @@ def gen_wrapper(func: FuncDecl) -> str:
typedef struct {struct_name} {{
BdrvPollCo poll_state;
+{func.return_type} ret;
{ func.gen_block('{decl};') }
}} {struct_name};
@@ -189,7 +192,7 @@ def gen_wrapper(func: FuncDecl) -> str:
{{
{struct_name} *s = opaque;
-s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
+s->ret = {name}({ func.gen_list('s->{name}') });
s->poll_state.in_progress = false;
aio_wait_kick();
--
2.31.1
