| Message ID | 20231013092143.365296-2-f.ebner@proxmox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mirror: allow switching from background to active mode | expand |
Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben: > which will allow changing job-type-specific options after job > creation. > > In the JobVerbTable, the same allow bits as for set-speed are used, > because set-speed can be considered an existing change command. > > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > diff --git a/job.c b/job.c > index 72d57f0934..99a2e54b54 100644 > --- a/job.c > +++ b/job.c > @@ -80,6 +80,7 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = { > [JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0}, > [JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0}, > [JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0}, > + [JOB_VERB_CHANGE] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, > }; I'm not sure if I would have included JOB_STATUS_CREATED, i.e. before the job has even started, but it's not necessarily a problem. The implementation just need to be careful to work even in early stages. But probably the early stages include some part of JOB_STATUS_RUNNING, too, so they'd have to do this anyway. Kevin
Am 18.10.23 um 17:52 schrieb Kevin Wolf: > Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben: >> which will allow changing job-type-specific options after job >> creation. >> >> In the JobVerbTable, the same allow bits as for set-speed are used, >> because set-speed can be considered an existing change command. >> >> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > >> diff --git a/job.c b/job.c >> index 72d57f0934..99a2e54b54 100644 >> --- a/job.c >> +++ b/job.c >> @@ -80,6 +80,7 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = { >> [JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0}, >> [JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0}, >> [JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0}, >> + [JOB_VERB_CHANGE] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, >> }; > > I'm not sure if I would have included JOB_STATUS_CREATED, i.e. before > the job has even started, but it's not necessarily a problem. The > implementation just need to be careful to work even in early stages. But > probably the early stages include some part of JOB_STATUS_RUNNING, too, > so they'd have to do this anyway. > As mentioned in the commit message, I copied the bits from JOB_VERB_SET_SPEED, because that seemed to be very similar, as it can also be seen as a change operation (and who knows, maybe it can be merged into JOB_VERB_CHANGE at some point in the future). But I can remove the bit if you want me to. Best Regards, Fiona
Am 23.10.2023 um 11:31 hat Fiona Ebner geschrieben: > Am 18.10.23 um 17:52 schrieb Kevin Wolf: > > Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben: > >> which will allow changing job-type-specific options after job > >> creation. > >> > >> In the JobVerbTable, the same allow bits as for set-speed are used, > >> because set-speed can be considered an existing change command. > >> > >> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> > >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > >> diff --git a/job.c b/job.c > >> index 72d57f0934..99a2e54b54 100644 > >> --- a/job.c > >> +++ b/job.c > >> @@ -80,6 +80,7 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = { > >> [JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0}, > >> [JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0}, > >> [JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0}, > >> + [JOB_VERB_CHANGE] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, > >> }; > > > > I'm not sure if I would have included JOB_STATUS_CREATED, i.e. before > > the job has even started, but it's not necessarily a problem. The > > implementation just need to be careful to work even in early stages. But > > probably the early stages include some part of JOB_STATUS_RUNNING, too, > > so they'd have to do this anyway. > > > > As mentioned in the commit message, I copied the bits from > JOB_VERB_SET_SPEED, because that seemed to be very similar, as it can > also be seen as a change operation (and who knows, maybe it can be > merged into JOB_VERB_CHANGE at some point in the future). But I can > remove the bit if you want me to. I think it's fine as it is, just something to keep in mind while reviewing implementations. What you could do is adding something to the comment for the change callback in blockjob_int.h, like "Note that this can already be called before the job coroutine is running". Kevin
diff --git a/blockdev.c b/blockdev.c index 325b7a3bef..d0e274ff8b 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3344,6 +3344,20 @@ void qmp_block_job_dismiss(const char *id, Error **errp) job_dismiss_locked(&job, errp); } +void qmp_block_job_change(BlockJobChangeOptions *opts, Error **errp) +{ + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(opts->id, errp); + + if (!job) { + return; + } + + block_job_change_locked(job, opts, errp); +} + void qmp_change_backing_file(const char *device, const char *image_node_name, const char *backing_file, diff --git a/blockjob.c b/blockjob.c index 58c5d64539..d53bc775d2 100644 --- a/blockjob.c +++ b/blockjob.c @@ -328,6 +328,26 @@ static bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) return block_job_set_speed_locked(job, speed, errp); } +void block_job_change_locked(BlockJob *job, BlockJobChangeOptions *opts, + Error **errp) +{ + const BlockJobDriver *drv = block_job_driver(job); + + GLOBAL_STATE_CODE(); + + if (job_apply_verb_locked(&job->job, JOB_VERB_CHANGE, errp)) { + return; + } + + if (drv->change) { + job_unlock(); + drv->change(job, opts, errp); + job_lock(); + } else { + error_setg(errp, "Job type does not support change"); + } +} + void block_job_ratelimit_processed_bytes(BlockJob *job, uint64_t n) { IO_CODE(); diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 058b0c824c..95854f1477 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -172,6 +172,17 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs); */ bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp); +/** + * block_job_change_locked: + * @job: The job to change. + * @opts: The new options. + * @errp: Error object. + * + * Change the job according to opts. + */ +void block_job_change_locked(BlockJob *job, BlockJobChangeOptions *opts, + Error **errp); + /** * block_job_query_locked: * @job: The job to get information about. diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index 104824040c..f604985315 100644 --- a/include/block/blockjob_int.h +++ b/include/block/blockjob_int.h @@ -67,6 +67,11 @@ struct BlockJobDriver { void (*attached_aio_context)(BlockJob *job, AioContext *new_context); void (*set_speed)(BlockJob *job, int64_t speed); + + /* + * Change the @job's options according to @opts. + */ + void (*change)(BlockJob *job, BlockJobChangeOptions *opts, Error **errp); }; /* diff --git a/job.c b/job.c index 72d57f0934..99a2e54b54 100644 --- a/job.c +++ b/job.c @@ -80,6 +80,7 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = { [JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0}, [JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0}, [JOB_VERB_DISMISS] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0}, + [JOB_VERB_CHANGE] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0}, }; /* Transactional group of jobs */ diff --git a/qapi/block-core.json b/qapi/block-core.json index 89751d81f2..c6f31a9399 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3044,6 +3044,32 @@ { 'command': 'block-job-finalize', 'data': { 'id': 'str' }, 'allow-preconfig': true } +## +# @BlockJobChangeOptions: +# +# Block job options that can be changed after job creation. +# +# @id: The job identifier +# +# @type: The job type +# +# Since 8.2 +## +{ 'union': 'BlockJobChangeOptions', + 'base': { 'id': 'str', 'type': 'JobType' }, + 'discriminator': 'type', + 'data': {} } + +## +# @block-job-change: +# +# Change the block job's options. +# +# Since: 8.2 +## +{ 'command': 'block-job-change', + 'data': 'BlockJobChangeOptions', 'boxed': true } + ## # @BlockdevDiscardOptions: # diff --git a/qapi/job.json b/qapi/job.json index 7f0ba090de..b3957207a4 100644 --- a/qapi/job.json +++ b/qapi/job.json @@ -105,11 +105,13 @@ # # @finalize: see @job-finalize # +# @change: see @block-job-change (since 8.2) +# # Since: 2.12 ## { 'enum': 'JobVerb', 'data': ['cancel', 'pause', 'resume', 'set-speed', 'complete', 'dismiss', - 'finalize' ] } + 'finalize', 'change' ] } ## # @JOB_STATUS_CHANGE: