On Mon, 12 Jul 2021 13:06:19 +0300 Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> wrote:
> 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? Good Idea. I will add assertions to secondary_do_checkpoint and replication_co_writev too. > > > >>> > >>> 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 > >>> > >> > >> > > > > > > > > --
pgpp8b3MQx9M5.pgp
Description: OpenPGP digital signature