blockjobs: add explicit job management
v3 cover blockjobs: add explicit job management
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(-)
Signature · 1 line
2.14.3
RFC v3 1/14 blockjobs: add manual property
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.
--- 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(-)
block/backup.c +10 -10
@@ -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,
block/commit.c +1 -1
@@ -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 ;
block/mirror.c +1 -1
@@ -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,
block/replication.c +3 -2
@@ -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);
block/stream.c +1 -1
@@ -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 |
blockdev.c +2 -2
@@ -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);
blockjob.c +2 -1
@@ -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);
include/block/block_int.h +3 -3
@@ -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,
include/block/blockjob.h +5 -0
@@ -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;
include/block/blockjob_int.h +1 -1
@@ -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);
tests/test-bdrv-drain.c +2 -2
@@ -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);
tests/test-blockjob-txn.c +1 -1
@@ -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;
tests/test-blockjob.c +2 -2
@@ -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);
Signature · 1 line
2.14.3
Re: [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 11 lines
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 <[email protected]>include/block/blockjob.h +0
-0
include/block/blockjob.h typedef struct BlockJob { · line 147
Quoted 7 lines
include/block/blockjob.h +4
-0@@ -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
Re: [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property
Quoted 15 lines · Kevin Wolf
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 14 lines
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 <[email protected]>include/block/blockjob.h +0
-0@@ -141,6 +141,11 @@ typedef struct BlockJob {*/ QEMUTimer sleep_timer;
Quoted 9 lines
include/block/blockjob.h typedef struct BlockJob { · line 147
Quoted 4 lines
include/block/blockjob.h +4
-0+/* 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.
Quoted 4 lines
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.
Quoted 12 lines
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.
Re: [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property
Am 29.01.2018 um 18:34 hat John Snow geschrieben:
Quoted 29 lines · Kevin Wolf
Quoted 15 lines
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 14 lines
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 <[email protected]>include/block/blockjob.h +0
-0@@ -141,6 +141,11 @@ typedef struct BlockJob {*/ QEMUTimer sleep_timer;Quoted 9 lines
include/block/blockjob.h typedef struct BlockJob { · line 147
Quoted 4 lines
include/block/blockjob.h +4
-0+/* 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.
Quoted 8 lines
Quoted 4 lines
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
Re: [Qemu-devel] [RFC v3 01/14] blockjobs: add manual property
Quoted 48 lines · Kevin Wolf
Am 29.01.2018 um 18:34 hat John Snow geschrieben:
Quoted 29 lines · Kevin Wolf
Quoted 15 lines
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 14 lines
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 <[email protected]>include/block/blockjob.h +0
-0@@ -141,6 +141,11 @@ typedef struct BlockJob {*/ QEMUTimer sleep_timer;Quoted 9 lines
include/block/blockjob.h typedef struct BlockJob { · line 147
Quoted 4 lines
include/block/blockjob.h +4
-0+/* 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.
Quoted 8 lines
Quoted 4 lines
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. :-)
Quoted 8 lines
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.
RFC v3 2/14 blockjobs: Add status enum
In reply to John SnowWe'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.
--- blockjob.c | 10 ++++++++++ include/block/blockjob.h | 4 ++++ qapi/block-core.json | 17 ++++++++++++++++- 3 files changed, 30 insertions(+), 1 deletion(-)
blockjob.c +10 -0
@@ -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)) {
include/block/blockjob.h +4 -0
@@ -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;
qapi/block-core.json +16 -1
@@ -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:
Signature · 1 line
2.14.3
Re: [Qemu-devel] [RFC v3 02/14] blockjobs: Add status enum
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 25 lines
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: replacesjob->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 viablock_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 viablock_job_dismiss.
Quoted 1 line
Signed-off-by John Snow <[email protected]>
Empty line before S-o-b?
Quoted 18 lines
blockjob.c | 10 ++++++++++ include/block/blockjob.h | 4 ++++ qapi/block-core.json | 17 ++++++++++++++++- 3 files changed, 30 insertions(+), 1 deletion(-)blockjob.c +1
-0@@ -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); }
blockjob.c BlockJobInfo *block_job_query(BlockJob *job, Error **errp) · line 608
Quoted 7 lines
blockjob.c +4
-0@@ -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?
Quoted 56 lines
blockjob.c +5
-0return 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)) {include/block/blockjob.h +4
-0@@ -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;qapi/block-core.json +1
-0@@ -955,6 +955,18 @@ { 'enum': 'BlockJobType', 'data': ['commit', 'stream', 'mirror', 'backup'] } +##
qapi/block-core.json line 968
Quoted 10 lines
qapi/block-core.json +10
-0+# @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?
Quoted 7 lines
qapi/block-core.json +0
-0## # @BlockJobInfo: #@@ -981,12 +993,15 @@ # # @ready: true if the job may be completed (since 2.2) #
qapi/block-core.json line 1004
Quoted 10 lines
qapi/block-core.json +4
-1+# @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
Re: [Qemu-devel] [RFC v3 02/14] blockjobs: Add status enum
Quoted 47 lines · Kevin Wolf
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 25 lines
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: replacesjob->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 viablock_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 viablock_job_dismiss.Quoted 1 line
Signed-off-by John Snow <[email protected]>Empty line before S-o-b?
Quoted 17 lines
blockjob.c | 10 ++++++++++ include/block/blockjob.h | 4 ++++ qapi/block-core.json | 17 ++++++++++++++++- 3 files changed, 30 insertions(+), 1 deletion(-)blockjob.c +1
-0@@ -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); }
Quoted 12 lines
blockjob.c BlockJobInfo *block_job_query(BlockJob *job, Error **errp) · line 608
Quoted 8 lines
blockjob.c +4
-0@@ -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.
Quoted 55 lines
Quoted 55 lines
blockjob.c +5
-0return 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)) {include/block/blockjob.h +4
-0@@ -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;qapi/block-core.json +0
-0@@ -955,6 +955,18 @@ { 'enum': 'BlockJobType', 'data': ['commit', 'stream', 'mirror', 'backup'] }
Quoted 15 lines
qapi/block-core.json line 968
Quoted 11 lines
qapi/block-core.json +11
-0+## +# @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.
Quoted 21 lines
Quoted 7 lines
qapi/block-core.json +0
-0## # @BlockJobInfo: #@@ -981,12 +993,15 @@ # # @ready: true if the job may be completed (since 2.2) #qapi/block-core.json line 1004
Quoted 10 lines
qapi/block-core.json +4
-1+# @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.
Quoted 2 lines
Kevin
Yeah, not a strong reason in retrospect. I will expose the field.
RFC v3 3/14 blockjobs: add state transition table
In reply to John SnowThe 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.
--- blockjob.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)
blockjob.c +34 -5
@@ -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)) {
Signature · 1 line
2.14.3
Re: [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 6 lines
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.
Quoted 10 lines
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->yis 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.
Quoted 30 lines
ready: May transition to paused.
Signed-off-by John Snow <[email protected]>--- blockjob.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)blockjob.c +16
-0@@ -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 ; + }
blockjob.c line 70
Quoted 10 lines
blockjob.c +10
-0+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.
blockjob.c line 73
Quoted 3 lines
blockjob.c +3
-0+ job->status = s1; +} +
Kevin
Re: [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table
Quoted 18 lines · Kevin Wolf
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 6 lines
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.
Quoted 10 lines
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->yis not possible, but this is not encapsulated by the STM table.
Quoted 1 line
Do you mean
y->p->r->yhere? 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.
Quoted 2 lines
Quoted 2 lines
ready: May transition to paused.
I swear my script used to add blank lines for me here. *shrug*
Quoted 33 lines
Quoted 33 lines
Signed-off-by John Snow <[email protected]>--- blockjob.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)blockjob.c +20
-0@@ -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 :
Quoted 9 lines
blockjob.c line 70
Quoted 6 lines
blockjob.c +6
-0+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.
Quoted 8 lines
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.
Quoted 1 line
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.
Quoted 6 lines
blockjob.c line 73
Quoted 3 lines
blockjob.c +3
-0+ job->status = s1; +} +Kevin
Thanks!
Re: [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table
Am 29.01.2018 um 20:07 hat John Snow geschrieben:
Quoted 21 lines · Kevin Wolf
Quoted 18 lines
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 16 lines
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->yis not possible, but this is not encapsulated by the STM table.
Quoted 8 lines
Quoted 1 line
Do you mean
y->p->r->yhere? If not, I don't understand.Whoops, Yes, I mean to say that
Y->P->Ris 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.
Quoted 54 lines
Quoted 2 lines
Quoted 2 lines
ready: May transition to paused.
I swear my script used to add blank lines for me here. *shrug*
Quoted 33 lines
Quoted 33 lines
Signed-off-by John Snow <[email protected]>--- blockjob.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)blockjob.c +20
-0@@ -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 :Quoted 9 lines
blockjob.c line 70
Quoted 6 lines
blockjob.c +6
-0+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.
Quoted 4 lines
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.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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
RFC v3 4/14 blockjobs: RFC add block_job_verb permission table
In reply to John SnowWhich 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.
--- blockjob.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
blockjob.c +18 -0
@@ -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;
Signature · 1 line
2.14.3
RFC v3 5/14 blockjobs: add block_job_dismiss
In reply to John SnowFor 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.
--- 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(+)
block/trace-events +1 -0
@@ -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
blockdev.c +14 -0
@@ -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,
blockjob.c +30 -0
@@ -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;
include/block/blockjob.h +9 -0
@@ -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.
qapi/block-core.json +19 -0
@@ -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: #
Signature · 1 line
2.14.3
Re: [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 74 lines
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 <[email protected]>--- 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(+)block/trace-events +1
-0@@ -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.cblockdev.c +14
-0@@ -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,blockjob.c +1
-0@@ -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 };
blockjob.c bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { · line 72
Quoted 4 lines
blockjob.c +1
-0@@ -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.
blockjob.c static void block_job_cancel_async(BlockJob *job) · line 434
Quoted 11 lines
blockjob.c +4
-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 );
I don't think you made that an allowed transition from anywhere?
Quoted 21 lines
blockjob.c +7
-0+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.c void block_job_complete(BlockJob *job, Error **errp) · line 595
Quoted 10 lines
blockjob.c +10
-0+{ +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.
blockjob.c void block_job_complete(BlockJob *job, Error **errp) · line 597
Quoted 1 line
blockjob.c +1
-0+error_setg (errp,"unimplemented" );
This should hopefully go away when you reorder the patches.
blockjob.c void block_job_complete(BlockJob *job, Error **errp) · line 601
Quoted 3 lines
blockjob.c +3
-0+block_job_do_dismiss (job); + *jobptr =NULL ; +}
Kevin
Re: [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss
Quoted 74 lines · Kevin Wolf
Am 27.01.2018 um 03:05 hat John Snow geschrieben:
Quoted 73 lines
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 <[email protected]>--- 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(+)block/trace-events +1
-0@@ -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.cblockdev.c +14
-0@@ -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,blockjob.c +1
-0@@ -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 };
Quoted 9 lines
blockjob.c bool BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { · line 72
Quoted 5 lines
blockjob.c +1
-0@@ -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.
Quoted 7 lines
Quoted 7 lines
blockjob.c +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; }
Quoted 7 lines
blockjob.c static void block_job_cancel_async(BlockJob *job) · line 434
Quoted 4 lines
blockjob.c +4
-0+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.
Quoted 38 lines
Quoted 21 lines
blockjob.c +7
-0+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.c void block_job_complete(BlockJob *job, Error **errp) · line 595
Quoted 10 lines
blockjob.c +10
-0+{ +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->manualcheck wouldn't be needed any more because the user can never catch an automatically completed job in CONCLUDED.blockjob.c void block_job_complete(BlockJob *job, Error **errp) · line 597
Quoted 1 line
blockjob.c +1
-0+error_setg (errp,"unimplemented" );
Quoted 8 lines
This should hopefully go away when you reorder the patches.
blockjob.c void block_job_complete(BlockJob *job, Error **errp) · line 601
Quoted 3 lines
blockjob.c +3
-0+block_job_do_dismiss (job); + *jobptr =NULL ; +}Kevin
Thanks,
--js
RFC v3 6/14 blockjobs: add CONCLUDED state
In reply to John Snowadd 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.
--- blockjob.c | 56 +++++++++++++++++++++++++++++++++++++--------------- qapi/block-core.json | 8 +++++--- 2 files changed, 45 insertions(+), 19 deletions(-)
blockjob.c +40 -16
@@ -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; }
qapi/block-core.json +5 -3
@@ -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. #
Signature · 1 line
2.14.3
RFC v3 7/14 blockjobs: ensure abort is called for cancelled jobs
In reply to John SnowPresently, 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.
--- block/backup.c | 2 +- blockjob.c | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
block/backup.c +1 -1
@@ -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);
blockjob.c +5 -0
@@ -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);
Signature · 1 line
2.14.3
RFC v3 8/14 blockjobs: add commit, abort, clean helpers
In reply to John SnowThe completed_single function is getting a little mucked up with checking to see which callbacks exist, so let's factor them out.
--- blockjob.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)
blockjob.c +26 -9
@@ -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);
Signature · 1 line
2.14.3
RFC v3 9/14 blockjobs: add prepare callback
In reply to John SnowSome 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.
--- blockjob.c | 35 ++++++++++++++++++++++++++++++++++- include/block/blockjob.h | 3 +++ include/block/blockjob_int.h | 10 ++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-)
blockjob.c +34 -1
@@ -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);
include/block/blockjob.h +3 -0
@@ -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;
include/block/blockjob_int.h +10 -0
@@ -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
Signature · 1 line
2.14.3
RFC v3 10/14 blockjobs: Add waiting event
In reply to John SnowFor 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.
--- blockjob.c | 45 +++++++++++++++++++++++++++++++-------------- qapi/block-core.json | 25 ++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 15 deletions(-)
blockjob.c +31 -14
@@ -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); } }
qapi/block-core.json +24 -1
@@ -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: #
Signature · 1 line
2.14.3
RFC v3 11/14 blockjobs: add PENDING status and event
In reply to John SnowFor 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.
--- blockjob.c | 45 ++++++++++++++++++++++++++++++--------------- qapi/block-core.json | 26 +++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 16 deletions(-)
blockjob.c +30 -15
@@ -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) {
qapi/block-core.json +25 -1
@@ -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: #
Signature · 1 line
2.14.3
RFC v3 12/14 blockjobs: add block-job-finalize
In reply to John Snow--- 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(-)
block/trace-events +1 -0
@@ -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"
blockdev.c +14 -0
@@ -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;
blockjob.c +59 -13
128 lines not on this page · raw message
include/block/blockjob.h +17 -0
@@ -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.
qapi/block-core.json +18 -0
@@ -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: #
Signature · 1 line
2.14.3
RFC v3 13/14 blockjobs: Expose manual property
In reply to John SnowExpose 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.
--- blockdev.c | 14 ++++++++++---- include/block/block_int.h | 3 +++ qapi/block-core.json | 32 ++++++++++++++++++++++++++------ 3 files changed, 39 insertions(+), 10 deletions(-)
blockdev.c +10 -4
@@ -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 ) {
include/block/block_int.h +3 -0
@@ -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.
qapi/block-core.json +26 -6
@@ -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' } }
Signature · 1 line
2.14.3
RFC v3 14/14 iotests: test manual job dismissal
In reply to John SnowRFC: 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!"
--- tests/qemu-iotests/056 | 241 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/056.out | 4 +- 2 files changed, 243 insertions(+), 2 deletions(-)
tests/qemu-iotests/056 +241 -0
@@ -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'])
tests/qemu-iotests/056.out +2 -2
@@ -1,5 +1,5 @@ -... +......... ---------------------------------------------------------------------- -Ran 3 tests +Ran 9 tests OK
Signature · 1 line
2.14.3
This series seems to have some coding style problems. See output below for more information:
Hi,
This series seems to have some coding style problems. See output below for more information:
Type: series
Message-id: [email protected]
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/[email protected] -> patchew/[email protected]
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 event22b6b95b6e blockjobs: add prepare callback51530e8516 blockjobs: add commit, abort, clean helpers 45761a2219 blockjobs: ensure abort is called for cancelled jobs d7317d4525 blockjobs: add CONCLUDED statea4939037ed blockjobs: add block_job_dismiss 83b14366be blockjobs: RFC add block_job_verb permission table 446a6e487b blockjobs: add state transition table bf5fb419c1 blockjobs: Add status enum85487f375a 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 [email protected]
John Snow
NAKQuoted 70 lines · 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
- Addedblock_job_finalizeverb
- Added .pending() callback for jobs
- Tweaked how .commit/.abort workV2:
- 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-reapThis version is tagged block-job-reap-v3:
https://github.com/jnsnow/qemu/releases/tag/block-job-reap-v3John Snow (14):
blockjobs: add manual property
blockjobs: Add status enum
blockjobs: add state transition table
blockjobs: RFC addblock_job_verbpermission table
blockjobs: addblock_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 dismissalblock/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 ++++++
Quoted 9 lines
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