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

====
"V4"
====

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

001/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:[----] [--] 'block/commit: refactor commit to use job callbacks'
005/15:[0024] [FC] 'block/mirror: don't install backing chain on abort'
006/15:[0007] [FC] 'block/mirror: conservative mirror_exit refactor'
007/15:[down] 'block/stream: refactor stream to use job callbacks'
008/15:[----] [--] 'tests/blockjob: replace Blockjob with Job'
009/15:[----] [--] 'tests/test-blockjob: remove exit callback'
010/15:[----] [--] 'tests/test-blockjob-txn: move .exit to .clean'
011/15:[----] [--] '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'

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

====
"V3"
====

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

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

John Snow (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/stream: refactor stream to use job callbacks
  tests/blockjob: replace Blockjob with Job
  tests/test-blockjob: remove exit callback
  tests/test-blockjob-txn: move .exit to .clean
  jobs: remove .exit callback
  qapi/block-commit: expose new job properties
  qapi/block-mirror: expose new job properties
  qapi/block-stream: expose new job properties
  block/backup: qapi documentation fixup

 block/commit.c            |  95 +++++++++++++++++++++-----------------
 block/mirror.c            |  44 ++++++++++++------
 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, 315 insertions(+), 202 deletions(-)

-- 
2.14.4

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

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

Add support for taking and passing forward job creation flags.

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

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

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

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

Add support for taking and passing forward job creation flags.

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

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

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

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

Add support for taking and passing forward job creation flags.

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

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

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

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

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

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

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

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

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

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

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

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

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

* [Qemu-devel] [PATCH v4 10/15] tests/test-blockjob-txn: move .exit to .clean
  2018-09-04 17:09 [Qemu-devel] [PATCH v4 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
                   ` (8 preceding siblings ...)
  2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 09/15] tests/test-blockjob: remove exit callback John Snow
@ 2018-09-04 17:09 ` John Snow
  2018-09-05 10:45   ` Max Reitz
  2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 11/15] jobs: remove .exit callback John Snow
                   ` (4 subsequent siblings)
  14 siblings, 1 reply; 31+ messages in thread
From: John Snow @ 2018-09-04 17:09 UTC (permalink / raw)
  To: qemu-block, qemu-devel
  Cc: Dr. David Alan Gilbert, Jeff Cody, Markus Armbruster, Kevin Wolf,
	jtc, Max Reitz, Eric Blake, 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] 31+ messages in thread

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

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

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

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

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

* Re: [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks
  2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks John Snow
@ 2018-09-04 18:46   ` Jeff Cody
  2018-09-04 20:32     ` John Snow
  0 siblings, 1 reply; 31+ messages in thread
From: Jeff Cody @ 2018-09-04 18:46 UTC (permalink / raw)
  To: John Snow
  Cc: qemu-block, qemu-devel, Dr. David Alan Gilbert,
	Markus Armbruster, Kevin Wolf, Max Reitz, Eric Blake

On Tue, Sep 04, 2018 at 01:09:19PM -0400, John Snow wrote:
> Use the component callbacks; prepare, abort, and clean.
> 
> NB: prepare is only called when the job has not yet failed;
> and abort can be called after prepare.
> 
> complete -> prepare -> abort -> clean
> complete -> abort -> clean
> 
> 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);
> +}

If we can go from prepare->abort->clean, then that means to me that every
failure case of .prepare() can be resolved without permanent changes / data
loss.  Is this necessarily the case?

>From bdrv_drop_intermediate():

    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
        /* Check whether we are allowed to switch c from top to base */
        GSList *ignore_children = g_slist_prepend(NULL, c);
        bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
                               ignore_children, &local_err);
        g_slist_free(ignore_children);
        if (local_err) {
            ret = -EPERM;
            error_report_err(local_err);
            goto exit;
        }

        /* If so, update the backing file path in the image file */
        if (c->role->update_filename) {
            ret = c->role->update_filename(c, base, backing_file_str,
                                           &local_err);
            if (ret < 0) {
                bdrv_abort_perm_update(base);
                error_report_err(local_err);
                goto exit;
            }
        }

        [...]
     }

We could fail this but still have modified an image file backing filenames,
right?

Or am I incorrect about the intention here, that abort() can always be clean?

-Jeff

> +
> +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	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks
  2018-09-04 18:46   ` Jeff Cody
@ 2018-09-04 20:32     ` John Snow
  2018-09-05 10:27       ` Max Reitz
  0 siblings, 1 reply; 31+ messages in thread
From: John Snow @ 2018-09-04 20:32 UTC (permalink / raw)
  To: Jeff Cody
  Cc: qemu-block, qemu-devel, Dr. David Alan Gilbert,
	Markus Armbruster, Kevin Wolf, Max Reitz, Eric Blake



On 09/04/2018 02:46 PM, Jeff Cody wrote:
> On Tue, Sep 04, 2018 at 01:09:19PM -0400, John Snow wrote:
>> Use the component callbacks; prepare, abort, and clean.
>>
>> NB: prepare is only called when the job has not yet failed;
>> and abort can be called after prepare.
>>
>> complete -> prepare -> abort -> clean
>> complete -> abort -> clean
>>
>> 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);
>> +}
> 
> If we can go from prepare->abort->clean, then that means to me that every
> failure case of .prepare() can be resolved without permanent changes / data
> loss.  Is this necessarily the case?
> 

That'd be a requisite to make the job a transaction, but commit, mirror
and stream are not currently transactionable.

The way commit already works, for example, can leave the base and
intermediate images as unusable as standalone images. This refactoring
will not change that alone.

So it's not necessarily a problem, but it's something that would need to
be fixed if we ever wanted transaction support.

However, in talking on IRC we did realize that this patch does change
behavior...

Before:

If bdrv_drop_intermediate fails, we store the retcode but continue
cleaning up as if it didn't fail. i.e., we don't remove the commit job's
installed top_bs node.

After:

if bdrv_drop_intermediate fails, we return the failure retcode and
.abort gets called as a result, i.e. we will remove the commit job's
installed top_bs node in favor of the original top_bs node.

I think this behavior is an improvement, however it raises a question
about the nature of failures in bdrv_drop_intermediate.

If this function fails without making any changes, the new commit
behavior is good. If it succeeds, we're also good. The problem is with
intermediate or partial successes.

If top has multiple parents (I think under normal circumstances it
won't, but I'm not absolutely sure) and it fails to update their backing
file references, it might partially succeed.

I think commit's usage here is correct, but I think we might need to
update bdrv_drop_intermediate to make it roll back changes if it
experiences a partial failure to give all-or-nothing semantics.

Thoughts?

> From bdrv_drop_intermediate():
> 
>     QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
>         /* Check whether we are allowed to switch c from top to base */
>         GSList *ignore_children = g_slist_prepend(NULL, c);
>         bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
>                                ignore_children, &local_err);
>         g_slist_free(ignore_children);
>         if (local_err) {
>             ret = -EPERM;
>             error_report_err(local_err);
>             goto exit;
>         }
> 
>         /* If so, update the backing file path in the image file */
>         if (c->role->update_filename) {
>             ret = c->role->update_filename(c, base, backing_file_str,
>                                            &local_err);
>             if (ret < 0) {
>                 bdrv_abort_perm_update(base);
>                 error_report_err(local_err);
>                 goto exit;
>             }
>         }
> 
>         [...]
>      }
> 
> We could fail this but still have modified an image file backing filenames,
> right?
> 
> Or am I incorrect about the intention here, that abort() can always be clean?
> 
> -Jeff
> 
>> +
>> +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	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks
  2018-09-04 20:32     ` John Snow
@ 2018-09-05 10:27       ` Max Reitz
  2018-09-05 10:49         ` Kevin Wolf
  2018-09-05 19:05         ` John Snow
  0 siblings, 2 replies; 31+ messages in thread
From: Max Reitz @ 2018-09-05 10:27 UTC (permalink / raw)
  To: John Snow, Jeff Cody
  Cc: qemu-block, qemu-devel, Dr. David Alan Gilbert,
	Markus Armbruster, Kevin Wolf, Eric Blake

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

On 2018-09-04 22:32, John Snow wrote:
> 
> 
> On 09/04/2018 02:46 PM, Jeff Cody wrote:
>> On Tue, Sep 04, 2018 at 01:09:19PM -0400, John Snow wrote:
>>> Use the component callbacks; prepare, abort, and clean.
>>>
>>> NB: prepare is only called when the job has not yet failed;
>>> and abort can be called after prepare.
>>>
>>> complete -> prepare -> abort -> clean
>>> complete -> abort -> clean
>>>
>>> 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);
>>> +}
>>
>> If we can go from prepare->abort->clean, then that means to me that every
>> failure case of .prepare() can be resolved without permanent changes / data
>> loss.  Is this necessarily the case?
>>
> 
> That'd be a requisite to make the job a transaction, but commit, mirror
> and stream are not currently transactionable.

Is that already documented anywhere?

(Otherwise I'd be afraid of us forgetting in like a year, asking "Why
isn't this a transaction already?", just making it one, and then
remembering half a year later.)

> The way commit already works, for example, can leave the base and
> intermediate images as unusable as standalone images. This refactoring
> will not change that alone.
> 
> So it's not necessarily a problem, but it's something that would need to
> be fixed if we ever wanted transaction support.
> 
> However, in talking on IRC we did realize that this patch does change
> behavior...
> 
> Before:
> 
> If bdrv_drop_intermediate fails, we store the retcode but continue
> cleaning up as if it didn't fail. i.e., we don't remove the commit job's
> installed top_bs node.
> 
> After:
> 
> if bdrv_drop_intermediate fails, we return the failure retcode and
> .abort gets called as a result, i.e. we will remove the commit job's
> installed top_bs node in favor of the original top_bs node.
> 
> I think this behavior is an improvement,

I agree.

> however it raises a question
> about the nature of failures in bdrv_drop_intermediate.
> 
> If this function fails without making any changes, the new commit
> behavior is good. If it succeeds, we're also good. The problem is with
> intermediate or partial successes.
> 
> If top has multiple parents (I think under normal circumstances it
> won't, but I'm not absolutely sure) and it fails to update their backing
> file references, it might partially succeed.
> 
> I think commit's usage here is correct, but I think we might need to
> update bdrv_drop_intermediate to make it roll back changes if it
> experiences a partial failure to give all-or-nothing semantics.

Sure, that would be good.

> Thoughts?

We could start by calling bdrv_check_update_perm() on all parents before
doing any changes.  Then the roll back would consist only of invoking
bdrv_abort_perm_update() and in theory reverting the
c->update_filename() changes.

In practice...  How do we want to revert c->update_filename()?  There
currently is no way of getting the old value.  (And just using the old
child's filename may well be wrong, because the old child might not be
the one referenced by the image header.)

I have three ideas:
1) We could introduce a way of getting the old filename the parent has,
so we can restore it.

2) We could make .update_filename() kind of transactionable (seems like
overkill, but it would be easier in practice, I think).

3) We basically ignore .update_filename() errors.  We'd still return
them, but we don't abort the graph change operation.  So after
bdrv_drop_intermediate() is done, the graph has been changed
succesfully, or it hasn't changed at all -- whether the filename updates
all went through, that's a different story.

#3 would be the simplest solution.  It's a bit stupid, but it would work
for most problems, I think; at least the callers would know that the
graph is in exactly one of two well-defined states.

Max


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

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

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

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

On 2018-09-04 19:09, 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.

Sounds a bit weird now that you don't do any moving.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index cba555b4ef..3365bcfdfb 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -642,7 +642,7 @@ static void mirror_exit(Job *job)
>       * required before it could become a backing file of target_bs. */
>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>                              &error_abort);
> -    if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> +    if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>          BlockDriverState *backing = s->is_none_mode ? src : s->base;
>          if (backing_bs(target_bs) != backing) {
>              bdrv_set_backing_hd(target_bs, backing, &local_err);
> @@ -659,10 +659,7 @@ 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 (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
>              bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);

And this hunk now looks out of place.  Sure, it makes sense, but why is
it in this patch now? :-)

(Moving it into the next patch would make more sense, I think.)

I'd like to give an R-b anyway, but I know that I shouldn't, so I won't.

Max


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

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

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

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

On 2018-09-04 19:09, 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 | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)

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

(Although I believe the ?: hunk from the previous patch should be here.
Also note that we have a couple of places that make use of the GNU
extension for "?:" as a binary operator (as in "x ?: y" returns x if
x != 0).  Just in case you find "s->to_replace ?: src" as appealing as I
do.)


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

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

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

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

On 2018-09-04 19:09, 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] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks
  2018-09-05 10:27       ` Max Reitz
@ 2018-09-05 10:49         ` Kevin Wolf
  2018-09-05 11:37           ` Max Reitz
  2018-09-05 19:05         ` John Snow
  1 sibling, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2018-09-05 10:49 UTC (permalink / raw)
  To: Max Reitz
  Cc: John Snow, Jeff Cody, qemu-block, qemu-devel,
	Dr. David Alan Gilbert, Markus Armbruster, Eric Blake

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

Am 05.09.2018 um 12:27 hat Max Reitz geschrieben:
> On 2018-09-04 22:32, John Snow wrote:
> > 
> > 
> > On 09/04/2018 02:46 PM, Jeff Cody wrote:
> >> On Tue, Sep 04, 2018 at 01:09:19PM -0400, John Snow wrote:
> >>> Use the component callbacks; prepare, abort, and clean.
> >>>
> >>> NB: prepare is only called when the job has not yet failed;
> >>> and abort can be called after prepare.
> >>>
> >>> complete -> prepare -> abort -> clean
> >>> complete -> abort -> clean
> >>>
> >>> 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);
> >>> +}
> >>
> >> If we can go from prepare->abort->clean, then that means to me that every
> >> failure case of .prepare() can be resolved without permanent changes / data
> >> loss.  Is this necessarily the case?
> >>
> > 
> > That'd be a requisite to make the job a transaction, but commit, mirror
> > and stream are not currently transactionable.
> 
> Is that already documented anywhere?
> 
> (Otherwise I'd be afraid of us forgetting in like a year, asking "Why
> isn't this a transaction already?", just making it one, and then
> remembering half a year later.)
> 
> > The way commit already works, for example, can leave the base and
> > intermediate images as unusable as standalone images. This refactoring
> > will not change that alone.
> > 
> > So it's not necessarily a problem, but it's something that would need to
> > be fixed if we ever wanted transaction support.
> > 
> > However, in talking on IRC we did realize that this patch does change
> > behavior...
> > 
> > Before:
> > 
> > If bdrv_drop_intermediate fails, we store the retcode but continue
> > cleaning up as if it didn't fail. i.e., we don't remove the commit job's
> > installed top_bs node.
> > 
> > After:
> > 
> > if bdrv_drop_intermediate fails, we return the failure retcode and
> > .abort gets called as a result, i.e. we will remove the commit job's
> > installed top_bs node in favor of the original top_bs node.
> > 
> > I think this behavior is an improvement,
> 
> I agree.
> 
> > however it raises a question
> > about the nature of failures in bdrv_drop_intermediate.
> > 
> > If this function fails without making any changes, the new commit
> > behavior is good. If it succeeds, we're also good. The problem is with
> > intermediate or partial successes.
> > 
> > If top has multiple parents (I think under normal circumstances it
> > won't, but I'm not absolutely sure) and it fails to update their backing
> > file references, it might partially succeed.
> > 
> > I think commit's usage here is correct, but I think we might need to
> > update bdrv_drop_intermediate to make it roll back changes if it
> > experiences a partial failure to give all-or-nothing semantics.
> 
> Sure, that would be good.
> 
> > Thoughts?
> 
> We could start by calling bdrv_check_update_perm() on all parents before
> doing any changes.  Then the roll back would consist only of invoking
> bdrv_abort_perm_update() and in theory reverting the
> c->update_filename() changes.
> 
> In practice...  How do we want to revert c->update_filename()?  There
> currently is no way of getting the old value.  (And just using the old
> child's filename may well be wrong, because the old child might not be
> the one referenced by the image header.)
> 
> I have three ideas:
> 1) We could introduce a way of getting the old filename the parent has,
> so we can restore it.
> 
> 2) We could make .update_filename() kind of transactionable (seems like
> overkill, but it would be easier in practice, I think).
> 
> 3) We basically ignore .update_filename() errors.  We'd still return
> them, but we don't abort the graph change operation.  So after
> bdrv_drop_intermediate() is done, the graph has been changed
> succesfully, or it hasn't changed at all -- whether the filename updates
> all went through, that's a different story.
> 
> #3 would be the simplest solution.  It's a bit stupid, but it would work
> for most problems, I think; at least the callers would know that the
> graph is in exactly one of two well-defined states.

Option 2 sounds nice in theory, but how do you make things
transactionable when they require writing to an image? Either you make
the change or you don't. If you make it and later notice that you
shouldn't have done so, you can try to write the old value back, but
that can fail, too.

So making .update_filename() transactionable is probably not feasible.
The choice that is left is whether we update it in .prepare (and
continue with .abort if it fails) or in .commit (and ignore errors).

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

* Re: [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks
  2018-09-05 10:49         ` Kevin Wolf
@ 2018-09-05 11:37           ` Max Reitz
  2018-09-05 11:53             ` Kevin Wolf
  0 siblings, 1 reply; 31+ messages in thread
From: Max Reitz @ 2018-09-05 11:37 UTC (permalink / raw)
  To: Kevin Wolf
  Cc: John Snow, Jeff Cody, qemu-block, qemu-devel,
	Dr. David Alan Gilbert, Markus Armbruster, Eric Blake

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

On 2018-09-05 12:49, Kevin Wolf wrote:
> Am 05.09.2018 um 12:27 hat Max Reitz geschrieben:
>> On 2018-09-04 22:32, John Snow wrote:
>>>
>>>
>>> On 09/04/2018 02:46 PM, Jeff Cody wrote:
>>>> On Tue, Sep 04, 2018 at 01:09:19PM -0400, John Snow wrote:
>>>>> Use the component callbacks; prepare, abort, and clean.
>>>>>
>>>>> NB: prepare is only called when the job has not yet failed;
>>>>> and abort can be called after prepare.
>>>>>
>>>>> complete -> prepare -> abort -> clean
>>>>> complete -> abort -> clean
>>>>>
>>>>> 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);
>>>>> +}
>>>>
>>>> If we can go from prepare->abort->clean, then that means to me that every
>>>> failure case of .prepare() can be resolved without permanent changes / data
>>>> loss.  Is this necessarily the case?
>>>>
>>>
>>> That'd be a requisite to make the job a transaction, but commit, mirror
>>> and stream are not currently transactionable.
>>
>> Is that already documented anywhere?
>>
>> (Otherwise I'd be afraid of us forgetting in like a year, asking "Why
>> isn't this a transaction already?", just making it one, and then
>> remembering half a year later.)
>>
>>> The way commit already works, for example, can leave the base and
>>> intermediate images as unusable as standalone images. This refactoring
>>> will not change that alone.
>>>
>>> So it's not necessarily a problem, but it's something that would need to
>>> be fixed if we ever wanted transaction support.
>>>
>>> However, in talking on IRC we did realize that this patch does change
>>> behavior...
>>>
>>> Before:
>>>
>>> If bdrv_drop_intermediate fails, we store the retcode but continue
>>> cleaning up as if it didn't fail. i.e., we don't remove the commit job's
>>> installed top_bs node.
>>>
>>> After:
>>>
>>> if bdrv_drop_intermediate fails, we return the failure retcode and
>>> .abort gets called as a result, i.e. we will remove the commit job's
>>> installed top_bs node in favor of the original top_bs node.
>>>
>>> I think this behavior is an improvement,
>>
>> I agree.
>>
>>> however it raises a question
>>> about the nature of failures in bdrv_drop_intermediate.
>>>
>>> If this function fails without making any changes, the new commit
>>> behavior is good. If it succeeds, we're also good. The problem is with
>>> intermediate or partial successes.
>>>
>>> If top has multiple parents (I think under normal circumstances it
>>> won't, but I'm not absolutely sure) and it fails to update their backing
>>> file references, it might partially succeed.
>>>
>>> I think commit's usage here is correct, but I think we might need to
>>> update bdrv_drop_intermediate to make it roll back changes if it
>>> experiences a partial failure to give all-or-nothing semantics.
>>
>> Sure, that would be good.
>>
>>> Thoughts?
>>
>> We could start by calling bdrv_check_update_perm() on all parents before
>> doing any changes.  Then the roll back would consist only of invoking
>> bdrv_abort_perm_update() and in theory reverting the
>> c->update_filename() changes.
>>
>> In practice...  How do we want to revert c->update_filename()?  There
>> currently is no way of getting the old value.  (And just using the old
>> child's filename may well be wrong, because the old child might not be
>> the one referenced by the image header.)
>>
>> I have three ideas:
>> 1) We could introduce a way of getting the old filename the parent has,
>> so we can restore it.
>>
>> 2) We could make .update_filename() kind of transactionable (seems like
>> overkill, but it would be easier in practice, I think).
>>
>> 3) We basically ignore .update_filename() errors.  We'd still return
>> them, but we don't abort the graph change operation.  So after
>> bdrv_drop_intermediate() is done, the graph has been changed
>> succesfully, or it hasn't changed at all -- whether the filename updates
>> all went through, that's a different story.
>>
>> #3 would be the simplest solution.  It's a bit stupid, but it would work
>> for most problems, I think; at least the callers would know that the
>> graph is in exactly one of two well-defined states.
> 
> Option 2 sounds nice in theory, but how do you make things
> transactionable when they require writing to an image? Either you make
> the change or you don't. If you make it and later notice that you
> shouldn't have done so, you can try to write the old value back, but
> that can fail, too.

Yes, that's what I mean by "kind of".  It's just #1 with the code moved
somewhere else.

> So making .update_filename() transactionable is probably not feasible.
> The choice that is left is whether we update it in .prepare (and
> continue with .abort if it fails) or in .commit (and ignore errors).

My idea was "if we could update it one time, we can probably update it a
second time", so reverting in .abort() would usually work.  I know that
a "usually" is not a "definitely", so that's the "kind of" again.

So the abort could still throw an error which we would have to handle
like in #3 anyway, I suppose.

Max


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

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

* Re: [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks
  2018-09-05 11:37           ` Max Reitz
@ 2018-09-05 11:53             ` Kevin Wolf
  2018-09-05 12:25               ` Max Reitz
  0 siblings, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2018-09-05 11:53 UTC (permalink / raw)
  To: Max Reitz
  Cc: John Snow, Jeff Cody, qemu-block, qemu-devel,
	Dr. David Alan Gilbert, Markus Armbruster, Eric Blake

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

Am 05.09.2018 um 13:37 hat Max Reitz geschrieben:
> On 2018-09-05 12:49, Kevin Wolf wrote:
> > Am 05.09.2018 um 12:27 hat Max Reitz geschrieben:
> >> On 2018-09-04 22:32, John Snow wrote:
> >>>
> >>>
> >>> On 09/04/2018 02:46 PM, Jeff Cody wrote:
> >>>> On Tue, Sep 04, 2018 at 01:09:19PM -0400, John Snow wrote:
> >>>>> Use the component callbacks; prepare, abort, and clean.
> >>>>>
> >>>>> NB: prepare is only called when the job has not yet failed;
> >>>>> and abort can be called after prepare.
> >>>>>
> >>>>> complete -> prepare -> abort -> clean
> >>>>> complete -> abort -> clean
> >>>>>
> >>>>> 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);
> >>>>> +}
> >>>>
> >>>> If we can go from prepare->abort->clean, then that means to me that every
> >>>> failure case of .prepare() can be resolved without permanent changes / data
> >>>> loss.  Is this necessarily the case?
> >>>>
> >>>
> >>> That'd be a requisite to make the job a transaction, but commit, mirror
> >>> and stream are not currently transactionable.
> >>
> >> Is that already documented anywhere?
> >>
> >> (Otherwise I'd be afraid of us forgetting in like a year, asking "Why
> >> isn't this a transaction already?", just making it one, and then
> >> remembering half a year later.)
> >>
> >>> The way commit already works, for example, can leave the base and
> >>> intermediate images as unusable as standalone images. This refactoring
> >>> will not change that alone.
> >>>
> >>> So it's not necessarily a problem, but it's something that would need to
> >>> be fixed if we ever wanted transaction support.
> >>>
> >>> However, in talking on IRC we did realize that this patch does change
> >>> behavior...
> >>>
> >>> Before:
> >>>
> >>> If bdrv_drop_intermediate fails, we store the retcode but continue
> >>> cleaning up as if it didn't fail. i.e., we don't remove the commit job's
> >>> installed top_bs node.
> >>>
> >>> After:
> >>>
> >>> if bdrv_drop_intermediate fails, we return the failure retcode and
> >>> .abort gets called as a result, i.e. we will remove the commit job's
> >>> installed top_bs node in favor of the original top_bs node.
> >>>
> >>> I think this behavior is an improvement,
> >>
> >> I agree.
> >>
> >>> however it raises a question
> >>> about the nature of failures in bdrv_drop_intermediate.
> >>>
> >>> If this function fails without making any changes, the new commit
> >>> behavior is good. If it succeeds, we're also good. The problem is with
> >>> intermediate or partial successes.
> >>>
> >>> If top has multiple parents (I think under normal circumstances it
> >>> won't, but I'm not absolutely sure) and it fails to update their backing
> >>> file references, it might partially succeed.
> >>>
> >>> I think commit's usage here is correct, but I think we might need to
> >>> update bdrv_drop_intermediate to make it roll back changes if it
> >>> experiences a partial failure to give all-or-nothing semantics.
> >>
> >> Sure, that would be good.
> >>
> >>> Thoughts?
> >>
> >> We could start by calling bdrv_check_update_perm() on all parents before
> >> doing any changes.  Then the roll back would consist only of invoking
> >> bdrv_abort_perm_update() and in theory reverting the
> >> c->update_filename() changes.
> >>
> >> In practice...  How do we want to revert c->update_filename()?  There
> >> currently is no way of getting the old value.  (And just using the old
> >> child's filename may well be wrong, because the old child might not be
> >> the one referenced by the image header.)
> >>
> >> I have three ideas:
> >> 1) We could introduce a way of getting the old filename the parent has,
> >> so we can restore it.
> >>
> >> 2) We could make .update_filename() kind of transactionable (seems like
> >> overkill, but it would be easier in practice, I think).
> >>
> >> 3) We basically ignore .update_filename() errors.  We'd still return
> >> them, but we don't abort the graph change operation.  So after
> >> bdrv_drop_intermediate() is done, the graph has been changed
> >> succesfully, or it hasn't changed at all -- whether the filename updates
> >> all went through, that's a different story.
> >>
> >> #3 would be the simplest solution.  It's a bit stupid, but it would work
> >> for most problems, I think; at least the callers would know that the
> >> graph is in exactly one of two well-defined states.
> > 
> > Option 2 sounds nice in theory, but how do you make things
> > transactionable when they require writing to an image? Either you make
> > the change or you don't. If you make it and later notice that you
> > shouldn't have done so, you can try to write the old value back, but
> > that can fail, too.
> 
> Yes, that's what I mean by "kind of".  It's just #1 with the code moved
> somewhere else.
> 
> > So making .update_filename() transactionable is probably not feasible.
> > The choice that is left is whether we update it in .prepare (and
> > continue with .abort if it fails) or in .commit (and ignore errors).
> 
> My idea was "if we could update it one time, we can probably update it a
> second time", so reverting in .abort() would usually work.  I know that
> a "usually" is not a "definitely", so that's the "kind of" again.
> 
> So the abort could still throw an error which we would have to handle
> like in #3 anyway, I suppose.

The question is whether it's better to have the new filename on-disk
when the transaction aborted and the graph stays as it was (if reverting
in .abort fails), or to have the old filename there when the transaction
succeeded.

Both are confusing, but confusing error cases are a bit more reasonable
than confusing success cases, so I tend to agree that updating in
.prepare and reverting in .abort is probably the best we can do.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

* Re: [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks
  2018-09-05 11:53             ` Kevin Wolf
@ 2018-09-05 12:25               ` Max Reitz
  0 siblings, 0 replies; 31+ messages in thread
From: Max Reitz @ 2018-09-05 12:25 UTC (permalink / raw)
  To: Kevin Wolf
  Cc: John Snow, Jeff Cody, qemu-block, qemu-devel,
	Dr. David Alan Gilbert, Markus Armbruster, Eric Blake

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

On 2018-09-05 13:53, Kevin Wolf wrote:
> Am 05.09.2018 um 13:37 hat Max Reitz geschrieben:
>> On 2018-09-05 12:49, Kevin Wolf wrote:
>>> Am 05.09.2018 um 12:27 hat Max Reitz geschrieben:
>>>> On 2018-09-04 22:32, John Snow wrote:
>>>>>
>>>>>
>>>>> On 09/04/2018 02:46 PM, Jeff Cody wrote:
>>>>>> On Tue, Sep 04, 2018 at 01:09:19PM -0400, John Snow wrote:
>>>>>>> Use the component callbacks; prepare, abort, and clean.
>>>>>>>
>>>>>>> NB: prepare is only called when the job has not yet failed;
>>>>>>> and abort can be called after prepare.
>>>>>>>
>>>>>>> complete -> prepare -> abort -> clean
>>>>>>> complete -> abort -> clean
>>>>>>>
>>>>>>> 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);
>>>>>>> +}
>>>>>>
>>>>>> If we can go from prepare->abort->clean, then that means to me that every
>>>>>> failure case of .prepare() can be resolved without permanent changes / data
>>>>>> loss.  Is this necessarily the case?
>>>>>>
>>>>>
>>>>> That'd be a requisite to make the job a transaction, but commit, mirror
>>>>> and stream are not currently transactionable.
>>>>
>>>> Is that already documented anywhere?
>>>>
>>>> (Otherwise I'd be afraid of us forgetting in like a year, asking "Why
>>>> isn't this a transaction already?", just making it one, and then
>>>> remembering half a year later.)
>>>>
>>>>> The way commit already works, for example, can leave the base and
>>>>> intermediate images as unusable as standalone images. This refactoring
>>>>> will not change that alone.
>>>>>
>>>>> So it's not necessarily a problem, but it's something that would need to
>>>>> be fixed if we ever wanted transaction support.
>>>>>
>>>>> However, in talking on IRC we did realize that this patch does change
>>>>> behavior...
>>>>>
>>>>> Before:
>>>>>
>>>>> If bdrv_drop_intermediate fails, we store the retcode but continue
>>>>> cleaning up as if it didn't fail. i.e., we don't remove the commit job's
>>>>> installed top_bs node.
>>>>>
>>>>> After:
>>>>>
>>>>> if bdrv_drop_intermediate fails, we return the failure retcode and
>>>>> .abort gets called as a result, i.e. we will remove the commit job's
>>>>> installed top_bs node in favor of the original top_bs node.
>>>>>
>>>>> I think this behavior is an improvement,
>>>>
>>>> I agree.
>>>>
>>>>> however it raises a question
>>>>> about the nature of failures in bdrv_drop_intermediate.
>>>>>
>>>>> If this function fails without making any changes, the new commit
>>>>> behavior is good. If it succeeds, we're also good. The problem is with
>>>>> intermediate or partial successes.
>>>>>
>>>>> If top has multiple parents (I think under normal circumstances it
>>>>> won't, but I'm not absolutely sure) and it fails to update their backing
>>>>> file references, it might partially succeed.
>>>>>
>>>>> I think commit's usage here is correct, but I think we might need to
>>>>> update bdrv_drop_intermediate to make it roll back changes if it
>>>>> experiences a partial failure to give all-or-nothing semantics.
>>>>
>>>> Sure, that would be good.
>>>>
>>>>> Thoughts?
>>>>
>>>> We could start by calling bdrv_check_update_perm() on all parents before
>>>> doing any changes.  Then the roll back would consist only of invoking
>>>> bdrv_abort_perm_update() and in theory reverting the
>>>> c->update_filename() changes.
>>>>
>>>> In practice...  How do we want to revert c->update_filename()?  There
>>>> currently is no way of getting the old value.  (And just using the old
>>>> child's filename may well be wrong, because the old child might not be
>>>> the one referenced by the image header.)
>>>>
>>>> I have three ideas:
>>>> 1) We could introduce a way of getting the old filename the parent has,
>>>> so we can restore it.
>>>>
>>>> 2) We could make .update_filename() kind of transactionable (seems like
>>>> overkill, but it would be easier in practice, I think).
>>>>
>>>> 3) We basically ignore .update_filename() errors.  We'd still return
>>>> them, but we don't abort the graph change operation.  So after
>>>> bdrv_drop_intermediate() is done, the graph has been changed
>>>> succesfully, or it hasn't changed at all -- whether the filename updates
>>>> all went through, that's a different story.
>>>>
>>>> #3 would be the simplest solution.  It's a bit stupid, but it would work
>>>> for most problems, I think; at least the callers would know that the
>>>> graph is in exactly one of two well-defined states.
>>>
>>> Option 2 sounds nice in theory, but how do you make things
>>> transactionable when they require writing to an image? Either you make
>>> the change or you don't. If you make it and later notice that you
>>> shouldn't have done so, you can try to write the old value back, but
>>> that can fail, too.
>>
>> Yes, that's what I mean by "kind of".  It's just #1 with the code moved
>> somewhere else.
>>
>>> So making .update_filename() transactionable is probably not feasible.
>>> The choice that is left is whether we update it in .prepare (and
>>> continue with .abort if it fails) or in .commit (and ignore errors).
>>
>> My idea was "if we could update it one time, we can probably update it a
>> second time", so reverting in .abort() would usually work.  I know that
>> a "usually" is not a "definitely", so that's the "kind of" again.
>>
>> So the abort could still throw an error which we would have to handle
>> like in #3 anyway, I suppose.
> 
> The question is whether it's better to have the new filename on-disk
> when the transaction aborted and the graph stays as it was (if reverting
> in .abort fails), or to have the old filename there when the transaction
> succeeded.

Hm.  Thanks for expressing it this way.  Both cases are bad.

In the former case, the user gets pleasant surprise when restarting the
VM and seeing that the commit actually worked, despite having gotten an
error before (and that the graph stayed unchanged).
But the bad thing is that if any more graph manipulations are done on
the basis that the commit failed, things may break when the VM is
restarted.  Like, maybe the user decides to write into the overlay's
backing file (I don't know) and expects to see that data through the
overlay, all of it will be effectively lost when the VM is restarted.

In the latter case, we have basically the same problem.  When the VM is
restarted and all the images we had wanted to discard suddenly reappear,
that may break things in much the same way.
The VM restart surprise here is that the commit seemed to work, but then
you see that it actually kind of didn't?  (Because all the images
reappear.  Maybe you deleted them in the meantime, and then the VM won't
even want to restart.)

So especially the last part seems to swing the pendulum in the direction
of me preferring having the new filename when the transaction failed.

But still, I don't know whether that's really the only question.

I don't quite see how you want to do the update only in .commit(), and
still call it a transaction.  Do you want to do the bdrv_reopen() (for
bdrv_backing_update_filename()) in prepare, and then invoke
bdrv_backing_update_filename() in commit?  A bit better than nothing,
but it's not really a transaction.

As I said, the benefit of doing it in .prepare() and then rolling back
in .abort() would be the idea of "if I can update it once, I can
probably update it twice -- especially if I write a value to it that it
already had before".

Max

> Both are confusing, but confusing error cases are a bit more reasonable
> than confusing success cases, so I tend to agree that updating in
> .prepare and reverting in .abort is probably the best we can do.
> 
> Kevin


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

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

* Re: [Qemu-devel] [PATCH v4 06/15] block/mirror: conservative mirror_exit refactor
  2018-09-05 10:43   ` Max Reitz
@ 2018-09-05 13:09     ` John Snow
  2018-09-05 15:50       ` Eric Blake
  0 siblings, 1 reply; 31+ messages in thread
From: John Snow @ 2018-09-05 13:09 UTC (permalink / raw)
  To: Max Reitz, qemu-block, qemu-devel
  Cc: Dr. David Alan Gilbert, Jeff Cody, Markus Armbruster, Kevin Wolf,
	jtc, Eric Blake

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



On 09/05/2018 06:43 AM, Max Reitz wrote:
> On 2018-09-04 19:09, 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 | 34 +++++++++++++++++++++++++++-------
>>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> (Although I believe the ?: hunk from the previous patch should be here.
> Also note that we have a couple of places that make use of the GNU
> extension for "?:" as a binary operator (as in "x ?: y" returns x if
> x != 0).  Just in case you find "s->to_replace ?: src" as appealing as I
> do.)
> 

Ah, I wasn't sure that was OK to use. Meh, since I goofed up the last
patch I'll use that version.


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

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

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



On 09/05/2018 06:40 AM, Max Reitz wrote:
> On 2018-09-04 19:09, 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.
> 
> Sounds a bit weird now that you don't do any moving.
> 

I ought to proofread my commit messages when I make changes...

>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/mirror.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index cba555b4ef..3365bcfdfb 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -642,7 +642,7 @@ static void mirror_exit(Job *job)
>>       * required before it could become a backing file of target_bs. */
>>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>>                              &error_abort);
>> -    if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>> +    if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>>          BlockDriverState *backing = s->is_none_mode ? src : s->base;
>>          if (backing_bs(target_bs) != backing) {
>>              bdrv_set_backing_hd(target_bs, backing, &local_err);
>> @@ -659,10 +659,7 @@ 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 (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
>>              bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
> 
> And this hunk now looks out of place.  Sure, it makes sense, but why is
> it in this patch now? :-)
> 
> (Moving it into the next patch would make more sense, I think.)
> 
> I'd like to give an R-b anyway, but I know that I shouldn't, so I won't.
> 
> Max
> 

I have to admit that my appetite for patch purity is just... low. I know
it's something we care a lot in the QEMU project, but after a number of
years I'm just not overwhelmed to care about it in any significant capacity.

I suppose the main argument for this practice is ease of backporting, yes?

--js

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

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

On 09/05/2018 08:09 AM, John Snow wrote:
> 
> 
> On 09/05/2018 06:43 AM, Max Reitz wrote:
>> On 2018-09-04 19:09, 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 | 34 +++++++++++++++++++++++++++-------
>>>   1 file changed, 27 insertions(+), 7 deletions(-)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> (Although I believe the ?: hunk from the previous patch should be here.
>> Also note that we have a couple of places that make use of the GNU
>> extension for "?:" as a binary operator (as in "x ?: y" returns x if
>> x != 0).  Just in case you find "s->to_replace ?: src" as appealing as I
>> do.)
>>
> 
> Ah, I wasn't sure that was OK to use. Meh, since I goofed up the last
> patch I'll use that version.

My minor reason for liking 'a ?: b' - it's less typing, and makes 
avoiding long lines easier.  My major reason for liking it: 'a() ?: b' 
only evaluates a() once, unlike 'a() ? a() : b' when taking the true 
branch, which can be particularly important if a() has side effects, but 
is still an efficiency issue even when side effects are not in play. 
Yes, you can always rewrite things to use a temporary variable to avoid 
the ?: operator (which in turn can inflate a single-line expression into 
multiple lines), but there are indeed enough places in the code base 
where we rely on this extension that it doesn't hurt to add more uses.

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

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

* Re: [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks
  2018-09-05 10:27       ` Max Reitz
  2018-09-05 10:49         ` Kevin Wolf
@ 2018-09-05 19:05         ` John Snow
  1 sibling, 0 replies; 31+ messages in thread
From: John Snow @ 2018-09-05 19:05 UTC (permalink / raw)
  To: Max Reitz, Jeff Cody
  Cc: qemu-block, qemu-devel, Dr. David Alan Gilbert,
	Markus Armbruster, Kevin Wolf, Eric Blake



On 09/05/2018 06:27 AM, Max Reitz wrote:
> On 2018-09-04 22:32, John Snow wrote:
>>
>>
>> On 09/04/2018 02:46 PM, Jeff Cody wrote:
>>> On Tue, Sep 04, 2018 at 01:09:19PM -0400, John Snow wrote:
>>>> Use the component callbacks; prepare, abort, and clean.
>>>>
>>>> NB: prepare is only called when the job has not yet failed;
>>>> and abort can be called after prepare.
>>>>
>>>> complete -> prepare -> abort -> clean
>>>> complete -> abort -> clean
>>>>
>>>> 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);
>>>> +}
>>>
>>> If we can go from prepare->abort->clean, then that means to me that every
>>> failure case of .prepare() can be resolved without permanent changes / data
>>> loss.  Is this necessarily the case?
>>>
>>
>> That'd be a requisite to make the job a transaction, but commit, mirror
>> and stream are not currently transactionable.
> 
> Is that already documented anywhere?
> 

Hm, no, not really.

I'm most inclined to document it near the action table because it would
be hard to miss if you went to add it.

I'll add this in an extra patch at the end in case you want to debate
the wording and/or location.

> (Otherwise I'd be afraid of us forgetting in like a year, asking "Why
> isn't this a transaction already?", just making it one, and then
> remembering half a year later.)
> 

No, it's a good point.

>> The way commit already works, for example, can leave the base and
>> intermediate images as unusable as standalone images. This refactoring
>> will not change that alone.
>>
>> So it's not necessarily a problem, but it's something that would need to
>> be fixed if we ever wanted transaction support.
>>
>> However, in talking on IRC we did realize that this patch does change
>> behavior...
>>
>> Before:
>>
>> If bdrv_drop_intermediate fails, we store the retcode but continue
>> cleaning up as if it didn't fail. i.e., we don't remove the commit job's
>> installed top_bs node.
>>
>> After:
>>
>> if bdrv_drop_intermediate fails, we return the failure retcode and
>> .abort gets called as a result, i.e. we will remove the commit job's
>> installed top_bs node in favor of the original top_bs node.
>>
>> I think this behavior is an improvement,
> 
> I agree.
> 

Based on this I will leave the stickier fix to a future patch... I will
add a FIXME note detailing the shortfall in this patch, but I am
asserting that the current behavior is /not worse/ than the old
behavior, while there is still a bug that we might need to fix in the
future.

>> however it raises a question
>> about the nature of failures in bdrv_drop_intermediate.
>>
>> If this function fails without making any changes, the new commit
>> behavior is good. If it succeeds, we're also good. The problem is with
>> intermediate or partial successes.
>>
>> If top has multiple parents (I think under normal circumstances it
>> won't, but I'm not absolutely sure) and it fails to update their backing
>> file references, it might partially succeed.
>>
>> I think commit's usage here is correct, but I think we might need to
>> update bdrv_drop_intermediate to make it roll back changes if it
>> experiences a partial failure to give all-or-nothing semantics.
> 
> Sure, that would be good.
> 
>> Thoughts?
> 
> We could start by calling bdrv_check_update_perm() on all parents before
> doing any changes.  Then the roll back would consist only of invoking
> bdrv_abort_perm_update() and in theory reverting the
> c->update_filename() changes.
> 
> In practice...  How do we want to revert c->update_filename()?  There
> currently is no way of getting the old value.  (And just using the old
> child's filename may well be wrong, because the old child might not be
> the one referenced by the image header.)
> 
> I have three ideas:
> 1) We could introduce a way of getting the old filename the parent has,
> so we can restore it.
> 
> 2) We could make .update_filename() kind of transactionable (seems like
> overkill, but it would be easier in practice, I think).
> 
> 3) We basically ignore .update_filename() errors.  We'd still return
> them, but we don't abort the graph change operation.  So after
> bdrv_drop_intermediate() is done, the graph has been changed
> succesfully, or it hasn't changed at all -- whether the filename updates
> all went through, that's a different story.
> 
> #3 would be the simplest solution.  It's a bit stupid, but it would work
> for most problems, I think; at least the callers would know that the
> graph is in exactly one of two well-defined states.
> 
> Max
> 

I suppose another option would be to just update the function to return
which kind of error it had.

If it couldn't update *anything*, we can treat this as a hard failure.
If it managed to update *some things*, we can ignore the error for the
purposes of cleanup, but report the error. "Hey, something... happened.
The commit worked but the graph change failed. Please investigate."
And, of course, success is success.

I don't really need or want "transaction" semantics here, just the
ability to have well-defined ending states. The trinary outcome might be
enough -- it's perhaps not the end of the world to ask for manual
intervention after a failure. It seems conservatively the safest option.

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

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

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

On 2018-09-05 17:39, John Snow wrote:
> 
> 
> On 09/05/2018 06:40 AM, Max Reitz wrote:
>> On 2018-09-04 19:09, 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.
>>
>> Sounds a bit weird now that you don't do any moving.
>>
> 
> I ought to proofread my commit messages when I make changes...
> 
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  block/mirror.c | 7 ++-----
>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index cba555b4ef..3365bcfdfb 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -642,7 +642,7 @@ static void mirror_exit(Job *job)
>>>       * required before it could become a backing file of target_bs. */
>>>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>>>                              &error_abort);
>>> -    if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>>> +    if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>>>          BlockDriverState *backing = s->is_none_mode ? src : s->base;
>>>          if (backing_bs(target_bs) != backing) {
>>>              bdrv_set_backing_hd(target_bs, backing, &local_err);
>>> @@ -659,10 +659,7 @@ 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 (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
>>>              bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
>>
>> And this hunk now looks out of place.  Sure, it makes sense, but why is
>> it in this patch now? :-)
>>
>> (Moving it into the next patch would make more sense, I think.)
>>
>> I'd like to give an R-b anyway, but I know that I shouldn't, so I won't.
>>
>> Max
>>
> 
> I have to admit that my appetite for patch purity is just... low. I know
> it's something we care a lot in the QEMU project, but after a number of
> years I'm just not overwhelmed to care about it in any significant capacity.
> 
> I suppose the main argument for this practice is ease of backporting, yes?

I suppose.  And ease of bisecting.  And ease of reviewing.

Max


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

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

end of thread, other threads:[~2018-09-07 11:40 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-04 17:09 [Qemu-devel] [PATCH v4 00/15] jobs: Job Exit Refactoring Pt 2 John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 01/15] block/commit: add block job creation flags John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 02/15] block/mirror: " John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 03/15] block/stream: " John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks John Snow
2018-09-04 18:46   ` Jeff Cody
2018-09-04 20:32     ` John Snow
2018-09-05 10:27       ` Max Reitz
2018-09-05 10:49         ` Kevin Wolf
2018-09-05 11:37           ` Max Reitz
2018-09-05 11:53             ` Kevin Wolf
2018-09-05 12:25               ` Max Reitz
2018-09-05 19:05         ` John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 05/15] block/mirror: don't install backing chain on abort John Snow
2018-09-05 10:40   ` Max Reitz
2018-09-05 15:39     ` John Snow
2018-09-07 11:40       ` Max Reitz
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 06/15] block/mirror: conservative mirror_exit refactor John Snow
2018-09-05 10:43   ` Max Reitz
2018-09-05 13:09     ` John Snow
2018-09-05 15:50       ` Eric Blake
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 07/15] block/stream: refactor stream to use job callbacks John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 08/15] tests/blockjob: replace Blockjob with Job John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 09/15] tests/test-blockjob: remove exit callback John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 10/15] tests/test-blockjob-txn: move .exit to .clean John Snow
2018-09-05 10:45   ` Max Reitz
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 11/15] jobs: remove .exit callback John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 12/15] qapi/block-commit: expose new job properties John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 13/15] qapi/block-mirror: " John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 14/15] qapi/block-stream: " John Snow
2018-09-04 17:09 ` [Qemu-devel] [PATCH v4 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.