| Message ID | 20180223235142.21501-8-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 24.02.2018 um 00:51 hat John Snow geschrieben: > Which commands ("verbs") are appropriate for jobs in which state is > also somewhat burdensome to keep track of. > > As of this commit, it looks rather useless, but begins to look more > interesting the more states we add to the STM table. > > A recurring theme is that no verb will apply to an 'undefined' job. > > Further, it's not presently possible to restrict the "pause" or "resume" > verbs any more than they are in this commit because of the asynchronous > nature of how jobs enter the PAUSED state; justifications for some > seemingly erroneous applications are given below. > > ===== > Verbs > ===== > > Cancel: Any state except undefined. > Pause: Any state except undefined; > 'created': Requests that the job pauses as it starts. > 'running': Normal usage. (PAUSED) > 'paused': The job may be paused for internal reasons, > but the user may wish to force an indefinite > user-pause, so this is allowed. > 'ready': Normal usage. (STANDBY) > 'standby': Same logic as above. > Resume: Any state except undefined; > 'created': Will lift a user's pause-on-start request. > 'running': Will lift a pause request before it takes effect. > 'paused': Normal usage. > 'ready': Will lift a pause request before it takes effect. > 'standby': Normal usage. > Set-speed: Any state except undefined, though ready may not be meaningful. > Complete: Only a 'ready' job may accept a complete request. > > > ======= > Changes > ======= > > (1) > > To facilitate "nice" error checking, all five major block-job verb > interfaces in blockjob.c now support an errp parameter: > > - block_job_user_cancel is added as a new interface. > - block_job_user_pause gains an errp paramter > - block_job_user_resume gains an errp parameter > - block_job_set_speed already had an errp parameter. > - block_job_complete already had an errp parameter. > > (2) > > block-job-pause and block-job-resume will no longer no-op when trying > to pause an already paused job, or trying to resume a job that isn't > paused. These functions will now report that they did not perform the > action requested because it was not possible. > > iotests have been adjusted to address this new behavior. > > (3) > > block-job-complete doesn't worry about checking !block_job_started, > because the permission table guards against this. > > (4) > > test-bdrv-drain's job implementation needs to announce that it is > 'ready' now, in order to be completed. > > Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
On 02/23/2018 05:51 PM, John Snow wrote: > Which commands ("verbs") are appropriate for jobs in which state is > also somewhat burdensome to keep track of. > > As of this commit, it looks rather useless, but begins to look more > interesting the more states we add to the STM table. > > A recurring theme is that no verb will apply to an 'undefined' job. Back to the argument of whether we even want that state. > > ======= > Changes > ======= > > (1) > > To facilitate "nice" error checking, all five major block-job verb > interfaces in blockjob.c now support an errp parameter: > > - block_job_user_cancel is added as a new interface. > - block_job_user_pause gains an errp paramter > - block_job_user_resume gains an errp parameter > - block_job_set_speed already had an errp parameter. > - block_job_complete already had an errp parameter. > > (2) > > block-job-pause and block-job-resume will no longer no-op when trying > to pause an already paused job, or trying to resume a job that isn't > paused. These functions will now report that they did not perform the > action requested because it was not possible. > > iotests have been adjusted to address this new behavior. Seems reasonable. Hopefully shouldn't trip up libvirt too badly (if libvirt attempted a redundant job transition that used to silently succeed and now fails, the failure message should be pretty obvious that it was a no-op attempt). > > (3) > > block-job-complete doesn't worry about checking !block_job_started, > because the permission table guards against this. > > (4) > > test-bdrv-drain's job implementation needs to announce that it is > 'ready' now, in order to be completed. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- Reviewed-by: Eric Blake <eblake@redhat.com>
On 02/27/2018 02:25 PM, Eric Blake wrote: > On 02/23/2018 05:51 PM, John Snow wrote: >> Which commands ("verbs") are appropriate for jobs in which state is >> also somewhat burdensome to keep track of. >> >> As of this commit, it looks rather useless, but begins to look more >> interesting the more states we add to the STM table. >> >> A recurring theme is that no verb will apply to an 'undefined' job. > > Back to the argument of whether we even want that state. > By design, it is intended to reject all commands. Though, what I can do is just remove the UNDEFINED state but have the "CREATED" state start at "1" still. In effect, this gives you the empty row and column for the 'UNDEFINED' state, except now it's *literally* undefined. However, the transition then needs to be set explicitly. The way the code is written now, the status *only* ever changes from the transition function, which makes it easy to search for and reason about all possible transition points which I consider a feature of the design. >> >> ======= >> Changes >> ======= >> >> (1) >> >> To facilitate "nice" error checking, all five major block-job verb >> interfaces in blockjob.c now support an errp parameter: >> >> - block_job_user_cancel is added as a new interface. >> - block_job_user_pause gains an errp paramter >> - block_job_user_resume gains an errp parameter >> - block_job_set_speed already had an errp parameter. >> - block_job_complete already had an errp parameter. >> >> (2) >> >> block-job-pause and block-job-resume will no longer no-op when trying >> to pause an already paused job, or trying to resume a job that isn't >> paused. These functions will now report that they did not perform the >> action requested because it was not possible. >> >> iotests have been adjusted to address this new behavior. > > Seems reasonable. Hopefully shouldn't trip up libvirt too badly (if > libvirt attempted a redundant job transition that used to silently > succeed and now fails, the failure message should be pretty obvious that > it was a no-op attempt). > Yeah, it's arguable whether or not this is an API change. (A) I'm not prohibiting anything that would have worked before. I am just notifying the client that whatever they were trying to do had no effect. (B) That notification is an error, though, so some code paths that may have relied on jamming pause signals into the pipe may be surprised at the new response. If I can get away with it, I obviously prefer to warn the user that the pause/resume had no effect or couldn't be applied. >> >> (3) >> >> block-job-complete doesn't worry about checking !block_job_started, >> because the permission table guards against this. >> >> (4) >> >> test-bdrv-drain's job implementation needs to announce that it is >> 'ready' now, in order to be completed. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- > > Reviewed-by: Eric Blake <eblake@redhat.com> >
=====
Verbs
=====
Cancel: Any state except undefined.
Pause: Any state except undefined;
'created': Requests that the job pauses as it starts.
'running': Normal usage. (PAUSED)
'paused': The job may be paused for internal reasons,
but the user may wish to force an indefinite
user-pause, so this is allowed.
'ready': Normal usage. (STANDBY)
'standby': Same logic as above.
Resume: Any state except undefined;
'created': Will lift a user's pause-on-start request.
'running': Will lift a pause request before it takes effect.
'paused': Normal usage.
'ready': Will lift a pause request before it takes effect.
'standby': Normal usage.
Set-speed: Any state except undefined, though ready may not be meaningful.
Complete: Only a 'ready' job may accept a complete request.
=======
Changes
=======
(1)
To facilitate "nice" error checking, all five major block-job verb
interfaces in blockjob.c now support an errp parameter:
- block_job_user_cancel is added as a new interface.
- block_job_user_pause gains an errp paramter
- block_job_user_resume gains an errp parameter
- block_job_set_speed already had an errp parameter.
- block_job_complete already had an errp parameter.
(2)
block-job-pause and block-job-resume will no longer no-op when trying
to pause an already paused job, or trying to resume a job that isn't
paused. These functions will now report that they did not perform the
action requested because it was not possible.
iotests have been adjusted to address this new behavior.
(3)
block-job-complete doesn't worry about checking !block_job_started,
because the permission table guards against this.
(4)
test-bdrv-drain's job implementation needs to announce that it is
'ready' now, in order to be completed.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/trace-events | 1 +
blockdev.c | 10 +++----
blockjob.c | 71 ++++++++++++++++++++++++++++++++++++++++++------
include/block/blockjob.h | 13 +++++++--
qapi/block-core.json | 20 ++++++++++++++
tests/test-bdrv-drain.c | 1 +
6 files changed, 100 insertions(+), 16 deletions(-)
diff --git a/block/trace-events b/block/trace-events
index b75a0c8409..3fe89f7ea6 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -6,6 +6,7 @@ bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
# blockjob.c
block_job_state_transition(void *job, int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)"
+block_job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)"
# block/block-backend.c
blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x"
diff --git a/blockdev.c b/blockdev.c
index 3fb1ca803c..cba935a0a6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3805,7 +3805,7 @@ void qmp_block_job_cancel(const char *device,
}
trace_qmp_block_job_cancel(job);
- block_job_cancel(job);
+ block_job_user_cancel(job, errp);
out:
aio_context_release(aio_context);
}
@@ -3815,12 +3815,12 @@ void qmp_block_job_pause(const char *device, Error **errp)
AioContext *aio_context;
BlockJob *job = find_block_job(device, &aio_context, errp);
- if (!job || block_job_user_paused(job)) {
+ if (!job) {
return;
}
trace_qmp_block_job_pause(job);
- block_job_user_pause(job);
+ block_job_user_pause(job, errp);
aio_context_release(aio_context);
}
@@ -3829,12 +3829,12 @@ void qmp_block_job_resume(const char *device, Error **errp)
AioContext *aio_context;
BlockJob *job = find_block_job(device, &aio_context, errp);
- if (!job || !block_job_user_paused(job)) {
+ if (!job) {
return;
}
trace_qmp_block_job_resume(job);
- block_job_user_resume(job);
+ block_job_user_resume(job, errp);
aio_context_release(aio_context);
}
diff --git a/blockjob.c b/blockjob.c
index d745b3bb69..4e424fef72 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -53,6 +53,15 @@ bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = {
/* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0},
};
+bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
+ /* U, C, R, P, Y, S */
+ [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1},
+ [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1},
+ [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1},
+ [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1},
+ [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0},
+};
+
static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
{
BlockJobStatus s0 = job->status;
@@ -70,6 +79,23 @@ static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
job->status = s1;
}
+static int block_job_apply_verb(BlockJob *job, BlockJobVerb bv, Error **errp)
+{
+ assert(bv >= 0 && bv <= BLOCK_JOB_VERB__MAX);
+ trace_block_job_apply_verb(job, qapi_enum_lookup(&BlockJobStatus_lookup,
+ job->status),
+ qapi_enum_lookup(&BlockJobVerb_lookup, bv),
+ BlockJobVerbTable[bv][job->status] ?
+ "allowed" : "prohibited");
+ if (BlockJobVerbTable[bv][job->status]) {
+ return 0;
+ }
+ error_setg(errp, "Job '%s' in state '%s' cannot accept command verb '%s'",
+ job->id, qapi_enum_lookup(&BlockJobStatus_lookup, job->status),
+ qapi_enum_lookup(&BlockJobVerb_lookup, bv));
+ return -EPERM;
+}
+
static void block_job_lock(void)
{
qemu_mutex_lock(&block_job_mutex);
@@ -520,6 +546,9 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
error_setg(errp, QERR_UNSUPPORTED);
return;
}
+ if (block_job_apply_verb(job, BLOCK_JOB_VERB_SET_SPEED, errp)) {
+ return;
+ }
job->driver->set_speed(job, speed, &local_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -539,8 +568,10 @@ void block_job_complete(BlockJob *job, Error **errp)
{
/* Should not be reachable via external interface for internal jobs */
assert(job->id);
- if (job->pause_count || job->cancelled ||
- !block_job_started(job) || !job->driver->complete) {
+ if (block_job_apply_verb(job, BLOCK_JOB_VERB_COMPLETE, errp)) {
+ return;
+ }
+ if (job->pause_count || job->cancelled || !job->driver->complete) {
error_setg(errp, "The active block job '%s' cannot be completed",
job->id);
return;
@@ -549,8 +580,15 @@ void block_job_complete(BlockJob *job, Error **errp)
job->driver->complete(job, errp);
}
-void block_job_user_pause(BlockJob *job)
+void block_job_user_pause(BlockJob *job, Error **errp)
{
+ if (block_job_apply_verb(job, BLOCK_JOB_VERB_PAUSE, errp)) {
+ return;
+ }
+ if (job->user_paused) {
+ error_setg(errp, "Job is already paused");
+ return;
+ }
job->user_paused = true;
block_job_pause(job);
}
@@ -560,13 +598,19 @@ bool block_job_user_paused(BlockJob *job)
return job->user_paused;
}
-void block_job_user_resume(BlockJob *job)
+void block_job_user_resume(BlockJob *job, Error **errp)
{
- if (job && job->user_paused && job->pause_count > 0) {
- block_job_iostatus_reset(job);
- job->user_paused = false;
- block_job_resume(job);
+ assert(job);
+ if (!job->user_paused || job->pause_count <= 0) {
+ error_setg(errp, "Can't resume a job that was not paused");
+ return;
}
+ if (block_job_apply_verb(job, BLOCK_JOB_VERB_RESUME, errp)) {
+ return;
+ }
+ block_job_iostatus_reset(job);
+ job->user_paused = false;
+ block_job_resume(job);
}
void block_job_cancel(BlockJob *job)
@@ -579,6 +623,14 @@ void block_job_cancel(BlockJob *job)
}
}
+void block_job_user_cancel(BlockJob *job, Error **errp)
+{
+ if (block_job_apply_verb(job, BLOCK_JOB_VERB_CANCEL, errp)) {
+ return;
+ }
+ block_job_cancel(job);
+}
+
/* A wrapper around block_job_cancel() taking an Error ** parameter so it may be
* used with block_job_finish_sync() without the need for (rather nasty)
* function pointer casts there. */
@@ -1004,8 +1056,9 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
action, &error_abort);
}
if (action == BLOCK_ERROR_ACTION_STOP) {
+ block_job_pause(job);
/* make the pause user visible, which will be resumed from QMP. */
- block_job_user_pause(job);
+ job->user_paused = true;
block_job_iostatus_set_err(job, error);
}
return action;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index e254359d6b..403a168073 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -260,7 +260,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
* Asynchronously pause the specified job.
* Do not allow a resume until a matching call to block_job_user_resume.
*/
-void block_job_user_pause(BlockJob *job);
+void block_job_user_pause(BlockJob *job, Error **errp);
/**
* block_job_paused:
@@ -277,7 +277,16 @@ bool block_job_user_paused(BlockJob *job);
* Resume the specified job.
* Must be paired with a preceding block_job_user_pause.
*/
-void block_job_user_resume(BlockJob *job);
+void block_job_user_resume(BlockJob *job, Error **errp);
+
+/**
+ * block_job_user_cancel:
+ * @job: The job to be cancelled.
+ *
+ * Cancels the specified job, but may refuse to do so if the
+ * operation isn't currently meaningful.
+ */
+void block_job_user_cancel(BlockJob *job, Error **errp);
/**
* block_job_cancel_sync:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index deddee616a..11659496c5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -955,6 +955,26 @@
{ 'enum': 'BlockJobType',
'data': ['commit', 'stream', 'mirror', 'backup'] }
+##
+# @BlockJobVerb:
+#
+# Represents command verbs that can be applied to a blockjob.
+#
+# @cancel: see @block-job-cancel
+#
+# @pause: see @block-job-pause
+#
+# @resume: see @block-job-resume
+#
+# @set-speed: see @block-job-set-speed
+#
+# @complete: see @block-job-complete
+#
+# Since: 2.12
+##
+{ 'enum': 'BlockJobVerb',
+ 'data': ['cancel', 'pause', 'resume', 'set-speed', 'complete' ] }
+
##
# @BlockJobStatus:
#
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index a7eecf1c1e..7673de1062 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -505,6 +505,7 @@ static void coroutine_fn test_job_start(void *opaque)
{
TestBlockJob *s = opaque;
+ block_job_event_ready(&s->common);
while (!s->should_complete) {
block_job_sleep_ns(&s->common, 100000);
}
Which commands ("verbs") are appropriate for jobs in which state is also somewhat burdensome to keep track of. As of this commit, it looks rather useless, but begins to look more interesting the more states we add to the STM table. A recurring theme is that no verb will apply to an 'undefined' job. Further, it's not presently possible to restrict the "pause" or "resume" verbs any more than they are in this commit because of the asynchronous nature of how jobs enter the PAUSED state; justifications for some seemingly erroneous applications are given below.