Re: [PATCH v3 3/4] replication: Properly attach children

[Vladimir Sementsov-Ogievskiy]

07.07.2021 21:15, Lukas Straub wrote:
> The replication driver needs access to the children block-nodes of
> it's child so it can issue bdrv_make_empty() and bdrv_co_pwritev()
> to manage the replication. However, it does this by directly copying
> the BdrvChilds, which is wrong.
>
> Fix this by properly attaching the block-nodes with
> bdrv_attach_child() and requesting the required permissions.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>   block/replication.c | 30 +++++++++++++++++++++++++++---
>   1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/block/replication.c b/block/replication.c
> index 74adf30f54..c0d4a6c264 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -165,7 +165,12 @@ static void replication_child_perm(BlockDriverState *bs, 
> BdrvChild *c,
>                                      uint64_t perm, uint64_t shared,
>                                      uint64_t *nperm, uint64_t *nshared)
>   {
> -    *nperm = BLK_PERM_CONSISTENT_READ;
> +    if (role & BDRV_CHILD_PRIMARY) {
> +        *nperm = BLK_PERM_CONSISTENT_READ;
> +    } else {
> +        *nperm = 0;
> +    }
> +
>       if ((bs->open_flags & (BDRV_O_INACTIVE | BDRV_O_RDWR)) == BDRV_O_RDWR) {
>           *nperm |= BLK_PERM_WRITE;
>       }
> @@ -552,8 +557,25 @@ static void replication_start(ReplicationState *rs, 
> ReplicationMode mode,
>               return;
>           }
>
> -        s->hidden_disk = hidden_disk;
> -        s->secondary_disk = secondary_disk;
> +        bdrv_ref(hidden_disk->bs);
> +        s->hidden_disk = bdrv_attach_child(bs, hidden_disk->bs, "hidden disk",
> +                                           &child_of_bds, BDRV_CHILD_DATA,
> +                                           &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            aio_context_release(aio_context);
> +            return;
> +        }
> +
> +        bdrv_ref(secondary_disk->bs);
> +        s->secondary_disk = bdrv_attach_child(bs, secondary_disk->bs,
> +                                              "secondary disk", &child_of_bds,
> +                                              BDRV_CHILD_DATA, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            aio_context_release(aio_context);
> +            return;
> +        }

Now looking at this I can say that design is a bit strange:

If these children are children of replication state, we probably want something 
like bdrv_root_attach_child(), and not attach children to the active disk but 
to the state itself (like block-job children).. But than we'll need new class 
of bdrv children (child_of_replication?) and that obviously not worth the 
complexity..

So, we want new children to be children of our filter driver. Than, we probably 
should create them in repliction_open(), together with bs->file..

Still, it should work as is I think:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


>           /* start backup job now */
>           error_setg(&s->blocker,
> @@ -659,7 +681,9 @@ static void replication_done(void *opaque, int ret)
>       if (ret == 0) {
>           s->stage = BLOCK_REPLICATION_DONE;
>
> +        bdrv_unref_child(bs, s->secondary_disk);
>           s->secondary_disk = NULL;
> +        bdrv_unref_child(bs, s->hidden_disk);
>           s->hidden_disk = NULL;
>           s->error = 0;
>       } else {
> --
> 2.20.1


--
Best regards,
Vladimir