Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben: > Using bdrv_replace_node() for removing filter is not good enough: it > keeps child reference of the filter, which may conflict with original > top node during permission update. > > Instead let's create new interface, which will do all graph > modifications first and then update permissions. > > Let's modify bdrv_replace_node_common(), allowing it additionally drop > backing chain child link pointing to new node. This is quite > appropriate for bdrv_drop_intermediate() and makes possible to add > new bdrv_drop_filter() as a simple wrapper. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > include/block/block.h | 1 + > block.c | 42 ++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index 8f6100dad7..0f21ef313f 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -348,6 +348,7 @@ int bdrv_append(BlockDriverState *bs_new, > BlockDriverState *bs_top, > Error **errp); > int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, > Error **errp); > +int bdrv_drop_filter(BlockDriverState *bs, Error **errp); > > int bdrv_parse_aio(const char *mode, int *flags); > int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough); > diff --git a/block.c b/block.c > index b1394b721c..e835a78f06 100644 > --- a/block.c > +++ b/block.c > @@ -4919,7 +4919,6 @@ static TransactionActionDrv bdrv_remove_backing_drv = { > .commit = bdrv_child_free, > }; > > -__attribute__((unused)) > static void bdrv_remove_backing(BlockDriverState *bs, GSList **tran) > { > if (!bs->backing) { > @@ -4968,15 +4967,30 @@ static int bdrv_replace_node_noperm(BlockDriverState > *from, > * > * With auto_skip=false the error is returned if from has a parent which > should > * not be updated. > + * > + * With detach_subchain to must be in a backing chain of from. In this case
@to and @from make it easier to read. > + * backing link of the cow-parent of @to is removed. > */ > static int bdrv_replace_node_common(BlockDriverState *from, > BlockDriverState *to, > - bool auto_skip, Error **errp) > + bool auto_skip, bool detach_subchain, > + Error **errp) > { > int ret = -EPERM; > GSList *tran = NULL; > g_autoptr(GHashTable) found = NULL; > g_autoptr(GSList) refresh_list = NULL; > + BlockDriverState *to_cow_parent; > + > + if (detach_subchain) { > + assert(bdrv_chain_contains(from, to)); The loop below also relies on from != to, so maybe assert that, too. > + for (to_cow_parent = from; > + bdrv_filter_or_cow_bs(to_cow_parent) != to; > + to_cow_parent = bdrv_filter_or_cow_bs(to_cow_parent)) > + { > + ; > + } > + } > > /* Make sure that @from doesn't go away until we have successfully > attached > * all of its parents to @to. */ > @@ -4997,6 +5011,10 @@ static int bdrv_replace_node_common(BlockDriverState > *from, > goto out; > } > > + if (detach_subchain) { > + bdrv_remove_backing(to_cow_parent, &tran); > + } So bdrv_drop_filter() only works for filters that go through bs->backing? Wouldn't it have been more useful to make it bdrv_remove_filter_or_cow() like you use already use in other places in this patch? If not, the limitation needs to be documented for bdrv_drop_filter(). Kevin