[RFC,v2,13/25] include/systemu/blockdev.h: global state API

Commit Message

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

blockdev functions run always under the BQL lock.

Signed-off-by: Emanuele Giuseppe Esposito <[e-mail]>
---
 include/sysemu/blockdev.h | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

Comments

Paolo Bonzini Oct. 7, 2021, noon UTC | #1

On 05/10/21 16:32, Emanuele Giuseppe Esposito wrote:
>   DriveInfo *drive_get_next(BlockInterfaceType type);
>   
> +DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
> +                     Error **errp);
> +
> +/* Common functions that are neither I/O nor Global State */
> +
> +DriveInfo *blk_legacy_dinfo(BlockBackend *blk);
> +int drive_get_max_devs(BlockInterfaceType type);
> +
>   QemuOpts *drive_def(const char *optstr);
> +
>   QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>                       const char *optstr);
> -DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
> -                     Error **errp);

drive_add and drive_def touch global state (QemuOpts).  But really 
neither should be in this header: drive_add can be moved to 
softmmu/vl.c, while drive_def can be inlined into its two callers.  With 
that changed,

Reviewed-by: Paolo Bonzini <[e-mail]>

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

On Tue, Oct 05, 2021 at 10:32:03AM -0400, Emanuele Giuseppe Esposito wrote:
> blockdev functions run always under the BQL lock.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <[e-mail]>
> ---
>  include/sysemu/blockdev.h | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 32c2d6023c..28233f6b63 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -38,24 +38,49 @@ struct DriveInfo {
>      QTAILQ_ENTRY(DriveInfo) next;
>  };
>  
> -DriveInfo *blk_legacy_dinfo(BlockBackend *blk);
> +/*
> + * 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 in this header must use this assertion:
> + * g_assert(qemu_in_main_thread());
> + * to catch when they are accidentally called without the BQL.
> + */
> +
>  DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo);
>  BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
>  
>  void override_max_devs(BlockInterfaceType type, int max_devs);
>  
>  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
> -void drive_mark_claimed_by_board(void);

This function prototype is no longer used. Please make a note of this in
the commit description so reviewers know the deletion is intentional and
the reason for it. (It could have been an accident so I had to grep the
code to figure out why you did this.)

Otherwise:

Reviewed-by: Stefan Hajnoczi <[e-mail]>

Patch

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 32c2d6023c..28233f6b63 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -38,24 +38,49 @@  struct DriveInfo {
     QTAILQ_ENTRY(DriveInfo) next;
 };
 
-DriveInfo *blk_legacy_dinfo(BlockBackend *blk);
+/*
+ * 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 in this header must use this assertion:
+ * g_assert(qemu_in_main_thread());
+ * to catch when they are accidentally called without the BQL.
+ */
+
 DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo);
 BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
 
 void override_max_devs(BlockInterfaceType type, int max_devs);
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
-void drive_mark_claimed_by_board(void);
 void drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
-int drive_get_max_devs(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
 
+DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
+                     Error **errp);
+
+/* Common functions that are neither I/O nor Global State */
+
+DriveInfo *blk_legacy_dinfo(BlockBackend *blk);
+int drive_get_max_devs(BlockInterfaceType type);
+
 QemuOpts *drive_def(const char *optstr);
+
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr);
-DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
-                     Error **errp);
 
 #endif