[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PULL 15/15] block: make bdrv_drop_intermediate() less wrong
From: |
Max Reitz |
Subject: |
[PULL 15/15] block: make bdrv_drop_intermediate() less wrong |
Date: |
Mon, 9 Nov 2020 18:38:39 +0100 |
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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>
---
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.28.0
- [PULL 05/15] hw/block/nvme: fix uint16_t use of uint32_t sgls member, (continued)
- [PULL 05/15] hw/block/nvme: fix uint16_t use of uint32_t sgls member, Max Reitz, 2020/11/09
- [PULL 06/15] hw/block/nvme: fix free of array-typed value, Max Reitz, 2020/11/09
- [PULL 08/15] iotests: rewrite iotest 240 in python, Max Reitz, 2020/11/09
- [PULL 07/15] iotests: add filter_qmp_virtio_scsi function, Max Reitz, 2020/11/09
- [PULL 09/15] block: Fixes nfs compiling error on msys2/mingw, Max Reitz, 2020/11/09
- [PULL 13/15] block: add forgotten bdrv_abort_perm_update() to bdrv_co_invalidate_cache(), Max Reitz, 2020/11/09
- [PULL 10/15] block: enable libnfs on msys2/mingw in cirrus.yml, Max Reitz, 2020/11/09
- [PULL 14/15] block: add bdrv_replace_node_common(), Max Reitz, 2020/11/09
- [PULL 12/15] block: Fix some code style problems, "foo* bar" should be "foo *bar", Max Reitz, 2020/11/09
- [PULL 11/15] block: Fix integer promotion error in bdrv_getlength(), Max Reitz, 2020/11/09
- [PULL 15/15] block: make bdrv_drop_intermediate() less wrong,
Max Reitz <=
- Re: [PULL 00/15] Block patches for 5.2.0-rc1, Max Reitz, 2020/11/09