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