On Mon, Oct 25, 2021 at 04:09:41PM +0200, Philippe Mathieu-Daudé wrote:
> On 10/25/21 12:17, Emanuele Giuseppe Esposito wrote:
> [...]
> 
> > Each function in the GS API will have an assertion, checking
> > that it is always running under BQL.
> > I/O functions are instead thread safe (or so should be), meaning
> > that they *can* run under BQL, but also in an iothread in another
> > AioContext. Therefore they do not provide any assertion, and
> > need to be audited manually to verify the correctness.
> > 
> > Adding assetions has helped finding 2 bugs already, as shown in
> > my series "Migration: fix missing iothread locking".
> > 
> > Tested this series by running unit tests, qemu-iotests and qtests
> > (x86_64).
> > Some functions in the GS API are used everywhere but not
> > properly tested. Therefore their assertion is never actually run in
> > the tests, so despite my very careful auditing, it is not impossible
> > to exclude that some will trigger while actually using QEMU.
> > 
> > Patch 1 introduces qemu_in_main_thread(), the function used in
> > all assertions. This had to be introduced otherwise all unit tests
> > would fail, since they run in the main loop but use the code in
> > stubs/iothread.c
> > Patches 2-14 and 19-25 (with the exception of patch 9, that is an additional
> > assert) are all structured in the same way: first we split the header
> > and in the next (even) patch we add assertions.
> > The rest of the patches ontain either both assertions and split,
> > or have no assertions.
> 
> This seems a lot of assertions added in hot-path code.
> 
> Does it makes sense to use a BLOCK_ASSERT() macro instead,
> only expanded when configure with --enable-debug?

I think the assertions are only in the slow path (functions that must be
run with the BQL held from the main thread). The I/O request code path
does not have new assertions.

Stefan