[PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[PATCH v2 00/11] 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>
---
v2:
* remove transaction patch, and drain transactions
* re-add old AioContext lock/unlock, since it makes sense to have it
* typos in commit message
* various cleanups in block-backend callbacks
* use hash map instead of glist when marking visited nodes
* 2 more additional patches, getting rid of
  bdrv_try_set_aio_context and bdrv_child_try_change_aio_context

Emanuele Giuseppe Esposito (11):
  block.c: assert bs->aio_context is written under BQL and drains
  block: use transactions as a replacement of ->{can_}set_aio_context()
  bdrv_change_aio_context: use hash table instead of list of visited
    nodes
  bdrv_child_try_change_aio_context: add transaction parameter
  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: rename bdrv_child_try_change_aio_context in
    bdrv_try_change_aio_context
  block: remove bdrv_try_set_aio_context and replace it with
    bdrv_try_change_aio_context

 block.c                            | 360 ++++++++++++++++-------------
 block/block-backend.c              |  74 +++---
 block/export/export.c              |   2 +-
 blockdev.c                         |  22 +-
 blockjob.c                         |  50 ++--
 docs/devel/multiple-iothreads.txt  |   4 +-
 include/block/block-global-state.h |  18 +-
 include/block/block_int-common.h   |   6 +-
 job.c                              |   2 +-
 tests/unit/test-bdrv-drain.c       |   6 +-
 tests/unit/test-block-iothread.c   |  10 +-
 11 files changed, 310 insertions(+), 244 deletions(-)

-- 
2.31.1

[PATCH v2 01/11] 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.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>
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 7559965dbc..58a9cfc8b7 100644
--- a/block.c
+++ b/block.c
@@ -7282,6 +7282,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;
 }
 
@@ -7295,6 +7296,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

Re: [PATCH v2 01/11] block.c: assert bs->aio_context is written under BQL and drains

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> Also here ->aio_context is read by I/O threads and written
> under BQL.
> 
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v2 02/11] block: use transactions as a replacement of ->{can_}set_aio_context()

[Emanuele Giuseppe Esposito]

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, drain it and add a new transaction
  that implements a callback that effectively changes the aiocontext.
- Once done, the recursive function returns if *all* nodes can change
  the AioContext. If so, commit the above transactions.
  Regardless of the outcome, call transaction.clean() to undo all drains
  done in the recursion.
- The transaction list is scanned only after all nodes are being drained, so
  we are sure that they all are in the same context, and then
  we switch their AioContext, concluding the drain only after all nodes
  switched to the new 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                            | 203 ++++++++++++++++++++++++++++-
 include/block/block-global-state.h |   6 +
 include/block/block_int-common.h   |   3 +
 3 files changed, 211 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 58a9cfc8b7..c80e49009a 100644
--- a/block.c
+++ b/block.c
@@ -108,6 +108,10 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 
 static bool bdrv_backing_overridden(BlockDriverState *bs);
 
+static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+                                    GSList **visited, Transaction *tran,
+                                    Error **errp);
+
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
@@ -7325,7 +7329,7 @@ static void bdrv_attach_aio_context(BlockDriverState *bs,
  * 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
+ * @ignore will accumulate all visited BdrvChild objects. The caller is
  * responsible for freeing the list afterwards.
  */
 void bdrv_set_aio_context_ignore(BlockDriverState *bs,
@@ -7434,6 +7438,38 @@ static bool bdrv_parent_can_set_aio_context(BdrvChild *c, AioContext *ctx,
     return true;
 }
 
+typedef struct BdrvStateSetAioContext {
+    AioContext *new_ctx;
+    BlockDriverState *bs;
+} BdrvStateSetAioContext;
+
+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)
 {
@@ -7445,6 +7481,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,
@@ -7472,6 +7520,85 @@ bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
     return true;
 }
 
+static void bdrv_drained_end_clean(void *opaque)
+{
+    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
+    BlockDriverState *bs = (BlockDriverState *) state->bs;
+
+    /* Paired with bdrv_drained_begin in bdrv_change_aio_context() */
+    bdrv_drained_end(bs);
+
+    g_free(state);
+}
+
+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 = bdrv_drained_end_clean,
+};
+
+/*
+ * 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_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,
+    };
+
+    /* Paired with bdrv_drained_end in bdrv_drained_end_clean() */
+    bdrv_drained_begin(bs);
+
+    tran_add(tran, &set_aio_context, state);
+
+    return true;
+}
+
 int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
                                    BdrvChild *ignore_child, Error **errp)
 {
@@ -7495,6 +7622,80 @@ int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
     return 0;
 }
 
+/*
+ * 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.
+ *
+ * There are two phases: recursion check and linear change
+ * Recursion takes care of checking that all nodes support changing AioContext
+ * and drains them, builing a linear list of callbacks to run if it is
+ * successful (the transaction itself).
+ * Linear change consists in running all callbacks collected in the recursion
+ * to switch all nodes AioContext lock (transaction commit).
+ *
+ * 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 will fail since AIO_WAIT_WHILE
+ * assumes the lock is always held if bs is in another AioContext.
+ * For the same reason, it temporarily holds also the new AioContext, since
+ * bdrv_drained_end calls BDRV_POLL_WHILE that assumes the lock is taken too.
+ */
+int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+                                      BdrvChild *ignore_child, Error **errp)
+{
+    Transaction *tran;
+    GSList *visited;
+    int ret;
+    AioContext *old_context = bdrv_get_aio_context(bs);
+    GLOBAL_STATE_CODE();
+
+    /* Recursion phase: go through all nodes of the graph */
+    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);
+
+    /* Linear phase: go through all callbacks collected in the transaction */
+
+    if (!ret) {
+        /* Just run clean() callbacks. No AioContext changed. */
+        tran_abort(tran);
+        return -EPERM;
+    }
+
+    /*
+     * Release old AioContext, it won't be needed anymore, as all
+     * bdrv_drained_begin() have been called already.
+     */
+    if (qemu_get_aio_context() != old_context) {
+        aio_context_release(old_context);
+    }
+
+    /*
+     * Acquire new AioContext since bdrv_drained_end() is going to be called
+     * after we switched all nodes in the new AioContext, and the function
+     * assumes that the lock of the bs is always taken.
+     */
+    if (qemu_get_aio_context() != ctx) {
+        aio_context_acquire(ctx);
+    }
+
+    tran_commit(tran);
+
+    if (qemu_get_aio_context() != ctx) {
+        aio_context_release(ctx);
+    }
+
+    /* Re-acquire the old AioContext, since the caller takes and releases it. */
+    if (qemu_get_aio_context() != old_context) {
+        aio_context_acquire(old_context);
+    }
+
+    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..fdcb81a175 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -229,6 +229,12 @@ 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);
+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..43828cf74f 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 **visited, Transaction *tran, Error **errp);
+
     AioContext *(*get_parent_aio_context)(BdrvChild *child);
 
     /*
-- 
2.31.1

Re: [PATCH v2 02/11] block: use transactions as a replacement of ->{can_}set_aio_context()

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> 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, drain it and add a new transaction
>   that implements a callback that effectively changes the aiocontext.
> - Once done, the recursive function returns if *all* nodes can change
>   the AioContext. If so, commit the above transactions.
>   Regardless of the outcome, call transaction.clean() to undo all drains
>   done in the recursion.
> - The transaction list is scanned only after all nodes are being drained, so
>   we are sure that they all are in the same context, and then
>   we switch their AioContext, concluding the drain only after all nodes
>   switched to the new 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>

For future work, please change the way you construct your series. It's
not good practice to have many patches that just add dead code (and even
patches that optimise that dead code!) and then a final patch to enable
everything at once.

It's not only hard to review because you never know what to compare it
to, but also any regression will always happen on the final patch and
you can't know which patch actually contains the broken code.

Or looking at it from a slightly different angle, we should also try to
ensure that the code makes sense after each individual commit. Having
lots of duplicated code doesn't necessarily make a lot of sense.

> diff --git a/block.c b/block.c
> index 58a9cfc8b7..c80e49009a 100644
> --- a/block.c
> +++ b/block.c
> @@ -108,6 +108,10 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
>  
>  static bool bdrv_backing_overridden(BlockDriverState *bs);
>  
> +static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> +                                    GSList **visited, Transaction *tran,
> +                                    Error **errp);
> +
>  /* If non-zero, use only whitelisted block drivers */
>  static int use_bdrv_whitelist;
>  
> @@ -7325,7 +7329,7 @@ static void bdrv_attach_aio_context(BlockDriverState *bs,
>   * 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
> + * @ignore will accumulate all visited BdrvChild objects. The caller is
>   * responsible for freeing the list afterwards.
>   */
>  void bdrv_set_aio_context_ignore(BlockDriverState *bs,
> @@ -7434,6 +7438,38 @@ static bool bdrv_parent_can_set_aio_context(BdrvChild *c, AioContext *ctx,
>      return true;
>  }
>  
> +typedef struct BdrvStateSetAioContext {
> +    AioContext *new_ctx;
> +    BlockDriverState *bs;
> +} BdrvStateSetAioContext;
> +
> +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;
> +}

This is an exact copy of bdrv_parent_can_set_aio_context() except for
renames and adding a tran parameter.

Of course, nobody implements .change_aio_ctx() yet, so this doesn't
actually work yet after this patch.

>  bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
>                                      GSList **ignore, Error **errp)
>  {
> @@ -7445,6 +7481,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);
> +}

This is an exact copy of bdrv_child_can_set_aio_context() except for
renames and adding a tran parameter.

Same as above, doesn't work yet after this patch.

>  /* @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,
> @@ -7472,6 +7520,85 @@ bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>      return true;
>  }
>  
> +static void bdrv_drained_end_clean(void *opaque)

bdrv_set_aio_context_clean() is a better name for this given that the
transaction is called set_aio_context.

> +{
> +    BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
> +    BlockDriverState *bs = (BlockDriverState *) state->bs;
> +
> +    /* Paired with bdrv_drained_begin in bdrv_change_aio_context() */
> +    bdrv_drained_end(bs);
> +
> +    g_free(state);
> +}
> +
> +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);
> +}

This replaces bdrv_set_aio_context_ignore(), except for draining and
AioContext locking, which is now handled on the top level by
bdrv_child_try_change_aio_context().

Let's see what other differences I can find:

* bdrv_set_aio_context_ignore() has a old_context == new_context check
  and does nothing in this case. bdrv_change_aio_context() wouldn't even
  have put the node into the transaction in this case, so this callback
  won't be called in the first place and we don't need it here. Good.

* Recursion to children and parents: To be covered by the commit action
  of transaction action added by new and not yet implemented callback
  .change_aio_ctx().

* bdrv_detach_aio_context(bs) used to run with the AioContext lock of
  the old context, from which we are detachting. Now it runs with the
  AioContext lock of the new AioContext. Are we sure that this is safe?
  Please explain in the commit message.

> +static TransactionActionDrv set_aio_context = {
> +    .commit = bdrv_set_aio_context_commit,
> +    .clean = bdrv_drained_end_clean,
> +};
> +
> +/*
> + * 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

s/object/objects/

> + * responsible for freeing the list afterwards.
> + */
> +static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> +                                    GSList **visited, Transaction *tran,
> +                                    Error **errp)

This is roughly speaking the replacement for bdrv_can_set_aio_context().
Let's see what changes in the details.

> +{
> +    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;
> +        }
> +    }

Assuming that bdrv_child/parent_change_aio_context() still do the same
as their existing counterparts, until here it's the same as before.

> +    state = g_new(BdrvStateSetAioContext, 1);
> +    *state = (BdrvStateSetAioContext) {
> +        .new_ctx = ctx,
> +        .bs = bs,
> +    };
> +
> +    /* Paired with bdrv_drained_end in bdrv_drained_end_clean() */
> +    bdrv_drained_begin(bs);

This one is the new thing.

> +    tran_add(tran, &set_aio_context, state);
> +    return true;
> +}
> +
>  int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>                                     BdrvChild *ignore_child, Error **errp)
>  {
> @@ -7495,6 +7622,80 @@ int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>      return 0;
>  }
>  
> +/*
> + * 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.
> + *
> + * There are two phases: recursion check and linear change
> + * Recursion takes care of checking that all nodes support changing AioContext
> + * and drains them, builing a linear list of callbacks to run if it is
> + * successful (the transaction itself).
> + * Linear change consists in running all callbacks collected in the recursion
> + * to switch all nodes AioContext lock (transaction commit).

This sounds more like implementation details that are not relevant for
the caller, and could be documented in the body of the function.

> + * 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 will fail since AIO_WAIT_WHILE
> + * assumes the lock is always held if bs is in another AioContext.
> + * For the same reason, it temporarily holds also the new AioContext, since
> + * bdrv_drained_end calls BDRV_POLL_WHILE that assumes the lock is taken too.

So what is the contract regarding the new context? Like in
bdrv_child_try_set_aio_context(), the caller must *not* hold its lock?

> + */
> +int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> +                                      BdrvChild *ignore_child, Error **errp)
> +{
> +    Transaction *tran;
> +    GSList *visited;
> +    int ret;
> +    AioContext *old_context = bdrv_get_aio_context(bs);
> +    GLOBAL_STATE_CODE();
> +
> +    /* Recursion phase: go through all nodes of the graph */
> +    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);

So the immediate action of bdrv_change_aio_context() replaces
bdrv_can_set_aio_context(). The code here is identical to
bdrv_child_try_set_aio_context() then.

> +    /* Linear phase: go through all callbacks collected in the transaction */
> +
> +    if (!ret) {
> +        /* Just run clean() callbacks. No AioContext changed. */
> +        tran_abort(tran);
> +        return -EPERM;
> +    }
> +
> +    /*
> +     * Release old AioContext, it won't be needed anymore, as all
> +     * bdrv_drained_begin() have been called already.
> +     */
> +    if (qemu_get_aio_context() != old_context) {
> +        aio_context_release(old_context);
> +    }
> +
> +    /*
> +     * Acquire new AioContext since bdrv_drained_end() is going to be called
> +     * after we switched all nodes in the new AioContext, and the function
> +     * assumes that the lock of the bs is always taken.
> +     */
> +    if (qemu_get_aio_context() != ctx) {
> +        aio_context_acquire(ctx);
> +    }

This part is new compared to bdrv_child_try_set_aio_context(). So
essentially the part that was bdrv_set_aio_context_ignore() runs under
the new AioContext's lock instead of the old one.

> +    tran_commit(tran);

This is what replaces bdrv_set_aio_context_ignore(). We don't build the
ignore list a second time because we're still using the list of nodes
that we collected above.

It is also what undrains all nodes again. This is why we now need to
hold the lock of the new AioContext.

> +    if (qemu_get_aio_context() != ctx) {
> +        aio_context_release(ctx);
> +    }
> +
> +    /* Re-acquire the old AioContext, since the caller takes and releases it. */
> +    if (qemu_get_aio_context() != old_context) {
> +        aio_context_acquire(old_context);
> +    }
> +
> +    return 0;
> +}

Trying to summarise the differences in this function compared to the
existing mechanism:

* bdrv_set_aio_context_ignore() run with the old AioContext locked and
  the new one not locked, bdrv_set_aio_context_commit() runs with the
  new one locked and the old one not locked.

* All affected nodes are now drained before calling what was
  bdrv_set_aio_context_ignore() instead of individually inside the
  function. This is the improvement promised in the commit message as it
  avoids polling while the graph is in an inconsistent state.

Everything else is essentially just a copy of the existing code.

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

So this looks good under two conditions that I haven't checked yet: That
bdrv_detach_aio_context() is actually safe when called with the "wrong"
AioContext lock held, and that the .change_aio_context() callbacks are
implemented correctly in a later patch.

To reiterate my initial point, reviewing this took me some effort to
match the new functions with the existing ones they duplicate and then
manual diffing of each, which is kind of error prone. I feel the better
approach would have been adding a tran parameter (with empty commit and
abort) to the existing functions in a first patch and then change stuff
in a second patch (in the real code, not dead code to be enabled later),
so that you would actually see the real changes instead of having to
find them between lots of unchanged copied code.

Kevin

Re: [PATCH v2 02/11] block: use transactions as a replacement of ->{can_}set_aio_context()

[Emanuele Giuseppe Esposito]

> So this looks good under two conditions that I haven't checked yet: That
> bdrv_detach_aio_context() is actually safe when called with the "wrong"
> AioContext lock held, and that the .change_aio_context() callbacks are
> implemented correctly in a later patch.

So regarding bdrv_detach_aio_context(), I added a
aio_context_lock_acquire/release pair around that function in commit().
The callbacks should be implemented correctly, as you didn't point out
anything later on :)

> 
> To reiterate my initial point, reviewing this took me some effort to
> match the new functions with the existing ones they duplicate and then
> manual diffing of each, which is kind of error prone. I feel the better
> approach would have been adding a tran parameter (with empty commit and
> abort) to the existing functions in a first patch and then change stuff
> in a second patch (in the real code, not dead code to be enabled later),
> so that you would actually see the real changes instead of having to
> find them between lots of unchanged copied code.
> 

Yes, I see what you mean. I'll change my approach on the next series,
thanks for the suggestion!

Thank you,
Emanuele

[PATCH v2 03/11] bdrv_change_aio_context: use hash table instead of list of visited nodes

[Emanuele Giuseppe Esposito]

Minor performance improvement, but given that we have hash tables
available, avoid iterating in the visited nodes list every time just
to check if a node has been already visited.

The data structure is not actually a proper hash map, but an hash set,
as we are just adding nodes and not key,value pairs.

Suggested-by: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                            | 28 ++++++++++++++++------------
 include/block/block-global-state.h |  2 +-
 include/block/block_int-common.h   |  3 ++-
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index c80e49009a..c02a628336 100644
--- a/block.c
+++ b/block.c
@@ -109,7 +109,7 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 static bool bdrv_backing_overridden(BlockDriverState *bs);
 
 static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
-                                    GSList **visited, Transaction *tran,
+                                    GHashTable *visited, Transaction *tran,
                                     Error **errp);
 
 /* If non-zero, use only whitelisted block drivers */
@@ -7444,14 +7444,15 @@ typedef struct BdrvStateSetAioContext {
 } BdrvStateSetAioContext;
 
 static bool bdrv_parent_change_aio_context(BdrvChild *c, AioContext *ctx,
-                                           GSList **visited, Transaction *tran,
+                                           GHashTable *visited,
+                                           Transaction *tran,
                                            Error **errp)
 {
     GLOBAL_STATE_CODE();
-    if (g_slist_find(*visited, c)) {
+    if (g_hash_table_contains(visited, c)) {
         return true;
     }
-    *visited = g_slist_prepend(*visited, c);
+    g_hash_table_add(visited, c);
 
     /*
      * A BdrvChildClass that doesn't handle AioContext changes cannot
@@ -7482,14 +7483,14 @@ bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
 }
 
 bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
-                                   GSList **visited, Transaction *tran,
+                                   GHashTable *visited, Transaction *tran,
                                    Error **errp)
 {
     GLOBAL_STATE_CODE();
-    if (g_slist_find(*visited, c)) {
+    if (g_hash_table_contains(visited, c)) {
         return true;
     }
-    *visited = g_slist_prepend(*visited, c);
+    g_hash_table_add(visited, c);
     return bdrv_change_aio_context(c->bs, ctx, visited, tran, errp);
 }
 
@@ -7561,7 +7562,7 @@ static TransactionActionDrv set_aio_context = {
  * responsible for freeing the list afterwards.
  */
 static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
-                                    GSList **visited, Transaction *tran,
+                                    GHashTable *visited, Transaction *tran,
                                     Error **errp)
 {
     BdrvChild *c;
@@ -7646,16 +7647,19 @@ int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
                                       BdrvChild *ignore_child, Error **errp)
 {
     Transaction *tran;
-    GSList *visited;
+    GHashTable *visited;
     int ret;
     AioContext *old_context = bdrv_get_aio_context(bs);
     GLOBAL_STATE_CODE();
 
     /* Recursion phase: go through all nodes of the graph */
     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);
+    visited = g_hash_table_new(NULL, NULL);
+    if (ignore_child) {
+        g_hash_table_add(visited, ignore_child);
+    }
+    ret = bdrv_change_aio_context(bs, ctx, visited, tran, errp);
+    g_hash_table_destroy(visited);
 
     /* Linear phase: go through all callbacks collected in the transaction */
 
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index fdcb81a175..ceecf0aa8e 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -230,7 +230,7 @@ bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
 AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c);
 
 bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
-                                   GSList **visited, Transaction *tran,
+                                   GHashTable *visited, Transaction *tran,
                                    Error **errp);
 int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
                                       BdrvChild *ignore_child, Error **errp);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 43828cf74f..c639873487 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -897,7 +897,8 @@ struct BdrvChildClass {
     void (*set_aio_ctx)(BdrvChild *child, AioContext *ctx, GSList **ignore);
 
     bool (*change_aio_ctx)(BdrvChild *child, AioContext *ctx,
-                           GSList **visited, Transaction *tran, Error **errp);
+                           GHashTable *visited, Transaction *tran,
+                           Error **errp);
 
     AioContext *(*get_parent_aio_context)(BdrvChild *child);
 
-- 
2.31.1

Re: [PATCH v2 03/11] bdrv_change_aio_context: use hash table instead of list of visited nodes

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> Minor performance improvement, but given that we have hash tables
> available, avoid iterating in the visited nodes list every time just
> to check if a node has been already visited.
> 
> The data structure is not actually a proper hash map, but an hash set,
> as we are just adding nodes and not key,value pairs.
> 
> Suggested-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v2 04/11] bdrv_child_try_change_aio_context: add transaction parameter

[Emanuele Giuseppe Esposito]

This enables the caller to use the same transaction to also
keep track of aiocontext changes.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                            | 31 ++++++++++++++++++++++++------
 include/block/block-global-state.h |  5 +++++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index c02a628336..221bf90268 100644
--- a/block.c
+++ b/block.c
@@ -7643,17 +7643,16 @@ int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
  * For the same reason, it temporarily holds also the new AioContext, since
  * bdrv_drained_end calls BDRV_POLL_WHILE that assumes the lock is taken too.
  */
-int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
-                                      BdrvChild *ignore_child, Error **errp)
+int bdrv_child_try_change_aio_context_tran(BlockDriverState *bs,
+                                           AioContext *ctx,
+                                           BdrvChild *ignore_child,
+                                           Transaction *tran,
+                                           Error **errp)
 {
-    Transaction *tran;
     GHashTable *visited;
     int ret;
-    AioContext *old_context = bdrv_get_aio_context(bs);
     GLOBAL_STATE_CODE();
 
-    /* Recursion phase: go through all nodes of the graph */
-    tran = tran_new();
     visited = g_hash_table_new(NULL, NULL);
     if (ignore_child) {
         g_hash_table_add(visited, ignore_child);
@@ -7661,6 +7660,26 @@ int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
     ret = bdrv_change_aio_context(bs, ctx, visited, tran, errp);
     g_hash_table_destroy(visited);
 
+    return ret;
+}
+
+/*
+ * See bdrv_child_try_change_aio_context_tran for invariants on
+ * AioContext locks.
+ */
+int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+                                      BdrvChild *ignore_child, Error **errp)
+{
+    Transaction *tran;
+    int ret;
+    AioContext *old_context = bdrv_get_aio_context(bs);
+    GLOBAL_STATE_CODE();
+
+    /* Recursion phase: go through all nodes of the graph */
+    tran = tran_new();
+    ret = bdrv_child_try_change_aio_context_tran(bs, ctx, ignore_child, tran,
+                                                 errp);
+
     /* Linear phase: go through all callbacks collected in the transaction */
 
     if (!ret) {
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index ceecf0aa8e..1bd445b507 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -234,6 +234,11 @@ bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
                                    Error **errp);
 int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
                                       BdrvChild *ignore_child, Error **errp);
+int bdrv_child_try_change_aio_context_tran(BlockDriverState *bs,
+                                           AioContext *ctx,
+                                           BdrvChild *ignore_child,
+                                           Transaction *tran,
+                                           Error **errp);
 
 int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
 int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo);
-- 
2.31.1

Re: [PATCH v2 04/11] bdrv_child_try_change_aio_context: add transaction parameter

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> This enables the caller to use the same transaction to also
> keep track of aiocontext changes.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

What you're really doing here is factoring out the recursive phase.
However, the factored out function is never used from anywhere else,
so I don't understand the purpose of this patch. It feels like an
unnecessary complication of the code.

The commit message is unclear to me, too: Who is the caller of
bdrv_child_try_change_aio_context() that it mentions, and why does it
make a difference to it how the code is organised internally?

Is this some artifact of changes you made and we don't need it any more
now?

>  block.c                            | 31 ++++++++++++++++++++++++------
>  include/block/block-global-state.h |  5 +++++
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c02a628336..221bf90268 100644
> --- a/block.c
> +++ b/block.c
> @@ -7643,17 +7643,16 @@ int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>   * For the same reason, it temporarily holds also the new AioContext, since
>   * bdrv_drained_end calls BDRV_POLL_WHILE that assumes the lock is taken too.
>   */
> -int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> -                                      BdrvChild *ignore_child, Error **errp)
> +int bdrv_child_try_change_aio_context_tran(BlockDriverState *bs,
> +                                           AioContext *ctx,
> +                                           BdrvChild *ignore_child,
> +                                           Transaction *tran,
> +                                           Error **errp)

As mentioned above, this is never used anywhere else than from
bdrv_child_try_change_aio_context(), so if we want to keep the patch, it
should be static at least.

Maybe find a better name, too, because all of the transaction related
operations are in the caller.

The function comment is not accurate any more either because it
described the whole of bdrv_child_try_change_aio_context(), while this
function only contains the recursive part.

Kevin

Re: [PATCH v2 04/11] bdrv_child_try_change_aio_context: add transaction parameter

[Emanuele Giuseppe Esposito]

Am 07/10/2022 um 18:10 schrieb Kevin Wolf:
> Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
>> This enables the caller to use the same transaction to also
>> keep track of aiocontext changes.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> What you're really doing here is factoring out the recursive phase.
> However, the factored out function is never used from anywhere else,
> so I don't understand the purpose of this patch. It feels like an
> unnecessary complication of the code.
> 
> The commit message is unclear to me, too: Who is the caller of
> bdrv_child_try_change_aio_context() that it mentions, and why does it
> make a difference to it how the code is organised internally?

So this was an initial (mis)understanding from what Vladimir suggested,
where everything should be part of a single transaction.
If you want, I can drop this.

Thank you,
Emanuele

> 
> Is this some artifact of changes you made and we don't need it any more
> now?
> 
>>  block.c                            | 31 ++++++++++++++++++++++++------
>>  include/block/block-global-state.h |  5 +++++
>>  2 files changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index c02a628336..221bf90268 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -7643,17 +7643,16 @@ int bdrv_child_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
>>   * For the same reason, it temporarily holds also the new AioContext, since
>>   * bdrv_drained_end calls BDRV_POLL_WHILE that assumes the lock is taken too.
>>   */
>> -int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
>> -                                      BdrvChild *ignore_child, Error **errp)
>> +int bdrv_child_try_change_aio_context_tran(BlockDriverState *bs,
>> +                                           AioContext *ctx,
>> +                                           BdrvChild *ignore_child,
>> +                                           Transaction *tran,
>> +                                           Error **errp)
> 
> As mentioned above, this is never used anywhere else than from
> bdrv_child_try_change_aio_context(), so if we want to keep the patch, it
> should be static at least.
> 
> Maybe find a better name, too, because all of the transaction related
> operations are in the caller.
> 
> The function comment is not accurate any more either because it
> described the whole of bdrv_child_try_change_aio_context(), while this
> function only contains the recursive part.
> 
> Kevin
> 

[PATCH v2 05/11] 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.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>
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 375c90e4b8..704bab060f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -126,6 +126,50 @@ static void child_job_drained_end(BdrvChild *c, int *drained_end_counter)
     job_resume(&job->job);
 }
 
+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,
+                                     GHashTable *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(tran, &change_child_job_context, s);
+    return true;
+}
+
 static bool child_job_can_set_aio_ctx(BdrvChild *c, AioContext *ctx,
                                       GSList **ignore, Error **errp)
 {
@@ -174,6 +218,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

Re: [PATCH v2 05/11] blockjob: implement .change_aio_ctx in child_job

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> 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.
> 
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v2 06/11] 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.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>
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 221bf90268..bcc9b0d099 100644
--- a/block.c
+++ b/block.c
@@ -1239,6 +1239,14 @@ static int bdrv_child_cb_inactivate(BdrvChild *child)
     return 0;
 }
 
+static bool bdrv_child_cb_change_aio_ctx(BdrvChild *child, AioContext *ctx,
+                                         GHashTable *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)
 {
@@ -1495,6 +1503,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

Re: [PATCH v2 06/11] block: implement .change_aio_ctx in child_of_bds

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> 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.
> 
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v2 07/11] 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 | 52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index f425b00793..b4951c6e21 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,
+                                    GHashTable *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,54 @@ 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,
+                                    GHashTable *visited, Transaction *tran,
+                                    Error **errp)
+{
+    BlockBackend *blk = child->opaque;
+    BdrvStateBlkRootContext *s;
+
+    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) {
+            /* TODO Add BB name/QOM path */
+            error_setg(errp, "Cannot change iothread of active block backend");
+            return false;
+        }
+    }
+
+    s = g_new(BdrvStateBlkRootContext, 1);
+    *s = (BdrvStateBlkRootContext) {
+        .new_ctx = ctx,
+        .blk = blk,
+    };
+
+    tran_add(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

Re: [PATCH v2 07/11] block-backend: implement .change_aio_ctx in child_root

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v2 08/11] 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               | 44 ++++++++++++++++++++++++-------------------
 block/block-backend.c |  8 ++++++--
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index bcc9b0d099..9b47aacad2 100644
--- a/block.c
+++ b/block.c
@@ -2970,17 +2970,21 @@ static void bdrv_attach_child_common_abort(void *opaque)
     }
 
     if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
-        GSList *ignore;
+        Transaction *tran;
+        GHashTable *visited;
+        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();
 
-        ignore = NULL;
-        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
-        g_slist_free(ignore);
+        /* No need to visit `child`, because it has been detached already */
+        visited = g_hash_table_new(NULL, NULL);
+        ret = child->klass->change_aio_ctx(child, s->old_parent_ctx, visited,
+                                           tran, &error_abort);
+        g_hash_table_destroy(visited);
+
+        /* transaction is supposed to always succeed */
+        assert(ret == true);
+        tran_commit(tran);
     }
 
     bdrv_unref(bs);
@@ -3041,18 +3045,20 @@ 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) {
-            GSList *ignore = g_slist_prepend(NULL, new_child);
-            if (child_class->can_set_aio_ctx(new_child, child_ctx, &ignore,
-                                             NULL))
-            {
+        if (ret < 0 && child_class->change_aio_ctx) {
+            Transaction *tran = tran_new();
+            GHashTable *visited = g_hash_table_new(NULL, NULL);
+            bool ret_child;
+
+            g_hash_table_add(visited, new_child);
+            ret_child = child_class->change_aio_ctx(new_child, child_ctx,
+                                                    visited, tran, NULL);
+            if (ret_child == true) {
                 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);
             }
-            g_slist_free(ignore);
+            tran_finalize(tran, ret_child == true ? 0 : -1);
+            g_hash_table_destroy(visited);
         }
 
         if (ret < 0) {
@@ -7732,7 +7738,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 b4951c6e21..3046b4cc54 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

Re: [PATCH v2 08/11] block: use the new _change_ API instead of _can_set_ and _set_

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v2 09/11] 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 9b47aacad2..c066b41c8c 100644
--- a/block.c
+++ b/block.c
@@ -1247,20 +1247,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
@@ -1501,8 +1487,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,
@@ -7334,125 +7318,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 objects. 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;
@@ -7486,17 +7351,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,
                                    GHashTable *visited, Transaction *tran,
                                    Error **errp)
@@ -7509,33 +7363,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_end_clean(void *opaque)
 {
     BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
@@ -7615,29 +7442,6 @@ static 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;
-}
-
 /*
  * 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.
diff --git a/block/block-backend.c b/block/block-backend.c
index 3046b4cc54..a27b8b7a89 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,
                                     GHashTable *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,
@@ -2264,33 +2258,6 @@ static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx,
     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 704bab060f..dadb2feca4 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -170,39 +170,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;
@@ -216,8 +183,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 1bd445b507..54fd008442 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,
                                    GHashTable *visited, Transaction *tran,
                                    Error **errp);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index c639873487..8f3102cc3d 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,
                            GHashTable *visited, Transaction *tran,
                            Error **errp);
-- 
2.31.1

Re: [PATCH v2 09/11] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> Together with all _can_set_ and _set_ APIs, as they are not needed
> anymore.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v2 10/11] block: rename bdrv_child_try_change_aio_context in bdrv_try_change_aio_context

[Emanuele Giuseppe Esposito]

No functional changes intended.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                            | 19 ++++++++-----------
 block/block-backend.c              |  3 +--
 include/block/block-global-state.h | 12 +++++-------
 3 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/block.c b/block.c
index c066b41c8c..b82eb0ba6d 100644
--- a/block.c
+++ b/block.c
@@ -7462,11 +7462,9 @@ static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
  * For the same reason, it temporarily holds also the new AioContext, since
  * bdrv_drained_end calls BDRV_POLL_WHILE that assumes the lock is taken too.
  */
-int bdrv_child_try_change_aio_context_tran(BlockDriverState *bs,
-                                           AioContext *ctx,
-                                           BdrvChild *ignore_child,
-                                           Transaction *tran,
-                                           Error **errp)
+int bdrv_try_change_aio_context_tran(BlockDriverState *bs, AioContext *ctx,
+                                     BdrvChild *ignore_child, Transaction *tran,
+                                     Error **errp)
 {
     GHashTable *visited;
     int ret;
@@ -7483,11 +7481,11 @@ int bdrv_child_try_change_aio_context_tran(BlockDriverState *bs,
 }
 
 /*
- * See bdrv_child_try_change_aio_context_tran for invariants on
+ * See bdrv_try_change_aio_context_tran for invariants on
  * AioContext locks.
  */
-int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
-                                      BdrvChild *ignore_child, Error **errp)
+int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+                                BdrvChild *ignore_child, Error **errp)
 {
     Transaction *tran;
     int ret;
@@ -7496,8 +7494,7 @@ int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
 
     /* Recursion phase: go through all nodes of the graph */
     tran = tran_new();
-    ret = bdrv_child_try_change_aio_context_tran(bs, ctx, ignore_child, tran,
-                                                 errp);
+    ret = bdrv_try_change_aio_context_tran(bs, ctx, ignore_child, tran, errp);
 
     /* Linear phase: go through all callbacks collected in the transaction */
 
@@ -7542,7 +7539,7 @@ int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
                              Error **errp)
 {
     GLOBAL_STATE_CODE();
-    return bdrv_child_try_change_aio_context(bs, ctx, NULL, errp);
+    return bdrv_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 a27b8b7a89..f785c1e7e2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2182,8 +2182,7 @@ static int blk_do_set_aio_context(BlockBackend *blk, AioContext *new_context,
              * 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);
+            ret = bdrv_try_change_aio_context(bs, new_context, blk->root, errp);
             if (ret < 0) {
                 bdrv_unref(bs);
                 return ret;
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 54fd008442..11f80768c3 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -223,13 +223,11 @@ AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c);
 bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
                                    GHashTable *visited, Transaction *tran,
                                    Error **errp);
-int bdrv_child_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
-                                      BdrvChild *ignore_child, Error **errp);
-int bdrv_child_try_change_aio_context_tran(BlockDriverState *bs,
-                                           AioContext *ctx,
-                                           BdrvChild *ignore_child,
-                                           Transaction *tran,
-                                           Error **errp);
+int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+                                BdrvChild *ignore_child, Error **errp);
+int bdrv_try_change_aio_context_tran(BlockDriverState *bs, AioContext *ctx,
+                                     BdrvChild *ignore_child, Transaction *tran,
+                                     Error **errp);
 
 int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
 int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo);
-- 
2.31.1

Re: [PATCH v2 10/11] block: rename bdrv_child_try_change_aio_context in bdrv_try_change_aio_context

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> No functional changes intended.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v2 11/11] block: remove bdrv_try_set_aio_context and replace it with bdrv_try_change_aio_context

[Emanuele Giuseppe Esposito]

No functional change intended.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                            | 14 ++++----------
 block/export/export.c              |  2 +-
 blockdev.c                         | 22 +++++++++++-----------
 docs/devel/multiple-iothreads.txt  |  4 ++--
 include/block/block-global-state.h |  2 --
 job.c                              |  2 +-
 tests/unit/test-bdrv-drain.c       |  6 +++---
 tests/unit/test-block-iothread.c   | 10 +++++-----
 8 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/block.c b/block.c
index b82eb0ba6d..435e8fe731 100644
--- a/block.c
+++ b/block.c
@@ -2950,7 +2950,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
     bdrv_replace_child_noperm(s->child, NULL, false);
 
     if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
-        bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
+        bdrv_try_change_aio_context(bs, s->old_child_ctx, NULL, &error_abort);
     }
 
     if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
@@ -3027,7 +3027,8 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
     parent_ctx = bdrv_child_get_parent_aio_context(new_child);
     if (child_ctx != parent_ctx) {
         Error *local_err = NULL;
-        int ret = bdrv_try_set_aio_context(child_bs, parent_ctx, &local_err);
+        int ret = bdrv_try_change_aio_context(child_bs, parent_ctx, NULL,
+                                              &local_err);
 
         if (ret < 0 && child_class->change_aio_ctx) {
             Transaction *tran = tran_new();
@@ -3130,7 +3131,7 @@ static void bdrv_detach_child(BdrvChild **childp)
          * When the parent requiring a non-default AioContext is removed, the
          * node moves back to the main AioContext
          */
-        bdrv_try_set_aio_context(old_bs, qemu_get_aio_context(), NULL);
+        bdrv_try_change_aio_context(old_bs, qemu_get_aio_context(), NULL, NULL);
     }
 }
 
@@ -7535,13 +7536,6 @@ int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
     return 0;
 }
 
-int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
-                             Error **errp)
-{
-    GLOBAL_STATE_CODE();
-    return bdrv_try_change_aio_context(bs, ctx, NULL, errp);
-}
-
 void bdrv_add_aio_context_notifier(BlockDriverState *bs,
         void (*attached_aio_context)(AioContext *new_context, void *opaque),
         void (*detach_aio_context)(void *opaque), void *opaque)
diff --git a/block/export/export.c b/block/export/export.c
index 4744862915..7cc0c25c1c 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -129,7 +129,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
 
         /* Ignore errors with fixed-iothread=false */
         set_context_errp = fixed_iothread ? errp : NULL;
-        ret = bdrv_try_set_aio_context(bs, new_ctx, set_context_errp);
+        ret = bdrv_try_change_aio_context(bs, new_ctx, NULL, set_context_errp);
         if (ret == 0) {
             aio_context_release(ctx);
             aio_context_acquire(new_ctx);
diff --git a/blockdev.c b/blockdev.c
index 2cd84d206c..fb0ec67523 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1619,8 +1619,8 @@ static void external_snapshot_abort(BlkActionState *common)
                 aio_context_release(aio_context);
                 aio_context_acquire(tmp_context);
 
-                ret = bdrv_try_set_aio_context(state->old_bs,
-                                               aio_context, NULL);
+                ret = bdrv_try_change_aio_context(state->old_bs,
+                                                  aio_context, NULL, NULL);
                 assert(ret == 0);
 
                 aio_context_release(tmp_context);
@@ -1781,12 +1781,12 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
         goto out;
     }
 
-    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+    /* Honor bdrv_try_change_aio_context() context acquisition requirements. */
     old_context = bdrv_get_aio_context(target_bs);
     aio_context_release(aio_context);
     aio_context_acquire(old_context);
 
-    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+    ret = bdrv_try_change_aio_context(target_bs, aio_context, NULL, errp);
     if (ret < 0) {
         bdrv_unref(target_bs);
         aio_context_release(old_context);
@@ -1881,12 +1881,12 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
         return;
     }
 
-    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+    /* Honor bdrv_try_change_aio_context() context acquisition requirements. */
     aio_context = bdrv_get_aio_context(bs);
     old_context = bdrv_get_aio_context(target_bs);
     aio_context_acquire(old_context);
 
-    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+    ret = bdrv_try_change_aio_context(target_bs, aio_context, NULL, errp);
     if (ret < 0) {
         aio_context_release(old_context);
         return;
@@ -3183,12 +3183,12 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
                     !bdrv_has_zero_init(target_bs)));
 
 
-    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+    /* Honor bdrv_try_change_aio_context() context acquisition requirements. */
     old_context = bdrv_get_aio_context(target_bs);
     aio_context_release(aio_context);
     aio_context_acquire(old_context);
 
-    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+    ret = bdrv_try_change_aio_context(target_bs, aio_context, NULL, errp);
     if (ret < 0) {
         bdrv_unref(target_bs);
         aio_context_release(old_context);
@@ -3255,12 +3255,12 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
 
     zero_target = (sync == MIRROR_SYNC_MODE_FULL);
 
-    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+    /* Honor bdrv_try_change_aio_context() context acquisition requirements. */
     old_context = bdrv_get_aio_context(target_bs);
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(old_context);
 
-    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+    ret = bdrv_try_change_aio_context(target_bs, aio_context, NULL, errp);
 
     aio_context_release(old_context);
     aio_context_acquire(aio_context);
@@ -3756,7 +3756,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
     old_context = bdrv_get_aio_context(bs);
     aio_context_acquire(old_context);
 
-    bdrv_try_set_aio_context(bs, new_context, errp);
+    bdrv_try_change_aio_context(bs, new_context, NULL, errp);
 
     aio_context_release(old_context);
 }
diff --git a/docs/devel/multiple-iothreads.txt b/docs/devel/multiple-iothreads.txt
index aeb997bed5..343120f2ef 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -109,7 +109,7 @@ The AioContext originates from the QEMU block layer, even though nowadays
 AioContext is a generic event loop that can be used by any QEMU subsystem.
 
 The block layer has support for AioContext integrated.  Each BlockDriverState
-is associated with an AioContext using bdrv_try_set_aio_context() and
+is associated with an AioContext using bdrv_try_change_aio_context() and
 bdrv_get_aio_context().  This allows block layer code to process I/O inside the
 right AioContext.  Other subsystems may wish to follow a similar approach.
 
@@ -134,5 +134,5 @@ Long-running jobs (usually in the form of coroutines) are best scheduled in
 the BlockDriverState's AioContext to avoid the need to acquire/release around
 each bdrv_*() call.  The functions bdrv_add/remove_aio_context_notifier,
 or alternatively blk_add/remove_aio_context_notifier if you use BlockBackends,
-can be used to get a notification whenever bdrv_try_set_aio_context() moves a
+can be used to get a notification whenever bdrv_try_change_aio_context() moves a
 BlockDriverState to a different AioContext.
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 11f80768c3..3c5a8fb61b 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -217,8 +217,6 @@ void coroutine_fn bdrv_co_lock(BlockDriverState *bs);
  */
 void coroutine_fn bdrv_co_unlock(BlockDriverState *bs);
 
-int bdrv_try_set_aio_context(BlockDriverState *bs, AioContext *ctx,
-                             Error **errp);
 AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c);
 bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
                                    GHashTable *visited, Transaction *tran,
diff --git a/job.c b/job.c
index 747871c2e5..637bc6592c 100644
--- a/job.c
+++ b/job.c
@@ -588,7 +588,7 @@ static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns)
     next_aio_context = job->aio_context;
     /*
      * Coroutine has resumed, but in the meanwhile the job AioContext
-     * might have changed via bdrv_try_set_aio_context(), so we need to move
+     * might have changed via bdrv_try_change_aio_context(), so we need to move
      * the coroutine too in the new aiocontext.
      */
     while (qemu_get_current_aio_context() != next_aio_context) {
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 4924ceb562..e465e4349d 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1538,16 +1538,16 @@ static void test_set_aio_context(void)
                               &error_abort);
 
     bdrv_drained_begin(bs);
-    bdrv_try_set_aio_context(bs, ctx_a, &error_abort);
+    bdrv_try_change_aio_context(bs, ctx_a, NULL, &error_abort);
 
     aio_context_acquire(ctx_a);
     bdrv_drained_end(bs);
 
     bdrv_drained_begin(bs);
-    bdrv_try_set_aio_context(bs, ctx_b, &error_abort);
+    bdrv_try_change_aio_context(bs, ctx_b, NULL, &error_abort);
     aio_context_release(ctx_a);
     aio_context_acquire(ctx_b);
-    bdrv_try_set_aio_context(bs, qemu_get_aio_context(), &error_abort);
+    bdrv_try_change_aio_context(bs, qemu_get_aio_context(), NULL, &error_abort);
     aio_context_release(ctx_b);
     bdrv_drained_end(bs);
 
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 9d7c8be00f..0626ab83a6 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -638,7 +638,7 @@ static void test_propagate_mirror(void)
     filter = bdrv_find_node("filter_node");
 
     /* Change the AioContext of src */
-    bdrv_try_set_aio_context(src, ctx, &error_abort);
+    bdrv_try_change_aio_context(src, ctx, NULL, &error_abort);
     g_assert(bdrv_get_aio_context(src) == ctx);
     g_assert(bdrv_get_aio_context(target) == ctx);
     g_assert(bdrv_get_aio_context(filter) == ctx);
@@ -646,7 +646,7 @@ static void test_propagate_mirror(void)
 
     /* Change the AioContext of target */
     aio_context_acquire(ctx);
-    bdrv_try_set_aio_context(target, main_ctx, &error_abort);
+    bdrv_try_change_aio_context(target, main_ctx, NULL, &error_abort);
     aio_context_release(ctx);
     g_assert(bdrv_get_aio_context(src) == main_ctx);
     g_assert(bdrv_get_aio_context(target) == main_ctx);
@@ -656,7 +656,7 @@ static void test_propagate_mirror(void)
     blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
     blk_insert_bs(blk, src, &error_abort);
 
-    bdrv_try_set_aio_context(target, ctx, &local_err);
+    bdrv_try_change_aio_context(target, ctx, NULL, &local_err);
     error_free_or_abort(&local_err);
 
     g_assert(blk_get_aio_context(blk) == main_ctx);
@@ -667,7 +667,7 @@ static void test_propagate_mirror(void)
     /* ...unless we explicitly allow it */
     aio_context_acquire(ctx);
     blk_set_allow_aio_context_change(blk, true);
-    bdrv_try_set_aio_context(target, ctx, &error_abort);
+    bdrv_try_change_aio_context(target, ctx, NULL, &error_abort);
     aio_context_release(ctx);
 
     g_assert(blk_get_aio_context(blk) == ctx);
@@ -679,7 +679,7 @@ static void test_propagate_mirror(void)
 
     aio_context_acquire(ctx);
     blk_set_aio_context(blk, main_ctx, &error_abort);
-    bdrv_try_set_aio_context(target, main_ctx, &error_abort);
+    bdrv_try_change_aio_context(target, main_ctx, NULL, &error_abort);
     aio_context_release(ctx);
 
     blk_unref(blk);
-- 
2.31.1

Re: [PATCH v2 11/11] block: remove bdrv_try_set_aio_context and replace it with bdrv_try_change_aio_context

[Kevin Wolf]

Am 25.07.2022 um 14:21 hat Emanuele Giuseppe Esposito geschrieben:
> No functional change intended.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Vladimir Sementsov-Ogievskiy]

On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
> 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

patch 2 now.

> I have with AioContext locks).
> 

Can't apply to master (neither patchew can: https://patchew.org/QEMU/20220725122120.309236-1-eesposit@redhat.com/ )
Do you have a published branch somewhere to look at?

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Emanuele Giuseppe Esposito]

Am 26/07/2022 um 16:24 schrieb Vladimir Sementsov-Ogievskiy:
> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>> 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
> 
> patch 2 now.
> 
>> I have with AioContext locks).
>>
> 
> Can't apply to master (neither patchew can:
> https://patchew.org/QEMU/20220725122120.309236-1-eesposit@redhat.com/ )
> Do you have a published branch somewhere to look at?
> 

You need to apply the latest job series first.
Anyways, the branch is here:
https://gitlab.com/eesposit/qemu/-/tree/dp_tryset_final

Emanuele

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Vladimir Sementsov-Ogievskiy]

On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
> 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>


What I dislike here is that you refactor aio-context-change to use transactions, but you use it "internally" for aio-context-change. aio-context-change doesn't become a native part of block-graph modifiction transaction system after the series.

For example, bdrv_attach_child_common(..., tran) still calls bdrv_try_change_aio_context() function which doesn't take @tran argument. And we have to call bdrv_try_change_aio_context() again in bdrv_attach_child_common_abort() with opposite arguments by hand. We create in _try_ separate Transaction object which is unrelated to the original block-graph-change transaction.

With good refactoring we should get rid of these _try_ functions, and have just bdrv_change_aio_context(..., tran) that can be natively used with external tran object, where other block-graph change actions participate. This way we'll not have to call reverse aio_context_change() in .abort, it will be done automatically.

Moreover, your  *aio_context_change* functions that do have tran parameter cannot be simply used in the block-graph change transaction, as you don't follow the common paradigm, that in .prepare we do all visible changes. That's why you have to still use _try_*() version that creates seaparate transaction object and completes it: after that the action is actually done and other graph-modifying actions can be done on top.

So, IMHO, we shouldn't go this way, as that adds transaction actions that actually can't be used in common block-graph-modifying transaction but only context of bdrv_try_change_aio_context() internal transaction.

I agree that aio-context change should finally be rewritten to take a native place in block-graph transactions, but that should be a complete solution, introducing native bdrv_change_aio_context(..., tran) transaction action that is directly used in the block-graph transaction, do visible effect in .prepare and don't create extra Transaction objects.

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Emanuele Giuseppe Esposito]

Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>> 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>
> 
> 

So, I read your email before going on PTO and at that point I got what
your concerns were, but now after re-reading it I don't understand
anymore what you mean :)

> What I dislike here is that you refactor aio-context-change to use
> transactions, but you use it "internally" for aio-context-change.
> aio-context-change doesn't become a native part of block-graph
> modifiction transaction system after the series.
> 
> For example, bdrv_attach_child_common(..., tran) still calls
> bdrv_try_change_aio_context() function which doesn't take @tran
> argument. And we have to call bdrv_try_change_aio_context() again in
> bdrv_attach_child_common_abort() with opposite arguments by hand. We
> create in _try_ separate Transaction object which is unrelated to the
> original block-graph-change transaction.
> 

This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
transaction parameter" supports taking transaction as a parameter.
bdrv_attach_child_common could simply call
bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
it could work).

I think the main concern here is that during the prepare phase this
serie doesn't change any aiocontext, so until we don't commit the rest
of the code cannot assume that the aiocontext has been changed.

But isn't it what happens also for permissions? Permission functions
like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then
bdrv_set_perm() in commit.

Another important question is that if we actually want to put everything
in a single transaction, because .prepare functions like the one
proposed here perform drains, so the logic following prepare and
preceding commit must take into account that everything is drained. Also
prepare itself has to take into account that maybe other .prepare took
locks or drained themselves...

> With good refactoring we should get rid of these _try_ functions, and
> have just bdrv_change_aio_context(..., tran) that can be natively used
> with external tran object, where other block-graph change actions
> participate. This way we'll not have to call reverse
> aio_context_change() in .abort, it will be done automatically.
> 
> Moreover, your  *aio_context_change* functions that do have tran
> parameter cannot be simply used in the block-graph change transaction,
> as you don't follow the common paradigm, that in .prepare we do all
> visible changes. That's why you have to still use _try_*() version that
> creates seaparate transaction object and completes it: after that the
> action is actually done and other graph-modifying actions can be done on
> top.
> 
> So, IMHO, we shouldn't go this way, as that adds transaction actions
> that actually can't be used in common block-graph-modifying transaction
> but only context of bdrv_try_change_aio_context() internal transaction.
> 
> I agree that aio-context change should finally be rewritten to take a
> native place in block-graph transactions, but that should be a complete
> solution, introducing native bdrv_change_aio_context(..., tran)
> transaction action that is directly used in the block-graph transaction,
> do visible effect in .prepare and don't create extra Transaction objects.
> 

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Emanuele Giuseppe Esposito]

Am 05/08/2022 um 15:22 schrieb Emanuele Giuseppe Esposito:
> 
> 
> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>>> 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>
>>
>>
> 
> So, I read your email before going on PTO and at that point I got what
> your concerns were, but now after re-reading it I don't understand
> anymore what you mean :)
> 
>> What I dislike here is that you refactor aio-context-change to use
>> transactions, but you use it "internally" for aio-context-change.
>> aio-context-change doesn't become a native part of block-graph
>> modifiction transaction system after the series.
>>
>> For example, bdrv_attach_child_common(..., tran) still calls
>> bdrv_try_change_aio_context() function which doesn't take @tran
>> argument. And we have to call bdrv_try_change_aio_context() again in
>> bdrv_attach_child_common_abort() with opposite arguments by hand. We
>> create in _try_ separate Transaction object which is unrelated to the
>> original block-graph-change transaction.
>>
> 
> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
> transaction parameter" supports taking transaction as a parameter.
> bdrv_attach_child_common could simply call
> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
> it could work).

No actually I don't get how it can work in bdrv_attach_child_common.
We have the following:

parent_ctx = bdrv_child_get_parent_aio_context(new_child);
if (child_ctx != parent_ctx) {
      int ret = bdrv_try_change_aio_context(child_bs, parent_ctx, NULL,
                                            &local_err);

      if (ret < 0 && child_class->change_aio_ctx) {
          ret_child = child_class->change_aio_ctx(new_child, child_ctx,
                                                  visited, tran, NULL);

          tran_finalize(tran, ret_child == true ? 0 : -1);
      }

      if (ret < 0) {
          return ret;
      }
}

bdrv_replace_child_noperm(&new_child, child_bs, true);

So bdrv_try_change_aio_context would be changed in
bdrv_try_change_aio_context_tran, but then how can we call
bdrv_replace_child_noperm if no aiocontext has been changed at all?
I don't think we can mix transactional operations with non-transactional.

Emanuele

> 
> I think the main concern here is that during the prepare phase this
> serie doesn't change any aiocontext, so until we don't commit the rest
> of the code cannot assume that the aiocontext has been changed.
> 
> But isn't it what happens also for permissions? Permission functions
> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then
> bdrv_set_perm() in commit.
> 
> Another important question is that if we actually want to put everything
> in a single transaction, because .prepare functions like the one
> proposed here perform drains, so the logic following prepare and
> preceding commit must take into account that everything is drained. Also
> prepare itself has to take into account that maybe other .prepare took
> locks or drained themselves...
> 
>> With good refactoring we should get rid of these _try_ functions, and
>> have just bdrv_change_aio_context(..., tran) that can be natively used
>> with external tran object, where other block-graph change actions
>> participate. This way we'll not have to call reverse
>> aio_context_change() in .abort, it will be done automatically.
>>
>> Moreover, your  *aio_context_change* functions that do have tran
>> parameter cannot be simply used in the block-graph change transaction,
>> as you don't follow the common paradigm, that in .prepare we do all
>> visible changes. That's why you have to still use _try_*() version that
>> creates seaparate transaction object and completes it: after that the
>> action is actually done and other graph-modifying actions can be done on
>> top.
>>
>> So, IMHO, we shouldn't go this way, as that adds transaction actions
>> that actually can't be used in common block-graph-modifying transaction
>> but only context of bdrv_try_change_aio_context() internal transaction.
>>
>> I agree that aio-context change should finally be rewritten to take a
>> native place in block-graph transactions, but that should be a complete
>> solution, introducing native bdrv_change_aio_context(..., tran)
>> transaction action that is directly used in the block-graph transaction,
>> do visible effect in .prepare and don't create extra Transaction objects.
>>

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Vladimir Sementsov-Ogievskiy]

On 8/5/22 16:36, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 05/08/2022 um 15:22 schrieb Emanuele Giuseppe Esposito:
>>
>>
>> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>>>> 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>
>>>
>>>
>>
>> So, I read your email before going on PTO and at that point I got what
>> your concerns were, but now after re-reading it I don't understand
>> anymore what you mean :)
>>
>>> What I dislike here is that you refactor aio-context-change to use
>>> transactions, but you use it "internally" for aio-context-change.
>>> aio-context-change doesn't become a native part of block-graph
>>> modifiction transaction system after the series.
>>>
>>> For example, bdrv_attach_child_common(..., tran) still calls
>>> bdrv_try_change_aio_context() function which doesn't take @tran
>>> argument. And we have to call bdrv_try_change_aio_context() again in
>>> bdrv_attach_child_common_abort() with opposite arguments by hand. We
>>> create in _try_ separate Transaction object which is unrelated to the
>>> original block-graph-change transaction.
>>>
>>
>> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
>> transaction parameter" supports taking transaction as a parameter.
>> bdrv_attach_child_common could simply call
>> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
>> it could work).
> 
> No actually I don't get how it can work in bdrv_attach_child_common.
> We have the following:
> 
> parent_ctx = bdrv_child_get_parent_aio_context(new_child);
> if (child_ctx != parent_ctx) {
>        int ret = bdrv_try_change_aio_context(child_bs, parent_ctx, NULL,
>                                              &local_err);
> 
>        if (ret < 0 && child_class->change_aio_ctx) {
>            ret_child = child_class->change_aio_ctx(new_child, child_ctx,
>                                                    visited, tran, NULL);
> 
>            tran_finalize(tran, ret_child == true ? 0 : -1);
>        }
> 
>        if (ret < 0) {
>            return ret;
>        }
> }
> 
> bdrv_replace_child_noperm(&new_child, child_bs, true);
> 
> So bdrv_try_change_aio_context would be changed in
> bdrv_try_change_aio_context_tran, but then how can we call
> bdrv_replace_child_noperm if no aiocontext has been changed at all?
> I don't think we can mix transactional operations with non-transactional.
> 

So here, bdrv_try_change_aio_context() is .prepare in the way I mean.

And than in .abort we call bdrv_try_change_aio_context() again but with reverse argument, and it's a kind of ".abort".

Probably, we can refactor that, making a function bdrv_change_aio_context(, .., tran), which does what bdrv_try_change_aio_context does, and registers .abort callback, that will simulate calling bdrv_try_change_aio_context() with opposite arguement.  But we should carefully refactor all the function names and avoid having nested transaction.

> 
>>
>> I think the main concern here is that during the prepare phase this
>> serie doesn't change any aiocontext, so until we don't commit the rest
>> of the code cannot assume that the aiocontext has been changed.
>>
>> But isn't it what happens also for permissions? Permission functions
>> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then
>> bdrv_set_perm() in commit.
>>
>> Another important question is that if we actually want to put everything
>> in a single transaction, because .prepare functions like the one
>> proposed here perform drains, so the logic following prepare and
>> preceding commit must take into account that everything is drained. Also
>> prepare itself has to take into account that maybe other .prepare took
>> locks or drained themselves...
>>
>>> With good refactoring we should get rid of these _try_ functions, and
>>> have just bdrv_change_aio_context(..., tran) that can be natively used
>>> with external tran object, where other block-graph change actions
>>> participate. This way we'll not have to call reverse
>>> aio_context_change() in .abort, it will be done automatically.
>>>
>>> Moreover, your  *aio_context_change* functions that do have tran
>>> parameter cannot be simply used in the block-graph change transaction,
>>> as you don't follow the common paradigm, that in .prepare we do all
>>> visible changes. That's why you have to still use _try_*() version that
>>> creates seaparate transaction object and completes it: after that the
>>> action is actually done and other graph-modifying actions can be done on
>>> top.
>>>
>>> So, IMHO, we shouldn't go this way, as that adds transaction actions
>>> that actually can't be used in common block-graph-modifying transaction
>>> but only context of bdrv_try_change_aio_context() internal transaction.
>>>
>>> I agree that aio-context change should finally be rewritten to take a
>>> native place in block-graph transactions, but that should be a complete
>>> solution, introducing native bdrv_change_aio_context(..., tran)
>>> transaction action that is directly used in the block-graph transaction,
>>> do visible effect in .prepare and don't create extra Transaction objects.
>>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Vladimir Sementsov-Ogievskiy]

On 8/5/22 16:22, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>>> 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>
>>
>>
> 
> So, I read your email before going on PTO and at that point I got what
> your concerns were, but now after re-reading it I don't understand
> anymore what you mean :)
> 
>> What I dislike here is that you refactor aio-context-change to use
>> transactions, but you use it "internally" for aio-context-change.
>> aio-context-change doesn't become a native part of block-graph
>> modifiction transaction system after the series.
>>
>> For example, bdrv_attach_child_common(..., tran) still calls
>> bdrv_try_change_aio_context() function which doesn't take @tran
>> argument. And we have to call bdrv_try_change_aio_context() again in
>> bdrv_attach_child_common_abort() with opposite arguments by hand. We
>> create in _try_ separate Transaction object which is unrelated to the
>> original block-graph-change transaction.
>>
> 
> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
> transaction parameter" supports taking transaction as a parameter.
> bdrv_attach_child_common could simply call
> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
> it could work).
> 
> I think the main concern here is that during the prepare phase this
> serie doesn't change any aiocontext, so until we don't commit the rest
> of the code cannot assume that the aiocontext has been changed.
> 
> But isn't it what happens also for permissions? Permission functions
> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then
> bdrv_set_perm() in commit.

Not exactly.

Partly that's just old bad naming. Ideally, driver handlers should be refactored to have one
.bdrv_set_perm(, ... tran, errp) handler. Or at least renamed to .prepare and .commit instead of misleading .check and .set.

Secondly what is important, is that corresponding .set actions are not visible to other block-graph modifying actions (like taking locks on fd. other actions, like attach/detach children don't care of it)/ (Or, at least, they _shoud not be_ visible :) To be honest, I don't have real expertise, of how much these .set actions are visible or not by other block-graph modifying actions, but I believe that we are OK in common scenarios).

What is really visible at generic block layer? Visible is change of .perm / .shared_perm fields of BdrvChild. And they are set in .prepare, look at bdrv_child_set_perm().

So, after calling bdrv_child_set_perm() other actions of .prepare stage will see _updated_ permissions. And if transaction fails, we rollback .perm and .shared_perm fields in bdrv_child_set_perm_abort()


> 
> Another important question is that if we actually want to put everything
> in a single transaction, because .prepare functions like the one
> proposed here perform drains, so the logic following prepare and
> preceding commit must take into account that everything is drained. Also
> prepare itself has to take into account that maybe other .prepare took
> locks or drained themselves...

Yes, untie the knot of drained sections during aio context change is a challenge.. And that's (at least on of) the reason why aio-context change is still not a native part of block graph modifying transactions.

Could there be some simple solution?

Like

1. drain the whole subgraph of all nodes connected with nodes that we are going to touch

2. do block graph modifying transaction (including aio context change) knowing that everything we need is drained. (so we don't have to care about drain during aio context change)

3. undrain the subgraph

In other words, is that really necessary to lock/unlock different contexts during the aio-context-change procedure? Or we can move to a lot larger and simpler drained section?

> 
>> With good refactoring we should get rid of these _try_ functions, and
>> have just bdrv_change_aio_context(..., tran) that can be natively used
>> with external tran object, where other block-graph change actions
>> participate. This way we'll not have to call reverse
>> aio_context_change() in .abort, it will be done automatically.
>>
>> Moreover, your  *aio_context_change* functions that do have tran
>> parameter cannot be simply used in the block-graph change transaction,
>> as you don't follow the common paradigm, that in .prepare we do all
>> visible changes. That's why you have to still use _try_*() version that
>> creates seaparate transaction object and completes it: after that the
>> action is actually done and other graph-modifying actions can be done on
>> top.
>>
>> So, IMHO, we shouldn't go this way, as that adds transaction actions
>> that actually can't be used in common block-graph-modifying transaction
>> but only context of bdrv_try_change_aio_context() internal transaction.
>>
>> I agree that aio-context change should finally be rewritten to take a
>> native place in block-graph transactions, but that should be a complete
>> solution, introducing native bdrv_change_aio_context(..., tran)
>> transaction action that is directly used in the block-graph transaction,
>> do visible effect in .prepare and don't create extra Transaction objects.
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Emanuele Giuseppe Esposito]

Am 05/08/2022 um 16:35 schrieb Vladimir Sementsov-Ogievskiy:
> On 8/5/22 16:22, Emanuele Giuseppe Esposito wrote:
>>
>>
>> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>>>> 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>
>>>
>>>
>>
>> So, I read your email before going on PTO and at that point I got what
>> your concerns were, but now after re-reading it I don't understand
>> anymore what you mean :)
>>
>>> What I dislike here is that you refactor aio-context-change to use
>>> transactions, but you use it "internally" for aio-context-change.
>>> aio-context-change doesn't become a native part of block-graph
>>> modifiction transaction system after the series.
>>>
>>> For example, bdrv_attach_child_common(..., tran) still calls
>>> bdrv_try_change_aio_context() function which doesn't take @tran
>>> argument. And we have to call bdrv_try_change_aio_context() again in
>>> bdrv_attach_child_common_abort() with opposite arguments by hand. We
>>> create in _try_ separate Transaction object which is unrelated to the
>>> original block-graph-change transaction.
>>>
>>
>> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
>> transaction parameter" supports taking transaction as a parameter.
>> bdrv_attach_child_common could simply call
>> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
>> it could work).
>>
>> I think the main concern here is that during the prepare phase this
>> serie doesn't change any aiocontext, so until we don't commit the rest
>> of the code cannot assume that the aiocontext has been changed.
>>
>> But isn't it what happens also for permissions? Permission functions
>> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then
>> bdrv_set_perm() in commit.
> 
> Not exactly.
> 
> Partly that's just old bad naming. Ideally, driver handlers should be
> refactored to have one
> .bdrv_set_perm(, ... tran, errp) handler. Or at least renamed to
> .prepare and .commit instead of misleading .check and .set.
> 
> Secondly what is important, is that corresponding .set actions are not
> visible to other block-graph modifying actions (like taking locks on fd.
> other actions, like attach/detach children don't care of it)/ (Or, at
> least, they _shoud not be_ visible :) To be honest, I don't have real
> expertise, of how much these .set actions are visible or not by other
> block-graph modifying actions, but I believe that we are OK in common
> scenarios).
> 
> What is really visible at generic block layer? Visible is change of
> .perm / .shared_perm fields of BdrvChild. And they are set in .prepare,
> look at bdrv_child_set_perm().
> 
> So, after calling bdrv_child_set_perm() other actions of .prepare stage
> will see _updated_ permissions. And if transaction fails, we rollback
> .perm and .shared_perm fields in bdrv_child_set_perm_abort()
> 
> 
>>
>> Another important question is that if we actually want to put everything
>> in a single transaction, because .prepare functions like the one
>> proposed here perform drains, so the logic following prepare and
>> preceding commit must take into account that everything is drained. Also
>> prepare itself has to take into account that maybe other .prepare took
>> locks or drained themselves...
> 
> Yes, untie the knot of drained sections during aio context change is a
> challenge.. And that's (at least on of) the reason why aio-context
> change is still not a native part of block graph modifying transactions.
> 
> Could there be some simple solution?
> 
> Like
> 
> 1. drain the whole subgraph of all nodes connected with nodes that we
> are going to touch
> 
> 2. do block graph modifying transaction (including aio context change)
> knowing that everything we need is drained. (so we don't have to care
> about drain during aio context change)
> 
> 3. undrain the subgraph
> 
> In other words, is that really necessary to lock/unlock different
> contexts during the aio-context-change procedure? Or we can move to a
> lot larger and simpler drained section?

Unfortunately I think the aiocontext lock have to stay where they
currently are. I documented it in this serie.

drained begin needs the old aiocontext, and drained end the new one,
since we moved the graph to a different aiocontext.

Also, if I understand correctly you suggest:

.prepare = check and then change aiocontext
.abort = revert aiocontext change
.commit = nothing

drain_begin_all();
prepare();
drain_end_all();

if prepare is not OK:
	tran_abort(); // or simply return error so the caller calls abort

But then:
1) .abort needs draining too
2) it is not so different from what it was before, isn't it?

Emanuele


> 
>>
>>> With good refactoring we should get rid of these _try_ functions, and
>>> have just bdrv_change_aio_context(..., tran) that can be natively used
>>> with external tran object, where other block-graph change actions
>>> participate. This way we'll not have to call reverse
>>> aio_context_change() in .abort, it will be done automatically.
>>>
>>> Moreover, your  *aio_context_change* functions that do have tran
>>> parameter cannot be simply used in the block-graph change transaction,
>>> as you don't follow the common paradigm, that in .prepare we do all
>>> visible changes. That's why you have to still use _try_*() version that
>>> creates seaparate transaction object and completes it: after that the
>>> action is actually done and other graph-modifying actions can be done on
>>> top.
>>>
>>> So, IMHO, we shouldn't go this way, as that adds transaction actions
>>> that actually can't be used in common block-graph-modifying transaction
>>> but only context of bdrv_try_change_aio_context() internal transaction.
>>>
>>> I agree that aio-context change should finally be rewritten to take a
>>> native place in block-graph transactions, but that should be a complete
>>> solution, introducing native bdrv_change_aio_context(..., tran)
>>> transaction action that is directly used in the block-graph transaction,
>>> do visible effect in .prepare and don't create extra Transaction
>>> objects.
>>>
>>
> 
> 

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Vladimir Sementsov-Ogievskiy]

On 8/5/22 17:57, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 05/08/2022 um 16:35 schrieb Vladimir Sementsov-Ogievskiy:
>> On 8/5/22 16:22, Emanuele Giuseppe Esposito wrote:
>>>
>>>
>>> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
>>>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>>>>> 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>
>>>>
>>>>
>>>
>>> So, I read your email before going on PTO and at that point I got what
>>> your concerns were, but now after re-reading it I don't understand
>>> anymore what you mean :)
>>>
>>>> What I dislike here is that you refactor aio-context-change to use
>>>> transactions, but you use it "internally" for aio-context-change.
>>>> aio-context-change doesn't become a native part of block-graph
>>>> modifiction transaction system after the series.
>>>>
>>>> For example, bdrv_attach_child_common(..., tran) still calls
>>>> bdrv_try_change_aio_context() function which doesn't take @tran
>>>> argument. And we have to call bdrv_try_change_aio_context() again in
>>>> bdrv_attach_child_common_abort() with opposite arguments by hand. We
>>>> create in _try_ separate Transaction object which is unrelated to the
>>>> original block-graph-change transaction.
>>>>
>>>
>>> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
>>> transaction parameter" supports taking transaction as a parameter.
>>> bdrv_attach_child_common could simply call
>>> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
>>> it could work).
>>>
>>> I think the main concern here is that during the prepare phase this
>>> serie doesn't change any aiocontext, so until we don't commit the rest
>>> of the code cannot assume that the aiocontext has been changed.
>>>
>>> But isn't it what happens also for permissions? Permission functions
>>> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then
>>> bdrv_set_perm() in commit.
>>
>> Not exactly.
>>
>> Partly that's just old bad naming. Ideally, driver handlers should be
>> refactored to have one
>> .bdrv_set_perm(, ... tran, errp) handler. Or at least renamed to
>> .prepare and .commit instead of misleading .check and .set.
>>
>> Secondly what is important, is that corresponding .set actions are not
>> visible to other block-graph modifying actions (like taking locks on fd.
>> other actions, like attach/detach children don't care of it)/ (Or, at
>> least, they _shoud not be_ visible :) To be honest, I don't have real
>> expertise, of how much these .set actions are visible or not by other
>> block-graph modifying actions, but I believe that we are OK in common
>> scenarios).
>>
>> What is really visible at generic block layer? Visible is change of
>> .perm / .shared_perm fields of BdrvChild. And they are set in .prepare,
>> look at bdrv_child_set_perm().
>>
>> So, after calling bdrv_child_set_perm() other actions of .prepare stage
>> will see _updated_ permissions. And if transaction fails, we rollback
>> .perm and .shared_perm fields in bdrv_child_set_perm_abort()
>>
>>
>>>
>>> Another important question is that if we actually want to put everything
>>> in a single transaction, because .prepare functions like the one
>>> proposed here perform drains, so the logic following prepare and
>>> preceding commit must take into account that everything is drained. Also
>>> prepare itself has to take into account that maybe other .prepare took
>>> locks or drained themselves...
>>
>> Yes, untie the knot of drained sections during aio context change is a
>> challenge.. And that's (at least on of) the reason why aio-context
>> change is still not a native part of block graph modifying transactions.
>>
>> Could there be some simple solution?
>>
>> Like
>>
>> 1. drain the whole subgraph of all nodes connected with nodes that we
>> are going to touch
>>
>> 2. do block graph modifying transaction (including aio context change)
>> knowing that everything we need is drained. (so we don't have to care
>> about drain during aio context change)
>>
>> 3. undrain the subgraph
>>
>> In other words, is that really necessary to lock/unlock different
>> contexts during the aio-context-change procedure? Or we can move to a
>> lot larger and simpler drained section?
> 
> Unfortunately I think the aiocontext lock have to stay where they
> currently are. I documented it in this serie.
> 
> drained begin needs the old aiocontext, and drained end the new one,
> since we moved the graph to a different aiocontext.
> 
> Also, if I understand correctly you suggest:
> 
> .prepare = check and then change aiocontext
> .abort = revert aiocontext change
> .commit = nothing

Yes. And that's is how it used actually now in transactions, for example:
   bdrv_attach_child_common (which is .prepare) calls bdrv_try_set_aio_context() (which is check and then change)
   bdrv_attach_child_common_abort (which is .abort) calls bdrv_try_set_aio_context() to revert .prepare

> 
> drain_begin_all();
> prepare();
> drain_end_all();
> 
> if prepare is not OK:
> 	tran_abort(); // or simply return error so the caller calls abort
> 
> But then:
> 1) .abort needs draining too
> 2) it is not so different from what it was before, isn't it?
> 

No, I mean

drain_begin_all();

Do the whole transaction, i.e. prepare + commit or prepare + abort. All the actions, connected into transaction: changing aio context, permissions and block graph relations.

drain_end_all();


> 
> 
>>
>>>
>>>> With good refactoring we should get rid of these _try_ functions, and
>>>> have just bdrv_change_aio_context(..., tran) that can be natively used
>>>> with external tran object, where other block-graph change actions
>>>> participate. This way we'll not have to call reverse
>>>> aio_context_change() in .abort, it will be done automatically.
>>>>
>>>> Moreover, your  *aio_context_change* functions that do have tran
>>>> parameter cannot be simply used in the block-graph change transaction,
>>>> as you don't follow the common paradigm, that in .prepare we do all
>>>> visible changes. That's why you have to still use _try_*() version that
>>>> creates seaparate transaction object and completes it: after that the
>>>> action is actually done and other graph-modifying actions can be done on
>>>> top.
>>>>
>>>> So, IMHO, we shouldn't go this way, as that adds transaction actions
>>>> that actually can't be used in common block-graph-modifying transaction
>>>> but only context of bdrv_try_change_aio_context() internal transaction.
>>>>
>>>> I agree that aio-context change should finally be rewritten to take a
>>>> native place in block-graph transactions, but that should be a complete
>>>> solution, introducing native bdrv_change_aio_context(..., tran)
>>>> transaction action that is directly used in the block-graph transaction,
>>>> do visible effect in .prepare and don't create extra Transaction
>>>> objects.
>>>>
>>>
>>
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Paolo Bonzini]

On 8/6/22 17:32, Vladimir Sementsov-Ogievskiy wrote:
>> if I understand correctly you suggest:
>>
>> .prepare = check and then change aiocontext
>> .abort = revert aiocontext change
>> .commit = nothing
> 
> Yes. And that's is how it used actually now in transactions, for example:
>    bdrv_attach_child_common (which is .prepare) calls 
> bdrv_try_set_aio_context() (which is check and then change)
>    bdrv_attach_child_common_abort (which is .abort) calls 
> bdrv_try_set_aio_context() to revert .prepare

I see your point, but I think this can be solved just with a doc comment 
explaining why the transaction is "local".  This is already clear in 
bdrv_child_try_change_aio_context (which doesn't take a Transaction*), 
it can be documented in bdrv_child_change_aio_context as well.

After all, the point of this series is just to avoid code duplication in 
the two visits of the graph, and secondarily to unify the .can_set and 
.set callbacks into one.  We could do it just as well without 
transaction with just a GList of BdrvChild* (keeping the callbcks 
separate); what the Transaction API gives is a little more abstraction, 
by not having to do linked list manipulation by hand.

To be honest I also don't like very much the placement of the 
drained_begin/drained_end in bdrv_change_aio_context.  But if the needs 
arises to call bdrv_change_aio_context within another transaction, I 
think they can be pulled relatively easily (as a subtree drain) in 
bdrv_child_try_change_aio_context or in its callers.  For now, this 
series is already an improvement on several counts.

Paolo