On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
All the global state (GS) API functions will check that
qemu_in_main_thread() returns true. If not, it means
that the safety of BQL cannot be guaranteed, and
they need to be moved to I/O.
Signed-off-by: Emanuele Giuseppe Esposito <eespo...@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
---
block.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++--
block/commit.c | 2 +
block/io.c | 20 ++++++++
blockdev.c | 1 +
4 files changed, 156 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 6fdb4d7712..672f946065 100644
--- a/block.c
+++ b/block.c
[...]
@@ -5606,7 +5678,6 @@ int64_t bdrv_getlength(BlockDriverState *bs)
void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
{
int64_t nb_sectors = bdrv_nb_sectors(bs);
-
*nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
}
This hunk seems at least unrelated.
[...]
@@ -5958,6 +6043,7 @@ const char *bdrv_get_parent_name(const BlockDriverState
*bs)
/* TODO check what callers really want: bs->node_name or blk_name() */
const char *bdrv_get_device_name(const BlockDriverState *bs)
{
+ assert(qemu_in_main_thread());
return bdrv_get_parent_name(bs) ?: "";
}
This function is invoked from qcow2_signal_corruption(), which comes
generally from an I/O path. Is it safe to assert that we’re in the main
thread here?
Well, the question is probably rather whether this needs really be a
considered a global-state function, or whether putting it in common or
I/O is fine. I believe you’re right given that it invokes
bdrv_get_parent_name(), it cannot be thread-safe, but then we’ll have to
change qcow2_signal_corruption() so it doesn’t invoke this function.
[...]
diff --git a/block/io.c b/block/io.c
index bb0a254def..c5d7f8495e 100644
--- a/block/io.c
+++ b/block/io.c
[...]
@@ -544,6 +546,7 @@ void bdrv_drained_end(BlockDriverState *bs)
void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter)
{
+ assert(qemu_in_main_thread());
bdrv_do_drained_end(bs, false, NULL, false, drained_end_counter);
}
Why is bdrv_drained_end an I/O function and this is a GS function, even
though it does just a subset?
@@ -586,12 +589,14 @@ void bdrv_unapply_subtree_drain(BdrvChild *child,
BlockDriverState *old_parent)
void coroutine_fn bdrv_co_drain(BlockDriverState *bs)
{
assert(qemu_in_coroutine());
+ assert(qemu_in_main_thread());
bdrv_drained_begin(bs);
bdrv_drained_end(bs);
}
void bdrv_drain(BlockDriverState *bs)
{
+ assert(qemu_in_main_thread());
bdrv_drained_begin(bs);
bdrv_drained_end(bs);
}
Why are these GS functions when both bdrv_drained_begin() and
bdrv_drained_end() are I/O functions?
I can understand making the drain_all functions GS functions, but it
seems weird to say it’s an I/O function when a single BDS is drained via
bdrv_drained_begin() and bdrv_drained_end(), but not via bdrv_drain(),
which just does both.
(I can see that there are no I/O path callers, but I still find it strange.)
[...]
@@ -2731,6 +2742,7 @@ int bdrv_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes,
int64_t *pnum, int64_t *map, BlockDriverState **file)
{
+ assert(qemu_in_main_thread());
return bdrv_block_status_above(bs, bdrv_filter_or_cow_bs(bs),
offset, bytes, pnum, map, file);
}
Why is this a GS function as opposed to all other block-status
functions? Because of the bdrv_filter_or_cow_bs() call?
And isn’t the call from nvme_block_status_all() basically an I/O path?
(Or is that always run in the main thread?)
@@ -2800,6 +2812,7 @@ int bdrv_is_allocated_above(BlockDriverState *top,
int64_t bytes, int64_t *pnum)
{
int depth;
+
int ret = bdrv_common_block_status_above(top, base, include_base, false,
offset, bytes, pnum, NULL, NULL,
&depth);
This hunk too seems unrelated.
Hanna
