motorhead/qemu
1
0
Fork 0
mirror of https://github.com/Motorhead1991/qemu.git synced 2025-08-16 14:41:55 -06:00
Code Issues Projects Releases Packages Wiki Activity Actions

block: make bdrv_drop_intermediate() less wrong

Browse source 
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 <vsementsov@virtuozzo.com>
Message-Id: <20201106124241.16950-4-vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
This commit is contained in:
Vladimir Sementsov-Ogievskiy 2020-11-06 15:42:37 +03:00 • committed by Max Reitz
parent 313274bbd4
commit d669ed6ab0
1 changed files with 24 additions and 30 deletions
Download patch file Download diff file Expand all files Collapse all files

54
block.c
View file

@ -4910,9 +4910,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
{ {
BlockDriverState *explicit_top = top; BlockDriverState *explicit_top = top;
bool update_inherits_from; bool update_inherits_from;
BdrvChild *c, *next; BdrvChild *c;
Error *local_err = NULL; Error *local_err = NULL;
int ret = -EIO; int ret = -EIO;
g_autoptr(GSList) updated_children = NULL;
GSList *p;
bdrv_ref(top); bdrv_ref(top);
bdrv_subtree_drained_begin(top); bdrv_subtree_drained_begin(top);
@ -4926,14 +4928,6 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
goto exit; 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 /* If 'base' recursively inherits from 'top' then we should set
* base->inherits_from to top->inherits_from after 'top' and all * base->inherits_from to top->inherits_from after 'top' and all
* other intermediate nodes have been dropped. * other intermediate nodes have been dropped.
@ -4950,36 +4944,36 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
backing_file_str = base->filename; backing_file_str = base->filename;
} }
QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) { QLIST_FOREACH(c, &top->parents, next_parent) {
/* Check whether we are allowed to switch c from top to base */ updated_children = g_slist_prepend(updated_children, c);
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); bdrv_replace_node_common(top, base, false, &local_err);
g_slist_free(ignore_children); if (local_err) {
if (ret < 0) { error_report_err(local_err);
error_report_err(local_err); goto exit;
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) { if (c->klass->update_filename) {
ret = c->klass->update_filename(c, base, backing_file_str, ret = c->klass->update_filename(c, base, backing_file_str,
&local_err); &local_err);
if (ret < 0) { 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); error_report_err(local_err);
goto exit; 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) { if (update_inherits_from) {