Re: [PATCH v4 03/25] assertions for block global state API
On 15.11.21 13:27, Emanuele Giuseppe Esposito wrote:
@@ -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.)
The problem as you saw is on the path callers: bdrv_drain seems to be
called only in main thread, while others or similar drains are also
coming from I/O. I would say maybe it's a better idea to just put
everything I/O, maybe (probably) there are cases not caught by the tests.
The only ones in global-state will be:
- bdrv_apply_subtree_drain and bdrv_unapply_subtree_drain (called only
in .attach and .detach callbacks of BdrvChildClass, run under BQL).
- bdrv_drain_all_end_quiesce (called only by bdrv_close thus under BQL).
- bdrv_drain_all_begin, bdrv_drain_all_end and bdrv_drain_all (have
already BQL assertions)
In general, this is the underlying problem with these assertions: if a
test doesn't test a specific code path, an unneeded assertion might
pass undetected.
I believe the I/O vs. GS classification should not only be done based on
functional concerns, i.e. who calls this function (is it called by an
I/O function?) and whether it can be thread-safe or not (does it call a
BQL function?), but also needs to be done semantically. I believe there
are some functions that we consider thread-safe but are also only called
by BQL functions, for example apparently bdrv_drain(),
bdrv_apply_subtree_drain(), and bdrv_unapply_subtree_drain(). Such
functions can functionally be considered both GS and I/O functions, so
the decision should be done semantically. I believe that it makes sense
to say all drain-related functions for a single subtree are I/O functions.
OTOH, functions operating on multiple trees in the block graph (i.e. all
functions that have some “_all_” in their name) should naturally be
considered GS functions, regardless of whether their implementation is
thread-safe or not, because they are by nature global functions.
That’s why I’m wondering why some drain functions are I/O and others are
GS; or why this block-status function is I/O and this one is GS. It may
make sense functionally, but semantically it’s strange.
I understand it may be difficult for you to know which functions relate
to each other and thus know these semantic relationships, but in most of
the series the split seems very reasonable to me, semantically. If it
didn’t, I said so. :)
Hanna
Re: [PATCH v4 03/25] assertions for block global state API
@@ -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.
I think that the assertion is wrong here. As long as a single caller is
not under BQL, we cannot keep the function in global-state headers.
It should go into block-io.h, together with bdrv_get_parent_name and
bdrv_get_device_or_node_name (caller of bdrv_get_parent_name).
Since bdrv_get_parent_name only scans through the list of bs->parents,
as long as we can assert that the parent list is modified only under BQL
+ drain, it is safe to be read and can be I/O.
[...]
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?
Right this is clearly called in bdrv_drained_end. I don't know how is it
possible that no test managed to trigger this assertion... This is
actually invoked in both unit and qemu-iothread tests...
@@ -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.)
The problem as you saw is on the path callers: bdrv_drain seems to be
called only in main thread, while others or similar drains are also
coming from I/O. I would say maybe it's a better idea to just put
everything I/O, maybe (probably) there are cases not caught by the tests.
The only ones in global-state will be:
- bdrv_apply_subtree_drain and bdrv_unapply_subtree_drain (called only
in .attach and .detach callbacks of BdrvChildClass, run under BQL).
- bdrv_drain_all_end_quiesce (called only by bdrv_close thus under BQL).
- bdrv_drain_all_begin, bdrv_drain_all_end and bdrv_drain_all (have
already BQL assertions)
In general, this is the underlying problem with these assertions: if a
test doesn't test a specific code path, an unneeded assertion might pass
undetected.
[...]
@@ -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?
This function is in block.io but has the assertion. I think it's a
proper I/O but I forgot to remove the assertion in my various trial
Let me know if you disagree on anything of what I said.
Thank you,
Emanuele
Re: [PATCH v4 03/25] assertions for block global state API
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 Reviewed-by: Stefan Hajnoczi --- block.c| 136 +++-- block/commit.c | 2 + block/io.c | 20 blockdev.c | 1 + 4 files changed, 156 insertions(+), 3 deletions(-) bdrv_make_zero() seems missing here – it can be considered an I/O or a GS function, but patch 2 classified it as GS. Hanna
Re: [PATCH v4 03/25] assertions for block global state API
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
Reviewed-by: Stefan Hajnoczi
---
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,
);
This hunk too seems unrelated.
Hanna
