| Message ID | c50f2ea294e2f08f7fc85f29182105ed944e9e7b.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: > 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 >
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; + } /* 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 {
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(-) -- 2.20.1