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

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

[Fiona Ebner]

Changes in v3:
    * unlock the job mutex when calling the new block job driver
      'query' handler
    * squash patch adapting iotest output into patch that changes the
      output
    * turn accesses to copy_mode and actively_synced atomic
    * slightly rework error handling in mirror_change

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 v2:
https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02290.html

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
to avoid mutual inclusion of block-core.json and job.json, more
preparation would be required.


Fiona Ebner (9):
  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

 block/mirror.c                 | 111 ++++++++++++++++++++++-----------
 block/monitor/block-hmp-cmds.c |   4 +-
 blockdev.c                     |  14 +++++
 blockjob.c                     |  28 ++++++++-
 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, 212 insertions(+), 54 deletions(-)

-- 
2.39.2

[PATCH v3 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 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>
---

No changes in v3.

 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

Re: [PATCH v3 1/9] blockjob: introduce block-job-change QMP command

[Kevin Wolf]

Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
> 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>

> 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},
>  };

I'm not sure if I would have included JOB_STATUS_CREATED, i.e. before
the job has even started, but it's not necessarily a problem. The
implementation just need to be careful to work even in early stages. But
probably the early stages include some part of JOB_STATUS_RUNNING, too,
so they'd have to do this anyway.

Kevin

Re: [PATCH v3 1/9] blockjob: introduce block-job-change QMP command

[Fiona Ebner]

Am 18.10.23 um 17:52 schrieb Kevin Wolf:
> Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
>> 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>
> 
>> 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},
>>  };
> 
> I'm not sure if I would have included JOB_STATUS_CREATED, i.e. before
> the job has even started, but it's not necessarily a problem. The
> implementation just need to be careful to work even in early stages. But
> probably the early stages include some part of JOB_STATUS_RUNNING, too,
> so they'd have to do this anyway.
> 

As mentioned in the commit message, I copied the bits from
JOB_VERB_SET_SPEED, because that seemed to be very similar, as it can
also be seen as a change operation (and who knows, maybe it can be
merged into JOB_VERB_CHANGE at some point in the future). But I can
remove the bit if you want me to.

Best Regards,
Fiona

Re: [PATCH v3 1/9] blockjob: introduce block-job-change QMP command

[Kevin Wolf]

Am 23.10.2023 um 11:31 hat Fiona Ebner geschrieben:
> Am 18.10.23 um 17:52 schrieb Kevin Wolf:
> > Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
> >> 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>
> > 
> >> 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},
> >>  };
> > 
> > I'm not sure if I would have included JOB_STATUS_CREATED, i.e. before
> > the job has even started, but it's not necessarily a problem. The
> > implementation just need to be careful to work even in early stages. But
> > probably the early stages include some part of JOB_STATUS_RUNNING, too,
> > so they'd have to do this anyway.
> > 
> 
> As mentioned in the commit message, I copied the bits from
> JOB_VERB_SET_SPEED, because that seemed to be very similar, as it can
> also be seen as a change operation (and who knows, maybe it can be
> merged into JOB_VERB_CHANGE at some point in the future). But I can
> remove the bit if you want me to.

I think it's fine as it is, just something to keep in mind while
reviewing implementations.

What you could do is adding something to the comment for the change
callback in blockjob_int.h, like "Note that this can already be called
before the job coroutine is running".

Kevin

[PATCH v3 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

No changes in v3.

 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 v3 3/9] 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

No changes in v3.

 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 v3 4/9] 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

No changes in v3.

 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 v3 5/9] 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.

In presence of an iothread, the copy_mode member is now shared between
the iothread and the main thread, so turn accesses to it atomic.

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

Changes in v3:
    * turn accesses to copy_mode atomic and...
    * ...slightly adapt error handling in mirror_change as a
      consequence

 block/mirror.c       | 33 ++++++++++++++++++++++++++++++---
 qapi/block-core.json | 13 ++++++++++++-
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 8992c09172..889cce5414 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1075,7 +1075,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
                  */
                 job_transition_to_ready(&s->common.job);
             }
-            if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
+            if (qatomic_read(&s->copy_mode) != MIRROR_COPY_MODE_BACKGROUND) {
                 s->actively_synced = true;
             }
 
@@ -1246,6 +1246,32 @@ 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;
+    MirrorCopyMode current;
+
+    if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
+        return;
+    }
+
+    if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
+        error_setg(errp, "Change to copy mode '%s' is not implemented",
+                   MirrorCopyMode_str(change_opts->copy_mode));
+        return;
+    }
+
+    current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
+                              change_opts->copy_mode);
+    if (current != MIRROR_COPY_MODE_BACKGROUND) {
+        error_setg(errp, "Expected current copy mode '%s', got '%s'",
+                   MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
+                   MirrorCopyMode_str(current));
+    }
+}
+
 static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1260,6 +1286,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 = {
@@ -1467,7 +1494,7 @@ 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;
+        qatomic_read(&s->job->copy_mode) == MIRROR_COPY_MODE_WRITE_BLOCKING;
 }
 
 static int coroutine_fn GRAPH_RDLOCK
@@ -1812,7 +1839,7 @@ static BlockJob *mirror_start_job(
     s->is_none_mode = is_none_mode;
     s->backing_mode = backing_mode;
     s->zero_target = zero_target;
-    s->copy_mode = copy_mode;
+    qatomic_set(&s->copy_mode, copy_mode);
     s->base = base;
     s->base_overlay = bdrv_find_overlay(bs, base);
     s->granularity = granularity;
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

Re: [PATCH v3 5/9] mirror: implement mirror_change method

[Markus Armbruster]

Fiona Ebner <f.ebner@proxmox.com> writes:

> 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.
>
> In presence of an iothread, the copy_mode member is now shared between
> the iothread and the main thread, so turn accesses to it atomic.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

[...]

> 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

Typo: Currently

Also, two spaces between sentences for consistency, please.

> +#     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:

Re: [PATCH v3 5/9] mirror: implement mirror_change method

[Kevin Wolf]

Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
> 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.
> 
> In presence of an iothread, the copy_mode member is now shared between
> the iothread and the main thread, so turn accesses to it atomic.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v3:
>     * turn accesses to copy_mode atomic and...
>     * ...slightly adapt error handling in mirror_change as a
>       consequence

It would be good to have a comment at the field declaration that it's
meant to be accessed with atomics.

As we don't have further synchonisation, is the idea that during the
switchover it basically doesn't matter if we read the old or the new
value?

After reading the whole patch, it seems that the field is only ever
written under the BQL, while iothreads only read it, and only once per
request (after the previous patch). This is why no further
synchonisation is needed. If other threads could write it, too,
mirror_change() would probably have to be more careful. As the code
depends on this, adding that to the comment would be useful, too.

>  block/mirror.c       | 33 ++++++++++++++++++++++++++++++---
>  qapi/block-core.json | 13 ++++++++++++-
>  2 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 8992c09172..889cce5414 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1075,7 +1075,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>                   */
>                  job_transition_to_ready(&s->common.job);
>              }
> -            if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
> +            if (qatomic_read(&s->copy_mode) != MIRROR_COPY_MODE_BACKGROUND) {
>                  s->actively_synced = true;
>              }

What resets s->actively_synced when we switch away from active mode?

>  
> @@ -1246,6 +1246,32 @@ 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;
> +    MirrorCopyMode current;

This is GLOBAL_STATE_CODE(), right? Let's be explicit about it.

> +
> +    if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
> +        return;
> +    }
> +
> +    if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
> +        error_setg(errp, "Change to copy mode '%s' is not implemented",
> +                   MirrorCopyMode_str(change_opts->copy_mode));
> +        return;
> +    }

Ah, ok, we don't even allow the switch I was wondering about above. What
would be needed, apart from removing this check, to make it work?

> +    current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
> +                              change_opts->copy_mode);
> +    if (current != MIRROR_COPY_MODE_BACKGROUND) {
> +        error_setg(errp, "Expected current copy mode '%s', got '%s'",
> +                   MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
> +                   MirrorCopyMode_str(current));
> +    }

The error path is strange. We return an error, but the new mode is still
set. On the other hand, this is probably also the old mode unless
someone added a new value to the enum, so it didn't actually change. And
because this function is the only place that changes copy_mode and we're
holding the BQL, the case can't even happen and this could be an
assertion.

> +}
> +
>  static const BlockJobDriver mirror_job_driver = {
>      .job_driver = {
>          .instance_size          = sizeof(MirrorBlockJob),
> @@ -1260,6 +1286,7 @@ static const BlockJobDriver mirror_job_driver = {
>          .cancel                 = mirror_cancel,
>      },
>      .drained_poll           = mirror_drained_poll,
> +    .change                 = mirror_change,
>  };

Kevin

Re: [PATCH v3 5/9] mirror: implement mirror_change method

[Fiona Ebner]

Am 18.10.23 um 18:59 schrieb Kevin Wolf:
> Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
>> 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.
>>
>> In presence of an iothread, the copy_mode member is now shared between
>> the iothread and the main thread, so turn accesses to it atomic.
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Changes in v3:
>>     * turn accesses to copy_mode atomic and...
>>     * ...slightly adapt error handling in mirror_change as a
>>       consequence
> 
> It would be good to have a comment at the field declaration that it's
> meant to be accessed with atomics.
> 
> As we don't have further synchonisation, is the idea that during the
> switchover it basically doesn't matter if we read the old or the new
> value?
> 
> After reading the whole patch, it seems that the field is only ever
> written under the BQL, while iothreads only read it, and only once per
> request (after the previous patch). This is why no further
> synchonisation is needed. If other threads could write it, too,
> mirror_change() would probably have to be more careful. As the code
> depends on this, adding that to the comment would be useful, too.
> 

Will do in v4.

>>  block/mirror.c       | 33 ++++++++++++++++++++++++++++++---
>>  qapi/block-core.json | 13 ++++++++++++-
>>  2 files changed, 42 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 8992c09172..889cce5414 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -1075,7 +1075,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>>                   */
>>                  job_transition_to_ready(&s->common.job);
>>              }
>> -            if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
>> +            if (qatomic_read(&s->copy_mode) != MIRROR_COPY_MODE_BACKGROUND) {
>>                  s->actively_synced = true;
>>              }
> 
> What resets s->actively_synced when we switch away from active mode?
> 
>>  
>> @@ -1246,6 +1246,32 @@ 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;
>> +    MirrorCopyMode current;
> 
> This is GLOBAL_STATE_CODE(), right? Let's be explicit about it.
> 

Maybe it wouldn't need to be if we also set actively_synced to false in
bdrv_mirror_top_do_write() if/when setting the bitmap. Thinking about
it, that change shouldn't hurt in any case. But sure, I'll add the
GLOBAL_STATE_CODE annotation here. If ever required not to be
GLOBAL_STATE_CODE code, it can still be adapted later.

>> +
>> +    if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
>> +        return;
>> +    }
>> +
>> +    if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
>> +        error_setg(errp, "Change to copy mode '%s' is not implemented",
>> +                   MirrorCopyMode_str(change_opts->copy_mode));
>> +        return;
>> +    }
> 
> Ah, ok, we don't even allow the switch I was wondering about above. What
> would be needed, apart from removing this check, to make it work?
> 

Of course, setting actively_synced to false, as you pointed out above.
But I think it would also require more synchronization, because I think
otherwise the iothread could still read the old value of copy_mode (as
MIRROR_COPY_MODE_WRITE_BLOCKING) right afterwards and might set
actively_synced to true again. Do you want me to think it through in
detail and allow the change in the other direction too? I guess that
would also require using the job mutex instead of atomics. Or should we
wait until somebody actually requires that?

>> +    current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
>> +                              change_opts->copy_mode);
>> +    if (current != MIRROR_COPY_MODE_BACKGROUND) {
>> +        error_setg(errp, "Expected current copy mode '%s', got '%s'",
>> +                   MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
>> +                   MirrorCopyMode_str(current));
>> +    }
> 
> The error path is strange. We return an error, but the new mode is still
> set. On the other hand, this is probably also the old mode unless
> someone added a new value to the enum, so it didn't actually change. And
> because this function is the only place that changes copy_mode and we're
> holding the BQL, the case can't even happen and this could be an
> assertion.
> 

AFAIU and testing seem to confirm this, the new mode is only set when
the current mode is MIRROR_COPY_MODE_BACKGROUND. The error is only set
when the current mode is not MIRROR_COPY_MODE_BACKGROUND and thus when
the mode wasn't changed.

Adding a new copy mode shouldn't cause issues either? It's just not
going to be supported to change away from that mode (or to that mode,
because of the change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING
check above) without adapting the code first.

Of course, if we want to allow switching from active to background mode,
the function needs to be adapted too.

I wanted to make it more future-proof for the case where it might not be
the only place changing the value and based it on what Vladimir
suggested in the review of v2:
https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg03552.html

Best Regards,
Fiona

Re: [PATCH v3 5/9] mirror: implement mirror_change method

[Kevin Wolf]

Am 23.10.2023 um 13:37 hat Fiona Ebner geschrieben:
> Am 18.10.23 um 18:59 schrieb Kevin Wolf:
> > Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
> >> 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.
> >>
> >> In presence of an iothread, the copy_mode member is now shared between
> >> the iothread and the main thread, so turn accesses to it atomic.
> >>
> >> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> >> ---
> >>
> >> Changes in v3:
> >>     * turn accesses to copy_mode atomic and...
> >>     * ...slightly adapt error handling in mirror_change as a
> >>       consequence
> > 
> > It would be good to have a comment at the field declaration that it's
> > meant to be accessed with atomics.
> > 
> > As we don't have further synchonisation, is the idea that during the
> > switchover it basically doesn't matter if we read the old or the new
> > value?
> > 
> > After reading the whole patch, it seems that the field is only ever
> > written under the BQL, while iothreads only read it, and only once per
> > request (after the previous patch). This is why no further
> > synchonisation is needed. If other threads could write it, too,
> > mirror_change() would probably have to be more careful. As the code
> > depends on this, adding that to the comment would be useful, too.
> > 
> 
> Will do in v4.
> 
> >>  block/mirror.c       | 33 ++++++++++++++++++++++++++++++---
> >>  qapi/block-core.json | 13 ++++++++++++-
> >>  2 files changed, 42 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/block/mirror.c b/block/mirror.c
> >> index 8992c09172..889cce5414 100644
> >> --- a/block/mirror.c
> >> +++ b/block/mirror.c
> >> @@ -1075,7 +1075,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
> >>                   */
> >>                  job_transition_to_ready(&s->common.job);
> >>              }
> >> -            if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
> >> +            if (qatomic_read(&s->copy_mode) != MIRROR_COPY_MODE_BACKGROUND) {
> >>                  s->actively_synced = true;
> >>              }
> > 
> > What resets s->actively_synced when we switch away from active mode?
> > 
> >>  
> >> @@ -1246,6 +1246,32 @@ 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;
> >> +    MirrorCopyMode current;
> > 
> > This is GLOBAL_STATE_CODE(), right? Let's be explicit about it.
> > 
> 
> Maybe it wouldn't need to be if we also set actively_synced to false in
> bdrv_mirror_top_do_write() if/when setting the bitmap. Thinking about
> it, that change shouldn't hurt in any case. But sure, I'll add the
> GLOBAL_STATE_CODE annotation here. If ever required not to be
> GLOBAL_STATE_CODE code, it can still be adapted later.

Maybe we'll want to change it later so that we have some kind of
automatic switch in the job coroutine, then it might not be
GLOBAL_STATE_CODE() any more. For now, that's not a concern and it makes
reasoning about the rest of the function easier.

> >> +
> >> +    if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
> >> +        return;
> >> +    }
> >> +
> >> +    if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
> >> +        error_setg(errp, "Change to copy mode '%s' is not implemented",
> >> +                   MirrorCopyMode_str(change_opts->copy_mode));
> >> +        return;
> >> +    }
> > 
> > Ah, ok, we don't even allow the switch I was wondering about above. What
> > would be needed, apart from removing this check, to make it work?
> > 
> 
> Of course, setting actively_synced to false, as you pointed out above.
> But I think it would also require more synchronization, because I think
> otherwise the iothread could still read the old value of copy_mode (as
> MIRROR_COPY_MODE_WRITE_BLOCKING) right afterwards and might set
> actively_synced to true again. Do you want me to think it through in
> detail and allow the change in the other direction too? I guess that
> would also require using the job mutex instead of atomics. Or should we
> wait until somebody actually requires that?

I won't stop you if you want to implement it, it's an obvious feature to
add and I like symmetry. But if nobody has a use for it, there is no
reason to make your life harder. I was mostly just curious if you're
already aware of any challenges in the other direction that made you
tackle a more limited problem for now.

> >> +    current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
> >> +                              change_opts->copy_mode);
> >> +    if (current != MIRROR_COPY_MODE_BACKGROUND) {
> >> +        error_setg(errp, "Expected current copy mode '%s', got '%s'",
> >> +                   MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
> >> +                   MirrorCopyMode_str(current));
> >> +    }
> > 
> > The error path is strange. We return an error, but the new mode is still
> > set. On the other hand, this is probably also the old mode unless
> > someone added a new value to the enum, so it didn't actually change. And
> > because this function is the only place that changes copy_mode and we're
> > holding the BQL, the case can't even happen and this could be an
> > assertion.
> > 
> 
> AFAIU and testing seem to confirm this, the new mode is only set when
> the current mode is MIRROR_COPY_MODE_BACKGROUND. The error is only set
> when the current mode is not MIRROR_COPY_MODE_BACKGROUND and thus when
> the mode wasn't changed.

Yes, the new mode is only set when it was MIRROR_COPY_MODE_BACKGROUND,
that's the meaning of cmpxchg.

And now that I checked the return value of qatomic_cmpxchg(), it's not
the actual value, but it returns the second parameter (the expected old
value). As this is a constant in our call, that's what we'll always get
back. So the whole check is pointless, even as an assertion. It's
trivially true, and I expect it's even obvious enough for the compiler
that it might just optimise it away.

Just qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
change_opts->copy_mode); without using the (constant) result should be
enough.

> Adding a new copy mode shouldn't cause issues either? It's just not
> going to be supported to change away from that mode (or to that mode,
> because of the change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING
> check above) without adapting the code first.

The checks above won't prevent NEW_MODE -> WRITE_BLOCKING. Of course,
the cmpxchg() won't actually do anything as long as we still have
BACKGROUND there as the expected old value. So in this case, QMP would
probably return success, but we would stay in NEW_MODE.

That's different from what I thought (I didn't really realise that we
have a cmpxchg here and not just a xchg), but also not entirely right.

Of course, all of this is hypothetical. I'm not aware of any desire to
add a new copy mode.

> Of course, if we want to allow switching from active to background mode,
> the function needs to be adapted too.
> 
> I wanted to make it more future-proof for the case where it might not be
> the only place changing the value and based it on what Vladimir
> suggested in the review of v2:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg03552.html

As long as all of these places are GLOBAL_STATE_CODE(), we should be
fine. If we get iothread code that changes it, too, I think your code
becomes racy because the value could be changed by the iothread between
the first check if we already have the new value and the actual change.

Kevin

Re: [PATCH v3 5/9] mirror: implement mirror_change method

[Fiona Ebner]

Am 23.10.23 um 14:59 schrieb Kevin Wolf:
> Am 23.10.2023 um 13:37 hat Fiona Ebner geschrieben: 
>>>> +    current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
>>>> +                              change_opts->copy_mode);
>>>> +    if (current != MIRROR_COPY_MODE_BACKGROUND) {
>>>> +        error_setg(errp, "Expected current copy mode '%s', got '%s'",
>>>> +                   MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
>>>> +                   MirrorCopyMode_str(current));
>>>> +    }
>>>
>>> The error path is strange. We return an error, but the new mode is still
>>> set. On the other hand, this is probably also the old mode unless
>>> someone added a new value to the enum, so it didn't actually change. And
>>> because this function is the only place that changes copy_mode and we're
>>> holding the BQL, the case can't even happen and this could be an
>>> assertion.
>>>
>>
>> AFAIU and testing seem to confirm this, the new mode is only set when
>> the current mode is MIRROR_COPY_MODE_BACKGROUND. The error is only set
>> when the current mode is not MIRROR_COPY_MODE_BACKGROUND and thus when
>> the mode wasn't changed.
> 
> Yes, the new mode is only set when it was MIRROR_COPY_MODE_BACKGROUND,
> that's the meaning of cmpxchg.
> 
> And now that I checked the return value of qatomic_cmpxchg(), it's not
> the actual value, but it returns the second parameter (the expected old
> value). As this is a constant in our call, that's what we'll always get
> back. So the whole check is pointless, even as an assertion. It's
> trivially true, and I expect it's even obvious enough for the compiler
> that it might just optimise it away.
> 

From testing, I can see that it returns the current value, not the
second parameter. I.e. if I am in MIRROR_COPY_MODE_WRITE_BLOCKING, it
will return MIRROR_COPY_MODE_WRITE_BLOCKING. (Of course, I have to
comment out the other check to reach the cmpxchg call while in that mode).

> Just qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
> change_opts->copy_mode); without using the (constant) result should be
> enough.
> 
>> Adding a new copy mode shouldn't cause issues either? It's just not
>> going to be supported to change away from that mode (or to that mode,
>> because of the change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING
>> check above) without adapting the code first.
> 
> The checks above won't prevent NEW_MODE -> WRITE_BLOCKING. Of course,
> the cmpxchg() won't actually do anything as long as we still have
> BACKGROUND there as the expected old value. So in this case, QMP would
> probably return success, but we would stay in NEW_MODE.
> 

No, that's the whole point of the check. It would fail with the error,
saying that it expected the current mode to be background and not the
new mode.

> That's different from what I thought (I didn't really realise that we
> have a cmpxchg here and not just a xchg), but also not entirely right.
> 
> Of course, all of this is hypothetical. I'm not aware of any desire to
> add a new copy mode.
> 
>> Of course, if we want to allow switching from active to background mode,
>> the function needs to be adapted too.
>>
>> I wanted to make it more future-proof for the case where it might not be
>> the only place changing the value and based it on what Vladimir
>> suggested in the review of v2:
>> https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg03552.html
> 
> As long as all of these places are GLOBAL_STATE_CODE(), we should be
> fine. If we get iothread code that changes it, too, I think your code
> becomes racy because the value could be changed by the iothread between
> the first check if we already have the new value and the actual change.
> 

Right, but I think the only issue would be if the mode changes from
MIRROR_COPY_MODE_BACKGROUND to MIRROR_COPY_MODE_WRITE_BLOCKING between
the checks, because then the QMP call would fail with the error that the
mode was not the expected MIRROR_COPY_MODE_BACKGROUND. But arguably,
that is still correct. If we are already in the requested mode at the
time of the first check, we're fine.

Still, I'll add the GLOBAL_STATE_CODE() and a comment for the future :)

Best Regards,
Fiona

Re: [PATCH v3 5/9] mirror: implement mirror_change method

[Kevin Wolf]

Am 23.10.2023 um 16:14 hat Fiona Ebner geschrieben:
> Am 23.10.23 um 14:59 schrieb Kevin Wolf:
> > Am 23.10.2023 um 13:37 hat Fiona Ebner geschrieben: 
> >>>> +    current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
> >>>> +                              change_opts->copy_mode);
> >>>> +    if (current != MIRROR_COPY_MODE_BACKGROUND) {
> >>>> +        error_setg(errp, "Expected current copy mode '%s', got '%s'",
> >>>> +                   MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
> >>>> +                   MirrorCopyMode_str(current));
> >>>> +    }
> >>>
> >>> The error path is strange. We return an error, but the new mode is still
> >>> set. On the other hand, this is probably also the old mode unless
> >>> someone added a new value to the enum, so it didn't actually change. And
> >>> because this function is the only place that changes copy_mode and we're
> >>> holding the BQL, the case can't even happen and this could be an
> >>> assertion.
> >>>
> >>
> >> AFAIU and testing seem to confirm this, the new mode is only set when
> >> the current mode is MIRROR_COPY_MODE_BACKGROUND. The error is only set
> >> when the current mode is not MIRROR_COPY_MODE_BACKGROUND and thus when
> >> the mode wasn't changed.
> > 
> > Yes, the new mode is only set when it was MIRROR_COPY_MODE_BACKGROUND,
> > that's the meaning of cmpxchg.
> > 
> > And now that I checked the return value of qatomic_cmpxchg(), it's not
> > the actual value, but it returns the second parameter (the expected old
> > value). As this is a constant in our call, that's what we'll always get
> > back. So the whole check is pointless, even as an assertion. It's
> > trivially true, and I expect it's even obvious enough for the compiler
> > that it might just optimise it away.
> > 
> 
> From testing, I can see that it returns the current value, not the
> second parameter. I.e. if I am in MIRROR_COPY_MODE_WRITE_BLOCKING, it
> will return MIRROR_COPY_MODE_WRITE_BLOCKING. (Of course, I have to
> comment out the other check to reach the cmpxchg call while in that mode).

You're right, I misread. Sorry for the noise.

> > Just qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
> > change_opts->copy_mode); without using the (constant) result should be
> > enough.
> > 
> >> Adding a new copy mode shouldn't cause issues either? It's just not
> >> going to be supported to change away from that mode (or to that mode,
> >> because of the change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING
> >> check above) without adapting the code first.
> > 
> > The checks above won't prevent NEW_MODE -> WRITE_BLOCKING. Of course,
> > the cmpxchg() won't actually do anything as long as we still have
> > BACKGROUND there as the expected old value. So in this case, QMP would
> > probably return success, but we would stay in NEW_MODE.
> > 
> 
> No, that's the whole point of the check. It would fail with the error,
> saying that it expected the current mode to be background and not the
> new mode.

Yes, this makes sense now.

> > That's different from what I thought (I didn't really realise that we
> > have a cmpxchg here and not just a xchg), but also not entirely right.
> > 
> > Of course, all of this is hypothetical. I'm not aware of any desire to
> > add a new copy mode.
> > 
> >> Of course, if we want to allow switching from active to background mode,
> >> the function needs to be adapted too.
> >>
> >> I wanted to make it more future-proof for the case where it might not be
> >> the only place changing the value and based it on what Vladimir
> >> suggested in the review of v2:
> >> https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg03552.html
> > 
> > As long as all of these places are GLOBAL_STATE_CODE(), we should be
> > fine. If we get iothread code that changes it, too, I think your code
> > becomes racy because the value could be changed by the iothread between
> > the first check if we already have the new value and the actual change.
> > 
> 
> Right, but I think the only issue would be if the mode changes from
> MIRROR_COPY_MODE_BACKGROUND to MIRROR_COPY_MODE_WRITE_BLOCKING between
> the checks, because then the QMP call would fail with the error that the
> mode was not the expected MIRROR_COPY_MODE_BACKGROUND. But arguably,
> that is still correct. If we are already in the requested mode at the
> time of the first check, we're fine.
> 
> Still, I'll add the GLOBAL_STATE_CODE() and a comment for the future :)

Thanks, sounds good.

Kevin

[PATCH v3 6/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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

No changes in v3.

 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

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

[Markus Armbruster]

Fiona Ebner <f.ebner@proxmox.com> writes:

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

Replacing str by enum makes sense whether we need enum for a union or
not.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v3 7/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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

No changes in v3.

 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 v3 8/9] blockjob: query driver-specific info via a new 'query' driver method

[Fiona Ebner]

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

Changes in v3:
    * Unlock job mutex while calling the driver's handler for query.

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

diff --git a/blockjob.c b/blockjob.c
index f8cf6e58e2..9c1cf2ba78 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,11 @@ 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) {
+        job_unlock();
+        drv->query(job, info);
+        job_lock();
+    }
     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 v3 9/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.

In presence of an iothread, the actively_synced member is now shared
between the iothread and the main thread, so turn accesses to it
atomic.

Requires to adapt the output for iotest 109.

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

Changes in v3:
    * squash with patch adapting iotest output
    * turn accesses to actively_synced atomic

 block/mirror.c             | 21 ++++++++++++++++-----
 qapi/block-core.json       | 16 +++++++++++++++-
 tests/qemu-iotests/109.out | 24 ++++++++++++------------
 3 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 889cce5414..b305c03918 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -122,7 +122,7 @@ typedef enum MirrorMethod {
 static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
                                             int error)
 {
-    s->actively_synced = false;
+    qatomic_set(&s->actively_synced, false);
     if (read) {
         return block_job_error_action(&s->common, s->on_source_error,
                                       true, error);
@@ -962,7 +962,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
     if (s->bdev_length == 0) {
         /* Transition to the READY state and wait for complete. */
         job_transition_to_ready(&s->common.job);
-        s->actively_synced = true;
+        qatomic_set(&s->actively_synced, true);
         while (!job_cancel_requested(&s->common.job) && !s->should_complete) {
             job_yield(&s->common.job);
         }
@@ -1076,7 +1076,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
                 job_transition_to_ready(&s->common.job);
             }
             if (qatomic_read(&s->copy_mode) != MIRROR_COPY_MODE_BACKGROUND) {
-                s->actively_synced = true;
+                qatomic_set(&s->actively_synced, true);
             }
 
             should_complete = s->should_complete ||
@@ -1272,6 +1272,15 @@ static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
     }
 }
 
+static void mirror_query(BlockJob *job, BlockJobInfo *info)
+{
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+
+    info->u.mirror = (BlockJobInfoMirror) {
+        .actively_synced = qatomic_read(&s->actively_synced),
+    };
+}
+
 static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1287,6 +1296,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 = {
@@ -1405,7 +1415,7 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod method,
         bitmap_end = QEMU_ALIGN_UP(offset + bytes, job->granularity);
         bdrv_set_dirty_bitmap(job->dirty_bitmap, bitmap_offset,
                               bitmap_end - bitmap_offset);
-        job->actively_synced = false;
+        qatomic_set(&job->actively_synced, false);
 
         action = mirror_error_action(job, false, -ret);
         if (action == BLOCK_ERROR_ACTION_REPORT) {
@@ -1464,7 +1474,8 @@ static void coroutine_fn GRAPH_RDLOCK active_write_settle(MirrorOp *op)
     uint64_t end_chunk = DIV_ROUND_UP(op->offset + op->bytes,
                                       op->s->granularity);
 
-    if (!--op->s->in_active_write_counter && op->s->actively_synced) {
+    if (!--op->s->in_active_write_counter &&
+        qatomic_read(&op->s->actively_synced)) {
         BdrvChild *source = op->s->mirror_top_bs->backing;
 
         if (QLIST_FIRST(&source->bs->parents) == source &&
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:
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 v3 0/9] mirror: allow switching from background to active mode

[Markus Armbruster]

Fiona Ebner <f.ebner@proxmox.com> writes:

> Changes in v3:
>     * unlock the job mutex when calling the new block job driver
>       'query' handler
>     * squash patch adapting iotest output into patch that changes the
>       output
>     * turn accesses to copy_mode and actively_synced atomic
>     * slightly rework error handling in mirror_change
>
> 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 v2:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02290.html
>
> 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
> to avoid mutual inclusion of block-core.json and job.json, more
> preparation would be required.

Can you elaborate a bit?  A more generic command is preferable, and we
need to understand what it would take.

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

[Fiona Ebner]

Am 18.10.23 um 11:41 schrieb Markus Armbruster:
> Fiona Ebner <f.ebner@proxmox.com> writes:
>>
>> Initially, I tried to go for a more general 'job-change' command, but
>> to avoid mutual inclusion of block-core.json and job.json, more
>> preparation would be required.
> 
> Can you elaborate a bit?  A more generic command is preferable, and we
> need to understand what it would take.
> 

Yes, I did so during the discussion of v2:

https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02993.html

Best Regards,
Fiona

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

[Markus Armbruster]

Fiona Ebner <f.ebner@proxmox.com> writes:

> Am 18.10.23 um 11:41 schrieb Markus Armbruster:
>> Fiona Ebner <f.ebner@proxmox.com> writes:
>>>
>>> Initially, I tried to go for a more general 'job-change' command, but
>>> to avoid mutual inclusion of block-core.json and job.json, more
>>> preparation would be required.
>> 
>> Can you elaborate a bit?  A more generic command is preferable, and we
>> need to understand what it would take.
>> 
>
> Yes, I did so during the discussion of v2:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02993.html

Replied there.  Sorry for the delay!

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

[Kevin Wolf]

Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
> Changes in v3:
>     * unlock the job mutex when calling the new block job driver
>       'query' handler
>     * squash patch adapting iotest output into patch that changes the
>       output
>     * turn accesses to copy_mode and actively_synced atomic
>     * slightly rework error handling in mirror_change
> 
> 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 v2:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02290.html
> 
> 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.

Most of this series looks good to me. Apart from the comments I made in
the individual patches, I would like to see iotests coverage of changing
the mirroring mode. At the least to show that the query result changes,
but ideally also that requests really block after switchting to active.
I think with a throttled target node and immediately reading the target
when the write request completes we should be able to check this.

Kevin

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

[Fiona Ebner]

Am 19.10.23 um 15:36 schrieb Kevin Wolf:
> Most of this series looks good to me. Apart from the comments I made in
> the individual patches, I would like to see iotests coverage of changing
> the mirroring mode. At the least to show that the query result changes,
> but ideally also that requests really block after switchting to active.
> I think with a throttled target node and immediately reading the target
> when the write request completes we should be able to check this.
> 

I'll try to work something out for v4.

Best Regards,
Fiona

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

[Fiona Ebner]

Am 23.10.23 um 13:39 schrieb Fiona Ebner:
> Am 19.10.23 um 15:36 schrieb Kevin Wolf:
>> Most of this series looks good to me. Apart from the comments I made in
>> the individual patches, I would like to see iotests coverage of changing
>> the mirroring mode. At the least to show that the query result changes,
>> but ideally also that requests really block after switchting to active.
>> I think with a throttled target node and immediately reading the target
>> when the write request completes we should be able to check this.
>>
> 
> I'll try to work something out for v4.
> 

I'm having a bit of a hard time unfortunately. I created a throttle
group with

>                 'iops-total': iops,
>                 'iops-total-max': iops

and used that for the 'throttle' driver for the target. I then tried
issuing requests via qemu_io

>             self.vm.hmp_qemu_io('mirror-top',
>                                 f'aio_write -P 1 {req_size * i} {req_size * (i + 1)}')

but when I create more requests than the 'iops' limit (to ensure that
not all are completed immediately), it will get stuck when draining the
temporary BlockBackend used by qemu_io [0]. Note this is while still in
background mode.

I also wanted to have request going on while the copy mode is changed
and for that, I was able to work around the issue by creating an NBD
export of the source like in iotest 151 and issuing the requests to the
NBD socket instead.

But after I switch to active mode and when I issue more than the 'iops'
limit requests to the NBD export then, it also seems to get stuck,
visible during shutdown when it tries to close the export[1].

Happy about any hints/suggestions :)

Best Regards,
Fiona

[0]:

> Thread 4 (Thread 0x7f0101f876c0 (LWP 46638) "qemu-system-x86"):
> #0  0x00007f010decbc02 in __GI___sigtimedwait (set=set@entry=0x7f0101f822e0, info=info@entry=0x7f0101f82210, timeout=timeout@entry=0x0) at ../sysdeps/unix/sysv/linux/sigtimedwait.c:31
> #1  0x00007f010decb31c in __GI___sigwait (set=0x7f0101f822e0, sig=0x7f0101f822d0) at ../sysdeps/unix/sysv/linux/sigwait.c:28
> #2  0x00005612182e2c96 in dummy_cpu_thread_fn (arg=0x561219892a90) at ../accel/dummy-cpus.c:50
> #3  0x0000561218729f03 in qemu_thread_start (args=0x56121989bc80) at ../util/qemu-thread-posix.c:541
> #4  0x00007f010df18044 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
> #5  0x00007f010df9861c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> 
> Thread 3 (Thread 0x7f01027886c0 (LWP 46637) "qemu-system-x86"):
> #0  0x00007f010df8b05f in __GI___poll (fds=0x7f00f4003320, nfds=3, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
> #1  0x00007f010f2009ae in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #2  0x00007f010f200cef in g_main_loop_run () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #3  0x000056121856b683 in iothread_run (opaque=0x5612194df940) at ../iothread.c:70
> #4  0x0000561218729f03 in qemu_thread_start (args=0x56121955c1d0) at ../util/qemu-thread-posix.c:541
> #5  0x00007f010df18044 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
> #6  0x00007f010df9861c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> 
> Thread 2 (Thread 0x7f010308a6c0 (LWP 46636) "qemu-system-x86"):
> #0  syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
> #1  0x0000561218729b69 in qemu_futex_wait (f=0x561219185498 <rcu_call_ready_event>, val=4294967295) at /home/febner/repos/qemu/include/qemu/futex.h:29
> #2  0x0000561218729d50 in qemu_event_wait (ev=0x561219185498 <rcu_call_ready_event>) at ../util/qemu-thread-posix.c:464
> #3  0x0000561218737107 in call_rcu_thread (opaque=0x0) at ../util/rcu.c:278
> #4  0x0000561218729f03 in qemu_thread_start (args=0x5612194c35b0) at ../util/qemu-thread-posix.c:541
> #5  0x00007f010df18044 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
> #6  0x00007f010df9861c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> 
> Thread 1 (Thread 0x7f01031f81c0 (LWP 46635) "qemu-system-x86"):
> #0  0x00007f010df8b156 in __ppoll (fds=0x561219fe4060, nfds=1, timeout=<optimized out>, sigmask=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:42
> #1  0x000056121874808e in qemu_poll_ns (fds=0x561219fe4060, nfds=1, timeout=62500000) at ../util/qemu-timer.c:351
> #2  0x00005612187252f4 in fdmon_poll_wait (ctx=0x56121955e540, ready_list=0x7ffe4c596330, timeout=62500000) at ../util/fdmon-poll.c:79
> #3  0x0000561218724c30 in aio_poll (ctx=0x56121955e540, blocking=true) at ../util/aio-posix.c:670
> #4  0x00005612185b614f in bdrv_do_drained_begin (bs=0x5612198b9da0, parent=0x0, poll=true) at ../block/io.c:399
> #5  0x00005612185b626b in bdrv_drained_begin (bs=0x5612198b9da0) at ../block/io.c:412
> #6  0x00005612185a7277 in blk_drain (blk=0x56121a22abd0) at ../block/block-backend.c:2086
> #7  0x00005612185a3bdf in blk_unref (blk=0x56121a22abd0) at ../block/block-backend.c:553
> #8  0x000056121824f7e2 in hmp_qemu_io (mon=0x7ffe4c596630, qdict=0x5612198c0410) at ../block/monitor/block-hmp-cmds.c:621
> #9  0x00005612182b7224 in handle_hmp_command_exec (mon=0x7ffe4c596630, cmd=0x561219099a00 <hmp_cmds+6080>, qdict=0x5612198c0410) at ../monitor/hmp.c:1106
> #10 0x00005612182b7451 in handle_hmp_command (mon=0x7ffe4c596630, cmdline=0x56121a0dfbc8 "mirror-top \"aio_write -P 1 8192 8704\"") at ../monitor/hmp.c:1158
> #11 0x00005612182b8a3e in qmp_human_monitor_command (command_line=0x56121a0dfbc0 "qemu-io mirror-top \"aio_write -P 1 8192 8704\"", has_cpu_index=false, cpu_index=0, errp=0x7ffe4c596740) at ../monitor/qmp-cmds.c:182
> #12 0x00005612186d12e6 in qmp_marshal_human_monitor_command (args=0x7f00f4005e70, ret=0x7f0102889d98, errp=0x7f0102889d90) at qapi/qapi-commands-misc.c:347
> #13 0x00005612187171d2 in do_qmp_dispatch_bh (opaque=0x7f0102889e30) at ../qapi/qmp-dispatch.c:128
> #14 0x0000561218741b1b in aio_bh_call (bh=0x56121a3cf2c0) at ../util/async.c:169
> #15 0x0000561218741c36 in aio_bh_poll (ctx=0x56121955e540) at ../util/async.c:216
> #16 0x00005612187244da in aio_dispatch (ctx=0x56121955e540) at ../util/aio-posix.c:423
> #17 0x0000561218742075 in aio_ctx_dispatch (source=0x56121955e540, callback=0x0, user_data=0x0) at ../util/async.c:358
> #18 0x00007f010f2007a9 in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #19 0x0000561218743830 in glib_pollfds_poll () at ../util/main-loop.c:290
> #20 0x00005612187438ad in os_host_main_loop_wait (timeout=0) at ../util/main-loop.c:313
> #21 0x00005612187439bb in main_loop_wait (nonblocking=0) at ../util/main-loop.c:592
> #22 0x00005612182610e7 in qemu_main_loop () at ../system/runstate.c:782
> #23 0x000056121851ab0e in qemu_default_main () at ../system/main.c:37
> #24 0x000056121851ab49 in main (argc=20, argv=0x7ffe4c596b68) at ../system/main.c:48

[1]:

Note there's also threads 6-18 which are also worker threads with a
similar backtrace as thread 5.

> Thread 5 (Thread 0x7f867ffff6c0 (LWP 48159) "qemu-system-x86"):
> #0  __futex_abstimed_wait_common64 (private=0, cancel=true, abstime=0x7f867fffa300, op=393, expected=0, futex_word=0x562994eef7a8) at ./nptl/futex-internal.c:57
> #1  __futex_abstimed_wait_common (futex_word=futex_word@entry=0x562994eef7a8, expected=expected@entry=0, clockid=clockid@entry=0, abstime=abstime@entry=0x7f867fffa300, private=private@entry=0, cancel=cancel@entry=true) at ./nptl/futex-internal.c:87
> #2  0x00007f86aa314e0b in __GI___futex_abstimed_wait_cancelable64 (futex_word=futex_word@entry=0x562994eef7a8, expected=expected@entry=0, clockid=clockid@entry=0, abstime=abstime@entry=0x7f867fffa300, private=private@entry=0) at ./nptl/futex-internal.c:139
> #3  0x00007f86aa31774c in __pthread_cond_wait_common (abstime=0x7f867fffa300, clockid=0, mutex=0x562994eef710, cond=0x562994eef780) at ./nptl/pthread_cond_wait.c:503
> #4  ___pthread_cond_timedwait64 (cond=0x562994eef780, mutex=0x562994eef710, abstime=0x7f867fffa300) at ./nptl/pthread_cond_wait.c:643
> #5  0x000056299413676e in qemu_cond_timedwait_ts (cond=0x562994eef780, mutex=0x562994eef710, ts=0x7f867fffa300, file=0x562994412d1d "../util/thread-pool.c", line=90) at ../util/qemu-thread-posix.c:239
> #6  0x0000562994136809 in qemu_cond_timedwait_impl (cond=0x562994eef780, mutex=0x562994eef710, ms=10000, file=0x562994412d1d "../util/thread-pool.c", line=90) at ../util/qemu-thread-posix.c:253
> #7  0x00005629941538ac in worker_thread (opaque=0x562994eef700) at ../util/thread-pool.c:90
> #8  0x0000562994136f03 in qemu_thread_start (args=0x562995710f80) at ../util/qemu-thread-posix.c:541
> #9  0x00007f86aa318044 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
> #10 0x00007f86aa39861c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> 
> Thread 4 (Thread 0x7f869e3c66c0 (LWP 48157) "qemu-system-x86"):
> #0  0x00007f86aa2cbc02 in __GI___sigtimedwait (set=set@entry=0x7f869e3c12e0, info=info@entry=0x7f869e3c1210, timeout=timeout@entry=0x0) at ../sysdeps/unix/sysv/linux/sigtimedwait.c:31
> #1  0x00007f86aa2cb31c in __GI___sigwait (set=0x7f869e3c12e0, sig=0x7f869e3c12d0) at ../sysdeps/unix/sysv/linux/sigwait.c:28
> #2  0x0000562993cefc96 in dummy_cpu_thread_fn (arg=0x5629951f7a90) at ../accel/dummy-cpus.c:50
> #3  0x0000562994136f03 in qemu_thread_start (args=0x562995200c80) at ../util/qemu-thread-posix.c:541
> #4  0x00007f86aa318044 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
> #5  0x00007f86aa39861c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> 
> Thread 3 (Thread 0x7f869ebc76c0 (LWP 48156) "qemu-system-x86"):
> #0  0x00007f86aa38b05f in __GI___poll (fds=0x7f8690003320, nfds=2, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
> #1  0x00007f86ab63f9ae in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #2  0x00007f86ab63fcef in g_main_loop_run () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #3  0x0000562993f78683 in iothread_run (opaque=0x562994e44940) at ../iothread.c:70
> #4  0x0000562994136f03 in qemu_thread_start (args=0x562994ec11d0) at ../util/qemu-thread-posix.c:541
> #5  0x00007f86aa318044 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
> #6  0x00007f86aa39861c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> 
> Thread 2 (Thread 0x7f869f4c96c0 (LWP 48155) "qemu-system-x86"):
> #0  syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
> #1  0x0000562994136b69 in qemu_futex_wait (f=0x562994b92498 <rcu_call_ready_event>, val=4294967295) at /home/febner/repos/qemu/include/qemu/futex.h:29
> #2  0x0000562994136d50 in qemu_event_wait (ev=0x562994b92498 <rcu_call_ready_event>) at ../util/qemu-thread-posix.c:464
> #3  0x0000562994144107 in call_rcu_thread (opaque=0x0) at ../util/rcu.c:278
> #4  0x0000562994136f03 in qemu_thread_start (args=0x562994e285b0) at ../util/qemu-thread-posix.c:541
> #5  0x00007f86aa318044 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
> #6  0x00007f86aa39861c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> 
> Thread 1 (Thread 0x7f869f6371c0 (LWP 48154) "qemu-system-x86"):
> #0  0x00007f86aa38b156 in __ppoll (fds=0x562994e3b260, nfds=1, timeout=<optimized out>, sigmask=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:42
> #1  0x000056299415508e in qemu_poll_ns (fds=0x562994e3b260, nfds=1, timeout=62500000) at ../util/qemu-timer.c:351
> #2  0x00005629941322f4 in fdmon_poll_wait (ctx=0x562994ec3540, ready_list=0x7fffe1a38d20, timeout=62500000) at ../util/fdmon-poll.c:79
> #3  0x0000562994131c30 in aio_poll (ctx=0x562994ec3540, blocking=true) at ../util/aio-posix.c:670
> #4  0x0000562993f6a48b in blk_exp_close_all_type (type=BLOCK_EXPORT_TYPE__MAX) at ../block/export/export.c:315
> #5  0x0000562993f6a4ba in blk_exp_close_all () at ../block/export/export.c:320
> #6  0x0000562993c6e26b in qemu_cleanup (status=0) at ../system/runstate.c:853
> #7  0x0000562993f27b1b in qemu_default_main () at ../system/main.c:38
> #8  0x0000562993f27b49 in main (argc=20, argv=0x7fffe1a38f48) at ../system/main.c:48

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

[Kevin Wolf]

Am 25.10.2023 um 14:27 hat Fiona Ebner geschrieben:
> Am 23.10.23 um 13:39 schrieb Fiona Ebner:
> > Am 19.10.23 um 15:36 schrieb Kevin Wolf:
> >> Most of this series looks good to me. Apart from the comments I made in
> >> the individual patches, I would like to see iotests coverage of changing
> >> the mirroring mode. At the least to show that the query result changes,
> >> but ideally also that requests really block after switchting to active.
> >> I think with a throttled target node and immediately reading the target
> >> when the write request completes we should be able to check this.
> >>
> > 
> > I'll try to work something out for v4.
> > 
> 
> I'm having a bit of a hard time unfortunately. I created a throttle
> group with
> 
> >                 'iops-total': iops,
> >                 'iops-total-max': iops

I would have throttled only writes, because you need to do a read to
check the target and don't want that one to be throttled until the
writes have completed.

> and used that for the 'throttle' driver for the target. I then tried
> issuing requests via qemu_io
> 
> >             self.vm.hmp_qemu_io('mirror-top',
> >                                 f'aio_write -P 1 {req_size * i} {req_size * (i + 1)}')
> 
> but when I create more requests than the 'iops' limit (to ensure that
> not all are completed immediately), it will get stuck when draining the
> temporary BlockBackend used by qemu_io [0]. Note this is while still in
> background mode.

You should be able to get around this by using an existing named
BlockBackend (created with -drive if=none) instead of a node name.
mirror-top stays at the root of the tree, right?

> I also wanted to have request going on while the copy mode is changed
> and for that, I was able to work around the issue by creating an NBD
> export of the source like in iotest 151 and issuing the requests to the
> NBD socket instead.
> 
> But after I switch to active mode and when I issue more than the 'iops'
> limit requests to the NBD export then, it also seems to get stuck,
> visible during shutdown when it tries to close the export[1].

Because the NBD server still throttles the I/O that needs to complete
until QEMU can shut down? If this is a problem, I suppose you can
lift the limit on the NBD server side if you use QSD.

graph-changes-while-io is an example of a test cases that uses QSD.

Kevin