[PATCH v3 00/20] block: remove aio_disable_external() API

Stefan Hajnoczi posted 20 patches 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230420113732.336620-1-stefanha@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Xie Yongji <xieyongji@bytedance.com>, Coiby Xu <Coiby.Xu@gmail.com>, Fam Zheng <fam@euphon.net>, Aarushi Mehta <mehta.aaru20@gmail.com>, Julia Suvorova <jusual@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, Ronnie Sahlberg <ronniesahlberg@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Lieven <pl@kamp.de>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Richard W.M. Jones" <rjones@redhat.com>, Stefan Weil <sw@weilnetz.de>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, "Michael S. Tsirkin" <mst@redhat.com>, David Woodhouse <dwmw2@infradead.org>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Juan Quintela <quintela@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>
There is a newer version of this series
hw/block/dataplane/xen-block.h              |   2 +
include/block/aio.h                         |  55 ---------
include/block/export.h                      |   2 +
include/hw/qdev-core.h                      |  17 ++-
include/hw/scsi/scsi.h                      |  14 +++
include/qemu/vhost-user-server.h            |   8 +-
include/sysemu/block-backend-common.h       |  25 ++--
include/sysemu/block-backend-global-state.h |   1 +
util/aio-posix.h                            |   1 -
block.c                                     |   7 --
block/blkio.c                               |  15 +--
block/block-backend.c                       |   7 ++
block/curl.c                                |  10 +-
block/export/export.c                       |  13 +-
block/export/fuse.c                         |  58 ++++++++-
block/export/vduse-blk.c                    | 128 ++++++++++++++------
block/export/vhost-user-blk-server.c        |  73 +++++++----
block/io.c                                  |   5 +-
block/io_uring.c                            |   4 +-
block/iscsi.c                               |   3 +-
block/linux-aio.c                           |   4 +-
block/nfs.c                                 |   5 +-
block/nvme.c                                |   8 +-
block/ssh.c                                 |   4 +-
block/win32-aio.c                           |   6 +-
hw/block/dataplane/virtio-blk.c             |  19 ++-
hw/block/dataplane/xen-block.c              |  42 +++++--
hw/block/virtio-blk.c                       |  38 +++++-
hw/block/xen-block.c                        |  24 +++-
hw/i386/kvm/xen_xenstore.c                  |   2 +-
hw/scsi/scsi-bus.c                          |  46 ++++++-
hw/scsi/scsi-disk.c                         |  28 ++++-
hw/scsi/virtio-scsi-dataplane.c             |  31 +++--
hw/scsi/virtio-scsi.c                       |  59 +++++++--
hw/virtio/virtio.c                          |   6 +-
hw/xen/xen-bus.c                            |  11 +-
io/channel-command.c                        |   6 +-
io/channel-file.c                           |   3 +-
io/channel-socket.c                         |   3 +-
migration/rdma.c                            |  16 +--
tests/unit/test-aio.c                       |  27 +----
tests/unit/test-fdmon-epoll.c               |  73 -----------
util/aio-posix.c                            |  20 +--
util/aio-win32.c                            |   8 +-
util/async.c                                |   3 +-
util/fdmon-epoll.c                          |  10 --
util/fdmon-io_uring.c                       |   8 +-
util/fdmon-poll.c                           |   3 +-
util/main-loop.c                            |   7 +-
util/qemu-coroutine-io.c                    |   7 +-
util/vhost-user-server.c                    |  38 +++---
hw/scsi/trace-events                        |   2 +
tests/unit/meson.build                      |   3 -
53 files changed, 582 insertions(+), 436 deletions(-)
delete mode 100644 tests/unit/test-fdmon-epoll.c
[PATCH v3 00/20] block: remove aio_disable_external() API
Posted by Stefan Hajnoczi 1 year ago
v3:
- Resend full patch series. v2 was sent in the middle of a git rebase and was
  missing patches. [Eric]
- Apply Reviewed-by tags.
v2:
- Do not rely on BlockBackend request queuing, implement .drained_begin/end()
  instead in xen-block, virtio-blk, and virtio-scsi [Paolo]
- Add qdev_is_realized() API [Philippe]
- Add patch to avoid AioContext lock around blk_exp_ref/unref() [Paolo]
- Add patch to call .drained_begin/end() from main loop thread to simplify
  callback implementations

The aio_disable_external() API temporarily suspends file descriptor monitoring
in the event loop. The block layer uses this to prevent new I/O requests being
submitted from the guest and elsewhere between bdrv_drained_begin() and
bdrv_drained_end().

While the block layer still needs to prevent new I/O requests in drained
sections, the aio_disable_external() API can be replaced with
.drained_begin/end/poll() callbacks that have been added to BdrvChildClass and
BlockDevOps.

This newer .bdrained_begin/end/poll() approach is attractive because it works
without specifying a specific AioContext. The block layer is moving towards
multi-queue and that means multiple AioContexts may be processing I/O
simultaneously.

The aio_disable_external() was always somewhat hacky. It suspends all file
descriptors that were registered with is_external=true, even if they have
nothing to do with the BlockDriverState graph nodes that are being drained.
It's better to solve a block layer problem in the block layer than to have an
odd event loop API solution.

The approach in this patch series is to implement BlockDevOps
.drained_begin/end() callbacks that temporarily stop file descriptor handlers.
This ensures that new I/O requests are not submitted in drained sections.

The first two virtio-scsi patches were already sent as a separate series. I
included them because they are necessary in order to fully remove
aio_disable_external().

Based-on: 087bc644b7634436ca9d52fe58ba9234e2bef026 (kevin/block-next)

Stefan Hajnoczi (20):
  hw/qdev: introduce qdev_is_realized() helper
  virtio-scsi: avoid race between unplug and transport event
  virtio-scsi: stop using aio_disable_external() during unplug
  block/export: only acquire AioContext once for
    vhost_user_server_stop()
  util/vhost-user-server: rename refcount to in_flight counter
  block/export: wait for vhost-user-blk requests when draining
  block/export: stop using is_external in vhost-user-blk server
  hw/xen: do not use aio_set_fd_handler(is_external=true) in
    xen_xenstore
  block: add blk_in_drain() API
  block: drain from main loop thread in bdrv_co_yield_to_drain()
  xen-block: implement BlockDevOps->drained_begin()
  hw/xen: do not set is_external=true on evtchn fds
  block/export: rewrite vduse-blk drain code
  block/export: don't require AioContext lock around blk_exp_ref/unref()
  block/fuse: do not set is_external=true on FUSE fd
  virtio: make it possible to detach host notifier from any thread
  virtio-blk: implement BlockDevOps->drained_begin()
  virtio-scsi: implement BlockDevOps->drained_begin()
  virtio: do not set is_external=true on host notifiers
  aio: remove aio_disable_external() API

 hw/block/dataplane/xen-block.h              |   2 +
 include/block/aio.h                         |  55 ---------
 include/block/export.h                      |   2 +
 include/hw/qdev-core.h                      |  17 ++-
 include/hw/scsi/scsi.h                      |  14 +++
 include/qemu/vhost-user-server.h            |   8 +-
 include/sysemu/block-backend-common.h       |  25 ++--
 include/sysemu/block-backend-global-state.h |   1 +
 util/aio-posix.h                            |   1 -
 block.c                                     |   7 --
 block/blkio.c                               |  15 +--
 block/block-backend.c                       |   7 ++
 block/curl.c                                |  10 +-
 block/export/export.c                       |  13 +-
 block/export/fuse.c                         |  58 ++++++++-
 block/export/vduse-blk.c                    | 128 ++++++++++++++------
 block/export/vhost-user-blk-server.c        |  73 +++++++----
 block/io.c                                  |   5 +-
 block/io_uring.c                            |   4 +-
 block/iscsi.c                               |   3 +-
 block/linux-aio.c                           |   4 +-
 block/nfs.c                                 |   5 +-
 block/nvme.c                                |   8 +-
 block/ssh.c                                 |   4 +-
 block/win32-aio.c                           |   6 +-
 hw/block/dataplane/virtio-blk.c             |  19 ++-
 hw/block/dataplane/xen-block.c              |  42 +++++--
 hw/block/virtio-blk.c                       |  38 +++++-
 hw/block/xen-block.c                        |  24 +++-
 hw/i386/kvm/xen_xenstore.c                  |   2 +-
 hw/scsi/scsi-bus.c                          |  46 ++++++-
 hw/scsi/scsi-disk.c                         |  28 ++++-
 hw/scsi/virtio-scsi-dataplane.c             |  31 +++--
 hw/scsi/virtio-scsi.c                       |  59 +++++++--
 hw/virtio/virtio.c                          |   6 +-
 hw/xen/xen-bus.c                            |  11 +-
 io/channel-command.c                        |   6 +-
 io/channel-file.c                           |   3 +-
 io/channel-socket.c                         |   3 +-
 migration/rdma.c                            |  16 +--
 tests/unit/test-aio.c                       |  27 +----
 tests/unit/test-fdmon-epoll.c               |  73 -----------
 util/aio-posix.c                            |  20 +--
 util/aio-win32.c                            |   8 +-
 util/async.c                                |   3 +-
 util/fdmon-epoll.c                          |  10 --
 util/fdmon-io_uring.c                       |   8 +-
 util/fdmon-poll.c                           |   3 +-
 util/main-loop.c                            |   7 +-
 util/qemu-coroutine-io.c                    |   7 +-
 util/vhost-user-server.c                    |  38 +++---
 hw/scsi/trace-events                        |   2 +
 tests/unit/meson.build                      |   3 -
 53 files changed, 582 insertions(+), 436 deletions(-)
 delete mode 100644 tests/unit/test-fdmon-epoll.c

-- 
2.39.2
Re: [PATCH v3 00/20] block: remove aio_disable_external() API
Posted by Philippe Mathieu-Daudé 1 year ago
Hi Stefan,

On 20/4/23 13:37, Stefan Hajnoczi wrote:
> v3:
> - Resend full patch series. v2 was sent in the middle of a git rebase and was
>    missing patches. [Eric]
> - Apply Reviewed-by tags.

> Based-on: 087bc644b7634436ca9d52fe58ba9234e2bef026 (kevin/block-next)

It seems kevin/block-next got rebased and doesn't contain 087bc644b76.

Based on 3d1ba50c4b ("vmdk: make vmdk_is_cid_valid a coroutine_fn")
I get:

Applying: hw/qdev: introduce qdev_is_realized() helper
Applying: virtio-scsi: avoid race between unplug and transport event
Applying: virtio-scsi: stop using aio_disable_external() during unplug
Applying: block/export: only acquire AioContext once for 
vhost_user_server_stop()
error: patch failed: util/vhost-user-server.c:346
error: util/vhost-user-server.c: patch does not apply
Patch failed at 0004 block/export: only acquire AioContext once for 
vhost_user_server_stop()

Hmm patch #4 is already merged as commit 2957dc40a2, let's skip it:

$ git am --skip
Applying: util/vhost-user-server: rename refcount to in_flight counter
Applying: block/export: wait for vhost-user-blk requests when draining
Applying: block/export: stop using is_external in vhost-user-blk server
Applying: hw/xen: do not use aio_set_fd_handler(is_external=true) in 
xen_xenstore
Applying: block: add blk_in_drain() API
Applying: block: drain from main loop thread in bdrv_co_yield_to_drain()
Applying: xen-block: implement BlockDevOps->drained_begin()
Applying: hw/xen: do not set is_external=true on evtchn fds
Applying: block/export: rewrite vduse-blk drain code
Applying: block/export: don't require AioContext lock around 
blk_exp_ref/unref()
Applying: block/fuse: do not set is_external=true on FUSE fd
Applying: virtio: make it possible to detach host notifier from any thread
Applying: virtio-blk: implement BlockDevOps->drained_begin()
Applying: virtio-scsi: implement BlockDevOps->drained_begin()
Applying: virtio: do not set is_external=true on host notifiers
Applying: aio: remove aio_disable_external() API
error: patch failed: util/fdmon-epoll.c:131
error: util/fdmon-epoll.c: patch does not apply
Patch failed at 0020 aio: remove aio_disable_external() API

Now this clashes with commit e62da98527 ("aio-posix: fix race between 
epoll upgrade and aio_set_fd_handler()").

Indeed reverting both e62da98527 / 2957dc40a2, I can apply your
series.
Re: [PATCH v3 00/20] block: remove aio_disable_external() API
Posted by Stefan Hajnoczi 1 year ago
On Thu, Apr 20, 2023 at 03:39:59PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
> 
> On 20/4/23 13:37, Stefan Hajnoczi wrote:
> > v3:
> > - Resend full patch series. v2 was sent in the middle of a git rebase and was
> >    missing patches. [Eric]
> > - Apply Reviewed-by tags.
> 
> > Based-on: 087bc644b7634436ca9d52fe58ba9234e2bef026 (kevin/block-next)
> 
> It seems kevin/block-next got rebased and doesn't contain 087bc644b76.
> 
> Based on 3d1ba50c4b ("vmdk: make vmdk_is_cid_valid a coroutine_fn")
> I get:
> 
> Applying: hw/qdev: introduce qdev_is_realized() helper
> Applying: virtio-scsi: avoid race between unplug and transport event
> Applying: virtio-scsi: stop using aio_disable_external() during unplug
> Applying: block/export: only acquire AioContext once for
> vhost_user_server_stop()
> error: patch failed: util/vhost-user-server.c:346
> error: util/vhost-user-server.c: patch does not apply
> Patch failed at 0004 block/export: only acquire AioContext once for
> vhost_user_server_stop()
> 
> Hmm patch #4 is already merged as commit 2957dc40a2, let's skip it:
> 
> $ git am --skip
> Applying: util/vhost-user-server: rename refcount to in_flight counter
> Applying: block/export: wait for vhost-user-blk requests when draining
> Applying: block/export: stop using is_external in vhost-user-blk server
> Applying: hw/xen: do not use aio_set_fd_handler(is_external=true) in
> xen_xenstore
> Applying: block: add blk_in_drain() API
> Applying: block: drain from main loop thread in bdrv_co_yield_to_drain()
> Applying: xen-block: implement BlockDevOps->drained_begin()
> Applying: hw/xen: do not set is_external=true on evtchn fds
> Applying: block/export: rewrite vduse-blk drain code
> Applying: block/export: don't require AioContext lock around
> blk_exp_ref/unref()
> Applying: block/fuse: do not set is_external=true on FUSE fd
> Applying: virtio: make it possible to detach host notifier from any thread
> Applying: virtio-blk: implement BlockDevOps->drained_begin()
> Applying: virtio-scsi: implement BlockDevOps->drained_begin()
> Applying: virtio: do not set is_external=true on host notifiers
> Applying: aio: remove aio_disable_external() API
> error: patch failed: util/fdmon-epoll.c:131
> error: util/fdmon-epoll.c: patch does not apply
> Patch failed at 0020 aio: remove aio_disable_external() API
> 
> Now this clashes with commit e62da98527 ("aio-posix: fix race between epoll
> upgrade and aio_set_fd_handler()").
> 
> Indeed reverting both e62da98527 / 2957dc40a2, I can apply your
> series.

Thanks, I will rebase to origin/master now to make this patch series
easier to merge.

Stefan