[Qemu-devel] [PATCH v5 00/16] jobs: Job Exit Refactoring Pt 2

[Qemu-devel] [PATCH v5 00/16] jobs: Job Exit Refactoring Pt 2

[John Snow]

This is part two of a two part series that refactors the exit logic
of jobs.

This series forces all jobs to use the "finalize" semantics that were
introduced previously, but only exposed via the backup jobs.

Patches 1-3 add plumbing for the auto-dismiss and auto-finalize flags
but do not expose these via QAPI/QMP.

Patches 4-7 refactor the .exit() callbacks into the component pieces
of .prepare(), .commit(), .abort() and .clean(). Except mirror, which
I cheat with.

Patches 8-10 remove the last usages of .exit in a test.
Patche 11 removes the .exit callback and the machinery to invoke it.

Patches 12-14 expose the new QMP options to all of the jobs.
Patch 15 is a doc fixup.

====
"V5"
====

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/16:[----] [--] 'block/commit: add block job creation flags'
002/16:[----] [--] 'block/mirror: add block job creation flags'
003/16:[----] [--] 'block/stream: add block job creation flags'
004/16:[0002] [FC] 'block/commit: refactor commit to use job callbacks'
005/16:[0005] [FC] 'block/mirror: don't install backing chain on abort'
006/16:[0005] [FC] 'block/mirror: conservative mirror_exit refactor'
007/16:[----] [--] 'block/stream: refactor stream to use job callbacks'
008/16:[----] [--] 'tests/blockjob: replace Blockjob with Job'
009/16:[----] [--] 'tests/test-blockjob: remove exit callback'
010/16:[----] [--] 'tests/test-blockjob-txn: move .exit to .clean'
011/16:[----] [--] 'jobs: remove .exit callback'
012/16:[----] [--] 'qapi/block-commit: expose new job properties'
013/16:[----] [--] 'qapi/block-mirror: expose new job properties'
014/16:[----] [--] 'qapi/block-stream: expose new job properties'
015/16:[----] [--] 'block/backup: qapi documentation fixup'
016/16:[down] 'blockdev: document transactional shortcomings'

004: Add FIXME comment (Jeff)
005: Update commit message, move ternary touchup to next patch (Max)
006: Adopt ternary fixup from previous patch (Max)
016: New, does what it says on the tin (Max)

====
"V4"
====

002, 003: Fix typo (Eric)
005: Don't not install the backing chain if
     we cancel a successful job (Max)
006: Fallout from 005
007: Fixed commit title. (Max)

====
"V3"
====

000: Fixed my cover letter subject. Phew!
004: Adjusted comment, added R-B
005: New; based on discussions from what is now 006 (was 005)
006: mirror_exit_common returns ret == 0 if we are aborting
     and it aborts successfully (Max)
     Added assertion that we aborted successfully (Max)
010: New, minor cleanup before we can actually remove .exit.
011: Removed what's now patch 10, added R-B per Max's comment.

=====
"V2":
=====
 - Split off the first part of the series to Pt.1
 - More aggressively refactored .commit()
 - Went all the way to deleting .exit() callback (Kevin)

John Snow (16):
  block/commit: add block job creation flags
  block/mirror: add block job creation flags
  block/stream: add block job creation flags
  block/commit: refactor commit to use job callbacks
  block/mirror: don't install backing chain on abort
  block/mirror: conservative mirror_exit refactor
  block/stream: refactor stream to use job callbacks
  tests/blockjob: replace Blockjob with Job
  tests/test-blockjob: remove exit callback
  tests/test-blockjob-txn: move .exit to .clean
  jobs: remove .exit callback
  qapi/block-commit: expose new job properties
  qapi/block-mirror: expose new job properties
  qapi/block-stream: expose new job properties
  block/backup: qapi documentation fixup
  blockdev: document transactional shortcomings

 block/commit.c            |  97 ++++++++++++++++++++++-----------------
 block/mirror.c            |  44 ++++++++++++------
 block/stream.c            |  28 ++++++++----
 blockdev.c                |  52 ++++++++++++++++++---
 hmp.c                     |   5 +-
 include/block/block_int.h |  15 ++++--
 include/qemu/job.h        |  11 -----
 job.c                     |  77 ++++++++++++++-----------------
 qapi/block-core.json      |  80 +++++++++++++++++++++++++++-----
 tests/test-blockjob-txn.c |   4 +-
 tests/test-blockjob.c     | 114 +++++++++++++++++++++++-----------------------
 11 files changed, 324 insertions(+), 203 deletions(-)

-- 
2.14.4

[Qemu-devel] [PATCH v5 01/16] block/commit: add block job creation flags

[John Snow]

Add support for taking and passing forward job creation flags.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
 block/commit.c            | 5 +++--
 blockdev.c                | 7 ++++---
 include/block/block_int.h | 5 ++++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index da69165de3..b6e8969877 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -249,7 +249,8 @@ static BlockDriver bdrv_commit_top = {
 };
 
 void commit_start(const char *job_id, BlockDriverState *bs,
-                  BlockDriverState *base, BlockDriverState *top, int64_t speed,
+                  BlockDriverState *base, BlockDriverState *top,
+                  int creation_flags, int64_t speed,
                   BlockdevOnError on_error, const char *backing_file_str,
                   const char *filter_node_name, Error **errp)
 {
@@ -267,7 +268,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     }
 
     s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL,
-                         speed, JOB_DEFAULT, NULL, NULL, errp);
+                         speed, creation_flags, NULL, NULL, errp);
     if (!s) {
         return;
     }
diff --git a/blockdev.c b/blockdev.c
index 72f5347df5..c15a1e624b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3214,6 +3214,7 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
      * BlockdevOnError change for blkmirror makes it in
      */
     BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
+    int job_flags = JOB_DEFAULT;
 
     if (!has_speed) {
         speed = 0;
@@ -3295,15 +3296,15 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
             goto out;
         }
         commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
-                            JOB_DEFAULT, speed, on_error,
+                            job_flags, speed, on_error,
                             filter_node_name, NULL, NULL, false, &local_err);
     } else {
         BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
         if (bdrv_op_is_blocked(overlay_bs, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
             goto out;
         }
-        commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
-                     on_error, has_backing_file ? backing_file : NULL,
+        commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, job_flags,
+                     speed, on_error, has_backing_file ? backing_file : NULL,
                      filter_node_name, &local_err);
     }
     if (local_err != NULL) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 903b9c1034..ffab0b4d3e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -980,6 +980,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
  * @bs: Active block device.
  * @top: Top block device to be committed.
  * @base: Block device that will be written into, and become the new top.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ *                  See @BlockJobCreateFlags
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @on_error: The action to take upon error.
  * @backing_file_str: String to use as the backing file in @top's overlay
@@ -990,7 +992,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
  *
  */
 void commit_start(const char *job_id, BlockDriverState *bs,
-                  BlockDriverState *base, BlockDriverState *top, int64_t speed,
+                  BlockDriverState *base, BlockDriverState *top,
+                  int creation_flags, int64_t speed,
                   BlockdevOnError on_error, const char *backing_file_str,
                   const char *filter_node_name, Error **errp);
 /**
-- 
2.14.4

[Qemu-devel] [PATCH v5 02/16] block/mirror: add block job creation flags

[John Snow]

Add support for taking and passing forward job creation flags.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
 block/mirror.c            | 5 +++--
 blockdev.c                | 3 ++-
 include/block/block_int.h | 5 ++++-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index b8941db6c1..cba555b4ef 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1639,7 +1639,8 @@ fail:
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *target, const char *replaces,
-                  int64_t speed, uint32_t granularity, int64_t buf_size,
+                  int creation_flags, int64_t speed,
+                  uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
@@ -1655,7 +1656,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
     }
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
-    mirror_start_job(job_id, bs, JOB_DEFAULT, target, replaces,
+    mirror_start_job(job_id, bs, creation_flags, target, replaces,
                      speed, granularity, buf_size, backing_mode,
                      on_source_error, on_target_error, unmap, NULL, NULL,
                      &mirror_job_driver, is_none_mode, base, false,
diff --git a/blockdev.c b/blockdev.c
index c15a1e624b..6574356708 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3590,6 +3590,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
                                    bool has_copy_mode, MirrorCopyMode copy_mode,
                                    Error **errp)
 {
+    int job_flags = JOB_DEFAULT;
 
     if (!has_speed) {
         speed = 0;
@@ -3642,7 +3643,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
      * and will allow to check whether the node still exist at mirror completion
      */
     mirror_start(job_id, bs, target,
-                 has_replaces ? replaces : NULL,
+                 has_replaces ? replaces : NULL, job_flags,
                  speed, granularity, buf_size, sync, backing_mode,
                  on_source_error, on_target_error, unmap, filter_node_name,
                  copy_mode, errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ffab0b4d3e..b40f0bfc9b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1029,6 +1029,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
  * @target: Block device to write to.
  * @replaces: Block graph node name to replace once the mirror is done. Can
  *            only be used when full mirroring is selected.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ *                  See @BlockJobCreateFlags
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @granularity: The chosen granularity for the dirty bitmap.
  * @buf_size: The amount of data that can be in flight at one time.
@@ -1050,7 +1052,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
  */
 void mirror_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *target, const char *replaces,
-                  int64_t speed, uint32_t granularity, int64_t buf_size,
+                  int creation_flags, int64_t speed,
+                  uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
-- 
2.14.4

[Qemu-devel] [PATCH v5 03/16] block/stream: add block job creation flags

[John Snow]

Add support for taking and passing forward job creation flags.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
 block/stream.c            | 5 +++--
 blockdev.c                | 3 ++-
 include/block/block_int.h | 5 ++++-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 67e1e72e23..700eb239e4 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -214,7 +214,8 @@ static const BlockJobDriver stream_job_driver = {
 
 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
-                  int64_t speed, BlockdevOnError on_error, Error **errp)
+                  int creation_flags, int64_t speed,
+                  BlockdevOnError on_error, Error **errp)
 {
     StreamBlockJob *s;
     BlockDriverState *iter;
@@ -236,7 +237,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
                          BLK_PERM_GRAPH_MOD,
                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
                          BLK_PERM_WRITE,
-                         speed, JOB_DEFAULT, NULL, NULL, errp);
+                         speed, creation_flags, NULL, NULL, errp);
     if (!s) {
         goto fail;
     }
diff --git a/blockdev.c b/blockdev.c
index 6574356708..ec90eb1cf9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3123,6 +3123,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     AioContext *aio_context;
     Error *local_err = NULL;
     const char *base_name = NULL;
+    int job_flags = JOB_DEFAULT;
 
     if (!has_on_error) {
         on_error = BLOCKDEV_ON_ERROR_REPORT;
@@ -3185,7 +3186,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     base_name = has_backing_file ? backing_file : base_name;
 
     stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
-                 has_speed ? speed : 0, on_error, &local_err);
+                 job_flags, has_speed ? speed : 0, on_error, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b40f0bfc9b..4000d2af45 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -958,6 +958,8 @@ int is_windows_drive(const char *filename);
  * flatten the whole backing file chain onto @bs.
  * @backing_file_str: The file name that will be written to @bs as the
  * the new backing file if the job completes. Ignored if @base is %NULL.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ *                  See @BlockJobCreateFlags
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @on_error: The action to take upon error.
  * @errp: Error object.
@@ -971,7 +973,8 @@ int is_windows_drive(const char *filename);
  */
 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
-                  int64_t speed, BlockdevOnError on_error, Error **errp);
+                  int creation_flags, int64_t speed,
+                  BlockdevOnError on_error, Error **errp);
 
 /**
  * commit_start:
-- 
2.14.4

[Qemu-devel] [PATCH v5 04/16] block/commit: refactor commit to use job callbacks

[John Snow]

Use the component callbacks; prepare, abort, and clean.

NB: prepare is only called when the job has not yet failed;
and abort can be called after prepare.

complete -> prepare -> abort -> clean
complete -> abort -> clean

During refactor, a potential problem with bdrv_drop_intermediate
was identified, The patched behavior is no worse than the pre-patch
behavior, so leave a FIXME for now to be fixed in a future patch.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/commit.c | 92 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index b6e8969877..a2da5740b0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -36,6 +36,7 @@ typedef struct CommitBlockJob {
     BlockDriverState *commit_top_bs;
     BlockBackend *top;
     BlockBackend *base;
+    BlockDriverState *base_bs;
     BlockdevOnError on_error;
     int base_flags;
     char *backing_file_str;
@@ -68,61 +69,67 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
     return 0;
 }
 
-static void commit_exit(Job *job)
+static int commit_prepare(Job *job)
 {
     CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
-    BlockJob *bjob = &s->common;
-    BlockDriverState *top = blk_bs(s->top);
-    BlockDriverState *base = blk_bs(s->base);
-    BlockDriverState *commit_top_bs = s->commit_top_bs;
-    bool remove_commit_top_bs = false;
-
-    /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
-    bdrv_ref(top);
-    bdrv_ref(commit_top_bs);
 
     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
      * the normal backing chain can be restored. */
     blk_unref(s->base);
+    s->base = NULL;
 
-    if (!job_is_cancelled(job) && job->ret == 0) {
-        /* success */
-        job->ret = bdrv_drop_intermediate(s->commit_top_bs, base,
-                                          s->backing_file_str);
-    } else {
-        /* XXX Can (or should) we somehow keep 'consistent read' blocked even
-         * after the failed/cancelled commit job is gone? If we already wrote
-         * something to base, the intermediate images aren't valid any more. */
-        remove_commit_top_bs = true;
+    /* FIXME: bdrv_drop_intermediate treats total failures and partial failures
+     * identically. Further work is needed to disambiguate these cases. */
+    return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
+                                  s->backing_file_str);
+}
+
+static void commit_abort(Job *job)
+{
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
+    BlockDriverState *top_bs = blk_bs(s->top);
+
+    /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
+    bdrv_ref(top_bs);
+    bdrv_ref(s->commit_top_bs);
+
+    if (s->base) {
+        blk_unref(s->base);
     }
 
+    /* free the blockers on the intermediate nodes so that bdrv_replace_nodes
+     * can succeed */
+    block_job_remove_all_bdrv(&s->common);
+
+    /* If bdrv_drop_intermediate() failed (or was not invoked), remove the
+     * commit filter driver from the backing chain now. Do this as the final
+     * step so that the 'consistent read' permission can be granted.
+     *
+     * XXX Can (or should) we somehow keep 'consistent read' blocked even
+     * after the failed/cancelled commit job is gone? If we already wrote
+     * something to base, the intermediate images aren't valid any more. */
+    bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
+                            &error_abort);
+    bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
+                      &error_abort);
+
+    bdrv_unref(s->commit_top_bs);
+    bdrv_unref(top_bs);
+}
+
+static void commit_clean(Job *job)
+{
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
+
     /* restore base open flags here if appropriate (e.g., change the base back
      * to r/o). These reopens do not need to be atomic, since we won't abort
      * even on failure here */
-    if (s->base_flags != bdrv_get_flags(base)) {
-        bdrv_reopen(base, s->base_flags, NULL);
+    if (s->base_flags != bdrv_get_flags(s->base_bs)) {
+        bdrv_reopen(s->base_bs, s->base_flags, NULL);
     }
+
     g_free(s->backing_file_str);
     blk_unref(s->top);
-
-    /* If there is more than one reference to the job (e.g. if called from
-     * job_finish_sync()), job_completed() won't free it and therefore the
-     * blockers on the intermediate nodes remain. This would cause
-     * bdrv_set_backing_hd() to fail. */
-    block_job_remove_all_bdrv(bjob);
-
-    /* If bdrv_drop_intermediate() didn't already do that, remove the commit
-     * filter driver from the backing chain. Do this as the final step so that
-     * the 'consistent read' permission can be granted.  */
-    if (remove_commit_top_bs) {
-        bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
-                                &error_abort);
-        bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
-                          &error_abort);
-    }
-
-    bdrv_unref(commit_top_bs);
-    bdrv_unref(top);
 }
 
 static int coroutine_fn commit_run(Job *job, Error **errp)
@@ -211,7 +218,9 @@ static const BlockJobDriver commit_job_driver = {
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
         .run           = commit_run,
-        .exit          = commit_exit,
+        .prepare       = commit_prepare,
+        .abort         = commit_abort,
+        .clean         = commit_clean
     },
 };
 
@@ -345,6 +354,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     if (ret < 0) {
         goto fail;
     }
+    s->base_bs = base;
 
     /* Required permissions are already taken with block_job_add_bdrv() */
     s->top = blk_new(0, BLK_PERM_ALL);
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 04/16] block/commit: refactor commit to use job callbacks

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:13AM -0400, John Snow wrote:
> Use the component callbacks; prepare, abort, and clean.
> 
> NB: prepare is only called when the job has not yet failed;
> and abort can be called after prepare.
> 
> complete -> prepare -> abort -> clean
> complete -> abort -> clean
> 
> During refactor, a potential problem with bdrv_drop_intermediate
> was identified, The patched behavior is no worse than the pre-patch
> behavior, so leave a FIXME for now to be fixed in a future patch.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/commit.c | 92 ++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 51 insertions(+), 41 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index b6e8969877..a2da5740b0 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -36,6 +36,7 @@ typedef struct CommitBlockJob {
>      BlockDriverState *commit_top_bs;
>      BlockBackend *top;
>      BlockBackend *base;
> +    BlockDriverState *base_bs;
>      BlockdevOnError on_error;
>      int base_flags;
>      char *backing_file_str;
> @@ -68,61 +69,67 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
>      return 0;
>  }
>  
> -static void commit_exit(Job *job)
> +static int commit_prepare(Job *job)
>  {
>      CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> -    BlockJob *bjob = &s->common;
> -    BlockDriverState *top = blk_bs(s->top);
> -    BlockDriverState *base = blk_bs(s->base);
> -    BlockDriverState *commit_top_bs = s->commit_top_bs;
> -    bool remove_commit_top_bs = false;
> -
> -    /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
> -    bdrv_ref(top);
> -    bdrv_ref(commit_top_bs);
>  
>      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
>       * the normal backing chain can be restored. */
>      blk_unref(s->base);
> +    s->base = NULL;
>  
> -    if (!job_is_cancelled(job) && job->ret == 0) {
> -        /* success */
> -        job->ret = bdrv_drop_intermediate(s->commit_top_bs, base,
> -                                          s->backing_file_str);
> -    } else {
> -        /* XXX Can (or should) we somehow keep 'consistent read' blocked even
> -         * after the failed/cancelled commit job is gone? If we already wrote
> -         * something to base, the intermediate images aren't valid any more. */
> -        remove_commit_top_bs = true;
> +    /* FIXME: bdrv_drop_intermediate treats total failures and partial failures
> +     * identically. Further work is needed to disambiguate these cases. */
> +    return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
> +                                  s->backing_file_str);
> +}
> +
> +static void commit_abort(Job *job)
> +{
> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> +    BlockDriverState *top_bs = blk_bs(s->top);
> +
> +    /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
> +    bdrv_ref(top_bs);
> +    bdrv_ref(s->commit_top_bs);
> +
> +    if (s->base) {
> +        blk_unref(s->base);
>      }
>  
> +    /* free the blockers on the intermediate nodes so that bdrv_replace_nodes
> +     * can succeed */
> +    block_job_remove_all_bdrv(&s->common);
> +
> +    /* If bdrv_drop_intermediate() failed (or was not invoked), remove the
> +     * commit filter driver from the backing chain now. Do this as the final
> +     * step so that the 'consistent read' permission can be granted.
> +     *
> +     * XXX Can (or should) we somehow keep 'consistent read' blocked even
> +     * after the failed/cancelled commit job is gone? If we already wrote
> +     * something to base, the intermediate images aren't valid any more. */
> +    bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
> +                            &error_abort);
> +    bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
> +                      &error_abort);
> +
> +    bdrv_unref(s->commit_top_bs);
> +    bdrv_unref(top_bs);
> +}
> +
> +static void commit_clean(Job *job)
> +{
> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> +
>      /* restore base open flags here if appropriate (e.g., change the base back
>       * to r/o). These reopens do not need to be atomic, since we won't abort
>       * even on failure here */
> -    if (s->base_flags != bdrv_get_flags(base)) {
> -        bdrv_reopen(base, s->base_flags, NULL);
> +    if (s->base_flags != bdrv_get_flags(s->base_bs)) {
> +        bdrv_reopen(s->base_bs, s->base_flags, NULL);
>      }
> +
>      g_free(s->backing_file_str);
>      blk_unref(s->top);
> -
> -    /* If there is more than one reference to the job (e.g. if called from
> -     * job_finish_sync()), job_completed() won't free it and therefore the
> -     * blockers on the intermediate nodes remain. This would cause
> -     * bdrv_set_backing_hd() to fail. */
> -    block_job_remove_all_bdrv(bjob);
> -
> -    /* If bdrv_drop_intermediate() didn't already do that, remove the commit
> -     * filter driver from the backing chain. Do this as the final step so that
> -     * the 'consistent read' permission can be granted.  */
> -    if (remove_commit_top_bs) {
> -        bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
> -                                &error_abort);
> -        bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
> -                          &error_abort);
> -    }
> -
> -    bdrv_unref(commit_top_bs);
> -    bdrv_unref(top);
>  }
>  
>  static int coroutine_fn commit_run(Job *job, Error **errp)
> @@ -211,7 +218,9 @@ static const BlockJobDriver commit_job_driver = {
>          .user_resume   = block_job_user_resume,
>          .drain         = block_job_drain,
>          .run           = commit_run,
> -        .exit          = commit_exit,
> +        .prepare       = commit_prepare,
> +        .abort         = commit_abort,
> +        .clean         = commit_clean
>      },
>  };
>  
> @@ -345,6 +354,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
>      if (ret < 0) {
>          goto fail;
>      }
> +    s->base_bs = base;
>  
>      /* Required permissions are already taken with block_job_add_bdrv() */
>      s->top = blk_new(0, BLK_PERM_ALL);
> -- 
> 2.14.4
> 

Re: [Qemu-devel] [PATCH v5 04/16] block/commit: refactor commit to use job callbacks

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On 2018-09-06 15:02, John Snow wrote:
> Use the component callbacks; prepare, abort, and clean.
> 
> NB: prepare is only called when the job has not yet failed;
> and abort can be called after prepare.
> 
> complete -> prepare -> abort -> clean
> complete -> abort -> clean
> 
> During refactor, a potential problem with bdrv_drop_intermediate
> was identified, The patched behavior is no worse than the pre-patch

(s/T/t/)

(Max)

> behavior, so leave a FIXME for now to be fixed in a future patch.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/commit.c | 92 ++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 51 insertions(+), 41 deletions(-)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v5 05/16] block/mirror: don't install backing chain on abort

[John Snow]

In cases where we abort the block/mirror job, there's no point in
installing the new backing chain before we finish aborting.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index cba555b4ef..bd3e908710 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -642,7 +642,7 @@ static void mirror_exit(Job *job)
      * required before it could become a backing file of target_bs. */
     bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
                             &error_abort);
-    if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
+    if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
         BlockDriverState *backing = s->is_none_mode ? src : s->base;
         if (backing_bs(target_bs) != backing) {
             bdrv_set_backing_hd(target_bs, backing, &local_err);
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 05/16] block/mirror: don't install backing chain on abort

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:14AM -0400, John Snow wrote:
> In cases where we abort the block/mirror job, there's no point in
> installing the new backing chain before we finish aborting.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index cba555b4ef..bd3e908710 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -642,7 +642,7 @@ static void mirror_exit(Job *job)
>       * required before it could become a backing file of target_bs. */
>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>                              &error_abort);
> -    if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> +    if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>          BlockDriverState *backing = s->is_none_mode ? src : s->base;
>          if (backing_bs(target_bs) != backing) {
>              bdrv_set_backing_hd(target_bs, backing, &local_err);
> -- 
> 2.14.4
> 

Re: [Qemu-devel] [PATCH v5 05/16] block/mirror: don't install backing chain on abort

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

On 2018-09-06 15:02, John Snow wrote:
> In cases where we abort the block/mirror job, there's no point in
> installing the new backing chain before we finish aborting.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v5 06/16] block/mirror: conservative mirror_exit refactor

[John Snow]

For purposes of minimum code movement, refactor the mirror_exit
callback to use the post-finalization callbacks in a trivial way.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index bd3e908710..a92b4702c5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
     int max_iov;
     bool initial_zeroing_ongoing;
     int in_active_write_counter;
+    bool prepared;
 } MirrorBlockJob;
 
 typedef struct MirrorBDSOpaque {
@@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
     }
 }
 
-static void mirror_exit(Job *job)
+static int mirror_exit_common(Job *job)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     BlockJob *bjob = &s->common;
@@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
     BlockDriverState *target_bs = blk_bs(s->target);
     BlockDriverState *mirror_top_bs = s->mirror_top_bs;
     Error *local_err = NULL;
-    int ret = job->ret;
+    bool abort = job->ret < 0;
+    int ret = 0;
+
+    if (s->prepared) {
+        return 0;
+    }
+    s->prepared = true;
 
     bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
 
@@ -642,7 +649,7 @@ static void mirror_exit(Job *job)
      * required before it could become a backing file of target_bs. */
     bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
                             &error_abort);
-    if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
+    if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
         BlockDriverState *backing = s->is_none_mode ? src : s->base;
         if (backing_bs(target_bs) != backing) {
             bdrv_set_backing_hd(target_bs, backing, &local_err);
@@ -658,11 +665,8 @@ static void mirror_exit(Job *job)
         aio_context_acquire(replace_aio_context);
     }
 
-    if (s->should_complete && ret == 0) {
-        BlockDriverState *to_replace = src;
-        if (s->to_replace) {
-            to_replace = s->to_replace;
-        }
+    if (s->should_complete && !abort) {
+        BlockDriverState *to_replace = s->to_replace ?: src;
 
         if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
             bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
@@ -711,7 +715,18 @@ static void mirror_exit(Job *job)
     bdrv_unref(mirror_top_bs);
     bdrv_unref(src);
 
-    job->ret = ret;
+    return ret;
+}
+
+static int mirror_prepare(Job *job)
+{
+    return mirror_exit_common(job);
+}
+
+static void mirror_abort(Job *job)
+{
+    int ret = mirror_exit_common(job);
+    assert(ret == 0);
 }
 
 static void mirror_throttle(MirrorBlockJob *s)
@@ -1132,7 +1147,8 @@ static const BlockJobDriver mirror_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .run                    = mirror_run,
-        .exit                   = mirror_exit,
+        .prepare                = mirror_prepare,
+        .abort                  = mirror_abort,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
@@ -1149,7 +1165,8 @@ static const BlockJobDriver commit_active_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .run                    = mirror_run,
-        .exit                   = mirror_exit,
+        .prepare                = mirror_prepare,
+        .abort                  = mirror_abort,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 06/16] block/mirror: conservative mirror_exit refactor

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:15AM -0400, John Snow wrote:
> For purposes of minimum code movement, refactor the mirror_exit
> callback to use the post-finalization callbacks in a trivial way.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index bd3e908710..a92b4702c5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
>      int max_iov;
>      bool initial_zeroing_ongoing;
>      int in_active_write_counter;
> +    bool prepared;
>  } MirrorBlockJob;
>  
>  typedef struct MirrorBDSOpaque {
> @@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
>      }
>  }
>  
> -static void mirror_exit(Job *job)
> +static int mirror_exit_common(Job *job)
>  {
>      MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>      BlockJob *bjob = &s->common;
> @@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
>      BlockDriverState *target_bs = blk_bs(s->target);
>      BlockDriverState *mirror_top_bs = s->mirror_top_bs;
>      Error *local_err = NULL;
> -    int ret = job->ret;
> +    bool abort = job->ret < 0;
> +    int ret = 0;
> +
> +    if (s->prepared) {
> +        return 0;
> +    }
> +    s->prepared = true;
>  
>      bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>  
> @@ -642,7 +649,7 @@ static void mirror_exit(Job *job)
>       * required before it could become a backing file of target_bs. */
>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>                              &error_abort);
> -    if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> +    if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>          BlockDriverState *backing = s->is_none_mode ? src : s->base;
>          if (backing_bs(target_bs) != backing) {
>              bdrv_set_backing_hd(target_bs, backing, &local_err);
> @@ -658,11 +665,8 @@ static void mirror_exit(Job *job)
>          aio_context_acquire(replace_aio_context);
>      }
>  
> -    if (s->should_complete && ret == 0) {
> -        BlockDriverState *to_replace = src;
> -        if (s->to_replace) {
> -            to_replace = s->to_replace;
> -        }
> +    if (s->should_complete && !abort) {
> +        BlockDriverState *to_replace = s->to_replace ?: src;
>  
>          if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
>              bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
> @@ -711,7 +715,18 @@ static void mirror_exit(Job *job)
>      bdrv_unref(mirror_top_bs);
>      bdrv_unref(src);
>  
> -    job->ret = ret;
> +    return ret;
> +}
> +
> +static int mirror_prepare(Job *job)
> +{
> +    return mirror_exit_common(job);
> +}
> +
> +static void mirror_abort(Job *job)
> +{
> +    int ret = mirror_exit_common(job);
> +    assert(ret == 0);

Not something to hold the series up, but in case a v6 is called for due to
other changes: I think it may be worth a comment in mirror_exit_common()
that if abort is true, and we don't return success, QEMU will hit an assert.
Mainly to prevent someone from including a call with a potential error
return in the abort path in the future.

Reviewed-by: Jeff Cody <jcody@redhat.com>


>  }
>  
>  static void mirror_throttle(MirrorBlockJob *s)
> @@ -1132,7 +1147,8 @@ static const BlockJobDriver mirror_job_driver = {
>          .user_resume            = block_job_user_resume,
>          .drain                  = block_job_drain,
>          .run                    = mirror_run,
> -        .exit                   = mirror_exit,
> +        .prepare                = mirror_prepare,
> +        .abort                  = mirror_abort,
>          .pause                  = mirror_pause,
>          .complete               = mirror_complete,
>      },
> @@ -1149,7 +1165,8 @@ static const BlockJobDriver commit_active_job_driver = {
>          .user_resume            = block_job_user_resume,
>          .drain                  = block_job_drain,
>          .run                    = mirror_run,
> -        .exit                   = mirror_exit,
> +        .prepare                = mirror_prepare,
> +        .abort                  = mirror_abort,
>          .pause                  = mirror_pause,
>          .complete               = mirror_complete,
>      },
> -- 
> 2.14.4
> 

Re: [Qemu-devel] [PATCH v5 06/16] block/mirror: conservative mirror_exit refactor

[John Snow]

On 09/06/2018 12:57 PM, Jeff Cody wrote:
> On Thu, Sep 06, 2018 at 09:02:15AM -0400, John Snow wrote:
>> For purposes of minimum code movement, refactor the mirror_exit
>> callback to use the post-finalization callbacks in a trivial way.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/mirror.c | 39 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index bd3e908710..a92b4702c5 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
>>      int max_iov;
>>      bool initial_zeroing_ongoing;
>>      int in_active_write_counter;
>> +    bool prepared;
>>  } MirrorBlockJob;
>>  
>>  typedef struct MirrorBDSOpaque {
>> @@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
>>      }
>>  }
>>  
>> -static void mirror_exit(Job *job)

/**
 * mirror_exit_common: handle both abort() and prepare() cases.
 * for .prepare, returns 0 on success and -errno on failure.
 * for .abort cases, denoted by abort = true, MUST return 0.
 */

>> +static int mirror_exit_common(Job *job)
>>  {
>>      MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>>      BlockJob *bjob = &s->common;
>> @@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
>>      BlockDriverState *target_bs = blk_bs(s->target);
>>      BlockDriverState *mirror_top_bs = s->mirror_top_bs;
>>      Error *local_err = NULL;
>> -    int ret = job->ret;
>> +    bool abort = job->ret < 0;
>> +    int ret = 0;
>> +
>> +    if (s->prepared) {
>> +        return 0;
>> +    }
>> +    s->prepared = true;
>>  
>>      bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>>  
>> @@ -642,7 +649,7 @@ static void mirror_exit(Job *job)
>>       * required before it could become a backing file of target_bs. */
>>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>>                              &error_abort);
>> -    if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>> +    if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>>          BlockDriverState *backing = s->is_none_mode ? src : s->base;
>>          if (backing_bs(target_bs) != backing) {
>>              bdrv_set_backing_hd(target_bs, backing, &local_err);
>> @@ -658,11 +665,8 @@ static void mirror_exit(Job *job)
>>          aio_context_acquire(replace_aio_context);
>>      }
>>  
>> -    if (s->should_complete && ret == 0) {
>> -        BlockDriverState *to_replace = src;
>> -        if (s->to_replace) {
>> -            to_replace = s->to_replace;
>> -        }
>> +    if (s->should_complete && !abort) {
>> +        BlockDriverState *to_replace = s->to_replace ?: src;
>>  
>>          if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
>>              bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
>> @@ -711,7 +715,18 @@ static void mirror_exit(Job *job)
>>      bdrv_unref(mirror_top_bs);
>>      bdrv_unref(src);
>>  
>> -    job->ret = ret;
>> +    return ret;
>> +}
>> +
>> +static int mirror_prepare(Job *job)
>> +{
>> +    return mirror_exit_common(job);
>> +}
>> +
>> +static void mirror_abort(Job *job)
>> +{
>> +    int ret = mirror_exit_common(job);
>> +    assert(ret == 0);
> 
> Not something to hold the series up, but in case a v6 is called for due to
> other changes: I think it may be worth a comment in mirror_exit_common()
> that if abort is true, and we don't return success, QEMU will hit an assert.
> Mainly to prevent someone from including a call with a potential error
> return in the abort path in the future.
> 
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> 

Yeah, I should have added a comment, like the one you read before you
got to this comment, by the very nature of how email works.

--js

Re: [Qemu-devel] [PATCH v5 06/16] block/mirror: conservative mirror_exit refactor

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

On 2018-09-06 22:31, John Snow wrote:
> 
> 
> On 09/06/2018 12:57 PM, Jeff Cody wrote:
>> On Thu, Sep 06, 2018 at 09:02:15AM -0400, John Snow wrote:
>>> For purposes of minimum code movement, refactor the mirror_exit
>>> callback to use the post-finalization callbacks in a trivial way.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  block/mirror.c | 39 ++++++++++++++++++++++++++++-----------
>>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index bd3e908710..a92b4702c5 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
>>>      int max_iov;
>>>      bool initial_zeroing_ongoing;
>>>      int in_active_write_counter;
>>> +    bool prepared;
>>>  } MirrorBlockJob;
>>>  
>>>  typedef struct MirrorBDSOpaque {
>>> @@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
>>>      }
>>>  }
>>>  
>>> -static void mirror_exit(Job *job)
> 
> /**
>  * mirror_exit_common: handle both abort() and prepare() cases.
>  * for .prepare, returns 0 on success and -errno on failure.
>  * for .abort cases, denoted by abort = true, MUST return 0.
>  */

Any case:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v5 07/16] block/stream: refactor stream to use job callbacks

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/stream.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 700eb239e4..81a7ec8ece 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -54,16 +54,16 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
     return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
 }
 
-static void stream_exit(Job *job)
+static int stream_prepare(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockJob *bjob = &s->common;
     BlockDriverState *bs = blk_bs(bjob->blk);
     BlockDriverState *base = s->base;
     Error *local_err = NULL;
-    int ret = job->ret;
+    int ret = 0;
 
-    if (!job_is_cancelled(job) && bs->backing && ret == 0) {
+    if (bs->backing) {
         const char *base_id = NULL, *base_fmt = NULL;
         if (base) {
             base_id = s->backing_file_str;
@@ -75,12 +75,19 @@ static void stream_exit(Job *job)
         bdrv_set_backing_hd(bs, base, &local_err);
         if (local_err) {
             error_report_err(local_err);
-            ret = -EPERM;
-            goto out;
+            return -EPERM;
         }
     }
 
-out:
+    return ret;
+}
+
+static void stream_clean(Job *job)
+{
+    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
+    BlockJob *bjob = &s->common;
+    BlockDriverState *bs = blk_bs(bjob->blk);
+
     /* Reopen the image back in read-only mode if necessary */
     if (s->bs_flags != bdrv_get_flags(bs)) {
         /* Give up write permissions before making it read-only */
@@ -89,7 +96,6 @@ out:
     }
 
     g_free(s->backing_file_str);
-    job->ret = ret;
 }
 
 static int coroutine_fn stream_run(Job *job, Error **errp)
@@ -206,7 +212,8 @@ static const BlockJobDriver stream_job_driver = {
         .job_type      = JOB_TYPE_STREAM,
         .free          = block_job_free,
         .run           = stream_run,
-        .exit          = stream_exit,
+        .prepare       = stream_prepare,
+        .clean         = stream_clean,
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
     },
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 07/16] block/stream: refactor stream to use job callbacks

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:16AM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/stream.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 700eb239e4..81a7ec8ece 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -54,16 +54,16 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
>      return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
>  }
>  
> -static void stream_exit(Job *job)
> +static int stream_prepare(Job *job)
>  {
>      StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>      BlockJob *bjob = &s->common;
>      BlockDriverState *bs = blk_bs(bjob->blk);
>      BlockDriverState *base = s->base;
>      Error *local_err = NULL;
> -    int ret = job->ret;
> +    int ret = 0;
>  
> -    if (!job_is_cancelled(job) && bs->backing && ret == 0) {
> +    if (bs->backing) {
>          const char *base_id = NULL, *base_fmt = NULL;
>          if (base) {
>              base_id = s->backing_file_str;
> @@ -75,12 +75,19 @@ static void stream_exit(Job *job)
>          bdrv_set_backing_hd(bs, base, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
> -            ret = -EPERM;
> -            goto out;
> +            return -EPERM;
>          }
>      }
>  
> -out:
> +    return ret;
> +}
> +
> +static void stream_clean(Job *job)
> +{
> +    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> +    BlockJob *bjob = &s->common;
> +    BlockDriverState *bs = blk_bs(bjob->blk);
> +
>      /* Reopen the image back in read-only mode if necessary */
>      if (s->bs_flags != bdrv_get_flags(bs)) {
>          /* Give up write permissions before making it read-only */
> @@ -89,7 +96,6 @@ out:
>      }
>  
>      g_free(s->backing_file_str);
> -    job->ret = ret;
>  }
>  
>  static int coroutine_fn stream_run(Job *job, Error **errp)
> @@ -206,7 +212,8 @@ static const BlockJobDriver stream_job_driver = {
>          .job_type      = JOB_TYPE_STREAM,
>          .free          = block_job_free,
>          .run           = stream_run,
> -        .exit          = stream_exit,
> +        .prepare       = stream_prepare,
> +        .clean         = stream_clean,
>          .user_resume   = block_job_user_resume,
>          .drain         = block_job_drain,
>      },
> -- 
> 2.14.4
> 

[Qemu-devel] [PATCH v5 08/16] tests/blockjob: replace Blockjob with Job

[John Snow]

These tests don't actually test blockjobs anymore, they test
generic Job lifetimes. Change the types accordingly.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/test-blockjob.c | 98 ++++++++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 48 deletions(-)

diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index ad4a65bc78..8e8b680416 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -206,18 +206,20 @@ static const BlockJobDriver test_cancel_driver = {
     },
 };
 
-static CancelJob *create_common(BlockJob **pjob)
+static CancelJob *create_common(Job **pjob)
 {
     BlockBackend *blk;
-    BlockJob *job;
+    Job *job;
+    BlockJob *bjob;
     CancelJob *s;
 
     blk = create_blk(NULL);
-    job = mk_job(blk, "Steve", &test_cancel_driver, true,
-                 JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
-    job_ref(&job->job);
-    assert(job->job.status == JOB_STATUS_CREATED);
-    s = container_of(job, CancelJob, common);
+    bjob = mk_job(blk, "Steve", &test_cancel_driver, true,
+                  JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
+    job = &bjob->job;
+    job_ref(job);
+    assert(job->status == JOB_STATUS_CREATED);
+    s = container_of(bjob, CancelJob, common);
     s->blk = blk;
 
     *pjob = job;
@@ -242,7 +244,7 @@ static void cancel_common(CancelJob *s)
 
 static void test_cancel_created(void)
 {
-    BlockJob *job;
+    Job *job;
     CancelJob *s;
 
     s = create_common(&job);
@@ -251,119 +253,119 @@ static void test_cancel_created(void)
 
 static void test_cancel_running(void)
 {
-    BlockJob *job;
+    Job *job;
     CancelJob *s;
 
     s = create_common(&job);
 
-    job_start(&job->job);
-    assert(job->job.status == JOB_STATUS_RUNNING);
+    job_start(job);
+    assert(job->status == JOB_STATUS_RUNNING);
 
     cancel_common(s);
 }
 
 static void test_cancel_paused(void)
 {
-    BlockJob *job;
+    Job *job;
     CancelJob *s;
 
     s = create_common(&job);
 
-    job_start(&job->job);
-    assert(job->job.status == JOB_STATUS_RUNNING);
+    job_start(job);
+    assert(job->status == JOB_STATUS_RUNNING);
 
-    job_user_pause(&job->job, &error_abort);
-    job_enter(&job->job);
-    assert(job->job.status == JOB_STATUS_PAUSED);
+    job_user_pause(job, &error_abort);
+    job_enter(job);
+    assert(job->status == JOB_STATUS_PAUSED);
 
     cancel_common(s);
 }
 
 static void test_cancel_ready(void)
 {
-    BlockJob *job;
+    Job *job;
     CancelJob *s;
 
     s = create_common(&job);
 
-    job_start(&job->job);
-    assert(job->job.status == JOB_STATUS_RUNNING);
+    job_start(job);
+    assert(job->status == JOB_STATUS_RUNNING);
 
     s->should_converge = true;
-    job_enter(&job->job);
-    assert(job->job.status == JOB_STATUS_READY);
+    job_enter(job);
+    assert(job->status == JOB_STATUS_READY);
 
     cancel_common(s);
 }
 
 static void test_cancel_standby(void)
 {
-    BlockJob *job;
+    Job *job;
     CancelJob *s;
 
     s = create_common(&job);
 
-    job_start(&job->job);
-    assert(job->job.status == JOB_STATUS_RUNNING);
+    job_start(job);
+    assert(job->status == JOB_STATUS_RUNNING);
 
     s->should_converge = true;
-    job_enter(&job->job);
-    assert(job->job.status == JOB_STATUS_READY);
+    job_enter(job);
+    assert(job->status == JOB_STATUS_READY);
 
-    job_user_pause(&job->job, &error_abort);
-    job_enter(&job->job);
-    assert(job->job.status == JOB_STATUS_STANDBY);
+    job_user_pause(job, &error_abort);
+    job_enter(job);
+    assert(job->status == JOB_STATUS_STANDBY);
 
     cancel_common(s);
 }
 
 static void test_cancel_pending(void)
 {
-    BlockJob *job;
+    Job *job;
     CancelJob *s;
 
     s = create_common(&job);
 
-    job_start(&job->job);
-    assert(job->job.status == JOB_STATUS_RUNNING);
+    job_start(job);
+    assert(job->status == JOB_STATUS_RUNNING);
 
     s->should_converge = true;
-    job_enter(&job->job);
-    assert(job->job.status == JOB_STATUS_READY);
+    job_enter(job);
+    assert(job->status == JOB_STATUS_READY);
 
-    job_complete(&job->job, &error_abort);
-    job_enter(&job->job);
+    job_complete(job, &error_abort);
+    job_enter(job);
     while (!s->completed) {
         aio_poll(qemu_get_aio_context(), true);
     }
-    assert(job->job.status == JOB_STATUS_PENDING);
+    assert(job->status == JOB_STATUS_PENDING);
 
     cancel_common(s);
 }
 
 static void test_cancel_concluded(void)
 {
-    BlockJob *job;
+    Job *job;
     CancelJob *s;
 
     s = create_common(&job);
 
-    job_start(&job->job);
-    assert(job->job.status == JOB_STATUS_RUNNING);
+    job_start(job);
+    assert(job->status == JOB_STATUS_RUNNING);
 
     s->should_converge = true;
-    job_enter(&job->job);
-    assert(job->job.status == JOB_STATUS_READY);
+    job_enter(job);
+    assert(job->status == JOB_STATUS_READY);
 
-    job_complete(&job->job, &error_abort);
-    job_enter(&job->job);
+    job_complete(job, &error_abort);
+    job_enter(job);
     while (!s->completed) {
         aio_poll(qemu_get_aio_context(), true);
     }
-    assert(job->job.status == JOB_STATUS_PENDING);
+    assert(job->status == JOB_STATUS_PENDING);
 
-    job_finalize(&job->job, &error_abort);
-    assert(job->job.status == JOB_STATUS_CONCLUDED);
+    job_finalize(job, &error_abort);
+    assert(job->status == JOB_STATUS_CONCLUDED);
 
     cancel_common(s);
 }
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 08/16] tests/blockjob: replace Blockjob with Job

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:17AM -0400, John Snow wrote:
> These tests don't actually test blockjobs anymore, they test
> generic Job lifetimes. Change the types accordingly.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  tests/test-blockjob.c | 98 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 50 insertions(+), 48 deletions(-)
> 
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index ad4a65bc78..8e8b680416 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -206,18 +206,20 @@ static const BlockJobDriver test_cancel_driver = {
>      },
>  };
>  
> -static CancelJob *create_common(BlockJob **pjob)
> +static CancelJob *create_common(Job **pjob)
>  {
>      BlockBackend *blk;
> -    BlockJob *job;
> +    Job *job;
> +    BlockJob *bjob;
>      CancelJob *s;
>  
>      blk = create_blk(NULL);
> -    job = mk_job(blk, "Steve", &test_cancel_driver, true,
> -                 JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
> -    job_ref(&job->job);
> -    assert(job->job.status == JOB_STATUS_CREATED);
> -    s = container_of(job, CancelJob, common);
> +    bjob = mk_job(blk, "Steve", &test_cancel_driver, true,
> +                  JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
> +    job = &bjob->job;
> +    job_ref(job);
> +    assert(job->status == JOB_STATUS_CREATED);
> +    s = container_of(bjob, CancelJob, common);
>      s->blk = blk;
>  
>      *pjob = job;
> @@ -242,7 +244,7 @@ static void cancel_common(CancelJob *s)
>  
>  static void test_cancel_created(void)
>  {
> -    BlockJob *job;
> +    Job *job;
>      CancelJob *s;
>  
>      s = create_common(&job);
> @@ -251,119 +253,119 @@ static void test_cancel_created(void)
>  
>  static void test_cancel_running(void)
>  {
> -    BlockJob *job;
> +    Job *job;
>      CancelJob *s;
>  
>      s = create_common(&job);
>  
> -    job_start(&job->job);
> -    assert(job->job.status == JOB_STATUS_RUNNING);
> +    job_start(job);
> +    assert(job->status == JOB_STATUS_RUNNING);
>  
>      cancel_common(s);
>  }
>  
>  static void test_cancel_paused(void)
>  {
> -    BlockJob *job;
> +    Job *job;
>      CancelJob *s;
>  
>      s = create_common(&job);
>  
> -    job_start(&job->job);
> -    assert(job->job.status == JOB_STATUS_RUNNING);
> +    job_start(job);
> +    assert(job->status == JOB_STATUS_RUNNING);
>  
> -    job_user_pause(&job->job, &error_abort);
> -    job_enter(&job->job);
> -    assert(job->job.status == JOB_STATUS_PAUSED);
> +    job_user_pause(job, &error_abort);
> +    job_enter(job);
> +    assert(job->status == JOB_STATUS_PAUSED);
>  
>      cancel_common(s);
>  }
>  
>  static void test_cancel_ready(void)
>  {
> -    BlockJob *job;
> +    Job *job;
>      CancelJob *s;
>  
>      s = create_common(&job);
>  
> -    job_start(&job->job);
> -    assert(job->job.status == JOB_STATUS_RUNNING);
> +    job_start(job);
> +    assert(job->status == JOB_STATUS_RUNNING);
>  
>      s->should_converge = true;
> -    job_enter(&job->job);
> -    assert(job->job.status == JOB_STATUS_READY);
> +    job_enter(job);
> +    assert(job->status == JOB_STATUS_READY);
>  
>      cancel_common(s);
>  }
>  
>  static void test_cancel_standby(void)
>  {
> -    BlockJob *job;
> +    Job *job;
>      CancelJob *s;
>  
>      s = create_common(&job);
>  
> -    job_start(&job->job);
> -    assert(job->job.status == JOB_STATUS_RUNNING);
> +    job_start(job);
> +    assert(job->status == JOB_STATUS_RUNNING);
>  
>      s->should_converge = true;
> -    job_enter(&job->job);
> -    assert(job->job.status == JOB_STATUS_READY);
> +    job_enter(job);
> +    assert(job->status == JOB_STATUS_READY);
>  
> -    job_user_pause(&job->job, &error_abort);
> -    job_enter(&job->job);
> -    assert(job->job.status == JOB_STATUS_STANDBY);
> +    job_user_pause(job, &error_abort);
> +    job_enter(job);
> +    assert(job->status == JOB_STATUS_STANDBY);
>  
>      cancel_common(s);
>  }
>  
>  static void test_cancel_pending(void)
>  {
> -    BlockJob *job;
> +    Job *job;
>      CancelJob *s;
>  
>      s = create_common(&job);
>  
> -    job_start(&job->job);
> -    assert(job->job.status == JOB_STATUS_RUNNING);
> +    job_start(job);
> +    assert(job->status == JOB_STATUS_RUNNING);
>  
>      s->should_converge = true;
> -    job_enter(&job->job);
> -    assert(job->job.status == JOB_STATUS_READY);
> +    job_enter(job);
> +    assert(job->status == JOB_STATUS_READY);
>  
> -    job_complete(&job->job, &error_abort);
> -    job_enter(&job->job);
> +    job_complete(job, &error_abort);
> +    job_enter(job);
>      while (!s->completed) {
>          aio_poll(qemu_get_aio_context(), true);
>      }
> -    assert(job->job.status == JOB_STATUS_PENDING);
> +    assert(job->status == JOB_STATUS_PENDING);
>  
>      cancel_common(s);
>  }
>  
>  static void test_cancel_concluded(void)
>  {
> -    BlockJob *job;
> +    Job *job;
>      CancelJob *s;
>  
>      s = create_common(&job);
>  
> -    job_start(&job->job);
> -    assert(job->job.status == JOB_STATUS_RUNNING);
> +    job_start(job);
> +    assert(job->status == JOB_STATUS_RUNNING);
>  
>      s->should_converge = true;
> -    job_enter(&job->job);
> -    assert(job->job.status == JOB_STATUS_READY);
> +    job_enter(job);
> +    assert(job->status == JOB_STATUS_READY);
>  
> -    job_complete(&job->job, &error_abort);
> -    job_enter(&job->job);
> +    job_complete(job, &error_abort);
> +    job_enter(job);
>      while (!s->completed) {
>          aio_poll(qemu_get_aio_context(), true);
>      }
> -    assert(job->job.status == JOB_STATUS_PENDING);
> +    assert(job->status == JOB_STATUS_PENDING);
>  
> -    job_finalize(&job->job, &error_abort);
> -    assert(job->job.status == JOB_STATUS_CONCLUDED);
> +    job_finalize(job, &error_abort);
> +    assert(job->status == JOB_STATUS_CONCLUDED);
>  
>      cancel_common(s);
>  }
> -- 
> 2.14.4
> 

[Qemu-devel] [PATCH v5 09/16] tests/test-blockjob: remove exit callback

[John Snow]

We remove the exit callback and the completed boolean along with it.
We can simulate it just fine by waiting for the job to defer to the
main loop, and then giving it one final kick to get the main loop
portion to run.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/test-blockjob.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 8e8b680416..de4c1c20aa 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -160,15 +160,8 @@ typedef struct CancelJob {
     BlockBackend *blk;
     bool should_converge;
     bool should_complete;
-    bool completed;
 } CancelJob;
 
-static void cancel_job_exit(Job *job)
-{
-    CancelJob *s = container_of(job, CancelJob, common.job);
-    s->completed = true;
-}
-
 static void cancel_job_complete(Job *job, Error **errp)
 {
     CancelJob *s = container_of(job, CancelJob, common.job);
@@ -201,7 +194,6 @@ static const BlockJobDriver test_cancel_driver = {
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
         .run           = cancel_job_run,
-        .exit          = cancel_job_exit,
         .complete      = cancel_job_complete,
     },
 };
@@ -335,9 +327,11 @@ static void test_cancel_pending(void)
 
     job_complete(job, &error_abort);
     job_enter(job);
-    while (!s->completed) {
+    while (!job->deferred_to_main_loop) {
         aio_poll(qemu_get_aio_context(), true);
     }
+    assert(job->status == JOB_STATUS_READY);
+    aio_poll(qemu_get_aio_context(), true);
     assert(job->status == JOB_STATUS_PENDING);
 
     cancel_common(s);
@@ -359,9 +353,11 @@ static void test_cancel_concluded(void)
 
     job_complete(job, &error_abort);
     job_enter(job);
-    while (!s->completed) {
+    while (!job->deferred_to_main_loop) {
         aio_poll(qemu_get_aio_context(), true);
     }
+    assert(job->status == JOB_STATUS_READY);
+    aio_poll(qemu_get_aio_context(), true);
     assert(job->status == JOB_STATUS_PENDING);
 
     job_finalize(job, &error_abort);
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 09/16] tests/test-blockjob: remove exit callback

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:18AM -0400, John Snow wrote:
> We remove the exit callback and the completed boolean along with it.
> We can simulate it just fine by waiting for the job to defer to the
> main loop, and then giving it one final kick to get the main loop
> portion to run.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  tests/test-blockjob.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 8e8b680416..de4c1c20aa 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -160,15 +160,8 @@ typedef struct CancelJob {
>      BlockBackend *blk;
>      bool should_converge;
>      bool should_complete;
> -    bool completed;
>  } CancelJob;
>  
> -static void cancel_job_exit(Job *job)
> -{
> -    CancelJob *s = container_of(job, CancelJob, common.job);
> -    s->completed = true;
> -}
> -
>  static void cancel_job_complete(Job *job, Error **errp)
>  {
>      CancelJob *s = container_of(job, CancelJob, common.job);
> @@ -201,7 +194,6 @@ static const BlockJobDriver test_cancel_driver = {
>          .user_resume   = block_job_user_resume,
>          .drain         = block_job_drain,
>          .run           = cancel_job_run,
> -        .exit          = cancel_job_exit,
>          .complete      = cancel_job_complete,
>      },
>  };
> @@ -335,9 +327,11 @@ static void test_cancel_pending(void)
>  
>      job_complete(job, &error_abort);
>      job_enter(job);
> -    while (!s->completed) {
> +    while (!job->deferred_to_main_loop) {
>          aio_poll(qemu_get_aio_context(), true);
>      }
> +    assert(job->status == JOB_STATUS_READY);
> +    aio_poll(qemu_get_aio_context(), true);
>      assert(job->status == JOB_STATUS_PENDING);
>  
>      cancel_common(s);
> @@ -359,9 +353,11 @@ static void test_cancel_concluded(void)
>  
>      job_complete(job, &error_abort);
>      job_enter(job);
> -    while (!s->completed) {
> +    while (!job->deferred_to_main_loop) {
>          aio_poll(qemu_get_aio_context(), true);
>      }
> +    assert(job->status == JOB_STATUS_READY);
> +    aio_poll(qemu_get_aio_context(), true);
>      assert(job->status == JOB_STATUS_PENDING);
>  
>      job_finalize(job, &error_abort);
> -- 
> 2.14.4
> 

[Qemu-devel] [PATCH v5 10/16] tests/test-blockjob-txn: move .exit to .clean

[John Snow]

The exit callback in this test actually only performs cleanup.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/test-blockjob-txn.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index ef29f35e44..86606f92b3 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -24,7 +24,7 @@ typedef struct {
     int *result;
 } TestBlockJob;
 
-static void test_block_job_exit(Job *job)
+static void test_block_job_clean(Job *job)
 {
     BlockJob *bjob = container_of(job, BlockJob, job);
     BlockDriverState *bs = blk_bs(bjob->blk);
@@ -73,7 +73,7 @@ static const BlockJobDriver test_block_job_driver = {
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
         .run           = test_block_job_run,
-        .exit          = test_block_job_exit,
+        .clean         = test_block_job_clean,
     },
 };
 
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 10/16] tests/test-blockjob-txn: move .exit to .clean

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:19AM -0400, John Snow wrote:
> The exit callback in this test actually only performs cleanup.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  tests/test-blockjob-txn.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index ef29f35e44..86606f92b3 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -24,7 +24,7 @@ typedef struct {
>      int *result;
>  } TestBlockJob;
>  
> -static void test_block_job_exit(Job *job)
> +static void test_block_job_clean(Job *job)
>  {
>      BlockJob *bjob = container_of(job, BlockJob, job);
>      BlockDriverState *bs = blk_bs(bjob->blk);
> @@ -73,7 +73,7 @@ static const BlockJobDriver test_block_job_driver = {
>          .user_resume   = block_job_user_resume,
>          .drain         = block_job_drain,
>          .run           = test_block_job_run,
> -        .exit          = test_block_job_exit,
> +        .clean         = test_block_job_clean,
>      },
>  };
>  
> -- 
> 2.14.4
> 

[Qemu-devel] [PATCH v5 11/16] jobs: remove .exit callback

[John Snow]

Now that all of the jobs use the component finalization callbacks,
there's no use for the heavy-hammer .exit callback anymore.

job_exit becomes a glorified type shim so that we can call
job_completed from aio_bh_schedule_oneshot.

Move these three functions down into job.c to eliminate a
forward reference.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/job.h | 11 --------
 job.c              | 77 ++++++++++++++++++++++++------------------------------
 2 files changed, 34 insertions(+), 54 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index e0cff702b7..5cb0681834 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -221,17 +221,6 @@ struct JobDriver {
      */
     void (*drain)(Job *job);
 
-    /**
-     * If the callback is not NULL, exit will be invoked from the main thread
-     * when the job's coroutine has finished, but before transactional
-     * convergence; before @prepare or @abort.
-     *
-     * FIXME TODO: This callback is only temporary to transition remaining jobs
-     * to prepare/commit/abort/clean callbacks and will be removed before 3.1.
-     * is released.
-     */
-    void (*exit)(Job *job);
-
     /**
      * If the callback is not NULL, prepare will be invoked when all the jobs
      * belonging to the same transaction complete; or upon this job's completion
diff --git a/job.c b/job.c
index 01dd97fee3..72f7de1f36 100644
--- a/job.c
+++ b/job.c
@@ -535,49 +535,6 @@ void job_drain(Job *job)
     }
 }
 
-static void job_completed(Job *job);
-
-static void job_exit(void *opaque)
-{
-    Job *job = (Job *)opaque;
-    AioContext *aio_context = job->aio_context;
-
-    if (job->driver->exit) {
-        aio_context_acquire(aio_context);
-        job->driver->exit(job);
-        aio_context_release(aio_context);
-    }
-    job_completed(job);
-}
-
-/**
- * All jobs must allow a pause point before entering their job proper. This
- * ensures that jobs can be paused prior to being started, then resumed later.
- */
-static void coroutine_fn job_co_entry(void *opaque)
-{
-    Job *job = opaque;
-
-    assert(job && job->driver && job->driver->run);
-    job_pause_point(job);
-    job->ret = job->driver->run(job, &job->err);
-    job->deferred_to_main_loop = true;
-    aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
-}
-
-
-void job_start(Job *job)
-{
-    assert(job && !job_started(job) && job->paused &&
-           job->driver && job->driver->run);
-    job->co = qemu_coroutine_create(job_co_entry, job);
-    job->pause_count--;
-    job->busy = true;
-    job->paused = false;
-    job_state_transition(job, JOB_STATUS_RUNNING);
-    aio_co_enter(job->aio_context, job->co);
-}
-
 /* Assumes the block_job_mutex is held */
 static bool job_timer_not_pending(Job *job)
 {
@@ -894,6 +851,40 @@ static void job_completed(Job *job)
     }
 }
 
+/** Useful only as a type shim for aio_bh_schedule_oneshot. */
+static void job_exit(void *opaque)
+{
+    Job *job = (Job *)opaque;
+    job_completed(job);
+}
+
+/**
+ * All jobs must allow a pause point before entering their job proper. This
+ * ensures that jobs can be paused prior to being started, then resumed later.
+ */
+static void coroutine_fn job_co_entry(void *opaque)
+{
+    Job *job = opaque;
+
+    assert(job && job->driver && job->driver->run);
+    job_pause_point(job);
+    job->ret = job->driver->run(job, &job->err);
+    job->deferred_to_main_loop = true;
+    aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
+}
+
+void job_start(Job *job)
+{
+    assert(job && !job_started(job) && job->paused &&
+           job->driver && job->driver->run);
+    job->co = qemu_coroutine_create(job_co_entry, job);
+    job->pause_count--;
+    job->busy = true;
+    job->paused = false;
+    job_state_transition(job, JOB_STATUS_RUNNING);
+    aio_co_enter(job->aio_context, job->co);
+}
+
 void job_cancel(Job *job, bool force)
 {
     if (job->status == JOB_STATUS_CONCLUDED) {
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 11/16] jobs: remove .exit callback

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:20AM -0400, John Snow wrote:
> Now that all of the jobs use the component finalization callbacks,
> there's no use for the heavy-hammer .exit callback anymore.
> 
> job_exit becomes a glorified type shim so that we can call
> job_completed from aio_bh_schedule_oneshot.
> 
> Move these three functions down into job.c to eliminate a
> forward reference.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  include/qemu/job.h | 11 --------
>  job.c              | 77 ++++++++++++++++++++++++------------------------------
>  2 files changed, 34 insertions(+), 54 deletions(-)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index e0cff702b7..5cb0681834 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -221,17 +221,6 @@ struct JobDriver {
>       */
>      void (*drain)(Job *job);
>  
> -    /**
> -     * If the callback is not NULL, exit will be invoked from the main thread
> -     * when the job's coroutine has finished, but before transactional
> -     * convergence; before @prepare or @abort.
> -     *
> -     * FIXME TODO: This callback is only temporary to transition remaining jobs
> -     * to prepare/commit/abort/clean callbacks and will be removed before 3.1.
> -     * is released.
> -     */
> -    void (*exit)(Job *job);
> -
>      /**
>       * If the callback is not NULL, prepare will be invoked when all the jobs
>       * belonging to the same transaction complete; or upon this job's completion
> diff --git a/job.c b/job.c
> index 01dd97fee3..72f7de1f36 100644
> --- a/job.c
> +++ b/job.c
> @@ -535,49 +535,6 @@ void job_drain(Job *job)
>      }
>  }
>  
> -static void job_completed(Job *job);
> -
> -static void job_exit(void *opaque)
> -{
> -    Job *job = (Job *)opaque;
> -    AioContext *aio_context = job->aio_context;
> -
> -    if (job->driver->exit) {
> -        aio_context_acquire(aio_context);
> -        job->driver->exit(job);
> -        aio_context_release(aio_context);
> -    }
> -    job_completed(job);
> -}
> -
> -/**
> - * All jobs must allow a pause point before entering their job proper. This
> - * ensures that jobs can be paused prior to being started, then resumed later.
> - */
> -static void coroutine_fn job_co_entry(void *opaque)
> -{
> -    Job *job = opaque;
> -
> -    assert(job && job->driver && job->driver->run);
> -    job_pause_point(job);
> -    job->ret = job->driver->run(job, &job->err);
> -    job->deferred_to_main_loop = true;
> -    aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
> -}
> -
> -
> -void job_start(Job *job)
> -{
> -    assert(job && !job_started(job) && job->paused &&
> -           job->driver && job->driver->run);
> -    job->co = qemu_coroutine_create(job_co_entry, job);
> -    job->pause_count--;
> -    job->busy = true;
> -    job->paused = false;
> -    job_state_transition(job, JOB_STATUS_RUNNING);
> -    aio_co_enter(job->aio_context, job->co);
> -}
> -
>  /* Assumes the block_job_mutex is held */
>  static bool job_timer_not_pending(Job *job)
>  {
> @@ -894,6 +851,40 @@ static void job_completed(Job *job)
>      }
>  }
>  
> +/** Useful only as a type shim for aio_bh_schedule_oneshot. */
> +static void job_exit(void *opaque)
> +{
> +    Job *job = (Job *)opaque;
> +    job_completed(job);
> +}
> +
> +/**
> + * All jobs must allow a pause point before entering their job proper. This
> + * ensures that jobs can be paused prior to being started, then resumed later.
> + */
> +static void coroutine_fn job_co_entry(void *opaque)
> +{
> +    Job *job = opaque;
> +
> +    assert(job && job->driver && job->driver->run);
> +    job_pause_point(job);
> +    job->ret = job->driver->run(job, &job->err);
> +    job->deferred_to_main_loop = true;
> +    aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
> +}
> +
> +void job_start(Job *job)
> +{
> +    assert(job && !job_started(job) && job->paused &&
> +           job->driver && job->driver->run);
> +    job->co = qemu_coroutine_create(job_co_entry, job);
> +    job->pause_count--;
> +    job->busy = true;
> +    job->paused = false;
> +    job_state_transition(job, JOB_STATUS_RUNNING);
> +    aio_co_enter(job->aio_context, job->co);
> +}
> +
>  void job_cancel(Job *job, bool force)
>  {
>      if (job->status == JOB_STATUS_CONCLUDED) {
> -- 
> 2.14.4
> 

[Qemu-devel] [PATCH v5 12/16] qapi/block-commit: expose new job properties

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c           |  8 ++++++++
 qapi/block-core.json | 16 +++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index ec90eb1cf9..98b91e75a7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3204,6 +3204,8 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
                       bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       bool has_filter_node_name, const char *filter_node_name,
+                      bool has_auto_finalize, bool auto_finalize,
+                      bool has_auto_dismiss, bool auto_dismiss,
                       Error **errp)
 {
     BlockDriverState *bs;
@@ -3223,6 +3225,12 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
     if (!has_filter_node_name) {
         filter_node_name = NULL;
     }
+    if (has_auto_finalize && !auto_finalize) {
+        job_flags |= JOB_MANUAL_FINALIZE;
+    }
+    if (has_auto_dismiss && !auto_dismiss) {
+        job_flags |= JOB_MANUAL_DISMISS;
+    }
 
     /* Important Note:
      *  libvirt relies on the DeviceNotFound error class in order to probe for
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4c7a37afdc..d5b62e50d7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1498,6 +1498,19 @@
 #                    above @top. If this option is not given, a node name is
 #                    autogenerated. (Since: 2.9)
 #
+# @auto-finalize: When false, this job will wait in a PENDING state after it has
+#                 finished its work, waiting for @block-job-finalize before
+#                 making any block graph changes.
+#                 When true, this job will automatically
+#                 perform its abort or commit actions.
+#                 Defaults to true. (Since 3.1)
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#                has completely ceased all work, and awaits @block-job-dismiss.
+#                When true, this job will automatically disappear from the query
+#                list without user intervention.
+#                Defaults to true. (Since 3.1)
+#
 # Returns: Nothing on success
 #          If @device does not exist, DeviceNotFound
 #          Any other error returns a GenericError.
@@ -1515,7 +1528,8 @@
 { 'command': 'block-commit',
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
             '*backing-file': 'str', '*speed': 'int',
-            '*filter-node-name': 'str' } }
+            '*filter-node-name': 'str',
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
 ##
 # @drive-backup:
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 12/16] qapi/block-commit: expose new job properties

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:21AM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  blockdev.c           |  8 ++++++++
>  qapi/block-core.json | 16 +++++++++++++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index ec90eb1cf9..98b91e75a7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3204,6 +3204,8 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
>                        bool has_backing_file, const char *backing_file,
>                        bool has_speed, int64_t speed,
>                        bool has_filter_node_name, const char *filter_node_name,
> +                      bool has_auto_finalize, bool auto_finalize,
> +                      bool has_auto_dismiss, bool auto_dismiss,
>                        Error **errp)
>  {
>      BlockDriverState *bs;
> @@ -3223,6 +3225,12 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
>      if (!has_filter_node_name) {
>          filter_node_name = NULL;
>      }
> +    if (has_auto_finalize && !auto_finalize) {
> +        job_flags |= JOB_MANUAL_FINALIZE;
> +    }
> +    if (has_auto_dismiss && !auto_dismiss) {
> +        job_flags |= JOB_MANUAL_DISMISS;
> +    }
>  
>      /* Important Note:
>       *  libvirt relies on the DeviceNotFound error class in order to probe for
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4c7a37afdc..d5b62e50d7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1498,6 +1498,19 @@
>  #                    above @top. If this option is not given, a node name is
>  #                    autogenerated. (Since: 2.9)
>  #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it has
> +#                 finished its work, waiting for @block-job-finalize before
> +#                 making any block graph changes.
> +#                 When true, this job will automatically
> +#                 perform its abort or commit actions.
> +#                 Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#                has completely ceased all work, and awaits @block-job-dismiss.
> +#                When true, this job will automatically disappear from the query
> +#                list without user intervention.
> +#                Defaults to true. (Since 3.1)
> +#
>  # Returns: Nothing on success
>  #          If @device does not exist, DeviceNotFound
>  #          Any other error returns a GenericError.
> @@ -1515,7 +1528,8 @@
>  { 'command': 'block-commit',
>    'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
>              '*backing-file': 'str', '*speed': 'int',
> -            '*filter-node-name': 'str' } }
> +            '*filter-node-name': 'str',
> +            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>  
>  ##
>  # @drive-backup:
> -- 
> 2.14.4
> 

[Qemu-devel] [PATCH v5 13/16] qapi/block-mirror: expose new job properties

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c           | 14 ++++++++++++++
 qapi/block-core.json | 30 ++++++++++++++++++++++++++++--
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 98b91e75a7..429cdf9901 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3597,6 +3597,8 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
                                    bool has_filter_node_name,
                                    const char *filter_node_name,
                                    bool has_copy_mode, MirrorCopyMode copy_mode,
+                                   bool has_auto_finalize, bool auto_finalize,
+                                   bool has_auto_dismiss, bool auto_dismiss,
                                    Error **errp)
 {
     int job_flags = JOB_DEFAULT;
@@ -3625,6 +3627,12 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
     if (!has_copy_mode) {
         copy_mode = MIRROR_COPY_MODE_BACKGROUND;
     }
+    if (has_auto_finalize && !auto_finalize) {
+        job_flags |= JOB_MANUAL_FINALIZE;
+    }
+    if (has_auto_dismiss && !auto_dismiss) {
+        job_flags |= JOB_MANUAL_DISMISS;
+    }
 
     if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
@@ -3802,6 +3810,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
                            arg->has_unmap, arg->unmap,
                            false, NULL,
                            arg->has_copy_mode, arg->copy_mode,
+                           arg->has_auto_finalize, arg->auto_finalize,
+                           arg->has_auto_dismiss, arg->auto_dismiss,
                            &local_err);
     bdrv_unref(target_bs);
     error_propagate(errp, local_err);
@@ -3823,6 +3833,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
                          bool has_filter_node_name,
                          const char *filter_node_name,
                          bool has_copy_mode, MirrorCopyMode copy_mode,
+                         bool has_auto_finalize, bool auto_finalize,
+                         bool has_auto_dismiss, bool auto_dismiss,
                          Error **errp)
 {
     BlockDriverState *bs;
@@ -3856,6 +3868,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
                            true, true,
                            has_filter_node_name, filter_node_name,
                            has_copy_mode, copy_mode,
+                           has_auto_finalize, auto_finalize,
+                           has_auto_dismiss, auto_dismiss,
                            &local_err);
     error_propagate(errp, local_err);
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d5b62e50d7..e785c2e9fe 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1729,6 +1729,18 @@
 # @copy-mode: when to copy data to the destination; defaults to 'background'
 #             (Since: 3.0)
 #
+# @auto-finalize: When false, this job will wait in a PENDING state after it has
+#                 finished its work, waiting for @block-job-finalize before
+#                 making any block graph changes.
+#                 When true, this job will automatically
+#                 perform its abort or commit actions.
+#                 Defaults to true. (Since 3.1)
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#                has completely ceased all work, and awaits @block-job-dismiss.
+#                When true, this job will automatically disappear from the query
+#                list without user intervention.
+#                Defaults to true. (Since 3.1)
 # Since: 1.3
 ##
 { 'struct': 'DriveMirror',
@@ -1738,7 +1750,8 @@
             '*speed': 'int', '*granularity': 'uint32',
             '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
-            '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode' } }
+            '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode',
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
 ##
 # @BlockDirtyBitmap:
@@ -2004,6 +2017,18 @@
 # @copy-mode: when to copy data to the destination; defaults to 'background'
 #             (Since: 3.0)
 #
+# @auto-finalize: When false, this job will wait in a PENDING state after it has
+#                 finished its work, waiting for @block-job-finalize before
+#                 making any block graph changes.
+#                 When true, this job will automatically
+#                 perform its abort or commit actions.
+#                 Defaults to true. (Since 3.1)
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#                has completely ceased all work, and awaits @block-job-dismiss.
+#                When true, this job will automatically disappear from the query
+#                list without user intervention.
+#                Defaults to true. (Since 3.1)
 # Returns: nothing on success.
 #
 # Since: 2.6
@@ -2025,7 +2050,8 @@
             '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
             '*filter-node-name': 'str',
-            '*copy-mode': 'MirrorCopyMode' } }
+            '*copy-mode': 'MirrorCopyMode',
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
 ##
 # @block_set_io_throttle:
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 13/16] qapi/block-mirror: expose new job properties

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:22AM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  blockdev.c           | 14 ++++++++++++++
>  qapi/block-core.json | 30 ++++++++++++++++++++++++++++--
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 98b91e75a7..429cdf9901 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3597,6 +3597,8 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
>                                     bool has_filter_node_name,
>                                     const char *filter_node_name,
>                                     bool has_copy_mode, MirrorCopyMode copy_mode,
> +                                   bool has_auto_finalize, bool auto_finalize,
> +                                   bool has_auto_dismiss, bool auto_dismiss,
>                                     Error **errp)
>  {
>      int job_flags = JOB_DEFAULT;
> @@ -3625,6 +3627,12 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
>      if (!has_copy_mode) {
>          copy_mode = MIRROR_COPY_MODE_BACKGROUND;
>      }
> +    if (has_auto_finalize && !auto_finalize) {
> +        job_flags |= JOB_MANUAL_FINALIZE;
> +    }
> +    if (has_auto_dismiss && !auto_dismiss) {
> +        job_flags |= JOB_MANUAL_DISMISS;
> +    }
>  
>      if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
> @@ -3802,6 +3810,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>                             arg->has_unmap, arg->unmap,
>                             false, NULL,
>                             arg->has_copy_mode, arg->copy_mode,
> +                           arg->has_auto_finalize, arg->auto_finalize,
> +                           arg->has_auto_dismiss, arg->auto_dismiss,
>                             &local_err);
>      bdrv_unref(target_bs);
>      error_propagate(errp, local_err);
> @@ -3823,6 +3833,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
>                           bool has_filter_node_name,
>                           const char *filter_node_name,
>                           bool has_copy_mode, MirrorCopyMode copy_mode,
> +                         bool has_auto_finalize, bool auto_finalize,
> +                         bool has_auto_dismiss, bool auto_dismiss,
>                           Error **errp)
>  {
>      BlockDriverState *bs;
> @@ -3856,6 +3868,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
>                             true, true,
>                             has_filter_node_name, filter_node_name,
>                             has_copy_mode, copy_mode,
> +                           has_auto_finalize, auto_finalize,
> +                           has_auto_dismiss, auto_dismiss,
>                             &local_err);
>      error_propagate(errp, local_err);
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d5b62e50d7..e785c2e9fe 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1729,6 +1729,18 @@
>  # @copy-mode: when to copy data to the destination; defaults to 'background'
>  #             (Since: 3.0)
>  #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it has
> +#                 finished its work, waiting for @block-job-finalize before
> +#                 making any block graph changes.
> +#                 When true, this job will automatically
> +#                 perform its abort or commit actions.
> +#                 Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#                has completely ceased all work, and awaits @block-job-dismiss.
> +#                When true, this job will automatically disappear from the query
> +#                list without user intervention.
> +#                Defaults to true. (Since 3.1)
>  # Since: 1.3
>  ##
>  { 'struct': 'DriveMirror',
> @@ -1738,7 +1750,8 @@
>              '*speed': 'int', '*granularity': 'uint32',
>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>              '*on-target-error': 'BlockdevOnError',
> -            '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode' } }
> +            '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode',
> +            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>  
>  ##
>  # @BlockDirtyBitmap:
> @@ -2004,6 +2017,18 @@
>  # @copy-mode: when to copy data to the destination; defaults to 'background'
>  #             (Since: 3.0)
>  #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it has
> +#                 finished its work, waiting for @block-job-finalize before
> +#                 making any block graph changes.
> +#                 When true, this job will automatically
> +#                 perform its abort or commit actions.
> +#                 Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#                has completely ceased all work, and awaits @block-job-dismiss.
> +#                When true, this job will automatically disappear from the query
> +#                list without user intervention.
> +#                Defaults to true. (Since 3.1)
>  # Returns: nothing on success.
>  #
>  # Since: 2.6
> @@ -2025,7 +2050,8 @@
>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>              '*on-target-error': 'BlockdevOnError',
>              '*filter-node-name': 'str',
> -            '*copy-mode': 'MirrorCopyMode' } }
> +            '*copy-mode': 'MirrorCopyMode',
> +            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>  
>  ##
>  # @block_set_io_throttle:
> -- 
> 2.14.4
> 

[Qemu-devel] [PATCH v5 14/16] qapi/block-stream: expose new job properties

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c           |  9 +++++++++
 hmp.c                |  5 +++--
 qapi/block-core.json | 16 +++++++++++++++-
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 429cdf9901..0cf8febe6c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3116,6 +3116,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
                       bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
+                      bool has_auto_finalize, bool auto_finalize,
+                      bool has_auto_dismiss, bool auto_dismiss,
                       Error **errp)
 {
     BlockDriverState *bs, *iter;
@@ -3185,6 +3187,13 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     /* backing_file string overrides base bs filename */
     base_name = has_backing_file ? backing_file : base_name;
 
+    if (has_auto_finalize && !auto_finalize) {
+        job_flags |= JOB_MANUAL_FINALIZE;
+    }
+    if (has_auto_dismiss && !auto_dismiss) {
+        job_flags |= JOB_MANUAL_DISMISS;
+    }
+
     stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
                  job_flags, has_speed ? speed : 0, on_error, &local_err);
     if (local_err) {
diff --git a/hmp.c b/hmp.c
index 4975fa56b0..868c1a049d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1905,8 +1905,9 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
     qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
-                     false, NULL, qdict_haskey(qdict, "speed"), speed,
-                     true, BLOCKDEV_ON_ERROR_REPORT, &error);
+                     false, NULL, qdict_haskey(qdict, "speed"), speed, true,
+                     BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
+                     &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e785c2e9fe..f877e9e414 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2317,6 +2317,19 @@
 #            'stop' and 'enospc' can only be used if the block device
 #            supports io-status (see BlockInfo).  Since 1.3.
 #
+# @auto-finalize: When false, this job will wait in a PENDING state after it has
+#                 finished its work, waiting for @block-job-finalize before
+#                 making any block graph changes.
+#                 When true, this job will automatically
+#                 perform its abort or commit actions.
+#                 Defaults to true. (Since 3.1)
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#                has completely ceased all work, and awaits @block-job-dismiss.
+#                When true, this job will automatically disappear from the query
+#                list without user intervention.
+#                Defaults to true. (Since 3.1)
+#
 # Returns: Nothing on success. If @device does not exist, DeviceNotFound.
 #
 # Since: 1.1
@@ -2332,7 +2345,8 @@
 { 'command': 'block-stream',
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
             '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
-            '*on-error': 'BlockdevOnError' } }
+            '*on-error': 'BlockdevOnError',
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
 ##
 # @block-job-set-speed:
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 14/16] qapi/block-stream: expose new job properties

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:23AM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  blockdev.c           |  9 +++++++++
>  hmp.c                |  5 +++--
>  qapi/block-core.json | 16 +++++++++++++++-
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 429cdf9901..0cf8febe6c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3116,6 +3116,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>                        bool has_backing_file, const char *backing_file,
>                        bool has_speed, int64_t speed,
>                        bool has_on_error, BlockdevOnError on_error,
> +                      bool has_auto_finalize, bool auto_finalize,
> +                      bool has_auto_dismiss, bool auto_dismiss,
>                        Error **errp)
>  {
>      BlockDriverState *bs, *iter;
> @@ -3185,6 +3187,13 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>      /* backing_file string overrides base bs filename */
>      base_name = has_backing_file ? backing_file : base_name;
>  
> +    if (has_auto_finalize && !auto_finalize) {
> +        job_flags |= JOB_MANUAL_FINALIZE;
> +    }
> +    if (has_auto_dismiss && !auto_dismiss) {
> +        job_flags |= JOB_MANUAL_DISMISS;
> +    }
> +
>      stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
>                   job_flags, has_speed ? speed : 0, on_error, &local_err);
>      if (local_err) {
> diff --git a/hmp.c b/hmp.c
> index 4975fa56b0..868c1a049d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1905,8 +1905,9 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
>      int64_t speed = qdict_get_try_int(qdict, "speed", 0);
>  
>      qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
> -                     false, NULL, qdict_haskey(qdict, "speed"), speed,
> -                     true, BLOCKDEV_ON_ERROR_REPORT, &error);
> +                     false, NULL, qdict_haskey(qdict, "speed"), speed, true,
> +                     BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
> +                     &error);
>  
>      hmp_handle_error(mon, &error);
>  }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e785c2e9fe..f877e9e414 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2317,6 +2317,19 @@
>  #            'stop' and 'enospc' can only be used if the block device
>  #            supports io-status (see BlockInfo).  Since 1.3.
>  #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it has
> +#                 finished its work, waiting for @block-job-finalize before
> +#                 making any block graph changes.
> +#                 When true, this job will automatically
> +#                 perform its abort or commit actions.
> +#                 Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#                has completely ceased all work, and awaits @block-job-dismiss.
> +#                When true, this job will automatically disappear from the query
> +#                list without user intervention.
> +#                Defaults to true. (Since 3.1)
> +#
>  # Returns: Nothing on success. If @device does not exist, DeviceNotFound.
>  #
>  # Since: 1.1
> @@ -2332,7 +2345,8 @@
>  { 'command': 'block-stream',
>    'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
>              '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
> -            '*on-error': 'BlockdevOnError' } }
> +            '*on-error': 'BlockdevOnError',
> +            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>  
>  ##
>  # @block-job-set-speed:
> -- 
> 2.14.4
> 

[Qemu-devel] [PATCH v5 15/16] block/backup: qapi documentation fixup

[John Snow]

Fix documentation to match the other jobs amended for 3.1.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index f877e9e414..c0b3d33dbb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1272,13 +1272,14 @@
 #                   a different block device than @device).
 #
 # @auto-finalize: When false, this job will wait in a PENDING state after it has
-#                 finished its work, waiting for @block-job-finalize.
-#                 When true, this job will automatically perform its abort or
-#                 commit actions.
+#                 finished its work, waiting for @block-job-finalize before
+#                 making any block graph changes.
+#                 When true, this job will automatically
+#                 perform its abort or commit actions.
 #                 Defaults to true. (Since 2.12)
 #
 # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
-#                has completed ceased all work, and wait for @block-job-dismiss.
+#                has completely ceased all work, and awaits @block-job-dismiss.
 #                When true, this job will automatically disappear from the query
 #                list without user intervention.
 #                Defaults to true. (Since 2.12)
@@ -1327,13 +1328,14 @@
 #                   a different block device than @device).
 #
 # @auto-finalize: When false, this job will wait in a PENDING state after it has
-#                 finished its work, waiting for @block-job-finalize.
-#                 When true, this job will automatically perform its abort or
-#                 commit actions.
+#                 finished its work, waiting for @block-job-finalize before
+#                 making any block graph changes.
+#                 When true, this job will automatically
+#                 perform its abort or commit actions.
 #                 Defaults to true. (Since 2.12)
 #
 # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
-#                has completed ceased all work, and wait for @block-job-dismiss.
+#                has completely ceased all work, and awaits @block-job-dismiss.
 #                When true, this job will automatically disappear from the query
 #                list without user intervention.
 #                Defaults to true. (Since 2.12)
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 15/16] block/backup: qapi documentation fixup

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:24AM -0400, John Snow wrote:
> Fix documentation to match the other jobs amended for 3.1.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  qapi/block-core.json | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f877e9e414..c0b3d33dbb 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1272,13 +1272,14 @@
>  #                   a different block device than @device).
>  #
>  # @auto-finalize: When false, this job will wait in a PENDING state after it has
> -#                 finished its work, waiting for @block-job-finalize.
> -#                 When true, this job will automatically perform its abort or
> -#                 commit actions.
> +#                 finished its work, waiting for @block-job-finalize before
> +#                 making any block graph changes.
> +#                 When true, this job will automatically
> +#                 perform its abort or commit actions.
>  #                 Defaults to true. (Since 2.12)
>  #
>  # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> -#                has completed ceased all work, and wait for @block-job-dismiss.
> +#                has completely ceased all work, and awaits @block-job-dismiss.
>  #                When true, this job will automatically disappear from the query
>  #                list without user intervention.
>  #                Defaults to true. (Since 2.12)
> @@ -1327,13 +1328,14 @@
>  #                   a different block device than @device).
>  #
>  # @auto-finalize: When false, this job will wait in a PENDING state after it has
> -#                 finished its work, waiting for @block-job-finalize.
> -#                 When true, this job will automatically perform its abort or
> -#                 commit actions.
> +#                 finished its work, waiting for @block-job-finalize before
> +#                 making any block graph changes.
> +#                 When true, this job will automatically
> +#                 perform its abort or commit actions.
>  #                 Defaults to true. (Since 2.12)
>  #
>  # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> -#                has completed ceased all work, and wait for @block-job-dismiss.
> +#                has completely ceased all work, and awaits @block-job-dismiss.
>  #                When true, this job will automatically disappear from the query
>  #                list without user intervention.
>  #                Defaults to true. (Since 2.12)
> -- 
> 2.14.4
> 

[Qemu-devel] [PATCH v5 16/16] blockdev: document transactional shortcomings

[John Snow]

Presently only the backup job really guarantees what one would consider
transactional semantics. To guard against someone helpfully adding them
in the future, document that there are shortcomings in the model that
would need to be audited at that time.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 0cf8febe6c..d4b42403df 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2182,7 +2182,13 @@ static const BlkActionOps actions[] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_disable_prepare,
         .abort = block_dirty_bitmap_disable_abort,
-     }
+    },
+    /* Where are transactions for MIRROR, COMMIT and STREAM?
+     * Although these blockjobs use transaction callbacks like the backup job,
+     * these jobs do not necessarily adhere to transaction semantics.
+     * These jobs may not fully undo all of their actions on abort, nor do they
+     * necessarily work in transactions with more than one job in them.
+     */
 };
 
 /**
-- 
2.14.4

Re: [Qemu-devel] [PATCH v5 16/16] blockdev: document transactional shortcomings

[Jeff Cody]

On Thu, Sep 06, 2018 at 09:02:25AM -0400, John Snow wrote:
> Presently only the backup job really guarantees what one would consider
> transactional semantics. To guard against someone helpfully adding them
> in the future, document that there are shortcomings in the model that
> would need to be audited at that time.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  blockdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 0cf8febe6c..d4b42403df 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2182,7 +2182,13 @@ static const BlkActionOps actions[] = {
>          .instance_size = sizeof(BlockDirtyBitmapState),
>          .prepare = block_dirty_bitmap_disable_prepare,
>          .abort = block_dirty_bitmap_disable_abort,
> -     }
> +    },
> +    /* Where are transactions for MIRROR, COMMIT and STREAM?
> +     * Although these blockjobs use transaction callbacks like the backup job,
> +     * these jobs do not necessarily adhere to transaction semantics.
> +     * These jobs may not fully undo all of their actions on abort, nor do they
> +     * necessarily work in transactions with more than one job in them.
> +     */
>  };
>  
>  /**
> -- 
> 2.14.4
> 

Re: [Qemu-devel] [PATCH v5 16/16] blockdev: document transactional shortcomings

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

On 2018-09-06 15:02, John Snow wrote:
> Presently only the backup job really guarantees what one would consider
> transactional semantics. To guard against someone helpfully adding them
> in the future, document that there are shortcomings in the model that
> would need to be audited at that time.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v5 00/16] jobs: Job Exit Refactoring Pt 2

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On 2018-09-06 15:02, John Snow wrote:
> This is part two of a two part series that refactors the exit logic
> of jobs.
> 
> This series forces all jobs to use the "finalize" semantics that were
> introduced previously, but only exposed via the backup jobs.
> 
> Patches 1-3 add plumbing for the auto-dismiss and auto-finalize flags
> but do not expose these via QAPI/QMP.
> 
> Patches 4-7 refactor the .exit() callbacks into the component pieces
> of .prepare(), .commit(), .abort() and .clean(). Except mirror, which
> I cheat with.
> 
> Patches 8-10 remove the last usages of .exit in a test.
> Patche 11 removes the .exit callback and the machinery to invoke it.
> 
> Patches 12-14 expose the new QMP options to all of the jobs.
> Patch 15 is a doc fixup.

Thanks, fixed the capitalization in patch 4's commit message, added the
mirror_exit() comment you proposed in patch 6, and applied to my block
branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]