|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH v2 23/36] block: adapt bdrv_append() for inserting filters |
Date: | Thu, 4 Feb 2021 11:30:11 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
04.02.2021 00:33, Kevin Wolf wrote:
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?
Hm. I considered it mostly like a lack not a bug. We just have to workaround this lack by "inactive" mode of filters. But adding a test is good idea anyway. Will do.
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().
I decided that bdrv_append() is only for appending new nodes, so frozen and inherts_from checks are not needed. And I've added assert(!bs_new->backing)... Checking this now: - appending filters is obvious - bdrv_append_temp_snapshot() creates new qcow2 node based on tmp file, don't see any backing initialization (and it would be rather strange) - external_snapshot_prepare() do check if (bdrv_cow_child(state->new_bs)) { error-out } So everything is OK. I should describe it in commit message and add a comment to 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
-- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |