[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 25/36] block: introduce bdrv_drop_filter()
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v2 25/36] block: introduce bdrv_drop_filter() |
Date: |
Thu, 4 Feb 2021 12:31:28 +0100 |
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 <vsementsov@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
- Re: [PATCH v2 25/36] block: introduce bdrv_drop_filter(),
Kevin Wolf <=