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(-)

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

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

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
>>>    
>>
>>
> 
> 
> 


-- 
Best regards,
Vladimir

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
> >>>      
> >>
> >>  
> > 
> > 
> >   
> 
> 



--