[PATCH 00/19] block: fix coroutine_fn annotations

Paolo Bonzini posted 19 patches 2 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220415131900.793161-1-pbonzini@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, John Snow <jsnow@redhat.com>, Vladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>, Fam Zheng <fam@euphon.net>, Ronnie Sahlberg <ronniesahlberg@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Lieven <pl@kamp.de>, Eric Blake <eblake@redhat.com>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, "Denis V. Lunev" <den@openvz.org>, Alberto Garcia <berto@igalia.com>, Greg Kurz <groug@kaod.org>, Christian Schoenebeck <qemu_oss@crudebyte.com>, Juan Quintela <quintela@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>
There is a newer version of this series
block/blkdebug.c            | 14 +++++++-------
block/blkverify.c           |  2 +-
block/block-backend.c       | 26 +++++++++++++-------------
block/copy-before-write.c   |  8 ++++----
block/curl.c                |  2 +-
block/file-posix.c          |  2 +-
block/io.c                  | 24 ++++++++++++------------
block/iscsi.c               |  2 +-
block/nbd.c                 | 10 +++++-----
block/nfs.c                 |  2 +-
block/nvme.c                |  5 +++--
block/parallels.c           |  5 +++--
block/qcow2-cluster.c       | 18 +++++++++---------
block/qcow2-refcount.c      |  6 +++---
block/qcow2.c               |  4 ++--
block/qcow2.h               | 18 +++++++++---------
block/qed.c                 |  4 ++--
block/quorum.c              | 35 ++++++++++++++++++-----------------
block/raw-format.c          |  2 +-
block/throttle.c            |  2 +-
block/vmdk.c                | 20 ++++++++++----------
hw/9pfs/9p.h                |  9 ++++++---
include/block/nbd.h         |  2 +-
include/qemu/coroutine.h    |  2 +-
include/qemu/job.h          |  2 +-
job.c                       |  2 +-
migration/migration.c       |  3 ++-
tests/unit/test-coroutine.c |  2 +-
util/qemu-coroutine-lock.c  | 14 +++++++-------
util/qemu-coroutine.c       |  2 +-
30 files changed, 128 insertions(+), 121 deletions(-)
[PATCH 00/19] block: fix coroutine_fn annotations
Posted by Paolo Bonzini 2 years ago
This is the initial result of reviving Marc-Andr\ufffd\ufffd's series at
https://patchew.org/QEMU/20170704220346.29244-1-marcandre.lureau@redhat.com/.
A lot of the patches are similar to the ones that Marc-Andr\ufffd\ufffd wrote,
but due to the changes in the code it was easier to redo them.

For nbd, the patch is on top of "nbd: mark more coroutine_fns" that
I sent a few days ago and that (AIUI) Eric has already queued; only
one function was missing, much to my surprise.

Apart from this, I also identified the following functions that
can be called both in coroutine context and outside:

- qmp_dispatch
- schedule_next_request
- nvme_get_free_req
- bdrv_create
- bdrv_remove_persistent_dirty_bitmap
- bdrv_can_store_new_dirty_bitmap
- bdrv_do_drained_begin
- bdrv_do_drained_end
- bdrv_drain_all_begin
- qcow2_open
- qcow2_has_zero_init
- bdrv_qed_open
- qio_channel_readv_full_all_eof
- qio_channel_writev_full_all

besides, of course, everything that is generated by
scripts/block-coroutine-wrapper.py.

Thanks,

Paolo

Supersedes: <20170704220346.29244-1-marcandre.lureau@redhat.com>

Marc-Andr\ufffd\ufffd Lureau (3):
  9p: add missing coroutine_fn annotations
  migration: add missing coroutine_fn annotations
  test-coroutine: add missing coroutine_fn annotations

Paolo Bonzini (23):
  block: remove incorrect coroutine_fn annotations
  qcow2: remove incorrect coroutine_fn annotations
  nbd: remove incorrect coroutine_fn annotations
  coroutine: remove incorrect coroutine_fn annotations
  blkdebug: add missing coroutine_fn annotations
  blkverify: add missing coroutine_fn annotations
  block: add missing coroutine_fn annotations
  file-posix: add missing coroutine_fn annotations
  iscsi: add missing coroutine_fn annotations
  nbd: add missing coroutine_fn annotations
  nfs: add missing coroutine_fn annotations
  nvme: add missing coroutine_fn annotations
  parallels: add missing coroutine_fn annotations
  qcow2: add missing coroutine_fn annotations
  copy-before-write: add missing coroutine_fn annotations
  curl: add missing coroutine_fn annotations
  qed: add missing coroutine_fn annotations
  quorum: add missing coroutine_fn annotations
  throttle: add missing coroutine_fn annotations
  vmdk: add missing coroutine_fn annotations
  job: add missing coroutine_fn annotations
  coroutine-lock: add missing coroutine_fn annotations
  raw-format: add missing coroutine_fn annotations

 block/blkdebug.c            | 14 +++++++-------
 block/blkverify.c           |  2 +-
 block/block-backend.c       | 26 +++++++++++++-------------
 block/copy-before-write.c   |  8 ++++----
 block/curl.c                |  2 +-
 block/file-posix.c          |  2 +-
 block/io.c                  | 24 ++++++++++++------------
 block/iscsi.c               |  2 +-
 block/nbd.c                 | 10 +++++-----
 block/nfs.c                 |  2 +-
 block/nvme.c                |  5 +++--
 block/parallels.c           |  5 +++--
 block/qcow2-cluster.c       | 18 +++++++++---------
 block/qcow2-refcount.c      |  6 +++---
 block/qcow2.c               |  4 ++--
 block/qcow2.h               | 18 +++++++++---------
 block/qed.c                 |  4 ++--
 block/quorum.c              | 35 ++++++++++++++++++-----------------
 block/raw-format.c          |  2 +-
 block/throttle.c            |  2 +-
 block/vmdk.c                | 20 ++++++++++----------
 hw/9pfs/9p.h                |  9 ++++++---
 include/block/nbd.h         |  2 +-
 include/qemu/coroutine.h    |  2 +-
 include/qemu/job.h          |  2 +-
 job.c                       |  2 +-
 migration/migration.c       |  3 ++-
 tests/unit/test-coroutine.c |  2 +-
 util/qemu-coroutine-lock.c  | 14 +++++++-------
 util/qemu-coroutine.c       |  2 +-
 30 files changed, 128 insertions(+), 121 deletions(-)

-- 
2.35.1
Re: [PATCH 00/19] block: fix coroutine_fn annotations
Posted by Stefan Hajnoczi 2 years ago
On Fri, Apr 15, 2022 at 03:18:34PM +0200, Paolo Bonzini wrote:
> This is the initial result of reviving Marc-André's series at
> https://patchew.org/QEMU/20170704220346.29244-1-marcandre.lureau@redhat.com/.
> A lot of the patches are similar to the ones that Marc-André wrote,
> but due to the changes in the code it was easier to redo them.
> 
> For nbd, the patch is on top of "nbd: mark more coroutine_fns" that
> I sent a few days ago and that (AIUI) Eric has already queued; only
> one function was missing, much to my surprise.
> 
> Apart from this, I also identified the following functions that
> can be called both in coroutine context and outside:
> 
> - qmp_dispatch
> - schedule_next_request
> - nvme_get_free_req
> - bdrv_create
> - bdrv_remove_persistent_dirty_bitmap
> - bdrv_can_store_new_dirty_bitmap
> - bdrv_do_drained_begin
> - bdrv_do_drained_end
> - bdrv_drain_all_begin
> - qcow2_open
> - qcow2_has_zero_init
> - bdrv_qed_open
> - qio_channel_readv_full_all_eof
> - qio_channel_writev_full_all
> 
> besides, of course, everything that is generated by
> scripts/block-coroutine-wrapper.py.

This looks useful, thanks for bringing it back!

As Eric mentioned, the commits need justifications. The following cases
come to mind:

1. Add coroutine_fn because the function calls a function that is marked
   with coroutine_fn. This must be fixed because it can lead to crashes.

2. Remove coroutine_fn because the function does not call any functions
   marked with coroutine_fn. This is optional because it does not lead
   to crashes and maybe the author intended to be explicit that this
   function runs only in coroutine context even though it doesn't yield.

3. Variants of these cases but related to runtime qemu_in_coroutine()
   checks. Functions should not have coroutine_fn if they legitimately
   are called in both contexts. Any calls to coroutine_fn child
   functions must be conditional on qemu_in_coroutine() or something
   else that indicates whether we are running in coroutine context.

Stefan