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