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.
Transitions:
Undefined -> Created: During job initialization.
Created -> Running: Once the job is started.
Jobs cannot transition from "Created" to "Paused"
directly, but will instead synchronously transition
to running to paused immediately.
Running -> Paused: Normal workflow for pauses.
Running -> Ready: Normal workflow for jobs reaching their sync point.
(e.g. mirror)
Ready -> Standby: Normal workflow for pausing ready jobs.
Paused -> Running: Normal resume.
Standby -> Ready: Resume of a Standby job.
+---------+
|UNDEFINED|
+--+------+
|
+--v----+
|CREATED|
+--+----+
|
+--v----+ +------+
|RUNNING<----->PAUSED|
+--+----+ +------+
|
+--v--+ +-------+
|READY<------->STANDBY|
+-----+ +-------+
Notably, there is no state presently defined as of this commit that
deals with a job after the "running" or "ready" states, so this table
will be adjusted alongside the commits that introduce those states.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/trace-events | 3 +++
blockjob.c | 42 ++++++++++++++++++++++++++++++++++++------
2 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/block/trace-events b/block/trace-events
index 02dd80ff0c..b75a0c8409 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -4,6 +4,9 @@
bdrv_open_common(void *bs, const char *filename, int flags, const char *format_name) "bs %p filename \"%s\" flags 0x%x format_name \"%s\""
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/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"
blk_co_pwritev(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/blockjob.c b/blockjob.c
index 1be9c20cff..d745b3bb69 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -28,6 +28,7 @@
#include "block/block.h"
#include "block/blockjob_int.h"
#include "block/block_int.h"
+#include "block/trace.h"
#include "sysemu/block-backend.h"
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
@@ -41,6 +42,34 @@
* 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, S */
+ /* 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, 0},
+ /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0},
+ /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1},
+ /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 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);
+ trace_block_job_state_transition(job, job->ret, BlockJobSTT[s0][s1] ?
+ "allowed" : "disallowed",
+ qapi_enum_lookup(&BlockJobStatus_lookup,
+ s0),
+ qapi_enum_lookup(&BlockJobStatus_lookup,
+ s1));
+ assert(BlockJobSTT[s0][s1]);
+ job->status = s1;
+}
+
static void block_job_lock(void)
{
qemu_mutex_lock(&block_job_mutex);
@@ -320,7 +349,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);
}
@@ -704,6 +733,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
job->refcnt = 1;
job->manual = (flags & BLOCK_JOB_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);
@@ -818,13 +848,13 @@ 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 = status == BLOCK_JOB_STATUS_READY ? \
- BLOCK_JOB_STATUS_STANDBY : \
- BLOCK_JOB_STATUS_PAUSED;
+ block_job_state_transition(job, status == BLOCK_JOB_STATUS_READY ? \
+ BLOCK_JOB_STATUS_STANDBY : \
+ 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) {
@@ -930,7 +960,7 @@ void block_job_iostatus_reset(BlockJob *job)
void block_job_event_ready(BlockJob *job)
{
- job->status = BLOCK_JOB_STATUS_READY;
+ block_job_state_transition(job, BLOCK_JOB_STATUS_READY);
job->ready = true;
if (block_job_is_internal(job)) {
--
2.14.3
Am 24.02.2018 um 00:51 hat John Snow geschrieben:
> The state transition table has mostly been implied. We're about to make
> it a bit more complex, so let's make the STM explicit instead.
>
> Perform state transitions with a function that for now just asserts the
> transition is appropriate.
>
> Transitions:
> Undefined -> Created: During job initialization.
> Created -> Running: Once the job is started.
> Jobs cannot transition from "Created" to "Paused"
> directly, but will instead synchronously transition
> to running to paused immediately.
> Running -> Paused: Normal workflow for pauses.
> Running -> Ready: Normal workflow for jobs reaching their sync point.
> (e.g. mirror)
> Ready -> Standby: Normal workflow for pausing ready jobs.
> Paused -> Running: Normal resume.
> Standby -> Ready: Resume of a Standby job.
>
>
> +---------+
> |UNDEFINED|
> +--+------+
> |
> +--v----+
> |CREATED|
> +--+----+
> |
> +--v----+ +------+
> |RUNNING<----->PAUSED|
> +--+----+ +------+
> |
> +--v--+ +-------+
> |READY<------->STANDBY|
> +-----+ +-------+
>
>
> Notably, there is no state presently defined as of this commit that
> deals with a job after the "running" or "ready" states, so this table
> will be adjusted alongside the commits that introduce those states.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/trace-events | 3 +++
> blockjob.c | 42 ++++++++++++++++++++++++++++++++++++------
> 2 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/block/trace-events b/block/trace-events
> index 02dd80ff0c..b75a0c8409 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -4,6 +4,9 @@
> bdrv_open_common(void *bs, const char *filename, int flags, const char *format_name) "bs %p filename \"%s\" flags 0x%x format_name \"%s\""
> 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/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"
> blk_co_pwritev(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/blockjob.c b/blockjob.c
> index 1be9c20cff..d745b3bb69 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -28,6 +28,7 @@
> #include "block/block.h"
> #include "block/blockjob_int.h"
> #include "block/block_int.h"
> +#include "block/trace.h"
> #include "sysemu/block-backend.h"
> #include "qapi/error.h"
> #include "qapi/qmp/qerror.h"
> @@ -41,6 +42,34 @@
> * 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, S */
> + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0},
Even at the end of the series, this is the only use of
BLOCK_JOB_STATUS_UNDEFINED.
> + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0},
> + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0},
> + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0},
> + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1},
> + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 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);
> + trace_block_job_state_transition(job, job->ret, BlockJobSTT[s0][s1] ?
> + "allowed" : "disallowed",
> + qapi_enum_lookup(&BlockJobStatus_lookup,
> + s0),
> + qapi_enum_lookup(&BlockJobStatus_lookup,
> + s1));
> + assert(BlockJobSTT[s0][s1]);
> + job->status = s1;
> +}
> +
> static void block_job_lock(void)
> {
> qemu_mutex_lock(&block_job_mutex);
> @@ -320,7 +349,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);
> }
>
> @@ -704,6 +733,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> job->refcnt = 1;
> job->manual = (flags & BLOCK_JOB_MANUAL);
> job->status = BLOCK_JOB_STATUS_CREATED;
> + block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED);
So did you intend to start with BLOCK_JOB_STATUS_UNDEFINED and then
transition to BLOCK_JOB_STATUS_CREATED?
Or should we completely remove BLOCK_JOB_STATUS_UNDEFINED, keep the
initialisation and not call block_job_state_transition() here?
Kevin
On 02/27/2018 11:27 AM, Kevin Wolf wrote:
> Am 24.02.2018 um 00:51 hat John Snow geschrieben:
>> The state transition table has mostly been implied. We're about to make
>> it a bit more complex, so let's make the STM explicit instead.
>>
>> Perform state transitions with a function that for now just asserts the
>> transition is appropriate.
>>
>> Transitions:
>> Undefined -> Created: During job initialization.
>> Created -> Running: Once the job is started.
>> Jobs cannot transition from "Created" to "Paused"
>> directly, but will instead synchronously transition
>> to running to paused immediately.
>> Running -> Paused: Normal workflow for pauses.
>> Running -> Ready: Normal workflow for jobs reaching their sync point.
>> (e.g. mirror)
>> Ready -> Standby: Normal workflow for pausing ready jobs.
>> Paused -> Running: Normal resume.
>> Standby -> Ready: Resume of a Standby job.
>>
>>
>> +---------+
>> |UNDEFINED|
>> +--+------+
>> |
>> +--v----+
>> |CREATED|
>> +--+----+
>> |
>> +--v----+ +------+
>> |RUNNING<----->PAUSED|
>> +--+----+ +------+
>> |
>> +--v--+ +-------+
>> |READY<------->STANDBY|
>> +-----+ +-------+
>>
>>
>> Notably, there is no state presently defined as of this commit that
>> deals with a job after the "running" or "ready" states, so this table
>> will be adjusted alongside the commits that introduce those states.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/trace-events | 3 +++
>> blockjob.c | 42 ++++++++++++++++++++++++++++++++++++------
>> 2 files changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/trace-events b/block/trace-events
>> index 02dd80ff0c..b75a0c8409 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -4,6 +4,9 @@
>> bdrv_open_common(void *bs, const char *filename, int flags, const char *format_name) "bs %p filename \"%s\" flags 0x%x format_name \"%s\""
>> 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/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"
>> blk_co_pwritev(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/blockjob.c b/blockjob.c
>> index 1be9c20cff..d745b3bb69 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -28,6 +28,7 @@
>> #include "block/block.h"
>> #include "block/blockjob_int.h"
>> #include "block/block_int.h"
>> +#include "block/trace.h"
>> #include "sysemu/block-backend.h"
>> #include "qapi/error.h"
>> #include "qapi/qmp/qerror.h"
>> @@ -41,6 +42,34 @@
>> * 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, S */
>> + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0},
>
> Even at the end of the series, this is the only use of
> BLOCK_JOB_STATUS_UNDEFINED.
>
>> + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0},
>> + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0},
>> + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0},
>> + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1},
>> + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 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);
>> + trace_block_job_state_transition(job, job->ret, BlockJobSTT[s0][s1] ?
>> + "allowed" : "disallowed",
>> + qapi_enum_lookup(&BlockJobStatus_lookup,
>> + s0),
>> + qapi_enum_lookup(&BlockJobStatus_lookup,
>> + s1));
>> + assert(BlockJobSTT[s0][s1]);
>> + job->status = s1;
>> +}
>> +
>> static void block_job_lock(void)
>> {
>> qemu_mutex_lock(&block_job_mutex);
>> @@ -320,7 +349,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);
>> }
>>
>> @@ -704,6 +733,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>> job->refcnt = 1;
>> job->manual = (flags & BLOCK_JOB_MANUAL);
>> job->status = BLOCK_JOB_STATUS_CREATED;
>> + block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED);
>
> So did you intend to start with BLOCK_JOB_STATUS_UNDEFINED and then
> transition to BLOCK_JOB_STATUS_CREATED?
>
> Or should we completely remove BLOCK_JOB_STATUS_UNDEFINED, keep the
> initialisation and not call block_job_state_transition() here?
>
> Kevin
>
We can do that;
I had it start as "Undefined" because I liked how a g_new0() object will
default to that state, so it felt "safe."
On the negatives, it does mean that technically you COULD witness a job
in this state if QEMU did something wrong, which would be confusing
because you wouldn't be able to fix it via QMP.
--js
Am 27.02.2018 um 17:45 hat John Snow geschrieben:
>
>
> On 02/27/2018 11:27 AM, Kevin Wolf wrote:
> > Am 24.02.2018 um 00:51 hat John Snow geschrieben:
> >> The state transition table has mostly been implied. We're about to make
> >> it a bit more complex, so let's make the STM explicit instead.
> >>
> >> Perform state transitions with a function that for now just asserts the
> >> transition is appropriate.
> >>
> >> Transitions:
> >> Undefined -> Created: During job initialization.
> >> Created -> Running: Once the job is started.
> >> Jobs cannot transition from "Created" to "Paused"
> >> directly, but will instead synchronously transition
> >> to running to paused immediately.
> >> Running -> Paused: Normal workflow for pauses.
> >> Running -> Ready: Normal workflow for jobs reaching their sync point.
> >> (e.g. mirror)
> >> Ready -> Standby: Normal workflow for pausing ready jobs.
> >> Paused -> Running: Normal resume.
> >> Standby -> Ready: Resume of a Standby job.
> >>
> >>
> >> +---------+
> >> |UNDEFINED|
> >> +--+------+
> >> |
> >> +--v----+
> >> |CREATED|
> >> +--+----+
> >> |
> >> +--v----+ +------+
> >> |RUNNING<----->PAUSED|
> >> +--+----+ +------+
> >> |
> >> +--v--+ +-------+
> >> |READY<------->STANDBY|
> >> +-----+ +-------+
> >>
> >>
> >> Notably, there is no state presently defined as of this commit that
> >> deals with a job after the "running" or "ready" states, so this table
> >> will be adjusted alongside the commits that introduce those states.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >> block/trace-events | 3 +++
> >> blockjob.c | 42 ++++++++++++++++++++++++++++++++++++------
> >> 2 files changed, 39 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/block/trace-events b/block/trace-events
> >> index 02dd80ff0c..b75a0c8409 100644
> >> --- a/block/trace-events
> >> +++ b/block/trace-events
> >> @@ -4,6 +4,9 @@
> >> bdrv_open_common(void *bs, const char *filename, int flags, const char *format_name) "bs %p filename \"%s\" flags 0x%x format_name \"%s\""
> >> 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/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"
> >> blk_co_pwritev(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/blockjob.c b/blockjob.c
> >> index 1be9c20cff..d745b3bb69 100644
> >> --- a/blockjob.c
> >> +++ b/blockjob.c
> >> @@ -28,6 +28,7 @@
> >> #include "block/block.h"
> >> #include "block/blockjob_int.h"
> >> #include "block/block_int.h"
> >> +#include "block/trace.h"
> >> #include "sysemu/block-backend.h"
> >> #include "qapi/error.h"
> >> #include "qapi/qmp/qerror.h"
> >> @@ -41,6 +42,34 @@
> >> * 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, S */
> >> + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0},
> >
> > Even at the end of the series, this is the only use of
> > BLOCK_JOB_STATUS_UNDEFINED.
> >
> >> + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0},
> >> + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0},
> >> + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0},
> >> + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1},
> >> + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 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);
> >> + trace_block_job_state_transition(job, job->ret, BlockJobSTT[s0][s1] ?
> >> + "allowed" : "disallowed",
> >> + qapi_enum_lookup(&BlockJobStatus_lookup,
> >> + s0),
> >> + qapi_enum_lookup(&BlockJobStatus_lookup,
> >> + s1));
> >> + assert(BlockJobSTT[s0][s1]);
> >> + job->status = s1;
> >> +}
> >> +
> >> static void block_job_lock(void)
> >> {
> >> qemu_mutex_lock(&block_job_mutex);
> >> @@ -320,7 +349,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);
> >> }
> >>
> >> @@ -704,6 +733,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> >> job->refcnt = 1;
> >> job->manual = (flags & BLOCK_JOB_MANUAL);
> >> job->status = BLOCK_JOB_STATUS_CREATED;
> >> + block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED);
> >
> > So did you intend to start with BLOCK_JOB_STATUS_UNDEFINED and then
> > transition to BLOCK_JOB_STATUS_CREATED?
> >
> > Or should we completely remove BLOCK_JOB_STATUS_UNDEFINED, keep the
> > initialisation and not call block_job_state_transition() here?
> >
> > Kevin
> >
>
> We can do that;
>
> I had it start as "Undefined" because I liked how a g_new0() object will
> default to that state, so it felt "safe."
>
> On the negatives, it does mean that technically you COULD witness a job
> in this state if QEMU did something wrong, which would be confusing
> because you wouldn't be able to fix it via QMP.
I don't really mind which way you do it as long as the code seems
self-consistent. You could change the initialisation this way:
job->status = BLOCK_JOB_STATUS_UNDEFINED;
block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED);
Or if you want to make use of the fact that g_new0() already results in
BLOCK_JOB_STATUS_UNDEFINED, you can omit the first line.
I'm also not strictly opposed to a CREATED -> CREATED transition, even
though it looks a bit odd. But then there is no reason to allow an
UNDEFINED -> CREATED transition that never happens in practice.
UNDEFINED would then be a completely unused state that could only be
active in the case of a bug.
Kevin
On 02/27/2018 12:08 PM, Kevin Wolf wrote:
> Am 27.02.2018 um 17:45 hat John Snow geschrieben:
>>
>>
>> On 02/27/2018 11:27 AM, Kevin Wolf wrote:
>>> Am 24.02.2018 um 00:51 hat John Snow geschrieben:
>>>> The state transition table has mostly been implied. We're about to make
>>>> it a bit more complex, so let's make the STM explicit instead.
>>>>
>>>> Perform state transitions with a function that for now just asserts the
>>>> transition is appropriate.
>>>>
>>>> Transitions:
>>>> Undefined -> Created: During job initialization.
>>>> Created -> Running: Once the job is started.
>>>> Jobs cannot transition from "Created" to "Paused"
>>>> directly, but will instead synchronously transition
>>>> to running to paused immediately.
>>>> Running -> Paused: Normal workflow for pauses.
>>>> Running -> Ready: Normal workflow for jobs reaching their sync point.
>>>> (e.g. mirror)
>>>> Ready -> Standby: Normal workflow for pausing ready jobs.
>>>> Paused -> Running: Normal resume.
>>>> Standby -> Ready: Resume of a Standby job.
>>>>
>>>>
>>>> +---------+
>>>> |UNDEFINED|
>>>> +--+------+
>>>> |
>>>> +--v----+
>>>> |CREATED|
>>>> +--+----+
>>>> |
>>>> +--v----+ +------+
>>>> |RUNNING<----->PAUSED|
>>>> +--+----+ +------+
>>>> |
>>>> +--v--+ +-------+
>>>> |READY<------->STANDBY|
>>>> +-----+ +-------+
>>>>
>>>>
>>>> Notably, there is no state presently defined as of this commit that
>>>> deals with a job after the "running" or "ready" states, so this table
>>>> will be adjusted alongside the commits that introduce those states.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> block/trace-events | 3 +++
>>>> blockjob.c | 42 ++++++++++++++++++++++++++++++++++++------
>>>> 2 files changed, 39 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/block/trace-events b/block/trace-events
>>>> index 02dd80ff0c..b75a0c8409 100644
>>>> --- a/block/trace-events
>>>> +++ b/block/trace-events
>>>> @@ -4,6 +4,9 @@
>>>> bdrv_open_common(void *bs, const char *filename, int flags, const char *format_name) "bs %p filename \"%s\" flags 0x%x format_name \"%s\""
>>>> 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/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"
>>>> blk_co_pwritev(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/blockjob.c b/blockjob.c
>>>> index 1be9c20cff..d745b3bb69 100644
>>>> --- a/blockjob.c
>>>> +++ b/blockjob.c
>>>> @@ -28,6 +28,7 @@
>>>> #include "block/block.h"
>>>> #include "block/blockjob_int.h"
>>>> #include "block/block_int.h"
>>>> +#include "block/trace.h"
>>>> #include "sysemu/block-backend.h"
>>>> #include "qapi/error.h"
>>>> #include "qapi/qmp/qerror.h"
>>>> @@ -41,6 +42,34 @@
>>>> * 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, S */
>>>> + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0},
>>>
>>> Even at the end of the series, this is the only use of
>>> BLOCK_JOB_STATUS_UNDEFINED.
>>>
>>>> + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0},
>>>> + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0},
>>>> + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0},
>>>> + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1},
>>>> + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 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);
>>>> + trace_block_job_state_transition(job, job->ret, BlockJobSTT[s0][s1] ?
>>>> + "allowed" : "disallowed",
>>>> + qapi_enum_lookup(&BlockJobStatus_lookup,
>>>> + s0),
>>>> + qapi_enum_lookup(&BlockJobStatus_lookup,
>>>> + s1));
>>>> + assert(BlockJobSTT[s0][s1]);
>>>> + job->status = s1;
>>>> +}
>>>> +
>>>> static void block_job_lock(void)
>>>> {
>>>> qemu_mutex_lock(&block_job_mutex);
>>>> @@ -320,7 +349,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);
>>>> }
>>>>
>>>> @@ -704,6 +733,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>>>> job->refcnt = 1;
>>>> job->manual = (flags & BLOCK_JOB_MANUAL);
>>>> job->status = BLOCK_JOB_STATUS_CREATED;
>>>> + block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED);
>>>
>>> So did you intend to start with BLOCK_JOB_STATUS_UNDEFINED and then
>>> transition to BLOCK_JOB_STATUS_CREATED?
>>>
Oh, crud, yes. This is a refactoring issue that snuck in. I didn't even
realize after your first mail because I believed so strongly that I had
not done it this way.
>>> Or should we completely remove BLOCK_JOB_STATUS_UNDEFINED, keep the
>>> initialisation and not call block_job_state_transition() here?
>>>
>>> Kevin
>>>
>>
>> We can do that;
>>
>> I had it start as "Undefined" because I liked how a g_new0() object will
>> default to that state, so it felt "safe."
>>
>> On the negatives, it does mean that technically you COULD witness a job
>> in this state if QEMU did something wrong, which would be confusing
>> because you wouldn't be able to fix it via QMP.
>
> I don't really mind which way you do it as long as the code seems
> self-consistent. You could change the initialisation this way:
>
> job->status = BLOCK_JOB_STATUS_UNDEFINED;
> block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED);
>
> Or if you want to make use of the fact that g_new0() already results in
> BLOCK_JOB_STATUS_UNDEFINED, you can omit the first line.
>
> I'm also not strictly opposed to a CREATED -> CREATED transition, even
> though it looks a bit odd. But then there is no reason to allow an
> UNDEFINED -> CREATED transition that never happens in practice.
> UNDEFINED would then be a completely unused state that could only be
> active in the case of a bug.
>
> Kevin
>
On 02/23/2018 05:51 PM, John Snow wrote:
> 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.
>
> Transitions:
> Undefined -> Created: During job initialization.
Unless we use Created as the default state 0 for g_new0().
> Created -> Running: Once the job is started.
> Jobs cannot transition from "Created" to "Paused"
> directly, but will instead synchronously transition
> to running to paused immediately.
> Running -> Paused: Normal workflow for pauses.
> Running -> Ready: Normal workflow for jobs reaching their sync point.
> (e.g. mirror)
> Ready -> Standby: Normal workflow for pausing ready jobs.
> Paused -> Running: Normal resume.
> Standby -> Ready: Resume of a Standby job.
>
>
> +---------+
> |UNDEFINED|
> +--+------+
> |
> +--v----+
> |CREATED|
> +--+----+
> |
> +--v----+ +------+
> |RUNNING<----->PAUSED|
> +--+----+ +------+
> |
> +--v--+ +-------+
> |READY<------->STANDBY|
> +-----+ +-------+
>
>
> Notably, there is no state presently defined as of this commit that
> deals with a job after the "running" or "ready" states, so this table
> will be adjusted alongside the commits that introduce those states.
The ascii-art tables help in this and other patches. Thanks for
producing them.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/trace-events | 3 +++
> blockjob.c | 42 ++++++++++++++++++++++++++++++++++++------
> 2 files changed, 39 insertions(+), 6 deletions(-)
>
> +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);
Or, if you keep the zero state distinct from valid states, this could be
'assert(s1 > 0 && ...)'
> @@ -320,7 +349,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);
> }
>
> @@ -704,6 +733,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> job->refcnt = 1;
> job->manual = (flags & BLOCK_JOB_MANUAL);
> job->status = BLOCK_JOB_STATUS_CREATED;
> + block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED);
Oops, missing a deletion of the job->status assignment line.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 02/27/2018 01:58 PM, Eric Blake wrote: > The ascii-art tables help in this and other patches. Thanks for > producing them. Funny story: the original version of the STM present in these patches was too complex and I was not able to draw a chart for them. Kevin's insistence that I do draw a chart led to considerable slimming of possible transitions. --js
On 02/27/2018 01:58 PM, Eric Blake wrote:
> On 02/23/2018 05:51 PM, John Snow wrote:
>> 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.
>>
>> Transitions:
>> Undefined -> Created: During job initialization.
>
> Unless we use Created as the default state 0 for g_new0().
>
I liked the idea of letting jobs be created in an "indeterminate" state
until we actually initialize them to be of use -- that is, jobs that
could be said to semantically understand and act on the "START" verb
(which is, as of today, an internal command only.)
The only meaningful action on a job of indeterminate state, then, would
be to DEFINE that job. (I.e. what block_job_create does.)
What I'm getting at is that block_job_start() on a job that was just
created will explode, and I'd like "created" to mean "This job can be
started."
It's not a distinction that matters in the codebase RIGHT NOW, but
that's how I came to think of the STM. We could likely optimize that
transition out because we always create and immediately define it, but
it felt ... nicer from an (internal) API point of view to have defined
the construction and destruction transitions explicitly.
Anyway, it can be removed; it's not a hill worth dying on. I only insist
that the bike shed not be olive green.
>> Created -> Running: Once the job is started.
>> Jobs cannot transition from "Created" to "Paused"
>> directly, but will instead synchronously
>> transition
>> to running to paused immediately.
>> Running -> Paused: Normal workflow for pauses.
>> Running -> Ready: Normal workflow for jobs reaching their sync point.
>> (e.g. mirror)
>> Ready -> Standby: Normal workflow for pausing ready jobs.
>> Paused -> Running: Normal resume.
>> Standby -> Ready: Resume of a Standby job.
>>
>>
>> +---------+
>> |UNDEFINED|
>> +--+------+
>> |
>> +--v----+
>> |CREATED|
>> +--+----+
>> |
>> +--v----+ +------+
>> |RUNNING<----->PAUSED|
>> +--+----+ +------+
>> |
>> +--v--+ +-------+
>> |READY<------->STANDBY|
>> +-----+ +-------+
>>
>>
>> Notably, there is no state presently defined as of this commit that
>> deals with a job after the "running" or "ready" states, so this table
>> will be adjusted alongside the commits that introduce those states.
>
> The ascii-art tables help in this and other patches. Thanks for
> producing them.
>
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/trace-events | 3 +++
>> blockjob.c | 42 ++++++++++++++++++++++++++++++++++++------
>> 2 files changed, 39 insertions(+), 6 deletions(-)
>>
>
>> +static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
>> +{
>> + BlockJobStatus s0 = job->status;
>> + if (s0 == s1) {
>> + return;
>> + }
If I remove this clause, I actually would have noticed that technically
I attempt to transition from CREATED to CREATED. Maybe I ought to remove
this...
>> + assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX);
>
> Or, if you keep the zero state distinct from valid states, this could be
> 'assert(s1 > 0 && ...)'
>
>> @@ -320,7 +349,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);
>> }
>> @@ -704,6 +733,7 @@ void *block_job_create(const char *job_id, const
>> BlockJobDriver *driver,
>> job->refcnt = 1;
>> job->manual = (flags & BLOCK_JOB_MANUAL);
>> job->status = BLOCK_JOB_STATUS_CREATED;
>> + block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED);
>
> Oops, missing a deletion of the job->status assignment line.
>
I am indeed.
© 2016 - 2025 Red Hat, Inc.