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= 0x1