Re: [Qemu-devel] [PATCH v4 00/17] aio: drop io_flush()

[Paolo Bonzini <pbonzini@redhat.com>]

Il 14/06/2013 13:43, Stefan Hajnoczi ha scritto:
> v4:
>  * Ensure pending BHs are processed in bdrv_drain_all() [bonzini]
> 
> v3:
>  * I forgot about this series, time to push it again!
>  * Rebase onto qemu.git/master
>  * Drop now-unused AioFlushHandler typedef [bonzini]
> 
> This series gets rid of aio's .io_flush() callback.  It's based on Paolo's
> insight that bdrv_drain_all() can be implemented using the bs->tracked_requests
> list.  io_flush() is redundant since the block layer already knows if requests
> are pending.
> 
> The point of this effort is to simplify our event loop(s).  If we can reduce
> custom features like io_flush() it becomes easier to unify event loops and
> reuse glib or other options.
> 
> This is also important to me for dataplane, since bdrv_drain_all() is one of
> the synchronization points between threads.  QEMU monitor commands invoke
> bdrv_drain_all() while the block device is accessed from a dataplane thread.
> 
> Background on io_flush() semantics:
> 
> The io_flush() handler must return 1 if this aio fd handler is active.  That
> is, requests are pending and we'd like to make progress by monitoring the fd.
> 
> If io_flush() returns 0, the aio event loop skips monitoring this fd.  This is
> critical for block drivers like iscsi where we have an idle TCP socket which we
> want to block on *only* when there are pending requests.
> 
> The series works as follows:
> 
> Part 1 - stop relying on .io_flush()
> 
> The first patches change aio_poll() callers to check their termination
> condition themselves instead of relying on .io_flush():
> 
>   76f9848 block: stop relying on io_flush() in bdrv_drain_all()
>   42c7aac dataplane/virtio-blk: check exit conditions before aio_poll()
>   b7c8b9a tests: adjust test-aio to new aio_poll() semantics
>   55c7714 tests: adjust test-thread-pool to new aio_poll() semantics
> 
> Part 2 - stop calling .io_flush() from aio_poll()
> 
> The semantic change to aio_poll() is made here:
> 
>   8f188ac aio: stop using .io_flush()
> 
> Part 3 - drop io_flush() handlers, just pass NULL for the io_flush argument
> 
> Remove the now dead code from all .io_flush() users:
> 
>   5967680 block/curl: drop curl_aio_flush()
>   16ee264 block/gluster: drop qemu_gluster_aio_flush_cb()
>   9563d0f block/iscsi: drop iscsi_process_flush()
>   6690260 block/linux-aio: drop qemu_laio_completion_cb()
>   3c30643 block/nbd: drop nbd_have_request()
>   d47edee block/rbd: drop qemu_rbd_aio_flush_cb()
>   edbf4b5 block/sheepdog: drop have_co_req() and aio_flush_request()
>   3fee517 block/ssh: drop return_true()
>   d59650d dataplane/virtio-blk: drop flush_true() and flush_io()
>   0492713 thread-pool: drop thread_pool_active()
>   77b3518 tests: drop event_active_cb()
> 
> Part 4 - remove io_flush arguments from aio functions
> 
> The big, mechanical patch that drops the io_flush argument:
> 
>   5938cf4 aio: drop io_flush argument
> 
> Note that I split Part 3 from Part 4 so it's easy to review individual block
> drivers.
> 
> Stefan Hajnoczi (17):
>   block: stop relying on io_flush() in bdrv_drain_all()
>   dataplane/virtio-blk: check exit conditions before aio_poll()
>   tests: adjust test-aio to new aio_poll() semantics
>   tests: adjust test-thread-pool to new aio_poll() semantics
>   aio: stop using .io_flush()
>   block/curl: drop curl_aio_flush()
>   block/gluster: drop qemu_gluster_aio_flush_cb()
>   block/iscsi: drop iscsi_process_flush()
>   block/linux-aio: drop qemu_laio_completion_cb()
>   block/nbd: drop nbd_have_request()
>   block/rbd: drop qemu_rbd_aio_flush_cb()
>   block/sheepdog: drop have_co_req() and aio_flush_request()
>   block/ssh: drop return_true()
>   dataplane/virtio-blk: drop flush_true() and flush_io()
>   thread-pool: drop thread_pool_active()
>   tests: drop event_active_cb()
>   aio: drop io_flush argument
> 
>  aio-posix.c                     | 36 ++++++------------
>  aio-win32.c                     | 37 ++++++++-----------
>  async.c                         |  4 +-
>  block.c                         | 50 ++++++++++++++++++-------
>  block/curl.c                    | 25 ++-----------
>  block/gluster.c                 | 21 ++---------
>  block/iscsi.c                   | 10 +----
>  block/linux-aio.c               | 18 +--------
>  block/nbd.c                     | 18 ++-------
>  block/rbd.c                     | 16 +-------
>  block/sheepdog.c                | 33 ++++-------------
>  block/ssh.c                     | 12 +-----
>  hw/block/dataplane/virtio-blk.c | 25 +++----------
>  include/block/aio.h             | 14 +------
>  main-loop.c                     |  9 ++---
>  tests/test-aio.c                | 82 +++++++++++++++++++++--------------------
>  tests/test-thread-pool.c        | 24 ++++++------
>  thread-pool.c                   | 11 +-----
>  18 files changed, 158 insertions(+), 287 deletions(-)
> 

The series looks fine, just two nits: one patch to extract out of the
first, and one commit message to adjust in patch 16.  Otherwise

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo