[RFC PATCH 0/8] Refactor bdrv_try_set_aio_context using transactions

[RFC PATCH 0/8] Refactor bdrv_try_set_aio_context using transactions

[Emanuele Giuseppe Esposito]

The aim of this series is to reorganize bdrv_try_set_aio_context
and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in
favour of a new one, ->change_aio_ctx.

More informations in patch 3 (which is also RFC, due to the doubts
I have with AioContext locks).

Patch 1 just add assertions in the code, 2 extends the transactions API to be able to add also transactions in the tail
of the list.
Patch 3 is the core of this series, and introduces the new callback.
It is marked as RFC and the reason is explained in the commit message.
Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob
and block-backend BDSes.
Patch 7 substitutes ->change_aio_ctx with the old callbacks, and
patch 8 takes care of deleting the old callbacks and unused code.

This series is based on "job: replace AioContext lock with job_mutex",
but just because it uses job_set_aio_context() introduced there.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Based-on: <20220706201533.289775-1-eesposit@redhat.com>

Emanuele Giuseppe Esposito (8):
  block.c: assert bs->aio_context is written under BQL and drains
  transactions: add tran_add_back
  RFC: block: use transactions as a replacement of
    ->{can_}set_aio_context()
  blockjob: implement .change_aio_ctx in child_job
  block: implement .change_aio_ctx in child_of_bds
  block-backend: implement .change_aio_ctx in child_root
  block: use the new _change_ API instead of _can_set_ and _set_
  block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

 block.c                            | 288 +++++++++++++++--------------
 block/block-backend.c              |  65 +++++--
 blockjob.c                         |  50 +++--
 include/block/block-global-state.h |  16 +-
 include/block/block_int-common.h   |   5 +-
 include/qemu/transactions.h        |   9 +
 util/transactions.c                |  29 ++-
 7 files changed, 266 insertions(+), 196 deletions(-)

-- 
2.31.1

[RFC PATCH 1/8] block.c: assert bs->aio_context is written under BQL and drains

[Emanuele Giuseppe Esposito]

Also here ->aio_context is read by I/O threads and written
under BQL.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index d0db104d71..267a39c0de 100644
--- a/block.c
+++ b/block.c
@@ -7285,6 +7285,7 @@ static void bdrv_detach_aio_context(BlockDriverState *bs)
     if (bs->quiesce_counter) {
         aio_enable_external(bs->aio_context);
     }
+    assert_bdrv_graph_writable(bs);
     bs->aio_context = NULL;
 }
 
@@ -7298,6 +7299,7 @@ static void bdrv_attach_aio_context(BlockDriverState *bs,
         aio_disable_external(new_context);
     }
 
+    assert_bdrv_graph_writable(bs);
     bs->aio_context = new_context;
 
     if (bs->drv && bs->drv->bdrv_attach_aio_context) {
-- 
2.31.1

[RFC PATCH 2/8] transactions: add tran_add_back

[Emanuele Giuseppe Esposito]

First change the transactions from a QLIST to QSIMPLEQ, then
use it to implement tran_add_tail, which allows adding elements
to the end of list transactions.

This is useful if we have some "preparation" transiction callbacks
that we want to run before the others but still only when invoking
finalize/commit/abort.

For example (A and B are lists transaction callbacks):

for (i=0; i < 3; i++) {
	tran_add(A[i]);
	tran_add_tail(B[i]);
}

tran_commit();

Will process transactions in this order: A2 - A1 - A0 - B0 - B1 - B2

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/qemu/transactions.h |  9 +++++++++
 util/transactions.c         | 29 +++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h
index 2f2060acd9..42783720b9 100644
--- a/include/qemu/transactions.h
+++ b/include/qemu/transactions.h
@@ -50,7 +50,16 @@ typedef struct TransactionActionDrv {
 typedef struct Transaction Transaction;
 
 Transaction *tran_new(void);
+/*
+ * Add transaction at the beginning of the transaction list.
+ * @tran will be the first transaction to be processed in finalize/commit/abort.
+ */
 void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque);
+/*
+ * Add transaction at the end of the transaction list.
+ * @tran will be the last transaction to be processed in finalize/commit/abort.
+ */
+void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque);
 void tran_abort(Transaction *tran);
 void tran_commit(Transaction *tran);
 
diff --git a/util/transactions.c b/util/transactions.c
index 2dbdedce95..89e541c4a4 100644
--- a/util/transactions.c
+++ b/util/transactions.c
@@ -28,18 +28,18 @@
 typedef struct TransactionAction {
     TransactionActionDrv *drv;
     void *opaque;
-    QSLIST_ENTRY(TransactionAction) entry;
+    QSIMPLEQ_ENTRY(TransactionAction) entry;
 } TransactionAction;
 
 struct Transaction {
-    QSLIST_HEAD(, TransactionAction) actions;
+    QSIMPLEQ_HEAD(, TransactionAction) actions;
 };
 
 Transaction *tran_new(void)
 {
     Transaction *tran = g_new(Transaction, 1);
 
-    QSLIST_INIT(&tran->actions);
+    QSIMPLEQ_INIT(&tran->actions);
 
     return tran;
 }
@@ -54,20 +54,33 @@ void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque)
         .opaque = opaque
     };
 
-    QSLIST_INSERT_HEAD(&tran->actions, act, entry);
+    QSIMPLEQ_INSERT_HEAD(&tran->actions, act, entry);
+}
+
+void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque)
+{
+    TransactionAction *act;
+
+    act = g_new(TransactionAction, 1);
+    *act = (TransactionAction) {
+        .drv = drv,
+        .opaque = opaque
+    };
+
+    QSIMPLEQ_INSERT_TAIL(&tran->actions, act, entry);
 }
 
 void tran_abort(Transaction *tran)
 {
     TransactionAction *act, *next;
 
-    QSLIST_FOREACH(act, &tran->actions, entry) {
+    QSIMPLEQ_FOREACH(act, &tran->actions, entry) {
         if (act->drv->abort) {
             act->drv->abort(act->opaque);
         }
     }
 
-    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
+    QSIMPLEQ_FOREACH_SAFE(act, &tran->actions, entry, next) {
         if (act->drv->clean) {
             act->drv->clean(act->opaque);
         }
@@ -82,13 +95,13 @@ void tran_commit(Transaction *tran)
 {
     TransactionAction *act, *next;
 
-    QSLIST_FOREACH(act, &tran->actions, entry) {
+    QSIMPLEQ_FOREACH(act, &tran->actions, entry) {
         if (act->drv->commit) {
             act->drv->commit(act->opaque);
         }
     }
 
-    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
+    QSIMPLEQ_FOREACH_SAFE(act, &tran->actions, entry, next) {
         if (act->drv->clean) {
             act->drv->clean(act->opaque);
         }
-- 
2.31.1

[RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

[Emanuele Giuseppe Esposito]

---------
RFC because I am not sure about the AioContext locks.
- Do we need to take the new AioContext lock? what does it protect?
- Taking the old AioContext lock is required now, because of
  bdrv_drained_begin calling AIO_WAIT_WHILE that unlocks the
  aiocontext. If we replace it with AIO_WAIT_WHILE_UNLOCKED,
  could we get rid of taking every time the old AioContext?
  drain would be enough to protect the graph modification.
----------

Simplify the way the aiocontext can be changed in a BDS graph.
There are currently two problems in bdrv_try_set_aio_context:
- There is a confusion of AioContext locks taken and released, because
  we assume that old aiocontext is always taken and new one is
  taken inside.

- It doesn't look very safe to call bdrv_drained_begin while some
  nodes have already switched to the new aiocontext and others haven't.
  This could be especially dangerous because bdrv_drained_begin polls, so
  something else could be executed while graph is in an inconsistent
  state.

Additional minor nitpick: can_set and set_ callbacks both traverse the
graph, both using the ignored list of visited nodes in a different way.

Therefore, get rid of all of this and introduce a new callback,
change_aio_context, that uses transactions to efficiently, cleanly
and most importantly safely change the aiocontext of a graph.

This new callback is a "merge" of the two previous ones:
- Just like can_set_aio_context, recursively traverses the graph.
  Marks all nodes that are visited using a GList, and checks if
  they *could* change the aio_context.
- For each node that passes the above check, add a new transaction
  that implements a callback that effectively changes the aiocontext.
- If a node is a BDS, add two transactions: one taking care of draining
  the node at the beginning of the list (so that will be executed first)
  and one at the end taking care of changing the AioContext.
- Once done, the recursive function returns if *all* nodes can change
  the AioContext. If so, commit the above transactions. Otherwise don't
  do anything.
- The transaction list contains first all "drain" transactions, so
  we are sure we are draining all nodes in the same context, and then
  all the other switch the AioContext. In this way we make sure that
  bdrv_drained_begin() is always called under the old AioContext, and
  bdrv_drained_end() under the new one.
- Because of the above, we don't need to release and re-acquire the
  old AioContext every time, as everything is done once (and not
  per-node drain and aiocontext change).

Note that the "change" API is not yet invoked anywhere.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                            | 197 +++++++++++++++++++++++++++++
 include/block/block-global-state.h |   9 ++
 include/block/block_int-common.h   |   3 +
 3 files changed, 209 insertions(+)

diff --git a/block.c b/block.c
index 267a39c0de..bda4e1bcef 100644
--- a/block.c
+++ b/block.c
@@ -7437,6 +7437,51 @@ static bool bdrv_parent_can_set_aio_context(BdrvChild *c, AioContext *ctx,
     return true;
 }
 
+typedef struct BdrvStateSetAioContext {
+    AioContext *new_ctx;
+    BlockDriverState *bs;
+} BdrvStateSetAioContext;
+
+/*
+ * Changes the AioContext used for fd handlers, timers, and BHs by this
+ * BlockDriverState and all its children and parents.
+ *
+ * Must be called from the main AioContext.
+ *
+ * The caller must own the AioContext lock for the old AioContext of bs, but it
+ * must not own the AioContext lock for new_context (unless new_context is the
+ * same as the current context of bs).
+ *
+ * @visited will accumulate all visited BdrvChild object. The caller is
+ * responsible for freeing the list afterwards.
+ */
+static bool bdrv_parent_change_aio_context(BdrvChild *c, AioContext *ctx,
+                                           GSList **visited, Transaction *tran,
+                                           Error **errp)
+{
+    GLOBAL_STATE_CODE();
+    if (g_slist_find(*visited, c)) {
+        return true;
+    }
+    *visited = g_slist_prepend(*visited, c);
+
+    /*
+     * A BdrvChildClass that doesn't handle AioContext changes cannot
+     * tolerate any AioContext changes
+     */
+    if (!c->klass->change_aio_ctx) {
+        char *user = bdrv_child_user_desc(c);
+        error_setg(errp, "Changing iothreads is not supported by %s", user);
+        g_free(user);
+        return false;
+    }
+    if (!c->klass->change_aio_ctx(c, ctx, visited, tran, errp)) {
+        assert(!errp || *errp);
+        return false;
+    }
+    return true;
+}
+
 bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
                                     GSList **ignore, Error **errp)
 {
@@ -7448,6 +7493,18 @@ bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
     return bdrv_can_set_aio_context(c->bs, ctx, ignore, errp);
 }
 
+bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
+                                   GSList **visited, Transaction *tran,
+                                   Error **errp)
+{
+    GLOBAL_STATE_CODE();
+    if (g_slist_find(*visited, c)) {
+        return true;
+    }
+    *visited = g_slist_prepend(*visited, c);
+    return bdrv_change_aio_context(c->bs, ctx, visited, tran, errp);
+}
+
 /* @ignore will accumulate all visited BdrvChild object. The caller is
  * responsible for freeing the list afterwards. */
 bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
@@ -7475,6 +7532,99 @@ bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
     return true;
 }
 
+static void bdrv_drained_begin_commit(void *opaque)
+{
+    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
+
+    /* Paired with bdrv_drained_end in bdrv_drained_end_clean() */
+    bdrv_drained_begin(state->bs);
+}
+
+static void bdrv_drained_end_clean(void *opaque)
+{
+    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
+    BlockDriverState *bs = (BlockDriverState *) state->bs;
+    AioContext *new_context = state->new_ctx;
+
+    /*
+     * drain only if bdrv_drained_begin_commit() and
+     * bdrv_set_aio_context_commit() executed
+     */
+    if (bdrv_get_aio_context(bs) == new_context) {
+        /* Paired with bdrv_drained_begin in bdrv_drained_begin_commit() */
+        bdrv_drained_end(bs);
+    }
+
+    /* state is freed by set_aio_context->clean() */
+}
+
+static void bdrv_set_aio_context_commit(void *opaque)
+{
+    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
+    BlockDriverState *bs = (BlockDriverState *) state->bs;
+    AioContext *new_context = state->new_ctx;
+    assert_bdrv_graph_writable(bs);
+
+    bdrv_detach_aio_context(bs);
+    bdrv_attach_aio_context(bs, new_context);
+}
+
+static TransactionActionDrv set_aio_context = {
+    .commit = bdrv_set_aio_context_commit,
+    .clean = g_free,
+};
+
+static TransactionActionDrv drained_begin_end = {
+    .commit = bdrv_drained_begin_commit,
+    .clean = bdrv_drained_end_clean,
+};
+
+/*
+ * @visited will accumulate all visited BdrvChild object. The caller is
+ * responsible for freeing the list afterwards.
+ */
+bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+                             GSList **visited, Transaction *tran,
+                             Error **errp)
+{
+    BdrvChild *c;
+    BdrvStateSetAioContext *state;
+
+    GLOBAL_STATE_CODE();
+
+    if (bdrv_get_aio_context(bs) == ctx) {
+        return true;
+    }
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (!bdrv_parent_change_aio_context(c, ctx, visited, tran, errp)) {
+            return false;
+        }
+    }
+
+    QLIST_FOREACH(c, &bs->children, next) {
+        if (!bdrv_child_change_aio_context(c, ctx, visited, tran, errp)) {
+            return false;
+        }
+    }
+
+    state = g_new(BdrvStateSetAioContext, 1);
+    *state = (BdrvStateSetAioContext) {
+        .new_ctx = ctx,
+        .bs = bs,
+    };
+
+    /* First half of transactions are drains */
+    tran_add(tran, &drained_begin_end, state);
+    /*
+     * Second half of transactions takes care of setting the
+     * AioContext and free the state
+     */
+    tran_add_tail(tran, &set_aio_context, state);
+
+    return true;
+}
+
 int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
                                    BdrvChild *ignore_child, Error **errp)
 {
@@ -7498,6 +7648,53 @@ int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
     return 0;
 }
 
+/*
+ * First, go recursively through all nodes in the graph, and see if they
+ * can change their AioContext.
+ * If so, add for each node a new transaction with a callback to change the
+ * AioContext with the new one.
+ * Once recursion is completed, if all nodes are allowed to change their
+ * AioContext then commit the transaction, running all callbacks in order.
+ * Otherwise don't do anything.
+ *
+ * This function still requires the caller to take the bs current
+ * AioContext lock, otherwise draining could fail since AIO_WAIT_WHILE
+ * assumes the lock is always held if bs is in another AioContext.
+ */
+int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+                                      BdrvChild *ignore_child, Error **errp)
+{
+    Transaction *tran;
+    GSList *visited;
+    int ret;
+    GLOBAL_STATE_CODE();
+
+    tran = tran_new();
+    visited = ignore_child ? g_slist_prepend(NULL, ignore_child) : NULL;
+    ret = bdrv_change_aio_context(bs, ctx, &visited, tran, errp);
+    g_slist_free(visited);
+
+    if (!ret) {
+        /* just run clean() callbacks */
+        tran_abort(tran);
+        return -EPERM;
+    }
+
+    /* Acquire the new context, if necessary */
+    /* TODO: why do we need to take this AioContext lock? */
+    if (qemu_get_aio_context() != ctx) {
+        aio_context_acquire(ctx);
+    }
+
+    tran_commit(tran);
+
+    if (qemu_get_aio_context() != ctx) {
+        aio_context_release(ctx);
+    }
+
+    return 0;
+}
+
 int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
                              Error **errp)
 {
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 21265e3966..5847b64f95 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -229,6 +229,15 @@ bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
                               GSList **ignore, Error **errp);
 AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c);
 
+bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
+                                   GSList **visited, Transaction *tran,
+                                   Error **errp);
+bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+                             GSList **visited, Transaction *tran,
+                             Error **errp);
+int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+                                      BdrvChild *ignore_child, Error **errp);
+
 int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
 int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo);
 
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..b35a0fe840 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -896,6 +896,9 @@ struct BdrvChildClass {
                         GSList **ignore, Error **errp);
     void (*set_aio_ctx)(BdrvChild *child, AioContext *ctx, GSList **ignore);
 
+    bool (*change_aio_ctx)(BdrvChild *child, AioContext *ctx,
+                           GSList **ignore, Transaction *tran, Error **errp);
+
     AioContext *(*get_parent_aio_context)(BdrvChild *child);
 
     /*
-- 
2.31.1

[RFC PATCH 4/8] blockjob: implement .change_aio_ctx in child_job

[Emanuele Giuseppe Esposito]

child_job_change_aio_ctx() is very similar to
child_job_can_set_aio_ctx(), but it implements a new transaction
so that if all check pass, the new transaction's .commit()
will take care of changin the BlockJob AioContext.
child_job_set_aio_ctx_commit() is similar to child_job_set_aio_ctx(),
but it doesn't need to invoke the recursion, as this is already
taken care by child_job_change_aio_ctx().

Note: bdrv_child_try_change_aio_context() is not called by
anyone at this point.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 blockjob.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index bb82e8d04c..9e26b06ed4 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -124,6 +124,50 @@ static void child_job_drained_end(BdrvChild *c, int *drained_end_counter)
     }
 }
 
+typedef struct BdrvStateChildJobContext {
+    AioContext *new_ctx;
+    BlockJob *job;
+} BdrvStateChildJobContext;
+
+static void child_job_set_aio_ctx_commit(void *opaque)
+{
+    BdrvStateChildJobContext *s = opaque;
+    BlockJob *job = s->job;
+
+    job_set_aio_context(&job->job, s->new_ctx);
+}
+
+static TransactionActionDrv change_child_job_context = {
+    .commit = child_job_set_aio_ctx_commit,
+    .clean = g_free,
+};
+
+static bool child_job_change_aio_ctx(BdrvChild *c, AioContext *ctx,
+                                     GSList **visited, Transaction *tran,
+                                     Error **errp)
+{
+    BlockJob *job = c->opaque;
+    BdrvStateChildJobContext *s;
+    GSList *l;
+
+    for (l = job->nodes; l; l = l->next) {
+        BdrvChild *sibling = l->data;
+        if (!bdrv_child_change_aio_context(sibling, ctx, visited,
+                                           tran, errp)) {
+            return false;
+        }
+    }
+
+    s = g_new(BdrvStateChildJobContext, 1);
+    *s = (BdrvStateChildJobContext) {
+        .new_ctx = ctx,
+        .job = job,
+    };
+
+    tran_add_tail(tran, &change_child_job_context, s);
+    return true;
+}
+
 static bool child_job_can_set_aio_ctx(BdrvChild *c, AioContext *ctx,
                                       GSList **ignore, Error **errp)
 {
@@ -172,6 +216,7 @@ static const BdrvChildClass child_job = {
     .drained_end        = child_job_drained_end,
     .can_set_aio_ctx    = child_job_can_set_aio_ctx,
     .set_aio_ctx        = child_job_set_aio_ctx,
+    .change_aio_ctx     = child_job_change_aio_ctx,
     .stay_at_node       = true,
     .get_parent_aio_context = child_job_get_parent_aio_context,
 };
-- 
2.31.1

[RFC PATCH 5/8] block: implement .change_aio_ctx in child_of_bds

[Emanuele Giuseppe Esposito]

bdrv_child_cb_change_aio_ctx() is identical to
bdrv_child_cb_can_set_aio_ctx(), as we only need
to recursively go on the parent bs.

Note: bdrv_child_try_change_aio_context() is not called by
anyone at this point.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block.c b/block.c
index bda4e1bcef..a7ba590dfa 100644
--- a/block.c
+++ b/block.c
@@ -1235,6 +1235,14 @@ static int bdrv_child_cb_inactivate(BdrvChild *child)
     return 0;
 }
 
+static bool bdrv_child_cb_change_aio_ctx(BdrvChild *child, AioContext *ctx,
+                                         GSList **visited, Transaction *tran,
+                                         Error **errp)
+{
+    BlockDriverState *bs = child->opaque;
+    return bdrv_change_aio_context(bs, ctx, visited, tran, errp);
+}
+
 static bool bdrv_child_cb_can_set_aio_ctx(BdrvChild *child, AioContext *ctx,
                                           GSList **ignore, Error **errp)
 {
@@ -1491,6 +1499,7 @@ const BdrvChildClass child_of_bds = {
     .inactivate      = bdrv_child_cb_inactivate,
     .can_set_aio_ctx = bdrv_child_cb_can_set_aio_ctx,
     .set_aio_ctx     = bdrv_child_cb_set_aio_ctx,
+    .change_aio_ctx  = bdrv_child_cb_change_aio_ctx,
     .update_filename = bdrv_child_cb_update_filename,
     .get_parent_aio_context = child_of_bds_get_parent_aio_context,
 };
-- 
2.31.1

[RFC PATCH 6/8] block-backend: implement .change_aio_ctx in child_root

[Emanuele Giuseppe Esposito]

blk_root_change_aio_ctx() is very similar to blk_root_can_set_aio_ctx(),
but implements a new transaction so that if all check pass, the new
transaction's .commit will take care of changing the BlockBackend
AioContext. blk_root_set_aio_ctx_commit() is the same as
blk_root_set_aio_ctx().

Note: bdrv_child_try_change_aio_context() is not called by
anyone at this point.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index f425b00793..674eaaa2bf 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -138,6 +138,9 @@ static bool blk_root_can_set_aio_ctx(BdrvChild *child, AioContext *ctx,
                                      GSList **ignore, Error **errp);
 static void blk_root_set_aio_ctx(BdrvChild *child, AioContext *ctx,
                                  GSList **ignore);
+static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx,
+                                    GSList **visited, Transaction *tran,
+                                    Error **errp);
 
 static char *blk_root_get_parent_desc(BdrvChild *child)
 {
@@ -336,6 +339,7 @@ static const BdrvChildClass child_root = {
 
     .can_set_aio_ctx    = blk_root_can_set_aio_ctx,
     .set_aio_ctx        = blk_root_set_aio_ctx,
+    .change_aio_ctx     = blk_root_change_aio_ctx,
 
     .get_parent_aio_context = blk_root_get_parent_aio_context,
 };
@@ -2208,6 +2212,56 @@ int blk_set_aio_context(BlockBackend *blk, AioContext *new_context,
     return blk_do_set_aio_context(blk, new_context, true, errp);
 }
 
+typedef struct BdrvStateBlkRootContext {
+    AioContext *new_ctx;
+    BlockBackend *blk;
+} BdrvStateBlkRootContext;
+
+static void blk_root_set_aio_ctx_commit(void *opaque)
+{
+    BdrvStateBlkRootContext *s = opaque;
+    BlockBackend *blk = s->blk;
+
+    blk_do_set_aio_context(blk, s->new_ctx, false, &error_abort);
+}
+
+static TransactionActionDrv set_blk_root_context = {
+    .commit = blk_root_set_aio_ctx_commit,
+    .clean = g_free,
+};
+
+static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx,
+                                    GSList **visited, Transaction *tran,
+                                    Error **errp)
+{
+    BlockBackend *blk = child->opaque;
+    BdrvStateBlkRootContext *s;
+
+    if (blk->allow_aio_context_change) {
+        goto finish;
+    }
+
+    /*
+     * Only manually created BlockBackends that are not attached to anything
+     * can change their AioContext without updating their user.
+     */
+    if (!blk->name || blk->dev) {
+        /* TODO Add BB name/QOM path */
+        error_setg(errp, "Cannot change iothread of active block backend");
+        return false;
+    }
+
+finish:
+    s = g_new(BdrvStateBlkRootContext, 1);
+    *s = (BdrvStateBlkRootContext) {
+        .new_ctx = ctx,
+        .blk = blk,
+    };
+
+    tran_add_tail(tran, &set_blk_root_context, s);
+    return true;
+}
+
 static bool blk_root_can_set_aio_ctx(BdrvChild *child, AioContext *ctx,
                                      GSList **ignore, Error **errp)
 {
-- 
2.31.1

[RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

[Emanuele Giuseppe Esposito]

Replace all direct usage of ->can_set_aio_ctx and ->set_aio_ctx,
and call bdrv_child_try_change_aio_context() in
bdrv_try_set_aio_context(), the main function called through
the whole block layer.

From this point onwards, ->can_set_aio_ctx and ->set_aio_ctx
won't be used anymore.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c               | 30 ++++++++++++++++--------------
 block/block-backend.c |  8 ++++++--
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index a7ba590dfa..101188a2d4 100644
--- a/block.c
+++ b/block.c
@@ -2966,17 +2966,18 @@ static void bdrv_attach_child_common_abort(void *opaque)
     }
 
     if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
+        Transaction *tran;
         GSList *ignore;
+        bool ret;
 
-        /* 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);
+        tran = tran_new();
 
+        /* No need to ignore `child`, because it has been detached already */
         ignore = NULL;
-        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
+        ret = child->klass->change_aio_ctx(child, s->old_parent_ctx, &ignore,
+                                           tran, &error_abort);
         g_slist_free(ignore);
+        tran_finalize(tran, ret ? ret : -1);
     }
 
     bdrv_unref(bs);
@@ -3037,17 +3038,18 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
         Error *local_err = NULL;
         int ret = bdrv_try_set_aio_context(child_bs, parent_ctx, &local_err);
 
-        if (ret < 0 && child_class->can_set_aio_ctx) {
+        if (ret < 0 && child_class->change_aio_ctx) {
+            Transaction *tran = tran_new();
             GSList *ignore = g_slist_prepend(NULL, new_child);
-            if (child_class->can_set_aio_ctx(new_child, child_ctx, &ignore,
-                                             NULL))
-            {
+            bool ret_child;
+
+            ret_child = child_class->change_aio_ctx(new_child, child_ctx,
+                                                    &ignore, tran, NULL);
+            if (ret_child) {
                 error_free(local_err);
                 ret = 0;
-                g_slist_free(ignore);
-                ignore = g_slist_prepend(NULL, new_child);
-                child_class->set_aio_ctx(new_child, child_ctx, &ignore);
             }
+            tran_finalize(tran, ret_child ? ret_child : -1);
             g_slist_free(ignore);
         }
 
@@ -7708,7 +7710,7 @@ int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
                              Error **errp)
 {
     GLOBAL_STATE_CODE();
-    return bdrv_child_try_set_aio_context(bs, ctx, NULL, errp);
+    return bdrv_child_try_change_aio_context(bs, ctx, NULL, errp);
 }
 
 void bdrv_add_aio_context_notifier(BlockDriverState *bs,
diff --git a/block/block-backend.c b/block/block-backend.c
index 674eaaa2bf..6e90ac3a6a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2184,8 +2184,12 @@ static int blk_do_set_aio_context(BlockBackend *blk, AioContext *new_context,
         bdrv_ref(bs);
 
         if (update_root_node) {
-            ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
-                                                 errp);
+            /*
+             * update_root_node MUST be false for blk_root_set_aio_ctx_commit(),
+             * as we are already in the commit function of a transaction.
+             */
+            ret = bdrv_child_try_change_aio_context(bs, new_context, blk->root,
+                                                    errp);
             if (ret < 0) {
                 bdrv_unref(bs);
                 return ret;
-- 
2.31.1

[RFC PATCH 8/8] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

[Emanuele Giuseppe Esposito]

Together with all _can_set_ and _set_ APIs, as they are not needed
anymore.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                            | 196 -----------------------------
 block/block-backend.c              |  33 -----
 blockjob.c                         |  35 ------
 include/block/block-global-state.h |   9 --
 include/block/block_int-common.h   |   4 -
 5 files changed, 277 deletions(-)

diff --git a/block.c b/block.c
index 101188a2d4..d99784dff1 100644
--- a/block.c
+++ b/block.c
@@ -1243,20 +1243,6 @@ static bool bdrv_child_cb_change_aio_ctx(BdrvChild *child, AioContext *ctx,
     return bdrv_change_aio_context(bs, ctx, visited, tran, errp);
 }
 
-static bool bdrv_child_cb_can_set_aio_ctx(BdrvChild *child, AioContext *ctx,
-                                          GSList **ignore, Error **errp)
-{
-    BlockDriverState *bs = child->opaque;
-    return bdrv_can_set_aio_context(bs, ctx, ignore, errp);
-}
-
-static void bdrv_child_cb_set_aio_ctx(BdrvChild *child, AioContext *ctx,
-                                      GSList **ignore)
-{
-    BlockDriverState *bs = child->opaque;
-    return bdrv_set_aio_context_ignore(bs, ctx, ignore);
-}
-
 /*
  * Returns the options and flags that a temporary snapshot should get, based on
  * the originally requested flags (the originally requested image will have
@@ -1497,8 +1483,6 @@ const BdrvChildClass child_of_bds = {
     .attach          = bdrv_child_cb_attach,
     .detach          = bdrv_child_cb_detach,
     .inactivate      = bdrv_child_cb_inactivate,
-    .can_set_aio_ctx = bdrv_child_cb_can_set_aio_ctx,
-    .set_aio_ctx     = bdrv_child_cb_set_aio_ctx,
     .change_aio_ctx  = bdrv_child_cb_change_aio_ctx,
     .update_filename = bdrv_child_cb_update_filename,
     .get_parent_aio_context = child_of_bds_get_parent_aio_context,
@@ -7329,125 +7313,6 @@ static void bdrv_attach_aio_context(BlockDriverState *bs,
     bs->walking_aio_notifiers = false;
 }
 
-/*
- * Changes the AioContext used for fd handlers, timers, and BHs by this
- * BlockDriverState and all its children and parents.
- *
- * Must be called from the main AioContext.
- *
- * The caller must own the AioContext lock for the old AioContext of bs, but it
- * must not own the AioContext lock for new_context (unless new_context is the
- * same as the current context of bs).
- *
- * @ignore will accumulate all visited BdrvChild object. The caller is
- * responsible for freeing the list afterwards.
- */
-void bdrv_set_aio_context_ignore(BlockDriverState *bs,
-                                 AioContext *new_context, GSList **ignore)
-{
-    AioContext *old_context = bdrv_get_aio_context(bs);
-    GSList *children_to_process = NULL;
-    GSList *parents_to_process = NULL;
-    GSList *entry;
-    BdrvChild *child, *parent;
-
-    g_assert(qemu_get_current_aio_context() == qemu_get_aio_context());
-    GLOBAL_STATE_CODE();
-
-    if (old_context == new_context) {
-        return;
-    }
-
-    bdrv_drained_begin(bs);
-
-    QLIST_FOREACH(child, &bs->children, next) {
-        if (g_slist_find(*ignore, child)) {
-            continue;
-        }
-        *ignore = g_slist_prepend(*ignore, child);
-        children_to_process = g_slist_prepend(children_to_process, child);
-    }
-
-    QLIST_FOREACH(parent, &bs->parents, next_parent) {
-        if (g_slist_find(*ignore, parent)) {
-            continue;
-        }
-        *ignore = g_slist_prepend(*ignore, parent);
-        parents_to_process = g_slist_prepend(parents_to_process, parent);
-    }
-
-    for (entry = children_to_process;
-         entry != NULL;
-         entry = g_slist_next(entry)) {
-        child = entry->data;
-        bdrv_set_aio_context_ignore(child->bs, new_context, ignore);
-    }
-    g_slist_free(children_to_process);
-
-    for (entry = parents_to_process;
-         entry != NULL;
-         entry = g_slist_next(entry)) {
-        parent = entry->data;
-        assert(parent->klass->set_aio_ctx);
-        parent->klass->set_aio_ctx(parent, new_context, ignore);
-    }
-    g_slist_free(parents_to_process);
-
-    bdrv_detach_aio_context(bs);
-
-    /* Acquire the new context, if necessary */
-    if (qemu_get_aio_context() != new_context) {
-        aio_context_acquire(new_context);
-    }
-
-    bdrv_attach_aio_context(bs, new_context);
-
-    /*
-     * If this function was recursively called from
-     * bdrv_set_aio_context_ignore(), there may be nodes in the
-     * subtree that have not yet been moved to the new AioContext.
-     * Release the old one so bdrv_drained_end() can poll them.
-     */
-    if (qemu_get_aio_context() != old_context) {
-        aio_context_release(old_context);
-    }
-
-    bdrv_drained_end(bs);
-
-    if (qemu_get_aio_context() != old_context) {
-        aio_context_acquire(old_context);
-    }
-    if (qemu_get_aio_context() != new_context) {
-        aio_context_release(new_context);
-    }
-}
-
-static bool bdrv_parent_can_set_aio_context(BdrvChild *c, AioContext *ctx,
-                                            GSList **ignore, Error **errp)
-{
-    GLOBAL_STATE_CODE();
-    if (g_slist_find(*ignore, c)) {
-        return true;
-    }
-    *ignore = g_slist_prepend(*ignore, c);
-
-    /*
-     * A BdrvChildClass that doesn't handle AioContext changes cannot
-     * tolerate any AioContext changes
-     */
-    if (!c->klass->can_set_aio_ctx) {
-        char *user = bdrv_child_user_desc(c);
-        error_setg(errp, "Changing iothreads is not supported by %s", user);
-        g_free(user);
-        return false;
-    }
-    if (!c->klass->can_set_aio_ctx(c, ctx, ignore, errp)) {
-        assert(!errp || *errp);
-        return false;
-    }
-    return true;
-}
-
 typedef struct BdrvStateSetAioContext {
     AioContext *new_ctx;
     BlockDriverState *bs;
@@ -7493,17 +7358,6 @@ static bool bdrv_parent_change_aio_context(BdrvChild *c, AioContext *ctx,
     return true;
 }
 
-bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
-                                    GSList **ignore, Error **errp)
-{
-    GLOBAL_STATE_CODE();
-    if (g_slist_find(*ignore, c)) {
-        return true;
-    }
-    *ignore = g_slist_prepend(*ignore, c);
-    return bdrv_can_set_aio_context(c->bs, ctx, ignore, errp);
-}
-
 bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
                                    GSList **visited, Transaction *tran,
                                    Error **errp)
@@ -7516,33 +7370,6 @@ bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
     return bdrv_change_aio_context(c->bs, ctx, visited, tran, errp);
 }
 
-/* @ignore will accumulate all visited BdrvChild object. The caller is
- * responsible for freeing the list afterwards. */
-bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
-                              GSList **ignore, Error **errp)
-{
-    BdrvChild *c;
-
-    if (bdrv_get_aio_context(bs) == ctx) {
-        return true;
-    }
-
-    GLOBAL_STATE_CODE();
-
-    QLIST_FOREACH(c, &bs->parents, next_parent) {
-        if (!bdrv_parent_can_set_aio_context(c, ctx, ignore, errp)) {
-            return false;
-        }
-    }
-    QLIST_FOREACH(c, &bs->children, next) {
-        if (!bdrv_child_can_set_aio_context(c, ctx, ignore, errp)) {
-            return false;
-        }
-    }
-
-    return true;
-}
-
 static void bdrv_drained_begin_commit(void *opaque)
 {
     BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
@@ -7636,29 +7463,6 @@ bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
     return true;
 }
 
-int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
-                                   BdrvChild *ignore_child, Error **errp)
-{
-    GSList *ignore;
-    bool ret;
-
-    GLOBAL_STATE_CODE();
-
-    ignore = ignore_child ? g_slist_prepend(NULL, ignore_child) : NULL;
-    ret = bdrv_can_set_aio_context(bs, ctx, &ignore, errp);
-    g_slist_free(ignore);
-
-    if (!ret) {
-        return -EPERM;
-    }
-
-    ignore = ignore_child ? g_slist_prepend(NULL, ignore_child) : NULL;
-    bdrv_set_aio_context_ignore(bs, ctx, &ignore);
-    g_slist_free(ignore);
-
-    return 0;
-}
-
 /*
  * First, go recursively through all nodes in the graph, and see if they
  * can change their AioContext.
diff --git a/block/block-backend.c b/block/block-backend.c
index 6e90ac3a6a..9925663ca1 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -134,10 +134,6 @@ static void blk_root_drained_end(BdrvChild *child, int *drained_end_counter);
 static void blk_root_change_media(BdrvChild *child, bool load);
 static void blk_root_resize(BdrvChild *child);
 
-static bool blk_root_can_set_aio_ctx(BdrvChild *child, AioContext *ctx,
-                                     GSList **ignore, Error **errp);
-static void blk_root_set_aio_ctx(BdrvChild *child, AioContext *ctx,
-                                 GSList **ignore);
 static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx,
                                     GSList **visited, Transaction *tran,
                                     Error **errp);
@@ -337,8 +333,6 @@ static const BdrvChildClass child_root = {
     .attach             = blk_root_attach,
     .detach             = blk_root_detach,
 
-    .can_set_aio_ctx    = blk_root_can_set_aio_ctx,
-    .set_aio_ctx        = blk_root_set_aio_ctx,
     .change_aio_ctx     = blk_root_change_aio_ctx,
 
     .get_parent_aio_context = blk_root_get_parent_aio_context,
@@ -2266,33 +2260,6 @@ finish:
     return true;
 }
 
-static bool blk_root_can_set_aio_ctx(BdrvChild *child, AioContext *ctx,
-                                     GSList **ignore, Error **errp)
-{
-    BlockBackend *blk = child->opaque;
-
-    if (blk->allow_aio_context_change) {
-        return true;
-    }
-
-    /* Only manually created BlockBackends that are not attached to anything
-     * can change their AioContext without updating their user. */
-    if (!blk->name || blk->dev) {
-        /* TODO Add BB name/QOM path */
-        error_setg(errp, "Cannot change iothread of active block backend");
-        return false;
-    }
-
-    return true;
-}
-
-static void blk_root_set_aio_ctx(BdrvChild *child, AioContext *ctx,
-                                 GSList **ignore)
-{
-    BlockBackend *blk = child->opaque;
-    blk_do_set_aio_context(blk, ctx, false, &error_abort);
-}
-
 void blk_add_aio_context_notifier(BlockBackend *blk,
         void (*attached_aio_context)(AioContext *new_context, void *opaque),
         void (*detach_aio_context)(void *opaque), void *opaque)
diff --git a/blockjob.c b/blockjob.c
index 9e26b06ed4..219a444108 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -168,39 +168,6 @@ static bool child_job_change_aio_ctx(BdrvChild *c, AioContext *ctx,
     return true;
 }
 
-static bool child_job_can_set_aio_ctx(BdrvChild *c, AioContext *ctx,
-                                      GSList **ignore, Error **errp)
-{
-    BlockJob *job = c->opaque;
-    GSList *l;
-
-    for (l = job->nodes; l; l = l->next) {
-        BdrvChild *sibling = l->data;
-        if (!bdrv_child_can_set_aio_context(sibling, ctx, ignore, errp)) {
-            return false;
-        }
-    }
-    return true;
-}
-
-static void child_job_set_aio_ctx(BdrvChild *c, AioContext *ctx,
-                                  GSList **ignore)
-{
-    BlockJob *job = c->opaque;
-    GSList *l;
-
-    for (l = job->nodes; l; l = l->next) {
-        BdrvChild *sibling = l->data;
-        if (g_slist_find(*ignore, sibling)) {
-            continue;
-        }
-        *ignore = g_slist_prepend(*ignore, sibling);
-        bdrv_set_aio_context_ignore(sibling->bs, ctx, ignore);
-    }
-
-    job_set_aio_context(&job->job, ctx);
-}
-
 static AioContext *child_job_get_parent_aio_context(BdrvChild *c)
 {
     BlockJob *job = c->opaque;
@@ -214,8 +181,6 @@ static const BdrvChildClass child_job = {
     .drained_begin      = child_job_drained_begin,
     .drained_poll       = child_job_drained_poll,
     .drained_end        = child_job_drained_end,
-    .can_set_aio_ctx    = child_job_can_set_aio_ctx,
-    .set_aio_ctx        = child_job_set_aio_ctx,
     .change_aio_ctx     = child_job_change_aio_ctx,
     .stay_at_node       = true,
     .get_parent_aio_context = child_job_get_parent_aio_context,
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 5847b64f95..65262781ab 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -217,18 +217,9 @@ void coroutine_fn bdrv_co_lock(BlockDriverState *bs);
  */
 void coroutine_fn bdrv_co_unlock(BlockDriverState *bs);
 
-void bdrv_set_aio_context_ignore(BlockDriverState *bs,
-                                 AioContext *new_context, GSList **ignore);
 int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
                              Error **errp);
-int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
-                                   BdrvChild *ignore_child, Error **errp);
-bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
-                                    GSList **ignore, Error **errp);
-bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
-                              GSList **ignore, Error **errp);
 AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c);
-
 bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
                                    GSList **visited, Transaction *tran,
                                    Error **errp);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index b35a0fe840..3c0e059602 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -892,10 +892,6 @@ struct BdrvChildClass {
     int (*update_filename)(BdrvChild *child, BlockDriverState *new_base,
                            const char *filename, Error **errp);
 
-    bool (*can_set_aio_ctx)(BdrvChild *child, AioContext *ctx,
-                        GSList **ignore, Error **errp);
-    void (*set_aio_ctx)(BdrvChild *child, AioContext *ctx, GSList **ignore);
-
     bool (*change_aio_ctx)(BdrvChild *child, AioContext *ctx,
                            GSList **ignore, Transaction *tran, Error **errp);
 
-- 
2.31.1

Re: [RFC PATCH 1/8] block.c: assert bs->aio_context is written under BQL and drains

[Hanna Reitz]

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> Also here ->aio_context is read by I/O threads and written
> under BQL.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [RFC PATCH 2/8] transactions: add tran_add_back

[Hanna Reitz]

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> First change the transactions from a QLIST to QSIMPLEQ, then
> use it to implement tran_add_tail, which allows adding elements
> to the end of list transactions.

The subject still calls it `tran_add_back()` (perhaps from a preliminary 
version?), I think that needs adjustment.

> This is useful if we have some "preparation" transiction callbacks

*transaction

> that we want to run before the others but still only when invoking
> finalize/commit/abort.

I don’t understand this yet (but perhaps it’ll become clearer with the 
following patches); doesn’t the new function do the opposite? I.e., 
basically add some clean-up that’s only used after everything else?

> For example (A and B are lists transaction callbacks):
>
> for (i=0; i < 3; i++) {
> 	tran_add(A[i]);
> 	tran_add_tail(B[i]);
> }
>
> tran_commit();
>
> Will process transactions in this order: A2 - A1 - A0 - B0 - B1 - B2
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   include/qemu/transactions.h |  9 +++++++++
>   util/transactions.c         | 29 +++++++++++++++++++++--------
>   2 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h
> index 2f2060acd9..42783720b9 100644
> --- a/include/qemu/transactions.h
> +++ b/include/qemu/transactions.h
> @@ -50,7 +50,16 @@ typedef struct TransactionActionDrv {
>   typedef struct Transaction Transaction;
>   
>   Transaction *tran_new(void);
> +/*
> + * Add transaction at the beginning of the transaction list.
> + * @tran will be the first transaction to be processed in finalize/commit/abort.

Of course, if you call tran_add() afterwards, this transaction will no 
longer be the first one.  I mean, that’s kind of obvious, but perhaps we 
can still express that here.

Like, perhaps, “finalize/commit/abort process this list in order, so 
after this call, @tran will be the first transaction to be processed”?

> + */
>   void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque);
> +/*
> + * Add transaction at the end of the transaction list.
> + * @tran will be the last transaction to be processed in finalize/commit/abort.

(And then “finalize/commit/abort process this list in order, so after 
this call, @tran will be the last transaction to be processed”)

> + */
> +void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque);
>   void tran_abort(Transaction *tran);
>   void tran_commit(Transaction *tran);
>   
> diff --git a/util/transactions.c b/util/transactions.c
> index 2dbdedce95..89e541c4a4 100644
> --- a/util/transactions.c
> +++ b/util/transactions.c

[...]

> @@ -54,20 +54,33 @@ void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque)
>           .opaque = opaque
>       };
>   
> -    QSLIST_INSERT_HEAD(&tran->actions, act, entry);
> +    QSIMPLEQ_INSERT_HEAD(&tran->actions, act, entry);
> +}
> +
> +void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque)
> +{
> +    TransactionAction *act;
> +
> +    act = g_new(TransactionAction, 1);
> +    *act = (TransactionAction) {
> +        .drv = drv,
> +        .opaque = opaque
> +    };
> +
> +    QSIMPLEQ_INSERT_TAIL(&tran->actions, act, entry);
>   }

Perhaps this could benefit from a function encompassing the common 
functionality, i.e. a tran_do_add(..., bool tail) with

if (tail) {
     QSIMPLEQ_INSERT_TAIL(...);
} else {
     QSIMPLEQ_INSERT_HEAD(...);
}

(Just a light suggestion.)

Hanna

Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

[Hanna Reitz]

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> ---------
> RFC because I am not sure about the AioContext locks.
> - Do we need to take the new AioContext lock? what does it protect?
> - Taking the old AioContext lock is required now, because of
>    bdrv_drained_begin calling AIO_WAIT_WHILE that unlocks the
>    aiocontext. If we replace it with AIO_WAIT_WHILE_UNLOCKED,
>    could we get rid of taking every time the old AioContext?
>    drain would be enough to protect the graph modification.

It’s been a while, but as far as I remember (which may be wrong), the 
reason for how the locks are supposed to be taken was mostly that we 
need some defined state so that we know how to invoke 
bdrv_drained_begin() and bdrv_drained_end() (i.e. call the first one 
as-is, and switch the locks around for the latter one).

The idea of using _UNLOCKED sounds interesting, almost too obvious. I 
can’t see why that wouldn’t work, actually.

> ----------
>
> Simplify the way the aiocontext can be changed in a BDS graph.
> There are currently two problems in bdrv_try_set_aio_context:
> - There is a confusion of AioContext locks taken and released, because
>    we assume that old aiocontext is always taken and new one is
>    taken inside.

Yep, and that assumption is just broken in some cases, which is the main 
pain point I’m feeling with it right now.

For example, look at bdrv_attach_child_common(): Here, we attach a child 
to a parent, so we need to get them into a common AioContext. So first 
we try to put the child into the parent’s context, and if that fails, 
we’ll try the other way, putting the parent into the child’s context.

The problem is clear: The bdrv_try_set_aio_context() call requires us to 
hold the child’s current context but not the parent’s, and the 
child_class->set_aio_ctx() call requires the exact opposite.  But we 
never switch the context we have acquired, so this can’t both work.  
Better yet, nowhere is it defined what context a caller to 
bdrv_attach_child_common() will hold.

In practice, what happens here most of the time is that something will 
be moved from the main context to some other context, and since we’re in 
the main context already, that’ll just work.  But you can construct 
cases where something is attempted to be moved from an I/O thread into a 
different thread and then you’ll get a crash.

I’d be happy if we could do away with the requirement of having to hold 
any lock for changing a node’s AioContext.

> - It doesn't look very safe to call bdrv_drained_begin while some
>    nodes have already switched to the new aiocontext and others haven't.
>    This could be especially dangerous because bdrv_drained_begin polls, so
>    something else could be executed while graph is in an inconsistent
>    state.
>
> Additional minor nitpick: can_set and set_ callbacks both traverse the
> graph, both using the ignored list of visited nodes in a different way.
>
> Therefore, get rid of all of this and introduce a new callback,
> change_aio_context, that uses transactions to efficiently, cleanly
> and most importantly safely change the aiocontext of a graph.
>
> This new callback is a "merge" of the two previous ones:
> - Just like can_set_aio_context, recursively traverses the graph.
>    Marks all nodes that are visited using a GList, and checks if
>    they *could* change the aio_context.
> - For each node that passes the above check, add a new transaction
>    that implements a callback that effectively changes the aiocontext.
> - If a node is a BDS, add two transactions: one taking care of draining
>    the node at the beginning of the list (so that will be executed first)
>    and one at the end taking care of changing the AioContext.
> - Once done, the recursive function returns if *all* nodes can change
>    the AioContext. If so, commit the above transactions. Otherwise don't
>    do anything.
> - The transaction list contains first all "drain" transactions, so
>    we are sure we are draining all nodes in the same context, and then
>    all the other switch the AioContext. In this way we make sure that
>    bdrv_drained_begin() is always called under the old AioContext, and
>    bdrv_drained_end() under the new one.
> - Because of the above, we don't need to release and re-acquire the
>    old AioContext every time, as everything is done once (and not
>    per-node drain and aiocontext change).
>
> Note that the "change" API is not yet invoked anywhere.

So the idea is that we introduce a completely new transaction-based API 
to change BDSs’ AioContext, and then drop the old one, right?

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c                            | 197 +++++++++++++++++++++++++++++
>   include/block/block-global-state.h |   9 ++
>   include/block/block_int-common.h   |   3 +
>   3 files changed, 209 insertions(+)
>
> diff --git a/block.c b/block.c
> index 267a39c0de..bda4e1bcef 100644
> --- a/block.c
> +++ b/block.c
> @@ -7437,6 +7437,51 @@ static bool bdrv_parent_can_set_aio_context(BdrvChild *c, AioContext *ctx,
>       return true;
>   }
>   
> +typedef struct BdrvStateSetAioContext {
> +    AioContext *new_ctx;
> +    BlockDriverState *bs;
> +} BdrvStateSetAioContext;
> +
> +/*
> + * Changes the AioContext used for fd handlers, timers, and BHs by this
> + * BlockDriverState and all its children and parents.
> + *
> + * Must be called from the main AioContext.
> + *
> + * The caller must own the AioContext lock for the old AioContext of bs, but it
> + * must not own the AioContext lock for new_context (unless new_context is the
> + * same as the current context of bs).

:(  Too bad.

> + *
> + * @visited will accumulate all visited BdrvChild object. The caller is

*objects

> + * responsible for freeing the list afterwards.
> + */
> +static bool bdrv_parent_change_aio_context(BdrvChild *c, AioContext *ctx,

The comment says “this BlockDriverState”, but then this function is 
called “_parent_” and takes a BdrvChild object.  Not quite clear what is 
meant.

You could argue it doesn’t matter because the function acts on all 
parents and children recursively anyway, so it’s doesn’t matter whether 
this is about c->bs or c’s parent, which is true, but still, the wording 
doesn’t seem quite as clear to me as it could be.

I also wonder why this function is so syntactically focused on BdrvChild 
object when I would have thought that those objects aren’t really 
associated with any AioContext, only the BDS is.  A BdrvChild just gives 
you a way to change a BDS’s parents’ AioContext.  Is it because of 
bdrv_child_try_change_aio_context()’s ignore_child parameter?

> +                                           GSList **visited, Transaction *tran,
> +                                           Error **errp)
> +{
> +    GLOBAL_STATE_CODE();
> +    if (g_slist_find(*visited, c)) {
> +        return true;
> +    }
> +    *visited = g_slist_prepend(*visited, c);
> +
> +    /*
> +     * A BdrvChildClass that doesn't handle AioContext changes cannot
> +     * tolerate any AioContext changes
> +     */
> +    if (!c->klass->change_aio_ctx) {
> +        char *user = bdrv_child_user_desc(c);
> +        error_setg(errp, "Changing iothreads is not supported by %s", user);
> +        g_free(user);
> +        return false;
> +    }
> +    if (!c->klass->change_aio_ctx(c, ctx, visited, tran, errp)) {
> +        assert(!errp || *errp);
> +        return false;
> +    }

This makes me wonder how this is supposed to change the children’s 
AioContext, because traditionally, BdrvChildClass contains only 
callbacks that affect the parent.

> +    return true;
> +}
> +
>   bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
>                                       GSList **ignore, Error **errp)
>   {
> @@ -7448,6 +7493,18 @@ bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
>       return bdrv_can_set_aio_context(c->bs, ctx, ignore, errp);
>   }
>   
> +bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
> +                                   GSList **visited, Transaction *tran,
> +                                   Error **errp)
> +{
> +    GLOBAL_STATE_CODE();
> +    if (g_slist_find(*visited, c)) {

Not the first time that we’re using linear lookup for such a thing, but 
it really irks me badly.  I think in any language that would provide 
collections by default, we’d be using a hash map here.

> +        return true;
> +    }
> +    *visited = g_slist_prepend(*visited, c);
> +    return bdrv_change_aio_context(c->bs, ctx, visited, tran, errp);
> +}
> +

OK, so this looks like the function that will change the children’s 
AioContext, and now I guess we’ll have a function that invokes both 
bdrv_{child,parent}_change_aio_context()...  And yes, we do, that is 
bdrv_change_aio_context().  So now it looks to me like the comment 
that’s above bdrv_parent_change_aio_context() actually belongs to 
bdrv_change_aio_context().

>   /* @ignore will accumulate all visited BdrvChild object. The caller is
>    * responsible for freeing the list afterwards. */
>   bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
> @@ -7475,6 +7532,99 @@ bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>       return true;
>   }
>   
> +static void bdrv_drained_begin_commit(void *opaque)
> +{
> +    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
> +
> +    /* Paired with bdrv_drained_end in bdrv_drained_end_clean() */
> +    bdrv_drained_begin(state->bs);
> +}
> +
> +static void bdrv_drained_end_clean(void *opaque)
> +{
> +    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
> +    BlockDriverState *bs = (BlockDriverState *) state->bs;
> +    AioContext *new_context = state->new_ctx;
> +
> +    /*
> +     * drain only if bdrv_drained_begin_commit() and
> +     * bdrv_set_aio_context_commit() executed
> +     */
> +    if (bdrv_get_aio_context(bs) == new_context) {
> +        /* Paired with bdrv_drained_begin in bdrv_drained_begin_commit() */
> +        bdrv_drained_end(bs);
> +    }

So as far as I understood, in bdrv_set_aio_context_ignore(), we switch 
around the currently held context because this will poll bs’s context, 
and so because bs’s context changed, we now need to hold the new 
context.  I don’t know off the top of my head whether there’s a problem 
with holding both contexts simultaneously.

> +
> +    /* state is freed by set_aio_context->clean() */
> +}
> +
> +static void bdrv_set_aio_context_commit(void *opaque)
> +{
> +    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
> +    BlockDriverState *bs = (BlockDriverState *) state->bs;
> +    AioContext *new_context = state->new_ctx;
> +    assert_bdrv_graph_writable(bs);
> +
> +    bdrv_detach_aio_context(bs);
> +    bdrv_attach_aio_context(bs, new_context);
> +}
> +
> +static TransactionActionDrv set_aio_context = {
> +    .commit = bdrv_set_aio_context_commit,
> +    .clean = g_free,
> +};
> +
> +static TransactionActionDrv drained_begin_end = {
> +    .commit = bdrv_drained_begin_commit,
> +    .clean = bdrv_drained_end_clean,
> +};
> +
> +/*
> + * @visited will accumulate all visited BdrvChild object. The caller is
> + * responsible for freeing the list afterwards.
> + */
> +bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> +                             GSList **visited, Transaction *tran,
> +                             Error **errp)
> +{
> +    BdrvChild *c;
> +    BdrvStateSetAioContext *state;
> +
> +    GLOBAL_STATE_CODE();
> +
> +    if (bdrv_get_aio_context(bs) == ctx) {
> +        return true;
> +    }
> +
> +    QLIST_FOREACH(c, &bs->parents, next_parent) {
> +        if (!bdrv_parent_change_aio_context(c, ctx, visited, tran, errp)) {
> +            return false;
> +        }
> +    }
> +
> +    QLIST_FOREACH(c, &bs->children, next) {
> +        if (!bdrv_child_change_aio_context(c, ctx, visited, tran, errp)) {
> +            return false;
> +        }
> +    }
> +
> +    state = g_new(BdrvStateSetAioContext, 1);
> +    *state = (BdrvStateSetAioContext) {
> +        .new_ctx = ctx,
> +        .bs = bs,
> +    };
> +
> +    /* First half of transactions are drains */
> +    tran_add(tran, &drained_begin_end, state);
> +    /*
> +     * Second half of transactions takes care of setting the
> +     * AioContext and free the state
> +     */
> +    tran_add_tail(tran, &set_aio_context, state);

Completely subjective and I struggle to explain my feelings:

I don’t quite like the “first half, second half” explanation.  This 
makes it sound like these should be separate phases in the transaction, 
and if so, we should have an interface that allows you to put 
transaction handlers into an arbitrary number of phases.

We don’t have that interface, though, all we have are functions that 
carry the connotation of “execute as soon as possible” and “execute as 
late as possible”, so that if you use both in conjunction like here, you 
can be certain that drained_begin_end is executed before the respective 
set_aio_context.

The comments also struggle to convey how much black magic this is.

(Note that this is just my POV; also note that I’m not judging, black 
magic is cool, just difficult to grasp)

So what’s going on is (AFAIU) that we have this black magic 
drained_begin_end transaction which is added such that its commit 
handler will be run before the set_aio_context’s commit handler, but its 
clean handler will be run after set_aio_context’s commit handler.  So 
that’s why I call it black magic, because drained_begin_end can do both, 
it’ll first begin the drain, and then end it, all around some normal 
other transaction, such that this other one can operate on a completely 
drained graph.

I don’t hate this, it’s just that the comments don’t convey it.  I think 
there should be a big comment above drained_begin_end describing how 
it’s supposed to be used (i.e. you can wrap some other transaction in 
it, if you take care to add that other transaction with tran_add_tail(), 
and then its commit handlers will run on a drained graph); and then here 
we just describe that we’re doing exactly that, adding both transactions 
such that set_aio_context’s commit handler will run on a drained graph.


What I don’t like is that a BdrvChildClass.change_aio_ctx implementation 
must know this.  If it adds handlers to the transaction, it must be 
aware that if it does so with tran_add(), the commit handler won’t 
necessarily run on a fully drained graph; it must use tran_add_tail() 
for this, which isn’t obvious (and might be called a layering violation).

Perhaps that doesn’t matter because none of those .change_aio_ctx commit 
handlers will care, but, well.

The point is that the drained_begin_end transaction kind of abuses the 
system and adds new non-obvious semantics.

I can’t think of something better off the top of my head, though, and as 
long as this will remain the only place to call tran_add_tail(), I 
probably shouldn’t care.

...Ah, I can see patch 4 and 6 add exactly what I had feared.  Well, 
that isn’t great. :/

> +
> +    return true;
> +}
> +
>   int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>                                      BdrvChild *ignore_child, Error **errp)
>   {
> @@ -7498,6 +7648,53 @@ int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>       return 0;
>   }
>   
> +/*
> + * First, go recursively through all nodes in the graph, and see if they
> + * can change their AioContext.
> + * If so, add for each node a new transaction with a callback to change the
> + * AioContext with the new one.
> + * Once recursion is completed, if all nodes are allowed to change their
> + * AioContext then commit the transaction, running all callbacks in order.
> + * Otherwise don't do anything.

Nice explanation, but I’d start with something more simple to describe 
the external interface, like “Change bs's and recursively all of its 
parents' and children's AioContext to the given new context, returning 
an error if that isn't possible.  If ignore_child is not NULL, that 
child (and its subgraph) will not be touched.”

> + *
> + * This function still requires the caller to take the bs current
> + * AioContext lock, otherwise draining could fail since AIO_WAIT_WHILE

Well, let’s just say “will” instead of “could”.  Callers don’t need to 
know they can get lucky when they ignore the rules.

> + * assumes the lock is always held if bs is in another AioContext.
> + */
> +int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> +                                      BdrvChild *ignore_child, Error **errp)

Do the other functions need to be public?  Outside of this file, this 
one seems sufficient to me, but I may well be overlooking something.

Also, why is this function called bdrv_child_*, and not just 
bdrv_try_change_aio_context()?  (Or maybe have a 
bdrv_try_change_aio_context() variant without the ignore_child 
parameter, just like bdrv_try_set_aio_context()?)

Actually, wouldn’t just bdrv_change_aio_context() be sufficient?  I 
mean, it isn’t called bdrv_try_co_pwritev().  Most functions try to do 
something and return errors if they can’t.  Not sure if we need to make 
that clear in the name.

(I may be wrong, but I suspect bdrv_try_set_aio_context() is only named 
such that the compiler will check that the bdrv_set_aio_context() 
interface is completely unused; 
42a65f02f9b380bd8074882d5844d4ea033389cc’s commit message does seem to 
confirm that.)

> +{
> +    Transaction *tran;
> +    GSList *visited;
> +    int ret;
> +    GLOBAL_STATE_CODE();
> +
> +    tran = tran_new();
> +    visited = ignore_child ? g_slist_prepend(NULL, ignore_child) : NULL;
> +    ret = bdrv_change_aio_context(bs, ctx, &visited, tran, errp);
> +    g_slist_free(visited);
> +
> +    if (!ret) {
> +        /* just run clean() callbacks */
> +        tran_abort(tran);
> +        return -EPERM;
> +    }
> +
> +    /* Acquire the new context, if necessary */
> +    /* TODO: why do we need to take this AioContext lock? */

As I’ve said somewhere above, I think it’s because we need it for 
bdrv_drained_end().  I don’t know if there’s a problem with holding both 
contexts simultaneously.  (I would’ve just assumed that there must be a 
reason for bdrv_set_aio_context_ignore() to release old_context, but 
maybe there is actually no reason to it?)

> +    if (qemu_get_aio_context() != ctx) {
> +        aio_context_acquire(ctx);
> +    }
> +
> +    tran_commit(tran);
> +
> +    if (qemu_get_aio_context() != ctx) {
> +        aio_context_release(ctx);
> +    }
> +
> +    return 0;
> +}

Might be nice to have a version that has @tran in its interface so 
callers can incorporate it; Vladimir wanted transactionable 
context-setting.  Then again, doesn’t seem to hard to introduce that 
when needed.

> +
>   int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>                                Error **errp)
>   {

I like the idea of reimplementing it all on top of transactions!  I 
think it needs some tuning, but it definitely looks doable.

Hanna

Re: [RFC PATCH 4/8] blockjob: implement .change_aio_ctx in child_job

[Hanna Reitz]

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> child_job_change_aio_ctx() is very similar to
> child_job_can_set_aio_ctx(), but it implements a new transaction
> so that if all check pass, the new transaction's .commit()
> will take care of changin the BlockJob AioContext.
> child_job_set_aio_ctx_commit() is similar to child_job_set_aio_ctx(),
> but it doesn't need to invoke the recursion, as this is already
> taken care by child_job_change_aio_ctx().
>
> Note: bdrv_child_try_change_aio_context() is not called by
> anyone at this point.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   blockjob.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)

Looks good, disregarding the fact that I’d like it very much if we could 
find some other primitive than tran_add_trail() to get these handlers to 
run on a drained graph.

But that’s taste (and something to talk about in patch 3), so I’ll just 
give a

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [RFC PATCH 5/8] block: implement .change_aio_ctx in child_of_bds

[Hanna Reitz]

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> bdrv_child_cb_change_aio_ctx() is identical to
> bdrv_child_cb_can_set_aio_ctx(), as we only need
> to recursively go on the parent bs.
>
> Note: bdrv_child_try_change_aio_context() is not called by
> anyone at this point.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c | 9 +++++++++
>   1 file changed, 9 insertions(+)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [RFC PATCH 6/8] block-backend: implement .change_aio_ctx in child_root

[Hanna Reitz]

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> blk_root_change_aio_ctx() is very similar to blk_root_can_set_aio_ctx(),
> but implements a new transaction so that if all check pass, the new
> transaction's .commit will take care of changing the BlockBackend
> AioContext. blk_root_set_aio_ctx_commit() is the same as
> blk_root_set_aio_ctx().
>
> Note: bdrv_child_try_change_aio_context() is not called by
> anyone at this point.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/block-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index f425b00793..674eaaa2bf 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c

[...]

> @@ -2208,6 +2212,56 @@ int blk_set_aio_context(BlockBackend *blk, AioContext *new_context,

[...]

> +static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx,
> +                                    GSList **visited, Transaction *tran,
> +                                    Error **errp)
> +{
> +    BlockBackend *blk = child->opaque;
> +    BdrvStateBlkRootContext *s;
> +
> +    if (blk->allow_aio_context_change) {
> +        goto finish;
> +    }
> +
> +    /*
> +     * Only manually created BlockBackends that are not attached to anything
> +     * can change their AioContext without updating their user.
> +     */
> +    if (!blk->name || blk->dev) {
> +        /* TODO Add BB name/QOM path */
> +        error_setg(errp, "Cannot change iothread of active block backend");
> +        return false;
> +    }

Is the goto really necessary?  Or, rather, do you prefer this to 
something like

if (!blk->allow_aio_context_change) {
     /*
      * Manually created BlockBackends (those with a name) that are not
      * attached to anything can change their AioContext without updating
      * their user; return an error for others.
      */
     if (!blk->name || blk->dev) {
         ...
     }
}

If you prefer the goto, I’d at least rename the label to 
“change_context” or “allowed” or something.

Hanna

> +
> +finish:
> +    s = g_new(BdrvStateBlkRootContext, 1);
> +    *s = (BdrvStateBlkRootContext) {
> +        .new_ctx = ctx,
> +        .blk = blk,
> +    };
> +
> +    tran_add_tail(tran, &set_blk_root_context, s);

(Again, not a huge fan of this.)

> +    return true;
> +}
> +
>   static bool blk_root_can_set_aio_ctx(BdrvChild *child, AioContext *ctx,
>                                        GSList **ignore, Error **errp)
>   {

Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

[Hanna Reitz]

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> Replace all direct usage of ->can_set_aio_ctx and ->set_aio_ctx,
> and call bdrv_child_try_change_aio_context() in
> bdrv_try_set_aio_context(), the main function called through
> the whole block layer.
>
>  From this point onwards, ->can_set_aio_ctx and ->set_aio_ctx
> won't be used anymore.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c               | 30 ++++++++++++++++--------------
>   block/block-backend.c |  8 ++++++--
>   2 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index a7ba590dfa..101188a2d4 100644
> --- a/block.c
> +++ b/block.c
> @@ -2966,17 +2966,18 @@ static void bdrv_attach_child_common_abort(void *opaque)
>       }
>   
>       if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
> +        Transaction *tran;
>           GSList *ignore;
> +        bool ret;
>   
> -        /* 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);
> +        tran = tran_new();
>   
> +        /* No need to ignore `child`, because it has been detached already */
>           ignore = NULL;
> -        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
> +        ret = child->klass->change_aio_ctx(child, s->old_parent_ctx, &ignore,
> +                                           tran, &error_abort);
>           g_slist_free(ignore);
> +        tran_finalize(tran, ret ? ret : -1);

As far as I understand, the transaction is supposed to always succeed; 
that’s why we pass `&error_abort`, I thought.

If so, `ret` should always be true.  More importantly, though, I think 
the `ret ? ret : -1` is wrong because it’ll always evaluate to either 1 
or -1, but never to 0, which would indicate success.  I think it should 
be `ret == true ? 0 : -1`, or even better `assert(ret == true); 
tran_finalize(tran, 0);`.

>       }
>   
>       bdrv_unref(bs);
> @@ -3037,17 +3038,18 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
>           Error *local_err = NULL;
>           int ret = bdrv_try_set_aio_context(child_bs, parent_ctx, &local_err);
>   
> -        if (ret < 0 && child_class->can_set_aio_ctx) {
> +        if (ret < 0 && child_class->change_aio_ctx) {
> +            Transaction *tran = tran_new();
>               GSList *ignore = g_slist_prepend(NULL, new_child);
> -            if (child_class->can_set_aio_ctx(new_child, child_ctx, &ignore,
> -                                             NULL))
> -            {
> +            bool ret_child;
> +
> +            ret_child = child_class->change_aio_ctx(new_child, child_ctx,
> +                                                    &ignore, tran, NULL);
> +            if (ret_child) {

To be honest, due to the mix of return value styles we have, perhaps a 
`ret_child == true` would help to signal that this is a success path.

>                   error_free(local_err);
>                   ret = 0;
> -                g_slist_free(ignore);
> -                ignore = g_slist_prepend(NULL, new_child);
> -                child_class->set_aio_ctx(new_child, child_ctx, &ignore);
>               }
> +            tran_finalize(tran, ret_child ? ret_child : -1);

This too should probably be `ret_child == true ? 0 : -1`.

>               g_slist_free(ignore);
>           }
>   
> @@ -7708,7 +7710,7 @@ int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>                                Error **errp)
>   {
>       GLOBAL_STATE_CODE();
> -    return bdrv_child_try_set_aio_context(bs, ctx, NULL, errp);
> +    return bdrv_child_try_change_aio_context(bs, ctx, NULL, errp);

Why not remove this function and adjust all callers?

Hanna

>   }
>   
>   void bdrv_add_aio_context_notifier(BlockDriverState *bs,

Re: [RFC PATCH 8/8] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

[Hanna Reitz]

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> Together with all _can_set_ and _set_ APIs, as they are not needed
> anymore.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c                            | 196 -----------------------------
>   block/block-backend.c              |  33 -----
>   blockjob.c                         |  35 ------
>   include/block/block-global-state.h |   9 --
>   include/block/block_int-common.h   |   4 -
>   5 files changed, 277 deletions(-)

Looks good!  I’d just like a follow-up commit that also drops 
bdrv_try_set_aio_context(), so it’s all gone (I think that’s the final 
remnant?).

Hanna

Re: [RFC PATCH 8/8] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

[Emanuele Giuseppe Esposito]

Am 15/07/2022 um 16:34 schrieb Hanna Reitz:
> On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
>> Together with all _can_set_ and _set_ APIs, as they are not needed
>> anymore.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   block.c                            | 196 -----------------------------
>>   block/block-backend.c              |  33 -----
>>   blockjob.c                         |  35 ------
>>   include/block/block-global-state.h |   9 --
>>   include/block/block_int-common.h   |   4 -
>>   5 files changed, 277 deletions(-)
> 
> Looks good!  I’d just like a follow-up commit that also drops
> bdrv_try_set_aio_context(), so it’s all gone (I think that’s the final
> remnant?).
> 

It's the same for me, I thought renaming bdrv_try_set_aio_context was a
little bit unnecessary. You want to rename it to something else, or
directly call bdrv_child_try_change_aio_context?

I agree with the rest of comments in this series :)

Emanuele

Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

[Emanuele Giuseppe Esposito]

Am 14/07/2022 um 18:45 schrieb Hanna Reitz:
> On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
>> ---------
>> RFC because I am not sure about the AioContext locks.
>> - Do we need to take the new AioContext lock? what does it protect?
>> - Taking the old AioContext lock is required now, because of
>>    bdrv_drained_begin calling AIO_WAIT_WHILE that unlocks the
>>    aiocontext. If we replace it with AIO_WAIT_WHILE_UNLOCKED,
>>    could we get rid of taking every time the old AioContext?
>>    drain would be enough to protect the graph modification.
> 
> It’s been a while, but as far as I remember (which may be wrong), the
> reason for how the locks are supposed to be taken was mostly that we
> need some defined state so that we know how to invoke
> bdrv_drained_begin() and bdrv_drained_end() (i.e. call the first one
> as-is, and switch the locks around for the latter one).

Right, so bdrv_drained_begin must be always under old aiocontext lock,
bdrv_drained_end always under the new one.

If so, then I think I can make the whole thing even cleaner:

1. bdrv_drained_begin is taken in bdrv_change_aio_context, and not in a
transaction. This was the initial idea, but somehow it didn't work,
something was failing. However, I must have changed something now
because all tests are passing :)
Then old aiocontext lock is assumed to be taken by the caller.

2. old aiocontext lock is released

3. set_aio_context (actual detach + attach operation) are kept in a
commit transaciton, as it is now. They don't need any aiocontext lock,
as far as I understand. bdrv_drained_end is implemented in a .clean
transaction

4. new aiocontext lock is taken

5. either tran_abort or tran_commit, depending on the result of the
recursion.

Disadvantage: the unlock/lock mess is still there, but at least we know
now why we need that.
Advantage: nothing should break (because it is similar as it was
before), no double lock held, and I can always add an additional patch
where I use _UNLOCKED functions and see that happens. In addition, no
tran_add_tail needed.

> 
> The idea of using _UNLOCKED sounds interesting, almost too obvious. I
> can’t see why that wouldn’t work, actually.
> 
>> ----------
>>
>> Simplify the way the aiocontext can be changed in a BDS graph.
>> There are currently two problems in bdrv_try_set_aio_context:
>> - There is a confusion of AioContext locks taken and released, because
>>    we assume that old aiocontext is always taken and new one is
>>    taken inside.
> 
> Yep, and that assumption is just broken in some cases, which is the main
> pain point I’m feeling with it right now.
> 
> For example, look at bdrv_attach_child_common(): Here, we attach a child
> to a parent, so we need to get them into a common AioContext. So first
> we try to put the child into the parent’s context, and if that fails,
> we’ll try the other way, putting the parent into the child’s context.
> 
> The problem is clear: The bdrv_try_set_aio_context() call requires us to
> hold the child’s current context but not the parent’s, and the
> child_class->set_aio_ctx() call requires the exact opposite.  But we
> never switch the context we have acquired, so this can’t both work. 
> Better yet, nowhere is it defined what context a caller to
> bdrv_attach_child_common() will hold.
> 
> In practice, what happens here most of the time is that something will
> be moved from the main context to some other context, and since we’re in
> the main context already, that’ll just work.  But you can construct
> cases where something is attempted to be moved from an I/O thread into a
> different thread and then you’ll get a crash.
> 
> I’d be happy if we could do away with the requirement of having to hold
> any lock for changing a node’s AioContext.
> 
>> - It doesn't look very safe to call bdrv_drained_begin while some
>>    nodes have already switched to the new aiocontext and others haven't.
>>    This could be especially dangerous because bdrv_drained_begin
>> polls, so
>>    something else could be executed while graph is in an inconsistent
>>    state.
>>
>> Additional minor nitpick: can_set and set_ callbacks both traverse the
>> graph, both using the ignored list of visited nodes in a different way.
>>
>> Therefore, get rid of all of this and introduce a new callback,
>> change_aio_context, that uses transactions to efficiently, cleanly
>> and most importantly safely change the aiocontext of a graph.
>>
>> This new callback is a "merge" of the two previous ones:
>> - Just like can_set_aio_context, recursively traverses the graph.
>>    Marks all nodes that are visited using a GList, and checks if
>>    they *could* change the aio_context.
>> - For each node that passes the above check, add a new transaction
>>    that implements a callback that effectively changes the aiocontext.
>> - If a node is a BDS, add two transactions: one taking care of draining
>>    the node at the beginning of the list (so that will be executed first)
>>    and one at the end taking care of changing the AioContext.
>> - Once done, the recursive function returns if *all* nodes can change
>>    the AioContext. If so, commit the above transactions. Otherwise don't
>>    do anything.
>> - The transaction list contains first all "drain" transactions, so
>>    we are sure we are draining all nodes in the same context, and then
>>    all the other switch the AioContext. In this way we make sure that
>>    bdrv_drained_begin() is always called under the old AioContext, and
>>    bdrv_drained_end() under the new one.
>> - Because of the above, we don't need to release and re-acquire the
>>    old AioContext every time, as everything is done once (and not
>>    per-node drain and aiocontext change).
>>
>> Note that the "change" API is not yet invoked anywhere.
> 
> So the idea is that we introduce a completely new transaction-based API
> to change BDSs’ AioContext, and then drop the old one, right?
> 
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   block.c                            | 197 +++++++++++++++++++++++++++++
>>   include/block/block-global-state.h |   9 ++
>>   include/block/block_int-common.h   |   3 +
>>   3 files changed, 209 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 267a39c0de..bda4e1bcef 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -7437,6 +7437,51 @@ static bool
>> bdrv_parent_can_set_aio_context(BdrvChild *c, AioContext *ctx,
>>       return true;
>>   }
>>   +typedef struct BdrvStateSetAioContext {
>> +    AioContext *new_ctx;
>> +    BlockDriverState *bs;
>> +} BdrvStateSetAioContext;
>> +
>> +/*
>> + * Changes the AioContext used for fd handlers, timers, and BHs by this
>> + * BlockDriverState and all its children and parents.
>> + *
>> + * Must be called from the main AioContext.
>> + *
>> + * The caller must own the AioContext lock for the old AioContext of
>> bs, but it
>> + * must not own the AioContext lock for new_context (unless
>> new_context is the
>> + * same as the current context of bs).
> 
> :(  Too bad.
> 
>> + *
>> + * @visited will accumulate all visited BdrvChild object. The caller is
> 
> *objects
> 
>> + * responsible for freeing the list afterwards.
>> + */
>> +static bool bdrv_parent_change_aio_context(BdrvChild *c, AioContext
>> *ctx,
> 
> The comment says “this BlockDriverState”, but then this function is
> called “_parent_” and takes a BdrvChild object.  Not quite clear what is
> meant.
> 
> You could argue it doesn’t matter because the function acts on all
> parents and children recursively anyway, so it’s doesn’t matter whether
> this is about c->bs or c’s parent, which is true, but still, the wording
> doesn’t seem quite as clear to me as it could be.
> 
> I also wonder why this function is so syntactically focused on BdrvChild
> object when I would have thought that those objects aren’t really
> associated with any AioContext, only the BDS is.  A BdrvChild just gives
> you a way to change a BDS’s parents’ AioContext.  Is it because of
> bdrv_child_try_change_aio_context()’s ignore_child parameter?
> 
>> +                                           GSList **visited,
>> Transaction *tran,
>> +                                           Error **errp)
>> +{
>> +    GLOBAL_STATE_CODE();
>> +    if (g_slist_find(*visited, c)) {
>> +        return true;
>> +    }
>> +    *visited = g_slist_prepend(*visited, c);
>> +
>> +    /*
>> +     * A BdrvChildClass that doesn't handle AioContext changes cannot
>> +     * tolerate any AioContext changes
>> +     */
>> +    if (!c->klass->change_aio_ctx) {
>> +        char *user = bdrv_child_user_desc(c);
>> +        error_setg(errp, "Changing iothreads is not supported by %s",
>> user);
>> +        g_free(user);
>> +        return false;
>> +    }
>> +    if (!c->klass->change_aio_ctx(c, ctx, visited, tran, errp)) {
>> +        assert(!errp || *errp);
>> +        return false;
>> +    }
> 
> This makes me wonder how this is supposed to change the children’s
> AioContext, because traditionally, BdrvChildClass contains only
> callbacks that affect the parent.
> 
>> +    return true;
>> +}
>> +
>>   bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
>>                                       GSList **ignore, Error **errp)
>>   {
>> @@ -7448,6 +7493,18 @@ bool bdrv_child_can_set_aio_context(BdrvChild
>> *c, AioContext *ctx,
>>       return bdrv_can_set_aio_context(c->bs, ctx, ignore, errp);
>>   }
>>   +bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
>> +                                   GSList **visited, Transaction *tran,
>> +                                   Error **errp)
>> +{
>> +    GLOBAL_STATE_CODE();
>> +    if (g_slist_find(*visited, c)) {
> 
> Not the first time that we’re using linear lookup for such a thing, but
> it really irks me badly.  I think in any language that would provide
> collections by default, we’d be using a hash map here.
> 
>> +        return true;
>> +    }
>> +    *visited = g_slist_prepend(*visited, c);
>> +    return bdrv_change_aio_context(c->bs, ctx, visited, tran, errp);
>> +}
>> +
> 
> OK, so this looks like the function that will change the children’s
> AioContext, and now I guess we’ll have a function that invokes both
> bdrv_{child,parent}_change_aio_context()...  And yes, we do, that is
> bdrv_change_aio_context().  So now it looks to me like the comment
> that’s above bdrv_parent_change_aio_context() actually belongs to
> bdrv_change_aio_context().
> 
>>   /* @ignore will accumulate all visited BdrvChild object. The caller is
>>    * responsible for freeing the list afterwards. */
>>   bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>> @@ -7475,6 +7532,99 @@ bool bdrv_can_set_aio_context(BlockDriverState
>> *bs, AioContext *ctx,
>>       return true;
>>   }
>>   +static void bdrv_drained_begin_commit(void *opaque)
>> +{
>> +    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
>> +
>> +    /* Paired with bdrv_drained_end in bdrv_drained_end_clean() */
>> +    bdrv_drained_begin(state->bs);
>> +}
>> +
>> +static void bdrv_drained_end_clean(void *opaque)
>> +{
>> +    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
>> +    BlockDriverState *bs = (BlockDriverState *) state->bs;
>> +    AioContext *new_context = state->new_ctx;
>> +
>> +    /*
>> +     * drain only if bdrv_drained_begin_commit() and
>> +     * bdrv_set_aio_context_commit() executed
>> +     */
>> +    if (bdrv_get_aio_context(bs) == new_context) {
>> +        /* Paired with bdrv_drained_begin in
>> bdrv_drained_begin_commit() */
>> +        bdrv_drained_end(bs);
>> +    }
> 
> So as far as I understood, in bdrv_set_aio_context_ignore(), we switch
> around the currently held context because this will poll bs’s context,
> and so because bs’s context changed, we now need to hold the new
> context.  I don’t know off the top of my head whether there’s a problem
> with holding both contexts simultaneously.
> 
>> +
>> +    /* state is freed by set_aio_context->clean() */
>> +}
>> +
>> +static void bdrv_set_aio_context_commit(void *opaque)
>> +{
>> +    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
>> +    BlockDriverState *bs = (BlockDriverState *) state->bs;
>> +    AioContext *new_context = state->new_ctx;
>> +    assert_bdrv_graph_writable(bs);
>> +
>> +    bdrv_detach_aio_context(bs);
>> +    bdrv_attach_aio_context(bs, new_context);
>> +}
>> +
>> +static TransactionActionDrv set_aio_context = {
>> +    .commit = bdrv_set_aio_context_commit,
>> +    .clean = g_free,
>> +};
>> +
>> +static TransactionActionDrv drained_begin_end = {
>> +    .commit = bdrv_drained_begin_commit,
>> +    .clean = bdrv_drained_end_clean,
>> +};
>> +
>> +/*
>> + * @visited will accumulate all visited BdrvChild object. The caller is
>> + * responsible for freeing the list afterwards.
>> + */
>> +bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
>> +                             GSList **visited, Transaction *tran,
>> +                             Error **errp)
>> +{
>> +    BdrvChild *c;
>> +    BdrvStateSetAioContext *state;
>> +
>> +    GLOBAL_STATE_CODE();
>> +
>> +    if (bdrv_get_aio_context(bs) == ctx) {
>> +        return true;
>> +    }
>> +
>> +    QLIST_FOREACH(c, &bs->parents, next_parent) {
>> +        if (!bdrv_parent_change_aio_context(c, ctx, visited, tran,
>> errp)) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    QLIST_FOREACH(c, &bs->children, next) {
>> +        if (!bdrv_child_change_aio_context(c, ctx, visited, tran,
>> errp)) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    state = g_new(BdrvStateSetAioContext, 1);
>> +    *state = (BdrvStateSetAioContext) {
>> +        .new_ctx = ctx,
>> +        .bs = bs,
>> +    };
>> +
>> +    /* First half of transactions are drains */
>> +    tran_add(tran, &drained_begin_end, state);
>> +    /*
>> +     * Second half of transactions takes care of setting the
>> +     * AioContext and free the state
>> +     */
>> +    tran_add_tail(tran, &set_aio_context, state);
> 
> Completely subjective and I struggle to explain my feelings:
> 
> I don’t quite like the “first half, second half” explanation.  This
> makes it sound like these should be separate phases in the transaction,
> and if so, we should have an interface that allows you to put
> transaction handlers into an arbitrary number of phases.
> 
> We don’t have that interface, though, all we have are functions that
> carry the connotation of “execute as soon as possible” and “execute as
> late as possible”, so that if you use both in conjunction like here, you
> can be certain that drained_begin_end is executed before the respective
> set_aio_context.
> 
> The comments also struggle to convey how much black magic this is.
> 
> (Note that this is just my POV; also note that I’m not judging, black
> magic is cool, just difficult to grasp)
> 
> So what’s going on is (AFAIU) that we have this black magic
> drained_begin_end transaction which is added such that its commit
> handler will be run before the set_aio_context’s commit handler, but its
> clean handler will be run after set_aio_context’s commit handler.  So
> that’s why I call it black magic, because drained_begin_end can do both,
> it’ll first begin the drain, and then end it, all around some normal
> other transaction, such that this other one can operate on a completely
> drained graph.
> 
> I don’t hate this, it’s just that the comments don’t convey it.  I think
> there should be a big comment above drained_begin_end describing how
> it’s supposed to be used (i.e. you can wrap some other transaction in
> it, if you take care to add that other transaction with tran_add_tail(),
> and then its commit handlers will run on a drained graph); and then here
> we just describe that we’re doing exactly that, adding both transactions
> such that set_aio_context’s commit handler will run on a drained graph.
> 
> 
> What I don’t like is that a BdrvChildClass.change_aio_ctx implementation
> must know this.  If it adds handlers to the transaction, it must be
> aware that if it does so with tran_add(), the commit handler won’t
> necessarily run on a fully drained graph; it must use tran_add_tail()
> for this, which isn’t obvious (and might be called a layering violation).
> 
> Perhaps that doesn’t matter because none of those .change_aio_ctx commit
> handlers will care, but, well.
> 
> The point is that the drained_begin_end transaction kind of abuses the
> system and adds new non-obvious semantics.
> 
> I can’t think of something better off the top of my head, though, and as
> long as this will remain the only place to call tran_add_tail(), I
> probably shouldn’t care.
> 
> ...Ah, I can see patch 4 and 6 add exactly what I had feared.  Well,
> that isn’t great. :/
> 
>> +
>> +    return true;
>> +}
>> +
>>   int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext
>> *ctx,
>>                                      BdrvChild *ignore_child, Error
>> **errp)
>>   {
>> @@ -7498,6 +7648,53 @@ int
>> bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>>       return 0;
>>   }
>>   +/*
>> + * First, go recursively through all nodes in the graph, and see if they
>> + * can change their AioContext.
>> + * If so, add for each node a new transaction with a callback to
>> change the
>> + * AioContext with the new one.
>> + * Once recursion is completed, if all nodes are allowed to change their
>> + * AioContext then commit the transaction, running all callbacks in
>> order.
>> + * Otherwise don't do anything.
> 
> Nice explanation, but I’d start with something more simple to describe
> the external interface, like “Change bs's and recursively all of its
> parents' and children's AioContext to the given new context, returning
> an error if that isn't possible.  If ignore_child is not NULL, that
> child (and its subgraph) will not be touched.”
> 
>> + *
>> + * This function still requires the caller to take the bs current
>> + * AioContext lock, otherwise draining could fail since AIO_WAIT_WHILE
> 
> Well, let’s just say “will” instead of “could”.  Callers don’t need to
> know they can get lucky when they ignore the rules.
> 
>> + * assumes the lock is always held if bs is in another AioContext.
>> + */
>> +int bdrv_child_try_change_aio_context(BlockDriverState *bs,
>> AioContext *ctx,
>> +                                      BdrvChild *ignore_child, Error
>> **errp)
> 
> Do the other functions need to be public?  Outside of this file, this
> one seems sufficient to me, but I may well be overlooking something.
> 
> Also, why is this function called bdrv_child_*, and not just
> bdrv_try_change_aio_context()?  (Or maybe have a
> bdrv_try_change_aio_context() variant without the ignore_child
> parameter, just like bdrv_try_set_aio_context()?)
> 
> Actually, wouldn’t just bdrv_change_aio_context() be sufficient?  I
> mean, it isn’t called bdrv_try_co_pwritev().  Most functions try to do
> something and return errors if they can’t.  Not sure if we need to make
> that clear in the name.
> 
> (I may be wrong, but I suspect bdrv_try_set_aio_context() is only named
> such that the compiler will check that the bdrv_set_aio_context()
> interface is completely unused;
> 42a65f02f9b380bd8074882d5844d4ea033389cc’s commit message does seem to
> confirm that.)
> 
>> +{
>> +    Transaction *tran;
>> +    GSList *visited;
>> +    int ret;
>> +    GLOBAL_STATE_CODE();
>> +
>> +    tran = tran_new();
>> +    visited = ignore_child ? g_slist_prepend(NULL, ignore_child) : NULL;
>> +    ret = bdrv_change_aio_context(bs, ctx, &visited, tran, errp);
>> +    g_slist_free(visited);
>> +
>> +    if (!ret) {
>> +        /* just run clean() callbacks */
>> +        tran_abort(tran);
>> +        return -EPERM;
>> +    }
>> +
>> +    /* Acquire the new context, if necessary */
>> +    /* TODO: why do we need to take this AioContext lock? */
> 
> As I’ve said somewhere above, I think it’s because we need it for
> bdrv_drained_end().  I don’t know if there’s a problem with holding both
> contexts simultaneously.  (I would’ve just assumed that there must be a
> reason for bdrv_set_aio_context_ignore() to release old_context, but
> maybe there is actually no reason to it?)
> 
>> +    if (qemu_get_aio_context() != ctx) {
>> +        aio_context_acquire(ctx);
>> +    }
>> +
>> +    tran_commit(tran);
>> +
>> +    if (qemu_get_aio_context() != ctx) {
>> +        aio_context_release(ctx);
>> +    }
>> +
>> +    return 0;
>> +}
> 
> Might be nice to have a version that has @tran in its interface so
> callers can incorporate it; Vladimir wanted transactionable
> context-setting.  Then again, doesn’t seem to hard to introduce that
> when needed.
> 
>> +
>>   int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>>                                Error **errp)
>>   {
> 
> I like the idea of reimplementing it all on top of transactions!  I
> think it needs some tuning, but it definitely looks doable.
> 
> Hanna
> 

Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

[Emanuele Giuseppe Esposito]

Am 14/07/2022 um 18:45 schrieb Hanna Reitz:
>> + * First, go recursively through all nodes in the graph, and see if they
>> + * can change their AioContext.
>> + * If so, add for each node a new transaction with a callback to
>> change the
>> + * AioContext with the new one.
>> + * Once recursion is completed, if all nodes are allowed to change their
>> + * AioContext then commit the transaction, running all callbacks in
>> order.
>> + * Otherwise don't do anything.
> 
> Nice explanation, but I’d start with something more simple to describe
> the external interface, like “Change bs's and recursively all of its
> parents' and children's AioContext to the given new context, returning
> an error if that isn't possible.  If ignore_child is not NULL, that
> child (and its subgraph) will not be touched.”
You want to have your suggestion as a replacement or addition to mine?
> 
>> + *
>> + * This function still requires the caller to take the bs current
>> + * AioContext lock, otherwise draining could fail since AIO_WAIT_WHILE
> 
> Well, let’s just say “will” instead of “could”.  Callers don’t need to
> know they can get lucky when they ignore the rules.
> 
>> + * assumes the lock is always held if bs is in another AioContext.
>> + */
>> +int bdrv_child_try_change_aio_context(BlockDriverState *bs,
>> AioContext *ctx,
>> +                                      BdrvChild *ignore_child, Error
>> **errp)
> 
> Do the other functions need to be public?  Outside of this file, this
> one seems sufficient to me, but I may well be overlooking something.
> 
> Also, why is this function called bdrv_child_*, and not just
> bdrv_try_change_aio_context()?  (Or maybe have a
> bdrv_try_change_aio_context() variant without the ignore_child
> parameter, just like bdrv_try_set_aio_context()?)
> 
> Actually, wouldn’t just bdrv_change_aio_context() be sufficient?  I
> mean, it isn’t called bdrv_try_co_pwritev().  Most functions try to do
> something and return errors if they can’t.  Not sure if we need to make
> that clear in the name.
> 
> (I may be wrong, but I suspect bdrv_try_set_aio_context() is only named
> such that the compiler will check that the bdrv_set_aio_context()
> interface is completely unused;
> 42a65f02f9b380bd8074882d5844d4ea033389cc’s commit message does seem to
> confirm that.)

Ok, I will change the name in bdrv_change_aio_context. And I now
understand your suggestion on the last patch to remove
bdrv_try_set_aio_context.

Emanuele

Re: [RFC PATCH 8/8] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

[Emanuele Giuseppe Esposito]

Am 18/07/2022 um 10:45 schrieb Emanuele Giuseppe Esposito:
> 
> 
> Am 15/07/2022 um 16:34 schrieb Hanna Reitz:
>> On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
>>> Together with all _can_set_ and _set_ APIs, as they are not needed
>>> anymore.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>   block.c                            | 196 -----------------------------
>>>   block/block-backend.c              |  33 -----
>>>   blockjob.c                         |  35 ------
>>>   include/block/block-global-state.h |   9 --
>>>   include/block/block_int-common.h   |   4 -
>>>   5 files changed, 277 deletions(-)
>>
>> Looks good!  I’d just like a follow-up commit that also drops
>> bdrv_try_set_aio_context(), so it’s all gone (I think that’s the final
>> remnant?).
>>
> 
> It's the same for me, I thought renaming bdrv_try_set_aio_context was a
> little bit unnecessary. You want to rename it to something else, or
> directly call bdrv_child_try_change_aio_context?

Wait we have 2 functions that need to be renamed:

- bdrv_child_try_change_aio_context, called only once in block-backend
because we need the ignore_child parameter.
- bdrv_try_set_aio_context, public, called everywhere, wraps
bdrv_child_try_change_aio_context setting ignore_child=NULL.

Name suggestions?


> 
> I agree with the rest of comments in this series :)
> 
> Emanuele
> 

Re: [RFC PATCH 2/8] transactions: add tran_add_back

[Paolo Bonzini]

On 7/14/22 17:13, Hanna Reitz wrote:
>> that we want to run before the others but still only when invoking
>> finalize/commit/abort.
> 
> I don’t understand this yet (but perhaps it’ll become clearer with the 
> following patches); doesn’t the new function do the opposite? I.e., 
> basically add some clean-up that’s only used after everything else?

I agree about the commit message being incorrect, but is there any 
reason why transactions work LIFO by default?  Is there anything that 
requires that behavior?

Paolo

Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

[Paolo Bonzini]

On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote:
> +        /* No need to ignore `child`, because it has been detached already */
>           ignore = NULL;
> -        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
> +        ret = child->klass->change_aio_ctx(child, s->old_parent_ctx, &ignore,
> +                                           tran, &error_abort);
>           g_slist_free(ignore);
> +        tran_finalize(tran, ret ? ret : -1);

Should this instead assert that ret is true, and call tran_commit() 
directly?

Paolo

Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

[Paolo Bonzini]

On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote:
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 674eaaa2bf..6e90ac3a6a 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2184,8 +2184,12 @@ static int blk_do_set_aio_context(BlockBackend *blk, AioContext *new_context,
>           bdrv_ref(bs);
>   
>           if (update_root_node) {
> -            ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
> -                                                 errp);
> +            /*
> +             * update_root_node MUST be false for blk_root_set_aio_ctx_commit(),
> +             * as we are already in the commit function of a transaction.
> +             */
> +            ret = bdrv_child_try_change_aio_context(bs, new_context, blk->root,
> +                                                    errp);
>               if (ret < 0) {
>                   bdrv_unref(bs);
>                   return ret;


Looking further at blk_do_set_aio_context:

         if (tgm->throttle_state) {
             bdrv_drained_begin(bs);
             throttle_group_detach_aio_context(tgm);
             throttle_group_attach_aio_context(tgm, new_context);
             bdrv_drained_end(bs);
         }

Perhaps the drained_begin/drained_end pair can be moved to 
blk_set_aio_context?  It shouldn't be needed from the change_aio_ctx 
callback, because bs is already drained.  If so, blk_do_set_aio_context 
would become just:

      if (tgm->throttle_state) {
          throttle_group_detach_aio_context(tgm);
          throttle_group_attach_aio_context(tgm, new_context);
      }
      blk->ctx = new_context;

and blk_set_aio_context would be something like:

     if (bs) {
         bdrv_ref(bs);
         ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
                                              errp);
         if (ret < 0) {
             goto out_no_drain;
         }
         bdrv_drained_begin(bs);
     }
     ret = blk_do_set_aio_context(blk, new_context, errp);
     if (bs) {
         bdrv_drained_end(bs);
out_no_drain;
         bdrv_unref(bs);
     }
     return ret;

Paolo

Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

[Emanuele Giuseppe Esposito]

Am 18/07/2022 um 18:39 schrieb Paolo Bonzini:
> On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote:
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 674eaaa2bf..6e90ac3a6a 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2184,8 +2184,12 @@ static int blk_do_set_aio_context(BlockBackend
>> *blk, AioContext *new_context,
>>           bdrv_ref(bs);
>>             if (update_root_node) {
>> -            ret = bdrv_child_try_set_aio_context(bs, new_context,
>> blk->root,
>> -                                                 errp);
>> +            /*
>> +             * update_root_node MUST be false for
>> blk_root_set_aio_ctx_commit(),
>> +             * as we are already in the commit function of a
>> transaction.
>> +             */
>> +            ret = bdrv_child_try_change_aio_context(bs, new_context,
>> blk->root,
>> +                                                    errp);
>>               if (ret < 0) {
>>                   bdrv_unref(bs);
>>                   return ret;
> 
> 
> Looking further at blk_do_set_aio_context:
> 
>         if (tgm->throttle_state) {
>             bdrv_drained_begin(bs);
>             throttle_group_detach_aio_context(tgm);
>             throttle_group_attach_aio_context(tgm, new_context);
>             bdrv_drained_end(bs);
>         }
> 
> Perhaps the drained_begin/drained_end pair can be moved to
> blk_set_aio_context?  It shouldn't be needed from the change_aio_ctx
> callback, because bs is already drained.  If so, blk_do_set_aio_context
> would become just:
> 
>      if (tgm->throttle_state) {
>          throttle_group_detach_aio_context(tgm);
>          throttle_group_attach_aio_context(tgm, new_context);
>      }
>      blk->ctx = new_context;
> 
> and blk_set_aio_context would be something like:
> 
>     if (bs) {
>         bdrv_ref(bs);
>         ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
>                                              errp);
>         if (ret < 0) {
>             goto out_no_drain;
>         }
>         bdrv_drained_begin(bs);
>     }
>     ret = blk_do_set_aio_context(blk, new_context, errp);
>     if (bs) {
>         bdrv_drained_end(bs);
> out_no_drain;
>         bdrv_unref(bs);
>     }
>     return ret;
> 
> Paolo
> 

This is another example on how things here do not take into account many
other use cases outside the common ones: if I use the above suggestion,
test-block-iothread fails.

The reason why it fails is simple: now we drain regardless of
tgm->throttle_state being true or false. And this requires yet another
aiocontext lock.

Which means that if we are calling blk_set_aio_context where the bs
exists and tgm->throttle_state is true, we have a bug.

Test is test_attach_blockjob and function is line 435
blk_set_aio_context(blk, ctx, &error_abort);

The reason is that bs is first switched to the new aiocontext and then
we try to drain it without holding the lock.

Wrapping the new drains in aio_context_acquire/release(new_context) is
not so much helpful either, since apparently the following
blk_set_aio_context makes aio_poll() hang.
I am not sure why, any ideas?


Code:

static int blk_do_set_aio_context(BlockBackend *blk, AioContext
*new_context,
                                  Error **errp)
{
    BlockDriverState *bs = blk_bs(blk);
    ThrottleGroupMember *tgm = &blk->public.throttle_group_member;

    if (bs) {
        bdrv_ref(bs);

        if (tgm->throttle_state) {
            throttle_group_detach_aio_context(tgm);
            throttle_group_attach_aio_context(tgm, new_context);
        }

        bdrv_unref(bs);
    }

    blk->ctx = new_context;
    return 0;
}


int blk_set_aio_context(BlockBackend *blk, AioContext *new_context,
                        Error **errp)
{
    BlockDriverState *bs = blk_bs(blk);
    int ret;
    GLOBAL_STATE_CODE();

    if (bs) {
        bdrv_ref(bs);
        ret = bdrv_child_try_change_aio_context(bs, new_context, blk->root,
                                                errp);
        if (ret < 0) {
            goto out_no_drain;
        }
        if (new_context != qemu_get_aio_context()) {
            aio_context_acquire(new_context);
        }
        bdrv_drained_begin(bs); // <-------------------- hangs here!
    }

    ret = blk_do_set_aio_context(blk, new_context, errp);

    if (bs) {
        bdrv_drained_end(bs);
        if (new_context != qemu_get_aio_context()) {
            aio_context_release(new_context);
        }
out_no_drain:
        bdrv_unref(bs);
    }

    return ret;
}

Emanuele

Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

[Paolo Bonzini]

On 7/19/22 11:57, Emanuele Giuseppe Esposito wrote:
> 
> Wrapping the new drains in aio_context_acquire/release(new_context) is
> not so much helpful either, since apparently the following
> blk_set_aio_context makes aio_poll() hang.
> I am not sure why, any ideas?

I'll take a look, thanks.  In any case this doesn't block this series, 
it was just a suggestion and blk_do_set_aio_context can be improved on top.

Paolo

Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

[Emanuele Giuseppe Esposito]

Am 19/07/2022 um 20:07 schrieb Paolo Bonzini:
> On 7/19/22 11:57, Emanuele Giuseppe Esposito wrote:
>>
>> Wrapping the new drains in aio_context_acquire/release(new_context) is
>> not so much helpful either, since apparently the following
>> blk_set_aio_context makes aio_poll() hang.
>> I am not sure why, any ideas?
> 
> I'll take a look, thanks.  In any case this doesn't block this series,
> it was just a suggestion and blk_do_set_aio_context can be improved on top.
> 

Neither is the "using drain_begin/end_unlocked" part. That won't be
straightforward either. That will go in a future serie.

Emanuele

Re: [RFC PATCH 2/8] transactions: add tran_add_back

[Vladimir Sementsov-Ogievskiy]

On 7/18/22 19:20, Paolo Bonzini wrote:
> On 7/14/22 17:13, Hanna Reitz wrote:
>>> that we want to run before the others but still only when invoking
>>> finalize/commit/abort.
>>
>> I don’t understand this yet (but perhaps it’ll become clearer with the following patches); doesn’t the new function do the opposite? I.e., basically add some clean-up that’s only used after everything else?
> 
> I agree about the commit message being incorrect, but is there any reason why transactions work LIFO by default?  Is there anything that requires that behavior?
> 

Yes. On abort() we want to rollback actions in reverse order. When we create _abort() part of some action we assume that current graph state is exactly the same like it was after _prepare() call, otherwise it just will not work. That means that all actions called _after_ action X, are already rolled-back when we call X_abort().

And keeping that in mind I'm afraid of implementing a possibility to break this order..

Note also, that in block transaction API, most of the action is usually done in _prepare(), so that next action in transaction can rely on result of previous action.



-- 
Best regards,
Vladimir

Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

[Vladimir Sementsov-Ogievskiy]

On 7/13/22 00:19, Emanuele Giuseppe Esposito wrote:
> +/*
> + * @visited will accumulate all visited BdrvChild object. The caller is
> + * responsible for freeing the list afterwards.
> + */
> +bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> +                             GSList **visited, Transaction *tran,
> +                             Error **errp)
> +{
> +    BdrvChild *c;
> +    BdrvStateSetAioContext *state;
> +
> +    GLOBAL_STATE_CODE();
> +
> +    if (bdrv_get_aio_context(bs) == ctx) {
> +        return true;
> +    }
> +
> +    QLIST_FOREACH(c, &bs->parents, next_parent) {
> +        if (!bdrv_parent_change_aio_context(c, ctx, visited, tran, errp)) {
> +            return false;
> +        }
> +    }
> +
> +    QLIST_FOREACH(c, &bs->children, next) {
> +        if (!bdrv_child_change_aio_context(c, ctx, visited, tran, errp)) {
> +            return false;
> +        }
> +    }
> +
> +    state = g_new(BdrvStateSetAioContext, 1);
> +    *state = (BdrvStateSetAioContext) {
> +        .new_ctx = ctx,
> +        .bs = bs,
> +    };
> +
> +    /* First half of transactions are drains */
> +    tran_add(tran, &drained_begin_end, state);
> +    /*
> +     * Second half of transactions takes care of setting the
> +     * AioContext and free the state
> +     */
> +    tran_add_tail(tran, &set_aio_context, state);
> +
> +    return true;
> +}


First, it looks like you want to use .commit() as .prepare(), .clean() as commit(), and .prepare() as something that just schedule other actions.

In block transaction any effect that is visible to other transaction actions should be done in .prepare(). (and .prepare is the main function of the action with *tran parameter, i.e. bdrv_change_aio_context() function is actually .prepare() phase).

So, ideally, the action:

  - does the complete thing in .prepare (i.e. in the main function of the action)
  - rollback it in .abort
  - nothing in .commit

Usually we still need a .commit phase for irreversible part of the action (like calling free() on the object). But that should be transparent for other actions.

So, generally we do:

tran = tran_new()

action_a(..., tran);
action_b(..., tran);
action_c(..., tran);


And we expect, that action_b() operates on top of action_a() already done. And action_c() operate on top of action_b() done. So we cannot postpone visible (to other actions) effect of the action to .commit phase.

So, for example, if you want a kind of smart drained section in transaction, you may add simple

bdrv_drain_tran(..., tran);

that just call drained_begin(), and have only .clean() handler that call drained_end(). This way you may do

bdrv_drain_tran(...., tran);
action_a(..., tran);
action_b(..., tran);

And be sure that both actions and all their .abort/.commit/.clean handlers are called inside drained seaction. Isn't it what you need?


Second, this all becomes extremely difficult when we mix transactions and recursion. When I moved permission system to transactions, I've introduced topological dfs search to just get a list of nodes to handle. I think, if we want to rework aio context change, we should first get rid of recursion.

-- 
Best regards,
Vladimir

Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

[Emanuele Giuseppe Esposito]

Am 20/07/2022 um 16:09 schrieb Vladimir Sementsov-Ogievskiy:
> On 7/13/22 00:19, Emanuele Giuseppe Esposito wrote:
>> +/*
>> + * @visited will accumulate all visited BdrvChild object. The caller is
>> + * responsible for freeing the list afterwards.
>> + */
>> +bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
>> +                             GSList **visited, Transaction *tran,
>> +                             Error **errp)
>> +{
>> +    BdrvChild *c;
>> +    BdrvStateSetAioContext *state;
>> +
>> +    GLOBAL_STATE_CODE();
>> +
>> +    if (bdrv_get_aio_context(bs) == ctx) {
>> +        return true;
>> +    }
>> +
>> +    QLIST_FOREACH(c, &bs->parents, next_parent) {
>> +        if (!bdrv_parent_change_aio_context(c, ctx, visited, tran,
>> errp)) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    QLIST_FOREACH(c, &bs->children, next) {
>> +        if (!bdrv_child_change_aio_context(c, ctx, visited, tran,
>> errp)) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    state = g_new(BdrvStateSetAioContext, 1);
>> +    *state = (BdrvStateSetAioContext) {
>> +        .new_ctx = ctx,
>> +        .bs = bs,
>> +    };
>> +
>> +    /* First half of transactions are drains */
>> +    tran_add(tran, &drained_begin_end, state);
>> +    /*
>> +     * Second half of transactions takes care of setting the
>> +     * AioContext and free the state
>> +     */
>> +    tran_add_tail(tran, &set_aio_context, state);
>> +
>> +    return true;
>> +}
> 
> 
> First, it looks like you want to use .commit() as .prepare(), .clean()
> as commit(), and .prepare() as something that just schedule other actions.
> 
> In block transaction any effect that is visible to other transaction
> actions should be done in .prepare(). (and .prepare is the main function
> of the action with *tran parameter, i.e. bdrv_change_aio_context()
> function is actually .prepare() phase).
> 
> So, ideally, the action:
> 
>  - does the complete thing in .prepare (i.e. in the main function of the
> action)
>  - rollback it in .abort
>  - nothing in .commit
> 
> Usually we still need a .commit phase for irreversible part of the
> action (like calling free() on the object). But that should be
> transparent for other actions.
> 
> So, generally we do:
> 
> tran = tran_new()
> 
> action_a(..., tran);
> action_b(..., tran);
> action_c(..., tran);
> 
> 
> And we expect, that action_b() operates on top of action_a() already
> done. And action_c() operate on top of action_b() done. So we cannot
> postpone visible (to other actions) effect of the action to .commit phase.
> 
> So, for example, if you want a kind of smart drained section in
> transaction, you may add simple
> 
> bdrv_drain_tran(..., tran);
> 
> that just call drained_begin(), and have only .clean() handler that call
> drained_end(). This way you may do
> 
> bdrv_drain_tran(...., tran);
> action_a(..., tran);
> action_b(..., tran);
> 
> And be sure that both actions and all their .abort/.commit/.clean
> handlers are called inside drained seaction. Isn't it what you need?

I understand how you see transactions, but I think we can also use them
in a different way than intended (with proper documentation).

I don't think it makes sense to use transactions as rollback in this
case, even though with the coming v2 it would be more similar to what
you propose:

.prepare takes care of drain and checking if the node is ok
.commit changes the aiocontext lock only if all nodes are drained and
passed the test
.clean undrains (drained end)

> 
> 
> Second, this all becomes extremely difficult when we mix transactions
> and recursion. When I moved permission system to transactions, I've
> introduced topological dfs search to just get a list of nodes to handle.
> I think, if we want to rework aio context change, we should first get
> rid of recursion.
> 

I honestly don't see how recursion complicates things. It's just graph
traversal, and building a list of things to do while doing that.

Isn't it much more complex to do it with a loop (ie not recursively?).
Your bdrv_topological_dfs is recursive too.

Think about it as 2 separate steps:
- Recursion takes care of checking the nodes and draining them
- Once done, if all nodes are ready to switch, switch linearly by
iterating in a list of callbacks

Anyways, I am probably not going to wait your feedback and send v2 just
because I think the way I did this double transaction list in v1
confuses people.

Feel free to continue the discussion here or in v2 directly, though.

Thank you,
Emanuele