[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 4/4] replication: Remove workaround

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v3 4/4] replication: Remove workaround
Date: Mon, 12 Jul 2021 13:06:19 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

11.07.2021 23:33, Lukas Straub wrote:
On Fri, 9 Jul 2021 10:49:23 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

07.07.2021 21:15, Lukas Straub wrote:
Remove the workaround introduced in commit
"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 

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 <lukasstraub2@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 <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 c0d4a6c264..68b46d65a8 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -348,17 +348,7 @@ static void secondary_do_checkpoint(BlockDriverState *bs, 
Error **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);
+    ret = bdrv_make_empty(s->hidden_disk, errp);
       if (ret < 0) {

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]