This serie aims to ensure necessary protection to the BlockDriverState
.children and .parents lists, modified in block.c:bdrv_replace_child_noperm().
Thanks to the assertion qemu_in_main_thread() introduced in "block layer:
split block APIs in global state and I/O", we can verify that these lists are
always modified under BQL, but are also read by I/O functions.
This means that alone the BQL is not enough, but that we need to add also
drains, to make sure that there is no I/O running in parallel.

Once we know that these fields are thread safe, we can continue doing until
the various Aiocontext lock that are taken and released to protect them can
be removed.
Currently the AioContext is used pretty much throughout the whole block layer,
and it is a little bit confusing to understand what it exactly protects, and I
am starting to think that in some places it is being taken just because of the
block API assumptions.
For example, some functions like AIO_WAIT_WHILE() release the lock with the
assumption that it is always held, so all callers must take it just to allow
the function to release it.

We can assert that some function is under drains and BQL by using
assert_bdrv_graph_writable(), introduced in the block API splitup
(patch 9 in v5), even though it is currently not checking for drains but only
for main loop. That series added the necessary asserts for .parents and
.children, but as explained we could not enable them, because drains
were missing.
This serie adds the necessary drains and patch 11 fully enables the assertion.

The main function that modifies the ->children and ->parent lists is bdrv_replace_child_noperm. So we need to protect that, and make sure it is
always under drain.

It is necessary to use subtree drains in this case, because it takes care of
draining the whole graph of the node, while bdrv_drained_* does not cover the
child of the given node. And bdrv_replace_child_noperm modifies also that.

Major improvements that this serie brings:
* BDRV_POLL_WHILE_UNLOCKED and bdrv_subtree_drained_begin/end_unlocked.
  This is helpful because current drains always assume that the AioContext
  lock is taken, and thus release it. We don't want to use Aiocontext at all.
* Fix/improve tests. Some tests perform what are now defined as illegal
  operations (I/O modifying a graph) or fail due to the increase of drains
  due to these patches. This brings possible bugs to the light, as shown in
  patches 2,3,4,5,6,8,9. Most of the patches try to fix all bugs that come
  out from adding such drains.
* Protect BlockDriverState's .children and .parents lists, making them thread
  safe.
* Finally fully enable assert_bdrv_graph_writable(), checking also for the
  drains, in patch 11.

This series is based on "job: replace AioContext lock with job_mutex" that in
turns is based on the block API split ("block layer: split block APIs in
global state and I/O").

Based-on: <20220105140208.365608-1-eesposit@redhat.com>

Emanuele Giuseppe Esposito (12):
  introduce BDRV_POLL_WHILE_UNLOCKED
  block/io.c: make bdrv_do_drained_begin_quiesce static and introduce
    bdrv_drained_begin_no_poll
  block.c: bdrv_replace_child_noperm: first remove the child, and then
    call ->detach()
  block.c: bdrv_replace_child_noperm: first call ->attach(), and then
    add child
  test-bdrv-drain.c: adapt test to the coming subtree drains
  test-bdrv-drain.c: remove test_detach_by_parent_cb()
  block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked
  reopen: add a transaction to drain_end nodes picked in
    bdrv_reopen_parse_file_or_backing
  jobs: ensure sleep in job_sleep_ns is fully performed
  block.c: add subtree_drains where needed
  block/io.c: fully enable assert_bdrv_graph_writable
  block.c: additional assert qemu in main tread

 block.c                            | 95 ++++++++++++++++++++++++------
 block/block-backend.c              |  3 +
 block/io.c                         | 62 +++++++++++++------
 include/block/block-global-state.h |  5 ++
 include/block/block-io.h           | 10 +++-
 job.c                              | 28 +++++----
 tests/qemu-iotests/030             |  2 +-
 tests/qemu-iotests/151             |  4 +-
 tests/unit/test-bdrv-drain.c       | 53 ++++++-----------
 tests/unit/test-blockjob.c         |  2 +-
 10 files changed, 174 insertions(+), 90 deletions(-)

-- 
2.31.1

On 1/18/22 17:27, Emanuele Giuseppe Esposito wrote:
> * Finally fully enable assert_bdrv_graph_writable(), checking also for the
>    drains, in patch 11.

Wow, this is definitely not just moving code around!  It's great that 
you could pull this off.  It gives me a lot more confidence that the 
locking model and the graph/IO split are correct, and also a lot more 
confidence about the logic to move the drains back and forth.

It does add a lot more complication to the API with all the _unlocked 
variants, but I think we're getting closer and closer to the endgame for 
the AioContext lock.

Paolo

If I understand correctly aio_context_acquire() calls are not explicitly
removed in this patch series but switching to BDRV_POLL_WHILE_UNLOCKED()
means that we no longer need to run under the AioContext lock?

Still, I'm a bit surprised I didn't notice any
aio_context_acquire/release() removals in this patch series because I
thought callers need that before they switch to
BDRV_POLL_WHILE_UNLOCKED()?

On 1/26/22 12:29, Stefan Hajnoczi wrote:
> Still, I'm a bit surprised I didn't notice any
> aio_context_acquire/release() removals in this patch series because I
> thought callers need that before they switch to
> BDRV_POLL_WHILE_UNLOCKED()?

I think the callers are new and were not calling 
bdrv_subtree_drained_begin() (and thus BDRV_POLL_WHILE) at all?

Emanuele, enlighten us. :)

Paolo

On 27/01/2022 14:46, Paolo Bonzini wrote:
> On 1/26/22 12:29, Stefan Hajnoczi wrote:
>> Still, I'm a bit surprised I didn't notice any
>> aio_context_acquire/release() removals in this patch series because I
>> thought callers need that before they switch to
>> BDRV_POLL_WHILE_UNLOCKED()?
> 
> I think the callers are new and were not calling
> bdrv_subtree_drained_begin() (and thus BDRV_POLL_WHILE) at all?
> 
> Emanuele, enlighten us. :)

Yes, the callers were not calling any kind of drains (or at least most
of them) *and* no context lock was acquired before calling them.

The current logic (or at least how I see it) when draining is:
"ok, we need to use bdrv_drain or bdrv_subtree_drain, but these function
call BDRV_POLL_WHILE(), which in turns calls AIO_WAIT_WHILE and thus
performs aio_context_release(lock); [...] aio_context_acquire(lock);
*Therefore* we need to acquire the lock". The lock is taken as a
consequence of the drain implementation.
This makes the lock usage useless, because we are just blindly acquiring
it for the purpose of making BDRV_POLL_WHILE happy.

On the other side, here no lock was acquired before, and
BDRV_POLL_WHILE_UNLOCKED is not releasing anything, thus no lock is needed.

This seems to hold and kinda proves my logic above.

Thank you,
Emanuele