From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57662) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsC9N-0001A6-28 for qemu-devel@nongnu.org; Thu, 27 Jun 2013 09:22:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UsC9I-0007aZ-6c for qemu-devel@nongnu.org; Thu, 27 Jun 2013 09:22:13 -0400 Received: from mail-bk0-x22a.google.com ([2a00:1450:4008:c01::22a]:42219) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsC9H-0007aP-TO for qemu-devel@nongnu.org; Thu, 27 Jun 2013 09:22:08 -0400 Received: by mail-bk0-f42.google.com with SMTP id jk13so308880bkc.15 for ; Thu, 27 Jun 2013 06:22:07 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <51CC3C78.9080807@redhat.com> Date: Thu, 27 Jun 2013 15:22:00 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1371210243-6099-1-git-send-email-stefanha@redhat.com> In-Reply-To: <1371210243-6099-1-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 00/17] aio: drop io_flush() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Anthony Liguori , Ping Fan Liu , qemu-devel@nongnu.org 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 Paolo