The following changes since commit f1d33f55c47dfdaf8daacd618588ad3ae4c452d1: Merge tag 'pull-testing-gdbstub-plugins-gitdm-061022-3' of https://github.com/stsquad/qemu into staging (2022-10-06 07:11:56 -0400) are available in the Git repository at: git://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to a7ca2eb488ff149c898f43abe103f8bd8e3ca3c4: file-posix: Remove unused s->discard_zeroes (2022-10-07 12:11:41 +0200) ---------------------------------------------------------------- Block layer patches - job: replace AioContext lock with job_mutex - Fixes to make coroutine_fn annotations more accurate - QAPI schema: Fix incorrect example - Code cleanup ---------------------------------------------------------------- Alberto Faria (1): coroutine: Drop coroutine_fn annotation from qemu_coroutine_self() Emanuele Giuseppe Esposito (20): job.c: make job_mutex and job_lock/unlock() public job.h: categorize fields in struct Job job.c: API functions not used outside should be static aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED job.c: add job_lock/unlock while keeping job.h intact job: move and update comments from blockjob.c blockjob: introduce block_job _locked() APIs jobs: add job lock in find_* functions jobs: use job locks also in the unit tests block/mirror.c: use of job helpers in drivers jobs: group together API calls under the same job lock jobs: protect job.aio_context with BQL and job_mutex blockjob.h: categorize fields in struct BlockJob blockjob: rename notifier callbacks as _locked blockjob: protect iostatus field in BlockJob struct job.h: categorize JobDriver callbacks that need the AioContext lock job.c: enable job lock/unlock and remove Aiocontext locks block_job_query: remove atomic read blockjob: remove unused functions job: remove unused functions Kevin Wolf (2): quorum: Remove unnecessary forward declaration file-posix: Remove unused s->discard_zeroes Marc-André Lureau (3): 9p: add missing coroutine_fn annotations migration: add missing coroutine_fn annotations test-coroutine: add missing coroutine_fn annotations Markus Armbruster (1): Revert "qapi: fix examples of blockdev-add with qcow2" Paolo Bonzini (23): block/nvme: separate nvme_get_free_req cases for coroutine/non-coroutine context block: add missing coroutine_fn annotations qcow2: remove incorrect coroutine_fn annotations nbd: remove incorrect coroutine_fn annotations coroutine: remove incorrect coroutine_fn annotations blkverify: add missing coroutine_fn annotations file-posix: add missing coroutine_fn annotations iscsi: add missing coroutine_fn annotations nbd: add missing coroutine_fn annotations nfs: add missing coroutine_fn annotations nvme: add missing coroutine_fn annotations parallels: add missing coroutine_fn annotations qcow2: add missing coroutine_fn annotations copy-before-write: add missing coroutine_fn annotations curl: add missing coroutine_fn annotations qed: add missing coroutine_fn annotations quorum: add missing coroutine_fn annotations throttle: add missing coroutine_fn annotations vmdk: add missing coroutine_fn annotations job: add missing coroutine_fn annotations coroutine-lock: add missing coroutine_fn annotations raw-format: add missing coroutine_fn annotations job: detect change of aiocontext within job coroutine qapi/block-core.json | 10 +- block/qcow2.h | 19 +- hw/9pfs/9p.h | 9 +- include/block/aio-wait.h | 17 +- include/block/blockjob.h | 59 +++- include/block/nbd.h | 2 +- include/qemu/coroutine.h | 4 +- include/qemu/job.h | 306 +++++++++++++----- block.c | 24 +- block/blkverify.c | 2 +- block/block-backend.c | 10 +- block/copy-before-write.c | 9 +- block/curl.c | 2 +- block/file-posix.c | 11 +- block/io.c | 22 +- block/iscsi.c | 3 +- block/mirror.c | 19 +- block/nbd.c | 11 +- block/nfs.c | 2 +- block/nvme.c | 54 ++-- block/parallels.c | 5 +- block/qcow2-cluster.c | 21 +- block/qcow2-refcount.c | 6 +- block/qcow2.c | 5 +- block/qed.c | 4 +- block/quorum.c | 38 +-- block/raw-format.c | 3 +- block/replication.c | 3 + block/throttle.c | 2 +- block/vmdk.c | 22 +- blockdev.c | 129 ++++---- blockjob.c | 132 ++++---- job-qmp.c | 92 +++--- job.c | 674 +++++++++++++++++++++++++-------------- migration/migration.c | 3 +- monitor/qmp-cmds.c | 7 +- qemu-img.c | 17 +- tests/unit/test-bdrv-drain.c | 80 +++-- tests/unit/test-block-iothread.c | 8 +- tests/unit/test-blockjob-txn.c | 24 +- tests/unit/test-blockjob.c | 136 ++++---- tests/unit/test-coroutine.c | 2 +- util/qemu-coroutine-lock.c | 14 +- util/qemu-coroutine.c | 2 +- 44 files changed, 1237 insertions(+), 787 deletions(-) From: Markus Armbruster <armbru@redhat.com> This reverts commit b6522938327141235b97ab38e40c6c4512587373. Kevin Wolf NAKed this patch, because: 'file' is a required member (defined in BlockdevOptionsGenericFormat), removing it makes the example invalid. 'data-file' is only an additional optional member to be used for external data files (i.e. when the guest data is kept separate from the metadata in the .qcow2 file). However, it had already been merged then. Revert. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <20220930171908.846769-1-armbru@redhat.com> Reviewed-by: Victor Toso <victortoso@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -XXX,XX +XXX,XX @@ # -> { "execute": "blockdev-add", # "arguments": { "driver": "qcow2", # "node-name": "node1534", -# "data-file": { "driver": "file", -# "filename": "hd1.qcow2" }, +# "file": { "driver": "file", +# "filename": "hd1.qcow2" }, # "backing": null } } # # <- { "return": {} } @@ -XXX,XX +XXX,XX @@ # "arguments": { # "driver": "qcow2", # "node-name": "test1", -# "data-file": { +# "file": { # "driver": "file", # "filename": "test.qcow2" # } @@ -XXX,XX +XXX,XX @@ # "cache": { # "direct": true # }, -# "data-file": { +# "file": { # "driver": "file", # "filename": "/tmp/test.qcow2" # }, @@ -XXX,XX +XXX,XX @@ # "arguments": { # "driver": "qcow2", # "node-name": "node0", -# "data-file": { +# "file": { # "driver": "file", # "filename": "test.qcow2" # } -- 2.37.3 From: Alberto Faria <afaria@redhat.com> qemu_coroutine_self() can be called from outside coroutine context, returning the leader coroutine, and several such invocations currently exist (mostly in qcow2 tracing calls). Signed-off-by: Alberto Faria <afaria@redhat.com> Message-Id: <20221005175209.975797-1-afaria@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/coroutine.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -XXX,XX +XXX,XX @@ AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co); /** * Get the currently executing coroutine */ -Coroutine *coroutine_fn qemu_coroutine_self(void); +Coroutine *qemu_coroutine_self(void); /** * Return whether or not currently inside a coroutine -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> nvme_get_free_req has very difference semantics when called in coroutine context (where it waits) and in non-coroutine context (where it doesn't). Split the two cases to make it clear what is being requested. Cc: qemu-block@nongnu.org Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-2-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/nvme.c | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/block/nvme.c b/block/nvme.c index XXXXXXX..XXXXXXX 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -XXX,XX +XXX,XX @@ static void nvme_kick(NVMeQueuePair *q) q->need_kick = 0; } -/* Find a free request element if any, otherwise: - * a) if in coroutine context, try to wait for one to become available; - * b) if not in coroutine, return NULL; - */ -static NVMeRequest *nvme_get_free_req(NVMeQueuePair *q) +static NVMeRequest *nvme_get_free_req_nofail_locked(NVMeQueuePair *q) { NVMeRequest *req; - qemu_mutex_lock(&q->lock); - - while (q->free_req_head == -1) { - if (qemu_in_coroutine()) { - trace_nvme_free_req_queue_wait(q->s, q->index); - qemu_co_queue_wait(&q->free_req_queue, &q->lock); - } else { - qemu_mutex_unlock(&q->lock); - return NULL; - } - } - req = &q->reqs[q->free_req_head]; q->free_req_head = req->free_req_next; req->free_req_next = -1; - - qemu_mutex_unlock(&q->lock); return req; } +/* Return a free request element if any, otherwise return NULL. */ +static NVMeRequest *nvme_get_free_req_nowait(NVMeQueuePair *q) +{ + QEMU_LOCK_GUARD(&q->lock); + if (q->free_req_head == -1) { + return NULL; + } + return nvme_get_free_req_nofail_locked(q); +} + +/* + * Wait for a free request to become available if necessary, then + * return it. + */ +static coroutine_fn NVMeRequest *nvme_get_free_req(NVMeQueuePair *q) +{ + QEMU_LOCK_GUARD(&q->lock); + + while (q->free_req_head == -1) { + trace_nvme_free_req_queue_wait(q->s, q->index); + qemu_co_queue_wait(&q->free_req_queue, &q->lock); + } + + return nvme_get_free_req_nofail_locked(q); +} + /* With q->lock */ static void nvme_put_free_req_locked(NVMeQueuePair *q, NVMeRequest *req) { @@ -XXX,XX +XXX,XX @@ static int nvme_admin_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd) AioContext *aio_context = bdrv_get_aio_context(bs); NVMeRequest *req; int ret = -EINPROGRESS; - req = nvme_get_free_req(q); + req = nvme_get_free_req_nowait(q); if (!req) { return -EBUSY; } -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-3-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 7 ++++--- block/block-backend.c | 10 +++++----- block/io.c | 22 +++++++++++----------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static int64_t create_file_fallback_truncate(BlockBackend *blk, * Helper function for bdrv_create_file_fallback(): Zero the first * sector to remove any potentially pre-existing image header. */ -static int create_file_fallback_zero_first_sector(BlockBackend *blk, - int64_t current_size, - Error **errp) +static int coroutine_fn +create_file_fallback_zero_first_sector(BlockBackend *blk, + int64_t current_size, + Error **errp) { int64_t bytes_to_clear; int ret; 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 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, return &acb->common; } -static void blk_aio_read_entry(void *opaque) +static void coroutine_fn blk_aio_read_entry(void *opaque) { BlkAioEmAIOCB *acb = opaque; BlkRwCo *rwco = &acb->rwco; @@ -XXX,XX +XXX,XX @@ static void blk_aio_read_entry(void *opaque) blk_aio_complete(acb); } -static void blk_aio_write_entry(void *opaque) +static void coroutine_fn blk_aio_write_entry(void *opaque) { BlkAioEmAIOCB *acb = opaque; BlkRwCo *rwco = &acb->rwco; @@ -XXX,XX +XXX,XX @@ int coroutine_fn blk_co_ioctl(BlockBackend *blk, unsigned long int req, return ret; } -static void blk_aio_ioctl_entry(void *opaque) +static void coroutine_fn blk_aio_ioctl_entry(void *opaque) { BlkAioEmAIOCB *acb = opaque; BlkRwCo *rwco = &acb->rwco; @@ -XXX,XX +XXX,XX @@ blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes) return bdrv_co_pdiscard(blk->root, offset, bytes); } -static void blk_aio_pdiscard_entry(void *opaque) +static void coroutine_fn blk_aio_pdiscard_entry(void *opaque) { BlkAioEmAIOCB *acb = opaque; BlkRwCo *rwco = &acb->rwco; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn blk_co_do_flush(BlockBackend *blk) return bdrv_co_flush(blk_bs(blk)); } -static void blk_aio_flush_entry(void *opaque) +static void coroutine_fn blk_aio_flush_entry(void *opaque) { BlkAioEmAIOCB *acb = opaque; BlkRwCo *rwco = &acb->rwco; 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 @@ static void coroutine_fn tracked_request_end(BdrvTrackedRequest *req) /** * Add an active request to the tracked requests list */ -static void tracked_request_begin(BdrvTrackedRequest *req, - BlockDriverState *bs, - int64_t offset, - int64_t bytes, - enum BdrvTrackedRequestType type) +static void coroutine_fn tracked_request_begin(BdrvTrackedRequest *req, + BlockDriverState *bs, + int64_t offset, + int64_t bytes, + enum BdrvTrackedRequestType type) { bdrv_check_request(offset, bytes, &error_abort); @@ -XXX,XX +XXX,XX @@ static bool tracked_request_overlaps(BdrvTrackedRequest *req, } /* Called with self->bs->reqs_lock held */ -static BdrvTrackedRequest * +static coroutine_fn BdrvTrackedRequest * bdrv_find_conflicting_request(BdrvTrackedRequest *self) { BdrvTrackedRequest *req; @@ -XXX,XX +XXX,XX @@ static bool bdrv_init_padding(BlockDriverState *bs, return true; } -static int bdrv_padding_rmw_read(BdrvChild *child, - BdrvTrackedRequest *req, - BdrvRequestPadding *pad, - bool zero_middle) +static coroutine_fn int bdrv_padding_rmw_read(BdrvChild *child, + BdrvTrackedRequest *req, + BdrvRequestPadding *pad, + bool zero_middle) { QEMUIOVector local_qiov; BlockDriverState *bs = child->bs; @@ -XXX,XX +XXX,XX @@ out: return ret; } -int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf) +int coroutine_fn bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf) { BlockDriver *drv = bs->drv; CoroutineIOCompletion co = { -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> This is incorrect because qcow2_mark_clean() calls qcow2_flush_caches(). qcow2_mark_clean() is called from non-coroutine context in qcow2_inactivate() and qcow2_amend_options(). Reviewed-by: Alberto Faria <afaria@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-4-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.h | 4 ++-- block/qcow2-refcount.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -XXX,XX +XXX,XX @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry, int qcow2_update_snapshot_refcount(BlockDriverState *bs, int64_t l1_table_offset, int l1_size, int addend); -int coroutine_fn qcow2_flush_caches(BlockDriverState *bs); -int coroutine_fn qcow2_write_caches(BlockDriverState *bs); +int qcow2_flush_caches(BlockDriverState *bs); +int qcow2_write_caches(BlockDriverState *bs); int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix); diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -XXX,XX +XXX,XX @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry, } } -int coroutine_fn qcow2_write_caches(BlockDriverState *bs) +int qcow2_write_caches(BlockDriverState *bs) { BDRVQcow2State *s = bs->opaque; int ret; @@ -XXX,XX +XXX,XX @@ int coroutine_fn qcow2_write_caches(BlockDriverState *bs) return 0; } -int coroutine_fn qcow2_flush_caches(BlockDriverState *bs) +int qcow2_flush_caches(BlockDriverState *bs) { int ret = qcow2_write_caches(bs); if (ret < 0) { -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> nbd_co_establish_connection_cancel() cancels a coroutine but is not called from coroutine context itself, for example in nbd_cancel_in_flight() and in timer callbacks reconnect_delay_timer_cb() and open_timer_cb(). Reviewed-by: Alberto Faria <afaria@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-5-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/nbd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -XXX,XX +XXX,XX @@ QIOChannel *coroutine_fn nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info, bool blocking, Error **errp); -void coroutine_fn nbd_co_establish_connection_cancel(NBDClientConnection *conn); +void nbd_co_establish_connection_cancel(NBDClientConnection *conn); #endif -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> qemu_coroutine_get_aio_context inspects a coroutine, but it does not have to be called from the coroutine itself (or from any coroutine). Reviewed-by: Alberto Faria <afaria@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-6-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/coroutine.h | 2 +- util/qemu-coroutine.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_coroutine_yield(void); /** * Get the AioContext of the given coroutine */ -AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co); +AioContext *qemu_coroutine_get_aio_context(Coroutine *co); /** * Get the currently executing coroutine diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c index XXXXXXX..XXXXXXX 100644 --- a/util/qemu-coroutine.c +++ b/util/qemu-coroutine.c @@ -XXX,XX +XXX,XX @@ bool qemu_coroutine_entered(Coroutine *co) return co->caller; } -AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co) +AioContext *qemu_coroutine_get_aio_context(Coroutine *co) { return co->ctx; } -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-8-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/blkverify.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/blkverify.c b/block/blkverify.c index XXXXXXX..XXXXXXX 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -XXX,XX +XXX,XX @@ blkverify_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes, return blkverify_co_prwv(bs, &r, offset, bytes, qiov, qiov, flags, true); } -static int blkverify_co_flush(BlockDriverState *bs) +static int coroutine_fn blkverify_co_flush(BlockDriverState *bs) { BDRVBlkverifyState *s = bs->opaque; -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-9-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -XXX,XX +XXX,XX @@ static void raw_aio_unplug(BlockDriverState *bs) #endif } -static int raw_co_flush_to_disk(BlockDriverState *bs) +static int coroutine_fn raw_co_flush_to_disk(BlockDriverState *bs) { BDRVRawState *s = bs->opaque; RawPosixAIOData acb; -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-10-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/iscsi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/iscsi.c b/block/iscsi.c index XXXXXXX..XXXXXXX 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -XXX,XX +XXX,XX @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status, } } -static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, struct IscsiTask *iTask) +static void coroutine_fn +iscsi_co_init_iscsitask(IscsiLun *iscsilun, struct IscsiTask *iTask) { *iTask = (struct IscsiTask) { .co = qemu_coroutine_self(), -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-11-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/nbd.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index XXXXXXX..XXXXXXX 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -XXX,XX +XXX,XX @@ static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret) * nbd_reply_chunk_iter_receive * The pointer stored in @payload requires g_free() to free it. */ -static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s, - NBDReplyChunkIter *iter, - uint64_t handle, - QEMUIOVector *qiov, NBDReply *reply, - void **payload) +static bool coroutine_fn nbd_reply_chunk_iter_receive(BDRVNBDState *s, + NBDReplyChunkIter *iter, + uint64_t handle, + QEMUIOVector *qiov, + NBDReply *reply, + void **payload) { int ret, request_ret; NBDReply local_reply; -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-12-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/nfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/nfs.c b/block/nfs.c index XXXXXXX..XXXXXXX 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -XXX,XX +XXX,XX @@ static void nfs_process_write(void *arg) qemu_mutex_unlock(&client->mutex); } -static void nfs_co_init_task(BlockDriverState *bs, NFSRPC *task) +static void coroutine_fn nfs_co_init_task(BlockDriverState *bs, NFSRPC *task) { *task = (NFSRPC) { .co = qemu_coroutine_self(), -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-13-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/nvme.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/block/nvme.c b/block/nvme.c index XXXXXXX..XXXXXXX 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -XXX,XX +XXX,XX @@ static inline bool nvme_qiov_aligned(BlockDriverState *bs, return true; } -static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes, - QEMUIOVector *qiov, bool is_write, int flags) +static coroutine_fn int nvme_co_prw(BlockDriverState *bs, + uint64_t offset, uint64_t bytes, + QEMUIOVector *qiov, bool is_write, + int flags) { BDRVNVMeState *s = bs->opaque; int r; -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-14-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/parallels.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 int64_t block_status(BDRVParallelsState *s, int64_t sector_num, return start_off; } -static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum) +static coroutine_fn int64_t allocate_clusters(BlockDriverState *bs, + int64_t sector_num, + int nb_sectors, int *pnum) { int ret = 0; BDRVParallelsState *s = bs->opaque; -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-15-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.h | 15 ++++++++------- block/qcow2-cluster.c | 21 ++++++++++++--------- block/qcow2-refcount.c | 2 +- block/qcow2.c | 5 +++-- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -XXX,XX +XXX,XX @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order, void *cb_opaque, Error **errp); int qcow2_shrink_reftable(BlockDriverState *bs); int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size); -int qcow2_detect_metadata_preallocation(BlockDriverState *bs); +int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs); /* qcow2-cluster.c functions */ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, @@ -XXX,XX +XXX,XX @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num, int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset, unsigned int *bytes, uint64_t *host_offset, QCow2SubclusterType *subcluster_type); -int qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset, - unsigned int *bytes, uint64_t *host_offset, - QCowL2Meta **m); +int coroutine_fn qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset, + unsigned int *bytes, + uint64_t *host_offset, QCowL2Meta **m); int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, uint64_t offset, int compressed_size, @@ -XXX,XX +XXX,XX @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, void qcow2_parse_compressed_l2_entry(BlockDriverState *bs, uint64_t l2_entry, uint64_t *coffset, int *csize); -int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m); +int coroutine_fn qcow2_alloc_cluster_link_l2(BlockDriverState *bs, + QCowL2Meta *m); void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m); int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset, uint64_t bytes, enum qcow2_discard_type type, bool full_discard); -int qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset, - uint64_t bytes, int flags); +int coroutine_fn qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset, + uint64_t bytes, int flags); int qcow2_expand_zero_clusters(BlockDriverState *bs, BlockDriverAmendStatusCB *status_cb, diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -XXX,XX +XXX,XX @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, return 0; } -static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) +static int coroutine_fn perform_cow(BlockDriverState *bs, QCowL2Meta *m) { BDRVQcow2State *s = bs->opaque; Qcow2COWRegion *start = &m->cow_start; @@ -XXX,XX +XXX,XX @@ fail: return ret; } -int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) +int coroutine_fn qcow2_alloc_cluster_link_l2(BlockDriverState *bs, + QCowL2Meta *m) { BDRVQcow2State *s = bs->opaque; int i, j = 0, l2_index, ret; @@ -XXX,XX +XXX,XX @@ static int count_single_write_clusters(BlockDriverState *bs, int nb_clusters, * information on cluster allocation may be invalid now. The caller * must start over anyway, so consider *cur_bytes undefined. */ -static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset, - uint64_t *cur_bytes, QCowL2Meta **m) +static int coroutine_fn handle_dependencies(BlockDriverState *bs, + uint64_t guest_offset, + uint64_t *cur_bytes, QCowL2Meta **m) { BDRVQcow2State *s = bs->opaque; QCowL2Meta *old_alloc; @@ -XXX,XX +XXX,XX @@ out: * * Return 0 on success and -errno in error cases */ -int qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset, - unsigned int *bytes, uint64_t *host_offset, - QCowL2Meta **m) +int coroutine_fn qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset, + unsigned int *bytes, + uint64_t *host_offset, + QCowL2Meta **m) { BDRVQcow2State *s = bs->opaque; uint64_t start, remaining; @@ -XXX,XX +XXX,XX @@ out: return ret; } -int qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset, - uint64_t bytes, int flags) +int coroutine_fn qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset, + uint64_t bytes, int flags) { BDRVQcow2State *s = bs->opaque; uint64_t end_offset = offset + bytes; diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -XXX,XX +XXX,XX @@ int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size) return -EIO; } -int qcow2_detect_metadata_preallocation(BlockDriverState *bs) +int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs) { BDRVQcow2State *s = bs->opaque; int64_t i, end_cluster, cluster_count = 0, threshold; 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 bool merge_cow(uint64_t offset, unsigned bytes, * Return 1 if the COW regions read as zeroes, 0 if not, < 0 on error. * Note that returning 0 does not guarantee non-zero data. */ -static int is_zero_cow(BlockDriverState *bs, QCowL2Meta *m) +static int coroutine_fn is_zero_cow(BlockDriverState *bs, QCowL2Meta *m) { /* * This check is designed for optimization shortcut so it must be @@ -XXX,XX +XXX,XX @@ static int is_zero_cow(BlockDriverState *bs, QCowL2Meta *m) m->cow_end.nb_bytes); } -static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta) +static int coroutine_fn handle_alloc_space(BlockDriverState *bs, + QCowL2Meta *l2meta) { BDRVQcow2State *s = bs->opaque; QCowL2Meta *m; -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-16-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/copy-before-write.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn cbw_co_flush(BlockDriverState *bs) * It's guaranteed that guest writes will not interact in the region until * cbw_snapshot_read_unlock() called. */ -static BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs, - int64_t offset, int64_t bytes, - int64_t *pnum, BdrvChild **file) +static coroutine_fn BlockReq * +cbw_snapshot_read_lock(BlockDriverState *bs, int64_t offset, int64_t bytes, + int64_t *pnum, BdrvChild **file) { BDRVCopyBeforeWriteState *s = bs->opaque; BlockReq *req = g_new(BlockReq, 1); @@ -XXX,XX +XXX,XX @@ static BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs, return req; } -static void cbw_snapshot_read_unlock(BlockDriverState *bs, BlockReq *req) +static coroutine_fn void +cbw_snapshot_read_unlock(BlockDriverState *bs, BlockReq *req) { BDRVCopyBeforeWriteState *s = bs->opaque; -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-17-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/curl.c b/block/curl.c index XXXXXXX..XXXXXXX 100644 --- a/block/curl.c +++ b/block/curl.c @@ -XXX,XX +XXX,XX @@ out_noclean: return -EINVAL; } -static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb) +static void coroutine_fn curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb) { CURLState *state; int running; -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-18-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qed.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 CachedL2Table *qed_new_l2_table(BDRVQEDState *s) return l2_table; } -static bool qed_plug_allocating_write_reqs(BDRVQEDState *s) +static bool coroutine_fn qed_plug_allocating_write_reqs(BDRVQEDState *s) { qemu_co_mutex_lock(&s->table_lock); @@ -XXX,XX +XXX,XX @@ static bool qed_plug_allocating_write_reqs(BDRVQEDState *s) return true; } -static void qed_unplug_allocating_write_reqs(BDRVQEDState *s) +static void coroutine_fn qed_unplug_allocating_write_reqs(BDRVQEDState *s) { qemu_co_mutex_lock(&s->table_lock); assert(s->allocating_write_reqs_plugged); -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-19-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/quorum.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/block/quorum.c b/block/quorum.c index XXXXXXX..XXXXXXX 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -XXX,XX +XXX,XX @@ static bool quorum_64bits_compare(QuorumVoteValue *a, QuorumVoteValue *b) return a->l == b->l; } -static QuorumAIOCB *quorum_aio_get(BlockDriverState *bs, - QEMUIOVector *qiov, - uint64_t offset, - uint64_t bytes, - int flags) +static QuorumAIOCB *coroutine_fn quorum_aio_get(BlockDriverState *bs, + QEMUIOVector *qiov, + uint64_t offset, uint64_t bytes, + int flags) { BDRVQuorumState *s = bs->opaque; QuorumAIOCB *acb = g_new(QuorumAIOCB, 1); @@ -XXX,XX +XXX,XX @@ static void quorum_report_bad_versions(BDRVQuorumState *s, } } -static void quorum_rewrite_entry(void *opaque) +static void coroutine_fn quorum_rewrite_entry(void *opaque) { QuorumCo *co = opaque; QuorumAIOCB *acb = co->acb; @@ -XXX,XX +XXX,XX @@ free_exit: quorum_free_vote_list(&acb->votes); } -static void read_quorum_children_entry(void *opaque) +static void coroutine_fn read_quorum_children_entry(void *opaque) { QuorumCo *co = opaque; QuorumAIOCB *acb = co->acb; @@ -XXX,XX +XXX,XX @@ static void read_quorum_children_entry(void *opaque) } } -static int read_quorum_children(QuorumAIOCB *acb) +static int coroutine_fn read_quorum_children(QuorumAIOCB *acb) { BDRVQuorumState *s = acb->bs->opaque; int i; @@ -XXX,XX +XXX,XX @@ static int read_quorum_children(QuorumAIOCB *acb) return acb->vote_ret; } -static int read_fifo_child(QuorumAIOCB *acb) +static int coroutine_fn read_fifo_child(QuorumAIOCB *acb) { BDRVQuorumState *s = acb->bs->opaque; int n, ret; @@ -XXX,XX +XXX,XX @@ static int read_fifo_child(QuorumAIOCB *acb) return ret; } -static int quorum_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, - QEMUIOVector *qiov, BdrvRequestFlags flags) +static int coroutine_fn quorum_co_preadv(BlockDriverState *bs, + int64_t offset, int64_t bytes, + QEMUIOVector *qiov, + BdrvRequestFlags flags) { BDRVQuorumState *s = bs->opaque; QuorumAIOCB *acb = quorum_aio_get(bs, qiov, offset, bytes, flags); @@ -XXX,XX +XXX,XX @@ static int quorum_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, return ret; } -static void write_quorum_entry(void *opaque) +static void coroutine_fn write_quorum_entry(void *opaque) { QuorumCo *co = opaque; QuorumAIOCB *acb = co->acb; @@ -XXX,XX +XXX,XX @@ static void write_quorum_entry(void *opaque) } } -static int quorum_co_pwritev(BlockDriverState *bs, int64_t offset, - int64_t bytes, QEMUIOVector *qiov, - BdrvRequestFlags flags) +static int coroutine_fn quorum_co_pwritev(BlockDriverState *bs, int64_t offset, + int64_t bytes, QEMUIOVector *qiov, + BdrvRequestFlags flags) { BDRVQuorumState *s = bs->opaque; QuorumAIOCB *acb = quorum_aio_get(bs, qiov, offset, bytes, flags); @@ -XXX,XX +XXX,XX @@ static int quorum_co_pwritev(BlockDriverState *bs, int64_t offset, return ret; } -static int quorum_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, - int64_t bytes, BdrvRequestFlags flags) +static int coroutine_fn quorum_co_pwrite_zeroes(BlockDriverState *bs, + int64_t offset, int64_t bytes, + BdrvRequestFlags flags) { return quorum_co_pwritev(bs, offset, bytes, NULL, -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-20-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/throttle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/throttle.c b/block/throttle.c index XXXXXXX..XXXXXXX 100644 --- a/block/throttle.c +++ b/block/throttle.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn throttle_co_pwritev_compressed(BlockDriverState *bs, BDRV_REQ_WRITE_COMPRESSED); } -static int throttle_co_flush(BlockDriverState *bs) +static int coroutine_fn throttle_co_flush(BlockDriverState *bs) { return bdrv_co_flush(bs->file->bs); } -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-21-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vmdk.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 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 @@ static int coroutine_fn vmdk_co_block_status(BlockDriverState *bs, return ret; } -static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset, - int64_t offset_in_cluster, QEMUIOVector *qiov, - uint64_t qiov_offset, uint64_t n_bytes, - uint64_t offset) +static int coroutine_fn +vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset, + int64_t offset_in_cluster, QEMUIOVector *qiov, + uint64_t qiov_offset, uint64_t n_bytes, + uint64_t offset) { int ret; VmdkGrainMarker *data = NULL; @@ -XXX,XX +XXX,XX @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset, return ret; } -static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset, - int64_t offset_in_cluster, QEMUIOVector *qiov, - int bytes) +static int coroutine_fn +vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset, + int64_t offset_in_cluster, QEMUIOVector *qiov, + int bytes) { int ret; int cluster_bytes, buf_bytes; @@ -XXX,XX +XXX,XX @@ fail: * * Returns: error code with 0 for success. */ -static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset, - uint64_t bytes, QEMUIOVector *qiov, - bool zeroed, bool zero_dry_run) +static int coroutine_fn vmdk_pwritev(BlockDriverState *bs, uint64_t offset, + uint64_t bytes, QEMUIOVector *qiov, + bool zeroed, bool zero_dry_run) { BDRVVmdkState *s = bs->opaque; VmdkExtent *extent = NULL; -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-22-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 2 +- job.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ void coroutine_fn job_pause_point(Job *job); * * Yield the job coroutine. */ -void job_yield(Job *job); +void coroutine_fn job_yield(Job *job); /** * @job: The job that calls the function. diff --git a/job.c b/job.c index XXXXXXX..XXXXXXX 100644 --- a/job.c +++ b/job.c @@ -XXX,XX +XXX,XX @@ void coroutine_fn job_pause_point(Job *job) } } -void job_yield(Job *job) +void coroutine_fn job_yield(Job *job) { assert(job->busy); -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-23-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- util/qemu-coroutine-lock.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c index XXXXXXX..XXXXXXX 100644 --- a/util/qemu-coroutine-lock.c +++ b/util/qemu-coroutine-lock.c @@ -XXX,XX +XXX,XX @@ typedef struct CoWaitRecord { QSLIST_ENTRY(CoWaitRecord) next; } CoWaitRecord; -static void push_waiter(CoMutex *mutex, CoWaitRecord *w) +static void coroutine_fn push_waiter(CoMutex *mutex, CoWaitRecord *w) { w->co = qemu_coroutine_self(); QSLIST_INSERT_HEAD_ATOMIC(&mutex->from_push, w, next); @@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_init(CoRwlock *lock) } /* Releases the internal CoMutex. */ -static void qemu_co_rwlock_maybe_wake_one(CoRwlock *lock) +static void coroutine_fn qemu_co_rwlock_maybe_wake_one(CoRwlock *lock) { CoRwTicket *tkt = QSIMPLEQ_FIRST(&lock->tickets); Coroutine *co = NULL; @@ -XXX,XX +XXX,XX @@ static void qemu_co_rwlock_maybe_wake_one(CoRwlock *lock) } } -void qemu_co_rwlock_rdlock(CoRwlock *lock) +void coroutine_fn qemu_co_rwlock_rdlock(CoRwlock *lock) { Coroutine *self = qemu_coroutine_self(); @@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_rdlock(CoRwlock *lock) self->locks_held++; } -void qemu_co_rwlock_unlock(CoRwlock *lock) +void coroutine_fn qemu_co_rwlock_unlock(CoRwlock *lock) { Coroutine *self = qemu_coroutine_self(); @@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_unlock(CoRwlock *lock) qemu_co_rwlock_maybe_wake_one(lock); } -void qemu_co_rwlock_downgrade(CoRwlock *lock) +void coroutine_fn qemu_co_rwlock_downgrade(CoRwlock *lock) { qemu_co_mutex_lock(&lock->mutex); assert(lock->owners == -1); @@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_downgrade(CoRwlock *lock) qemu_co_rwlock_maybe_wake_one(lock); } -void qemu_co_rwlock_wrlock(CoRwlock *lock) +void coroutine_fn qemu_co_rwlock_wrlock(CoRwlock *lock) { Coroutine *self = qemu_coroutine_self(); @@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_wrlock(CoRwlock *lock) self->locks_held++; } -void qemu_co_rwlock_upgrade(CoRwlock *lock) +void coroutine_fn qemu_co_rwlock_upgrade(CoRwlock *lock) { qemu_co_mutex_lock(&lock->mutex); assert(lock->owners > 0); -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-24-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/raw-format.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 void raw_lock_medium(BlockDriverState *bs, bool locked) bdrv_lock_medium(bs->file->bs, locked); } -static int raw_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf) +static int coroutine_fn raw_co_ioctl(BlockDriverState *bs, + unsigned long int req, void *buf) { BDRVRawState *s = bs->opaque; if (s->offset || s->has_size) { -- 2.37.3 From: Marc-André Lureau <marcandre.lureau@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Acked-by: Greg Kurz <groug@kaod.org> Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-25-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/9pfs/9p.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h index XXXXXXX..XXXXXXX 100644 --- a/hw/9pfs/9p.h +++ b/hw/9pfs/9p.h @@ -XXX,XX +XXX,XX @@ typedef struct V9fsGetlock extern int open_fd_hw; extern int total_open_fd; -static inline void v9fs_path_write_lock(V9fsState *s) +static inline void coroutine_fn +v9fs_path_write_lock(V9fsState *s) { if (s->ctx.export_flags & V9FS_PATHNAME_FSCONTEXT) { qemu_co_rwlock_wrlock(&s->rename_lock); } } -static inline void v9fs_path_read_lock(V9fsState *s) +static inline void coroutine_fn +v9fs_path_read_lock(V9fsState *s) { if (s->ctx.export_flags & V9FS_PATHNAME_FSCONTEXT) { qemu_co_rwlock_rdlock(&s->rename_lock); } } -static inline void v9fs_path_unlock(V9fsState *s) +static inline void coroutine_fn +v9fs_path_unlock(V9fsState *s) { if (s->ctx.export_flags & V9FS_PATHNAME_FSCONTEXT) { qemu_co_rwlock_unlock(&s->rename_lock); -- 2.37.3 From: Marc-André Lureau <marcandre.lureau@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-26-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- migration/migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/migration/migration.c b/migration/migration.c index XXXXXXX..XXXXXXX 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -XXX,XX +XXX,XX @@ static void process_incoming_migration_bh(void *opaque) migration_incoming_state_destroy(); } -static void process_incoming_migration_co(void *opaque) +static void coroutine_fn +process_incoming_migration_co(void *opaque) { MigrationIncomingState *mis = migration_incoming_get_current(); PostcopyState ps; -- 2.37.3 From: Marc-André Lureau <marcandre.lureau@redhat.com> Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-27-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/unit/test-coroutine.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test-coroutine.c b/tests/unit/test-coroutine.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-coroutine.c +++ b/tests/unit/test-coroutine.c @@ -XXX,XX +XXX,XX @@ static void perf_baseline(void) g_test_message("Function call %u iterations: %f s", maxcycles, duration); } -static __attribute__((noinline)) void perf_cost_func(void *opaque) +static __attribute__((noinline)) void coroutine_fn perf_cost_func(void *opaque) { qemu_coroutine_yield(); } -- 2.37.3 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20221006122607.162769-1-kwolf@redhat.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/quorum.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/block/quorum.c b/block/quorum.c index XXXXXXX..XXXXXXX 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -XXX,XX +XXX,XX @@ static bool quorum_has_too_much_io_failed(QuorumAIOCB *acb) return false; } -static int read_fifo_child(QuorumAIOCB *acb); - static void quorum_copy_qiov(QEMUIOVector *dest, QEMUIOVector *source) { int i; -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> job mutex will be used to protect the job struct elements and list, replacing AioContext locks. Right now use a shared lock for all jobs, in order to keep things simple. Once the AioContext lock is gone, we can introduce per-job locks. To simplify the switch from aiocontext to job lock, introduce *nop* lock/unlock functions and macros. We want to always call job_lock/unlock outside the AioContext locks, and not vice-versa, otherwise we might get a deadlock. This is not straightforward to do, and that's why we start with nop functions. Once everything is protected by job_lock/unlock, we can change the nop into an actual mutex and remove the aiocontext lock. Since job_mutex is already being used, add static real_job_{lock/unlock} for the existing usage. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20220926093214.506243-2-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 24 ++++++++++++++++++++++++ job.c | 35 +++++++++++++++++++++++------------ 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ typedef enum JobCreateFlags { JOB_MANUAL_DISMISS = 0x04, } JobCreateFlags; +extern QemuMutex job_mutex; + +#define JOB_LOCK_GUARD() /* QEMU_LOCK_GUARD(&job_mutex) */ + +#define WITH_JOB_LOCK_GUARD() /* WITH_QEMU_LOCK_GUARD(&job_mutex) */ + +/** + * job_lock: + * + * Take the mutex protecting the list of jobs and their status. + * Most functions called by the monitor need to call job_lock + * and job_unlock manually. On the other hand, function called + * by the block jobs themselves and by the block layer will take the + * lock for you. + */ +void job_lock(void); + +/** + * job_unlock: + * + * Release the mutex protecting the list of jobs and their status. + */ +void job_unlock(void); + /** * Allocate and return a new job transaction. Jobs can be added to the * transaction using job_txn_add_job(). diff --git a/job.c b/job.c index XXXXXXX..XXXXXXX 100644 --- a/job.c +++ b/job.c @@ -XXX,XX +XXX,XX @@ #include "trace/trace-root.h" #include "qapi/qapi-events-job.h" +/* + * job_mutex protects the jobs list, but also makes the + * struct job fields thread-safe. + */ +QemuMutex job_mutex; + static QLIST_HEAD(, Job) jobs = QLIST_HEAD_INITIALIZER(jobs); /* Job State Transition Table */ @@ -XXX,XX +XXX,XX @@ struct JobTxn { int refcnt; }; -/* Right now, this mutex is only needed to synchronize accesses to job->busy - * and job->sleep_timer, such as concurrent calls to job_do_yield and - * job_enter. */ -static QemuMutex job_mutex; +void job_lock(void) +{ + /* nop */ +} + +void job_unlock(void) +{ + /* nop */ +} -static void job_lock(void) +static void real_job_lock(void) { qemu_mutex_lock(&job_mutex); } -static void job_unlock(void) +static void real_job_unlock(void) { qemu_mutex_unlock(&job_mutex); } @@ -XXX,XX +XXX,XX @@ void job_enter_cond(Job *job, bool(*fn)(Job *job)) return; } - job_lock(); + real_job_lock(); if (job->busy) { - job_unlock(); + real_job_unlock(); return; } if (fn && !fn(job)) { - job_unlock(); + real_job_unlock(); return; } assert(!job->deferred_to_main_loop); timer_del(&job->sleep_timer); job->busy = true; - job_unlock(); + real_job_unlock(); aio_co_enter(job->aio_context, job->co); } @@ -XXX,XX +XXX,XX @@ void job_enter(Job *job) * called explicitly. */ static void coroutine_fn job_do_yield(Job *job, uint64_t ns) { - job_lock(); + real_job_lock(); if (ns != -1) { timer_mod(&job->sleep_timer, ns); } job->busy = false; job_event_idle(job); - job_unlock(); + real_job_unlock(); qemu_coroutine_yield(); /* Set by job_enter_cond() before re-entering the coroutine. */ -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Categorize the fields in struct Job to understand which ones need to be protected by the job mutex and which don't. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20220926093214.506243-3-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 61 +++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ typedef struct JobTxn JobTxn; * Long-running operation. */ typedef struct Job { + + /* Fields set at initialization (job_create), and never modified */ + /** The ID of the job. May be NULL for internal jobs. */ char *id; - /** The type of this job. */ + /** + * The type of this job. + * All callbacks are called with job_mutex *not* held. + */ const JobDriver *driver; - /** Reference count of the block job */ - int refcnt; - - /** Current state; See @JobStatus for details. */ - JobStatus status; - - /** AioContext to run the job coroutine in */ - AioContext *aio_context; - /** * The coroutine that executes the job. If not NULL, it is reentered when * busy is false and the job is cancelled. + * Initialized in job_start() */ Coroutine *co; + /** True if this job should automatically finalize itself */ + bool auto_finalize; + + /** True if this job should automatically dismiss itself */ + bool auto_dismiss; + + /** The completion function that will be called when the job completes. */ + BlockCompletionFunc *cb; + + /** The opaque value that is passed to the completion function. */ + void *opaque; + + /* ProgressMeter API is thread-safe */ + ProgressMeter progress; + + + /** Protected by AioContext lock */ + + /** AioContext to run the job coroutine in */ + AioContext *aio_context; + + /** Reference count of the block job */ + int refcnt; + + /** Current state; See @JobStatus for details. */ + JobStatus status; + /** * Timer that is used by @job_sleep_ns. Accessed under job_mutex (in * job.c). @@ -XXX,XX +XXX,XX @@ typedef struct Job { /** Set to true when the job has deferred work to the main loop. */ bool deferred_to_main_loop; - /** True if this job should automatically finalize itself */ - bool auto_finalize; - - /** True if this job should automatically dismiss itself */ - bool auto_dismiss; - - ProgressMeter progress; - /** * Return code from @run and/or @prepare callback(s). * Not final until the job has reached the CONCLUDED status. @@ -XXX,XX +XXX,XX @@ typedef struct Job { */ Error *err; - /** The completion function that will be called when the job completes. */ - BlockCompletionFunc *cb; - - /** The opaque value that is passed to the completion function. */ - void *opaque; - /** Notifiers called when a cancelled job is finalised */ NotifierList on_finalize_cancelled; @@ -XXX,XX +XXX,XX @@ typedef struct Job { /** * Callbacks and other information about a Job driver. + * All callbacks are invoked with job_mutex *not* held. */ struct JobDriver { @@ -XXX,XX +XXX,XX @@ void coroutine_fn job_yield(Job *job); */ void coroutine_fn job_sleep_ns(Job *job, int64_t ns); - /** Returns the JobType of a given Job. */ JobType job_type(const Job *job); -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> job_event_* functions can all be static, as they are not used outside job.c. Same applies for job_txn_add_job(). Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20220926093214.506243-4-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 18 ------------------ job.c | 22 +++++++++++++++++++--- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ JobTxn *job_txn_new(void); */ void job_txn_unref(JobTxn *txn); -/** - * @txn: The transaction (may be NULL) - * @job: Job to add to the transaction - * - * Add @job to the transaction. The @job must not already be in a transaction. - * The caller must call either job_txn_unref() or job_completed() to release - * the reference that is automatically grabbed here. - * - * If @txn is NULL, the function does nothing. - */ -void job_txn_add_job(JobTxn *txn, Job *job); - /** * Create a new long-running job and return it. * @@ -XXX,XX +XXX,XX @@ void job_progress_set_remaining(Job *job, uint64_t remaining); */ void job_progress_increase_remaining(Job *job, uint64_t delta); -/** To be called when a cancelled job is finalised. */ -void job_event_cancelled(Job *job); - -/** To be called when a successfully completed job is finalised. */ -void job_event_completed(Job *job); - /** * Conditionally enter the job coroutine if the job is ready to run, not * already busy and fn() returns true. fn() is called while under the job_lock diff --git a/job.c b/job.c index XXXXXXX..XXXXXXX 100644 --- a/job.c +++ b/job.c @@ -XXX,XX +XXX,XX @@ void job_txn_unref(JobTxn *txn) } } -void job_txn_add_job(JobTxn *txn, Job *job) +/** + * @txn: The transaction (may be NULL) + * @job: Job to add to the transaction + * + * Add @job to the transaction. The @job must not already be in a transaction. + * The caller must call either job_txn_unref() or job_completed() to release + * the reference that is automatically grabbed here. + * + * If @txn is NULL, the function does nothing. + */ +static void job_txn_add_job(JobTxn *txn, Job *job) { if (!txn) { return; @@ -XXX,XX +XXX,XX @@ void job_progress_increase_remaining(Job *job, uint64_t delta) progress_increase_remaining(&job->progress, delta); } -void job_event_cancelled(Job *job) +/** + * To be called when a cancelled job is finalised. + */ +static void job_event_cancelled(Job *job) { notifier_list_notify(&job->on_finalize_cancelled, job); } -void job_event_completed(Job *job) +/** + * To be called when a successfully completed job is finalised. + */ +static void job_event_completed(Job *job) { notifier_list_notify(&job->on_finalize_completed, job); } -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Same as AIO_WAIT_WHILE macro, but if we are in the Main loop do not release and then acquire ctx_ 's aiocontext. Once all Aiocontext locks go away, this macro will replace AIO_WAIT_WHILE. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20220926093214.506243-5-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/aio-wait.h | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/aio-wait.h +++ b/include/block/aio-wait.h @@ -XXX,XX +XXX,XX @@ typedef struct { extern AioWait global_aio_wait; /** - * AIO_WAIT_WHILE: + * AIO_WAIT_WHILE_INTERNAL: * @ctx: the aio context, or NULL if multiple aio contexts (for which the * caller does not hold a lock) are involved in the polling condition. * @cond: wait while this conditional expression is true + * @unlock: whether to unlock and then lock again @ctx. This apples + * only when waiting for another AioContext from the main loop. + * Otherwise it's ignored. * * Wait while a condition is true. Use this to implement synchronous * operations that require event loop activity. @@ -XXX,XX +XXX,XX @@ extern AioWait global_aio_wait; * wait on conditions between two IOThreads since that could lead to deadlock, * go via the main loop instead. */ -#define AIO_WAIT_WHILE(ctx, cond) ({ \ +#define AIO_WAIT_WHILE_INTERNAL(ctx, cond, unlock) ({ \ bool waited_ = false; \ AioWait *wait_ = &global_aio_wait; \ AioContext *ctx_ = (ctx); \ @@ -XXX,XX +XXX,XX @@ extern AioWait global_aio_wait; assert(qemu_get_current_aio_context() == \ qemu_get_aio_context()); \ while ((cond)) { \ - if (ctx_) { \ + if (unlock && ctx_) { \ aio_context_release(ctx_); \ } \ aio_poll(qemu_get_aio_context(), true); \ - if (ctx_) { \ + if (unlock && ctx_) { \ aio_context_acquire(ctx_); \ } \ waited_ = true; \ @@ -XXX,XX +XXX,XX @@ extern AioWait global_aio_wait; qatomic_dec(&wait_->num_waiters); \ waited_; }) +#define AIO_WAIT_WHILE(ctx, cond) \ + AIO_WAIT_WHILE_INTERNAL(ctx, cond, true) + +#define AIO_WAIT_WHILE_UNLOCKED(ctx, cond) \ + AIO_WAIT_WHILE_INTERNAL(ctx, cond, false) + /** * aio_wait_kick: * Wake up the main thread if it is waiting on AIO_WAIT_WHILE(). During -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> With "intact" we mean that all job.h functions implicitly take the lock. Therefore API callers are unmodified. This means that: - many static functions that will be always called with job lock held become _locked, and call _locked functions - all public functions take the lock internally if needed, and call _locked functions - all public functions called internally by other functions in job.c will have a _locked counterpart (sometimes public), to avoid deadlocks (job lock already taken). These functions are not used for now. - some public functions called only from exernal files (not job.c) do not have _locked() counterpart and take the lock inside. Others won't need the lock at all because use fields only set at initialization and never modified. job_{lock/unlock} is independent from real_job_{lock/unlock}. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop* Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20220926093214.506243-6-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 138 +++++++++- job.c | 610 ++++++++++++++++++++++++++++++++------------- 2 files changed, 561 insertions(+), 187 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ JobTxn *job_txn_new(void); */ void job_txn_unref(JobTxn *txn); +/* + * Same as job_txn_unref(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_txn_unref_locked(JobTxn *txn); + /** * Create a new long-running job and return it. + * Called with job_mutex *not* held. * * @job_id: The id of the newly-created job, or %NULL for internal jobs * @driver: The class object for the newly-created job. @@ -XXX,XX +XXX,XX @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, */ void job_ref(Job *job); +/* Same as job_ref(), but called with job lock held. */ +void job_ref_locked(Job *job); + /** * Release a reference that was previously acquired with job_ref() or * job_create(). If it's the last reference to the object, it will be freed. */ void job_unref(Job *job); +/* Same as job_unref(), but called with job lock held. */ +void job_unref_locked(Job *job); + /** * @job: The job that has made progress * @done: How much progress the job made since the last call * * Updates the progress counter of the job. + * + * May be called with mutex held or not held. */ void job_progress_update(Job *job, uint64_t done); @@ -XXX,XX +XXX,XX @@ void job_progress_update(Job *job, uint64_t done); * * Sets the expected end value of the progress counter of a job so that a * completion percentage can be calculated when the progress is updated. + * + * May be called with mutex held or not held. */ void job_progress_set_remaining(Job *job, uint64_t remaining); @@ -XXX,XX +XXX,XX @@ void job_progress_set_remaining(Job *job, uint64_t remaining); * length before, and job_progress_update() afterwards. * (So the operation acts as a parenthesis in regards to the main job * operation running in background.) + * + * May be called with mutex held or not held. */ void job_progress_increase_remaining(Job *job, uint64_t delta); @@ -XXX,XX +XXX,XX @@ void job_progress_increase_remaining(Job *job, uint64_t delta); */ void job_enter_cond(Job *job, bool(*fn)(Job *job)); +/* + * Same as job_enter_cond(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)); + /** * @job: A job that has not yet been started. * * Begins execution of a job. * Takes ownership of one reference to the job object. + * + * Called with job_mutex *not* held. */ void job_start(Job *job); @@ -XXX,XX +XXX,XX @@ void job_start(Job *job); * @job: The job to enter. * * Continue the specified job by entering the coroutine. + * Called with job_mutex *not* held. */ void job_enter(Job *job); @@ -XXX,XX +XXX,XX @@ void job_enter(Job *job); * * Pause now if job_pause() has been called. Jobs that perform lots of I/O * must call this between requests so that the job can be paused. + * + * Called with job_mutex *not* held. */ void coroutine_fn job_pause_point(Job *job); @@ -XXX,XX +XXX,XX @@ void coroutine_fn job_pause_point(Job *job); * @job: The job that calls the function. * * Yield the job coroutine. + * Called with job_mutex *not* held. */ void coroutine_fn job_yield(Job *job); @@ -XXX,XX +XXX,XX @@ void coroutine_fn job_yield(Job *job); * Put the job to sleep (assuming that it wasn't canceled) for @ns * %QEMU_CLOCK_REALTIME nanoseconds. Canceling the job will immediately * interrupt the wait. + * + * Called with job_mutex *not* held. */ void coroutine_fn job_sleep_ns(Job *job, int64_t ns); @@ -XXX,XX +XXX,XX @@ const char *job_type_str(const Job *job); /** Returns true if the job should not be visible to the management layer. */ bool job_is_internal(Job *job); -/** Returns whether the job is being cancelled. */ +/** + * Returns whether the job is being cancelled. + * Called with job_mutex *not* held. + */ bool job_is_cancelled(Job *job); +/* Same as job_is_cancelled(), but called with job lock held. */ +bool job_is_cancelled_locked(Job *job); + /** * Returns whether the job is scheduled for cancellation (at an * indefinite point). + * Called with job_mutex *not* held. */ bool job_cancel_requested(Job *job); -/** Returns whether the job is in a completed state. */ +/** + * Returns whether the job is in a completed state. + * Called with job_mutex *not* held. + */ bool job_is_completed(Job *job); -/** Returns whether the job is ready to be completed. */ +/* Same as job_is_completed(), but called with job lock held. */ +bool job_is_completed_locked(Job *job); + +/** + * Returns whether the job is ready to be completed. + * Called with job_mutex *not* held. + */ bool job_is_ready(Job *job); +/* Same as job_is_ready(), but called with job lock held. */ +bool job_is_ready_locked(Job *job); + /** * Request @job to pause at the next pause point. Must be paired with * job_resume(). If the job is supposed to be resumed by user action, call @@ -XXX,XX +XXX,XX @@ bool job_is_ready(Job *job); */ void job_pause(Job *job); +/* Same as job_pause(), but called with job lock held. */ +void job_pause_locked(Job *job); + /** Resumes a @job paused with job_pause. */ void job_resume(Job *job); +/* + * Same as job_resume(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_resume_locked(Job *job); + /** * Asynchronously pause the specified @job. * Do not allow a resume until a matching call to job_user_resume. */ void job_user_pause(Job *job, Error **errp); +/* Same as job_user_pause(), but called with job lock held. */ +void job_user_pause_locked(Job *job, Error **errp); + /** Returns true if the job is user-paused. */ bool job_user_paused(Job *job); +/* Same as job_user_paused(), but called with job lock held. */ +bool job_user_paused_locked(Job *job); + /** * Resume the specified @job. * Must be paired with a preceding job_user_pause. */ void job_user_resume(Job *job, Error **errp); +/* + * Same as job_user_resume(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_user_resume_locked(Job *job, Error **errp); + /** * Get the next element from the list of block jobs after @job, or the * first one if @job is %NULL. @@ -XXX,XX +XXX,XX @@ void job_user_resume(Job *job, Error **errp); */ Job *job_next(Job *job); +/* Same as job_next(), but called with job lock held. */ +Job *job_next_locked(Job *job); + /** * Get the job identified by @id (which must not be %NULL). * @@ -XXX,XX +XXX,XX @@ Job *job_next(Job *job); */ Job *job_get(const char *id); +/* Same as job_get(), but called with job lock held. */ +Job *job_get_locked(const char *id); + /** * Check whether the verb @verb can be applied to @job in its current state. * Returns 0 if the verb can be applied; otherwise errp is set and -EPERM @@ -XXX,XX +XXX,XX @@ Job *job_get(const char *id); */ int job_apply_verb(Job *job, JobVerb verb, Error **errp); -/** The @job could not be started, free it. */ +/* Same as job_apply_verb, but called with job lock held. */ +int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp); + +/** + * The @job could not be started, free it. + * Called with job_mutex *not* held. + */ void job_early_fail(Job *job); -/** Moves the @job from RUNNING to READY */ +/** + * Moves the @job from RUNNING to READY. + * Called with job_mutex *not* held. + */ void job_transition_to_ready(Job *job); /** Asynchronously complete the specified @job. */ void job_complete(Job *job, Error **errp); +/* + * Same as job_complete(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_complete_locked(Job *job, Error **errp); + /** * Asynchronously cancel the specified @job. If @force is true, the job should * be cancelled immediately without waiting for a consistent state. */ void job_cancel(Job *job, bool force); +/* Same as job_cancel(), but called with job lock held. */ +void job_cancel_locked(Job *job, bool force); + /** * Cancels the specified job like job_cancel(), but may refuse to do so if the * operation isn't meaningful in the current state of the job. */ void job_user_cancel(Job *job, bool force, Error **errp); +/* Same as job_user_cancel(), but called with job lock held. */ +void job_user_cancel_locked(Job *job, bool force, Error **errp); + /** * Synchronously cancel the @job. The completion callback is called * before the function returns. If @force is false, the job may @@ -XXX,XX +XXX,XX @@ void job_user_cancel(Job *job, bool force, Error **errp); */ int job_cancel_sync(Job *job, bool force); -/** Synchronously force-cancels all jobs using job_cancel_sync(). */ +/* Same as job_cancel_sync, but called with job lock held. */ +int job_cancel_sync_locked(Job *job, bool force); + +/** + * Synchronously force-cancels all jobs using job_cancel_sync_locked(). + * + * Called with job_lock *not* held. + */ void job_cancel_sync_all(void); /** @@ -XXX,XX +XXX,XX @@ void job_cancel_sync_all(void); */ int job_complete_sync(Job *job, Error **errp); +/* Same as job_complete_sync, but called with job lock held. */ +int job_complete_sync_locked(Job *job, Error **errp); + /** * For a @job that has finished its work and is pending awaiting explicit * acknowledgement to commit its work, this will commit that work. @@ -XXX,XX +XXX,XX @@ int job_complete_sync(Job *job, Error **errp); */ void job_finalize(Job *job, Error **errp); +/* Same as job_finalize(), but called with job lock held. */ +void job_finalize_locked(Job *job, Error **errp); + /** * Remove the concluded @job from the query list and resets the passed pointer * to %NULL. Returns an error if the job is not actually concluded. */ void job_dismiss(Job **job, Error **errp); +/* Same as job_dismiss(), but called with job lock held. */ +void job_dismiss_locked(Job **job, Error **errp); + /** * Synchronously finishes the given @job. If @finish is given, it is called to * trigger completion or cancellation of the job. @@ -XXX,XX +XXX,XX @@ void job_dismiss(Job **job, Error **errp); * * Callers must hold the AioContext lock of job->aio_context. */ -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp); +int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), + Error **errp); + +/* + * Same as job_finish_sync(), but called with job lock held. + * Might release the lock temporarily. + */ +int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), + Error **errp); #endif diff --git a/job.c b/job.c index XXXXXXX..XXXXXXX 100644 --- a/job.c +++ b/job.c @@ -XXX,XX +XXX,XX @@ */ QemuMutex job_mutex; +/* Protected by job_mutex */ static QLIST_HEAD(, Job) jobs = QLIST_HEAD_INITIALIZER(jobs); /* Job State Transition Table */ @@ -XXX,XX +XXX,XX @@ JobTxn *job_txn_new(void) return txn; } -static void job_txn_ref(JobTxn *txn) +/* Called with job_mutex held. */ +static void job_txn_ref_locked(JobTxn *txn) { txn->refcnt++; } -void job_txn_unref(JobTxn *txn) +void job_txn_unref_locked(JobTxn *txn) { if (txn && --txn->refcnt == 0) { g_free(txn); } } +void job_txn_unref(JobTxn *txn) +{ + JOB_LOCK_GUARD(); + job_txn_unref_locked(txn); +} + /** * @txn: The transaction (may be NULL) * @job: Job to add to the transaction @@ -XXX,XX +XXX,XX @@ void job_txn_unref(JobTxn *txn) * the reference that is automatically grabbed here. * * If @txn is NULL, the function does nothing. + * + * Called with job_mutex held. */ -static void job_txn_add_job(JobTxn *txn, Job *job) +static void job_txn_add_job_locked(JobTxn *txn, Job *job) { if (!txn) { return; @@ -XXX,XX +XXX,XX @@ static void job_txn_add_job(JobTxn *txn, Job *job) job->txn = txn; QLIST_INSERT_HEAD(&txn->jobs, job, txn_list); - job_txn_ref(txn); + job_txn_ref_locked(txn); } -static void job_txn_del_job(Job *job) +/* Called with job_mutex held. */ +static void job_txn_del_job_locked(Job *job) { if (job->txn) { QLIST_REMOVE(job, txn_list); - job_txn_unref(job->txn); + job_txn_unref_locked(job->txn); job->txn = NULL; } } -static int job_txn_apply(Job *job, int fn(Job *)) +/* Called with job_mutex held, but releases it temporarily. */ +static int job_txn_apply_locked(Job *job, int fn(Job *)) { AioContext *inner_ctx; Job *other_job, *next; @@ -XXX,XX +XXX,XX @@ static int job_txn_apply(Job *job, int fn(Job *)) * we need to release it here to avoid holding the lock twice - which would * break AIO_WAIT_WHILE from within fn. */ - job_ref(job); + job_ref_locked(job); aio_context_release(job->aio_context); QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) { @@ -XXX,XX +XXX,XX @@ static int job_txn_apply(Job *job, int fn(Job *)) * can't use a local variable to cache it. */ aio_context_acquire(job->aio_context); - job_unref(job); + job_unref_locked(job); return rc; } @@ -XXX,XX +XXX,XX @@ bool job_is_internal(Job *job) return (job->id == NULL); } -static void job_state_transition(Job *job, JobStatus s1) +/* Called with job_mutex held. */ +static void job_state_transition_locked(Job *job, JobStatus s1) { JobStatus s0 = job->status; assert(s1 >= 0 && s1 < JOB_STATUS__MAX); @@ -XXX,XX +XXX,XX @@ static void job_state_transition(Job *job, JobStatus s1) } } -int job_apply_verb(Job *job, JobVerb verb, Error **errp) +int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp) { JobStatus s0 = job->status; assert(verb >= 0 && verb < JOB_VERB__MAX); @@ -XXX,XX +XXX,XX @@ int job_apply_verb(Job *job, JobVerb verb, Error **errp) return -EPERM; } +int job_apply_verb(Job *job, JobVerb verb, Error **errp) +{ + JOB_LOCK_GUARD(); + return job_apply_verb_locked(job, verb, errp); +} + JobType job_type(const Job *job) { return job->driver->job_type; @@ -XXX,XX +XXX,XX @@ const char *job_type_str(const Job *job) return JobType_str(job_type(job)); } -bool job_is_cancelled(Job *job) +bool job_is_cancelled_locked(Job *job) { /* force_cancel may be true only if cancelled is true, too */ assert(job->cancelled || !job->force_cancel); return job->force_cancel; } -bool job_cancel_requested(Job *job) +bool job_is_cancelled(Job *job) +{ + JOB_LOCK_GUARD(); + return job_is_cancelled_locked(job); +} + +/* Called with job_mutex held. */ +static bool job_cancel_requested_locked(Job *job) { return job->cancelled; } -bool job_is_ready(Job *job) +bool job_cancel_requested(Job *job) +{ + JOB_LOCK_GUARD(); + return job_cancel_requested_locked(job); +} + +bool job_is_ready_locked(Job *job) { switch (job->status) { case JOB_STATUS_UNDEFINED: @@ -XXX,XX +XXX,XX @@ bool job_is_ready(Job *job) return false; } -bool job_is_completed(Job *job) +bool job_is_ready(Job *job) +{ + JOB_LOCK_GUARD(); + return job_is_ready_locked(job); +} + +bool job_is_completed_locked(Job *job) { switch (job->status) { case JOB_STATUS_UNDEFINED: @@ -XXX,XX +XXX,XX @@ bool job_is_completed(Job *job) return false; } -static bool job_started(Job *job) +bool job_is_completed(Job *job) +{ + JOB_LOCK_GUARD(); + return job_is_completed_locked(job); +} + +static bool job_started_locked(Job *job) { return job->co; } -static bool job_should_pause(Job *job) +/* Called with job_mutex held. */ +static bool job_should_pause_locked(Job *job) { return job->pause_count > 0; } -Job *job_next(Job *job) +Job *job_next_locked(Job *job) { if (!job) { return QLIST_FIRST(&jobs); @@ -XXX,XX +XXX,XX @@ Job *job_next(Job *job) return QLIST_NEXT(job, job_list); } -Job *job_get(const char *id) +Job *job_next(Job *job) +{ + JOB_LOCK_GUARD(); + return job_next_locked(job); +} + +Job *job_get_locked(const char *id) { Job *job; @@ -XXX,XX +XXX,XX @@ Job *job_get(const char *id) return NULL; } +Job *job_get(const char *id) +{ + JOB_LOCK_GUARD(); + return job_get_locked(id); +} + +/* Called with job_mutex *not* held. */ static void job_sleep_timer_cb(void *opaque) { Job *job = opaque; @@ -XXX,XX +XXX,XX @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, { Job *job; + JOB_LOCK_GUARD(); + if (job_id) { if (flags & JOB_INTERNAL) { error_setg(errp, "Cannot specify job ID for internal job"); @@ -XXX,XX +XXX,XX @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, error_setg(errp, "Invalid job ID '%s'", job_id); return NULL; } - if (job_get(job_id)) { + if (job_get_locked(job_id)) { error_setg(errp, "Job ID '%s' already in use", job_id); return NULL; } @@ -XXX,XX +XXX,XX @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, notifier_list_init(&job->on_ready); notifier_list_init(&job->on_idle); - job_state_transition(job, JOB_STATUS_CREATED); + job_state_transition_locked(job, JOB_STATUS_CREATED); aio_timer_init(qemu_get_aio_context(), &job->sleep_timer, QEMU_CLOCK_REALTIME, SCALE_NS, job_sleep_timer_cb, job); @@ -XXX,XX +XXX,XX @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, * consolidating the job management logic */ if (!txn) { txn = job_txn_new(); - job_txn_add_job(txn, job); - job_txn_unref(txn); + job_txn_add_job_locked(txn, job); + job_txn_unref_locked(txn); } else { - job_txn_add_job(txn, job); + job_txn_add_job_locked(txn, job); } return job; } -void job_ref(Job *job) +void job_ref_locked(Job *job) { ++job->refcnt; } -void job_unref(Job *job) +void job_ref(Job *job) +{ + JOB_LOCK_GUARD(); + job_ref_locked(job); +} + +void job_unref_locked(Job *job) { GLOBAL_STATE_CODE(); @@ -XXX,XX +XXX,XX @@ void job_unref(Job *job) assert(!job->txn); if (job->driver->free) { + job_unlock(); job->driver->free(job); + job_lock(); } QLIST_REMOVE(job, job_list); @@ -XXX,XX +XXX,XX @@ void job_unref(Job *job) } } +void job_unref(Job *job) +{ + JOB_LOCK_GUARD(); + job_unref_locked(job); +} + void job_progress_update(Job *job, uint64_t done) { progress_work_done(&job->progress, done); @@ -XXX,XX +XXX,XX @@ void job_progress_increase_remaining(Job *job, uint64_t delta) /** * To be called when a cancelled job is finalised. + * Called with job_mutex held. */ -static void job_event_cancelled(Job *job) +static void job_event_cancelled_locked(Job *job) { notifier_list_notify(&job->on_finalize_cancelled, job); } /** * To be called when a successfully completed job is finalised. + * Called with job_mutex held. */ -static void job_event_completed(Job *job) +static void job_event_completed_locked(Job *job) { notifier_list_notify(&job->on_finalize_completed, job); } -static void job_event_pending(Job *job) +/* Called with job_mutex held. */ +static void job_event_pending_locked(Job *job) { notifier_list_notify(&job->on_pending, job); } -static void job_event_ready(Job *job) +/* Called with job_mutex held. */ +static void job_event_ready_locked(Job *job) { notifier_list_notify(&job->on_ready, job); } -static void job_event_idle(Job *job) +/* Called with job_mutex held. */ +static void job_event_idle_locked(Job *job) { notifier_list_notify(&job->on_idle, job); } -void job_enter_cond(Job *job, bool(*fn)(Job *job)) +void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)) { - if (!job_started(job)) { + if (!job_started_locked(job)) { return; } if (job->deferred_to_main_loop) { @@ -XXX,XX +XXX,XX @@ void job_enter_cond(Job *job, bool(*fn)(Job *job)) timer_del(&job->sleep_timer); job->busy = true; real_job_unlock(); + job_unlock(); aio_co_enter(job->aio_context, job->co); + job_lock(); +} + +void job_enter_cond(Job *job, bool(*fn)(Job *job)) +{ + JOB_LOCK_GUARD(); + job_enter_cond_locked(job, fn); } void job_enter(Job *job) { - job_enter_cond(job, NULL); + JOB_LOCK_GUARD(); + job_enter_cond_locked(job, NULL); } /* Yield, and schedule a timer to reenter the coroutine after @ns nanoseconds. @@ -XXX,XX +XXX,XX @@ void job_enter(Job *job) * is allowed and cancels the timer. * * If @ns is (uint64_t) -1, no timer is scheduled and job_enter() must be - * called explicitly. */ -static void coroutine_fn job_do_yield(Job *job, uint64_t ns) + * called explicitly. + * + * Called with job_mutex held, but releases it temporarily. + */ +static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns) { real_job_lock(); if (ns != -1) { timer_mod(&job->sleep_timer, ns); } job->busy = false; - job_event_idle(job); + job_event_idle_locked(job); real_job_unlock(); + job_unlock(); qemu_coroutine_yield(); + job_lock(); /* Set by job_enter_cond() before re-entering the coroutine. */ assert(job->busy); } -void coroutine_fn job_pause_point(Job *job) +/* Called with job_mutex held, but releases it temporarily. */ +static void coroutine_fn job_pause_point_locked(Job *job) { - assert(job && job_started(job)); + assert(job && job_started_locked(job)); - if (!job_should_pause(job)) { + if (!job_should_pause_locked(job)) { return; } - if (job_is_cancelled(job)) { + if (job_is_cancelled_locked(job)) { return; } if (job->driver->pause) { + job_unlock(); job->driver->pause(job); + job_lock(); } - if (job_should_pause(job) && !job_is_cancelled(job)) { + if (job_should_pause_locked(job) && !job_is_cancelled_locked(job)) { JobStatus status = job->status; - job_state_transition(job, status == JOB_STATUS_READY - ? JOB_STATUS_STANDBY - : JOB_STATUS_PAUSED); + job_state_transition_locked(job, status == JOB_STATUS_READY + ? JOB_STATUS_STANDBY + : JOB_STATUS_PAUSED); job->paused = true; - job_do_yield(job, -1); + job_do_yield_locked(job, -1); job->paused = false; - job_state_transition(job, status); + job_state_transition_locked(job, status); } if (job->driver->resume) { + job_unlock(); job->driver->resume(job); + job_lock(); } } -void coroutine_fn job_yield(Job *job) +void coroutine_fn job_pause_point(Job *job) +{ + JOB_LOCK_GUARD(); + job_pause_point_locked(job); +} + +static void coroutine_fn job_yield_locked(Job *job) { assert(job->busy); /* Check cancellation *before* setting busy = false, too! */ - if (job_is_cancelled(job)) { + if (job_is_cancelled_locked(job)) { return; } - if (!job_should_pause(job)) { - job_do_yield(job, -1); + if (!job_should_pause_locked(job)) { + job_do_yield_locked(job, -1); } - job_pause_point(job); + job_pause_point_locked(job); +} + +void coroutine_fn job_yield(Job *job) +{ + JOB_LOCK_GUARD(); + job_yield_locked(job); } void coroutine_fn job_sleep_ns(Job *job, int64_t ns) { + JOB_LOCK_GUARD(); assert(job->busy); /* Check cancellation *before* setting busy = false, too! */ - if (job_is_cancelled(job)) { + if (job_is_cancelled_locked(job)) { return; } - if (!job_should_pause(job)) { - job_do_yield(job, qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + ns); + if (!job_should_pause_locked(job)) { + job_do_yield_locked(job, qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + ns); } - job_pause_point(job); + job_pause_point_locked(job); } -/* Assumes the block_job_mutex is held */ -static bool job_timer_not_pending(Job *job) +/* Assumes the job_mutex is held */ +static bool job_timer_not_pending_locked(Job *job) { return !timer_pending(&job->sleep_timer); } -void job_pause(Job *job) +void job_pause_locked(Job *job) { job->pause_count++; if (!job->paused) { - job_enter(job); + job_enter_cond_locked(job, NULL); } } -void job_resume(Job *job) +void job_pause(Job *job) +{ + JOB_LOCK_GUARD(); + job_pause_locked(job); +} + +void job_resume_locked(Job *job) { assert(job->pause_count > 0); job->pause_count--; @@ -XXX,XX +XXX,XX @@ void job_resume(Job *job) } /* kick only if no timer is pending */ - job_enter_cond(job, job_timer_not_pending); + job_enter_cond_locked(job, job_timer_not_pending_locked); } -void job_user_pause(Job *job, Error **errp) +void job_resume(Job *job) { - if (job_apply_verb(job, JOB_VERB_PAUSE, errp)) { + JOB_LOCK_GUARD(); + job_resume_locked(job); +} + +void job_user_pause_locked(Job *job, Error **errp) +{ + if (job_apply_verb_locked(job, JOB_VERB_PAUSE, errp)) { return; } if (job->user_paused) { @@ -XXX,XX +XXX,XX @@ void job_user_pause(Job *job, Error **errp) return; } job->user_paused = true; - job_pause(job); + job_pause_locked(job); } -bool job_user_paused(Job *job) +void job_user_pause(Job *job, Error **errp) +{ + JOB_LOCK_GUARD(); + job_user_pause_locked(job, errp); +} + +bool job_user_paused_locked(Job *job) { return job->user_paused; } -void job_user_resume(Job *job, Error **errp) +bool job_user_paused(Job *job) +{ + JOB_LOCK_GUARD(); + return job_user_paused_locked(job); +} + +void job_user_resume_locked(Job *job, Error **errp) { assert(job); GLOBAL_STATE_CODE(); @@ -XXX,XX +XXX,XX @@ void job_user_resume(Job *job, Error **errp) error_setg(errp, "Can't resume a job that was not paused"); return; } - if (job_apply_verb(job, JOB_VERB_RESUME, errp)) { + if (job_apply_verb_locked(job, JOB_VERB_RESUME, errp)) { return; } if (job->driver->user_resume) { + job_unlock(); job->driver->user_resume(job); + job_lock(); } job->user_paused = false; - job_resume(job); + job_resume_locked(job); } -static void job_do_dismiss(Job *job) +void job_user_resume(Job *job, Error **errp) +{ + JOB_LOCK_GUARD(); + job_user_resume_locked(job, errp); +} + +/* Called with job_mutex held, but releases it temporarily. */ +static void job_do_dismiss_locked(Job *job) { assert(job); job->busy = false; job->paused = false; job->deferred_to_main_loop = true; - job_txn_del_job(job); + job_txn_del_job_locked(job); - job_state_transition(job, JOB_STATUS_NULL); - job_unref(job); + job_state_transition_locked(job, JOB_STATUS_NULL); + job_unref_locked(job); } -void job_dismiss(Job **jobptr, Error **errp) +void job_dismiss_locked(Job **jobptr, Error **errp) { Job *job = *jobptr; /* similarly to _complete, this is QMP-interface only. */ assert(job->id); - if (job_apply_verb(job, JOB_VERB_DISMISS, errp)) { + if (job_apply_verb_locked(job, JOB_VERB_DISMISS, errp)) { return; } - job_do_dismiss(job); + job_do_dismiss_locked(job); *jobptr = NULL; } +void job_dismiss(Job **jobptr, Error **errp) +{ + JOB_LOCK_GUARD(); + job_dismiss_locked(jobptr, errp); +} + void job_early_fail(Job *job) { + JOB_LOCK_GUARD(); assert(job->status == JOB_STATUS_CREATED); - job_do_dismiss(job); + job_do_dismiss_locked(job); } -static void job_conclude(Job *job) +/* Called with job_mutex held. */ +static void job_conclude_locked(Job *job) { - job_state_transition(job, JOB_STATUS_CONCLUDED); - if (job->auto_dismiss || !job_started(job)) { - job_do_dismiss(job); + job_state_transition_locked(job, JOB_STATUS_CONCLUDED); + if (job->auto_dismiss || !job_started_locked(job)) { + job_do_dismiss_locked(job); } } -static void job_update_rc(Job *job) +/* Called with job_mutex held. */ +static void job_update_rc_locked(Job *job) { - if (!job->ret && job_is_cancelled(job)) { + if (!job->ret && job_is_cancelled_locked(job)) { job->ret = -ECANCELED; } if (job->ret) { if (!job->err) { error_setg(&job->err, "%s", strerror(-job->ret)); } - job_state_transition(job, JOB_STATUS_ABORTING); + job_state_transition_locked(job, JOB_STATUS_ABORTING); } } @@ -XXX,XX +XXX,XX @@ static void job_clean(Job *job) } } -static int job_finalize_single(Job *job) +/* Called with job_mutex held, but releases it temporarily */ +static int job_finalize_single_locked(Job *job) { - assert(job_is_completed(job)); + int job_ret; + + assert(job_is_completed_locked(job)); /* Ensure abort is called for late-transactional failures */ - job_update_rc(job); + job_update_rc_locked(job); + + job_ret = job->ret; + job_unlock(); - if (!job->ret) { + if (!job_ret) { job_commit(job); } else { job_abort(job); } job_clean(job); + job_lock(); + if (job->cb) { - job->cb(job->opaque, job->ret); + job_ret = job->ret; + job_unlock(); + job->cb(job->opaque, job_ret); + job_lock(); } /* Emit events only if we actually started */ - if (job_started(job)) { - if (job_is_cancelled(job)) { - job_event_cancelled(job); + if (job_started_locked(job)) { + if (job_is_cancelled_locked(job)) { + job_event_cancelled_locked(job); } else { - job_event_completed(job); + job_event_completed_locked(job); } } - job_txn_del_job(job); - job_conclude(job); + job_txn_del_job_locked(job); + job_conclude_locked(job); return 0; } -static void job_cancel_async(Job *job, bool force) +/* Called with job_mutex held, but releases it temporarily */ +static void job_cancel_async_locked(Job *job, bool force) { GLOBAL_STATE_CODE(); if (job->driver->cancel) { + job_unlock(); force = job->driver->cancel(job, force); + job_lock(); } else { /* No .cancel() means the job will behave as if force-cancelled */ force = true; @@ -XXX,XX +XXX,XX @@ static void job_cancel_async(Job *job, bool force) if (job->user_paused) { /* Do not call job_enter here, the caller will handle it. */ if (job->driver->user_resume) { + job_unlock(); job->driver->user_resume(job); + job_lock(); } job->user_paused = false; assert(job->pause_count > 0); @@ -XXX,XX +XXX,XX @@ static void job_cancel_async(Job *job, bool force) } } -static void job_completed_txn_abort(Job *job) +/* Called with job_mutex held, but releases it temporarily. */ +static void job_completed_txn_abort_locked(Job *job) { AioContext *ctx; JobTxn *txn = job->txn; @@ -XXX,XX +XXX,XX @@ static void job_completed_txn_abort(Job *job) return; } txn->aborting = true; - job_txn_ref(txn); + job_txn_ref_locked(txn); /* * We can only hold the single job's AioContext lock while calling @@ -XXX,XX +XXX,XX @@ static void job_completed_txn_abort(Job *job) * calls of AIO_WAIT_WHILE(), which could deadlock otherwise. * Note that the job's AioContext may change when it is finalized. */ - job_ref(job); + job_ref_locked(job); aio_context_release(job->aio_context); /* Other jobs are effectively cancelled by us, set the status for @@ -XXX,XX +XXX,XX @@ static void job_completed_txn_abort(Job *job) * Therefore, pass force=true to terminate all other jobs as quickly * as possible. */ - job_cancel_async(other_job, true); + job_cancel_async_locked(other_job, true); aio_context_release(ctx); } } @@ -XXX,XX +XXX,XX @@ static void job_completed_txn_abort(Job *job) */ ctx = other_job->aio_context; aio_context_acquire(ctx); - if (!job_is_completed(other_job)) { - assert(job_cancel_requested(other_job)); - job_finish_sync(other_job, NULL, NULL); + if (!job_is_completed_locked(other_job)) { + assert(job_cancel_requested_locked(other_job)); + job_finish_sync_locked(other_job, NULL, NULL); } - job_finalize_single(other_job); + job_finalize_single_locked(other_job); aio_context_release(ctx); } @@ -XXX,XX +XXX,XX @@ static void job_completed_txn_abort(Job *job) * even if the job went away during job_finalize_single(). */ aio_context_acquire(job->aio_context); - job_unref(job); + job_unref_locked(job); - job_txn_unref(txn); + job_txn_unref_locked(txn); } -static int job_prepare(Job *job) +/* Called with job_mutex held, but releases it temporarily */ +static int job_prepare_locked(Job *job) { + int ret; + GLOBAL_STATE_CODE(); if (job->ret == 0 && job->driver->prepare) { - job->ret = job->driver->prepare(job); - job_update_rc(job); + job_unlock(); + ret = job->driver->prepare(job); + job_lock(); + job->ret = ret; + job_update_rc_locked(job); } return job->ret; } -static int job_needs_finalize(Job *job) +/* Called with job_mutex held */ +static int job_needs_finalize_locked(Job *job) { return !job->auto_finalize; } -static void job_do_finalize(Job *job) +/* Called with job_mutex held */ +static void job_do_finalize_locked(Job *job) { int rc; assert(job && job->txn); /* prepare the transaction to complete */ - rc = job_txn_apply(job, job_prepare); + rc = job_txn_apply_locked(job, job_prepare_locked); if (rc) { - job_completed_txn_abort(job); + job_completed_txn_abort_locked(job); } else { - job_txn_apply(job, job_finalize_single); + job_txn_apply_locked(job, job_finalize_single_locked); } } -void job_finalize(Job *job, Error **errp) +void job_finalize_locked(Job *job, Error **errp) { assert(job && job->id); - if (job_apply_verb(job, JOB_VERB_FINALIZE, errp)) { + if (job_apply_verb_locked(job, JOB_VERB_FINALIZE, errp)) { return; } - job_do_finalize(job); + job_do_finalize_locked(job); } -static int job_transition_to_pending(Job *job) +void job_finalize(Job *job, Error **errp) { - job_state_transition(job, JOB_STATUS_PENDING); + JOB_LOCK_GUARD(); + job_finalize_locked(job, errp); +} + +/* Called with job_mutex held. */ +static int job_transition_to_pending_locked(Job *job) +{ + job_state_transition_locked(job, JOB_STATUS_PENDING); if (!job->auto_finalize) { - job_event_pending(job); + job_event_pending_locked(job); } return 0; } void job_transition_to_ready(Job *job) { - job_state_transition(job, JOB_STATUS_READY); - job_event_ready(job); + JOB_LOCK_GUARD(); + job_state_transition_locked(job, JOB_STATUS_READY); + job_event_ready_locked(job); } -static void job_completed_txn_success(Job *job) +/* Called with job_mutex held. */ +static void job_completed_txn_success_locked(Job *job) { JobTxn *txn = job->txn; Job *other_job; - job_state_transition(job, JOB_STATUS_WAITING); + job_state_transition_locked(job, JOB_STATUS_WAITING); /* * Successful completion, see if there are other running jobs in this * txn. */ QLIST_FOREACH(other_job, &txn->jobs, txn_list) { - if (!job_is_completed(other_job)) { + if (!job_is_completed_locked(other_job)) { return; } assert(other_job->ret == 0); } - job_txn_apply(job, job_transition_to_pending); + job_txn_apply_locked(job, job_transition_to_pending_locked); /* If no jobs need manual finalization, automatically do so */ - if (job_txn_apply(job, job_needs_finalize) == 0) { - job_do_finalize(job); + if (job_txn_apply_locked(job, job_needs_finalize_locked) == 0) { + job_do_finalize_locked(job); } } -static void job_completed(Job *job) +/* Called with job_mutex held. */ +static void job_completed_locked(Job *job) { - assert(job && job->txn && !job_is_completed(job)); + assert(job && job->txn && !job_is_completed_locked(job)); - job_update_rc(job); + job_update_rc_locked(job); trace_job_completed(job, job->ret); if (job->ret) { - job_completed_txn_abort(job); + job_completed_txn_abort_locked(job); } else { - job_completed_txn_success(job); + job_completed_txn_success_locked(job); } } -/** Useful only as a type shim for aio_bh_schedule_oneshot. */ +/** + * Useful only as a type shim for aio_bh_schedule_oneshot. + * Called with job_mutex *not* held. + */ static void job_exit(void *opaque) { Job *job = (Job *)opaque; AioContext *ctx; + JOB_LOCK_GUARD(); - job_ref(job); + job_ref_locked(job); aio_context_acquire(job->aio_context); /* This is a lie, we're not quiescent, but still doing the completion @@ -XXX,XX +XXX,XX @@ static void job_exit(void *opaque) * drain block nodes, and if .drained_poll still returned true, we would * deadlock. */ job->busy = false; - job_event_idle(job); + job_event_idle_locked(job); - job_completed(job); + job_completed_locked(job); /* * Note that calling job_completed can move the job to a different @@ -XXX,XX +XXX,XX @@ static void job_exit(void *opaque) * the job underneath us. */ ctx = job->aio_context; - job_unref(job); + job_unref_locked(job); aio_context_release(ctx); } @@ -XXX,XX +XXX,XX @@ static void job_exit(void *opaque) static void coroutine_fn job_co_entry(void *opaque) { Job *job = opaque; + int ret; assert(job && job->driver && job->driver->run); - assert(job->aio_context == qemu_get_current_aio_context()); - job_pause_point(job); - job->ret = job->driver->run(job, &job->err); - job->deferred_to_main_loop = true; - job->busy = true; + WITH_JOB_LOCK_GUARD() { + assert(job->aio_context == qemu_get_current_aio_context()); + job_pause_point_locked(job); + } + ret = job->driver->run(job, &job->err); + WITH_JOB_LOCK_GUARD() { + job->ret = ret; + job->deferred_to_main_loop = true; + job->busy = true; + } aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job); } void job_start(Job *job) { - assert(job && !job_started(job) && job->paused && - job->driver && job->driver->run); - job->co = qemu_coroutine_create(job_co_entry, job); - job->pause_count--; - job->busy = true; - job->paused = false; - job_state_transition(job, JOB_STATUS_RUNNING); + assert(qemu_in_main_thread()); + + WITH_JOB_LOCK_GUARD() { + assert(job && !job_started_locked(job) && job->paused && + job->driver && job->driver->run); + job->co = qemu_coroutine_create(job_co_entry, job); + job->pause_count--; + job->busy = true; + job->paused = false; + job_state_transition_locked(job, JOB_STATUS_RUNNING); + } aio_co_enter(job->aio_context, job->co); } -void job_cancel(Job *job, bool force) +void job_cancel_locked(Job *job, bool force) { if (job->status == JOB_STATUS_CONCLUDED) { - job_do_dismiss(job); + job_do_dismiss_locked(job); return; } - job_cancel_async(job, force); - if (!job_started(job)) { - job_completed(job); + job_cancel_async_locked(job, force); + if (!job_started_locked(job)) { + job_completed_locked(job); } else if (job->deferred_to_main_loop) { /* * job_cancel_async() ignores soft-cancel requests for jobs @@ -XXX,XX +XXX,XX @@ void job_cancel(Job *job, bool force) * choose to call job_is_cancelled() to show that we invoke * job_completed_txn_abort() only for force-cancelled jobs.) */ - if (job_is_cancelled(job)) { - job_completed_txn_abort(job); + if (job_is_cancelled_locked(job)) { + job_completed_txn_abort_locked(job); } } else { - job_enter(job); + job_enter_cond_locked(job, NULL); } } -void job_user_cancel(Job *job, bool force, Error **errp) +void job_cancel(Job *job, bool force) { - if (job_apply_verb(job, JOB_VERB_CANCEL, errp)) { + JOB_LOCK_GUARD(); + job_cancel_locked(job, force); +} + +void job_user_cancel_locked(Job *job, bool force, Error **errp) +{ + if (job_apply_verb_locked(job, JOB_VERB_CANCEL, errp)) { return; } - job_cancel(job, force); + job_cancel_locked(job, force); +} + +void job_user_cancel(Job *job, bool force, Error **errp) +{ + JOB_LOCK_GUARD(); + job_user_cancel_locked(job, force, errp); } /* A wrapper around job_cancel() taking an Error ** parameter so it may be * used with job_finish_sync() without the need for (rather nasty) function - * pointer casts there. */ -static void job_cancel_err(Job *job, Error **errp) + * pointer casts there. + * + * Called with job_mutex held. + */ +static void job_cancel_err_locked(Job *job, Error **errp) { - job_cancel(job, false); + job_cancel_locked(job, false); } /** * Same as job_cancel_err(), but force-cancel. + * Called with job_mutex held. */ -static void job_force_cancel_err(Job *job, Error **errp) +static void job_force_cancel_err_locked(Job *job, Error **errp) { - job_cancel(job, true); + job_cancel_locked(job, true); } -int job_cancel_sync(Job *job, bool force) +int job_cancel_sync_locked(Job *job, bool force) { if (force) { - return job_finish_sync(job, &job_force_cancel_err, NULL); + return job_finish_sync_locked(job, &job_force_cancel_err_locked, NULL); } else { - return job_finish_sync(job, &job_cancel_err, NULL); + return job_finish_sync_locked(job, &job_cancel_err_locked, NULL); } } +int job_cancel_sync(Job *job, bool force) +{ + JOB_LOCK_GUARD(); + return job_cancel_sync_locked(job, force); +} + void job_cancel_sync_all(void) { Job *job; AioContext *aio_context; + JOB_LOCK_GUARD(); - while ((job = job_next(NULL))) { + while ((job = job_next_locked(NULL))) { aio_context = job->aio_context; aio_context_acquire(aio_context); - job_cancel_sync(job, true); + job_cancel_sync_locked(job, true); aio_context_release(aio_context); } } +int job_complete_sync_locked(Job *job, Error **errp) +{ + return job_finish_sync_locked(job, job_complete_locked, errp); +} + int job_complete_sync(Job *job, Error **errp) { - return job_finish_sync(job, job_complete, errp); + JOB_LOCK_GUARD(); + return job_complete_sync_locked(job, errp); } -void job_complete(Job *job, Error **errp) +void job_complete_locked(Job *job, Error **errp) { /* Should not be reachable via external interface for internal jobs */ assert(job->id); GLOBAL_STATE_CODE(); - if (job_apply_verb(job, JOB_VERB_COMPLETE, errp)) { + if (job_apply_verb_locked(job, JOB_VERB_COMPLETE, errp)) { return; } - if (job_cancel_requested(job) || !job->driver->complete) { + if (job_cancel_requested_locked(job) || !job->driver->complete) { error_setg(errp, "The active block job '%s' cannot be completed", job->id); return; } + job_unlock(); job->driver->complete(job, errp); + job_lock(); } -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp) +void job_complete(Job *job, Error **errp) +{ + JOB_LOCK_GUARD(); + job_complete_locked(job, errp); +} + +int job_finish_sync_locked(Job *job, + void (*finish)(Job *, Error **errp), + Error **errp) { Error *local_err = NULL; int ret; - job_ref(job); + job_ref_locked(job); if (finish) { finish(job, &local_err); } if (local_err) { error_propagate(errp, local_err); - job_unref(job); + job_unref_locked(job); return -EBUSY; } + job_unlock(); AIO_WAIT_WHILE(job->aio_context, (job_enter(job), !job_is_completed(job))); + job_lock(); - ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret; - job_unref(job); + ret = (job_is_cancelled_locked(job) && job->ret == 0) + ? -ECANCELED : job->ret; + job_unref_locked(job); return ret; } + +int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp) +{ + JOB_LOCK_GUARD(); + return job_finish_sync_locked(job, finish, errp); +} -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> This comment applies more on job, it was left in blockjob as in the past the whole job logic was implemented there. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20220926093214.506243-7-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockjob.c | 20 -------------------- job.c | 16 ++++++++++++++++ 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ #include "qemu/main-loop.h" #include "qemu/timer.h" -/* - * The block job API is composed of two categories of functions. - * - * The first includes functions used by the monitor. The monitor is - * peculiar in that it accesses the block job list with block_job_get, and - * therefore needs consistency across block_job_get and the actual operation - * (e.g. block_job_set_speed). The consistency is achieved with - * aio_context_acquire/release. These functions are declared in blockjob.h. - * - * The second includes functions used by the block job drivers and sometimes - * by the core block layer. These do not care about locking, because the - * whole coroutine runs under the AioContext lock, and are declared in - * blockjob_int.h. - */ - static bool is_block_job(Job *job) { return job_type(job) == JOB_TYPE_BACKUP || @@ -XXX,XX +XXX,XX @@ static void block_job_event_ready(Notifier *n, void *opaque) } -/* - * API for block job drivers and the block layer. These functions are - * declared in blockjob_int.h. - */ - void *block_job_create(const char *job_id, const BlockJobDriver *driver, JobTxn *txn, BlockDriverState *bs, uint64_t perm, uint64_t shared_perm, int64_t speed, int flags, diff --git a/job.c b/job.c index XXXXXXX..XXXXXXX 100644 --- a/job.c +++ b/job.c @@ -XXX,XX +XXX,XX @@ #include "trace/trace-root.h" #include "qapi/qapi-events-job.h" +/* + * The job API is composed of two categories of functions. + * + * The first includes functions used by the monitor. The monitor is + * peculiar in that it accesses the job list with job_get, and + * therefore needs consistency across job_get and the actual operation + * (e.g. job_user_cancel). To achieve this consistency, the caller + * calls job_lock/job_unlock itself around the whole operation. + * + * + * The second includes functions used by the job drivers and sometimes + * by the core block layer. These delegate the locking to the callee instead. + * + * TODO Actually make this true + */ + /* * job_mutex protects the jobs list, but also makes the * struct job fields thread-safe. -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Just as done with job.h, create _locked() functions in blockjob.h These functions will be later useful when caller has already taken the lock. All blockjob _locked functions call job _locked functions. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20220926093214.506243-8-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/blockjob.h | 18 ++++++++++++++ blockjob.c | 52 ++++++++++++++++++++++++++++++++-------- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/include/block/blockjob.h b/include/block/blockjob.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -XXX,XX +XXX,XX @@ typedef struct BlockJob { */ BlockJob *block_job_next(BlockJob *job); +/* Same as block_job_next(), but called with job lock held. */ +BlockJob *block_job_next_locked(BlockJob *job); + /** * block_job_get: * @id: The id of the block job. @@ -XXX,XX +XXX,XX @@ BlockJob *block_job_next(BlockJob *job); */ BlockJob *block_job_get(const char *id); +/* Same as block_job_get(), but called with job lock held. */ +BlockJob *block_job_get_locked(const char *id); + /** * block_job_add_bdrv: * @job: A block job @@ -XXX,XX +XXX,XX @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs); */ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); +/* + * Same as block_job_set_speed(), but called with job lock held. + * Might release the lock temporarily. + */ +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp); + /** * block_job_query: * @job: The job to get information about. @@ -XXX,XX +XXX,XX @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); */ BlockJobInfo *block_job_query(BlockJob *job, Error **errp); +/* Same as block_job_query(), but called with job lock held. */ +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp); + /** * block_job_iostatus_reset: * @job: The job whose I/O status should be reset. @@ -XXX,XX +XXX,XX @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp); */ void block_job_iostatus_reset(BlockJob *job); +/* Same as block_job_iostatus_reset(), but called with job lock held. */ +void block_job_iostatus_reset_locked(BlockJob *job); + /* * block_job_get_aio_context: * diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static bool is_block_job(Job *job) job_type(job) == JOB_TYPE_STREAM; } -BlockJob *block_job_next(BlockJob *bjob) +BlockJob *block_job_next_locked(BlockJob *bjob) { Job *job = bjob ? &bjob->job : NULL; GLOBAL_STATE_CODE(); do { - job = job_next(job); + job = job_next_locked(job); } while (job && !is_block_job(job)); return job ? container_of(job, BlockJob, job) : NULL; } -BlockJob *block_job_get(const char *id) +BlockJob *block_job_next(BlockJob *bjob) { - Job *job = job_get(id); + JOB_LOCK_GUARD(); + return block_job_next_locked(bjob); +} + +BlockJob *block_job_get_locked(const char *id) +{ + Job *job = job_get_locked(id); GLOBAL_STATE_CODE(); if (job && is_block_job(job)) { @@ -XXX,XX +XXX,XX @@ BlockJob *block_job_get(const char *id) } } +BlockJob *block_job_get(const char *id) +{ + JOB_LOCK_GUARD(); + return block_job_get_locked(id); +} + void block_job_free(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); @@ -XXX,XX +XXX,XX @@ static bool job_timer_pending(Job *job) return timer_pending(&job->sleep_timer); } -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp) { const BlockJobDriver *drv = block_job_driver(job); int64_t old_speed = job->speed; GLOBAL_STATE_CODE(); - if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { + if (job_apply_verb_locked(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { return false; } if (speed < 0) { @@ -XXX,XX +XXX,XX @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) job->speed = speed; if (drv->set_speed) { + job_unlock(); drv->set_speed(job, speed); + job_lock(); } if (speed && speed <= old_speed) { @@ -XXX,XX +XXX,XX @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) } /* kick only if a timer is pending */ - job_enter_cond(&job->job, job_timer_pending); + job_enter_cond_locked(&job->job, job_timer_pending); return true; } +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) +{ + JOB_LOCK_GUARD(); + return block_job_set_speed_locked(job, speed, errp); +} + int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) { IO_CODE(); return ratelimit_calculate_delay(&job->limit, n); } -BlockJobInfo *block_job_query(BlockJob *job, Error **errp) +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) { BlockJobInfo *info; uint64_t progress_current, progress_total; @@ -XXX,XX +XXX,XX @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) info->len = progress_total; info->speed = job->speed; info->io_status = job->iostatus; - info->ready = job_is_ready(&job->job), + info->ready = job_is_ready_locked(&job->job), info->status = job->job.status; info->auto_finalize = job->job.auto_finalize; info->auto_dismiss = job->job.auto_dismiss; @@ -XXX,XX +XXX,XX @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) return info; } +BlockJobInfo *block_job_query(BlockJob *job, Error **errp) +{ + JOB_LOCK_GUARD(); + return block_job_query_locked(job, errp); +} + static void block_job_iostatus_set_err(BlockJob *job, int error) { if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { @@ -XXX,XX +XXX,XX @@ fail: return NULL; } -void block_job_iostatus_reset(BlockJob *job) +void block_job_iostatus_reset_locked(BlockJob *job) { GLOBAL_STATE_CODE(); if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { @@ -XXX,XX +XXX,XX @@ void block_job_iostatus_reset(BlockJob *job) job->iostatus = BLOCK_DEVICE_IO_STATUS_OK; } +void block_job_iostatus_reset(BlockJob *job) +{ + JOB_LOCK_GUARD(); + block_job_iostatus_reset_locked(job); +} + void block_job_user_resume(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Both blockdev.c and job-qmp.c have TOC/TOU conditions, because they first search for the job and then perform an action on it. Therefore, we need to do the search + action under the same job mutex critical section. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20220926093214.506243-9-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 67 +++++++++++++++++++++++++++++++++++++----------------- job-qmp.c | 57 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 86 insertions(+), 38 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ out: aio_context_release(aio_context); } -/* Get a block job using its ID and acquire its AioContext */ -static BlockJob *find_block_job(const char *id, AioContext **aio_context, - Error **errp) +/* + * Get a block job using its ID and acquire its AioContext. + * Called with job_mutex held. + */ +static BlockJob *find_block_job_locked(const char *id, + AioContext **aio_context, + Error **errp) { BlockJob *job; @@ -XXX,XX +XXX,XX @@ static BlockJob *find_block_job(const char *id, AioContext **aio_context, *aio_context = NULL; - job = block_job_get(id); + job = block_job_get_locked(id); if (!job) { error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, @@ -XXX,XX +XXX,XX @@ static BlockJob *find_block_job(const char *id, AioContext **aio_context, void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; } - block_job_set_speed(job, speed, errp); + block_job_set_speed_locked(job, speed, errp); aio_context_release(aio_context); } @@ -XXX,XX +XXX,XX @@ void qmp_block_job_cancel(const char *device, bool has_force, bool force, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_block_job_cancel(const char *device, force = false; } - if (job_user_paused(&job->job) && !force) { + if (job_user_paused_locked(&job->job) && !force) { error_setg(errp, "The block job for device '%s' is currently paused", device); goto out; } trace_qmp_block_job_cancel(job); - job_user_cancel(&job->job, force, errp); + job_user_cancel_locked(&job->job, force, errp); out: aio_context_release(aio_context); } @@ -XXX,XX +XXX,XX @@ out: void qmp_block_job_pause(const char *device, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; } trace_qmp_block_job_pause(job); - job_user_pause(&job->job, errp); + job_user_pause_locked(&job->job, errp); aio_context_release(aio_context); } void qmp_block_job_resume(const char *device, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; } trace_qmp_block_job_resume(job); - job_user_resume(&job->job, errp); + job_user_resume_locked(&job->job, errp); aio_context_release(aio_context); } void qmp_block_job_complete(const char *device, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; } trace_qmp_block_job_complete(job); - job_complete(&job->job, errp); + job_complete_locked(&job->job, errp); aio_context_release(aio_context); } void qmp_block_job_finalize(const char *id, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(id, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_block_job_finalize(job); - job_ref(&job->job); - job_finalize(&job->job, errp); + job_ref_locked(&job->job); + job_finalize_locked(&job->job, errp); /* * Job's context might have changed via job_finalize (and job_txn_apply @@ -XXX,XX +XXX,XX @@ void qmp_block_job_finalize(const char *id, Error **errp) * one. */ aio_context = block_job_get_aio_context(job); - job_unref(&job->job); + job_unref_locked(&job->job); aio_context_release(aio_context); } void qmp_block_job_dismiss(const char *id, Error **errp) { AioContext *aio_context; - BlockJob *bjob = find_block_job(id, &aio_context, errp); + BlockJob *bjob; Job *job; + JOB_LOCK_GUARD(); + bjob = find_block_job_locked(id, &aio_context, errp); + if (!bjob) { return; } trace_qmp_block_job_dismiss(bjob); job = &bjob->job; - job_dismiss(&job, errp); + job_dismiss_locked(&job, errp); aio_context_release(aio_context); } diff --git a/job-qmp.c b/job-qmp.c index XXXXXXX..XXXXXXX 100644 --- a/job-qmp.c +++ b/job-qmp.c @@ -XXX,XX +XXX,XX @@ #include "qapi/error.h" #include "trace/trace-root.h" -/* Get a job using its ID and acquire its AioContext */ -static Job *find_job(const char *id, AioContext **aio_context, Error **errp) +/* + * Get a job using its ID and acquire its AioContext. + * Called with job_mutex held. + */ +static Job *find_job_locked(const char *id, + AioContext **aio_context, + Error **errp) { Job *job; *aio_context = NULL; - job = job_get(id); + job = job_get_locked(id); if (!job) { error_setg(errp, "Job not found"); return NULL; @@ -XXX,XX +XXX,XX @@ static Job *find_job(const char *id, AioContext **aio_context, Error **errp) void qmp_job_cancel(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_cancel(job); - job_user_cancel(job, true, errp); + job_user_cancel_locked(job, true, errp); aio_context_release(aio_context); } void qmp_job_pause(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_pause(job); - job_user_pause(job, errp); + job_user_pause_locked(job, errp); aio_context_release(aio_context); } void qmp_job_resume(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_resume(job); - job_user_resume(job, errp); + job_user_resume_locked(job, errp); aio_context_release(aio_context); } void qmp_job_complete(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_complete(job); - job_complete(job, errp); + job_complete_locked(job, errp); aio_context_release(aio_context); } void qmp_job_finalize(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_finalize(job); - job_ref(job); - job_finalize(job, errp); + job_ref_locked(job); + job_finalize_locked(job, errp); /* * Job's context might have changed via job_finalize (and job_txn_apply @@ -XXX,XX +XXX,XX @@ void qmp_job_finalize(const char *id, Error **errp) * one. */ aio_context = job->aio_context; - job_unref(job); + job_unref_locked(job); aio_context_release(aio_context); } void qmp_job_dismiss(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_dismiss(job); - job_dismiss(&job, errp); + job_dismiss_locked(&job, errp); aio_context_release(aio_context); } -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Add missing job synchronization in the unit tests, with explicit locks. We are deliberately using _locked functions wrapped by a guard instead of a normal call because the normal call will be removed in future, as the only usage is limited to the tests. In other words, if a function like job_pause() is/will be only used in tests to avoid: WITH_JOB_LOCK_GUARD(){ job_pause_locked(); } then it is not worth keeping job_pause(), and just use the guard. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20220926093214.506243-10-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/unit/test-bdrv-drain.c | 76 ++++++++++++-------- tests/unit/test-block-iothread.c | 8 ++- tests/unit/test-blockjob-txn.c | 24 ++++--- tests/unit/test-blockjob.c | 115 +++++++++++++++++++------------ 4 files changed, 140 insertions(+), 83 deletions(-) diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void test_blockjob_common_drain_node(enum drain_type drain_type, } } - g_assert_cmpint(job->job.pause_count, ==, 0); - g_assert_false(job->job.paused); - g_assert_true(tjob->running); - g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + WITH_JOB_LOCK_GUARD() { + g_assert_cmpint(job->job.pause_count, ==, 0); + g_assert_false(job->job.paused); + g_assert_true(tjob->running); + g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + } do_drain_begin_unlocked(drain_type, drain_bs); - if (drain_type == BDRV_DRAIN_ALL) { - /* bdrv_drain_all() drains both src and target */ - g_assert_cmpint(job->job.pause_count, ==, 2); - } else { - g_assert_cmpint(job->job.pause_count, ==, 1); + WITH_JOB_LOCK_GUARD() { + if (drain_type == BDRV_DRAIN_ALL) { + /* bdrv_drain_all() drains both src and target */ + g_assert_cmpint(job->job.pause_count, ==, 2); + } else { + g_assert_cmpint(job->job.pause_count, ==, 1); + } + g_assert_true(job->job.paused); + g_assert_false(job->job.busy); /* The job is paused */ } - g_assert_true(job->job.paused); - g_assert_false(job->job.busy); /* The job is paused */ do_drain_end_unlocked(drain_type, drain_bs); if (use_iothread) { - /* paused is reset in the I/O thread, wait for it */ + /* + * Here we are waiting for the paused status to change, + * so don't bother protecting the read every time. + * + * paused is reset in the I/O thread, wait for it + */ while (job->job.paused) { aio_poll(qemu_get_aio_context(), false); } } - g_assert_cmpint(job->job.pause_count, ==, 0); - g_assert_false(job->job.paused); - g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + WITH_JOB_LOCK_GUARD() { + g_assert_cmpint(job->job.pause_count, ==, 0); + g_assert_false(job->job.paused); + g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + } do_drain_begin_unlocked(drain_type, target); - if (drain_type == BDRV_DRAIN_ALL) { - /* bdrv_drain_all() drains both src and target */ - g_assert_cmpint(job->job.pause_count, ==, 2); - } else { - g_assert_cmpint(job->job.pause_count, ==, 1); + WITH_JOB_LOCK_GUARD() { + if (drain_type == BDRV_DRAIN_ALL) { + /* bdrv_drain_all() drains both src and target */ + g_assert_cmpint(job->job.pause_count, ==, 2); + } else { + g_assert_cmpint(job->job.pause_count, ==, 1); + } + g_assert_true(job->job.paused); + g_assert_false(job->job.busy); /* The job is paused */ } - g_assert_true(job->job.paused); - g_assert_false(job->job.busy); /* The job is paused */ do_drain_end_unlocked(drain_type, target); if (use_iothread) { - /* paused is reset in the I/O thread, wait for it */ + /* + * Here we are waiting for the paused status to change, + * so don't bother protecting the read every time. + * + * paused is reset in the I/O thread, wait for it + */ while (job->job.paused) { aio_poll(qemu_get_aio_context(), false); } } - g_assert_cmpint(job->job.pause_count, ==, 0); - g_assert_false(job->job.paused); - g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + WITH_JOB_LOCK_GUARD() { + g_assert_cmpint(job->job.pause_count, ==, 0); + g_assert_false(job->job.paused); + g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + } aio_context_acquire(ctx); - ret = job_complete_sync(&job->job, &error_abort); + WITH_JOB_LOCK_GUARD() { + ret = job_complete_sync_locked(&job->job, &error_abort); + } g_assert_cmpint(ret, ==, (result == TEST_JOB_SUCCESS ? 0 : -EIO)); if (use_iothread) { diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-block-iothread.c +++ b/tests/unit/test-block-iothread.c @@ -XXX,XX +XXX,XX @@ static void test_attach_blockjob(void) } aio_context_acquire(ctx); - job_complete_sync(&tjob->common.job, &error_abort); + WITH_JOB_LOCK_GUARD() { + job_complete_sync_locked(&tjob->common.job, &error_abort); + } blk_set_aio_context(blk, qemu_get_aio_context(), &error_abort); aio_context_release(ctx); @@ -XXX,XX +XXX,XX @@ static void test_propagate_mirror(void) BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT, false, "filter_node", MIRROR_COPY_MODE_BACKGROUND, &error_abort); - job = job_get("job0"); + WITH_JOB_LOCK_GUARD() { + job = job_get_locked("job0"); + } filter = bdrv_find_node("filter_node"); /* Change the AioContext of src */ diff --git a/tests/unit/test-blockjob-txn.c b/tests/unit/test-blockjob-txn.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-blockjob-txn.c +++ b/tests/unit/test-blockjob-txn.c @@ -XXX,XX +XXX,XX @@ static void test_single_job(int expected) job = test_block_job_start(1, true, expected, &result, txn); job_start(&job->job); - if (expected == -ECANCELED) { - job_cancel(&job->job, false); + WITH_JOB_LOCK_GUARD() { + if (expected == -ECANCELED) { + job_cancel_locked(&job->job, false); + } } while (result == -EINPROGRESS) { @@ -XXX,XX +XXX,XX @@ static void test_pair_jobs(int expected1, int expected2) /* Release our reference now to trigger as many nice * use-after-free bugs as possible. */ - job_txn_unref(txn); + WITH_JOB_LOCK_GUARD() { + job_txn_unref_locked(txn); - if (expected1 == -ECANCELED) { - job_cancel(&job1->job, false); - } - if (expected2 == -ECANCELED) { - job_cancel(&job2->job, false); + if (expected1 == -ECANCELED) { + job_cancel_locked(&job1->job, false); + } + if (expected2 == -ECANCELED) { + job_cancel_locked(&job2->job, false); + } } while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) { @@ -XXX,XX +XXX,XX @@ static void test_pair_jobs_fail_cancel_race(void) job_start(&job1->job); job_start(&job2->job); - job_cancel(&job1->job, false); + WITH_JOB_LOCK_GUARD() { + job_cancel_locked(&job1->job, false); + } /* Now make job2 finish before the main loop kicks jobs. This simulates * the race between a pending kick and another job completing. diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-blockjob.c +++ b/tests/unit/test-blockjob.c @@ -XXX,XX +XXX,XX @@ static CancelJob *create_common(Job **pjob) bjob = mk_job(blk, "Steve", &test_cancel_driver, true, JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS); job = &bjob->job; - job_ref(job); - assert(job->status == JOB_STATUS_CREATED); + WITH_JOB_LOCK_GUARD() { + job_ref_locked(job); + assert(job->status == JOB_STATUS_CREATED); + } + s = container_of(bjob, CancelJob, common); s->blk = blk; @@ -XXX,XX +XXX,XX @@ static void cancel_common(CancelJob *s) aio_context_acquire(ctx); job_cancel_sync(&job->job, true); - if (sts != JOB_STATUS_CREATED && sts != JOB_STATUS_CONCLUDED) { - Job *dummy = &job->job; - job_dismiss(&dummy, &error_abort); + WITH_JOB_LOCK_GUARD() { + if (sts != JOB_STATUS_CREATED && sts != JOB_STATUS_CONCLUDED) { + Job *dummy = &job->job; + job_dismiss_locked(&dummy, &error_abort); + } + assert(job->job.status == JOB_STATUS_NULL); + job_unref_locked(&job->job); } - assert(job->job.status == JOB_STATUS_NULL); - job_unref(&job->job); destroy_blk(blk); aio_context_release(ctx); @@ -XXX,XX +XXX,XX @@ static void test_cancel_created(void) cancel_common(s); } +static void assert_job_status_is(Job *job, int status) +{ + WITH_JOB_LOCK_GUARD() { + assert(job->status == status); + } +} + static void test_cancel_running(void) { Job *job; @@ -XXX,XX +XXX,XX @@ static void test_cancel_running(void) s = create_common(&job); job_start(job); - assert(job->status == JOB_STATUS_RUNNING); + assert_job_status_is(job, JOB_STATUS_RUNNING); cancel_common(s); } @@ -XXX,XX +XXX,XX @@ static void test_cancel_paused(void) s = create_common(&job); job_start(job); - assert(job->status == JOB_STATUS_RUNNING); - - job_user_pause(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + assert(job->status == JOB_STATUS_RUNNING); + job_user_pause_locked(job, &error_abort); + } job_enter(job); - assert(job->status == JOB_STATUS_PAUSED); + assert_job_status_is(job, JOB_STATUS_PAUSED); cancel_common(s); } @@ -XXX,XX +XXX,XX @@ static void test_cancel_ready(void) s = create_common(&job); job_start(job); - assert(job->status == JOB_STATUS_RUNNING); + assert_job_status_is(job, JOB_STATUS_RUNNING); s->should_converge = true; job_enter(job); - assert(job->status == JOB_STATUS_READY); + assert_job_status_is(job, JOB_STATUS_READY); cancel_common(s); } @@ -XXX,XX +XXX,XX @@ static void test_cancel_standby(void) s = create_common(&job); job_start(job); - assert(job->status == JOB_STATUS_RUNNING); + assert_job_status_is(job, JOB_STATUS_RUNNING); s->should_converge = true; job_enter(job); - assert(job->status == JOB_STATUS_READY); - - job_user_pause(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + assert(job->status == JOB_STATUS_READY); + job_user_pause_locked(job, &error_abort); + } job_enter(job); - assert(job->status == JOB_STATUS_STANDBY); + assert_job_status_is(job, JOB_STATUS_STANDBY); cancel_common(s); } @@ -XXX,XX +XXX,XX @@ static void test_cancel_pending(void) s = create_common(&job); job_start(job); - assert(job->status == JOB_STATUS_RUNNING); + assert_job_status_is(job, JOB_STATUS_RUNNING); s->should_converge = true; job_enter(job); - assert(job->status == JOB_STATUS_READY); - - job_complete(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + assert(job->status == JOB_STATUS_READY); + job_complete_locked(job, &error_abort); + } job_enter(job); while (!job->deferred_to_main_loop) { aio_poll(qemu_get_aio_context(), true); } - assert(job->status == JOB_STATUS_READY); + assert_job_status_is(job, JOB_STATUS_READY); aio_poll(qemu_get_aio_context(), true); - assert(job->status == JOB_STATUS_PENDING); + assert_job_status_is(job, JOB_STATUS_PENDING); cancel_common(s); } @@ -XXX,XX +XXX,XX @@ static void test_cancel_concluded(void) s = create_common(&job); job_start(job); - assert(job->status == JOB_STATUS_RUNNING); + assert_job_status_is(job, JOB_STATUS_RUNNING); s->should_converge = true; job_enter(job); - assert(job->status == JOB_STATUS_READY); - - job_complete(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + assert(job->status == JOB_STATUS_READY); + job_complete_locked(job, &error_abort); + } job_enter(job); while (!job->deferred_to_main_loop) { aio_poll(qemu_get_aio_context(), true); } - assert(job->status == JOB_STATUS_READY); + assert_job_status_is(job, JOB_STATUS_READY); aio_poll(qemu_get_aio_context(), true); - assert(job->status == JOB_STATUS_PENDING); + assert_job_status_is(job, JOB_STATUS_PENDING); aio_context_acquire(job->aio_context); - job_finalize(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + job_finalize_locked(job, &error_abort); + } aio_context_release(job->aio_context); - assert(job->status == JOB_STATUS_CONCLUDED); + assert_job_status_is(job, JOB_STATUS_CONCLUDED); cancel_common(s); } @@ -XXX,XX +XXX,XX @@ static void test_complete_in_standby(void) bjob = mk_job(blk, "job", &test_yielding_driver, true, JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS); job = &bjob->job; - assert(job->status == JOB_STATUS_CREATED); + assert_job_status_is(job, JOB_STATUS_CREATED); /* Wait for the job to become READY */ job_start(job); aio_context_acquire(ctx); + /* + * Here we are waiting for the status to change, so don't bother + * protecting the read every time. + */ AIO_WAIT_WHILE(ctx, job->status != JOB_STATUS_READY); aio_context_release(ctx); /* Begin the drained section, pausing the job */ bdrv_drain_all_begin(); - assert(job->status == JOB_STATUS_STANDBY); + assert_job_status_is(job, JOB_STATUS_STANDBY); + /* Lock the IO thread to prevent the job from being run */ aio_context_acquire(ctx); /* This will schedule the job to resume it */ bdrv_drain_all_end(); - /* But the job cannot run, so it will remain on standby */ - assert(job->status == JOB_STATUS_STANDBY); + WITH_JOB_LOCK_GUARD() { + /* But the job cannot run, so it will remain on standby */ + assert(job->status == JOB_STATUS_STANDBY); - /* Even though the job is on standby, this should work */ - job_complete(job, &error_abort); + /* Even though the job is on standby, this should work */ + job_complete_locked(job, &error_abort); - /* The test is done now, clean up. */ - job_finish_sync(job, NULL, &error_abort); - assert(job->status == JOB_STATUS_PENDING); + /* The test is done now, clean up. */ + job_finish_sync_locked(job, NULL, &error_abort); + assert(job->status == JOB_STATUS_PENDING); - job_finalize(job, &error_abort); - assert(job->status == JOB_STATUS_CONCLUDED); + job_finalize_locked(job, &error_abort); + assert(job->status == JOB_STATUS_CONCLUDED); - job_dismiss(&job, &error_abort); + job_dismiss_locked(&job, &error_abort); + } destroy_blk(blk); aio_context_release(ctx); -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Once job lock is used and aiocontext is removed, mirror has to perform job operations under the same critical section, Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20220926093214.506243-11-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/mirror.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index XXXXXXX..XXXXXXX 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -XXX,XX +XXX,XX @@ static void mirror_complete(Job *job, Error **errp) s->should_complete = true; /* If the job is paused, it will be re-entered when it is resumed */ - if (!job->paused) { - job_enter(job); + WITH_JOB_LOCK_GUARD() { + if (!job->paused) { + job_enter_cond_locked(job, NULL); + } } } @@ -XXX,XX +XXX,XX @@ static bool mirror_drained_poll(BlockJob *job) * from one of our own drain sections, to avoid a deadlock waiting for * ourselves. */ - if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain) { - return true; + WITH_JOB_LOCK_GUARD() { + if (!s->common.job.paused && !job_is_cancelled_locked(&job->job) + && !s->in_drain) { + return true; + } } return !!s->in_flight; -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Now that the API offers also _locked() functions, take advantage of it and give also the caller control to take the lock and call _locked functions. This makes sense especially when we have for loops, because it makes no sense to have: for(job = job_next(); ...) where each job_next() takes the lock internally. Instead we want JOB_LOCK_GUARD(); for(job = job_next_locked(); ...) In addition, protect also direct field accesses, by either creating a new critical section or widening the existing ones. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20220926093214.506243-12-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 17 ++++++++++------- blockdev.c | 14 ++++++++++---- blockjob.c | 35 ++++++++++++++++++++++------------- job-qmp.c | 9 ++++++--- monitor/qmp-cmds.c | 7 +++++-- qemu-img.c | 15 ++++++++++----- 6 files changed, 63 insertions(+), 34 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 bdrv_close(BlockDriverState *bs) void bdrv_close_all(void) { - assert(job_next(NULL) == NULL); GLOBAL_STATE_CODE(); + assert(job_next(NULL) == NULL); /* Drop references from requests still in flight, such as canceled block * jobs whose AIO context has not been polled yet */ @@ -XXX,XX +XXX,XX @@ XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp) } } - for (job = block_job_next(NULL); job; job = block_job_next(job)) { - GSList *el; + WITH_JOB_LOCK_GUARD() { + for (job = block_job_next_locked(NULL); job; + job = block_job_next_locked(job)) { + GSList *el; - xdbg_graph_add_node(gr, job, X_DBG_BLOCK_GRAPH_NODE_TYPE_BLOCK_JOB, - job->job.id); - for (el = job->nodes; el; el = el->next) { - xdbg_graph_add_edge(gr, job, (BdrvChild *)el->data); + xdbg_graph_add_node(gr, job, X_DBG_BLOCK_GRAPH_NODE_TYPE_BLOCK_JOB, + job->job.id); + for (el = job->nodes; el; el = el->next) { + xdbg_graph_add_edge(gr, job, (BdrvChild *)el->data); + } } } diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ void blockdev_mark_auto_del(BlockBackend *blk) return; } - for (job = block_job_next(NULL); job; job = block_job_next(job)) { + JOB_LOCK_GUARD(); + + for (job = block_job_next_locked(NULL); job; + job = block_job_next_locked(job)) { if (block_job_has_bdrv(job, blk_bs(blk))) { AioContext *aio_context = job->job.aio_context; aio_context_acquire(aio_context); - job_cancel(&job->job, false); + job_cancel_locked(&job->job, false); aio_context_release(aio_context); } @@ -XXX,XX +XXX,XX @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) BlockJobInfoList *head = NULL, **tail = &head; BlockJob *job; - for (job = block_job_next(NULL); job; job = block_job_next(job)) { + JOB_LOCK_GUARD(); + + for (job = block_job_next_locked(NULL); job; + job = block_job_next_locked(job)) { BlockJobInfo *value; AioContext *aio_context; @@ -XXX,XX +XXX,XX @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) } aio_context = block_job_get_aio_context(job); aio_context_acquire(aio_context); - value = block_job_query(job, errp); + value = block_job_query_locked(job, errp); aio_context_release(aio_context); if (!value) { qapi_free_BlockJobInfoList(head); diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static bool child_job_drained_poll(BdrvChild *c) /* An inactive or completed job doesn't have any pending requests. Jobs * with !job->busy are either already paused or have a pause point after * being reentered, so no job driver code will run before they pause. */ - if (!job->busy || job_is_completed(job)) { - return false; + WITH_JOB_LOCK_GUARD() { + if (!job->busy || job_is_completed_locked(job)) { + return false; + } } /* Otherwise, assume that it isn't fully stopped yet, but allow the job to @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, job->ready_notifier.notify = block_job_event_ready; job->idle_notifier.notify = block_job_on_idle; - notifier_list_add(&job->job.on_finalize_cancelled, - &job->finalize_cancelled_notifier); - notifier_list_add(&job->job.on_finalize_completed, - &job->finalize_completed_notifier); - notifier_list_add(&job->job.on_pending, &job->pending_notifier); - notifier_list_add(&job->job.on_ready, &job->ready_notifier); - notifier_list_add(&job->job.on_idle, &job->idle_notifier); + WITH_JOB_LOCK_GUARD() { + notifier_list_add(&job->job.on_finalize_cancelled, + &job->finalize_cancelled_notifier); + notifier_list_add(&job->job.on_finalize_completed, + &job->finalize_completed_notifier); + notifier_list_add(&job->job.on_pending, &job->pending_notifier); + notifier_list_add(&job->job.on_ready, &job->ready_notifier); + notifier_list_add(&job->job.on_idle, &job->idle_notifier); + } error_setg(&job->blocker, "block device is in use by block job: %s", job_type_str(&job->job)); @@ -XXX,XX +XXX,XX @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, action); } if (action == BLOCK_ERROR_ACTION_STOP) { - if (!job->job.user_paused) { - job_pause(&job->job); - /* make the pause user visible, which will be resumed from QMP. */ - job->job.user_paused = true; + WITH_JOB_LOCK_GUARD() { + if (!job->job.user_paused) { + job_pause_locked(&job->job); + /* + * make the pause user visible, which will be + * resumed from QMP. + */ + job->job.user_paused = true; + } } block_job_iostatus_set_err(job, error); } diff --git a/job-qmp.c b/job-qmp.c index XXXXXXX..XXXXXXX 100644 --- a/job-qmp.c +++ b/job-qmp.c @@ -XXX,XX +XXX,XX @@ void qmp_job_dismiss(const char *id, Error **errp) aio_context_release(aio_context); } -static JobInfo *job_query_single(Job *job, Error **errp) +/* Called with job_mutex held. */ +static JobInfo *job_query_single_locked(Job *job, Error **errp) { JobInfo *info; uint64_t progress_current; @@ -XXX,XX +XXX,XX @@ JobInfoList *qmp_query_jobs(Error **errp) JobInfoList *head = NULL, **tail = &head; Job *job; - for (job = job_next(NULL); job; job = job_next(job)) { + JOB_LOCK_GUARD(); + + for (job = job_next_locked(NULL); job; job = job_next_locked(job)) { JobInfo *value; AioContext *aio_context; @@ -XXX,XX +XXX,XX @@ JobInfoList *qmp_query_jobs(Error **errp) } aio_context = job->aio_context; aio_context_acquire(aio_context); - value = job_query_single(job, errp); + value = job_query_single_locked(job, errp); aio_context_release(aio_context); if (!value) { qapi_free_JobInfoList(head); diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -XXX,XX +XXX,XX @@ void qmp_cont(Error **errp) blk_iostatus_reset(blk); } - for (job = block_job_next(NULL); job; job = block_job_next(job)) { - block_job_iostatus_reset(job); + WITH_JOB_LOCK_GUARD() { + for (job = block_job_next_locked(NULL); job; + job = block_job_next_locked(job)) { + block_job_iostatus_reset_locked(job); + } } /* Continuing after completed migration. Images have been inactivated to 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 void run_block_job(BlockJob *job, Error **errp) int ret = 0; aio_context_acquire(aio_context); - job_ref(&job->job); + job_lock(); + job_ref_locked(&job->job); do { float progress = 0.0f; + job_unlock(); aio_poll(aio_context, true); progress_get_snapshot(&job->job.progress, &progress_current, @@ -XXX,XX +XXX,XX @@ static void run_block_job(BlockJob *job, Error **errp) progress = (float)progress_current / progress_total * 100.f; } qemu_progress_print(progress, 0); - } while (!job_is_ready(&job->job) && !job_is_completed(&job->job)); + job_lock(); + } while (!job_is_ready_locked(&job->job) && + !job_is_completed_locked(&job->job)); - if (!job_is_completed(&job->job)) { - ret = job_complete_sync(&job->job, errp); + if (!job_is_completed_locked(&job->job)) { + ret = job_complete_sync_locked(&job->job, errp); } else { ret = job->job.ret; } - job_unref(&job->job); + job_unref_locked(&job->job); + job_unlock(); aio_context_release(aio_context); /* publish completion progress only when success */ -- 2.37.3 From: Paolo Bonzini <pbonzini@redhat.com> We want to make sure access of job->aio_context is always done under either BQL or job_mutex. The problem is that using aio_co_enter(job->aiocontext, job->co) in job_start and job_enter_cond makes the coroutine immediately resume, so we can't hold the job lock. And caching it is not safe either, as it might change. job_start is under BQL, so it can freely read job->aiocontext, but job_enter_cond is not. We want to avoid reading job->aio_context in job_enter_cond, therefore: 1) use aio_co_wake(), since it doesn't want an aiocontext as argument but uses job->co->ctx 2) detect possible discrepancy between job->co->ctx and job->aio_context by checking right after the coroutine resumes back from yielding if job->aio_context has changed. If so, reschedule the coroutine to the new context. Calling bdrv_try_set_aio_context() will issue the following calls (simplified): * in terms of bdrv callbacks: .drained_begin -> .set_aio_context -> .drained_end * in terms of child_job functions: child_job_drained_begin -> child_job_set_aio_context -> child_job_drained_end * in terms of job functions: job_pause_locked -> job_set_aio_context -> job_resume_locked We can see that after setting the new aio_context, job_resume_locked calls again job_enter_cond, which then invokes aio_co_wake(). But while job->aiocontext has been set in job_set_aio_context, job->co->ctx has not changed, so the coroutine would be entering in the wrong aiocontext. Using aio_co_schedule in job_resume_locked() might seem as a valid alternative, but the problem is that the bh resuming the coroutine is not scheduled immediately, and if in the meanwhile another bdrv_try_set_aio_context() is run (see test_propagate_mirror() in test-block-iothread.c), we would have the first schedule in the wrong aiocontext, and the second set of drains won't even manage to schedule the coroutine, as job->busy would still be true from the previous job_resume_locked(). The solution is to stick with aio_co_wake() and detect every time the coroutine resumes back from yielding if job->aio_context has changed. If so, we can reschedule it to the new context. Check for the aiocontext change in job_do_yield_locked because: 1) aio_co_reschedule_self requires to be in the running coroutine 2) since child_job_set_aio_context allows changing the aiocontext only while the job is paused, this is the exact place where the coroutine resumes, before running JobDriver's code. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220926093214.506243-13-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- job.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/job.c b/job.c index XXXXXXX..XXXXXXX 100644 --- a/job.c +++ b/job.c @@ -XXX,XX +XXX,XX @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)) job->busy = true; real_job_unlock(); job_unlock(); - aio_co_enter(job->aio_context, job->co); + aio_co_wake(job->co); job_lock(); } @@ -XXX,XX +XXX,XX @@ void job_enter(Job *job) */ static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns) { + AioContext *next_aio_context; + real_job_lock(); if (ns != -1) { timer_mod(&job->sleep_timer, ns); @@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns) qemu_coroutine_yield(); job_lock(); - /* Set by job_enter_cond() before re-entering the coroutine. */ + next_aio_context = job->aio_context; + /* + * Coroutine has resumed, but in the meanwhile the job AioContext + * might have changed via bdrv_try_set_aio_context(), so we need to move + * the coroutine too in the new aiocontext. + */ + while (qemu_get_current_aio_context() != next_aio_context) { + job_unlock(); + aio_co_reschedule_self(next_aio_context); + job_lock(); + next_aio_context = job->aio_context; + } + + /* Set by job_enter_cond_locked() before re-entering the coroutine. */ assert(job->busy); } -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> In order to make it thread safe, implement a "fake rwlock", where we allow reads under BQL *or* job_mutex held, but writes only under BQL *and* job_mutex. The only write we have is in child_job_set_aio_ctx, which always happens under drain (so the job is paused). For this reason, introduce job_set_aio_context and make sure that the context is set under BQL, job_mutex and drain. Also make sure all other places where the aiocontext is read are protected. The reads in commit.c and mirror.c are actually safe, because always done under BQL. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220926093214.506243-14-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 23 ++++++++++++++++++++--- block/replication.c | 1 + blockjob.c | 3 ++- job.c | 12 ++++++++++++ 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ typedef struct Job { /* ProgressMeter API is thread-safe */ ProgressMeter progress; + /** + * AioContext to run the job coroutine in. + * The job Aiocontext can be read when holding *either* + * the BQL (so we are in the main loop) or the job_mutex. + * It can only be written when we hold *both* BQL + * and the job_mutex. + */ + AioContext *aio_context; - /** Protected by AioContext lock */ - /** AioContext to run the job coroutine in */ - AioContext *aio_context; + /** Protected by AioContext lock */ /** Reference count of the block job */ int refcnt; @@ -XXX,XX +XXX,XX @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), Error **errp); +/** + * Sets the @job->aio_context. + * Called with job_mutex *not* held. + * + * This function must run in the main thread to protect against + * concurrent read in job_finish_sync_locked(), takes the job_mutex + * lock to protect against the read in job_do_yield_locked(), and must + * be called when the job is quiescent. + */ +void job_set_aio_context(Job *job, AioContext *ctx); + #endif diff --git a/block/replication.c b/block/replication.c index XXXXXXX..XXXXXXX 100644 --- a/block/replication.c +++ b/block/replication.c @@ -XXX,XX +XXX,XX @@ static void replication_close(BlockDriverState *bs) { BDRVReplicationState *s = bs->opaque; Job *commit_job; + GLOBAL_STATE_CODE(); if (s->stage == BLOCK_REPLICATION_RUNNING) { replication_stop(s->rs, false, NULL); diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static void child_job_set_aio_ctx(BdrvChild *c, AioContext *ctx, bdrv_set_aio_context_ignore(sibling->bs, ctx, ignore); } - job->job.aio_context = ctx; + job_set_aio_context(&job->job, ctx); } static AioContext *child_job_get_parent_aio_context(BdrvChild *c) { BlockJob *job = c->opaque; + GLOBAL_STATE_CODE(); return job->job.aio_context; } diff --git a/job.c b/job.c index XXXXXXX..XXXXXXX 100644 --- a/job.c +++ b/job.c @@ -XXX,XX +XXX,XX @@ Job *job_get(const char *id) return job_get_locked(id); } +void job_set_aio_context(Job *job, AioContext *ctx) +{ + /* protect against read in job_finish_sync_locked and job_start */ + GLOBAL_STATE_CODE(); + /* protect against read in job_do_yield_locked */ + JOB_LOCK_GUARD(); + /* ensure the job is quiescent while the AioContext is changed */ + assert(job->paused || job_is_completed_locked(job)); + job->aio_context = ctx; +} + /* Called with job_mutex *not* held. */ static void job_sleep_timer_cb(void *opaque) { @@ -XXX,XX +XXX,XX @@ int job_finish_sync_locked(Job *job, { Error *local_err = NULL; int ret; + GLOBAL_STATE_CODE(); job_ref_locked(job); -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> The same job lock is being used also to protect some of blockjob fields. Categorize them just as done in job.h. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220926093214.506243-15-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/blockjob.h | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/include/block/blockjob.h b/include/block/blockjob.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -XXX,XX +XXX,XX @@ typedef struct BlockJobDriver BlockJobDriver; * Long-running operation on a BlockDriverState. */ typedef struct BlockJob { - /** Data belonging to the generic Job infrastructure */ + /** + * Data belonging to the generic Job infrastructure. + * Protected by job mutex. + */ Job job; - /** Status that is published by the query-block-jobs QMP API */ + /** + * Status that is published by the query-block-jobs QMP API. + * Protected by job mutex. + */ BlockDeviceIoStatus iostatus; - /** Speed that was set with @block_job_set_speed. */ + /** + * Speed that was set with @block_job_set_speed. + * Always modified and read under QEMU global mutex (GLOBAL_STATE_CODE). + */ int64_t speed; - /** Rate limiting data structure for implementing @speed. */ + /** + * Rate limiting data structure for implementing @speed. + * RateLimit API is thread-safe. + */ RateLimit limit; - /** Block other operations when block job is running */ + /** + * Block other operations when block job is running. + * Always modified and read under QEMU global mutex (GLOBAL_STATE_CODE). + */ Error *blocker; + /** All notifiers are set once in block_job_create() and never modified. */ + /** Called when a cancelled job is finalised. */ Notifier finalize_cancelled_notifier; @@ -XXX,XX +XXX,XX @@ typedef struct BlockJob { /** Called when the job coroutine yields or terminates */ Notifier idle_notifier; - /** BlockDriverStates that are involved in this block job */ + /** + * BlockDriverStates that are involved in this block job. + * Always modified and read under QEMU global mutex (GLOBAL_STATE_CODE). + */ GSList *nodes; } BlockJob; -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> They all are called with job_lock held, in job_event_*_locked() Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20220926093214.506243-16-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockjob.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs, return 0; } -static void block_job_on_idle(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_on_idle_locked(Notifier *n, void *opaque) { aio_wait_kick(); } @@ -XXX,XX +XXX,XX @@ static void block_job_iostatus_set_err(BlockJob *job, int error) } } -static void block_job_event_cancelled(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_event_cancelled_locked(Notifier *n, void *opaque) { BlockJob *job = opaque; uint64_t progress_current, progress_total; @@ -XXX,XX +XXX,XX @@ static void block_job_event_cancelled(Notifier *n, void *opaque) job->speed); } -static void block_job_event_completed(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_event_completed_locked(Notifier *n, void *opaque) { BlockJob *job = opaque; const char *msg = NULL; @@ -XXX,XX +XXX,XX @@ static void block_job_event_completed(Notifier *n, void *opaque) msg); } -static void block_job_event_pending(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_event_pending_locked(Notifier *n, void *opaque) { BlockJob *job = opaque; @@ -XXX,XX +XXX,XX @@ static void block_job_event_pending(Notifier *n, void *opaque) job->job.id); } -static void block_job_event_ready(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_event_ready_locked(Notifier *n, void *opaque) { BlockJob *job = opaque; uint64_t progress_current, progress_total; @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, ratelimit_init(&job->limit); - job->finalize_cancelled_notifier.notify = block_job_event_cancelled; - job->finalize_completed_notifier.notify = block_job_event_completed; - job->pending_notifier.notify = block_job_event_pending; - job->ready_notifier.notify = block_job_event_ready; - job->idle_notifier.notify = block_job_on_idle; + job->finalize_cancelled_notifier.notify = block_job_event_cancelled_locked; + job->finalize_completed_notifier.notify = block_job_event_completed_locked; + job->pending_notifier.notify = block_job_event_pending_locked; + job->ready_notifier.notify = block_job_event_ready_locked; + job->idle_notifier.notify = block_job_on_idle_locked; WITH_JOB_LOCK_GUARD() { notifier_list_add(&job->job.on_finalize_cancelled, -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> iostatus is the only field (together with .job) that needs protection using the job mutex. It is set in the main loop (GLOBAL_STATE functions) but read in I/O code (block_job_error_action). In order to protect it, change block_job_iostatus_set_err to block_job_iostatus_set_err_locked(), always called under job lock. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20220926093214.506243-17-eesposit@redhat.com> [kwolf: Fixed up type of iostatus] Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/mirror.c | 6 +++++- blockjob.c | 5 +++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index XXXXXXX..XXXXXXX 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_run(Job *job, Error **errp) BlockDriverState *bs = s->mirror_top_bs->backing->bs; BlockDriverState *target_bs = blk_bs(s->target); bool need_drain = true; + BlockDeviceIoStatus iostatus; int64_t length; int64_t target_length; BlockDriverInfo bdi; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_run(Job *job, Error **errp) * We do so every BLKOCK_JOB_SLICE_TIME nanoseconds, or when there is * an error, or when the source is clean, whichever comes first. */ delta = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->last_pause_ns; + WITH_JOB_LOCK_GUARD() { + iostatus = s->common.iostatus; + } if (delta < BLOCK_JOB_SLICE_TIME && - s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) { + iostatus == BLOCK_DEVICE_IO_STATUS_OK) { if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 || (cnt == 0 && s->in_flight > 0)) { trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight); diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) return block_job_query_locked(job, errp); } -static void block_job_iostatus_set_err(BlockJob *job, int error) +/* Called with job lock held */ +static void block_job_iostatus_set_err_locked(BlockJob *job, int error) { if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { job->iostatus = error == ENOSPC ? BLOCK_DEVICE_IO_STATUS_NOSPACE : @@ -XXX,XX +XXX,XX @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, */ job->job.user_paused = true; } + block_job_iostatus_set_err_locked(job, error); } - block_job_iostatus_set_err(job, error); } return action; } -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Some callbacks implementation use bdrv_* APIs that assume the AioContext lock is held. Make sure this invariant is documented. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20220926093214.506243-18-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ typedef struct Job { /** True if this job should automatically dismiss itself */ bool auto_dismiss; - /** The completion function that will be called when the job completes. */ + /** + * The completion function that will be called when the job completes. + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. + */ BlockCompletionFunc *cb; /** The opaque value that is passed to the completion function. */ @@ -XXX,XX +XXX,XX @@ struct JobDriver { * * This callback will not be invoked if the job has already failed. * If it fails, abort and then clean will be called. + * + * Called with AioContext lock held, since many callbacs implementations + * use bdrv_* functions that require to hold the lock. */ int (*prepare)(Job *job); @@ -XXX,XX +XXX,XX @@ struct JobDriver { * * All jobs will complete with a call to either .commit() or .abort() but * never both. + * + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. */ void (*commit)(Job *job); @@ -XXX,XX +XXX,XX @@ struct JobDriver { * * All jobs will complete with a call to either .commit() or .abort() but * never both. + * + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. */ void (*abort)(Job *job); @@ -XXX,XX +XXX,XX @@ struct JobDriver { * .commit() or .abort(). Regardless of which callback is invoked after * completion, .clean() will always be called, even if the job does not * belong to a transaction group. + * + * Called with AioContext lock held, since many callbacs implementations + * use bdrv_* functions that require to hold the lock. */ void (*clean)(Job *job); @@ -XXX,XX +XXX,XX @@ struct JobDriver { * READY). * (If the callback is NULL, the job is assumed to terminate * without I/O.) + * + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. */ bool (*cancel)(Job *job, bool force); - /** Called when the job is freed */ + /** + * Called when the job is freed. + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. + */ void (*free)(Job *job); }; -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Change the job_{lock/unlock} and macros to use job_mutex. Now that they are not nop anymore, remove the aiocontext to avoid deadlocks. Therefore: - when possible, remove completely the aiocontext lock/unlock pair - if it is used by some other function too, reduce the locking section as much as possible, leaving the job API outside. - change AIO_WAIT_WHILE in AIO_WAIT_WHILE_UNLOCKED, since we are not using the aiocontext lock anymore The only functions that still need the aiocontext lock are: - the JobDriver callbacks, already documented in job.h - job_cancel_sync() in replication.c is called with aio_context_lock taken, but now job is using AIO_WAIT_WHILE_UNLOCKED so we need to release the lock. Reduce the locking section to only cover the callback invocation and document the functions that take the AioContext lock, to avoid taking it twice. Also remove real_job_{lock/unlock}, as they are replaced by the public functions. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220926093214.506243-19-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 17 ++--- block/replication.c | 2 + blockdev.c | 72 +++----------------- job-qmp.c | 46 +++---------- job.c | 111 +++++++++---------------------- qemu-img.c | 2 - tests/unit/test-bdrv-drain.c | 4 +- tests/unit/test-block-iothread.c | 2 +- tests/unit/test-blockjob.c | 19 +++--- 9 files changed, 72 insertions(+), 203 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ typedef struct Job { AioContext *aio_context; - /** Protected by AioContext lock */ + /** Protected by job_mutex */ /** Reference count of the block job */ int refcnt; @@ -XXX,XX +XXX,XX @@ typedef struct Job { /** * Set to false by the job while the coroutine has yielded and may be * re-entered by job_enter(). There may still be I/O or event loop activity - * pending. Accessed under block_job_mutex (in blockjob.c). + * pending. Accessed under job_mutex. * * When the job is deferred to the main loop, busy is true as long as the * bottom half is still pending. @@ -XXX,XX +XXX,XX @@ typedef enum JobCreateFlags { extern QemuMutex job_mutex; -#define JOB_LOCK_GUARD() /* QEMU_LOCK_GUARD(&job_mutex) */ +#define JOB_LOCK_GUARD() QEMU_LOCK_GUARD(&job_mutex) -#define WITH_JOB_LOCK_GUARD() /* WITH_QEMU_LOCK_GUARD(&job_mutex) */ +#define WITH_JOB_LOCK_GUARD() WITH_QEMU_LOCK_GUARD(&job_mutex) /** * job_lock: @@ -XXX,XX +XXX,XX @@ void job_ref_locked(Job *job); /** * Release a reference that was previously acquired with job_ref() or * job_create(). If it's the last reference to the object, it will be freed. + * + * Takes AioContext lock internally to invoke a job->driver callback. */ void job_unref(Job *job); @@ -XXX,XX +XXX,XX @@ void job_user_cancel_locked(Job *job, bool force, Error **errp); * Returns the return value from the job if the job actually completed * during the call, or -ECANCELED if it was canceled. * - * Callers must hold the AioContext lock of job->aio_context. + * Called with job_lock *not* held. */ int job_cancel_sync(Job *job, bool force); @@ -XXX,XX +XXX,XX @@ void job_cancel_sync_all(void); * function). * * Returns the return value from the job. - * - * Callers must hold the AioContext lock of job->aio_context. + * Called with job_lock *not* held. */ int job_complete_sync(Job *job, Error **errp); @@ -XXX,XX +XXX,XX @@ void job_dismiss_locked(Job **job, Error **errp); * Returns 0 if the job is successfully completed, -ECANCELED if the job was * cancelled before completing, and -errno in other error cases. * - * Callers must hold the AioContext lock of job->aio_context. + * Called with job_lock *not* held. */ int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp); diff --git a/block/replication.c b/block/replication.c index XXXXXXX..XXXXXXX 100644 --- a/block/replication.c +++ b/block/replication.c @@ -XXX,XX +XXX,XX @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp) * disk, secondary disk in backup_job_completed(). */ if (s->backup_job) { + aio_context_release(aio_context); job_cancel_sync(&s->backup_job->job, true); + aio_context_acquire(aio_context); } if (!failover) { diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ void blockdev_mark_auto_del(BlockBackend *blk) for (job = block_job_next_locked(NULL); job; job = block_job_next_locked(job)) { if (block_job_has_bdrv(job, blk_bs(blk))) { - AioContext *aio_context = job->job.aio_context; - aio_context_acquire(aio_context); - job_cancel_locked(&job->job, false); - - aio_context_release(aio_context); } } @@ -XXX,XX +XXX,XX @@ static void drive_backup_abort(BlkActionState *common) DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); if (state->job) { - AioContext *aio_context; - - aio_context = bdrv_get_aio_context(state->bs); - aio_context_acquire(aio_context); - job_cancel_sync(&state->job->job, true); - - aio_context_release(aio_context); } } @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_abort(BlkActionState *common) BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); if (state->job) { - AioContext *aio_context; - - aio_context = bdrv_get_aio_context(state->bs); - aio_context_acquire(aio_context); - job_cancel_sync(&state->job->job, true); - - aio_context_release(aio_context); } } @@ -XXX,XX +XXX,XX @@ out: } /* - * Get a block job using its ID and acquire its AioContext. - * Called with job_mutex held. + * Get a block job using its ID. Called with job_mutex held. */ -static BlockJob *find_block_job_locked(const char *id, - AioContext **aio_context, - Error **errp) +static BlockJob *find_block_job_locked(const char *id, Error **errp) { BlockJob *job; assert(id != NULL); - *aio_context = NULL; - job = block_job_get_locked(id); if (!job) { @@ -XXX,XX +XXX,XX @@ static BlockJob *find_block_job_locked(const char *id, return NULL; } - *aio_context = block_job_get_aio_context(job); - aio_context_acquire(*aio_context); - return job; } void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; } block_job_set_speed_locked(job, speed, errp); - aio_context_release(aio_context); } void qmp_block_job_cancel(const char *device, bool has_force, bool force, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_block_job_cancel(const char *device, if (job_user_paused_locked(&job->job) && !force) { error_setg(errp, "The block job for device '%s' is currently paused", device); - goto out; + return; } trace_qmp_block_job_cancel(job); job_user_cancel_locked(&job->job, force, errp); -out: - aio_context_release(aio_context); } void qmp_block_job_pause(const char *device, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_block_job_pause(const char *device, Error **errp) trace_qmp_block_job_pause(job); job_user_pause_locked(&job->job, errp); - aio_context_release(aio_context); } void qmp_block_job_resume(const char *device, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_block_job_resume(const char *device, Error **errp) trace_qmp_block_job_resume(job); job_user_resume_locked(&job->job, errp); - aio_context_release(aio_context); } void qmp_block_job_complete(const char *device, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_block_job_complete(const char *device, Error **errp) trace_qmp_block_job_complete(job); job_complete_locked(&job->job, errp); - aio_context_release(aio_context); } void qmp_block_job_finalize(const char *id, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(id, &aio_context, errp); + job = find_block_job_locked(id, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_block_job_finalize(const char *id, Error **errp) job_ref_locked(&job->job); job_finalize_locked(&job->job, errp); - /* - * Job's context might have changed via job_finalize (and job_txn_apply - * automatically acquires the new one), so make sure we release the correct - * one. - */ - aio_context = block_job_get_aio_context(job); job_unref_locked(&job->job); - aio_context_release(aio_context); } void qmp_block_job_dismiss(const char *id, Error **errp) { - AioContext *aio_context; BlockJob *bjob; Job *job; JOB_LOCK_GUARD(); - bjob = find_block_job_locked(id, &aio_context, errp); + bjob = find_block_job_locked(id, errp); if (!bjob) { return; @@ -XXX,XX +XXX,XX @@ void qmp_block_job_dismiss(const char *id, Error **errp) trace_qmp_block_job_dismiss(bjob); job = &bjob->job; job_dismiss_locked(&job, errp); - aio_context_release(aio_context); } void qmp_change_backing_file(const char *device, @@ -XXX,XX +XXX,XX @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) for (job = block_job_next_locked(NULL); job; job = block_job_next_locked(job)) { BlockJobInfo *value; - AioContext *aio_context; if (block_job_is_internal(job)) { continue; } - aio_context = block_job_get_aio_context(job); - aio_context_acquire(aio_context); value = block_job_query_locked(job, errp); - aio_context_release(aio_context); if (!value) { qapi_free_BlockJobInfoList(head); return NULL; diff --git a/job-qmp.c b/job-qmp.c index XXXXXXX..XXXXXXX 100644 --- a/job-qmp.c +++ b/job-qmp.c @@ -XXX,XX +XXX,XX @@ #include "trace/trace-root.h" /* - * Get a job using its ID and acquire its AioContext. - * Called with job_mutex held. + * Get a job using its ID. Called with job_mutex held. */ -static Job *find_job_locked(const char *id, - AioContext **aio_context, - Error **errp) +static Job *find_job_locked(const char *id, Error **errp) { Job *job; - *aio_context = NULL; - job = job_get_locked(id); if (!job) { error_setg(errp, "Job not found"); return NULL; } - *aio_context = job->aio_context; - aio_context_acquire(*aio_context); - return job; } void qmp_job_cancel(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_job_cancel(const char *id, Error **errp) trace_qmp_job_cancel(job); job_user_cancel_locked(job, true, errp); - aio_context_release(aio_context); } void qmp_job_pause(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_job_pause(const char *id, Error **errp) trace_qmp_job_pause(job); job_user_pause_locked(job, errp); - aio_context_release(aio_context); } void qmp_job_resume(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_job_resume(const char *id, Error **errp) trace_qmp_job_resume(job); job_user_resume_locked(job, errp); - aio_context_release(aio_context); } void qmp_job_complete(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_job_complete(const char *id, Error **errp) trace_qmp_job_complete(job); job_complete_locked(job, errp); - aio_context_release(aio_context); } void qmp_job_finalize(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_job_finalize(const char *id, Error **errp) job_ref_locked(job); job_finalize_locked(job, errp); - /* - * Job's context might have changed via job_finalize (and job_txn_apply - * automatically acquires the new one), so make sure we release the correct - * one. - */ - aio_context = job->aio_context; job_unref_locked(job); - aio_context_release(aio_context); } void qmp_job_dismiss(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -XXX,XX +XXX,XX @@ void qmp_job_dismiss(const char *id, Error **errp) trace_qmp_job_dismiss(job); job_dismiss_locked(&job, errp); - aio_context_release(aio_context); } /* Called with job_mutex held. */ @@ -XXX,XX +XXX,XX @@ JobInfoList *qmp_query_jobs(Error **errp) for (job = job_next_locked(NULL); job; job = job_next_locked(job)) { JobInfo *value; - AioContext *aio_context; if (job_is_internal(job)) { continue; } - aio_context = job->aio_context; - aio_context_acquire(aio_context); value = job_query_single_locked(job, errp); - aio_context_release(aio_context); if (!value) { qapi_free_JobInfoList(head); return NULL; diff --git a/job.c b/job.c index XXXXXXX..XXXXXXX 100644 --- a/job.c +++ b/job.c @@ -XXX,XX +XXX,XX @@ * * The second includes functions used by the job drivers and sometimes * by the core block layer. These delegate the locking to the callee instead. - * - * TODO Actually make this true */ /* @@ -XXX,XX +XXX,XX @@ struct JobTxn { }; void job_lock(void) -{ - /* nop */ -} - -void job_unlock(void) -{ - /* nop */ -} - -static void real_job_lock(void) { qemu_mutex_lock(&job_mutex); } -static void real_job_unlock(void) +void job_unlock(void) { qemu_mutex_unlock(&job_mutex); } @@ -XXX,XX +XXX,XX @@ static void job_txn_del_job_locked(Job *job) /* Called with job_mutex held, but releases it temporarily. */ static int job_txn_apply_locked(Job *job, int fn(Job *)) { - AioContext *inner_ctx; Job *other_job, *next; JobTxn *txn = job->txn; int rc = 0; @@ -XXX,XX +XXX,XX @@ static int job_txn_apply_locked(Job *job, int fn(Job *)) * break AIO_WAIT_WHILE from within fn. */ job_ref_locked(job); - aio_context_release(job->aio_context); QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) { - inner_ctx = other_job->aio_context; - aio_context_acquire(inner_ctx); rc = fn(other_job); - aio_context_release(inner_ctx); if (rc) { break; } } - /* - * Note that job->aio_context might have been changed by calling fn, so we - * can't use a local variable to cache it. - */ - aio_context_acquire(job->aio_context); job_unref_locked(job); return rc; } @@ -XXX,XX +XXX,XX @@ void job_unref_locked(Job *job) assert(!job->txn); if (job->driver->free) { + AioContext *aio_context = job->aio_context; job_unlock(); + /* FIXME: aiocontext lock is required because cb calls blk_unref */ + aio_context_acquire(aio_context); job->driver->free(job); + aio_context_release(aio_context); job_lock(); } @@ -XXX,XX +XXX,XX @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)) return; } - real_job_lock(); if (job->busy) { - real_job_unlock(); return; } if (fn && !fn(job)) { - real_job_unlock(); return; } assert(!job->deferred_to_main_loop); timer_del(&job->sleep_timer); job->busy = true; - real_job_unlock(); job_unlock(); aio_co_wake(job->co); job_lock(); @@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns) { AioContext *next_aio_context; - real_job_lock(); if (ns != -1) { timer_mod(&job->sleep_timer, ns); } job->busy = false; job_event_idle_locked(job); - real_job_unlock(); job_unlock(); qemu_coroutine_yield(); job_lock(); @@ -XXX,XX +XXX,XX @@ static void job_clean(Job *job) } } -/* Called with job_mutex held, but releases it temporarily */ +/* + * Called with job_mutex held, but releases it temporarily. + * Takes AioContext lock internally to invoke a job->driver callback. + */ static int job_finalize_single_locked(Job *job) { int job_ret; + AioContext *ctx = job->aio_context; assert(job_is_completed_locked(job)); @@ -XXX,XX +XXX,XX @@ static int job_finalize_single_locked(Job *job) job_ret = job->ret; job_unlock(); + aio_context_acquire(ctx); if (!job_ret) { job_commit(job); @@ -XXX,XX +XXX,XX @@ static int job_finalize_single_locked(Job *job) } job_clean(job); - job_lock(); - if (job->cb) { - job_ret = job->ret; - job_unlock(); job->cb(job->opaque, job_ret); - job_lock(); } + aio_context_release(ctx); + job_lock(); + /* Emit events only if we actually started */ if (job_started_locked(job)) { if (job_is_cancelled_locked(job)) { @@ -XXX,XX +XXX,XX @@ static int job_finalize_single_locked(Job *job) return 0; } -/* Called with job_mutex held, but releases it temporarily */ +/* + * Called with job_mutex held, but releases it temporarily. + * Takes AioContext lock internally to invoke a job->driver callback. + */ static void job_cancel_async_locked(Job *job, bool force) { + AioContext *ctx = job->aio_context; GLOBAL_STATE_CODE(); if (job->driver->cancel) { job_unlock(); + aio_context_acquire(ctx); force = job->driver->cancel(job, force); + aio_context_release(ctx); job_lock(); } else { /* No .cancel() means the job will behave as if force-cancelled */ @@ -XXX,XX +XXX,XX @@ static void job_cancel_async_locked(Job *job, bool force) } } -/* Called with job_mutex held, but releases it temporarily. */ +/* + * Called with job_mutex held, but releases it temporarily. + * Takes AioContext lock internally to invoke a job->driver callback. + */ static void job_completed_txn_abort_locked(Job *job) { - AioContext *ctx; JobTxn *txn = job->txn; Job *other_job; @@ -XXX,XX +XXX,XX @@ static void job_completed_txn_abort_locked(Job *job) txn->aborting = true; job_txn_ref_locked(txn); - /* - * We can only hold the single job's AioContext lock while calling - * job_finalize_single() because the finalization callbacks can involve - * calls of AIO_WAIT_WHILE(), which could deadlock otherwise. - * Note that the job's AioContext may change when it is finalized. - */ job_ref_locked(job); - aio_context_release(job->aio_context); /* Other jobs are effectively cancelled by us, set the status for * them; this job, however, may or may not be cancelled, depending * on the caller, so leave it. */ QLIST_FOREACH(other_job, &txn->jobs, txn_list) { if (other_job != job) { - ctx = other_job->aio_context; - aio_context_acquire(ctx); /* * This is a transaction: If one job failed, no result will matter. * Therefore, pass force=true to terminate all other jobs as quickly * as possible. */ job_cancel_async_locked(other_job, true); - aio_context_release(ctx); } } while (!QLIST_EMPTY(&txn->jobs)) { other_job = QLIST_FIRST(&txn->jobs); - /* - * The job's AioContext may change, so store it in @ctx so we - * release the same context that we have acquired before. - */ - ctx = other_job->aio_context; - aio_context_acquire(ctx); if (!job_is_completed_locked(other_job)) { assert(job_cancel_requested_locked(other_job)); job_finish_sync_locked(other_job, NULL, NULL); } job_finalize_single_locked(other_job); - aio_context_release(ctx); } - /* - * Use job_ref()/job_unref() so we can read the AioContext here - * even if the job went away during job_finalize_single(). - */ - aio_context_acquire(job->aio_context); job_unref_locked(job); - job_txn_unref_locked(txn); } @@ -XXX,XX +XXX,XX @@ static void job_completed_txn_abort_locked(Job *job) static int job_prepare_locked(Job *job) { int ret; + AioContext *ctx = job->aio_context; GLOBAL_STATE_CODE(); + if (job->ret == 0 && job->driver->prepare) { job_unlock(); + aio_context_acquire(ctx); ret = job->driver->prepare(job); + aio_context_release(ctx); job_lock(); job->ret = ret; job_update_rc_locked(job); } + return job->ret; } @@ -XXX,XX +XXX,XX @@ static void job_completed_locked(Job *job) static void job_exit(void *opaque) { Job *job = (Job *)opaque; - AioContext *ctx; JOB_LOCK_GUARD(); - job_ref_locked(job); - aio_context_acquire(job->aio_context); /* This is a lie, we're not quiescent, but still doing the completion * callbacks. However, completion callbacks tend to involve operations that @@ -XXX,XX +XXX,XX @@ static void job_exit(void *opaque) job_event_idle_locked(job); job_completed_locked(job); - - /* - * Note that calling job_completed can move the job to a different - * aio_context, so we cannot cache from above. job_txn_apply takes care of - * acquiring the new lock, and we ref/unref to avoid job_completed freeing - * the job underneath us. - */ - ctx = job->aio_context; job_unref_locked(job); - aio_context_release(ctx); } /** @@ -XXX,XX +XXX,XX @@ int job_cancel_sync(Job *job, bool force) void job_cancel_sync_all(void) { Job *job; - AioContext *aio_context; JOB_LOCK_GUARD(); while ((job = job_next_locked(NULL))) { - aio_context = job->aio_context; - aio_context_acquire(aio_context); job_cancel_sync_locked(job, true); - aio_context_release(aio_context); } } @@ -XXX,XX +XXX,XX @@ int job_finish_sync_locked(Job *job, } job_unlock(); - AIO_WAIT_WHILE(job->aio_context, - (job_enter(job), !job_is_completed(job))); + AIO_WAIT_WHILE_UNLOCKED(job->aio_context, + (job_enter(job), !job_is_completed(job))); job_lock(); ret = (job_is_cancelled_locked(job) && job->ret == 0) 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 void run_block_job(BlockJob *job, Error **errp) AioContext *aio_context = block_job_get_aio_context(job); int ret = 0; - aio_context_acquire(aio_context); job_lock(); job_ref_locked(&job->job); do { @@ -XXX,XX +XXX,XX @@ static void run_block_job(BlockJob *job, Error **errp) } job_unref_locked(&job->job); job_unlock(); - aio_context_release(aio_context); /* publish completion progress only when success */ if (!ret) { diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void test_blockjob_common_drain_node(enum drain_type drain_type, tjob->prepare_ret = -EIO; break; } + aio_context_release(ctx); job_start(&job->job); - aio_context_release(ctx); if (use_iothread) { /* job_co_entry() is run in the I/O thread, wait for the actual job @@ -XXX,XX +XXX,XX @@ static void test_blockjob_common_drain_node(enum drain_type drain_type, g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ } - aio_context_acquire(ctx); WITH_JOB_LOCK_GUARD() { ret = job_complete_sync_locked(&job->job, &error_abort); } g_assert_cmpint(ret, ==, (result == TEST_JOB_SUCCESS ? 0 : -EIO)); + aio_context_acquire(ctx); if (use_iothread) { blk_set_aio_context(blk_src, qemu_get_aio_context(), &error_abort); assert(blk_get_aio_context(blk_target) == qemu_get_aio_context()); diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-block-iothread.c +++ b/tests/unit/test-block-iothread.c @@ -XXX,XX +XXX,XX @@ static void test_attach_blockjob(void) aio_poll(qemu_get_aio_context(), false); } - aio_context_acquire(ctx); WITH_JOB_LOCK_GUARD() { job_complete_sync_locked(&tjob->common.job, &error_abort); } + aio_context_acquire(ctx); blk_set_aio_context(blk, qemu_get_aio_context(), &error_abort); aio_context_release(ctx); diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-blockjob.c +++ b/tests/unit/test-blockjob.c @@ -XXX,XX +XXX,XX @@ static void cancel_common(CancelJob *s) BlockJob *job = &s->common; BlockBackend *blk = s->blk; JobStatus sts = job->job.status; - AioContext *ctx; - - ctx = job->job.aio_context; - aio_context_acquire(ctx); + AioContext *ctx = job->job.aio_context; job_cancel_sync(&job->job, true); WITH_JOB_LOCK_GUARD() { @@ -XXX,XX +XXX,XX @@ static void cancel_common(CancelJob *s) assert(job->job.status == JOB_STATUS_NULL); job_unref_locked(&job->job); } - destroy_blk(blk); + aio_context_acquire(ctx); + destroy_blk(blk); aio_context_release(ctx); + } static void test_cancel_created(void) @@ -XXX,XX +XXX,XX @@ static void test_cancel_concluded(void) aio_poll(qemu_get_aio_context(), true); assert_job_status_is(job, JOB_STATUS_PENDING); - aio_context_acquire(job->aio_context); WITH_JOB_LOCK_GUARD() { job_finalize_locked(job, &error_abort); + assert(job->status == JOB_STATUS_CONCLUDED); } - aio_context_release(job->aio_context); - assert_job_status_is(job, JOB_STATUS_CONCLUDED); cancel_common(s); } @@ -XXX,XX +XXX,XX @@ static void test_complete_in_standby(void) /* Wait for the job to become READY */ job_start(job); - aio_context_acquire(ctx); /* * Here we are waiting for the status to change, so don't bother * protecting the read every time. */ - AIO_WAIT_WHILE(ctx, job->status != JOB_STATUS_READY); - aio_context_release(ctx); + AIO_WAIT_WHILE_UNLOCKED(ctx, job->status != JOB_STATUS_READY); /* Begin the drained section, pausing the job */ bdrv_drain_all_begin(); @@ -XXX,XX +XXX,XX @@ static void test_complete_in_standby(void) aio_context_acquire(ctx); /* This will schedule the job to resume it */ bdrv_drain_all_end(); + aio_context_release(ctx); WITH_JOB_LOCK_GUARD() { /* But the job cannot run, so it will remain on standby */ @@ -XXX,XX +XXX,XX @@ static void test_complete_in_standby(void) job_dismiss_locked(&job, &error_abort); } + aio_context_acquire(ctx); destroy_blk(blk); aio_context_release(ctx); iothread_join(iothread); -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Not sure what the atomic here was supposed to do, since job.busy is protected by the job lock. Since the whole function is called under job_mutex, just remove the atomic. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20220926093214.506243-20-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockjob.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) info = g_new0(BlockJobInfo, 1); info->type = g_strdup(job_type_str(&job->job)); info->device = g_strdup(job->job.id); - info->busy = qatomic_read(&job->job.busy); + info->busy = job->job.busy; info->paused = job->job.pause_count > 0; info->offset = progress_current; info->len = progress_total; -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> These public functions are not used anywhere, thus can be dropped. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20220926093214.506243-21-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/blockjob.h | 31 ++++++++++++------------------- blockjob.c | 16 ++-------------- 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/include/block/blockjob.h b/include/block/blockjob.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -XXX,XX +XXX,XX @@ typedef struct BlockJob { */ /** - * block_job_next: + * block_job_next_locked: * @job: A block job, or %NULL. * * Get the next element from the list of block jobs after @job, or the * first one if @job is %NULL. * * Returns the requested job, or %NULL if there are no more jobs left. + * Called with job lock held. */ -BlockJob *block_job_next(BlockJob *job); - -/* Same as block_job_next(), but called with job lock held. */ BlockJob *block_job_next_locked(BlockJob *job); /** @@ -XXX,XX +XXX,XX @@ BlockJob *block_job_next_locked(BlockJob *job); * Get the block job identified by @id (which must not be %NULL). * * Returns the requested job, or %NULL if it doesn't exist. + * Called with job lock *not* held. */ BlockJob *block_job_get(const char *id); @@ -XXX,XX +XXX,XX @@ void block_job_remove_all_bdrv(BlockJob *job); bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs); /** - * block_job_set_speed: + * block_job_set_speed_locked: * @job: The job to set the speed for. * @speed: The new value * @errp: Error object. * * Set a rate-limiting parameter for the job; the actual meaning may * vary depending on the job type. - */ -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); - -/* - * Same as block_job_set_speed(), but called with job lock held. - * Might release the lock temporarily. + * + * Called with job lock held, but might release it temporarily. */ bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp); /** - * block_job_query: + * block_job_query_locked: * @job: The job to get information about. * * Return information about a job. + * + * Called with job lock held. */ -BlockJobInfo *block_job_query(BlockJob *job, Error **errp); - -/* Same as block_job_query(), but called with job lock held. */ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp); /** - * block_job_iostatus_reset: + * block_job_iostatus_reset_locked: * @job: The job whose I/O status should be reset. * * Reset I/O status on @job and on BlockDriverState objects it uses, * other than job->blk. + * + * Called with job lock held. */ -void block_job_iostatus_reset(BlockJob *job); - -/* Same as block_job_iostatus_reset(), but called with job lock held. */ void block_job_iostatus_reset_locked(BlockJob *job); /* diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ BlockJob *block_job_next_locked(BlockJob *bjob) return job ? container_of(job, BlockJob, job) : NULL; } -BlockJob *block_job_next(BlockJob *bjob) -{ - JOB_LOCK_GUARD(); - return block_job_next_locked(bjob); -} - BlockJob *block_job_get_locked(const char *id) { Job *job = job_get_locked(id); @@ -XXX,XX +XXX,XX @@ bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp) return true; } -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) +static bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) { JOB_LOCK_GUARD(); return block_job_set_speed_locked(job, speed, errp); @@ -XXX,XX +XXX,XX @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) return info; } -BlockJobInfo *block_job_query(BlockJob *job, Error **errp) -{ - JOB_LOCK_GUARD(); - return block_job_query_locked(job, errp); -} - /* Called with job lock held */ static void block_job_iostatus_set_err_locked(BlockJob *job, int error) { @@ -XXX,XX +XXX,XX @@ void block_job_iostatus_reset_locked(BlockJob *job) job->iostatus = BLOCK_DEVICE_IO_STATUS_OK; } -void block_job_iostatus_reset(BlockJob *job) +static void block_job_iostatus_reset(BlockJob *job) { JOB_LOCK_GUARD(); block_job_iostatus_reset_locked(job); -- 2.37.3 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> These public functions are not used anywhere, thus can be dropped. Also, since this is the final job API that doesn't use AioContext lock and replaces it with job_lock, adjust all remaining function documentation to clearly specify if the job lock is taken or not. Also document the locking requirements for a few functions where the second version is not removed. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20220926093214.506243-22-eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 110 +++++++++++++------------------------ job.c | 107 ++---------------------------------- tests/unit/test-blockjob.c | 4 +- 3 files changed, 46 insertions(+), 175 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ JobTxn *job_txn_new(void); /** * Release a reference that was previously acquired with job_txn_add_job or * job_txn_new. If it's the last reference to the object, it will be freed. + * + * Called with job lock *not* held. */ void job_txn_unref(JobTxn *txn); @@ -XXX,XX +XXX,XX @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, /** * Add a reference to Job refcnt, it will be decreased with job_unref, and then * be freed if it comes to be the last reference. + * + * Called with job lock held. */ -void job_ref(Job *job); - -/* Same as job_ref(), but called with job lock held. */ void job_ref_locked(Job *job); /** - * Release a reference that was previously acquired with job_ref() or + * Release a reference that was previously acquired with job_ref_locked() or * job_create(). If it's the last reference to the object, it will be freed. * * Takes AioContext lock internally to invoke a job->driver callback. + * Called with job lock held. */ -void job_unref(Job *job); - -/* Same as job_unref(), but called with job lock held. */ void job_unref_locked(Job *job); /** @@ -XXX,XX +XXX,XX @@ void job_progress_increase_remaining(Job *job, uint64_t delta); * Conditionally enter the job coroutine if the job is ready to run, not * already busy and fn() returns true. fn() is called while under the job_lock * critical section. - */ -void job_enter_cond(Job *job, bool(*fn)(Job *job)); - -/* - * Same as job_enter_cond(), but called with job lock held. - * Might release the lock temporarily. + * + * Called with job lock held, but might release it temporarily. */ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)); @@ -XXX,XX +XXX,XX @@ bool job_cancel_requested(Job *job); /** * Returns whether the job is in a completed state. - * Called with job_mutex *not* held. + * Called with job lock held. */ -bool job_is_completed(Job *job); - -/* Same as job_is_completed(), but called with job lock held. */ bool job_is_completed_locked(Job *job); /** @@ -XXX,XX +XXX,XX @@ bool job_is_ready_locked(Job *job); /** * Request @job to pause at the next pause point. Must be paired with * job_resume(). If the job is supposed to be resumed by user action, call - * job_user_pause() instead. + * job_user_pause_locked() instead. + * + * Called with job lock *not* held. */ void job_pause(Job *job); /* Same as job_pause(), but called with job lock held. */ void job_pause_locked(Job *job); -/** Resumes a @job paused with job_pause. */ +/** Resumes a @job paused with job_pause. Called with job lock *not* held. */ void job_resume(Job *job); /* @@ -XXX,XX +XXX,XX @@ void job_resume_locked(Job *job); /** * Asynchronously pause the specified @job. * Do not allow a resume until a matching call to job_user_resume. + * Called with job lock held. */ -void job_user_pause(Job *job, Error **errp); - -/* Same as job_user_pause(), but called with job lock held. */ void job_user_pause_locked(Job *job, Error **errp); -/** Returns true if the job is user-paused. */ -bool job_user_paused(Job *job); - -/* Same as job_user_paused(), but called with job lock held. */ +/** + * Returns true if the job is user-paused. + * Called with job lock held. + */ bool job_user_paused_locked(Job *job); /** * Resume the specified @job. - * Must be paired with a preceding job_user_pause. - */ -void job_user_resume(Job *job, Error **errp); - -/* - * Same as job_user_resume(), but called with job lock held. - * Might release the lock temporarily. + * Must be paired with a preceding job_user_pause_locked. + * Called with job lock held, but might release it temporarily. */ void job_user_resume_locked(Job *job, Error **errp); @@ -XXX,XX +XXX,XX @@ void job_user_resume_locked(Job *job, Error **errp); * first one if @job is %NULL. * * Returns the requested job, or %NULL if there are no more jobs left. + * Called with job lock *not* held. */ Job *job_next(Job *job); @@ -XXX,XX +XXX,XX @@ Job *job_next_locked(Job *job); * Get the job identified by @id (which must not be %NULL). * * Returns the requested job, or %NULL if it doesn't exist. + * Called with job lock held. */ -Job *job_get(const char *id); - -/* Same as job_get(), but called with job lock held. */ Job *job_get_locked(const char *id); /** * Check whether the verb @verb can be applied to @job in its current state. * Returns 0 if the verb can be applied; otherwise errp is set and -EPERM * returned. + * + * Called with job lock held. */ -int job_apply_verb(Job *job, JobVerb verb, Error **errp); - -/* Same as job_apply_verb, but called with job lock held. */ int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp); /** @@ -XXX,XX +XXX,XX @@ void job_early_fail(Job *job); */ void job_transition_to_ready(Job *job); -/** Asynchronously complete the specified @job. */ -void job_complete(Job *job, Error **errp); - -/* - * Same as job_complete(), but called with job lock held. - * Might release the lock temporarily. +/** + * Asynchronously complete the specified @job. + * Called with job lock held, but might release it temporarily. */ void job_complete_locked(Job *job, Error **errp); /** * Asynchronously cancel the specified @job. If @force is true, the job should * be cancelled immediately without waiting for a consistent state. + * Called with job lock held. */ -void job_cancel(Job *job, bool force); - -/* Same as job_cancel(), but called with job lock held. */ void job_cancel_locked(Job *job, bool force); /** - * Cancels the specified job like job_cancel(), but may refuse to do so if the - * operation isn't meaningful in the current state of the job. + * Cancels the specified job like job_cancel_locked(), but may refuse + * to do so if the operation isn't meaningful in the current state of the job. + * Called with job lock held. */ -void job_user_cancel(Job *job, bool force, Error **errp); - -/* Same as job_user_cancel(), but called with job lock held. */ void job_user_cancel_locked(Job *job, bool force, Error **errp); /** @@ -XXX,XX +XXX,XX @@ void job_cancel_sync_all(void); /** * @job: The job to be completed. - * @errp: Error object which may be set by job_complete(); this is not + * @errp: Error object which may be set by job_complete_locked(); this is not * necessarily set on every error, the job return value has to be * checked as well. * @@ -XXX,XX +XXX,XX @@ void job_cancel_sync_all(void); * function). * * Returns the return value from the job. - * Called with job_lock *not* held. + * Called with job_lock held. */ -int job_complete_sync(Job *job, Error **errp); - -/* Same as job_complete_sync, but called with job lock held. */ int job_complete_sync_locked(Job *job, Error **errp); /** @@ -XXX,XX +XXX,XX @@ int job_complete_sync_locked(Job *job, Error **errp); * FIXME: Make the below statement universally true: * For jobs that support the manual workflow mode, all graph changes that occur * as a result will occur after this command and before a successful reply. + * + * Called with job lock held. */ -void job_finalize(Job *job, Error **errp); - -/* Same as job_finalize(), but called with job lock held. */ void job_finalize_locked(Job *job, Error **errp); /** * Remove the concluded @job from the query list and resets the passed pointer * to %NULL. Returns an error if the job is not actually concluded. + * + * Called with job lock held. */ -void job_dismiss(Job **job, Error **errp); - -/* Same as job_dismiss(), but called with job lock held. */ void job_dismiss_locked(Job **job, Error **errp); /** @@ -XXX,XX +XXX,XX @@ void job_dismiss_locked(Job **job, Error **errp); * Returns 0 if the job is successfully completed, -ECANCELED if the job was * cancelled before completing, and -errno in other error cases. * - * Called with job_lock *not* held. - */ -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), - Error **errp); - -/* - * Same as job_finish_sync(), but called with job lock held. - * Might release the lock temporarily. + * Called with job_lock held, but might release it temporarily. */ int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), Error **errp); diff --git a/job.c b/job.c index XXXXXXX..XXXXXXX 100644 --- a/job.c +++ b/job.c @@ -XXX,XX +XXX,XX @@ int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp) return -EPERM; } -int job_apply_verb(Job *job, JobVerb verb, Error **errp) -{ - JOB_LOCK_GUARD(); - return job_apply_verb_locked(job, verb, errp); -} - JobType job_type(const Job *job) { return job->driver->job_type; @@ -XXX,XX +XXX,XX @@ bool job_is_completed_locked(Job *job) return false; } -bool job_is_completed(Job *job) +static bool job_is_completed(Job *job) { JOB_LOCK_GUARD(); return job_is_completed_locked(job); @@ -XXX,XX +XXX,XX @@ Job *job_get_locked(const char *id) return NULL; } -Job *job_get(const char *id) -{ - JOB_LOCK_GUARD(); - return job_get_locked(id); -} - void job_set_aio_context(Job *job, AioContext *ctx) { /* protect against read in job_finish_sync_locked and job_start */ @@ -XXX,XX +XXX,XX @@ void job_ref_locked(Job *job) ++job->refcnt; } -void job_ref(Job *job) -{ - JOB_LOCK_GUARD(); - job_ref_locked(job); -} - void job_unref_locked(Job *job) { GLOBAL_STATE_CODE(); @@ -XXX,XX +XXX,XX @@ void job_unref_locked(Job *job) } } -void job_unref(Job *job) -{ - JOB_LOCK_GUARD(); - job_unref_locked(job); -} - void job_progress_update(Job *job, uint64_t done) { progress_work_done(&job->progress, done); @@ -XXX,XX +XXX,XX @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)) job_lock(); } -void job_enter_cond(Job *job, bool(*fn)(Job *job)) -{ - JOB_LOCK_GUARD(); - job_enter_cond_locked(job, fn); -} - void job_enter(Job *job) { JOB_LOCK_GUARD(); @@ -XXX,XX +XXX,XX @@ void coroutine_fn job_pause_point(Job *job) job_pause_point_locked(job); } -static void coroutine_fn job_yield_locked(Job *job) +void coroutine_fn job_yield(Job *job) { + JOB_LOCK_GUARD(); assert(job->busy); /* Check cancellation *before* setting busy = false, too! */ @@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_yield_locked(Job *job) job_pause_point_locked(job); } -void coroutine_fn job_yield(Job *job) -{ - JOB_LOCK_GUARD(); - job_yield_locked(job); -} - void coroutine_fn job_sleep_ns(Job *job, int64_t ns) { JOB_LOCK_GUARD(); @@ -XXX,XX +XXX,XX @@ void job_user_pause_locked(Job *job, Error **errp) job_pause_locked(job); } -void job_user_pause(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - job_user_pause_locked(job, errp); -} - bool job_user_paused_locked(Job *job) { return job->user_paused; } -bool job_user_paused(Job *job) -{ - JOB_LOCK_GUARD(); - return job_user_paused_locked(job); -} - void job_user_resume_locked(Job *job, Error **errp) { assert(job); @@ -XXX,XX +XXX,XX @@ void job_user_resume_locked(Job *job, Error **errp) job_resume_locked(job); } -void job_user_resume(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - job_user_resume_locked(job, errp); -} - /* Called with job_mutex held, but releases it temporarily. */ static void job_do_dismiss_locked(Job *job) { @@ -XXX,XX +XXX,XX @@ void job_dismiss_locked(Job **jobptr, Error **errp) *jobptr = NULL; } -void job_dismiss(Job **jobptr, Error **errp) -{ - JOB_LOCK_GUARD(); - job_dismiss_locked(jobptr, errp); -} - void job_early_fail(Job *job) { JOB_LOCK_GUARD(); @@ -XXX,XX +XXX,XX @@ void job_finalize_locked(Job *job, Error **errp) job_do_finalize_locked(job); } -void job_finalize(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - job_finalize_locked(job, errp); -} - /* Called with job_mutex held. */ static int job_transition_to_pending_locked(Job *job) { @@ -XXX,XX +XXX,XX @@ void job_cancel_locked(Job *job, bool force) } } -void job_cancel(Job *job, bool force) -{ - JOB_LOCK_GUARD(); - job_cancel_locked(job, force); -} - void job_user_cancel_locked(Job *job, bool force, Error **errp) { if (job_apply_verb_locked(job, JOB_VERB_CANCEL, errp)) { @@ -XXX,XX +XXX,XX @@ void job_user_cancel_locked(Job *job, bool force, Error **errp) job_cancel_locked(job, force); } -void job_user_cancel(Job *job, bool force, Error **errp) -{ - JOB_LOCK_GUARD(); - job_user_cancel_locked(job, force, errp); -} - -/* A wrapper around job_cancel() taking an Error ** parameter so it may be - * used with job_finish_sync() without the need for (rather nasty) function - * pointer casts there. +/* A wrapper around job_cancel_locked() taking an Error ** parameter so it may + * be used with job_finish_sync_locked() without the need for (rather nasty) + * function pointer casts there. * * Called with job_mutex held. */ @@ -XXX,XX +XXX,XX @@ int job_complete_sync_locked(Job *job, Error **errp) return job_finish_sync_locked(job, job_complete_locked, errp); } -int job_complete_sync(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - return job_complete_sync_locked(job, errp); -} - void job_complete_locked(Job *job, Error **errp) { /* Should not be reachable via external interface for internal jobs */ @@ -XXX,XX +XXX,XX @@ void job_complete_locked(Job *job, Error **errp) job_lock(); } -void job_complete(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - job_complete_locked(job, errp); -} - int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), Error **errp) @@ -XXX,XX +XXX,XX @@ int job_finish_sync_locked(Job *job, job_unref_locked(job); return ret; } - -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp) -{ - JOB_LOCK_GUARD(); - return job_finish_sync_locked(job, finish, errp); -} diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-blockjob.c +++ b/tests/unit/test-blockjob.c @@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_yielding_driver = { }; /* - * Test that job_complete() works even on jobs that are in a paused + * Test that job_complete_locked() works even on jobs that are in a paused * state (i.e., STANDBY). * * To do this, run YieldingJob in an IO thread, get it into the READY @@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_yielding_driver = { * acquire the context so the job will not be entered and will thus * remain on STANDBY. * - * job_complete() should still work without error. + * job_complete_locked() should still work without error. * * Note that on the QMP interface, it is impossible to lock an IO * thread before a drained section ends. In practice, the -- 2.37.3 The field is unused (only ever set, but never read) since commit ac9185603. Additionally, the commit message of commit 34fa110e already explained earlier why it's unreliable. Remove it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20220923142838.91043-1-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/file-posix.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/block/file-posix.c b/block/file-posix.c index XXXXXXX..XXXXXXX 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -XXX,XX +XXX,XX @@ typedef struct BDRVRawState { bool has_discard:1; bool has_write_zeroes:1; - bool discard_zeroes:1; bool use_linux_aio:1; bool use_linux_io_uring:1; int page_cache_inconsistent; /* errno from fdatasync failure */ @@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options, ret = -EINVAL; goto fail; } else { - s->discard_zeroes = true; s->has_fallocate = true; } } else { @@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options, } if (S_ISBLK(st.st_mode)) { -#ifdef BLKDISCARDZEROES - unsigned int arg; - if (ioctl(s->fd, BLKDISCARDZEROES, &arg) == 0 && arg) { - s->discard_zeroes = true; - } -#endif #ifdef __linux__ /* On Linux 3.10, BLKDISCARD leaves stale data in the page cache. Do * not rely on the contents of discarded blocks unless using O_DIRECT. * Same for BLKZEROOUT. */ if (!(bs->open_flags & BDRV_O_NOCACHE)) { - s->discard_zeroes = false; s->has_write_zeroes = false; } #endif -- 2.37.3 The following changes since commit ba49d760eb04630e7b15f423ebecf6c871b8f77b: Merge tag 'pull-maintainer-final-130324-1' of https://gitlab.com/stsquad/qemu into staging (2024-03-13 15:12:14 +0000) are available in the Git repository at: https://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 39a94d7c34ce9d222fa9c0c99a14e20a567456d7: iotests: adapt to output change for recently introduced 'detached header' field (2024-03-18 13:33:54 +0100) ---------------------------------------------------------------- Block layer patches - mirror: Fix deadlock - nbd/server: Fix race in draining the export - qemu-img snapshot: Fix formatting with large values - Fix blockdev-snapshot-sync error reporting for no medium - iotests fixes ---------------------------------------------------------------- Abhiram Tilak (1): qemu-img: Fix Column Width and Improve Formatting in snapshot list Fiona Ebner (1): iotests: adapt to output change for recently introduced 'detached header' field Kevin Wolf (3): mirror: Don't call job_pause_point() under graph lock nbd/server: Fix race in draining the export iotests: Add test for reset/AioContext switches with NBD exports Markus Armbruster (1): blockdev: Fix blockdev-snapshot-sync error reporting for no medium Thomas Huth (9): tests/qemu-iotests: Fix test 033 for running with non-file protocols tests/qemu-iotests: Restrict test 066 to the 'file' protocol tests/qemu-iotests: Restrict test 114 to the 'file' protocol tests/qemu-iotests: Restrict test 130 to the 'file' protocol tests/qemu-iotests: Restrict test 134 and 158 to the 'file' protocol tests/qemu-iotests: Restrict test 156 to the 'file' protocol tests/qemu-iotests: Restrict tests that use --image-opts to the 'file' protocol tests/qemu-iotests: Fix some tests that use --image-opts for other protocols tests/qemu-iotests: Restrict tests using "--blockdev file" to the file protocol include/qemu/job.h | 2 +- block/mirror.c | 10 ++-- block/qapi.c | 10 ++-- blockdev.c | 3 +- nbd/server.c | 15 +++-- tests/qemu-iotests/033 | 6 +- tests/qemu-iotests/066 | 2 +- tests/qemu-iotests/114 | 2 +- tests/qemu-iotests/130 | 2 +- tests/qemu-iotests/134 | 2 +- tests/qemu-iotests/156 | 2 +- tests/qemu-iotests/158 | 2 +- tests/qemu-iotests/176.out | 16 +++--- tests/qemu-iotests/188 | 2 +- tests/qemu-iotests/189 | 2 +- tests/qemu-iotests/198 | 2 +- tests/qemu-iotests/198.out | 2 + tests/qemu-iotests/206.out | 1 + tests/qemu-iotests/261 | 4 +- tests/qemu-iotests/263 | 6 +- tests/qemu-iotests/267.out | 48 ++++++++-------- tests/qemu-iotests/284 | 7 +-- tests/qemu-iotests/286 | 3 +- tests/qemu-iotests/286.out | 2 +- .../tests/detect-zeroes-registered-buf | 4 +- tests/qemu-iotests/tests/iothreads-nbd-export | 66 ++++++++++++++++++++++ tests/qemu-iotests/tests/iothreads-nbd-export.out | 19 +++++++ tests/qemu-iotests/tests/qcow2-internal-snapshots | 2 +- .../tests/qcow2-internal-snapshots.out | 14 ++--- tests/qemu-iotests/tests/qsd-jobs | 2 +- 30 files changed, 178 insertions(+), 82 deletions(-) create mode 100755 tests/qemu-iotests/tests/iothreads-nbd-export create mode 100644 tests/qemu-iotests/tests/iothreads-nbd-export.out Calling job_pause_point() while holding the graph reader lock potentially results in a deadlock: bdrv_graph_wrlock() first drains everything, including the mirror job, which pauses it. The job is only unpaused at the end of the drain section, which is when the graph writer lock has been successfully taken. However, if the job happens to be paused at a pause point where it still holds the reader lock, the writer lock can't be taken as long as the job is still paused. Mark job_pause_point() as GRAPH_UNLOCKED and fix mirror accordingly. Cc: qemu-stable@nongnu.org Buglink: https://issues.redhat.com/browse/RHEL-28125 Fixes: 004915a96a7a ("block: Protect bs->backing with graph_lock") Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20240313153000.33121-1-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/job.h | 2 +- block/mirror.c | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -XXX,XX +XXX,XX @@ void job_enter(Job *job); * * Called with job_mutex *not* held. */ -void coroutine_fn job_pause_point(Job *job); +void coroutine_fn GRAPH_UNLOCKED job_pause_point(Job *job); /** * @job: The job that calls the function. diff --git a/block/mirror.c b/block/mirror.c index XXXXXXX..XXXXXXX 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -XXX,XX +XXX,XX @@ static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset, return bytes_handled; } -static void coroutine_fn GRAPH_RDLOCK mirror_iteration(MirrorBlockJob *s) +static void coroutine_fn GRAPH_UNLOCKED mirror_iteration(MirrorBlockJob *s) { - BlockDriverState *source = s->mirror_top_bs->backing->bs; + BlockDriverState *source; MirrorOp *pseudo_op; int64_t offset; /* At least the first dirty chunk is mirrored in one iteration. */ @@ -XXX,XX +XXX,XX @@ static void coroutine_fn GRAPH_RDLOCK mirror_iteration(MirrorBlockJob *s) bool write_zeroes_ok = bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)); int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES); + bdrv_graph_co_rdlock(); + source = s->mirror_top_bs->backing->bs; + bdrv_graph_co_rdunlock(); + bdrv_dirty_bitmap_lock(s->dirty_bitmap); offset = bdrv_dirty_iter_next(s->dbi); if (offset < 0) { @@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_run(Job *job, Error **errp) mirror_wait_for_free_in_flight_slot(s); continue; } else if (cnt != 0) { - bdrv_graph_co_rdlock(); mirror_iteration(s); - bdrv_graph_co_rdunlock(); } } -- 2.44.0 When draining an NBD export, nbd_drained_begin() first sets client->quiescing so that nbd_client_receive_next_request() won't start any new request coroutines. Then nbd_drained_poll() tries to makes sure that we wait for any existing request coroutines by checking that client->nb_requests has become 0. However, there is a small window between creating a new request coroutine and increasing client->nb_requests. If a coroutine is in this state, it won't be waited for and drain returns too early. In the context of switching to a different AioContext, this means that blk_aio_attached() will see client->recv_coroutine != NULL and fail its assertion. Fix this by increasing client->nb_requests immediately when starting the coroutine. Doing this after the checks if we should create a new coroutine is okay because client->lock is held. Cc: qemu-stable@nongnu.org Fixes: fd6afc501a01 ("nbd/server: Use drained block ops to quiesce the server") Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20240314165825.40261-2-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- nbd/server.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 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 coroutine_fn int nbd_handle_request(NBDClient *client, /* Owns a reference to the NBDClient passed as opaque. */ static coroutine_fn void nbd_trip(void *opaque) { - NBDClient *client = opaque; - NBDRequestData *req = NULL; + NBDRequestData *req = opaque; + NBDClient *client = req->client; NBDRequest request = { 0 }; /* GCC thinks it can be used uninitialized */ int ret; Error *local_err = NULL; @@ -XXX,XX +XXX,XX @@ static coroutine_fn void nbd_trip(void *opaque) goto done; } - req = nbd_request_get(client); - /* * nbd_co_receive_request() returns -EAGAIN when nbd_drained_begin() has * set client->quiescing but by the time we get back nbd_drained_end() may @@ -XXX,XX +XXX,XX @@ static coroutine_fn void nbd_trip(void *opaque) } done: - if (req) { - nbd_request_put(req); - } + nbd_request_put(req); qemu_mutex_unlock(&client->lock); @@ -XXX,XX +XXX,XX @@ disconnect: */ static void nbd_client_receive_next_request(NBDClient *client) { + NBDRequestData *req; + if (!client->recv_coroutine && client->nb_requests < MAX_NBD_REQUESTS && !client->quiescing) { nbd_client_get(client); - client->recv_coroutine = qemu_coroutine_create(nbd_trip, client); + req = nbd_request_get(client); + client->recv_coroutine = qemu_coroutine_create(nbd_trip, req); aio_co_schedule(client->exp->common.ctx, client->recv_coroutine); } } -- 2.44.0 This replicates the scenario in which the bug was reported. Unfortunately this relies on actually executing a guest (so that the firmware initialises the virtio-blk device and moves it to its configured iothread), so this can't make use of the qtest accelerator like most other test cases. I tried to find a different easy way to trigger the bug, but couldn't find one. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20240314165825.40261-3-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/tests/iothreads-nbd-export | 66 +++++++++++++++++++ .../tests/iothreads-nbd-export.out | 19 ++++++ 2 files changed, 85 insertions(+) create mode 100755 tests/qemu-iotests/tests/iothreads-nbd-export create mode 100644 tests/qemu-iotests/tests/iothreads-nbd-export.out diff --git a/tests/qemu-iotests/tests/iothreads-nbd-export b/tests/qemu-iotests/tests/iothreads-nbd-export new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/tests/iothreads-nbd-export @@ -XXX,XX +XXX,XX @@ +#!/usr/bin/env python3 +# group: rw quick +# +# Copyright (C) 2024 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# +# Creator/Owner: Kevin Wolf <kwolf@redhat.com> + +import time +import qemu +import iotests + +iotests.script_initialize(supported_fmts=['qcow2'], + supported_platforms=['linux']) + +with iotests.FilePath('disk1.img') as path, \ + iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock, \ + qemu.machine.QEMUMachine(iotests.qemu_prog) as vm: + + img_size = '10M' + + iotests.log('Preparing disk...') + iotests.qemu_img_create('-f', iotests.imgfmt, path, img_size) + vm.add_args('-blockdev', f'file,node-name=disk-file,filename={path}') + vm.add_args('-blockdev', 'qcow2,node-name=disk,file=disk-file') + vm.add_args('-object', 'iothread,id=iothread0') + vm.add_args('-device', + 'virtio-blk,drive=disk,iothread=iothread0,share-rw=on') + + iotests.log('Launching VM...') + vm.add_args('-accel', 'kvm', '-accel', 'tcg') + #vm.add_args('-accel', 'qtest') + vm.launch() + + iotests.log('Exporting to NBD...') + iotests.log(vm.qmp('nbd-server-start', + addr={'type': 'unix', 'data': {'path': nbd_sock}})) + iotests.log(vm.qmp('block-export-add', type='nbd', id='exp0', + node_name='disk', writable=True)) + + iotests.log('Connecting qemu-img...') + qemu_io = iotests.QemuIoInteractive('-f', 'raw', + f'nbd+unix:///disk?socket={nbd_sock}') + + iotests.log('Moving the NBD export to a different iothread...') + for i in range(0, 10): + iotests.log(vm.qmp('system_reset')) + time.sleep(0.1) + + iotests.log('Checking that it is still alive...') + iotests.log(vm.qmp('query-status')) + + qemu_io.close() + vm.shutdown() diff --git a/tests/qemu-iotests/tests/iothreads-nbd-export.out b/tests/qemu-iotests/tests/iothreads-nbd-export.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/tests/iothreads-nbd-export.out @@ -XXX,XX +XXX,XX @@ +Preparing disk... +Launching VM... +Exporting to NBD... +{"return": {}} +{"return": {}} +Connecting qemu-img... +Moving the NBD export to a different iothread... +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +Checking that it is still alive... +{"return": {"running": true, "status": "running"}} -- 2.44.0 From: Markus Armbruster <armbru@redhat.com> When external_snapshot_abort() rejects a BlockDriverState without a medium, it creates an error like this: error_setg(errp, "Device '%s' has no medium", device); Trouble is @device can be null. My system formats null as "(null)", but other systems might crash. Reproducer: 1. Create a block device without a medium -> {"execute": "blockdev-add", "arguments": {"driver": "host_cdrom", "node-name": "blk0", "filename": "/dev/sr0"}} <- {"return": {}} 3. Attempt to snapshot it -> {"execute":"blockdev-snapshot-sync", "arguments": { "node-name": "blk0", "snapshot-file":"/tmp/foo.qcow2","format":"qcow2"}} <- {"error": {"class": "GenericError", "desc": "Device '(null)' has no medium"}} Broken when commit 0901f67ecdb made @device optional. Use bdrv_get_device_or_node_name() instead. Now it fails as it should: <- {"error": {"class": "GenericError", "desc": "Device 'blk0' has no medium"}} Fixes: 0901f67ecdb7 ("qmp: Allow to take external snapshots on bs graphs node.") Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-ID: <20240306142831.2514431-1-armbru@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ static void external_snapshot_action(TransactionAction *action, bdrv_drained_begin(state->old_bs); if (!bdrv_is_inserted(state->old_bs)) { - error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); + error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, + bdrv_get_device_or_node_name(state->old_bs)); return; } -- 2.44.0 From: Abhiram Tilak <atp.exp@gmail.com> When running the command `qemu-img snapshot -l SNAPSHOT` the output of VM_CLOCK (measures the offset between host and VM clock) cannot to accommodate values in the order of thousands (4-digit). This line [1] hints on the problem. Additionally, the column width for the VM_CLOCK field was reduced from 15 to 13 spaces in commit b39847a5 in line [2], resulting in a shortage of space. [1]: https://gitlab.com/qemu-project/qemu/-/blob/master/block/qapi.c?ref_type=heads#L753 [2]: https://gitlab.com/qemu-project/qemu/-/blob/master/block/qapi.c?ref_type=heads#L763 This patch restores the column width to 15 spaces and makes adjustments to the affected iotests accordingly. Furthermore, addresses a potential source of confusion by removing whitespace in column headers. Example, VM CLOCK is modified to VM_CLOCK. Additionally a '--' symbol is introduced when ICOUNT returns no output for clarity. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2062 Fixes: b39847a50553 ("migration: introduce icount field for snapshots") Signed-off-by: Abhiram Tilak <atp.exp@gmail.com> Message-ID: <20240123050354.22152-2-atp.exp@gmail.com> [kwolf: Fixed up qemu-iotests 261 and 286] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qapi.c | 10 ++-- tests/qemu-iotests/176.out | 16 +++---- tests/qemu-iotests/261 | 4 +- tests/qemu-iotests/267.out | 48 +++++++++---------- tests/qemu-iotests/286 | 3 +- tests/qemu-iotests/286.out | 2 +- .../tests/qcow2-internal-snapshots.out | 14 +++--- 7 files changed, 50 insertions(+), 47 deletions(-) diff --git a/block/qapi.c b/block/qapi.c index XXXXXXX..XXXXXXX 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -XXX,XX +XXX,XX @@ void bdrv_snapshot_dump(QEMUSnapshotInfo *sn) char *sizing = NULL; if (!sn) { - qemu_printf("%-10s%-17s%8s%20s%13s%11s", - "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK", "ICOUNT"); + qemu_printf("%-7s %-16s %8s %19s %15s %10s", + "ID", "TAG", "VM_SIZE", "DATE", "VM_CLOCK", "ICOUNT"); } else { g_autoptr(GDateTime) date = g_date_time_new_from_unix_local(sn->date_sec); g_autofree char *date_buf = g_date_time_format(date, "%Y-%m-%d %H:%M:%S"); secs = sn->vm_clock_nsec / 1000000000; snprintf(clock_buf, sizeof(clock_buf), - "%02d:%02d:%02d.%03d", + "%04d:%02d:%02d.%03d", (int)(secs / 3600), (int)((secs / 60) % 60), (int)(secs % 60), @@ -XXX,XX +XXX,XX @@ void bdrv_snapshot_dump(QEMUSnapshotInfo *sn) if (sn->icount != -1ULL) { snprintf(icount_buf, sizeof(icount_buf), "%"PRId64, sn->icount); + } else { + snprintf(icount_buf, sizeof(icount_buf), "--"); } - qemu_printf("%-9s %-16s %8s%20s%13s%11s", + qemu_printf("%-7s %-16s %8s %19s %15s %10s", sn->id_str, sn->name, sizing, date_buf, diff --git a/tests/qemu-iotests/176.out b/tests/qemu-iotests/176.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/176.out +++ b/tests/qemu-iotests/176.out @@ -XXX,XX +XXX,XX @@ Offset Length File 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd 0x83400000 0x200 TEST_DIR/t.IMGFMT.itmd Snapshot list: -ID TAG -1 snap +ID TAG +1 snap === Test pass snapshot.1 === @@ -XXX,XX +XXX,XX @@ Offset Length File 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT 0x83400000 0x200 TEST_DIR/t.IMGFMT Snapshot list: -ID TAG -1 snap +ID TAG +1 snap === Test pass snapshot.2 === @@ -XXX,XX +XXX,XX @@ Offset Length File 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT 0x83400000 0x200 TEST_DIR/t.IMGFMT Snapshot list: -ID TAG -1 snap +ID TAG +1 snap === Test pass snapshot.3 === @@ -XXX,XX +XXX,XX @@ Offset Length File 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT 0x83400000 0x200 TEST_DIR/t.IMGFMT Snapshot list: -ID TAG -1 snap +ID TAG +1 snap === Test pass bitmap.0 === diff --git a/tests/qemu-iotests/261 b/tests/qemu-iotests/261 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/261 +++ b/tests/qemu-iotests/261 @@ -XXX,XX +XXX,XX @@ _check_test_img -r all echo echo "$((sn_count - 1)) snapshots should remain:" -echo " qemu-img info reports $(_img_info | grep -c '^ \{32\}') snapshots" +echo " qemu-img info reports $(_img_info | grep -c '^ \{30\}') snapshots" echo " Image header reports $(peek_file_be "$TEST_IMG" 60 4) snapshots" echo @@ -XXX,XX +XXX,XX @@ _check_test_img -r all echo echo '65536 snapshots should remain:' -echo " qemu-img info reports $(_img_info | grep -c '^ \{32\}') snapshots" +echo " qemu-img info reports $(_img_info | grep -c '^ \{30\}') snapshots" echo " Image header reports $(peek_file_be "$TEST_IMG" 60 4) snapshots" # success, all done diff --git a/tests/qemu-iotests/267.out b/tests/qemu-iotests/267.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/267.out +++ b/tests/qemu-iotests/267.out @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm snap0 (qemu) info snapshots List of snapshots present on all disks: -ID TAG VM SIZE DATE VM CLOCK ICOUNT --- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +-- snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- (qemu) loadvm snap0 (qemu) quit @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm snap0 (qemu) info snapshots List of snapshots present on all disks: -ID TAG VM SIZE DATE VM CLOCK ICOUNT --- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +-- snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- (qemu) loadvm snap0 (qemu) quit @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm snap0 (qemu) info snapshots List of snapshots present on all disks: -ID TAG VM SIZE DATE VM CLOCK ICOUNT --- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +-- snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- (qemu) loadvm snap0 (qemu) quit @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm snap0 (qemu) info snapshots List of snapshots present on all disks: -ID TAG VM SIZE DATE VM CLOCK ICOUNT --- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +-- snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- (qemu) loadvm snap0 (qemu) quit @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm snap0 (qemu) info snapshots List of snapshots present on all disks: -ID TAG VM SIZE DATE VM CLOCK ICOUNT --- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +-- snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- (qemu) loadvm snap0 (qemu) quit @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm snap0 (qemu) info snapshots List of snapshots present on all disks: -ID TAG VM SIZE DATE VM CLOCK ICOUNT --- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +-- snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- (qemu) loadvm snap0 (qemu) quit @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm snap0 (qemu) info snapshots List of snapshots present on all disks: -ID TAG VM SIZE DATE VM CLOCK ICOUNT --- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +-- snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- (qemu) loadvm snap0 (qemu) quit @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm snap0 (qemu) info snapshots List of snapshots present on all disks: -ID TAG VM SIZE DATE VM CLOCK ICOUNT --- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +-- snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- (qemu) loadvm snap0 (qemu) quit Internal snapshots on overlay: Snapshot list: -ID TAG VM SIZE DATE VM CLOCK ICOUNT -1 snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +1 snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- Internal snapshots on backing file: === -blockdev with NBD server on the backing file === @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm snap0 (qemu) info snapshots List of snapshots present on all disks: -ID TAG VM SIZE DATE VM CLOCK ICOUNT --- snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +-- snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- (qemu) loadvm snap0 (qemu) quit Internal snapshots on overlay: Snapshot list: -ID TAG VM SIZE DATE VM CLOCK ICOUNT -1 snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +1 snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- Internal snapshots on backing file: Snapshot list: -ID TAG VM SIZE DATE VM CLOCK ICOUNT -1 snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +1 snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- *** done diff --git a/tests/qemu-iotests/286 b/tests/qemu-iotests/286 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/286 +++ b/tests/qemu-iotests/286 @@ -XXX,XX +XXX,XX @@ $QEMU_IMG snapshot -l "$TEST_IMG" | tail -n 1 | tr -s ' ' \ -e 's/\./(VM state size unit)/' \ -e 's/\./(snapshot date)/' \ -e 's/\./(snapshot time)/' \ - -e 's/\./(VM clock)/' + -e 's/\./(VM clock)/' \ + -e 's/\./(icount)/' # success, all done echo "*** done" diff --git a/tests/qemu-iotests/286.out b/tests/qemu-iotests/286.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/286.out +++ b/tests/qemu-iotests/286.out @@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz (qemu) quit Output structure: -(snapshot ID) (snapshot name) (VM state size value) (VM state size unit) (snapshot date) (snapshot time) (VM clock) +(snapshot ID) (snapshot name) (VM state size value) (VM state size unit) (snapshot date) (snapshot time) (VM clock) (icount) *** done diff --git a/tests/qemu-iotests/tests/qcow2-internal-snapshots.out b/tests/qemu-iotests/tests/qcow2-internal-snapshots.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/tests/qcow2-internal-snapshots.out +++ b/tests/qemu-iotests/tests/qcow2-internal-snapshots.out @@ -XXX,XX +XXX,XX @@ wrote 524288/524288 bytes at offset 0 (qemu) quit Snapshot list: -ID TAG VM SIZE DATE VM CLOCK ICOUNT -1 snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +1 snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- No errors were found on the image. === Verify that loading the snapshot reverts to the old content === @@ -XXX,XX +XXX,XX @@ read 64512/64512 bytes at offset 66560 (qemu) quit Snapshot list: -ID TAG VM SIZE DATE VM CLOCK ICOUNT -1 snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 -2 snap1 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +1 snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- +2 snap1 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- No errors were found on the image. === qemu-img snapshot can revert to snapshots === @@ -XXX,XX +XXX,XX @@ read 64512/64512 bytes at offset 66560 (qemu) quit Snapshot list: -ID TAG VM SIZE DATE VM CLOCK ICOUNT -1 snap0 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000 +ID TAG VM_SIZE DATE VM_CLOCK ICOUNT +1 snap0 SIZE yyyy-mm-dd hh:mm:ss 0000:00:00.000 -- No errors were found on the image. === Error cases === -- 2.44.0 From: Thomas Huth <thuth@redhat.com> When running iotest 033 with the ssh protocol, it fails with: 033 fail [14:48:31] [14:48:41] 10.2s output mismatch --- /.../tests/qemu-iotests/033.out +++ /.../tests/qemu-iotests/scratch/qcow2-ssh-033/033.out.bad @@ -174,6 +174,7 @@ 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 512/512 bytes at offset 2097152 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io: warning: Failed to truncate the tail of the image: ssh driver does not support shrinking files read 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) We already check for the qcow2 format here, so let's simply also add a check for the protocol here, too, to only test the truncation with the file protocol. Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20240315111108.153201-2-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/033 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/033 b/tests/qemu-iotests/033 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/033 +++ b/tests/qemu-iotests/033 @@ -XXX,XX +XXX,XX @@ do_test 512 "write -P 1 0 0x200" "$TEST_IMG" | _filter_qemu_io # next L2 table do_test 512 "write -P 1 $L2_COVERAGE 0x200" "$TEST_IMG" | _filter_qemu_io -# only interested in qcow2 here; also other formats might respond with -# "not supported" error message -if [ $IMGFMT = "qcow2" ]; then +# only interested in qcow2 with file protocol here; also other formats +# might respond with "not supported" error message +if [ $IMGFMT = "qcow2" ] && [ $IMGPROTO = "file" ]; then do_test 512 "truncate $L2_COVERAGE" "$TEST_IMG" | _filter_qemu_io fi -- 2.44.0 From: Thomas Huth <thuth@redhat.com> The hand-crafted json statement in this test only works if the test is run with the "file" protocol, so mark this test accordingly. Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20240315111108.153201-3-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/066 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/066 +++ b/tests/qemu-iotests/066 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 # This tests qcow2-specific low-level functionality _supported_fmt qcow2 -_supported_proto generic +_supported_proto file # We need zero clusters and snapshots # (TODO: Consider splitting the snapshot part into a separate test # file, so this one runs with refcount_bits=1 and data_file) -- 2.44.0 From: Thomas Huth <thuth@redhat.com> iotest 114 uses "truncate" and the qcow2.py script on the destination file, which both cannot deal with URIs. Thus this test needs the "file" protocol, otherwise it fails with an error message like this: truncate: cannot open 'ssh://127.0.0.1/tmp/qemu-build/tests/qemu-iotests/scratch/qcow2-ssh-114/t.qcow2.orig' for writing: No such file or directory Thus mark this test for "file protocol only" accordingly. Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20240315111108.153201-4-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/114 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/114 +++ b/tests/qemu-iotests/114 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.filter _supported_fmt qcow2 -_supported_proto generic +_supported_proto file # At least OpenBSD doesn't seem to have truncate _supported_os Linux # qcow2.py does not work too well with external data files -- 2.44.0 From: Thomas Huth <thuth@redhat.com> Using "-drive ...,backing.file.filename=..." only works with the file protocol, but not with URIs, so mark this test accordingly. Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20240315111108.153201-5-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/130 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/130 +++ b/tests/qemu-iotests/130 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.qemu _supported_fmt qcow2 -_supported_proto generic +_supported_proto file _supported_os Linux # We are going to use lazy-refcounts _unsupported_imgopts 'compat=0.10' -- 2.44.0 From: Thomas Huth <thuth@redhat.com> Commit b25b387fa592 updated the iotests 134 and 158 to use the --image-opts parameter for qemu-io with file protocol related options, but forgot to update the _supported_proto line accordingly. So let's do that now. Fixes: b25b387fa5 ("qcow2: convert QCow2 to use QCryptoBlock for encryption") Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20240315111108.153201-6-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/134 | 2 +- tests/qemu-iotests/158 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/134 b/tests/qemu-iotests/134 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/134 +++ b/tests/qemu-iotests/134 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.filter _supported_fmt qcow qcow2 -_supported_proto generic +_supported_proto file size=128M diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/158 +++ b/tests/qemu-iotests/158 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.filter _supported_fmt qcow qcow2 -_supported_proto generic +_supported_proto file size=128M -- 2.44.0 From: Thomas Huth <thuth@redhat.com> The test fails completely when you try to use it with a different protocol, e.g. with "./check -ssh -qcow2 156". The test uses some hand-crafted JSON statements which cannot work with other protocols, thus let's change this test to only support the 'file' protocol. Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20240315111108.153201-7-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/156 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/156 +++ b/tests/qemu-iotests/156 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.qemu _supported_fmt qcow2 qed -_supported_proto generic +_supported_proto file # Copying files around with cp does not work with external data files _unsupported_imgopts data_file -- 2.44.0 From: Thomas Huth <thuth@redhat.com> These tests 188, 189 and 198 use qemu-io with --image-opts with additional hard-coded parameters for the file protocol, so they cannot work for other protocols. Thus we have to limit these tests to the file protocol only. Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20240315111108.153201-8-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/188 | 2 +- tests/qemu-iotests/189 | 2 +- tests/qemu-iotests/198 | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/188 b/tests/qemu-iotests/188 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/188 +++ b/tests/qemu-iotests/188 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.filter _supported_fmt qcow2 -_supported_proto generic +_supported_proto file _supported_os Linux _require_working_luks diff --git a/tests/qemu-iotests/189 b/tests/qemu-iotests/189 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/189 +++ b/tests/qemu-iotests/189 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.filter _supported_fmt qcow2 -_supported_proto generic +_supported_proto file _supported_os Linux _require_working_luks diff --git a/tests/qemu-iotests/198 b/tests/qemu-iotests/198 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/198 +++ b/tests/qemu-iotests/198 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.filter _supported_fmt qcow2 -_supported_proto generic +_supported_proto file _supported_os Linux _require_working_luks -- 2.44.0 From: Thomas Huth <thuth@redhat.com> Tests 263, 284 and detect-zeroes-registered-buf use qemu-io with --image-opts so we have to enforce IMGOPTSSYNTAX=true here to get $TEST_IMG in shape for other protocols than "file". Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20240315111108.153201-9-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/263 | 6 ++++-- tests/qemu-iotests/284 | 7 +++---- tests/qemu-iotests/tests/detect-zeroes-registered-buf | 4 +++- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/263 +++ b/tests/qemu-iotests/263 @@ -XXX,XX +XXX,XX @@ _cleanup() } trap "_cleanup; exit \$status" 0 1 2 3 15 +IMGOPTSSYNTAX=true + # get standard environment, filters and checks . ./common.rc . ./common.filter @@ -XXX,XX +XXX,XX @@ echo "testing LUKS qcow2 encryption" echo _make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,cluster_size=64K" $size -_run_test "driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG" +_run_test "$TEST_IMG,encrypt.key-secret=sec0" _cleanup_test_img echo @@ -XXX,XX +XXX,XX @@ echo _make_test_img --object $SECRET -o "encrypt.format=aes,encrypt.key-secret=sec0,cluster_size=64K" $size -_run_test "driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG" +_run_test "$TEST_IMG,encrypt.key-secret=sec0" _cleanup_test_img diff --git a/tests/qemu-iotests/284 b/tests/qemu-iotests/284 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/284 +++ b/tests/qemu-iotests/284 @@ -XXX,XX +XXX,XX @@ _cleanup() } trap "_cleanup; exit \$status" 0 1 2 3 15 +IMGOPTSSYNTAX=true + # get standard environment, filters and checks . ./common.rc . ./common.filter @@ -XXX,XX +XXX,XX @@ size=1M SECRET="secret,id=sec0,data=astrochicken" -IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0" QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT _run_test() { - IMGOPTSSYNTAX=true OLD_TEST_IMG="$TEST_IMG" - TEST_IMG="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0" + TEST_IMG="$TEST_IMG,encrypt.key-secret=sec0" QEMU_IMG_EXTRA_ARGS="--image-opts --object $SECRET" echo @@ -XXX,XX +XXX,XX @@ _run_test() TEST_IMG="$OLD_TEST_IMG" QEMU_IMG_EXTRA_ARGS= - IMGOPTSSYNTAX= } diff --git a/tests/qemu-iotests/tests/detect-zeroes-registered-buf b/tests/qemu-iotests/tests/detect-zeroes-registered-buf index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/tests/detect-zeroes-registered-buf +++ b/tests/qemu-iotests/tests/detect-zeroes-registered-buf @@ -XXX,XX +XXX,XX @@ _cleanup() } trap "_cleanup; exit \$status" 0 1 2 3 15 +IMGOPTSSYNTAX=true + # get standard environment, filters and checks cd .. . ./common.rc @@ -XXX,XX +XXX,XX @@ _supported_proto generic size=128M _make_test_img $size -IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,discard=unmap,detect-zeroes=unmap" +IMGSPEC="$TEST_IMG,discard=unmap,detect-zeroes=unmap" echo echo "== writing zero buffer to image ==" -- 2.44.0 From: Thomas Huth <thuth@redhat.com> Tests that use "--blockdev" with the "file" driver cannot work with other protocols, so we should mark them accordingly. Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20240315111108.153201-10-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/tests/qcow2-internal-snapshots | 2 +- tests/qemu-iotests/tests/qsd-jobs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/tests/qcow2-internal-snapshots b/tests/qemu-iotests/tests/qcow2-internal-snapshots index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/tests/qcow2-internal-snapshots +++ b/tests/qemu-iotests/tests/qcow2-internal-snapshots @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 # This tests qcow2-specific low-level functionality _supported_fmt qcow2 -_supported_proto generic +_supported_proto file # Internal snapshots are (currently) impossible with refcount_bits=1, # and generally impossible with external data files _unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file diff --git a/tests/qemu-iotests/tests/qsd-jobs b/tests/qemu-iotests/tests/qsd-jobs index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/tests/qsd-jobs +++ b/tests/qemu-iotests/tests/qsd-jobs @@ -XXX,XX +XXX,XX @@ cd .. . ./common.filter _supported_fmt qcow2 -_supported_proto generic +_supported_proto file size=128M -- 2.44.0 From: Fiona Ebner <f.ebner@proxmox.com> Failure was noticed when running the tests for the qcow2 image format. Fixes: 0bd779e27e ("crypto: Introduce 'detached-header' field in QCryptoBlockInfoLUKS") Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Message-ID: <20240216101415.293769-1-f.ebner@proxmox.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/198.out | 2 ++ tests/qemu-iotests/206.out | 1 + 2 files changed, 3 insertions(+) diff --git a/tests/qemu-iotests/198.out b/tests/qemu-iotests/198.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/198.out +++ b/tests/qemu-iotests/198.out @@ -XXX,XX +XXX,XX @@ Format specific information: compression type: COMPRESSION_TYPE encrypt: ivgen alg: plain64 + detached header: false hash alg: sha256 cipher alg: aes-256 uuid: 00000000-0000-0000-0000-000000000000 @@ -XXX,XX +XXX,XX @@ Format specific information: compression type: COMPRESSION_TYPE encrypt: ivgen alg: plain64 + detached header: false hash alg: sha256 cipher alg: aes-256 uuid: 00000000-0000-0000-0000-000000000000 diff --git a/tests/qemu-iotests/206.out b/tests/qemu-iotests/206.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/206.out +++ b/tests/qemu-iotests/206.out @@ -XXX,XX +XXX,XX @@ Format specific information: refcount bits: 16 encrypt: ivgen alg: plain64 + detached header: false hash alg: sha1 cipher alg: aes-128 uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX -- 2.44.0