From d669ed6ab028497d634e1f236c74a98725f9e45f Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 6 Nov 2020 15:42:37 +0300 Subject: [PATCH] block: make bdrv_drop_intermediate() less wrong First, permission update loop tries to do iterations transactionally, but the whole update is not transactional: nobody roll-back successful loop iterations when some iteration fails. Second, in the iteration we have nested permission update: c->klass->update_filename may point to bdrv_child_cb_update_filename() which calls bdrv_backing_update_filename(), which may do node reopen to RW. Permission update system is not prepared to nested updates, at least it has intermediate permission-update state stored in BdrvChild structures: has_backup_perm, backup_perm and backup_shared_perm. So, let's first do bdrv_replace_node_common() (which is more transactional than open-coded update in bdrv_drop_intermediate()) and then call update_filename() in separate. We still do not rollback changes in case of update_filename() failure but it's not much worse than pre-patch behavior. Note that bdrv_replace_node_common() does check for frozen children, so corresponding check is dropped in bdrv_drop_intermediate(). Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201106124241.16950-4-vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia Signed-off-by: Max Reitz --- block.c | 54 ++++++++++++++++++++++++------------------------------ 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/block.c b/block.c index 9a945a058d..f1cedac362 100644 --- a/block.c +++ b/block.c @@ -4910,9 +4910,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, { BlockDriverState *explicit_top = top; bool update_inherits_from; - BdrvChild *c, *next; + BdrvChild *c; Error *local_err = NULL; int ret = -EIO; + g_autoptr(GSList) updated_children = NULL; + GSList *p; bdrv_ref(top); bdrv_subtree_drained_begin(top); @@ -4926,14 +4928,6 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, goto exit; } - /* This function changes all links that point to top and makes - * them point to base. Check that none of them is frozen. */ - QLIST_FOREACH(c, &top->parents, next_parent) { - if (c->frozen) { - goto exit; - } - } - /* If 'base' recursively inherits from 'top' then we should set * base->inherits_from to top->inherits_from after 'top' and all * other intermediate nodes have been dropped. @@ -4950,36 +4944,36 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, backing_file_str = base->filename; } - QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) { - /* Check whether we are allowed to switch c from top to base */ - GSList *ignore_children = g_slist_prepend(NULL, c); - ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm, - ignore_children, NULL, &local_err); - g_slist_free(ignore_children); - if (ret < 0) { - error_report_err(local_err); - goto exit; - } + QLIST_FOREACH(c, &top->parents, next_parent) { + updated_children = g_slist_prepend(updated_children, c); + } + + bdrv_replace_node_common(top, base, false, &local_err); + if (local_err) { + error_report_err(local_err); + goto exit; + } + + for (p = updated_children; p; p = p->next) { + c = p->data; - /* If so, update the backing file path in the image file */ if (c->klass->update_filename) { ret = c->klass->update_filename(c, base, backing_file_str, &local_err); if (ret < 0) { - bdrv_abort_perm_update(base); + /* + * TODO: Actually, we want to rollback all previous iterations + * of this loop, and (which is almost impossible) previous + * bdrv_replace_node()... + * + * Note, that c->klass->update_filename may lead to permission + * update, so it's a bad idea to call it inside permission + * update transaction of bdrv_replace_node. + */ error_report_err(local_err); goto exit; } } - - /* - * Do the actual switch in the in-memory graph. - * Completes bdrv_check_update_perm() transaction internally. - * c->frozen is false, we have checked that above. - */ - bdrv_ref(base); - bdrv_replace_child(c, base); - bdrv_unref(top); } if (update_inherits_from) { -- 2.11.0