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

[Vladimir Sementsov-Ogievskiy]

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


--
Best regards,
Vladimir