11.07.2021 23:33, Lukas Straub wrote:
On Fri, 9 Jul 2021 10:49:23 +0300
Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> wrote:
07.07.2021 21:15, Lukas Straub wrote:
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 doesn't inactivate disks.
If look at replication_child_perm() you should also be sure that it always
works only with RW disks..
Actually, I think that it would be correct just require BLK_PERM_WRITE in
replication_child_perm() unconditionally. Let generic layer care about all
these RD/WR things. In _child_perm() we can require WRITE and don't care. If
something goes wrong and we can't get WRITE permission we should see clean
error-out.
Opposite, if we don't require WRITE permission in some case and still do WRITE
request, it may crash.
Still, this may be considered as a preexisting problem of
replication_child_perm() and fixed separately.
Hmm, unconditionally requesting write doesn't work, since qemu on the
secondary side is started with "-miration incoming", it goes into
runstate RUN_STATE_INMIGRATE from the beginning and then blockdev_init()
opens every blockdev with BDRV_O_INACTIVE and then it errors out with
-drive driver=replication,...: Block node is read-only.
Ah, OK. So we need this check in _child_perm().. Then, maybe, leave check or
assertion in secondary_do_checkpoint, that hidden_disk is writable?
Signed-off-by: Lukas Straub <lukasstra...@web.de>
So, for this one commit (with probably updated commit message accordingly to my
comments, or even rebased on fixed replication_child_perm()):
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
block/replication.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index c0d4a6c264..68b46d65a8 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -348,17 +348,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
--
Best regards,
Vladimir
