[PATCH 0/9] mirror: allow switching from background to active mode

[PATCH 0/9] mirror: allow switching from background to active mode

[Fiona Ebner]

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 acheive 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 (9):
  blockjob: introduce block-job-change QMP command
  block/mirror: set actively_synced even after the job is ready
  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
  mirror: return the remaining dirty bytes upon query
  mirror: return the total number of bytes sent upon query

 block/mirror.c                 | 61 ++++++++++++++++++++++++++++++--
 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           | 63 ++++++++++++++++++++++++++++++++--
 qapi/job.json                  |  4 ++-
 9 files changed, 184 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH 1/9] 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 could be considered an existing change command.

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

Tried to go more general at first with a 'job-change' command, but I
couldn't figure out a way to avoid mutual inclusion between
block-core.json and job.json.

 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 d7b5c18f0a..ceb367dcb8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3416,6 +3416,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 659c3cb9de..b933d1c631 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -319,6 +319,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");
+    }
+}
+
 int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
 {
     IO_CODE();
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 058b0c824c..d8b80d9625 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.
+ * @opt: 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 f008446285..055bee5020 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 7f331eb8ea..714cabd49d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2964,6 +2964,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.0
+##
+{ 'union': 'BlockJobChangeOptions',
+  'base': { 'id': 'str', 'type': 'JobType' },
+  'discriminator': 'type',
+  'data': {} }
+
+##
+# @block-job-change:
+#
+# Change the block job's options.
+#
+# Since: 8.0
+##
+{ 'command': 'block-job-change',
+  'data': 'BlockJobChangeOptions', 'boxed': true }
+
 ##
 # @BlockdevDiscardOptions:
 #
diff --git a/qapi/job.json b/qapi/job.json
index d5f84e9615..7d9d328d7f 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -100,11 +100,13 @@
 #
 # @finalize: see @job-finalize
 #
+# @change: see @block-job-change (since 8.0)
+#
 # Since: 2.12
 ##
 { 'enum': 'JobVerb',
   'data': ['cancel', 'pause', 'resume', 'set-speed', 'complete', 'dismiss',
-           'finalize' ] }
+           'finalize', 'change' ] }
 
 ##
 # @JOB_STATUS_CHANGE:
-- 
2.30.2

[PATCH 2/9] 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>
---
 block/mirror.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index ab326b67c9..ca87492fcc 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1046,9 +1046,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.30.2

[PATCH 3/9] mirror: implement mirror_change method

[Fiona Ebner]

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

Once the job is in active mode, no new writes need to be registered in
the dirty bitmap, because they are synchronously written to the
target. But since the method is called from the monitor and IO might
be happening in an iothread at the same time, a drained section is
needed.

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 fullfilled. Currently, the
amount of information that can be used for those criteria is a bit
limited, so the plan is to extend quering of block jobs to return more
information relevant for mirror.

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

Sorry, I still haven't fully grasped the drained logic. Is my
rationale for the drained section correct? There also are some yield
points in block/io.c, for example when the driver implements
bdrv_aio_pwritev (file-win32 and null), and the bitmap is only updated
after that. Is that another reason it's required?

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

diff --git a/block/mirror.c b/block/mirror.c
index ca87492fcc..961aaa5cd6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1216,6 +1216,43 @@ 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;
+    BlockDriverState *bs = s->mirror_top_bs->backing->bs;
+    AioContext *ctx = bdrv_get_aio_context(bs);
+
+    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);
+
+    /*
+     * Once the job is in active mode, no new writes need to be registered in
+     * the dirty bitmap, because they are synchronously written to the target.
+     * Ensure the bitmap is up-to-date first, using a drained section.
+     */
+    s->in_drain = true;
+    aio_context_acquire(ctx);
+    bdrv_drained_begin(bs);
+
+    s->copy_mode = MIRROR_COPY_MODE_WRITE_BLOCKING;
+    bdrv_disable_dirty_bitmap(s->dirty_bitmap);
+
+    bdrv_drained_end(bs);
+    aio_context_release(ctx);
+    s->in_drain = false;
+}
+
 static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1230,6 +1267,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 714cabd49d..f9f464b25a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2964,6 +2964,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.0
+##
+{ 'struct': 'BlockJobChangeOptionsMirror',
+  'data': { 'copy-mode' : 'MirrorCopyMode' } }
+
 ##
 # @BlockJobChangeOptions:
 #
@@ -2978,7 +2989,7 @@
 { 'union': 'BlockJobChangeOptions',
   'base': { 'id': 'str', 'type': 'JobType' },
   'discriminator': 'type',
-  'data': {} }
+  'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }
 
 ##
 # @block-job-change:
-- 
2.30.2

[PATCH 4/9] 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>
---
 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 6aa5f1be0c..76e3254e93 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -840,7 +840,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",
@@ -852,7 +852,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 b933d1c631..9bd51bc6ae 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -361,7 +361,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 f9f464b25a..c1ac6de238 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1343,7 +1343,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.30.2

[PATCH 5/9] 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>
---
 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 c1ac6de238..adb43a4592 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1342,13 +1342,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.30.2

[PATCH 6/9] blockjob: query driver-specific info via a new 'query' driver method

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 blockjob.c                   | 4 ++++
 include/block/blockjob_int.h | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 9bd51bc6ae..5570890001 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -349,6 +349,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();
 
@@ -378,6 +379,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 055bee5020..e4543e9885 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.30.2

[PATCH 7/9] 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>
---
 block/mirror.c       | 10 ++++++++++
 qapi/block-core.json | 15 ++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 961aaa5cd6..02b5bd8bd2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1253,6 +1253,15 @@ static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
     s->in_drain = false;
 }
 
+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),
@@ -1268,6 +1277,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 adb43a4592..07e0f30492 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1300,6 +1300,19 @@
 { '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.0
+##
+{ 'struct': 'BlockJobInfoMirror',
+  'data': { 'actively-synced': 'bool' } }
+
 ##
 # @BlockJobInfo:
 #
@@ -1350,7 +1363,7 @@
            'auto-finalize': 'bool', 'auto-dismiss': 'bool',
            '*error': 'str' },
   'discriminator': 'type',
-  'data': {} }
+  'data': { 'mirror': 'BlockJobInfoMirror' } }
 
 ##
 # @query-block-jobs:
-- 
2.30.2

[PATCH 8/9] mirror: return the remaining dirty bytes upon query

[Fiona Ebner]

This can be used by management applications starting with a job in
background mode to determine when the switch to active mode should
happen.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 block/mirror.c       | 1 +
 qapi/block-core.json | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 02b5bd8bd2..ac83309b82 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1259,6 +1259,7 @@ static void mirror_query(BlockJob *job, BlockJobInfo *info)
 
     info->u.mirror = (BlockJobInfoMirror) {
         .actively_synced = s->actively_synced,
+        .remaining_dirty = bdrv_get_dirty_count(s->dirty_bitmap),
     };
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 07e0f30492..91594eace4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1308,10 +1308,12 @@
 # @actively-synced: Whether the source is actively synced to the target, i.e.
 #                   same data and new writes are done synchronously to both.
 #
+# @remaining-dirty: How much of the source is dirty relative to the target.
+#
 # Since 8.0
 ##
 { 'struct': 'BlockJobInfoMirror',
-  'data': { 'actively-synced': 'bool' } }
+  'data': { 'actively-synced': 'bool', 'remaining-dirty': 'int64' } }
 
 ##
 # @BlockJobInfo:
-- 
2.30.2

[PATCH 9/9] mirror: return the total number of bytes sent upon query

[Fiona Ebner]

This can be used by management applications starting with a job in
background mode to determine when the switch to active mode should
happen.

While the same information is already there, as the job-agnostic
@offset, it's not clear to users what the value means for mirror: it's
documentet to be relative to @len only and @len is documented to be
able to change in both directions while the job runs.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 block/mirror.c       | 6 ++++++
 qapi/block-core.json | 5 ++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index ac83309b82..e7b4905b70 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -86,6 +86,9 @@ typedef struct MirrorBlockJob {
     int64_t active_write_bytes_in_flight;
     bool prepared;
     bool in_drain;
+
+    /* Additional information intended for users (returned in mirror_query) */
+    int64_t data_sent;
 } MirrorBlockJob;
 
 typedef struct MirrorBDSOpaque {
@@ -215,6 +218,7 @@ static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
         }
         if (!s->initial_zeroing_ongoing) {
             job_progress_update(&s->common.job, op->bytes);
+            s->data_sent += op->bytes;
         }
     }
     qemu_iovec_destroy(&op->qiov);
@@ -1259,6 +1263,7 @@ static void mirror_query(BlockJob *job, BlockJobInfo *info)
 
     info->u.mirror = (BlockJobInfoMirror) {
         .actively_synced = s->actively_synced,
+        .data_sent       = s->data_sent,
         .remaining_dirty = bdrv_get_dirty_count(s->dirty_bitmap),
     };
 }
@@ -1384,6 +1389,7 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod method,
     job->active_write_bytes_in_flight -= bytes;
     if (ret >= 0) {
         job_progress_update(&job->common.job, bytes);
+        job->data_sent += bytes;
     } else {
         BlockErrorAction action;
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 91594eace4..656be5ce2e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1308,12 +1308,15 @@
 # @actively-synced: Whether the source is actively synced to the target, i.e.
 #                   same data and new writes are done synchronously to both.
 #
+# @data-sent: How much data was sent in total until now.
+#
 # @remaining-dirty: How much of the source is dirty relative to the target.
 #
 # Since 8.0
 ##
 { 'struct': 'BlockJobInfoMirror',
-  'data': { 'actively-synced': 'bool', 'remaining-dirty': 'int64' } }
+  'data': { 'actively-synced': 'bool', 'data-sent': 'int64',
+            'remaining-dirty': 'int64' } }
 
 ##
 # @BlockJobInfo:
-- 
2.30.2

Re: [PATCH 2/9] block/mirror: set actively_synced even after the job is ready

[Vladimir Sementsov-Ogievskiy]

On 24.02.23 17:48, Fiona Ebner wrote:
> 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>

-- 
Best regards,
Vladimir

Re: [PATCH 0/9] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

On 24.02.23 17:48, Fiona Ebner wrote:
> 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 acheive 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.

Hmm, what do you mean? As I understand job-* API is "new" and block-job-* is "old", so we'd better add job-change command

-- 
Best regards,
Vladimir

Re: [PATCH 0/9] mirror: allow switching from background to active mode

[Fiona Ebner]

Am 01.03.23 um 15:34 schrieb Vladimir Sementsov-Ogievskiy:
> On 24.02.23 17:48, Fiona Ebner wrote:
>> 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 acheive 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.
> 
> Hmm, what do you mean? As I understand job-* API is "new" and
> block-job-* is "old", so we'd better add job-change command
> 

My issue is that when I go for that with a JobChangeOptions union in
job.json and declare JobChangeOptionsMirror in block-core.json (it needs
to know about MirrorCopyMode and all other mirror-related declarations
are there so it's the natural place), then I need to include
block-core.json in job.json so that the JobChangeOptions union can
reference JobChangeOptionsMirror. But that is a mutual inclusion.

I guess it would be possible to move MirrorCopyMode to job.json and
declare JobChangeOptionsMirror in job.json, but that didn't feel right
and would mean moving more and more to job.json whenever extending
job-change requires it.

But maybe I'm missing something obvious? Haven't worked much with the
QAPI yet.

Re: [PATCH 3/9] mirror: implement mirror_change method

[Vladimir Sementsov-Ogievskiy]

On 24.02.23 17:48, Fiona Ebner wrote:
> which allows switching the @copy-mode from 'background' to
> 'write-blocking'.
> 
> Once the job is in active mode, no new writes need to be registered in
> the dirty bitmap, because they are synchronously written to the
> target. But since the method is called from the monitor and IO might
> be happening in an iothread at the same time, a drained section is
> needed.
> 
> 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 fullfilled. Currently, the
> amount of information that can be used for those criteria is a bit
> limited, so the plan is to extend quering of block jobs to return more
> information relevant for mirror.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Sorry, I still haven't fully grasped the drained logic. Is my
> rationale for the drained section correct? There also are some yield
> points in block/io.c, for example when the driver implements
> bdrv_aio_pwritev (file-win32 and null), and the bitmap is only updated
> after that. Is that another reason it's required?
> 

I think your logic is correct. But draining is quite expensive.

Could we avoid it? For example, just count separately in_flight_non_active writes (the writes that are started with copy_mode = BACKGROUND), and disable dirty bitmap when this counter becomes 0.

Or better idea: move the dirty bitmap to the filter, and make it always disabled. Then, we can set it by hand in bdrv_mirror_top_do_write() only when copy_to_target = false.


-- 
Best regards,
Vladimir

Re: [PATCH 4/9] qapi/block-core: use JobType for BlockJobInfo's type

[Vladimir Sementsov-Ogievskiy]

On 24.02.23 17:48, Fiona Ebner wrote:
> 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>

-- 
Best regards,
Vladimir

Re: [PATCH 5/9] qapi/block-core: turn BlockJobInfo into a union

[Vladimir Sementsov-Ogievskiy]

On 24.02.23 17:48, Fiona Ebner wrote:
> 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>

-- 
Best regards,
Vladimir

Re: [PATCH 8/9] mirror: return the remaining dirty bytes upon query

[Vladimir Sementsov-Ogievskiy]

On 24.02.23 17:48, Fiona Ebner wrote:
> This can be used by management applications starting with a job in
> background mode to determine when the switch to active mode should
> happen.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>   block/mirror.c       | 1 +
>   qapi/block-core.json | 4 +++-
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 02b5bd8bd2..ac83309b82 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1259,6 +1259,7 @@ static void mirror_query(BlockJob *job, BlockJobInfo *info)
>   
>       info->u.mirror = (BlockJobInfoMirror) {
>           .actively_synced = s->actively_synced,
> +        .remaining_dirty = bdrv_get_dirty_count(s->dirty_bitmap),

Doesn't it duplicate info->len - info->offset in meaning?

>       };
>   }
>   
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 07e0f30492..91594eace4 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1308,10 +1308,12 @@
>   # @actively-synced: Whether the source is actively synced to the target, i.e.
>   #                   same data and new writes are done synchronously to both.
>   #
> +# @remaining-dirty: How much of the source is dirty relative to the target.
> +#
>   # Since 8.0
>   ##
>   { 'struct': 'BlockJobInfoMirror',
> -  'data': { 'actively-synced': 'bool' } }
> +  'data': { 'actively-synced': 'bool', 'remaining-dirty': 'int64' } }
>   
>   ##
>   # @BlockJobInfo:

-- 
Best regards,
Vladimir

Re: [PATCH 8/9] mirror: return the remaining dirty bytes upon query

[Fiona Ebner]

Am 01.03.23 um 17:31 schrieb Vladimir Sementsov-Ogievskiy:
> On 24.02.23 17:48, Fiona Ebner wrote:
>> This can be used by management applications starting with a job in
>> background mode to determine when the switch to active mode should
>> happen.
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>   block/mirror.c       | 1 +
>>   qapi/block-core.json | 4 +++-
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 02b5bd8bd2..ac83309b82 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -1259,6 +1259,7 @@ static void mirror_query(BlockJob *job,
>> BlockJobInfo *info)
>>         info->u.mirror = (BlockJobInfoMirror) {
>>           .actively_synced = s->actively_synced,
>> +        .remaining_dirty = bdrv_get_dirty_count(s->dirty_bitmap),
> 
> Doesn't it duplicate info->len - info->offset in meaning?
> 

Essentially yes, apart from the in-flight bytes:
>         job_progress_set_remaining(&s->common.job,
>                                    s->bytes_in_flight + cnt +
>                                    s->active_write_bytes_in_flight);

Should I rather use that value (and rename it to e.g. data_remaining to
be more similar to data_sent from 9/9)?

But I'd argue the same way as in 9/9: it's not transparent to users what
offset and len mean for the mirror job, because their documentation is
for a generic block job. E.g. len is documented to be able to change in
both directions while the job runs.

Best Regards,
Fiona

Re: [PATCH 3/9] mirror: implement mirror_change method

[Fiona Ebner]

Am 01.03.23 um 16:18 schrieb Vladimir Sementsov-Ogievskiy:
> On 24.02.23 17:48, Fiona Ebner wrote:
>> which allows switching the @copy-mode from 'background' to
>> 'write-blocking'.
>>
>> Once the job is in active mode, no new writes need to be registered in
>> the dirty bitmap, because they are synchronously written to the
>> target. But since the method is called from the monitor and IO might
>> be happening in an iothread at the same time, a drained section is
>> needed.
>>
>> 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 fullfilled. Currently, the
>> amount of information that can be used for those criteria is a bit
>> limited, so the plan is to extend quering of block jobs to return more
>> information relevant for mirror.
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Sorry, I still haven't fully grasped the drained logic. Is my
>> rationale for the drained section correct? There also are some yield
>> points in block/io.c, for example when the driver implements
>> bdrv_aio_pwritev (file-win32 and null), and the bitmap is only updated
>> after that. Is that another reason it's required?
>>
> 
> I think your logic is correct. But draining is quite expensive.
> 
> Could we avoid it? For example, just count separately
> in_flight_non_active writes (the writes that are started with copy_mode
> = BACKGROUND), and disable dirty bitmap when this counter becomes 0.
> 
> Or better idea: move the dirty bitmap to the filter, and make it always
> disabled. Then, we can set it by hand in bdrv_mirror_top_do_write() only
> when copy_to_target = false.
> 
Sounds good to me! I'll try to go for the second approach in the next
version.

Best Regards,
Fiona

Re: [PATCH 8/9] mirror: return the remaining dirty bytes upon query

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 13:00, Fiona Ebner wrote:
> Am 01.03.23 um 17:31 schrieb Vladimir Sementsov-Ogievskiy:
>> On 24.02.23 17:48, Fiona Ebner wrote:
>>> This can be used by management applications starting with a job in
>>> background mode to determine when the switch to active mode should
>>> happen.
>>>
>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>>> ---
>>>    block/mirror.c       | 1 +
>>>    qapi/block-core.json | 4 +++-
>>>    2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 02b5bd8bd2..ac83309b82 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -1259,6 +1259,7 @@ static void mirror_query(BlockJob *job,
>>> BlockJobInfo *info)
>>>          info->u.mirror = (BlockJobInfoMirror) {
>>>            .actively_synced = s->actively_synced,
>>> +        .remaining_dirty = bdrv_get_dirty_count(s->dirty_bitmap),
>>
>> Doesn't it duplicate info->len - info->offset in meaning?
>>
> 
> Essentially yes, apart from the in-flight bytes:

Is it worth reporting to user?

>>          job_progress_set_remaining(&s->common.job,
>>                                     s->bytes_in_flight + cnt +
>>                                     s->active_write_bytes_in_flight);
> 
> Should I rather use that value (and rename it to e.g. data_remaining to
> be more similar to data_sent from 9/9)?
> 
> But I'd argue the same way as in 9/9: it's not transparent to users what
> offset and len mean for the mirror job, because their documentation is
> for a generic block job. E.g. len is documented to be able to change in
> both directions while the job runs.
> 

Still I'm not sure that we need new status values. I.e. if you need some new ones, you should explain the case and why existing information is not enough.

Especially when documentation of existing things is unclear, its better to start from improving it. And when we understand what len and offset means for mirror, it would probably be enough.


-- 
Best regards,
Vladimir

Re: [PATCH 8/9] mirror: return the remaining dirty bytes upon query

[Fiona Ebner]

Am 02.03.23 um 11:13 schrieb Vladimir Sementsov-Ogievskiy:
> On 02.03.23 13:00, Fiona Ebner wrote:
>> Am 01.03.23 um 17:31 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 24.02.23 17:48, Fiona Ebner wrote:
>>>> This can be used by management applications starting with a job in
>>>> background mode to determine when the switch to active mode should
>>>> happen.
>>>>
>>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>>>> ---
>>>>    block/mirror.c       | 1 +
>>>>    qapi/block-core.json | 4 +++-
>>>>    2 files changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/mirror.c b/block/mirror.c
>>>> index 02b5bd8bd2..ac83309b82 100644
>>>> --- a/block/mirror.c
>>>> +++ b/block/mirror.c
>>>> @@ -1259,6 +1259,7 @@ static void mirror_query(BlockJob *job,
>>>> BlockJobInfo *info)
>>>>          info->u.mirror = (BlockJobInfoMirror) {
>>>>            .actively_synced = s->actively_synced,
>>>> +        .remaining_dirty = bdrv_get_dirty_count(s->dirty_bitmap),
>>>
>>> Doesn't it duplicate info->len - info->offset in meaning?
>>>
>>
>> Essentially yes, apart from the in-flight bytes:
> 
> Is it worth reporting to user?
> 

You suggested that data_sent and remaining_dirty are important:
https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg03831.html

But I guess info->len - info->offset is just as good as part of a
heuristic to decide when the switch to active mode should happen.

For us, it doesn't really matter right now, because our users didn't
report issues with convergence, so our plan is to just switch to active
mode after the job is ready. We just need actively_synced to infer when
the switch is complete.

>>>          job_progress_set_remaining(&s->common.job,
>>>                                     s->bytes_in_flight + cnt +
>>>                                     s->active_write_bytes_in_flight);
>>
>> Should I rather use that value (and rename it to e.g. data_remaining to
>> be more similar to data_sent from 9/9)?
>>
>> But I'd argue the same way as in 9/9: it's not transparent to users what
>> offset and len mean for the mirror job, because their documentation is
>> for a generic block job. E.g. len is documented to be able to change in
>> both directions while the job runs.
>>
> 
> Still I'm not sure that we need new status values. I.e. if you need some
> new ones, you should explain the case and why existing information is
> not enough.
> 
> Especially when documentation of existing things is unclear, its better
> to start from improving it. And when we understand what len and offset
> means for mirror, it would probably be enough.
> 

Okay, makes sense! But I'm not sure how. Should I just add a paragraph
describing what the values mean for mirror in the description of @len
and @offset in @BlockJobInfo? Or where should this be documented?

Re: [PATCH 8/9] mirror: return the remaining dirty bytes upon query

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 15:34, Fiona Ebner wrote:
> Am 02.03.23 um 11:13 schrieb Vladimir Sementsov-Ogievskiy:
>> On 02.03.23 13:00, Fiona Ebner wrote:
>>> Am 01.03.23 um 17:31 schrieb Vladimir Sementsov-Ogievskiy:
>>>> On 24.02.23 17:48, Fiona Ebner wrote:
>>>>> This can be used by management applications starting with a job in
>>>>> background mode to determine when the switch to active mode should
>>>>> happen.
>>>>>
>>>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>>>>> ---
>>>>>     block/mirror.c       | 1 +
>>>>>     qapi/block-core.json | 4 +++-
>>>>>     2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/block/mirror.c b/block/mirror.c
>>>>> index 02b5bd8bd2..ac83309b82 100644
>>>>> --- a/block/mirror.c
>>>>> +++ b/block/mirror.c
>>>>> @@ -1259,6 +1259,7 @@ static void mirror_query(BlockJob *job,
>>>>> BlockJobInfo *info)
>>>>>           info->u.mirror = (BlockJobInfoMirror) {
>>>>>             .actively_synced = s->actively_synced,
>>>>> +        .remaining_dirty = bdrv_get_dirty_count(s->dirty_bitmap),
>>>>
>>>> Doesn't it duplicate info->len - info->offset in meaning?
>>>>
>>>
>>> Essentially yes, apart from the in-flight bytes:
>>
>> Is it worth reporting to user?
>>
> 
> You suggested that data_sent and remaining_dirty are important:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg03831.html
> 

Yes, sorry if it made you implement these fields :/ I was just thinking.


> But I guess info->len - info->offset is just as good as part of a
> heuristic to decide when the switch to active mode should happen.
> 
> For us, it doesn't really matter right now, because our users didn't
> report issues with convergence, so our plan is to just switch to active
> mode after the job is ready. We just need actively_synced to infer when
> the switch is complete.
> 

Hmm. But mirror can't become "actively_synced" until it not switched to active mode?

>>>>           job_progress_set_remaining(&s->common.job,
>>>>                                      s->bytes_in_flight + cnt +
>>>>                                      s->active_write_bytes_in_flight);
>>>
>>> Should I rather use that value (and rename it to e.g. data_remaining to
>>> be more similar to data_sent from 9/9)?
>>>
>>> But I'd argue the same way as in 9/9: it's not transparent to users what
>>> offset and len mean for the mirror job, because their documentation is
>>> for a generic block job. E.g. len is documented to be able to change in
>>> both directions while the job runs.
>>>
>>
>> Still I'm not sure that we need new status values. I.e. if you need some
>> new ones, you should explain the case and why existing information is
>> not enough.
>>
>> Especially when documentation of existing things is unclear, its better
>> to start from improving it. And when we understand what len and offset
>> means for mirror, it would probably be enough.
>>
> 
> Okay, makes sense! But I'm not sure how. Should I just add a paragraph
> describing what the values mean for mirror in the description of @len
> and @offset in @BlockJobInfo? Or where should this be documented?
> 

Hmm, or just in description of blockdev-mirror command. Still, I don't mean that you should do it.

If we want additional similar fields - then yes, we should describe why and what is different with existing fields, and good start for it - add details to documentation.
If we don't add them - current heuristical understanding that "remaining ~= len - offset" is enough.
So, I think better not add these fields now if you don't need them.

-- 
Best regards,
Vladimir

Re: [PATCH 8/9] mirror: return the remaining dirty bytes upon query

[Fiona Ebner]

Am 02.03.23 um 17:31 schrieb Vladimir Sementsov-Ogievskiy:
> On 02.03.23 15:34, Fiona Ebner wrote:
>> Am 02.03.23 um 11:13 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 02.03.23 13:00, Fiona Ebner wrote:
>>>> Am 01.03.23 um 17:31 schrieb Vladimir Sementsov-Ogievskiy:
>>>>> On 24.02.23 17:48, Fiona Ebner wrote:
>>>>>> This can be used by management applications starting with a job in
>>>>>> background mode to determine when the switch to active mode should
>>>>>> happen.
>>>>>>
>>>>>> Suggested-by: Vladimir Sementsov-Ogievskiy
>>>>>> <vsementsov@yandex-team.ru>
>>>>>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>>>>>> ---
>>>>>>     block/mirror.c       | 1 +
>>>>>>     qapi/block-core.json | 4 +++-
>>>>>>     2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/block/mirror.c b/block/mirror.c
>>>>>> index 02b5bd8bd2..ac83309b82 100644
>>>>>> --- a/block/mirror.c
>>>>>> +++ b/block/mirror.c
>>>>>> @@ -1259,6 +1259,7 @@ static void mirror_query(BlockJob *job,
>>>>>> BlockJobInfo *info)
>>>>>>           info->u.mirror = (BlockJobInfoMirror) {
>>>>>>             .actively_synced = s->actively_synced,
>>>>>> +        .remaining_dirty = bdrv_get_dirty_count(s->dirty_bitmap),
>>>>>
>>>>> Doesn't it duplicate info->len - info->offset in meaning?
>>>>>
>>>>
>>>> Essentially yes, apart from the in-flight bytes:
>>>
>>> Is it worth reporting to user?
>>>
>>
>> You suggested that data_sent and remaining_dirty are important:
>> https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg03831.html
>>
> 
> Yes, sorry if it made you implement these fields :/ I was just thinking.

It was no big deal :)

> 
>> But I guess info->len - info->offset is just as good as part of a
>> heuristic to decide when the switch to active mode should happen.
>>
>> For us, it doesn't really matter right now, because our users didn't
>> report issues with convergence, so our plan is to just switch to active
>> mode after the job is ready. We just need actively_synced to infer when
>> the switch is complete.
>>
> 
> Hmm. But mirror can't become "actively_synced" until it not switched to
> active mode?

Yes, but that's fine. We'd wait for the job to be ready, then switch to
active mode and once actively_synced is true, we'd start migration. Then
we don't need to worry about triggering the assertion after inactivating
block devices upon switchover.

> 
>>>>>           job_progress_set_remaining(&s->common.job,
>>>>>                                      s->bytes_in_flight + cnt +
>>>>>                                      s->active_write_bytes_in_flight);
>>>>
>>>> Should I rather use that value (and rename it to e.g. data_remaining to
>>>> be more similar to data_sent from 9/9)?
>>>>
>>>> But I'd argue the same way as in 9/9: it's not transparent to users
>>>> what
>>>> offset and len mean for the mirror job, because their documentation is
>>>> for a generic block job. E.g. len is documented to be able to change in
>>>> both directions while the job runs.
>>>>
>>>
>>> Still I'm not sure that we need new status values. I.e. if you need some
>>> new ones, you should explain the case and why existing information is
>>> not enough.
>>>
>>> Especially when documentation of existing things is unclear, its better
>>> to start from improving it. And when we understand what len and offset
>>> means for mirror, it would probably be enough.
>>>
>>
>> Okay, makes sense! But I'm not sure how. Should I just add a paragraph
>> describing what the values mean for mirror in the description of @len
>> and @offset in @BlockJobInfo? Or where should this be documented?
>>
> 
> Hmm, or just in description of blockdev-mirror command. Still, I don't
> mean that you should do it.
> 
> If we want additional similar fields - then yes, we should describe why
> and what is different with existing fields, and good start for it - add
> details to documentation.
> If we don't add them - current heuristical understanding that "remaining
> ~= len - offset" is enough.
> So, I think better not add these fields now if you don't need them.
> 

Okay, sure. I'll let the people that actually need additional fields add
them :)

Best Regards,
Fiona