| Message ID | 20180127020515.27137-4-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | blockjobs: add explicit job management | expand |
Am 27.01.2018 um 03:05 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. > > undefined: May only transition to 'created'. > created: May only transition to 'running'. > It cannot transition to pause directly, but if a created job > is requested to pause prior to starting, it will transition > synchronously to 'running' and then to 'paused'. > running: May transition either to 'paused' or 'ready'. > paused: May transition to either 'running' or 'ready', but only in > terms of returning to that prior state. > p->r->y is not possible, but this is not encapsulated by the > STM table. Do you mean y->p->r->y here? If not, I don't understand. > ready: May transition to paused. > Signed-off-by: John Snow <jsnow@redhat.com> > --- > blockjob.c | 39 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 34 insertions(+), 5 deletions(-) > > diff --git a/blockjob.c b/blockjob.c > index 6eb783a354..d084a1e318 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -42,6 +42,35 @@ > * 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 */ > + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0}, > + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0}, > + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1}, > + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1}, > + /* Y: */ [BLOCK_JOB_STATUS_READY] = {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); > + assert(BlockJobSTT[s0][s1]); > + switch (s1) { > + case BLOCK_JOB_STATUS_WAITING: > + case BLOCK_JOB_STATUS_PENDING: > + case BLOCK_JOB_STATUS_CONCLUDED: > + assert(job->manual); > + default: > + break; > + } This doesn't compile because the constants don't exist yet. Apart from that, I think the assertion is misguided, too. Which states a job goes through shouldn't depend on whether the client wants to complete the job manually or have it completed automatically. The difference should only be which state transitions are automatic. In other words, WAITING/PENDING/CONCLUDED may never be visible in query-block-job for automatically completed jobs, but the jobs should still (synchronously) go through all of these states. > + job->status = s1; > +} > + Kevin
On 01/29/2018 12:17 PM, Kevin Wolf wrote: > Am 27.01.2018 um 03:05 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. >> >> undefined: May only transition to 'created'. >> created: May only transition to 'running'. >> It cannot transition to pause directly, but if a created job >> is requested to pause prior to starting, it will transition >> synchronously to 'running' and then to 'paused'. >> running: May transition either to 'paused' or 'ready'. >> paused: May transition to either 'running' or 'ready', but only in >> terms of returning to that prior state. >> p->r->y is not possible, but this is not encapsulated by the >> STM table. > > Do you mean y->p->r->y here? If not, I don't understand. Whoops, Yes, I mean to say that Y->P->R is not possible. That is, a paused state can only return to where it came from, but the STM doesn't show this restriction. Really, this hints at there being *two* paused states, but I didn't want to go through the trouble of adding a new one. > >> ready: May transition to paused. I swear my script used to add blank lines for me here. *shrug* >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> blockjob.c | 39 ++++++++++++++++++++++++++++++++++----- >> 1 file changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/blockjob.c b/blockjob.c >> index 6eb783a354..d084a1e318 100644 >> --- a/blockjob.c >> +++ b/blockjob.c >> @@ -42,6 +42,35 @@ >> * 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 */ >> + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0}, >> + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0}, >> + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1}, >> + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1}, >> + /* Y: */ [BLOCK_JOB_STATUS_READY] = {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); >> + assert(BlockJobSTT[s0][s1]); >> + switch (s1) { >> + case BLOCK_JOB_STATUS_WAITING: >> + case BLOCK_JOB_STATUS_PENDING: >> + case BLOCK_JOB_STATUS_CONCLUDED: >> + assert(job->manual); >> + default: >> + break; >> + } > > This doesn't compile because the constants don't exist yet. > *cough* oops. > Apart from that, I think the assertion is misguided, too. Which states a > job goes through shouldn't depend on whether the client wants to > complete the job manually or have it completed automatically. The > difference should only be which state transitions are automatic. > > In other words, WAITING/PENDING/CONCLUDED may never be visible in > query-block-job for automatically completed jobs, but the jobs should > still (synchronously) go through all of these states. > Hmm. OK, I can look at doing it in this way. I will probably also have it omit the events in this case too, though. >> + job->status = s1; >> +} >> + > > Kevin > Thanks!
Am 29.01.2018 um 20:07 hat John Snow geschrieben: > > > On 01/29/2018 12:17 PM, Kevin Wolf wrote: > > Am 27.01.2018 um 03:05 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. > >> > >> undefined: May only transition to 'created'. > >> created: May only transition to 'running'. > >> It cannot transition to pause directly, but if a created job > >> is requested to pause prior to starting, it will transition > >> synchronously to 'running' and then to 'paused'. > >> running: May transition either to 'paused' or 'ready'. > >> paused: May transition to either 'running' or 'ready', but only in > >> terms of returning to that prior state. > >> p->r->y is not possible, but this is not encapsulated by the > >> STM table. > > > > Do you mean y->p->r->y here? If not, I don't understand. > > Whoops, Yes, I mean to say that Y->P->R is not possible. > > That is, a paused state can only return to where it came from, but the > STM doesn't show this restriction. Really, this hints at there being > *two* paused states, but I didn't want to go through the trouble of > adding a new one. Hm, yes... It would probably be cleaner to have two states. If a client queries the state and just sees PAUSED, it doesn't know whether the bulk phase has already finished. > > > >> ready: May transition to paused. > > I swear my script used to add blank lines for me here. *shrug* > > >> Signed-off-by: John Snow <jsnow@redhat.com> > >> --- > >> blockjob.c | 39 ++++++++++++++++++++++++++++++++++----- > >> 1 file changed, 34 insertions(+), 5 deletions(-) > >> > >> diff --git a/blockjob.c b/blockjob.c > >> index 6eb783a354..d084a1e318 100644 > >> --- a/blockjob.c > >> +++ b/blockjob.c > >> @@ -42,6 +42,35 @@ > >> * 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 */ > >> + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0}, > >> + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0}, > >> + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1}, > >> + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1}, > >> + /* Y: */ [BLOCK_JOB_STATUS_READY] = {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); > >> + assert(BlockJobSTT[s0][s1]); > >> + switch (s1) { > >> + case BLOCK_JOB_STATUS_WAITING: > >> + case BLOCK_JOB_STATUS_PENDING: > >> + case BLOCK_JOB_STATUS_CONCLUDED: > >> + assert(job->manual); > >> + default: > >> + break; > >> + } > > > > This doesn't compile because the constants don't exist yet. > > > > *cough* oops. > > > Apart from that, I think the assertion is misguided, too. Which states a > > job goes through shouldn't depend on whether the client wants to > > complete the job manually or have it completed automatically. The > > difference should only be which state transitions are automatic. > > > In other words, WAITING/PENDING/CONCLUDED may never be visible in > > query-block-job for automatically completed jobs, but the jobs should > > still (synchronously) go through all of these states. > > > > Hmm. OK, I can look at doing it in this way. I will probably also have > it omit the events in this case too, though. Actually, while there is probably no use for the events, I don't think they would hurt if omitting them isn't completely trivial. Clients always need to be prepared to see new unknown events. Kevin
diff --git a/blockjob.c b/blockjob.c index 6eb783a354..d084a1e318 100644 --- a/blockjob.c +++ b/blockjob.c @@ -42,6 +42,35 @@ * 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 */ + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0}, + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0}, + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1}, + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 1}, + /* Y: */ [BLOCK_JOB_STATUS_READY] = {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); + assert(BlockJobSTT[s0][s1]); + switch (s1) { + case BLOCK_JOB_STATUS_WAITING: + case BLOCK_JOB_STATUS_PENDING: + case BLOCK_JOB_STATUS_CONCLUDED: + assert(job->manual); + default: + break; + } + job->status = s1; +} + static void block_job_lock(void) { qemu_mutex_lock(&block_job_mutex); @@ -321,7 +350,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); } @@ -709,7 +738,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, job->pause_count = 1; job->refcnt = 1; job->manual = manual; - job->status = BLOCK_JOB_STATUS_CREATED; + block_job_state_transition(job, BLOCK_JOB_STATUS_CREATED); aio_timer_init(qemu_get_aio_context(), &job->sleep_timer, QEMU_CLOCK_REALTIME, SCALE_NS, block_job_sleep_timer_cb, job); @@ -815,11 +844,11 @@ void coroutine_fn block_job_pause_point(BlockJob *job) if (block_job_should_pause(job) && !block_job_is_cancelled(job)) { BlockJobStatus status = job->status; - job->status = BLOCK_JOB_STATUS_PAUSED; + block_job_state_transition(job, BLOCK_JOB_STATUS_PAUSED); job->paused = true; block_job_do_yield(job, -1); job->paused = false; - job->status = status; + block_job_state_transition(job, status); } if (job->driver->resume) { @@ -925,7 +954,7 @@ void block_job_iostatus_reset(BlockJob *job) void block_job_event_ready(BlockJob *job) { - job->status = BLOCK_JOB_STATUS_READY; + block_job_state_transition(job, BLOCK_JOB_STATUS_READY); job->ready = true; if (block_job_is_internal(job)) {
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. undefined: May only transition to 'created'. created: May only transition to 'running'. It cannot transition to pause directly, but if a created job is requested to pause prior to starting, it will transition synchronously to 'running' and then to 'paused'. running: May transition either to 'paused' or 'ready'. paused: May transition to either 'running' or 'ready', but only in terms of returning to that prior state. p->r->y is not possible, but this is not encapsulated by the STM table. ready: May transition to paused. Signed-off-by: John Snow <jsnow@redhat.com> --- blockjob.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)