From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZYpuv-0001rR-ID for qemu-devel@nongnu.org; Mon, 07 Sep 2015 02:28:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZYpuu-00062v-Av for qemu-devel@nongnu.org; Mon, 07 Sep 2015 02:28:37 -0400 Date: Mon, 7 Sep 2015 14:28:26 +0800 From: Fam Zheng Message-ID: <20150907062826.GC3958@ad.nay.redhat.com> References: <1438144934-23619-1-git-send-email-famz@redhat.com> <20150828115320.GK4917@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150828115320.GK4917@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, stefanha@redhat.com On Fri, 08/28 12:53, Stefan Hajnoczi wrote: > On Wed, Jul 29, 2015 at 12:42:03PM +0800, Fam Zheng wrote: > > v2: Switch to disable/enable model. [Paolo] > > > > Most existing nested aio_poll()'s in block layer are inconsiderate of > > dispatching potential new r/w requests from ioeventfds and nbd exports, which > > might result in responsiveness issues (e.g. bdrv_drain_all will not return when > > new requests keep coming), or even wrong semantics (e.g. qmp_transaction cannot > > enforce atomicity due to aio_poll in bdrv_drain_all). > > > > Previous attampts to address this issue include new op blocker[1], bdrv_lock[2] > > and nested AioContext (patches not posted to qemu-devel). > > > > This approach is based on the idea proposed by Paolo Bonzini. The original idea > > is introducing "aio_context_disable_client / aio_context_enable_client to > > filter AioContext handlers according to the "client", e.g. > > AIO_CLIENT_DATAPLANE (ioeventfd), AIO_CLIENT_PROTOCOL, AIO_CLIENT_NBD_SERVER, > > AIO_CLIENT_CONTEXT, ... Extend aio_set_{event_notifier,fd}_handler to pass a > > client (type)." > > > > After this series, block layer aio_poll() will only process those "protocol" > > fds that are used in block I/O, plus the ctx->notifier for aio_notify(); other > > aio_poll()'s keep unchanged. > > > > The biggest advantage over approaches [1] and [2] is, no change is needed in > > virtio-{blk,scsi}-dataplane code, also this doesn't depend on converting QMP to > > coroutines. > > > > The windows implementation is not tested except for compiling. > > > > [1]: https://lists.gnu.org/archive/html/qemu-block/2015-05/msg00800.html > > [2]: http://lists.nongnu.org/archive/html/qemu-block/2015-06/msg00027.html > > > > > > Fam Zheng (11): > > aio: Introduce "type" in aio_set_fd_handler and aio_set_event_notifier > > aio: Save type to AioHandler > > block: Mark fd handlers as "protocol" > > nbd: Mark fd handlers client type as "nbd server" > > aio: Mark ctx->notifier's client type as "context" > > dataplane: Mark host notifiers' client type as "dataplane" > > aio-posix: introduce aio_{disable,enable}_clients > > aio-win32: Implement aio_{disable,enable}_clients > > block: Introduce bdrv_aio_poll > > block: Replace nested aio_poll with bdrv_aio_poll > > block: Only poll block layer fds in bdrv_aio_poll > > > > aio-posix.c | 23 ++++++++++++++-- > > aio-win32.c | 22 +++++++++++++++- > > async.c | 3 ++- > > block.c | 2 +- > > block/curl.c | 16 +++++++----- > > block/io.c | 28 +++++++++++++------- > > block/iscsi.c | 9 +++---- > > block/linux-aio.c | 5 ++-- > > block/nbd-client.c | 10 ++++--- > > block/nfs.c | 19 ++++++-------- > > block/qed-table.c | 8 +++--- > > block/sheepdog.c | 32 +++++++++++++++-------- > > block/ssh.c | 5 ++-- > > block/win32-aio.c | 5 ++-- > > blockjob.c | 2 +- > > hw/block/dataplane/virtio-blk.c | 6 +++-- > > hw/scsi/virtio-scsi-dataplane.c | 24 +++++++++++------ > > include/block/aio.h | 33 +++++++++++++++++++++++ > > include/block/block.h | 2 ++ > > nbd.c | 4 ++- > > qemu-img.c | 2 +- > > qemu-io-cmds.c | 4 +-- > > tests/test-aio.c | 58 +++++++++++++++++++++++------------------ > > 23 files changed, 219 insertions(+), 103 deletions(-) > > What is the status of this series? > > Paolo's points about Patch 11 & 12 stand. It seems the fd type concept > is good but the bdrv_aio_poll() mechanism requires changes to fully > solve the problem. Let's drop bdrv_aio_poll(), introduce bdrv_quiesce() and bdrv_unquiesce() to protect nested aio_poll(). For example in QMP, add them between prepare and commit. That needs more changes, like splitting drive_backup_prepare() and put the starting of coroutine and installing of "before_write" notifier to BdrvActionOps.commit. Fam