[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 23/36] block: adapt bdrv_append() for inserting filters
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v2 23/36] block: adapt bdrv_append() for inserting filters |
Date: |
Wed, 3 Feb 2021 22:33:46 +0100 |
Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> bdrv_append is not very good for inserting filters: it does extra
> permission update as part of bdrv_set_backing_hd(). During this update
> filter may conflict with other parents of top_bs.
>
> Instead, let's first do all graph modifications and after it update
> permissions.
This sounds like it fixes a bug. If so, should we have a test like for
the other cases fixed by this series?
> Note: bdrv_append() is still only works for backing-child based
> filters. It's something to improve later.
>
> It simplifies the fact that bdrv_append() used to append new nodes,
> without backing child. Let's add an assertion.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block.c | 28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/block.c b/block.c
> index 02da1a90bc..7094922509 100644
> --- a/block.c
> +++ b/block.c
> @@ -4998,22 +4998,28 @@ int bdrv_replace_node(BlockDriverState *from,
> BlockDriverState *to,
> int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> Error **errp)
> {
> - Error *local_err = NULL;
> + int ret;
> + GSList *tran = NULL;
>
> - bdrv_set_backing_hd(bs_new, bs_top, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return -EPERM;
> + assert(!bs_new->backing);
> +
> + ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
> + &child_of_bds, bdrv_backing_role(bs_new),
> + &bs_new->backing, &tran, errp);
> + if (ret < 0) {
> + goto out;
> }
I don't think changing bs->backing without bdrv_set_backing_hd() is
correct at the moment. We lose a few things:
1. The bdrv_is_backing_chain_frozen() check
2. Updating backing_hd->inherits_from if necessary
3. bdrv_refresh_limits()
If I'm not missing anything, all of these are needed in the context of
bdrv_append().
> - bdrv_replace_node(bs_top, bs_new, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - bdrv_set_backing_hd(bs_new, NULL, &error_abort);
> - return -EPERM;
> + ret = bdrv_replace_node_noperm(bs_top, bs_new, true, &tran, errp);
> + if (ret < 0) {
> + goto out;
> }
>
> - return 0;
> + ret = bdrv_refresh_perms(bs_new, errp);
> +out:
> + tran_finalize(tran, ret);
> +
> + return ret;
> }
Kevin
- Re: [PATCH v2 23/36] block: adapt bdrv_append() for inserting filters,
Kevin Wolf <=