[PATCH] replication: Avoid blk_make_empty() on read-only child

[PATCH] replication: Avoid blk_make_empty() on read-only child

[Kevin Wolf]

This is just a bandaid to keep tests/test-replication working after
bdrv_make_empty() starts to assert that we're not trying to call it on a
read-only child.

For the real solution in the future, replication should not steal the
BdrvChild from its backing file (this is never correct to do!), but
instead have its own child node references, with the appropriate
permissions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/replication.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/replication.c b/block/replication.c
index f1820ab1d0..ccf7b78160 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -343,7 +343,17 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
         return;
     }
 
-    ret = bdrv_make_empty(s->hidden_disk, errp);
+    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);
     if (ret < 0) {
         return;
     }
-- 
2.25.4

Re: [PATCH] replication: Avoid blk_make_empty() on read-only child

[Kevin Wolf]

Am 15.05.2020 um 14:03 hat Kevin Wolf geschrieben:
> This is just a bandaid to keep tests/test-replication working after
> bdrv_make_empty() starts to assert that we're not trying to call it on a
> read-only child.
> 
> For the real solution in the future, replication should not steal the
> BdrvChild from its backing file (this is never correct to do!), but
> instead have its own child node references, with the appropriate
> permissions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

I'll commit this patch for now so the brokenness of the replication
driver won't stop my pull request, but this is really not how things
should work.

Can someone who is interested in replication have a look at the real
solution, please? It's not trivial enough for me to fix it without
learning a lot about using replication first.

Essentially, the problem is that you can't just copy the BdrvChild from
bs->backing like replication_start() does, but you need to use
bdrv_attach_child() to attach the respective node directly to bs, too.
This gives you the opportunity to request the write permissions you need
for bdrv_make_empty().

However, it seems that during replication_start(), the backing file
isn't writable yet (which is not surprising, backing files are supposed
to be read-only), so my attempt to request write permissions right there
failed. So it seems we need to figure out where the backing file becomes
writable and then take write permissions after this point. Or make it
writable earlier. I don't understand enough about the whole setup to
figure out where this should happen, which is why I'm asking you.

Kevin