The following changes since commit 22ef7ba8e8ce7fef297549b3defcac333742b804: Merge remote-tracking branch 'remotes/famz/tags/staging-pull-request' into staging (2018-03-13 11:42:45 +0000) are available in the git repository at: git://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to be6c885842efded81a20f4ca24f0d4e123a80c00: block/mirror: change the semantic of 'force' of block-job-cancel (2018-03-13 16:54:47 +0100) ---------------------------------------------------------------- Block layer patches ---------------------------------------------------------------- Fam Zheng (2): block: Fix flags in reopen queue iotests: Add regression test for commit base locking John Snow (21): blockjobs: fix set-speed kick blockjobs: model single jobs as transactions Blockjobs: documentation touchup blockjobs: add status enum blockjobs: add state transition table iotests: add pause_wait blockjobs: add block_job_verb permission table blockjobs: add ABORTING state blockjobs: add CONCLUDED state blockjobs: add NULL state blockjobs: add block_job_dismiss blockjobs: ensure abort is called for cancelled jobs blockjobs: add commit, abort, clean helpers blockjobs: add block_job_txn_apply function blockjobs: add prepare callback blockjobs: add waiting status blockjobs: add PENDING status and event blockjobs: add block-job-finalize blockjobs: Expose manual property iotests: test manual job dismissal tests/test-blockjob: test cancellations Kevin Wolf (14): luks: Separate image file creation from formatting luks: Create block_crypto_co_create_generic() luks: Support .bdrv_co_create luks: Turn invalid assertion into check luks: Catch integer overflow for huge sizes qemu-iotests: Test luks QMP image creation parallels: Support .bdrv_co_create qemu-iotests: Enable write tests for parallels qcow: Support .bdrv_co_create qed: Support .bdrv_co_create vdi: Make comments consistent with other drivers vhdx: Support .bdrv_co_create vpc: Support .bdrv_co_create vpc: Require aligned size in .bdrv_co_create Liang Li (1): block/mirror: change the semantic of 'force' of block-job-cancel Max Reitz (3): vdi: Pull option parsing from vdi_co_create vdi: Move file creation to vdi_co_create_opts vdi: Implement .bdrv_co_create qapi/block-core.json | 363 ++++++++++++++++++++++++++++++++++++++++-- include/block/blockjob.h | 71 ++++++++- include/block/blockjob_int.h | 17 +- block.c | 8 + block/backup.c | 5 +- block/commit.c | 2 +- block/crypto.c | 150 ++++++++++++----- block/mirror.c | 12 +- block/parallels.c | 199 +++++++++++++++++------ block/qcow.c | 196 +++++++++++++++-------- block/qed.c | 204 ++++++++++++++++-------- block/stream.c | 2 +- block/vdi.c | 147 +++++++++++++---- block/vhdx.c | 216 +++++++++++++++++++------ block/vpc.c | 241 +++++++++++++++++++++------- blockdev.c | 71 +++++++-- blockjob.c | 358 +++++++++++++++++++++++++++++++++++------ tests/test-bdrv-drain.c | 5 +- tests/test-blockjob-txn.c | 27 ++-- tests/test-blockjob.c | 233 ++++++++++++++++++++++++++- block/trace-events | 7 + hmp-commands.hx | 3 +- tests/qemu-iotests/030 | 6 +- tests/qemu-iotests/055 | 17 +- tests/qemu-iotests/056 | 187 ++++++++++++++++++++++ tests/qemu-iotests/056.out | 4 +- tests/qemu-iotests/109.out | 24 +-- tests/qemu-iotests/153 | 12 ++ tests/qemu-iotests/153.out | 5 + tests/qemu-iotests/181 | 2 +- tests/qemu-iotests/209 | 210 ++++++++++++++++++++++++ tests/qemu-iotests/209.out | 136 ++++++++++++++++ tests/qemu-iotests/check | 1 - tests/qemu-iotests/common.rc | 2 +- tests/qemu-iotests/group | 1 + tests/qemu-iotests/iotests.py | 12 +- 36 files changed, 2642 insertions(+), 514 deletions(-) create mode 100755 tests/qemu-iotests/209 create mode 100644 tests/qemu-iotests/209.out From: John Snow <jsnow@redhat.com> If speed is '0' it's not actually "less than" the previous speed. Kick the job in this case too. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@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 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) } job->speed = speed; - if (speed <= old_speed) { + if (speed && speed <= old_speed) { return; } -- 2.13.6 From: John Snow <jsnow@redhat.com> model all independent jobs as single job transactions. It's one less case we have to worry about when we add more states to the transition machine. This way, we can just treat all job lifetimes exactly the same. This helps tighten assertions of the STM graph and removes some conditionals that would have been needed in the coming commits adding a more explicit job lifetime management API. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/blockjob.h | 1 - include/block/blockjob_int.h | 3 ++- block/backup.c | 3 +-- block/commit.c | 2 +- block/mirror.c | 2 +- block/stream.c | 2 +- blockjob.c | 25 ++++++++++++++++--------- tests/test-bdrv-drain.c | 4 ++-- tests/test-blockjob-txn.c | 19 +++++++------------ tests/test-blockjob.c | 2 +- 10 files changed, 32 insertions(+), 31 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 { */ QEMUTimer sleep_timer; - /** Non-NULL if this job is part of a transaction */ BlockJobTxn *txn; QLIST_ENTRY(BlockJob) txn_list; } BlockJob; diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/blockjob_int.h +++ b/include/block/blockjob_int.h @@ -XXX,XX +XXX,XX @@ struct BlockJobDriver { * @job_id: The id of the newly-created job, or %NULL to have one * generated automatically. * @job_type: The class object for the newly-created job. + * @txn: The transaction this job belongs to, if any. %NULL otherwise. * @bs: The block * @perm, @shared_perm: Permissions to request for @bs * @speed: The maximum speed, in bytes per second, or 0 for unlimited. @@ -XXX,XX +XXX,XX @@ struct BlockJobDriver { * called from a wrapper that is specific to the job type. */ void *block_job_create(const char *job_id, const BlockJobDriver *driver, - BlockDriverState *bs, uint64_t perm, + BlockJobTxn *txn, BlockDriverState *bs, uint64_t perm, uint64_t shared_perm, int64_t speed, int flags, BlockCompletionFunc *cb, void *opaque, Error **errp); diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, } /* job->common.len is fixed, so we can't allow resize */ - job = block_job_create(job_id, &backup_job_driver, bs, + job = block_job_create(job_id, &backup_job_driver, txn, bs, BLK_PERM_CONSISTENT_READ, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD, @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL, &error_abort); job->common.len = len; - block_job_txn_add_job(txn, &job->common); return &job->common; diff --git a/block/commit.c b/block/commit.c index XXXXXXX..XXXXXXX 100644 --- a/block/commit.c +++ b/block/commit.c @@ -XXX,XX +XXX,XX @@ void commit_start(const char *job_id, BlockDriverState *bs, return; } - s = block_job_create(job_id, &commit_job_driver, bs, 0, BLK_PERM_ALL, + s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL, speed, BLOCK_JOB_DEFAULT, NULL, NULL, errp); if (!s) { return; 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_start_job(const char *job_id, BlockDriverState *bs, } /* Make sure that the source is not resized while the job is running */ - s = block_job_create(job_id, driver, mirror_top_bs, + s = block_job_create(job_id, driver, NULL, mirror_top_bs, BLK_PERM_CONSISTENT_READ, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed, diff --git a/block/stream.c b/block/stream.c index XXXXXXX..XXXXXXX 100644 --- a/block/stream.c +++ b/block/stream.c @@ -XXX,XX +XXX,XX @@ void stream_start(const char *job_id, BlockDriverState *bs, /* Prevent concurrent jobs trying to modify the graph structure here, we * already have our own plans. Also don't allow resize as the image size is * queried only at the job start and then cached. */ - s = block_job_create(job_id, &stream_job_driver, bs, + s = block_job_create(job_id, &stream_job_driver, NULL, bs, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static void block_job_completed_single(BlockJob *job) } } - if (job->txn) { - QLIST_REMOVE(job, txn_list); - block_job_txn_unref(job->txn); - } + QLIST_REMOVE(job, txn_list); + block_job_txn_unref(job->txn); block_job_unref(job); } @@ -XXX,XX +XXX,XX @@ static void block_job_event_completed(BlockJob *job, const char *msg) */ void *block_job_create(const char *job_id, const BlockJobDriver *driver, - BlockDriverState *bs, uint64_t perm, + BlockJobTxn *txn, BlockDriverState *bs, uint64_t perm, uint64_t shared_perm, int64_t speed, int flags, BlockCompletionFunc *cb, void *opaque, Error **errp) { @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, return NULL; } } + + /* Single jobs are modeled as single-job transactions for sake of + * consolidating the job management logic */ + if (!txn) { + txn = block_job_txn_new(); + block_job_txn_add_job(txn, job); + block_job_txn_unref(txn); + } else { + block_job_txn_add_job(txn, job); + } + return job; } @@ -XXX,XX +XXX,XX @@ void block_job_early_fail(BlockJob *job) void block_job_completed(BlockJob *job, int ret) { + assert(job && job->txn && !job->completed); assert(blk_bs(job->blk)->job == job); - assert(!job->completed); job->completed = true; job->ret = ret; - if (!job->txn) { - block_job_completed_single(job); - } else if (ret < 0 || block_job_is_cancelled(job)) { + if (ret < 0 || block_job_is_cancelled(job)) { block_job_completed_txn_abort(job); } else { block_job_completed_txn_success(job); diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void test_blockjob_common(enum drain_type drain_type) blk_target = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); blk_insert_bs(blk_target, target, &error_abort); - job = block_job_create("job0", &test_job_driver, src, 0, BLK_PERM_ALL, 0, - 0, NULL, NULL, &error_abort); + job = block_job_create("job0", &test_job_driver, NULL, src, 0, BLK_PERM_ALL, + 0, 0, NULL, NULL, &error_abort); block_job_add_bdrv(job, "target", target, 0, BLK_PERM_ALL, &error_abort); block_job_start(job); diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-blockjob-txn.c +++ b/tests/test-blockjob-txn.c @@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_block_job_driver = { */ static BlockJob *test_block_job_start(unsigned int iterations, bool use_timer, - int rc, int *result) + int rc, int *result, BlockJobTxn *txn) { BlockDriverState *bs; TestBlockJob *s; @@ -XXX,XX +XXX,XX @@ static BlockJob *test_block_job_start(unsigned int iterations, g_assert_nonnull(bs); snprintf(job_id, sizeof(job_id), "job%u", counter++); - s = block_job_create(job_id, &test_block_job_driver, bs, + s = block_job_create(job_id, &test_block_job_driver, txn, bs, 0, BLK_PERM_ALL, 0, BLOCK_JOB_DEFAULT, test_block_job_cb, data, &error_abort); s->iterations = iterations; @@ -XXX,XX +XXX,XX @@ static void test_single_job(int expected) int result = -EINPROGRESS; txn = block_job_txn_new(); - job = test_block_job_start(1, true, expected, &result); - block_job_txn_add_job(txn, job); + job = test_block_job_start(1, true, expected, &result, txn); block_job_start(job); if (expected == -ECANCELED) { @@ -XXX,XX +XXX,XX @@ static void test_pair_jobs(int expected1, int expected2) int result2 = -EINPROGRESS; txn = block_job_txn_new(); - job1 = test_block_job_start(1, true, expected1, &result1); - block_job_txn_add_job(txn, job1); - job2 = test_block_job_start(2, true, expected2, &result2); - block_job_txn_add_job(txn, job2); + job1 = test_block_job_start(1, true, expected1, &result1, txn); + job2 = test_block_job_start(2, true, expected2, &result2, txn); block_job_start(job1); block_job_start(job2); @@ -XXX,XX +XXX,XX @@ static void test_pair_jobs_fail_cancel_race(void) int result2 = -EINPROGRESS; txn = block_job_txn_new(); - job1 = test_block_job_start(1, true, -ECANCELED, &result1); - block_job_txn_add_job(txn, job1); - job2 = test_block_job_start(2, false, 0, &result2); - block_job_txn_add_job(txn, job2); + job1 = test_block_job_start(1, true, -ECANCELED, &result1, txn); + job2 = test_block_job_start(2, false, 0, &result2, txn); block_job_start(job1); block_job_start(job2); diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-blockjob.c +++ b/tests/test-blockjob.c @@ -XXX,XX +XXX,XX @@ static BlockJob *do_test_id(BlockBackend *blk, const char *id, BlockJob *job; Error *errp = NULL; - job = block_job_create(id, &test_block_job_driver, blk_bs(blk), + job = block_job_create(id, &test_block_job_driver, NULL, blk_bs(blk), 0, BLK_PERM_ALL, 0, BLOCK_JOB_DEFAULT, block_job_cb, NULL, &errp); if (should_succeed) { -- 2.13.6 From: John Snow <jsnow@redhat.com> Trivial; Document what the job creation flags do, and some general tidying. Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/blockjob.h | 8 ++++---- include/block/blockjob_int.h | 4 +++- 2 files changed, 7 insertions(+), 5 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 { /** Reference count of the block job */ int refcnt; - /* True if this job has reported completion by calling block_job_completed. - */ + /** True when job has reported completion by calling block_job_completed. */ bool completed; - /* ret code passed to block_job_completed. - */ + /** ret code passed to block_job_completed. */ int ret; /** @@ -XXX,XX +XXX,XX @@ typedef struct BlockJob { } BlockJob; typedef enum BlockJobCreateFlags { + /* Default behavior */ BLOCK_JOB_DEFAULT = 0x00, + /* BlockJob is not QMP-created and should not send QMP events */ BLOCK_JOB_INTERNAL = 0x01, } BlockJobCreateFlags; diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/blockjob_int.h +++ b/include/block/blockjob_int.h @@ -XXX,XX +XXX,XX @@ struct BlockJobDriver { * block_job_create: * @job_id: The id of the newly-created job, or %NULL to have one * generated automatically. - * @job_type: The class object for the newly-created job. + * @driver: The class object for the newly-created job. * @txn: The transaction this job belongs to, if any. %NULL otherwise. * @bs: The block * @perm, @shared_perm: Permissions to request for @bs * @speed: The maximum speed, in bytes per second, or 0 for unlimited. + * @flags: Creation flags for the Block Job. + * See @BlockJobCreateFlags * @cb: Completion function for the job. * @opaque: Opaque pointer value passed to @cb. * @errp: Error object. -- 2.13.6 From: John Snow <jsnow@redhat.com> We're about to add several new states, and booleans are becoming unwieldly and difficult to reason about. It would help to have a more explicit bookkeeping of the state of blockjobs. To this end, add a new "status" field and add our existing states in a redundant manner alongside the bools they are replacing: UNDEFINED: Placeholder, default state. Not currently visible to QMP unless changes occur in the future to allow creating jobs without starting them via QMP. CREATED: replaces !!job->co && paused && !busy RUNNING: replaces effectively (!paused && busy) PAUSED: Nearly redundant with info->paused, which shows pause_count. This reports the actual status of the job, which almost always matches the paused request status. It differs in that it is strictly only true when the job has actually gone dormant. READY: replaces job->ready. STANDBY: Paused, but job->ready is true. New state additions in coming commits will not be quite so redundant: WAITING: Waiting on transaction. This job has finished all the work it can until the transaction converges, fails, or is canceled. PENDING: Pending authorization from user. This job has finished all the work it can until the job or transaction is finalized via block_job_finalize. This implies the transaction has converged and left the WAITING phase. ABORTING: Job has encountered an error condition and is in the process of aborting. CONCLUDED: Job has ceased all operations and has a return code available for query and may be dismissed via block_job_dismiss. NULL: Job has been dismissed and (should) be destroyed. Should never be visible to QMP. Some of these states appear somewhat superfluous, but it helps define the expected flow of a job; so some of the states wind up being synchronous empty transitions. Importantly, jobs can be in only one of these states at any given time, which helps code and external users alike reason about the current condition of a job unambiguously. Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 31 ++++++++++++++++++++++++++++++- include/block/blockjob.h | 3 +++ blockjob.c | 9 +++++++++ tests/qemu-iotests/109.out | 24 ++++++++++++------------ 4 files changed, 54 insertions(+), 13 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 @@ 'data': ['commit', 'stream', 'mirror', 'backup'] } ## +# @BlockJobStatus: +# +# Indicates the present state of a given blockjob in its lifetime. +# +# @undefined: Erroneous, default state. Should not ever be visible. +# +# @created: The job has been created, but not yet started. +# +# @running: The job is currently running. +# +# @paused: The job is running, but paused. The pause may be requested by +# either the QMP user or by internal processes. +# +# @ready: The job is running, but is ready for the user to signal completion. +# This is used for long-running jobs like mirror that are designed to +# run indefinitely. +# +# @standby: The job is ready, but paused. This is nearly identical to @paused. +# The job may return to @ready or otherwise be canceled. +# +# Since: 2.12 +## +{ 'enum': 'BlockJobStatus', + 'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby'] } + +## # @BlockJobInfo: # # Information about a long-running block device operation. @@ -XXX,XX +XXX,XX @@ # # @ready: true if the job may be completed (since 2.2) # +# @status: Current job state/status (since 2.12) +# # Since: 1.1 ## { 'struct': 'BlockJobInfo', 'data': {'type': 'str', 'device': 'str', 'len': 'int', 'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int', - 'io-status': 'BlockDeviceIoStatus', 'ready': 'bool'} } + 'io-status': 'BlockDeviceIoStatus', 'ready': 'bool', + 'status': 'BlockJobStatus' } } ## # @query-block-jobs: 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 { */ QEMUTimer sleep_timer; + /** Current state; See @BlockJobStatus for details. */ + BlockJobStatus status; + BlockJobTxn *txn; QLIST_ENTRY(BlockJob) txn_list; } BlockJob; diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ void block_job_start(BlockJob *job) job->pause_count--; job->busy = true; job->paused = false; + job->status = BLOCK_JOB_STATUS_RUNNING; bdrv_coroutine_enter(blk_bs(job->blk), job->co); } @@ -XXX,XX +XXX,XX @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) info->speed = job->speed; info->io_status = job->iostatus; info->ready = job->ready; + info->status = job->status; return info; } @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, job->paused = true; job->pause_count = 1; job->refcnt = 1; + job->status = BLOCK_JOB_STATUS_CREATED; aio_timer_init(qemu_get_aio_context(), &job->sleep_timer, QEMU_CLOCK_REALTIME, SCALE_NS, block_job_sleep_timer_cb, job); @@ -XXX,XX +XXX,XX @@ void coroutine_fn block_job_pause_point(BlockJob *job) } if (block_job_should_pause(job) && !block_job_is_cancelled(job)) { + BlockJobStatus status = job->status; + job->status = status == BLOCK_JOB_STATUS_READY ? \ + BLOCK_JOB_STATUS_STANDBY : \ + BLOCK_JOB_STATUS_PAUSED; job->paused = true; block_job_do_yield(job, -1); job->paused = false; + job->status = status; } if (job->driver->resume) { @@ -XXX,XX +XXX,XX @@ void block_job_iostatus_reset(BlockJob *job) void block_job_event_ready(BlockJob *job) { + job->status = BLOCK_JOB_STATUS_READY; job->ready = true; if (block_job_is_internal(job)) { diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/109.out +++ b/tests/qemu-iotests/109.out @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 197120, "offset": 197120, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 197120, "offset": 197120, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 327680, "offset": 327680, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 31457280, "offset": 31457280, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 31457280, "offset": 31457280, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 31457280, "offset": 31457280, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 31457280, "offset": 31457280, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 327680, "offset": 327680, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2048, "offset": 2048, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2048, "offset": 2048, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ Automatically detecting the format is dangerous for raw images, write operations Specify the 'raw' format explicitly to remove the restrictions. {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ Images are identical. {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} -- 2.13.6 From: John Snow <jsnow@redhat.com> The state transition table has mostly been implied. We're about to make it a bit more complex, so let's make the STM explicit instead. Perform state transitions with a function that for now just asserts the transition is appropriate. Transitions: Undefined -> Created: During job initialization. Created -> Running: Once the job is started. Jobs cannot transition from "Created" to "Paused" directly, but will instead synchronously transition to running to paused immediately. Running -> Paused: Normal workflow for pauses. Running -> Ready: Normal workflow for jobs reaching their sync point. (e.g. mirror) Ready -> Standby: Normal workflow for pausing ready jobs. Paused -> Running: Normal resume. Standby -> Ready: Resume of a Standby job. +---------+ |UNDEFINED| +--+------+ | +--v----+ |CREATED| +--+----+ | +--v----+ +------+ |RUNNING<----->PAUSED| +--+----+ +------+ | +--v--+ +-------+ |READY<------->STANDBY| +-----+ +-------+ Notably, there is no state presently defined as of this commit that deals with a job after the "running" or "ready" states, so this table will be adjusted alongside the commits that introduce those states. Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockjob.c | 40 +++++++++++++++++++++++++++++++++------- block/trace-events | 3 +++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ #include "block/block.h" #include "block/blockjob_int.h" #include "block/block_int.h" +#include "block/trace.h" #include "sysemu/block-backend.h" #include "qapi/error.h" #include "qapi/qapi-events-block-core.h" @@ -XXX,XX +XXX,XX @@ * block_job_enter. */ static QemuMutex block_job_mutex; +/* BlockJob State Transition Table */ +bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = { + /* U, C, R, P, Y, S */ + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0}, + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0}, + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0}, + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0}, + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1}, + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0}, +}; + +static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) +{ + BlockJobStatus s0 = job->status; + assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX); + trace_block_job_state_transition(job, job->ret, BlockJobSTT[s0][s1] ? + "allowed" : "disallowed", + qapi_enum_lookup(&BlockJobStatus_lookup, + s0), + qapi_enum_lookup(&BlockJobStatus_lookup, + s1)); + assert(BlockJobSTT[s0][s1]); + job->status = s1; +} + static void block_job_lock(void) { qemu_mutex_lock(&block_job_mutex); @@ -XXX,XX +XXX,XX @@ void block_job_start(BlockJob *job) job->pause_count--; job->busy = true; job->paused = false; - job->status = BLOCK_JOB_STATUS_RUNNING; + block_job_state_transition(job, BLOCK_JOB_STATUS_RUNNING); bdrv_coroutine_enter(blk_bs(job->blk), job->co); } @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, job->paused = true; job->pause_count = 1; job->refcnt = 1; - job->status = BLOCK_JOB_STATUS_CREATED; + block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED); aio_timer_init(qemu_get_aio_context(), &job->sleep_timer, QEMU_CLOCK_REALTIME, SCALE_NS, block_job_sleep_timer_cb, job); @@ -XXX,XX +XXX,XX @@ void coroutine_fn block_job_pause_point(BlockJob *job) if (block_job_should_pause(job) && !block_job_is_cancelled(job)) { BlockJobStatus status = job->status; - job->status = status == BLOCK_JOB_STATUS_READY ? \ - BLOCK_JOB_STATUS_STANDBY : \ - BLOCK_JOB_STATUS_PAUSED; + block_job_state_transition(job, status == BLOCK_JOB_STATUS_READY ? \ + BLOCK_JOB_STATUS_STANDBY : \ + BLOCK_JOB_STATUS_PAUSED); job->paused = true; block_job_do_yield(job, -1); job->paused = false; - job->status = status; + block_job_state_transition(job, status); } if (job->driver->resume) { @@ -XXX,XX +XXX,XX @@ void block_job_iostatus_reset(BlockJob *job) void block_job_event_ready(BlockJob *job) { - job->status = BLOCK_JOB_STATUS_READY; + block_job_state_transition(job, BLOCK_JOB_STATUS_READY); job->ready = true; if (block_job_is_internal(job)) { diff --git a/block/trace-events b/block/trace-events index XXXXXXX..XXXXXXX 100644 --- a/block/trace-events +++ b/block/trace-events @@ -XXX,XX +XXX,XX @@ bdrv_open_common(void *bs, const char *filename, int flags, const char *format_name) "bs %p filename \"%s\" flags 0x%x format_name \"%s\"" bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d" +# blockjob.c +block_job_state_transition(void *job, int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)" + # block/block-backend.c blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x" blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x" -- 2.13.6 From: John Snow <jsnow@redhat.com> Split out the pause command into the actual pause and the wait. Not every usage presently needs to resubmit a pause request. The intent with the next commit will be to explicitly disallow redundant or meaningless pause/resume requests, so the tests need to become more judicious to reflect that. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/030 | 6 ++---- tests/qemu-iotests/055 | 17 ++++++----------- tests/qemu-iotests/iotests.py | 12 ++++++++---- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/030 +++ b/tests/qemu-iotests/030 @@ -XXX,XX +XXX,XX @@ class TestSingleDrive(iotests.QMPTestCase): result = self.vm.qmp('block-stream', device='drive0') self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('block-job-pause', device='drive0') - self.assert_qmp(result, 'return', {}) - + self.pause_job('drive0', wait=False) self.vm.resume_drive('drive0') - self.pause_job('drive0') + self.pause_wait('drive0') result = self.vm.qmp('query-block-jobs') offset = self.dictpath(result, 'return[0]/offset') diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -XXX,XX +XXX,XX @@ class TestSingleDrive(iotests.QMPTestCase): target=target, sync='full') self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('block-job-pause', device='drive0') - self.assert_qmp(result, 'return', {}) - + self.pause_job('drive0', wait=False) self.vm.resume_drive('drive0') - self.pause_job('drive0') + self.pause_wait('drive0') result = self.vm.qmp('query-block-jobs') offset = self.dictpath(result, 'return[0]/offset') @@ -XXX,XX +XXX,XX @@ class TestSingleTransaction(iotests.QMPTestCase): ]) self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('block-job-pause', device='drive0') - self.assert_qmp(result, 'return', {}) + self.pause_job('drive0', wait=False) result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0) self.assert_qmp(result, 'return', {}) - self.pause_job('drive0') + self.pause_wait('drive0') result = self.vm.qmp('query-block-jobs') offset = self.dictpath(result, 'return[0]/offset') @@ -XXX,XX +XXX,XX @@ class TestDriveCompression(iotests.QMPTestCase): result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args) self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('block-job-pause', device='drive0') - self.assert_qmp(result, 'return', {}) - + self.pause_job('drive0', wait=False) self.vm.resume_drive('drive0') - self.pause_job('drive0') + self.pause_wait('drive0') result = self.vm.qmp('query-block-jobs') offset = self.dictpath(result, 'return[0]/offset') diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -XXX,XX +XXX,XX @@ class QMPTestCase(unittest.TestCase): event = self.wait_until_completed(drive=drive) self.assert_qmp(event, 'data/type', 'mirror') - def pause_job(self, job_id='job0'): - result = self.vm.qmp('block-job-pause', device=job_id) - self.assert_qmp(result, 'return', {}) - + def pause_wait(self, job_id='job0'): with Timeout(1, "Timeout waiting for job to pause"): while True: result = self.vm.qmp('query-block-jobs') @@ -XXX,XX +XXX,XX @@ class QMPTestCase(unittest.TestCase): if job['device'] == job_id and job['paused'] == True and job['busy'] == False: return job + def pause_job(self, job_id='job0', wait=True): + result = self.vm.qmp('block-job-pause', device=job_id) + self.assert_qmp(result, 'return', {}) + if wait: + return self.pause_wait(job_id) + return result + def notrun(reason): '''Skip this test suite''' -- 2.13.6 From: John Snow <jsnow@redhat.com> Which commands ("verbs") are appropriate for jobs in which state is also somewhat burdensome to keep track of. As of this commit, it looks rather useless, but begins to look more interesting the more states we add to the STM table. A recurring theme is that no verb will apply to an 'undefined' job. Further, it's not presently possible to restrict the "pause" or "resume" verbs any more than they are in this commit because of the asynchronous nature of how jobs enter the PAUSED state; justifications for some seemingly erroneous applications are given below. ===== Verbs ===== Cancel: Any state except undefined. Pause: Any state except undefined; 'created': Requests that the job pauses as it starts. 'running': Normal usage. (PAUSED) 'paused': The job may be paused for internal reasons, but the user may wish to force an indefinite user-pause, so this is allowed. 'ready': Normal usage. (STANDBY) 'standby': Same logic as above. Resume: Any state except undefined; 'created': Will lift a user's pause-on-start request. 'running': Will lift a pause request before it takes effect. 'paused': Normal usage. 'ready': Will lift a pause request before it takes effect. 'standby': Normal usage. Set-speed: Any state except undefined, though ready may not be meaningful. Complete: Only a 'ready' job may accept a complete request. ======= Changes ======= (1) To facilitate "nice" error checking, all five major block-job verb interfaces in blockjob.c now support an errp parameter: - block_job_user_cancel is added as a new interface. - block_job_user_pause gains an errp paramter - block_job_user_resume gains an errp parameter - block_job_set_speed already had an errp parameter. - block_job_complete already had an errp parameter. (2) block-job-pause and block-job-resume will no longer no-op when trying to pause an already paused job, or trying to resume a job that isn't paused. These functions will now report that they did not perform the action requested because it was not possible. iotests have been adjusted to address this new behavior. (3) block-job-complete doesn't worry about checking !block_job_started, because the permission table guards against this. (4) test-bdrv-drain's job implementation needs to announce that it is 'ready' now, in order to be completed. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 20 ++++++++++++++ include/block/blockjob.h | 13 +++++++-- blockdev.c | 10 +++---- blockjob.c | 71 ++++++++++++++++++++++++++++++++++++++++++------ tests/test-bdrv-drain.c | 1 + block/trace-events | 1 + 6 files changed, 100 insertions(+), 16 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 @@ 'data': ['commit', 'stream', 'mirror', 'backup'] } ## +# @BlockJobVerb: +# +# Represents command verbs that can be applied to a blockjob. +# +# @cancel: see @block-job-cancel +# +# @pause: see @block-job-pause +# +# @resume: see @block-job-resume +# +# @set-speed: see @block-job-set-speed +# +# @complete: see @block-job-complete +# +# Since: 2.12 +## +{ 'enum': 'BlockJobVerb', + 'data': ['cancel', 'pause', 'resume', 'set-speed', 'complete' ] } + +## # @BlockJobStatus: # # Indicates the present state of a given blockjob in its lifetime. 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 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp); * Asynchronously pause the specified job. * Do not allow a resume until a matching call to block_job_user_resume. */ -void block_job_user_pause(BlockJob *job); +void block_job_user_pause(BlockJob *job, Error **errp); /** * block_job_paused: @@ -XXX,XX +XXX,XX @@ bool block_job_user_paused(BlockJob *job); * Resume the specified job. * Must be paired with a preceding block_job_user_pause. */ -void block_job_user_resume(BlockJob *job); +void block_job_user_resume(BlockJob *job, Error **errp); + +/** + * block_job_user_cancel: + * @job: The job to be cancelled. + * + * Cancels the specified job, but may refuse to do so if the + * operation isn't currently meaningful. + */ +void block_job_user_cancel(BlockJob *job, Error **errp); /** * block_job_cancel_sync: diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ void qmp_block_job_cancel(const char *device, } trace_qmp_block_job_cancel(job); - block_job_cancel(job); + block_job_user_cancel(job, errp); out: aio_context_release(aio_context); } @@ -XXX,XX +XXX,XX @@ void qmp_block_job_pause(const char *device, Error **errp) AioContext *aio_context; BlockJob *job = find_block_job(device, &aio_context, errp); - if (!job || block_job_user_paused(job)) { + if (!job) { return; } trace_qmp_block_job_pause(job); - block_job_user_pause(job); + block_job_user_pause(job, errp); aio_context_release(aio_context); } @@ -XXX,XX +XXX,XX @@ void qmp_block_job_resume(const char *device, Error **errp) AioContext *aio_context; BlockJob *job = find_block_job(device, &aio_context, errp); - if (!job || !block_job_user_paused(job)) { + if (!job) { return; } trace_qmp_block_job_resume(job); - block_job_user_resume(job); + block_job_user_resume(job, errp); aio_context_release(aio_context); } diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = { /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0}, }; +bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { + /* U, C, R, P, Y, S */ + [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1}, + [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1}, + [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1}, + [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1}, + [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0}, +}; + static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) { BlockJobStatus s0 = job->status; @@ -XXX,XX +XXX,XX @@ static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) job->status = s1; } +static int block_job_apply_verb(BlockJob *job, BlockJobVerb bv, Error **errp) +{ + assert(bv >= 0 && bv <= BLOCK_JOB_VERB__MAX); + trace_block_job_apply_verb(job, qapi_enum_lookup(&BlockJobStatus_lookup, + job->status), + qapi_enum_lookup(&BlockJobVerb_lookup, bv), + BlockJobVerbTable[bv][job->status] ? + "allowed" : "prohibited"); + if (BlockJobVerbTable[bv][job->status]) { + return 0; + } + error_setg(errp, "Job '%s' in state '%s' cannot accept command verb '%s'", + job->id, qapi_enum_lookup(&BlockJobStatus_lookup, job->status), + qapi_enum_lookup(&BlockJobVerb_lookup, bv)); + return -EPERM; +} + static void block_job_lock(void) { qemu_mutex_lock(&block_job_mutex); @@ -XXX,XX +XXX,XX @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) error_setg(errp, QERR_UNSUPPORTED); return; } + if (block_job_apply_verb(job, BLOCK_JOB_VERB_SET_SPEED, errp)) { + return; + } job->driver->set_speed(job, speed, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -XXX,XX +XXX,XX @@ void block_job_complete(BlockJob *job, Error **errp) { /* Should not be reachable via external interface for internal jobs */ assert(job->id); - if (job->pause_count || job->cancelled || - !block_job_started(job) || !job->driver->complete) { + if (block_job_apply_verb(job, BLOCK_JOB_VERB_COMPLETE, errp)) { + return; + } + if (job->pause_count || job->cancelled || !job->driver->complete) { error_setg(errp, "The active block job '%s' cannot be completed", job->id); return; @@ -XXX,XX +XXX,XX @@ void block_job_complete(BlockJob *job, Error **errp) job->driver->complete(job, errp); } -void block_job_user_pause(BlockJob *job) +void block_job_user_pause(BlockJob *job, Error **errp) { + if (block_job_apply_verb(job, BLOCK_JOB_VERB_PAUSE, errp)) { + return; + } + if (job->user_paused) { + error_setg(errp, "Job is already paused"); + return; + } job->user_paused = true; block_job_pause(job); } @@ -XXX,XX +XXX,XX @@ bool block_job_user_paused(BlockJob *job) return job->user_paused; } -void block_job_user_resume(BlockJob *job) +void block_job_user_resume(BlockJob *job, Error **errp) { - if (job && job->user_paused && job->pause_count > 0) { - block_job_iostatus_reset(job); - job->user_paused = false; - block_job_resume(job); + assert(job); + if (!job->user_paused || job->pause_count <= 0) { + error_setg(errp, "Can't resume a job that was not paused"); + return; + } + if (block_job_apply_verb(job, BLOCK_JOB_VERB_RESUME, errp)) { + return; } + block_job_iostatus_reset(job); + job->user_paused = false; + block_job_resume(job); } void block_job_cancel(BlockJob *job) @@ -XXX,XX +XXX,XX @@ void block_job_cancel(BlockJob *job) } } +void block_job_user_cancel(BlockJob *job, Error **errp) +{ + if (block_job_apply_verb(job, BLOCK_JOB_VERB_CANCEL, errp)) { + return; + } + block_job_cancel(job); +} + /* A wrapper around block_job_cancel() taking an Error ** parameter so it may be * used with block_job_finish_sync() without the need for (rather nasty) * function pointer casts there. */ @@ -XXX,XX +XXX,XX @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, action, &error_abort); } if (action == BLOCK_ERROR_ACTION_STOP) { + block_job_pause(job); /* make the pause user visible, which will be resumed from QMP. */ - block_job_user_pause(job); + job->user_paused = true; block_job_iostatus_set_err(job, error); } return action; diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void coroutine_fn test_job_start(void *opaque) { TestBlockJob *s = opaque; + block_job_event_ready(&s->common); while (!s->should_complete) { block_job_sleep_ns(&s->common, 100000); } diff --git a/block/trace-events b/block/trace-events index XXXXXXX..XXXXXXX 100644 --- a/block/trace-events +++ b/block/trace-events @@ -XXX,XX +XXX,XX @@ bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d" # blockjob.c block_job_state_transition(void *job, int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)" +block_job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)" # block/block-backend.c blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x" -- 2.13.6 From: John Snow <jsnow@redhat.com> Add a new state ABORTING. This makes transitions from normative states to error states explicit in the STM, and serves as a disambiguation for which states may complete normally when normal end-states (CONCLUDED) are added in future commits. Notably, Paused/Standby jobs do not transition directly to aborting, as they must wake up first and cooperate in their cancellation. Transitions: Created -> Aborting: can be cancelled (by the system) Running -> Aborting: can be cancelled or encounter an error Ready -> Aborting: can be cancelled or encounter an error Verbs: None. The job must finish cleaning itself up and report its final status. +---------+ |UNDEFINED| +--+------+ | +--v----+ +---------+CREATED| | +--+----+ | | | +--v----+ +------+ +---------+RUNNING<----->PAUSED| | +--+----+ +------+ | | | +--v--+ +-------+ +---------+READY<------->STANDBY| | +-----+ +-------+ | +--v-----+ |ABORTING| +--------+ Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 7 ++++++- blockjob.c | 31 ++++++++++++++++++------------- 2 files changed, 24 insertions(+), 14 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 @@ # @standby: The job is ready, but paused. This is nearly identical to @paused. # The job may return to @ready or otherwise be canceled. # +# @aborting: The job is in the process of being aborted, and will finish with +# an error. +# This status may not be visible to the management process. +# # Since: 2.12 ## { 'enum': 'BlockJobStatus', - 'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby'] } + 'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby', + 'aborting' ] } ## # @BlockJobInfo: diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static QemuMutex block_job_mutex; /* BlockJob State Transition Table */ bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S */ - /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0}, - /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0}, - /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0}, - /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0}, - /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1}, - /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0}, + /* U, C, R, P, Y, S, X */ + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0}, + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 1}, + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1}, + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0}, + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1}, + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0}, + /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0}, }; bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S */ - [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1}, - [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1}, - [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1}, - [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1}, - [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0}, + /* U, C, R, P, Y, S, X */ + [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0}, + [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0}, + [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0}, + [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0}, + [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0}, }; static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) @@ -XXX,XX +XXX,XX @@ static void block_job_completed_single(BlockJob *job) { assert(job->completed); + if (job->ret || block_job_is_cancelled(job)) { + block_job_state_transition(job, BLOCK_JOB_STATUS_ABORTING); + } + if (!job->ret) { if (job->driver->commit) { job->driver->commit(job); -- 2.13.6 From: John Snow <jsnow@redhat.com> add a new state "CONCLUDED" that identifies a job that has ceased all operations. The wording was chosen to avoid any phrasing that might imply success, error, or cancellation. The task has simply ceased all operation and can never again perform any work. ("finished", "done", and "completed" might all imply success.) Transitions: Running -> Concluded: normal completion Ready -> Concluded: normal completion Aborting -> Concluded: error and cancellations Verbs: None as of this commit. (a future commit adds 'dismiss') +---------+ |UNDEFINED| +--+------+ | +--v----+ +---------+CREATED| | +--+----+ | | | +--v----+ +------+ +---------+RUNNING<----->PAUSED| | +--+-+--+ +------+ | | | | | +------------------+ | | | | +--v--+ +-------+ | +---------+READY<------->STANDBY| | | +--+--+ +-------+ | | | | +--v-----+ +--v------+ | |ABORTING+--->CONCLUDED<-------------+ +--------+ +---------+ Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 7 +++++-- blockjob.c | 39 ++++++++++++++++++++++++--------------- 2 files changed, 29 insertions(+), 17 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 @@ # The job may return to @ready or otherwise be canceled. # # @aborting: The job is in the process of being aborted, and will finish with -# an error. +# an error. The job will afterwards report that it is @concluded. # This status may not be visible to the management process. # +# @concluded: The job has finished all work. If manual was set to true, the job +# will remain in the query list until it is dismissed. +# # Since: 2.12 ## { 'enum': 'BlockJobStatus', 'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby', - 'aborting' ] } + 'aborting', 'concluded' ] } ## # @BlockJobInfo: diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static QemuMutex block_job_mutex; /* BlockJob State Transition Table */ bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S, X */ - /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0}, - /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 1}, - /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1}, - /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0}, - /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1}, - /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0}, - /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0}, + /* U, C, R, P, Y, S, X, E */ + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0}, + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 1, 0}, + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 1}, + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0}, + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1}, + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0}, + /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 1}, + /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0}, }; bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S, X */ - [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0}, - [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0}, - [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0}, - [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0}, - [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0}, + /* U, C, R, P, Y, S, X, E */ + [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0, 0}, + [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0, 0}, + [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0}, + [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0}, + [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0}, }; static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) @@ -XXX,XX +XXX,XX @@ void block_job_start(BlockJob *job) bdrv_coroutine_enter(blk_bs(job->blk), job->co); } +static void block_job_conclude(BlockJob *job) +{ + block_job_state_transition(job, BLOCK_JOB_STATUS_CONCLUDED); +} + static void block_job_completed_single(BlockJob *job) { assert(job->completed); @@ -XXX,XX +XXX,XX @@ static void block_job_completed_single(BlockJob *job) QLIST_REMOVE(job, txn_list); block_job_txn_unref(job->txn); + block_job_conclude(job); block_job_unref(job); } @@ -XXX,XX +XXX,XX @@ void block_job_user_resume(BlockJob *job, Error **errp) void block_job_cancel(BlockJob *job) { - if (block_job_started(job)) { + if (job->status == BLOCK_JOB_STATUS_CONCLUDED) { + return; + } else if (block_job_started(job)) { block_job_cancel_async(job); block_job_enter(job); } else { -- 2.13.6 From: John Snow <jsnow@redhat.com> Add a new state that specifically demarcates when we begin to permanently demolish a job after it has performed all work. This makes the transition explicit in the STM table and highlights conditions under which a job may be demolished. Alongside this state, add a new helper command "block_job_decommission", which transitions to the NULL state and puts down our implicit reference. This separates instances in the code for "block_job_unref" which merely undo a matching "block_job_ref" with instances intended to initiate the full destruction of the object. This decommission action also sets a number of fields to make sure that block internals or external users that are holding a reference to a job to see when it "finishes" are convinced that the job object is "done." This is necessary, for instance, to do a block_job_cancel_sync on a created object which will not make any progress. Now, all jobs must go through block_job_decommission prior to being freed, giving us start-to-finish state machine coverage for jobs. Transitions: Created -> Null: Early failure event before the job is started Concluded -> Null: Standard transition. Verbs: None. This should not ever be visible to the monitor. +---------+ |UNDEFINED| +--+------+ | +--v----+ +---------+CREATED+------------------+ | +--+----+ | | | | | +--v----+ +------+ | +---------+RUNNING<----->PAUSED| | | +--+-+--+ +------+ | | | | | | | +------------------+ | | | | | | +--v--+ +-------+ | | +---------+READY<------->STANDBY| | | | +--+--+ +-------+ | | | | | | +--v-----+ +--v------+ | | |ABORTING+--->CONCLUDED<-------------+ | +--------+ +--+------+ | | | +--v-+ | |NULL<---------------------+ +----+ Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 5 ++++- blockjob.c | 50 ++++++++++++++++++++++++++++++++------------------ 2 files changed, 36 insertions(+), 19 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 @@ # @concluded: The job has finished all work. If manual was set to true, the job # will remain in the query list until it is dismissed. # +# @null: The job is in the process of being dismantled. This state should not +# ever be visible externally. +# # Since: 2.12 ## { 'enum': 'BlockJobStatus', 'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby', - 'aborting', 'concluded' ] } + 'aborting', 'concluded', 'null' ] } ## # @BlockJobInfo: diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static QemuMutex block_job_mutex; /* BlockJob State Transition Table */ bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S, X, E */ - /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0}, - /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 1, 0}, - /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 1}, - /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0}, - /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1}, - /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0}, - /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 1}, - /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0}, + /* U, C, R, P, Y, S, X, E, N */ + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0, 0}, + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 1, 0, 1}, + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 1, 0}, + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0, 0}, + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1, 0}, + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0, 0}, + /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 1, 0}, + /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0, 1}, + /* N: */ [BLOCK_JOB_STATUS_NULL] = {0, 0, 0, 0, 0, 0, 0, 0, 0}, }; bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S, X, E */ - [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0, 0}, - [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0, 0}, - [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0}, - [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0}, - [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0}, + /* U, C, R, P, Y, S, X, E, N */ + [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0}, }; static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) @@ -XXX,XX +XXX,XX @@ static void block_job_detach_aio_context(void *opaque); void block_job_unref(BlockJob *job) { if (--job->refcnt == 0) { + assert(job->status == BLOCK_JOB_STATUS_NULL); BlockDriverState *bs = blk_bs(job->blk); QLIST_REMOVE(job, job_list); bs->job = NULL; @@ -XXX,XX +XXX,XX @@ void block_job_start(BlockJob *job) bdrv_coroutine_enter(blk_bs(job->blk), job->co); } +static void block_job_decommission(BlockJob *job) +{ + assert(job); + job->completed = true; + job->busy = false; + job->paused = false; + job->deferred_to_main_loop = true; + block_job_state_transition(job, BLOCK_JOB_STATUS_NULL); + block_job_unref(job); +} + static void block_job_conclude(BlockJob *job) { block_job_state_transition(job, BLOCK_JOB_STATUS_CONCLUDED); @@ -XXX,XX +XXX,XX @@ static void block_job_completed_single(BlockJob *job) QLIST_REMOVE(job, txn_list); block_job_txn_unref(job->txn); block_job_conclude(job); - block_job_unref(job); + block_job_decommission(job); } static void block_job_cancel_async(BlockJob *job) @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, block_job_set_speed(job, speed, &local_err); if (local_err) { - block_job_unref(job); + block_job_early_fail(job); error_propagate(errp, local_err); return NULL; } @@ -XXX,XX +XXX,XX @@ void block_job_pause_all(void) void block_job_early_fail(BlockJob *job) { - block_job_unref(job); + assert(job->status == BLOCK_JOB_STATUS_CREATED); + block_job_decommission(job); } void block_job_completed(BlockJob *job, int ret) -- 2.13.6 From: John Snow <jsnow@redhat.com> For jobs that have reached their CONCLUDED state, prior to having their last reference put down (meaning jobs that have completed successfully, unsuccessfully, or have been canceled), allow the user to dismiss the job's lingering status report via block-job-dismiss. This gives management APIs the chance to conclusively determine if a job failed or succeeded, even if the event broadcast was missed. Note: block_job_do_dismiss and block_job_decommission happen to do exactly the same thing, but they're called from different semantic contexts, so both aliases are kept to improve readability. Note 2: Don't worry about the 0x04 flag definition for AUTO_DISMISS, she has a friend coming in a future patch to fill the hole where 0x02 is. Verbs: Dismiss: operates on CONCLUDED jobs only. Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 24 +++++++++++++++++++++++- include/block/blockjob.h | 14 ++++++++++++++ blockdev.c | 14 ++++++++++++++ blockjob.c | 26 ++++++++++++++++++++++++-- block/trace-events | 1 + 5 files changed, 76 insertions(+), 3 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 @@ # # @complete: see @block-job-complete # +# @dismiss: see @block-job-dismiss +# # Since: 2.12 ## { 'enum': 'BlockJobVerb', - 'data': ['cancel', 'pause', 'resume', 'set-speed', 'complete' ] } + 'data': ['cancel', 'pause', 'resume', 'set-speed', 'complete', 'dismiss' ] } ## # @BlockJobStatus: @@ -XXX,XX +XXX,XX @@ { 'command': 'block-job-complete', 'data': { 'device': 'str' } } ## +# @block-job-dismiss: +# +# For jobs that have already concluded, remove them from the block-job-query +# list. This command only needs to be run for jobs which were started with +# QEMU 2.12+ job lifetime management semantics. +# +# This command will refuse to operate on any job that has not yet reached +# its terminal state, BLOCK_JOB_STATUS_CONCLUDED. For jobs that make use of +# BLOCK_JOB_READY event, block-job-cancel or block-job-complete will still need +# to be used as appropriate. +# +# @id: The job identifier. +# +# Returns: Nothing on success +# +# Since: 2.12 +## +{ 'command': 'block-job-dismiss', 'data': { 'id': 'str' } } + +## # @BlockdevDiscardOptions: # # Determines how to handle discard requests. 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 { /** Current state; See @BlockJobStatus for details. */ BlockJobStatus status; + /** True if this job should automatically dismiss itself */ + bool auto_dismiss; + BlockJobTxn *txn; QLIST_ENTRY(BlockJob) txn_list; } BlockJob; @@ -XXX,XX +XXX,XX @@ typedef enum BlockJobCreateFlags { BLOCK_JOB_DEFAULT = 0x00, /* BlockJob is not QMP-created and should not send QMP events */ BLOCK_JOB_INTERNAL = 0x01, + /* BlockJob requires manual dismiss step */ + BLOCK_JOB_MANUAL_DISMISS = 0x04, } BlockJobCreateFlags; /** @@ -XXX,XX +XXX,XX @@ void block_job_cancel(BlockJob *job); void block_job_complete(BlockJob *job, Error **errp); /** + * block_job_dismiss: + * @job: The job to be dismissed. + * @errp: Error object. + * + * Remove a concluded job from the query list. + */ +void block_job_dismiss(BlockJob **job, Error **errp); + +/** * block_job_query: * @job: The job to get information about. * diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ void qmp_block_job_complete(const char *device, Error **errp) aio_context_release(aio_context); } +void qmp_block_job_dismiss(const char *id, Error **errp) +{ + AioContext *aio_context; + BlockJob *job = find_block_job(id, &aio_context, errp); + + if (!job) { + return; + } + + trace_qmp_block_job_dismiss(job); + block_job_dismiss(&job, errp); + aio_context_release(aio_context); +} + void qmp_change_backing_file(const char *device, const char *image_node_name, const char *backing_file, diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 1, 0}, }; static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) @@ -XXX,XX +XXX,XX @@ static void block_job_decommission(BlockJob *job) block_job_unref(job); } +static void block_job_do_dismiss(BlockJob *job) +{ + block_job_decommission(job); +} + static void block_job_conclude(BlockJob *job) { block_job_state_transition(job, BLOCK_JOB_STATUS_CONCLUDED); + if (job->auto_dismiss || !block_job_started(job)) { + block_job_do_dismiss(job); + } } static void block_job_completed_single(BlockJob *job) @@ -XXX,XX +XXX,XX @@ static void block_job_completed_single(BlockJob *job) QLIST_REMOVE(job, txn_list); block_job_txn_unref(job->txn); block_job_conclude(job); - block_job_decommission(job); } static void block_job_cancel_async(BlockJob *job) @@ -XXX,XX +XXX,XX @@ void block_job_complete(BlockJob *job, Error **errp) job->driver->complete(job, errp); } +void block_job_dismiss(BlockJob **jobptr, Error **errp) +{ + BlockJob *job = *jobptr; + /* similarly to _complete, this is QMP-interface only. */ + assert(job->id); + if (block_job_apply_verb(job, BLOCK_JOB_VERB_DISMISS, errp)) { + return; + } + + block_job_do_dismiss(job); + *jobptr = NULL; +} + void block_job_user_pause(BlockJob *job, Error **errp) { if (block_job_apply_verb(job, BLOCK_JOB_VERB_PAUSE, errp)) { @@ -XXX,XX +XXX,XX @@ void block_job_user_resume(BlockJob *job, Error **errp) void block_job_cancel(BlockJob *job) { if (job->status == BLOCK_JOB_STATUS_CONCLUDED) { - return; + block_job_do_dismiss(job); } else if (block_job_started(job)) { block_job_cancel_async(job); block_job_enter(job); @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, job->paused = true; job->pause_count = 1; job->refcnt = 1; + job->auto_dismiss = !(flags & BLOCK_JOB_MANUAL_DISMISS); block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED); aio_timer_init(qemu_get_aio_context(), &job->sleep_timer, QEMU_CLOCK_REALTIME, SCALE_NS, diff --git a/block/trace-events b/block/trace-events index XXXXXXX..XXXXXXX 100644 --- a/block/trace-events +++ b/block/trace-events @@ -XXX,XX +XXX,XX @@ qmp_block_job_cancel(void *job) "job %p" qmp_block_job_pause(void *job) "job %p" qmp_block_job_resume(void *job) "job %p" qmp_block_job_complete(void *job) "job %p" +qmp_block_job_dismiss(void *job) "job %p" qmp_block_stream(void *bs, void *job) "bs %p job %p" # block/file-win32.c -- 2.13.6 From: John Snow <jsnow@redhat.com> Presently, even if a job is canceled post-completion as a result of a failing peer in a transaction, it will still call .commit because nothing has updated or changed its return code. The reason why this does not cause problems currently is because backup's implementation of .commit checks for cancellation itself. I'd like to simplify this contract: (1) Abort is called if the job/transaction fails (2) Commit is called if the job/transaction succeeds To this end: A job's return code, if 0, will be forcibly set as -ECANCELED if that job has already concluded. Remove the now redundant check in the backup job implementation. We need to check for cancellation in both block_job_completed AND block_job_completed_single, because jobs may be cancelled between those two calls; for instance in transactions. This also necessitates an ABORTING -> ABORTING transition to be allowed. The check in block_job_completed could be removed, but there's no point in starting to attempt to succeed a transaction that we know in advance will fail. This does NOT affect mirror jobs that are "canceled" during their synchronous phase. The mirror job itself forcibly sets the canceled property to false prior to ceding control, so such cases will invoke the "commit" callback. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/backup.c | 2 +- blockjob.c | 21 ++++++++++++++++----- block/trace-events | 1 + 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) BdrvDirtyBitmap *bm; BlockDriverState *bs = blk_bs(job->common.blk); - if (ret < 0 || block_job_is_cancelled(&job->common)) { + if (ret < 0) { /* Merge the successor back into the parent, delete nothing. */ bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); assert(bm); diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = { /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0, 0}, /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1, 0}, /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0, 0}, - /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 1, 0}, + /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 1, 1, 0}, /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0, 1}, /* N: */ [BLOCK_JOB_STATUS_NULL] = {0, 0, 0, 0, 0, 0, 0, 0, 0}, }; @@ -XXX,XX +XXX,XX @@ static void block_job_conclude(BlockJob *job) } } +static void block_job_update_rc(BlockJob *job) +{ + if (!job->ret && block_job_is_cancelled(job)) { + job->ret = -ECANCELED; + } + if (job->ret) { + block_job_state_transition(job, BLOCK_JOB_STATUS_ABORTING); + } +} + static void block_job_completed_single(BlockJob *job) { assert(job->completed); - if (job->ret || block_job_is_cancelled(job)) { - block_job_state_transition(job, BLOCK_JOB_STATUS_ABORTING); - } + /* Ensure abort is called for late-transactional failures */ + block_job_update_rc(job); if (!job->ret) { if (job->driver->commit) { @@ -XXX,XX +XXX,XX @@ void block_job_completed(BlockJob *job, int ret) assert(blk_bs(job->blk)->job == job); job->completed = true; job->ret = ret; - if (ret < 0 || block_job_is_cancelled(job)) { + block_job_update_rc(job); + trace_block_job_completed(job, ret, job->ret); + if (job->ret) { block_job_completed_txn_abort(job); } else { block_job_completed_txn_success(job); diff --git a/block/trace-events b/block/trace-events index XXXXXXX..XXXXXXX 100644 --- a/block/trace-events +++ b/block/trace-events @@ -XXX,XX +XXX,XX @@ bdrv_open_common(void *bs, const char *filename, int flags, const char *format_n bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d" # blockjob.c +block_job_completed(void *job, int ret, int jret) "job %p ret %d corrected ret %d" block_job_state_transition(void *job, int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)" block_job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)" -- 2.13.6 From: John Snow <jsnow@redhat.com> The completed_single function is getting a little mucked up with checking to see which callbacks exist, so let's factor them out. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockjob.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static void block_job_update_rc(BlockJob *job) } } +static void block_job_commit(BlockJob *job) +{ + assert(!job->ret); + if (job->driver->commit) { + job->driver->commit(job); + } +} + +static void block_job_abort(BlockJob *job) +{ + assert(job->ret); + if (job->driver->abort) { + job->driver->abort(job); + } +} + +static void block_job_clean(BlockJob *job) +{ + if (job->driver->clean) { + job->driver->clean(job); + } +} + static void block_job_completed_single(BlockJob *job) { assert(job->completed); @@ -XXX,XX +XXX,XX @@ static void block_job_completed_single(BlockJob *job) block_job_update_rc(job); if (!job->ret) { - if (job->driver->commit) { - job->driver->commit(job); - } + block_job_commit(job); } else { - if (job->driver->abort) { - job->driver->abort(job); - } - } - if (job->driver->clean) { - job->driver->clean(job); + block_job_abort(job); } + block_job_clean(job); if (job->cb) { job->cb(job->opaque, job->ret); -- 2.13.6 From: John Snow <jsnow@redhat.com> Simply apply a function transaction-wide. A few more uses of this in forthcoming patches. Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockjob.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static void block_job_cancel_async(BlockJob *job) job->cancelled = true; } +static void block_job_txn_apply(BlockJobTxn *txn, void fn(BlockJob *)) +{ + AioContext *ctx; + BlockJob *job, *next; + + QLIST_FOREACH_SAFE(job, &txn->jobs, txn_list, next) { + ctx = blk_get_aio_context(job->blk); + aio_context_acquire(ctx); + fn(job); + aio_context_release(ctx); + } +} + static int block_job_finish_sync(BlockJob *job, void (*finish)(BlockJob *, Error **errp), Error **errp) @@ -XXX,XX +XXX,XX @@ static void block_job_completed_txn_abort(BlockJob *job) static void block_job_completed_txn_success(BlockJob *job) { - AioContext *ctx; BlockJobTxn *txn = job->txn; - BlockJob *other_job, *next; + BlockJob *other_job; /* * Successful completion, see if there are other running jobs in this * txn. @@ -XXX,XX +XXX,XX @@ static void block_job_completed_txn_success(BlockJob *job) if (!other_job->completed) { return; } - } - /* We are the last completed job, commit the transaction. */ - QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) { - ctx = blk_get_aio_context(other_job->blk); - aio_context_acquire(ctx); assert(other_job->ret == 0); - block_job_completed_single(other_job); - aio_context_release(ctx); } + /* We are the last completed job, commit the transaction. */ + block_job_txn_apply(txn, block_job_completed_single); } /* Assumes the block_job_mutex is held */ -- 2.13.6 From: John Snow <jsnow@redhat.com> Some jobs upon finalization may need to perform some work that can still fail. If these jobs are part of a transaction, it's important that these callbacks fail the entire transaction. We allow for a new callback in addition to commit/abort/clean that allows us the opportunity to have fairly late-breaking failures in the transactional process. The expected flow is: - All jobs in a transaction converge to the PENDING state, added in a forthcoming commit. - Upon being finalized, either automatically or explicitly by the user, jobs prepare to complete. - If any job fails preparation, all jobs call .abort. - Otherwise, they succeed and call .commit. Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/blockjob_int.h | 10 ++++++++++ blockjob.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/blockjob_int.h +++ b/include/block/blockjob_int.h @@ -XXX,XX +XXX,XX @@ struct BlockJobDriver { void (*complete)(BlockJob *job, Error **errp); /** + * If the callback is not NULL, prepare will be invoked when all the jobs + * belonging to the same transaction complete; or upon this job's completion + * if it is not in a transaction. + * + * This callback will not be invoked if the job has already failed. + * If it fails, abort and then clean will be called. + */ + int (*prepare)(BlockJob *job); + + /** * If the callback is not NULL, it will be invoked when all the jobs * belonging to the same transaction complete; or upon this job's * completion if it is not in a transaction. Skipped if 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 block_job_update_rc(BlockJob *job) } } +static int block_job_prepare(BlockJob *job) +{ + if (job->ret == 0 && job->driver->prepare) { + job->ret = job->driver->prepare(job); + } + return job->ret; +} + static void block_job_commit(BlockJob *job) { assert(!job->ret); @@ -XXX,XX +XXX,XX @@ static void block_job_clean(BlockJob *job) } } -static void block_job_completed_single(BlockJob *job) +static int block_job_completed_single(BlockJob *job) { assert(job->completed); @@ -XXX,XX +XXX,XX @@ static void block_job_completed_single(BlockJob *job) QLIST_REMOVE(job, txn_list); block_job_txn_unref(job->txn); block_job_conclude(job); + return 0; } static void block_job_cancel_async(BlockJob *job) @@ -XXX,XX +XXX,XX @@ static void block_job_cancel_async(BlockJob *job) job->cancelled = true; } -static void block_job_txn_apply(BlockJobTxn *txn, void fn(BlockJob *)) +static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *)) { AioContext *ctx; BlockJob *job, *next; + int rc; QLIST_FOREACH_SAFE(job, &txn->jobs, txn_list, next) { ctx = blk_get_aio_context(job->blk); aio_context_acquire(ctx); - fn(job); + rc = fn(job); aio_context_release(ctx); + if (rc) { + break; + } } + return rc; } static int block_job_finish_sync(BlockJob *job, @@ -XXX,XX +XXX,XX @@ static void block_job_completed_txn_success(BlockJob *job) { BlockJobTxn *txn = job->txn; BlockJob *other_job; + int rc = 0; + /* * Successful completion, see if there are other running jobs in this * txn. @@ -XXX,XX +XXX,XX @@ static void block_job_completed_txn_success(BlockJob *job) } assert(other_job->ret == 0); } + + /* Jobs may require some prep-work to complete without failure */ + rc = block_job_txn_apply(txn, block_job_prepare); + if (rc) { + block_job_completed_txn_abort(job); + return; + } + /* We are the last completed job, commit the transaction. */ block_job_txn_apply(txn, block_job_completed_single); } -- 2.13.6 From: John Snow <jsnow@redhat.com> For jobs that are stuck waiting on others in a transaction, it would be nice to know that they are no longer "running" in that sense, but instead are waiting on other jobs in the transaction. Jobs that are "waiting" in this sense cannot be meaningfully altered any longer as they have left their running loop. The only meaningful user verb for jobs in this state is "cancel," which will cancel the whole transaction, too. Transitions: Running -> Waiting: Normal transition. Ready -> Waiting: Normal transition. Waiting -> Aborting: Transactional cancellation. Waiting -> Concluded: Normal transition. Removed Transitions: Running -> Concluded: Jobs must go to WAITING first. Ready -> Concluded: Jobs must go to WAITING first. Verbs: Cancel: Can be applied to WAITING jobs. +---------+ |UNDEFINED| +--+------+ | +--v----+ +---------+CREATED+-----------------+ | +--+----+ | | | | | +--v----+ +------+ | +---------+RUNNING<----->PAUSED| | | +--+-+--+ +------+ | | | | | | | +------------------+ | | | | | | +--v--+ +-------+ | | +---------+READY<------->STANDBY| | | | +--+--+ +-------+ | | | | | | | +--v----+ | | +---------+WAITING<---------------+ | | +--+----+ | | | | +--v-----+ +--v------+ | |ABORTING+--->CONCLUDED| | +--------+ +--+------+ | | | +--v-+ | |NULL<--------------------+ +----+ Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 6 +++++- blockjob.c | 37 ++++++++++++++++++++----------------- 2 files changed, 25 insertions(+), 18 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 @@ # @standby: The job is ready, but paused. This is nearly identical to @paused. # The job may return to @ready or otherwise be canceled. # +# @waiting: The job is waiting for other jobs in the transaction to converge +# to the waiting state. This status will likely not be visible for +# the last job in a transaction. +# # @aborting: The job is in the process of being aborted, and will finish with # an error. The job will afterwards report that it is @concluded. # This status may not be visible to the management process. @@ -XXX,XX +XXX,XX @@ ## { 'enum': 'BlockJobStatus', 'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby', - 'aborting', 'concluded', 'null' ] } + 'waiting', 'aborting', 'concluded', 'null' ] } ## # @BlockJobInfo: diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static QemuMutex block_job_mutex; /* BlockJob State Transition Table */ bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S, X, E, N */ - /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0, 0}, - /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 1, 0, 1}, - /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 1, 0}, - /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0, 0}, - /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1, 0}, - /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0, 0}, - /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 1, 1, 0}, - /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0, 1}, - /* N: */ [BLOCK_JOB_STATUS_NULL] = {0, 0, 0, 0, 0, 0, 0, 0, 0}, + /* U, C, R, P, Y, S, W, X, E, N */ + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0, 0, 0}, + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0, 1, 0, 1}, + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 1, 0, 0}, + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0, 0, 0}, + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1, 0, 0}, + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0}, + /* W: */ [BLOCK_JOB_STATUS_WAITING] = {0, 0, 0, 0, 0, 0, 0, 1, 1, 0}, + /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 1, 1, 0}, + /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1}, + /* N: */ [BLOCK_JOB_STATUS_NULL] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, }; bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S, X, E, N */ - [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, - [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, - [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, - [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, - [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0}, - [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 1, 0}, + /* U, C, R, P, Y, S, W, X, E, N */ + [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 0, 1, 0}, }; static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) @@ -XXX,XX +XXX,XX @@ static void block_job_completed_txn_success(BlockJob *job) BlockJob *other_job; int rc = 0; + block_job_state_transition(job, BLOCK_JOB_STATUS_WAITING); + /* * Successful completion, see if there are other running jobs in this * txn. -- 2.13.6 From: John Snow <jsnow@redhat.com> For jobs utilizing the new manual workflow, we intend to prohibit them from modifying the block graph until the management layer provides an explicit ACK via block-job-finalize to move the process forward. To distinguish this runstate from "ready" or "waiting," we add a new "pending" event and status. For now, the transition from PENDING to CONCLUDED/ABORTING is automatic, but a future commit will add the explicit block-job-finalize step. Transitions: Waiting -> Pending: Normal transition. Pending -> Concluded: Normal transition. Pending -> Aborting: Late transactional failures and cancellations. Removed Transitions: Waiting -> Concluded: Jobs must go to PENDING first. Verbs: Cancel: Can be applied to a pending job. +---------+ |UNDEFINED| +--+------+ | +--v----+ +---------+CREATED+-----------------+ | +--+----+ | | | | | +--+----+ +------+ | +---------+RUNNING<----->PAUSED| | | +--+-+--+ +------+ | | | | | | | +------------------+ | | | | | | +--v--+ +-------+ | | +---------+READY<------->STANDBY| | | | +--+--+ +-------+ | | | | | | | +--v----+ | | +---------+WAITING<---------------+ | | +--+----+ | | | | | +--v----+ | +---------+PENDING| | | +--+----+ | | | | +--v-----+ +--v------+ | |ABORTING+--->CONCLUDED| | +--------+ +--+------+ | | | +--v-+ | |NULL<--------------------+ +----+ Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 31 +++++++++++++++++++++- include/block/blockjob.h | 5 ++++ blockjob.c | 67 +++++++++++++++++++++++++++++++----------------- 3 files changed, 78 insertions(+), 25 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 @@ # to the waiting state. This status will likely not be visible for # the last job in a transaction. # +# @pending: The job has finished its work, but has finalization steps that it +# needs to make prior to completing. These changes may require +# manual intervention by the management process if manual was set +# to true. These changes may still fail. +# # @aborting: The job is in the process of being aborted, and will finish with # an error. The job will afterwards report that it is @concluded. # This status may not be visible to the management process. @@ -XXX,XX +XXX,XX @@ ## { 'enum': 'BlockJobStatus', 'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby', - 'waiting', 'aborting', 'concluded', 'null' ] } + 'waiting', 'pending', 'aborting', 'concluded', 'null' ] } ## # @BlockJobInfo: @@ -XXX,XX +XXX,XX @@ 'speed' : 'int' } } ## +# @BLOCK_JOB_PENDING: +# +# Emitted when a block job is awaiting explicit authorization to finalize graph +# changes via @block-job-finalize. If this job is part of a transaction, it will +# not emit this event until the transaction has converged first. +# +# @type: job type +# +# @id: The job identifier. +# +# Since: 2.12 +# +# Example: +# +# <- { "event": "BLOCK_JOB_WAITING", +# "data": { "device": "drive0", "type": "mirror" }, +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } +# +## +{ 'event': 'BLOCK_JOB_PENDING', + 'data': { 'type' : 'BlockJobType', + 'id' : 'str' } } + +## # @PreallocMode: # # Preallocation mode of QEMU image file 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 { /** Current state; See @BlockJobStatus for details. */ BlockJobStatus status; + /** True if this job should automatically finalize itself */ + bool auto_finalize; + /** True if this job should automatically dismiss itself */ bool auto_dismiss; @@ -XXX,XX +XXX,XX @@ typedef enum BlockJobCreateFlags { BLOCK_JOB_DEFAULT = 0x00, /* BlockJob is not QMP-created and should not send QMP events */ BLOCK_JOB_INTERNAL = 0x01, + /* BlockJob requires manual finalize step */ + BLOCK_JOB_MANUAL_FINALIZE = 0x02, /* BlockJob requires manual dismiss step */ BLOCK_JOB_MANUAL_DISMISS = 0x04, } BlockJobCreateFlags; diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static QemuMutex block_job_mutex; /* BlockJob State Transition Table */ bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S, W, X, E, N */ - /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0, 0, 0}, - /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0, 1, 0, 1}, - /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 1, 0, 0}, - /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0, 0, 0}, - /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1, 0, 0}, - /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0}, - /* W: */ [BLOCK_JOB_STATUS_WAITING] = {0, 0, 0, 0, 0, 0, 0, 1, 1, 0}, - /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 1, 1, 0}, - /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1}, - /* N: */ [BLOCK_JOB_STATUS_NULL] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, + /* U, C, R, P, Y, S, W, D, X, E, N */ + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0}, + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0, 0, 1, 0, 1}, + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 0, 1, 0, 0}, + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0}, + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 0, 1, 0, 0}, + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0}, + /* W: */ [BLOCK_JOB_STATUS_WAITING] = {0, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0}, + /* D: */ [BLOCK_JOB_STATUS_PENDING] = {0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 0}, + /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 0}, + /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}, + /* N: */ [BLOCK_JOB_STATUS_NULL] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, }; bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S, W, X, E, N */ - [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 1, 0, 0, 0}, - [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0}, - [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0}, - [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0}, - [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0}, - [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 0, 1, 0}, + /* U, C, R, P, Y, S, W, D, X, E, N */ + [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0}, }; static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) @@ -XXX,XX +XXX,XX @@ static void __attribute__((__constructor__)) block_job_init(void) static void block_job_event_cancelled(BlockJob *job); static void block_job_event_completed(BlockJob *job, const char *msg); +static int block_job_event_pending(BlockJob *job); static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job)); /* Transactional group of block jobs */ @@ -XXX,XX +XXX,XX @@ static void block_job_cancel_async(BlockJob *job) job->cancelled = true; } -static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *)) +static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *), bool lock) { AioContext *ctx; BlockJob *job, *next; int rc; QLIST_FOREACH_SAFE(job, &txn->jobs, txn_list, next) { - ctx = blk_get_aio_context(job->blk); - aio_context_acquire(ctx); + if (lock) { + ctx = blk_get_aio_context(job->blk); + aio_context_acquire(ctx); + } rc = fn(job); - aio_context_release(ctx); + if (lock) { + aio_context_release(ctx); + } if (rc) { break; } @@ -XXX,XX +XXX,XX @@ static void block_job_completed_txn_success(BlockJob *job) } /* Jobs may require some prep-work to complete without failure */ - rc = block_job_txn_apply(txn, block_job_prepare); + rc = block_job_txn_apply(txn, block_job_prepare, true); if (rc) { block_job_completed_txn_abort(job); return; } /* We are the last completed job, commit the transaction. */ - block_job_txn_apply(txn, block_job_completed_single); + block_job_txn_apply(txn, block_job_event_pending, false); + block_job_txn_apply(txn, block_job_completed_single, true); } /* Assumes the block_job_mutex is held */ @@ -XXX,XX +XXX,XX @@ static void block_job_event_completed(BlockJob *job, const char *msg) &error_abort); } +static int block_job_event_pending(BlockJob *job) +{ + block_job_state_transition(job, BLOCK_JOB_STATUS_PENDING); + if (!job->auto_finalize && !block_job_is_internal(job)) { + qapi_event_send_block_job_pending(job->driver->job_type, + job->id, + &error_abort); + } + return 0; +} + /* * API for block job drivers and the block layer. These functions are * declared in blockjob_int.h. @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, job->paused = true; job->pause_count = 1; job->refcnt = 1; + job->auto_finalize = !(flags & BLOCK_JOB_MANUAL_FINALIZE); job->auto_dismiss = !(flags & BLOCK_JOB_MANUAL_DISMISS); block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED); aio_timer_init(qemu_get_aio_context(), &job->sleep_timer, -- 2.13.6 From: John Snow <jsnow@redhat.com> Instead of automatically transitioning from PENDING to CONCLUDED, gate the .prepare() and .commit() phases behind an explicit acknowledgement provided by the QMP monitor if auto_finalize = false has been requested. This allows us to perform graph changes in prepare and/or commit so that graph changes do not occur autonomously without knowledge of the controlling management layer. Transactions that have reached the "PENDING" state together can all be moved to invoke their finalization methods by issuing block_job_finalize to any one job in the transaction. Jobs in a transaction with mixed job->auto_finalize settings will all remain stuck in the "PENDING" state, as if the entire transaction was specified with auto_finalize = false. Jobs that specified auto_finalize = true, however, will still not emit the PENDING event. Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 23 ++++++++++++++++++- include/block/blockjob.h | 17 ++++++++++++++ blockdev.c | 14 +++++++++++ blockjob.c | 60 +++++++++++++++++++++++++++++++++++------------- block/trace-events | 1 + 5 files changed, 98 insertions(+), 17 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 @@ # # @dismiss: see @block-job-dismiss # +# @finalize: see @block-job-finalize +# # Since: 2.12 ## { 'enum': 'BlockJobVerb', - 'data': ['cancel', 'pause', 'resume', 'set-speed', 'complete', 'dismiss' ] } + 'data': ['cancel', 'pause', 'resume', 'set-speed', 'complete', 'dismiss', + 'finalize' ] } ## # @BlockJobStatus: @@ -XXX,XX +XXX,XX @@ { 'command': 'block-job-dismiss', 'data': { 'id': 'str' } } ## +# @block-job-finalize: +# +# Once a job that has manual=true reaches the pending state, it can be +# instructed to finalize any graph changes and do any necessary cleanup +# via this command. +# For jobs in a transaction, instructing one job to finalize will force +# ALL jobs in the transaction to finalize, so it is only necessary to instruct +# a single member job to finalize. +# +# @id: The job identifier. +# +# Returns: Nothing on success +# +# Since: 2.12 +## +{ 'command': 'block-job-finalize', 'data': { 'id': 'str' } } + +## # @BlockdevDiscardOptions: # # Determines how to handle discard requests. 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 @@ void block_job_cancel(BlockJob *job); */ void block_job_complete(BlockJob *job, Error **errp); + +/** + * block_job_finalize: + * @job: The job to fully commit and finish. + * @errp: Error object. + * + * For jobs that have finished their work and are pending + * awaiting explicit acknowledgement to commit their work, + * This will commit that work. + * + * 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. + */ +void block_job_finalize(BlockJob *job, Error **errp); + /** * block_job_dismiss: * @job: The job to be dismissed. diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ void qmp_block_job_complete(const char *device, Error **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); + + if (!job) { + return; + } + + trace_qmp_block_job_finalize(job); + block_job_finalize(job, errp); + aio_context_release(aio_context); +} + void qmp_block_job_dismiss(const char *id, Error **errp) { AioContext *aio_context; diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0}, + [BLOCK_JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0}, [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0}, }; @@ -XXX,XX +XXX,XX @@ static void block_job_clean(BlockJob *job) } } -static int block_job_completed_single(BlockJob *job) +static int block_job_finalize_single(BlockJob *job) { assert(job->completed); @@ -XXX,XX +XXX,XX @@ static void block_job_completed_txn_abort(BlockJob *job) assert(other_job->cancelled); block_job_finish_sync(other_job, NULL, NULL); } - block_job_completed_single(other_job); + block_job_finalize_single(other_job); aio_context_release(ctx); } block_job_txn_unref(txn); } +static int block_job_needs_finalize(BlockJob *job) +{ + return !job->auto_finalize; +} + +static void block_job_do_finalize(BlockJob *job) +{ + int rc; + assert(job && job->txn); + + /* prepare the transaction to complete */ + rc = block_job_txn_apply(job->txn, block_job_prepare, true); + if (rc) { + block_job_completed_txn_abort(job); + } else { + block_job_txn_apply(job->txn, block_job_finalize_single, true); + } +} + static void block_job_completed_txn_success(BlockJob *job) { BlockJobTxn *txn = job->txn; BlockJob *other_job; - int rc = 0; block_job_state_transition(job, BLOCK_JOB_STATUS_WAITING); @@ -XXX,XX +XXX,XX @@ static void block_job_completed_txn_success(BlockJob *job) assert(other_job->ret == 0); } - /* Jobs may require some prep-work to complete without failure */ - rc = block_job_txn_apply(txn, block_job_prepare, true); - if (rc) { - block_job_completed_txn_abort(job); - return; - } - - /* We are the last completed job, commit the transaction. */ block_job_txn_apply(txn, block_job_event_pending, false); - block_job_txn_apply(txn, block_job_completed_single, true); + + /* If no jobs need manual finalization, automatically do so */ + if (block_job_txn_apply(txn, block_job_needs_finalize, false) == 0) { + block_job_do_finalize(job); + } } /* Assumes the block_job_mutex is held */ @@ -XXX,XX +XXX,XX @@ void block_job_complete(BlockJob *job, Error **errp) job->driver->complete(job, errp); } +void block_job_finalize(BlockJob *job, Error **errp) +{ + assert(job && job->id && job->txn); + if (block_job_apply_verb(job, BLOCK_JOB_VERB_FINALIZE, errp)) { + return; + } + block_job_do_finalize(job); +} + void block_job_dismiss(BlockJob **jobptr, Error **errp) { BlockJob *job = *jobptr; @@ -XXX,XX +XXX,XX @@ void block_job_cancel(BlockJob *job) { if (job->status == BLOCK_JOB_STATUS_CONCLUDED) { block_job_do_dismiss(job); - } else if (block_job_started(job)) { - block_job_cancel_async(job); - block_job_enter(job); - } else { + return; + } + block_job_cancel_async(job); + if (!block_job_started(job)) { block_job_completed(job, -ECANCELED); + } else if (job->deferred_to_main_loop) { + block_job_completed_txn_abort(job); + } else { + block_job_enter(job); } } diff --git a/block/trace-events b/block/trace-events index XXXXXXX..XXXXXXX 100644 --- a/block/trace-events +++ b/block/trace-events @@ -XXX,XX +XXX,XX @@ qmp_block_job_cancel(void *job) "job %p" qmp_block_job_pause(void *job) "job %p" qmp_block_job_resume(void *job) "job %p" qmp_block_job_complete(void *job) "job %p" +qmp_block_job_finalize(void *job) "job %p" qmp_block_job_dismiss(void *job) "job %p" qmp_block_stream(void *bs, void *job) "bs %p job %p" -- 2.13.6 From: John Snow <jsnow@redhat.com> Expose the "manual" property via QAPI for the backup-related jobs. As of this commit, this allows the management API to request the "concluded" and "dismiss" semantics for backup jobs. Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 48 ++++++++++++++++++++++++++++++++++++++-------- blockdev.c | 31 +++++++++++++++++++++++++++--- blockjob.c | 2 ++ tests/qemu-iotests/109.out | 24 +++++++++++------------ 4 files changed, 82 insertions(+), 23 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 @@ # # @status: Current job state/status (since 2.12) # +# @auto-finalize: Job will finalize itself when PENDING, moving to +# the CONCLUDED state. (since 2.12) +# +# @auto-dismiss: Job will dismiss itself when CONCLUDED, moving to the NULL +# state and disappearing from the query list. (since 2.12) +# # Since: 1.1 ## { 'struct': 'BlockJobInfo', 'data': {'type': 'str', 'device': 'str', 'len': 'int', 'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int', 'io-status': 'BlockDeviceIoStatus', 'ready': 'bool', - 'status': 'BlockJobStatus' } } + 'status': 'BlockJobStatus', + 'auto-finalize': 'bool', 'auto-dismiss': 'bool' } } ## # @query-block-jobs: @@ -XXX,XX +XXX,XX @@ # default 'report' (no limitations, since this applies to # a different block device than @device). # +# @auto-finalize: When false, this job will wait in a PENDING state after it has +# finished its work, waiting for @block-job-finalize. +# When true, this job will automatically perform its abort or +# commit actions. +# Defaults to true. (Since 2.12) +# +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it +# has completed ceased all work, and wait for @block-job-dismiss. +# When true, this job will automatically disappear from the query +# list without user intervention. +# Defaults to true. (Since 2.12) +# # Note: @on-source-error and @on-target-error only affect background # I/O. If an error occurs during a guest write request, the device's # rerror/werror actions will be used. @@ -XXX,XX +XXX,XX @@ ## { 'struct': 'DriveBackup', 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', - '*format': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', - '*speed': 'int', '*bitmap': 'str', '*compress': 'bool', + '*format': 'str', 'sync': 'MirrorSyncMode', + '*mode': 'NewImageMode', '*speed': 'int', + '*bitmap': 'str', '*compress': 'bool', '*on-source-error': 'BlockdevOnError', - '*on-target-error': 'BlockdevOnError' } } + '*on-target-error': 'BlockdevOnError', + '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } ## # @BlockdevBackup: @@ -XXX,XX +XXX,XX @@ # default 'report' (no limitations, since this applies to # a different block device than @device). # +# @auto-finalize: When false, this job will wait in a PENDING state after it has +# finished its work, waiting for @block-job-finalize. +# When true, this job will automatically perform its abort or +# commit actions. +# Defaults to true. (Since 2.12) +# +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it +# has completed ceased all work, and wait for @block-job-dismiss. +# When true, this job will automatically disappear from the query +# list without user intervention. +# Defaults to true. (Since 2.12) +# # Note: @on-source-error and @on-target-error only affect background # I/O. If an error occurs during a guest write request, the device's # rerror/werror actions will be used. @@ -XXX,XX +XXX,XX @@ ## { 'struct': 'BlockdevBackup', 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', - 'sync': 'MirrorSyncMode', - '*speed': 'int', - '*compress': 'bool', + 'sync': 'MirrorSyncMode', '*speed': 'int', '*compress': 'bool', '*on-source-error': 'BlockdevOnError', - '*on-target-error': 'BlockdevOnError' } } + '*on-target-error': 'BlockdevOnError', + '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } ## # @blockdev-snapshot-sync: diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, AioContext *aio_context; QDict *options = NULL; Error *local_err = NULL; - int flags; + int flags, job_flags = BLOCK_JOB_DEFAULT; int64_t size; bool set_backing_hd = false; @@ -XXX,XX +XXX,XX @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, if (!backup->has_job_id) { backup->job_id = NULL; } + if (!backup->has_auto_finalize) { + backup->auto_finalize = true; + } + if (!backup->has_auto_dismiss) { + backup->auto_dismiss = true; + } if (!backup->has_compress) { backup->compress = false; } @@ -XXX,XX +XXX,XX @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, goto out; } } + if (!backup->auto_finalize) { + job_flags |= BLOCK_JOB_MANUAL_FINALIZE; + } + if (!backup->auto_dismiss) { + job_flags |= BLOCK_JOB_MANUAL_DISMISS; + } job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, backup->sync, bmap, backup->compress, backup->on_source_error, backup->on_target_error, - BLOCK_JOB_DEFAULT, NULL, NULL, txn, &local_err); + job_flags, NULL, NULL, txn, &local_err); bdrv_unref(target_bs); if (local_err != NULL) { error_propagate(errp, local_err); @@ -XXX,XX +XXX,XX @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error *local_err = NULL; AioContext *aio_context; BlockJob *job = NULL; + int job_flags = BLOCK_JOB_DEFAULT; if (!backup->has_speed) { backup->speed = 0; @@ -XXX,XX +XXX,XX @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, if (!backup->has_job_id) { backup->job_id = NULL; } + if (!backup->has_auto_finalize) { + backup->auto_finalize = true; + } + if (!backup->has_auto_dismiss) { + backup->auto_dismiss = true; + } if (!backup->has_compress) { backup->compress = false; } @@ -XXX,XX +XXX,XX @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, goto out; } } + if (!backup->auto_finalize) { + job_flags |= BLOCK_JOB_MANUAL_FINALIZE; + } + if (!backup->auto_dismiss) { + job_flags |= BLOCK_JOB_MANUAL_DISMISS; + } job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, backup->sync, NULL, backup->compress, backup->on_source_error, backup->on_target_error, - BLOCK_JOB_DEFAULT, NULL, NULL, txn, &local_err); + job_flags, NULL, NULL, txn, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); } 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) info->io_status = job->iostatus; info->ready = job->ready; info->status = job->status; + info->auto_finalize = job->auto_finalize; + info->auto_dismiss = job->auto_dismiss; return info; } diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/109.out +++ b/tests/qemu-iotests/109.out @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 197120, "offset": 197120, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 197120, "offset": 197120, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 65536, "offset": 65536, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 31457280, "offset": 31457280, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 31457280, "offset": 31457280, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 31457280, "offset": 31457280, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 31457280, "offset": 31457280, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 0 {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2048, "offset": 2048, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 2048, "offset": 2048, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ Automatically detecting the format is dangerous for raw images, write operations Specify the 'raw' format explicitly to remove the restrictions. {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} @@ -XXX,XX +XXX,XX @@ Images are identical. {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} -{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} -- 2.13.6 From: John Snow <jsnow@redhat.com> Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/056 | 187 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/056.out | 4 +- 2 files changed, 189 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/056 +++ b/tests/qemu-iotests/056 @@ -XXX,XX +XXX,XX @@ backing_img = os.path.join(iotests.test_dir, 'backing.img') test_img = os.path.join(iotests.test_dir, 'test.img') target_img = os.path.join(iotests.test_dir, 'target.img') +def img_create(img, fmt=iotests.imgfmt, size='64M', **kwargs): + fullname = os.path.join(iotests.test_dir, '%s.%s' % (img, fmt)) + optargs = [] + for k,v in kwargs.iteritems(): + optargs = optargs + ['-o', '%s=%s' % (k,v)] + args = ['create', '-f', fmt] + optargs + [fullname, size] + iotests.qemu_img(*args) + return fullname + +def try_remove(img): + try: + os.remove(img) + except OSError: + pass + +def io_write_patterns(img, patterns): + for pattern in patterns: + iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img) + + class TestSyncModesNoneAndTop(iotests.QMPTestCase): image_len = 64 * 1024 * 1024 # MB @@ -XXX,XX +XXX,XX @@ class TestBeforeWriteNotifier(iotests.QMPTestCase): event = self.cancel_and_wait() self.assert_qmp(event, 'data/type', 'backup') +class BackupTest(iotests.QMPTestCase): + def setUp(self): + self.vm = iotests.VM() + self.test_img = img_create('test') + self.dest_img = img_create('dest') + self.vm.add_drive(self.test_img) + self.vm.launch() + + def tearDown(self): + self.vm.shutdown() + try_remove(self.test_img) + try_remove(self.dest_img) + + def hmp_io_writes(self, drive, patterns): + for pattern in patterns: + self.vm.hmp_qemu_io(drive, 'write -P%s %s %s' % pattern) + self.vm.hmp_qemu_io(drive, 'flush') + + def qmp_backup_and_wait(self, cmd='drive-backup', serror=None, + aerror=None, **kwargs): + if not self.qmp_backup(cmd, serror, **kwargs): + return False + return self.qmp_backup_wait(kwargs['device'], aerror) + + def qmp_backup(self, cmd='drive-backup', + error=None, **kwargs): + self.assertTrue('device' in kwargs) + res = self.vm.qmp(cmd, **kwargs) + if error: + self.assert_qmp(res, 'error/desc', error) + return False + self.assert_qmp(res, 'return', {}) + return True + + def qmp_backup_wait(self, device, error=None): + event = self.vm.event_wait(name="BLOCK_JOB_COMPLETED", + match={'data': {'device': device}}) + self.assertNotEqual(event, None) + try: + failure = self.dictpath(event, 'data/error') + except AssertionError: + # Backup succeeded. + self.assert_qmp(event, 'data/offset', event['data']['len']) + return True + else: + # Failure. + self.assert_qmp(event, 'data/error', qerror) + return False + + def test_dismiss_false(self): + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return', []) + self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt, + sync='full', target=self.dest_img, + auto_dismiss=True) + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return', []) + + def test_dismiss_true(self): + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return', []) + self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt, + sync='full', target=self.dest_img, + auto_dismiss=False) + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return[0]/status', 'concluded') + res = self.vm.qmp('block-job-dismiss', id='drive0') + self.assert_qmp(res, 'return', {}) + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return', []) + + def test_dismiss_bad_id(self): + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return', []) + res = self.vm.qmp('block-job-dismiss', id='foobar') + self.assert_qmp(res, 'error/class', 'DeviceNotActive') + + def test_dismiss_collision(self): + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return', []) + self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt, + sync='full', target=self.dest_img, + auto_dismiss=False) + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return[0]/status', 'concluded') + # Leave zombie job un-dismissed, observe a failure: + res = self.qmp_backup_and_wait(serror='Need a root block node', + device='drive0', format=iotests.imgfmt, + sync='full', target=self.dest_img, + auto_dismiss=False) + self.assertEqual(res, False) + # OK, dismiss the zombie. + res = self.vm.qmp('block-job-dismiss', id='drive0') + self.assert_qmp(res, 'return', {}) + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return', []) + # Ensure it's really gone. + self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt, + sync='full', target=self.dest_img, + auto_dismiss=False) + + def dismissal_failure(self, dismissal_opt): + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return', []) + # Give blkdebug something to chew on + self.hmp_io_writes('drive0', + (('0x9a', 0, 512), + ('0x55', '8M', '352k'), + ('0x78', '15872k', '1M'))) + # Add destination node via blkdebug + res = self.vm.qmp('blockdev-add', + node_name='target0', + driver=iotests.imgfmt, + file={ + 'driver': 'blkdebug', + 'image': { + 'driver': 'file', + 'filename': self.dest_img + }, + 'inject-error': [{ + 'event': 'write_aio', + 'errno': 5, + 'immediately': False, + 'once': True + }], + }) + self.assert_qmp(res, 'return', {}) + + res = self.qmp_backup(cmd='blockdev-backup', + device='drive0', target='target0', + on_target_error='stop', + sync='full', + auto_dismiss=dismissal_opt) + self.assertTrue(res) + event = self.vm.event_wait(name="BLOCK_JOB_ERROR", + match={'data': {'device': 'drive0'}}) + self.assertNotEqual(event, None) + # OK, job should be wedged + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return[0]/status', 'paused') + res = self.vm.qmp('block-job-dismiss', id='drive0') + self.assert_qmp(res, 'error/desc', + "Job 'drive0' in state 'paused' cannot accept" + " command verb 'dismiss'") + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return[0]/status', 'paused') + # OK, unstick job and move forward. + res = self.vm.qmp('block-job-resume', device='drive0') + self.assert_qmp(res, 'return', {}) + # And now we need to wait for it to conclude; + res = self.qmp_backup_wait(device='drive0') + self.assertTrue(res) + if not dismissal_opt: + # Job should now be languishing: + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return[0]/status', 'concluded') + res = self.vm.qmp('block-job-dismiss', id='drive0') + self.assert_qmp(res, 'return', {}) + res = self.vm.qmp('query-block-jobs') + self.assert_qmp(res, 'return', []) + + def test_dismiss_premature(self): + self.dismissal_failure(False) + + def test_dismiss_erroneous(self): + self.dismissal_failure(True) + if __name__ == '__main__': iotests.main(supported_fmts=['qcow2', 'qed']) diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/056.out +++ b/tests/qemu-iotests/056.out @@ -XXX,XX +XXX,XX @@ -... +......... ---------------------------------------------------------------------- -Ran 3 tests +Ran 9 tests OK -- 2.13.6 From: John Snow <jsnow@redhat.com> Whatever the state a blockjob is in, it should be able to be canceled by the block layer. Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-blockjob.c | 233 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 229 insertions(+), 4 deletions(-) diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-blockjob.c +++ b/tests/test-blockjob.c @@ -XXX,XX +XXX,XX @@ static void block_job_cb(void *opaque, int ret) { } -static BlockJob *do_test_id(BlockBackend *blk, const char *id, - bool should_succeed) +static BlockJob *mk_job(BlockBackend *blk, const char *id, + const BlockJobDriver *drv, bool should_succeed, + int flags) { BlockJob *job; Error *errp = NULL; - job = block_job_create(id, &test_block_job_driver, NULL, blk_bs(blk), - 0, BLK_PERM_ALL, 0, BLOCK_JOB_DEFAULT, block_job_cb, + job = block_job_create(id, drv, NULL, blk_bs(blk), + 0, BLK_PERM_ALL, 0, flags, block_job_cb, NULL, &errp); if (should_succeed) { g_assert_null(errp); @@ -XXX,XX +XXX,XX @@ static BlockJob *do_test_id(BlockBackend *blk, const char *id, return job; } +static BlockJob *do_test_id(BlockBackend *blk, const char *id, + bool should_succeed) +{ + return mk_job(blk, id, &test_block_job_driver, + should_succeed, BLOCK_JOB_DEFAULT); +} + /* This creates a BlockBackend (optionally with a name) with a * BlockDriverState inserted. */ static BlockBackend *create_blk(const char *name) @@ -XXX,XX +XXX,XX @@ static void test_job_ids(void) destroy_blk(blk[2]); } +typedef struct CancelJob { + BlockJob common; + BlockBackend *blk; + bool should_converge; + bool should_complete; + bool completed; +} CancelJob; + +static void cancel_job_completed(BlockJob *job, void *opaque) +{ + CancelJob *s = opaque; + s->completed = true; + block_job_completed(job, 0); +} + +static void cancel_job_complete(BlockJob *job, Error **errp) +{ + CancelJob *s = container_of(job, CancelJob, common); + s->should_complete = true; +} + +static void coroutine_fn cancel_job_start(void *opaque) +{ + CancelJob *s = opaque; + + while (!s->should_complete) { + if (block_job_is_cancelled(&s->common)) { + goto defer; + } + + if (!s->common.ready && s->should_converge) { + block_job_event_ready(&s->common); + } + + block_job_sleep_ns(&s->common, 100000); + } + + defer: + block_job_defer_to_main_loop(&s->common, cancel_job_completed, s); +} + +static const BlockJobDriver test_cancel_driver = { + .instance_size = sizeof(CancelJob), + .start = cancel_job_start, + .complete = cancel_job_complete, +}; + +static CancelJob *create_common(BlockJob **pjob) +{ + BlockBackend *blk; + BlockJob *job; + CancelJob *s; + + blk = create_blk(NULL); + job = mk_job(blk, "Steve", &test_cancel_driver, true, + BLOCK_JOB_MANUAL_FINALIZE | BLOCK_JOB_MANUAL_DISMISS); + block_job_ref(job); + assert(job->status == BLOCK_JOB_STATUS_CREATED); + s = container_of(job, CancelJob, common); + s->blk = blk; + + *pjob = job; + return s; +} + +static void cancel_common(CancelJob *s) +{ + BlockJob *job = &s->common; + BlockBackend *blk = s->blk; + BlockJobStatus sts = job->status; + + block_job_cancel_sync(job); + if ((sts != BLOCK_JOB_STATUS_CREATED) && + (sts != BLOCK_JOB_STATUS_CONCLUDED)) { + BlockJob *dummy = job; + block_job_dismiss(&dummy, &error_abort); + } + assert(job->status == BLOCK_JOB_STATUS_NULL); + block_job_unref(job); + destroy_blk(blk); +} + +static void test_cancel_created(void) +{ + BlockJob *job; + CancelJob *s; + + s = create_common(&job); + cancel_common(s); +} + +static void test_cancel_running(void) +{ + BlockJob *job; + CancelJob *s; + + s = create_common(&job); + + block_job_start(job); + assert(job->status == BLOCK_JOB_STATUS_RUNNING); + + cancel_common(s); +} + +static void test_cancel_paused(void) +{ + BlockJob *job; + CancelJob *s; + + s = create_common(&job); + + block_job_start(job); + assert(job->status == BLOCK_JOB_STATUS_RUNNING); + + block_job_user_pause(job, &error_abort); + block_job_enter(job); + assert(job->status == BLOCK_JOB_STATUS_PAUSED); + + cancel_common(s); +} + +static void test_cancel_ready(void) +{ + BlockJob *job; + CancelJob *s; + + s = create_common(&job); + + block_job_start(job); + assert(job->status == BLOCK_JOB_STATUS_RUNNING); + + s->should_converge = true; + block_job_enter(job); + assert(job->status == BLOCK_JOB_STATUS_READY); + + cancel_common(s); +} + +static void test_cancel_standby(void) +{ + BlockJob *job; + CancelJob *s; + + s = create_common(&job); + + block_job_start(job); + assert(job->status == BLOCK_JOB_STATUS_RUNNING); + + s->should_converge = true; + block_job_enter(job); + assert(job->status == BLOCK_JOB_STATUS_READY); + + block_job_user_pause(job, &error_abort); + block_job_enter(job); + assert(job->status == BLOCK_JOB_STATUS_STANDBY); + + cancel_common(s); +} + +static void test_cancel_pending(void) +{ + BlockJob *job; + CancelJob *s; + + s = create_common(&job); + + block_job_start(job); + assert(job->status == BLOCK_JOB_STATUS_RUNNING); + + s->should_converge = true; + block_job_enter(job); + assert(job->status == BLOCK_JOB_STATUS_READY); + + block_job_complete(job, &error_abort); + block_job_enter(job); + while (!s->completed) { + aio_poll(qemu_get_aio_context(), true); + } + assert(job->status == BLOCK_JOB_STATUS_PENDING); + + cancel_common(s); +} + +static void test_cancel_concluded(void) +{ + BlockJob *job; + CancelJob *s; + + s = create_common(&job); + + block_job_start(job); + assert(job->status == BLOCK_JOB_STATUS_RUNNING); + + s->should_converge = true; + block_job_enter(job); + assert(job->status == BLOCK_JOB_STATUS_READY); + + block_job_complete(job, &error_abort); + block_job_enter(job); + while (!s->completed) { + aio_poll(qemu_get_aio_context(), true); + } + assert(job->status == BLOCK_JOB_STATUS_PENDING); + + block_job_finalize(job, &error_abort); + assert(job->status == BLOCK_JOB_STATUS_CONCLUDED); + + cancel_common(s); +} + int main(int argc, char **argv) { qemu_init_main_loop(&error_abort); @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_init(&argc, &argv, NULL); g_test_add_func("/blockjob/ids", test_job_ids); + g_test_add_func("/blockjob/cancel/created", test_cancel_created); + g_test_add_func("/blockjob/cancel/running", test_cancel_running); + g_test_add_func("/blockjob/cancel/paused", test_cancel_paused); + g_test_add_func("/blockjob/cancel/ready", test_cancel_ready); + g_test_add_func("/blockjob/cancel/standby", test_cancel_standby); + g_test_add_func("/blockjob/cancel/pending", test_cancel_pending); + g_test_add_func("/blockjob/cancel/concluded", test_cancel_concluded); return g_test_run(); } -- 2.13.6 The crypto driver used to create the image file in a callback from the crypto subsystem. If we want to implement .bdrv_co_create, this needs to go away because that callback will get a reference to an already existing block node. Move the image file creation to block_crypto_create_generic(). Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/crypto.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/block/crypto.c b/block/crypto.c index XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ static ssize_t block_crypto_read_func(QCryptoBlock *block, struct BlockCryptoCreateData { - const char *filename; - QemuOpts *opts; BlockBackend *blk; uint64_t size; }; @@ -XXX,XX +XXX,XX @@ static ssize_t block_crypto_init_func(QCryptoBlock *block, Error **errp) { struct BlockCryptoCreateData *data = opaque; - int ret; /* User provided size should reflect amount of space made * available to the guest, so we must take account of that * which will be used by the crypto header */ - data->size += headerlen; - - qemu_opt_set_number(data->opts, BLOCK_OPT_SIZE, data->size, &error_abort); - ret = bdrv_create_file(data->filename, data->opts, errp); - if (ret < 0) { - return -1; - } - - data->blk = blk_new_open(data->filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_PROTOCOL, errp); - if (!data->blk) { - return -1; - } - - return 0; + return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF, + errp); } @@ -XXX,XX +XXX,XX @@ static int block_crypto_create_generic(QCryptoBlockFormat format, struct BlockCryptoCreateData data = { .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), BDRV_SECTOR_SIZE), - .opts = opts, - .filename = filename, }; QDict *cryptoopts; + /* Parse options */ cryptoopts = qemu_opts_to_qdict(opts, NULL); create_opts = block_crypto_create_opts_init(format, cryptoopts, errp); @@ -XXX,XX +XXX,XX @@ static int block_crypto_create_generic(QCryptoBlockFormat format, return -1; } + /* Create protocol layer */ + ret = bdrv_create_file(filename, opts, errp); + if (ret < 0) { + return ret; + } + + data.blk = blk_new_open(filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, + errp); + if (!data.blk) { + return -EINVAL; + } + + /* Create format layer */ crypto = qcrypto_block_create(create_opts, NULL, block_crypto_init_func, block_crypto_write_func, -- 2.13.6 Everything that refers to the protocol layer or QemuOpts is moved out of block_crypto_create_generic(), so that the remaining function is suitable to be called by a .bdrv_co_create implementation. LUKS is the only driver that actually implements the old interface, and we don't intend to use it in any new drivers, so put the moved out code directly into a LUKS function rather than creating a generic intermediate one. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/crypto.c | 95 +++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 61 insertions(+), 34 deletions(-) diff --git a/block/crypto.c b/block/crypto.c index XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ static int block_crypto_open_generic(QCryptoBlockFormat format, } -static int block_crypto_create_generic(QCryptoBlockFormat format, - const char *filename, - QemuOpts *opts, - Error **errp) +static int block_crypto_co_create_generic(BlockDriverState *bs, + int64_t size, + QCryptoBlockCreateOptions *opts, + Error **errp) { - int ret = -EINVAL; - QCryptoBlockCreateOptions *create_opts = NULL; + int ret; + BlockBackend *blk; QCryptoBlock *crypto = NULL; - struct BlockCryptoCreateData data = { - .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE), - }; - QDict *cryptoopts; - - /* Parse options */ - cryptoopts = qemu_opts_to_qdict(opts, NULL); + struct BlockCryptoCreateData data; - create_opts = block_crypto_create_opts_init(format, cryptoopts, errp); - if (!create_opts) { - return -1; - } + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); - /* Create protocol layer */ - ret = bdrv_create_file(filename, opts, errp); + ret = blk_insert_bs(blk, bs, errp); if (ret < 0) { - return ret; + goto cleanup; } - data.blk = blk_new_open(filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, - errp); - if (!data.blk) { - return -EINVAL; - } + data = (struct BlockCryptoCreateData) { + .blk = blk, + .size = size, + }; - /* Create format layer */ - crypto = qcrypto_block_create(create_opts, NULL, + crypto = qcrypto_block_create(opts, NULL, block_crypto_init_func, block_crypto_write_func, &data, @@ -XXX,XX +XXX,XX @@ static int block_crypto_create_generic(QCryptoBlockFormat format, ret = 0; cleanup: - QDECREF(cryptoopts); qcrypto_block_free(crypto); - blk_unref(data.blk); - qapi_free_QCryptoBlockCreateOptions(create_opts); + blk_unref(blk); return ret; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, QemuOpts *opts, Error **errp) { - return block_crypto_create_generic(Q_CRYPTO_BLOCK_FORMAT_LUKS, - filename, opts, errp); + QCryptoBlockCreateOptions *create_opts = NULL; + BlockDriverState *bs = NULL; + QDict *cryptoopts; + int64_t size; + int ret; + + /* Parse options */ + size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0); + + cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL, + &block_crypto_create_opts_luks, + true); + + create_opts = block_crypto_create_opts_init(Q_CRYPTO_BLOCK_FORMAT_LUKS, + cryptoopts, errp); + if (!create_opts) { + ret = -EINVAL; + goto fail; + } + + /* Create protocol layer */ + ret = bdrv_create_file(filename, opts, errp); + if (ret < 0) { + return ret; + } + + bs = bdrv_open(filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); + if (!bs) { + ret = -EINVAL; + goto fail; + } + + /* Create format layer */ + ret = block_crypto_co_create_generic(bs, size, create_opts, errp); + if (ret < 0) { + goto fail; + } + + ret = 0; +fail: + bdrv_unref(bs); + qapi_free_QCryptoBlockCreateOptions(create_opts); + QDECREF(cryptoopts); + return ret; } static int block_crypto_get_info_luks(BlockDriverState *bs, -- 2.13.6 This adds the .bdrv_co_create driver callback to luks, which enables image creation over QMP. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- qapi/block-core.json | 17 ++++++++++++++++- block/crypto.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) 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 @@ '*preallocation': 'PreallocMode' } } ## +# @BlockdevCreateOptionsLUKS: +# +# Driver specific image creation options for LUKS. +# +# @file Node to create the image format on +# @size Size of the virtual disk in bytes +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsLUKS', + 'base': 'QCryptoBlockCreateOptionsLUKS', + 'data': { 'file': 'BlockdevRef', + 'size': 'size' } } + +## # @BlockdevCreateOptionsNfs: # # Driver specific image creation options for NFS. @@ -XXX,XX +XXX,XX @@ 'http': 'BlockdevCreateNotSupported', 'https': 'BlockdevCreateNotSupported', 'iscsi': 'BlockdevCreateNotSupported', - 'luks': 'BlockdevCreateNotSupported', + 'luks': 'BlockdevCreateOptionsLUKS', 'nbd': 'BlockdevCreateNotSupported', 'nfs': 'BlockdevCreateOptionsNfs', 'null-aio': 'BlockdevCreateNotSupported', diff --git a/block/crypto.c b/block/crypto.c index XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ static int block_crypto_open_luks(BlockDriverState *bs, bs, options, flags, errp); } +static int coroutine_fn +block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) +{ + BlockdevCreateOptionsLUKS *luks_opts; + BlockDriverState *bs = NULL; + QCryptoBlockCreateOptions create_opts; + int ret; + + assert(create_options->driver == BLOCKDEV_DRIVER_LUKS); + luks_opts = &create_options->u.luks; + + bs = bdrv_open_blockdev_ref(luks_opts->file, errp); + if (bs == NULL) { + return -EIO; + } + + create_opts = (QCryptoBlockCreateOptions) { + .format = Q_CRYPTO_BLOCK_FORMAT_LUKS, + .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(luks_opts), + }; + + ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts, + errp); + if (ret < 0) { + goto fail; + } + + ret = 0; +fail: + bdrv_unref(bs); + return ret; +} + static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, QemuOpts *opts, Error **errp) @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_crypto_luks = { .bdrv_open = block_crypto_open_luks, .bdrv_close = block_crypto_close, .bdrv_child_perm = bdrv_format_default_perms, + .bdrv_co_create = block_crypto_co_create_luks, .bdrv_co_create_opts = block_crypto_co_create_opts_luks, .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks, -- 2.13.6 The .bdrv_getlength implementation of the crypto block driver asserted that the payload offset isn't after EOF. This is an invalid assertion to make as the image file could be corrupted. Instead, check it and return -EIO if the file is too small for the payload offset. Zero length images are fine, so trigger -EIO only on offset > len, not on offset >= len as the assertion did before. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- block/crypto.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/crypto.c b/block/crypto.c index XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ static int64_t block_crypto_getlength(BlockDriverState *bs) uint64_t offset = qcrypto_block_get_payload_offset(crypto->block); assert(offset < INT64_MAX); - assert(offset < len); + + if (offset > len) { + return -EIO; + } len -= offset; -- 2.13.6 When you request an image size close to UINT64_MAX, the addition of the crypto header may cause an integer overflow. Catch it instead of silently truncating the image size. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- block/crypto.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/block/crypto.c b/block/crypto.c index XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ static ssize_t block_crypto_init_func(QCryptoBlock *block, { struct BlockCryptoCreateData *data = opaque; + if (data->size > INT64_MAX || headerlen > INT64_MAX - data->size) { + error_setg(errp, "The requested file size is too large"); + return -EFBIG; + } + /* User provided size should reflect amount of space made * available to the guest, so we must take account of that * which will be used by the crypto header -- 2.13.6 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/qemu-iotests/209 | 210 +++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/209.out | 136 ++++++++++++++++++++++++++++ tests/qemu-iotests/common.rc | 2 +- tests/qemu-iotests/group | 1 + 4 files changed, 348 insertions(+), 1 deletion(-) create mode 100755 tests/qemu-iotests/209 create mode 100644 tests/qemu-iotests/209.out diff --git a/tests/qemu-iotests/209 b/tests/qemu-iotests/209 new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/209 @@ -XXX,XX +XXX,XX @@ +#!/bin/bash +# +# Test luks and file image creation +# +# Copyright (C) 2018 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=kwolf@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt luks +_supported_proto file +_supported_os Linux + +function do_run_qemu() +{ + echo Testing: "$@" + $QEMU -nographic -qmp stdio -serial none "$@" + echo +} + +function run_qemu() +{ + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \ + | _filter_qemu | _filter_imgfmt \ + | _filter_actual_image_size +} + +echo +echo "=== Successful image creation (defaults) ===" +echo + +size=$((128 * 1024 * 1024)) + +run_qemu -object secret,id=keysec0,data="foo" <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG_FILE", + "size": 0 + } +} +{ "execute": "blockdev-add", + "arguments": { + "driver": "file", + "node-name": "imgfile", + "filename": "$TEST_IMG_FILE" + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "imgfile", + "key-secret": "keysec0", + "size": $size, + "iter-time": 10 + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific + +echo +echo "=== Successful image creation (with non-default options) ===" +echo + +# Choose a different size to show that we got a new image +size=$((64 * 1024 * 1024)) + +run_qemu -object secret,id=keysec0,data="foo" <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG_FILE", + "size": 0 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_IMG_FILE" + }, + "size": $size, + "key-secret": "keysec0", + "cipher-alg": "twofish-128", + "cipher-mode": "ctr", + "ivgen-alg": "plain64", + "ivgen-hash-alg": "md5", + "hash-alg": "sha1", + "iter-time": 10 + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific + +echo +echo "=== Invalid BlockdevRef ===" +echo + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "this doesn't exist", + "size": $size + } +} +{ "execute": "quit" } +EOF + +echo +echo "=== Zero size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \ + -object secret,id=keysec0,data="foo" <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "key-secret": "keysec0", + "size": 0, + "iter-time": 10 + } +} +{ "execute": "quit" } +EOF + +_img_info | _filter_img_info + + +echo +echo "=== Invalid sizes ===" +echo + +# TODO Negative image sizes aren't handled correctly, but this is a problem +# with QAPI's implementation of the 'size' type and affects other commands as +# well. Once this is fixed, we may want to add a test case here. + +# 1. 2^64 - 512 +# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this) +# 3. 2^63 - 512 (generally valid, but with the crypto header the file will +# exceed 63 bits) + +run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \ + -object secret,id=keysec0,data="foo" <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "key-secret": "keysec0", + "size": 18446744073709551104 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "key-secret": "keysec0", + "size": 9223372036854775808 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "key-secret": "keysec0", + "size": 9223372036854775296 + } +} +{ "execute": "quit" } +EOF + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/209.out b/tests/qemu-iotests/209.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/209.out @@ -XXX,XX +XXX,XX @@ +QA output created by 209 + +=== Successful image creation (defaults) === + +Testing: -object secret,id=keysec0,data=foo +QMP_VERSION +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + +image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"} +file format: IMGFMT +virtual size: 128M (134217728 bytes) +Format specific information: + ivgen alg: plain64 + hash alg: sha256 + cipher alg: aes-256 + uuid: 00000000-0000-0000-0000-000000000000 + cipher mode: xts + slots: + [0]: + active: true + iters: 1024 + key offset: 4096 + stripes: 4000 + [1]: + active: false + key offset: 262144 + [2]: + active: false + key offset: 520192 + [3]: + active: false + key offset: 778240 + [4]: + active: false + key offset: 1036288 + [5]: + active: false + key offset: 1294336 + [6]: + active: false + key offset: 1552384 + [7]: + active: false + key offset: 1810432 + payload offset: 2068480 + master key iters: 1024 + +=== Successful image creation (with non-default options) === + +Testing: -object secret,id=keysec0,data=foo +QMP_VERSION +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + +image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"} +file format: IMGFMT +virtual size: 64M (67108864 bytes) +Format specific information: + ivgen alg: plain64 + hash alg: sha1 + cipher alg: twofish-128 + uuid: 00000000-0000-0000-0000-000000000000 + cipher mode: ctr + slots: + [0]: + active: true + iters: 1024 + key offset: 4096 + stripes: 4000 + [1]: + active: false + key offset: 69632 + [2]: + active: false + key offset: 135168 + [3]: + active: false + key offset: 200704 + [4]: + active: false + key offset: 266240 + [5]: + active: false + key offset: 331776 + [6]: + active: false + key offset: 397312 + [7]: + active: false + key offset: 462848 + payload offset: 528384 + master key iters: 1024 + +=== Invalid BlockdevRef === + +Testing: +QMP_VERSION +{"return": {}} +{"error": {"class": "GenericError", "desc": "Cannot find device=this doesn't exist nor node_name=this doesn't exist"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + + +=== Zero size === + +Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0 -object secret,id=keysec0,data=foo +QMP_VERSION +{"return": {}} +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + +image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"} +file format: IMGFMT +virtual size: 0 (0 bytes) + +=== Invalid sizes === + +Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0 -object secret,id=keysec0,data=foo +QMP_VERSION +{"return": {}} +{"error": {"class": "GenericError", "desc": "The requested file size is too large"}} +{"error": {"class": "GenericError", "desc": "The requested file size is too large"}} +{"error": {"class": "GenericError", "desc": "The requested file size is too large"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + +*** done diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -XXX,XX +XXX,XX @@ _img_info() discard=0 regex_json_spec_start='^ *"format-specific": \{' - $QEMU_IMG info "$@" "$TEST_IMG" 2>&1 | \ + $QEMU_IMG info $QEMU_IMG_EXTRA_ARGS "$@" "$TEST_IMG" 2>&1 | \ sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \ -e "s#$TEST_DIR#TEST_DIR#g" \ -e "s#$IMGFMT#IMGFMT#g" \ diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -XXX,XX +XXX,XX @@ 205 rw auto quick 206 rw auto 207 rw auto +209 rw auto -- 2.13.6 From: Max Reitz <mreitz@redhat.com> In preparation of QAPI-fying VDI image creation, we have to create a BlockdevCreateOptionsVdi type which is received by a (future) vdi_co_create(). vdi_co_create_opts() now converts the QemuOpts object into such a BlockdevCreateOptionsVdi object. The protocol-layer file is still created in vdi_co_do_create() (and BlockdevCreateOptionsVdi.file is set to an empty string), but that will be addressed by a follow-up patch. Note that cluster-size is not part of the QAPI schema because it is not supported by default. Signed-off-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 18 +++++++++++ block/vdi.c | 91 ++++++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 95 insertions(+), 14 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 @@ 'size': 'size' } } ## +# @BlockdevCreateOptionsVdi: +# +# Driver specific image creation options for VDI. +# +# @file Node to create the image format on +# @size Size of the virtual disk in bytes +# @static Whether to create a statically (true) or +# dynamically (false) allocated image +# (default: false, i.e. dynamic) +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsVdi', + 'data': { 'file': 'BlockdevRef', + 'size': 'size', + '*static': 'bool' } } + +## # @BlockdevCreateNotSupported: # # This is used for all drivers that don't support creating images. diff --git a/block/vdi.c b/block/vdi.c index XXXXXXX..XXXXXXX 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -XXX,XX +XXX,XX @@ #include "qemu/osdep.h" #include "qapi/error.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-visit-block-core.h" #include "block/block_int.h" #include "sysemu/block-backend.h" #include "qemu/module.h" @@ -XXX,XX +XXX,XX @@ #define VDI_DISK_SIZE_MAX ((uint64_t)VDI_BLOCKS_IN_IMAGE_MAX * \ (uint64_t)DEFAULT_CLUSTER_SIZE) +static QemuOptsList vdi_create_opts; + typedef struct { char text[0x40]; uint32_t signature; @@ -XXX,XX +XXX,XX @@ nonallocating_write: return ret; } -static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, - Error **errp) +static int coroutine_fn vdi_co_do_create(const char *filename, + QemuOpts *file_opts, + BlockdevCreateOptionsVdi *vdi_opts, + size_t block_size, Error **errp) { int ret = 0; uint64_t bytes = 0; uint32_t blocks; - size_t block_size = DEFAULT_CLUSTER_SIZE; uint32_t image_type = VDI_TYPE_DYNAMIC; VdiHeader header; size_t i; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, logout("\n"); /* Read out options. */ - bytes = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); -#if defined(CONFIG_VDI_BLOCK_SIZE) - /* TODO: Additional checks (SECTOR_SIZE * 2^n, ...). */ - block_size = qemu_opt_get_size_del(opts, - BLOCK_OPT_CLUSTER_SIZE, - DEFAULT_CLUSTER_SIZE); -#endif -#if defined(CONFIG_VDI_STATIC_IMAGE) - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_STATIC, false)) { + bytes = vdi_opts->size; + if (vdi_opts->q_static) { image_type = VDI_TYPE_STATIC; } +#ifndef CONFIG_VDI_STATIC_IMAGE + if (image_type == VDI_TYPE_STATIC) { + ret = -ENOTSUP; + error_setg(errp, "Statically allocated images cannot be created in " + "this build"); + goto exit; + } +#endif +#ifndef CONFIG_VDI_BLOCK_SIZE + if (block_size != DEFAULT_CLUSTER_SIZE) { + ret = -ENOTSUP; + error_setg(errp, + "A non-default cluster size is not supported in this build"); + goto exit; + } #endif if (bytes > VDI_DISK_SIZE_MAX) { @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, goto exit; } - ret = bdrv_create_file(filename, opts, &local_err); + ret = bdrv_create_file(filename, file_opts, &local_err); if (ret < 0) { error_propagate(errp, local_err); goto exit; @@ -XXX,XX +XXX,XX @@ exit: return ret; } +static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, + Error **errp) +{ + QDict *qdict = NULL; + BlockdevCreateOptionsVdi *create_options = NULL; + uint64_t block_size = DEFAULT_CLUSTER_SIZE; + Visitor *v; + Error *local_err = NULL; + int ret; + + /* Since CONFIG_VDI_BLOCK_SIZE is disabled by default, + * cluster-size is not part of the QAPI schema; therefore we have + * to parse it before creating the QAPI object. */ +#if defined(CONFIG_VDI_BLOCK_SIZE) + block_size = qemu_opt_get_size_del(opts, + BLOCK_OPT_CLUSTER_SIZE, + DEFAULT_CLUSTER_SIZE); + if (block_size < BDRV_SECTOR_SIZE || block_size > UINT32_MAX || + !is_power_of_2(block_size)) + { + error_setg(errp, "Invalid cluster size"); + ret = -EINVAL; + goto done; + } +#endif + + qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vdi_create_opts, true); + + qdict_put_str(qdict, "file", ""); /* FIXME */ + + /* Get the QAPI object */ + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + visit_type_BlockdevCreateOptionsVdi(v, NULL, &create_options, &local_err); + visit_free(v); + + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto done; + } + + create_options->size = ROUND_UP(create_options->size, BDRV_SECTOR_SIZE); + + ret = vdi_co_do_create(filename, opts, create_options, block_size, errp); +done: + QDECREF(qdict); + qapi_free_BlockdevCreateOptionsVdi(create_options); + return ret; +} + static void vdi_close(BlockDriverState *bs) { BDRVVdiState *s = bs->opaque; -- 2.13.6 From: Max Reitz <mreitz@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vdi.c | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/block/vdi.c b/block/vdi.c index XXXXXXX..XXXXXXX 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -XXX,XX +XXX,XX @@ nonallocating_write: return ret; } -static int coroutine_fn vdi_co_do_create(const char *filename, - QemuOpts *file_opts, - BlockdevCreateOptionsVdi *vdi_opts, +static int coroutine_fn vdi_co_do_create(BlockdevCreateOptionsVdi *vdi_opts, size_t block_size, Error **errp) { int ret = 0; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_do_create(const char *filename, size_t i; size_t bmap_size; int64_t offset = 0; - Error *local_err = NULL; + BlockDriverState *bs_file = NULL; BlockBackend *blk = NULL; uint32_t *bmap = NULL; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_do_create(const char *filename, goto exit; } - ret = bdrv_create_file(filename, file_opts, &local_err); - if (ret < 0) { - error_propagate(errp, local_err); + bs_file = bdrv_open_blockdev_ref(vdi_opts->file, errp); + if (!bs_file) { + ret = -EIO; goto exit; } - blk = blk_new_open(filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, - &local_err); - if (blk == NULL) { - error_propagate(errp, local_err); - ret = -EIO; + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); + ret = blk_insert_bs(blk, bs_file, errp); + if (ret < 0) { goto exit; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_do_create(const char *filename, vdi_header_to_le(&header); ret = blk_pwrite(blk, offset, &header, sizeof(header), 0); if (ret < 0) { - error_setg(errp, "Error writing header to %s", filename); + error_setg(errp, "Error writing header"); goto exit; } offset += sizeof(header); @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_do_create(const char *filename, } ret = blk_pwrite(blk, offset, bmap, bmap_size, 0); if (ret < 0) { - error_setg(errp, "Error writing bmap to %s", filename); + error_setg(errp, "Error writing bmap"); goto exit; } offset += bmap_size; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_do_create(const char *filename, ret = blk_truncate(blk, offset + blocks * block_size, PREALLOC_MODE_OFF, errp); if (ret < 0) { - error_prepend(errp, "Failed to statically allocate %s", filename); + error_prepend(errp, "Failed to statically allocate file"); goto exit; } } exit: blk_unref(blk); + bdrv_unref(bs_file); g_free(bmap); return ret; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, { QDict *qdict = NULL; BlockdevCreateOptionsVdi *create_options = NULL; + BlockDriverState *bs_file = NULL; uint64_t block_size = DEFAULT_CLUSTER_SIZE; Visitor *v; Error *local_err = NULL; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vdi_create_opts, true); - qdict_put_str(qdict, "file", ""); /* FIXME */ + ret = bdrv_create_file(filename, opts, errp); + if (ret < 0) { + goto done; + } + + bs_file = bdrv_open(filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); + if (!bs_file) { + ret = -EIO; + goto done; + } + + qdict_put_str(qdict, "file", bs_file->node_name); /* Get the QAPI object */ v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, create_options->size = ROUND_UP(create_options->size, BDRV_SECTOR_SIZE); - ret = vdi_co_do_create(filename, opts, create_options, block_size, errp); + ret = vdi_co_do_create(create_options, block_size, errp); done: QDECREF(qdict); qapi_free_BlockdevCreateOptionsVdi(create_options); + bdrv_unref(bs_file); return ret; } -- 2.13.6 From: Max Reitz <mreitz@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 2 +- block/vdi.c | 24 +++++++++++++++++++----- 2 files changed, 20 insertions(+), 6 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 @@ 'sheepdog': 'BlockdevCreateOptionsSheepdog', 'ssh': 'BlockdevCreateOptionsSsh', 'throttle': 'BlockdevCreateNotSupported', - 'vdi': 'BlockdevCreateNotSupported', + 'vdi': 'BlockdevCreateOptionsVdi', 'vhdx': 'BlockdevCreateNotSupported', 'vmdk': 'BlockdevCreateNotSupported', 'vpc': 'BlockdevCreateNotSupported', diff --git a/block/vdi.c b/block/vdi.c index XXXXXXX..XXXXXXX 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -XXX,XX +XXX,XX @@ nonallocating_write: return ret; } -static int coroutine_fn vdi_co_do_create(BlockdevCreateOptionsVdi *vdi_opts, +static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options, size_t block_size, Error **errp) { + BlockdevCreateOptionsVdi *vdi_opts; int ret = 0; uint64_t bytes = 0; uint32_t blocks; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptionsVdi *vdi_opts, BlockBackend *blk = NULL; uint32_t *bmap = NULL; + assert(create_options->driver == BLOCKDEV_DRIVER_VDI); + vdi_opts = &create_options->u.vdi; + logout("\n"); /* Read out options. */ @@ -XXX,XX +XXX,XX @@ exit: return ret; } +static int coroutine_fn vdi_co_create(BlockdevCreateOptions *create_options, + Error **errp) +{ + return vdi_co_do_create(create_options, DEFAULT_CLUSTER_SIZE, errp); +} + static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, Error **errp) { QDict *qdict = NULL; - BlockdevCreateOptionsVdi *create_options = NULL; + BlockdevCreateOptions *create_options = NULL; BlockDriverState *bs_file = NULL; uint64_t block_size = DEFAULT_CLUSTER_SIZE; Visitor *v; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, goto done; } + qdict_put_str(qdict, "driver", "vdi"); qdict_put_str(qdict, "file", bs_file->node_name); /* Get the QAPI object */ v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); - visit_type_BlockdevCreateOptionsVdi(v, NULL, &create_options, &local_err); + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); if (local_err) { @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, goto done; } - create_options->size = ROUND_UP(create_options->size, BDRV_SECTOR_SIZE); + assert(create_options->driver == BLOCKDEV_DRIVER_VDI); + create_options->u.vdi.size = ROUND_UP(create_options->u.vdi.size, + BDRV_SECTOR_SIZE); ret = vdi_co_do_create(create_options, block_size, errp); done: QDECREF(qdict); - qapi_free_BlockdevCreateOptionsVdi(create_options); + qapi_free_BlockdevCreateOptions(create_options); bdrv_unref(bs_file); return ret; } @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_vdi = { .bdrv_reopen_prepare = vdi_reopen_prepare, .bdrv_child_perm = bdrv_format_default_perms, .bdrv_co_create_opts = vdi_co_create_opts, + .bdrv_co_create = vdi_co_create, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_block_status = vdi_co_block_status, .bdrv_make_empty = vdi_make_empty, -- 2.13.6 From: Fam Zheng <famz@redhat.com> Reopen flags are not synchronized according to the bdrv_reopen_queue_child precedence until bdrv_reopen_prepare. It is a bit too late: we already check the consistency in bdrv_check_perm before that. This fixes the bug that when bdrv_reopen a RO node as RW, the flags for backing child are wrong. Before, we could recurse with flags.rw=1; now, role->inherit_options + update_flags_from_options will make sure to clear the bit when necessary. Note that this will not clear an explicitly set bit, as in the case of parallel block jobs (e.g. test_stream_parallel in 030), because the explicit options include 'read-only=false' (for an intermediate node used by a different job). Signed-off-by: Fam Zheng <famz@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, /* Inherit from parent node */ if (parent_options) { + QemuOpts *opts; + QDict *options_copy; assert(!flags); role->inherit_options(&flags, options, parent_flags, parent_options); + options_copy = qdict_clone_shallow(options); + opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort); + qemu_opts_absorb_qdict(opts, options_copy, NULL); + update_flags_from_options(&flags, opts); + qemu_opts_del(opts); + QDECREF(options_copy); } /* Old values are used for options that aren't set yet */ -- 2.13.6 From: Fam Zheng <famz@redhat.com> Signed-off-by: Fam Zheng <famz@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/153 | 12 ++++++++++++ tests/qemu-iotests/153.out | 5 +++++ 2 files changed, 17 insertions(+) diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/153 +++ b/tests/qemu-iotests/153 @@ -XXX,XX +XXX,XX @@ rm -f "${TEST_IMG}.lnk" &>/dev/null ln -s ${TEST_IMG} "${TEST_IMG}.lnk" || echo "Failed to create link" _run_qemu_with_images "${TEST_IMG}.lnk" "${TEST_IMG}" +echo +echo "== Active commit to intermediate layer should work when base in use ==" +_launch_qemu -drive format=$IMGFMT,file="${TEST_IMG}.a",id=drive0,if=none \ + -device virtio-blk,drive=drive0 + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'qmp_capabilities' }" \ + 'return' +_run_cmd $QEMU_IMG commit -b "${TEST_IMG}.b" "${TEST_IMG}.c" + +_cleanup_qemu + _launch_qemu _send_qemu_cmd $QEMU_HANDLE \ diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/153.out +++ b/tests/qemu-iotests/153.out @@ -XXX,XX +XXX,XX @@ Is another process using the image? == Symbolic link == QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2: Failed to get "write" lock Is another process using the image? + +== Active commit to intermediate layer should work when base in use == +{"return": {}} + +_qemu_img_wrapper commit -b TEST_DIR/t.qcow2.b TEST_DIR/t.qcow2.c {"return": {}} Adding drive -- 2.13.6 This adds the .bdrv_co_create driver callback to parallels, which enables image creation over QMP. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Jeff Cody <jcody@redhat.com> --- qapi/block-core.json | 18 ++++- block/parallels.c | 199 ++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 168 insertions(+), 49 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 @@ 'size': 'size' } } ## +# @BlockdevCreateOptionsParallels: +# +# Driver specific image creation options for parallels. +# +# @file Node to create the image format on +# @size Size of the virtual disk in bytes +# @cluster-size Cluster size in bytes (default: 1 MB) +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsParallels', + 'data': { 'file': 'BlockdevRef', + 'size': 'size', + '*cluster-size': 'size' } } + +## # @BlockdevQcow2Version: # # @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2) @@ -XXX,XX +XXX,XX @@ 'null-aio': 'BlockdevCreateNotSupported', 'null-co': 'BlockdevCreateNotSupported', 'nvme': 'BlockdevCreateNotSupported', - 'parallels': 'BlockdevCreateNotSupported', + 'parallels': 'BlockdevCreateOptionsParallels', 'qcow2': 'BlockdevCreateOptionsQcow2', 'qcow': 'BlockdevCreateNotSupported', 'qed': 'BlockdevCreateNotSupported', 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 @@ #include "sysemu/block-backend.h" #include "qemu/module.h" #include "qemu/option.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-visit-block-core.h" #include "qemu/bswap.h" #include "qemu/bitmap.h" #include "migration/blocker.h" @@ -XXX,XX +XXX,XX @@ static QemuOptsList parallels_runtime_opts = { }, }; +static QemuOptsList parallels_create_opts = { + .name = "parallels-create-opts", + .head = QTAILQ_HEAD_INITIALIZER(parallels_create_opts.head), + .desc = { + { + .name = BLOCK_OPT_SIZE, + .type = QEMU_OPT_SIZE, + .help = "Virtual disk size", + }, + { + .name = BLOCK_OPT_CLUSTER_SIZE, + .type = QEMU_OPT_SIZE, + .help = "Parallels image cluster size", + .def_value_str = stringify(DEFAULT_CLUSTER_SIZE), + }, + { /* end of list */ } + } +}; + static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx) { @@ -XXX,XX +XXX,XX @@ out: } -static int coroutine_fn parallels_co_create_opts(const char *filename, - QemuOpts *opts, - Error **errp) +static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, + Error **errp) { + BlockdevCreateOptionsParallels *parallels_opts; + BlockDriverState *bs; + BlockBackend *blk; int64_t total_size, cl_size; - uint8_t tmp[BDRV_SECTOR_SIZE]; - Error *local_err = NULL; - BlockBackend *file; uint32_t bat_entries, bat_sectors; ParallelsHeader header; + uint8_t tmp[BDRV_SECTOR_SIZE]; int ret; - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - cl_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, - DEFAULT_CLUSTER_SIZE), BDRV_SECTOR_SIZE); + assert(opts->driver == BLOCKDEV_DRIVER_PARALLELS); + parallels_opts = &opts->u.parallels; + + /* Sanity checks */ + total_size = parallels_opts->size; + + if (parallels_opts->has_cluster_size) { + cl_size = parallels_opts->cluster_size; + } else { + cl_size = DEFAULT_CLUSTER_SIZE; + } + if (total_size >= MAX_PARALLELS_IMAGE_FACTOR * cl_size) { - error_propagate(errp, local_err); + error_setg(errp, "Image size is too large for this cluster size"); return -E2BIG; } - ret = bdrv_create_file(filename, opts, &local_err); - if (ret < 0) { - error_propagate(errp, local_err); - return ret; + if (!QEMU_IS_ALIGNED(total_size, BDRV_SECTOR_SIZE)) { + error_setg(errp, "Image size must be a multiple of 512 bytes"); + return -EINVAL; } - file = blk_new_open(filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, - &local_err); - if (file == NULL) { - error_propagate(errp, local_err); + if (!QEMU_IS_ALIGNED(cl_size, BDRV_SECTOR_SIZE)) { + error_setg(errp, "Cluster size must be a multiple of 512 bytes"); + return -EINVAL; + } + + /* Create BlockBackend to write to the image */ + bs = bdrv_open_blockdev_ref(parallels_opts->file, errp); + if (bs == NULL) { return -EIO; } - blk_set_allow_write_beyond_eof(file, true); + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); + ret = blk_insert_bs(blk, bs, errp); + if (ret < 0) { + goto out; + } + blk_set_allow_write_beyond_eof(blk, true); - ret = blk_truncate(file, 0, PREALLOC_MODE_OFF, errp); + /* Create image format */ + ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp); if (ret < 0) { - goto exit; + goto out; } bat_entries = DIV_ROUND_UP(total_size, cl_size); @@ -XXX,XX +XXX,XX @@ static int coroutine_fn parallels_co_create_opts(const char *filename, memset(tmp, 0, sizeof(tmp)); memcpy(tmp, &header, sizeof(header)); - ret = blk_pwrite(file, 0, tmp, BDRV_SECTOR_SIZE, 0); + ret = blk_pwrite(blk, 0, tmp, BDRV_SECTOR_SIZE, 0); if (ret < 0) { goto exit; } - ret = blk_pwrite_zeroes(file, BDRV_SECTOR_SIZE, + ret = blk_pwrite_zeroes(blk, BDRV_SECTOR_SIZE, (bat_sectors - 1) << BDRV_SECTOR_BITS, 0); if (ret < 0) { goto exit; } - ret = 0; -done: - blk_unref(file); + ret = 0; +out: + blk_unref(blk); + bdrv_unref(bs); return ret; exit: error_setg_errno(errp, -ret, "Failed to create Parallels image"); - goto done; + goto out; +} + +static int coroutine_fn parallels_co_create_opts(const char *filename, + QemuOpts *opts, + Error **errp) +{ + BlockdevCreateOptions *create_options = NULL; + Error *local_err = NULL; + BlockDriverState *bs = NULL; + QDict *qdict = NULL; + QObject *qobj; + Visitor *v; + int ret; + + static const QDictRenames opt_renames[] = { + { BLOCK_OPT_CLUSTER_SIZE, "cluster-size" }, + { NULL, NULL }, + }; + + /* Parse options and convert legacy syntax */ + qdict = qemu_opts_to_qdict_filtered(opts, NULL, &parallels_create_opts, + true); + + if (!qdict_rename_keys(qdict, opt_renames, errp)) { + ret = -EINVAL; + goto done; + } + + /* Create and open the file (protocol layer) */ + ret = bdrv_create_file(filename, opts, &local_err); + if (ret < 0) { + error_propagate(errp, local_err); + goto done; + } + + bs = bdrv_open(filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); + if (bs == NULL) { + ret = -EIO; + goto done; + } + + /* Now get the QAPI type BlockdevCreateOptions */ + qdict_put_str(qdict, "driver", "parallels"); + qdict_put_str(qdict, "file", bs->node_name); + + qobj = qdict_crumple(qdict, errp); + QDECREF(qdict); + qdict = qobject_to_qdict(qobj); + if (qdict == NULL) { + ret = -EINVAL; + goto done; + } + + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); + visit_free(v); + + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto done; + } + + /* Silently round up sizes */ + create_options->u.parallels.size = + ROUND_UP(create_options->u.parallels.size, BDRV_SECTOR_SIZE); + create_options->u.parallels.cluster_size = + ROUND_UP(create_options->u.parallels.cluster_size, BDRV_SECTOR_SIZE); + + /* Create the Parallels image (format layer) */ + ret = parallels_co_create(create_options, errp); + if (ret < 0) { + goto done; + } + ret = 0; + +done: + QDECREF(qdict); + bdrv_unref(bs); + qapi_free_BlockdevCreateOptions(create_options); + return ret; } @@ -XXX,XX +XXX,XX @@ static void parallels_close(BlockDriverState *bs) error_free(s->migration_blocker); } -static QemuOptsList parallels_create_opts = { - .name = "parallels-create-opts", - .head = QTAILQ_HEAD_INITIALIZER(parallels_create_opts.head), - .desc = { - { - .name = BLOCK_OPT_SIZE, - .type = QEMU_OPT_SIZE, - .help = "Virtual disk size", - }, - { - .name = BLOCK_OPT_CLUSTER_SIZE, - .type = QEMU_OPT_SIZE, - .help = "Parallels image cluster size", - .def_value_str = stringify(DEFAULT_CLUSTER_SIZE), - }, - { /* end of list */ } - } -}; - static BlockDriver bdrv_parallels = { .format_name = "parallels", .instance_size = sizeof(BDRVParallelsState), @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_parallels = { .bdrv_co_readv = parallels_co_readv, .bdrv_co_writev = parallels_co_writev, .supports_backing = true, + .bdrv_co_create = parallels_co_create, .bdrv_co_create_opts = parallels_co_create_opts, .bdrv_co_check = parallels_co_check, .create_opts = &parallels_create_opts, -- 2.13.6 Originally we added parallels as a read-only format to qemu-iotests where we did just some tests with a binary image. Since then, write and image creation support has been added to the driver, so we can now enable it in _supported_fmt generic. The driver doesn't support migration yet, though, so we need to add it to the list of exceptions in 181. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Jeff Cody <jcody@redhat.com> --- tests/qemu-iotests/181 | 2 +- tests/qemu-iotests/check | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/qemu-iotests/181 b/tests/qemu-iotests/181 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/181 +++ b/tests/qemu-iotests/181 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt generic # Formats that do not support live migration -_unsupported_fmt qcow vdi vhdx vmdk vpc vvfat +_unsupported_fmt qcow vdi vhdx vmdk vpc vvfat parallels _supported_proto generic _supported_os Linux diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -XXX,XX +XXX,XX @@ testlist options -parallels) IMGFMT=parallels - IMGFMT_GENERIC=false xpand=false ;; -- 2.13.6 This adds the .bdrv_co_create driver callback to qcow, which enables image creation over QMP. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Jeff Cody <jcody@redhat.com> --- qapi/block-core.json | 21 +++++- block/qcow.c | 196 ++++++++++++++++++++++++++++++++++----------------- 2 files changed, 150 insertions(+), 67 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 @@ '*cluster-size': 'size' } } ## +# @BlockdevCreateOptionsQcow: +# +# Driver specific image creation options for qcow. +# +# @file Node to create the image format on +# @size Size of the virtual disk in bytes +# @backing-file File name of the backing file if a backing file +# should be used +# @encrypt Encryption options if the image should be encrypted +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsQcow', + 'data': { 'file': 'BlockdevRef', + 'size': 'size', + '*backing-file': 'str', + '*encrypt': 'QCryptoBlockCreateOptions' } } + +## # @BlockdevQcow2Version: # # @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2) @@ -XXX,XX +XXX,XX @@ 'null-co': 'BlockdevCreateNotSupported', 'nvme': 'BlockdevCreateNotSupported', 'parallels': 'BlockdevCreateOptionsParallels', + 'qcow': 'BlockdevCreateOptionsQcow', 'qcow2': 'BlockdevCreateOptionsQcow2', - 'qcow': 'BlockdevCreateNotSupported', 'qed': 'BlockdevCreateNotSupported', 'quorum': 'BlockdevCreateNotSupported', 'raw': 'BlockdevCreateNotSupported', diff --git a/block/qcow.c b/block/qcow.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -XXX,XX +XXX,XX @@ #include <zlib.h> #include "qapi/qmp/qdict.h" #include "qapi/qmp/qstring.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-visit-block-core.h" #include "crypto/block.h" #include "migration/blocker.h" #include "block/crypto.h" @@ -XXX,XX +XXX,XX @@ typedef struct BDRVQcowState { Error *migration_blocker; } BDRVQcowState; +static QemuOptsList qcow_create_opts; + static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset); static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename) @@ -XXX,XX +XXX,XX @@ static void qcow_close(BlockDriverState *bs) error_free(s->migration_blocker); } -static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts, - Error **errp) +static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts, + Error **errp) { + BlockdevCreateOptionsQcow *qcow_opts; int header_size, backing_filename_len, l1_size, shift, i; QCowHeader header; uint8_t *tmp; int64_t total_size = 0; - char *backing_file = NULL; - Error *local_err = NULL; int ret; + BlockDriverState *bs; BlockBackend *qcow_blk; - char *encryptfmt = NULL; - QDict *options; - QDict *encryptopts = NULL; - QCryptoBlockCreateOptions *crypto_opts = NULL; QCryptoBlock *crypto = NULL; - /* Read out options */ - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); + assert(opts->driver == BLOCKDEV_DRIVER_QCOW); + qcow_opts = &opts->u.qcow; + + /* Sanity checks */ + total_size = qcow_opts->size; if (total_size == 0) { error_setg(errp, "Image size is too small, cannot be zero length"); - ret = -EINVAL; - goto cleanup; + return -EINVAL; } - backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE); - encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT); - if (encryptfmt) { - if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) { - error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and " - BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive"); - ret = -EINVAL; - goto cleanup; - } - } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) { - encryptfmt = g_strdup("aes"); + if (qcow_opts->has_encrypt && + qcow_opts->encrypt->format != Q_CRYPTO_BLOCK_FORMAT_QCOW) + { + error_setg(errp, "Unsupported encryption format"); + return -EINVAL; } - ret = bdrv_create_file(filename, opts, &local_err); - if (ret < 0) { - error_propagate(errp, local_err); - goto cleanup; + /* Create BlockBackend to write to the image */ + bs = bdrv_open_blockdev_ref(qcow_opts->file, errp); + if (bs == NULL) { + return -EIO; } - qcow_blk = blk_new_open(filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, - &local_err); - if (qcow_blk == NULL) { - error_propagate(errp, local_err); - ret = -EIO; - goto cleanup; + qcow_blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); + ret = blk_insert_bs(qcow_blk, bs, errp); + if (ret < 0) { + goto exit; } - blk_set_allow_write_beyond_eof(qcow_blk, true); + /* Create image format */ ret = blk_truncate(qcow_blk, 0, PREALLOC_MODE_OFF, errp); if (ret < 0) { goto exit; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts header.size = cpu_to_be64(total_size); header_size = sizeof(header); backing_filename_len = 0; - if (backing_file) { - if (strcmp(backing_file, "fat:")) { + if (qcow_opts->has_backing_file) { + if (strcmp(qcow_opts->backing_file, "fat:")) { header.backing_file_offset = cpu_to_be64(header_size); - backing_filename_len = strlen(backing_file); + backing_filename_len = strlen(qcow_opts->backing_file); header.backing_file_size = cpu_to_be32(backing_filename_len); header_size += backing_filename_len; } else { /* special backing file for vvfat */ - g_free(backing_file); - backing_file = NULL; + qcow_opts->has_backing_file = false; } header.cluster_bits = 9; /* 512 byte cluster to avoid copying unmodified sectors */ @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts header.l1_table_offset = cpu_to_be64(header_size); - options = qemu_opts_to_qdict(opts, NULL); - qdict_extract_subqdict(options, &encryptopts, "encrypt."); - QDECREF(options); - if (encryptfmt) { - if (!g_str_equal(encryptfmt, "aes")) { - error_setg(errp, "Unknown encryption format '%s', expected 'aes'", - encryptfmt); - ret = -EINVAL; - goto exit; - } + if (qcow_opts->has_encrypt) { header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES); - crypto_opts = block_crypto_create_opts_init( - Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, errp); - if (!crypto_opts) { - ret = -EINVAL; - goto exit; - } - - crypto = qcrypto_block_create(crypto_opts, "encrypt.", + crypto = qcrypto_block_create(qcow_opts->encrypt, "encrypt.", NULL, NULL, NULL, errp); if (!crypto) { ret = -EINVAL; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts goto exit; } - if (backing_file) { + if (qcow_opts->has_backing_file) { ret = blk_pwrite(qcow_blk, sizeof(header), - backing_file, backing_filename_len, 0); + qcow_opts->backing_file, backing_filename_len, 0); if (ret != backing_filename_len) { goto exit; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts ret = 0; exit: blk_unref(qcow_blk); -cleanup: - QDECREF(encryptopts); - g_free(encryptfmt); qcrypto_block_free(crypto); - qapi_free_QCryptoBlockCreateOptions(crypto_opts); - g_free(backing_file); + return ret; +} + +static int coroutine_fn qcow_co_create_opts(const char *filename, + QemuOpts *opts, Error **errp) +{ + BlockdevCreateOptions *create_options = NULL; + BlockDriverState *bs = NULL; + QDict *qdict = NULL; + QObject *qobj; + Visitor *v; + const char *val; + Error *local_err = NULL; + int ret; + + static const QDictRenames opt_renames[] = { + { BLOCK_OPT_BACKING_FILE, "backing-file" }, + { BLOCK_OPT_ENCRYPT, BLOCK_OPT_ENCRYPT_FORMAT }, + { NULL, NULL }, + }; + + /* Parse options and convert legacy syntax */ + qdict = qemu_opts_to_qdict_filtered(opts, NULL, &qcow_create_opts, true); + + val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT); + if (val && !strcmp(val, "on")) { + qdict_put_str(qdict, BLOCK_OPT_ENCRYPT, "qcow"); + } else if (val && !strcmp(val, "off")) { + qdict_del(qdict, BLOCK_OPT_ENCRYPT); + } + + val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT); + if (val && !strcmp(val, "aes")) { + qdict_put_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT, "qcow"); + } + + if (!qdict_rename_keys(qdict, opt_renames, errp)) { + ret = -EINVAL; + goto fail; + } + + /* Create and open the file (protocol layer) */ + ret = bdrv_create_file(filename, opts, &local_err); + if (ret < 0) { + error_propagate(errp, local_err); + goto fail; + } + + bs = bdrv_open(filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); + if (bs == NULL) { + ret = -EIO; + goto fail; + } + + /* Now get the QAPI type BlockdevCreateOptions */ + qdict_put_str(qdict, "driver", "qcow"); + qdict_put_str(qdict, "file", bs->node_name); + + qobj = qdict_crumple(qdict, errp); + QDECREF(qdict); + qdict = qobject_to_qdict(qobj); + if (qdict == NULL) { + ret = -EINVAL; + goto fail; + } + + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); + visit_free(v); + + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto fail; + } + + /* Silently round up size */ + assert(create_options->driver == BLOCKDEV_DRIVER_QCOW); + create_options->u.qcow.size = + ROUND_UP(create_options->u.qcow.size, BDRV_SECTOR_SIZE); + + /* Create the qcow image (format layer) */ + ret = qcow_co_create(create_options, errp); + if (ret < 0) { + goto fail; + } + + ret = 0; +fail: + QDECREF(qdict); + bdrv_unref(bs); + qapi_free_BlockdevCreateOptions(create_options); return ret; } @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_qcow = { .bdrv_close = qcow_close, .bdrv_child_perm = bdrv_format_default_perms, .bdrv_reopen_prepare = qcow_reopen_prepare, + .bdrv_co_create = qcow_co_create, .bdrv_co_create_opts = qcow_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, .supports_backing = true, -- 2.13.6 This adds the .bdrv_co_create driver callback to qed, which enables image creation over QMP. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- qapi/block-core.json | 25 ++++++- block/qed.c | 204 ++++++++++++++++++++++++++++++++++----------------- 2 files changed, 162 insertions(+), 67 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 @@ '*refcount-bits': 'int' } } ## +# @BlockdevCreateOptionsQed: +# +# Driver specific image creation options for qed. +# +# @file Node to create the image format on +# @size Size of the virtual disk in bytes +# @backing-file File name of the backing file if a backing file +# should be used +# @backing-fmt Name of the block driver to use for the backing file +# @cluster-size Cluster size in bytes (default: 65536) +# @table-size L1/L2 table size (in clusters) +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsQed', + 'data': { 'file': 'BlockdevRef', + 'size': 'size', + '*backing-file': 'str', + '*backing-fmt': 'BlockdevDriver', + '*cluster-size': 'size', + '*table-size': 'int' } } + +## # @BlockdevCreateOptionsRbd: # # Driver specific image creation options for rbd/Ceph. @@ -XXX,XX +XXX,XX @@ 'parallels': 'BlockdevCreateOptionsParallels', 'qcow': 'BlockdevCreateOptionsQcow', 'qcow2': 'BlockdevCreateOptionsQcow2', - 'qed': 'BlockdevCreateNotSupported', + 'qed': 'BlockdevCreateOptionsQed', 'quorum': 'BlockdevCreateNotSupported', 'raw': 'BlockdevCreateNotSupported', 'rbd': 'BlockdevCreateOptionsRbd', 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 @@ #include "trace.h" #include "qed.h" #include "sysemu/block-backend.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-visit-block-core.h" + +static QemuOptsList qed_create_opts; static int bdrv_qed_probe(const uint8_t *buf, int buf_size, const char *filename) @@ -XXX,XX +XXX,XX @@ static void bdrv_qed_close(BlockDriverState *bs) qemu_vfree(s->l1_table); } -static int qed_create(const char *filename, uint32_t cluster_size, - uint64_t image_size, uint32_t table_size, - const char *backing_file, const char *backing_fmt, - QemuOpts *opts, Error **errp) +static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts, + Error **errp) { - QEDHeader header = { - .magic = QED_MAGIC, - .cluster_size = cluster_size, - .table_size = table_size, - .header_size = 1, - .features = 0, - .compat_features = 0, - .l1_table_offset = cluster_size, - .image_size = image_size, - }; + BlockdevCreateOptionsQed *qed_opts; + BlockBackend *blk = NULL; + BlockDriverState *bs = NULL; + + QEDHeader header; QEDHeader le_header; uint8_t *l1_table = NULL; - size_t l1_size = header.cluster_size * header.table_size; - Error *local_err = NULL; + size_t l1_size; int ret = 0; - BlockBackend *blk; - ret = bdrv_create_file(filename, opts, &local_err); - if (ret < 0) { - error_propagate(errp, local_err); - return ret; + assert(opts->driver == BLOCKDEV_DRIVER_QED); + qed_opts = &opts->u.qed; + + /* Validate options and set default values */ + if (!qed_opts->has_cluster_size) { + qed_opts->cluster_size = QED_DEFAULT_CLUSTER_SIZE; + } + if (!qed_opts->has_table_size) { + qed_opts->table_size = QED_DEFAULT_TABLE_SIZE; } - blk = blk_new_open(filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, - &local_err); - if (blk == NULL) { - error_propagate(errp, local_err); + if (!qed_is_cluster_size_valid(qed_opts->cluster_size)) { + error_setg(errp, "QED cluster size must be within range [%u, %u] " + "and power of 2", + QED_MIN_CLUSTER_SIZE, QED_MAX_CLUSTER_SIZE); + return -EINVAL; + } + if (!qed_is_table_size_valid(qed_opts->table_size)) { + error_setg(errp, "QED table size must be within range [%u, %u] " + "and power of 2", + QED_MIN_TABLE_SIZE, QED_MAX_TABLE_SIZE); + return -EINVAL; + } + if (!qed_is_image_size_valid(qed_opts->size, qed_opts->cluster_size, + qed_opts->table_size)) + { + error_setg(errp, "QED image size must be a non-zero multiple of " + "cluster size and less than %" PRIu64 " bytes", + qed_max_image_size(qed_opts->cluster_size, + qed_opts->table_size)); + return -EINVAL; + } + + /* Create BlockBackend to write to the image */ + bs = bdrv_open_blockdev_ref(qed_opts->file, errp); + if (bs == NULL) { return -EIO; } + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); + ret = blk_insert_bs(blk, bs, errp); + if (ret < 0) { + goto out; + } blk_set_allow_write_beyond_eof(blk, true); + /* Prepare image format */ + header = (QEDHeader) { + .magic = QED_MAGIC, + .cluster_size = qed_opts->cluster_size, + .table_size = qed_opts->table_size, + .header_size = 1, + .features = 0, + .compat_features = 0, + .l1_table_offset = qed_opts->cluster_size, + .image_size = qed_opts->size, + }; + + l1_size = header.cluster_size * header.table_size; + /* File must start empty and grow, check truncate is supported */ ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp); if (ret < 0) { goto out; } - if (backing_file) { + if (qed_opts->has_backing_file) { header.features |= QED_F_BACKING_FILE; header.backing_filename_offset = sizeof(le_header); - header.backing_filename_size = strlen(backing_file); + header.backing_filename_size = strlen(qed_opts->backing_file); - if (qed_fmt_is_raw(backing_fmt)) { - header.features |= QED_F_BACKING_FORMAT_NO_PROBE; + if (qed_opts->has_backing_fmt) { + const char *backing_fmt = BlockdevDriver_str(qed_opts->backing_fmt); + if (qed_fmt_is_raw(backing_fmt)) { + header.features |= QED_F_BACKING_FORMAT_NO_PROBE; + } } } @@ -XXX,XX +XXX,XX @@ static int qed_create(const char *filename, uint32_t cluster_size, if (ret < 0) { goto out; } - ret = blk_pwrite(blk, sizeof(le_header), backing_file, + ret = blk_pwrite(blk, sizeof(le_header), qed_opts->backing_file, header.backing_filename_size, 0); if (ret < 0) { goto out; @@ -XXX,XX +XXX,XX @@ static int qed_create(const char *filename, uint32_t cluster_size, out: g_free(l1_table); blk_unref(blk); + bdrv_unref(bs); return ret; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename, QemuOpts *opts, Error **errp) { - uint64_t image_size = 0; - uint32_t cluster_size = QED_DEFAULT_CLUSTER_SIZE; - uint32_t table_size = QED_DEFAULT_TABLE_SIZE; - char *backing_file = NULL; - char *backing_fmt = NULL; + BlockdevCreateOptions *create_options = NULL; + QDict *qdict = NULL; + QObject *qobj; + Visitor *v; + BlockDriverState *bs = NULL; + Error *local_err = NULL; int ret; - image_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE); - backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT); - cluster_size = qemu_opt_get_size_del(opts, - BLOCK_OPT_CLUSTER_SIZE, - QED_DEFAULT_CLUSTER_SIZE); - table_size = qemu_opt_get_size_del(opts, BLOCK_OPT_TABLE_SIZE, - QED_DEFAULT_TABLE_SIZE); - - if (!qed_is_cluster_size_valid(cluster_size)) { - error_setg(errp, "QED cluster size must be within range [%u, %u] " - "and power of 2", - QED_MIN_CLUSTER_SIZE, QED_MAX_CLUSTER_SIZE); + static const QDictRenames opt_renames[] = { + { BLOCK_OPT_BACKING_FILE, "backing-file" }, + { BLOCK_OPT_BACKING_FMT, "backing-fmt" }, + { BLOCK_OPT_CLUSTER_SIZE, "cluster-size" }, + { BLOCK_OPT_TABLE_SIZE, "table-size" }, + { NULL, NULL }, + }; + + /* Parse options and convert legacy syntax */ + qdict = qemu_opts_to_qdict_filtered(opts, NULL, &qed_create_opts, true); + + if (!qdict_rename_keys(qdict, opt_renames, errp)) { ret = -EINVAL; - goto finish; + goto fail; } - if (!qed_is_table_size_valid(table_size)) { - error_setg(errp, "QED table size must be within range [%u, %u] " - "and power of 2", - QED_MIN_TABLE_SIZE, QED_MAX_TABLE_SIZE); + + /* Create and open the file (protocol layer) */ + ret = bdrv_create_file(filename, opts, &local_err); + if (ret < 0) { + error_propagate(errp, local_err); + goto fail; + } + + bs = bdrv_open(filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); + if (bs == NULL) { + ret = -EIO; + goto fail; + } + + /* Now get the QAPI type BlockdevCreateOptions */ + qdict_put_str(qdict, "driver", "qed"); + qdict_put_str(qdict, "file", bs->node_name); + + qobj = qdict_crumple(qdict, errp); + QDECREF(qdict); + qdict = qobject_to_qdict(qobj); + if (qdict == NULL) { ret = -EINVAL; - goto finish; + goto fail; } - if (!qed_is_image_size_valid(image_size, cluster_size, table_size)) { - error_setg(errp, "QED image size must be a non-zero multiple of " - "cluster size and less than %" PRIu64 " bytes", - qed_max_image_size(cluster_size, table_size)); + + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); + visit_free(v); + + if (local_err) { + error_propagate(errp, local_err); ret = -EINVAL; - goto finish; + goto fail; } - ret = qed_create(filename, cluster_size, image_size, table_size, - backing_file, backing_fmt, opts, errp); + /* Silently round up size */ + assert(create_options->driver == BLOCKDEV_DRIVER_QED); + create_options->u.qed.size = + ROUND_UP(create_options->u.qed.size, BDRV_SECTOR_SIZE); + + /* Create the qed image (format layer) */ + ret = bdrv_qed_co_create(create_options, errp); -finish: - g_free(backing_file); - g_free(backing_fmt); +fail: + QDECREF(qdict); + bdrv_unref(bs); + qapi_free_BlockdevCreateOptions(create_options); return ret; } @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_qed = { .bdrv_close = bdrv_qed_close, .bdrv_reopen_prepare = bdrv_qed_reopen_prepare, .bdrv_child_perm = bdrv_format_default_perms, + .bdrv_co_create = bdrv_qed_co_create, .bdrv_co_create_opts = bdrv_qed_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_block_status = bdrv_qed_co_block_status, -- 2.13.6 This makes the .bdrv_co_create(_opts) implementation of vdi look more like the other recently converted block drivers. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- block/vdi.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/block/vdi.c b/block/vdi.c index XXXXXXX..XXXXXXX 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options, logout("\n"); - /* Read out options. */ + /* Validate options and set default values */ bytes = vdi_opts->size; if (vdi_opts->q_static) { image_type = VDI_TYPE_STATIC; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options, goto exit; } + /* Create BlockBackend to write to the image */ bs_file = bdrv_open_blockdev_ref(vdi_opts->file, errp); if (!bs_file) { ret = -EIO; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, Error *local_err = NULL; int ret; - /* Since CONFIG_VDI_BLOCK_SIZE is disabled by default, + /* Parse options and convert legacy syntax. + * + * Since CONFIG_VDI_BLOCK_SIZE is disabled by default, * cluster-size is not part of the QAPI schema; therefore we have * to parse it before creating the QAPI object. */ #if defined(CONFIG_VDI_BLOCK_SIZE) @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vdi_create_opts, true); + /* Create and open the file (protocol layer) */ ret = bdrv_create_file(filename, opts, errp); if (ret < 0) { goto done; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, goto done; } + /* Silently round up size */ assert(create_options->driver == BLOCKDEV_DRIVER_VDI); create_options->u.vdi.size = ROUND_UP(create_options->u.vdi.size, BDRV_SECTOR_SIZE); + /* Create the vdi image (format layer) */ ret = vdi_co_do_create(create_options, block_size, errp); done: QDECREF(qdict); @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_vdi = { .bdrv_close = vdi_close, .bdrv_reopen_prepare = vdi_reopen_prepare, .bdrv_child_perm = bdrv_format_default_perms, - .bdrv_co_create_opts = vdi_co_create_opts, .bdrv_co_create = vdi_co_create, + .bdrv_co_create_opts = vdi_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_block_status = vdi_co_block_status, .bdrv_make_empty = vdi_make_empty, -- 2.13.6 This adds the .bdrv_co_create driver callback to vhdx, which enables image creation over QMP. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- qapi/block-core.json | 40 +++++++++- block/vhdx.c | 216 ++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 203 insertions(+), 53 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 @@ '*static': 'bool' } } ## +# @BlockdevVhdxSubformat: +# +# @dynamic: Growing image file +# @fixed: Preallocated fixed-size image file +# +# Since: 2.12 +## +{ 'enum': 'BlockdevVhdxSubformat', + 'data': [ 'dynamic', 'fixed' ] } + +## +# @BlockdevCreateOptionsVhdx: +# +# Driver specific image creation options for vhdx. +# +# @file Node to create the image format on +# @size Size of the virtual disk in bytes +# @log-size Log size in bytes, must be a multiple of 1 MB +# (default: 1 MB) +# @block-size Block size in bytes, must be a multiple of 1 MB and not +# larger than 256 MB (default: automatically choose a block +# size depending on the image size) +# @subformat vhdx subformat (default: dynamic) +# @block-state-zero Force use of payload blocks of type 'ZERO'. Non-standard, +# but default. Do not set to 'off' when using 'qemu-img +# convert' with subformat=dynamic. +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsVhdx', + 'data': { 'file': 'BlockdevRef', + 'size': 'size', + '*log-size': 'size', + '*block-size': 'size', + '*subformat': 'BlockdevVhdxSubformat', + '*block-state-zero': 'bool' } } + +## # @BlockdevCreateNotSupported: # # This is used for all drivers that don't support creating images. @@ -XXX,XX +XXX,XX @@ 'ssh': 'BlockdevCreateOptionsSsh', 'throttle': 'BlockdevCreateNotSupported', 'vdi': 'BlockdevCreateOptionsVdi', - 'vhdx': 'BlockdevCreateNotSupported', + 'vhdx': 'BlockdevCreateOptionsVhdx', 'vmdk': 'BlockdevCreateNotSupported', 'vpc': 'BlockdevCreateNotSupported', 'vvfat': 'BlockdevCreateNotSupported', diff --git a/block/vhdx.c b/block/vhdx.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -XXX,XX +XXX,XX @@ #include "block/vhdx.h" #include "migration/blocker.h" #include "qemu/uuid.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-visit-block-core.h" /* Options for VHDX creation */ @@ -XXX,XX +XXX,XX @@ typedef enum VHDXImageType { VHDX_TYPE_DIFFERENCING, /* Currently unsupported */ } VHDXImageType; +static QemuOptsList vhdx_create_opts; + /* Several metadata and region table data entries are identified by * guids in a MS-specific GUID format. */ @@ -XXX,XX +XXX,XX @@ exit: * .---- ~ ----------- ~ ------------ ~ ---------------- ~ -----------. * 1MB */ -static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts, - Error **errp) +static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts, + Error **errp) { + BlockdevCreateOptionsVhdx *vhdx_opts; + BlockBackend *blk = NULL; + BlockDriverState *bs = NULL; + int ret = 0; - uint64_t image_size = (uint64_t) 2 * GiB; - uint32_t log_size = 1 * MiB; - uint32_t block_size = 0; + uint64_t image_size; + uint32_t log_size; + uint32_t block_size; uint64_t signature; uint64_t metadata_offset; bool use_zero_blocks = false; gunichar2 *creator = NULL; glong creator_items; - BlockBackend *blk; - char *type = NULL; VHDXImageType image_type; - Error *local_err = NULL; - image_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - log_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_LOG_SIZE, 0); - block_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_BLOCK_SIZE, 0); - type = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT); - use_zero_blocks = qemu_opt_get_bool_del(opts, VHDX_BLOCK_OPT_ZERO, true); + assert(opts->driver == BLOCKDEV_DRIVER_VHDX); + vhdx_opts = &opts->u.vhdx; + /* Validate options and set default values */ + image_size = vhdx_opts->size; if (image_size > VHDX_MAX_IMAGE_SIZE) { error_setg_errno(errp, EINVAL, "Image size too large; max of 64TB"); - ret = -EINVAL; - goto exit; + return -EINVAL; } - if (type == NULL) { - type = g_strdup("dynamic"); + if (!vhdx_opts->has_log_size) { + log_size = DEFAULT_LOG_SIZE; + } else { + log_size = vhdx_opts->log_size; + } + if (log_size < MiB || (log_size % MiB) != 0) { + error_setg_errno(errp, EINVAL, "Log size must be a multiple of 1 MB"); + return -EINVAL; } - if (!strcmp(type, "dynamic")) { + if (!vhdx_opts->has_block_state_zero) { + use_zero_blocks = true; + } else { + use_zero_blocks = vhdx_opts->block_state_zero; + } + + if (!vhdx_opts->has_subformat) { + vhdx_opts->subformat = BLOCKDEV_VHDX_SUBFORMAT_DYNAMIC; + } + + switch (vhdx_opts->subformat) { + case BLOCKDEV_VHDX_SUBFORMAT_DYNAMIC: image_type = VHDX_TYPE_DYNAMIC; - } else if (!strcmp(type, "fixed")) { + break; + case BLOCKDEV_VHDX_SUBFORMAT_FIXED: image_type = VHDX_TYPE_FIXED; - } else if (!strcmp(type, "differencing")) { - error_setg_errno(errp, ENOTSUP, - "Differencing files not yet supported"); - ret = -ENOTSUP; - goto exit; - } else { - error_setg(errp, "Invalid subformat '%s'", type); - ret = -EINVAL; - goto exit; + break; + default: + g_assert_not_reached(); } /* These are pretty arbitrary, and mainly designed to keep the BAT * size reasonable to load into RAM */ - if (block_size == 0) { + if (vhdx_opts->has_block_size) { + block_size = vhdx_opts->block_size; + } else { if (image_size > 32 * TiB) { block_size = 64 * MiB; } else if (image_size > (uint64_t) 100 * GiB) { @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts } } - - /* make the log size close to what was specified, but must be - * min 1MB, and multiple of 1MB */ - log_size = ROUND_UP(log_size, MiB); - - block_size = ROUND_UP(block_size, MiB); - block_size = block_size > VHDX_BLOCK_SIZE_MAX ? VHDX_BLOCK_SIZE_MAX : - block_size; - - ret = bdrv_create_file(filename, opts, &local_err); - if (ret < 0) { - error_propagate(errp, local_err); - goto exit; + if (block_size < MiB || (block_size % MiB) != 0) { + error_setg_errno(errp, EINVAL, "Block size must be a multiple of 1 MB"); + return -EINVAL; + } + if (block_size > VHDX_BLOCK_SIZE_MAX) { + error_setg_errno(errp, EINVAL, "Block size must not exceed %d", + VHDX_BLOCK_SIZE_MAX); + return -EINVAL; } - blk = blk_new_open(filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, - &local_err); - if (blk == NULL) { - error_propagate(errp, local_err); - ret = -EIO; - goto exit; + /* Create BlockBackend to write to the image */ + bs = bdrv_open_blockdev_ref(vhdx_opts->file, errp); + if (bs == NULL) { + return -EIO; } + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); + ret = blk_insert_bs(blk, bs, errp); + if (ret < 0) { + goto delete_and_exit; + } blk_set_allow_write_beyond_eof(blk, true); /* Create (A) */ @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts delete_and_exit: blk_unref(blk); -exit: - g_free(type); + bdrv_unref(bs); g_free(creator); return ret; } +static int coroutine_fn vhdx_co_create_opts(const char *filename, + QemuOpts *opts, + Error **errp) +{ + BlockdevCreateOptions *create_options = NULL; + QDict *qdict = NULL; + QObject *qobj; + Visitor *v; + BlockDriverState *bs = NULL; + Error *local_err = NULL; + int ret; + + static const QDictRenames opt_renames[] = { + { VHDX_BLOCK_OPT_LOG_SIZE, "log-size" }, + { VHDX_BLOCK_OPT_BLOCK_SIZE, "block-size" }, + { VHDX_BLOCK_OPT_ZERO, "block-state-zero" }, + { NULL, NULL }, + }; + + /* Parse options and convert legacy syntax */ + qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vhdx_create_opts, true); + + if (!qdict_rename_keys(qdict, opt_renames, errp)) { + ret = -EINVAL; + goto fail; + } + + /* Create and open the file (protocol layer) */ + ret = bdrv_create_file(filename, opts, &local_err); + if (ret < 0) { + error_propagate(errp, local_err); + goto fail; + } + + bs = bdrv_open(filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); + if (bs == NULL) { + ret = -EIO; + goto fail; + } + + /* Now get the QAPI type BlockdevCreateOptions */ + qdict_put_str(qdict, "driver", "vhdx"); + qdict_put_str(qdict, "file", bs->node_name); + + qobj = qdict_crumple(qdict, errp); + QDECREF(qdict); + qdict = qobject_to_qdict(qobj); + if (qdict == NULL) { + ret = -EINVAL; + goto fail; + } + + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); + visit_free(v); + + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto fail; + } + + /* Silently round up sizes: + * The image size is rounded to 512 bytes. Make the block and log size + * close to what was specified, but must be at least 1MB, and a multiple of + * 1 MB. Also respect VHDX_BLOCK_SIZE_MAX for block sizes. block_size = 0 + * means auto, which is represented by a missing key in QAPI. */ + assert(create_options->driver == BLOCKDEV_DRIVER_VHDX); + create_options->u.vhdx.size = + ROUND_UP(create_options->u.vhdx.size, BDRV_SECTOR_SIZE); + + if (create_options->u.vhdx.has_log_size) { + create_options->u.vhdx.log_size = + ROUND_UP(create_options->u.vhdx.log_size, MiB); + } + if (create_options->u.vhdx.has_block_size) { + create_options->u.vhdx.block_size = + ROUND_UP(create_options->u.vhdx.block_size, MiB); + + if (create_options->u.vhdx.block_size == 0) { + create_options->u.vhdx.has_block_size = false; + } + if (create_options->u.vhdx.block_size > VHDX_BLOCK_SIZE_MAX) { + create_options->u.vhdx.block_size = VHDX_BLOCK_SIZE_MAX; + } + } + + /* Create the vhdx image (format layer) */ + ret = vhdx_co_create(create_options, errp); + +fail: + QDECREF(qdict); + bdrv_unref(bs); + qapi_free_BlockdevCreateOptions(create_options); + return ret; +} + /* If opened r/w, the VHDX driver will automatically replay the log, * if one is present, inside the vhdx_open() call. * @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_vhdx = { .bdrv_child_perm = bdrv_format_default_perms, .bdrv_co_readv = vhdx_co_readv, .bdrv_co_writev = vhdx_co_writev, + .bdrv_co_create = vhdx_co_create, .bdrv_co_create_opts = vhdx_co_create_opts, .bdrv_get_info = vhdx_get_info, .bdrv_co_check = vhdx_co_check, -- 2.13.6 This adds the .bdrv_co_create driver callback to vpc, which enables image creation over QMP. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- qapi/block-core.json | 33 ++++++++++- block/vpc.c | 152 ++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 147 insertions(+), 38 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 @@ '*block-state-zero': 'bool' } } ## +# @BlockdevVpcSubformat: +# +# @dynamic: Growing image file +# @fixed: Preallocated fixed-size image file +# +# Since: 2.12 +## +{ 'enum': 'BlockdevVpcSubformat', + 'data': [ 'dynamic', 'fixed' ] } + +## +# @BlockdevCreateOptionsVpc: +# +# Driver specific image creation options for vpc (VHD). +# +# @file Node to create the image format on +# @size Size of the virtual disk in bytes +# @subformat vhdx subformat (default: dynamic) +# @force-size Force use of the exact byte size instead of rounding to the +# next size that can be represented in CHS geometry +# (default: false) +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsVpc', + 'data': { 'file': 'BlockdevRef', + 'size': 'size', + '*subformat': 'BlockdevVpcSubformat', + '*force-size': 'bool' } } + +## # @BlockdevCreateNotSupported: # # This is used for all drivers that don't support creating images. @@ -XXX,XX +XXX,XX @@ 'vdi': 'BlockdevCreateOptionsVdi', 'vhdx': 'BlockdevCreateOptionsVhdx', 'vmdk': 'BlockdevCreateNotSupported', - 'vpc': 'BlockdevCreateNotSupported', + 'vpc': 'BlockdevCreateOptionsVpc', 'vvfat': 'BlockdevCreateNotSupported', 'vxhs': 'BlockdevCreateNotSupported' } } diff --git a/block/vpc.c b/block/vpc.c index XXXXXXX..XXXXXXX 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -XXX,XX +XXX,XX @@ #include "migration/blocker.h" #include "qemu/bswap.h" #include "qemu/uuid.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-visit-block-core.h" /**************************************************************/ @@ -XXX,XX +XXX,XX @@ static QemuOptsList vpc_runtime_opts = { } }; +static QemuOptsList vpc_create_opts; + static uint32_t vpc_checksum(uint8_t* buf, size_t size) { uint32_t res = 0; @@ -XXX,XX +XXX,XX @@ static int create_fixed_disk(BlockBackend *blk, uint8_t *buf, return ret; } -static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts, - Error **errp) +static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts, + Error **errp) { + BlockdevCreateOptionsVpc *vpc_opts; + BlockBackend *blk = NULL; + BlockDriverState *bs = NULL; + uint8_t buf[1024]; VHDFooter *footer = (VHDFooter *) buf; - char *disk_type_param; int i; uint16_t cyls = 0; uint8_t heads = 0; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts, int64_t total_size; int disk_type; int ret = -EIO; - bool force_size; - Error *local_err = NULL; - BlockBackend *blk = NULL; - /* Read out options */ - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - disk_type_param = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT); - if (disk_type_param) { - if (!strcmp(disk_type_param, "dynamic")) { - disk_type = VHD_DYNAMIC; - } else if (!strcmp(disk_type_param, "fixed")) { - disk_type = VHD_FIXED; - } else { - error_setg(errp, "Invalid disk type, %s", disk_type_param); - ret = -EINVAL; - goto out; - } - } else { + assert(opts->driver == BLOCKDEV_DRIVER_VPC); + vpc_opts = &opts->u.vpc; + + /* Validate options and set default values */ + total_size = vpc_opts->size; + + if (!vpc_opts->has_subformat) { + vpc_opts->subformat = BLOCKDEV_VPC_SUBFORMAT_DYNAMIC; + } + switch (vpc_opts->subformat) { + case BLOCKDEV_VPC_SUBFORMAT_DYNAMIC: disk_type = VHD_DYNAMIC; + break; + case BLOCKDEV_VPC_SUBFORMAT_FIXED: + disk_type = VHD_FIXED; + break; + default: + g_assert_not_reached(); } - force_size = qemu_opt_get_bool_del(opts, VPC_OPT_FORCE_SIZE, false); - - ret = bdrv_create_file(filename, opts, &local_err); - if (ret < 0) { - error_propagate(errp, local_err); - goto out; + /* Create BlockBackend to write to the image */ + bs = bdrv_open_blockdev_ref(vpc_opts->file, errp); + if (bs == NULL) { + return -EIO; } - blk = blk_new_open(filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, - &local_err); - if (blk == NULL) { - error_propagate(errp, local_err); - ret = -EIO; + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); + ret = blk_insert_bs(blk, bs, errp); + if (ret < 0) { goto out; } - blk_set_allow_write_beyond_eof(blk, true); /* @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts, * we set the geometry to 65535 x 16 x 255 (CxHxS) sectors and use * the image size from the VHD footer to calculate total_sectors. */ - if (force_size) { + if (vpc_opts->force_size) { /* This will force the use of total_size for sector count, below */ cyls = VHD_CHS_MAX_C; heads = VHD_CHS_MAX_H; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts, memset(buf, 0, 1024); memcpy(footer->creator, "conectix", 8); - if (force_size) { + if (vpc_opts->force_size) { memcpy(footer->creator_app, "qem2", 4); } else { memcpy(footer->creator_app, "qemu", 4); @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts, out: blk_unref(blk); - g_free(disk_type_param); + bdrv_unref(bs); + return ret; +} + +static int coroutine_fn vpc_co_create_opts(const char *filename, + QemuOpts *opts, Error **errp) +{ + BlockdevCreateOptions *create_options = NULL; + QDict *qdict = NULL; + QObject *qobj; + Visitor *v; + BlockDriverState *bs = NULL; + Error *local_err = NULL; + int ret; + + static const QDictRenames opt_renames[] = { + { VPC_OPT_FORCE_SIZE, "force-size" }, + { NULL, NULL }, + }; + + /* Parse options and convert legacy syntax */ + qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vpc_create_opts, true); + + if (!qdict_rename_keys(qdict, opt_renames, errp)) { + ret = -EINVAL; + goto fail; + } + + /* Create and open the file (protocol layer) */ + ret = bdrv_create_file(filename, opts, &local_err); + if (ret < 0) { + error_propagate(errp, local_err); + goto fail; + } + + bs = bdrv_open(filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); + if (bs == NULL) { + ret = -EIO; + goto fail; + } + + /* Now get the QAPI type BlockdevCreateOptions */ + qdict_put_str(qdict, "driver", "vpc"); + qdict_put_str(qdict, "file", bs->node_name); + + qobj = qdict_crumple(qdict, errp); + QDECREF(qdict); + qdict = qobject_to_qdict(qobj); + if (qdict == NULL) { + ret = -EINVAL; + goto fail; + } + + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); + visit_free(v); + + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto fail; + } + + /* Silently round up size */ + assert(create_options->driver == BLOCKDEV_DRIVER_VPC); + create_options->u.vpc.size = + ROUND_UP(create_options->u.vpc.size, BDRV_SECTOR_SIZE); + + /* Create the vpc image (format layer) */ + ret = vpc_co_create(create_options, errp); + +fail: + QDECREF(qdict); + bdrv_unref(bs); + qapi_free_BlockdevCreateOptions(create_options); return ret; } + static int vpc_has_zero_init(BlockDriverState *bs) { BDRVVPCState *s = bs->opaque; @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_vpc = { .bdrv_close = vpc_close, .bdrv_reopen_prepare = vpc_reopen_prepare, .bdrv_child_perm = bdrv_format_default_perms, + .bdrv_co_create = vpc_co_create, .bdrv_co_create_opts = vpc_co_create_opts, .bdrv_co_preadv = vpc_co_preadv, -- 2.13.6 Perform the rounding to match a CHS geometry only in the legacy code path in .bdrv_co_create_opts. QMP now requires that the user already passes a CHS aligned image size, unless force-size=true is given. CHS alignment is required to make the image compatible with Virtual PC, but not for use with newer Microsoft hypervisors. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- block/vpc.c | 113 +++++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 82 insertions(+), 31 deletions(-) diff --git a/block/vpc.c b/block/vpc.c index XXXXXXX..XXXXXXX 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -XXX,XX +XXX,XX @@ static int create_fixed_disk(BlockBackend *blk, uint8_t *buf, return ret; } +static int calculate_rounded_image_size(BlockdevCreateOptionsVpc *vpc_opts, + uint16_t *out_cyls, + uint8_t *out_heads, + uint8_t *out_secs_per_cyl, + int64_t *out_total_sectors, + Error **errp) +{ + int64_t total_size = vpc_opts->size; + uint16_t cyls = 0; + uint8_t heads = 0; + uint8_t secs_per_cyl = 0; + int64_t total_sectors; + int i; + + /* + * Calculate matching total_size and geometry. Increase the number of + * sectors requested until we get enough (or fail). This ensures that + * qemu-img convert doesn't truncate images, but rather rounds up. + * + * If the image size can't be represented by a spec conformant CHS geometry, + * we set the geometry to 65535 x 16 x 255 (CxHxS) sectors and use + * the image size from the VHD footer to calculate total_sectors. + */ + if (vpc_opts->force_size) { + /* This will force the use of total_size for sector count, below */ + cyls = VHD_CHS_MAX_C; + heads = VHD_CHS_MAX_H; + secs_per_cyl = VHD_CHS_MAX_S; + } else { + total_sectors = MIN(VHD_MAX_GEOMETRY, total_size / BDRV_SECTOR_SIZE); + for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) { + calculate_geometry(total_sectors + i, &cyls, &heads, &secs_per_cyl); + } + } + + if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) { + total_sectors = total_size / BDRV_SECTOR_SIZE; + /* Allow a maximum disk size of 2040 GiB */ + if (total_sectors > VHD_MAX_SECTORS) { + error_setg(errp, "Disk size is too large, max size is 2040 GiB"); + return -EFBIG; + } + } else { + total_sectors = (int64_t) cyls * heads * secs_per_cyl; + } + + *out_total_sectors = total_sectors; + if (out_cyls) { + *out_cyls = cyls; + *out_heads = heads; + *out_secs_per_cyl = secs_per_cyl; + } + + return 0; +} + static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts, Error **errp) { @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts, uint8_t buf[1024]; VHDFooter *footer = (VHDFooter *) buf; - int i; uint16_t cyls = 0; uint8_t heads = 0; uint8_t secs_per_cyl = 0; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts, } blk_set_allow_write_beyond_eof(blk, true); - /* - * Calculate matching total_size and geometry. Increase the number of - * sectors requested until we get enough (or fail). This ensures that - * qemu-img convert doesn't truncate images, but rather rounds up. - * - * If the image size can't be represented by a spec conformant CHS geometry, - * we set the geometry to 65535 x 16 x 255 (CxHxS) sectors and use - * the image size from the VHD footer to calculate total_sectors. - */ - if (vpc_opts->force_size) { - /* This will force the use of total_size for sector count, below */ - cyls = VHD_CHS_MAX_C; - heads = VHD_CHS_MAX_H; - secs_per_cyl = VHD_CHS_MAX_S; - } else { - total_sectors = MIN(VHD_MAX_GEOMETRY, total_size / BDRV_SECTOR_SIZE); - for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) { - calculate_geometry(total_sectors + i, &cyls, &heads, &secs_per_cyl); - } + /* Get geometry and check that it matches the image size*/ + ret = calculate_rounded_image_size(vpc_opts, &cyls, &heads, &secs_per_cyl, + &total_sectors, errp); + if (ret < 0) { + goto out; } - if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) { - total_sectors = total_size / BDRV_SECTOR_SIZE; - /* Allow a maximum disk size of 2040 GiB */ - if (total_sectors > VHD_MAX_SECTORS) { - error_setg(errp, "Disk size is too large, max size is 2040 GiB"); - ret = -EFBIG; - goto out; - } - } else { - total_sectors = (int64_t)cyls * heads * secs_per_cyl; - total_size = total_sectors * BDRV_SECTOR_SIZE; + if (total_size != total_sectors * BDRV_SECTOR_SIZE) { + error_setg(errp, "The requested image size cannot be represented in " + "CHS geometry"); + error_append_hint(errp, "Try size=%llu or force-size=on (the " + "latter makes the image incompatible with " + "Virtual PC)", + total_sectors * BDRV_SECTOR_SIZE); + ret = -EINVAL; + goto out; } /* Prepare the Hard Disk Footer */ @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vpc_co_create_opts(const char *filename, create_options->u.vpc.size = ROUND_UP(create_options->u.vpc.size, BDRV_SECTOR_SIZE); + if (!create_options->u.vpc.force_size) { + int64_t total_sectors; + ret = calculate_rounded_image_size(&create_options->u.vpc, NULL, NULL, + NULL, &total_sectors, errp); + if (ret < 0) { + goto fail; + } + + create_options->u.vpc.size = total_sectors * BDRV_SECTOR_SIZE; + } + + /* Create the vpc image (format layer) */ ret = vpc_co_create(create_options, errp); -- 2.13.6 From: Liang Li <liliang.opensource@gmail.com> When doing drive mirror to a low speed shared storage, if there was heavy BLK IO write workload in VM after the 'ready' event, drive mirror block job can't be canceled immediately, it would keep running until the heavy BLK IO workload stopped in the VM. Libvirt depends on the current block-job-cancel semantics, which is that when used without a flag after the 'ready' event, the command blocks until data is in sync. However, these semantics are awkward in other situations, for example, people may use drive mirror for realtime backups while still wanting to use block live migration. Libvirt cannot start a block live migration while another drive mirror is in progress, but the user would rather abandon the backup attempt as broken and proceed with the live migration than be stuck waiting for the current drive mirror backup to finish. The drive-mirror command already includes a 'force' flag, which libvirt does not use, although it documented the flag as only being useful to quit a job which is paused. However, since quitting a paused job has the same effect as abandoning a backup in a non-paused job (namely, the destination file is not in sync, and the command completes immediately), we can just improve the documentation to make the force flag obviously useful. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Jeff Cody <jcody@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Cc: Eric Blake <eblake@redhat.com> Cc: John Snow <jsnow@redhat.com> Reported-by: Huaitong Han <huanhuaitong@didichuxing.com> Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com> Signed-off-by: Liang Li <liliangleo@didichuxing.com> Signed-off-by: Jeff Cody <jcody@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 5 +++-- include/block/blockjob.h | 12 ++++++++++-- block/mirror.c | 10 ++++------ blockdev.c | 4 ++-- blockjob.c | 16 +++++++++------- tests/test-blockjob-txn.c | 8 ++++---- hmp-commands.hx | 3 ++- 7 files changed, 34 insertions(+), 24 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 @@ # the name of the parameter), but since QEMU 2.7 it can have # other values. # -# @force: whether to allow cancellation of a paused job (default -# false). Since 1.3. +# @force: If true, and the job has already emitted the event BLOCK_JOB_READY, +# abandon the job immediately (even if it is paused) instead of waiting +# for the destination to complete its final synchronization (since 1.3) # # Returns: Nothing on success # If no background operation is active on this device, DeviceNotActive 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 { bool cancelled; /** + * Set to true if the job should abort immediately without waiting + * for data to be in sync. + */ + bool force; + + /** * Counter for pause request. If non-zero, the block job is either paused, * or if busy == true will pause itself as soon as possible. */ @@ -XXX,XX +XXX,XX @@ void block_job_start(BlockJob *job); /** * block_job_cancel: * @job: The job to be canceled. + * @force: Quit a job without waiting for data to be in sync. * * Asynchronously cancel the specified job. */ -void block_job_cancel(BlockJob *job); +void block_job_cancel(BlockJob *job, bool force); /** * block_job_complete: @@ -XXX,XX +XXX,XX @@ void block_job_user_resume(BlockJob *job, Error **errp); /** * block_job_user_cancel: * @job: The job to be cancelled. + * @force: Quit a job without waiting for data to be in sync. * * Cancels the specified job, but may refuse to do so if the * operation isn't currently meaningful. */ -void block_job_user_cancel(BlockJob *job, Error **errp); +void block_job_user_cancel(BlockJob *job, bool force, Error **errp); /** * block_job_cancel_sync: 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 coroutine_fn mirror_run(void *opaque) ret = 0; trace_mirror_before_sleep(s, cnt, s->synced, delay_ns); - if (!s->synced) { - block_job_sleep_ns(&s->common, delay_ns); - if (block_job_is_cancelled(&s->common)) { - break; - } + if (block_job_is_cancelled(&s->common) && s->common.force) { + break; } else if (!should_complete) { delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0); block_job_sleep_ns(&s->common, delay_ns); @@ -XXX,XX +XXX,XX @@ immediate_exit: * or it was cancelled prematurely so that we do not guarantee that * the target is a copy of the source. */ - assert(ret < 0 || (!s->synced && block_job_is_cancelled(&s->common))); + assert(ret < 0 || ((s->common.force || !s->synced) && + block_job_is_cancelled(&s->common))); assert(need_drain); mirror_wait_for_all_io(s); } 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) aio_context_acquire(aio_context); if (bs->job) { - block_job_cancel(bs->job); + block_job_cancel(bs->job, false); } aio_context_release(aio_context); @@ -XXX,XX +XXX,XX @@ void qmp_block_job_cancel(const char *device, } trace_qmp_block_job_cancel(job); - block_job_user_cancel(job, errp); + block_job_user_cancel(job, force, errp); out: aio_context_release(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 int block_job_finalize_single(BlockJob *job) return 0; } -static void block_job_cancel_async(BlockJob *job) +static void block_job_cancel_async(BlockJob *job, bool force) { if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) { block_job_iostatus_reset(job); @@ -XXX,XX +XXX,XX @@ static void block_job_cancel_async(BlockJob *job) job->pause_count--; } job->cancelled = true; + /* To prevent 'force == false' overriding a previous 'force == true' */ + job->force |= force; } static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *), bool lock) @@ -XXX,XX +XXX,XX @@ static void block_job_completed_txn_abort(BlockJob *job) * on the caller, so leave it. */ QLIST_FOREACH(other_job, &txn->jobs, txn_list) { if (other_job != job) { - block_job_cancel_async(other_job); + block_job_cancel_async(other_job, false); } } while (!QLIST_EMPTY(&txn->jobs)) { @@ -XXX,XX +XXX,XX @@ void block_job_user_resume(BlockJob *job, Error **errp) block_job_resume(job); } -void block_job_cancel(BlockJob *job) +void block_job_cancel(BlockJob *job, bool force) { if (job->status == BLOCK_JOB_STATUS_CONCLUDED) { block_job_do_dismiss(job); return; } - block_job_cancel_async(job); + block_job_cancel_async(job, force); if (!block_job_started(job)) { block_job_completed(job, -ECANCELED); } else if (job->deferred_to_main_loop) { @@ -XXX,XX +XXX,XX @@ void block_job_cancel(BlockJob *job) } } -void block_job_user_cancel(BlockJob *job, Error **errp) +void block_job_user_cancel(BlockJob *job, bool force, Error **errp) { if (block_job_apply_verb(job, BLOCK_JOB_VERB_CANCEL, errp)) { return; } - block_job_cancel(job); + block_job_cancel(job, force); } /* A wrapper around block_job_cancel() taking an Error ** parameter so it may be @@ -XXX,XX +XXX,XX @@ void block_job_user_cancel(BlockJob *job, Error **errp) * function pointer casts there. */ static void block_job_cancel_err(BlockJob *job, Error **errp) { - block_job_cancel(job); + block_job_cancel(job, false); } int block_job_cancel_sync(BlockJob *job) diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-blockjob-txn.c +++ b/tests/test-blockjob-txn.c @@ -XXX,XX +XXX,XX @@ static void test_single_job(int expected) block_job_start(job); if (expected == -ECANCELED) { - block_job_cancel(job); + block_job_cancel(job, false); } while (result == -EINPROGRESS) { @@ -XXX,XX +XXX,XX @@ static void test_pair_jobs(int expected1, int expected2) block_job_txn_unref(txn); if (expected1 == -ECANCELED) { - block_job_cancel(job1); + block_job_cancel(job1, false); } if (expected2 == -ECANCELED) { - block_job_cancel(job2); + block_job_cancel(job2, false); } while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) { @@ -XXX,XX +XXX,XX @@ static void test_pair_jobs_fail_cancel_race(void) block_job_start(job1); block_job_start(job2); - block_job_cancel(job1); + block_job_cancel(job1, 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/hmp-commands.hx b/hmp-commands.hx index XXXXXXX..XXXXXXX 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -XXX,XX +XXX,XX @@ ETEXI .args_type = "force:-f,device:B", .params = "[-f] device", .help = "stop an active background block operation (use -f" - "\n\t\t\t if the operation is currently paused)", + "\n\t\t\t if you want to abort the operation immediately" + "\n\t\t\t instead of keep running until data is in sync)", .cmd = hmp_block_job_cancel, }, -- 2.13.6 The following changes since commit 13356edb87506c148b163b8c7eb0695647d00c2a: Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging (2023-01-24 09:45:33 +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 d570177b50c389f379f93183155a27d44856ab46: qemu-img: Change info key names for protocol nodes (2023-02-01 16:52:33 +0100) v4: - Fixed the 'qemu-img-close-errors' test case to run only on Linux and only with the file protocol, use qemu-io instead of truncate v3: - Make the compiler happier on BSD and CentOS Stream 8 v2: - Rebased to resolve merge conflicts in coroutine.h ---------------------------------------------------------------- Block layer patches - qemu-img info: Show protocol-level information - Move more functions to coroutines - Make coroutine annotations ready for static analysis - qemu-img: Fix exit code for errors closing the image - qcow2 bitmaps: Fix theoretical corruption in error path - pflash: Only load non-zero parts of backend image to save memory - Code cleanup and test case improvements ---------------------------------------------------------------- Alberto Faria (2): coroutine: annotate coroutine_fn for libclang block: Add no_coroutine_fn and coroutine_mixed_fn marker Emanuele Giuseppe Esposito (14): block-coroutine-wrapper: support void functions block: Convert bdrv_io_plug() to co_wrapper block: Convert bdrv_io_unplug() to co_wrapper block: Convert bdrv_is_inserted() to co_wrapper block: Rename refresh_total_sectors to bdrv_refresh_total_sectors block: Convert bdrv_refresh_total_sectors() to co_wrapper_mixed block-backend: use bdrv_getlength instead of blk_getlength block: use bdrv_co_refresh_total_sectors when possible block: Convert bdrv_get_allocated_file_size() to co_wrapper block: Convert bdrv_get_info() to co_wrapper_mixed block: Convert bdrv_eject() to co_wrapper block: Convert bdrv_lock_medium() to co_wrapper block: Convert bdrv_debug_event() to co_wrapper_mixed block: Rename bdrv_load/save_vmstate() to bdrv_co_load/save_vmstate() Hanna Reitz (12): block: Improve empty format-specific info dump block/file: Add file-specific image info block/vmdk: Change extent info type block: Split BlockNodeInfo off of ImageInfo qemu-img: Use BlockNodeInfo block/qapi: Let bdrv_query_image_info() recurse block/qapi: Introduce BlockGraphInfo block/qapi: Add indentation to bdrv_node_info_dump() iotests: Filter child node information iotests/106, 214, 308: Read only one size line qemu-img: Let info print block graph qemu-img: Change info key names for protocol nodes Kevin Wolf (4): qcow2: Fix theoretical corruption in store_bitmap() error path qemu-img commit: Report errors while closing the image qemu-img bitmap: Report errors while closing the image qemu-iotests: Test qemu-img bitmap/commit exit code on error Paolo Bonzini (2): qemu-io: do not reinvent the blk_pwrite_zeroes wheel block: remove bdrv_coroutine_enter Philippe Mathieu-Daudé (1): block/nbd: Add missing <qemu/bswap.h> include Thomas Huth (2): tests/qemu-iotests/312: Mark "quorum" as required driver tests/qemu-iotests/262: Check for availability of "blkverify" first Xiang Zheng (1): pflash: Only read non-zero parts of backend image qapi/block-core.json | 123 +++++++- include/block/block-common.h | 11 +- include/block/block-io.h | 41 ++- include/block/block_int-common.h | 26 +- include/block/block_int-io.h | 5 +- include/block/nbd.h | 1 + include/block/qapi.h | 14 +- include/qemu/osdep.h | 44 +++ include/sysemu/block-backend-io.h | 31 +- block.c | 88 +++--- block/blkdebug.c | 11 +- block/blkio.c | 15 +- block/blklogwrites.c | 6 +- block/blkreplay.c | 6 +- block/blkverify.c | 6 +- block/block-backend.c | 38 +-- block/commit.c | 4 +- block/copy-on-read.c | 18 +- block/crypto.c | 14 +- block/curl.c | 10 +- block/file-posix.c | 137 +++++---- block/file-win32.c | 18 +- block/filter-compress.c | 20 +- block/gluster.c | 23 +- block/io.c | 76 ++--- block/iscsi.c | 17 +- block/mirror.c | 6 +- block/monitor/block-hmp-cmds.c | 2 +- block/nbd.c | 8 +- block/nfs.c | 4 +- block/null.c | 13 +- block/nvme.c | 14 +- block/preallocate.c | 16 +- block/qapi.c | 317 ++++++++++++++++----- block/qcow.c | 5 +- block/qcow2-bitmap.c | 5 +- block/qcow2-refcount.c | 2 +- block/qcow2.c | 17 +- block/qed.c | 11 +- block/quorum.c | 8 +- block/raw-format.c | 25 +- block/rbd.c | 9 +- block/replication.c | 6 +- block/ssh.c | 4 +- block/throttle.c | 6 +- block/vdi.c | 7 +- block/vhdx.c | 5 +- block/vmdk.c | 22 +- block/vpc.c | 5 +- blockdev.c | 8 +- hw/block/block.c | 36 ++- hw/scsi/scsi-disk.c | 5 + qemu-img.c | 100 +++++-- qemu-io-cmds.c | 62 +--- tests/unit/test-block-iothread.c | 3 + scripts/block-coroutine-wrapper.py | 20 +- tests/qemu-iotests/iotests.py | 18 +- block/meson.build | 1 + tests/qemu-iotests/065 | 2 +- tests/qemu-iotests/106 | 4 +- tests/qemu-iotests/214 | 6 +- tests/qemu-iotests/262 | 3 +- tests/qemu-iotests/302.out | 5 + tests/qemu-iotests/308 | 4 +- tests/qemu-iotests/312 | 1 + tests/qemu-iotests/common.filter | 22 +- tests/qemu-iotests/common.rc | 22 +- tests/qemu-iotests/tests/qemu-img-close-errors | 96 +++++++ tests/qemu-iotests/tests/qemu-img-close-errors.out | 23 ++ 69 files changed, 1209 insertions(+), 552 deletions(-) create mode 100755 tests/qemu-iotests/tests/qemu-img-close-errors create mode 100644 tests/qemu-iotests/tests/qemu-img-close-errors.out