Re: [PATCH v4 02/25] include/block/block: split header into I/O and global state API
On 12/11/2021 13:25, Hanna Reitz wrote:
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
block.h currently contains a mix of functions:
some of them run under the BQL and modify the block layer graph,
others are instead thread-safe and perform I/O in iothreads.
It is not easy to understand which function is part of which
group (I/O vs GS), and this patch aims to clarify it.
The "GS" functions need the BQL, and often use
aio_context_acquire/release and/or drain to be sure they
can modify the graph safely.
The I/O function are instead thread safe, and can run in
any AioContext.
By splitting the header in two files, block-io.h
and block-global-state.h we have a clearer view on what
needs what kind of protection. block-common.h
contains common structures shared by both headers.
block.h is left there for legacy and to avoid changing
all includes in all c files that use the block APIs.
Assertions are added in the next patch.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
block.c | 3 +
block/meson.build | 7 +-
include/block/block-common.h | 389 +
include/block/block-global-state.h | 286 ++
include/block/block-io.h | 306 ++
include/block/block.h | 878 +
6 files changed, 1012 insertions(+), 857 deletions(-)
create mode 100644 include/block/block-common.h
create mode 100644 include/block/block-global-state.h
create mode 100644 include/block/block-io.h
[...]
diff --git a/include/block/block-common.h b/include/block/block-common.h
new file mode 100644
index 00..4f1fd8de21
--- /dev/null
+++ b/include/block/block-common.h
[...]
+#define BLKDBG_EVENT(child, evt) \
+ do { \
+ if (child) { \
+ bdrv_debug_event(child->bs, evt); \
+ } \
+ } while (0)
This is defined twice, once here, and...
This is unnecessary, as bdrv_debug_event is in block-io.c
Will remove that.
diff --git a/include/block/block-io.h b/include/block/block-io.h
new file mode 100644
index 00..9af4609ccb
--- /dev/null
+++ b/include/block/block-io.h
[...]
+#define BLKDBG_EVENT(child, evt) \
+ do { \
+ if (child) { \
+ bdrv_debug_event(child->bs, evt); \
+ } \
+ } while (0)
...once here.
[...]
+/**
+ * bdrv_drained_begin:
+ *
+ * Begin a quiesced section for exclusive access to the BDS, by
disabling
+ * external request sources including NBD server and device model.
Note that
+ * this doesn't block timers or coroutines from submitting more
requests, which
+ * means block_job_pause is still necessary.
Where does this sentence come from? I can’t see it in master or in the
lines removed from block.h:
This is another mistake, I copied the old header. This sentence was
removed by this patch (sent by me) and then merged in master:
https://patchew.org/QEMU/20210903113800.59970-1-eespo...@redhat.com/
Will fix this and double check all headers so that the comments are
updated (but there shouldn't be any other mistakes).
Thank you,
Emanuele
Re: [PATCH v4 02/25] include/block/block: split header into I/O and global state API
+/* + * I/O API functions. These functions are thread-safe, and therefore + * can run in any thread as long as the thread has called + * aio_context_acquire/release(). + */ + +int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, + Error **errp); Why is this function here? Naïvely, I would’ve assumed as a graph-modifying function it should be in block-global-state.h. I mean, perhaps it’s thread-safe and then it can fit here, too. Still, it surprises me a bit to find this here. Hanna Agree, I also tested this, it can go in global state. Will fix that. Thank you, Emanuele
Re: [PATCH v4 02/25] include/block/block: split header into I/O and global state API
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
block.h currently contains a mix of functions:
some of them run under the BQL and modify the block layer graph,
others are instead thread-safe and perform I/O in iothreads.
It is not easy to understand which function is part of which
group (I/O vs GS), and this patch aims to clarify it.
The "GS" functions need the BQL, and often use
aio_context_acquire/release and/or drain to be sure they
can modify the graph safely.
The I/O function are instead thread safe, and can run in
any AioContext.
By splitting the header in two files, block-io.h
and block-global-state.h we have a clearer view on what
needs what kind of protection. block-common.h
contains common structures shared by both headers.
block.h is left there for legacy and to avoid changing
all includes in all c files that use the block APIs.
Assertions are added in the next patch.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
block.c| 3 +
block/meson.build | 7 +-
include/block/block-common.h | 389 +
include/block/block-global-state.h | 286 ++
include/block/block-io.h | 306 ++
include/block/block.h | 878 +
6 files changed, 1012 insertions(+), 857 deletions(-)
create mode 100644 include/block/block-common.h
create mode 100644 include/block/block-global-state.h
create mode 100644 include/block/block-io.h
[...]
diff --git a/include/block/block-common.h b/include/block/block-common.h
new file mode 100644
index 00..4f1fd8de21
--- /dev/null
+++ b/include/block/block-common.h
[...]
+#define BLKDBG_EVENT(child, evt) \
+do { \
+if (child) { \
+bdrv_debug_event(child->bs, evt); \
+} \
+} while (0)
This is defined twice, once here, and...
diff --git a/include/block/block-io.h b/include/block/block-io.h
new file mode 100644
index 00..9af4609ccb
--- /dev/null
+++ b/include/block/block-io.h
[...]
+#define BLKDBG_EVENT(child, evt) \
+do { \
+if (child) { \
+bdrv_debug_event(child->bs, evt); \
+} \
+} while (0)
...once here.
[...]
+/**
+ * bdrv_drained_begin:
+ *
+ * Begin a quiesced section for exclusive access to the BDS, by disabling
+ * external request sources including NBD server and device model. Note that
+ * this doesn't block timers or coroutines from submitting more requests, which
+ * means block_job_pause is still necessary.
Where does this sentence come from? I can’t see it in master or in the
lines removed from block.h:
+ *
+ * This function can be recursive.
+ */
+void bdrv_drained_begin(BlockDriverState *bs);
[...]
diff --git a/include/block/block.h b/include/block/block.h
index e5dd22b034..1e6b8fef1e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
[...]
-/**
- * bdrv_drained_begin:
- *
- * Begin a quiesced section for exclusive access to the BDS, by disabling
- * external request sources including NBD server, block jobs, and device model.
- *
- * This function can be recursive.
- */
-void bdrv_drained_begin(BlockDriverState *bs);
Re: [PATCH v4 02/25] include/block/block: split header into I/O and global state API
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote: block.h currently contains a mix of functions: some of them run under the BQL and modify the block layer graph, others are instead thread-safe and perform I/O in iothreads. It is not easy to understand which function is part of which group (I/O vs GS), and this patch aims to clarify it. The "GS" functions need the BQL, and often use aio_context_acquire/release and/or drain to be sure they can modify the graph safely. The I/O function are instead thread safe, and can run in any AioContext. By splitting the header in two files, block-io.h and block-global-state.h we have a clearer view on what needs what kind of protection. block-common.h contains common structures shared by both headers. block.h is left there for legacy and to avoid changing all includes in all c files that use the block APIs. Assertions are added in the next patch. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block.c| 3 + block/meson.build | 7 +- include/block/block-common.h | 389 + include/block/block-global-state.h | 286 ++ include/block/block-io.h | 306 ++ include/block/block.h | 878 + 6 files changed, 1012 insertions(+), 857 deletions(-) create mode 100644 include/block/block-common.h create mode 100644 include/block/block-global-state.h create mode 100644 include/block/block-io.h [...] diff --git a/include/block/block-io.h b/include/block/block-io.h new file mode 100644 index 00..9af4609ccb --- /dev/null +++ b/include/block/block-io.h [...] +/* + * I/O API functions. These functions are thread-safe, and therefore + * can run in any thread as long as the thread has called + * aio_context_acquire/release(). + */ + +int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, + Error **errp); Why is this function here? Naïvely, I would’ve assumed as a graph-modifying function it should be in block-global-state.h. I mean, perhaps it’s thread-safe and then it can fit here, too. Still, it surprises me a bit to find this here. Hanna
Re: [PATCH v4 02/25] include/block/block: split header into I/O and global state API
Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block.c| 3 + block/meson.build | 7 +- include/block/block-common.h | 389 + Can this patch be split in 3? (first) include/block/block-global-state.h | 286 ++ (second) include/block/block-io.h | 306 ++ (third) I think it is a good idea especially for future patches, since it improves readability. For this series I think it has already been fully reviewed, so it won't matter too much. But I will follow this logic for the upcoming job patches, thanks. include/block/block.h | 878 + 6 files changed, 1012 insertions(+), 857 deletions(-) create mode 100644 include/block/block-common.h create mode 100644 include/block/block-global-state.h create mode 100644 include/block/block-io.h Also consider enabling scripts/git.orderfile to ease patch review. Done, thanks for pointing it out. Emanuele
Re: [PATCH v4 02/25] include/block/block: split header into I/O and global state API
On 10/25/21 12:17, Emanuele Giuseppe Esposito wrote: > block.h currently contains a mix of functions: > some of them run under the BQL and modify the block layer graph, > others are instead thread-safe and perform I/O in iothreads. > It is not easy to understand which function is part of which > group (I/O vs GS), and this patch aims to clarify it. > > The "GS" functions need the BQL, and often use > aio_context_acquire/release and/or drain to be sure they > can modify the graph safely. > The I/O function are instead thread safe, and can run in > any AioContext. > > By splitting the header in two files, block-io.h > and block-global-state.h we have a clearer view on what > needs what kind of protection. block-common.h > contains common structures shared by both headers. > > block.h is left there for legacy and to avoid changing > all includes in all c files that use the block APIs. > > Assertions are added in the next patch. > > Signed-off-by: Emanuele Giuseppe Esposito > Reviewed-by: Stefan Hajnoczi > --- > block.c| 3 + > block/meson.build | 7 +- > include/block/block-common.h | 389 + Can this patch be split in 3? (first) > include/block/block-global-state.h | 286 ++ (second) > include/block/block-io.h | 306 ++ (third) > include/block/block.h | 878 + > 6 files changed, 1012 insertions(+), 857 deletions(-) > create mode 100644 include/block/block-common.h > create mode 100644 include/block/block-global-state.h > create mode 100644 include/block/block-io.h Also consider enabling scripts/git.orderfile to ease patch review.
[PATCH v4 02/25] include/block/block: split header into I/O and global state API
block.h currently contains a mix of functions:
some of them run under the BQL and modify the block layer graph,
others are instead thread-safe and perform I/O in iothreads.
It is not easy to understand which function is part of which
group (I/O vs GS), and this patch aims to clarify it.
The "GS" functions need the BQL, and often use
aio_context_acquire/release and/or drain to be sure they
can modify the graph safely.
The I/O function are instead thread safe, and can run in
any AioContext.
By splitting the header in two files, block-io.h
and block-global-state.h we have a clearer view on what
needs what kind of protection. block-common.h
contains common structures shared by both headers.
block.h is left there for legacy and to avoid changing
all includes in all c files that use the block APIs.
Assertions are added in the next patch.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
block.c| 3 +
block/meson.build | 7 +-
include/block/block-common.h | 389 +
include/block/block-global-state.h | 286 ++
include/block/block-io.h | 306 ++
include/block/block.h | 878 +
6 files changed, 1012 insertions(+), 857 deletions(-)
create mode 100644 include/block/block-common.h
create mode 100644 include/block/block-global-state.h
create mode 100644 include/block/block-io.h
diff --git a/block.c b/block.c
index 45f653a88b..6fdb4d7712 100644
--- a/block.c
+++ b/block.c
@@ -67,12 +67,15 @@
#define NOT_DONE 0x7fff /* used while emulated sync operation in progress
*/
+/* Protected by BQL */
static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
+/* Protected by BQL */
static QTAILQ_HEAD(, BlockDriverState) all_bdrv_states =
QTAILQ_HEAD_INITIALIZER(all_bdrv_states);
+/* Protected by BQL */
static QLIST_HEAD(, BlockDriver) bdrv_drivers =
QLIST_HEAD_INITIALIZER(bdrv_drivers);
diff --git a/block/meson.build b/block/meson.build
index deb73ca389..ee636e2754 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -113,8 +113,11 @@ block_ss.add(module_block_h)
wrapper_py = find_program('../scripts/block-coroutine-wrapper.py')
block_gen_c = custom_target('block-gen.c',
output: 'block-gen.c',
-input: files('../include/block/block.h',
- 'coroutines.h'),
+input: files(
+ '../include/block/block-io.h',
+ '../include/block/block-global-state.h',
+ 'coroutines.h'
+ ),
command: [wrapper_py, '@OUTPUT@', '@INPUT@'])
block_ss.add(block_gen_c)
diff --git a/include/block/block-common.h b/include/block/block-common.h
new file mode 100644
index 00..4f1fd8de21
--- /dev/null
+++ b/include/block/block-common.h
@@ -0,0 +1,389 @@
+#ifndef BLOCK_COMMON_H
+#define BLOCK_COMMON_H
+
+#include "block/aio.h"
+#include "block/aio-wait.h"
+#include "qemu/iov.h"
+#include "qemu/coroutine.h"
+#include "block/accounting.h"
+#include "block/dirty-bitmap.h"
+#include "block/blockjob.h"
+#include "qemu/hbitmap.h"
+#include "qemu/transactions.h"
+
+/*
+ * generated_co_wrapper
+ *
+ * Function specifier, which does nothing but mark functions to be
+ * generated by scripts/block-coroutine-wrapper.py
+ *
+ * Read more in docs/devel/block-coroutine-wrapper.rst
+ */
+#define generated_co_wrapper
+
+#define BLKDBG_EVENT(child, evt) \
+do { \
+if (child) { \
+bdrv_debug_event(child->bs, evt); \
+} \
+} while (0)
+
+/* block.c */
+typedef struct BlockDriver BlockDriver;
+typedef struct BdrvChild BdrvChild;
+typedef struct BdrvChildClass BdrvChildClass;
+
+typedef struct BlockDriverInfo {
+/* in bytes, 0 if irrelevant */
+int cluster_size;
+/* offset at which the VM state can be saved (0 if not possible) */
+int64_t vm_state_offset;
+bool is_dirty;
+/*
+ * True if this block driver only supports compressed writes
+ */
+bool needs_compressed_writes;
+} BlockDriverInfo;
+
+typedef struct BlockFragInfo {
+uint64_t allocated_clusters;
+uint64_t total_clusters;
+uint64_t fragmented_clusters;
+uint64_t compressed_clusters;
+} BlockFragInfo;
+
+typedef enum {
+BDRV_REQ_COPY_ON_READ = 0x1,
+BDRV_REQ_ZERO_WRITE = 0x2,
+
+/*
+ * The BDRV_REQ_MAY_UNMAP flag is used in write_zeroes requests to indicate
+ * that the block driver should unmap (discard) blocks if it is guaranteed
+ * that the result will read back as zeroes. The flag is only passed to the
+ * driver if the block device is opened with BDRV_O_UNMAP.
+ */
+BDRV_REQ_MAY_UNMAP = 0x4,
+
+BDRV_REQ_FUA=
