| Message ID | 906c163474aa1fcdf4ffa3cdfb4ad39cb7fc49cb.1625680555.git.lukasstraub2@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | replication: Properly attach children | expand |
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. > > 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) > 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 >
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 > > 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. > > > > 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) > > 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 > > > > --
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 >>> 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 <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) >>> 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 >>> >> >> > > >
On Mon, 12 Jul 2021 13:06:19 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 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 > >>> 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 <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) > >>> 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 > >>> > >> > >> > > > > > > > > --
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; }
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. Signed-off-by: Lukas Straub <lukasstraub2@web.de> --- block/replication.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) -- 2.20.1