Re: [PATCH v2 02/11] block: use transactions as a replacement of ->{can_}set_aio_context()
> 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
Re: [PATCH v2 02/11] block: use transactions as a replacement of ->{can_}set_aio_context()
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
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);
>
[PATCH v2 02/11] block: use transactions as a replacement of ->{can_}set_aio_context()
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
---
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();
+
