[PATCH v6 0/4] replication: Bugfix and properly attach children

[PATCH v6 0/4] replication: Bugfix and properly attach children

[Lukas Straub]

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

Hello Everyone,
A while ago Kevin noticed that the replication driver doesn't properly attach
the children it wants to use. Instead, it directly copies the BdrvChilds from
it's backing file, which is wrong. Ths Patchset fixes the problem, fixes a
potential crash in replication_co_writev due to missing permissions and removes
a workaround that was put in place back then.

Tested with full COLO-migration setup in my COLO testsuite.

Regards,
Lukas Straub

Changes:

-v6:
    -Drop "replication: Assert that children are writable"
    -Added Reviewed-by tags

-v5:
    -Assert that children are writable where it's needed

-v4:
    -minor style fixes
    -clarify why children areguaranteed to be writable in
     "replication: Remove workaround"
    -Added Reviewed-by tags

-v3:
    -Split up into multiple patches
    -Remove s->active_disk
    -Clarify child permissions in commit message

-v2: Test for BDRV_CHILD_PRIMARY in replication_child_perm, since
     bs->file might not be set yet. (Vladimir)

Lukas Straub (4):
  replication: Remove s->active_disk
  replication: Reduce usage of s->hidden_disk and s->secondary_disk
  replication: Properly attach children
  replication: Remove workaround

 block/replication.c | 111 +++++++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 43 deletions(-)

--
2.20.1

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

[PATCH v6 1/4] replication: Remove s->active_disk

[Lukas Straub]

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

s->active_disk is bs->file. Remove it and use local variables instead.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/replication.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 774e15df16..9ad2dfdc69 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -35,7 +35,6 @@ typedef enum {
 typedef struct BDRVReplicationState {
     ReplicationMode mode;
     ReplicationStage stage;
-    BdrvChild *active_disk;
     BlockJob *commit_job;
     BdrvChild *hidden_disk;
     BdrvChild *secondary_disk;
@@ -307,8 +306,10 @@ out:
     return ret;
 }

-static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
+static void secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
 {
+    BDRVReplicationState *s = bs->opaque;
+    BdrvChild *active_disk = bs->file;
     Error *local_err = NULL;
     int ret;

@@ -323,13 +324,13 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
         return;
     }

-    if (!s->active_disk->bs->drv) {
+    if (!active_disk->bs->drv) {
         error_setg(errp, "Active disk %s is ejected",
-                   s->active_disk->bs->node_name);
+                   active_disk->bs->node_name);
         return;
     }

-    ret = bdrv_make_empty(s->active_disk, errp);
+    ret = bdrv_make_empty(active_disk, errp);
     if (ret < 0) {
         return;
     }
@@ -458,6 +459,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
     BlockDriverState *bs = rs->opaque;
     BDRVReplicationState *s;
     BlockDriverState *top_bs;
+    BdrvChild *active_disk;
     int64_t active_length, hidden_length, disk_length;
     AioContext *aio_context;
     Error *local_err = NULL;
@@ -495,15 +497,14 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
     case REPLICATION_MODE_PRIMARY:
         break;
     case REPLICATION_MODE_SECONDARY:
-        s->active_disk = bs->file;
-        if (!s->active_disk || !s->active_disk->bs ||
-                                    !s->active_disk->bs->backing) {
+        active_disk = bs->file;
+        if (!active_disk || !active_disk->bs || !active_disk->bs->backing) {
             error_setg(errp, "Active disk doesn't have backing file");
             aio_context_release(aio_context);
             return;
         }

-        s->hidden_disk = s->active_disk->bs->backing;
+        s->hidden_disk = active_disk->bs->backing;
         if (!s->hidden_disk->bs || !s->hidden_disk->bs->backing) {
             error_setg(errp, "Hidden disk doesn't have backing file");
             aio_context_release(aio_context);
@@ -518,7 +519,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
         }

         /* verify the length */
-        active_length = bdrv_getlength(s->active_disk->bs);
+        active_length = bdrv_getlength(active_disk->bs);
         hidden_length = bdrv_getlength(s->hidden_disk->bs);
         disk_length = bdrv_getlength(s->secondary_disk->bs);
         if (active_length < 0 || hidden_length < 0 || disk_length < 0 ||
@@ -530,9 +531,9 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
         }

         /* Must be true, or the bdrv_getlength() calls would have failed */
-        assert(s->active_disk->bs->drv && s->hidden_disk->bs->drv);
+        assert(active_disk->bs->drv && s->hidden_disk->bs->drv);

-        if (!s->active_disk->bs->drv->bdrv_make_empty ||
+        if (!active_disk->bs->drv->bdrv_make_empty ||
             !s->hidden_disk->bs->drv->bdrv_make_empty) {
             error_setg(errp,
                        "Active disk or hidden disk doesn't support make_empty");
@@ -586,7 +587,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
     s->stage = BLOCK_REPLICATION_RUNNING;

     if (s->mode == REPLICATION_MODE_SECONDARY) {
-        secondary_do_checkpoint(s, errp);
+        secondary_do_checkpoint(bs, errp);
     }

     s->error = 0;
@@ -615,7 +616,7 @@ static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
     }

     if (s->mode == REPLICATION_MODE_SECONDARY) {
-        secondary_do_checkpoint(s, errp);
+        secondary_do_checkpoint(bs, errp);
     }
     aio_context_release(aio_context);
 }
@@ -652,7 +653,6 @@ static void replication_done(void *opaque, int ret)
     if (ret == 0) {
         s->stage = BLOCK_REPLICATION_DONE;

-        s->active_disk = NULL;
         s->secondary_disk = NULL;
         s->hidden_disk = NULL;
         s->error = 0;
@@ -705,7 +705,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
         }

         if (!failover) {
-            secondary_do_checkpoint(s, errp);
+            secondary_do_checkpoint(bs, errp);
             s->stage = BLOCK_REPLICATION_DONE;
             aio_context_release(aio_context);
             return;
@@ -713,7 +713,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)

         s->stage = BLOCK_REPLICATION_FAILOVER;
         s->commit_job = commit_active_start(
-                            NULL, s->active_disk->bs, s->secondary_disk->bs,
+                            NULL, bs->file->bs, s->secondary_disk->bs,
                             JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
                             NULL, replication_done, bs, true, errp);
         break;
--
2.20.1


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

[PATCH v6 2/4] replication: Reduce usage of s->hidden_disk and s->secondary_disk

[Lukas Straub]

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

In preparation for the next patch, initialize s->hidden_disk and
s->secondary_disk later and replace access to them with local variables
in the places where they aren't initialized yet.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/replication.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 9ad2dfdc69..25bbdf5d4b 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -366,27 +366,35 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
                                 Error **errp)
 {
     BDRVReplicationState *s = bs->opaque;
+    BdrvChild *hidden_disk, *secondary_disk;
     BlockReopenQueue *reopen_queue = NULL;

+    /*
+     * s->hidden_disk and s->secondary_disk may not be set yet, as they will
+     * only be set after the children are writable.
+     */
+    hidden_disk = bs->file->bs->backing;
+    secondary_disk = hidden_disk->bs->backing;
+
     if (writable) {
-        s->orig_hidden_read_only = bdrv_is_read_only(s->hidden_disk->bs);
-        s->orig_secondary_read_only = bdrv_is_read_only(s->secondary_disk->bs);
+        s->orig_hidden_read_only = bdrv_is_read_only(hidden_disk->bs);
+        s->orig_secondary_read_only = bdrv_is_read_only(secondary_disk->bs);
     }

-    bdrv_subtree_drained_begin(s->hidden_disk->bs);
-    bdrv_subtree_drained_begin(s->secondary_disk->bs);
+    bdrv_subtree_drained_begin(hidden_disk->bs);
+    bdrv_subtree_drained_begin(secondary_disk->bs);

     if (s->orig_hidden_read_only) {
         QDict *opts = qdict_new();
         qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
-        reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
+        reopen_queue = bdrv_reopen_queue(reopen_queue, hidden_disk->bs,
                                          opts, true);
     }

     if (s->orig_secondary_read_only) {
         QDict *opts = qdict_new();
         qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
-        reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
+        reopen_queue = bdrv_reopen_queue(reopen_queue, secondary_disk->bs,
                                          opts, true);
     }

@@ -401,8 +409,8 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
         }
     }

-    bdrv_subtree_drained_end(s->hidden_disk->bs);
-    bdrv_subtree_drained_end(s->secondary_disk->bs);
+    bdrv_subtree_drained_end(hidden_disk->bs);
+    bdrv_subtree_drained_end(secondary_disk->bs);
 }

 static void backup_job_cleanup(BlockDriverState *bs)
@@ -459,7 +467,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
     BlockDriverState *bs = rs->opaque;
     BDRVReplicationState *s;
     BlockDriverState *top_bs;
-    BdrvChild *active_disk;
+    BdrvChild *active_disk, *hidden_disk, *secondary_disk;
     int64_t active_length, hidden_length, disk_length;
     AioContext *aio_context;
     Error *local_err = NULL;
@@ -504,15 +512,15 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
             return;
         }

-        s->hidden_disk = active_disk->bs->backing;
-        if (!s->hidden_disk->bs || !s->hidden_disk->bs->backing) {
+        hidden_disk = active_disk->bs->backing;
+        if (!hidden_disk->bs || !hidden_disk->bs->backing) {
             error_setg(errp, "Hidden disk doesn't have backing file");
             aio_context_release(aio_context);
             return;
         }

-        s->secondary_disk = s->hidden_disk->bs->backing;
-        if (!s->secondary_disk->bs || !bdrv_has_blk(s->secondary_disk->bs)) {
+        secondary_disk = hidden_disk->bs->backing;
+        if (!secondary_disk->bs || !bdrv_has_blk(secondary_disk->bs)) {
             error_setg(errp, "The secondary disk doesn't have block backend");
             aio_context_release(aio_context);
             return;
@@ -520,8 +528,8 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,

         /* verify the length */
         active_length = bdrv_getlength(active_disk->bs);
-        hidden_length = bdrv_getlength(s->hidden_disk->bs);
-        disk_length = bdrv_getlength(s->secondary_disk->bs);
+        hidden_length = bdrv_getlength(hidden_disk->bs);
+        disk_length = bdrv_getlength(secondary_disk->bs);
         if (active_length < 0 || hidden_length < 0 || disk_length < 0 ||
             active_length != hidden_length || hidden_length != disk_length) {
             error_setg(errp, "Active disk, hidden disk, secondary disk's length"
@@ -531,10 +539,10 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
         }

         /* Must be true, or the bdrv_getlength() calls would have failed */
-        assert(active_disk->bs->drv && s->hidden_disk->bs->drv);
+        assert(active_disk->bs->drv && hidden_disk->bs->drv);

         if (!active_disk->bs->drv->bdrv_make_empty ||
-            !s->hidden_disk->bs->drv->bdrv_make_empty) {
+            !hidden_disk->bs->drv->bdrv_make_empty) {
             error_setg(errp,
                        "Active disk or hidden disk doesn't support make_empty");
             aio_context_release(aio_context);
@@ -549,6 +557,9 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
             return;
         }

+        s->hidden_disk = hidden_disk;
+        s->secondary_disk = secondary_disk;
+
         /* start backup job now */
         error_setg(&s->blocker,
                    "Block device is in use by internal backup job");
--
2.20.1


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

[PATCH v6 3/4] replication: Properly attach children

[Lukas Straub]

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

The replication driver needs access to the children block-nodes of
it's child so it can issue bdrv_make_empty() and bdrv_co_pwritev()
to manage the replication. However, it does this by directly copying
the BdrvChilds, which is wrong.

Fix this by properly attaching the block-nodes with
bdrv_attach_child() and requesting the required permissions.

This ultimatively fixes a potential crash in replication_co_writev(),
because it may write to s->secondary_disk if it is in state
BLOCK_REPLICATION_FAILOVER_FAILED, without requesting write
permissions first. And now the workaround in
secondary_do_checkpoint() can be removed.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/replication.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 25bbdf5d4b..b74192f795 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -165,7 +165,12 @@ static void replication_child_perm(BlockDriverState *bs, BdrvChild *c,
                                    uint64_t perm, uint64_t shared,
                                    uint64_t *nperm, uint64_t *nshared)
 {
-    *nperm = BLK_PERM_CONSISTENT_READ;
+    if (role & BDRV_CHILD_PRIMARY) {
+        *nperm = BLK_PERM_CONSISTENT_READ;
+    } else {
+        *nperm = 0;
+    }
+
     if ((bs->open_flags & (BDRV_O_INACTIVE | BDRV_O_RDWR)) == BDRV_O_RDWR) {
         *nperm |= BLK_PERM_WRITE;
     }
@@ -557,8 +562,25 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
             return;
         }

-        s->hidden_disk = hidden_disk;
-        s->secondary_disk = secondary_disk;
+        bdrv_ref(hidden_disk->bs);
+        s->hidden_disk = bdrv_attach_child(bs, hidden_disk->bs, "hidden disk",
+                                           &child_of_bds, BDRV_CHILD_DATA,
+                                           &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            aio_context_release(aio_context);
+            return;
+        }
+
+        bdrv_ref(secondary_disk->bs);
+        s->secondary_disk = bdrv_attach_child(bs, secondary_disk->bs,
+                                              "secondary disk", &child_of_bds,
+                                              BDRV_CHILD_DATA, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            aio_context_release(aio_context);
+            return;
+        }

         /* start backup job now */
         error_setg(&s->blocker,
@@ -664,7 +686,9 @@ static void replication_done(void *opaque, int ret)
     if (ret == 0) {
         s->stage = BLOCK_REPLICATION_DONE;

+        bdrv_unref_child(bs, s->secondary_disk);
         s->secondary_disk = NULL;
+        bdrv_unref_child(bs, s->hidden_disk);
         s->hidden_disk = NULL;
         s->error = 0;
     } else {
--
2.20.1


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

[PATCH v6 4/4] replication: Remove workaround

[Lukas Straub]

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

Remove the workaround introduced in commit
6ecbc6c52672db5c13805735ca02784879ce8285
"replication: Avoid blk_make_empty() on read-only child".

It is not needed anymore since s->hidden_disk is guaranteed to be
writable when secondary_do_checkpoint() runs. Because replication_start(),
_do_checkpoint() and _stop() are only called by COLO migration code
and COLO-migration activates all disks via bdrv_invalidate_cache_all()
before it calls these functions.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/replication.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index b74192f795..32444b9a8f 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -346,17 +346,7 @@ static void secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
         return;
     }

-    BlockBackend *blk = blk_new(qemu_get_current_aio_context(),
-                                BLK_PERM_WRITE, BLK_PERM_ALL);
-    blk_insert_bs(blk, s->hidden_disk->bs, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        blk_unref(blk);
-        return;
-    }
-
-    ret = blk_make_empty(blk, errp);
-    blk_unref(blk);
+    ret = bdrv_make_empty(s->hidden_disk, errp);
     if (ret < 0) {
         return;
     }
--
2.20.1

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

RE: [PATCH v6 1/4] replication: Remove s->active_disk

[Zhang, Chen]

> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Lukas Straub
> Sent: Sunday, July 18, 2021 10:48 PM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Kevin Wolf <kwolf@redhat.com>; Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com>; qemu-block <qemu-block@nongnu.org>;
> Wen Congyang <wencongyang2@huawei.com>; Xie Changlong
> <xiechanglong.d@gmail.com>; Max Reitz <mreitz@redhat.com>
> Subject: [PATCH v6 1/4] replication: Remove s->active_disk
> 
> s->active_disk is bs->file. Remove it and use local variables instead.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Looks good for me.
Reviewed-by: Zhang Chen <chen.zhang@intel.com>

> ---
>  block/replication.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/block/replication.c b/block/replication.c index
> 774e15df16..9ad2dfdc69 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -35,7 +35,6 @@ typedef enum {
>  typedef struct BDRVReplicationState {
>      ReplicationMode mode;
>      ReplicationStage stage;
> -    BdrvChild *active_disk;
>      BlockJob *commit_job;
>      BdrvChild *hidden_disk;
>      BdrvChild *secondary_disk;
> @@ -307,8 +306,10 @@ out:
>      return ret;
>  }
> 
> -static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
> +static void secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
>  {
> +    BDRVReplicationState *s = bs->opaque;
> +    BdrvChild *active_disk = bs->file;
>      Error *local_err = NULL;
>      int ret;
> 
> @@ -323,13 +324,13 @@ static void
> secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
>          return;
>      }
> 
> -    if (!s->active_disk->bs->drv) {
> +    if (!active_disk->bs->drv) {
>          error_setg(errp, "Active disk %s is ejected",
> -                   s->active_disk->bs->node_name);
> +                   active_disk->bs->node_name);
>          return;
>      }
> 
> -    ret = bdrv_make_empty(s->active_disk, errp);
> +    ret = bdrv_make_empty(active_disk, errp);
>      if (ret < 0) {
>          return;
>      }
> @@ -458,6 +459,7 @@ static void replication_start(ReplicationState *rs,
> ReplicationMode mode,
>      BlockDriverState *bs = rs->opaque;
>      BDRVReplicationState *s;
>      BlockDriverState *top_bs;
> +    BdrvChild *active_disk;
>      int64_t active_length, hidden_length, disk_length;
>      AioContext *aio_context;
>      Error *local_err = NULL;
> @@ -495,15 +497,14 @@ static void replication_start(ReplicationState *rs,
> ReplicationMode mode,
>      case REPLICATION_MODE_PRIMARY:
>          break;
>      case REPLICATION_MODE_SECONDARY:
> -        s->active_disk = bs->file;
> -        if (!s->active_disk || !s->active_disk->bs ||
> -                                    !s->active_disk->bs->backing) {
> +        active_disk = bs->file;
> +        if (!active_disk || !active_disk->bs ||
> + !active_disk->bs->backing) {
>              error_setg(errp, "Active disk doesn't have backing file");
>              aio_context_release(aio_context);
>              return;
>          }
> 
> -        s->hidden_disk = s->active_disk->bs->backing;
> +        s->hidden_disk = active_disk->bs->backing;
>          if (!s->hidden_disk->bs || !s->hidden_disk->bs->backing) {
>              error_setg(errp, "Hidden disk doesn't have backing file");
>              aio_context_release(aio_context); @@ -518,7 +519,7 @@ static void
> replication_start(ReplicationState *rs, ReplicationMode mode,
>          }
> 
>          /* verify the length */
> -        active_length = bdrv_getlength(s->active_disk->bs);
> +        active_length = bdrv_getlength(active_disk->bs);
>          hidden_length = bdrv_getlength(s->hidden_disk->bs);
>          disk_length = bdrv_getlength(s->secondary_disk->bs);
>          if (active_length < 0 || hidden_length < 0 || disk_length < 0 || @@ -
> 530,9 +531,9 @@ static void replication_start(ReplicationState *rs,
> ReplicationMode mode,
>          }
> 
>          /* Must be true, or the bdrv_getlength() calls would have failed */
> -        assert(s->active_disk->bs->drv && s->hidden_disk->bs->drv);
> +        assert(active_disk->bs->drv && s->hidden_disk->bs->drv);
> 
> -        if (!s->active_disk->bs->drv->bdrv_make_empty ||
> +        if (!active_disk->bs->drv->bdrv_make_empty ||
>              !s->hidden_disk->bs->drv->bdrv_make_empty) {
>              error_setg(errp,
>                         "Active disk or hidden disk doesn't support make_empty"); @@ -
> 586,7 +587,7 @@ static void replication_start(ReplicationState *rs,
> ReplicationMode mode,
>      s->stage = BLOCK_REPLICATION_RUNNING;
> 
>      if (s->mode == REPLICATION_MODE_SECONDARY) {
> -        secondary_do_checkpoint(s, errp);
> +        secondary_do_checkpoint(bs, errp);
>      }
> 
>      s->error = 0;
> @@ -615,7 +616,7 @@ static void
> replication_do_checkpoint(ReplicationState *rs, Error **errp)
>      }
> 
>      if (s->mode == REPLICATION_MODE_SECONDARY) {
> -        secondary_do_checkpoint(s, errp);
> +        secondary_do_checkpoint(bs, errp);
>      }
>      aio_context_release(aio_context);
>  }
> @@ -652,7 +653,6 @@ static void replication_done(void *opaque, int ret)
>      if (ret == 0) {
>          s->stage = BLOCK_REPLICATION_DONE;
> 
> -        s->active_disk = NULL;
>          s->secondary_disk = NULL;
>          s->hidden_disk = NULL;
>          s->error = 0;
> @@ -705,7 +705,7 @@ static void replication_stop(ReplicationState *rs, bool
> failover, Error **errp)
>          }
> 
>          if (!failover) {
> -            secondary_do_checkpoint(s, errp);
> +            secondary_do_checkpoint(bs, errp);
>              s->stage = BLOCK_REPLICATION_DONE;
>              aio_context_release(aio_context);
>              return;
> @@ -713,7 +713,7 @@ static void replication_stop(ReplicationState *rs, bool
> failover, Error **errp)
> 
>          s->stage = BLOCK_REPLICATION_FAILOVER;
>          s->commit_job = commit_active_start(
> -                            NULL, s->active_disk->bs, s->secondary_disk->bs,
> +                            NULL, bs->file->bs, s->secondary_disk->bs,
>                              JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
>                              NULL, replication_done, bs, true, errp);
>          break;
> --
> 2.20.1

Re: [PATCH v6 0/4] replication: Bugfix and properly attach children

[Kevin Wolf]

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

Am 18.07.2021 um 16:48 hat Lukas Straub geschrieben:
> Hello Everyone,
> A while ago Kevin noticed that the replication driver doesn't properly attach
> the children it wants to use. Instead, it directly copies the BdrvChilds from
> it's backing file, which is wrong. Ths Patchset fixes the problem, fixes a
> potential crash in replication_co_writev due to missing permissions and removes
> a workaround that was put in place back then.
> 
> Tested with full COLO-migration setup in my COLO testsuite.

Thanks for taking care of this, Lukas!

I've applied the series to the block branch.

Kevin

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