blockjobs: add explicit job culling
cover blockjobs: add explicit job culling
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.
This is an RFC; tests are on the way.
(Tested only manually via qmp-shell for now.)
John Snow (3):
blockjob: add manual-cull property
qmp: add block-job-cull command
blockjob: expose manual-cull property
block/backup.c | 20 +++++++++--------- block/commit.c | 2 +- block/mirror.c | 2 +- block/replication.c | 5 +++-- block/stream.c | 2 +- block/trace-events | 1 + blockdev.c | 28 +++++++++++++++++++++---- blockjob.c | 46 +++++++++++++++++++++++++++++++++++++++-- include/block/block_int.h | 8 +++++--- include/block/blockjob.h | 21 +++++++++++++++++++ include/block/blockjob_int.h | 2 +- qapi/block-core.json | 49 ++++++++++++++++++++++++++++++++++++-------- tests/test-blockjob-txn.c | 2 +- tests/test-blockjob.c | 2 +- 14 files changed, 155 insertions(+), 35 deletions(-)
Signature · 1 line
2.9.5
PATCH 1/3 blockjob: add manual-cull property
Add a "manual cull" property to block jobs that forces them to linger in the block job list (visible to QMP queries) until the user explicitly dismisses them via QMP.
The cull command itself is implemented in the next commit, and the feature is exposed to drive-backup and blockdev-backup in the subsequent commit.
--- block/backup.c | 20 +++++++++---------- block/commit.c | 2 +- block/mirror.c | 2 +- block/replication.c | 5 +++-- block/stream.c | 2 +- blockdev.c | 8 ++++---- blockjob.c | 46 ++++++++++++++++++++++++++++++++++++++++++-- include/block/block_int.h | 8 +++++--- include/block/blockjob.h | 21 ++++++++++++++++++++ include/block/blockjob_int.h | 2 +- qapi/block-core.json | 7 ++++--- tests/test-blockjob-txn.c | 2 +- tests/test-blockjob.c | 2 +- 13 files changed, 97 insertions(+), 30 deletions(-)
block/backup.c +10 -10
@@ -532,15 +532,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_cull, +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;
@@ -608,7 +608,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_cull, 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
@@ -304,7 +304,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
@@ -1180,7 +1180,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
@@ -539,8 +539,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 +4 -4
@@ -3290,8 +3290,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, } } - job =backup_job_create (backup->job_id, bs, target_bs, backup->speed, - backup->sync, bmap, backup->compress, + 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);bdrv_unref (target_bs);
@@ -3369,8 +3369,8 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, goto out; } } - job =backup_job_create (backup->job_id, bs, target_bs, backup->speed, - backup->sync,NULL , backup->compress, + 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);if (local_err !=NULL ) {
blockjob.c +44 -2
100 lines not on this page · raw message
include/block/block_int.h +5 -3
@@ -944,6 +944,8 @@ 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_cull: Whether or not this job, when completed, will need to be +* manually culled via block-job-cull or not. * @bs: Block device to operate on. * @target: Block device to write to. * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -960,9 +962,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_cull, + BlockDriverState *bs, BlockDriverState *target, + int64_t speed, MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, bool compress, BlockdevOnError on_source_error,
include/block/blockjob.h +21 -0
@@ -135,6 +135,15 @@ typedef struct BlockJob {*/ int ret; +/* True if this job must remain present post-completion until +* it can be queried and removed via block-job-cull. */ +bool manual_cull; + +/* True if and only if manual_cull is true and the job has finished +* calling either abort or commit (if applicable) and can be reaped +* via interactive user command. */ +bool finished; +/** Non-NULL if this job is part of a transaction */ BlockJobTxn *txn;QLIST_ENTRY (BlockJob) txn_list;
@@ -227,6 +236,18 @@ void block_job_cancel(BlockJob *job);void block_job_complete (BlockJob *job,Error **errp);/** +* block_job_cull: +* @job: The job to be culled. +* @errp: Error object. +* +* For completed or canceled jobs, remove the job object +* from the block_job_query list and perform final cleanup. +* +* Note that at this stage, the job cannot be rolled back. +*/ +void block_job_cull (BlockJob **job,Error **errp); + +/** * block_job_query: * @job: The job to get information about. *
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_cull,BlockDriverState *bs,uint64_t perm,uint64_t shared_perm,int64_t speed,int flags,BlockCompletionFunc *cb,void *opaque,Error **errp);
qapi/block-core.json +4 -3
@@ -984,9 +984,10 @@ # 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'} } + 'data': {'type': 'str', 'device': 'str', 'len': 'int', 'offset': 'int', + 'busy': 'bool', 'paused': 'bool', 'speed': 'int', + 'io-status': 'BlockDeviceIoStatus', 'ready': 'bool', + 'finished': 'bool'} } ## # @query-block-jobs:
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 +1 -1
@@ -30,7 +30,7 @@ static BlockJob *do_test_id(BlockBackend *blk, const char *id,BlockJob *job;Error *errp =NULL ; - job =block_job_create (id, &test_block_job_driver,blk_bs (blk), + job =block_job_create (id, &test_block_job_driver, false,blk_bs (blk),0 ,BLK_PERM_ALL ,0 ,BLOCK_JOB_DEFAULT , block_job_cb,NULL , &errp);if (should_succeed) {
Signature · 1 line
2.9.5
PATCH 2/3 qmp: add block-job-cull command
In reply to John SnowFor jobs that have finished (either completed or canceled), allow the user to dismiss the job's status reports via block-job-cull.
--- block/trace-events | 1 + blockdev.c | 14 ++++++++++++++ qapi/block-core.json | 21 +++++++++++++++++++++ 3 files changed, 36 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_cull(void *job) "job %p" qmp_block_stream(void *bs, void *job) "bs %p job %p" # block/file-win32.c
blockdev.c +14 -0
@@ -3766,6 +3766,20 @@ void qmp_block_job_complete(const char *device, Error **errp)aio_context_release (aio_context); } +void qmp_block_job_cull (const char *device,Error **errp) +{ +AioContext *aio_context; +BlockJob *job =find_block_job (device, &aio_context, errp); + +if (!job) { +return ; + } + +trace_qmp_block_job_cull (job); +block_job_cull (&job, errp); +aio_context_release (aio_context); +} + void qmp_change_backing_file(const char *device, const char *image_node_name, const char *backing_file,
qapi/block-core.json +21 -0
@@ -2161,6 +2161,27 @@ { 'command': 'block-job-complete', 'data': { 'device': 'str' } } ## +# @block-job-cull: +# +# For jobs that have already completed, remove them from the block-job-query +# list. This command only needs to be run for jobs which were started with the +# manual-cull=true option. +# +# 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. +# +# @device: The job identifier. This used to be a device name (hence +# the name of the parameter), but since QEMU 2.7 it can have +# other values. +# +# Returns: Nothing on success +# +# Since: 2.11 +## +{ 'command': 'block-job-cull', 'data': { 'device': 'str' } } + +## # @BlockdevDiscardOptions: # # Determines how to handle discard requests.
Signature · 1 line
2.9.5
PATCH 3/3 blockjob: expose manual-cull property
In reply to John SnowFor drive-backup and blockdev-backup, expose the manual-cull property, having it default to false. There are no universal creation parameters, so it must be added to each job type that it makes sense for individually.
--- blockdev.c | 10 ++++++++-- qapi/block-core.json | 21 ++++++++++++++++----- 2 files changed, 24 insertions(+), 7 deletions(-)
blockdev.c +8 -2
@@ -3198,6 +3198,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,if (!backup->has_job_id) { backup->job_id =NULL ; } +if (!backup->has_manual_cull) { + backup->manual_cull = false; + }if (!backup->has_compress) { backup->compress = false; }
@@ -3290,7 +3293,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, } } - job =backup_job_create (backup->job_id, false, bs, target_bs, + job =backup_job_create (backup->job_id, backup->manual_cull, 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);
@@ -3341,6 +3344,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,if (!backup->has_job_id) { backup->job_id =NULL ; } +if (!backup->has_manual_cull) { + backup->manual_cull = false; + }if (!backup->has_compress) { backup->compress = false; }
@@ -3369,7 +3375,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, goto out; } } - job =backup_job_create (backup->job_id, false, bs, target_bs, + job =backup_job_create (backup->job_id, backup->manual_cull, 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);
qapi/block-core.json +16 -5
@@ -1104,6 +1104,11 @@ # @job-id: identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # +# @manual-cull: Whether or not the job created by this command needs to be +# cleaned up manually via block-job-cull or not. The default is +# false. When true, the job will remain in a "completed" state +# until culled manually with block-job-cull. (Since 2.11) +# # @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
@@ -1144,9 +1149,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-cull': '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' } }
@@ -1156,6 +1162,11 @@ # @job-id: identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # +# @manual-cull: Whether or not the job created by this command needs to be +# cleaned up manually via block-job-cull or not. The default is +# false. When true, the job will remain in a "completed" state +# until culled manually with block-job-cull. (Since 2.11) +# # @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.
@@ -1185,8 +1196,8 @@ # Since: 2.3 ## { 'struct': 'BlockdevBackup', - 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', - 'sync': 'MirrorSyncMode', + 'data': { '*job-id': 'str', '*manual-cull': 'bool', 'device': 'str', + 'target': 'str', 'sync': 'MirrorSyncMode', '*speed': 'int', '*compress': 'bool', '*on-source-error': 'BlockdevOnError',
Signature · 1 line
2.9.5
Re: [Qemu-devel] [PATCH 3/3] blockjob: expose manual-cull property
Quoted 84 lines · John Snow
For drive-backup and blockdev-backup, expose the manual-cull property, having it default to false. There are no universal creation parameters, so it must be added to each job type that it makes sense for individually.
Signed-off-by John Snow <[email protected]>--- blockdev.c | 10 ++++++++-- qapi/block-core.json | 21 ++++++++++++++++----- 2 files changed, 24 insertions(+), 7 deletions(-)blockdev.c +8
-2@@ -3198,6 +3198,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,if (!backup->has_job_id) { backup->job_id =NULL ; } +if (!backup->has_manual_cull) { + backup->manual_cull = false; + }if (!backup->has_compress) { backup->compress = false; }@@ -3290,7 +3293,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, } } - job =backup_job_create (backup->job_id, false, bs, target_bs, + job =backup_job_create (backup->job_id, backup->manual_cull, 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);@@ -3341,6 +3344,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,if (!backup->has_job_id) { backup->job_id =NULL ; } +if (!backup->has_manual_cull) { + backup->manual_cull = false; + }if (!backup->has_compress) { backup->compress = false; }@@ -3369,7 +3375,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, goto out; } } - job =backup_job_create (backup->job_id, false, bs, target_bs, + job =backup_job_create (backup->job_id, backup->manual_cull, 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);qapi/block-core.json +9
-3@@ -1104,6 +1104,11 @@ # @job-id: identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # +# @manual-cull: Whether or not the job created by this command needs to be +# cleaned up manually via block-job-cull or not. The default is +# false. When true, the job will remain in a "completed" state +# until culled manually with block-job-cull. (Since 2.11) +# # @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@@ -1144,9 +1149,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-cull': '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' } }
qapi/block-core.json line 1168
Quoted 7 lines
qapi/block-core.json +4
-0@@ -1156,6 +1162,11 @@ # @job-id: identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # +# @manual-cull: Whether or not the job created by this command needs to be +# cleaned up manually via block-job-cull or not. The default is +# false. When true, the job will remain in a "completed" state +# until culled manually with block-job-cull. (Since 2.11)
The verb "cull" is a bit weird. The only alternative that comes to mind though are "reap" (like processes). There's also "join" (like threads), but would imply waiting if the jobs hasn't completed yet, and we probably don't want it.
Paolo
Quoted 15 lines
qapi/block-core.json +2
-2# @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.@@ -1185,8 +1196,8 @@ # Since: 2.3 ## { 'struct': 'BlockdevBackup', - 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', - 'sync': 'MirrorSyncMode', + 'data': { '*job-id': 'str', '*manual-cull': 'bool', 'device': 'str', + 'target': 'str', 'sync': 'MirrorSyncMode', '*speed': 'int', '*compress': 'bool', '*on-source-error': 'BlockdevOnError',
Re: [Qemu-devel] [PATCH 3/3] blockjob: expose manual-cull property
Quoted 7 lines · Paolo Bonzini
Quoted 6 lines · John Snow
For drive-backup and blockdev-backup, expose the manual-cull property, having it default to false. There are no universal creation parameters, so it must be added to each job type that it makes sense for individually.
Signed-off-by John Snow <[email protected]>
[...]
Quoted 8 lines
The verb "cull" is a bit weird. The only alternative that comes to mind though are "reap" (like processes). There's also "join" (like threads), but would imply waiting if the jobs hasn't completed yet, and we probably don't want it.
Paolo
Sure, open to suggestions. I think Kevin suggested "delete" which I have reservations about because of people potentially confusing it with "cancel" or "complete" -- it does not have the capacity to end/terminate/finish/complete/cancel a job.
"reap" might be fine. I don't really have any strong preference.
--js
Re: [Qemu-devel] [PATCH 3/3] blockjob: expose manual-cull property
Quoted 22 lines · John Snow
Quoted 7 lines · Paolo Bonzini
Quoted 6 lines · John Snow
For drive-backup and blockdev-backup, expose the manual-cull property, having it default to false. There are no universal creation parameters, so it must be added to each job type that it makes sense for individually.
Signed-off-by John Snow <[email protected]>[...]
Quoted 8 lines
The verb "cull" is a bit weird. The only alternative that comes to mind though are "reap" (like processes). There's also "join" (like threads), but would imply waiting if the jobs hasn't completed yet, and we probably don't want it.
Paolo
Quoted 7 lines
Sure, open to suggestions. I think Kevin suggested "delete" which I have reservations about because of people potentially confusing it with "cancel" or "complete" -- it does not have the capacity to end/terminate/finish/complete/cancel a job.
"reap" might be fine. I don't really have any strong preference.
As far as verbs go, I like both 'reap' and 'delete'. As far as the property, naming it 'manual_verb' is a bit odd, too. Maybe a clearer term for the property would just be 'persistent', with the QMP command being 'block_job_reap' or 'block_job_delete'?
-Jeff
Re: [Qemu-devel] [PATCH 3/3] blockjob: expose manual-cull property
Quoted 31 lines · Jeff Cody
Quoted 22 lines · John Snow
Quoted 7 lines · Paolo Bonzini
Quoted 6 lines · John Snow
For drive-backup and blockdev-backup, expose the manual-cull property, having it default to false. There are no universal creation parameters, so it must be added to each job type that it makes sense for individually.
Signed-off-by John Snow <[email protected]>[...]
Quoted 8 lines
The verb "cull" is a bit weird. The only alternative that comes to mind though are "reap" (like processes). There's also "join" (like threads), but would imply waiting if the jobs hasn't completed yet, and we probably don't want it.
Paolo
Quoted 7 lines
Sure, open to suggestions. I think Kevin suggested "delete" which I have reservations about because of people potentially confusing it with "cancel" or "complete" -- it does not have the capacity to end/terminate/finish/complete/cancel a job.
"reap" might be fine. I don't really have any strong preference.
Quoted 7 lines
As far as verbs go, I like both 'reap' and 'delete'. As far as the property, naming it '
manual_verb' is a bit odd, too. Maybe a clearer term for the property would just be 'persistent', with the QMP command being 'block_job_reap' or 'block_job_delete'?-Jeff
As they say, two hard problems in Computer Science ...
Quoted 6 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.
This is an RFC; tests are on the way.
(Tested only manually via qmp-shell for now.)
That's a cool feature!
What about transactions support?
Quoted 22 lines
John Snow (3):
blockjob: add manual-cull property
qmp: add block-job-cull command
blockjob: expose manual-cull propertyblock/backup.c | 20 +++++++++--------- block/commit.c | 2 +- block/mirror.c | 2 +- block/replication.c | 5 +++-- block/stream.c | 2 +- block/trace-events | 1 + blockdev.c | 28 +++++++++++++++++++++---- blockjob.c | 46 +++++++++++++++++++++++++++++++++++++++-- include/block/block_int.h | 8 +++++--- include/block/blockjob.h | 21 +++++++++++++++++++ include/block/blockjob_int.h | 2 +- qapi/block-core.json | 49 ++++++++++++++++++++++++++++++++++++-------- tests/test-blockjob-txn.c | 2 +- tests/test-blockjob.c | 2 +- 14 files changed, 155 insertions(+), 35 deletions(-)
Signature · 2 lines
Best regards, Vladimir
Quoted 11 lines · Vladimir Sementsov-Ogievskiy
Quoted 6 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.
This is an RFC; tests are on the way.
(Tested only manually via qmp-shell for now.)That's a cool feature!
What about transactions support?
Didn't test that yet (!) but the intent is that it will be compatible. The jobs in the transaction, whether using grouped-completion mode or not, will simply hang around in the list after completion.
For grouped-completion=false:
The jobs will complete individually and then remain in the list.
For grouped-completion=true:
The jobs will remain in their ready-to-commit-or-abort state until all jobs in the transaction are ready to commit-or-abort, then all jobs will either commit or abort. After commit-or-abort, all jobs (that were created with manual-cull=true !) will remain in the query list.
The intended effect here is that this property changes NOTHING except that the job will remain in the query list until it is dismissed, and should not change anything about how it behaves during its lifetime.
One downside here is that since we have no universal "job creation argument list" that I can't add it to all jobs universally. In the case of transactions, though, I could at least add a property that *forces* all jobs below to become manual-cull style jobs, and that way you'd only have to specify it once instead of for each action.
--js
Quoted 16 lines
Quoted 16 lines
John Snow (3):
blockjob: add manual-cull property
qmp: add block-job-cull command
blockjob: expose manual-cull propertyblock/backup.c | 20 +++++++++--------- block/commit.c | 2 +- block/mirror.c | 2 +-block/replication.c | 5 +++-- block/stream.c | 2 +- block/trace-events | 1 +
blockdev.c | 28 +++++++++++++++++++++---- blockjob.c | 46
+++++++++++++++++++++++++++++++++++++++--
include/block/block_int.h | 8 +++++---
Quoted 10 lines
Quoted 8 lines
include/block/blockjob.h | 21 +++++++++++++++++++
include/block/blockjob_int.h| 2 +- qapi/block-core.json | 49
++++++++++++++++++++++++++++++++++++--------
tests/test-blockjob-txn.c | 2 +- tests/test-blockjob.c | 2 +-14 files changed, 155 insertions(+), 35 deletions(-)
Oh, while I'm here, I should point out that another downside of this patch is that it doesn't prevent "cancel" from attempting to re-enter the job. Or rather, I had to patch that out specifically. The job remains in a list of jobs that some portions of the code consider to be "active" jobs. (look for any code that checks to see if the job has started.)
A (perhaps) more provably cleaner approach would be to simply move any completed job onto a different list upon completion, and patch query-jobs to query both lists, and allow the cull command to remove any jobs on that list. A downside of that approach, however, is that without multiple job support, you may launch a second job that perhaps overwrites the first job unless you're careful about how you manage that data.
There are pros and cons to either way, but I'd rather not get in the business of overhauling the blockjobs API unless it's for universal jobs.
--js