qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 20/25] block_int-common.h: assertion in the callers of Blo


From: Hanna Reitz
Subject: Re: [PATCH v4 20/25] block_int-common.h: assertion in the callers of BlockDriver function pointers
Date: Mon, 15 Nov 2021 15:15:36 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0

On 15.11.21 13:48, Hanna Reitz wrote:
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@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.


Second, it’s called by bdrv_child_refresh_perms(), which is called by block_crypto_amend_options_generic_luks().  This function is called by block_crypto_co_amend_luks(), which is a BlockDriver.bdrv_co_amend implementation, which is classified as an I/O function.

Honestly, I don’t know how to fix that mess.  The best would be if we could make the perm functions thread-safe and classify them as I/O, but it seems to me like that’s impossible (I sure hope I’m wrong).  On the other hand, .bdrv_co_amend very much strikes me like a GS function, but it isn’t.  I’m afraid it must work on nodes that are not in the main context, and it launches a job, so AFAIU we absolutely cannot run it under the BQL.

It almost seems to me like we’d need a thread-safe variant of the perm functions that’s allowed to fail when it cannot guarantee thread safety or something.  Or perhaps I’m wrong and the perm functions can actually be classified as thread-safe and I/O, that’d be great…

Hm.  Can we perhaps let block_crypto_amend_options_generic_luks() take the BQL just for the permission adjustment (i.e. the bdrv_child_refresh_perms() call)?  Would that work? :/

Hanna




reply via email to

[Prev in Thread] Current Thread [Next in Thread]