Re: [Qemu-devel] [RFC v4 05/21] blockjobs: add state transition table

[John Snow]

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 
>> ---
>>   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.

Re: [Qemu-devel] [RFC v4 05/21] blockjobs: add state transition table

[John Snow]

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

Re: [Qemu-devel] [RFC v4 05/21] blockjobs: add state transition table

[John Snow]

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 
 ---
  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(_lookup,
 +  s0),
 + 
 qapi_enum_lookup(_lookup,
 +  s1));
 +assert(BlockJobSTT[s0][s1]);
 +job->status = s1;
 +}
 +
  static void block_job_lock(void)
  {
  qemu_mutex_lock(_job_mutex);
 @@ -320,7 +349,7 @@ void block_job_start(BlockJob *job)
  job->pause_count--;
  

Re: [Qemu-devel] [RFC v4 05/21] blockjobs: add state transition table

[Eric Blake]

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 
---
  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

Re: [Qemu-devel] [RFC v4 05/21] blockjobs: add state transition table

[Kevin Wolf]

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 
> >> ---
> >>  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(_lookup,
> >> +  s0),
> >> + 
> >> qapi_enum_lookup(_lookup,
> >> +  s1));
> >> +assert(BlockJobSTT[s0][s1]);
> >> +job->status = s1;
> >> +}
> >> +
> >>  static void block_job_lock(void)
> >>  {
> >>  qemu_mutex_lock(_job_mutex);
> >> @@ -320,7 +349,7 @@ void block_job_start(BlockJob *job)
> >>  job->pause_count--;
> >>  job->busy = true;
> >>  job->paused = false;
> 

Re: [Qemu-devel] [RFC v4 05/21] blockjobs: add state transition table

[John Snow]

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 
>> ---
>>  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(_lookup,
>> +  s0),
>> + 
>> qapi_enum_lookup(_lookup,
>> +  s1));
>> +assert(BlockJobSTT[s0][s1]);
>> +job->status = s1;
>> +}
>> +
>>  static void block_job_lock(void)
>>  {
>>  qemu_mutex_lock(_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

Re: [Qemu-devel] [RFC v4 05/21] blockjobs: add state transition table

[Kevin Wolf]

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 
> ---
>  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(_lookup,
> +  s0),
> + qapi_enum_lookup(_lookup,
> +  s1));
> +assert(BlockJobSTT[s0][s1]);
> +job->status = s1;
> +}
> +
>  static void block_job_lock(void)
>  {
>  qemu_mutex_lock(_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, 

[Qemu-devel] [RFC v4 05/21] blockjobs: add state transition table

[John Snow]

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 
---
 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(_lookup,
+  s0),
+ qapi_enum_lookup(_lookup,
+  s1));
+assert(BlockJobSTT[s0][s1]);
+job->status = s1;
+}
+
 static void block_job_lock(void)
 {
 qemu_mutex_lock(_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(), >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 =