blockjobs: add explicit job management

If speed is '0' it's not actually "less than" the previous speed. Kick the job in this case too.

Signed-off-by John Snow <[email protected]>

---
  blockjob.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

blockjob.c +1 -1

index 3f52f29f75..24833ef30f 100644
@@ -499,7 +499,7 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
      }
 
      job->speed = speed;
-    if (speed <= old_speed) {
+    if (speed && speed <= old_speed) {
          return;
      }
 

If speed is '0' it's not actually "less than" the previous speed. Kick the job in this case too.

Signed-off-by John Snow <[email protected]>

model all independent jobs as single job transactions.

It's one less case we have to worry about when we add more states to the transition machine. This way, we can just treat all job lifetimes exactly the same. This helps tighten assertions of the STM graph and removes some conditionals that would have been needed in the coming commits adding a more explicit job lifetime management API.

Signed-off-by John Snow <[email protected]>

---

blockjob.c +11 -0

@@ -729,6 +727,17 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
              return NULL;
          }
      }
+
+    /* Single jobs are modeled as single-job transactions for sake of
+     * consolidating the job management logic */
+    if (!txn) {
+        txn = block_job_txn_new();
+        block_job_txn_add_job(txn, job);
+        block_job_txn_unref(txn);
+    } else {
+        block_job_txn_add_job(txn, job);
+    }
+
      return job;

model all independent jobs as single job transactions.

It's one less case we have to worry about when we add more states to the transition machine. This way, we can just treat all job lifetimes exactly the same. This helps tighten assertions of the STM graph and removes some conditionals that would have been needed in the coming commits adding a more explicit job lifetime management API.

Signed-off-by John Snow <[email protected]>

This property will be used to opt-in to the new BlockJobs workflow that allows a tighter, more explicit control over transitions from one runstate to another.

While we're here, fix up the documentation for block_job_create a little bit.

Signed-off-by John Snow <[email protected]>

---
  blockjob.c                   |  1 +
  include/block/blockjob.h     | 10 ++++++++++
  include/block/blockjob_int.h |  4 +++-
  3 files changed, 14 insertions(+), 1 deletion(-)

This property will be used to opt-in to the new BlockJobs workflow that allows a tighter, more explicit control over transitions from one runstate to another.

While we're here, fix up the documentation for block_job_create a little bit.

Signed-off-by John Snow <[email protected]>

We're about to add several new states, and booleans are becoming unwieldly and difficult to reason about. It would help to have a more explicit bookkeeping of the state of blockjobs. To this end, add a new "status" field and add our existing states in a redundant manner alongside the bools they are replacing:

Some of these states appear somewhat superfluous, but it helps define the expected flow of a job; so some of the states wind up being synchronous empty transitions. Importantly, jobs can be in only one of these states at any given time, which helps code and external users alike reason about the current condition of a job unambiguously.

Signed-off-by John Snow <[email protected]>

---

+++ b/qapi/block-core.json

qapi/block-core.json +15 -0

@@ -955,6 +955,32 @@
  { 'enum': 'BlockJobType',
    'data': ['commit', 'stream', 'mirror', 'backup'] }
 
+##
+# @BlockJobStatus:
+#
+# Indicates the present state of a given blockjob in its lifetime.
+#
+# @undefined: Erroneous, default state. Should not ever be visible.
+#
+# @created: The job has been created, but not yet started.
+#
+# @running: The job is currently running.
+#
+# @paused: The job is running, but paused. The pause may be requested by
+#          either the QMP user or by internal processes.
+#
+# @ready: The job is running, but is ready for the user to signal completion.

We're about to add several new states, and booleans are becoming unwieldly and difficult to reason about. It would help to have a more explicit bookkeeping of the state of blockjobs. To this end, add a new "status" field and add our existing states in a redundant manner alongside the bools they are replacing:

UNDEFINED: Placeholder, default state. Not currently visible to QMP
unless changes occur in the future to allow creating jobs without starting them via QMP.

CREATED:   replaces !!job->co && paused && !busy
RUNNING:   replaces effectively (!paused && busy)
PAUSED:    Nearly redundant with info->paused, which shows pause_count.
           This reports the actual status of the job, which almost always
           matches the paused request status. It differs in that it is
           strictly only true when the job has actually gone dormant.
READY:     replaces job->ready.
STANDBY:   Paused, but job->ready is true.

New state additions in coming commits will not be quite so redundant:

WAITING: Waiting on transaction. This job has finished all the work
it can until the transaction converges, fails, or is canceled.
PENDING: Pending authorization from user. This job has finished all the
work it can until the job or transaction is finalized via block_job_finalize. This implies the transaction has converged and left the WAITING phase.
ABORTING: Job has encountered an error condition and is in the process
of aborting.
CONCLUDED: Job has ceased all operations and has a return code available
for query and may be dismissed via block_job_dismiss.
NULL: Job has been dismissed and (should) be destroyed. Should never
be visible to QMP.

Some of these states appear somewhat superfluous, but it helps define the expected flow of a job; so some of the states wind up being synchronous empty transitions. Importantly, jobs can be in only one of these states at any given time, which helps code and external users alike reason about the current condition of a job unambiguously.

Signed-off-by John Snow <[email protected]>

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 <[email protected]>

---
 block/trace-events |  3 +++
 blockjob.c         | 42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 39 insertions(+), 6 deletions(-)

block/trace-events +3 -0

index 02dd80ff0c..b75a0c8409 100644
@@ -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"

blockjob.c +1 -0

index 1be9c20cff..d745b3bb69 100644
@@ -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"

blockjob.c +25 -1

+    /* 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);
 }
 

Am 24.02.2018 um 00:51 hat John Snow geschrieben:

Quoted 74 lines

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 <[email protected]>

---
 block/trace-events |  3 +++
 blockjob.c         | 42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 39 insertions(+), 6 deletions(-)

block/trace-events +3 -0

index 02dd80ff0c..b75a0c8409 100644
@@ -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"

blockjob.c +1 -0

index 1be9c20cff..d745b3bb69 100644
@@ -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"

blockjob.c line 48

Quoted 7 lines

blockjob.c +4 -0

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

Quoted 37 lines

blockjob.c +25 -1

+    /* 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);
 }
 

blockjob.c void *block_job_create(const char *job_id, const BlockJobDriver *driver, · line 736

Quoted 4 lines

blockjob.c +1 -0

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

Quoted 131 lines

Am 24.02.2018 um 00:51 hat John Snow geschrieben:

Quoted 74 lines

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 <[email protected]>

---
 block/trace-events |  3 +++
 blockjob.c         | 42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 39 insertions(+), 6 deletions(-)

block/trace-events +3 -0

index 02dd80ff0c..b75a0c8409 100644
@@ -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"

blockjob.c +1 -0

index 1be9c20cff..d745b3bb69 100644
@@ -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"

blockjob.c line 48

Quoted 7 lines

blockjob.c +4 -0

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

Quoted 37 lines

blockjob.c +25 -1

+    /* 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);
 }
 

blockjob.c void *block_job_create(const char *job_id, const BlockJobDriver *driver, · line 736

Quoted 4 lines

blockjob.c +1 -0

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

Quoted 5 lines

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.

Am 27.02.2018 um 17:45 hat John Snow geschrieben:

Quoted 134 lines · Kevin Wolf

Quoted 131 lines

Am 24.02.2018 um 00:51 hat John Snow geschrieben:

Quoted 74 lines

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 <[email protected]>

---
 block/trace-events |  3 +++
 blockjob.c         | 42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 39 insertions(+), 6 deletions(-)

block/trace-events +3 -0

index 02dd80ff0c..b75a0c8409 100644
@@ -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"

blockjob.c +1 -0

index 1be9c20cff..d745b3bb69 100644
@@ -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"

blockjob.c line 48

Quoted 7 lines

blockjob.c +4 -0

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

Quoted 37 lines

blockjob.c +25 -1

+    /* 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);
 }
 

blockjob.c void *block_job_create(const char *job_id, const BlockJobDriver *driver, · line 736

Quoted 4 lines

blockjob.c +1 -0

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

Quoted 6 lines

Quoted 5 lines

Or should we completely remove BLOCK_JOB_STATUS_UNDEFINED, keep the initialisation and not call block_job_state_transition() here?

Kevin

Quoted 8 lines

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

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 <[email protected]>

---
  block/trace-events |  3 +++
  blockjob.c         | 42 ++++++++++++++++++++++++++++++++++++------
  2 files changed, 39 insertions(+), 6 deletions(-)

blockjob.c +1 -1

@@ -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);
  }
 

The ascii-art tables help in this and other patches.  Thanks for producing them.

Quoted 3 lines · John Snow

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.

Quoted 5 lines

Perform state transitions with a function that for now just asserts the transition is appropriate.

Transitions:
Undefined -> Created: During job initialization.

Unless we use Created as the default state 0 for g_new0().

Quoted 30 lines

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

Quoted 3 lines

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.

The ascii-art tables help in this and other patches.  Thanks for producing them.

Quoted 7 lines

Signed-off-by John Snow <[email protected]>

---
  block/trace-events |  3 +++
  blockjob.c         | 42 ++++++++++++++++++++++++++++++++++++------

  2 files changed, 39 insertions(+), 6 deletions(-)

blockjob.c line 61

Quoted 6 lines

blockjob.c +6 -0

+static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
+{
+    BlockJobStatus s0 = job->status;
+    if (s0 == s1) {
+        return;
+    }

Quoted 1 line

+    assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX);

Or, if you keep the zero state distinct from valid states, this could be 'assert(s1 > 0 && ...)'

Quoted 8 lines

blockjob.c +1 -1

@@ -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);
   }

Quoted 6 lines

  @@ -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);

Oops, missing a deletion of the job->status assignment line.

Split out the pause command into the actual pause and the wait. Not every usage presently needs to resubmit a pause request.

The intent with the next commit will be to explicitly disallow redundant or meaningless pause/resume requests, so the tests need to become more judicious to reflect that.

Signed-off-by John Snow <[email protected]>

Split out the pause command into the actual pause and the wait. Not every usage presently needs to resubmit a pause request.

The intent with the next commit will be to explicitly disallow redundant or meaningless pause/resume requests, so the tests need to become more judicious to reflect that.

Signed-off-by John Snow <[email protected]>

---
  tests/qemu-iotests/030        |  6 ++----
  tests/qemu-iotests/055        | 17 ++++++-----------
  tests/qemu-iotests/iotests.py | 12 ++++++++----
  3 files changed, 16 insertions(+), 19 deletions(-)

Which commands ("verbs") are appropriate for jobs in which state is also somewhat burdensome to keep track of.

As of this commit, it looks rather useless, but begins to look more interesting the more states we add to the STM table.

A recurring theme is that no verb will apply to an 'undefined' job.

Further, it's not presently possible to restrict the "pause" or "resume" verbs any more than they are in this commit because of the asynchronous nature of how jobs enter the PAUSED state; justifications for some seemingly erroneous applications are given below.

=====
Verbs
=====

Cancel:    Any state except undefined.
Pause:     Any state except undefined;

'created': Requests that the job pauses as it starts. 'running': Normal usage. (PAUSED)
'paused': The job may be paused for internal reasons,
but the user may wish to force an indefinite user-pause, so this is allowed.
'ready': Normal usage. (STANDBY)
'standby': Same logic as above.
Resume: Any state except undefined;
'created': Will lift a user's pause-on-start request. 'running': Will lift a pause request before it takes effect. 'paused': Normal usage.
'ready': Will lift a pause request before it takes effect.
'standby': Normal usage.

Set-speed: Any state except undefined, though ready may not be meaningful.
Complete:  Only a 'ready' job may accept a complete request.

=======
Changes
=======

(1)

To facilitate "nice" error checking, all five major block-job verb interfaces in blockjob.c now support an errp parameter:

- block_job_user_cancel is added as a new interface.
- block_job_user_pause gains an errp paramter
- block_job_user_resume gains an errp parameter
- block_job_set_speed already had an errp parameter.
- block_job_complete already had an errp parameter.

(2)

block-job-pause and block-job-resume will no longer no-op when trying to pause an already paused job, or trying to resume a job that isn't paused. These functions will now report that they did not perform the action requested because it was not possible.

iotests have been adjusted to address this new behavior.

(3)

block-job-complete doesn't worry about checking !block_job_started, because the permission table guards against this.

(4)

test-bdrv-drain's job implementation needs to announce that it is 'ready' now, in order to be completed.

Signed-off-by John Snow <[email protected]>

Which commands ("verbs") are appropriate for jobs in which state is also somewhat burdensome to keep track of.

As of this commit, it looks rather useless, but begins to look more interesting the more states we add to the STM table.

A recurring theme is that no verb will apply to an 'undefined' job.

=======
Changes
=======

(1)

To facilitate "nice" error checking, all five major block-job verb interfaces in blockjob.c now support an errp parameter:

- block_job_user_cancel is added as a new interface.
- block_job_user_pause gains an errp paramter
- block_job_user_resume gains an errp parameter
- block_job_set_speed already had an errp parameter.
- block_job_complete already had an errp parameter.

(2)

block-job-pause and block-job-resume will no longer no-op when trying to pause an already paused job, or trying to resume a job that isn't paused. These functions will now report that they did not perform the action requested because it was not possible.

iotests have been adjusted to address this new behavior.

(3)

block-job-complete doesn't worry about checking !block_job_started, because the permission table guards against this.

(4)

test-bdrv-drain's job implementation needs to announce that it is 'ready' now, in order to be completed.

Signed-off-by John Snow <[email protected]>

---

Quoted 7 lines · John Snow

Which commands ("verbs") are appropriate for jobs in which state is also somewhat burdensome to keep track of.

As of this commit, it looks rather useless, but begins to look more interesting the more states we add to the STM table.

A recurring theme is that no verb will apply to an 'undefined' job.

Back to the argument of whether we even want that state.

Quoted 23 lines

=======
Changes
=======

(1)

To facilitate "nice" error checking, all five major block-job verb interfaces in blockjob.c now support an errp parameter:

- block_job_user_cancel is added as a new interface.
- block_job_user_pause gains an errp paramter
- block_job_user_resume gains an errp parameter
- block_job_set_speed already had an errp parameter.
- block_job_complete already had an errp parameter.

(2)

block-job-pause and block-job-resume will no longer no-op when trying to pause an already paused job, or trying to resume a job that isn't paused. These functions will now report that they did not perform the action requested because it was not possible.

Quoted 1 line

iotests have been adjusted to address this new behavior.

Seems reasonable.  Hopefully shouldn't trip up libvirt too badly (if libvirt attempted a redundant job transition that used to silently succeed and now fails, the failure message should be pretty obvious that it was a no-op attempt).

Quoted 6 lines

(3)

block-job-complete doesn't worry about checking !block_job_started, because the permission table guards against this.

Quoted 7 lines

(4)

test-bdrv-drain's job implementation needs to announce that it is 'ready' now, in order to be completed.

Signed-off-by John Snow <[email protected]>

---

Reviewed-by Eric Blake <[email protected]>

Add a new state ABORTING.

This makes transitions from normative states to error states explicit in the STM, and serves as a disambiguation for which states may complete normally when normal end-states (CONCLUDED) are added in future commits.

Notably, Paused/Standby jobs do not transition directly to aborting, as they must wake up first and cooperate in their cancellation.

Transitions:
Running -> Aborting: can be cancelled or encounter an error
Ready -> Aborting: can be cancelled or encounter an error

Verbs:
None. The job must finish cleaning itself up and report its final status.

              +---------+
              |UNDEFINED|
              +--+------+
                 |
              +--v----+
              |CREATED|
              +--+----+
                 |
              +--v----+     +------+
    +---------+RUNNING<----->PAUSED|
    |         +--+----+     +------+
    |            |
    |         +--v--+       +-------+
    +---------+READY<------->STANDBY|
    |         +-----+       +-------+
    |
+--v-----+
|ABORTING|
+--------+

Signed-off-by John Snow <[email protected]>

---
  blockjob.c           | 31 ++++++++++++++++++-------------
  qapi/block-core.json |  7 ++++++-
  2 files changed, 24 insertions(+), 14 deletions(-)

+++ b/qapi/block-core.json

Add a new state ABORTING.

This makes transitions from normative states to error states explicit in the STM, and serves as a disambiguation for which states may complete normally when normal end-states (CONCLUDED) are added in future commits.

Notably, Paused/Standby jobs do not transition directly to aborting, as they must wake up first and cooperate in their cancellation.

Transitions:
Running -> Aborting: can be cancelled or encounter an error
Ready -> Aborting: can be cancelled or encounter an error

Verbs:
None. The job must finish cleaning itself up and report its final status.

             +---------+
             |UNDEFINED|
             +--+------+
                |
             +--v----+
             |CREATED|
             +--+----+
                |
             +--v----+     +------+
   +---------+RUNNING<----->PAUSED|
   |         +--+----+     +------+
   |            |
   |         +--v--+       +-------+
   +---------+READY<------->STANDBY|
   |         +-----+       +-------+
   |
+--v-----+
|ABORTING|
+--------+

Signed-off-by John Snow <[email protected]>

Am 24.02.2018 um 00:51 hat John Snow geschrieben:

Quoted 39 lines

Add a new state ABORTING.

This makes transitions from normative states to error states explicit in the STM, and serves as a disambiguation for which states may complete normally when normal end-states (CONCLUDED) are added in future commits.

Notably, Paused/Standby jobs do not transition directly to aborting, as they must wake up first and cooperate in their cancellation.

Transitions:
Running -> Aborting: can be cancelled or encounter an error
Ready -> Aborting: can be cancelled or encounter an error

Verbs:
None. The job must finish cleaning itself up and report its final status.

             +---------+
             |UNDEFINED|
             +--+------+
                |
             +--v----+
             |CREATED|
             +--+----+
                |
             +--v----+     +------+
   +---------+RUNNING<----->PAUSED|
   |         +--+----+     +------+
   |            |
   |         +--v--+       +-------+
   +---------+READY<------->STANDBY|
   |         +-----+       +-------+
   |
+--v-----+
|ABORTING|
+--------+

Signed-off-by John Snow <[email protected]>

blockjob.c static void block_job_completed_single(BlockJob *job) · line 393

Quoted 10 lines

blockjob.c +4 -0

@@ -383,6 +384,10 @@ static void block_job_completed_single(BlockJob *job)
 {
     assert(job->completed);
 
+    if (job->ret || block_job_is_cancelled(job)) {
+        block_job_state_transition(job, BLOCK_JOB_STATUS_ABORTING);
+    }
+
     if (!job->ret) {
         if (job->driver->commit) {
             job->driver->commit(job);

Reviewed-by Kevin Wolf <[email protected]>

But I do have a question about the code below the new lines: I wonder where job->ret is set to an error value? I can clearly see that the job is marked as cancelled, so your added code should be working, but looking at the block job code, it looks to me as if the jobs were completing with job->cancelled = true and job->ret = 0.

For block_job_completed_single(), this means that we would call the .commit callback and .cb with a success return code, while sending a CANCELLED event on QMP.

Of course, the only job type that supports transactions is the backup job, and that one seems to compensate for this by explicitly checking block_job_is_cancelled() in its .commit implementation, but is that really how things should be working?

Kevin

add a new state "CONCLUDED" that identifies a job that has ceased all operations. The wording was chosen to avoid any phrasing that might imply success, error, or cancellation. The task has simply ceased all operation and can never again perform any work.

("finished", "done", and "completed" might all imply success.)

Transitions:
Running -> Concluded: normal completion
Ready -> Concluded: normal completion
Aborting -> Concluded: error and cancellations

Verbs:
None as of this commit. (a future commit adds 'dismiss')

Quoted 18 lines · John Snow

add a new state "CONCLUDED" that identifies a job that has ceased all operations. The wording was chosen to avoid any phrasing that might imply success, error, or cancellation. The task has simply ceased all operation and can never again perform any work.

("finished", "done", and "completed" might all imply success.)

Transitions:
Running  -> Concluded: normal completion
Ready    -> Concluded: normal completion
Aborting -> Concluded: error and cancellations

Verbs:
None as of this commit. (a future commit adds 'dismiss')

blockjob.c void block_job_user_resume(BlockJob *job, Error · line 628

Quoted 7 lines

blockjob.c +3 -1

@@ -620,7 +623,9 @@ void block_job_user_resume(BlockJob *job, Error
 **errp)
     void block_job_cancel(BlockJob *job)
   {
-    if (block_job_started(job)) {
+    if (job->status == BLOCK_JOB_STATUS_CONCLUDED) {
+        return;
+    } else if (block_job_started(job)) {

It's also possible to do:

if () {
  return ;
}
if (...)

instead of chaining with an 'else if'.  Matter of personal taste, so I won't make you change it.

Reviewed-by Eric Blake <[email protected]>

add a new state "CONCLUDED" that identifies a job that has ceased all operations. The wording was chosen to avoid any phrasing that might imply success, error, or cancellation. The task has simply ceased all operation and can never again perform any work.

("finished", "done", and "completed" might all imply success.)

Transitions:
Running -> Concluded: normal completion
Ready -> Concluded: normal completion
Aborting -> Concluded: error and cancellations

Verbs:
None as of this commit. (a future commit adds 'dismiss')

             +---------+
             |UNDEFINED|
             +--+------+
                |
             +--v----+
             |CREATED|
             +--+----+
                |
             +--v----+     +------+
   +---------+RUNNING<----->PAUSED|
   |         +--+-+--+     +------+
   |            | |
   |            | +------------------+
   |            |                    |
   |         +--v--+       +-------+ |
   +---------+READY<------->STANDBY| |
   |         +--+--+       +-------+ |
   |            |                    |
+--v-----+   +--v------+             |
|ABORTING+--->CONCLUDED<-------------+
+--------+   +---------+

Signed-off-by John Snow <[email protected]>

Am 24.02.2018 um 00:51 hat John Snow geschrieben:

Quoted 38 lines

add a new state "CONCLUDED" that identifies a job that has ceased all operations. The wording was chosen to avoid any phrasing that might imply success, error, or cancellation. The task has simply ceased all operation and can never again perform any work.

("finished", "done", and "completed" might all imply success.)

Transitions:
Running -> Concluded: normal completion
Ready -> Concluded: normal completion
Aborting -> Concluded: error and cancellations

Verbs:
None as of this commit. (a future commit adds 'dismiss')

             +---------+
             |UNDEFINED|
             +--+------+
                |
             +--v----+
             |CREATED|
             +--+----+
                |
             +--v----+     +------+
   +---------+RUNNING<----->PAUSED|
   |         +--+-+--+     +------+
   |            | |
   |            | +------------------+
   |            |                    |
   |         +--v--+       +-------+ |
   +---------+READY<------->STANDBY| |
   |         +--+--+       +-------+ |
   |            |                    |
+--v-----+   +--v------+             |
|ABORTING+--->CONCLUDED<-------------+
+--------+   +---------+

Signed-off-by John Snow <[email protected]>

blockjob.c static void block_job_event_completed(BlockJob *job, const char *msg) · line 741

Quoted 7 lines

blockjob.c +7 -0

+static void block_job_event_concluded(BlockJob *job)
+{
+    if (block_job_is_internal(job) || !job->manual) {
+        return;
+    }
+    block_job_state_transition(job, BLOCK_JOB_STATUS_CONCLUDED);
+}

I don't understand why internal or automatic jobs should follow a different state machine. Sure, they won't be in this state for long

because the job is immediately unref'ed. Though if someone holds an additional reference, it might be visible - I would consider this a bug fix because otherwise the job stays in READY and continues to accept the verbs for that state.

Kevin

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.

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.

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 <[email protected]>

blockjob.c +1 -0

@@ -423,6 +424,7 @@ static void block_job_completed_single(BlockJob *job)
     QLIST_REMOVE(job, txn_list);
     block_job_txn_unref(job->txn);
     block_job_event_concluded(job);
+    block_job_state_transition(job, BLOCK_JOB_STATUS_NULL);
     block_job_unref(job);
 }
 

Am 24.02.2018 um 00:51 hat John Snow geschrieben:

Quoted 39 lines

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.

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

Quoted 1 line

Signed-off-by John Snow <[email protected]>

The commit message does not match the code:

Quoted 10 lines

blockjob.c +1 -0

@@ -423,6 +424,7 @@ static void block_job_completed_single(BlockJob *job)
     QLIST_REMOVE(job, txn_list);
     block_job_txn_unref(job->txn);
     block_job_event_concluded(job);
+    block_job_state_transition(job, BLOCK_JOB_STATUS_NULL);
     block_job_unref(job);
 }
 

@@ -734,9 +736,6 @@ static void block_job_event_completed(BlockJob *job, const char *msg)
 

blockjob.c static void block_job_event_completed(BlockJob *job, const char *msg) · line 740

Quoted 7 lines

blockjob.c +0 -3

 static void block_job_event_concluded(BlockJob *job)
 {
-    if (block_job_is_internal(job) || !job->manual) {
-        return;
-    }
     block_job_state_transition(job, BLOCK_JOB_STATUS_CONCLUDED);
 }

Any job that transition to NULL goes first through CONCLUDED. There is no way to reach NULL directly from CREATED.

The second hunk addresses my comment in the previous patch, so it should probably be squashed in there.

Kevin

For jobs that have reached their CONCLUDED state, prior to having their last reference put down (meaning jobs that have completed successfully, unsuccessfully, or have been canceled), allow the user to dismiss the job's lingering status report via block-job-dismiss.

This gives management APIs the chance to conclusively determine if a job failed or succeeded, even if the event broadcast was missed.

Note that jobs do not yet linger in any such state, they are freed immediately upon reaching this previously-unnamed state. such a state is added immediately in the next commit.

Verbs:
Dismiss: operates on CONCLUDED jobs only.

Signed-off-by John Snow <[email protected]>

---

For jobs that have reached their CONCLUDED state, prior to having their last reference put down (meaning jobs that have completed successfully, unsuccessfully, or have been canceled), allow the user to dismiss the job's lingering status report via block-job-dismiss.

This gives management APIs the chance to conclusively determine if a job failed or succeeded, even if the event broadcast was missed.

Note that jobs do not yet linger in any such state, they are freed immediately upon reaching this previously-unnamed state. such a state is added immediately in the next commit.

Verbs:
Dismiss: operates on CONCLUDED jobs only.

Signed-off-by John Snow <[email protected]>

---
 block/trace-events       |  1 +
 blockdev.c               | 14 ++++++++++++++
 blockjob.c               | 34 ++++++++++++++++++++++++++++++++--
 include/block/blockjob.h |  9 +++++++++
 qapi/block-core.json     | 24 +++++++++++++++++++++++-
 5 files changed, 79 insertions(+), 3 deletions(-)

Am 24.02.2018 um 00:51 hat John Snow geschrieben:

Quoted 8 lines

For jobs that have reached their CONCLUDED state, prior to having their last reference put down (meaning jobs that have completed successfully, unsuccessfully, or have been canceled), allow the user to dismiss the job's lingering status report via block-job-dismiss.

This gives management APIs the chance to conclusively determine if a job failed or succeeded, even if the event broadcast was missed.

Quoted 6 lines

Note that jobs do not yet linger in any such state, they are freed immediately upon reaching this previously-unnamed state. such a state is added immediately in the next commit.

Verbs:
Dismiss: operates on CONCLUDED jobs only.

You want to insert an empty line here.

Quoted 8 lines

Signed-off-by John Snow <[email protected]>

---
 block/trace-events       |  1 +
 blockdev.c               | 14 ++++++++++++++
 blockjob.c               | 34 ++++++++++++++++++++++++++++++++--
 include/block/blockjob.h |  9 +++++++++
 qapi/block-core.json     | 24 +++++++++++++++++++++++-
 5 files changed, 79 insertions(+), 3 deletions(-)

blockjob.c void *block_job_create(const char *job_id, const BlockJobDriver *driver, · line 869

Quoted 6 lines

blockjob.c +2 -0

@@ -841,6 +865,9 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
         block_job_txn_add_job(txn, job);
     }
 
+    /* For the expanded job control STM, grab an extra
+     * reference for finalize() to put down */

Do you mean dismiss()?

Quoted 4 lines

blockjob.c +1 -0

+    block_job_ref(job);
     return job;
 }
 

blockjob.c void block_job_pause_all(void) · line 889

Quoted 5 lines

blockjob.c +1 -0

@@ -859,6 +886,9 @@ void block_job_pause_all(void)
 
 void block_job_early_fail(BlockJob *job)
 {
+    /* One for creation, one for finalize() */

And here?

blockjob.c void block_job_pause_all(void) · line 893

Quoted 4 lines

blockjob.c +2 -0

+    assert(job->status == BLOCK_JOB_STATUS_CREATED);
+    block_job_unref(job);
     block_job_unref(job);
 }

Kevin

Presently, even if a job is canceled post-completion as a result of a failing peer in a transaction, it will still call .commit because nothing has updated or changed its return code.

The reason why this does not cause problems currently is because backup's implementation of .commit checks for cancellation itself.

I'd like to simplify this contract:

(1) Abort is called if the job/transaction fails
(2) Commit is called if the job/transaction succeeds

To this end: A job's return code, if 0, will be forcibly set as -ECANCELED if that job has already concluded. Remove the now redundant check in the backup job implementation.

We need to check for cancellation in both block_job_completed AND block_job_completed_single, because jobs may be cancelled between those two calls; for instance in transactions.

The check in block_job_completed could be removed, but there's no point in starting to attempt to succeed a transaction that we know in advance will fail.

This does NOT affect mirror jobs that are "canceled" during their synchronous phase. The mirror job itself forcibly sets the canceled property to false prior to ceding control, so such cases will invoke the "commit" callback.

Signed-off-by John Snow <[email protected]>

---
  block/backup.c     |  2 +-
  block/trace-events |  1 +
  blockjob.c         | 19 +++++++++++++++----
  3 files changed, 17 insertions(+), 5 deletions(-)

Quoted 29 lines · John Snow

Presently, even if a job is canceled post-completion as a result of a failing peer in a transaction, it will still call .commit because nothing has updated or changed its return code.

The reason why this does not cause problems currently is because backup's implementation of .commit checks for cancellation itself.

I'd like to simplify this contract:

(1) Abort is called if the job/transaction fails
(2) Commit is called if the job/transaction succeeds

To this end: A job's return code, if 0, will be forcibly set as -ECANCELED if that job has already concluded. Remove the now redundant check in the backup job implementation.

We need to check for cancellation in both block_job_completed AND block_job_completed_single, because jobs may be cancelled between those two calls; for instance in transactions.

The check in block_job_completed could be removed, but there's no point in starting to attempt to succeed a transaction that we know in advance will fail.

This does NOT affect mirror jobs that are "canceled" during their synchronous phase. The mirror job itself forcibly sets the canceled property to false prior to ceding control, so such cases will invoke the "commit" callback.

Quoted 6 lines

Signed-off-by John Snow <[email protected]>

---
  block/backup.c     |  2 +-
  block/trace-events |  1 +
  blockjob.c         | 19 +++++++++++++++----

  3 files changed, 17 insertions(+), 5 deletions(-)

More lines of code, but the contract does seem simpler and useful for the later patches.

Reviewed-by Eric Blake <[email protected]>

Presently, even if a job is canceled post-completion as a result of a failing peer in a transaction, it will still call .commit because nothing has updated or changed its return code.

The reason why this does not cause problems currently is because backup's implementation of .commit checks for cancellation itself.

I'd like to simplify this contract:

(1) Abort is called if the job/transaction fails
(2) Commit is called if the job/transaction succeeds

To this end: A job's return code, if 0, will be forcibly set as -ECANCELED if that job has already concluded. Remove the now redundant check in the backup job implementation.

We need to check for cancellation in both block_job_completed AND block_job_completed_single, because jobs may be cancelled between those two calls; for instance in transactions.

The check in block_job_completed could be removed, but there's no point in starting to attempt to succeed a transaction that we know in advance will fail.

This does NOT affect mirror jobs that are "canceled" during their synchronous phase. The mirror job itself forcibly sets the canceled property to false prior to ceding control, so such cases will invoke the "commit" callback.

Signed-off-by John Snow <[email protected]>

---
 block/backup.c     |  2 +-
 block/trace-events |  1 +
 blockjob.c         | 19 +++++++++++++++----
 3 files changed, 17 insertions(+), 5 deletions(-)

block/backup.c +1 -1

index 7e254dabff..453cd62c24 100644
@@ -206,7 +206,7 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
     BdrvDirtyBitmap *bm;
     BlockDriverState *bs = blk_bs(job->common.blk);
 
-    if (ret < 0 || block_job_is_cancelled(&job->common)) {
+    if (ret < 0) {
         /* Merge the successor back into the parent, delete nothing. */
         bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
         assert(bm);

block/trace-events +1 -0

index 266afd9e99..5e531e0310 100644
@@ -5,6 +5,7 @@ bdrv_open_common(void *bs, const char *filename, int flags, const char *format_n
 bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
 
 # blockjob.c
+block_job_completed(void *job, int ret, int jret) "job %p ret %d corrected ret %d"
 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_job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)"
 

blockjob.c +12 -3

index 4d29391673..ef17dea004 100644
@@ -384,13 +384,22 @@ void block_job_start(BlockJob *job)
     bdrv_coroutine_enter(blk_bs(job->blk), job->co);
 }
 
+static void block_job_update_rc(BlockJob *job)
+{
+    if (!job->ret && block_job_is_cancelled(job)) {
+        job->ret = -ECANCELED;
+    }
+    if (job->ret) {
+        block_job_state_transition(job, BLOCK_JOB_STATUS_ABORTING);
+    }
+}
+
 static void block_job_completed_single(BlockJob *job)
 {
     assert(job->completed);
 
-    if (job->ret || block_job_is_cancelled(job)) {
-        block_job_state_transition(job, BLOCK_JOB_STATUS_ABORTING);
-    }
+    /* Ensure abort is called for late-transactional failures */
+    block_job_update_rc(job);
 
     if (!job->ret) {
         if (job->driver->commit) {

The completed_single function is getting a little mucked up with checking to see which callbacks exist, so let's factor them out.

Signed-off-by John Snow <[email protected]>

---
  blockjob.c | 35 ++++++++++++++++++++++++++---------
  1 file changed, 26 insertions(+), 9 deletions(-)

The completed_single function is getting a little mucked up with checking to see which callbacks exist, so let's factor them out.

Signed-off-by John Snow <[email protected]>

Simply apply a function transaction-wide.
A few more uses of this in forthcoming patches.

Signed-off-by John Snow <[email protected]>

---
  blockjob.c | 24 +++++++++++++++---------
  1 file changed, 15 insertions(+), 9 deletions(-)

Simply apply a function transaction-wide.
A few more uses of this in forthcoming patches.

Signed-off-by John Snow <[email protected]>

---
 blockjob.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

blockjob.c +14 -2

index 431ce9c220..8f02c03880 100644
@@ -467,6 +467,19 @@ static void block_job_cancel_async(BlockJob *job)
     job->cancelled = true;
 }
 
+static void block_job_txn_apply(BlockJobTxn *txn, void fn(BlockJob *))
+{
+    AioContext *ctx;
+    BlockJob *job, *next;
+
+    QLIST_FOREACH_SAFE(job, &txn->jobs, txn_list, next) {
+        ctx = blk_get_aio_context(job->blk);
+        aio_context_acquire(ctx);
+        fn(job);
+        aio_context_release(ctx);
+    }
+}
+
 static void block_job_do_dismiss(BlockJob *job)
 {
     assert(job);

@@ -552,9 +565,8 @@ static void block_job_completed_txn_abort(BlockJob *job)
 
 static void block_job_completed_txn_success(BlockJob *job)
 {
-    AioContext *ctx;
     BlockJobTxn *txn = job->txn;
-    BlockJob *other_job, *next;
+    BlockJob *other_job;
     /*
      * Successful completion, see if there are other running jobs in this
      * txn.

Some jobs upon finalization may need to perform some work that can still fail. If these jobs are part of a transaction, it's important that these callbacks fail the entire transaction.

We allow for a new callback in addition to commit/abort/clean that allows us the opportunity to have fairly late-breaking failures in the transactional process.

The expected flow is:

- All jobs in a transaction converge to the WAITING state (added in a forthcoming commit)
- All jobs prepare to call either commit/abort
- If any job fails, is canceled, or fails preparation, all jobs call their .abort callback.
- All jobs enter the PENDING state, awaiting manual intervention (also added in a forthcoming commit)
- block-job-finalize is issued by the user/management layer
- All jobs call their commit callbacks.

Signed-off-by John Snow <[email protected]>

---
  blockjob.c                   | 34 +++++++++++++++++++++++++++++++---
  include/block/blockjob_int.h | 10 ++++++++++
  2 files changed, 41 insertions(+), 3 deletions(-)

blockjob.c +2 -1

@@ -467,17 +480,22 @@ static void block_job_cancel_async(BlockJob *job)
      job->cancelled = true;
  }
 
-static void block_job_txn_apply(BlockJobTxn *txn, void fn(BlockJob *))
+static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *))
  {
      AioContext *ctx;
      BlockJob *job, *next;
+    int rc;

Quoted 40 lines · John Snow

Some jobs upon finalization may need to perform some work that can still fail. If these jobs are part of a transaction, it's important that these callbacks fail the entire transaction.

We allow for a new callback in addition to commit/abort/clean that allows us the opportunity to have fairly late-breaking failures in the transactional process.

The expected flow is:

- All jobs in a transaction converge to the WAITING state (added in a forthcoming commit)
- All jobs prepare to call either commit/abort
- If any job fails, is canceled, or fails preparation, all jobs call their .abort callback.
- All jobs enter the PENDING state, awaiting manual intervention (also added in a forthcoming commit)
- block-job-finalize is issued by the user/management layer
- All jobs call their commit callbacks.

Signed-off-by John Snow <[email protected]>

---
  blockjob.c                   | 34 +++++++++++++++++++++++++++++++--- include/block/blockjob_int.h | 10 ++++++++++

  2 files changed, 41 insertions(+), 3 deletions(-)

blockjob.c +2 -1

@@ -467,17 +480,22 @@ static void block_job_cancel_async(BlockJob *job)
       job->cancelled = true;
   }
   -static void block_job_txn_apply(BlockJobTxn *txn, void fn(BlockJob *))
+static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *))
   {
       AioContext *ctx;
       BlockJob *job, *next;
+    int rc;
         QLIST_FOREACH_SAFE(job, &txn->jobs, txn_list, next) {
           ctx = blk_get_aio_context(job->blk);
           aio_context_acquire(ctx);
-        fn(job);