Re: [Qemu-block] [RFC v4 18/21] blockjobs: add block-job-finalize

[Kevin Wolf]

Am 24.02.2018 um 00:51 hat John Snow geschrieben:
> Instead of automatically transitioning from PENDING to CONCLUDED, gate
> the .prepare() and .commit() phases behind an explicit acknowledgement
> provided by the QMP monitor if manual completion mode has been requested.
> 
> This allows us to perform graph changes in prepare and/or commit so that
> graph changes do not occur autonomously without knowledge of the
> controlling management layer.
> 
> Transactions that have reached the "PENDING" state together can all be
> moved to invoke their finalization methods by issuing block_job_finalize
> to any one job in the transaction.
> 
> Jobs in a transaction with mixed job->manual settings will remain stuck
> in the "WAITING" state until block_job_finalize is authored on the job(s)
> that have reached the "PENDING" state.

Why? Removing this inconsistency would make the code slightly simpler.

Is it because you want to avoid that the user picks an automatic job for
completing the mixed transaction?

> These jobs are not allowed to progress because other jobs in the
> transaction may still fail during their preparation phase during
> finalization, so these jobs must remain in the WAITING phase until
> success is guaranteed. These jobs will then automatically dismiss
> themselves, but jobs that had the manual property set will remain
> at CONCLUDED as normal.
> 
> Signed-off-by: John Snow 
> ---
>  block/trace-events   |  1 +
>  blockdev.c   | 14 ++
>  blockjob.c   | 69 
> +---
>  include/block/blockjob.h | 17 
>  qapi/block-core.json | 23 +++-
>  5 files changed, 108 insertions(+), 16 deletions(-)
> 
> diff --git a/block/trace-events b/block/trace-events
> index 5e531e0310..a81b66ff36 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -51,6 +51,7 @@ qmp_block_job_cancel(void *job) "job %p"
>  qmp_block_job_pause(void *job) "job %p"
>  qmp_block_job_resume(void *job) "job %p"
>  qmp_block_job_complete(void *job) "job %p"
> +qmp_block_job_finalize(void *job) "job %p"
>  qmp_block_job_dismiss(void *job) "job %p"
>  qmp_block_stream(void *bs, void *job) "bs %p job %p"
>  
> diff --git a/blockdev.c b/blockdev.c
> index 3180130782..05fd421cdc 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3852,6 +3852,20 @@ void qmp_block_job_complete(const char *device, Error 
> **errp)
>  aio_context_release(aio_context);
>  }
>  
> +void qmp_block_job_finalize(const char *id, Error **errp)
> +{
> +AioContext *aio_context;
> +BlockJob *job = find_block_job(id, _context, errp);
> +
> +if (!job) {
> +return;
> +}
> +
> +trace_qmp_block_job_finalize(job);
> +block_job_finalize(job, errp);
> +aio_context_release(aio_context);
> +}
> +
>  void qmp_block_job_dismiss(const char *id, Error **errp)
>  {
>  AioContext *aio_context;
> diff --git a/blockjob.c b/blockjob.c
> index 23b4b99fd4..f9e8a64261 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -65,14 +65,15 @@ bool 
> BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
>  [BLOCK_JOB_VERB_RESUME]   = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 
> 0},
>  [BLOCK_JOB_VERB_SET_SPEED]= {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 
> 0},
>  [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 
> 0},
> +[BLOCK_JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 
> 0},
>  [BLOCK_JOB_VERB_DISMISS]  = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 
> 0},
>  };
>  
> -static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
> +static bool block_job_state_transition(BlockJob *job, BlockJobStatus s1)
>  {
>  BlockJobStatus s0 = job->status;
>  if (s0 == s1) {
> -return;
> +return false;
>  }
>  assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX);
>  trace_block_job_state_transition(job, job->ret, BlockJobSTT[s0][s1] ?
> @@ -83,6 +84,7 @@ static void block_job_state_transition(BlockJob *job, 
> BlockJobStatus s1)
>s1));
>  assert(BlockJobSTT[s0][s1]);
>  job->status = s1;
> +return true;
>  }
>  
>  static int block_job_apply_verb(BlockJob *job, BlockJobVerb bv, Error **errp)
> @@ -432,7 +434,7 @@ static void block_job_clean(BlockJob *job)
>  }
>  }
>  
> -static int block_job_completed_single(BlockJob *job)
> +static int block_job_finalize_single(BlockJob *job)
>  {
>  assert(job->completed);
>  
> @@ -581,18 +583,44 @@ static void block_job_completed_txn_abort(BlockJob *job)
>  assert(other_job->cancelled);
>  block_job_finish_sync(other_job, NULL, NULL);
>  }
> -block_job_completed_single(other_job);
> +block_job_finalize_single(other_job);
>  aio_context_release(ctx);
>  }
>  
>  block_job_txn_unref(txn);
>  }
>  
> +static int block_job_is_manual(BlockJob *job)
> +{
> 

[Qemu-block] [RFC v4 18/21] blockjobs: add block-job-finalize

[John Snow]

Instead of automatically transitioning from PENDING to CONCLUDED, gate
the .prepare() and .commit() phases behind an explicit acknowledgement
provided by the QMP monitor if manual completion mode has been requested.

This allows us to perform graph changes in prepare and/or commit so that
graph changes do not occur autonomously without knowledge of the
controlling management layer.

Transactions that have reached the "PENDING" state together can all be
moved to invoke their finalization methods by issuing block_job_finalize
to any one job in the transaction.

Jobs in a transaction with mixed job->manual settings will remain stuck
in the "WAITING" state until block_job_finalize is authored on the job(s)
that have reached the "PENDING" state.

These jobs are not allowed to progress because other jobs in the
transaction may still fail during their preparation phase during
finalization, so these jobs must remain in the WAITING phase until
success is guaranteed. These jobs will then automatically dismiss
themselves, but jobs that had the manual property set will remain
at CONCLUDED as normal.

Signed-off-by: John Snow 
---
 block/trace-events   |  1 +
 blockdev.c   | 14 ++
 blockjob.c   | 69 +---
 include/block/blockjob.h | 17 
 qapi/block-core.json | 23 +++-
 5 files changed, 108 insertions(+), 16 deletions(-)

diff --git a/block/trace-events b/block/trace-events
index 5e531e0310..a81b66ff36 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -51,6 +51,7 @@ qmp_block_job_cancel(void *job) "job %p"
 qmp_block_job_pause(void *job) "job %p"
 qmp_block_job_resume(void *job) "job %p"
 qmp_block_job_complete(void *job) "job %p"
+qmp_block_job_finalize(void *job) "job %p"
 qmp_block_job_dismiss(void *job) "job %p"
 qmp_block_stream(void *bs, void *job) "bs %p job %p"
 
diff --git a/blockdev.c b/blockdev.c
index 3180130782..05fd421cdc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3852,6 +3852,20 @@ void qmp_block_job_complete(const char *device, Error 
**errp)
 aio_context_release(aio_context);
 }
 
+void qmp_block_job_finalize(const char *id, Error **errp)
+{
+AioContext *aio_context;
+BlockJob *job = find_block_job(id, _context, errp);
+
+if (!job) {
+return;
+}
+
+trace_qmp_block_job_finalize(job);
+block_job_finalize(job, errp);
+aio_context_release(aio_context);
+}
+
 void qmp_block_job_dismiss(const char *id, Error **errp)
 {
 AioContext *aio_context;
diff --git a/blockjob.c b/blockjob.c
index 23b4b99fd4..f9e8a64261 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -65,14 +65,15 @@ bool 
BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
 [BLOCK_JOB_VERB_RESUME]   = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0},
 [BLOCK_JOB_VERB_SET_SPEED]= {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0},
 [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0},
+[BLOCK_JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0},
 [BLOCK_JOB_VERB_DISMISS]  = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0},
 };
 
-static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
+static bool block_job_state_transition(BlockJob *job, BlockJobStatus s1)
 {
 BlockJobStatus s0 = job->status;
 if (s0 == s1) {
-return;
+return false;
 }
 assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX);
 trace_block_job_state_transition(job, job->ret, BlockJobSTT[s0][s1] ?
@@ -83,6 +84,7 @@ static void block_job_state_transition(BlockJob *job, 
BlockJobStatus s1)
   s1));
 assert(BlockJobSTT[s0][s1]);
 job->status = s1;
+return true;
 }
 
 static int block_job_apply_verb(BlockJob *job, BlockJobVerb bv, Error **errp)
@@ -432,7 +434,7 @@ static void block_job_clean(BlockJob *job)
 }
 }
 
-static int block_job_completed_single(BlockJob *job)
+static int block_job_finalize_single(BlockJob *job)
 {
 assert(job->completed);
 
@@ -581,18 +583,44 @@ static void block_job_completed_txn_abort(BlockJob *job)
 assert(other_job->cancelled);
 block_job_finish_sync(other_job, NULL, NULL);
 }
-block_job_completed_single(other_job);
+block_job_finalize_single(other_job);
 aio_context_release(ctx);
 }
 
 block_job_txn_unref(txn);
 }
 
+static int block_job_is_manual(BlockJob *job)
+{
+return job->manual;
+}
+
+static void block_job_do_finalize(BlockJob *job)
+{
+int rc;
+assert(job && job->txn);
+
+/* For jobs set !job->manual, transition to pending synchronously now */
+block_job_txn_apply(job->txn, block_job_event_pending, false);
+
+/* prepare the transaction to complete */
+rc = block_job_txn_apply(job->txn, block_job_prepare, true);
+if (rc) {
+block_job_completed_txn_abort(job);
+} else {
+