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

[John Snow <jsnow@redhat.com>]

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