| Message ID | 20180310082746.24198-11-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | blockjobs: add explicit job management | expand |
Am 10.03.2018 um 09:27 hat John Snow geschrieben: > Add a new state that specifically demarcates when we begin to permanently > demolish a job after it has performed all work. This makes the transition > explicit in the STM table and highlights conditions under which a job may > be demolished. > > Alongside this state, add a new helper command "block_job_decommission", > which transitions to the NULL state and puts down our implicit reference. > This separates instances in the code for "block_job_unref" which merely > undo a matching "block_job_ref" with instances intended to initiate the > full destruction of the object. > > This decommission action also sets a number of fields to make sure that > block internals or external users that are holding a reference to a job > to see when it "finishes" are convinced that the job object is "done." > This is necessary, for instance, to do a block_job_cancel_sync on a > created object which will not make any progress. > > Now, all jobs must go through block_job_decommission prior to being > freed, giving us start-to-finish state machine coverage for jobs. > > > Transitions: > Created -> Null: Early failure event before the job is started > Concluded -> Null: Standard transition. > > Verbs: > None. This should not ever be visible to the monitor. > > +---------+ > |UNDEFINED| > +--+------+ > | > +--v----+ > +---------+CREATED+------------------+ > | +--+----+ | > | | | > | +--v----+ +------+ | > +---------+RUNNING<----->PAUSED| | > | +--+-+--+ +------+ | > | | | | > | | +------------------+ | > | | | | > | +--v--+ +-------+ | | > +---------+READY<------->STANDBY| | | > | +--+--+ +-------+ | | > | | | | > +--v-----+ +--v------+ | | > |ABORTING+--->CONCLUDED<-------------+ | > +--------+ +--+------+ | > | | > +--v-+ | > |NULL<---------------------+ > +----+ > > Signed-off-by: John Snow <jsnow@redhat.com> > +static void block_job_decommission(BlockJob *job) > +{ > + assert(job); > + job->completed = true; > + job->busy = false; > + job->paused = false; > + job->deferred_to_main_loop = true; Why do we set all of these fields now? I don't see the use of it, and overwriting fields here potentially makes debugging harder. Especially for deferred_to_main_loop I might expect an assert() that it already is true, but shouldn't setting it always be done while actually deferring to the main loop? Can we turn all of these assignments into asserts or are there some that actually aren't already guaranteed, but that we want anyway? > + block_job_state_transition(job, BLOCK_JOB_STATUS_NULL); > + block_job_unref(job); > +} Kevin
On 03/12/2018 11:28 AM, Kevin Wolf wrote: > Am 10.03.2018 um 09:27 hat John Snow geschrieben: >> Add a new state that specifically demarcates when we begin to permanently >> demolish a job after it has performed all work. This makes the transition >> explicit in the STM table and highlights conditions under which a job may >> be demolished. >> >> Alongside this state, add a new helper command "block_job_decommission", >> which transitions to the NULL state and puts down our implicit reference. >> This separates instances in the code for "block_job_unref" which merely >> undo a matching "block_job_ref" with instances intended to initiate the >> full destruction of the object. >> >> This decommission action also sets a number of fields to make sure that >> block internals or external users that are holding a reference to a job >> to see when it "finishes" are convinced that the job object is "done." >> This is necessary, for instance, to do a block_job_cancel_sync on a >> created object which will not make any progress. >> >> Now, all jobs must go through block_job_decommission prior to being >> freed, giving us start-to-finish state machine coverage for jobs. >> >> >> Transitions: >> Created -> Null: Early failure event before the job is started >> Concluded -> Null: Standard transition. >> >> Verbs: >> None. This should not ever be visible to the monitor. >> >> +---------+ >> |UNDEFINED| >> +--+------+ >> | >> +--v----+ >> +---------+CREATED+------------------+ >> | +--+----+ | >> | | | >> | +--v----+ +------+ | >> +---------+RUNNING<----->PAUSED| | >> | +--+-+--+ +------+ | >> | | | | >> | | +------------------+ | >> | | | | >> | +--v--+ +-------+ | | >> +---------+READY<------->STANDBY| | | >> | +--+--+ +-------+ | | >> | | | | >> +--v-----+ +--v------+ | | >> |ABORTING+--->CONCLUDED<-------------+ | >> +--------+ +--+------+ | >> | | >> +--v-+ | >> |NULL<---------------------+ >> +----+ >> >> Signed-off-by: John Snow <jsnow@redhat.com> > >> +static void block_job_decommission(BlockJob *job) >> +{ >> + assert(job); >> + job->completed = true; >> + job->busy = false; >> + job->paused = false; >> + job->deferred_to_main_loop = true; > > Why do we set all of these fields now? I don't see the use of it, and > overwriting fields here potentially makes debugging harder. > > Especially for deferred_to_main_loop I might expect an assert() that it > already is true, but shouldn't setting it always be done while actually > deferring to the main loop? > > Can we turn all of these assignments into asserts or are there some that > actually aren't already guaranteed, but that we want anyway? > >> + block_job_state_transition(job, BLOCK_JOB_STATUS_NULL); >> + block_job_unref(job); >> +} > > Kevin > Gonna be real honest; we probably only need to set maybe one field (job->completed = true) but it was late and I started hitting things with big hammers. The problem is that if jobs do not look "done" to functions like finish_sync, they will loop forever trying to make progress on a job that doesn't do anything. I set a bunch of fields here more as a semantic statement than a necessity, to be really really honest. ("Well, the job definitely has these properties if it made it here, so let's update these fields to be correct and the rest of the code will hopefully Do The Right Thing.")
Am 12.03.2018 um 16:41 hat John Snow geschrieben: > On 03/12/2018 11:28 AM, Kevin Wolf wrote: > > Am 10.03.2018 um 09:27 hat John Snow geschrieben: > >> Add a new state that specifically demarcates when we begin to permanently > >> demolish a job after it has performed all work. This makes the transition > >> explicit in the STM table and highlights conditions under which a job may > >> be demolished. > >> > >> Alongside this state, add a new helper command "block_job_decommission", > >> which transitions to the NULL state and puts down our implicit reference. > >> This separates instances in the code for "block_job_unref" which merely > >> undo a matching "block_job_ref" with instances intended to initiate the > >> full destruction of the object. > >> > >> This decommission action also sets a number of fields to make sure that > >> block internals or external users that are holding a reference to a job > >> to see when it "finishes" are convinced that the job object is "done." > >> This is necessary, for instance, to do a block_job_cancel_sync on a > >> created object which will not make any progress. > >> > >> Now, all jobs must go through block_job_decommission prior to being > >> freed, giving us start-to-finish state machine coverage for jobs. > >> > >> > >> Transitions: > >> Created -> Null: Early failure event before the job is started > >> Concluded -> Null: Standard transition. > >> > >> Verbs: > >> None. This should not ever be visible to the monitor. > >> > >> +---------+ > >> |UNDEFINED| > >> +--+------+ > >> | > >> +--v----+ > >> +---------+CREATED+------------------+ > >> | +--+----+ | > >> | | | > >> | +--v----+ +------+ | > >> +---------+RUNNING<----->PAUSED| | > >> | +--+-+--+ +------+ | > >> | | | | > >> | | +------------------+ | > >> | | | | > >> | +--v--+ +-------+ | | > >> +---------+READY<------->STANDBY| | | > >> | +--+--+ +-------+ | | > >> | | | | > >> +--v-----+ +--v------+ | | > >> |ABORTING+--->CONCLUDED<-------------+ | > >> +--------+ +--+------+ | > >> | | > >> +--v-+ | > >> |NULL<---------------------+ > >> +----+ > >> > >> Signed-off-by: John Snow <jsnow@redhat.com> > > > >> +static void block_job_decommission(BlockJob *job) > >> +{ > >> + assert(job); > >> + job->completed = true; > >> + job->busy = false; > >> + job->paused = false; > >> + job->deferred_to_main_loop = true; > > > > Why do we set all of these fields now? I don't see the use of it, and > > overwriting fields here potentially makes debugging harder. > > > > Especially for deferred_to_main_loop I might expect an assert() that it > > already is true, but shouldn't setting it always be done while actually > > deferring to the main loop? > > > > Can we turn all of these assignments into asserts or are there some that > > actually aren't already guaranteed, but that we want anyway? > > > >> + block_job_state_transition(job, BLOCK_JOB_STATUS_NULL); > >> + block_job_unref(job); > >> +} > > > > Kevin > > > > Gonna be real honest; we probably only need to set maybe one field > (job->completed = true) but it was late and I started hitting things > with big hammers. > > The problem is that if jobs do not look "done" to functions like > finish_sync, they will loop forever trying to make progress on a job > that doesn't do anything. > > I set a bunch of fields here more as a semantic statement than a > necessity, to be really really honest. ("Well, the job definitely has > these properties if it made it here, so let's update these fields to be > correct and the rest of the code will hopefully Do The Right Thing.") So essentially, we want this to be assert(), but currently that breaks for some reasons and we can't figure out why before the freeze? I guess that's fair enough, but then it would be good to use the freeze period to find the offenders and actually turn it into assertions. Kevin
On 03/12/2018 12:07 PM, Kevin Wolf wrote: > Am 12.03.2018 um 16:41 hat John Snow geschrieben: >> On 03/12/2018 11:28 AM, Kevin Wolf wrote: >>> Am 10.03.2018 um 09:27 hat John Snow geschrieben: >>>> Add a new state that specifically demarcates when we begin to permanently >>>> demolish a job after it has performed all work. This makes the transition >>>> explicit in the STM table and highlights conditions under which a job may >>>> be demolished. >>>> >>>> Alongside this state, add a new helper command "block_job_decommission", >>>> which transitions to the NULL state and puts down our implicit reference. >>>> This separates instances in the code for "block_job_unref" which merely >>>> undo a matching "block_job_ref" with instances intended to initiate the >>>> full destruction of the object. >>>> >>>> This decommission action also sets a number of fields to make sure that >>>> block internals or external users that are holding a reference to a job >>>> to see when it "finishes" are convinced that the job object is "done." >>>> This is necessary, for instance, to do a block_job_cancel_sync on a >>>> created object which will not make any progress. >>>> >>>> Now, all jobs must go through block_job_decommission prior to being >>>> freed, giving us start-to-finish state machine coverage for jobs. >>>> >>>> >>>> Transitions: >>>> Created -> Null: Early failure event before the job is started >>>> Concluded -> Null: Standard transition. >>>> >>>> Verbs: >>>> None. This should not ever be visible to the monitor. >>>> >>>> +---------+ >>>> |UNDEFINED| >>>> +--+------+ >>>> | >>>> +--v----+ >>>> +---------+CREATED+------------------+ >>>> | +--+----+ | >>>> | | | >>>> | +--v----+ +------+ | >>>> +---------+RUNNING<----->PAUSED| | >>>> | +--+-+--+ +------+ | >>>> | | | | >>>> | | +------------------+ | >>>> | | | | >>>> | +--v--+ +-------+ | | >>>> +---------+READY<------->STANDBY| | | >>>> | +--+--+ +-------+ | | >>>> | | | | >>>> +--v-----+ +--v------+ | | >>>> |ABORTING+--->CONCLUDED<-------------+ | >>>> +--------+ +--+------+ | >>>> | | >>>> +--v-+ | >>>> |NULL<---------------------+ >>>> +----+ >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>> >>>> +static void block_job_decommission(BlockJob *job) >>>> +{ >>>> + assert(job); >>>> + job->completed = true; >>>> + job->busy = false; >>>> + job->paused = false; >>>> + job->deferred_to_main_loop = true; >>> >>> Why do we set all of these fields now? I don't see the use of it, and >>> overwriting fields here potentially makes debugging harder. >>> >>> Especially for deferred_to_main_loop I might expect an assert() that it >>> already is true, but shouldn't setting it always be done while actually >>> deferring to the main loop? >>> >>> Can we turn all of these assignments into asserts or are there some that >>> actually aren't already guaranteed, but that we want anyway? >>> >>>> + block_job_state_transition(job, BLOCK_JOB_STATUS_NULL); >>>> + block_job_unref(job); >>>> +} >>> >>> Kevin >>> >> >> Gonna be real honest; we probably only need to set maybe one field >> (job->completed = true) but it was late and I started hitting things >> with big hammers. >> >> The problem is that if jobs do not look "done" to functions like >> finish_sync, they will loop forever trying to make progress on a job >> that doesn't do anything. >> >> I set a bunch of fields here more as a semantic statement than a >> necessity, to be really really honest. ("Well, the job definitely has >> these properties if it made it here, so let's update these fields to be >> correct and the rest of the code will hopefully Do The Right Thing.") > > So essentially, we want this to be assert(), but currently that breaks > for some reasons and we can't figure out why before the freeze? > Nah, I knew exactly why it broke. > I guess that's fair enough, but then it would be good to use the freeze > period to find the offenders and actually turn it into assertions. > > Kevin > I appear to be horridly confused, and you haven't seen the intermediate mess that caused my confusion. A veritable maelstrom of confusion. Mr Babbage would not be able to rightly comprehend, &c. Let's give this another shot. I added that code at a time when my local branch was not calling block_job_completed, because I declared in v4's STM that a pre-created job "shall not pass go, and shall not collect $200" -- that CREATED jobs should either go to RUNNING or NULL. The discovery here is that directly decommissioning a created job actually breaks finish_sync because it polls on the completed boolean, which nothing ever sets. So, under the reasoning I gave you in my last reply; "I'm simply setting these booleans based on the facts of the state machine at this point: we ARE completed, we AREN'T busy, we AREN'T paused, and we have technically now deferred back to the main loop (we never entered it.)" this was enough for finish_sync to get out of the way, but there were other problems with the approach -- a CREATED job has pre-2.12 called the abort/commit callbacks, so I went back to the 2.11 style and added the appropriate transition into the graph. I forgot to check if this code was still necessary at that point. So actually, as of right now, these lines in decommission are useless; but they shouldn't be assertions ... they might fail in a few cases. for instance, "deferred to main loop" won't be set when we cancel a CREATED job.
diff --git a/blockjob.c b/blockjob.c index 3f730967b3..2ef48075b0 100644 --- a/blockjob.c +++ b/blockjob.c @@ -44,24 +44,25 @@ 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, X, E */ - /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0}, - /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 1, 0}, - /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 1}, - /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0}, - /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1}, - /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0}, - /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 1}, - /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0}, + /* U, C, R, P, Y, S, X, E, N */ + /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0, 0}, + /* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 1, 0, 1}, + /* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 1, 0}, + /* P: */ [BLOCK_JOB_STATUS_PAUSED] = {0, 0, 1, 0, 0, 0, 0, 0, 0}, + /* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1, 0}, + /* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0, 0}, + /* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 1, 0}, + /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0, 1}, + /* N: */ [BLOCK_JOB_STATUS_NULL] = {0, 0, 0, 0, 0, 0, 0, 0, 0}, }; bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = { - /* U, C, R, P, Y, S, X, E */ - [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0, 0}, - [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0, 0}, - [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0}, - [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0}, - [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0}, + /* U, C, R, P, Y, S, X, E, N */ + [BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_PAUSE] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1, 1, 0, 0, 0}, + [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0}, }; static void block_job_state_transition(BlockJob *job, BlockJobStatus s1) @@ -225,6 +226,7 @@ static void block_job_detach_aio_context(void *opaque); void block_job_unref(BlockJob *job) { if (--job->refcnt == 0) { + assert(job->status == BLOCK_JOB_STATUS_NULL); BlockDriverState *bs = blk_bs(job->blk); QLIST_REMOVE(job, job_list); bs->job = NULL; @@ -378,6 +380,17 @@ void block_job_start(BlockJob *job) bdrv_coroutine_enter(blk_bs(job->blk), job->co); } +static void block_job_decommission(BlockJob *job) +{ + assert(job); + job->completed = true; + job->busy = false; + job->paused = false; + job->deferred_to_main_loop = true; + block_job_state_transition(job, BLOCK_JOB_STATUS_NULL); + block_job_unref(job); +} + static void block_job_conclude(BlockJob *job) { block_job_state_transition(job, BLOCK_JOB_STATUS_CONCLUDED); @@ -424,7 +437,7 @@ static void block_job_completed_single(BlockJob *job) QLIST_REMOVE(job, txn_list); block_job_txn_unref(job->txn); block_job_conclude(job); - block_job_unref(job); + block_job_decommission(job); } static void block_job_cancel_async(BlockJob *job) @@ -817,7 +830,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, block_job_set_speed(job, speed, &local_err); if (local_err) { - block_job_unref(job); + block_job_early_fail(job); error_propagate(errp, local_err); return NULL; } @@ -851,7 +864,8 @@ void block_job_pause_all(void) void block_job_early_fail(BlockJob *job) { - block_job_unref(job); + assert(job->status == BLOCK_JOB_STATUS_CREATED); + block_job_decommission(job); } void block_job_completed(BlockJob *job, int ret) diff --git a/qapi/block-core.json b/qapi/block-core.json index e489660886..dc25d1d306 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1003,11 +1003,14 @@ # @concluded: The job has finished all work. If manual was set to true, the job # will remain in the query list until it is dismissed. # +# @null: The job is in the process of being dismantled. This state should not +# ever be visible externally. +# # Since: 2.12 ## { 'enum': 'BlockJobStatus', 'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby', - 'aborting', 'concluded' ] } + 'aborting', 'concluded', 'null' ] } ## # @BlockJobInfo:
Add a new state that specifically demarcates when we begin to permanently demolish a job after it has performed all work. This makes the transition explicit in the STM table and highlights conditions under which a job may be demolished. Alongside this state, add a new helper command "block_job_decommission", which transitions to the NULL state and puts down our implicit reference. This separates instances in the code for "block_job_unref" which merely undo a matching "block_job_ref" with instances intended to initiate the full destruction of the object. This decommission action also sets a number of fields to make sure that block internals or external users that are holding a reference to a job to see when it "finishes" are convinced that the job object is "done." This is necessary, for instance, to do a block_job_cancel_sync on a created object which will not make any progress. Now, all jobs must go through block_job_decommission prior to being freed, giving us start-to-finish state machine coverage for jobs. Transitions: Created -> Null: Early failure event before the job is started Concluded -> Null: Standard transition. Verbs: None. This should not ever be visible to the monitor. +---------+ |UNDEFINED| +--+------+ | +--v----+ +---------+CREATED+------------------+ | +--+----+ | | | | | +--v----+ +------+ | +---------+RUNNING<----->PAUSED| | | +--+-+--+ +------+ | | | | | | | +------------------+ | | | | | | +--v--+ +-------+ | | +---------+READY<------->STANDBY| | | | +--+--+ +-------+ | | | | | | +--v-----+ +--v------+ | | |ABORTING+--->CONCLUDED<-------------+ | +--------+ +--+------+ | | | +--v-+ | |NULL<---------------------+ +----+ Signed-off-by: John Snow <jsnow@redhat.com> --- blockjob.c | 50 ++++++++++++++++++++++++++++++++------------------ qapi/block-core.json | 5 ++++- 2 files changed, 36 insertions(+), 19 deletions(-)