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