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() (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.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 36 +++++++++++++++---------------------
1 file changed, 15 insertions(+), 21 deletions(-)
diff --git a/block.c b/block.c
index 9538af4884..bd9f4e534b 100644
--- a/block.c
+++ b/block.c
@@ -4958,36 +4958,30 @@ 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;
- }
+ bdrv_replace_node(top, base, &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ goto exit;
+ }
- /* If so, update the backing file path in the image file */
+ QLIST_FOREACH_SAFE(c, &base->parents, next_parent, next) {
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.21.3
On 31.10.20 13:35, Vladimir Sementsov-Ogievskiy wrote: > 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. Indeed. Another thing that should be noted: bdrv_check_update_perm() doc: > Needs to be followed by a call to either bdrv_set_perm() > or bdrv_abort_perm_update(). And besides rolling back on error, we don’t commit on success either. > 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() (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. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block.c | 36 +++++++++++++++--------------------- > 1 file changed, 15 insertions(+), 21 deletions(-) And it’s also much simpler and clearer now. Reviewed-by: Max Reitz <mreitz@redhat.com>
On 05.11.20 14:22, Max Reitz wrote: > On 31.10.20 13:35, Vladimir Sementsov-Ogievskiy wrote: >> 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. [...] > And besides rolling back on error, we don’t commit on success either. (Oh, we do, through bdrv_replace_child(). Well.) Max
On Sat 31 Oct 2020 01:35:01 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> @@ -4958,36 +4958,30 @@ 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;
> - }
> + bdrv_replace_node(top, base, &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + goto exit;
> + }
At the beginning of this function there's a check for c->frozen. I think
you can remove it safely because you also have it in bdrv_replace_node()
Berto
On Sat 31 Oct 2020 01:35:01 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> - QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
/* ... */
> + QLIST_FOREACH_SAFE(c, &base->parents, next_parent, next) {
I also wonder, is top->parents and base->parents guaranteed to be the
same list in this case?
If not you could store the list of top->parents before calling
bdrv_replace_node() and use it afterwards.
QLIST_FOREACH(c, &top->parents, next_parent) {
parents = g_slist_prepend(parents, c);
}
Berto
05.11.2020 18:14, Alberto Garcia wrote:
> On Sat 31 Oct 2020 01:35:01 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> - QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
> /* ... */
>> + QLIST_FOREACH_SAFE(c, &base->parents, next_parent, next) {
>
> I also wonder, is top->parents and base->parents guaranteed to be the
> same list in this case?
>
> If not you could store the list of top->parents before calling
> bdrv_replace_node() and use it afterwards.
>
> QLIST_FOREACH(c, &top->parents, next_parent) {
> parents = g_slist_prepend(parents, c);
> }
>
> Berto
>
Hmm... We should not touch other parents of base, which it had prior to bdrv_replace_node().
On the other hand, it should be safe to call update_filename for not-updated parents..
And we should keep in mind that bdrv_replace_node may replace not all children. Still, in this
case we must replace all of them. Seems, we need additional argument for bdrv_replace_node()
to fail, if it want's to skip some children replacement.
So, I'm going to resend to add this parameter to bdrv_replace_node() and will use your
suggestion to be stricter on what we update, thanks!
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.