On 17.11.21 14:09, Emanuele Giuseppe Esposito wrote:
On 17/11/2021 13:51, Hanna Reitz wrote:
On 17.11.21 12:33, Emanuele Giuseppe Esposito wrote:
On 15/11/2021 13:48, Hanna Reitz wrote:
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
Signed-off-by: Emanuele Giuseppe Esposito <eespo...@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
---
block.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/block.c b/block.c
index 94bff5c757..40c4729b8d 100644
--- a/block.c
+++ b/block.c
[...]
@@ -2148,6 +2152,7 @@ static void bdrv_child_perm(BlockDriverState
*bs, BlockDriverState *child_bs,
uint64_t *nperm, uint64_t *nshared)
{
assert(bs->drv && bs->drv->bdrv_child_perm);
+ assert(qemu_in_main_thread());
bs->drv->bdrv_child_perm(bs, c, role, reopen_queue,
parent_perm, parent_shared,
nperm, nshared);
(Should’ve noticed earlier, but only did now...)
First, this function is indirectly called by bdrv_refresh_perms().
I understand that all perm-related functions are classified as GS.
However, bdrv_co_invalidate_cache() invokes bdrv_refresh_perms.
Being declared in block/coroutine.h, it’s an I/O function, so it
mustn’t call such a GS function.
BlockDriver.bdrv_co_invalidate_cache(), bdrv_invalidate_cache(),
and blk_invalidate_cache() are also classified as I/O functions.
Perhaps all of these functions should be classified as GS
functions? I believe their callers and their purpose would allow
for this.
I think that the *_invalidate_cache functions are I/O.
First of all, test-block-iothread.c calls bdrv_invalidate_cache in
test_sync_op_invalidate_cache, which is purposefully called in an
iothread. So that hints that we want it as I/O.
Hm, OK, but bdrv_co_invalidate_cache() calls bdrv_refresh_perms(),
which is a GS function, so that shouldn’t work, right?
Ok let's take a step back for one moment: can you tell me why the perm
functions should be GS?
On one side I see they are also used by I/O, as we can see above. On
the other side, I kinda see that permission should only be modified
under BQL. But I don't have any valid point to sustain that.
So I wonder if you have any specific and more valid reason to put them
as GS.
First I believe permissions to be part of the block graph state, and so
global state. But, well, that could be declared just a hunch.
Second permissions have transaction mechanisms – you try to update them
on every node, if one fails, all are aborted, else all are committed.
So this is by no means an atomic operation but quite drawn out.
The problem with this is that I/O operations rely on permissions, e.g.
you’ll get assertion failures when trying to write but don’t have the
WRITE permission. So it definitely doesn’t seem like something to me
that can be thread-safe in the sense of cooperating nicely with other
I/O threads.
Perhaps it’d be fine to do permission updates while the relevant
subgraph is drained (i.e. blocking all other I/O threads), but I kind of
feel like the same could be said for all (other) GS operations. Like,
you could probably do all kinds of graph changes while all involved
subgraphs are drained.
Hanna
