[RFC,v2,17/25] include/block/transactions: global state API + assertions

Commit Message

Emanuele Giuseppe Esposito Oct. 5, 2021, 2:32 p.m. UTC

transactions run always under the BQL lock, so they are all
in the global state API.

Signed-off-by: Emanuele Giuseppe Esposito <[e-mail]>
---
 include/qemu/transactions.h | 24 ++++++++++++++++++++++++
 util/transactions.c         |  4 ++++
 2 files changed, 28 insertions(+)

Comments

Stefan Hajnoczi Oct. 7, 2021, 2:47 p.m. UTC | #1

On Tue, Oct 05, 2021 at 10:32:07AM -0400, Emanuele Giuseppe Esposito wrote:
> transactions run always under the BQL lock, so they are all
> in the global state API.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <[e-mail]>
> ---
>  include/qemu/transactions.h | 24 ++++++++++++++++++++++++
>  util/transactions.c         |  4 ++++
>  2 files changed, 28 insertions(+)

Hmm...not sure about this. Maybe Vladimir can share his thoughts.

This seems like a library that can be used in various situations. It has
no connection to the BQL. There's no need to declare it GS or I/O
because it's just a utility just like QEMUIOVector, etc.

Emanuele Giuseppe Esposito Oct. 8, 2021, 7:18 a.m. UTC | #2

On 07/10/2021 16:47, Stefan Hajnoczi wrote:
> On Tue, Oct 05, 2021 at 10:32:07AM -0400, Emanuele Giuseppe Esposito wrote:
>> transactions run always under the BQL lock, so they are all
>> in the global state API.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <[e-mail]>
>> ---
>>   include/qemu/transactions.h | 24 ++++++++++++++++++++++++
>>   util/transactions.c         |  4 ++++
>>   2 files changed, 28 insertions(+)
> 
> Hmm...not sure about this. Maybe Vladimir can share his thoughts.
> 
> This seems like a library that can be used in various situations. It has
> no connection to the BQL. There's no need to declare it GS or I/O
> because it's just a utility just like QEMUIOVector, etc.
> 

Ok, I can remove this patch for next version, and then we can discuss 
whether to add it or not.

Thank you,
Emanuele

Patch

diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h
index 92c5965235..f4a7c473fa 100644
--- a/include/qemu/transactions.h
+++ b/include/qemu/transactions.h
@@ -37,6 +37,29 @@ 
 #define QEMU_TRANSACTIONS_H
 
 #include <gmodule.h>
+#include "qemu/main-loop.h"
+
+/*
+ * Global state (GS) API. These functions run under the BQL lock.
+ *
+ * If a function modifies the graph, it also uses drain and/or
+ * aio_context_acquire/release to be sure it has unique access.
+ * aio_context locking is needed together with BQL because of
+ * the thread-safe I/O API that concurrently runs and accesses
+ * the graph without the BQL.
+ *
+ * It is important to note that not all of these functions are
+ * necessarily limited to running under the BQL, but they would
+ * require additional auditing and may small thread-safety changes
+ * to move them into the I/O API. Often it's not worth doing that
+ * work since the APIs are only used with the BQL held at the
+ * moment, so they have been placed in the GS API (for now).
+ *
+ * All functions and function pointers in this header must use
+ * this assertion:
+ * g_assert(qemu_in_main_thread());
+ * to catch when they are accidentally called without the BQL.
+ */
 
 typedef struct TransactionActionDrv {
     void (*abort)(void *opaque);
@@ -53,6 +76,7 @@  void tran_commit(Transaction *tran);
 
 static inline void tran_finalize(Transaction *tran, int ret)
 {
+    g_assert(qemu_in_main_thread());
     if (ret < 0) {
         tran_abort(tran);
     } else {
diff --git a/util/transactions.c b/util/transactions.c
index d0bc9a3e73..20c3dafdb8 100644
--- a/util/transactions.c
+++ b/util/transactions.c
@@ -23,6 +23,7 @@ 
 #include "qemu/osdep.h"
 
 #include "qemu/transactions.h"
+#include "qemu/main-loop.h"
 #include "qemu/queue.h"
 
 typedef struct TransactionAction {
@@ -47,6 +48,7 @@  Transaction *tran_new(void)
 void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque)
 {
     TransactionAction *act;
+    g_assert(qemu_in_main_thread());
 
     act = g_new(TransactionAction, 1);
     *act = (TransactionAction) {
@@ -60,6 +62,7 @@  void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque)
 void tran_abort(Transaction *tran)
 {
     TransactionAction *act, *next;
+    g_assert(qemu_in_main_thread());
 
     QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
         if (act->drv->abort) {
@@ -79,6 +82,7 @@  void tran_abort(Transaction *tran)
 void tran_commit(Transaction *tran)
 {
     TransactionAction *act, *next;
+    g_assert(qemu_in_main_thread());
 
     QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
         if (act->drv->commit) {