Re: [PATCH v3 02/25] include/block/block: split header into I/O and global state API
On Tue, Oct 12, 2021 at 04:48:43AM -0400, 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 > instead 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 > --- > block.c| 3 + > block/meson.build | 7 +- > include/block/block-common.h | 389 + > include/block/block-global-state.h | 263 + > include/block/block-io.h | 283 ++ > include/block/block.h | 863 + > 6 files changed, 947 insertions(+), 861 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 Modulo Eric's comments: Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH v3 02/25] include/block/block: split header into I/O and global state API
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 As a new file, it probably deserves a copyright/license blurb copied from the file it is split out of. diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h new file mode 100644 index 00..b57e275da9 --- /dev/null +++ b/include/block/block-global-state.h @@ -0,0 +1,263 @@ +#ifndef BLOCK_GLOBAL_STATE_H +#define BLOCK_GLOBAL_STATE_H Likewise, here and in all other newly-split files in your series. In general, as you might have seen, I kept the same copyright/license from the original file I split. But block.h seems to be the only header with no license. +++ b/include/block/block.h @@ -1,864 +1,9 @@ #ifndef BLOCK_H #define BLOCK_H Oh. There wasn't one to copy from :( Well, now's as good a time to fix that as any. So now the question is which one to use, because I see 2 different types of copyrights templates: - long version copyright, used in block_int.h, blockjob_int.h and many others /* * QEMU System Emulator block driver * * Copyright (c) 2003 Fabrice Bellard * * Permission is hereby granted, free of charge, to any person obtaining a copy [...] * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR [...] */ - short version, used in block-backend.h and many others /* * QEMU Block backends * * Copyright (C) 2014-2016 Red Hat, Inc. * * Authors: * * * 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. */ Maybe since we are talking about block.h we should stick to the same format as block_int.h? I am not sure though. Thank you, Emanuele
Re: [PATCH v3 02/25] include/block/block: split header into I/O and global state API
On Tue, Oct 12, 2021 at 04:48:43AM -0400, 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 > instead contains common structures shared by both headers. s/instead // > > 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 > --- > 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 As a new file, it probably deserves a copyright/license blurb copied from the file it is split out of. > diff --git a/include/block/block-global-state.h > b/include/block/block-global-state.h > new file mode 100644 > index 00..b57e275da9 > --- /dev/null > +++ b/include/block/block-global-state.h > @@ -0,0 +1,263 @@ > +#ifndef BLOCK_GLOBAL_STATE_H > +#define BLOCK_GLOBAL_STATE_H Likewise, here and in all other newly-split files in your series. > +++ b/include/block/block.h > @@ -1,864 +1,9 @@ > #ifndef BLOCK_H > #define BLOCK_H Oh. There wasn't one to copy from :( Well, now's as good a time to fix that as any. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[PATCH v3 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
instead 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
---
block.c| 3 +
block/meson.build | 7 +-
include/block/block-common.h | 389 +
include/block/block-global-state.h | 263 +
include/block/block-io.h | 283 ++
include/block/block.h | 863 +
6 files changed, 947 insertions(+), 861 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 66ee11e62c..41ac0eeb07 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= 0x10,
+BDRV_REQ_WRITE_C
