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. Emanuele --- v2: * clarified commit message in patches 2/3/6 on why we add coroutine_fn Emanuele Giuseppe Esposito (9): block: call bdrv_co_drain_begin in a coroutine block-copy: add missing coroutine_fn annotations nbd/server.c: add missing coroutine_fn annotations block-backend: replace bdrv_*_above with blk_*_above block: distinguish between bdrv_create running in coroutine and not block/vmdk: add missing coroutine_fn annotations block: bdrv_create_file is a coroutine_fn block: bdrv_create is never called in non-coroutine context block/dirty-bitmap: remove unnecessary qemu_in_coroutine() case block.c | 111 +++++++++++++++++------------ block/block-backend.c | 21 ++++++ block/block-copy.c | 15 ++-- block/commit.c | 4 +- block/dirty-bitmap.c | 66 ++++++++--------- block/vmdk.c | 36 +++++----- include/block/block-global-state.h | 3 +- include/sysemu/block-backend-io.h | 9 +++ nbd/server.c | 43 +++++------ qemu-img.c | 4 +- 10 files changed, 182 insertions(+), 130 deletions(-) -- 2.31.1 It seems that bdrv_open_driver() forgot to create a coroutine where to call bs->drv->bdrv_co_drain_begin(), a callback marked as coroutine_fn. Because there is no active I/O at this point, the coroutine should end right after entering, so the caller does not need to poll until it is finished. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- block.c | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ out: g_free(gen_node_name); } +typedef struct DrainCo { + BlockDriverState *bs; + int ret; +} DrainCo; + +static void coroutine_fn bdrv_co_drain_begin(void *opaque) +{ + int i; + DrainCo *co = opaque; + BlockDriverState *bs = co->bs; + + for (i = 0; i < bs->quiesce_counter; i++) { + bs->drv->bdrv_co_drain_begin(bs); + } + co->ret = 0; +} + static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, const char *node_name, QDict *options, int open_flags, Error **errp) { Error *local_err = NULL; - int i, ret; + int ret; + Coroutine *co; + DrainCo dco = { + .bs = bs, + .ret = NOT_DONE, + }; GLOBAL_STATE_CODE(); bdrv_assign_node_name(bs, node_name, &local_err); @@ -XXX,XX +XXX,XX @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, assert(bdrv_min_mem_align(bs) != 0); assert(is_power_of_2(bs->bl.request_alignment)); - for (i = 0; i < bs->quiesce_counter; i++) { - if (drv->bdrv_co_drain_begin) { - drv->bdrv_co_drain_begin(bs); - } + if (drv->bdrv_co_drain_begin) { + co = qemu_coroutine_create(bdrv_co_drain_begin, &dco); + qemu_coroutine_enter(co); + /* + * There should be no reason for drv->bdrv_co_drain_begin to wait at + * this point, because the device does not have any active I/O. + */ + assert(dco.ret != NOT_DONE); } return 0; -- 2.31.1 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 <eesposit@redhat.com> --- block/block-copy.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/block/block-copy.c b/block/block-copy.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 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 <eesposit@redhat.com> --- nbd/server.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index XXXXXXX..XXXXXXX 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 Avoid mixing bdrv_* functions with blk_*, so create blk_* counterparts for: - bdrv_block_status_above - bdrv_is_allocated_above Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/block-backend.c | 21 +++++++++++++++++++++ block/commit.c | 4 ++-- include/sysemu/block-backend-io.h | 9 +++++++++ nbd/server.c | 28 ++++++++++++++-------------- qemu-img.c | 4 ++-- 5 files changed, 48 insertions(+), 18 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -XXX,XX +XXX,XX @@ 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_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_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_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 XXXXXXX..XXXXXXX 100644 --- a/block/commit.c +++ b/block/commit.c @@ -XXX,XX +XXX,XX @@ 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, &n); + ret = blk_is_allocated_above(s->top, s->base_overlay, true, + offset, COMMIT_BUFFER_SIZE, &n); 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 XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/block-backend-io.h +++ b/include/sysemu/block-backend-io.h @@ -XXX,XX +XXX,XX @@ 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_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 XXXXXXX..XXXXXXX 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -XXX,XX +XXX,XX @@ 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_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, @@ -XXX,XX +XXX,XX @@ 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, &pnum, NULL, - NULL); + int status = blk_block_status_above(exp->common.blk, NULL, + offset + progress, + size - progress, &pnum, NULL, + NULL); bool final; if (status < 0) { @@ -XXX,XX +XXX,XX @@ static int nbd_extent_array_add(NBDExtentArray *ea, return 0; } -static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs, +static int coroutine_fn blockstatus_to_extents(BlockBackend *blk, 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, &num, + int ret = blk_block_status_above(blk, NULL, offset, bytes, &num, NULL, NULL); if (ret < 0) { @@ -XXX,XX +XXX,XX @@ static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs, return 0; } -static int coroutine_fn blockalloc_to_extents(BlockDriverState *bs, +static int coroutine_fn blockalloc_to_extents(BlockBackend *blk, 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 = blk_is_allocated_above(blk, NULL, false, offset, bytes, &num); if (ret < 0) { @@ -XXX,XX +XXX,XX @@ 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 coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle, - BlockDriverState *bs, uint64_t offset, + BlockBackend *blk, uint64_t offset, uint32_t length, bool dont_fragment, bool last, uint32_t context_id, Error **errp) @@ -XXX,XX +XXX,XX @@ coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle, g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents); if (context_id == NBD_META_ID_BASE_ALLOCATION) { - ret = blockstatus_to_extents(bs, offset, length, ea); + ret = blockstatus_to_extents(blk, offset, length, ea); } else { - ret = blockalloc_to_extents(bs, offset, length, ea); + ret = blockalloc_to_extents(blk, offset, length, ea); } if (ret < 0) { return nbd_co_send_structured_error( @@ -XXX,XX +XXX,XX @@ static coroutine_fn int nbd_handle_request(NBDClient *client, if (client->export_meta.base_allocation) { ret = nbd_co_send_block_status(client, request->handle, - blk_bs(exp->common.blk), + exp->common.blk, request->from, request->len, dont_fragment, !--contexts_remaining, @@ -XXX,XX +XXX,XX @@ static coroutine_fn int nbd_handle_request(NBDClient *client, if (client->export_meta.allocation_depth) { ret = nbd_co_send_block_status(client, request->handle, - blk_bs(exp->common.blk), + exp->common.blk, request->from, request->len, dont_fragment, !--contexts_remaining, diff --git a/qemu-img.c b/qemu-img.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -XXX,XX +XXX,XX @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) do { count = n * BDRV_SECTOR_SIZE; - ret = bdrv_block_status_above(src_bs, base, offset, count, &count, - NULL, NULL); + ret = bdrv_block_status_above(src_bs, base, offset, count, + &count, NULL, NULL); if (ret < 0) { if (s->salvage) { -- 2.31.1 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 <eesposit@redhat.com> --- block.c | 74 ++++++++++++++++++++++++++++----------------------------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ 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(); + assert(*errp == NULL); + + 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(); + assert(cco->drv); - ret = cco->drv->bdrv_co_create_opts(cco->drv, - cco->filename, cco->opts, &local_err); - error_propagate(&cco->err, local_err); - cco->ret = ret; + cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, &cco->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 = g_strdup(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); - ret = -ENOTSUP; - goto out; - } - if (qemu_in_coroutine()) { /* Fast-path if already in coroutine context */ - bdrv_create_co_entry(&cco); + return bdrv_co_create(drv, filename, opts, errp); } else { + Coroutine *co; + CreateCo cco = { + .drv = drv, + .filename = g_strdup(filename), + .opts = opts, + .ret = NOT_DONE, + .err = NULL, + }; + co = qemu_coroutine_create(bdrv_create_co_entry, &cco); qemu_coroutine_enter(co); while (cco.ret == NOT_DONE) { aio_poll(qemu_get_aio_context(), true); } + error_propagate(errp, cco.err); + g_free(cco.filename); + 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"); - } - } - -out: - g_free(cco.filename); - return ret; } /** -- 2.31.1 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 <eesposit@redhat.com> --- block/vmdk.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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, @@ -XXX,XX +XXX,XX @@ 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) @@ -XXX,XX +XXX,XX @@ 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 It is always called in coroutine_fn callbacks, therefore it can directly call bdrv_co_create(). Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename, { int ret; GLOBAL_STATE_CODE(); + assert(qemu_in_coroutine()); assert(*errp == NULL); if (!drv->bdrv_co_create_opts) { @@ -XXX,XX +XXX,XX @@ 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) { QemuOpts *protocol_opts; BlockDriver *drv; @@ -XXX,XX +XXX,XX @@ 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/include/block/block-global-state.h b/include/block/block-global-state.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -XXX,XX +XXX,XX @@ 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_create_file(const char *filename, QemuOpts *opts, + Error **errp); BlockDriverState *bdrv_new(void); int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, -- 2.31.1 Delete the if case and make sure it won't be called again in coroutines. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- block.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static void coroutine_fn bdrv_create_co_entry(void *opaque) int bdrv_create(BlockDriver *drv, const char* filename, QemuOpts *opts, Error **errp) { + Coroutine *co; + CreateCo cco = { + .drv = drv, + .filename = g_strdup(filename), + .opts = opts, + .ret = NOT_DONE, + .err = NULL, + }; GLOBAL_STATE_CODE(); + assert(!qemu_in_coroutine()); - 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 = g_strdup(filename), - .opts = opts, - .ret = NOT_DONE, - .err = NULL, - }; - - co = qemu_coroutine_create(bdrv_create_co_entry, &cco); - qemu_coroutine_enter(co); - while (cco.ret == NOT_DONE) { - aio_poll(qemu_get_aio_context(), true); - } - error_propagate(errp, cco.err); - g_free(cco.filename); - return cco.ret; + co = qemu_coroutine_create(bdrv_create_co_entry, &cco); + qemu_coroutine_enter(co); + while (cco.ret == NOT_DONE) { + aio_poll(qemu_get_aio_context(), true); } + error_propagate(errp, cco.err); + g_free(cco.filename); + return cco.ret; } /** -- 2.31.1 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. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/dirty-bitmap.c | 66 +++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 37 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index XXXXXXX..XXXXXXX 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -XXX,XX +XXX,XX @@ bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque) 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, - &s); - bdrv_coroutine_enter(bs, co); - BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS); - - return s.ret; - } + Coroutine *co; + BdrvRemovePersistentDirtyBitmapCo s = { + .bs = bs, + .name = name, + .errp = errp, + .ret = -EINPROGRESS, + }; + assert(!qemu_in_coroutine()); + co = qemu_coroutine_create(bdrv_co_remove_persistent_dirty_bitmap_entry, + &s); + bdrv_coroutine_enter(bs, co); + BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS); + + return s.ret; } bool @@ -XXX,XX +XXX,XX @@ 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, - &s); - bdrv_coroutine_enter(bs, co); - BDRV_POLL_WHILE(bs, s.in_progress); - - return s.ret; - } + Coroutine *co; + BdrvCanStoreNewDirtyBitmapCo s = { + .bs = bs, + .name = name, + .granularity = granularity, + .errp = errp, + .in_progress = true, + }; + assert(!qemu_in_coroutine()); + co = qemu_coroutine_create(bdrv_co_can_store_new_dirty_bitmap_entry, + &s); + bdrv_coroutine_enter(bs, co); + BDRV_POLL_WHILE(bs, s.in_progress); + + return s.ret; } void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) -- 2.31.1 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-kwolf@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 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 <eesposit@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block/io.c +++ b/block/io.c @@ -XXX,XX +XXX,XX @@ 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) @@ -XXX,XX +XXX,XX @@ 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 : &dummy, 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) { @@ -XXX,XX +XXX,XX @@ 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, + &depth); + if (ret < 0) { + return ret; + } + + if (ret & BDRV_BLOCK_ALLOCATED) { + return depth; + } + return 0; +} + /* * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP] * @@ -XXX,XX +XXX,XX @@ 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, - &depth); + int ret; IO_CODE(); + + ret = bdrv_common_block_status_above(top, base, include_base, false, + offset, bytes, pnum, NULL, NULL, + &depth); if (ret < 0) { return ret; } diff --git a/include/block/block-io.h b/include/block/block-io.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -XXX,XX +XXX,XX @@ 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); -- 2.31.1 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 <eesposit@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ static int block_copy_block_status(BlockCopyState *s, int64_t offset, base = NULL; } - ret = bdrv_block_status_above(s->source->bs, base, offset, bytes, &num, - NULL, NULL); + ret = bdrv_co_block_status_above(s->source->bs, base, offset, bytes, &num, + NULL, NULL); if (ret < 0 || num < s->cluster_size) { /* * On error or if failed to obtain large enough chunk just fallback to @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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, &count); + ret = bdrv_co_is_allocated(bs, offset, bytes, &count); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/include/block/block-copy.h +++ b/include/block/block-copy.h @@ -XXX,XX +XXX,XX @@ 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 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 <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- nbd/server.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index XXXXXXX..XXXXXXX 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -XXX,XX +XXX,XX @@ 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, &num, - NULL, NULL); + int ret = bdrv_co_block_status_above(bs, NULL, offset, bytes, &num, + NULL, NULL); if (ret < 0) { return ret; @@ -XXX,XX +XXX,XX @@ 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, - &num); + int ret = bdrv_co_is_allocated_above(bs, NULL, false, offset, bytes, + &num); if (ret < 0) { return ret; @@ -XXX,XX +XXX,XX @@ 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 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 <eesposit@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/commit.c +++ b/block/commit.c @@ -XXX,XX +XXX,XX @@ 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, &n); + ret = blk_is_allocated_above(s->top, s->base_overlay, true, + offset, COMMIT_BUFFER_SIZE, &n); 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 XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/block-backend-io.h +++ b/include/sysemu/block-backend-io.h @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -XXX,XX +XXX,XX @@ 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, @@ -XXX,XX +XXX,XX @@ 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, &pnum, NULL, - NULL); + int status = blk_co_block_status_above(exp->common.blk, NULL, + offset + progress, + size - progress, &pnum, NULL, + NULL); bool final; if (status < 0) { @@ -XXX,XX +XXX,XX @@ static int nbd_extent_array_add(NBDExtentArray *ea, return 0; } -static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs, +static int coroutine_fn blockstatus_to_extents(BlockBackend *blk, uint64_t offset, uint64_t bytes, NBDExtentArray *ea) { while (bytes) { uint32_t flags; int64_t num; - int ret = bdrv_co_block_status_above(bs, NULL, offset, bytes, &num, - NULL, NULL); + int ret = blk_co_block_status_above(blk, NULL, offset, bytes, &num, + NULL, NULL); if (ret < 0) { return ret; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs, return 0; } -static int coroutine_fn blockalloc_to_extents(BlockDriverState *bs, +static int coroutine_fn blockalloc_to_extents(BlockBackend *blk, uint64_t offset, uint64_t bytes, NBDExtentArray *ea) { while (bytes) { int64_t num; - int ret = bdrv_co_is_allocated_above(bs, NULL, false, offset, bytes, - &num); + int ret = blk_is_allocated_above(blk, NULL, false, offset, bytes, + &num); if (ret < 0) { return ret; @@ -XXX,XX +XXX,XX @@ 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 coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle, - BlockDriverState *bs, uint64_t offset, + BlockBackend *blk, uint64_t offset, uint32_t length, bool dont_fragment, bool last, uint32_t context_id, Error **errp) @@ -XXX,XX +XXX,XX @@ coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle, g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents); if (context_id == NBD_META_ID_BASE_ALLOCATION) { - ret = blockstatus_to_extents(bs, offset, length, ea); + ret = blockstatus_to_extents(blk, offset, length, ea); } else { - ret = blockalloc_to_extents(bs, offset, length, ea); + ret = blockalloc_to_extents(blk, offset, length, ea); } if (ret < 0) { return nbd_co_send_structured_error( @@ -XXX,XX +XXX,XX @@ static coroutine_fn int nbd_handle_request(NBDClient *client, if (client->export_meta.base_allocation) { ret = nbd_co_send_block_status(client, request->handle, - blk_bs(exp->common.blk), + exp->common.blk, request->from, request->len, dont_fragment, !--contexts_remaining, @@ -XXX,XX +XXX,XX @@ static coroutine_fn int nbd_handle_request(NBDClient *client, if (client->export_meta.allocation_depth) { ret = nbd_co_send_block_status(client, request->handle, - blk_bs(exp->common.blk), + exp->common.blk, request->from, request->len, dont_fragment, !--contexts_remaining, -- 2.31.1 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 <eesposit@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> --- block/vmdk.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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, @@ -XXX,XX +XXX,XX @@ 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) @@ -XXX,XX +XXX,XX @@ 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 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 <kwolf@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ 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, @@ -XXX,XX +XXX,XX @@ 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()) { @@ -XXX,XX +XXX,XX @@ int bdrv_create(BlockDriver *drv, const char* filename, } } -out: - g_free(cco.filename); return ret; } -- 2.31.1 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 <eesposit@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/include/block/block-common.h +++ b/include/block/block-common.h @@ -XXX,XX +XXX,XX @@ #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 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 <eesposit@redhat.com> --- block.c | 72 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ 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, &local_err); - error_propagate(&cco->err, local_err); - cco->ret = ret; + cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, &cco->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(&cco); + 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, &cco); 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 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 <eesposit@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ 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; } @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(BlockDriver *drv, qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vdi_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/vhdx.c b/block/vhdx.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vhdx_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/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vmdk_create_extent(const char *filename, int ret; BlockBackend *blk = NULL; - ret = bdrv_create_file(filename, opts, errp); + ret = bdrv_co_create_file(filename, opts, errp); if (ret < 0) { goto exit; } diff --git a/block/vpc.c b/block/vpc.c index XXXXXXX..XXXXXXX 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vpc_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/include/block/block-global-state.h b/include/block/block-global-state.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -XXX,XX +XXX,XX @@ 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, -- 2.31.1 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 <eesposit@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/include/block/block-common.h +++ b/include/block/block-common.h @@ -XXX,XX +XXX,XX @@ * 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 XXXXXXX..XXXXXXX 100644 --- a/scripts/block-coroutine-wrapper.py +++ b/scripts/block-coroutine-wrapper.py @@ -XXX,XX +XXX,XX @@ 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) @@ -XXX,XX +XXX,XX @@ 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<variant>(_[a-z][a-z0-9_]*)?)\s*' r'(?P<wrapper_name>[a-z][a-z0-9_]*)' r'\((?P<args>[^)]*)\);$', re.MULTILINE) @@ -XXX,XX +XXX,XX @@ 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: @@ -XXX,XX +XXX,XX @@ 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, &s); + + return bdrv_poll_co(&s.poll_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, &s); + + return bdrv_poll_co(&s.poll_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_function = create_g_c_w + if func.create_only_co: + creation_function = create_coroutine_only return f"""\ /* @@ -XXX,XX +XXX,XX @@ def gen_wrapper(func: FuncDecl) -> str: aio_wait_kick(); }} -int {func.name}({ func.gen_list('{decl}') }) -{{ - if (qemu_in_coroutine()) {{ - return {name}({ func.gen_list('{name}') }); - }} else {{ - {struct_name} s = {{ - .poll_state.bs = {bs}, - .poll_state.in_progress = true, - -{ func.gen_block(' .{name} = {name},') } - }}; - - s.poll_state.co = qemu_coroutine_create({name}_entry, &s); - - return bdrv_poll_co(&s.poll_state); - }} -}}""" +{creation_function(func)}""" def gen_wrappers(input_code: str) -> str: -- 2.31.1 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 <eesposit@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/scripts/block-coroutine-wrapper.py +++ b/scripts/block-coroutine-wrapper.py @@ -XXX,XX +XXX,XX @@ 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: @@ -XXX,XX +XXX,XX @@ 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 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 <eesposit@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block/block-gen.h +++ b/block/block-gen.h @@ -XXX,XX +XXX,XX @@ /* 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 */ @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/scripts/block-coroutine-wrapper.py +++ b/scripts/block-coroutine-wrapper.py @@ -XXX,XX +XXX,XX @@ 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) @@ -XXX,XX +XXX,XX @@ 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},') } @@ -XXX,XX +XXX,XX @@ 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 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 <eesposit@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block/block-gen.h +++ b/block/block-gen.h @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/scripts/block-coroutine-wrapper.py +++ b/scripts/block-coroutine-wrapper.py @@ -XXX,XX +XXX,XX @@ 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<return_type>[a-zA-Z][a-zA-Z0-9_]* [*]?)' + r'\s*generated_co_wrapper' r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*' r'(?P<wrapper_name>[a-z][a-z0-9_]*)' r'\((?P<args>[^)]*)\);$', re.MULTILINE) @@ -XXX,XX +XXX,XX @@ 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')) @@ -XXX,XX +XXX,XX @@ 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}') }); @@ -XXX,XX +XXX,XX @@ def create_g_c_w(func: FuncDecl) -> str: s.poll_state.co = qemu_coroutine_create({name}_entry, &s); - return bdrv_poll_co(&s.poll_state); + bdrv_poll_co(&s.poll_state); + return s.ret; }} }}""" @@ -XXX,XX +XXX,XX @@ 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}, @@ -XXX,XX +XXX,XX @@ def create_coroutine_only(func: FuncDecl) -> str: s.poll_state.co = qemu_coroutine_create({name}_entry, &s); - return bdrv_poll_co(&s.poll_state); + bdrv_poll_co(&s.poll_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 @@ -XXX,XX +XXX,XX @@ def gen_wrapper(func: FuncDecl) -> str: typedef struct {struct_name} {{ BdrvPollCo poll_state; + {func.return_type} ret; { func.gen_block(' {decl};') } }} {struct_name}; @@ -XXX,XX +XXX,XX @@ 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 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 <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ 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(); @@ -XXX,XX +XXX,XX @@ 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, &cco->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, &cco); - 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 XXXXXXX..XXXXXXX 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -XXX,XX +XXX,XX @@ 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 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 <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -XXX,XX +XXX,XX @@ 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) { @@ -XXX,XX +XXX,XX @@ 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, - &s); - bdrv_coroutine_enter(bs, co); - BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS); - - return s.ret; - } -} - bool bdrv_supports_persistent_dirty_bitmap(BlockDriverState *bs) { @@ -XXX,XX +XXX,XX @@ 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) { @@ -XXX,XX +XXX,XX @@ 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, - &s); - 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 XXXXXXX..XXXXXXX 100644 --- a/block/meson.build +++ b/block/meson.build @@ -XXX,XX +XXX,XX @@ 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', '../include/sysemu/block-backend-io.h', 'coroutines.h' diff --git a/include/block/block-common.h b/include/block/block-common.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block-common.h +++ b/include/block/block-common.h @@ -XXX,XX +XXX,XX @@ #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" @@ -XXX,XX +XXX,XX @@ #define generated_co_wrapper #define generated_co_wrapper_simple +#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 XXXXXXX..XXXXXXX 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -XXX,XX +XXX,XX @@ 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 generated_co_wrapper_simple +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 XXXXXXX..XXXXXXX 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -XXX,XX +XXX,XX @@ 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 generated_co_wrapper_simple +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); -- 2.31.1