* [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management
@ 2018-01-27 2:05 John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property John Snow
` (15 more replies)
0 siblings, 16 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
For jobs that complete when a monitor isn't looking, there's no way to
tell what the job's final return code was. We need to allow jobs to
remain in the list until queried for reliable management.
Furthermore, it's not viable to have graph changes when the monitor
isn't looking either. We need at least another event for that.
This series is a rough sketch for the WAITING, PENDING and CONCLUDED
events that accompany an expanded job management scheme that a management
client can opt-in to.
V3:
- Added WAITING and PENDING events
- Added block_job_finalize verb
- Added .pending() callback for jobs
- Tweaked how .commit/.abort work
V2:
- Added tests!
- Changed property name (Jeff, Paolo)
RFC / Known problems:
- I need a lot more tests.
- Jobs need to actually implement their .pending callback for this to be of any
actual use.
- Mirror needs to be refactored to use the commit/abort/pending/clean callbacks
to fulfill the promise made by "no graph changes without user authorization"
that PENDING is supposed to offer
- Jobs beyond backup will be able to opt-in to the new management scheme in the
next version.
- V4 will include a forced synchronicity for jobs in a transaction; i.e. all
jobs will be forced to use either the old or new styles, but not a mix.
Please take a look and shout loudly if I'm wandering in the wrong direction.
________________________________________________________________________________
For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch block-job-reap
https://github.com/jnsnow/qemu/tree/block-job-reap
This version is tagged block-job-reap-v3:
https://github.com/jnsnow/qemu/releases/tag/block-job-reap-v3
John Snow (14):
blockjobs: add manual property
blockjobs: Add status enum
blockjobs: add state transition table
blockjobs: RFC add block_job_verb permission table
blockjobs: add block_job_dismiss
blockjobs: add CONCLUDED state
blockjobs: ensure abort is called for cancelled jobs
blockjobs: add commit, abort, clean helpers
blockjobs: add prepare callback
blockjobs: Add waiting event
blockjobs: add PENDING status and event
blockjobs: add block-job-finalize
blockjobs: Expose manual property
iotests: test manual job dismissal
block/backup.c | 22 ++--
block/commit.c | 2 +-
block/mirror.c | 2 +-
block/replication.c | 5 +-
block/stream.c | 2 +-
block/trace-events | 2 +
blockdev.c | 42 ++++++-
blockjob.c | 279 ++++++++++++++++++++++++++++++++++++++++---
include/block/block_int.h | 9 +-
include/block/blockjob.h | 38 ++++++
include/block/blockjob_int.h | 12 +-
qapi/block-core.json | 135 +++++++++++++++++++--
tests/qemu-iotests/056 | 241 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/056.out | 4 +-
tests/test-bdrv-drain.c | 4 +-
tests/test-blockjob-txn.c | 2 +-
tests/test-blockjob.c | 4 +-
17 files changed, 750 insertions(+), 55 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-29 16:59 ` Kevin Wolf
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 02/14] blockjobs: Add status enum John Snow
` (14 subsequent siblings)
15 siblings, 1 reply; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
This property will be used to opt-in to the new BlockJobs workflow
that allows a tighter, more explicit control over transitions from
one runstate to another.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 20 ++++++++++----------
block/commit.c | 2 +-
block/mirror.c | 2 +-
block/replication.c | 5 +++--
block/stream.c | 2 +-
blockdev.c | 4 ++--
blockjob.c | 3 ++-
include/block/block_int.h | 6 +++---
include/block/blockjob.h | 5 +++++
include/block/blockjob_int.h | 2 +-
tests/test-bdrv-drain.c | 4 ++--
tests/test-blockjob-txn.c | 2 +-
tests/test-blockjob.c | 4 ++--
13 files changed, 34 insertions(+), 27 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 4a16a37229..d729263708 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -545,15 +545,15 @@ static const BlockJobDriver backup_job_driver = {
.drain = backup_drain,
};
-BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
- BlockDriverState *target, int64_t speed,
- MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
- bool compress,
- BlockdevOnError on_source_error,
- BlockdevOnError on_target_error,
- int creation_flags,
- BlockCompletionFunc *cb, void *opaque,
- BlockJobTxn *txn, Error **errp)
+BlockJob *backup_job_create(const char *job_id, bool manual,
+ BlockDriverState *bs, BlockDriverState *target,
+ int64_t speed, MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap, bool compress,
+ BlockdevOnError on_source_error,
+ BlockdevOnError on_target_error,
+ int creation_flags,
+ BlockCompletionFunc *cb, void *opaque,
+ BlockJobTxn *txn, Error **errp)
{
int64_t len;
BlockDriverInfo bdi;
@@ -621,7 +621,7 @@ 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, manual, bs,
BLK_PERM_CONSISTENT_READ,
BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD,
diff --git a/block/commit.c b/block/commit.c
index bb6c904704..920fdabcc2 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -289,7 +289,7 @@ 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, false, 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 c9badc1203..8e8d3589f2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1166,7 +1166,7 @@ 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, false, 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/replication.c b/block/replication.c
index b1ea3caa4b..3ab0a08cd7 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -563,8 +563,9 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
bdrv_op_block_all(top_bs, s->blocker);
bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
- job = backup_job_create(NULL, s->secondary_disk->bs, s->hidden_disk->bs,
- 0, MIRROR_SYNC_MODE_NONE, NULL, false,
+ job = backup_job_create(NULL, false, s->secondary_disk->bs,
+ s->hidden_disk->bs, 0, MIRROR_SYNC_MODE_NONE,
+ NULL, false,
BLOCKDEV_ON_ERROR_REPORT,
BLOCKDEV_ON_ERROR_REPORT, BLOCK_JOB_INTERNAL,
backup_job_completed, bs, NULL, &local_err);
diff --git a/block/stream.c b/block/stream.c
index 499cdacdb0..f31785965c 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -244,7 +244,7 @@ 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, false, bs,
BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
BLK_PERM_GRAPH_MOD,
BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
diff --git a/blockdev.c b/blockdev.c
index 8e977eef11..2c0773bba7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3373,7 +3373,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
}
}
- job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
+ job = backup_job_create(backup->job_id, false, 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);
@@ -3452,7 +3452,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
goto out;
}
}
- job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
+ job = backup_job_create(backup->job_id, false, 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);
diff --git a/blockjob.c b/blockjob.c
index f5cea84e73..9850d70cb0 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -648,7 +648,7 @@ 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,
+ bool manual, BlockDriverState *bs, uint64_t perm,
uint64_t shared_perm, int64_t speed, int flags,
BlockCompletionFunc *cb, void *opaque, Error **errp)
{
@@ -703,6 +703,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
job->paused = true;
job->pause_count = 1;
job->refcnt = 1;
+ job->manual = manual;
aio_timer_init(qemu_get_aio_context(), &job->sleep_timer,
QEMU_CLOCK_REALTIME, SCALE_NS,
block_job_sleep_timer_cb, job);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 29cafa4236..08f5046c63 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -981,9 +981,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
* Create a backup operation on @bs. Clusters in @bs are written to @target
* until the job is cancelled or manually completed.
*/
-BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
- BlockDriverState *target, int64_t speed,
- MirrorSyncMode sync_mode,
+BlockJob *backup_job_create(const char *job_id, bool manual,
+ BlockDriverState *bs, BlockDriverState *target,
+ int64_t speed, MirrorSyncMode sync_mode,
BdrvDirtyBitmap *sync_bitmap,
bool compress,
BlockdevOnError on_source_error,
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 00403d9482..b94d0c9fa6 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -141,6 +141,11 @@ typedef struct BlockJob {
*/
QEMUTimer sleep_timer;
+ /* Set to true when management API has requested 2.12+ job lifetime
+ * management semantics.
+ */
+ bool manual;
+
/** Non-NULL if this job is part of a transaction */
BlockJobTxn *txn;
QLIST_ENTRY(BlockJob) txn_list;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index c9b23b0cc9..a24c3f90e5 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -132,7 +132,7 @@ 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,
+ bool manual, BlockDriverState *bs, uint64_t perm,
uint64_t shared_perm, int64_t speed, int flags,
BlockCompletionFunc *cb, void *opaque, Error **errp);
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index d760e2b243..184c5663b8 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -541,8 +541,8 @@ 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, false, 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 3591c9617f..2efaa554c0 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -101,7 +101,7 @@ 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, false, bs,
0, BLK_PERM_ALL, 0, BLOCK_JOB_DEFAULT,
test_block_job_cb, data, &error_abort);
s->iterations = iterations;
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 23bdf1a932..f08da8dca9 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -30,8 +30,8 @@ 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),
- 0, BLK_PERM_ALL, 0, BLOCK_JOB_DEFAULT, block_job_cb,
+ job = block_job_create(id, &test_block_job_driver, false, blk_bs(blk), 0,
+ BLK_PERM_ALL, 0, BLOCK_JOB_DEFAULT, block_job_cb,
NULL, &errp);
if (should_succeed) {
g_assert_null(errp);
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 02/14] blockjobs: Add status enum
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-29 17:04 ` Kevin Wolf
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table John Snow
` (13 subsequent siblings)
15 siblings, 1 reply; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
We're about to add several new states, and booleans are becoming
unwieldly and difficult to reason about. 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.
CREATED: replaces (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.
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.
This status does not feature for non-transactional jobs.
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. If this job is in a transaction, it has
already left the WAITING status.
CONCLUDED: Job has ceased all operations and has a return code available
for query and may be dismissed via block_job_dismiss.
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockjob.c | 10 ++++++++++
include/block/blockjob.h | 4 ++++
qapi/block-core.json | 17 ++++++++++++++++-
3 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/blockjob.c b/blockjob.c
index 9850d70cb0..6eb783a354 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -321,6 +321,7 @@ 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);
}
@@ -601,6 +602,10 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
info->speed = job->speed;
info->io_status = job->iostatus;
info->ready = job->ready;
+ if (job->manual) {
+ info->has_status = true;
+ info->status = job->status;
+ }
return info;
}
@@ -704,6 +709,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
job->pause_count = 1;
job->refcnt = 1;
job->manual = manual;
+ 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);
@@ -808,9 +814,12 @@ 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 = BLOCK_JOB_STATUS_PAUSED;
job->paused = true;
block_job_do_yield(job, -1);
job->paused = false;
+ job->status = status;
}
if (job->driver->resume) {
@@ -916,6 +925,7 @@ 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/include/block/blockjob.h b/include/block/blockjob.h
index b94d0c9fa6..d8e7df7e6e 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -146,6 +146,10 @@ typedef struct BlockJob {
*/
bool manual;
+ /* Current state, using 2.12+ state names
+ */
+ BlockJobStatus status;
+
/** Non-NULL if this job is part of a transaction */
BlockJobTxn *txn;
QLIST_ENTRY(BlockJob) txn_list;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8225308904..eac89754c1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -955,6 +955,18 @@
{ 'enum': 'BlockJobType',
'data': ['commit', 'stream', 'mirror', 'backup'] }
+##
+# @BlockJobStatus:
+#
+# Block Job State
+#
+#
+#
+# Since: 2.12
+##
+{ 'enum': 'BlockJobStatus',
+ 'data': ['undefined', 'created', 'running', 'paused', 'ready'] }
+
##
# @BlockJobInfo:
#
@@ -981,12 +993,15 @@
#
# @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:
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 02/14] blockjobs: Add status enum John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-29 17:17 ` Kevin Wolf
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 04/14] blockjobs: RFC add block_job_verb permission table John Snow
` (12 subsequent siblings)
15 siblings, 1 reply; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
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.
undefined: May only transition to 'created'.
created: May only transition to 'running'.
It cannot transition to pause directly, but if a created job
is requested to pause prior to starting, it will transition
synchronously to 'running' and then to 'paused'.
running: May transition either to 'paused' or 'ready'.
paused: May transition to either 'running' or 'ready', but only in
terms of returning to that prior state.
p->r->y is not possible, but this is not encapsulated by the
STM table.
ready: May transition to paused.
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockjob.c | 39 ++++++++++++++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 5 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 6eb783a354..d084a1e318 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -42,6 +42,35 @@
* 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 */
+ /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0},
+ /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0},
+ /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1},
+ /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1},
+ /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0},
+};
+
+static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
+{
+ BlockJobStatus s0 = job->status;
+ if (s0 == s1) {
+ return;
+ }
+ assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX);
+ assert(BlockJobSTT[s0][s1]);
+ switch (s1) {
+ case BLOCK_JOB_STATUS_WAITING:
+ case BLOCK_JOB_STATUS_PENDING:
+ case BLOCK_JOB_STATUS_CONCLUDED:
+ assert(job->manual);
+ default:
+ break;
+ }
+ job->status = s1;
+}
+
static void block_job_lock(void)
{
qemu_mutex_lock(&block_job_mutex);
@@ -321,7 +350,7 @@ 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);
}
@@ -709,7 +738,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
job->pause_count = 1;
job->refcnt = 1;
job->manual = manual;
- 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);
@@ -815,11 +844,11 @@ 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 = BLOCK_JOB_STATUS_PAUSED;
+ block_job_state_transition(job, 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) {
@@ -925,7 +954,7 @@ 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)) {
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 04/14] blockjobs: RFC add block_job_verb permission table
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (2 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss John Snow
` (11 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
Which commands are appropriate for jobs in which state is also somewhat
burdensome to keep track of. Introduce a verbs table that, as of this RFC
patch, does nothing but serve as a declaration of intent.
(At the very least, it forced me to consider all of the possibilities.)
If this idea seems appealing, I can expand the verbs concept into a list
of interfaces to consult the table and refuse inappropriate commands,
mostly serving as new errors for the QMP interface.
cancel: can apply to any created, running, paused or ready job.
pause: can apply to any created, running, or ready job. Addition pauses
can and do stack, so a paused job can also be paused.
Note that a pause from the QMP context is treated separately and
does not stack.
resume: Only a paused job can be resumed. Only a job that has been paused
via QMP can be resumed via QMP.
set-speed: Any created, running, paused or ready job can tolerate a
set-speed request.
complete: Only a ready job may accept a complete request.
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockjob.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/blockjob.c b/blockjob.c
index d084a1e318..ea216aca5e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -52,6 +52,24 @@ bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = {
/* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0},
};
+enum BlockJobVerb {
+ BLOCK_JOB_VERB_CANCEL,
+ BLOCK_JOB_VERB_PAUSE,
+ BLOCK_JOB_VERB_RESUME,
+ BLOCK_JOB_VERB_SET_SPEED,
+ BLOCK_JOB_VERB_COMPLETE,
+ BLOCK_JOB_VERB__MAX
+};
+
+bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
+ /* U, C, R, P, Y */
+ [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1},
+ [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1},
+ [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0},
+ [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1},
+ [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1},
+};
+
static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
{
BlockJobStatus s0 = job->status;
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (3 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 04/14] blockjobs: RFC add block_job_verb permission table John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-29 17:38 ` Kevin Wolf
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 06/14] blockjobs: add CONCLUDED state John Snow
` (10 subsequent siblings)
15 siblings, 1 reply; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
For jobs that have reached their terminal 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 that jobs do not yet linger in any such state, they are freed
immediately upon reaching this previously-unnamed state. such a state is
added immediately in the next commit.
Valid objects:
Concluded: (added in a future commit); dismisses the concluded job.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/trace-events | 1 +
blockdev.c | 14 ++++++++++++++
blockjob.c | 30 ++++++++++++++++++++++++++++++
include/block/blockjob.h | 9 +++++++++
qapi/block-core.json | 19 +++++++++++++++++++
5 files changed, 73 insertions(+)
diff --git a/block/trace-events b/block/trace-events
index 11c8d5f590..8f61566770 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -46,6 +46,7 @@ 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
diff --git a/blockdev.c b/blockdev.c
index 2c0773bba7..5e8edff322 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3849,6 +3849,20 @@ 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 ea216aca5e..5531f5c2ab 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -58,6 +58,7 @@ enum BlockJobVerb {
BLOCK_JOB_VERB_RESUME,
BLOCK_JOB_VERB_SET_SPEED,
BLOCK_JOB_VERB_COMPLETE,
+ BLOCK_JOB_VERB_DISMISS,
BLOCK_JOB_VERB__MAX
};
@@ -68,6 +69,7 @@ bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
[BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0},
[BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1},
[BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1},
+ [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0},
};
static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
@@ -426,6 +428,13 @@ static void block_job_cancel_async(BlockJob *job)
job->cancelled = true;
}
+static void block_job_do_dismiss(BlockJob *job)
+{
+ assert(job && job->manual == true);
+ block_job_state_transition(job, BLOCK_JOB_STATUS_UNDEFINED);
+ block_job_unref(job);
+}
+
static int block_job_finish_sync(BlockJob *job,
void (*finish)(BlockJob *, Error **errp),
Error **errp)
@@ -455,6 +464,9 @@ static int block_job_finish_sync(BlockJob *job,
aio_poll(qemu_get_aio_context(), true);
}
ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
+ if (job->manual) {
+ block_job_do_dismiss(job);
+ }
block_job_unref(job);
return ret;
}
@@ -570,6 +582,24 @@ 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 (!job->manual) {
+ error_setg(errp, "The active block job '%s' was not started with "
+ "\'manual\': true, and so cannot be dismissed as it will "
+ "clean up after itself automatically", job->id);
+ return;
+ }
+
+ error_setg(errp, "unimplemented");
+
+ block_job_do_dismiss(job);
+ *jobptr = NULL;
+}
+
void block_job_user_pause(BlockJob *job)
{
job->user_paused = true;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index d8e7df7e6e..7c71dc0ca7 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -241,6 +241,15 @@ 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/qapi/block-core.json b/qapi/block-core.json
index eac89754c1..32aefa5a27 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2198,6 +2198,25 @@
##
{ '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. "cancel" or "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:
#
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 06/14] blockjobs: add CONCLUDED state
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (4 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 07/14] blockjobs: ensure abort is called for cancelled jobs John Snow
` (9 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
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.)
Valid transitions:
Running -> Concluded (cancellation, normal completion)
Paused -> Concluded (cancellation)
Ready -> Concluded (cancellation, manual completion)
Concluded -> Undefined (immediately prior to destruction)
Valid verbs:
Dismiss: culls the job and job info from the query list.
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockjob.c | 56 +++++++++++++++++++++++++++++++++++++---------------
qapi/block-core.json | 8 +++++---
2 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 5531f5c2ab..0083fd7b0c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -44,12 +44,13 @@ static QemuMutex block_job_mutex;
/* BlockJob State Transition Table */
bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = {
- /* U, C, R, P, Y */
- /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0},
- /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0},
- /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1},
- /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1},
- /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0},
+ /* U, C, R, P, Y, E */
+ /* 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, 1},
+ /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1, 1},
+ /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0, 1},
+ /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0},
};
enum BlockJobVerb {
@@ -63,13 +64,13 @@ enum BlockJobVerb {
};
bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
- /* U, C, R, P, Y */
- [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1},
- [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1},
- [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0},
- [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1},
- [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1},
- [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0},
+ /* U, C, R, P, Y, E */
+ [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 0},
+ [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 0},
+ [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0, 0},
+ [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 0},
+ [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0},
+ [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 1},
};
static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
@@ -108,6 +109,7 @@ 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 void block_job_event_concluded(BlockJob *job);
static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job));
/* Transactional group of block jobs */
@@ -412,6 +414,8 @@ static void block_job_completed_single(BlockJob *job)
QLIST_REMOVE(job, txn_list);
block_job_txn_unref(job->txn);
}
+
+ block_job_event_concluded(job);
block_job_unref(job);
}
@@ -592,10 +596,14 @@ void block_job_dismiss(BlockJob **jobptr, Error **errp)
"\'manual\': true, and so cannot be dismissed as it will "
"clean up after itself automatically", job->id);
return;
+ } else if (job->status != BLOCK_JOB_STATUS_CONCLUDED) {
+ error_setg(errp, "The active block job '%s', status: '%s', has not yet "
+ "concluded, and cannot be dismissed yet",
+ job->id,
+ qapi_enum_lookup(&BlockJobStatus_lookup, job->status));
+ return;
}
- error_setg(errp, "unimplemented");
-
block_job_do_dismiss(job);
*jobptr = NULL;
}
@@ -622,7 +630,9 @@ void block_job_user_resume(BlockJob *job)
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 {
@@ -724,6 +734,14 @@ static void block_job_event_completed(BlockJob *job, const char *msg)
&error_abort);
}
+static void block_job_event_concluded(BlockJob *job)
+{
+ if (block_job_is_internal(job) || !job->manual) {
+ return;
+ }
+ block_job_state_transition(job, BLOCK_JOB_STATUS_CONCLUDED);
+}
+
/*
* API for block job drivers and the block layer. These functions are
* declared in blockjob_int.h.
@@ -814,6 +832,12 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
return NULL;
}
}
+
+ /* Hang on to an extra reference on behalf of the QMP monitor */
+ if (job->manual) {
+ block_job_ref(job);
+ }
+
return job;
}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 32aefa5a27..f27c7054d2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -965,7 +965,8 @@
# Since: 2.12
##
{ 'enum': 'BlockJobStatus',
- 'data': ['undefined', 'created', 'running', 'paused', 'ready'] }
+ 'data': ['undefined', 'created', 'running', 'paused', 'ready',
+ 'concluded' ] }
##
# @BlockJobInfo:
@@ -2206,8 +2207,9 @@
# QEMU 2.12+ job lifetime management semantics.
#
# This command will refuse to operate on any job that has not yet reached
-# its terminal state. "cancel" or "complete" will still need to be used as
-# appropriate.
+# 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.
#
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 07/14] blockjobs: ensure abort is called for cancelled jobs
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (5 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 06/14] blockjobs: add CONCLUDED state John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 08/14] blockjobs: add commit, abort, clean helpers John Snow
` (8 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
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.
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>
---
block/backup.c | 2 +-
blockjob.c | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/block/backup.c b/block/backup.c
index d729263708..a17248feab 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -206,7 +206,7 @@ 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 0083fd7b0c..3d678d6ce2 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -380,6 +380,11 @@ static void block_job_completed_single(BlockJob *job)
{
assert(job->completed);
+ /* Ensure abort is called and QMP client is notified of cancellation */
+ if (job->ret == 0 && block_job_is_cancelled(job)) {
+ job->ret = -ECANCELED;
+ }
+
if (!job->ret) {
if (job->driver->commit) {
job->driver->commit(job);
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 08/14] blockjobs: add commit, abort, clean helpers
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (6 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 07/14] blockjobs: ensure abort is called for cancelled jobs John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 09/14] blockjobs: add prepare callback John Snow
` (7 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
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>
---
blockjob.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 3d678d6ce2..1b169a0814 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -376,6 +376,29 @@ void block_job_start(BlockJob *job)
bdrv_coroutine_enter(blk_bs(job->blk), job->co);
}
+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);
@@ -386,17 +409,11 @@ static void block_job_completed_single(BlockJob *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.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 09/14] blockjobs: add prepare callback
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (7 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 08/14] blockjobs: add commit, abort, clean helpers John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 10/14] blockjobs: Add waiting event John Snow
` (6 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
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.
Thus, 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 as such:
- All jobs in a transaction converge to the WAITING state
(added in a forthcoming commit)
- All jobs prepare to call either commit/abort
- If any job fails, is canceled, or fails preparation, all jobs
call their .abort callback.
- All jobs enter the PENDING state, awaiting manual intervention
(also added in a forthcoming commit)
- block-job-finalize is issued by the user/management layer
- All jobs call their commit callbacks.
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockjob.c | 35 ++++++++++++++++++++++++++++++++++-
include/block/blockjob.h | 3 +++
include/block/blockjob_int.h | 10 ++++++++++
3 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/blockjob.c b/blockjob.c
index 1b169a0814..e52b4c4ce0 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -376,9 +376,21 @@ void block_job_start(BlockJob *job)
bdrv_coroutine_enter(blk_bs(job->blk), job->co);
}
+static int block_job_prepare(BlockJob *job)
+{
+ if (job->prepared) {
+ return job->ret;
+ }
+ job->prepared = true;
+ 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);
+ assert(!job->ret && job->prepared);
if (job->driver->commit) {
job->driver->commit(job);
}
@@ -408,6 +420,9 @@ static void block_job_completed_single(BlockJob *job)
job->ret = -ECANCELED;
}
+ /* NB: updates job->ret, only if not called on this job yet */
+ block_job_prepare(job);
+
if (!job->ret) {
block_job_commit(job);
} else {
@@ -545,6 +560,8 @@ static void block_job_completed_txn_success(BlockJob *job)
AioContext *ctx;
BlockJobTxn *txn = job->txn;
BlockJob *other_job, *next;
+ int rc = 0;
+
/*
* Successful completion, see if there are other running jobs in this
* txn.
@@ -554,6 +571,22 @@ static void block_job_completed_txn_success(BlockJob *job)
return;
}
}
+
+ /* Jobs may require some prep-work to complete without failure */
+ 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);
+ rc = block_job_prepare(job);
+ aio_context_release(ctx);
+
+ /* This job failed. Cancel this transaction */
+ if (rc) {
+ block_job_completed_txn_abort(other_job);
+ 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);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 7c71dc0ca7..5f73fc8831 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -150,6 +150,9 @@ typedef struct BlockJob {
*/
BlockJobStatus status;
+ /* Job has made preparations to call either commit or abort */
+ bool prepared;
+
/** Non-NULL if this job is part of a transaction */
BlockJobTxn *txn;
QLIST_ENTRY(BlockJob) txn_list;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index a24c3f90e5..689d1bc659 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -53,6 +53,16 @@ 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
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 10/14] blockjobs: Add waiting event
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (8 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 09/14] blockjobs: add prepare callback John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 11/14] blockjobs: add PENDING status and event John Snow
` (5 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
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.
Valid transitions:
Running -> Waiting (transactional, non-mirror jobs upon completion)
Ready -> Waiting (hypothetically: transactional mirror jobs upon
block-job-complete)
Waiting -> Concluded (transactional jobs of any kind upon convergence)
Valid verbs:
Cancel: A waiting job may still be canceled.
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockjob.c | 45 +++++++++++++++++++++++++++++++--------------
qapi/block-core.json | 25 ++++++++++++++++++++++++-
2 files changed, 55 insertions(+), 15 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index e52b4c4ce0..6a3a630517 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -44,13 +44,14 @@ static QemuMutex block_job_mutex;
/* BlockJob State Transition Table */
bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = {
- /* U, C, R, P, Y, E */
- /* 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, 1},
- /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1, 1},
- /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0, 1},
- /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0},
+ /* U, C, R, P, Y, W, E */
+ /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0},
+ /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0},
+ /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 1, 1},
+ /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1, 0, 1},
+ /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0, 1, 1},
+ /* W: */ [BLOCK_JOB_STATUS_WAITING] = {0, 0, 0, 0, 0, 0, 1},
+ /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0, 0},
};
enum BlockJobVerb {
@@ -64,13 +65,13 @@ enum BlockJobVerb {
};
bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
- /* U, C, R, P, Y, E */
- [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 0},
- [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 0},
- [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0, 0},
- [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 0},
- [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0},
- [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 1},
+ /* U, C, R, P, Y, W, E */
+ [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0},
+ [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 0, 0},
+ [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0, 0, 0},
+ [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 0, 0},
+ [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0},
+ [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 1},
};
static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
@@ -789,6 +790,21 @@ static void block_job_event_completed(BlockJob *job, const char *msg)
&error_abort);
}
+static void block_job_event_waiting(BlockJob *job)
+{
+ if (!job->manual) {
+ return;
+ }
+ block_job_state_transition(job, BLOCK_JOB_STATUS_WAITING);
+ if (block_job_is_internal(job)) {
+ return;
+ }
+
+ qapi_event_send_block_job_waiting(job->driver->job_type,
+ job->id,
+ &error_abort);
+}
+
static void block_job_event_concluded(BlockJob *job)
{
if (block_job_is_internal(job) || !job->manual) {
@@ -925,6 +941,7 @@ void block_job_completed(BlockJob *job, int ret)
} else if (ret < 0 || block_job_is_cancelled(job)) {
block_job_completed_txn_abort(job);
} else {
+ block_job_event_waiting(job);
block_job_completed_txn_success(job);
}
}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f27c7054d2..f26fd1d8fd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -966,7 +966,7 @@
##
{ 'enum': 'BlockJobStatus',
'data': ['undefined', 'created', 'running', 'paused', 'ready',
- 'concluded' ] }
+ 'waiting', 'concluded' ] }
##
# @BlockJobInfo:
@@ -3867,6 +3867,29 @@
'offset': 'int',
'speed' : 'int' } }
+##
+# @BLOCK_JOB_WAITING:
+#
+# Emitted when a block job that is part of a transction has stopped work and is
+# waiting for other jobs in the transaction to reach the same state.
+#
+# @type: job type
+#
+# @id: The job identifier.
+#
+# Since: 2.12
+#
+# Example:
+#
+# <- { "event": "BLOCK_JOB_WAITING",
+# "data": { "id": "drive0", "type": "mirror" },
+# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+#
+##
+{ 'event': 'BLOCK_JOB_WAITING',
+ 'data': { 'type' : 'BlockJobType',
+ 'id' : 'str' } }
+
##
# @PreallocMode:
#
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 11/14] blockjobs: add PENDING status and event
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (9 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 10/14] blockjobs: Add waiting event John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 12/14] blockjobs: add block-job-finalize John Snow
` (4 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
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.
Valid transitions:
Running -> Pending (non-transactional, non-mirror jobs)
Ready -> Pending (non-transactional mirror jobs)
Waiting -> Pending (transactional jobs)
Invalid transitions:
Waiting -> Concluded (no longer possible with this commit)
Valid verbs:
Cancel: A pending job may still be canceled.
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockjob.c | 45 ++++++++++++++++++++++++++++++---------------
qapi/block-core.json | 26 +++++++++++++++++++++++++-
2 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 6a3a630517..d31b65273c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -44,14 +44,15 @@ static QemuMutex block_job_mutex;
/* BlockJob State Transition Table */
bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = {
- /* U, C, R, P, Y, W, E */
- /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0},
- /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0},
- /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 1, 1},
- /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1, 0, 1},
- /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0, 1, 1},
- /* W: */ [BLOCK_JOB_STATUS_WAITING] = {0, 0, 0, 0, 0, 0, 1},
- /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0, 0},
+ /* U, C, R, P, Y, W, D, 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, 0, 0},
+ /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 1, 1, 1},
+ /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1, 0, 0, 1},
+ /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0, 1, 1, 1},
+ /* W: */ [BLOCK_JOB_STATUS_WAITING] = {0, 0, 0, 0, 0, 0, 1, 1},
+ /* D: */ [BLOCK_JOB_STATUS_PENDING] = {0, 0, 0, 0, 0, 0, 0, 1},
+ /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0, 0, 0},
};
enum BlockJobVerb {
@@ -65,13 +66,13 @@ enum BlockJobVerb {
};
bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
- /* U, C, R, P, Y, W, E */
- [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0},
- [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 0, 0},
- [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0, 0, 0},
- [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 0, 0},
- [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0},
- [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 1},
+ /* U, C, R, P, Y, W, D, E */
+ [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 1, 0},
+ [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 0, 0, 0},
+ [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0, 0, 0, 0},
+ [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 0, 0, 0},
+ [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0},
+ [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 1},
};
static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
@@ -111,6 +112,7 @@ 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 void block_job_event_concluded(BlockJob *job);
+static void block_job_event_pending(BlockJob *job);
static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job));
/* Transactional group of block jobs */
@@ -805,6 +807,19 @@ static void block_job_event_waiting(BlockJob *job)
&error_abort);
}
+__attribute__((__unused__)) /* FIXME */
+static void block_job_event_pending(BlockJob *job)
+{
+ if (block_job_is_internal(job) || !job->manual) {
+ return;
+ }
+ block_job_state_transition(job, BLOCK_JOB_STATUS_PENDING);
+
+ qapi_event_send_block_job_pending(job->driver->job_type,
+ job->id,
+ &error_abort);
+}
+
static void block_job_event_concluded(BlockJob *job)
{
if (block_job_is_internal(job) || !job->manual) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f26fd1d8fd..1f2eb39810 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -966,7 +966,7 @@
##
{ 'enum': 'BlockJobStatus',
'data': ['undefined', 'created', 'running', 'paused', 'ready',
- 'waiting', 'concluded' ] }
+ 'waiting', 'pending', 'concluded' ] }
##
# @BlockJobInfo:
@@ -3890,6 +3890,30 @@
'data': { 'type' : 'BlockJobType',
'id' : 'str' } }
+##
+# @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:
#
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 12/14] blockjobs: add block-job-finalize
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (10 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 11/14] blockjobs: add PENDING status and event John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 13/14] blockjobs: Expose manual property John Snow
` (3 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/trace-events | 1 +
blockdev.c | 14 ++++++++++
blockjob.c | 72 +++++++++++++++++++++++++++++++++++++++---------
include/block/blockjob.h | 17 ++++++++++++
qapi/block-core.json | 18 ++++++++++++
5 files changed, 109 insertions(+), 13 deletions(-)
diff --git a/block/trace-events b/block/trace-events
index 8f61566770..d3be349489 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -46,6 +46,7 @@ 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"
diff --git a/blockdev.c b/blockdev.c
index 5e8edff322..d387ef6ec0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3849,6 +3849,20 @@ 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 d31b65273c..b8d6dd3bb4 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -61,6 +61,7 @@ enum BlockJobVerb {
BLOCK_JOB_VERB_RESUME,
BLOCK_JOB_VERB_SET_SPEED,
BLOCK_JOB_VERB_COMPLETE,
+ BLOCK_JOB_VERB_FINALIZE,
BLOCK_JOB_VERB_DISMISS,
BLOCK_JOB_VERB__MAX
};
@@ -72,6 +73,7 @@ bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
[BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0, 0, 0, 0},
[BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 0, 0, 0},
[BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0},
+ [BLOCK_JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 1, 0},
[BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 1},
};
@@ -459,6 +461,15 @@ static void block_job_completed_single(BlockJob *job)
block_job_unref(job);
}
+static void block_job_await_finalization(BlockJob *job)
+{
+ if (!job->manual) {
+ block_job_completed_single(job);
+ } else {
+ block_job_event_pending(job);
+ }
+}
+
static void block_job_cancel_async(BlockJob *job)
{
if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) {
@@ -558,6 +569,19 @@ static void block_job_completed_txn_abort(BlockJob *job)
block_job_txn_unref(txn);
}
+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 void block_job_completed_txn_success(BlockJob *job)
{
AioContext *ctx;
@@ -590,14 +614,9 @@ static void block_job_completed_txn_success(BlockJob *job)
}
}
- /* 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, either commit the transaction
+ * or prepare for finalization via user intervention. */
+ block_job_txn_apply(txn, block_job_await_finalization);
}
/* Assumes the block_job_mutex is held */
@@ -606,6 +625,15 @@ static bool block_job_timer_pending(BlockJob *job)
return timer_pending(&job->sleep_timer);
}
+static void block_job_txn_completed(BlockJob *job, int ret)
+{
+ if (ret < 0 || block_job_is_cancelled(job)) {
+ block_job_completed_txn_abort(job);
+ } else {
+ block_job_completed_txn_success(job);
+ }
+}
+
void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
{
Error *local_err = NULL;
@@ -644,6 +672,27 @@ void block_job_complete(BlockJob *job, Error **errp)
job->driver->complete(job, errp);
}
+void block_job_finalize(BlockJob *job, Error **errp)
+{
+ assert(job->id);
+ if (!job->manual) {
+ error_setg(errp, "The block job '%s' was not started with "
+ "\'manual\': true, and so cannot be finalized as it will"
+ "do so automatically upon finishing its task", job->id);
+ return;
+ } else if (job->status != BLOCK_JOB_STATUS_PENDING) {
+ error_setg(errp, "The active block job '%s' is not yet awaiting "
+ "finalization and cannot be finalized", job->id);
+ return;
+ }
+
+ if (!job->txn) {
+ block_job_completed_single(job);
+ } else {
+ block_job_txn_apply(job->txn, block_job_completed_single);
+ }
+}
+
void block_job_dismiss(BlockJob **jobptr, Error **errp)
{
BlockJob *job = *jobptr;
@@ -807,7 +856,6 @@ static void block_job_event_waiting(BlockJob *job)
&error_abort);
}
-__attribute__((__unused__)) /* FIXME */
static void block_job_event_pending(BlockJob *job)
{
if (block_job_is_internal(job) || !job->manual) {
@@ -952,12 +1000,10 @@ void block_job_completed(BlockJob *job, int ret)
job->completed = true;
job->ret = ret;
if (!job->txn) {
- block_job_completed_single(job);
- } else if (ret < 0 || block_job_is_cancelled(job)) {
- block_job_completed_txn_abort(job);
+ block_job_await_finalization(job);
} else {
block_job_event_waiting(job);
- block_job_completed_txn_success(job);
+ block_job_txn_completed(job, ret);
}
}
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 5f73fc8831..188853ca77 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -244,6 +244,23 @@ 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/qapi/block-core.json b/qapi/block-core.json
index 1f2eb39810..bd4458bf57 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2219,6 +2219,24 @@
##
{ '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:
#
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 13/14] blockjobs: Expose manual property
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (11 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 12/14] blockjobs: add block-job-finalize John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 14/14] iotests: test manual job dismissal John Snow
` (2 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
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>
---
blockdev.c | 14 ++++++++++----
include/block/block_int.h | 3 +++
qapi/block-core.json | 32 ++++++++++++++++++++++++++------
3 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index d387ef6ec0..e7c3fc0607 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3281,6 +3281,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
if (!backup->has_job_id) {
backup->job_id = NULL;
}
+ if (!backup->has_manual) {
+ backup->manual = false;
+ }
if (!backup->has_compress) {
backup->compress = false;
}
@@ -3373,8 +3376,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
}
}
- job = backup_job_create(backup->job_id, false, bs, target_bs, backup->speed,
- backup->sync, bmap, backup->compress,
+ job = backup_job_create(backup->job_id, backup->manual, 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);
bdrv_unref(target_bs);
@@ -3424,6 +3427,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
if (!backup->has_job_id) {
backup->job_id = NULL;
}
+ if (!backup->has_manual) {
+ backup->manual = false;
+ }
if (!backup->has_compress) {
backup->compress = false;
}
@@ -3452,8 +3458,8 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
goto out;
}
}
- job = backup_job_create(backup->job_id, false, bs, target_bs, backup->speed,
- backup->sync, NULL, backup->compress,
+ job = backup_job_create(backup->job_id, backup->manual, 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);
if (local_err != NULL) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 08f5046c63..d91f50ca60 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -965,6 +965,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
* backup_job_create:
* @job_id: The id of the newly-created job, or %NULL to use the
* device name of @bs.
+ * @manual: Whether or not this job requires manual intervention to transition
+ * from "PENDING" state to "CONCLUDED" state, and again, manual
+ * intervention to dismiss CONCLUDED jobs.
* @bs: Block device to operate on.
* @target: Block device to write to.
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bd4458bf57..d8d82404da 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1119,6 +1119,16 @@
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
+# @manual: True to use an expanded, more explicit job control flow.
+# Jobs may transition from a running state to a pending state,
+# where they must be instructed to complete manually via
+# block-job-finalize.
+# Jobs belonging to a transaction must either all or all not use this
+# setting. Once a transaction reaches a pending state, issuing the
+# finalize command to any one job in the transaction is sufficient
+# to finalize the entire transaction.
+# (Since 2.12)
+#
# @device: the device name or node-name of a root node which should be copied.
#
# @target: the target of the new image. If the file exists, or if it
@@ -1159,9 +1169,10 @@
# Since: 1.6
##
{ 'struct': 'DriveBackup',
- 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
- '*format': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
- '*speed': 'int', '*bitmap': 'str', '*compress': 'bool',
+ 'data': { '*job-id': 'str', '*manual': 'bool', 'device': 'str',
+ 'target': 'str', '*format': 'str', 'sync': 'MirrorSyncMode',
+ '*mode': 'NewImageMode', '*speed': 'int',
+ '*bitmap': 'str', '*compress': 'bool',
'*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError' } }
@@ -1171,6 +1182,16 @@
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
+# @manual: True to use an expanded, more explicit job control flow.
+# Jobs may transition from a running state to a pending state,
+# where they must be instructed to complete manually via
+# block-job-finalize.
+# Jobs belonging to a transaction must either all or all not use this
+# setting. Once a transaction reaches a pending state, issuing the
+# finalize command to any one job in the transaction is sufficient
+# to finalize the entire transaction.
+# (Since 2.12)
+#
# @device: the device name or node-name of a root node which should be copied.
#
# @target: the device name or node-name of the backup target node.
@@ -1200,9 +1221,8 @@
# Since: 2.3
##
{ 'struct': 'BlockdevBackup',
- 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
- 'sync': 'MirrorSyncMode',
- '*speed': 'int',
+ 'data': { '*job-id': 'str', '*manual': 'bool', 'device': 'str',
+ 'target': 'str', 'sync': 'MirrorSyncMode', '*speed': 'int',
'*compress': 'bool',
'*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError' } }
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC v3 14/14] iotests: test manual job dismissal
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (12 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 13/14] blockjobs: Expose manual property John Snow
@ 2018-01-27 2:05 ` John Snow
2018-01-27 2:25 ` [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management no-reply
2018-02-01 0:08 ` John Snow
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-27 2:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel, John Snow
RFC: The error returned by a job creation command when that device
already has a job attached has become misleading; "Someone should
do something about that!"
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/056 | 241 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/056.out | 4 +-
2 files changed, 243 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index 04f2c3c841..846e0419d0 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -29,6 +29,26 @@ 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
@@ -108,5 +128,226 @@ 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_job_pending_wait(self, device):
+ event = self.vm.event_wait(name="BLOCK_JOB_PENDING",
+ match={'data': {'id': device}})
+ self.assertNotEqual(event, None)
+ res = self.vm.qmp("block-job-finalize", id=device)
+ self.assert_qmp(res, 'return', {})
+
+ def qmp_backup_and_wait(self, cmd='drive-backup', serror=None,
+ aerror=None, **kwargs):
+ if not self.qmp_backup(cmd, serror, **kwargs):
+ return False
+ if 'manual' in kwargs and kwargs['manual']:
+ self.qmp_job_pending_wait(kwargs['device'])
+ 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, manual=False)
+ 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, manual=True)
+ 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, manual=True)
+ 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,
+ manual=True)
+ 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, manual=True)
+
+ def test_dismiss_premature(self):
+ 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',
+ manual=True)
+ 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',
+ "The active block job 'drive0', status: 'paused', has not yet concluded, and cannot be dismissed yet")
+ 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', {})
+ # Job should now be pending...
+ self.qmp_job_pending_wait('drive0')
+ # And now we need to wait for it to conclude;
+ res = self.qmp_backup_wait(device='drive0')
+ self.assertTrue(res)
+ # 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_erroneous(self):
+ '''Same as above test, but manual is set to False.'''
+ 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',
+ manual=False)
+ 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')
+ # NB: No 'status' field when manual = false.
+ self.assert_qmp(res, 'return[0]/paused', True)
+ res = self.vm.qmp('block-job-dismiss', id='drive0')
+ self.assert_qmp(res, 'error/desc',
+ "The active block job 'drive0' was not started with 'manual': true, and so cannot be dismissed as it will clean up after itself automatically")
+ res = self.vm.qmp('query-block-jobs')
+ self.assert_qmp(res, 'return[0]/paused', True)
+ # OK, unstick job and move forward.
+ res = self.vm.qmp('block-job-resume', device='drive0')
+ self.assert_qmp(res, 'return', {})
+ res = self.qmp_backup_wait(device='drive0')
+ self.assertTrue(res)
+ # Job should now be gone:
+ res = self.vm.qmp('query-block-jobs')
+ self.assert_qmp(res, 'return', [])
+
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
index 8d7e996700..dae404e278 100644
--- a/tests/qemu-iotests/056.out
+++ b/tests/qemu-iotests/056.out
@@ -1,5 +1,5 @@
-...
+.........
----------------------------------------------------------------------
-Ran 3 tests
+Ran 9 tests
OK
--
2.14.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (13 preceding siblings ...)
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 14/14] iotests: test manual job dismissal John Snow
@ 2018-01-27 2:25 ` no-reply
2018-02-01 0:08 ` John Snow
15 siblings, 0 replies; 28+ messages in thread
From: no-reply @ 2018-01-27 2:25 UTC (permalink / raw)
To: jsnow; +Cc: famz, qemu-block, kwolf, pkrempa, jtc, qemu-devel
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180127020515.27137-1-jsnow@redhat.com
Subject: [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
t [tag update] patchew/151670719721.7533.6287116389556641300.stgit@bahia -> patchew/151670719721.7533.6287116389556641300.stgit@bahia
* [new tag] patchew/20180127020515.27137-1-jsnow@redhat.com -> patchew/20180127020515.27137-1-jsnow@redhat.com
Switched to a new branch 'test'
10959f8618 iotests: test manual job dismissal
fa3b3419d9 blockjobs: Expose manual property
acf29467dd blockjobs: add block-job-finalize
b8250669f5 blockjobs: add PENDING status and event
67b2f8d3ed blockjobs: Add waiting event
22b6b95b6e blockjobs: add prepare callback
51530e8516 blockjobs: add commit, abort, clean helpers
45761a2219 blockjobs: ensure abort is called for cancelled jobs
d7317d4525 blockjobs: add CONCLUDED state
a4939037ed blockjobs: add block_job_dismiss
83b14366be blockjobs: RFC add block_job_verb permission table
446a6e487b blockjobs: add state transition table
bf5fb419c1 blockjobs: Add status enum
85487f375a blockjobs: add manual property
=== OUTPUT BEGIN ===
Checking PATCH 1/14: blockjobs: add manual property...
Checking PATCH 2/14: blockjobs: Add status enum...
Checking PATCH 3/14: blockjobs: add state transition table...
ERROR: space prohibited before open square bracket '['
#37: FILE: blockjob.c:48:
+ /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#38: FILE: blockjob.c:49:
+ /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0},
ERROR: space prohibited before open square bracket '['
#39: FILE: blockjob.c:50:
+ /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1},
ERROR: space prohibited before open square bracket '['
#40: FILE: blockjob.c:51:
+ /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1},
ERROR: space prohibited before open square bracket '['
#41: FILE: blockjob.c:52:
+ /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0},
total: 5 errors, 0 warnings, 72 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 4/14: blockjobs: RFC add block_job_verb permission table...
Checking PATCH 5/14: blockjobs: add block_job_dismiss...
Checking PATCH 6/14: blockjobs: add CONCLUDED state...
ERROR: space prohibited before open square bracket '['
#40: FILE: blockjob.c:48:
+ /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#41: FILE: blockjob.c:49:
+ /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#42: FILE: blockjob.c:50:
+ /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 1},
ERROR: space prohibited before open square bracket '['
#43: FILE: blockjob.c:51:
+ /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1, 1},
ERROR: space prohibited before open square bracket '['
#44: FILE: blockjob.c:52:
+ /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0, 1},
ERROR: space prohibited before open square bracket '['
#45: FILE: blockjob.c:53:
+ /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0},
total: 6 errors, 0 warnings, 126 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 7/14: blockjobs: ensure abort is called for cancelled jobs...
Checking PATCH 8/14: blockjobs: add commit, abort, clean helpers...
Checking PATCH 9/14: blockjobs: add prepare callback...
Checking PATCH 10/14: blockjobs: Add waiting event...
ERROR: space prohibited before open square bracket '['
#43: FILE: blockjob.c:48:
+ /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#44: FILE: blockjob.c:49:
+ /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#45: FILE: blockjob.c:50:
+ /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 1, 1},
ERROR: space prohibited before open square bracket '['
#46: FILE: blockjob.c:51:
+ /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1, 0, 1},
ERROR: space prohibited before open square bracket '['
#47: FILE: blockjob.c:52:
+ /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0, 1, 1},
ERROR: space prohibited before open square bracket '['
#48: FILE: blockjob.c:53:
+ /* W: */ [BLOCK_JOB_STATUS_WAITING] = {0, 0, 0, 0, 0, 0, 1},
ERROR: space prohibited before open square bracket '['
#49: FILE: blockjob.c:54:
+ /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0, 0},
total: 7 errors, 0 warnings, 106 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 11/14: blockjobs: add PENDING status and event...
ERROR: space prohibited before open square bracket '['
#44: FILE: blockjob.c:48:
+ /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#45: FILE: blockjob.c:49:
+ /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#46: FILE: blockjob.c:50:
+ /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 1, 1, 1},
ERROR: space prohibited before open square bracket '['
#47: FILE: blockjob.c:51:
+ /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1, 0, 0, 1},
ERROR: space prohibited before open square bracket '['
#48: FILE: blockjob.c:52:
+ /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0, 1, 1, 1},
ERROR: space prohibited before open square bracket '['
#49: FILE: blockjob.c:53:
+ /* W: */ [BLOCK_JOB_STATUS_WAITING] = {0, 0, 0, 0, 0, 0, 1, 1},
ERROR: space prohibited before open square bracket '['
#50: FILE: blockjob.c:54:
+ /* D: */ [BLOCK_JOB_STATUS_PENDING] = {0, 0, 0, 0, 0, 0, 0, 1},
ERROR: space prohibited before open square bracket '['
#51: FILE: blockjob.c:55:
+ /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0, 0, 0},
total: 8 errors, 0 warnings, 107 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 12/14: blockjobs: add block-job-finalize...
Checking PATCH 13/14: blockjobs: Expose manual property...
Checking PATCH 14/14: iotests: test manual job dismissal...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property John Snow
@ 2018-01-29 16:59 ` Kevin Wolf
2018-01-29 17:34 ` John Snow
0 siblings, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2018-01-29 16:59 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, pkrempa, jtc, qemu-devel
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
> This property will be used to opt-in to the new BlockJobs workflow
> that allows a tighter, more explicit control over transitions from
> one runstate to another.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 00403d9482..b94d0c9fa6 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -141,6 +141,11 @@ typedef struct BlockJob {
> */
> QEMUTimer sleep_timer;
>
> + /* Set to true when management API has requested 2.12+ job lifetime
> + * management semantics.
> + */
> + bool manual;
Wouldn't it make more sense to describe what "2.12+ job lifetime
management semantics" actually are? Maybe then it would be easy to find
a more specific name, too, like manual_completion.
In fact, I wonder if the opposite flag wouldn't be nicer, i.e. having a
bool auto_completion (or finalization or whatever that extra step was
called in the final draft), defaulting to true.
Also, the comment style in this header is already pretty messed up, but
I think the styles that were originally meant to be used there, are
/** this one for single lines */
/**
* and this one if things get a bit longer
* and you need multiple lines.
*/
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 02/14] blockjobs: Add status enum
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 02/14] blockjobs: Add status enum John Snow
@ 2018-01-29 17:04 ` Kevin Wolf
2018-01-29 17:38 ` John Snow
0 siblings, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2018-01-29 17:04 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, pkrempa, jtc, qemu-devel
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
> We're about to add several new states, and booleans are becoming
> unwieldly and difficult to reason about. 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.
> CREATED: replaces (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.
>
> 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.
> This status does not feature for non-transactional jobs.
> 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. If this job is in a transaction, it has
> already left the WAITING status.
> CONCLUDED: Job has ceased all operations and has a return code available
> for query and may be dismissed via block_job_dismiss.
> Signed-off-by: John Snow <jsnow@redhat.com>
Empty line before S-o-b?
> blockjob.c | 10 ++++++++++
> include/block/blockjob.h | 4 ++++
> qapi/block-core.json | 17 ++++++++++++++++-
> 3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 9850d70cb0..6eb783a354 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -321,6 +321,7 @@ 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);
> }
>
> @@ -601,6 +602,10 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
> info->speed = job->speed;
> info->io_status = job->iostatus;
> info->ready = job->ready;
> + if (job->manual) {
> + info->has_status = true;
> + info->status = job->status;
> + }
Why do we want to hide the status from clients that don't want to
complete the job manually? Isn't this conflating two orthogonal things?
> return info;
> }
>
> @@ -704,6 +709,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> job->pause_count = 1;
> job->refcnt = 1;
> job->manual = manual;
> + 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);
> @@ -808,9 +814,12 @@ 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 = BLOCK_JOB_STATUS_PAUSED;
> job->paused = true;
> block_job_do_yield(job, -1);
> job->paused = false;
> + job->status = status;
> }
>
> if (job->driver->resume) {
> @@ -916,6 +925,7 @@ 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/include/block/blockjob.h b/include/block/blockjob.h
> index b94d0c9fa6..d8e7df7e6e 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -146,6 +146,10 @@ typedef struct BlockJob {
> */
> bool manual;
>
> + /* Current state, using 2.12+ state names
> + */
> + BlockJobStatus status;
> +
> /** Non-NULL if this job is part of a transaction */
> BlockJobTxn *txn;
> QLIST_ENTRY(BlockJob) txn_list;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8225308904..eac89754c1 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -955,6 +955,18 @@
> { 'enum': 'BlockJobType',
> 'data': ['commit', 'stream', 'mirror', 'backup'] }
>
> +##
> +# @BlockJobStatus:
> +#
> +# Block Job State
> +#
> +#
> +#
> +# Since: 2.12
> +##
> +{ 'enum': 'BlockJobStatus',
> + 'data': ['undefined', 'created', 'running', 'paused', 'ready'] }
I assume these multiple empty lines are left there so you have space to
fill in the information from the commit message?
> ##
> # @BlockJobInfo:
> #
> @@ -981,12 +993,15 @@
> #
> # @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' } }
If we don't actually have a reason to hide the status above, it can
become a non-opional field here.
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table John Snow
@ 2018-01-29 17:17 ` Kevin Wolf
2018-01-29 19:07 ` John Snow
0 siblings, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2018-01-29 17:17 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, pkrempa, jtc, qemu-devel
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
> 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.
>
> undefined: May only transition to 'created'.
> created: May only transition to 'running'.
> It cannot transition to pause directly, but if a created job
> is requested to pause prior to starting, it will transition
> synchronously to 'running' and then to 'paused'.
> running: May transition either to 'paused' or 'ready'.
> paused: May transition to either 'running' or 'ready', but only in
> terms of returning to that prior state.
> p->r->y is not possible, but this is not encapsulated by the
> STM table.
Do you mean y->p->r->y here? If not, I don't understand.
> ready: May transition to paused.
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> blockjob.c | 39 ++++++++++++++++++++++++++++++++++-----
> 1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 6eb783a354..d084a1e318 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -42,6 +42,35 @@
> * 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 */
> + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0},
> + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0},
> + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1},
> + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1},
> + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0},
> +};
> +
> +static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
> +{
> + BlockJobStatus s0 = job->status;
> + if (s0 == s1) {
> + return;
> + }
> + assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX);
> + assert(BlockJobSTT[s0][s1]);
> + switch (s1) {
> + case BLOCK_JOB_STATUS_WAITING:
> + case BLOCK_JOB_STATUS_PENDING:
> + case BLOCK_JOB_STATUS_CONCLUDED:
> + assert(job->manual);
> + default:
> + break;
> + }
This doesn't compile because the constants don't exist yet.
Apart from that, I think the assertion is misguided, too. Which states a
job goes through shouldn't depend on whether the client wants to
complete the job manually or have it completed automatically. The
difference should only be which state transitions are automatic.
In other words, WAITING/PENDING/CONCLUDED may never be visible in
query-block-job for automatically completed jobs, but the jobs should
still (synchronously) go through all of these states.
> + job->status = s1;
> +}
> +
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property
2018-01-29 16:59 ` Kevin Wolf
@ 2018-01-29 17:34 ` John Snow
2018-01-29 17:46 ` Kevin Wolf
0 siblings, 1 reply; 28+ messages in thread
From: John Snow @ 2018-01-29 17:34 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pkrempa, jtc, qemu-devel, qemu-block
On 01/29/2018 11:59 AM, Kevin Wolf wrote:
> Am 27.01.2018 um 03:05 hat John Snow geschrieben:
>> This property will be used to opt-in to the new BlockJobs workflow
>> that allows a tighter, more explicit control over transitions from
>> one runstate to another.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
>> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
>> index 00403d9482..b94d0c9fa6 100644
>> --- a/include/block/blockjob.h
>> +++ b/include/block/blockjob.h
>> @@ -141,6 +141,11 @@ typedef struct BlockJob {
>> */
>> QEMUTimer sleep_timer;
>>
>> + /* Set to true when management API has requested 2.12+ job lifetime
>> + * management semantics.
>> + */
>> + bool manual;
>
> Wouldn't it make more sense to describe what "2.12+ job lifetime
> management semantics" actually are? Maybe then it would be easy to find
> a more specific name, too, like manual_completion.
>
Sure. At the time I wrote it, I wasn't sure what they were. Maybe I'll
find out after the review; but I'll make a note to document it here.
> In fact, I wonder if the opposite flag wouldn't be nicer, i.e. having a
> bool auto_completion (or finalization or whatever that extra step was
> called in the final draft), defaulting to true.
>
I could do that, if you feel it'd be more readable. In terms of style I
tend to prefer new additions default to false as that feels more
permanently reliable, but it's only a preference.
> Also, the comment style in this header is already pretty messed up, but
> I think the styles that were originally meant to be used there, are
>
> /** this one for single lines */
>
> /**
> * and this one if things get a bit longer
> * and you need multiple lines.
> */
>
> Kevin
>
I can do a fixup and make 'em consistent.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss John Snow
@ 2018-01-29 17:38 ` Kevin Wolf
2018-01-29 20:33 ` John Snow
0 siblings, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2018-01-29 17:38 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, pkrempa, jtc, qemu-devel
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
> For jobs that have reached their terminal 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 that jobs do not yet linger in any such state, they are freed
> immediately upon reaching this previously-unnamed state. such a state is
> added immediately in the next commit.
>
> Valid objects:
> Concluded: (added in a future commit); dismisses the concluded job.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/trace-events | 1 +
> blockdev.c | 14 ++++++++++++++
> blockjob.c | 30 ++++++++++++++++++++++++++++++
> include/block/blockjob.h | 9 +++++++++
> qapi/block-core.json | 19 +++++++++++++++++++
> 5 files changed, 73 insertions(+)
>
> diff --git a/block/trace-events b/block/trace-events
> index 11c8d5f590..8f61566770 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -46,6 +46,7 @@ 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
> diff --git a/blockdev.c b/blockdev.c
> index 2c0773bba7..5e8edff322 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3849,6 +3849,20 @@ 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 ea216aca5e..5531f5c2ab 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -58,6 +58,7 @@ enum BlockJobVerb {
> BLOCK_JOB_VERB_RESUME,
> BLOCK_JOB_VERB_SET_SPEED,
> BLOCK_JOB_VERB_COMPLETE,
> + BLOCK_JOB_VERB_DISMISS,
> BLOCK_JOB_VERB__MAX
> };
>
> @@ -68,6 +69,7 @@ bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
> [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0},
> [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1},
> [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1},
> + [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0},
This makes it very obvious that the patches should probably be
reordered.
> };
>
> static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
> @@ -426,6 +428,13 @@ static void block_job_cancel_async(BlockJob *job)
> job->cancelled = true;
> }
>
> +static void block_job_do_dismiss(BlockJob *job)
> +{
> + assert(job && job->manual == true);
> + block_job_state_transition(job, BLOCK_JOB_STATUS_UNDEFINED);
I don't think you made that an allowed transition from anywhere?
> + block_job_unref(job);
> +}
> +
> static int block_job_finish_sync(BlockJob *job,
> void (*finish)(BlockJob *, Error **errp),
> Error **errp)
> @@ -455,6 +464,9 @@ static int block_job_finish_sync(BlockJob *job,
> aio_poll(qemu_get_aio_context(), true);
> }
> ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
> + if (job->manual) {
> + block_job_do_dismiss(job);
> + }
> block_job_unref(job);
> return ret;
> }
> @@ -570,6 +582,24 @@ 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 (!job->manual) {
> + error_setg(errp, "The active block job '%s' was not started with "
> + "\'manual\': true, and so cannot be dismissed as it will "
> + "clean up after itself automatically", job->id);
> + return;
> + }
If you check instead that the job is in the right state to be dismissed
(CONCLUDED), the job->manual check wouldn't be needed any more because
the user can never catch an automatically completed job in CONCLUDED.
> + error_setg(errp, "unimplemented");
This should hopefully go away when you reorder the patches.
> + block_job_do_dismiss(job);
> + *jobptr = NULL;
> +}
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 02/14] blockjobs: Add status enum
2018-01-29 17:04 ` Kevin Wolf
@ 2018-01-29 17:38 ` John Snow
0 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-29 17:38 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, pkrempa, jtc, qemu-devel
On 01/29/2018 12:04 PM, Kevin Wolf wrote:
> Am 27.01.2018 um 03:05 hat John Snow geschrieben:
>> We're about to add several new states, and booleans are becoming
>> unwieldly and difficult to reason about. 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.
>> CREATED: replaces (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.
>>
>> 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.
>> This status does not feature for non-transactional jobs.
>> 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. If this job is in a transaction, it has
>> already left the WAITING status.
>> CONCLUDED: Job has ceased all operations and has a return code available
>> for query and may be dismissed via block_job_dismiss.
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Empty line before S-o-b?
>
>> blockjob.c | 10 ++++++++++
>> include/block/blockjob.h | 4 ++++
>> qapi/block-core.json | 17 ++++++++++++++++-
>> 3 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/blockjob.c b/blockjob.c
>> index 9850d70cb0..6eb783a354 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -321,6 +321,7 @@ 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);
>> }
>>
>> @@ -601,6 +602,10 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
>> info->speed = job->speed;
>> info->io_status = job->iostatus;
>> info->ready = job->ready;
>> + if (job->manual) {
>> + info->has_status = true;
>> + info->status = job->status;
>> + }
>
> Why do we want to hide the status from clients that don't want to
> complete the job manually? Isn't this conflating two orthogonal things?
>
Later in the series I do take care to not set any "new" status booleans
if the manual property was unset, but at this point when I was writing
it I believe my thought process was to hide new information if possible.
(i.e.: please use the old management information or the new management
information, but ideally not both.)
Also, it is technically the case that the "pause" semantics between the
boolean and the status field may differ ever-so-slightly, but I suppose
there's no harm in exposing the extra information.
>> return info;
>> }
>>
>> @@ -704,6 +709,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>> job->pause_count = 1;
>> job->refcnt = 1;
>> job->manual = manual;
>> + 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);
>> @@ -808,9 +814,12 @@ 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 = BLOCK_JOB_STATUS_PAUSED;
>> job->paused = true;
>> block_job_do_yield(job, -1);
>> job->paused = false;
>> + job->status = status;
>> }
>>
>> if (job->driver->resume) {
>> @@ -916,6 +925,7 @@ 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/include/block/blockjob.h b/include/block/blockjob.h
>> index b94d0c9fa6..d8e7df7e6e 100644
>> --- a/include/block/blockjob.h
>> +++ b/include/block/blockjob.h
>> @@ -146,6 +146,10 @@ typedef struct BlockJob {
>> */
>> bool manual;
>>
>> + /* Current state, using 2.12+ state names
>> + */
>> + BlockJobStatus status;
>> +
>> /** Non-NULL if this job is part of a transaction */
>> BlockJobTxn *txn;
>> QLIST_ENTRY(BlockJob) txn_list;
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 8225308904..eac89754c1 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -955,6 +955,18 @@
>> { 'enum': 'BlockJobType',
>> 'data': ['commit', 'stream', 'mirror', 'backup'] }
>>
>> +##
>> +# @BlockJobStatus:
>> +#
>> +# Block Job State
>> +#
>> +#
>> +#
>> +# Since: 2.12
>> +##
>> +{ 'enum': 'BlockJobStatus',
>> + 'data': ['undefined', 'created', 'running', 'paused', 'ready'] }
>
> I assume these multiple empty lines are left there so you have space to
> fill in the information from the commit message?
>
Whoops. Yup.
>> ##
>> # @BlockJobInfo:
>> #
>> @@ -981,12 +993,15 @@
>> #
>> # @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' } }
>
> If we don't actually have a reason to hide the status above, it can
> become a non-opional field here.
>
> Kevin
>
Yeah, not a strong reason in retrospect. I will expose the field.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property
2018-01-29 17:34 ` John Snow
@ 2018-01-29 17:46 ` Kevin Wolf
2018-01-29 17:52 ` John Snow
0 siblings, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2018-01-29 17:46 UTC (permalink / raw)
To: John Snow; +Cc: pkrempa, jtc, qemu-devel, qemu-block
Am 29.01.2018 um 18:34 hat John Snow geschrieben:
> On 01/29/2018 11:59 AM, Kevin Wolf wrote:
> > Am 27.01.2018 um 03:05 hat John Snow geschrieben:
> >> This property will be used to opt-in to the new BlockJobs workflow
> >> that allows a tighter, more explicit control over transitions from
> >> one runstate to another.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >
> >> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> >> index 00403d9482..b94d0c9fa6 100644
> >> --- a/include/block/blockjob.h
> >> +++ b/include/block/blockjob.h
> >> @@ -141,6 +141,11 @@ typedef struct BlockJob {
> >> */
> >> QEMUTimer sleep_timer;
> >>
> >> + /* Set to true when management API has requested 2.12+ job lifetime
> >> + * management semantics.
> >> + */
> >> + bool manual;
> >
> > Wouldn't it make more sense to describe what "2.12+ job lifetime
> > management semantics" actually are? Maybe then it would be easy to find
> > a more specific name, too, like manual_completion.
> >
>
> Sure. At the time I wrote it, I wasn't sure what they were. Maybe I'll
> find out after the review; but I'll make a note to document it here.
>
> > In fact, I wonder if the opposite flag wouldn't be nicer, i.e. having a
> > bool auto_completion (or finalization or whatever that extra step was
> > called in the final draft), defaulting to true.
> >
>
> I could do that, if you feel it'd be more readable. In terms of style I
> tend to prefer new additions default to false as that feels more
> permanently reliable, but it's only a preference.
Yeah, that is one way to look at it. The other one is that options
should generally be positive, i.e. true enables a feature rather than
disabling it. If I look at the interface without its history, the
feature (the extra thing on top of the basic state machine) that is
enabled or disabled is automatic completion.
This is why my preference would be the other way round, but it's still
only a preference. :-)
After reading a few more patches, it also seems to me that you are
looking a bit differently at the whole state machine than I am. So you
seem to assume two different state machines depending on whether manual
is set, whereas I assume all jobs share the same state machine and only
some transitions are optionally manual or automatic.
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property
2018-01-29 17:46 ` Kevin Wolf
@ 2018-01-29 17:52 ` John Snow
0 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-29 17:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pkrempa, jtc, qemu-devel, qemu-block
On 01/29/2018 12:46 PM, Kevin Wolf wrote:
> Am 29.01.2018 um 18:34 hat John Snow geschrieben:
>> On 01/29/2018 11:59 AM, Kevin Wolf wrote:
>>> Am 27.01.2018 um 03:05 hat John Snow geschrieben:
>>>> This property will be used to opt-in to the new BlockJobs workflow
>>>> that allows a tighter, more explicit control over transitions from
>>>> one runstate to another.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>>> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
>>>> index 00403d9482..b94d0c9fa6 100644
>>>> --- a/include/block/blockjob.h
>>>> +++ b/include/block/blockjob.h
>>>> @@ -141,6 +141,11 @@ typedef struct BlockJob {
>>>> */
>>>> QEMUTimer sleep_timer;
>>>>
>>>> + /* Set to true when management API has requested 2.12+ job lifetime
>>>> + * management semantics.
>>>> + */
>>>> + bool manual;
>>>
>>> Wouldn't it make more sense to describe what "2.12+ job lifetime
>>> management semantics" actually are? Maybe then it would be easy to find
>>> a more specific name, too, like manual_completion.
>>>
>>
>> Sure. At the time I wrote it, I wasn't sure what they were. Maybe I'll
>> find out after the review; but I'll make a note to document it here.
>>
>>> In fact, I wonder if the opposite flag wouldn't be nicer, i.e. having a
>>> bool auto_completion (or finalization or whatever that extra step was
>>> called in the final draft), defaulting to true.
>>>
>>
>> I could do that, if you feel it'd be more readable. In terms of style I
>> tend to prefer new additions default to false as that feels more
>> permanently reliable, but it's only a preference.
>
> Yeah, that is one way to look at it. The other one is that options
> should generally be positive, i.e. true enables a feature rather than
> disabling it. If I look at the interface without its history, the
> feature (the extra thing on top of the basic state machine) that is
> enabled or disabled is automatic completion.
>
> This is why my preference would be the other way round, but it's still
> only a preference. :-)
>
> After reading a few more patches, it also seems to me that you are
> looking a bit differently at the whole state machine than I am. So you
> seem to assume two different state machines depending on whether manual
> is set, whereas I assume all jobs share the same state machine and only
> some transitions are optionally manual or automatic.
>
> Kevin
>
Yeah, I realize that splitting the state machine in two isn't the best
possible thing that's ever happened, but I did want to try my best to
isolate the changes to when the new boolean is supplied.
If you have a thought that gives us a more graceful unification without
breaking old behavior, please let me know. The code is already kind of
confusing and this does indeed make it worse.
Of course, for a generic jobs API, we can just say "Expanded workflow
or nothing" and be done with it.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table
2018-01-29 17:17 ` Kevin Wolf
@ 2018-01-29 19:07 ` John Snow
2018-01-29 19:56 ` Kevin Wolf
0 siblings, 1 reply; 28+ messages in thread
From: John Snow @ 2018-01-29 19:07 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pkrempa, jtc, qemu-devel, qemu-block
On 01/29/2018 12:17 PM, Kevin Wolf wrote:
> Am 27.01.2018 um 03:05 hat John Snow geschrieben:
>> 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.
>>
>> undefined: May only transition to 'created'.
>> created: May only transition to 'running'.
>> It cannot transition to pause directly, but if a created job
>> is requested to pause prior to starting, it will transition
>> synchronously to 'running' and then to 'paused'.
>> running: May transition either to 'paused' or 'ready'.
>> paused: May transition to either 'running' or 'ready', but only in
>> terms of returning to that prior state.
>> p->r->y is not possible, but this is not encapsulated by the
>> STM table.
>
> Do you mean y->p->r->y here? If not, I don't understand.
Whoops, Yes, I mean to say that Y->P->R is not possible.
That is, a paused state can only return to where it came from, but the
STM doesn't show this restriction. Really, this hints at there being
*two* paused states, but I didn't want to go through the trouble of
adding a new one.
>
>> ready: May transition to paused.
I swear my script used to add blank lines for me here. *shrug*
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> blockjob.c | 39 ++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/blockjob.c b/blockjob.c
>> index 6eb783a354..d084a1e318 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -42,6 +42,35 @@
>> * 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 */
>> + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0},
>> + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0},
>> + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1},
>> + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1},
>> + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0},
>> +};
>> +
>> +static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
>> +{
>> + BlockJobStatus s0 = job->status;
>> + if (s0 == s1) {
>> + return;
>> + }
>> + assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX);
>> + assert(BlockJobSTT[s0][s1]);
>> + switch (s1) {
>> + case BLOCK_JOB_STATUS_WAITING:
>> + case BLOCK_JOB_STATUS_PENDING:
>> + case BLOCK_JOB_STATUS_CONCLUDED:
>> + assert(job->manual);
>> + default:
>> + break;
>> + }
>
> This doesn't compile because the constants don't exist yet.
>
*cough* oops.
> Apart from that, I think the assertion is misguided, too. Which states a
> job goes through shouldn't depend on whether the client wants to
> complete the job manually or have it completed automatically. The
> difference should only be which state transitions are automatic.
> > In other words, WAITING/PENDING/CONCLUDED may never be visible in
> query-block-job for automatically completed jobs, but the jobs should
> still (synchronously) go through all of these states.
>
Hmm. OK, I can look at doing it in this way. I will probably also have
it omit the events in this case too, though.
>> + job->status = s1;
>> +}
>> +
>
> Kevin
>
Thanks!
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table
2018-01-29 19:07 ` John Snow
@ 2018-01-29 19:56 ` Kevin Wolf
0 siblings, 0 replies; 28+ messages in thread
From: Kevin Wolf @ 2018-01-29 19:56 UTC (permalink / raw)
To: John Snow; +Cc: pkrempa, jtc, qemu-devel, qemu-block
Am 29.01.2018 um 20:07 hat John Snow geschrieben:
>
>
> On 01/29/2018 12:17 PM, Kevin Wolf wrote:
> > Am 27.01.2018 um 03:05 hat John Snow geschrieben:
> >> 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.
> >>
> >> undefined: May only transition to 'created'.
> >> created: May only transition to 'running'.
> >> It cannot transition to pause directly, but if a created job
> >> is requested to pause prior to starting, it will transition
> >> synchronously to 'running' and then to 'paused'.
> >> running: May transition either to 'paused' or 'ready'.
> >> paused: May transition to either 'running' or 'ready', but only in
> >> terms of returning to that prior state.
> >> p->r->y is not possible, but this is not encapsulated by the
> >> STM table.
> >
> > Do you mean y->p->r->y here? If not, I don't understand.
>
> Whoops, Yes, I mean to say that Y->P->R is not possible.
>
> That is, a paused state can only return to where it came from, but the
> STM doesn't show this restriction. Really, this hints at there being
> *two* paused states, but I didn't want to go through the trouble of
> adding a new one.
Hm, yes... It would probably be cleaner to have two states. If a client
queries the state and just sees PAUSED, it doesn't know whether the bulk
phase has already finished.
> >
> >> ready: May transition to paused.
>
> I swear my script used to add blank lines for me here. *shrug*
>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >> blockjob.c | 39 ++++++++++++++++++++++++++++++++++-----
> >> 1 file changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/blockjob.c b/blockjob.c
> >> index 6eb783a354..d084a1e318 100644
> >> --- a/blockjob.c
> >> +++ b/blockjob.c
> >> @@ -42,6 +42,35 @@
> >> * 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 */
> >> + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0},
> >> + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0},
> >> + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1},
> >> + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1},
> >> + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 1, 0},
> >> +};
> >> +
> >> +static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
> >> +{
> >> + BlockJobStatus s0 = job->status;
> >> + if (s0 == s1) {
> >> + return;
> >> + }
> >> + assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX);
> >> + assert(BlockJobSTT[s0][s1]);
> >> + switch (s1) {
> >> + case BLOCK_JOB_STATUS_WAITING:
> >> + case BLOCK_JOB_STATUS_PENDING:
> >> + case BLOCK_JOB_STATUS_CONCLUDED:
> >> + assert(job->manual);
> >> + default:
> >> + break;
> >> + }
> >
> > This doesn't compile because the constants don't exist yet.
> >
>
> *cough* oops.
>
> > Apart from that, I think the assertion is misguided, too. Which states a
> > job goes through shouldn't depend on whether the client wants to
> > complete the job manually or have it completed automatically. The
> > difference should only be which state transitions are automatic.
> > > In other words, WAITING/PENDING/CONCLUDED may never be visible in
> > query-block-job for automatically completed jobs, but the jobs should
> > still (synchronously) go through all of these states.
> >
>
> Hmm. OK, I can look at doing it in this way. I will probably also have
> it omit the events in this case too, though.
Actually, while there is probably no use for the events, I don't think
they would hurt if omitting them isn't completely trivial. Clients
always need to be prepared to see new unknown events.
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss
2018-01-29 17:38 ` Kevin Wolf
@ 2018-01-29 20:33 ` John Snow
0 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-01-29 20:33 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pkrempa, jtc, qemu-devel, qemu-block
On 01/29/2018 12:38 PM, Kevin Wolf wrote:
> Am 27.01.2018 um 03:05 hat John Snow geschrieben:
>> For jobs that have reached their terminal 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 that jobs do not yet linger in any such state, they are freed
>> immediately upon reaching this previously-unnamed state. such a state is
>> added immediately in the next commit.
>>
>> Valid objects:
>> Concluded: (added in a future commit); dismisses the concluded job.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/trace-events | 1 +
>> blockdev.c | 14 ++++++++++++++
>> blockjob.c | 30 ++++++++++++++++++++++++++++++
>> include/block/blockjob.h | 9 +++++++++
>> qapi/block-core.json | 19 +++++++++++++++++++
>> 5 files changed, 73 insertions(+)
>>
>> diff --git a/block/trace-events b/block/trace-events
>> index 11c8d5f590..8f61566770 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -46,6 +46,7 @@ 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
>> diff --git a/blockdev.c b/blockdev.c
>> index 2c0773bba7..5e8edff322 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3849,6 +3849,20 @@ 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 ea216aca5e..5531f5c2ab 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -58,6 +58,7 @@ enum BlockJobVerb {
>> BLOCK_JOB_VERB_RESUME,
>> BLOCK_JOB_VERB_SET_SPEED,
>> BLOCK_JOB_VERB_COMPLETE,
>> + BLOCK_JOB_VERB_DISMISS,
>> BLOCK_JOB_VERB__MAX
>> };
>>
>> @@ -68,6 +69,7 @@ bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
>> [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0},
>> [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1},
>> [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1},
>> + [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0},
>
> This makes it very obvious that the patches should probably be
> reordered.
>
I think this bit is fine, but the half-baked dismiss error checking and
the broken transition to UNDEFINED before deference makes a better case.
I'll play with it.
>> };
>>
>> static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
>> @@ -426,6 +428,13 @@ static void block_job_cancel_async(BlockJob *job)
>> job->cancelled = true;
>> }
>>
>> +static void block_job_do_dismiss(BlockJob *job)
>> +{
>> + assert(job && job->manual == true);
>> + block_job_state_transition(job, BLOCK_JOB_STATUS_UNDEFINED);
>
> I don't think you made that an allowed transition from anywhere?
>
Ah, you know, this is true... until the next commit.
>> + block_job_unref(job);
>> +}
>> +
>> static int block_job_finish_sync(BlockJob *job,
>> void (*finish)(BlockJob *, Error **errp),
>> Error **errp)
>> @@ -455,6 +464,9 @@ static int block_job_finish_sync(BlockJob *job,
>> aio_poll(qemu_get_aio_context(), true);
>> }
>> ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
>> + if (job->manual) {
>> + block_job_do_dismiss(job);
>> + }
>> block_job_unref(job);
>> return ret;
>> }
>> @@ -570,6 +582,24 @@ 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 (!job->manual) {
>> + error_setg(errp, "The active block job '%s' was not started with "
>> + "\'manual\': true, and so cannot be dismissed as it will "
>> + "clean up after itself automatically", job->id);
>> + return;
>> + }
>
> If you check instead that the job is in the right state to be dismissed
> (CONCLUDED), the job->manual check wouldn't be needed any more because
> the user can never catch an automatically completed job in CONCLUDED.
>
>> + error_setg(errp, "unimplemented");
>
> This should hopefully go away when you reorder the patches.
>
>> + block_job_do_dismiss(job);
>> + *jobptr = NULL;
>> +}
>
> Kevin
>
Thanks,
--js
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
` (14 preceding siblings ...)
2018-01-27 2:25 ` [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management no-reply
@ 2018-02-01 0:08 ` John Snow
15 siblings, 0 replies; 28+ messages in thread
From: John Snow @ 2018-02-01 0:08 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, jtc, qemu-devel
On 01/26/2018 09:05 PM, John Snow wrote:
> For jobs that complete when a monitor isn't looking, there's no way to
> tell what the job's final return code was. We need to allow jobs to
> remain in the list until queried for reliable management.
>
> Furthermore, it's not viable to have graph changes when the monitor
> isn't looking either. We need at least another event for that.
>
> This series is a rough sketch for the WAITING, PENDING and CONCLUDED
> events that accompany an expanded job management scheme that a management
> client can opt-in to.
>
> V3:
> - Added WAITING and PENDING events
> - Added block_job_finalize verb
> - Added .pending() callback for jobs
> - Tweaked how .commit/.abort work
>
> V2:
> - Added tests!
> - Changed property name (Jeff, Paolo)
>
> RFC / Known problems:
> - I need a lot more tests.
> - Jobs need to actually implement their .pending callback for this to be of any
> actual use.
> - Mirror needs to be refactored to use the commit/abort/pending/clean callbacks
> to fulfill the promise made by "no graph changes without user authorization"
> that PENDING is supposed to offer
> - Jobs beyond backup will be able to opt-in to the new management scheme in the
> next version.
> - V4 will include a forced synchronicity for jobs in a transaction; i.e. all
> jobs will be forced to use either the old or new styles, but not a mix.
>
> Please take a look and shout loudly if I'm wandering in the wrong direction.
>
> ________________________________________________________________________________
>
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch block-job-reap
> https://github.com/jnsnow/qemu/tree/block-job-reap
>
> This version is tagged block-job-reap-v3:
> https://github.com/jnsnow/qemu/releases/tag/block-job-reap-v3
>
> John Snow (14):
> blockjobs: add manual property
> blockjobs: Add status enum
> blockjobs: add state transition table
> blockjobs: RFC add block_job_verb permission table
> blockjobs: add block_job_dismiss
> blockjobs: add CONCLUDED state
> blockjobs: ensure abort is called for cancelled jobs
> blockjobs: add commit, abort, clean helpers
> blockjobs: add prepare callback
> blockjobs: Add waiting event
> blockjobs: add PENDING status and event
> blockjobs: add block-job-finalize
> blockjobs: Expose manual property
> iotests: test manual job dismissal
>
> block/backup.c | 22 ++--
> block/commit.c | 2 +-
> block/mirror.c | 2 +-
> block/replication.c | 5 +-
> block/stream.c | 2 +-
> block/trace-events | 2 +
> blockdev.c | 42 ++++++-
> blockjob.c | 279 ++++++++++++++++++++++++++++++++++++++++---
> include/block/block_int.h | 9 +-
> include/block/blockjob.h | 38 ++++++
> include/block/blockjob_int.h | 12 +-
> qapi/block-core.json | 135 +++++++++++++++++++--
> tests/qemu-iotests/056 | 241 +++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/056.out | 4 +-
> tests/test-bdrv-drain.c | 4 +-
> tests/test-blockjob-txn.c | 2 +-
> tests/test-blockjob.c | 4 +-
> 17 files changed, 750 insertions(+), 55 deletions(-)
>
NACK
There are a lot of changes I've already made to this series based on
Kevin's recommendations; please wait for the next revision.
And a lot of bugs in this series I've already found.
--js
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2018-02-01 0:08 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-27 2:05 [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property John Snow
2018-01-29 16:59 ` Kevin Wolf
2018-01-29 17:34 ` John Snow
2018-01-29 17:46 ` Kevin Wolf
2018-01-29 17:52 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 02/14] blockjobs: Add status enum John Snow
2018-01-29 17:04 ` Kevin Wolf
2018-01-29 17:38 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table John Snow
2018-01-29 17:17 ` Kevin Wolf
2018-01-29 19:07 ` John Snow
2018-01-29 19:56 ` Kevin Wolf
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 04/14] blockjobs: RFC add block_job_verb permission table John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss John Snow
2018-01-29 17:38 ` Kevin Wolf
2018-01-29 20:33 ` John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 06/14] blockjobs: add CONCLUDED state John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 07/14] blockjobs: ensure abort is called for cancelled jobs John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 08/14] blockjobs: add commit, abort, clean helpers John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 09/14] blockjobs: add prepare callback John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 10/14] blockjobs: Add waiting event John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 11/14] blockjobs: add PENDING status and event John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 12/14] blockjobs: add block-job-finalize John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 13/14] blockjobs: Expose manual property John Snow
2018-01-27 2:05 ` [Qemu-devel] [RFC v3 14/14] iotests: test manual job dismissal John Snow
2018-01-27 2:25 ` [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management no-reply
2018-02-01 0:08 ` John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.