Re: [PATCH v2 25/36] block: introduce bdrv_drop_filter()

From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: qemu-block@nongnu.org, armbru@redhat.com, qemu-devel@nongnu.org,
	mreitz@redhat.com, den@openvz.org, jsnow@redhat.com
Subject: Re: [PATCH v2 25/36] block: introduce bdrv_drop_filter()
Date: Thu, 4 Feb 2021 12:31:28 +0100	[thread overview]
Message-ID: <20210204113128.GD6496@merkur.fritz.box> (raw)
In-Reply-To: <20201127144522.29991-26-vsementsov@virtuozzo.com>

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