[PATCH v2 00/10] mirror: allow switching from background to active mode

[PATCH v2 00/10] mirror: allow switching from background to active mode

[Fiona Ebner]

Changes in v2:
    * move bitmap to filter which allows to avoid draining when
      changing the copy mode
    * add patch to determine copy_to_target only once
    * drop patches returning redundant information upon query
    * update QEMU version in QAPI
    * update indentation in QAPI
    * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
      doc comments to conform to current conventions"))
    * add patch to adapt iotest output

Discussion of v1:
https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg07216.html

With active mode, the guest write speed is limited by the synchronous
writes to the mirror target. For this reason, management applications
might want to start out in background mode and only switch to active
mode later, when certain conditions are met. This series adds a
block-job-change QMP command to achieve that, as well as
job-type-specific information when querying block jobs, which
can be used to decide when the switch should happen.

For now, only the direction background -> active is supported.

The information added upon querying is whether the target is actively
synced, the total data sent, and the remaining dirty bytes.

Initially, I tried to go for a more general 'job-change' command, but
I couldn't figure out a way to avoid mutual inclusion between
block-core.json and job.json.


Fiona Ebner (10):
  blockjob: introduce block-job-change QMP command
  block/mirror: set actively_synced even after the job is ready
  block/mirror: move dirty bitmap to filter
  block/mirror: determine copy_to_target only once
  mirror: implement mirror_change method
  qapi/block-core: use JobType for BlockJobInfo's type
  qapi/block-core: turn BlockJobInfo into a union
  blockjob: query driver-specific info via a new 'query' driver method
  mirror: return mirror-specific information upon query
  iotests: adapt test output for new mirror query property

 block/mirror.c                 | 95 +++++++++++++++++++++++-----------
 block/monitor/block-hmp-cmds.c |  4 +-
 blockdev.c                     | 14 +++++
 blockjob.c                     | 26 +++++++++-
 include/block/blockjob.h       | 11 ++++
 include/block/blockjob_int.h   | 10 ++++
 job.c                          |  1 +
 qapi/block-core.json           | 59 +++++++++++++++++++--
 qapi/job.json                  |  4 +-
 tests/qemu-iotests/109.out     | 24 ++++-----
 10 files changed, 199 insertions(+), 49 deletions(-)

-- 
2.39.2

[PATCH v2 01/10] blockjob: introduce block-job-change QMP command

[Fiona Ebner]

which will allow changing job-type-specific options after job
creation.

In the JobVerbTable, the same allow bits as for set-speed are used,
because set-speed can be considered an existing change command.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
    * update QEMU version in QAPI
    * fix typo in function comment

 blockdev.c                   | 14 ++++++++++++++
 blockjob.c                   | 20 ++++++++++++++++++++
 include/block/blockjob.h     | 11 +++++++++++
 include/block/blockjob_int.h |  5 +++++
 job.c                        |  1 +
 qapi/block-core.json         | 26 ++++++++++++++++++++++++++
 qapi/job.json                |  4 +++-
 7 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 325b7a3bef..d0e274ff8b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3344,6 +3344,20 @@ void qmp_block_job_dismiss(const char *id, Error **errp)
     job_dismiss_locked(&job, errp);
 }
 
+void qmp_block_job_change(BlockJobChangeOptions *opts, Error **errp)
+{
+    BlockJob *job;
+
+    JOB_LOCK_GUARD();
+    job = find_block_job_locked(opts->id, errp);
+
+    if (!job) {
+        return;
+    }
+
+    block_job_change_locked(job, opts, errp);
+}
+
 void qmp_change_backing_file(const char *device,
                              const char *image_node_name,
                              const char *backing_file,
diff --git a/blockjob.c b/blockjob.c
index 58c5d64539..d53bc775d2 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -328,6 +328,26 @@ static bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
     return block_job_set_speed_locked(job, speed, errp);
 }
 
+void block_job_change_locked(BlockJob *job, BlockJobChangeOptions *opts,
+                             Error **errp)
+{
+    const BlockJobDriver *drv = block_job_driver(job);
+
+    GLOBAL_STATE_CODE();
+
+    if (job_apply_verb_locked(&job->job, JOB_VERB_CHANGE, errp)) {
+        return;
+    }
+
+    if (drv->change) {
+        job_unlock();
+        drv->change(job, opts, errp);
+        job_lock();
+    } else {
+        error_setg(errp, "Job type does not support change");
+    }
+}
+
 void block_job_ratelimit_processed_bytes(BlockJob *job, uint64_t n)
 {
     IO_CODE();
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 058b0c824c..95854f1477 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -172,6 +172,17 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs);
  */
 bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp);
 
+/**
+ * block_job_change_locked:
+ * @job: The job to change.
+ * @opts: The new options.
+ * @errp: Error object.
+ *
+ * Change the job according to opts.
+ */
+void block_job_change_locked(BlockJob *job, BlockJobChangeOptions *opts,
+                             Error **errp);
+
 /**
  * block_job_query_locked:
  * @job: The job to get information about.
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 104824040c..f604985315 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -67,6 +67,11 @@ struct BlockJobDriver {
     void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
 
     void (*set_speed)(BlockJob *job, int64_t speed);
+
+    /*
+     * Change the @job's options according to @opts.
+     */
+    void (*change)(BlockJob *job, BlockJobChangeOptions *opts, Error **errp);
 };
 
 /*
diff --git a/job.c b/job.c
index 72d57f0934..99a2e54b54 100644
--- a/job.c
+++ b/job.c
@@ -80,6 +80,7 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = {
     [JOB_VERB_COMPLETE]             = {0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0},
     [JOB_VERB_FINALIZE]             = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0},
     [JOB_VERB_DISMISS]              = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0},
+    [JOB_VERB_CHANGE]               = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0},
 };
 
 /* Transactional group of jobs */
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 89751d81f2..c6f31a9399 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3044,6 +3044,32 @@
 { 'command': 'block-job-finalize', 'data': { 'id': 'str' },
   'allow-preconfig': true }
 
+##
+# @BlockJobChangeOptions:
+#
+# Block job options that can be changed after job creation.
+#
+# @id: The job identifier
+#
+# @type: The job type
+#
+# Since 8.2
+##
+{ 'union': 'BlockJobChangeOptions',
+  'base': { 'id': 'str', 'type': 'JobType' },
+  'discriminator': 'type',
+  'data': {} }
+
+##
+# @block-job-change:
+#
+# Change the block job's options.
+#
+# Since: 8.2
+##
+{ 'command': 'block-job-change',
+  'data': 'BlockJobChangeOptions', 'boxed': true }
+
 ##
 # @BlockdevDiscardOptions:
 #
diff --git a/qapi/job.json b/qapi/job.json
index 7f0ba090de..b3957207a4 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -105,11 +105,13 @@
 #
 # @finalize: see @job-finalize
 #
+# @change: see @block-job-change (since 8.2)
+#
 # Since: 2.12
 ##
 { 'enum': 'JobVerb',
   'data': ['cancel', 'pause', 'resume', 'set-speed', 'complete', 'dismiss',
-           'finalize' ] }
+           'finalize', 'change' ] }
 
 ##
 # @JOB_STATUS_CHANGE:
-- 
2.39.2

[PATCH v2 02/10] block/mirror: set actively_synced even after the job is ready

[Fiona Ebner]

In preparation to allow switching from background to active mode. This
ensures that setting actively_synced will not be missed when the
switch happens after the job is ready.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

No changes in v2.

 block/mirror.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 3cc0757a03..b764ad5108 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1074,9 +1074,9 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
                  * the target in a consistent state.
                  */
                 job_transition_to_ready(&s->common.job);
-                if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
-                    s->actively_synced = true;
-                }
+            }
+            if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
+                s->actively_synced = true;
             }
 
             should_complete = s->should_complete ||
-- 
2.39.2

[PATCH v2 03/10] block/mirror: move dirty bitmap to filter

[Fiona Ebner]

In preparation to allow switching to active mode without draining.
Initialization of the bitmap in mirror_dirty_init() still happens with
the original/backing BlockDriverState, which should be fine, because
the mirror top has the same length.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

 block/mirror.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index b764ad5108..0ed54754e2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1500,6 +1500,10 @@ bdrv_mirror_top_do_write(BlockDriverState *bs, MirrorMethod method,
         abort();
     }
 
+    if (!copy_to_target && s->job && s->job->dirty_bitmap) {
+        bdrv_set_dirty_bitmap(s->job->dirty_bitmap, offset, bytes);
+    }
+
     if (ret < 0) {
         goto out;
     }
@@ -1823,13 +1827,17 @@ static BlockJob *mirror_start_job(
         s->should_complete = true;
     }
 
-    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+    s->dirty_bitmap = bdrv_create_dirty_bitmap(s->mirror_top_bs, granularity,
+                                               NULL, errp);
     if (!s->dirty_bitmap) {
         goto fail;
     }
-    if (s->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) {
-        bdrv_disable_dirty_bitmap(s->dirty_bitmap);
-    }
+
+    /*
+     * The dirty bitmap is set by bdrv_mirror_top_do_write() when not in active
+     * mode.
+     */
+    bdrv_disable_dirty_bitmap(s->dirty_bitmap);
 
     ret = block_job_add_bdrv(&s->common, "source", bs, 0,
                              BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE |
-- 
2.39.2

[PATCH v2 04/10] block/mirror: determine copy_to_target only once

[Fiona Ebner]

In preparation to allow changing the copy_mode via QMP. When running
in an iothread, it could be that copy_mode is changed from the main
thread in between reading copy_mode in bdrv_mirror_top_pwritev() and
reading copy_mode in bdrv_mirror_top_do_write(), so they might end up
disagreeing about whether copy_to_target is true or false. Avoid that
scenario by determining copy_to_target only once and passing it to
bdrv_mirror_top_do_write() as an argument.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

 block/mirror.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 0ed54754e2..8992c09172 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1463,21 +1463,21 @@ bdrv_mirror_top_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
     return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
 }
 
+static bool should_copy_to_target(MirrorBDSOpaque *s)
+{
+    return s->job && s->job->ret >= 0 &&
+        !job_is_cancelled(&s->job->common.job) &&
+        s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
+}
+
 static int coroutine_fn GRAPH_RDLOCK
 bdrv_mirror_top_do_write(BlockDriverState *bs, MirrorMethod method,
-                         uint64_t offset, uint64_t bytes, QEMUIOVector *qiov,
-                         int flags)
+                         bool copy_to_target, uint64_t offset, uint64_t bytes,
+                         QEMUIOVector *qiov, int flags)
 {
     MirrorOp *op = NULL;
     MirrorBDSOpaque *s = bs->opaque;
     int ret = 0;
-    bool copy_to_target = false;
-
-    if (s->job) {
-        copy_to_target = s->job->ret >= 0 &&
-                         !job_is_cancelled(&s->job->common.job) &&
-                         s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
-    }
 
     if (copy_to_target) {
         op = active_write_prepare(s->job, offset, bytes);
@@ -1523,17 +1523,10 @@ static int coroutine_fn GRAPH_RDLOCK
 bdrv_mirror_top_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
                         QEMUIOVector *qiov, BdrvRequestFlags flags)
 {
-    MirrorBDSOpaque *s = bs->opaque;
     QEMUIOVector bounce_qiov;
     void *bounce_buf;
     int ret = 0;
-    bool copy_to_target = false;
-
-    if (s->job) {
-        copy_to_target = s->job->ret >= 0 &&
-                         !job_is_cancelled(&s->job->common.job) &&
-                         s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
-    }
+    bool copy_to_target = should_copy_to_target(bs->opaque);
 
     if (copy_to_target) {
         /* The guest might concurrently modify the data to write; but
@@ -1550,8 +1543,8 @@ bdrv_mirror_top_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
         flags &= ~BDRV_REQ_REGISTERED_BUF;
     }
 
-    ret = bdrv_mirror_top_do_write(bs, MIRROR_METHOD_COPY, offset, bytes, qiov,
-                                   flags);
+    ret = bdrv_mirror_top_do_write(bs, MIRROR_METHOD_COPY, copy_to_target,
+                                   offset, bytes, qiov, flags);
 
     if (copy_to_target) {
         qemu_iovec_destroy(&bounce_qiov);
@@ -1574,15 +1567,17 @@ static int coroutine_fn GRAPH_RDLOCK
 bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
                               int64_t bytes, BdrvRequestFlags flags)
 {
-    return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_ZERO, offset, bytes, NULL,
-                                    flags);
+    bool copy_to_target = should_copy_to_target(bs->opaque);
+    return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_ZERO, copy_to_target,
+                                    offset, bytes, NULL, flags);
 }
 
 static int coroutine_fn GRAPH_RDLOCK
 bdrv_mirror_top_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
 {
-    return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_DISCARD, offset, bytes,
-                                    NULL, 0);
+    bool copy_to_target = should_copy_to_target(bs->opaque);
+    return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_DISCARD, copy_to_target,
+                                    offset, bytes, NULL, 0);
 }
 
 static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
-- 
2.39.2

[PATCH v2 05/10] mirror: implement mirror_change method

[Fiona Ebner]

which allows switching the @copy-mode from 'background' to
'write-blocking'.

This is useful for management applications, so they can start out in
background mode to avoid limiting guest write speed and switch to
active mode when certain criteria are fulfilled.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
    * update QEMU version in QAPI
    * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
      doc comments to conform to current conventions"))
    * drop drained section and disable dirty bitmap call. It's already
      disabled, because the bitmap is now attached to the filter and
      set in bdrv_mirror_top_do_write(). See the earlier patch
      "block/mirror: move dirty bitmap to filter"

 block/mirror.c       | 22 ++++++++++++++++++++++
 qapi/block-core.json | 13 ++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index b84de56734..83aa4176c2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1246,6 +1246,27 @@ static bool commit_active_cancel(Job *job, bool force)
     return force || !job_is_ready(job);
 }
 
+static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
+                          Error **errp)
+{
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+    BlockJobChangeOptionsMirror *change_opts = &opts->u.mirror;
+
+    if (s->copy_mode == change_opts->copy_mode) {
+        return;
+    }
+
+    if (s->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) {
+        error_setg(errp, "Cannot switch away from copy mode 'write-blocking'");
+        return;
+    }
+
+    assert(s->copy_mode == MIRROR_COPY_MODE_BACKGROUND &&
+           change_opts->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING);
+
+    s->copy_mode = MIRROR_COPY_MODE_WRITE_BLOCKING;
+}
+
 static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1260,6 +1281,7 @@ static const BlockJobDriver mirror_job_driver = {
         .cancel                 = mirror_cancel,
     },
     .drained_poll           = mirror_drained_poll,
+    .change                 = mirror_change,
 };
 
 static const BlockJobDriver commit_active_job_driver = {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c6f31a9399..01427c259a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3044,6 +3044,17 @@
 { 'command': 'block-job-finalize', 'data': { 'id': 'str' },
   'allow-preconfig': true }
 
+##
+# @BlockJobChangeOptionsMirror:
+#
+# @copy-mode: Switch to this copy mode. Currenlty, only the switch
+#     from 'background' to 'write-blocking' is implemented.
+#
+# Since: 8.2
+##
+{ 'struct': 'BlockJobChangeOptionsMirror',
+  'data': { 'copy-mode' : 'MirrorCopyMode' } }
+
 ##
 # @BlockJobChangeOptions:
 #
@@ -3058,7 +3069,7 @@
 { 'union': 'BlockJobChangeOptions',
   'base': { 'id': 'str', 'type': 'JobType' },
   'discriminator': 'type',
-  'data': {} }
+  'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }
 
 ##
 # @block-job-change:
-- 
2.39.2

[PATCH v2 06/10] qapi/block-core: use JobType for BlockJobInfo's type

[Fiona Ebner]

In preparation to turn BlockJobInfo into a union with @type as the
discriminator. That requires it to be an enum.

No functional change is intended.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

No changes in v2.

 block/monitor/block-hmp-cmds.c | 4 ++--
 blockjob.c                     | 2 +-
 qapi/block-core.json           | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index ca2599de44..f9f87e5c47 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -843,7 +843,7 @@ void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
     }
 
     while (list) {
-        if (strcmp(list->value->type, "stream") == 0) {
+        if (list->value->type == JOB_TYPE_STREAM) {
             monitor_printf(mon, "Streaming device %s: Completed %" PRId64
                            " of %" PRId64 " bytes, speed limit %" PRId64
                            " bytes/s\n",
@@ -855,7 +855,7 @@ void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
             monitor_printf(mon, "Type %s, device %s: Completed %" PRId64
                            " of %" PRId64 " bytes, speed limit %" PRId64
                            " bytes/s\n",
-                           list->value->type,
+                           JobType_str(list->value->type),
                            list->value->device,
                            list->value->offset,
                            list->value->len,
diff --git a/blockjob.c b/blockjob.c
index d53bc775d2..f8cf6e58e2 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -388,7 +388,7 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp)
                           &progress_total);
 
     info = g_new0(BlockJobInfo, 1);
-    info->type      = g_strdup(job_type_str(&job->job));
+    info->type      = job_type(&job->job);
     info->device    = g_strdup(job->job.id);
     info->busy      = job->job.busy;
     info->paused    = job->job.pause_count > 0;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 01427c259a..a19718a69f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1396,7 +1396,7 @@
 # Since: 1.1
 ##
 { 'struct': 'BlockJobInfo',
-  'data': {'type': 'str', 'device': 'str', 'len': 'int',
+  'data': {'type': 'JobType', 'device': 'str', 'len': 'int',
            'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int',
            'io-status': 'BlockDeviceIoStatus', 'ready': 'bool',
            'status': 'JobStatus',
-- 
2.39.2

[PATCH v2 07/10] qapi/block-core: turn BlockJobInfo into a union

[Fiona Ebner]

In preparation to additionally return job-type-specific information.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

No changes in v2.

 qapi/block-core.json | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index a19718a69f..950542b735 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1395,13 +1395,15 @@
 #
 # Since: 1.1
 ##
-{ 'struct': 'BlockJobInfo',
-  'data': {'type': 'JobType', 'device': 'str', 'len': 'int',
+{ 'union': 'BlockJobInfo',
+  'base': {'type': 'JobType', 'device': 'str', 'len': 'int',
            'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int',
            'io-status': 'BlockDeviceIoStatus', 'ready': 'bool',
            'status': 'JobStatus',
            'auto-finalize': 'bool', 'auto-dismiss': 'bool',
-           '*error': 'str' } }
+           '*error': 'str' },
+  'discriminator': 'type',
+  'data': {} }
 
 ##
 # @query-block-jobs:
-- 
2.39.2

[PATCH v2 08/10] blockjob: query driver-specific info via a new 'query' driver method

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

No changes in v2.

 blockjob.c                   | 4 ++++
 include/block/blockjob_int.h | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index f8cf6e58e2..7e8cfad0fd 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -376,6 +376,7 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp)
 {
     BlockJobInfo *info;
     uint64_t progress_current, progress_total;
+    const BlockJobDriver *drv = block_job_driver(job);
 
     GLOBAL_STATE_CODE();
 
@@ -405,6 +406,9 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp)
                         g_strdup(error_get_pretty(job->job.err)) :
                         g_strdup(strerror(-job->job.ret));
     }
+    if (drv->query) {
+        drv->query(job, info);
+    }
     return info;
 }
 
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f604985315..4ab88b3c97 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -72,6 +72,11 @@ struct BlockJobDriver {
      * Change the @job's options according to @opts.
      */
     void (*change)(BlockJob *job, BlockJobChangeOptions *opts, Error **errp);
+
+    /*
+     * Query information specific to this kind of block job.
+     */
+    void (*query)(BlockJob *job, BlockJobInfo *info);
 };
 
 /*
-- 
2.39.2

[PATCH v2 09/10] mirror: return mirror-specific information upon query

[Fiona Ebner]

To start out, only actively-synced is returned.

For example, this is useful for jobs that started out in background
mode and switched to active mode. Once actively-synced is true, it's
clear that the mode switch has been completed. Note that completion of
the switch might happen much earlier, e.g. if the switch happens
before the job is ready, once all background operations have finished.
It's assumed that whether the disks are actively-synced or not is more
interesting than whether the mode switch completed. That information
can still be added if required in the future.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
    * udpate QEMU version in QAPI
    * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
      doc comments to conform to current conventions"))

 block/mirror.c       | 10 ++++++++++
 qapi/block-core.json | 16 +++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 83aa4176c2..33b72ec5e5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1267,6 +1267,15 @@ static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
     s->copy_mode = MIRROR_COPY_MODE_WRITE_BLOCKING;
 }
 
+static void mirror_query(BlockJob *job, BlockJobInfo *info)
+{
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+
+    info->u.mirror = (BlockJobInfoMirror) {
+        .actively_synced = s->actively_synced,
+    };
+}
+
 static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1282,6 +1291,7 @@ static const BlockJobDriver mirror_job_driver = {
     },
     .drained_poll           = mirror_drained_poll,
     .change                 = mirror_change,
+    .query                  = mirror_query,
 };
 
 static const BlockJobDriver commit_active_job_driver = {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 950542b735..35d67410cc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1352,6 +1352,20 @@
 { 'enum': 'MirrorCopyMode',
   'data': ['background', 'write-blocking'] }
 
+##
+# @BlockJobInfoMirror:
+#
+# Information specific to mirror block jobs.
+#
+# @actively-synced: Whether the source is actively synced to the
+#     target, i.e. same data and new writes are done synchronously to
+#     both.
+#
+# Since 8.2
+##
+{ 'struct': 'BlockJobInfoMirror',
+  'data': { 'actively-synced': 'bool' } }
+
 ##
 # @BlockJobInfo:
 #
@@ -1403,7 +1417,7 @@
            'auto-finalize': 'bool', 'auto-dismiss': 'bool',
            '*error': 'str' },
   'discriminator': 'type',
-  'data': {} }
+  'data': { 'mirror': 'BlockJobInfoMirror' } }
 
 ##
 # @query-block-jobs:
-- 
2.39.2

[PATCH v2 10/10] iotests: adapt test output for new mirror query property

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

 tests/qemu-iotests/109.out | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out
index 2611d6a40f..965c9a6a0a 100644
--- a/tests/qemu-iotests/109.out
+++ b/tests/qemu-iotests/109.out
@@ -38,7 +38,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -90,7 +90,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 197120, "offset": 197120, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 197120, "offset": 197120, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -142,7 +142,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -194,7 +194,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 1024, "offset": 1024, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -246,7 +246,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 65536, "offset": 65536, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 65536, "offset": 65536, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -298,7 +298,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -349,7 +349,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 2560, "offset": 2560, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -400,7 +400,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 31457280, "offset": 31457280, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 31457280, "offset": 31457280, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 31457280, "offset": 31457280, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -451,7 +451,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 327680, "offset": 327680, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -502,7 +502,7 @@ read 512/512 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 2048, "offset": 2048, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 2048, "offset": 2048, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -533,7 +533,7 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
@@ -557,7 +557,7 @@ Images are identical.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}}
 {"execute":"query-block-jobs"}
-{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": [{"auto-finalize": true, "io-status": "ok", "device": "src", "auto-dismiss": true, "busy": false, "len": 512, "offset": 512, "status": "ready", "paused": false, "speed": 0, "ready": true, "type": "mirror", "actively-synced": false}]}
 {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
-- 
2.39.2

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 09.10.23 12:46, Fiona Ebner wrote:
> Changes in v2:
>      * move bitmap to filter which allows to avoid draining when
>        changing the copy mode
>      * add patch to determine copy_to_target only once
>      * drop patches returning redundant information upon query
>      * update QEMU version in QAPI
>      * update indentation in QAPI
>      * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
>        doc comments to conform to current conventions"))
>      * add patch to adapt iotest output
> 
> Discussion of v1:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg07216.html
> 
> With active mode, the guest write speed is limited by the synchronous
> writes to the mirror target. For this reason, management applications
> might want to start out in background mode and only switch to active
> mode later, when certain conditions are met. This series adds a
> block-job-change QMP command to achieve that, as well as
> job-type-specific information when querying block jobs, which
> can be used to decide when the switch should happen.
> 
> For now, only the direction background -> active is supported.
> 
> The information added upon querying is whether the target is actively
> synced, the total data sent, and the remaining dirty bytes.
> 
> Initially, I tried to go for a more general 'job-change' command, but
> I couldn't figure out a way to avoid mutual inclusion between
> block-core.json and job.json.
> 

What is the problem with it? I still think that job-change would be better.

> 
> Fiona Ebner (10):
>    blockjob: introduce block-job-change QMP command
>    block/mirror: set actively_synced even after the job is ready
>    block/mirror: move dirty bitmap to filter
>    block/mirror: determine copy_to_target only once
>    mirror: implement mirror_change method
>    qapi/block-core: use JobType for BlockJobInfo's type
>    qapi/block-core: turn BlockJobInfo into a union
>    blockjob: query driver-specific info via a new 'query' driver method
>    mirror: return mirror-specific information upon query
>    iotests: adapt test output for new mirror query property
> 
>   block/mirror.c                 | 95 +++++++++++++++++++++++-----------
>   block/monitor/block-hmp-cmds.c |  4 +-
>   blockdev.c                     | 14 +++++
>   blockjob.c                     | 26 +++++++++-
>   include/block/blockjob.h       | 11 ++++
>   include/block/blockjob_int.h   | 10 ++++
>   job.c                          |  1 +
>   qapi/block-core.json           | 59 +++++++++++++++++++--
>   qapi/job.json                  |  4 +-
>   tests/qemu-iotests/109.out     | 24 ++++-----
>   10 files changed, 199 insertions(+), 49 deletions(-)
> 

-- 
Best regards,
Vladimir

Re: [PATCH v2 01/10] blockjob: introduce block-job-change QMP command

[Vladimir Sementsov-Ogievskiy]

On 09.10.23 12:46, Fiona Ebner wrote:
> which will allow changing job-type-specific options after job
> creation.
> 
> In the JobVerbTable, the same allow bits as for set-speed are used,
> because set-speed can be considered an existing change command.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH v2 03/10] block/mirror: move dirty bitmap to filter

[Vladimir Sementsov-Ogievskiy]

On 09.10.23 12:46, Fiona Ebner wrote:
> In preparation to allow switching to active mode without draining.
> Initialization of the bitmap in mirror_dirty_init() still happens with
> the original/backing BlockDriverState, which should be fine, because
> the mirror top has the same length.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>


-- 
Best regards,
Vladimir

Re: [PATCH v2 04/10] block/mirror: determine copy_to_target only once

[Vladimir Sementsov-Ogievskiy]

On 09.10.23 12:46, Fiona Ebner wrote:
> In preparation to allow changing the copy_mode via QMP. When running
> in an iothread, it could be that copy_mode is changed from the main
> thread in between reading copy_mode in bdrv_mirror_top_pwritev() and
> reading copy_mode in bdrv_mirror_top_do_write(), so they might end up
> disagreeing about whether copy_to_target is true or false. Avoid that
> scenario by determining copy_to_target only once and passing it to
> bdrv_mirror_top_do_write() as an argument.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH v2 05/10] mirror: implement mirror_change method

[Vladimir Sementsov-Ogievskiy]

On 09.10.23 12:46, Fiona Ebner wrote:
> which allows switching the @copy-mode from 'background' to
> 'write-blocking'.
> 
> This is useful for management applications, so they can start out in
> background mode to avoid limiting guest write speed and switch to
> active mode when certain criteria are fulfilled.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v2:
>      * update QEMU version in QAPI
>      * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
>        doc comments to conform to current conventions"))
>      * drop drained section and disable dirty bitmap call. It's already
>        disabled, because the bitmap is now attached to the filter and
>        set in bdrv_mirror_top_do_write(). See the earlier patch
>        "block/mirror: move dirty bitmap to filter"
> 
>   block/mirror.c       | 22 ++++++++++++++++++++++
>   qapi/block-core.json | 13 ++++++++++++-
>   2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index b84de56734..83aa4176c2 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1246,6 +1246,27 @@ static bool commit_active_cancel(Job *job, bool force)
>       return force || !job_is_ready(job);
>   }
>   
> +static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
> +                          Error **errp)
> +{
> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
> +    BlockJobChangeOptionsMirror *change_opts = &opts->u.mirror;
> +
> +    if (s->copy_mode == change_opts->copy_mode) {
> +        return;
> +    }
> +
> +    if (s->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) {
> +        error_setg(errp, "Cannot switch away from copy mode 'write-blocking'");
> +        return;
> +    }
> +
> +    assert(s->copy_mode == MIRROR_COPY_MODE_BACKGROUND &&
> +           change_opts->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING);
> +
> +    s->copy_mode = MIRROR_COPY_MODE_WRITE_BLOCKING;
> +}

So, s->copy_mode becomes shared between main thread and iothread.

We should either use mutex or atomic operations.

Note, that the only realization of .set_speed uses thread-safe API.

> +
>   static const BlockJobDriver mirror_job_driver = {
>       .job_driver = {
>           .instance_size          = sizeof(MirrorBlockJob),
> @@ -1260,6 +1281,7 @@ static const BlockJobDriver mirror_job_driver = {
>           .cancel                 = mirror_cancel,
>       },
>       .drained_poll           = mirror_drained_poll,
> +    .change                 = mirror_change,
>   };
>   
>   static const BlockJobDriver commit_active_job_driver = {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c6f31a9399..01427c259a 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3044,6 +3044,17 @@
>   { 'command': 'block-job-finalize', 'data': { 'id': 'str' },
>     'allow-preconfig': true }
>   
> +##
> +# @BlockJobChangeOptionsMirror:
> +#
> +# @copy-mode: Switch to this copy mode. Currenlty, only the switch
> +#     from 'background' to 'write-blocking' is implemented.
> +#
> +# Since: 8.2
> +##
> +{ 'struct': 'BlockJobChangeOptionsMirror',
> +  'data': { 'copy-mode' : 'MirrorCopyMode' } }
> +
>   ##
>   # @BlockJobChangeOptions:
>   #
> @@ -3058,7 +3069,7 @@
>   { 'union': 'BlockJobChangeOptions',
>     'base': { 'id': 'str', 'type': 'JobType' },
>     'discriminator': 'type',
> -  'data': {} }
> +  'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }
>   
>   ##
>   # @block-job-change:

-- 
Best regards,
Vladimir

Re: [PATCH v2 08/10] blockjob: query driver-specific info via a new 'query' driver method

[Vladimir Sementsov-Ogievskiy]

On 09.10.23 12:46, Fiona Ebner wrote:
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> No changes in v2.
> 
>   blockjob.c                   | 4 ++++
>   include/block/blockjob_int.h | 5 +++++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/blockjob.c b/blockjob.c
> index f8cf6e58e2..7e8cfad0fd 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -376,6 +376,7 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp)
>   {
>       BlockJobInfo *info;
>       uint64_t progress_current, progress_total;
> +    const BlockJobDriver *drv = block_job_driver(job);
>   
>       GLOBAL_STATE_CODE();
>   
> @@ -405,6 +406,9 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp)
>                           g_strdup(error_get_pretty(job->job.err)) :
>                           g_strdup(strerror(-job->job.ret));
>       }
> +    if (drv->query) {
> +        drv->query(job, info);

Other handlers are called with job lock dropped.

> +    }
>       return info;
>   }
>   
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index f604985315..4ab88b3c97 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -72,6 +72,11 @@ struct BlockJobDriver {
>        * Change the @job's options according to @opts.
>        */
>       void (*change)(BlockJob *job, BlockJobChangeOptions *opts, Error **errp);
> +
> +    /*
> +     * Query information specific to this kind of block job.
> +     */
> +    void (*query)(BlockJob *job, BlockJobInfo *info);
>   };
>   
>   /*

-- 
Best regards,
Vladimir

Re: [PATCH v2 09/10] mirror: return mirror-specific information upon query

[Vladimir Sementsov-Ogievskiy]

On 09.10.23 12:46, Fiona Ebner wrote:
> To start out, only actively-synced is returned.
> 
> For example, this is useful for jobs that started out in background
> mode and switched to active mode. Once actively-synced is true, it's
> clear that the mode switch has been completed. Note that completion of
> the switch might happen much earlier, e.g. if the switch happens
> before the job is ready, once all background operations have finished.
> It's assumed that whether the disks are actively-synced or not is more
> interesting than whether the mode switch completed. That information
> can still be added if required in the future.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v2:
>      * udpate QEMU version in QAPI
>      * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
>        doc comments to conform to current conventions"))
> 
>   block/mirror.c       | 10 ++++++++++
>   qapi/block-core.json | 16 +++++++++++++++-
>   2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 83aa4176c2..33b72ec5e5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1267,6 +1267,15 @@ static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
>       s->copy_mode = MIRROR_COPY_MODE_WRITE_BLOCKING;
>   }
>   
> +static void mirror_query(BlockJob *job, BlockJobInfo *info)
> +{
> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
> +
> +    info->u.mirror = (BlockJobInfoMirror) {
> +        .actively_synced = s->actively_synced,

So, seems we should use atomic operations to access this field too

> +    };
> +}
> +
>   static const BlockJobDriver mirror_job_driver = {
>       .job_driver = {
>           .instance_size          = sizeof(MirrorBlockJob),
> @@ -1282,6 +1291,7 @@ static const BlockJobDriver mirror_job_driver = {
>       },
>       .drained_poll           = mirror_drained_poll,
>       .change                 = mirror_change,
> +    .query                  = mirror_query,
>   };
>   
>   static const BlockJobDriver commit_active_job_driver = {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 950542b735..35d67410cc 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1352,6 +1352,20 @@
>   { 'enum': 'MirrorCopyMode',
>     'data': ['background', 'write-blocking'] }
>   
> +##
> +# @BlockJobInfoMirror:
> +#
> +# Information specific to mirror block jobs.
> +#
> +# @actively-synced: Whether the source is actively synced to the
> +#     target, i.e. same data and new writes are done synchronously to
> +#     both.
> +#
> +# Since 8.2
> +##
> +{ 'struct': 'BlockJobInfoMirror',
> +  'data': { 'actively-synced': 'bool' } }
> +
>   ##
>   # @BlockJobInfo:
>   #
> @@ -1403,7 +1417,7 @@
>              'auto-finalize': 'bool', 'auto-dismiss': 'bool',
>              '*error': 'str' },
>     'discriminator': 'type',
> -  'data': {} }
> +  'data': { 'mirror': 'BlockJobInfoMirror' } }
>   
>   ##
>   # @query-block-jobs:

-- 
Best regards,
Vladimir

Re: [PATCH v2 10/10] iotests: adapt test output for new mirror query property

[Vladimir Sementsov-Ogievskiy]

On 09.10.23 12:46, Fiona Ebner wrote:
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

This should be merged to the previous patch, to not break git bisect. Tests should work at any commit.

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 10.10.23 20:55, Vladimir Sementsov-Ogievskiy wrote:
> On 09.10.23 12:46, Fiona Ebner wrote:
>> Changes in v2:
>>      * move bitmap to filter which allows to avoid draining when
>>        changing the copy mode
>>      * add patch to determine copy_to_target only once
>>      * drop patches returning redundant information upon query
>>      * update QEMU version in QAPI
>>      * update indentation in QAPI
>>      * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
>>        doc comments to conform to current conventions"))
>>      * add patch to adapt iotest output
>>
>> Discussion of v1:
>> https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg07216.html
>>
>> With active mode, the guest write speed is limited by the synchronous
>> writes to the mirror target. For this reason, management applications
>> might want to start out in background mode and only switch to active
>> mode later, when certain conditions are met. This series adds a
>> block-job-change QMP command to achieve that, as well as
>> job-type-specific information when querying block jobs, which
>> can be used to decide when the switch should happen.
>>
>> For now, only the direction background -> active is supported.
>>
>> The information added upon querying is whether the target is actively
>> synced, the total data sent, and the remaining dirty bytes.
>>
>> Initially, I tried to go for a more general 'job-change' command, but
>> I couldn't figure out a way to avoid mutual inclusion between
>> block-core.json and job.json.
>>
> 
> What is the problem with it? I still think that job-change would be better.

However, that's not a show-stopper. We have block-job-* commands, they are not deprecated, no problem to add one more.

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Fiona Ebner]

Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
> On 09.10.23 12:46, Fiona Ebner wrote:
>>
>> Initially, I tried to go for a more general 'job-change' command, but
>> I couldn't figure out a way to avoid mutual inclusion between
>> block-core.json and job.json.
>>
> 
> What is the problem with it? I still think that job-change would be better.
> 

If going for job-change in job.json, the dependencies would be

job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode

query-jobs -> JobInfo -> JobInfoMirror

and we can't include block-core.json in job.json, because an inclusion
loop gives a build error.

Could be made to work by moving MirrorCopyMode (and
JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
can be included by both job.json and block-core.json. Moving the
type-specific definitions to the general job.json didn't feel right to
me. Including another file with type-specific definitions in job.json
feels slightly less wrong, but still not quite right and I didn't want
to create a new file just for MirrorCopyMode (and
JobChangeOptionsMirror, JobInfoMirror).

And going further and moving all mirror-related things to a separate
file would require moving along things like NewImageMode with it or
create yet another file for such general things used by multiple block-jobs.

If preferred, I can try and go with some version of the above.

Best Regards,
Fiona

Re: [PATCH v2 05/10] mirror: implement mirror_change method

[Fiona Ebner]

Am 10.10.23 um 21:37 schrieb Vladimir Sementsov-Ogievskiy:
> On 09.10.23 12:46, Fiona Ebner wrote:
>>   +static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
>> +                          Error **errp)
>> +{
>> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
>> +    BlockJobChangeOptionsMirror *change_opts = &opts->u.mirror;
>> +
>> +    if (s->copy_mode == change_opts->copy_mode) {
>> +        return;
>> +    }
>> +
>> +    if (s->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) {
>> +        error_setg(errp, "Cannot switch away from copy mode
>> 'write-blocking'");
>> +        return;
>> +    }
>> +
>> +    assert(s->copy_mode == MIRROR_COPY_MODE_BACKGROUND &&
>> +           change_opts->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING);
>> +
>> +    s->copy_mode = MIRROR_COPY_MODE_WRITE_BLOCKING;
>> +}
> 
> So, s->copy_mode becomes shared between main thread and iothread.
> 
> We should either use mutex or atomic operations.
> 
> Note, that the only realization of .set_speed uses thread-safe API.
> 

Can it be an issue if it's only ever set from the main thread?

But sure, I'm implicitly relying on that, which is not ideal. The
mirror_change() function does multiple checks based on the current
value, and only then changes it, so I suppose it would actually need a
mutex rather than just changing to atomic accesses? Otherwise, the
current value can't be guaranteed to be the same in the different checks
if we ever add something that can change the value from another thread.

I suppose, I should re-use the job mutex then?

Best Regards,
Fiona

Re: [PATCH v2 05/10] mirror: implement mirror_change method

[Vladimir Sementsov-Ogievskiy]

On 11.10.23 14:22, Fiona Ebner wrote:
> Am 10.10.23 um 21:37 schrieb Vladimir Sementsov-Ogievskiy:
>> On 09.10.23 12:46, Fiona Ebner wrote:
>>>    +static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
>>> +                          Error **errp)
>>> +{
>>> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
>>> +    BlockJobChangeOptionsMirror *change_opts = &opts->u.mirror;
>>> +
>>> +    if (s->copy_mode == change_opts->copy_mode) {
>>> +        return;
>>> +    }
>>> +
>>> +    if (s->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) {
>>> +        error_setg(errp, "Cannot switch away from copy mode
>>> 'write-blocking'");
>>> +        return;
>>> +    }
>>> +
>>> +    assert(s->copy_mode == MIRROR_COPY_MODE_BACKGROUND &&
>>> +           change_opts->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING);
>>> +
>>> +    s->copy_mode = MIRROR_COPY_MODE_WRITE_BLOCKING;
>>> +}
>>
>> So, s->copy_mode becomes shared between main thread and iothread.
>>
>> We should either use mutex or atomic operations.
>>
>> Note, that the only realization of .set_speed uses thread-safe API.
>>
> 
> Can it be an issue if it's only ever set from the main thread?

Yes, I also think, that actually setting int variable is "atomic enough". But I'm not sure about all architectures/OSes/compilers)

> 
> But sure, I'm implicitly relying on that, which is not ideal. The
> mirror_change() function does multiple checks based on the current
> value, and only then changes it, so I suppose it would actually need a
> mutex rather than just changing to atomic accesses? Otherwise, the
> current value can't be guaranteed to be the same in the different checks
> if we ever add something that can change the value from another thread.


It could still be written like this

if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
   report error
}


if (qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND, MIRROR_COPY_MODE_WRITE_BLOCKING) != MIRROR_COPY_MODE_BACKGROUND) {
   report error
}

report success

===

and we'll have to access it as qatomic_read(&s->copy_mode) in other places

> 
> I suppose, I should re-use the job mutex then?
> 
> Best Regards,
> Fiona
> 

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 11.10.23 13:18, Fiona Ebner wrote:
> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
>> On 09.10.23 12:46, Fiona Ebner wrote:
>>>
>>> Initially, I tried to go for a more general 'job-change' command, but
>>> I couldn't figure out a way to avoid mutual inclusion between
>>> block-core.json and job.json.
>>>
>>
>> What is the problem with it? I still think that job-change would be better.
>>
> 
> If going for job-change in job.json, the dependencies would be
> 
> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
> 
> query-jobs -> JobInfo -> JobInfoMirror
> 
> and we can't include block-core.json in job.json, because an inclusion
> loop gives a build error.
> 
> Could be made to work by moving MirrorCopyMode (and
> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
> can be included by both job.json and block-core.json. Moving the
> type-specific definitions to the general job.json didn't feel right to
> me. Including another file with type-specific definitions in job.json
> feels slightly less wrong, but still not quite right and I didn't want
> to create a new file just for MirrorCopyMode (and
> JobChangeOptionsMirror, JobInfoMirror).
> 
> And going further and moving all mirror-related things to a separate
> file would require moving along things like NewImageMode with it or
> create yet another file for such general things used by multiple block-jobs.
> 
> If preferred, I can try and go with some version of the above.
> 

OK, I see the problem. Seems, that all requires some good refactoring. But that's a preexisting big work, and should not hold up your series. I'm OK to proceed with block-job-change.

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 11.10.23 13:18, Fiona Ebner wrote:
>> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 09.10.23 12:46, Fiona Ebner wrote:
>>>>
>>>> Initially, I tried to go for a more general 'job-change' command, but
>>>> I couldn't figure out a way to avoid mutual inclusion between
>>>> block-core.json and job.json.
>>>>
>>>
>>> What is the problem with it? I still think that job-change would be better.
>>>
>> If going for job-change in job.json, the dependencies would be
>> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
>> query-jobs -> JobInfo -> JobInfoMirror
>> and we can't include block-core.json in job.json, because an inclusion
>> loop gives a build error.

Let me try to understand this.

Command job-change needs its argument type JobChangeOptions.

JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
branches.

JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.

block-core.json needs job.json for JobType and JobStatus.

>> Could be made to work by moving MirrorCopyMode (and
>> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
>> can be included by both job.json and block-core.json. Moving the
>> type-specific definitions to the general job.json didn't feel right to
>> me. Including another file with type-specific definitions in job.json
>> feels slightly less wrong, but still not quite right and I didn't want
>> to create a new file just for MirrorCopyMode (and
>> JobChangeOptionsMirror, JobInfoMirror).
>> And going further and moving all mirror-related things to a separate
>> file would require moving along things like NewImageMode with it or
>> create yet another file for such general things used by multiple block-jobs.
>> If preferred, I can try and go with some version of the above.
>> 
>
> OK, I see the problem. Seems, that all requires some good refactoring. But that's a preexisting big work, and should not hold up your series. I'm OK to proceed with block-job-change.

Saving ourselves some internal refactoring is a poor excuse for
undesirable external interfaces.  We need to answer two questions before
we do that:

1. How much work would the refactoring be?

2. Is the interface improvement this enables worth the work?

Let's start with 1.

An obvious solution is to split JobType and JobStatus off job.json to
break the dependency of block-core.json on job.json.

But I'd like us to investigate another one.  block-core.json is *huge*.
It's almost a quarter of the entire QAPI schema.  Can we spin out block
jobs into block-job.json?  Moves the dependency on job.json from
block-core.json to block-job.json.

Thoughts?

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Kevin Wolf]

Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
> > On 11.10.23 13:18, Fiona Ebner wrote:
> >> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
> >>> On 09.10.23 12:46, Fiona Ebner wrote:
> >>>>
> >>>> Initially, I tried to go for a more general 'job-change' command, but
> >>>> I couldn't figure out a way to avoid mutual inclusion between
> >>>> block-core.json and job.json.
> >>>>
> >>>
> >>> What is the problem with it? I still think that job-change would be better.
> >>>
> >> If going for job-change in job.json, the dependencies would be
> >> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
> >> query-jobs -> JobInfo -> JobInfoMirror
> >> and we can't include block-core.json in job.json, because an inclusion
> >> loop gives a build error.
> 
> Let me try to understand this.
> 
> Command job-change needs its argument type JobChangeOptions.
> 
> JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
> branches.
> 
> JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.
> 
> block-core.json needs job.json for JobType and JobStatus.
> 
> >> Could be made to work by moving MirrorCopyMode (and
> >> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
> >> can be included by both job.json and block-core.json. Moving the
> >> type-specific definitions to the general job.json didn't feel right to
> >> me. Including another file with type-specific definitions in job.json
> >> feels slightly less wrong, but still not quite right and I didn't want
> >> to create a new file just for MirrorCopyMode (and
> >> JobChangeOptionsMirror, JobInfoMirror).
> >> And going further and moving all mirror-related things to a separate
> >> file would require moving along things like NewImageMode with it or
> >> create yet another file for such general things used by multiple block-jobs.
> >> If preferred, I can try and go with some version of the above.
> >> 
> >
> > OK, I see the problem. Seems, that all requires some good refactoring. But that's a preexisting big work, and should not hold up your series. I'm OK to proceed with block-job-change.
> 
> Saving ourselves some internal refactoring is a poor excuse for
> undesirable external interfaces.

I'm not sure how undesirable it is. We have block-job-* commands for
pretty much every other operation, so it's only consistent to have
block-job-change, too.

Having job-change, too, might be nice in theory, but we don't have even
a potential user for it at this point (i.e. a job type that isn't a
block job, but for which changing options at runtime makes sense).

> We need to answer two questions before we do that:
> 
> 1. How much work would the refactoring be?
> 
> 2. Is the interface improvement this enables worth the work?
> 
> Let's start with 1.
> 
> An obvious solution is to split JobType and JobStatus off job.json to
> break the dependency of block-core.json on job.json.
> 
> But I'd like us to investigate another one.  block-core.json is *huge*.
> It's almost a quarter of the entire QAPI schema.  Can we spin out block
> jobs into block-job.json?  Moves the dependency on job.json from
> block-core.json to block-job.json.

It also makes job.json depend on block-job.json instead of
block-core.json, so you only moved the problem without solving it.

Kevin

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>> > On 11.10.23 13:18, Fiona Ebner wrote:
>> >> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
>> >>> On 09.10.23 12:46, Fiona Ebner wrote:
>> >>>>
>> >>>> Initially, I tried to go for a more general 'job-change' command, but
>> >>>> I couldn't figure out a way to avoid mutual inclusion between
>> >>>> block-core.json and job.json.
>> >>>>
>> >>>
>> >>> What is the problem with it? I still think that job-change would be better.
>> >>>
>> >> If going for job-change in job.json, the dependencies would be
>> >> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
>> >> query-jobs -> JobInfo -> JobInfoMirror
>> >> and we can't include block-core.json in job.json, because an inclusion
>> >> loop gives a build error.
>> 
>> Let me try to understand this.
>> 
>> Command job-change needs its argument type JobChangeOptions.
>> 
>> JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
>> branches.
>> 
>> JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.
>> 
>> block-core.json needs job.json for JobType and JobStatus.
>> 
>> >> Could be made to work by moving MirrorCopyMode (and
>> >> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
>> >> can be included by both job.json and block-core.json. Moving the
>> >> type-specific definitions to the general job.json didn't feel right to
>> >> me. Including another file with type-specific definitions in job.json
>> >> feels slightly less wrong, but still not quite right and I didn't want
>> >> to create a new file just for MirrorCopyMode (and
>> >> JobChangeOptionsMirror, JobInfoMirror).
>> >> And going further and moving all mirror-related things to a separate
>> >> file would require moving along things like NewImageMode with it or
>> >> create yet another file for such general things used by multiple block-jobs.
>> >> If preferred, I can try and go with some version of the above.
>> >> 
>> >
>> > OK, I see the problem. Seems, that all requires some good refactoring. But that's a preexisting big work, and should not hold up your series. I'm OK to proceed with block-job-change.
>> 
>> Saving ourselves some internal refactoring is a poor excuse for
>> undesirable external interfaces.
>
> I'm not sure how undesirable it is. We have block-job-* commands for
> pretty much every other operation, so it's only consistent to have
> block-job-change, too.

Is the job abstraction a failure?

We have

    block-job- command      since   job- command    since
    -----------------------------------------------------
    block-job-set-speed     1.1
    block-job-cancel        1.1     job-cancel      3.0
    block-job-pause         1.3     job-pause       3.0
    block-job-resume        1.3     job-resume      3.0
    block-job-complete      1.3     job-complete    3.0
    block-job-dismiss       2.12    job-dismiss     3.0
    block-job-finalize      2.12    job-finalize    3.0
    block-job-change        8.2
    query-block-jobs        1.1     query-jobs

I was under the impression that we added the (more general) job-
commands to replace the (less general) block-job commands, and we're
keeping the latter just for compatibility.  Am I mistaken?

Which one should be used?

Why not deprecate the one that shouldn't be used?

The addition of block-job-change without even trying to do job-change
makes me wonder: have we given up on the job- interface?

I'm okay with giving up on failures.  All I want is clarity.  Right now,
I feel thoroughly confused about the status block-jobs and jobs, and how
they're related.

> Having job-change, too, might be nice in theory, but we don't have even
> a potential user for it at this point (i.e. a job type that isn't a
> block job, but for which changing options at runtime makes sense).
>
>> We need to answer two questions before we do that:
>> 
>> 1. How much work would the refactoring be?
>> 
>> 2. Is the interface improvement this enables worth the work?
>> 
>> Let's start with 1.
>> 
>> An obvious solution is to split JobType and JobStatus off job.json to
>> break the dependency of block-core.json on job.json.
>> 
>> But I'd like us to investigate another one.  block-core.json is *huge*.
>> It's almost a quarter of the entire QAPI schema.  Can we spin out block
>> jobs into block-job.json?  Moves the dependency on job.json from
>> block-core.json to block-job.json.
>
> It also makes job.json depend on block-job.json instead of
> block-core.json, so you only moved the problem without solving it.

block-job.json needs block-core.json and job.json.

job.json needs block-core.json.

No circle so far.

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 03.11.23 18:56, Markus Armbruster wrote:
> Kevin Wolf<kwolf@redhat.com>  writes:
> 
>> Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben:
>>> Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>  writes:
>>>
>>>> On 11.10.23 13:18, Fiona Ebner wrote:
>>>>> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
>>>>>> On 09.10.23 12:46, Fiona Ebner wrote:
>>>>>>> Initially, I tried to go for a more general 'job-change' command, but
>>>>>>> I couldn't figure out a way to avoid mutual inclusion between
>>>>>>> block-core.json and job.json.
>>>>>>>
>>>>>> What is the problem with it? I still think that job-change would be better.
>>>>>>
>>>>> If going for job-change in job.json, the dependencies would be
>>>>> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
>>>>> query-jobs -> JobInfo -> JobInfoMirror
>>>>> and we can't include block-core.json in job.json, because an inclusion
>>>>> loop gives a build error.
>>> Let me try to understand this.
>>>
>>> Command job-change needs its argument type JobChangeOptions.
>>>
>>> JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
>>> branches.
>>>
>>> JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.
>>>
>>> block-core.json needs job.json for JobType and JobStatus.
>>>
>>>>> Could be made to work by moving MirrorCopyMode (and
>>>>> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
>>>>> can be included by both job.json and block-core.json. Moving the
>>>>> type-specific definitions to the general job.json didn't feel right to
>>>>> me. Including another file with type-specific definitions in job.json
>>>>> feels slightly less wrong, but still not quite right and I didn't want
>>>>> to create a new file just for MirrorCopyMode (and
>>>>> JobChangeOptionsMirror, JobInfoMirror).
>>>>> And going further and moving all mirror-related things to a separate
>>>>> file would require moving along things like NewImageMode with it or
>>>>> create yet another file for such general things used by multiple block-jobs.
>>>>> If preferred, I can try and go with some version of the above.
>>>>>
>>>> OK, I see the problem. Seems, that all requires some good refactoring. But that's a preexisting big work, and should not hold up your series. I'm OK to proceed with block-job-change.
>>> Saving ourselves some internal refactoring is a poor excuse for
>>> undesirable external interfaces.
>> I'm not sure how undesirable it is. We have block-job-* commands for
>> pretty much every other operation, so it's only consistent to have
>> block-job-change, too.
> Is the job abstraction a failure?
> 
> We have
> 
>      block-job- command      since   job- command    since
>      -----------------------------------------------------
>      block-job-set-speed     1.1
>      block-job-cancel        1.1     job-cancel      3.0
>      block-job-pause         1.3     job-pause       3.0
>      block-job-resume        1.3     job-resume      3.0
>      block-job-complete      1.3     job-complete    3.0
>      block-job-dismiss       2.12    job-dismiss     3.0
>      block-job-finalize      2.12    job-finalize    3.0
>      block-job-change        8.2
>      query-block-jobs        1.1     query-jobs
> 
> I was under the impression that we added the (more general) job-
> commands to replace the (less general) block-job commands, and we're
> keeping the latter just for compatibility.  Am I mistaken?
> 
> Which one should be used?
> 
> Why not deprecate the one that shouldn't be used?
> 
> The addition of block-job-change without even trying to do job-change
> makes me wonder: have we given up on the job- interface?
> 
> I'm okay with giving up on failures.  All I want is clarity.  Right now,
> I feel thoroughly confused about the status block-jobs and jobs, and how
> they're related.

Hi! I didn't notice, that the series was finally merged.

About the APIs, I think, of course we should deprecate block-job-* API, because we already have jobs which are not block-jobs, so we can't deprecate job-* API.

So I suggest a plan:

1. Add job-change command simply in block-core.json, as a simple copy of block-job-change, to not care with resolving inclusion loops. (ha we could simply name our block-job-change to be job-change and place it in block-core.json, but now is too late)

2. Support changing speed in a new job-chage command. (or both in block-job-change and job-change, keeping them equal)

3. Deprecate block-job-* APIs

4. Wait 3 releases

5. Drop block-job-* APIs

6. Move all job-related stuff to job.json, drop `{ 'include': 'job.json' }` from block-core.json, and instead include block-core.json into job.json

If it's OK, I can go through the steps.

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 03.11.23 18:56, Markus Armbruster wrote:
>> Is the job abstraction a failure?
>>
>> We have
>>
>>      block-job- command      since   job- command    since
>>      -----------------------------------------------------
>>      block-job-set-speed     1.1
>>      block-job-cancel        1.1     job-cancel      3.0
>>      block-job-pause         1.3     job-pause       3.0
>>      block-job-resume        1.3     job-resume      3.0
>>      block-job-complete      1.3     job-complete    3.0
>>      block-job-dismiss       2.12    job-dismiss     3.0
>>      block-job-finalize      2.12    job-finalize    3.0
>>      block-job-change        8.2
>>      query-block-jobs        1.1     query-jobs
>>
>> I was under the impression that we added the (more general) job-
>> commands to replace the (less general) block-job commands, and we're
>> keeping the latter just for compatibility.  Am I mistaken?
>>
>> Which one should be used?
>>
>> Why not deprecate the one that shouldn't be used?
>>
>> The addition of block-job-change without even trying to do job-change
>> makes me wonder: have we given up on the job- interface?
>>
>> I'm okay with giving up on failures.  All I want is clarity.  Right now,
>> I feel thoroughly confused about the status block-jobs and jobs, and how
>> they're related.
>
> Hi! I didn't notice, that the series was finally merged.
>
> About the APIs, I think, of course we should deprecate block-job-* API, because we already have jobs which are not block-jobs, so we can't deprecate job-* API.
>
> So I suggest a plan:
>
> 1. Add job-change command simply in block-core.json, as a simple copy of block-job-change, to not care with resolving inclusion loops. (ha we could simply name our block-job-change to be job-change and place it in block-core.json, but now is too late)
>
> 2. Support changing speed in a new job-chage command. (or both in block-job-change and job-change, keeping them equal)
>
> 3. Deprecate block-job-* APIs
>
> 4. Wait 3 releases
>
> 5. Drop block-job-* APIs
>
> 6. Move all job-related stuff to job.json, drop `{ 'include': 'job.json' }` from block-core.json, and instead include block-core.json into job.json

Sounds good to me.

> If it's OK, I can go through the steps.

Lovely!

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Kevin Wolf]

Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 03.11.23 18:56, Markus Armbruster wrote:
> > Kevin Wolf<kwolf@redhat.com>  writes:
> > 
> > > Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben:
> > > > Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>  writes:
> > > > 
> > > > > On 11.10.23 13:18, Fiona Ebner wrote:
> > > > > > Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
> > > > > > > On 09.10.23 12:46, Fiona Ebner wrote:
> > > > > > > > Initially, I tried to go for a more general 'job-change' command, but
> > > > > > > > I couldn't figure out a way to avoid mutual inclusion between
> > > > > > > > block-core.json and job.json.
> > > > > > > > 
> > > > > > > What is the problem with it? I still think that job-change would be better.
> > > > > > > 
> > > > > > If going for job-change in job.json, the dependencies would be
> > > > > > job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
> > > > > > query-jobs -> JobInfo -> JobInfoMirror
> > > > > > and we can't include block-core.json in job.json, because an inclusion
> > > > > > loop gives a build error.
> > > > Let me try to understand this.
> > > > 
> > > > Command job-change needs its argument type JobChangeOptions.
> > > > 
> > > > JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
> > > > branches.
> > > > 
> > > > JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.
> > > > 
> > > > block-core.json needs job.json for JobType and JobStatus.
> > > > 
> > > > > > Could be made to work by moving MirrorCopyMode (and
> > > > > > JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
> > > > > > can be included by both job.json and block-core.json. Moving the
> > > > > > type-specific definitions to the general job.json didn't feel right to
> > > > > > me. Including another file with type-specific definitions in job.json
> > > > > > feels slightly less wrong, but still not quite right and I didn't want
> > > > > > to create a new file just for MirrorCopyMode (and
> > > > > > JobChangeOptionsMirror, JobInfoMirror).
> > > > > > And going further and moving all mirror-related things to a separate
> > > > > > file would require moving along things like NewImageMode with it or
> > > > > > create yet another file for such general things used by multiple block-jobs.
> > > > > > If preferred, I can try and go with some version of the above.
> > > > > > 
> > > > > OK, I see the problem. Seems, that all requires some good refactoring. But that's a preexisting big work, and should not hold up your series. I'm OK to proceed with block-job-change.
> > > > Saving ourselves some internal refactoring is a poor excuse for
> > > > undesirable external interfaces.
> > > I'm not sure how undesirable it is. We have block-job-* commands for
> > > pretty much every other operation, so it's only consistent to have
> > > block-job-change, too.
> > Is the job abstraction a failure?
> > 
> > We have
> > 
> >      block-job- command      since   job- command    since
> >      -----------------------------------------------------
> >      block-job-set-speed     1.1
> >      block-job-cancel        1.1     job-cancel      3.0
> >      block-job-pause         1.3     job-pause       3.0
> >      block-job-resume        1.3     job-resume      3.0
> >      block-job-complete      1.3     job-complete    3.0
> >      block-job-dismiss       2.12    job-dismiss     3.0
> >      block-job-finalize      2.12    job-finalize    3.0
> >      block-job-change        8.2
> >      query-block-jobs        1.1     query-jobs
> > 
> > I was under the impression that we added the (more general) job-
> > commands to replace the (less general) block-job commands, and we're
> > keeping the latter just for compatibility.  Am I mistaken?
> > 
> > Which one should be used?
> > 
> > Why not deprecate the one that shouldn't be used?
> > 
> > The addition of block-job-change without even trying to do job-change
> > makes me wonder: have we given up on the job- interface?
> > 
> > I'm okay with giving up on failures.  All I want is clarity.  Right now,
> > I feel thoroughly confused about the status block-jobs and jobs, and how
> > they're related.
> 
> Hi! I didn't notice, that the series was finally merged.
> 
> About the APIs, I think, of course we should deprecate block-job-* API, because we already have jobs which are not block-jobs, so we can't deprecate job-* API.
> 
> So I suggest a plan:
> 
> 1. Add job-change command simply in block-core.json, as a simple copy
>    of block-job-change, to not care with resolving inclusion loops.
>    (ha we could simply name our block-job-change to be job-change and
>    place it in block-core.json, but now is too late)
> 
> 2. Support changing speed in a new job-chage command. (or both in
>    block-job-change and job-change, keeping them equal)

It should be both block-job-change and job-change.

Having job-change in block-core.json rather than job.json is ugly, but
if Markus doesn't complain, why would I.

> 3. Deprecate block-job-* APIs
> 
> 4. Wait 3 releases
> 
> 5. Drop block-job-* APIs

I consider these strictly optional. We don't really have strong reasons
to deprecate these commands (they are just thin wrappers), and I think
libvirt still uses block-job-* in some places.

We also need to check if the interfaces are really the same. For
example, JobInfo is only a small subset of BlockJobInfo. Some things
could be added to JobInfo, other things like BlockDeviceIoStatus don't
really have a place there, so we would have to introduce job type
specific data in query-jobs first.

I'm sure it's all doable, but it might be more work than your list above
would make you think.

> 6. Move all job-related stuff to job.json, drop `{ 'include':
>    'job.json' }` from block-core.json, and instead include
>    block-core.json into job.json

Of course, this cleanup assumes that steps 3.-5. are really implemented.
If not, you would end up moving a lot more block related things to
job.json than after them.

Kevin

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Peter Krempa]

On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
> Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > On 03.11.23 18:56, Markus Armbruster wrote:
> > > Kevin Wolf<kwolf@redhat.com>  writes:

[...]

> > > Is the job abstraction a failure?
> > > 
> > > We have
> > > 
> > >      block-job- command      since   job- command    since
> > >      -----------------------------------------------------
> > >      block-job-set-speed     1.1
> > >      block-job-cancel        1.1     job-cancel      3.0
> > >      block-job-pause         1.3     job-pause       3.0
> > >      block-job-resume        1.3     job-resume      3.0
> > >      block-job-complete      1.3     job-complete    3.0
> > >      block-job-dismiss       2.12    job-dismiss     3.0
> > >      block-job-finalize      2.12    job-finalize    3.0
> > >      block-job-change        8.2
> > >      query-block-jobs        1.1     query-jobs

[...]

> I consider these strictly optional. We don't really have strong reasons
> to deprecate these commands (they are just thin wrappers), and I think
> libvirt still uses block-job-* in some places.

Libvirt uses 'block-job-cancel' because it has different semantics from
'job-cancel' which libvirt documented as the behaviour of the API that
uses it. (Semantics regarding the expectation of what is written to the
destination node at the point when the job is cancelled).

Libvirt also uses 'block-job-set-speed' and 'query-block-jobs' but those
can be replaced easily and looking at the above table even without any
feature checks.

Thus the plan to deprecate at least 'block-job-cancel' will not work
unless the semantics are ported into 'job-cancel'.

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:

[...]

>> About the APIs, I think, of course we should deprecate block-job-* API, because we already have jobs which are not block-jobs, so we can't deprecate job-* API.
>> 
>> So I suggest a plan:
>> 
>> 1. Add job-change command simply in block-core.json, as a simple copy
>>    of block-job-change, to not care with resolving inclusion loops.
>>    (ha we could simply name our block-job-change to be job-change and
>>    place it in block-core.json, but now is too late)
>> 
>> 2. Support changing speed in a new job-chage command. (or both in
>>    block-job-change and job-change, keeping them equal)
>
> It should be both block-job-change and job-change.
>
> Having job-change in block-core.json rather than job.json is ugly, but
> if Markus doesn't complain, why would I.

What we have is ugly and confusing: two interfaces with insufficient
guidance on which one to use.

Unifying the interfaces will reduce confusion immediately, and may
reduce ugliness eventually.

I take it.

[...]

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 04.03.24 14:09, Peter Krempa wrote:
> On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
>> Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> On 03.11.23 18:56, Markus Armbruster wrote:
>>>> Kevin Wolf<kwolf@redhat.com>  writes:
> 
> [...]
> 
>>>> Is the job abstraction a failure?
>>>>
>>>> We have
>>>>
>>>>       block-job- command      since   job- command    since
>>>>       -----------------------------------------------------
>>>>       block-job-set-speed     1.1
>>>>       block-job-cancel        1.1     job-cancel      3.0
>>>>       block-job-pause         1.3     job-pause       3.0
>>>>       block-job-resume        1.3     job-resume      3.0
>>>>       block-job-complete      1.3     job-complete    3.0
>>>>       block-job-dismiss       2.12    job-dismiss     3.0
>>>>       block-job-finalize      2.12    job-finalize    3.0
>>>>       block-job-change        8.2
>>>>       query-block-jobs        1.1     query-jobs
> 
> [...]
> 
>> I consider these strictly optional. We don't really have strong reasons
>> to deprecate these commands (they are just thin wrappers), and I think
>> libvirt still uses block-job-* in some places.
> 
> Libvirt uses 'block-job-cancel' because it has different semantics from
> 'job-cancel' which libvirt documented as the behaviour of the API that
> uses it. (Semantics regarding the expectation of what is written to the
> destination node at the point when the job is cancelled).
> 

That's the following semantics:

   # Note that if you issue 'block-job-cancel' after 'drive-mirror' has
   # indicated (via the event BLOCK_JOB_READY) that the source and
   # destination are synchronized, then the event triggered by this
   # command changes to BLOCK_JOB_COMPLETED, to indicate that the
   # mirroring has ended and the destination now has a point-in-time copy
   # tied to the time of the cancellation.

Hmm. Looking at this, it looks for me, that should probably a 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).

Actually, what is the difference between block-job-complete and block-job-cancel(force=false) for mirror in ready state?

I only see the following differencies:

1. block-job-complete documents that it completes the job synchronously.. But looking at mirror code I see it just set s->should_complete = true, which will be then handled asynchronously.. So I doubt that documentation is correct.

2. block-job-complete will trigger final graph changes. block-job-cancel will not.

Is [2] really useful? Seems yes: in case of some failure before starting migration target, we'd like to continue executing source. So, no reason to break block-graph in source, better keep it unchanged.

But I think, such behavior better be setup by mirror-job start parameter, rather then by special option for cancel (or even compelete) command, useful only for mirror.

So, what about the following substitution for block-job-cancel:

block-job-cancel(force=true)  -->  use job-cancel

block-job-cancel(force=false) for backup, stream, commit  -->  use job-cancel

block-job-cancel(force=false) for mirror in ready mode  -->

   instead, use block-job-complete. If you don't need final graph modification which mirror job normally does, use graph-change=false parameter for blockdev-mirror command.


(I can hardly remember, that we've already discussed something like this long time ago, but I don't remember the results)

I also a bit unsure about active commit soft-cancelling semantics. Is it actually useful? If yes, block-commit command will need similar option.

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Fiona Ebner]

Am 07.03.24 um 20:42 schrieb Vladimir Sementsov-Ogievskiy:
> On 04.03.24 14:09, Peter Krempa wrote:
>> On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
>>> Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> On 03.11.23 18:56, Markus Armbruster wrote:
>>>>> Kevin Wolf<kwolf@redhat.com>  writes:
>>
>> [...]
>>
>>>>> Is the job abstraction a failure?
>>>>>
>>>>> We have
>>>>>
>>>>>       block-job- command      since   job- command    since
>>>>>       -----------------------------------------------------
>>>>>       block-job-set-speed     1.1
>>>>>       block-job-cancel        1.1     job-cancel      3.0
>>>>>       block-job-pause         1.3     job-pause       3.0
>>>>>       block-job-resume        1.3     job-resume      3.0
>>>>>       block-job-complete      1.3     job-complete    3.0
>>>>>       block-job-dismiss       2.12    job-dismiss     3.0
>>>>>       block-job-finalize      2.12    job-finalize    3.0
>>>>>       block-job-change        8.2
>>>>>       query-block-jobs        1.1     query-jobs
>>
>> [...]
>>
>>> I consider these strictly optional. We don't really have strong reasons
>>> to deprecate these commands (they are just thin wrappers), and I think
>>> libvirt still uses block-job-* in some places.
>>
>> Libvirt uses 'block-job-cancel' because it has different semantics from
>> 'job-cancel' which libvirt documented as the behaviour of the API that
>> uses it. (Semantics regarding the expectation of what is written to the
>> destination node at the point when the job is cancelled).
>>
> 
> That's the following semantics:
> 
>   # Note that if you issue 'block-job-cancel' after 'drive-mirror' has
>   # indicated (via the event BLOCK_JOB_READY) that the source and
>   # destination are synchronized, then the event triggered by this
>   # command changes to BLOCK_JOB_COMPLETED, to indicate that the
>   # mirroring has ended and the destination now has a point-in-time copy
>   # tied to the time of the cancellation.
> 
> Hmm. Looking at this, it looks for me, that should probably a
> 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).
> 
> Actually, what is the difference between block-job-complete and
> block-job-cancel(force=false) for mirror in ready state?
> 
> I only see the following differencies:
> 
> 1. block-job-complete documents that it completes the job
> synchronously.. But looking at mirror code I see it just set
> s->should_complete = true, which will be then handled asynchronously..
> So I doubt that documentation is correct.
> 
> 2. block-job-complete will trigger final graph changes. block-job-cancel
> will not.
> 
> Is [2] really useful? Seems yes: in case of some failure before starting
> migration target, we'd like to continue executing source. So, no reason
> to break block-graph in source, better keep it unchanged.
> 

FWIW, we also rely on these special semantics. We allow cloning the disk
state of a running guest using drive-mirror (and before finishing,
fsfreeze in the guest for consistency). We cannot use block-job-complete
there, because we do not want to switch the source's drive.

> But I think, such behavior better be setup by mirror-job start
> parameter, rather then by special option for cancel (or even compelete)
> command, useful only for mirror.
> 
> So, what about the following substitution for block-job-cancel:
> 
> block-job-cancel(force=true)  -->  use job-cancel
> 
> block-job-cancel(force=false) for backup, stream, commit  -->  use
> job-cancel
> 
> block-job-cancel(force=false) for mirror in ready mode  -->
> 
>   instead, use block-job-complete. If you don't need final graph
> modification which mirror job normally does, use graph-change=false
> parameter for blockdev-mirror command.
> 

But yes, having a graph-change parameter would work for us too :)

Best Regards,
Fiona

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Kevin Wolf]

Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 04.03.24 14:09, Peter Krempa wrote:
> > On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
> > > Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > On 03.11.23 18:56, Markus Armbruster wrote:
> > > > > Kevin Wolf<kwolf@redhat.com>  writes:
> > 
> > [...]
> > 
> > > > > Is the job abstraction a failure?
> > > > > 
> > > > > We have
> > > > > 
> > > > >       block-job- command      since   job- command    since
> > > > >       -----------------------------------------------------
> > > > >       block-job-set-speed     1.1
> > > > >       block-job-cancel        1.1     job-cancel      3.0
> > > > >       block-job-pause         1.3     job-pause       3.0
> > > > >       block-job-resume        1.3     job-resume      3.0
> > > > >       block-job-complete      1.3     job-complete    3.0
> > > > >       block-job-dismiss       2.12    job-dismiss     3.0
> > > > >       block-job-finalize      2.12    job-finalize    3.0
> > > > >       block-job-change        8.2
> > > > >       query-block-jobs        1.1     query-jobs
> > 
> > [...]
> > 
> > > I consider these strictly optional. We don't really have strong reasons
> > > to deprecate these commands (they are just thin wrappers), and I think
> > > libvirt still uses block-job-* in some places.
> > 
> > Libvirt uses 'block-job-cancel' because it has different semantics from
> > 'job-cancel' which libvirt documented as the behaviour of the API that
> > uses it. (Semantics regarding the expectation of what is written to the
> > destination node at the point when the job is cancelled).
> > 
> 
> That's the following semantics:
> 
>   # Note that if you issue 'block-job-cancel' after 'drive-mirror' has
>   # indicated (via the event BLOCK_JOB_READY) that the source and
>   # destination are synchronized, then the event triggered by this
>   # command changes to BLOCK_JOB_COMPLETED, to indicate that the
>   # mirroring has ended and the destination now has a point-in-time copy
>   # tied to the time of the cancellation.
> 
> Hmm. Looking at this, it looks for me, that should probably a
> 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).

Yes, it's just a different completion mode.

> Actually, what is the difference between block-job-complete and
> block-job-cancel(force=false) for mirror in ready state?
> 
> I only see the following differencies:
> 
> 1. block-job-complete documents that it completes the job
>    synchronously.. But looking at mirror code I see it just set
>    s->should_complete = true, which will be then handled
>    asynchronously..  So I doubt that documentation is correct.
> 
> 2. block-job-complete will trigger final graph changes.
>    block-job-cancel will not.
> 
> Is [2] really useful? Seems yes: in case of some failure before
> starting migration target, we'd like to continue executing source. So,
> no reason to break block-graph in source, better keep it unchanged.
> 
> But I think, such behavior better be setup by mirror-job start
> parameter, rather then by special option for cancel (or even
> compelete) command, useful only for mirror.

I'm not sure, having the option on the complete command makes more sense
to me than having it in blockdev-mirror.

I do see the challenge of representing this meaningfully in QAPI,
though. Semantically it should be a union with job-specific options and
only mirror adds the graph-changes option. But the union variant
can't be directly selected from another option - instead we have a job
ID, and the variant is the job type of the job with this ID.

Practically speaking, we would probably indeed end up with an optional
field in the existing completion command.

> So, what about the following substitution for block-job-cancel:
> 
> block-job-cancel(force=true)  -->  use job-cancel
> 
> block-job-cancel(force=false) for backup, stream, commit  -->  use job-cancel
> 
> block-job-cancel(force=false) for mirror in ready mode  -->
> 
>   instead, use block-job-complete. If you don't need final graph
>   modification which mirror job normally does, use graph-change=false
>   parameter for blockdev-mirror command.

Apart from the open question where to put the option, agreed.

> (I can hardly remember, that we've already discussed something like
> this long time ago, but I don't remember the results)

I think everyone agreed that this is how things should be, and nobody
did anything to achieve it.

> I also a bit unsure about active commit soft-cancelling semantics. Is
> it actually useful? If yes, block-commit command will need similar
> option.

Hm... That would commit everything down to the lower layer and then keep
the old overlays still around?

I could see a limited use case for committing into the immediate backing
file and then keep using the overlay to accumulate new changes (would be
more useful if we discarded the old content). Once you have intermediate
backing files, I don't think it makes any sense any more.

Kevin

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Peter Krempa]

On Thu, Mar 07, 2024 at 22:42:56 +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 04.03.24 14:09, Peter Krempa wrote:
> > On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
> > > Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > On 03.11.23 18:56, Markus Armbruster wrote:
> > > > > Kevin Wolf<kwolf@redhat.com>  writes:

[...]

> 
> I only see the following differencies:
> 
> 1. block-job-complete documents that it completes the job synchronously.. But looking at mirror code I see it just set s->should_complete = true, which will be then handled asynchronously.. So I doubt that documentation is correct.
> 
> 2. block-job-complete will trigger final graph changes. block-job-cancel will not.
> 
> Is [2] really useful? Seems yes: in case of some failure before starting migration target, we'd like to continue executing source. So, no reason to break block-graph in source, better keep it unchanged.
> 
> But I think, such behavior better be setup by mirror-job start parameter, rather then by special option for cancel (or even compelete) command, useful only for mirror.

Libvirt's API design was based on the qemu quirk and thus we allow users
to do the decision after starting the job, thus anything you pick needs
to allow us to do this at "completion" time.

Libvirt can adapt to any option that will give us the above semantics
(extra parameter at completion time, different completion command or
extra command to switch job properties right before completion), but to
be honest all of these feel like they would be more hassle than keeping
'block-job-cancel' around from qemu's side.

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 08.03.24 11:52, Kevin Wolf wrote:
> Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> On 04.03.24 14:09, Peter Krempa wrote:
>>> On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
>>>> Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> On 03.11.23 18:56, Markus Armbruster wrote:
>>>>>> Kevin Wolf<kwolf@redhat.com>  writes:
>>>
>>> [...]
>>>
>>>>>> Is the job abstraction a failure?
>>>>>>
>>>>>> We have
>>>>>>
>>>>>>        block-job- command      since   job- command    since
>>>>>>        -----------------------------------------------------
>>>>>>        block-job-set-speed     1.1
>>>>>>        block-job-cancel        1.1     job-cancel      3.0
>>>>>>        block-job-pause         1.3     job-pause       3.0
>>>>>>        block-job-resume        1.3     job-resume      3.0
>>>>>>        block-job-complete      1.3     job-complete    3.0
>>>>>>        block-job-dismiss       2.12    job-dismiss     3.0
>>>>>>        block-job-finalize      2.12    job-finalize    3.0
>>>>>>        block-job-change        8.2
>>>>>>        query-block-jobs        1.1     query-jobs
>>>
>>> [...]
>>>
>>>> I consider these strictly optional. We don't really have strong reasons
>>>> to deprecate these commands (they are just thin wrappers), and I think
>>>> libvirt still uses block-job-* in some places.
>>>
>>> Libvirt uses 'block-job-cancel' because it has different semantics from
>>> 'job-cancel' which libvirt documented as the behaviour of the API that
>>> uses it. (Semantics regarding the expectation of what is written to the
>>> destination node at the point when the job is cancelled).
>>>
>>
>> That's the following semantics:
>>
>>    # Note that if you issue 'block-job-cancel' after 'drive-mirror' has
>>    # indicated (via the event BLOCK_JOB_READY) that the source and
>>    # destination are synchronized, then the event triggered by this
>>    # command changes to BLOCK_JOB_COMPLETED, to indicate that the
>>    # mirroring has ended and the destination now has a point-in-time copy
>>    # tied to the time of the cancellation.
>>
>> Hmm. Looking at this, it looks for me, that should probably a
>> 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).
> 
> Yes, it's just a different completion mode.
> 
>> Actually, what is the difference between block-job-complete and
>> block-job-cancel(force=false) for mirror in ready state?
>>
>> I only see the following differencies:
>>
>> 1. block-job-complete documents that it completes the job
>>     synchronously.. But looking at mirror code I see it just set
>>     s->should_complete = true, which will be then handled
>>     asynchronously..  So I doubt that documentation is correct.
>>
>> 2. block-job-complete will trigger final graph changes.
>>     block-job-cancel will not.
>>
>> Is [2] really useful? Seems yes: in case of some failure before
>> starting migration target, we'd like to continue executing source. So,
>> no reason to break block-graph in source, better keep it unchanged.
>>
>> But I think, such behavior better be setup by mirror-job start
>> parameter, rather then by special option for cancel (or even
>> compelete) command, useful only for mirror.
> 
> I'm not sure, having the option on the complete command makes more sense
> to me than having it in blockdev-mirror.
> 
> I do see the challenge of representing this meaningfully in QAPI,
> though. Semantically it should be a union with job-specific options and
> only mirror adds the graph-changes option. But the union variant
> can't be directly selected from another option - instead we have a job
> ID, and the variant is the job type of the job with this ID.

We already have such command: block-job-change. Which has id and type parameters, so user have to pass both, to identify the job itself and pick corresponding variant of the union type.

That would be good to somehow teach QAPI to get the type automatically from the job itself...


Probably, best way is to utilize the new "change" command?

So, to avoid final graph change, user should either set graph-change=false in blockdev-mirror, or just call job-change(graph-change=false) before job-complete?


Still having the option for job-complete looks nicer. But right, we either have to add type parameter like in block-job-change, or add a common option, which would be irrelevant to some jobs.

> 
> Practically speaking, we would probably indeed end up with an optional
> field in the existing completion command.
> 
>> So, what about the following substitution for block-job-cancel:
>>
>> block-job-cancel(force=true)  -->  use job-cancel
>>
>> block-job-cancel(force=false) for backup, stream, commit  -->  use job-cancel
>>
>> block-job-cancel(force=false) for mirror in ready mode  -->
>>
>>    instead, use block-job-complete. If you don't need final graph
>>    modification which mirror job normally does, use graph-change=false
>>    parameter for blockdev-mirror command.
> 
> Apart from the open question where to put the option, agreed.
> 
>> (I can hardly remember, that we've already discussed something like
>> this long time ago, but I don't remember the results)
> 
> I think everyone agreed that this is how things should be, and nobody
> did anything to achieve it.
> 
>> I also a bit unsure about active commit soft-cancelling semantics. Is
>> it actually useful? If yes, block-commit command will need similar
>> option.
> 
> Hm... That would commit everything down to the lower layer and then keep
> the old overlays still around?
> 
> I could see a limited use case for committing into the immediate backing
> file and then keep using the overlay to accumulate new changes (would be
> more useful if we discarded the old content). Once you have intermediate
> backing files, I don't think it makes any sense any more.
> 
> Kevin
> 

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 11.03.24 00:07, Peter Krempa wrote:
> On Thu, Mar 07, 2024 at 22:42:56 +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 04.03.24 14:09, Peter Krempa wrote:
>>> On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
>>>> Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> On 03.11.23 18:56, Markus Armbruster wrote:
>>>>>> Kevin Wolf<kwolf@redhat.com>  writes:
> 
> [...]
> 
>>
>> I only see the following differencies:
>>
>> 1. block-job-complete documents that it completes the job synchronously.. But looking at mirror code I see it just set s->should_complete = true, which will be then handled asynchronously.. So I doubt that documentation is correct.
>>
>> 2. block-job-complete will trigger final graph changes. block-job-cancel will not.
>>
>> Is [2] really useful? Seems yes: in case of some failure before starting migration target, we'd like to continue executing source. So, no reason to break block-graph in source, better keep it unchanged.
>>
>> But I think, such behavior better be setup by mirror-job start parameter, rather then by special option for cancel (or even compelete) command, useful only for mirror.
> 
> Libvirt's API design was based on the qemu quirk and thus we allow users
> to do the decision after starting the job, thus anything you pick needs
> to allow us to do this at "completion" time.
> 

OK, this means that simply switch to an option at job-start is not enough.

> Libvirt can adapt to any option that will give us the above semantics
> (extra parameter at completion time, different completion command or
> extra command to switch job properties right before completion), but to
> be honest all of these feel like they would be more hassle than keeping
> 'block-job-cancel' around from qemu's side.
> 

I understand. But still, it would be good to finally resolve the duplication between job-* and block-job-* APIs. We can keep old quirk working for a long time even after making a new consistent API.

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Peter Krempa]

On Mon, Mar 11, 2024 at 18:51:18 +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 11.03.24 00:07, Peter Krempa wrote:
> > On Thu, Mar 07, 2024 at 22:42:56 +0300, Vladimir Sementsov-Ogievskiy wrote:

[...]

> > Libvirt can adapt to any option that will give us the above semantics
> > (extra parameter at completion time, different completion command or
> > extra command to switch job properties right before completion), but to
> > be honest all of these feel like they would be more hassle than keeping
> > 'block-job-cancel' around from qemu's side.
> > 
> 
> I understand. But still, it would be good to finally resolve the duplication between job-* and block-job-* APIs. We can keep old quirk working for a long time even after making a new consistent API.

Sure, if you decide to go that way it's okay as long as we can avoid the
graph change at 'completion' time. However you decide to implement it
just let me know in advance so that I can prepare the libvirt patches
for it.

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 11.03.24 18:15, Vladimir Sementsov-Ogievskiy wrote:
> On 08.03.24 11:52, Kevin Wolf wrote:
>> Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> On 04.03.24 14:09, Peter Krempa wrote:
>>>> On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
>>>>> Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> On 03.11.23 18:56, Markus Armbruster wrote:
>>>>>>> Kevin Wolf<kwolf@redhat.com>  writes:
>>>>
>>>> [...]
>>>>
>>>>>>> Is the job abstraction a failure?
>>>>>>>
>>>>>>> We have
>>>>>>>
>>>>>>>        block-job- command      since   job- command    since
>>>>>>>        -----------------------------------------------------
>>>>>>>        block-job-set-speed     1.1
>>>>>>>        block-job-cancel        1.1     job-cancel      3.0
>>>>>>>        block-job-pause         1.3     job-pause       3.0
>>>>>>>        block-job-resume        1.3     job-resume      3.0
>>>>>>>        block-job-complete      1.3     job-complete    3.0
>>>>>>>        block-job-dismiss       2.12    job-dismiss     3.0
>>>>>>>        block-job-finalize      2.12    job-finalize    3.0
>>>>>>>        block-job-change        8.2
>>>>>>>        query-block-jobs        1.1     query-jobs
>>>>
>>>> [...]
>>>>
>>>>> I consider these strictly optional. We don't really have strong reasons
>>>>> to deprecate these commands (they are just thin wrappers), and I think
>>>>> libvirt still uses block-job-* in some places.
>>>>
>>>> Libvirt uses 'block-job-cancel' because it has different semantics from
>>>> 'job-cancel' which libvirt documented as the behaviour of the API that
>>>> uses it. (Semantics regarding the expectation of what is written to the
>>>> destination node at the point when the job is cancelled).
>>>>
>>>
>>> That's the following semantics:
>>>
>>>    # Note that if you issue 'block-job-cancel' after 'drive-mirror' has
>>>    # indicated (via the event BLOCK_JOB_READY) that the source and
>>>    # destination are synchronized, then the event triggered by this
>>>    # command changes to BLOCK_JOB_COMPLETED, to indicate that the
>>>    # mirroring has ended and the destination now has a point-in-time copy
>>>    # tied to the time of the cancellation.
>>>
>>> Hmm. Looking at this, it looks for me, that should probably a
>>> 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).
>>
>> Yes, it's just a different completion mode.
>>
>>> Actually, what is the difference between block-job-complete and
>>> block-job-cancel(force=false) for mirror in ready state?
>>>
>>> I only see the following differencies:
>>>
>>> 1. block-job-complete documents that it completes the job
>>>     synchronously.. But looking at mirror code I see it just set
>>>     s->should_complete = true, which will be then handled
>>>     asynchronously..  So I doubt that documentation is correct.
>>>
>>> 2. block-job-complete will trigger final graph changes.
>>>     block-job-cancel will not.
>>>
>>> Is [2] really useful? Seems yes: in case of some failure before
>>> starting migration target, we'd like to continue executing source. So,
>>> no reason to break block-graph in source, better keep it unchanged.
>>>
>>> But I think, such behavior better be setup by mirror-job start
>>> parameter, rather then by special option for cancel (or even
>>> compelete) command, useful only for mirror.
>>
>> I'm not sure, having the option on the complete command makes more sense
>> to me than having it in blockdev-mirror.
>>
>> I do see the challenge of representing this meaningfully in QAPI,
>> though. Semantically it should be a union with job-specific options and
>> only mirror adds the graph-changes option. But the union variant
>> can't be directly selected from another option - instead we have a job
>> ID, and the variant is the job type of the job with this ID.
> 
> We already have such command: block-job-change. Which has id and type parameters, so user have to pass both, to identify the job itself and pick corresponding variant of the union type.
> 
> That would be good to somehow teach QAPI to get the type automatically from the job itself...


Seems, that's easy enough to implement such a possibility, I'll try. At least now I have a prototype, which compiles

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0ae8ae62dc..332de67e52 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3116,13 +3116,11 @@
  #
  # @id: The job identifier
  #
-# @type: The job type
-#
  # Since: 8.2
  ##
  { 'union': 'BlockJobChangeOptions',
-  'base': { 'id': 'str', 'type': 'JobType' },
-  'discriminator': 'type',
+  'base': { 'id': 'str' },
+  'discriminator': 'JobType',
    'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }


to


bool visit_type_BlockJobChangeOptions_members(Visitor *v, BlockJobChangeOptions *obj, Error **errp)
{
     JobType tag;
     if (!visit_type_q_obj_BlockJobChangeOptions_base_members(v, (q_obj_BlockJobChangeOptions_base *)obj, errp)) {
         return false;
     }
     if (!BlockJobChangeOptions_mapper(obj, &tag, errp)) {
         return false;
     }
     switch (tag) {
     case JOB_TYPE_MIRROR:
         return visit_type_BlockJobChangeOptionsMirror_members(v, &obj->u.mirror, errp);
     case JOB_TYPE_COMMIT:
         break;
     ...



(specifying member as descriminator works too, and I'm not going to change the interface of block-job-change, it's just and example)

BlockJobChangeOptions_mapper() should be defined by hand, like this:

bool BlockJobChangeOptions_mapper(BlockJobChangeOptions *opts, JobType *out,
                                   Error **errp)
{
     BlockJob *job;

     JOB_LOCK_GUARD();

     job = find_block_job_locked(opts->id, errp);
     if (!job) {
         return false;
     }

     return job_type(&job->job);
}



-- 
Best regards,
Vladimir

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Kevin Wolf]

Am 12.03.2024 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 11.03.24 18:15, Vladimir Sementsov-Ogievskiy wrote:
> > On 08.03.24 11:52, Kevin Wolf wrote:
> > > Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > On 04.03.24 14:09, Peter Krempa wrote:
> > > > > On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
> > > > > > Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > > > On 03.11.23 18:56, Markus Armbruster wrote:
> > > > > > > > Kevin Wolf<kwolf@redhat.com>  writes:
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > > Is the job abstraction a failure?
> > > > > > > > 
> > > > > > > > We have
> > > > > > > > 
> > > > > > > >        block-job- command      since   job- command    since
> > > > > > > >        -----------------------------------------------------
> > > > > > > >        block-job-set-speed     1.1
> > > > > > > >        block-job-cancel        1.1     job-cancel      3.0
> > > > > > > >        block-job-pause         1.3     job-pause       3.0
> > > > > > > >        block-job-resume        1.3     job-resume      3.0
> > > > > > > >        block-job-complete      1.3     job-complete    3.0
> > > > > > > >        block-job-dismiss       2.12    job-dismiss     3.0
> > > > > > > >        block-job-finalize      2.12    job-finalize    3.0
> > > > > > > >        block-job-change        8.2
> > > > > > > >        query-block-jobs        1.1     query-jobs
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > I consider these strictly optional. We don't really have strong reasons
> > > > > > to deprecate these commands (they are just thin wrappers), and I think
> > > > > > libvirt still uses block-job-* in some places.
> > > > > 
> > > > > Libvirt uses 'block-job-cancel' because it has different semantics from
> > > > > 'job-cancel' which libvirt documented as the behaviour of the API that
> > > > > uses it. (Semantics regarding the expectation of what is written to the
> > > > > destination node at the point when the job is cancelled).
> > > > > 
> > > > 
> > > > That's the following semantics:
> > > > 
> > > >    # Note that if you issue 'block-job-cancel' after 'drive-mirror' has
> > > >    # indicated (via the event BLOCK_JOB_READY) that the source and
> > > >    # destination are synchronized, then the event triggered by this
> > > >    # command changes to BLOCK_JOB_COMPLETED, to indicate that the
> > > >    # mirroring has ended and the destination now has a point-in-time copy
> > > >    # tied to the time of the cancellation.
> > > > 
> > > > Hmm. Looking at this, it looks for me, that should probably a
> > > > 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).
> > > 
> > > Yes, it's just a different completion mode.
> > > 
> > > > Actually, what is the difference between block-job-complete and
> > > > block-job-cancel(force=false) for mirror in ready state?
> > > > 
> > > > I only see the following differencies:
> > > > 
> > > > 1. block-job-complete documents that it completes the job
> > > >     synchronously.. But looking at mirror code I see it just set
> > > >     s->should_complete = true, which will be then handled
> > > >     asynchronously..  So I doubt that documentation is correct.
> > > > 
> > > > 2. block-job-complete will trigger final graph changes.
> > > >     block-job-cancel will not.
> > > > 
> > > > Is [2] really useful? Seems yes: in case of some failure before
> > > > starting migration target, we'd like to continue executing source. So,
> > > > no reason to break block-graph in source, better keep it unchanged.
> > > > 
> > > > But I think, such behavior better be setup by mirror-job start
> > > > parameter, rather then by special option for cancel (or even
> > > > compelete) command, useful only for mirror.
> > > 
> > > I'm not sure, having the option on the complete command makes more sense
> > > to me than having it in blockdev-mirror.
> > > 
> > > I do see the challenge of representing this meaningfully in QAPI,
> > > though. Semantically it should be a union with job-specific options and
> > > only mirror adds the graph-changes option. But the union variant
> > > can't be directly selected from another option - instead we have a job
> > > ID, and the variant is the job type of the job with this ID.
> > 
> > We already have such command: block-job-change. Which has id and type parameters, so user have to pass both, to identify the job itself and pick corresponding variant of the union type.
> > 
> > That would be good to somehow teach QAPI to get the type automatically from the job itself...
> 
> 
> Seems, that's easy enough to implement such a possibility, I'll try. At least now I have a prototype, which compiles
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0ae8ae62dc..332de67e52 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3116,13 +3116,11 @@
>  #
>  # @id: The job identifier
>  #
> -# @type: The job type
> -#
>  # Since: 8.2
>  ##
>  { 'union': 'BlockJobChangeOptions',
> -  'base': { 'id': 'str', 'type': 'JobType' },
> -  'discriminator': 'type',
> +  'base': { 'id': 'str' },
> +  'discriminator': 'JobType',
>    'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }

We probably need some different syntax because I think in theory we
could get conflicts between option names and type names. But I'll leave
this discussion to Markus. I hope you can figure out something that he
is willing to accept, getting the variant from a callback looks like
useful functionality.

For output, your implementation is probably not optimal because you're
going to look up things the calling code already knows, but it's
probably not the end of the world.

Kevin

Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 12.03.24 18:49, Kevin Wolf wrote:
> Am 12.03.2024 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> On 11.03.24 18:15, Vladimir Sementsov-Ogievskiy wrote:
>>> On 08.03.24 11:52, Kevin Wolf wrote:
>>>> Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> On 04.03.24 14:09, Peter Krempa wrote:
>>>>>> On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
>>>>>>> Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>>>> On 03.11.23 18:56, Markus Armbruster wrote:
>>>>>>>>> Kevin Wolf<kwolf@redhat.com>  writes:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> Is the job abstraction a failure?
>>>>>>>>>
>>>>>>>>> We have
>>>>>>>>>
>>>>>>>>>         block-job- command      since   job- command    since
>>>>>>>>>         -----------------------------------------------------
>>>>>>>>>         block-job-set-speed     1.1
>>>>>>>>>         block-job-cancel        1.1     job-cancel      3.0
>>>>>>>>>         block-job-pause         1.3     job-pause       3.0
>>>>>>>>>         block-job-resume        1.3     job-resume      3.0
>>>>>>>>>         block-job-complete      1.3     job-complete    3.0
>>>>>>>>>         block-job-dismiss       2.12    job-dismiss     3.0
>>>>>>>>>         block-job-finalize      2.12    job-finalize    3.0
>>>>>>>>>         block-job-change        8.2
>>>>>>>>>         query-block-jobs        1.1     query-jobs
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> I consider these strictly optional. We don't really have strong reasons
>>>>>>> to deprecate these commands (they are just thin wrappers), and I think
>>>>>>> libvirt still uses block-job-* in some places.
>>>>>>
>>>>>> Libvirt uses 'block-job-cancel' because it has different semantics from
>>>>>> 'job-cancel' which libvirt documented as the behaviour of the API that
>>>>>> uses it. (Semantics regarding the expectation of what is written to the
>>>>>> destination node at the point when the job is cancelled).
>>>>>>
>>>>>
>>>>> That's the following semantics:
>>>>>
>>>>>     # Note that if you issue 'block-job-cancel' after 'drive-mirror' has
>>>>>     # indicated (via the event BLOCK_JOB_READY) that the source and
>>>>>     # destination are synchronized, then the event triggered by this
>>>>>     # command changes to BLOCK_JOB_COMPLETED, to indicate that the
>>>>>     # mirroring has ended and the destination now has a point-in-time copy
>>>>>     # tied to the time of the cancellation.
>>>>>
>>>>> Hmm. Looking at this, it looks for me, that should probably a
>>>>> 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).
>>>>
>>>> Yes, it's just a different completion mode.
>>>>
>>>>> Actually, what is the difference between block-job-complete and
>>>>> block-job-cancel(force=false) for mirror in ready state?
>>>>>
>>>>> I only see the following differencies:
>>>>>
>>>>> 1. block-job-complete documents that it completes the job
>>>>>      synchronously.. But looking at mirror code I see it just set
>>>>>      s->should_complete = true, which will be then handled
>>>>>      asynchronously..  So I doubt that documentation is correct.
>>>>>
>>>>> 2. block-job-complete will trigger final graph changes.
>>>>>      block-job-cancel will not.
>>>>>
>>>>> Is [2] really useful? Seems yes: in case of some failure before
>>>>> starting migration target, we'd like to continue executing source. So,
>>>>> no reason to break block-graph in source, better keep it unchanged.
>>>>>
>>>>> But I think, such behavior better be setup by mirror-job start
>>>>> parameter, rather then by special option for cancel (or even
>>>>> compelete) command, useful only for mirror.
>>>>
>>>> I'm not sure, having the option on the complete command makes more sense
>>>> to me than having it in blockdev-mirror.
>>>>
>>>> I do see the challenge of representing this meaningfully in QAPI,
>>>> though. Semantically it should be a union with job-specific options and
>>>> only mirror adds the graph-changes option. But the union variant
>>>> can't be directly selected from another option - instead we have a job
>>>> ID, and the variant is the job type of the job with this ID.
>>>
>>> We already have such command: block-job-change. Which has id and type parameters, so user have to pass both, to identify the job itself and pick corresponding variant of the union type.
>>>
>>> That would be good to somehow teach QAPI to get the type automatically from the job itself...
>>
>>
>> Seems, that's easy enough to implement such a possibility, I'll try. At least now I have a prototype, which compiles
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 0ae8ae62dc..332de67e52 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3116,13 +3116,11 @@
>>   #
>>   # @id: The job identifier
>>   #
>> -# @type: The job type
>> -#
>>   # Since: 8.2
>>   ##
>>   { 'union': 'BlockJobChangeOptions',
>> -  'base': { 'id': 'str', 'type': 'JobType' },
>> -  'discriminator': 'type',
>> +  'base': { 'id': 'str' },
>> +  'discriminator': 'JobType',
>>     'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }
> 
> We probably need some different syntax because I think in theory we
> could get conflicts between option names and type names. 

I think we shouldn't, as enum types in QAPI are CamelCase and members are lowercase. Still, it's probably a bad way to rely on text case (I just imagine documenting this:)) It could be "'discriminator-type': 'JobType'" instead.

> But I'll leave
> this discussion to Markus. I hope you can figure out something that he
> is willing to accept, getting the variant from a callback looks like
> useful functionality.
> 
> For output, your implementation is probably not optimal because you're
> going to look up things the calling code already knows, but it's
> probably not the end of the world.
> 

Yes I thought about it, we'll call find_block_job_locked() twice. That's probably solvable by including some opaque pointer to QAPI structure, which could be set during json parsing and then reused by qmp_* command itself.. But I don't think it worth doing for simple search for a job. Simpler approach would be to cache globally last result of find_block_job().. But again that would be extra optimization

-- 
Best regards,
Vladimir