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 <address@hidden>
>> ---
>>   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.