[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 4/4] replication: Remove workaround
From: |
Lukas Straub |
Subject: |
Re: [PATCH v3 4/4] replication: Remove workaround |
Date: |
Mon, 12 Jul 2021 13:12:07 +0200 |
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
> >>>
> >>
> >>
> >
> >
> >
>
>
--
pgpt8b2O2ZmIO.pgp
Description: OpenPGP digital signature
- Re: [PATCH v3 1/4] replication: Remove s->active_disk, (continued)