Re: [PATCH v4 04/25] include/sysemu/block-backend: split header into I/O and global state (GS) API
On 16.11.21 15:24, Emanuele Giuseppe Esposito wrote: On 12/11/2021 13:30, Hanna Reitz wrote: On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote: Similarly to the previous patches, split block-backend.h in block-backend-io.h and block-backend-global-state.h In addition, remove "block/block.h" include as it seems it is not necessary anymore, together with "qemu/iov.h" block-backend-common.h contains the structures shared between the two headers, and the functions that can't be categorized as I/O or global state. Assertions are added in the next patch. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block/block-backend.c | 9 +- include/sysemu/block-backend-common.h | 74 ++ include/sysemu/block-backend-global-state.h | 122 + include/sysemu/block-backend-io.h | 139 ++ include/sysemu/block-backend.h | 269 +--- 5 files changed, 344 insertions(+), 269 deletions(-) create mode 100644 include/sysemu/block-backend-common.h create mode 100644 include/sysemu/block-backend-global-state.h create mode 100644 include/sysemu/block-backend-io.h [...] diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index e5e1524f06..038be9fc40 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -13,272 +13,9 @@ #ifndef BLOCK_BACKEND_H #define BLOCK_BACKEND_H -#include "qemu/iov.h" -#include "block/throttle-groups.h" +#include "block-backend-global-state.h" +#include "block-backend-io.h" -/* - * TODO Have to include block/block.h for a bunch of block layer - * types. Unfortunately, this pulls in the whole BlockDriverState - * API, which we don't want used by many BlockBackend users. Some of - * the types belong here, and the rest should be split into a common - * header and one for the BlockDriverState API. - */ -#include "block/block.h" This note and the include is gone. Sounds like something positive, but why is this possible? Basically block/throttle-groups.h includes block/block_int.h that internally includes block/block.h. But I am not sure if you actually want to keep this comment as reminder for future work. Should I keep it? Good question. I think I’d keep it and the block.h include; I mean, the throttle-groups.h include was there before already, so perhaps this was indeed only intended as a reminder. The other reason to keep it is that ideal this is just a refactoring patch, so I wouldn’t touch anything that needn’t be touched. Hanna
Re: [PATCH v4 04/25] include/sysemu/block-backend: split header into I/O and global state (GS) API
On 12/11/2021 13:30, Hanna Reitz wrote: On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote: Similarly to the previous patches, split block-backend.h in block-backend-io.h and block-backend-global-state.h In addition, remove "block/block.h" include as it seems it is not necessary anymore, together with "qemu/iov.h" block-backend-common.h contains the structures shared between the two headers, and the functions that can't be categorized as I/O or global state. Assertions are added in the next patch. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block/block-backend.c | 9 +- include/sysemu/block-backend-common.h | 74 ++ include/sysemu/block-backend-global-state.h | 122 + include/sysemu/block-backend-io.h | 139 ++ include/sysemu/block-backend.h | 269 +--- 5 files changed, 344 insertions(+), 269 deletions(-) create mode 100644 include/sysemu/block-backend-common.h create mode 100644 include/sysemu/block-backend-global-state.h create mode 100644 include/sysemu/block-backend-io.h [...] diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index e5e1524f06..038be9fc40 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -13,272 +13,9 @@ #ifndef BLOCK_BACKEND_H #define BLOCK_BACKEND_H -#include "qemu/iov.h" -#include "block/throttle-groups.h" +#include "block-backend-global-state.h" +#include "block-backend-io.h" -/* - * TODO Have to include block/block.h for a bunch of block layer - * types. Unfortunately, this pulls in the whole BlockDriverState - * API, which we don't want used by many BlockBackend users. Some of - * the types belong here, and the rest should be split into a common - * header and one for the BlockDriverState API. - */ -#include "block/block.h" This note and the include is gone. Sounds like something positive, but why is this possible? Basically block/throttle-groups.h includes block/block_int.h that internally includes block/block.h. But I am not sure if you actually want to keep this comment as reminder for future work. Should I keep it? Thank you, Emanuele
Re: [PATCH v4 04/25] include/sysemu/block-backend: split header into I/O and global state (GS) API
+int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags);
+int64_t blk_nb_sectors(BlockBackend *blk);
I’d have considered this an I/O function, and blk_getlength() is
classified as such. Why not this?
This one by itself is only invoked under BQL. I believe in facts that in
migration/block.c is always wrapped by qemu_mutex_{lock/unlock}_iothread()
pairs.
But on the other side, as you said, semantically maybe it makes more
sense to put it as I/O. Also its bdrv_ counterpart, bdrv_nb_sectors, is
an I/O so you are right.
+int blk_probe_geometry(BlockBackend *blk, HDGeometry *geo);
+BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
+ BlockCompletionFunc *cb,
+ void *opaque, int ret);
This sounds more like an I/O function to me.
Semantically might make sense as an I/O. Functionally I don't see any
iothread using it.
I agree with the rest of the comments.
Thank you,
Emanuele
Re: [PATCH v4 04/25] include/sysemu/block-backend: split header into I/O and global state (GS) API
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote: Similarly to the previous patches, split block-backend.h in block-backend-io.h and block-backend-global-state.h In addition, remove "block/block.h" include as it seems it is not necessary anymore, together with "qemu/iov.h" block-backend-common.h contains the structures shared between the two headers, and the functions that can't be categorized as I/O or global state. Assertions are added in the next patch. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block/block-backend.c | 9 +- include/sysemu/block-backend-common.h | 74 ++ include/sysemu/block-backend-global-state.h | 122 + include/sysemu/block-backend-io.h | 139 ++ include/sysemu/block-backend.h | 269 +--- 5 files changed, 344 insertions(+), 269 deletions(-) create mode 100644 include/sysemu/block-backend-common.h create mode 100644 include/sysemu/block-backend-global-state.h create mode 100644 include/sysemu/block-backend-io.h [...] diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index e5e1524f06..038be9fc40 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -13,272 +13,9 @@ #ifndef BLOCK_BACKEND_H #define BLOCK_BACKEND_H -#include "qemu/iov.h" -#include "block/throttle-groups.h" +#include "block-backend-global-state.h" +#include "block-backend-io.h" -/* - * TODO Have to include block/block.h for a bunch of block layer - * types. Unfortunately, this pulls in the whole BlockDriverState - * API, which we don't want used by many BlockBackend users. Some of - * the types belong here, and the rest should be split into a common - * header and one for the BlockDriverState API. - */ -#include "block/block.h" This note and the include is gone. Sounds like something positive, but why is this possible? Hanna
Re: [PATCH v4 04/25] include/sysemu/block-backend: split header into I/O and global state (GS) API
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
Similarly to the previous patches, split block-backend.h
in block-backend-io.h and block-backend-global-state.h
In addition, remove "block/block.h" include as it seems
it is not necessary anymore, together with "qemu/iov.h"
block-backend-common.h contains the structures shared between
the two headers, and the functions that can't be categorized as
I/O or global state.
Assertions are added in the next patch.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
block/block-backend.c | 9 +-
include/sysemu/block-backend-common.h | 74 ++
include/sysemu/block-backend-global-state.h | 122 +
include/sysemu/block-backend-io.h | 139 ++
include/sysemu/block-backend.h | 269 +---
5 files changed, 344 insertions(+), 269 deletions(-)
create mode 100644 include/sysemu/block-backend-common.h
create mode 100644 include/sysemu/block-backend-global-state.h
create mode 100644 include/sysemu/block-backend-io.h
diff --git a/block/block-backend.c b/block/block-backend.c
index 39cd99df2b..0afc03fd66 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -79,6 +79,7 @@ struct BlockBackend {
bool allow_aio_context_change;
bool allow_write_beyond_eof;
+/* Protected by BQL lock */
NotifierList remove_bs_notifiers, insert_bs_notifiers;
QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;
@@ -111,12 +112,14 @@ static const AIOCBInfo block_backend_aiocb_info = {
static void drive_info_del(DriveInfo *dinfo);
static BlockBackend *bdrv_first_blk(BlockDriverState *bs);
-/* All BlockBackends */
+/* All BlockBackends. Protected by BQL lock. */
static QTAILQ_HEAD(, BlockBackend) block_backends =
QTAILQ_HEAD_INITIALIZER(block_backends);
-/* All BlockBackends referenced by the monitor and which are iterated through by
- * blk_next() */
+/*
+ * All BlockBackends referenced by the monitor and which are iterated through
by
+ * blk_next(). Protected by BQL lock.
+ */
static QTAILQ_HEAD(, BlockBackend) monitor_block_backends =
QTAILQ_HEAD_INITIALIZER(monitor_block_backends);
diff --git a/include/sysemu/block-backend-common.h b/include/sysemu/block-backend-common.h
new file mode 100644
index 00..52ff6a4d26
--- /dev/null
+++ b/include/sysemu/block-backend-common.h
@@ -0,0 +1,74 @@
+/*
+ * QEMU Block backends
+ *
+ * Copyright (C) 2014-2016 Red Hat, Inc.
+ *
+ * Authors:
+ * Markus Armbruster ,
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1
+ * or later. See the COPYING.LIB file in the top-level directory.
+ */
+
+#ifndef BLOCK_BACKEND_COMMON_H
+#define BLOCK_BACKEND_COMMON_H
+
+#include "block/throttle-groups.h"
+
+/* Callbacks for block device models */
+typedef struct BlockDevOps {
+/*
+ * Runs when virtual media changed (monitor commands eject, change)
+ * Argument load is true on load and false on eject.
+ * Beware: doesn't run when a host device's physical media
+ * changes. Sure would be useful if it did.
+ * Device models with removable media must implement this callback.
+ */
+void (*change_media_cb)(void *opaque, bool load, Error **errp);
+/*
+ * Runs when an eject request is issued from the monitor, the tray
+ * is closed, and the medium is locked.
+ * Device models that do not implement is_medium_locked will not need
+ * this callback. Device models that can lock the medium or tray might
+ * want to implement the callback and unlock the tray when "force" is
+ * true, even if they do not support eject requests.
+ */
+void (*eject_request_cb)(void *opaque, bool force);
+/*
+ * Is the virtual tray open?
+ * Device models implement this only when the device has a tray.
+ */
+bool (*is_tray_open)(void *opaque);
+/*
+ * Is the virtual medium locked into the device?
+ * Device models implement this only when device has such a lock.
+ */
+bool (*is_medium_locked)(void *opaque);
+/*
+ * Runs when the size changed (e.g. monitor command block_resize)
+ */
+void (*resize_cb)(void *opaque);
+/*
+ * Runs when the backend receives a drain request.
+ */
+void (*drained_begin)(void *opaque);
+/*
+ * Runs when the backend's last drain request ends.
+ */
+void (*drained_end)(void *opaque);
+/*
+ * Is the device still busy?
+ */
+bool (*drained_poll)(void *opaque);
+} BlockDevOps;
+
+/*
+ * This struct is embedded in (the private) BlockBackend struct and contains
+ * fields that must be public. This is in particular for QLIST_ENTRY() and
+ * friends so that BlockBackends can be kept in lists outside block-backend.c
+ */
+typedef struct BlockBackendPublic {
+ThrottleGroupMember throttle_group_member;
+} BlockBackendPublic;
+
+#endif /* BLOCK_BACKEND_COMMON_H */
diff --git
[PATCH v4 04/25] include/sysemu/block-backend: split header into I/O and global state (GS) API
Similarly to the previous patches, split block-backend.h
in block-backend-io.h and block-backend-global-state.h
In addition, remove "block/block.h" include as it seems
it is not necessary anymore, together with "qemu/iov.h"
block-backend-common.h contains the structures shared between
the two headers, and the functions that can't be categorized as
I/O or global state.
Assertions are added in the next patch.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
block/block-backend.c | 9 +-
include/sysemu/block-backend-common.h | 74 ++
include/sysemu/block-backend-global-state.h | 122 +
include/sysemu/block-backend-io.h | 139 ++
include/sysemu/block-backend.h | 269 +---
5 files changed, 344 insertions(+), 269 deletions(-)
create mode 100644 include/sysemu/block-backend-common.h
create mode 100644 include/sysemu/block-backend-global-state.h
create mode 100644 include/sysemu/block-backend-io.h
diff --git a/block/block-backend.c b/block/block-backend.c
index 39cd99df2b..0afc03fd66 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -79,6 +79,7 @@ struct BlockBackend {
bool allow_aio_context_change;
bool allow_write_beyond_eof;
+/* Protected by BQL lock */
NotifierList remove_bs_notifiers, insert_bs_notifiers;
QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;
@@ -111,12 +112,14 @@ static const AIOCBInfo block_backend_aiocb_info = {
static void drive_info_del(DriveInfo *dinfo);
static BlockBackend *bdrv_first_blk(BlockDriverState *bs);
-/* All BlockBackends */
+/* All BlockBackends. Protected by BQL lock. */
static QTAILQ_HEAD(, BlockBackend) block_backends =
QTAILQ_HEAD_INITIALIZER(block_backends);
-/* All BlockBackends referenced by the monitor and which are iterated through
by
- * blk_next() */
+/*
+ * All BlockBackends referenced by the monitor and which are iterated through
by
+ * blk_next(). Protected by BQL lock.
+ */
static QTAILQ_HEAD(, BlockBackend) monitor_block_backends =
QTAILQ_HEAD_INITIALIZER(monitor_block_backends);
diff --git a/include/sysemu/block-backend-common.h
b/include/sysemu/block-backend-common.h
new file mode 100644
index 00..52ff6a4d26
--- /dev/null
+++ b/include/sysemu/block-backend-common.h
@@ -0,0 +1,74 @@
+/*
+ * QEMU Block backends
+ *
+ * Copyright (C) 2014-2016 Red Hat, Inc.
+ *
+ * Authors:
+ * Markus Armbruster ,
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1
+ * or later. See the COPYING.LIB file in the top-level directory.
+ */
+
+#ifndef BLOCK_BACKEND_COMMON_H
+#define BLOCK_BACKEND_COMMON_H
+
+#include "block/throttle-groups.h"
+
+/* Callbacks for block device models */
+typedef struct BlockDevOps {
+/*
+ * Runs when virtual media changed (monitor commands eject, change)
+ * Argument load is true on load and false on eject.
+ * Beware: doesn't run when a host device's physical media
+ * changes. Sure would be useful if it did.
+ * Device models with removable media must implement this callback.
+ */
+void (*change_media_cb)(void *opaque, bool load, Error **errp);
+/*
+ * Runs when an eject request is issued from the monitor, the tray
+ * is closed, and the medium is locked.
+ * Device models that do not implement is_medium_locked will not need
+ * this callback. Device models that can lock the medium or tray might
+ * want to implement the callback and unlock the tray when "force" is
+ * true, even if they do not support eject requests.
+ */
+void (*eject_request_cb)(void *opaque, bool force);
+/*
+ * Is the virtual tray open?
+ * Device models implement this only when the device has a tray.
+ */
+bool (*is_tray_open)(void *opaque);
+/*
+ * Is the virtual medium locked into the device?
+ * Device models implement this only when device has such a lock.
+ */
+bool (*is_medium_locked)(void *opaque);
+/*
+ * Runs when the size changed (e.g. monitor command block_resize)
+ */
+void (*resize_cb)(void *opaque);
+/*
+ * Runs when the backend receives a drain request.
+ */
+void (*drained_begin)(void *opaque);
+/*
+ * Runs when the backend's last drain request ends.
+ */
+void (*drained_end)(void *opaque);
+/*
+ * Is the device still busy?
+ */
+bool (*drained_poll)(void *opaque);
+} BlockDevOps;
+
+/*
+ * This struct is embedded in (the private) BlockBackend struct and contains
+ * fields that must be public. This is in particular for QLIST_ENTRY() and
+ * friends so that BlockBackends can be kept in lists outside block-backend.c
+ */
+typedef struct BlockBackendPublic {
+ThrottleGroupMember throttle_group_member;
+} BlockBackendPublic;
+
+#endif /* BLOCK_BACKEND_COMMON_H */
diff --git a/include/sysemu/block-backend-global-state.h
b/include/sysemu/block-backend-global-state.h
