Message ID | 20220712211911.1302836-3-eesposit@redhat.com |
---|---|
State | New |
Headers | show |
Series | Refactor bdrv_try_set_aio_context using transactions | expand |
On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote: > First change the transactions from a QLIST to QSIMPLEQ, then > use it to implement tran_add_tail, which allows adding elements > to the end of list transactions. The subject still calls it `tran_add_back()` (perhaps from a preliminary version?), I think that needs adjustment. > This is useful if we have some "preparation" transiction callbacks *transaction > that we want to run before the others but still only when invoking > finalize/commit/abort. I don’t understand this yet (but perhaps it’ll become clearer with the following patches); doesn’t the new function do the opposite? I.e., basically add some clean-up that’s only used after everything else? > For example (A and B are lists transaction callbacks): > > for (i=0; i < 3; i++) { > tran_add(A[i]); > tran_add_tail(B[i]); > } > > tran_commit(); > > Will process transactions in this order: A2 - A1 - A0 - B0 - B1 - B2 > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > include/qemu/transactions.h | 9 +++++++++ > util/transactions.c | 29 +++++++++++++++++++++-------- > 2 files changed, 30 insertions(+), 8 deletions(-) > > diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h > index 2f2060acd9..42783720b9 100644 > --- a/include/qemu/transactions.h > +++ b/include/qemu/transactions.h > @@ -50,7 +50,16 @@ typedef struct TransactionActionDrv { > typedef struct Transaction Transaction; > > Transaction *tran_new(void); > +/* > + * Add transaction at the beginning of the transaction list. > + * @tran will be the first transaction to be processed in finalize/commit/abort. Of course, if you call tran_add() afterwards, this transaction will no longer be the first one. I mean, that’s kind of obvious, but perhaps we can still express that here. Like, perhaps, “finalize/commit/abort process this list in order, so after this call, @tran will be the first transaction to be processed”? > + */ > void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque); > +/* > + * Add transaction at the end of the transaction list. > + * @tran will be the last transaction to be processed in finalize/commit/abort. (And then “finalize/commit/abort process this list in order, so after this call, @tran will be the last transaction to be processed”) > + */ > +void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque); > void tran_abort(Transaction *tran); > void tran_commit(Transaction *tran); > > diff --git a/util/transactions.c b/util/transactions.c > index 2dbdedce95..89e541c4a4 100644 > --- a/util/transactions.c > +++ b/util/transactions.c [...] > @@ -54,20 +54,33 @@ void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque) > .opaque = opaque > }; > > - QSLIST_INSERT_HEAD(&tran->actions, act, entry); > + QSIMPLEQ_INSERT_HEAD(&tran->actions, act, entry); > +} > + > +void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque) > +{ > + TransactionAction *act; > + > + act = g_new(TransactionAction, 1); > + *act = (TransactionAction) { > + .drv = drv, > + .opaque = opaque > + }; > + > + QSIMPLEQ_INSERT_TAIL(&tran->actions, act, entry); > } Perhaps this could benefit from a function encompassing the common functionality, i.e. a tran_do_add(..., bool tail) with if (tail) { QSIMPLEQ_INSERT_TAIL(...); } else { QSIMPLEQ_INSERT_HEAD(...); } (Just a light suggestion.) Hanna
On 7/14/22 17:13, Hanna Reitz wrote: >> that we want to run before the others but still only when invoking >> finalize/commit/abort. > > I don’t understand this yet (but perhaps it’ll become clearer with the > following patches); doesn’t the new function do the opposite? I.e., > basically add some clean-up that’s only used after everything else? I agree about the commit message being incorrect, but is there any reason why transactions work LIFO by default? Is there anything that requires that behavior? Paolo
On 7/18/22 19:20, Paolo Bonzini wrote: > On 7/14/22 17:13, Hanna Reitz wrote: >>> that we want to run before the others but still only when invoking >>> finalize/commit/abort. >> >> I don’t understand this yet (but perhaps it’ll become clearer with the following patches); doesn’t the new function do the opposite? I.e., basically add some clean-up that’s only used after everything else? > > I agree about the commit message being incorrect, but is there any reason why transactions work LIFO by default? Is there anything that requires that behavior? > Yes. On abort() we want to rollback actions in reverse order. When we create _abort() part of some action we assume that current graph state is exactly the same like it was after _prepare() call, otherwise it just will not work. That means that all actions called _after_ action X, are already rolled-back when we call X_abort(). And keeping that in mind I'm afraid of implementing a possibility to break this order.. Note also, that in block transaction API, most of the action is usually done in _prepare(), so that next action in transaction can rely on result of previous action.
diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h index 2f2060acd9..42783720b9 100644 --- a/include/qemu/transactions.h +++ b/include/qemu/transactions.h @@ -50,7 +50,16 @@ typedef struct TransactionActionDrv { typedef struct Transaction Transaction; Transaction *tran_new(void); +/* + * Add transaction at the beginning of the transaction list. + * @tran will be the first transaction to be processed in finalize/commit/abort. + */ void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque); +/* + * Add transaction at the end of the transaction list. + * @tran will be the last transaction to be processed in finalize/commit/abort. + */ +void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque); void tran_abort(Transaction *tran); void tran_commit(Transaction *tran); diff --git a/util/transactions.c b/util/transactions.c index 2dbdedce95..89e541c4a4 100644 --- a/util/transactions.c +++ b/util/transactions.c @@ -28,18 +28,18 @@ typedef struct TransactionAction { TransactionActionDrv *drv; void *opaque; - QSLIST_ENTRY(TransactionAction) entry; + QSIMPLEQ_ENTRY(TransactionAction) entry; } TransactionAction; struct Transaction { - QSLIST_HEAD(, TransactionAction) actions; + QSIMPLEQ_HEAD(, TransactionAction) actions; }; Transaction *tran_new(void) { Transaction *tran = g_new(Transaction, 1); - QSLIST_INIT(&tran->actions); + QSIMPLEQ_INIT(&tran->actions); return tran; } @@ -54,20 +54,33 @@ void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque) .opaque = opaque }; - QSLIST_INSERT_HEAD(&tran->actions, act, entry); + QSIMPLEQ_INSERT_HEAD(&tran->actions, act, entry); +} + +void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque) +{ + TransactionAction *act; + + act = g_new(TransactionAction, 1); + *act = (TransactionAction) { + .drv = drv, + .opaque = opaque + }; + + QSIMPLEQ_INSERT_TAIL(&tran->actions, act, entry); } void tran_abort(Transaction *tran) { TransactionAction *act, *next; - QSLIST_FOREACH(act, &tran->actions, entry) { + QSIMPLEQ_FOREACH(act, &tran->actions, entry) { if (act->drv->abort) { act->drv->abort(act->opaque); } } - QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) { + QSIMPLEQ_FOREACH_SAFE(act, &tran->actions, entry, next) { if (act->drv->clean) { act->drv->clean(act->opaque); } @@ -82,13 +95,13 @@ void tran_commit(Transaction *tran) { TransactionAction *act, *next; - QSLIST_FOREACH(act, &tran->actions, entry) { + QSIMPLEQ_FOREACH(act, &tran->actions, entry) { if (act->drv->commit) { act->drv->commit(act->opaque); } } - QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) { + QSIMPLEQ_FOREACH_SAFE(act, &tran->actions, entry, next) { if (act->drv->clean) { act->drv->clean(act->opaque); }
First change the transactions from a QLIST to QSIMPLEQ, then use it to implement tran_add_tail, which allows adding elements to the end of list transactions. This is useful if we have some "preparation" transiction callbacks that we want to run before the others but still only when invoking finalize/commit/abort. For example (A and B are lists transaction callbacks): for (i=0; i < 3; i++) { tran_add(A[i]); tran_add_tail(B[i]); } tran_commit(); Will process transactions in this order: A2 - A1 - A0 - B0 - B1 - B2 Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- include/qemu/transactions.h | 9 +++++++++ util/transactions.c | 29 +++++++++++++++++++++-------- 2 files changed, 30 insertions(+), 8 deletions(-)