mbox series

[v3,00/11] block: Convert qmp_query_block into a coroutine

Message ID 20240409145917.6780-1-farosas@suse.de
Headers show
Series block: Convert qmp_query_block into a coroutine | expand

Message

Fabiano Rosas April 9, 2024, 2:59 p.m. UTC 
Hi, it's been a while since the last version, so a recap:

This series converts qmp_query_block() & qmp_query_named_block_nodes()
to coroutines so we can yield from them all the way back into the main
loop. This addresses a vcpu softlockup encountered when querying a
disk placed on NFS.

If the NFS server happens to have high latency, an fstat() issued from
raw_co_get_allocated_file_size() could take seconds while the whole
QMP command is holding the BQL and blocks a vcpu thread going out of
the guest to handle IO.

This scenario is clearly undesireable since a query command is of much
lower priority than the vcpu thread doing actual work.

Move the 'fstat' call into the thread-pool and make the necessary
adaptations to ensure the whole QMP command that calls
raw_co_get_allocated_file_size() runs in a coroutine.

Changes since v2:

- Do the changes more gradually to make it easier to reason about the
  safety of the change.

- Patch 4 addresses the issue I asked about recently on the ml [1]
  about how to avoid dispatching the QMP command during an aio_poll().

- Converted qmp_query_block and qmp_query_named_block_nodes in a
  single patch to avoid having hmp_info_block call a coroutine_fn out
  of coroutine context.

On v2, Hanna asked:

  "I wonder how the threading is actually supposed to work.  I assume
  QMP coroutines run in the main thread, so now we run
  bdrv_co_get_allocated_file_size() in the main thread – is that
  correct, or do we need to use bdrv_co_enter() like qmp_block_resize()
  does?  And so, if we run it in the main thread, is it OK not to
  acquire the AioContext around it to prevent interference from a
  potential I/O thread?"

The QMP coroutines and also bdrv_co_get_allocated_file_size() run in
the main thread. This series doesn't change that. The difference is
that instead of bdrv_co_get_allocated_file_size() yielding back to
bdrv_poll(), it now yields back to the main loop.

As for thread safety, that's basically what I asked about in [1], so
I'm still gathering information and don't have a definite answer for
it. Since we don't have the AioContext lock anymore, it seems that
safety is now dependant on not dispatching the QMP command while other
operations are ongoing.

Still, for this particular case of fstat(), I don't think interference
of an I/O thread could cause any problems, as long as the file
descriptor is not closed prematurely. The fstat() manual already
mentions that it is succeptible to return old information in some
cases.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1244905208

1- Advice on block QMP command coroutines
https://lore.kernel.org/r/87bk6trl9i.fsf@suse.de

Initial discussion:
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03141.html
v1:
https://lore.kernel.org/r/20230523213903.18418-1-farosas@suse.de
v2:
https://lore.kernel.org/r/20230609201910.12100-1-farosas@suse.de

Fabiano Rosas (9):
  block: Allow the wrapper script to see functions declared in qapi.h
  block: Temporarily mark bdrv_co_get_allocated_file_size as mixed
  block: Take the graph lock in bdrv_snapshot_list
  block: Reschedule query-block during qcow2 invalidation
  block: Run bdrv_do_query_node_info in a coroutine
  block: Convert bdrv_query_block_graph_info to coroutine
  block: Convert bdrv_query_image_info to coroutine
  block: Convert bdrv_block_device_info into co_wrapper
  block: Don't query all block devices at hmp_nbd_server_start

João Silva (1):
  block: Add a thread-pool version of fstat

Lin Ma (1):
  block: Convert qmp_query_block and qmp_query_named_block_nodes to
    coroutine

 block.c                            |  9 +++--
 block/file-posix.c                 | 40 +++++++++++++++++--
 block/meson.build                  |  1 +
 block/mirror.c                     |  1 +
 block/monitor/block-hmp-cmds.c     | 34 +++++++++++-----
 block/qapi.c                       | 63 +++++++++++++++---------------
 block/qcow2.c                      | 20 ++++++++++
 block/replication.c                |  1 +
 block/snapshot.c                   |  2 +-
 blockdev.c                         |  8 ++--
 blockjob.c                         |  1 +
 hmp-commands-info.hx               |  1 +
 include/block/block-common.h       |  1 +
 include/block/block-global-state.h |  3 +-
 include/block/block-hmp-cmds.h     |  2 +-
 include/block/qapi.h               | 24 ++++++++----
 include/block/raw-aio.h            |  4 +-
 migration/block.c                  |  1 +
 qapi/block-core.json               |  5 ++-
 qemu-img.c                         |  3 --
 scripts/block-coroutine-wrapper.py |  1 +
 21 files changed, 157 insertions(+), 68 deletions(-)