[PATCH v2 00/10] block: Attempt on fixing 030-reported errors

[PATCH v2 00/10] block: Attempt on fixing 030-reported errors

[Hanna Reitz]

Hi,

v1 cover letter:
https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg01287.html

In v2 I’ve addressed the comments I’ve received from Kevin and Vladimir.
To this end, I’ve retained only the non-controversial part in patch 5,
and split everything else (i.e. the stuff relating to
bdrv_replace_child_tran()) into the dedicated patches 6 and 8.

Kevin’s most important comments (to my understanding) were that:

(A) When bdrv_remove_file_or_backing_child() uses
    bdrv_replace_child_tran(), we have to ensure that the BDS lives as
    long as the transaction does.  This is solved by keeping a strong
    reference to it that’s released only when the transaction is cleaned
    up (and the new patch 7 ensures that the .clean() handler will be
    invoked after all .commit()/.abort() handlers, so the reference will
    really live as long as the transaction does).

(B) bdrv_replace_node_noperm() passes a pointer to loop-local variable,
    which is a really bad idea considering the transaction lives much
    longer than one loop iteration.
    Luckily, the new_bs it passes is always non-NULL, and so
    bdrv_replace_child_tran() actually doesn’t need to store the
    BdrvChild ** pointer, because for a non-NULL new_bs, there is
    nothing to revert in the abort handler.  We just need to clarify
    this, not store the pointer in case of a non-NULL new_bs, and assert
    that bdrv_replace_node_noperm() and its relatives are only used to
    replace an existing node by some other existing (i.e. non-NULL)
    node.


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/10:[----] [--] 'stream: Traverse graph after modification'
002/10:[0005] [FC] 'block: Manipulate children list in .attach/.detach'
003/10:[----] [--] 'block: Unite remove_empty_child and child_free'
004/10:[----] [--] 'block: Drop detached child from ignore list'
005/10:[0040] [FC] 'block: Pass BdrvChild ** to replace_child_noperm'
006/10:[down] 'block: Restructure remove_file_or_backing_child()'
007/10:[down] 'transactions: Invoke clean() after everything else'
008/10:[down] 'block: Let replace_child_tran keep indirect pointer'
009/10:[0020] [FC] 'block: Let replace_child_noperm free children'
010/10:[----] [--] 'iotests/030: Unthrottle parallel jobs in reverse'


Detailed per-patch changes:

Patch 2:
 - Dropped stale comment about undoing bdrv_attach_child_noperm()’s list
   insertion in the respective abort handler

Patch 5:
 - Split off everything related to bdrv_replace_child_tran()

Patch 6:
 - Added (split off from patch 5)
 - Keep the `if (!child) { return; }` path before getting childp

Patch 7:
 - Added: I wanted bdrv_remove_file_or_backing_child() to keep a strong
   BDS reference throughout the transaction, and the simplest way (i.e.
   the one where I wouldn’t have to think too hard) was to add this
   patch and have transactions invoke .clean() after all
   .commit()/.abort() handlers are done.  This way we can just drop the
   reference in .clean() and need not care about the order the
   transaction actions were added in.

Patch 8:
 - Added (split off from patch 5)
 - The BdrvChild ** pointer is now only stored in the
   BdrvReplaceChildState if new_bs is NULL.  Otherwise, we don’t need
   it, because we won’t need to reinstate the child in the abort
   handler.  This way we don’t have to worry about
   bdrv_replace_node_noperm() passing a pointer to a loop-local
   variable (because the new_bs it passes is never NULL).
 - In the same vein, assert that bdrv_replace_node() and relatives
   cannot be called to replace the node by NULL.
 - Have bdrv_remove_file_or_backing_child() keep a strong reference to
   the parent BDS throughout the transaction, so the &bs->backing or
   &bs->file pointer remains valid
 - Moved the comment explaining why we want to pass &s->child instead of
   s->childp to bdrv_replace_child_noperm() in
   bdrv_replace_child_abort() from patch 9 here (and also keep passing
   &s->child instead of s->childp).  It is already relevant now that
   s->childp is valid only if new_bs is NULL.

Patch 9:
 - The comment this used to add to bdrv_replace_child_abort() is now
   already added by patch 8, we just need to drop the TODO note there
 - Also drop the TODO note above bdrv_replace_child_tran()


Hanna Reitz (10):
  stream: Traverse graph after modification
  block: Manipulate children list in .attach/.detach
  block: Unite remove_empty_child and child_free
  block: Drop detached child from ignore list
  block: Pass BdrvChild ** to replace_child_noperm
  block: Restructure remove_file_or_backing_child()
  transactions: Invoke clean() after everything else
  block: Let replace_child_tran keep indirect pointer
  block: Let replace_child_noperm free children
  iotests/030: Unthrottle parallel jobs in reverse

 include/qemu/transactions.h |   3 +
 block.c                     | 233 +++++++++++++++++++++++++++---------
 block/stream.c              |   7 +-
 util/transactions.c         |   8 +-
 tests/qemu-iotests/030      |  11 +-
 5 files changed, 201 insertions(+), 61 deletions(-)

-- 
2.33.1

[PATCH v2 01/10] stream: Traverse graph after modification

[Hanna Reitz]

bdrv_cor_filter_drop() modifies the block graph.  That means that other
parties can also modify the block graph before it returns.  Therefore,
we cannot assume that the result of a graph traversal we did before
remains valid afterwards.

We should thus fetch `base` and `unfiltered_base` afterwards instead of
before.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/stream.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 97bee482dc..e45113aed6 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -54,8 +54,8 @@ static int stream_prepare(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
-    BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
-    BlockDriverState *unfiltered_base = bdrv_skip_filters(base);
+    BlockDriverState *base;
+    BlockDriverState *unfiltered_base;
     Error *local_err = NULL;
     int ret = 0;
 
@@ -63,6 +63,9 @@ static int stream_prepare(Job *job)
     bdrv_cor_filter_drop(s->cor_filter_bs);
     s->cor_filter_bs = NULL;
 
+    base = bdrv_filter_or_cow_bs(s->above_base);
+    unfiltered_base = bdrv_skip_filters(base);
+
     if (bdrv_cow_child(unfiltered_bs)) {
         const char *base_id = NULL, *base_fmt = NULL;
         if (unfiltered_base) {
-- 
2.33.1

[PATCH v2 02/10] block: Manipulate children list in .attach/.detach

[Hanna Reitz]

The children list is specific to BDS parents.  We should not modify it
in the general children modification code, but let BDS parents deal with
it in their .attach() and .detach() methods.

This also has the advantage that a BdrvChild is removed from the
children list before its .bs pointer can become NULL.  BDS parents
generally assume that their children's .bs pointer is never NULL, so
this is actually a bug fix.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 580cb77a70..ca024ffced 100644
--- a/block.c
+++ b/block.c
@@ -1387,6 +1387,8 @@ static void bdrv_child_cb_attach(BdrvChild *child)
 {
     BlockDriverState *bs = child->opaque;
 
+    QLIST_INSERT_HEAD(&bs->children, child, next);
+
     if (child->role & BDRV_CHILD_COW) {
         bdrv_backing_attach(child);
     }
@@ -1403,6 +1405,8 @@ static void bdrv_child_cb_detach(BdrvChild *child)
     }
 
     bdrv_unapply_subtree_drain(child, bs);
+
+    QLIST_REMOVE(child, next);
 }
 
 static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
@@ -2747,7 +2751,7 @@ static void bdrv_child_free(void *opaque)
 static void bdrv_remove_empty_child(BdrvChild *child)
 {
     assert(!child->bs);
-    QLIST_SAFE_REMOVE(child, next);
+    assert(!child->next.le_prev); /* not in children list */
     bdrv_child_free(child);
 }
 
@@ -2913,12 +2917,6 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
         return ret;
     }
 
-    QLIST_INSERT_HEAD(&parent_bs->children, *child, next);
-    /*
-     * child is removed in bdrv_attach_child_common_abort(), so don't care to
-     * abort this change separately.
-     */
-
     return 0;
 }
 
@@ -4851,7 +4849,6 @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque)
     BdrvRemoveFilterOrCowChild *s = opaque;
     BlockDriverState *parent_bs = s->child->opaque;
 
-    QLIST_INSERT_HEAD(&parent_bs->children, s->child, next);
     if (s->is_backing) {
         parent_bs->backing = s->child;
     } else {
@@ -4906,7 +4903,6 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
     };
     tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
 
-    QLIST_SAFE_REMOVE(child, next);
     if (s->is_backing) {
         bs->backing = NULL;
     } else {
-- 
2.33.1

[PATCH v2 03/10] block: Unite remove_empty_child and child_free

[Hanna Reitz]

Now that bdrv_remove_empty_child() no longer removes the child from the
parent's children list but only checks that it is not in such a list, it
is only a wrapper around bdrv_child_free() that checks that the child is
empty and unused.  That should apply to all children that we free, so
put those checks into bdrv_child_free() and drop
bdrv_remove_empty_child().

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index ca024ffced..19bff4f95c 100644
--- a/block.c
+++ b/block.c
@@ -2740,19 +2740,19 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
     }
 }
 
-static void bdrv_child_free(void *opaque)
-{
-    BdrvChild *c = opaque;
-
-    g_free(c->name);
-    g_free(c);
-}
-
-static void bdrv_remove_empty_child(BdrvChild *child)
+/**
+ * Free the given @child.
+ *
+ * The child must be empty (i.e. `child->bs == NULL`) and it must be
+ * unused (i.e. not in a children list).
+ */
+static void bdrv_child_free(BdrvChild *child)
 {
     assert(!child->bs);
     assert(!child->next.le_prev); /* not in children list */
-    bdrv_child_free(child);
+
+    g_free(child->name);
+    g_free(child);
 }
 
 typedef struct BdrvAttachChildCommonState {
@@ -2786,7 +2786,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
     }
 
     bdrv_unref(bs);
-    bdrv_remove_empty_child(child);
+    bdrv_child_free(child);
     *s->child = NULL;
 }
 
@@ -2859,7 +2859,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
 
         if (ret < 0) {
             error_propagate(errp, local_err);
-            bdrv_remove_empty_child(new_child);
+            bdrv_child_free(new_child);
             return ret;
         }
     }
@@ -2925,7 +2925,7 @@ static void bdrv_detach_child(BdrvChild *child)
     BlockDriverState *old_bs = child->bs;
 
     bdrv_replace_child_noperm(child, NULL);
-    bdrv_remove_empty_child(child);
+    bdrv_child_free(child);
 
     if (old_bs) {
         /*
-- 
2.33.1

[PATCH v2 04/10] block: Drop detached child from ignore list

[Hanna Reitz]

bdrv_attach_child_common_abort() restores the parent's AioContext.  To
do so, the child (which was supposed to be attached, but is now detached
again by this abort handler) is added to the ignore list for the
AioContext changing functions.

However, since we modify a BDS's children list in the BdrvChildClass's
.attach and .detach handlers, the child is already effectively detached
from the parent by this point.  We do not need to put it into the ignore
list.

Use this opportunity to clean up the empty line structure: Keep setting
the ignore list, invoking the AioContext function, and freeing the
ignore list in blocks separated by empty lines.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 19bff4f95c..c7d5aa5254 100644
--- a/block.c
+++ b/block.c
@@ -2774,14 +2774,16 @@ static void bdrv_attach_child_common_abort(void *opaque)
     }
 
     if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
-        GSList *ignore = g_slist_prepend(NULL, child);
+        GSList *ignore;
 
+        /* No need to ignore `child`, because it has been detached already */
+        ignore = NULL;
         child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore,
                                       &error_abort);
         g_slist_free(ignore);
-        ignore = g_slist_prepend(NULL, child);
-        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
 
+        ignore = NULL;
+        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
         g_slist_free(ignore);
     }
 
-- 
2.33.1

[PATCH v2 05/10] block: Pass BdrvChild ** to replace_child_noperm

[Hanna Reitz]

bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially
set it to NULL.  That is dangerous, because BDS parents generally assume
that their children's .bs pointer is never NULL.  We therefore want to
let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer
to NULL, too.

This patch lays the foundation for it by passing a BdrvChild ** pointer
to bdrv_replace_child_noperm() so that it can later use it to NULL the
BdrvChild pointer immediately after setting BdrvChild.bs to NULL.

(We will still need to undertake some intermediate steps, though.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index c7d5aa5254..d668156eca 100644
--- a/block.c
+++ b/block.c
@@ -87,7 +87,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
 static bool bdrv_recurse_has_child(BlockDriverState *bs,
                                    BlockDriverState *child);
 
-static void bdrv_replace_child_noperm(BdrvChild *child,
+static void bdrv_replace_child_noperm(BdrvChild **child,
                                       BlockDriverState *new_bs);
 static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
                                               BdrvChild *child,
@@ -2270,7 +2270,7 @@ static void bdrv_replace_child_abort(void *opaque)
     BlockDriverState *new_bs = s->child->bs;
 
     /* old_bs reference is transparently moved from @s to @s->child */
-    bdrv_replace_child_noperm(s->child, s->old_bs);
+    bdrv_replace_child_noperm(&s->child, s->old_bs);
     bdrv_unref(new_bs);
 }
 
@@ -2300,7 +2300,7 @@ static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
     if (new_bs) {
         bdrv_ref(new_bs);
     }
-    bdrv_replace_child_noperm(child, new_bs);
+    bdrv_replace_child_noperm(&child, new_bs);
     /* old_bs reference is transparently moved from @child to @s */
 }
 
@@ -2672,9 +2672,10 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
     return permissions[qapi_perm];
 }
 
-static void bdrv_replace_child_noperm(BdrvChild *child,
+static void bdrv_replace_child_noperm(BdrvChild **childp,
                                       BlockDriverState *new_bs)
 {
+    BdrvChild *child = *childp;
     BlockDriverState *old_bs = child->bs;
     int new_bs_quiesce_counter;
     int drain_saldo;
@@ -2767,7 +2768,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
     BdrvChild *child = *s->child;
     BlockDriverState *bs = child->bs;
 
-    bdrv_replace_child_noperm(child, NULL);
+    bdrv_replace_child_noperm(s->child, NULL);
 
     if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
         bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
@@ -2867,7 +2868,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
     }
 
     bdrv_ref(child_bs);
-    bdrv_replace_child_noperm(new_child, child_bs);
+    bdrv_replace_child_noperm(&new_child, child_bs);
 
     *child = new_child;
 
@@ -2922,12 +2923,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
     return 0;
 }
 
-static void bdrv_detach_child(BdrvChild *child)
+static void bdrv_detach_child(BdrvChild **childp)
 {
-    BlockDriverState *old_bs = child->bs;
+    BlockDriverState *old_bs = (*childp)->bs;
 
-    bdrv_replace_child_noperm(child, NULL);
-    bdrv_child_free(child);
+    bdrv_replace_child_noperm(childp, NULL);
+    bdrv_child_free(*childp);
 
     if (old_bs) {
         /*
@@ -3033,7 +3034,7 @@ void bdrv_root_unref_child(BdrvChild *child)
     BlockDriverState *child_bs;
 
     child_bs = child->bs;
-    bdrv_detach_child(child);
+    bdrv_detach_child(&child);
     bdrv_unref(child_bs);
 }
 
-- 
2.33.1

[PATCH v2 06/10] block: Restructure remove_file_or_backing_child()

[Hanna Reitz]

As of a future patch, bdrv_replace_child_tran() will take a BdrvChild **
pointer.  Prepare for that by getting such a pointer and using it where
applicable, and (dereferenced) as a parameter for
bdrv_replace_child_tran().

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index d668156eca..8da057f800 100644
--- a/block.c
+++ b/block.c
@@ -4887,30 +4887,33 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
                                               BdrvChild *child,
                                               Transaction *tran)
 {
+    BdrvChild **childp;
     BdrvRemoveFilterOrCowChild *s;
 
-    assert(child == bs->backing || child == bs->file);
-
     if (!child) {
         return;
     }
 
+    if (child == bs->backing) {
+        childp = &bs->backing;
+    } else if (child == bs->file) {
+        childp = &bs->file;
+    } else {
+        g_assert_not_reached();
+    }
+
     if (child->bs) {
-        bdrv_replace_child_tran(child, NULL, tran);
+        bdrv_replace_child_tran(*childp, NULL, tran);
     }
 
     s = g_new(BdrvRemoveFilterOrCowChild, 1);
     *s = (BdrvRemoveFilterOrCowChild) {
         .child = child,
-        .is_backing = (child == bs->backing),
+        .is_backing = (childp == &bs->backing),
     };
     tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
 
-    if (s->is_backing) {
-        bs->backing = NULL;
-    } else {
-        bs->file = NULL;
-    }
+    *childp = NULL;
 }
 
 /*
-- 
2.33.1

[PATCH v2 07/10] transactions: Invoke clean() after everything else

[Hanna Reitz]

Invoke the transaction drivers' .clean() methods only after all
.commit() or .abort() handlers are done.

This makes it easier to have nested transactions where the top-level
transactions pass objects to lower transactions that the latter can
still use throughout their commit/abort phases, while the top-level
transaction keeps a reference that is released in its .clean() method.

(Before this commit, that is also possible, but the top-level
transaction would need to take care to invoke tran_add() before the
lower-level transaction does.  This commit makes the ordering
irrelevant, which is just a bit nicer.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 include/qemu/transactions.h | 3 +++
 util/transactions.c         | 8 ++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h
index 92c5965235..2f2060acd9 100644
--- a/include/qemu/transactions.h
+++ b/include/qemu/transactions.h
@@ -31,6 +31,9 @@
  * tran_create(), call your "prepare" functions on it, and finally call
  * tran_abort() or tran_commit() to finalize the transaction by corresponding
  * finalization actions in reverse order.
+ *
+ * The clean() functions registered by the drivers in a transaction are called
+ * last, after all abort() or commit() functions have been called.
  */
 
 #ifndef QEMU_TRANSACTIONS_H
diff --git a/util/transactions.c b/util/transactions.c
index d0bc9a3e73..2dbdedce95 100644
--- a/util/transactions.c
+++ b/util/transactions.c
@@ -61,11 +61,13 @@ void tran_abort(Transaction *tran)
 {
     TransactionAction *act, *next;
 
-    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
+    QSLIST_FOREACH(act, &tran->actions, entry) {
         if (act->drv->abort) {
             act->drv->abort(act->opaque);
         }
+    }
 
+    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
         if (act->drv->clean) {
             act->drv->clean(act->opaque);
         }
@@ -80,11 +82,13 @@ void tran_commit(Transaction *tran)
 {
     TransactionAction *act, *next;
 
-    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
+    QSLIST_FOREACH(act, &tran->actions, entry) {
         if (act->drv->commit) {
             act->drv->commit(act->opaque);
         }
+    }
 
+    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
         if (act->drv->clean) {
             act->drv->clean(act->opaque);
         }
-- 
2.33.1

[PATCH v2 08/10] block: Let replace_child_tran keep indirect pointer

[Hanna Reitz]

As of a future commit, bdrv_replace_child_noperm() will clear the
indirect BdrvChild pointer passed to it if the new child BDS is NULL.
bdrv_replace_child_tran() will want to let it do that, but revert this
change in its abort handler.  For that, we need to have it receive a
BdrvChild ** pointer, too, and keep it stored in the
BdrvReplaceChildState object that we attach to the transaction.

Note that we do not need to store it in the BdrvReplaceChildState when
new_bs is not NULL, because then there is nothing to revert.  This is
important so that bdrv_replace_node_noperm() can pass a pointer to a
loop-local variable to bdrv_replace_child_tran() without worrying that
this pointer will outlive one loop iteration.

(Of course, for that to work, bdrv_replace_node_noperm() and in turn
bdrv_replace_node() and its relatives may not be called with a NULL @to
node.  Luckily, they already are not, but now we should assert this.)

bdrv_remove_file_or_backing_child() on the other hand needs to ensure
that the indirect pointer it passes will stay valid for the duration of
the transaction.  Ensure this by keeping a strong reference to the BDS
whose &bs->backing or &bs->file it passes to bdrv_replace_child_tran(),
and giving up that reference only in the transaction .clean() handler.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 73 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 8da057f800..a40027161c 100644
--- a/block.c
+++ b/block.c
@@ -2254,6 +2254,7 @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm,
 
 typedef struct BdrvReplaceChildState {
     BdrvChild *child;
+    BdrvChild **childp;
     BlockDriverState *old_bs;
 } BdrvReplaceChildState;
 
@@ -2269,7 +2270,29 @@ static void bdrv_replace_child_abort(void *opaque)
     BdrvReplaceChildState *s = opaque;
     BlockDriverState *new_bs = s->child->bs;
 
-    /* old_bs reference is transparently moved from @s to @s->child */
+    /*
+     * old_bs reference is transparently moved from @s to s->child.
+     *
+     * Pass &s->child here instead of s->childp, because:
+     * (1) s->old_bs must be non-NULL, so bdrv_replace_child_noperm() will not
+     *     modify the BdrvChild * pointer we indirectly pass to it, i.e. it
+     *     will not modify s->child.  From that perspective, it does not matter
+     *     whether we pass s->childp or &s->child.
+     *     (TODO: Right now, bdrv_replace_child_noperm() never modifies that
+     *     pointer anyway (though it will in the future), so at this point it
+     *     absolutely does not matter whether we pass s->childp or &s->child.)
+     * (2) If new_bs is not NULL, s->childp will be NULL.  We then cannot use
+     *     it here.
+     * (3) If new_bs is NULL, *s->childp will have been NULLed by
+     *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
+     *     must not pass a NULL *s->childp here.
+     *     (TODO: In its current state, bdrv_replace_child_noperm() will not
+     *     have NULLed *s->childp, so this does not apply yet.  It will in the
+     *     future.)
+     *
+     * So whether new_bs was NULL or not, we cannot pass s->childp here; and in
+     * any case, there is no reason to pass it anyway.
+     */
     bdrv_replace_child_noperm(&s->child, s->old_bs);
     bdrv_unref(new_bs);
 }
@@ -2286,22 +2309,32 @@ static TransactionActionDrv bdrv_replace_child_drv = {
  * Note: real unref of old_bs is done only on commit.
  *
  * The function doesn't update permissions, caller is responsible for this.
+ *
+ * Note that if new_bs == NULL, @childp is stored in a state object attached
+ * to @tran, so that the old child can be reinstated in the abort handler.
+ * Therefore, if @new_bs can be NULL, @childp must stay valid until the
+ * transaction is committed or aborted.
+ *
+ * (TODO: The reinstating does not happen yet, but it will once
+ * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
  */
-static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
+static void bdrv_replace_child_tran(BdrvChild **childp,
+                                    BlockDriverState *new_bs,
                                     Transaction *tran)
 {
     BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
     *s = (BdrvReplaceChildState) {
-        .child = child,
-        .old_bs = child->bs,
+        .child = *childp,
+        .childp = new_bs == NULL ? childp : NULL,
+        .old_bs = (*childp)->bs,
     };
     tran_add(tran, &bdrv_replace_child_drv, s);
 
     if (new_bs) {
         bdrv_ref(new_bs);
     }
-    bdrv_replace_child_noperm(&child, new_bs);
-    /* old_bs reference is transparently moved from @child to @s */
+    bdrv_replace_child_noperm(childp, new_bs);
+    /* old_bs reference is transparently moved from *childp to @s */
 }
 
 /*
@@ -4844,6 +4877,7 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
 
 typedef struct BdrvRemoveFilterOrCowChild {
     BdrvChild *child;
+    BlockDriverState *bs;
     bool is_backing;
 } BdrvRemoveFilterOrCowChild;
 
@@ -4873,10 +4907,19 @@ static void bdrv_remove_filter_or_cow_child_commit(void *opaque)
     bdrv_child_free(s->child);
 }
 
+static void bdrv_remove_filter_or_cow_child_clean(void *opaque)
+{
+    BdrvRemoveFilterOrCowChild *s = opaque;
+
+    /* Drop the bs reference after the transaction is done */
+    bdrv_unref(s->bs);
+    g_free(s);
+}
+
 static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = {
     .abort = bdrv_remove_filter_or_cow_child_abort,
     .commit = bdrv_remove_filter_or_cow_child_commit,
-    .clean = g_free,
+    .clean = bdrv_remove_filter_or_cow_child_clean,
 };
 
 /*
@@ -4894,6 +4937,11 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
         return;
     }
 
+    /*
+     * Keep a reference to @bs so @childp will stay valid throughout the
+     * transaction (required by bdrv_replace_child_tran())
+     */
+    bdrv_ref(bs);
     if (child == bs->backing) {
         childp = &bs->backing;
     } else if (child == bs->file) {
@@ -4903,12 +4951,13 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
     }
 
     if (child->bs) {
-        bdrv_replace_child_tran(*childp, NULL, tran);
+        bdrv_replace_child_tran(childp, NULL, tran);
     }
 
     s = g_new(BdrvRemoveFilterOrCowChild, 1);
     *s = (BdrvRemoveFilterOrCowChild) {
         .child = child,
+        .bs = bs,
         .is_backing = (childp == &bs->backing),
     };
     tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
@@ -4934,6 +4983,8 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
 {
     BdrvChild *c, *next;
 
+    assert(to != NULL);
+
     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
         assert(c->bs == from);
         if (!should_update_child(c, to)) {
@@ -4949,7 +5000,12 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
                        c->name, from->node_name);
             return -EPERM;
         }
-        bdrv_replace_child_tran(c, to, tran);
+
+        /*
+         * Passing a pointer to the local variable @c is fine here, because
+         * @to is not NULL, and so &c will not be attached to the transaction.
+         */
+        bdrv_replace_child_tran(&c, to, tran);
     }
 
     return 0;
@@ -4964,6 +5020,8 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
  *
  * With @detach_subchain=true @to must be in a backing chain of @from. In this
  * case backing link of the cow-parent of @to is removed.
+ *
+ * @to must not be NULL.
  */
 static int bdrv_replace_node_common(BlockDriverState *from,
                                     BlockDriverState *to,
@@ -4976,6 +5034,8 @@ static int bdrv_replace_node_common(BlockDriverState *from,
     BlockDriverState *to_cow_parent = NULL;
     int ret;
 
+    assert(to != NULL);
+
     if (detach_subchain) {
         assert(bdrv_chain_contains(from, to));
         assert(from != to);
@@ -5031,6 +5091,9 @@ out:
     return ret;
 }
 
+/**
+ * Replace node @from by @to (where neither may be NULL).
+ */
 int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
                       Error **errp)
 {
@@ -5098,7 +5161,7 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
     bdrv_drained_begin(old_bs);
     bdrv_drained_begin(new_bs);
 
-    bdrv_replace_child_tran(child, new_bs, tran);
+    bdrv_replace_child_tran(&child, new_bs, tran);
 
     found = g_hash_table_new(NULL, NULL);
     refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
-- 
2.33.1

[PATCH v2 09/10] block: Let replace_child_noperm free children

[Hanna Reitz]

In most of the block layer, especially when traversing down from other
BlockDriverStates, we assume that BdrvChild.bs can never be NULL.  When
it becomes NULL, it is expected that the corresponding BdrvChild pointer
also becomes NULL and the BdrvChild object is freed.

Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs
pointer to NULL, it should also immediately set the corresponding
BdrvChild pointer (like bs->file or bs->backing) to NULL.

In that context, it also makes sense for this function to free the
child.  Sometimes we cannot do so, though, because it is called in a
transactional context where the caller might still want to reinstate the
child in the abort branch (and free it only on commit), so this behavior
has to remain optional.

In bdrv_replace_child_tran()'s abort handler, we now rely on the fact
that the BdrvChild passed to bdrv_replace_child_tran() must have had a
non-NULL .bs pointer initially.  Make a note of that and assert it.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 102 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 79 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index a40027161c..0ac5b163d2 100644
--- a/block.c
+++ b/block.c
@@ -87,8 +87,10 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
 static bool bdrv_recurse_has_child(BlockDriverState *bs,
                                    BlockDriverState *child);
 
+static void bdrv_child_free(BdrvChild *child);
 static void bdrv_replace_child_noperm(BdrvChild **child,
-                                      BlockDriverState *new_bs);
+                                      BlockDriverState *new_bs,
+                                      bool free_empty_child);
 static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
                                               BdrvChild *child,
                                               Transaction *tran);
@@ -2256,12 +2258,16 @@ typedef struct BdrvReplaceChildState {
     BdrvChild *child;
     BdrvChild **childp;
     BlockDriverState *old_bs;
+    bool free_empty_child;
 } BdrvReplaceChildState;
 
 static void bdrv_replace_child_commit(void *opaque)
 {
     BdrvReplaceChildState *s = opaque;
 
+    if (s->free_empty_child && !s->child->bs) {
+        bdrv_child_free(s->child);
+    }
     bdrv_unref(s->old_bs);
 }
 
@@ -2278,22 +2284,26 @@ static void bdrv_replace_child_abort(void *opaque)
      *     modify the BdrvChild * pointer we indirectly pass to it, i.e. it
      *     will not modify s->child.  From that perspective, it does not matter
      *     whether we pass s->childp or &s->child.
-     *     (TODO: Right now, bdrv_replace_child_noperm() never modifies that
-     *     pointer anyway (though it will in the future), so at this point it
-     *     absolutely does not matter whether we pass s->childp or &s->child.)
      * (2) If new_bs is not NULL, s->childp will be NULL.  We then cannot use
      *     it here.
      * (3) If new_bs is NULL, *s->childp will have been NULLed by
      *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
      *     must not pass a NULL *s->childp here.
-     *     (TODO: In its current state, bdrv_replace_child_noperm() will not
-     *     have NULLed *s->childp, so this does not apply yet.  It will in the
-     *     future.)
      *
      * So whether new_bs was NULL or not, we cannot pass s->childp here; and in
      * any case, there is no reason to pass it anyway.
      */
-    bdrv_replace_child_noperm(&s->child, s->old_bs);
+    bdrv_replace_child_noperm(&s->child, s->old_bs, true);
+    /*
+     * The child was pre-existing, so s->old_bs must be non-NULL, and
+     * s->child thus must not have been freed
+     */
+    assert(s->child != NULL);
+    if (!new_bs) {
+        /* As described above, *s->childp was cleared, so restore it */
+        assert(s->childp != NULL);
+        *s->childp = s->child;
+    }
     bdrv_unref(new_bs);
 }
 
@@ -2310,30 +2320,44 @@ static TransactionActionDrv bdrv_replace_child_drv = {
  *
  * The function doesn't update permissions, caller is responsible for this.
  *
+ * (*childp)->bs must not be NULL.
+ *
  * Note that if new_bs == NULL, @childp is stored in a state object attached
  * to @tran, so that the old child can be reinstated in the abort handler.
  * Therefore, if @new_bs can be NULL, @childp must stay valid until the
  * transaction is committed or aborted.
  *
- * (TODO: The reinstating does not happen yet, but it will once
- * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
+ * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
+ * freed (on commit).  @free_empty_child should only be false if the
+ * caller will free the BDrvChild themselves (which may be important
+ * if this is in turn called in another transactional context).
  */
 static void bdrv_replace_child_tran(BdrvChild **childp,
                                     BlockDriverState *new_bs,
-                                    Transaction *tran)
+                                    Transaction *tran,
+                                    bool free_empty_child)
 {
     BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
     *s = (BdrvReplaceChildState) {
         .child = *childp,
         .childp = new_bs == NULL ? childp : NULL,
         .old_bs = (*childp)->bs,
+        .free_empty_child = free_empty_child,
     };
     tran_add(tran, &bdrv_replace_child_drv, s);
 
+    /* The abort handler relies on this */
+    assert(s->old_bs != NULL);
+
     if (new_bs) {
         bdrv_ref(new_bs);
     }
-    bdrv_replace_child_noperm(childp, new_bs);
+    /*
+     * Pass free_empty_child=false, we will free the child (if
+     * necessary) in bdrv_replace_child_commit() (if our
+     * @free_empty_child parameter was true).
+     */
+    bdrv_replace_child_noperm(childp, new_bs, false);
     /* old_bs reference is transparently moved from *childp to @s */
 }
 
@@ -2705,8 +2729,22 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
     return permissions[qapi_perm];
 }
 
+/**
+ * Replace (*childp)->bs by @new_bs.
+ *
+ * If @new_bs is NULL, *childp will be set to NULL, too: BDS parents
+ * generally cannot handle a BdrvChild with .bs == NULL, so clearing
+ * BdrvChild.bs should generally immediately be followed by the
+ * BdrvChild pointer being cleared as well.
+ *
+ * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
+ * freed.  @free_empty_child should only be false if the caller will
+ * free the BdrvChild themselves (this may be important in a
+ * transactional context, where it may only be freed on commit).
+ */
 static void bdrv_replace_child_noperm(BdrvChild **childp,
-                                      BlockDriverState *new_bs)
+                                      BlockDriverState *new_bs,
+                                      bool free_empty_child)
 {
     BdrvChild *child = *childp;
     BlockDriverState *old_bs = child->bs;
@@ -2743,6 +2781,9 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
     }
 
     child->bs = new_bs;
+    if (!new_bs) {
+        *childp = NULL;
+    }
 
     if (new_bs) {
         QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
@@ -2772,6 +2813,10 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
         bdrv_parent_drained_end_single(child);
         drain_saldo++;
     }
+
+    if (free_empty_child && !child->bs) {
+        bdrv_child_free(child);
+    }
 }
 
 /**
@@ -2801,7 +2846,14 @@ static void bdrv_attach_child_common_abort(void *opaque)
     BdrvChild *child = *s->child;
     BlockDriverState *bs = child->bs;
 
-    bdrv_replace_child_noperm(s->child, NULL);
+    /*
+     * Pass free_empty_child=false, because we still need the child
+     * for the AioContext operations on the parent below; those
+     * BdrvChildClass methods all work on a BdrvChild object, so we
+     * need to keep it as an empty shell (after this function, it will
+     * not be attached to any parent, and it will not have a .bs).
+     */
+    bdrv_replace_child_noperm(s->child, NULL, false);
 
     if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
         bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
@@ -2823,7 +2875,6 @@ static void bdrv_attach_child_common_abort(void *opaque)
 
     bdrv_unref(bs);
     bdrv_child_free(child);
-    *s->child = NULL;
 }
 
 static TransactionActionDrv bdrv_attach_child_common_drv = {
@@ -2901,7 +2952,9 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
     }
 
     bdrv_ref(child_bs);
-    bdrv_replace_child_noperm(&new_child, child_bs);
+    bdrv_replace_child_noperm(&new_child, child_bs, true);
+    /* child_bs was non-NULL, so new_child must not have been freed */
+    assert(new_child != NULL);
 
     *child = new_child;
 
@@ -2960,8 +3013,7 @@ static void bdrv_detach_child(BdrvChild **childp)
 {
     BlockDriverState *old_bs = (*childp)->bs;
 
-    bdrv_replace_child_noperm(childp, NULL);
-    bdrv_child_free(*childp);
+    bdrv_replace_child_noperm(childp, NULL, true);
 
     if (old_bs) {
         /*
@@ -4951,7 +5003,11 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
     }
 
     if (child->bs) {
-        bdrv_replace_child_tran(childp, NULL, tran);
+        /*
+         * Pass free_empty_child=false, we will free the child in
+         * bdrv_remove_filter_or_cow_child_commit()
+         */
+        bdrv_replace_child_tran(childp, NULL, tran, false);
     }
 
     s = g_new(BdrvRemoveFilterOrCowChild, 1);
@@ -4961,8 +5017,6 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
         .is_backing = (childp == &bs->backing),
     };
     tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
-
-    *childp = NULL;
 }
 
 /*
@@ -5005,7 +5059,7 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
          * Passing a pointer to the local variable @c is fine here, because
          * @to is not NULL, and so &c will not be attached to the transaction.
          */
-        bdrv_replace_child_tran(&c, to, tran);
+        bdrv_replace_child_tran(&c, to, tran, true);
     }
 
     return 0;
@@ -5161,7 +5215,9 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
     bdrv_drained_begin(old_bs);
     bdrv_drained_begin(new_bs);
 
-    bdrv_replace_child_tran(&child, new_bs, tran);
+    bdrv_replace_child_tran(&child, new_bs, tran, true);
+    /* @new_bs must have been non-NULL, so @child must not have been freed */
+    assert(child != NULL);
 
     found = g_hash_table_new(NULL, NULL);
     refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
-- 
2.33.1

[PATCH v2 10/10] iotests/030: Unthrottle parallel jobs in reverse

[Hanna Reitz]

See the comment for why this is necessary.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/030 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 5fb65b4bef..567bf1da67 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -251,7 +251,16 @@ class TestParallelOps(iotests.QMPTestCase):
                                  speed=1024)
             self.assert_qmp(result, 'return', {})
 
-        for job in pending_jobs:
+        # Do this in reverse: After unthrottling them, some jobs may finish
+        # before we have unthrottled all of them.  This will drain their
+        # subgraph, and this will make jobs above them advance (despite those
+        # jobs on top being throttled).  In the worst case, all jobs below the
+        # top one are finished before we can unthrottle it, and this makes it
+        # advance so far that it completes before we can unthrottle it - which
+        # results in an error.
+        # Starting from the top (i.e. in reverse) does not have this problem:
+        # When a job finishes, the ones below it are not advanced.
+        for job in reversed(pending_jobs):
             result = self.vm.qmp('block-job-set-speed', device=job, speed=0)
             self.assert_qmp(result, 'return', {})
 
-- 
2.33.1

Re: [PATCH v2 00/10] block: Attempt on fixing 030-reported errors

[Kevin Wolf]

Am 11.11.2021 um 13:08 hat Hanna Reitz geschrieben:
> Hi,
> 
> v1 cover letter:
> https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg01287.html
> 
> In v2 I’ve addressed the comments I’ve received from Kevin and Vladimir.
> To this end, I’ve retained only the non-controversial part in patch 5,
> and split everything else (i.e. the stuff relating to
> bdrv_replace_child_tran()) into the dedicated patches 6 and 8.
> 
> Kevin’s most important comments (to my understanding) were that:
> 
> (A) When bdrv_remove_file_or_backing_child() uses
>     bdrv_replace_child_tran(), we have to ensure that the BDS lives as
>     long as the transaction does.  This is solved by keeping a strong
>     reference to it that’s released only when the transaction is cleaned
>     up (and the new patch 7 ensures that the .clean() handler will be
>     invoked after all .commit()/.abort() handlers, so the reference will
>     really live as long as the transaction does).
> 
> (B) bdrv_replace_node_noperm() passes a pointer to loop-local variable,
>     which is a really bad idea considering the transaction lives much
>     longer than one loop iteration.
>     Luckily, the new_bs it passes is always non-NULL, and so
>     bdrv_replace_child_tran() actually doesn’t need to store the
>     BdrvChild ** pointer, because for a non-NULL new_bs, there is
>     nothing to revert in the abort handler.  We just need to clarify
>     this, not store the pointer in case of a non-NULL new_bs, and assert
>     that bdrv_replace_node_noperm() and its relatives are only used to
>     replace an existing node by some other existing (i.e. non-NULL)
>     node.

Thanks, applied to the block branch.

Kevin

Re: [PATCH v2 05/10] block: Pass BdrvChild ** to replace_child_noperm

[Vladimir Sementsov-Ogievskiy]

11.11.2021 15:08, Hanna Reitz wrote:
> bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially
> set it to NULL.  That is dangerous, because BDS parents generally assume
> that their children's .bs pointer is never NULL.  We therefore want to
> let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer
> to NULL, too.
> 
> This patch lays the foundation for it by passing a BdrvChild ** pointer
> to bdrv_replace_child_noperm() so that it can later use it to NULL the
> BdrvChild pointer immediately after setting BdrvChild.bs to NULL.
> 
> (We will still need to undertake some intermediate steps, though.)
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Series already applied, but I still feel myself responsible to track how transactions changed:)

Don't bother with applying my r-b marks into applied series.

> ---
>   block.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c7d5aa5254..d668156eca 100644
> --- a/block.c
> +++ b/block.c
> @@ -87,7 +87,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>   static bool bdrv_recurse_has_child(BlockDriverState *bs,
>                                      BlockDriverState *child);
>   
> -static void bdrv_replace_child_noperm(BdrvChild *child,
> +static void bdrv_replace_child_noperm(BdrvChild **child,
>                                         BlockDriverState *new_bs);
>   static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
>                                                 BdrvChild *child,
> @@ -2270,7 +2270,7 @@ static void bdrv_replace_child_abort(void *opaque)
>       BlockDriverState *new_bs = s->child->bs;
>   
>       /* old_bs reference is transparently moved from @s to @s->child */
> -    bdrv_replace_child_noperm(s->child, s->old_bs);
> +    bdrv_replace_child_noperm(&s->child, s->old_bs);

  - no sense / no harm in  clearing the pointer, as it's a field in transaction state struct, and should not be used after abort
  - hard to say do we really need clearing some another pointer, upper level should care about it

>       bdrv_unref(new_bs);
>   }
>   
> @@ -2300,7 +2300,7 @@ static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
>       if (new_bs) {
>           bdrv_ref(new_bs);
>       }
> -    bdrv_replace_child_noperm(child, new_bs);
> +    bdrv_replace_child_noperm(&child, new_bs);

  - no sence / no harm, as it's a local variable, which is not used anymore
  - most probably we have some another pointer that should be cleared, but it's not available here.. To make it available, bdrv_replace_child_tran() should get BdrvChild **.. maybe later patch will do it

>       /* old_bs reference is transparently moved from @child to @s */
>   }
>   
> @@ -2672,9 +2672,10 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
>       return permissions[qapi_perm];
>   }
>   
> -static void bdrv_replace_child_noperm(BdrvChild *child,
> +static void bdrv_replace_child_noperm(BdrvChild **childp,
>                                         BlockDriverState *new_bs)
>   {
> +    BdrvChild *child = *childp;

No real logic change for now, OK

>       BlockDriverState *old_bs = child->bs;
>       int new_bs_quiesce_counter;
>       int drain_saldo;
> @@ -2767,7 +2768,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
>       BdrvChild *child = *s->child;
>       BlockDriverState *bs = child->bs;
>   
> -    bdrv_replace_child_noperm(child, NULL);
> +    bdrv_replace_child_noperm(s->child, NULL);

More interesting. Currently bdrv_replace_child_tran() clear the pointer as last action, so later we can remove this last "*s->child = NULL" as bdrv_replace_child_noperm() will do it.
No harm: in the the function we use local variable, initialized as *s->child.

>   
>       if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
>           bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
> @@ -2867,7 +2868,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
>       }
>   
>       bdrv_ref(child_bs);
> -    bdrv_replace_child_noperm(new_child, child_bs);
> +    bdrv_replace_child_noperm(&new_child, child_bs);

Here child_bs must not be NULL, otherwise bdrv_ref() crashes. So, nothing would be cleared.

>   
>       *child = new_child;
>   
> @@ -2922,12 +2923,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
>       return 0;
>   }
>   
> -static void bdrv_detach_child(BdrvChild *child)
> +static void bdrv_detach_child(BdrvChild **childp)
>   {
> -    BlockDriverState *old_bs = child->bs;
> +    BlockDriverState *old_bs = (*childp)->bs;
>   
> -    bdrv_replace_child_noperm(child, NULL);
> -    bdrv_child_free(child);
> +    bdrv_replace_child_noperm(childp, NULL);

And here for sure we'll clear the pointer

> +    bdrv_child_free(*childp);

This obviously should be changed in further patches

>   
>       if (old_bs) {
>           /*
> @@ -3033,7 +3034,7 @@ void bdrv_root_unref_child(BdrvChild *child)
>       BlockDriverState *child_bs;
>   
>       child_bs = child->bs;
> -    bdrv_detach_child(child);
> +    bdrv_detach_child(&child);

  - no sence / no harm, as it's a local variable, which is not used anymore

>       bdrv_unref(child_bs);
>   }
>   
> 

Patch is correct:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 06/10] block: Restructure remove_file_or_backing_child()

[Vladimir Sementsov-Ogievskiy]

11.11.2021 15:08, Hanna Reitz wrote:
> As of a future patch, bdrv_replace_child_tran() will take a BdrvChild **
> pointer.  Prepare for that by getting such a pointer and using it where
> applicable, and (dereferenced) as a parameter for
> bdrv_replace_child_tran().
> 
> Signed-off-by: Hanna Reitz<hreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 07/10] transactions: Invoke clean() after everything else

[Vladimir Sementsov-Ogievskiy]

11.11.2021 15:08, Hanna Reitz wrote:
> Invoke the transaction drivers' .clean() methods only after all
> .commit() or .abort() handlers are done.
> 
> This makes it easier to have nested transactions where the top-level
> transactions pass objects to lower transactions that the latter can
> still use throughout their commit/abort phases, while the top-level
> transaction keeps a reference that is released in its .clean() method.
> 
> (Before this commit, that is also possible, but the top-level
> transaction would need to take care to invoke tran_add() before the
> lower-level transaction does.  This commit makes the ordering
> irrelevant, which is just a bit nicer.)
> 
> Signed-off-by: Hanna Reitz<hreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 08/10] block: Let replace_child_tran keep indirect pointer

[Vladimir Sementsov-Ogievskiy]

11.11.2021 15:08, Hanna Reitz wrote:
> As of a future commit, bdrv_replace_child_noperm() will clear the
> indirect BdrvChild pointer passed to it if the new child BDS is NULL.
> bdrv_replace_child_tran() will want to let it do that, but revert this
> change in its abort handler.  For that, we need to have it receive a
> BdrvChild ** pointer, too, and keep it stored in the
> BdrvReplaceChildState object that we attach to the transaction.
> 
> Note that we do not need to store it in the BdrvReplaceChildState when
> new_bs is not NULL, because then there is nothing to revert.  This is
> important so that bdrv_replace_node_noperm() can pass a pointer to a
> loop-local variable to bdrv_replace_child_tran() without worrying that
> this pointer will outlive one loop iteration.
> 
> (Of course, for that to work, bdrv_replace_node_noperm() and in turn
> bdrv_replace_node() and its relatives may not be called with a NULL @to
> node.  Luckily, they already are not, but now we should assert this.)
> 
> bdrv_remove_file_or_backing_child() on the other hand needs to ensure
> that the indirect pointer it passes will stay valid for the duration of
> the transaction.  Ensure this by keeping a strong reference to the BDS
> whose &bs->backing or &bs->file it passes to bdrv_replace_child_tran(),
> and giving up that reference only in the transaction .clean() handler.
> 
> Signed-off-by: Hanna Reitz<hreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH v2 09/10] block: Let replace_child_noperm free children

[Vladimir Sementsov-Ogievskiy]

11.11.2021 15:08, Hanna Reitz wrote:
> In most of the block layer, especially when traversing down from other
> BlockDriverStates, we assume that BdrvChild.bs can never be NULL.  When
> it becomes NULL, it is expected that the corresponding BdrvChild pointer
> also becomes NULL and the BdrvChild object is freed.
> 
> Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs
> pointer to NULL, it should also immediately set the corresponding
> BdrvChild pointer (like bs->file or bs->backing) to NULL.
> 
> In that context, it also makes sense for this function to free the
> child.  Sometimes we cannot do so, though, because it is called in a
> transactional context where the caller might still want to reinstate the
> child in the abort branch (and free it only on commit), so this behavior
> has to remain optional.
> 
> In bdrv_replace_child_tran()'s abort handler, we now rely on the fact
> that the BdrvChild passed to bdrv_replace_child_tran() must have had a
> non-NULL .bs pointer initially.  Make a note of that and assert it.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>   block.c | 102 +++++++++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 79 insertions(+), 23 deletions(-)
> 
> diff --git a/block.c b/block.c
> index a40027161c..0ac5b163d2 100644
> --- a/block.c
> +++ b/block.c
> @@ -87,8 +87,10 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>   static bool bdrv_recurse_has_child(BlockDriverState *bs,
>                                      BlockDriverState *child);
>   
> +static void bdrv_child_free(BdrvChild *child);
>   static void bdrv_replace_child_noperm(BdrvChild **child,
> -                                      BlockDriverState *new_bs);
> +                                      BlockDriverState *new_bs,
> +                                      bool free_empty_child);
>   static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
>                                                 BdrvChild *child,
>                                                 Transaction *tran);
> @@ -2256,12 +2258,16 @@ typedef struct BdrvReplaceChildState {
>       BdrvChild *child;
>       BdrvChild **childp;
>       BlockDriverState *old_bs;
> +    bool free_empty_child;
>   } BdrvReplaceChildState;
>   
>   static void bdrv_replace_child_commit(void *opaque)
>   {
>       BdrvReplaceChildState *s = opaque;
>   
> +    if (s->free_empty_child && !s->child->bs) {
> +        bdrv_child_free(s->child);
> +    }
>       bdrv_unref(s->old_bs);
>   }
>   
> @@ -2278,22 +2284,26 @@ static void bdrv_replace_child_abort(void *opaque)
>        *     modify the BdrvChild * pointer we indirectly pass to it, i.e. it
>        *     will not modify s->child.  From that perspective, it does not matter
>        *     whether we pass s->childp or &s->child.
> -     *     (TODO: Right now, bdrv_replace_child_noperm() never modifies that
> -     *     pointer anyway (though it will in the future), so at this point it
> -     *     absolutely does not matter whether we pass s->childp or &s->child.)
>        * (2) If new_bs is not NULL, s->childp will be NULL.  We then cannot use
>        *     it here.
>        * (3) If new_bs is NULL, *s->childp will have been NULLed by
>        *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
>        *     must not pass a NULL *s->childp here.
> -     *     (TODO: In its current state, bdrv_replace_child_noperm() will not
> -     *     have NULLed *s->childp, so this does not apply yet.  It will in the
> -     *     future.)

What I don't like about this patch is that it does two different things: zeroing the pointer and clearing the object. And if we look at the latter in separate, it seems that it's not needed:

Look: bdrv_replace_child_tran(): new parameter is set to true in two places, in both of them we are sure (and do assertion and comment) that new bs is not NULL and nothing will be freed.

Similarly, bdrv_replace_child_noperm() is called with true in two places where we sure that new bs is not NULL.

and only one place where new parameter set to true really do something:

> @@ -2960,8 +3013,7 @@ static void bdrv_detach_child(BdrvChild **childp)
>   {
>       BlockDriverState *old_bs = (*childp)->bs;
>   
> -    bdrv_replace_child_noperm(childp, NULL);
> -    bdrv_child_free(*childp);
> +    bdrv_replace_child_noperm(childp, NULL, true);
>   
>       if (old_bs) {
>           /*

And it doesn't worth the whole complexity of new parameters for two functions.

In this place we can simply do something like

BdrvChild *child = *childp;

bdrv_replace_child_noperm(childp, NULL);

bdrv_child_free(child);


I understand the idea: it seems good and intuitive to do zeroing the pointer and clearing the object in one shot. But this patch itself shows that we just can't do it in 90% of cases. So, I think better is not do it and live with only "zeroing the pointer" part of this patch.





Another idea that come to my mind while reviewing this series: did you consider zeroing bs->file / bs->backing in .detach, like you do with bs->children list at start of the series?  We can argue the same way that file and backing pointers are property of parent, and they should be zeroed in .detach, where element is removed from bs->children.




-- 
Best regards,
Vladimir

Re: [PATCH v2 10/10] iotests/030: Unthrottle parallel jobs in reverse

[Vladimir Sementsov-Ogievskiy]

11.11.2021 15:08, Hanna Reitz wrote:
> See the comment for why this is necessary.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>   tests/qemu-iotests/030 | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 5fb65b4bef..567bf1da67 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -251,7 +251,16 @@ class TestParallelOps(iotests.QMPTestCase):
>                                    speed=1024)
>               self.assert_qmp(result, 'return', {})
>   
> -        for job in pending_jobs:
> +        # Do this in reverse: After unthrottling them, some jobs may finish
> +        # before we have unthrottled all of them.  This will drain their
> +        # subgraph, and this will make jobs above them advance (despite those
> +        # jobs on top being throttled).  In the worst case, all jobs below the
> +        # top one are finished before we can unthrottle it, and this makes it
> +        # advance so far that it completes before we can unthrottle it - which
> +        # results in an error.
> +        # Starting from the top (i.e. in reverse) does not have this problem:
> +        # When a job finishes, the ones below it are not advanced.

Hmm, interesting why only jobs above the finished job may advance in the situation..

Looks like something may change and this workaround will stop working.

Isn't it better just handle the error, and don't care if job was just finished?

Something like

if result['return'] != {}:
    # Job was finished during drain caused by finish of already unthrottled job
    self.assert_qmp(result, 'error/class', 'DeviceNotActive')

Next thing in the test case is checking for completion events, so we'll get all events anyway.


> +        for job in reversed(pending_jobs):
>               result = self.vm.qmp('block-job-set-speed', device=job, speed=0)
>               self.assert_qmp(result, 'return', {})
>   
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 09/10] block: Let replace_child_noperm free children

[Hanna Reitz]

On 12.11.21 17:10, Vladimir Sementsov-Ogievskiy wrote:
> 11.11.2021 15:08, Hanna Reitz wrote:
>> In most of the block layer, especially when traversing down from other
>> BlockDriverStates, we assume that BdrvChild.bs can never be NULL.  When
>> it becomes NULL, it is expected that the corresponding BdrvChild pointer
>> also becomes NULL and the BdrvChild object is freed.
>>
>> Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs
>> pointer to NULL, it should also immediately set the corresponding
>> BdrvChild pointer (like bs->file or bs->backing) to NULL.
>>
>> In that context, it also makes sense for this function to free the
>> child.  Sometimes we cannot do so, though, because it is called in a
>> transactional context where the caller might still want to reinstate the
>> child in the abort branch (and free it only on commit), so this behavior
>> has to remain optional.
>>
>> In bdrv_replace_child_tran()'s abort handler, we now rely on the fact
>> that the BdrvChild passed to bdrv_replace_child_tran() must have had a
>> non-NULL .bs pointer initially.  Make a note of that and assert it.
>>
>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>> ---
>>   block.c | 102 +++++++++++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 79 insertions(+), 23 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index a40027161c..0ac5b163d2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -87,8 +87,10 @@ static BlockDriverState *bdrv_open_inherit(const 
>> char *filename,
>>   static bool bdrv_recurse_has_child(BlockDriverState *bs,
>>                                      BlockDriverState *child);
>>   +static void bdrv_child_free(BdrvChild *child);
>>   static void bdrv_replace_child_noperm(BdrvChild **child,
>> -                                      BlockDriverState *new_bs);
>> +                                      BlockDriverState *new_bs,
>> +                                      bool free_empty_child);
>>   static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
>>                                                 BdrvChild *child,
>>                                                 Transaction *tran);
>> @@ -2256,12 +2258,16 @@ typedef struct BdrvReplaceChildState {
>>       BdrvChild *child;
>>       BdrvChild **childp;
>>       BlockDriverState *old_bs;
>> +    bool free_empty_child;
>>   } BdrvReplaceChildState;
>>     static void bdrv_replace_child_commit(void *opaque)
>>   {
>>       BdrvReplaceChildState *s = opaque;
>>   +    if (s->free_empty_child && !s->child->bs) {
>> +        bdrv_child_free(s->child);
>> +    }
>>       bdrv_unref(s->old_bs);
>>   }
>>   @@ -2278,22 +2284,26 @@ static void bdrv_replace_child_abort(void 
>> *opaque)
>>        *     modify the BdrvChild * pointer we indirectly pass to it, 
>> i.e. it
>>        *     will not modify s->child.  From that perspective, it 
>> does not matter
>>        *     whether we pass s->childp or &s->child.
>> -     *     (TODO: Right now, bdrv_replace_child_noperm() never 
>> modifies that
>> -     *     pointer anyway (though it will in the future), so at this 
>> point it
>> -     *     absolutely does not matter whether we pass s->childp or 
>> &s->child.)
>>        * (2) If new_bs is not NULL, s->childp will be NULL. We then 
>> cannot use
>>        *     it here.
>>        * (3) If new_bs is NULL, *s->childp will have been NULLed by
>>        *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() 
>> call, and we
>>        *     must not pass a NULL *s->childp here.
>> -     *     (TODO: In its current state, bdrv_replace_child_noperm() 
>> will not
>> -     *     have NULLed *s->childp, so this does not apply yet.  It 
>> will in the
>> -     *     future.)
>
> What I don't like about this patch is that it does two different 
> things: zeroing the pointer and clearing the object. And if we look at 
> the latter in separate, it seems that it's not needed:
>
> Look: bdrv_replace_child_tran(): new parameter is set to true in two 
> places, in both of them we are sure (and do assertion and comment) 
> that new bs is not NULL and nothing will be freed.
>
> Similarly, bdrv_replace_child_noperm() is called with true in two 
> places where we sure that new bs is not NULL.
>
> and only one place where new parameter set to true really do something:
>
>> @@ -2960,8 +3013,7 @@ static void bdrv_detach_child(BdrvChild **childp)
>>   {
>>       BlockDriverState *old_bs = (*childp)->bs;
>>   -    bdrv_replace_child_noperm(childp, NULL);
>> -    bdrv_child_free(*childp);
>> +    bdrv_replace_child_noperm(childp, NULL, true);
>>         if (old_bs) {
>>           /*
>
> And it doesn't worth the whole complexity of new parameters for two 
> functions.
>
> In this place we can simply do something like
>
> BdrvChild *child = *childp;
>
> bdrv_replace_child_noperm(childp, NULL);
>
> bdrv_child_free(child);
>
>
> I understand the idea: it seems good and intuitive to do zeroing the 
> pointer and clearing the object in one shot. But this patch itself 
> shows that we just can't do it in 90% of cases. So, I think better is 
> not do it and live with only "zeroing the pointer" part of this patch.

I see your point, but I don’t find it too complex.  Passing `true` is 
the default and then calling the function is easy.  Passing `false` 
means there’s a catch, and then the caller is already complex anyway, so 
it doesn’t really make things worse.

I find the condition on when to pass `true` and when to pass `false` 
simple: Always pass true, unless the child cannot be deleted yet.

There are two reasons why I’d rather keep the parameter:
(1) That’s how it’s already merged, and I’m biased against respinning 
given that Kevin will be on PTO beginning tomorrow, and that we’re in 
freeze, so I’d rather not miss an RC.
(2) I really dislike a function that takes a pointer, NULLs it, and then 
doesn’t free the object it belongs to.  I find that a bad interface.  
Unfortunately we sometimes need this behavior, though, hence the 
additional parameter.  And this parameter basically asks the caller 
whether they want the reasonable interface (`true`) or the weird one 
where the pointer is NULLed but the object isn’t freed (`false`).  I 
find this makes the interface palatable to me.

>
> Another idea that come to my mind while reviewing this series: did you 
> consider zeroing bs->file / bs->backing in .detach, like you do with 
> bs->children list at start of the series?  We can argue the same way 
> that file and backing pointers are property of parent, and they should 
> be zeroed in .detach, where element is removed from bs->children.

Yes, I did.  The problem is that to make this right, .attach() would 
symmetrically need to put the child into bs->file or bs->backing (e.g. 
when the removal transaction is aborted).  That would not only be more 
invasive (we’d have to deal with and modify the places where bs->file or 
bs->backing is set), you’re then also facing the problem of giving 
.attach() this information.

Perhaps we could let .detach() clear the pointer and not set it in 
.attach(), but that seemed sufficiently wrong to me that I didn’t 
consider it further.

Hanna

Re: [PATCH v2 10/10] iotests/030: Unthrottle parallel jobs in reverse

[Hanna Reitz]

On 12.11.21 17:25, Vladimir Sementsov-Ogievskiy wrote:
> 11.11.2021 15:08, Hanna Reitz wrote:
>> See the comment for why this is necessary.
>>
>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/030 | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>> index 5fb65b4bef..567bf1da67 100755
>> --- a/tests/qemu-iotests/030
>> +++ b/tests/qemu-iotests/030
>> @@ -251,7 +251,16 @@ class TestParallelOps(iotests.QMPTestCase):
>>                                    speed=1024)
>>               self.assert_qmp(result, 'return', {})
>>   -        for job in pending_jobs:
>> +        # Do this in reverse: After unthrottling them, some jobs may 
>> finish
>> +        # before we have unthrottled all of them.  This will drain 
>> their
>> +        # subgraph, and this will make jobs above them advance 
>> (despite those
>> +        # jobs on top being throttled).  In the worst case, all jobs 
>> below the
>> +        # top one are finished before we can unthrottle it, and this 
>> makes it
>> +        # advance so far that it completes before we can unthrottle 
>> it - which
>> +        # results in an error.
>> +        # Starting from the top (i.e. in reverse) does not have this 
>> problem:
>> +        # When a job finishes, the ones below it are not advanced.
>
> Hmm, interesting why only jobs above the finished job may advance in 
> the situation..
>
> Looks like something may change and this workaround will stop working.
>
> Isn't it better just handle the error, and don't care if job was just 
> finished?
>
> Something like
>
> if result['return'] != {}:
>    # Job was finished during drain caused by finish of already 
> unthrottled job
>    self.assert_qmp(result, 'error/class', 'DeviceNotActive')

Well.  My explanation (excuse) is that I felt like this was the hack-ish 
solution that I could have gone for from the start without understanding 
what the issue is (and in fact it was the solution I used while 
debugging the other problems).  I went with `reversed()`, because that 
really addresses the problem.

You’re right in that it only addresses the problem for now and there’s a 
chance it might reappear.  If we want to go with ignoring 
DeviceNotActive errors, then I think we should at least query all block 
jobs before the unthrottle loop and see that at least at one point they 
were all running simultaneously.

I don’t really have a strong opinion.  We can exchange this patch now 
(though I’d rather not hold up the rest of the series for it), or have a 
patch on top later, or, well, just keep it for now.  I think the least 
stressful option would be to just fix it up later.

Hanna

Re: [PATCH v2 09/10] block: Let replace_child_noperm free children

[Vladimir Sementsov-Ogievskiy]

15.11.2021 16:04, Hanna Reitz wrote:
> On 12.11.21 17:10, Vladimir Sementsov-Ogievskiy wrote:
>> 11.11.2021 15:08, Hanna Reitz wrote:
>>> In most of the block layer, especially when traversing down from other
>>> BlockDriverStates, we assume that BdrvChild.bs can never be NULL.  When
>>> it becomes NULL, it is expected that the corresponding BdrvChild pointer
>>> also becomes NULL and the BdrvChild object is freed.
>>>
>>> Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs
>>> pointer to NULL, it should also immediately set the corresponding
>>> BdrvChild pointer (like bs->file or bs->backing) to NULL.
>>>
>>> In that context, it also makes sense for this function to free the
>>> child.  Sometimes we cannot do so, though, because it is called in a
>>> transactional context where the caller might still want to reinstate the
>>> child in the abort branch (and free it only on commit), so this behavior
>>> has to remain optional.
>>>
>>> In bdrv_replace_child_tran()'s abort handler, we now rely on the fact
>>> that the BdrvChild passed to bdrv_replace_child_tran() must have had a
>>> non-NULL .bs pointer initially.  Make a note of that and assert it.
>>>
>>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>>> ---
>>>   block.c | 102 +++++++++++++++++++++++++++++++++++++++++++-------------
>>>   1 file changed, 79 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index a40027161c..0ac5b163d2 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -87,8 +87,10 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>>>   static bool bdrv_recurse_has_child(BlockDriverState *bs,
>>>                                      BlockDriverState *child);
>>>   +static void bdrv_child_free(BdrvChild *child);
>>>   static void bdrv_replace_child_noperm(BdrvChild **child,
>>> -                                      BlockDriverState *new_bs);
>>> +                                      BlockDriverState *new_bs,
>>> +                                      bool free_empty_child);
>>>   static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
>>>                                                 BdrvChild *child,
>>>                                                 Transaction *tran);
>>> @@ -2256,12 +2258,16 @@ typedef struct BdrvReplaceChildState {
>>>       BdrvChild *child;
>>>       BdrvChild **childp;
>>>       BlockDriverState *old_bs;
>>> +    bool free_empty_child;
>>>   } BdrvReplaceChildState;
>>>     static void bdrv_replace_child_commit(void *opaque)
>>>   {
>>>       BdrvReplaceChildState *s = opaque;
>>>   +    if (s->free_empty_child && !s->child->bs) {
>>> +        bdrv_child_free(s->child);
>>> +    }
>>>       bdrv_unref(s->old_bs);
>>>   }
>>>   @@ -2278,22 +2284,26 @@ static void bdrv_replace_child_abort(void *opaque)
>>>        *     modify the BdrvChild * pointer we indirectly pass to it, i.e. it
>>>        *     will not modify s->child.  From that perspective, it does not matter
>>>        *     whether we pass s->childp or &s->child.
>>> -     *     (TODO: Right now, bdrv_replace_child_noperm() never modifies that
>>> -     *     pointer anyway (though it will in the future), so at this point it
>>> -     *     absolutely does not matter whether we pass s->childp or &s->child.)
>>>        * (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use
>>>        *     it here.
>>>        * (3) If new_bs is NULL, *s->childp will have been NULLed by
>>>        *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
>>>        *     must not pass a NULL *s->childp here.
>>> -     *     (TODO: In its current state, bdrv_replace_child_noperm() will not
>>> -     *     have NULLed *s->childp, so this does not apply yet.  It will in the
>>> -     *     future.)
>>
>> What I don't like about this patch is that it does two different things: zeroing the pointer and clearing the object. And if we look at the latter in separate, it seems that it's not needed:
>>
>> Look: bdrv_replace_child_tran(): new parameter is set to true in two places, in both of them we are sure (and do assertion and comment) that new bs is not NULL and nothing will be freed.
>>
>> Similarly, bdrv_replace_child_noperm() is called with true in two places where we sure that new bs is not NULL.
>>
>> and only one place where new parameter set to true really do something:
>>
>>> @@ -2960,8 +3013,7 @@ static void bdrv_detach_child(BdrvChild **childp)
>>>   {
>>>       BlockDriverState *old_bs = (*childp)->bs;
>>>   -    bdrv_replace_child_noperm(childp, NULL);
>>> -    bdrv_child_free(*childp);
>>> +    bdrv_replace_child_noperm(childp, NULL, true);
>>>         if (old_bs) {
>>>           /*
>>
>> And it doesn't worth the whole complexity of new parameters for two functions.
>>
>> In this place we can simply do something like
>>
>> BdrvChild *child = *childp;
>>
>> bdrv_replace_child_noperm(childp, NULL);
>>
>> bdrv_child_free(child);
>>
>>
>> I understand the idea: it seems good and intuitive to do zeroing the pointer and clearing the object in one shot. But this patch itself shows that we just can't do it in 90% of cases. So, I think better is not do it and live with only "zeroing the pointer" part of this patch.
> 
> I see your point, but I don’t find it too complex.  Passing `true` is the default and then calling the function is easy.  Passing `false` means there’s a catch, and then the caller is already complex anyway, so it doesn’t really make things worse.
> 
> I find the condition on when to pass `true` and when to pass `false` simple: Always pass true, unless the child cannot be deleted yet.
> 
> There are two reasons why I’d rather keep the parameter:
> (1) That’s how it’s already merged, and I’m biased against respinning given that Kevin will be on PTO beginning tomorrow, and that we’re in freeze, so I’d rather not miss an RC.

OK, that of course makes sense)

> (2) I really dislike a function that takes a pointer, NULLs it, and then doesn’t free the object it belongs to.  I find that a bad interface. Unfortunately we sometimes need this behavior, though, hence the additional parameter.  And this parameter basically asks the caller whether they want the reasonable interface (`true`) or the weird one where the pointer is NULLed but the object isn’t freed (`false`).  I find this makes the interface palatable to me.
> 
>>
>> Another idea that come to my mind while reviewing this series: did you consider zeroing bs->file / bs->backing in .detach, like you do with bs->children list at start of the series?  We can argue the same way that file and backing pointers are property of parent, and they should be zeroed in .detach, where element is removed from bs->children.
> 
> Yes, I did.  The problem is that to make this right, .attach() would symmetrically need to put the child into bs->file or bs->backing (e.g. when the removal transaction is aborted).  That would not only be more invasive (we’d have to deal with and modify the places where bs->file or bs->backing is set), you’re then also facing the problem of giving .attach() this information.
> 
> Perhaps we could let .detach() clear the pointer and not set it in .attach(), but that seemed sufficiently wrong to me that I didn’t consider it further.
> 

OK, reasonable.

If I have a good idea, I'll come with patches on top of this.

-- 
Best regards,
Vladimir

Re: [PATCH v2 10/10] iotests/030: Unthrottle parallel jobs in reverse

[Vladimir Sementsov-Ogievskiy]

15.11.2021 16:56, Hanna Reitz wrote:
> On 12.11.21 17:25, Vladimir Sementsov-Ogievskiy wrote:
>> 11.11.2021 15:08, Hanna Reitz wrote:
>>> See the comment for why this is necessary.
>>>
>>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>>> ---
>>>   tests/qemu-iotests/030 | 11 ++++++++++-
>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>>> index 5fb65b4bef..567bf1da67 100755
>>> --- a/tests/qemu-iotests/030
>>> +++ b/tests/qemu-iotests/030
>>> @@ -251,7 +251,16 @@ class TestParallelOps(iotests.QMPTestCase):
>>>                                    speed=1024)
>>>               self.assert_qmp(result, 'return', {})
>>>   -        for job in pending_jobs:
>>> +        # Do this in reverse: After unthrottling them, some jobs may finish
>>> +        # before we have unthrottled all of them.  This will drain their
>>> +        # subgraph, and this will make jobs above them advance (despite those
>>> +        # jobs on top being throttled).  In the worst case, all jobs below the
>>> +        # top one are finished before we can unthrottle it, and this makes it
>>> +        # advance so far that it completes before we can unthrottle it - which
>>> +        # results in an error.
>>> +        # Starting from the top (i.e. in reverse) does not have this problem:
>>> +        # When a job finishes, the ones below it are not advanced.
>>
>> Hmm, interesting why only jobs above the finished job may advance in the situation..
>>
>> Looks like something may change and this workaround will stop working.
>>
>> Isn't it better just handle the error, and don't care if job was just finished?
>>
>> Something like
>>
>> if result['return'] != {}:
>>    # Job was finished during drain caused by finish of already unthrottled job
>>    self.assert_qmp(result, 'error/class', 'DeviceNotActive')
> 
> Well.  My explanation (excuse) is that I felt like this was the hack-ish solution that I could have gone for from the start without understanding what the issue is (and in fact it was the solution I used while debugging the other problems).  I went with `reversed()`, because that really addresses the problem.
> 
> You’re right in that it only addresses the problem for now and there’s a chance it might reappear.  If we want to go with ignoring DeviceNotActive errors, then I think we should at least query all block jobs before the unthrottle loop and see that at least at one point they were all running simultaneously.
> 
> I don’t really have a strong opinion.  We can exchange this patch now (though I’d rather not hold up the rest of the series for it), or have a patch on top later, or, well, just keep it for now.  I think the least stressful option would be to just fix it up later.
> 

OK, I agree


-- 
Best regards,
Vladimir