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 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 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 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 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 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 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 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 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 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 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 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 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 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 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: 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
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 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 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 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
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 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 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 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 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
>
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)
>
[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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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
[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 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 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 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 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 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 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 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 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 v5 06/15] 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
---
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 v5 07/15] block: introduce QEMU_IN_COROUTINE macro
This macro will be used to mark all coroutine_fn functions.
Right now, it will be used for the newly introduced coroutine_fn, since
we know the callers.
As a TODO, in the future we might want to add this macro to all
corotuine_fn functions, to be sure that they are only called in
coroutines context.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/block-common.h | 11 +++
1 file changed, 11 insertions(+)
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 297704c1e9..24de1d63fd 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -34,6 +34,17 @@
#include "qemu/hbitmap.h"
#include "qemu/transactions.h"
+/*
+ * QEMU_IN_COROUTINE
+ *
+ * To be used in all coroutine_fn functions, to make sure that the caller
+ * is always a coroutine.
+ */
+#define QEMU_IN_COROUTINE() \
+do {\
+assert(qemu_in_coroutine());\
+} while (0)
+
/*
* generated_co_wrapper
*
--
2.31.1
[PATCH v5 03/15] 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 v5 05/15] 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 v5 14/15] block: convert bdrv_create to generated_co_wrapper_simple
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 g_c_w_simple.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
---
block.c| 40 ++
include/block/block-global-state.h | 10 ++--
2 files changed, 10 insertions(+), 40 deletions(-)
diff --git a/block.c b/block.c
index 8207f93353..5b4ba02d3a 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();
@@ -553,42 +553,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..8f1c8ca910 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -55,8 +55,14 @@ 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 generated_co_wrapper_simple 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 v5 11/15] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the functions that it uses are both using bdrv_get_aio_context, that defaults to qemu_get_aio_context() if bs is NULL. Therefore pass NULL to BdrvPollCo to automatically generate a function that create and runs a coroutine in the main loop. Signed-off-by: Emanuele Giuseppe Esposito --- scripts/block-coroutine-wrapper.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py index 2fa3d01898..7e8f2da84b 100644 --- a/scripts/block-coroutine-wrapper.py +++ b/scripts/block-coroutine-wrapper.py @@ -81,8 +81,10 @@ def __init__(self, return_type: str, name: str, args: str, bs = 'bs' elif t == 'BdrvChild *': bs = 'child->bs' -else: +elif t == 'BlockBackend *': bs = 'blk_bs(blk)' +else: +bs = 'NULL' self.bs = bs def gen_list(self, format: str) -> str: @@ -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
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 15/15] block/dirty-bitmap: convert coroutine-only functions to generated_co_wrapper_simple
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
g_c_w_simple.
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 | 9 +++-
include/block/dirty-bitmap.h | 10 +++-
5 files changed, 21 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 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
[PATCH v5 09/15] 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
---
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 8a7f87f783..8207f93353 100644
--- a/block.c
+++ b/block.c
@@ -724,7 +724,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;
@@ -765,7 +766,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;
}
diff --git a/block/
[PATCH v5 02/15] 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
---
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
[PATCH v5 01/15] 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
---
block/io.c | 64 ++--
include/block/block-io.h | 15 ++
2 files changed, 76 insertions(+), 3 deletions(-)
diff --git a/block/io.c b/block/io.c
index 38e57d1f67..1bc05c8282 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2533,6 +2533,19 @@ 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();
+/* If QEMU_IN_COROUTINE() fails, use bdrv_block_status_above() */
+QEMU_IN_COROUTINE();
+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 +2591,24 @@ 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();
+/* If QEMU_IN_COROUTINE() fails, use bdrv_is_allocated() */
+QEMU_IN_COROUTINE();
+
+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 +2625,31 @@ 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();
+/* If QEMU_IN_COROUTINE() fails, use bdrv_is_allocated_above() */
+QEMU_IN_COROUTINE();
+
+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 +2673,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,
+
[PATCH v5 04/15] 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 bdrv_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.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Paolo Bonzini
---
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..03bed68e4f 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_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..9d4d908344 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_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..108be63628 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_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..2663ea282f 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,
+ offset + progress,
+
[PATCH v5 10/15] block-coroutine-wrapper.py: introduce generated_co_wrapper_simple
This new annotation creates just a function wrapper that creates
a new coroutine. It assumes the caller is not a coroutine.
This is much better as g_c_w, 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 g_c_w functions will be
substituted on g_c_w_simple.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/block-common.h | 1 +
scripts/block-coroutine-wrapper.py | 105 -
2 files changed, 75 insertions(+), 31 deletions(-)
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 24de1d63fd..46b57501d8 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -54,6 +54,7 @@
* Read more in docs/devel/block-coroutine-wrapper.rst
*/
#define generated_co_wrapper
+#define generated_co_wrapper_simple
/* block.c */
typedef struct BlockDriver BlockDriver;
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index 08be813407..2fa3d01898 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -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 = False
+
+if variant == '_simple':
+self.create_only_co = True
+
+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)
@@ -75,7 +93,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\s*'
+func_decl_re = re.compile(r'^int\s*generated_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 snake_to_camel(func_name: str) -> str:
@@ -97,24 +117,63 @@ def snake_to_camel(func_name: str) -> str:
return ''.join(words)
+# Checks if we are already in coroutine
+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}') })
+{{
+if (qemu_in_coroutine()) {{
+return {name}({ func.gen_list('{name}') });
+}} else {{
+{struct_name} s = {{
+.poll_state.bs = {func.bs},
+.poll_state.in_progress = true,
+
+{ func.gen_block('.{name} = {name},') }
+}};
+
+s.poll_state.co = qemu_coroutine_create({name}_entry, );
+
+return bdrv_poll_co(_state);
+}}
+}}"""
+
+
+# Assumes we are not in coroutine, and creates one
+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}') })
+{{
+{struct_name} s = {{
+.poll_state.bs = {func.bs},
+.poll_state.in_progress = true,
+
+{ func.gen_block('.{name} = {name},') }
+}};
+assert(!qemu_in_coroutine());
+
+s.poll_state.co = qemu_coroutine_create({name}_entry, );
+
+return bdrv_poll_co(_state);
+}}"""
+
+
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 *']
-subsystem, subname = func.name.split('_', 1)
-
-name = f'{subsystem}_co_{subname}'
+name = func.co_name
+struct_name = func.struct_name
-t = func.args[0].type
-if t == 'BlockDriverState *':
-bs = 'bs'
-elif t == 'BdrvChild *':
-bs = 'child->bs'
-else:
-bs = 'blk_bs(blk)'
-struct_name = snake_to_camel(name)
+creation_func
[PATCH v5 08/15] 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
---
block.c | 72 +
1 file changed, 37 insertions(+), 35 deletions(-)
diff --git a/block.c b/block.c
index 9d51e7b6e5..8a7f87f783 100644
--- a/block.c
+++ b/block.c
@@ -528,63 +528,65 @@ 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();
+QEMU_IN_COROUTINE();
+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 v5 00/15] 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 --- 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 generated_co_wrapper_simple 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 (15): 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: introduce QEMU_IN_COROUTINE macro block: distinguish between bdrv_create running in coroutine and not block: bdrv_create_file is a coroutine_fn block-coroutine-wrapper.py: introduce generated_co_wrapper_simple block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter 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 generated_co_wrapper_simple block/dirty-bitmap: convert coroutine-only functions to generated_co_wrapper_simple block.c| 66 block/block-backend.c | 21 + block/block-copy.c | 21 ++--- block/block-gen.h | 11 +-- block/commit.c | 4 +- block/crypto.c | 2 +- block/dirty-bitmap.c | 88 + block/io.c | 64 +++- 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 +- include/block/block-common.h | 17 - include/block/block-copy.h | 5 +- include/block/block-global-state.h | 13 +++- include/block/block-io.h | 24 +- include/block/dirty-bitmap.h | 10 ++- include/sysemu/block-backend-io.h | 9 +++ nbd/server.c | 47 ++-- scripts/block-coroutine-wrapper.py | 118 - 26 files changed, 322 insertions(+), 255 deletions(-) -- 2.31.1
Re: [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn
Am 22/11/2022 um 09:58 schrieb Kevin Wolf:
> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>> It is always called in coroutine_fn callbacks, therefore
>> it can directly call bdrv_co_create().
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> block.c| 6 --
>> include/block/block-global-state.h | 3 ++-
>> 2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index c610a32e77..7a4c3eb540 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -534,6 +534,7 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv,
>> const char *filename,
>> int ret;
>> char *filename_copy;
>> GLOBAL_STATE_CODE();
>> +assert(qemu_in_coroutine());
>
> We don't generally assert this, otherwise it would have to be in every
> coroutine_fn.
That was my plan for the serie "Protect the block layer with a rwlock:
part 3", where I convert BlockDriver callbacks in coroutine, and thus
assert there because I know all the callers are coroutine_fn.
>
>> assert(*errp == NULL);
>> assert(drv);
>>
>> @@ -725,7 +726,8 @@ out:
>> return ret;
>> }
>>
>> -int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>> +int coroutine_fn bdrv_create_file(const char *filename, QemuOpts *opts,
>> + Error **errp)
>
> Should it be renamed as bdrv_co_create_file()?
>
Ok (when I don't answer just assume that I agree).
Thank you,
Emanuele
>> {
>> QemuOpts *protocol_opts;
>> BlockDriver *drv;
>> @@ -766,7 +768,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);
>
> Kevin
>
Re: [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations
Am 21/11/2022 um 17:01 schrieb Kevin Wolf: > Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben: >> 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 > > Just one remark about patch ordering: This doesn't require the > g_c_w_simple patches, so wouldn't it make more sense to move the > g_c_w_simple right before the first patch that actually makes use of > them? I thought about it, but then I thought it was too far from bdrv_create patches. Both g_c_w_simple and this patch are needed by bdrv_create_file, so I thought if you read this one first then you wouldn't have a clue on what is the end goal. Anyways, I'll change the order. Thank you, Emanuele > > Reviewed-by: Kevin Wolf >
Re: [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
Am 21/11/2022 um 16:30 schrieb Kevin Wolf: > Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben: >> Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the >> functions that it uses are both using bdrv_get_aio_context, that >> defaults to qemu_get_aio_context() if bs is NULL. >> >> Therefore pass NULL to BdrvPollCo to automatically generate a function >> that create and runs a coroutine in the main loop. >> >> Signed-off-by: Emanuele Giuseppe Esposito > > It happens to work, but it's kind of ugly to call bdrv_coroutine_enter() > and BDRV_POLL_WHILE() with a NULL bs. > > How hard would it be to generate code that doesn't use these functions, > but directly aio_co_enter() and AIO_WAIT_WHILE() for wrappers that are > not related to a BDS? > At this point, I would get rid of s->poll_state.bs and instead use s->poll_state.aio_context. Then call directly aio_co_enter and AIO_WAIT_WHILE, as you suggested but just everywhere, without differentiating the cases. Then we would have something similar to what it is currently done with bs: if t == 'BlockDriverState *': bs = 'bdrv_get_aio_context(bs)' elif t == 'BdrvChild *': bs = 'bdrv_get_aio_context(child->bs)' elif t == 'BlockBackend *': bs = 'bdrv_get_aio_context(blk_bs(blk))' else: bs = 'qemu_get_aio_context()' I haven't tried it yet, but it should work. Thank you, Emanuele
Re: [PATCH 0/6] Protect the block layer with a rwlock: part 2
Ok, as I expected simple changes in a previous based-on serie provoke a
cascade of changes that inevitably affect these patches too.
While I strongly suggest to have an initial look at these patches
because it gives an idea on what am I trying to accomplish, I would not
go looking at nitpicks and trivial errors that came up from the based-on
series (ie "just as in the previous serie, fix this").
The order of the series is:
1. Still more coroutine and various fixes in block layer
2. Protect the block layer with a rwlock: part 1
3. Protect the block layer with a rwlock: part 2
4. Protect the block layer with a rwlock: part 3
Thank you,
Emanuele
Am 16/11/2022 um 14:53 schrieb Emanuele Giuseppe Esposito:
> Please read "Protect the block layer with a rwlock: part 1" for an additional
> introduction and aim of this series.
>
> This second part aims to add the graph rdlock to the BlockDriver functions
> that already run in coroutine context and are classified as IO.
> Such functions will recursively traverse the BlockDriverState graph, therefore
> they need to be protected with the rdlock.
>
> Based-on: <20221116134850.3051419-1-eespo...@redhat.com>
>
> Thank you,
> Emanuele
>
> Emanuele Giuseppe Esposito (6):
> block: assert that bdrv_co_create is always called with graph rdlock
> taken
> block: assert that BlockDriver->bdrv_co_{amend/create} are called with
> graph rdlock taken
> block: assert that BlockDriver->bdrv_co_copy_range_{from/to} is always
> called with graph rdlock taken
> block/dirty-bitmap: assert that BlockDriver->bdrv_co_*_dirty_bitmap
> are always called with graph rdlock taken
> block/io: assert that BlockDriver->bdrv_co_*_snapshot_* are always
> called with graph rdlock taken
> block: assert that BlockDriver->bdrv_co_delete_file is always called
> with graph rdlock taken
>
> block.c | 2 ++
> block/amend.c| 1 +
> block/block-backend.c| 2 ++
> block/create.c | 1 +
> block/dirty-bitmap.c | 2 ++
> block/io.c | 7 +++
> include/block/block_int-common.h | 14 +-
> qemu-img.c | 4 +++-
> 8 files changed, 31 insertions(+), 2 deletions(-)
>
Re: [PATCH 00/15] Protect the block layer with a rwlock: part 3
Ok, as I expected simple changes in a previous based-on serie provoke a cascade of changes that inevitably affect these patches too. While I strongly suggest to have an initial look at these patches because it gives an idea on what am I trying to accomplish, I would not go looking at nitpicks and trivial errors that came up from the based-on series (ie "just as in the previous serie, fix this"). The order of the series is: 1. Still more coroutine and various fixes in block layer 2. Protect the block layer with a rwlock: part 1 3. Protect the block layer with a rwlock: part 2 4. Protect the block layer with a rwlock: part 3 Thank you, Emanuele Am 16/11/2022 um 15:07 schrieb Emanuele Giuseppe Esposito: > Please read "Protect the block layer with a rwlock: part 1" and > "Protect the block layer with a rwlock: part 2" for an > additional introduction and aim of this series. > > In this serie, we cover the remaining BlockDriver IO callbacks that were not > running in coroutine, therefore not using the graph rdlock. > Therefore convert them to coroutines, using either g_c_w or a new > variant introduced in this serie (see below). > > We need to convert these callbacks into coroutine because non-coroutine code > is tied to the main thread, even though it will still delegate I/O accesses to > the iothread (via the bdrv_coroutine_enter call in generated_co_wrappers). > Making callbacks run in coroutines provides more flexibility, because they run > entirely in iothreads and can use CoMutexes for mutual exclusion. > > 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. > > While we are at it, try to directly call the _co_ counterpart of a g_c_w when > we know already that the function always run in a coroutine. > > Based-on: <20221116135331.3052923-1-eespo...@redhat.com> > > Thank you, > Emanuele > > Emanuele Giuseppe Esposito (15): > block/qed: add missing graph rdlock in qed_need_check_timer_entry > block: rename refresh_total_sectors in bdrv_refresh_total_sectors > block-backend: use bdrv_getlength instead of blk_getlength > block: convert bdrv_refresh_total_sectors in generated_co_wrapper > block: use bdrv_co_refresh_total_sectors when possible > block: convert bdrv_get_allocated_file_size in > generated_co_wrapper_simple > block: convert bdrv_get_info in generated_co_wrapper > block: convert bdrv_is_inserted in generated_co_wrapper_simple > block-coroutine-wrapper: support void functions > block: convert bdrv_eject in generated_co_wrapper_simple > block: convert bdrv_lock_medium in generated_co_wrapper_simple > block: convert bdrv_debug_event in generated_co_wrapper > block: convert bdrv_io_plug in generated_co_wrapper_simple > block: convert bdrv_io_unplug in generated_co_wrapper_simple > block: rename newly converted BlockDriver IO coroutine functions > > block.c| 93 +++--- > block/blkdebug.c | 4 +- > block/blkio.c | 6 +- > block/blklogwrites.c | 2 +- > block/blkreplay.c | 2 +- > block/blkverify.c | 2 +- > block/block-backend.c | 43 -- > 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 | 8 +-- > block/filter-compress.c| 10 ++-- > block/gluster.c| 16 ++--- > block/io.c | 78 + > block/iscsi.c | 8 +-- > block/meson.build | 1 + > block/mirror.c | 17 -- > 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| 7 ++- > block/quorum.c | 2 +- > block/raw-format.c | 14 ++--- > block/rbd.c| 4 +- > block/replication.c| 2 +- > block/ssh.c| 2 +- > block/stream.c | 4
Re: [PATCH 00/20] Protect the block layer with a rwlock: part 1
Ok, as I expected simple changes in a previous based-on serie provoke a
cascade of changes that inevitably affect these patches too.
While I strongly suggest to have an initial look at these patches
because it gives an idea on what am I trying to accomplish, I would not
go looking at nitpicks and trivial errors that came up from the based-on
series (ie "just as in the previous serie, fix this").
The order of the series is:
1. Still more coroutine and various fixes in block layer
2. Protect the block layer with a rwlock: part 1
3. Protect the block layer with a rwlock: part 2
4. Protect the block layer with a rwlock: part 3
Thank you,
Emanuele
Am 16/11/2022 um 14:48 schrieb Emanuele Giuseppe Esposito:
> This serie is the first of four series that aim to introduce and use a new
> graph rwlock in the QEMU block layer.
> 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 1.
>
> 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 first serie, my aim is to introduce the lock (patches 1-3,6), cover
> the
> main graph writer (patch 4), define assertions (patch 5) and start using the
> read lock in the generated_co_wrapper functions (7-20).
> Such functions recursively traverse the BlockDriverState graph, so they must
> take the graph rdlock.
>
> We distinguish two cases related to the generated_co_wrapper (often shortened
> to g_c_w):
> - qemu_in_coroutine(), which means the function is already running in a
> coroutine. This means we don't take the lock, because the coroutine must
> have taken it when it started
> - !qemu_in_coroutine(), which means we need to create a new coroutine that
> performs the operation requested. In this case we take the rdlock as soon as
> the coroutine starts, and release only before finishing.
>
> 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 usually call blk_wait_while_drained(), therefore they
> must
> take the lock internally. Therefore we introduce generated_co_wrapper_blk,
> which does not take the rdlock when starting the coroutine.
>
> 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.
>
> This serie is based on v4 of "Still more coroutine and various fixes in block
> layer".
>
> Based-on: <20221116122241.2856527-1-eespo...@redhat.com>
>
> Thank you,
> Emanuele
>
> Emanuele Giuseppe Esposito (19):
> graph-lock: introduce BdrvGraphRWlock structure
> async: register/unregister aiocontext in graph lock list
> block.c: wrlock in bdrv_replace_child_noperm
> block: remove unnecessary assert_bdrv_graph_writable()
> block: assert that graph read and writes are performed correctly
> graph-lock: implement WITH_GRAPH_RDLOCK_GUARD and GRAPH_RDLOCK_GUARD
> macros
> block-coroutine-wrapper.py: take the graph rdlock in bdrv_* functions
> block-backend: introduce new generated_co_wrapper_blk annotation
> block-gen: assert that {bdrv/blk}_co_truncate is always called with
> graph rdlock taken
> block-gen: assert that bdrv_co_{check/invalidate_cache} are always
> called with graph rdlock taken
> block-gen: assert that bdrv_co_pwrite is always called with graph
> rdlock taken
> bl
