13.06.2019 1:09, Max Reitz wrote: > In order to make filters work in backing chains, the associated > functions must be able to deal with them and freeze all filter links, be > they COW or R/W filter links. > > While at it, add some comments that note which functions require their > caller to ensure that a given child link is not frozen, and how the > callers do so. > > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > block.c | 45 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 32 insertions(+), 13 deletions(-) > > diff --git a/block.c b/block.c > index 8438b0699e..45882a3470 100644 > --- a/block.c > +++ b/block.c > @@ -2214,12 +2214,15 @@ static void bdrv_replace_child_noperm(BdrvChild > *child, > * If @new_bs is not NULL, bdrv_check_perm() must be called beforehand, as > this > * function uses bdrv_set_perm() to update the permissions according to the > new > * reference that @new_bs gets. > + * > + * Callers must ensure that child->frozen is false. > */ > static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs) > { > BlockDriverState *old_bs = child->bs; > uint64_t perm, shared_perm; > > + /* Asserts that child->frozen == false */ > bdrv_replace_child_noperm(child, new_bs); > > if (old_bs) { > @@ -2360,6 +2363,7 @@ static void bdrv_detach_child(BdrvChild *child) > g_free(child); > } > > +/* Callers must ensure that child->frozen is false. */
Is such a comment better than one-line extra assertion at start of the function body? > void bdrv_root_unref_child(BdrvChild *child) > { > BlockDriverState *child_bs; > @@ -2369,6 +2373,7 @@ void bdrv_root_unref_child(BdrvChild *child) > bdrv_unref(child_bs); > } > > +/* Callers must ensure that child->frozen is false. */ > void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child) > { > if (child == NULL) { > @@ -2435,6 +2440,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, > BlockDriverState *backing_hd, > } > > if (bs->backing) { > + /* Cannot be frozen, we checked that above */ > bdrv_unref_child(bs, bs->backing); > } > > @@ -3908,6 +3914,7 @@ static void bdrv_close(BlockDriverState *bs) > > if (bs->drv) { > if (bs->drv->bdrv_close) { > + /* Must unfreeze all children, so bdrv_unref_child() works */ > bs->drv->bdrv_close(bs); > } > bs->drv = NULL; > @@ -4281,17 +4288,20 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs) > * Return true if at least one of the backing links between @bs and > * @base is frozen. @errp is set if that's the case. > * @base must be reachable from @bs, or NULL. > + * (Filters are treated as normal elements of the backing chain.) > */ > bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState > *base, > Error **errp) > { > BlockDriverState *i; > + BdrvChild *child; > > - for (i = bs; i != base; i = backing_bs(i)) { > - if (i->backing && i->backing->frozen) { > + for (i = bs; i != base; i = child_bs(child)) { > + child = bdrv_filtered_child(i); > + > + if (child && child->frozen) { > error_setg(errp, "Cannot change '%s' link from '%s' to '%s'", > - i->backing->name, i->node_name, > - backing_bs(i)->node_name); > + child->name, i->node_name, child->bs->node_name); > return true; > } > } > @@ -4305,19 +4315,22 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState > *bs, BlockDriverState *base, > * none of the links are modified. > * @base must be reachable from @bs, or NULL. > * Returns 0 on success. On failure returns < 0 and sets @errp. > + * (Filters are treated as normal elements of the backing chain.) > */ > int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, > Error **errp) > { > BlockDriverState *i; > + BdrvChild *child; > > if (bdrv_is_backing_chain_frozen(bs, base, errp)) { > return -EPERM; > } > > - for (i = bs; i != base; i = backing_bs(i)) { > - if (i->backing) { > - i->backing->frozen = true; > + for (i = bs; i != base; i = child_bs(child)) { > + child = bdrv_filtered_child(i); > + if (child) { > + child->frozen = true; > } > } > > @@ -4328,15 +4341,18 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, > BlockDriverState *base, > * Unfreeze all backing links between @bs and @base. The caller must > * ensure that all links are frozen before using this function. > * @base must be reachable from @bs, or NULL. > + * (Filters are treated as normal elements of the backing chain.) > */ > void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState > *base) > { > BlockDriverState *i; > + BdrvChild *child; > > - for (i = bs; i != base; i = backing_bs(i)) { > - if (i->backing) { > - assert(i->backing->frozen); > - i->backing->frozen = false; > + for (i = bs; i != base; i = child_bs(child)) { > + child = bdrv_filtered_child(i); > + if (child) { > + assert(child->frozen); > + child->frozen = false; > } > } > } > @@ -4438,8 +4454,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, > BlockDriverState *base, > } > } > > - /* Do the actual switch in the in-memory graph. > - * Completes bdrv_check_update_perm() transaction internally. */ > + /* > + * Do the actual switch in the in-memory graph. > + * Completes bdrv_check_update_perm() transaction internally. > + * c->frozen is false, we have checked that above. > + */ > bdrv_ref(base); > bdrv_replace_child(c, base); > bdrv_unref(top); > Hmm, OK, it's better than it was, so: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> But I have one thought: we check that frozen is false at some point, and then do some logic around this child. Where is guarantee, that someone else will not set frozen=true during some yield, for example? So, do we need a kind of child_mutex, or something like this? -- Best regards, Vladimir