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