All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2
@ 2018-08-31 22:28 John Snow
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 01/15] block/commit: add block job creation flags John Snow
                   ` (14 more replies)
  0 siblings, 15 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:28 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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.

====
"V3"
====

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

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 (15):
  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/commit: 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

 block/commit.c            |  95 +++++++++++++++++++++-----------------
 block/mirror.c            |  63 ++++++++++++++++---------
 block/stream.c            |  28 ++++++++----
 blockdev.c                |  44 ++++++++++++++++--
 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, 326 insertions(+), 210 deletions(-)

-- 
2.14.4

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 01/15] block/commit: add block job creation flags
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
@ 2018-08-31 22:28 ` John Snow
  2018-09-01  3:17   ` Jeff Cody
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 02/15] block/mirror: " John Snow
                   ` (13 subsequent siblings)
  14 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2018-08-31 22:28 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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>
---
 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 02/15] block/mirror: add block job creation flags
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 01/15] block/commit: add block job creation flags John Snow
@ 2018-08-31 22:28 ` John Snow
  2018-09-01  3:31   ` Jeff Cody
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 03/15] block/stream: " John Snow
                   ` (12 subsequent siblings)
  14 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2018-08-31 22:28 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, John Snow

Add support for taking and passing forward job creaton flags.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 03/15] block/stream: add block job creation flags
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 01/15] block/commit: add block job creation flags John Snow
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 02/15] block/mirror: " John Snow
@ 2018-08-31 22:28 ` John Snow
  2018-08-31 22:34   ` Eric Blake
  2018-09-01  3:33   ` Jeff Cody
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 04/15] block/commit: refactor commit to use job callbacks John Snow
                   ` (11 subsequent siblings)
  14 siblings, 2 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:28 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, John Snow

Add support for taking and passing forward job creaton flags.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 04/15] block/commit: refactor commit to use job callbacks
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (2 preceding siblings ...)
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 03/15] block/stream: " John Snow
@ 2018-08-31 22:28 ` John Snow
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort John Snow
                   ` (10 subsequent siblings)
  14 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:28 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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

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

diff --git a/block/commit.c b/block/commit.c
index b6e8969877..eb3941e545 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,65 @@ 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;
+    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 +216,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 +352,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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (3 preceding siblings ...)
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 04/15] block/commit: refactor commit to use job callbacks John Snow
@ 2018-08-31 22:28 ` John Snow
  2018-09-03  9:24   ` Max Reitz
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor John Snow
                   ` (9 subsequent siblings)
  14 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2018-08-31 22:28 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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.

Move this to the "success" portion of mirror_exit.

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

diff --git a/block/mirror.c b/block/mirror.c
index cba555b4ef..c164fee883 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -642,16 +642,6 @@ 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) {
-        BlockDriverState *backing = s->is_none_mode ? src : s->base;
-        if (backing_bs(target_bs) != backing) {
-            bdrv_set_backing_hd(target_bs, backing, &local_err);
-            if (local_err) {
-                error_report_err(local_err);
-                ret = -EPERM;
-            }
-        }
-    }
 
     if (s->to_replace) {
         replace_aio_context = bdrv_get_aio_context(s->to_replace);
@@ -659,9 +649,18 @@ static void mirror_exit(Job *job)
     }
 
     if (s->should_complete && ret == 0) {
-        BlockDriverState *to_replace = src;
-        if (s->to_replace) {
-            to_replace = s->to_replace;
+        BlockDriverState *to_replace = s->to_replace ? s->to_replace : src;
+
+        if (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);
+                if (local_err) {
+                    error_report_err(local_err);
+                    ret = -EPERM;
+                    goto clean;
+                }
+            }
         }
 
         if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
@@ -678,6 +677,8 @@ static void mirror_exit(Job *job)
             ret = -EPERM;
         }
     }
+
+ clean:
     if (s->to_replace) {
         bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
         error_free(s->replace_blocker);
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (4 preceding siblings ...)
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort John Snow
@ 2018-08-31 22:28 ` John Snow
  2018-09-03  9:32   ` Max Reitz
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 07/15] block/commit: refactor stream to use job callbacks John Snow
                   ` (8 subsequent siblings)
  14 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2018-08-31 22:28 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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 | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index c164fee883..5067f1764d 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;
+    int ret = 0;
+
+    if (s->prepared) {
+        return 0;
+    }
+    s->prepared = true;
 
     bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
 
@@ -648,7 +655,7 @@ static void mirror_exit(Job *job)
         aio_context_acquire(replace_aio_context);
     }
 
-    if (s->should_complete && ret == 0) {
+    if (s->should_complete && !abort) {
         BlockDriverState *to_replace = s->to_replace ? s->to_replace : src;
 
         if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
@@ -712,7 +719,17 @@ 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)
+{
+    assert(mirror_exit_common(job) == 0);
 }
 
 static void mirror_throttle(MirrorBlockJob *s)
@@ -1133,7 +1150,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,
     },
@@ -1150,7 +1168,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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 07/15] block/commit: refactor stream to use job callbacks
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (5 preceding siblings ...)
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor John Snow
@ 2018-08-31 22:28 ` John Snow
  2018-09-03  9:33   ` Max Reitz
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 08/15] tests/blockjob: replace Blockjob with Job John Snow
                   ` (7 subsequent siblings)
  14 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2018-08-31 22:28 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 08/15] tests/blockjob: replace Blockjob with Job
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (6 preceding siblings ...)
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 07/15] block/commit: refactor stream to use job callbacks John Snow
@ 2018-08-31 22:29 ` John Snow
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 09/15] tests/test-blockjob: remove exit callback John Snow
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:29 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 09/15] tests/test-blockjob: remove exit callback
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (7 preceding siblings ...)
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 08/15] tests/blockjob: replace Blockjob with Job John Snow
@ 2018-08-31 22:29 ` John Snow
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 10/15] tests/test-blockjob-txn: move .exit to .clean John Snow
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:29 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 10/15] tests/test-blockjob-txn: move .exit to .clean
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (8 preceding siblings ...)
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 09/15] tests/test-blockjob: remove exit callback John Snow
@ 2018-08-31 22:29 ` John Snow
  2018-09-03  9:41   ` Max Reitz
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 11/15] jobs: remove .exit callback John Snow
                   ` (4 subsequent siblings)
  14 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2018-08-31 22:29 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, John Snow

The exit callback in this test actually only performs cleanup.

Signed-off-by: John Snow <jsnow@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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 11/15] jobs: remove .exit callback
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (9 preceding siblings ...)
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 10/15] tests/test-blockjob-txn: move .exit to .clean John Snow
@ 2018-08-31 22:29 ` John Snow
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 12/15] qapi/block-commit: expose new job properties John Snow
                   ` (3 subsequent siblings)
  14 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:29 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 12/15] qapi/block-commit: expose new job properties
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (10 preceding siblings ...)
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 11/15] jobs: remove .exit callback John Snow
@ 2018-08-31 22:29 ` John Snow
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 13/15] qapi/block-mirror: " John Snow
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:29 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 13/15] qapi/block-mirror: expose new job properties
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (11 preceding siblings ...)
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 12/15] qapi/block-commit: expose new job properties John Snow
@ 2018-08-31 22:29 ` John Snow
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 14/15] qapi/block-stream: " John Snow
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 15/15] block/backup: qapi documentation fixup John Snow
  14 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:29 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 14/15] qapi/block-stream: expose new job properties
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (12 preceding siblings ...)
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 13/15] qapi/block-mirror: " John Snow
@ 2018-08-31 22:29 ` John Snow
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 15/15] block/backup: qapi documentation fixup John Snow
  14 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:29 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [Qemu-devel] [PATCH v3 15/15] block/backup: qapi documentation fixup
  2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (13 preceding siblings ...)
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 14/15] qapi/block-stream: " John Snow
@ 2018-08-31 22:29 ` John Snow
  14 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-08-31 22:29 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake, Jeff Cody, 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 03/15] block/stream: add block job creation flags
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 03/15] block/stream: " John Snow
@ 2018-08-31 22:34   ` Eric Blake
  2018-09-01  3:33   ` Jeff Cody
  1 sibling, 0 replies; 30+ messages in thread
From: Eric Blake @ 2018-08-31 22:34 UTC (permalink / raw)
  To: John Snow, qemu-devel, qemu-block
  Cc: jtc, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Jeff Cody

On 08/31/2018 05:28 PM, John Snow wrote:
> Add support for taking and passing forward job creaton flags.

s/creaton/creation/ (here and in 2/15 as well)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 01/15] block/commit: add block job creation flags
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 01/15] block/commit: add block job creation flags John Snow
@ 2018-09-01  3:17   ` Jeff Cody
  0 siblings, 0 replies; 30+ messages in thread
From: Jeff Cody @ 2018-09-01  3:17 UTC (permalink / raw)
  To: John Snow
  Cc: qemu-devel, qemu-block, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake

On Fri, Aug 31, 2018 at 06:28:53PM -0400, John Snow wrote:
> 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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 02/15] block/mirror: add block job creation flags
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 02/15] block/mirror: " John Snow
@ 2018-09-01  3:31   ` Jeff Cody
  0 siblings, 0 replies; 30+ messages in thread
From: Jeff Cody @ 2018-09-01  3:31 UTC (permalink / raw)
  To: John Snow
  Cc: qemu-devel, qemu-block, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake

On Fri, Aug 31, 2018 at 06:28:54PM -0400, John Snow wrote:
> Add support for taking and passing forward job creaton flags.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@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,

This is another one of those comments that do not pertain directly to this
patch, so it doesn't affect my r-b.  So let's get that out of the way:

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

Some of the block job functions have an impressive number of arguments now
(and have for a while, this isn't all that new).  I don't know if it is a
sign that our data design is a bit out of whack, or what.  But for instance,
mirror_start_job() takes 22 (!) parameters.  Seems like something worth
examining at some point.

-Jeff


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 03/15] block/stream: add block job creation flags
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 03/15] block/stream: " John Snow
  2018-08-31 22:34   ` Eric Blake
@ 2018-09-01  3:33   ` Jeff Cody
  1 sibling, 0 replies; 30+ messages in thread
From: Jeff Cody @ 2018-09-01  3:33 UTC (permalink / raw)
  To: John Snow
  Cc: qemu-devel, qemu-block, Max Reitz, Kevin Wolf, Markus Armbruster,
	Dr. David Alan Gilbert, Eric Blake

On Fri, Aug 31, 2018 at 06:28:55PM -0400, John Snow wrote:
> Add support for taking and passing forward job creaton flags.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

(with the misspelling that Eric pointed out fixed):

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort John Snow
@ 2018-09-03  9:24   ` Max Reitz
  2018-09-04 19:15     ` John Snow
  0 siblings, 1 reply; 30+ messages in thread
From: Max Reitz @ 2018-09-03  9:24 UTC (permalink / raw)
  To: John Snow, qemu-devel, qemu-block
  Cc: jtc, Kevin Wolf, Markus Armbruster, Dr. David Alan Gilbert,
	Eric Blake, Jeff Cody

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

On 2018-09-01 00:28, 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.
> 
> Move this to the "success" portion of mirror_exit.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index cba555b4ef..c164fee883 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -642,16 +642,6 @@ 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) {
> -        BlockDriverState *backing = s->is_none_mode ? src : s->base;
> -        if (backing_bs(target_bs) != backing) {
> -            bdrv_set_backing_hd(target_bs, backing, &local_err);
> -            if (local_err) {
> -                error_report_err(local_err);
> -                ret = -EPERM;
> -            }
> -        }
> -    }
>  
>      if (s->to_replace) {
>          replace_aio_context = bdrv_get_aio_context(s->to_replace);
> @@ -659,9 +649,18 @@ static void mirror_exit(Job *job)
>      }
>  
>      if (s->should_complete && ret == 0) {
> -        BlockDriverState *to_replace = src;
> -        if (s->to_replace) {
> -            to_replace = s->to_replace;
> +        BlockDriverState *to_replace = s->to_replace ? s->to_replace : src;
> +
> +        if (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);
> +                if (local_err) {
> +                    error_report_err(local_err);
> +                    ret = -EPERM;
> +                    goto clean;
> +                }
> +            }
>          }
>  
>          if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {

Testing shows that on post-READY cancel, s->should_complete is 0 (which
makes sense, because all the rest in this path is about replacing
to_replace by target_bs, which we don't want to do on cancel), so this
would not be executed.

However, I think we do want to give the target the correct backing chain
when the job is canceled this way (as I suppose there are ways to keep
the target around even with drive-mirror).

So we should attach the backing chain whenever ret == 0, not just when
s->should_complete is true.

Max

> @@ -678,6 +677,8 @@ static void mirror_exit(Job *job)
>              ret = -EPERM;
>          }
>      }
> +
> + clean:
>      if (s->to_replace) {
>          bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
>          error_free(s->replace_blocker);
> 



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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor John Snow
@ 2018-09-03  9:32   ` Max Reitz
  2018-09-04 16:17     ` John Snow
  0 siblings, 1 reply; 30+ messages in thread
From: Max Reitz @ 2018-09-03  9:32 UTC (permalink / raw)
  To: John Snow, qemu-devel, qemu-block
  Cc: jtc, Kevin Wolf, Markus Armbruster, Dr. David Alan Gilbert,
	Eric Blake, Jeff Cody

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

On 2018-09-01 00:28, 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 | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index c164fee883..5067f1764d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c

[...]

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

I think "job->ret < 0" could be read more easily.

> +    int ret = 0;
> +
> +    if (s->prepared) {
> +        return 0;
> +    }
> +    s->prepared = true;
>  
>      bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>  

[...]

> @@ -712,7 +719,17 @@ 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)
> +{
> +    assert(mirror_exit_common(job) == 0);

You shouldn't execute vital code in assert(), as NDEBUG would make that
code disappear.  As far as I know, we have decided (at some point) not
to ever enable NDEBUG in qemu, but, you know.  Doing it right only costs
one more line, and it would get you a

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

>  }
>  
>  static void mirror_throttle(MirrorBlockJob *s)


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 07/15] block/commit: refactor stream to use job callbacks
  2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 07/15] block/commit: refactor stream to use job callbacks John Snow
@ 2018-09-03  9:33   ` Max Reitz
  0 siblings, 0 replies; 30+ messages in thread
From: Max Reitz @ 2018-09-03  9:33 UTC (permalink / raw)
  To: John Snow, qemu-devel, qemu-block
  Cc: jtc, Kevin Wolf, Markus Armbruster, Dr. David Alan Gilbert,
	Eric Blake, Jeff Cody

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

On 2018-09-01 00:28, John Snow wrote:
> 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(-)

You forgot to fix the commit title. ;-)

Max


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 10/15] tests/test-blockjob-txn: move .exit to .clean
  2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 10/15] tests/test-blockjob-txn: move .exit to .clean John Snow
@ 2018-09-03  9:41   ` Max Reitz
  0 siblings, 0 replies; 30+ messages in thread
From: Max Reitz @ 2018-09-03  9:41 UTC (permalink / raw)
  To: John Snow, qemu-devel, qemu-block
  Cc: jtc, Kevin Wolf, Markus Armbruster, Dr. David Alan Gilbert,
	Eric Blake, Jeff Cody

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

On 2018-09-01 00:29, John Snow wrote:
> The exit callback in this test actually only performs cleanup.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/test-blockjob-txn.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor
  2018-09-03  9:32   ` Max Reitz
@ 2018-09-04 16:17     ` John Snow
  0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-09-04 16:17 UTC (permalink / raw)
  To: Max Reitz, qemu-devel, qemu-block
  Cc: jtc, Kevin Wolf, Markus Armbruster, Dr. David Alan Gilbert,
	Eric Blake, Jeff Cody



On 09/03/2018 05:32 AM, Max Reitz wrote:
> On 2018-09-01 00:28, 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 | 31 +++++++++++++++++++++++++------
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index c164fee883..5067f1764d 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
> 
> [...]
> 
>> @@ -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;
> 
> I think "job->ret < 0" could be read more easily.

Hm, OK. We shouldn't be using > 0 retcodes anyway.

> 
>> +    int ret = 0;
>> +
>> +    if (s->prepared) {
>> +        return 0;
>> +    }
>> +    s->prepared = true;
>>  
>>      bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>>  
> 
> [...]
> 
>> @@ -712,7 +719,17 @@ 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)
>> +{
>> +    assert(mirror_exit_common(job) == 0);
> 
> You shouldn't execute vital code in assert(), as NDEBUG would make that
> code disappear.  As far as I know, we have decided (at some point) not
> to ever enable NDEBUG in qemu, but, you know.  Doing it right only costs
> one more line, and it would get you a
> 

d'oh.

> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>>  }
>>  
>>  static void mirror_throttle(MirrorBlockJob *s)
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort
  2018-09-03  9:24   ` Max Reitz
@ 2018-09-04 19:15     ` John Snow
  2018-09-04 19:30       ` Eric Blake
  0 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2018-09-04 19:15 UTC (permalink / raw)
  To: Max Reitz, qemu-devel, qemu-block
  Cc: jtc, Kevin Wolf, Markus Armbruster, Dr. David Alan Gilbert,
	Eric Blake, Jeff Cody



On 09/03/2018 05:24 AM, Max Reitz wrote:
> On 2018-09-01 00:28, 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.
>>
>> Move this to the "success" portion of mirror_exit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/mirror.c | 27 ++++++++++++++-------------
>>  1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index cba555b4ef..c164fee883 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -642,16 +642,6 @@ 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) {
>> -        BlockDriverState *backing = s->is_none_mode ? src : s->base;
>> -        if (backing_bs(target_bs) != backing) {
>> -            bdrv_set_backing_hd(target_bs, backing, &local_err);
>> -            if (local_err) {
>> -                error_report_err(local_err);
>> -                ret = -EPERM;
>> -            }
>> -        }
>> -    }
>>  
>>      if (s->to_replace) {
>>          replace_aio_context = bdrv_get_aio_context(s->to_replace);
>> @@ -659,9 +649,18 @@ static void mirror_exit(Job *job)
>>      }
>>  
>>      if (s->should_complete && ret == 0) {
>> -        BlockDriverState *to_replace = src;
>> -        if (s->to_replace) {
>> -            to_replace = s->to_replace;
>> +        BlockDriverState *to_replace = s->to_replace ? s->to_replace : src;
>> +
>> +        if (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);
>> +                if (local_err) {
>> +                    error_report_err(local_err);
>> +                    ret = -EPERM;
>> +                    goto clean;
>> +                }
>> +            }
>>          }
>>  
>>          if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
> 
> Testing shows that on post-READY cancel, s->should_complete is 0 (which
> makes sense, because all the rest in this path is about replacing
> to_replace by target_bs, which we don't want to do on cancel), so this
> would not be executed.
> 
> However, I think we do want to give the target the correct backing chain
> when the job is canceled this way (as I suppose there are ways to keep
> the target around even with drive-mirror).
> 
> So we should attach the backing chain whenever ret == 0, not just when
> s->should_complete is true.
> 
> Max
> 

Ah, thank you for saving me from myself. I got too excited to have
everything under that one conditional.

post-script: I really, really hate the "fake cancel" we've implemented
for mirror. It makes the job logic so much worse.

--js

>> @@ -678,6 +677,8 @@ static void mirror_exit(Job *job)
>>              ret = -EPERM;
>>          }
>>      }
>> +
>> + clean:
>>      if (s->to_replace) {
>>          bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
>>          error_free(s->replace_blocker);
>>
> 
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort
  2018-09-04 19:15     ` John Snow
@ 2018-09-04 19:30       ` Eric Blake
  2018-09-04 20:10         ` John Snow
  2018-09-05  9:54         ` Kevin Wolf
  0 siblings, 2 replies; 30+ messages in thread
From: Eric Blake @ 2018-09-04 19:30 UTC (permalink / raw)
  To: John Snow, Max Reitz, qemu-devel, qemu-block
  Cc: jtc, Kevin Wolf, Markus Armbruster, Dr. David Alan Gilbert, Jeff Cody

On 09/04/2018 02:15 PM, John Snow wrote:

> 
> post-script: I really, really hate the "fake cancel" we've implemented
> for mirror. It makes the job logic so much worse.

Mirror really does have a tri-state way to end the job (and accessible 
only after the job has moved into the sync state):

cancel (gracefully end the job by detaching the mirror, so that the 
original volume remains active - your "fake cancel")
complete (gracefully end the job by pivoting to the mirror, so that the 
original volume is now the backup)
cancel --force (end the job now, even though it means you did not 
actually create a mirrored copy) - only useful since commit b76e4458 (2.12)

The first two are conceptually similar - the job succeeds, and one of 
the two files used in the mirroring is now the state of the other at the 
time the job concluded.

Back-compat says we can't really change the block-job terminology 
without an adequate deprecation cycle (and I don't know if libvirt has 
even been taught about cancel --force yet); but for the newer 'job' 
functionality (which we tried to shoehorn existing block jobs into), it 
does seem like it would be nicer to have 'cancel' mean what blockjob 
cancel --force implies, and instead focus on teaching 'complete' to have 
a way to select which of the two completion modes are desired (complete 
by return to original, or complete by pivot to mirror).  It might even 
be nice to have a way to specify which completion mode to default to at 
job creation time, and/or to change that default as the job is running 
(which also implies being able to query which completion mode is 
currently tied to the job, if you can complete without requesting either 
of the two modes explicitly).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort
  2018-09-04 19:30       ` Eric Blake
@ 2018-09-04 20:10         ` John Snow
  2018-09-05  9:54         ` Kevin Wolf
  1 sibling, 0 replies; 30+ messages in thread
From: John Snow @ 2018-09-04 20:10 UTC (permalink / raw)
  To: Eric Blake, Max Reitz, qemu-devel, qemu-block
  Cc: jtc, Kevin Wolf, Markus Armbruster, Dr. David Alan Gilbert, Jeff Cody



On 09/04/2018 03:30 PM, Eric Blake wrote:
> On 09/04/2018 02:15 PM, John Snow wrote:
> 
>>
>> post-script: I really, really hate the "fake cancel" we've implemented
>> for mirror. It makes the job logic so much worse.
> 
> Mirror really does have a tri-state way to end the job (and accessible
> only after the job has moved into the sync state):
> 
> cancel (gracefully end the job by detaching the mirror, so that the
> original volume remains active - your "fake cancel")
> complete (gracefully end the job by pivoting to the mirror, so that the
> original volume is now the backup)
> cancel --force (end the job now, even though it means you did not
> actually create a mirrored copy) - only useful since commit b76e4458 (2.12)
> 
> The first two are conceptually similar - the job succeeds, and one of
> the two files used in the mirroring is now the state of the other at the
> time the job concluded.
> 

Sure, but I dislike how we implemented it as cancel, which mucks up the
job mechanisms. I recognize there are two fully valid ways in which we
want to successfully complete a mirror job.

> Back-compat says we can't really change the block-job terminology
> without an adequate deprecation cycle (and I don't know if libvirt has
> even been taught about cancel --force yet); but for the newer 'job'
> functionality (which we tried to shoehorn existing block jobs into), it
> does seem like it would be nicer to have 'cancel' mean what blockjob
> cancel --force implies, and instead focus on teaching 'complete' to have
> a way to select which of the two completion modes are desired (complete
> by return to original, or complete by pivot to mirror).  It might even
> be nice to have a way to specify which completion mode to default to at
> job creation time, and/or to change that default as the job is running
> (which also implies being able to query which completion mode is
> currently tied to the job, if you can complete without requesting either
> of the two modes explicitly).
> 

Yup, that's my preference!

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort
  2018-09-04 19:30       ` Eric Blake
  2018-09-04 20:10         ` John Snow
@ 2018-09-05  9:54         ` Kevin Wolf
  2018-09-05 12:59           ` John Snow
  1 sibling, 1 reply; 30+ messages in thread
From: Kevin Wolf @ 2018-09-05  9:54 UTC (permalink / raw)
  To: Eric Blake
  Cc: John Snow, Max Reitz, qemu-devel, qemu-block, jtc,
	Markus Armbruster, Dr. David Alan Gilbert, Jeff Cody

Am 04.09.2018 um 21:30 hat Eric Blake geschrieben:
> On 09/04/2018 02:15 PM, John Snow wrote:
> 
> > 
> > post-script: I really, really hate the "fake cancel" we've implemented
> > for mirror. It makes the job logic so much worse.
> 
> Mirror really does have a tri-state way to end the job (and accessible only
> after the job has moved into the sync state):
> 
> cancel (gracefully end the job by detaching the mirror, so that the original
> volume remains active - your "fake cancel")
> complete (gracefully end the job by pivoting to the mirror, so that the
> original volume is now the backup)
> cancel --force (end the job now, even though it means you did not actually
> create a mirrored copy) - only useful since commit b76e4458 (2.12)
> 
> The first two are conceptually similar - the job succeeds, and one of the
> two files used in the mirroring is now the state of the other at the time
> the job concluded.
> 
> Back-compat says we can't really change the block-job terminology without an
> adequate deprecation cycle (and I don't know if libvirt has even been taught
> about cancel --force yet); but for the newer 'job' functionality (which we
> tried to shoehorn existing block jobs into), it does seem like it would be
> nicer to have 'cancel' mean what blockjob cancel --force implies, and
> instead focus on teaching 'complete' to have a way to select which of the
> two completion modes are desired (complete by return to original, or
> complete by pivot to mirror).  It might even be nice to have a way to
> specify which completion mode to default to at job creation time, and/or to
> change that default as the job is running (which also implies being able to
> query which completion mode is currently tied to the job, if you can
> complete without requesting either of the two modes explicitly).

I think it's better to select the completion mode in the job-complete
command than having to specify it from the start.

Once we have support multiple completion modes (even just internally),
nothing stops us from moving the "fake cancel" logic to the QMP layer
(i.e. the drive-mirror/blockdev-mirror implementations) and get rid of
it in the actual job.

Kevin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort
  2018-09-05  9:54         ` Kevin Wolf
@ 2018-09-05 12:59           ` John Snow
  0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2018-09-05 12:59 UTC (permalink / raw)
  To: Kevin Wolf, Eric Blake
  Cc: Max Reitz, qemu-devel, qemu-block, jtc, Markus Armbruster,
	Dr. David Alan Gilbert, Jeff Cody



On 09/05/2018 05:54 AM, Kevin Wolf wrote:
> Am 04.09.2018 um 21:30 hat Eric Blake geschrieben:
>> On 09/04/2018 02:15 PM, John Snow wrote:
>>
>>>
>>> post-script: I really, really hate the "fake cancel" we've implemented
>>> for mirror. It makes the job logic so much worse.
>>
>> Mirror really does have a tri-state way to end the job (and accessible only
>> after the job has moved into the sync state):
>>
>> cancel (gracefully end the job by detaching the mirror, so that the original
>> volume remains active - your "fake cancel")
>> complete (gracefully end the job by pivoting to the mirror, so that the
>> original volume is now the backup)
>> cancel --force (end the job now, even though it means you did not actually
>> create a mirrored copy) - only useful since commit b76e4458 (2.12)
>>
>> The first two are conceptually similar - the job succeeds, and one of the
>> two files used in the mirroring is now the state of the other at the time
>> the job concluded.
>>
>> Back-compat says we can't really change the block-job terminology without an
>> adequate deprecation cycle (and I don't know if libvirt has even been taught
>> about cancel --force yet); but for the newer 'job' functionality (which we
>> tried to shoehorn existing block jobs into), it does seem like it would be
>> nicer to have 'cancel' mean what blockjob cancel --force implies, and
>> instead focus on teaching 'complete' to have a way to select which of the
>> two completion modes are desired (complete by return to original, or
>> complete by pivot to mirror).  It might even be nice to have a way to
>> specify which completion mode to default to at job creation time, and/or to
>> change that default as the job is running (which also implies being able to
>> query which completion mode is currently tied to the job, if you can
>> complete without requesting either of the two modes explicitly).
> 
> I think it's better to select the completion mode in the job-complete
> command than having to specify it from the start.
> 
> Once we have support multiple completion modes (even just internally),
> nothing stops us from moving the "fake cancel" logic to the QMP layer
> (i.e. the drive-mirror/blockdev-mirror implementations) and get rid of
> it in the actual job.
> 

Oh! I hadn't considered that specific tactic before. That's a very good
idea!

Thanks!

--js

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2018-09-05 12:59 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-31 22:28 [Qemu-devel] [PATCH v3 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 01/15] block/commit: add block job creation flags John Snow
2018-09-01  3:17   ` Jeff Cody
2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 02/15] block/mirror: " John Snow
2018-09-01  3:31   ` Jeff Cody
2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 03/15] block/stream: " John Snow
2018-08-31 22:34   ` Eric Blake
2018-09-01  3:33   ` Jeff Cody
2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 04/15] block/commit: refactor commit to use job callbacks John Snow
2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 05/15] block/mirror: don't install backing chain on abort John Snow
2018-09-03  9:24   ` Max Reitz
2018-09-04 19:15     ` John Snow
2018-09-04 19:30       ` Eric Blake
2018-09-04 20:10         ` John Snow
2018-09-05  9:54         ` Kevin Wolf
2018-09-05 12:59           ` John Snow
2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor John Snow
2018-09-03  9:32   ` Max Reitz
2018-09-04 16:17     ` John Snow
2018-08-31 22:28 ` [Qemu-devel] [PATCH v3 07/15] block/commit: refactor stream to use job callbacks John Snow
2018-09-03  9:33   ` Max Reitz
2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 08/15] tests/blockjob: replace Blockjob with Job John Snow
2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 09/15] tests/test-blockjob: remove exit callback John Snow
2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 10/15] tests/test-blockjob-txn: move .exit to .clean John Snow
2018-09-03  9:41   ` Max Reitz
2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 11/15] jobs: remove .exit callback John Snow
2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 12/15] qapi/block-commit: expose new job properties John Snow
2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 13/15] qapi/block-mirror: " John Snow
2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 14/15] qapi/block-stream: " John Snow
2018-08-31 22:29 ` [Qemu-devel] [PATCH v3 15/15] block/backup: qapi documentation fixup John Snow

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.