I am glad to have an accurate sync bdrv_flush(). Code looks fine.
Reviewed-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com>
> This patch makes bdrv_flush a synchronous function and updates any callers
> from
> a coroutine context to use bdrv_co_flush instead.
>
> The motivation for this patch comes from the GSoC Continuation-Passing C
> project. When coroutines were introduced, synchronous functions in the block
> layer were converted to use asynchronous methods by dynamically detecting if
> they were being run from a coroutine context by calling qemu_in_coroutine(),
> and
> yielding if so. If they were not, they would spawn a new coroutine and poll
> until the asynchronous counterpart finished.
>
> However this approach does not work with CPC as the CPC translator converts
> all
> functions annotated coroutine_fn to a different (continuation based) calling
> convention. This means that coroutine_fn annotated functions cannot be called
> from a non-coroutine context.
>
> This patch is a Request For Comments on the approach of splitting these
> "dynamic" functions into synchronous and asynchronous versions. This is easy
> for
> bdrv_flush as it already has an asynchronous counterpart - bdrv_co_flush. The
> only caller of bdrv_flush from a coroutine context is mirror_drain in
> block/mirror.c - this should be annotated as a coroutine_fn as it calls
> qemu_coroutine_yield().
>
> If this approach meets with approval I will develop a patchset splitting the
> other "dynamic" functions in the block layer. This will allow all coroutine
> functions to have a coroutine_fn annotation that can be statically checked
> (CPC
> can be used to verify annotations).
>
> I have audited the other callers of bdrv_flush, they are included below:
>
> block.c: bdrv_reopen_prepare, bdrv_close, bdrv_commit, bdrv_pwrite_sync
> block/qcow2-cache.c: qcow2_cache_entry_flush, qcow2_cache_flush
> block/qcow2-refcount.c: qcow2_update_snapshot_refcount
> block/qcow2-snapshot.c: qcow2_write_snapshots
> block/qcow2.c: qcow2_mark_dirty, qcow2_mark_clean
> block/qed-check.c: qed_check_mark_clean
> block/qed.c: bdrv_qed_open, bdrv_qed_close
> blockdev.c: external_snapshot_prepare, do_drive_del
> cpus.c: do_vm_stop
> hw/block/nvme.c: nvme_clear_ctrl
> qemu-io-cmds.c: flush_f
> savevm.c: bdrv_fclose
>
> ---
> block.c | 13 ++++---------
> block/mirror.c | 4 ++--
> 2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/block.c b/block.c
> index 6c493ad..00d71df 100644
> --- a/block.c
> +++ b/block.c
> @@ -4110,15 +4110,10 @@ int bdrv_flush(BlockDriverState *bs)
> .ret = NOT_DONE,
> };
>
> - if (qemu_in_coroutine()) {
> - /* Fast-path if already in coroutine context */
> - bdrv_flush_co_entry(&rwco);
> - } else {
> - co = qemu_coroutine_create(bdrv_flush_co_entry);
> - qemu_coroutine_enter(co, &rwco);
> - while (rwco.ret == NOT_DONE) {
> - qemu_aio_wait();
> - }
> + co = qemu_coroutine_create(bdrv_flush_co_entry);
> + qemu_coroutine_enter(co, &rwco);
> + while (rwco.ret == NOT_DONE) {
> + qemu_aio_wait();
> }
>
> return rwco.ret;
> diff --git a/block/mirror.c b/block/mirror.c
> index bed4a7e..3d5da7e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -282,7 +282,7 @@ static void mirror_free_init(MirrorBlockJob *s)
> }
> }
>
> -static void mirror_drain(MirrorBlockJob *s)
> +static void coroutine_fn mirror_drain(MirrorBlockJob *s)
> {
> while (s->in_flight > 0) {
> qemu_coroutine_yield();
> @@ -390,7 +390,7 @@ static void coroutine_fn mirror_run(void *opaque)
> should_complete = false;
> if (s->in_flight == 0 && cnt == 0) {
> trace_mirror_before_flush(s);
> - ret = bdrv_flush(s->target);
> + ret = bdrv_co_flush(s->target);
> if (ret < 0) {
> if (mirror_error_action(s, false, -ret) ==
> BDRV_ACTION_REPORT) {
> goto immediate_exit;
>
--
Best Regards
Wenchao Xia
