[Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Fam Zheng]

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(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 01/11] aio: Introduce "type" in aio_set_fd_handler and aio_set_event_notifier

[Fam Zheng]

The parameter is added but not used.

The callers are converted with following coccinelle semantic patch:

    @@
    expression E1, E2, E3, E4, E5;
    @@
    (
    -aio_set_event_notifier(E1, E2, E3)
    +aio_set_event_notifier(E1, E2, AIO_CLIENT_UNSPECIFIED, E3)
    |
    -aio_set_fd_handler(E1, E2, E3, E4, E5)
    +aio_set_fd_handler(E1, E2, AIO_CLIENT_UNSPECIFIED, E3, E4, E5)
    )

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 aio-posix.c                     |  4 ++-
 aio-win32.c                     |  2 ++
 async.c                         |  3 ++-
 block/curl.c                    | 14 +++++-----
 block/iscsi.c                   |  9 +++----
 block/linux-aio.c               |  5 ++--
 block/nbd-client.c              | 10 ++++---
 block/nfs.c                     | 17 +++++-------
 block/sheepdog.c                | 32 +++++++++++++++--------
 block/ssh.c                     |  5 ++--
 block/win32-aio.c               |  5 ++--
 hw/block/dataplane/virtio-blk.c |  6 +++--
 hw/scsi/virtio-scsi-dataplane.c | 24 +++++++++++------
 include/block/aio.h             |  5 ++++
 nbd.c                           |  4 ++-
 tests/test-aio.c                | 58 +++++++++++++++++++++++------------------
 16 files changed, 122 insertions(+), 81 deletions(-)

diff --git a/aio-posix.c b/aio-posix.c
index d477033..56f2bce 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -43,6 +43,7 @@ static AioHandler *find_aio_handler(AioContext *ctx, int fd)
 
 void aio_set_fd_handler(AioContext *ctx,
                         int fd,
+                        int type,
                         IOHandler *io_read,
                         IOHandler *io_write,
                         void *opaque)
@@ -92,10 +93,11 @@ void aio_set_fd_handler(AioContext *ctx,
 
 void aio_set_event_notifier(AioContext *ctx,
                             EventNotifier *notifier,
+                            int type,
                             EventNotifierHandler *io_read)
 {
     aio_set_fd_handler(ctx, event_notifier_get_fd(notifier),
-                       (IOHandler *)io_read, NULL, notifier);
+                       type, (IOHandler *)io_read, NULL, notifier);
 }
 
 bool aio_prepare(AioContext *ctx)
diff --git a/aio-win32.c b/aio-win32.c
index 50a6867..90e7a4b 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -33,6 +33,7 @@ struct AioHandler {
 
 void aio_set_fd_handler(AioContext *ctx,
                         int fd,
+                        int type,
                         IOHandler *io_read,
                         IOHandler *io_write,
                         void *opaque)
@@ -98,6 +99,7 @@ void aio_set_fd_handler(AioContext *ctx,
 
 void aio_set_event_notifier(AioContext *ctx,
                             EventNotifier *e,
+                            int type,
                             EventNotifierHandler *io_notify)
 {
     AioHandler *node;
diff --git a/async.c b/async.c
index 9a98a74..43f9425 100644
--- a/async.c
+++ b/async.c
@@ -231,7 +231,7 @@ aio_ctx_finalize(GSource     *source)
     AioContext *ctx = (AioContext *) source;
 
     thread_pool_free(ctx->thread_pool);
-    aio_set_event_notifier(ctx, &ctx->notifier, NULL);
+    aio_set_event_notifier(ctx, &ctx->notifier, AIO_CLIENT_UNSPECIFIED, NULL);
     event_notifier_cleanup(&ctx->notifier);
     rfifolock_destroy(&ctx->lock);
     qemu_mutex_destroy(&ctx->bh_lock);
@@ -306,6 +306,7 @@ AioContext *aio_context_new(Error **errp)
     }
     g_source_set_can_recurse(&ctx->source, true);
     aio_set_event_notifier(ctx, &ctx->notifier,
+                           AIO_CLIENT_UNSPECIFIED,
                            (EventNotifierHandler *)
                            event_notifier_dummy_cb);
     ctx->thread_pool = NULL;
diff --git a/block/curl.c b/block/curl.c
index 032cc8a..6925672 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -154,18 +154,20 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, fd);
     switch (action) {
         case CURL_POLL_IN:
-            aio_set_fd_handler(s->aio_context, fd, curl_multi_read,
-                               NULL, state);
+            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+                               curl_multi_read, NULL, state);
             break;
         case CURL_POLL_OUT:
-            aio_set_fd_handler(s->aio_context, fd, NULL, curl_multi_do, state);
+            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+                               NULL, curl_multi_do, state);
             break;
         case CURL_POLL_INOUT:
-            aio_set_fd_handler(s->aio_context, fd, curl_multi_read,
-                               curl_multi_do, state);
+            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+                               curl_multi_read, curl_multi_do, state);
             break;
         case CURL_POLL_REMOVE:
-            aio_set_fd_handler(s->aio_context, fd, NULL, NULL, NULL);
+            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+                               NULL, NULL, NULL);
             break;
     }
 
diff --git a/block/iscsi.c b/block/iscsi.c
index 5002916..0ee1295 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -291,8 +291,8 @@ iscsi_set_events(IscsiLun *iscsilun)
     int ev = iscsi_which_events(iscsi);
 
     if (ev != iscsilun->events) {
-        aio_set_fd_handler(iscsilun->aio_context,
-                           iscsi_get_fd(iscsi),
+        aio_set_fd_handler(iscsilun->aio_context, iscsi_get_fd(iscsi),
+                           AIO_CLIENT_UNSPECIFIED,
                            (ev & POLLIN) ? iscsi_process_read : NULL,
                            (ev & POLLOUT) ? iscsi_process_write : NULL,
                            iscsilun);
@@ -1276,9 +1276,8 @@ static void iscsi_detach_aio_context(BlockDriverState *bs)
 {
     IscsiLun *iscsilun = bs->opaque;
 
-    aio_set_fd_handler(iscsilun->aio_context,
-                       iscsi_get_fd(iscsilun->iscsi),
-                       NULL, NULL, NULL);
+    aio_set_fd_handler(iscsilun->aio_context, iscsi_get_fd(iscsilun->iscsi),
+                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
     iscsilun->events = 0;
 
     if (iscsilun->nop_timer) {
diff --git a/block/linux-aio.c b/block/linux-aio.c
index c991443..0921bde 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -287,7 +287,7 @@ void laio_detach_aio_context(void *s_, AioContext *old_context)
 {
     struct qemu_laio_state *s = s_;
 
-    aio_set_event_notifier(old_context, &s->e, NULL);
+    aio_set_event_notifier(old_context, &s->e, AIO_CLIENT_UNSPECIFIED, NULL);
     qemu_bh_delete(s->completion_bh);
 }
 
@@ -296,7 +296,8 @@ void laio_attach_aio_context(void *s_, AioContext *new_context)
     struct qemu_laio_state *s = s_;
 
     s->completion_bh = aio_bh_new(new_context, qemu_laio_completion_bh, s);
-    aio_set_event_notifier(new_context, &s->e, qemu_laio_completion_cb);
+    aio_set_event_notifier(new_context, &s->e, AIO_CLIENT_UNSPECIFIED,
+                           qemu_laio_completion_cb);
 }
 
 void *laio_init(void)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index e1bb919..36c46c5 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -124,7 +124,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
     s->send_coroutine = qemu_coroutine_self();
     aio_context = bdrv_get_aio_context(bs);
 
-    aio_set_fd_handler(aio_context, s->sock,
+    aio_set_fd_handler(aio_context, s->sock, AIO_CLIENT_UNSPECIFIED,
                        nbd_reply_ready, nbd_restart_write, bs);
     if (qiov) {
         if (!s->is_unix) {
@@ -144,7 +144,8 @@ static int nbd_co_send_request(BlockDriverState *bs,
     } else {
         rc = nbd_send_request(s->sock, request);
     }
-    aio_set_fd_handler(aio_context, s->sock, nbd_reply_ready, NULL, bs);
+    aio_set_fd_handler(aio_context, s->sock, AIO_CLIENT_UNSPECIFIED,
+                       nbd_reply_ready, NULL, bs);
     s->send_coroutine = NULL;
     qemu_co_mutex_unlock(&s->send_mutex);
     return rc;
@@ -348,14 +349,15 @@ int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
 void nbd_client_detach_aio_context(BlockDriverState *bs)
 {
     aio_set_fd_handler(bdrv_get_aio_context(bs),
-                       nbd_get_client_session(bs)->sock, NULL, NULL, NULL);
+                       nbd_get_client_session(bs)->sock,
+                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
 }
 
 void nbd_client_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context)
 {
     aio_set_fd_handler(new_context, nbd_get_client_session(bs)->sock,
-                       nbd_reply_ready, NULL, bs);
+                       AIO_CLIENT_UNSPECIFIED, nbd_reply_ready, NULL, bs);
 }
 
 void nbd_client_close(BlockDriverState *bs)
diff --git a/block/nfs.c b/block/nfs.c
index c026ff6..a21dd6f 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -62,11 +62,10 @@ static void nfs_set_events(NFSClient *client)
 {
     int ev = nfs_which_events(client->context);
     if (ev != client->events) {
-        aio_set_fd_handler(client->aio_context,
-                           nfs_get_fd(client->context),
+        aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
+                           AIO_CLIENT_UNSPECIFIED,
                            (ev & POLLIN) ? nfs_process_read : NULL,
-                           (ev & POLLOUT) ? nfs_process_write : NULL,
-                           client);
+                           (ev & POLLOUT) ? nfs_process_write : NULL, client);
 
     }
     client->events = ev;
@@ -241,9 +240,8 @@ static void nfs_detach_aio_context(BlockDriverState *bs)
 {
     NFSClient *client = bs->opaque;
 
-    aio_set_fd_handler(client->aio_context,
-                       nfs_get_fd(client->context),
-                       NULL, NULL, NULL);
+    aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
+                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
     client->events = 0;
 }
 
@@ -262,9 +260,8 @@ static void nfs_client_close(NFSClient *client)
         if (client->fh) {
             nfs_close(client->context, client->fh);
         }
-        aio_set_fd_handler(client->aio_context,
-                           nfs_get_fd(client->context),
-                           NULL, NULL, NULL);
+        aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
+                           AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
         nfs_destroy_context(client->context);
     }
     memset(client, 0, sizeof(NFSClient));
diff --git a/block/sheepdog.c b/block/sheepdog.c
index bd7cbed..e0552b7 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -624,14 +624,16 @@ static coroutine_fn void do_co_req(void *opaque)
     unsigned int *rlen = srco->rlen;
 
     co = qemu_coroutine_self();
-    aio_set_fd_handler(srco->aio_context, sockfd, NULL, restart_co_req, co);
+    aio_set_fd_handler(srco->aio_context, sockfd, AIO_CLIENT_UNSPECIFIED,
+                       NULL, restart_co_req, co);
 
     ret = send_co_req(sockfd, hdr, data, wlen);
     if (ret < 0) {
         goto out;
     }
 
-    aio_set_fd_handler(srco->aio_context, sockfd, restart_co_req, NULL, co);
+    aio_set_fd_handler(srco->aio_context, sockfd, AIO_CLIENT_UNSPECIFIED,
+                       restart_co_req, NULL, co);
 
     ret = qemu_co_recv(sockfd, hdr, sizeof(*hdr));
     if (ret != sizeof(*hdr)) {
@@ -656,7 +658,8 @@ static coroutine_fn void do_co_req(void *opaque)
 out:
     /* there is at most one request for this sockfd, so it is safe to
      * set each handler to NULL. */
-    aio_set_fd_handler(srco->aio_context, sockfd, NULL, NULL, NULL);
+    aio_set_fd_handler(srco->aio_context, sockfd, AIO_CLIENT_UNSPECIFIED,
+                       NULL, NULL, NULL);
 
     srco->ret = ret;
     srco->finished = true;
@@ -740,7 +743,8 @@ static coroutine_fn void reconnect_to_sdog(void *opaque)
     BDRVSheepdogState *s = opaque;
     AIOReq *aio_req, *next;
 
-    aio_set_fd_handler(s->aio_context, s->fd, NULL, NULL, NULL);
+    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_UNSPECIFIED, NULL,
+                       NULL, NULL);
     close(s->fd);
     s->fd = -1;
 
@@ -953,7 +957,8 @@ static int get_sheep_fd(BDRVSheepdogState *s, Error **errp)
         return fd;
     }
 
-    aio_set_fd_handler(s->aio_context, fd, co_read_response, NULL, s);
+    aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+                       co_read_response, NULL, s);
     return fd;
 }
 
@@ -1208,7 +1213,7 @@ static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
 
     qemu_co_mutex_lock(&s->lock);
     s->co_send = qemu_coroutine_self();
-    aio_set_fd_handler(s->aio_context, s->fd,
+    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_UNSPECIFIED,
                        co_read_response, co_write_request, s);
     socket_set_cork(s->fd, 1);
 
@@ -1227,7 +1232,8 @@ static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
     }
 out:
     socket_set_cork(s->fd, 0);
-    aio_set_fd_handler(s->aio_context, s->fd, co_read_response, NULL, s);
+    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_UNSPECIFIED,
+                       co_read_response, NULL, s);
     s->co_send = NULL;
     qemu_co_mutex_unlock(&s->lock);
 }
@@ -1405,7 +1411,8 @@ static void sd_detach_aio_context(BlockDriverState *bs)
 {
     BDRVSheepdogState *s = bs->opaque;
 
-    aio_set_fd_handler(s->aio_context, s->fd, NULL, NULL, NULL);
+    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_UNSPECIFIED, NULL,
+                       NULL, NULL);
 }
 
 static void sd_attach_aio_context(BlockDriverState *bs,
@@ -1414,7 +1421,8 @@ static void sd_attach_aio_context(BlockDriverState *bs,
     BDRVSheepdogState *s = bs->opaque;
 
     s->aio_context = new_context;
-    aio_set_fd_handler(new_context, s->fd, co_read_response, NULL, s);
+    aio_set_fd_handler(new_context, s->fd, AIO_CLIENT_UNSPECIFIED,
+                       co_read_response, NULL, s);
 }
 
 /* TODO Convert to fine grained options */
@@ -1528,7 +1536,8 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     g_free(buf);
     return 0;
 out:
-    aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd, NULL, NULL, NULL);
+    aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
+                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
     if (s->fd >= 0) {
         closesocket(s->fd);
     }
@@ -1912,7 +1921,8 @@ static void sd_close(BlockDriverState *bs)
         error_report("%s, %s", sd_strerror(rsp->result), s->name);
     }
 
-    aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd, NULL, NULL, NULL);
+    aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
+                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
     closesocket(s->fd);
     g_free(s->host_spec);
 }
diff --git a/block/ssh.c b/block/ssh.c
index aebb18c..71d7ffe 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -803,14 +803,15 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s, BlockDriverState *bs)
             rd_handler, wr_handler);
 
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->sock,
-                       rd_handler, wr_handler, co);
+                       AIO_CLIENT_UNSPECIFIED, rd_handler, wr_handler, co);
 }
 
 static coroutine_fn void clear_fd_handler(BDRVSSHState *s,
                                           BlockDriverState *bs)
 {
     DPRINTF("s->sock=%d", s->sock);
-    aio_set_fd_handler(bdrv_get_aio_context(bs), s->sock, NULL, NULL, NULL);
+    aio_set_fd_handler(bdrv_get_aio_context(bs), s->sock,
+                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
 }
 
 /* A non-blocking call returned EAGAIN, so yield, ensuring the
diff --git a/block/win32-aio.c b/block/win32-aio.c
index 64e8682..0081886 100644
--- a/block/win32-aio.c
+++ b/block/win32-aio.c
@@ -174,7 +174,7 @@ int win32_aio_attach(QEMUWin32AIOState *aio, HANDLE hfile)
 void win32_aio_detach_aio_context(QEMUWin32AIOState *aio,
                                   AioContext *old_context)
 {
-    aio_set_event_notifier(old_context, &aio->e, NULL);
+    aio_set_event_notifier(old_context, &aio->e, AIO_CLIENT_UNSPECIFIED, NULL);
     aio->is_aio_context_attached = false;
 }
 
@@ -182,7 +182,8 @@ void win32_aio_attach_aio_context(QEMUWin32AIOState *aio,
                                   AioContext *new_context)
 {
     aio->is_aio_context_attached = true;
-    aio_set_event_notifier(new_context, &aio->e, win32_aio_completion_cb);
+    aio_set_event_notifier(new_context, &aio->e, AIO_CLIENT_UNSPECIFIED,
+                           win32_aio_completion_cb);
 }
 
 QEMUWin32AIOState *win32_aio_init(void)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 3db139b..e472154 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -283,7 +283,8 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
     /* Get this show started by hooking up our callbacks */
     aio_context_acquire(s->ctx);
-    aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify);
+    aio_set_event_notifier(s->ctx, &s->host_notifier, AIO_CLIENT_UNSPECIFIED,
+                           handle_notify);
     aio_context_release(s->ctx);
     return;
 
@@ -319,7 +320,8 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     aio_context_acquire(s->ctx);
 
     /* Stop notifications for new requests from guest */
-    aio_set_event_notifier(s->ctx, &s->host_notifier, NULL);
+    aio_set_event_notifier(s->ctx, &s->host_notifier, AIO_CLIENT_UNSPECIFIED,
+                           NULL);
 
     /* Drain and switch bs back to the QEMU main loop */
     blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 5575648..f7bab09 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -60,7 +60,8 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
     r = g_slice_new(VirtIOSCSIVring);
     r->host_notifier = *virtio_queue_get_host_notifier(vq);
     r->guest_notifier = *virtio_queue_get_guest_notifier(vq);
-    aio_set_event_notifier(s->ctx, &r->host_notifier, handler);
+    aio_set_event_notifier(s->ctx, &r->host_notifier, AIO_CLIENT_UNSPECIFIED,
+                           handler);
 
     r->parent = s;
 
@@ -71,7 +72,8 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
     return r;
 
 fail_vring:
-    aio_set_event_notifier(s->ctx, &r->host_notifier, NULL);
+    aio_set_event_notifier(s->ctx, &r->host_notifier, AIO_CLIENT_UNSPECIFIED,
+                           NULL);
     k->set_host_notifier(qbus->parent, n, false);
     g_slice_free(VirtIOSCSIVring, r);
     return NULL;
@@ -162,14 +164,17 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
     int i;
 
     if (s->ctrl_vring) {
-        aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
+        aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
+                               AIO_CLIENT_UNSPECIFIED, NULL);
     }
     if (s->event_vring) {
-        aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
+        aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
+                               AIO_CLIENT_UNSPECIFIED, NULL);
     }
     if (s->cmd_vrings) {
         for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
-            aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier, NULL);
+            aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
+                                   AIO_CLIENT_UNSPECIFIED, NULL);
         }
     }
 }
@@ -290,10 +295,13 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
 
     aio_context_acquire(s->ctx);
 
-    aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
-    aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
+    aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
+                           AIO_CLIENT_UNSPECIFIED, NULL);
+    aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
+                           AIO_CLIENT_UNSPECIFIED, NULL);
     for (i = 0; i < vs->conf.num_queues; i++) {
-        aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier, NULL);
+        aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
+                               AIO_CLIENT_UNSPECIFIED, NULL);
     }
 
     blk_drain_all(); /* ensure there are no in-flight requests */
diff --git a/include/block/aio.h b/include/block/aio.h
index 9dd32e0..bd1d44b 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -272,6 +272,9 @@ bool aio_pending(AioContext *ctx);
  */
 bool aio_dispatch(AioContext *ctx);
 
+#define AIO_CLIENT_UNSPECIFIED    (1 << 0)
+#define AIO_CLIENT_MASK_ALL       -1
+
 /* Progress in completing AIO work to occur.  This can issue new pending
  * aio as a result of executing I/O completion or bh callbacks.
  *
@@ -296,6 +299,7 @@ bool aio_poll(AioContext *ctx, bool blocking);
  */
 void aio_set_fd_handler(AioContext *ctx,
                         int fd,
+                        int type,
                         IOHandler *io_read,
                         IOHandler *io_write,
                         void *opaque);
@@ -309,6 +313,7 @@ void aio_set_fd_handler(AioContext *ctx,
  */
 void aio_set_event_notifier(AioContext *ctx,
                             EventNotifier *notifier,
+                            int type,
                             EventNotifierHandler *io_read);
 
 /* Return a GSource that lets the main loop poll the file descriptors attached
diff --git a/nbd.c b/nbd.c
index 06b501b..64ed91b 100644
--- a/nbd.c
+++ b/nbd.c
@@ -1437,6 +1437,7 @@ static void nbd_set_handlers(NBDClient *client)
 {
     if (client->exp && client->exp->ctx) {
         aio_set_fd_handler(client->exp->ctx, client->sock,
+                           AIO_CLIENT_UNSPECIFIED,
                            client->can_read ? nbd_read : NULL,
                            client->send_coroutine ? nbd_restart_write : NULL,
                            client);
@@ -1446,7 +1447,8 @@ static void nbd_set_handlers(NBDClient *client)
 static void nbd_unset_handlers(NBDClient *client)
 {
     if (client->exp && client->exp->ctx) {
-        aio_set_fd_handler(client->exp->ctx, client->sock, NULL, NULL, NULL);
+        aio_set_fd_handler(client->exp->ctx, client->sock,
+                           AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
     }
 }
 
diff --git a/tests/test-aio.c b/tests/test-aio.c
index 217e337..770119f 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -118,6 +118,12 @@ static void *test_acquire_thread(void *opaque)
     return NULL;
 }
 
+static void set_event_notifier(AioContext *ctx, EventNotifier *notifier,
+                               EventNotifierHandler *handler)
+{
+    aio_set_event_notifier(ctx, notifier, AIO_CLIENT_UNSPECIFIED, handler);
+}
+
 static void dummy_notifier_read(EventNotifier *unused)
 {
     g_assert(false); /* should never be invoked */
@@ -131,7 +137,7 @@ static void test_acquire(void)
 
     /* Dummy event notifier ensures aio_poll() will block */
     event_notifier_init(&notifier, false);
-    aio_set_event_notifier(ctx, &notifier, dummy_notifier_read);
+    set_event_notifier(ctx, &notifier, dummy_notifier_read);
     g_assert(!aio_poll(ctx, false)); /* consume aio_notify() */
 
     qemu_mutex_init(&data.start_lock);
@@ -149,7 +155,7 @@ static void test_acquire(void)
     aio_context_release(ctx);
 
     qemu_thread_join(&thread);
-    aio_set_event_notifier(ctx, &notifier, NULL);
+    set_event_notifier(ctx, &notifier, NULL);
     event_notifier_cleanup(&notifier);
 
     g_assert(data.thread_acquired);
@@ -308,11 +314,11 @@ static void test_set_event_notifier(void)
 {
     EventNotifierTestData data = { .n = 0, .active = 0 };
     event_notifier_init(&data.e, false);
-    aio_set_event_notifier(ctx, &data.e, event_ready_cb);
+    set_event_notifier(ctx, &data.e, event_ready_cb);
     g_assert(!aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 0);
 
-    aio_set_event_notifier(ctx, &data.e, NULL);
+    set_event_notifier(ctx, &data.e, NULL);
     g_assert(!aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 0);
     event_notifier_cleanup(&data.e);
@@ -322,7 +328,7 @@ static void test_wait_event_notifier(void)
 {
     EventNotifierTestData data = { .n = 0, .active = 1 };
     event_notifier_init(&data.e, false);
-    aio_set_event_notifier(ctx, &data.e, event_ready_cb);
+    set_event_notifier(ctx, &data.e, event_ready_cb);
     while (aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 0);
     g_assert_cmpint(data.active, ==, 1);
@@ -336,7 +342,7 @@ static void test_wait_event_notifier(void)
     g_assert_cmpint(data.n, ==, 1);
     g_assert_cmpint(data.active, ==, 0);
 
-    aio_set_event_notifier(ctx, &data.e, NULL);
+    set_event_notifier(ctx, &data.e, NULL);
     g_assert(!aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 1);
 
@@ -347,7 +353,7 @@ static void test_flush_event_notifier(void)
 {
     EventNotifierTestData data = { .n = 0, .active = 10, .auto_set = true };
     event_notifier_init(&data.e, false);
-    aio_set_event_notifier(ctx, &data.e, event_ready_cb);
+    set_event_notifier(ctx, &data.e, event_ready_cb);
     while (aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 0);
     g_assert_cmpint(data.active, ==, 10);
@@ -363,7 +369,7 @@ static void test_flush_event_notifier(void)
     g_assert_cmpint(data.active, ==, 0);
     g_assert(!aio_poll(ctx, false));
 
-    aio_set_event_notifier(ctx, &data.e, NULL);
+    set_event_notifier(ctx, &data.e, NULL);
     g_assert(!aio_poll(ctx, false));
     event_notifier_cleanup(&data.e);
 }
@@ -374,7 +380,7 @@ static void test_wait_event_notifier_noflush(void)
     EventNotifierTestData dummy = { .n = 0, .active = 1 };
 
     event_notifier_init(&data.e, false);
-    aio_set_event_notifier(ctx, &data.e, event_ready_cb);
+    set_event_notifier(ctx, &data.e, event_ready_cb);
 
     g_assert(!aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 0);
@@ -387,7 +393,7 @@ static void test_wait_event_notifier_noflush(void)
 
     /* An active event notifier forces aio_poll to look at EventNotifiers.  */
     event_notifier_init(&dummy.e, false);
-    aio_set_event_notifier(ctx, &dummy.e, event_ready_cb);
+    set_event_notifier(ctx, &dummy.e, event_ready_cb);
 
     event_notifier_set(&data.e);
     g_assert(aio_poll(ctx, false));
@@ -407,10 +413,10 @@ static void test_wait_event_notifier_noflush(void)
     g_assert_cmpint(dummy.n, ==, 1);
     g_assert_cmpint(dummy.active, ==, 0);
 
-    aio_set_event_notifier(ctx, &dummy.e, NULL);
+    set_event_notifier(ctx, &dummy.e, NULL);
     event_notifier_cleanup(&dummy.e);
 
-    aio_set_event_notifier(ctx, &data.e, NULL);
+    set_event_notifier(ctx, &data.e, NULL);
     g_assert(!aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 2);
 
@@ -428,7 +434,7 @@ static void test_timer_schedule(void)
      * an fd to wait on. Fixing this breaks other tests. So create a dummy one.
      */
     event_notifier_init(&e, false);
-    aio_set_event_notifier(ctx, &e, dummy_io_handler_read);
+    set_event_notifier(ctx, &e, dummy_io_handler_read);
     aio_poll(ctx, false);
 
     aio_timer_init(ctx, &data.timer, data.clock_type,
@@ -467,7 +473,7 @@ static void test_timer_schedule(void)
     g_assert(!aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 2);
 
-    aio_set_event_notifier(ctx, &e, NULL);
+    set_event_notifier(ctx, &e, NULL);
     event_notifier_cleanup(&e);
 
     timer_del(&data.timer);
@@ -638,11 +644,11 @@ static void test_source_set_event_notifier(void)
 {
     EventNotifierTestData data = { .n = 0, .active = 0 };
     event_notifier_init(&data.e, false);
-    aio_set_event_notifier(ctx, &data.e, event_ready_cb);
+    set_event_notifier(ctx, &data.e, event_ready_cb);
     while (g_main_context_iteration(NULL, false));
     g_assert_cmpint(data.n, ==, 0);
 
-    aio_set_event_notifier(ctx, &data.e, NULL);
+    set_event_notifier(ctx, &data.e, NULL);
     while (g_main_context_iteration(NULL, false));
     g_assert_cmpint(data.n, ==, 0);
     event_notifier_cleanup(&data.e);
@@ -652,7 +658,7 @@ static void test_source_wait_event_notifier(void)
 {
     EventNotifierTestData data = { .n = 0, .active = 1 };
     event_notifier_init(&data.e, false);
-    aio_set_event_notifier(ctx, &data.e, event_ready_cb);
+    set_event_notifier(ctx, &data.e, event_ready_cb);
     while (g_main_context_iteration(NULL, false));
     g_assert_cmpint(data.n, ==, 0);
     g_assert_cmpint(data.active, ==, 1);
@@ -666,7 +672,7 @@ static void test_source_wait_event_notifier(void)
     g_assert_cmpint(data.n, ==, 1);
     g_assert_cmpint(data.active, ==, 0);
 
-    aio_set_event_notifier(ctx, &data.e, NULL);
+    set_event_notifier(ctx, &data.e, NULL);
     while (g_main_context_iteration(NULL, false));
     g_assert_cmpint(data.n, ==, 1);
 
@@ -677,7 +683,7 @@ static void test_source_flush_event_notifier(void)
 {
     EventNotifierTestData data = { .n = 0, .active = 10, .auto_set = true };
     event_notifier_init(&data.e, false);
-    aio_set_event_notifier(ctx, &data.e, event_ready_cb);
+    set_event_notifier(ctx, &data.e, event_ready_cb);
     while (g_main_context_iteration(NULL, false));
     g_assert_cmpint(data.n, ==, 0);
     g_assert_cmpint(data.active, ==, 10);
@@ -693,7 +699,7 @@ static void test_source_flush_event_notifier(void)
     g_assert_cmpint(data.active, ==, 0);
     g_assert(!g_main_context_iteration(NULL, false));
 
-    aio_set_event_notifier(ctx, &data.e, NULL);
+    set_event_notifier(ctx, &data.e, NULL);
     while (g_main_context_iteration(NULL, false));
     event_notifier_cleanup(&data.e);
 }
@@ -704,7 +710,7 @@ static void test_source_wait_event_notifier_noflush(void)
     EventNotifierTestData dummy = { .n = 0, .active = 1 };
 
     event_notifier_init(&data.e, false);
-    aio_set_event_notifier(ctx, &data.e, event_ready_cb);
+    set_event_notifier(ctx, &data.e, event_ready_cb);
 
     while (g_main_context_iteration(NULL, false));
     g_assert_cmpint(data.n, ==, 0);
@@ -717,7 +723,7 @@ static void test_source_wait_event_notifier_noflush(void)
 
     /* An active event notifier forces aio_poll to look at EventNotifiers.  */
     event_notifier_init(&dummy.e, false);
-    aio_set_event_notifier(ctx, &dummy.e, event_ready_cb);
+    set_event_notifier(ctx, &dummy.e, event_ready_cb);
 
     event_notifier_set(&data.e);
     g_assert(g_main_context_iteration(NULL, false));
@@ -737,10 +743,10 @@ static void test_source_wait_event_notifier_noflush(void)
     g_assert_cmpint(dummy.n, ==, 1);
     g_assert_cmpint(dummy.active, ==, 0);
 
-    aio_set_event_notifier(ctx, &dummy.e, NULL);
+    set_event_notifier(ctx, &dummy.e, NULL);
     event_notifier_cleanup(&dummy.e);
 
-    aio_set_event_notifier(ctx, &data.e, NULL);
+    set_event_notifier(ctx, &data.e, NULL);
     while (g_main_context_iteration(NULL, false));
     g_assert_cmpint(data.n, ==, 2);
 
@@ -759,7 +765,7 @@ static void test_source_timer_schedule(void)
      * an fd to wait on. Fixing this breaks other tests. So create a dummy one.
      */
     event_notifier_init(&e, false);
-    aio_set_event_notifier(ctx, &e, dummy_io_handler_read);
+    set_event_notifier(ctx, &e, dummy_io_handler_read);
     do {} while (g_main_context_iteration(NULL, false));
 
     aio_timer_init(ctx, &data.timer, data.clock_type,
@@ -784,7 +790,7 @@ static void test_source_timer_schedule(void)
     g_assert_cmpint(data.n, ==, 2);
     g_assert(qemu_clock_get_ns(data.clock_type) > expiry);
 
-    aio_set_event_notifier(ctx, &e, NULL);
+    set_event_notifier(ctx, &e, NULL);
     event_notifier_cleanup(&e);
 
     timer_del(&data.timer);
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 01/11] aio: Introduce "type" in aio_set_fd_handler and aio_set_event_notifier

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:04PM +0800, Fam Zheng wrote:
> The parameter is added but not used.
> 
> The callers are converted with following coccinelle semantic patch:
> 
>     @@
>     expression E1, E2, E3, E4, E5;
>     @@
>     (
>     -aio_set_event_notifier(E1, E2, E3)
>     +aio_set_event_notifier(E1, E2, AIO_CLIENT_UNSPECIFIED, E3)
>     |
>     -aio_set_fd_handler(E1, E2, E3, E4, E5)
>     +aio_set_fd_handler(E1, E2, AIO_CLIENT_UNSPECIFIED, E3, E4, E5)
>     )
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  aio-posix.c                     |  4 ++-
>  aio-win32.c                     |  2 ++
>  async.c                         |  3 ++-
>  block/curl.c                    | 14 +++++-----
>  block/iscsi.c                   |  9 +++----
>  block/linux-aio.c               |  5 ++--
>  block/nbd-client.c              | 10 ++++---
>  block/nfs.c                     | 17 +++++-------
>  block/sheepdog.c                | 32 +++++++++++++++--------
>  block/ssh.c                     |  5 ++--
>  block/win32-aio.c               |  5 ++--
>  hw/block/dataplane/virtio-blk.c |  6 +++--
>  hw/scsi/virtio-scsi-dataplane.c | 24 +++++++++++------
>  include/block/aio.h             |  5 ++++
>  nbd.c                           |  4 ++-
>  tests/test-aio.c                | 58 +++++++++++++++++++++++------------------
>  16 files changed, 122 insertions(+), 81 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH v2 02/11] aio: Save type to AioHandler

[Fam Zheng]

So it can be used by aio_poll later.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 aio-posix.c | 2 ++
 aio-win32.c | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/aio-posix.c b/aio-posix.c
index 56f2bce..d25fcfc 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -25,6 +25,7 @@ struct AioHandler
     IOHandler *io_write;
     int deleted;
     void *opaque;
+    int type;
     QLIST_ENTRY(AioHandler) node;
 };
 
@@ -83,6 +84,7 @@ void aio_set_fd_handler(AioContext *ctx,
         node->io_read = io_read;
         node->io_write = io_write;
         node->opaque = opaque;
+        node->type = type;
 
         node->pfd.events = (io_read ? G_IO_IN | G_IO_HUP | G_IO_ERR : 0);
         node->pfd.events |= (io_write ? G_IO_OUT | G_IO_ERR : 0);
diff --git a/aio-win32.c b/aio-win32.c
index 90e7a4b..f5ecf57 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -28,6 +28,7 @@ struct AioHandler {
     GPollFD pfd;
     int deleted;
     void *opaque;
+    int type;
     QLIST_ENTRY(AioHandler) node;
 };
 
@@ -87,6 +88,7 @@ void aio_set_fd_handler(AioContext *ctx,
         node->opaque = opaque;
         node->io_read = io_read;
         node->io_write = io_write;
+        node->type = type;
 
         event = event_notifier_get_handle(&ctx->notifier);
         WSAEventSelect(node->pfd.fd, event,
@@ -135,6 +137,7 @@ void aio_set_event_notifier(AioContext *ctx,
             node->e = e;
             node->pfd.fd = (uintptr_t)event_notifier_get_handle(e);
             node->pfd.events = G_IO_IN;
+            node->type = type;
             QLIST_INSERT_HEAD(&ctx->aio_handlers, node, node);
 
             g_source_add_poll(&ctx->source, &node->pfd);
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 02/11] aio: Save type to AioHandler

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:05PM +0800, Fam Zheng wrote:
> So it can be used by aio_poll later.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  aio-posix.c | 2 ++
>  aio-win32.c | 3 +++
>  2 files changed, 5 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH v2 03/11] block: Mark fd handlers as "protocol"

[Fam Zheng]

The "protocol" type includes all the fd handlers and event notifiers used by
block layer, especially those that should be polled in a nested event loop.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/curl.c        |  8 ++++----
 block/iscsi.c       |  4 ++--
 block/linux-aio.c   |  4 ++--
 block/nbd-client.c  |  8 ++++----
 block/nfs.c         |  6 +++---
 block/sheepdog.c    | 22 +++++++++++-----------
 block/ssh.c         |  4 ++--
 block/win32-aio.c   |  4 ++--
 include/block/aio.h |  1 +
 9 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 6925672..75d237c 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -154,19 +154,19 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, fd);
     switch (action) {
         case CURL_POLL_IN:
-            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_PROTOCOL,
                                curl_multi_read, NULL, state);
             break;
         case CURL_POLL_OUT:
-            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_PROTOCOL,
                                NULL, curl_multi_do, state);
             break;
         case CURL_POLL_INOUT:
-            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_PROTOCOL,
                                curl_multi_read, curl_multi_do, state);
             break;
         case CURL_POLL_REMOVE:
-            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+            aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_PROTOCOL,
                                NULL, NULL, NULL);
             break;
     }
diff --git a/block/iscsi.c b/block/iscsi.c
index 0ee1295..1713625 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -292,7 +292,7 @@ iscsi_set_events(IscsiLun *iscsilun)
 
     if (ev != iscsilun->events) {
         aio_set_fd_handler(iscsilun->aio_context, iscsi_get_fd(iscsi),
-                           AIO_CLIENT_UNSPECIFIED,
+                           AIO_CLIENT_PROTOCOL,
                            (ev & POLLIN) ? iscsi_process_read : NULL,
                            (ev & POLLOUT) ? iscsi_process_write : NULL,
                            iscsilun);
@@ -1277,7 +1277,7 @@ static void iscsi_detach_aio_context(BlockDriverState *bs)
     IscsiLun *iscsilun = bs->opaque;
 
     aio_set_fd_handler(iscsilun->aio_context, iscsi_get_fd(iscsilun->iscsi),
-                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
+                       AIO_CLIENT_PROTOCOL, NULL, NULL, NULL);
     iscsilun->events = 0;
 
     if (iscsilun->nop_timer) {
diff --git a/block/linux-aio.c b/block/linux-aio.c
index 0921bde..1491684 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -287,7 +287,7 @@ void laio_detach_aio_context(void *s_, AioContext *old_context)
 {
     struct qemu_laio_state *s = s_;
 
-    aio_set_event_notifier(old_context, &s->e, AIO_CLIENT_UNSPECIFIED, NULL);
+    aio_set_event_notifier(old_context, &s->e, AIO_CLIENT_PROTOCOL, NULL);
     qemu_bh_delete(s->completion_bh);
 }
 
@@ -296,7 +296,7 @@ void laio_attach_aio_context(void *s_, AioContext *new_context)
     struct qemu_laio_state *s = s_;
 
     s->completion_bh = aio_bh_new(new_context, qemu_laio_completion_bh, s);
-    aio_set_event_notifier(new_context, &s->e, AIO_CLIENT_UNSPECIFIED,
+    aio_set_event_notifier(new_context, &s->e, AIO_CLIENT_PROTOCOL,
                            qemu_laio_completion_cb);
 }
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 36c46c5..edf2199 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -124,7 +124,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
     s->send_coroutine = qemu_coroutine_self();
     aio_context = bdrv_get_aio_context(bs);
 
-    aio_set_fd_handler(aio_context, s->sock, AIO_CLIENT_UNSPECIFIED,
+    aio_set_fd_handler(aio_context, s->sock, AIO_CLIENT_PROTOCOL,
                        nbd_reply_ready, nbd_restart_write, bs);
     if (qiov) {
         if (!s->is_unix) {
@@ -144,7 +144,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
     } else {
         rc = nbd_send_request(s->sock, request);
     }
-    aio_set_fd_handler(aio_context, s->sock, AIO_CLIENT_UNSPECIFIED,
+    aio_set_fd_handler(aio_context, s->sock, AIO_CLIENT_PROTOCOL,
                        nbd_reply_ready, NULL, bs);
     s->send_coroutine = NULL;
     qemu_co_mutex_unlock(&s->send_mutex);
@@ -350,14 +350,14 @@ void nbd_client_detach_aio_context(BlockDriverState *bs)
 {
     aio_set_fd_handler(bdrv_get_aio_context(bs),
                        nbd_get_client_session(bs)->sock,
-                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
+                       AIO_CLIENT_PROTOCOL, NULL, NULL, NULL);
 }
 
 void nbd_client_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context)
 {
     aio_set_fd_handler(new_context, nbd_get_client_session(bs)->sock,
-                       AIO_CLIENT_UNSPECIFIED, nbd_reply_ready, NULL, bs);
+                       AIO_CLIENT_PROTOCOL, nbd_reply_ready, NULL, bs);
 }
 
 void nbd_client_close(BlockDriverState *bs)
diff --git a/block/nfs.c b/block/nfs.c
index a21dd6f..4d12067 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -63,7 +63,7 @@ static void nfs_set_events(NFSClient *client)
     int ev = nfs_which_events(client->context);
     if (ev != client->events) {
         aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
-                           AIO_CLIENT_UNSPECIFIED,
+                           AIO_CLIENT_PROTOCOL,
                            (ev & POLLIN) ? nfs_process_read : NULL,
                            (ev & POLLOUT) ? nfs_process_write : NULL, client);
 
@@ -241,7 +241,7 @@ static void nfs_detach_aio_context(BlockDriverState *bs)
     NFSClient *client = bs->opaque;
 
     aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
-                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
+                       AIO_CLIENT_PROTOCOL, NULL, NULL, NULL);
     client->events = 0;
 }
 
@@ -261,7 +261,7 @@ static void nfs_client_close(NFSClient *client)
             nfs_close(client->context, client->fh);
         }
         aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
-                           AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
+                           AIO_CLIENT_PROTOCOL, NULL, NULL, NULL);
         nfs_destroy_context(client->context);
     }
     memset(client, 0, sizeof(NFSClient));
diff --git a/block/sheepdog.c b/block/sheepdog.c
index e0552b7..de9f8be 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -624,7 +624,7 @@ static coroutine_fn void do_co_req(void *opaque)
     unsigned int *rlen = srco->rlen;
 
     co = qemu_coroutine_self();
-    aio_set_fd_handler(srco->aio_context, sockfd, AIO_CLIENT_UNSPECIFIED,
+    aio_set_fd_handler(srco->aio_context, sockfd, AIO_CLIENT_PROTOCOL,
                        NULL, restart_co_req, co);
 
     ret = send_co_req(sockfd, hdr, data, wlen);
@@ -632,7 +632,7 @@ static coroutine_fn void do_co_req(void *opaque)
         goto out;
     }
 
-    aio_set_fd_handler(srco->aio_context, sockfd, AIO_CLIENT_UNSPECIFIED,
+    aio_set_fd_handler(srco->aio_context, sockfd, AIO_CLIENT_PROTOCOL,
                        restart_co_req, NULL, co);
 
     ret = qemu_co_recv(sockfd, hdr, sizeof(*hdr));
@@ -658,7 +658,7 @@ static coroutine_fn void do_co_req(void *opaque)
 out:
     /* there is at most one request for this sockfd, so it is safe to
      * set each handler to NULL. */
-    aio_set_fd_handler(srco->aio_context, sockfd, AIO_CLIENT_UNSPECIFIED,
+    aio_set_fd_handler(srco->aio_context, sockfd, AIO_CLIENT_PROTOCOL,
                        NULL, NULL, NULL);
 
     srco->ret = ret;
@@ -743,7 +743,7 @@ static coroutine_fn void reconnect_to_sdog(void *opaque)
     BDRVSheepdogState *s = opaque;
     AIOReq *aio_req, *next;
 
-    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_UNSPECIFIED, NULL,
+    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_PROTOCOL, NULL,
                        NULL, NULL);
     close(s->fd);
     s->fd = -1;
@@ -957,7 +957,7 @@ static int get_sheep_fd(BDRVSheepdogState *s, Error **errp)
         return fd;
     }
 
-    aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_UNSPECIFIED,
+    aio_set_fd_handler(s->aio_context, fd, AIO_CLIENT_PROTOCOL,
                        co_read_response, NULL, s);
     return fd;
 }
@@ -1213,7 +1213,7 @@ static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
 
     qemu_co_mutex_lock(&s->lock);
     s->co_send = qemu_coroutine_self();
-    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_UNSPECIFIED,
+    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_PROTOCOL,
                        co_read_response, co_write_request, s);
     socket_set_cork(s->fd, 1);
 
@@ -1232,7 +1232,7 @@ static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
     }
 out:
     socket_set_cork(s->fd, 0);
-    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_UNSPECIFIED,
+    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_PROTOCOL,
                        co_read_response, NULL, s);
     s->co_send = NULL;
     qemu_co_mutex_unlock(&s->lock);
@@ -1411,7 +1411,7 @@ static void sd_detach_aio_context(BlockDriverState *bs)
 {
     BDRVSheepdogState *s = bs->opaque;
 
-    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_UNSPECIFIED, NULL,
+    aio_set_fd_handler(s->aio_context, s->fd, AIO_CLIENT_PROTOCOL, NULL,
                        NULL, NULL);
 }
 
@@ -1421,7 +1421,7 @@ static void sd_attach_aio_context(BlockDriverState *bs,
     BDRVSheepdogState *s = bs->opaque;
 
     s->aio_context = new_context;
-    aio_set_fd_handler(new_context, s->fd, AIO_CLIENT_UNSPECIFIED,
+    aio_set_fd_handler(new_context, s->fd, AIO_CLIENT_PROTOCOL,
                        co_read_response, NULL, s);
 }
 
@@ -1537,7 +1537,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     return 0;
 out:
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
-                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
+                       AIO_CLIENT_PROTOCOL, NULL, NULL, NULL);
     if (s->fd >= 0) {
         closesocket(s->fd);
     }
@@ -1922,7 +1922,7 @@ static void sd_close(BlockDriverState *bs)
     }
 
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
-                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
+                       AIO_CLIENT_PROTOCOL, NULL, NULL, NULL);
     closesocket(s->fd);
     g_free(s->host_spec);
 }
diff --git a/block/ssh.c b/block/ssh.c
index 71d7ffe..3e721d2 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -803,7 +803,7 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s, BlockDriverState *bs)
             rd_handler, wr_handler);
 
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->sock,
-                       AIO_CLIENT_UNSPECIFIED, rd_handler, wr_handler, co);
+                       AIO_CLIENT_PROTOCOL, rd_handler, wr_handler, co);
 }
 
 static coroutine_fn void clear_fd_handler(BDRVSSHState *s,
@@ -811,7 +811,7 @@ static coroutine_fn void clear_fd_handler(BDRVSSHState *s,
 {
     DPRINTF("s->sock=%d", s->sock);
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->sock,
-                       AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
+                       AIO_CLIENT_PROTOCOL, NULL, NULL, NULL);
 }
 
 /* A non-blocking call returned EAGAIN, so yield, ensuring the
diff --git a/block/win32-aio.c b/block/win32-aio.c
index 0081886..57b1916 100644
--- a/block/win32-aio.c
+++ b/block/win32-aio.c
@@ -174,7 +174,7 @@ int win32_aio_attach(QEMUWin32AIOState *aio, HANDLE hfile)
 void win32_aio_detach_aio_context(QEMUWin32AIOState *aio,
                                   AioContext *old_context)
 {
-    aio_set_event_notifier(old_context, &aio->e, AIO_CLIENT_UNSPECIFIED, NULL);
+    aio_set_event_notifier(old_context, &aio->e, AIO_CLIENT_PROTOCOL, NULL);
     aio->is_aio_context_attached = false;
 }
 
@@ -182,7 +182,7 @@ void win32_aio_attach_aio_context(QEMUWin32AIOState *aio,
                                   AioContext *new_context)
 {
     aio->is_aio_context_attached = true;
-    aio_set_event_notifier(new_context, &aio->e, AIO_CLIENT_UNSPECIFIED,
+    aio_set_event_notifier(new_context, &aio->e, AIO_CLIENT_PROTOCOL,
                            win32_aio_completion_cb);
 }
 
diff --git a/include/block/aio.h b/include/block/aio.h
index bd1d44b..d02ddfa 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -273,6 +273,7 @@ bool aio_pending(AioContext *ctx);
 bool aio_dispatch(AioContext *ctx);
 
 #define AIO_CLIENT_UNSPECIFIED    (1 << 0)
+#define AIO_CLIENT_PROTOCOL       (1 << 1)
 #define AIO_CLIENT_MASK_ALL       -1
 
 /* Progress in completing AIO work to occur.  This can issue new pending
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 03/11] block: Mark fd handlers as "protocol"

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:06PM +0800, Fam Zheng wrote:
> diff --git a/include/block/aio.h b/include/block/aio.h
> index bd1d44b..d02ddfa 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -273,6 +273,7 @@ bool aio_pending(AioContext *ctx);
>  bool aio_dispatch(AioContext *ctx);
>  
>  #define AIO_CLIENT_UNSPECIFIED    (1 << 0)
> +#define AIO_CLIENT_PROTOCOL       (1 << 1)

The semantics need to be documented here, not just in the commit
description.

AIO_CLIENT_BLOCK_DRIVER or AIO_CLIENT_BLOCK_PROTOCOL seems like a
clearer name to me since "protocol" just by itself could mean many
things.

Re: [Qemu-devel] [Qemu-block] [PATCH v2 03/11] block: Mark fd handlers as "protocol"

[Fam Zheng]

On Thu, 08/27 14:53, Stefan Hajnoczi wrote:
> On Wed, Jul 29, 2015 at 12:42:06PM +0800, Fam Zheng wrote:
> > diff --git a/include/block/aio.h b/include/block/aio.h
> > index bd1d44b..d02ddfa 100644
> > --- a/include/block/aio.h
> > +++ b/include/block/aio.h
> > @@ -273,6 +273,7 @@ bool aio_pending(AioContext *ctx);
> >  bool aio_dispatch(AioContext *ctx);
> >  
> >  #define AIO_CLIENT_UNSPECIFIED    (1 << 0)
> > +#define AIO_CLIENT_PROTOCOL       (1 << 1)
> 
> The semantics need to be documented here, not just in the commit
> description.
> 
> AIO_CLIENT_BLOCK_DRIVER or AIO_CLIENT_BLOCK_PROTOCOL seems like a
> clearer name to me since "protocol" just by itself could mean many
> things.

OK, renaming to AIO_CLIENT_BLOCK_DRIVER.

Thanks,
Fam

[Qemu-devel] [PATCH v2 04/11] nbd: Mark fd handlers client type as "nbd server"

[Fam Zheng]

So we could distinguish it from "protocol" to avoid handling in nested aio
polls.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/block/aio.h | 1 +
 nbd.c               | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/block/aio.h b/include/block/aio.h
index d02ddfa..8b15105 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -274,6 +274,7 @@ bool aio_dispatch(AioContext *ctx);
 
 #define AIO_CLIENT_UNSPECIFIED    (1 << 0)
 #define AIO_CLIENT_PROTOCOL       (1 << 1)
+#define AIO_CLIENT_NBD_SERVER     (1 << 2)
 #define AIO_CLIENT_MASK_ALL       -1
 
 /* Progress in completing AIO work to occur.  This can issue new pending
diff --git a/nbd.c b/nbd.c
index 64ed91b..7b38334 100644
--- a/nbd.c
+++ b/nbd.c
@@ -1437,7 +1437,7 @@ static void nbd_set_handlers(NBDClient *client)
 {
     if (client->exp && client->exp->ctx) {
         aio_set_fd_handler(client->exp->ctx, client->sock,
-                           AIO_CLIENT_UNSPECIFIED,
+                           AIO_CLIENT_NBD_SERVER,
                            client->can_read ? nbd_read : NULL,
                            client->send_coroutine ? nbd_restart_write : NULL,
                            client);
@@ -1448,7 +1448,7 @@ static void nbd_unset_handlers(NBDClient *client)
 {
     if (client->exp && client->exp->ctx) {
         aio_set_fd_handler(client->exp->ctx, client->sock,
-                           AIO_CLIENT_UNSPECIFIED, NULL, NULL, NULL);
+                           AIO_CLIENT_NBD_SERVER, NULL, NULL, NULL);
     }
 }
 
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 04/11] nbd: Mark fd handlers client type as "nbd server"

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:07PM +0800, Fam Zheng wrote:
> So we could distinguish it from "protocol" to avoid handling in nested aio
> polls.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/block/aio.h | 1 +
>  nbd.c               | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH v2 05/11] aio: Mark ctx->notifier's client type as "context"

[Fam Zheng]

It is important to include this for any blocking poll, on the other hand it is
also OK to exclude it otherwise.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 async.c             | 4 ++--
 include/block/aio.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/async.c b/async.c
index 43f9425..0bea602 100644
--- a/async.c
+++ b/async.c
@@ -231,7 +231,7 @@ aio_ctx_finalize(GSource     *source)
     AioContext *ctx = (AioContext *) source;
 
     thread_pool_free(ctx->thread_pool);
-    aio_set_event_notifier(ctx, &ctx->notifier, AIO_CLIENT_UNSPECIFIED, NULL);
+    aio_set_event_notifier(ctx, &ctx->notifier, AIO_CLIENT_CONTEXT, NULL);
     event_notifier_cleanup(&ctx->notifier);
     rfifolock_destroy(&ctx->lock);
     qemu_mutex_destroy(&ctx->bh_lock);
@@ -306,7 +306,7 @@ AioContext *aio_context_new(Error **errp)
     }
     g_source_set_can_recurse(&ctx->source, true);
     aio_set_event_notifier(ctx, &ctx->notifier,
-                           AIO_CLIENT_UNSPECIFIED,
+                           AIO_CLIENT_CONTEXT,
                            (EventNotifierHandler *)
                            event_notifier_dummy_cb);
     ctx->thread_pool = NULL;
diff --git a/include/block/aio.h b/include/block/aio.h
index 8b15105..b0b2345 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -275,6 +275,7 @@ bool aio_dispatch(AioContext *ctx);
 #define AIO_CLIENT_UNSPECIFIED    (1 << 0)
 #define AIO_CLIENT_PROTOCOL       (1 << 1)
 #define AIO_CLIENT_NBD_SERVER     (1 << 2)
+#define AIO_CLIENT_CONTEXT        (1 << 3)
 #define AIO_CLIENT_MASK_ALL       -1
 
 /* Progress in completing AIO work to occur.  This can issue new pending
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 05/11] aio: Mark ctx->notifier's client type as "context"

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:08PM +0800, Fam Zheng wrote:
> It is important to include this for any blocking poll, on the other hand it is
> also OK to exclude it otherwise.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  async.c             | 4 ++--
>  include/block/aio.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH v2 06/11] dataplane: Mark host notifiers' client type as "dataplane"

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c |  4 ++--
 hw/scsi/virtio-scsi-dataplane.c | 16 ++++++++--------
 include/block/aio.h             |  1 +
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index e472154..5419f1c 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -283,7 +283,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
     /* Get this show started by hooking up our callbacks */
     aio_context_acquire(s->ctx);
-    aio_set_event_notifier(s->ctx, &s->host_notifier, AIO_CLIENT_UNSPECIFIED,
+    aio_set_event_notifier(s->ctx, &s->host_notifier, AIO_CLIENT_DATAPLANE,
                            handle_notify);
     aio_context_release(s->ctx);
     return;
@@ -320,7 +320,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     aio_context_acquire(s->ctx);
 
     /* Stop notifications for new requests from guest */
-    aio_set_event_notifier(s->ctx, &s->host_notifier, AIO_CLIENT_UNSPECIFIED,
+    aio_set_event_notifier(s->ctx, &s->host_notifier, AIO_CLIENT_DATAPLANE,
                            NULL);
 
     /* Drain and switch bs back to the QEMU main loop */
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index f7bab09..55c2524 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -60,7 +60,7 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
     r = g_slice_new(VirtIOSCSIVring);
     r->host_notifier = *virtio_queue_get_host_notifier(vq);
     r->guest_notifier = *virtio_queue_get_guest_notifier(vq);
-    aio_set_event_notifier(s->ctx, &r->host_notifier, AIO_CLIENT_UNSPECIFIED,
+    aio_set_event_notifier(s->ctx, &r->host_notifier, AIO_CLIENT_DATAPLANE,
                            handler);
 
     r->parent = s;
@@ -72,7 +72,7 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
     return r;
 
 fail_vring:
-    aio_set_event_notifier(s->ctx, &r->host_notifier, AIO_CLIENT_UNSPECIFIED,
+    aio_set_event_notifier(s->ctx, &r->host_notifier, AIO_CLIENT_DATAPLANE,
                            NULL);
     k->set_host_notifier(qbus->parent, n, false);
     g_slice_free(VirtIOSCSIVring, r);
@@ -165,16 +165,16 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
 
     if (s->ctrl_vring) {
         aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
-                               AIO_CLIENT_UNSPECIFIED, NULL);
+                               AIO_CLIENT_DATAPLANE, NULL);
     }
     if (s->event_vring) {
         aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
-                               AIO_CLIENT_UNSPECIFIED, NULL);
+                               AIO_CLIENT_DATAPLANE, NULL);
     }
     if (s->cmd_vrings) {
         for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
             aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
-                                   AIO_CLIENT_UNSPECIFIED, NULL);
+                                   AIO_CLIENT_DATAPLANE, NULL);
         }
     }
 }
@@ -296,12 +296,12 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
     aio_context_acquire(s->ctx);
 
     aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
-                           AIO_CLIENT_UNSPECIFIED, NULL);
+                           AIO_CLIENT_DATAPLANE, NULL);
     aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
-                           AIO_CLIENT_UNSPECIFIED, NULL);
+                           AIO_CLIENT_DATAPLANE, NULL);
     for (i = 0; i < vs->conf.num_queues; i++) {
         aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
-                               AIO_CLIENT_UNSPECIFIED, NULL);
+                               AIO_CLIENT_DATAPLANE, NULL);
     }
 
     blk_drain_all(); /* ensure there are no in-flight requests */
diff --git a/include/block/aio.h b/include/block/aio.h
index b0b2345..9541bdd 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -276,6 +276,7 @@ bool aio_dispatch(AioContext *ctx);
 #define AIO_CLIENT_PROTOCOL       (1 << 1)
 #define AIO_CLIENT_NBD_SERVER     (1 << 2)
 #define AIO_CLIENT_CONTEXT        (1 << 3)
+#define AIO_CLIENT_DATAPLANE      (1 << 4)
 #define AIO_CLIENT_MASK_ALL       -1
 
 /* Progress in completing AIO work to occur.  This can issue new pending
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 06/11] dataplane: Mark host notifiers' client type as "dataplane"

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:09PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c |  4 ++--
>  hw/scsi/virtio-scsi-dataplane.c | 16 ++++++++--------
>  include/block/aio.h             |  1 +
>  3 files changed, 11 insertions(+), 10 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH v2 07/11] aio-posix: introduce aio_{disable, enable}_clients

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 aio-posix.c         | 17 ++++++++++++++++-
 include/block/aio.h | 24 ++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/aio-posix.c b/aio-posix.c
index d25fcfc..840b769 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -26,6 +26,7 @@ struct AioHandler
     int deleted;
     void *opaque;
     int type;
+    int disable_cnt;
     QLIST_ENTRY(AioHandler) node;
 };
 
@@ -261,7 +262,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
 
     /* fill pollfds */
     QLIST_FOREACH(node, &ctx->aio_handlers, node) {
-        if (!node->deleted && node->pfd.events) {
+        if (!node->deleted && node->pfd.events && !node->disable_cnt) {
             add_pollfd(node);
         }
     }
@@ -301,3 +302,17 @@ bool aio_poll(AioContext *ctx, bool blocking)
 
     return progress;
 }
+
+void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
+                                bool is_disable)
+{
+    AioHandler *node;
+    aio_context_acquire(ctx);
+
+    QLIST_FOREACH(node, &ctx->aio_handlers, node) {
+        if (!node->deleted && node->type & clients_mask) {
+            node->disable_cnt += is_disable ? 1 : -1;
+        }
+    }
+    aio_context_release(ctx);
+}
diff --git a/include/block/aio.h b/include/block/aio.h
index 9541bdd..fb70cc5 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -379,4 +379,28 @@ static inline void aio_timer_init(AioContext *ctx,
  */
 int64_t aio_compute_timeout(AioContext *ctx);
 
+void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
+                                bool is_disable);
+/**
+ * aio_disable_clients:
+ * @ctx: the aio context
+ *
+ * Disable the furthur processing by aio_poll(ctx) of clients.
+ */
+static inline void aio_disable_clients(AioContext *ctx, int clients_mask)
+{
+    aio_disable_enable_clients(ctx, clients_mask, true);
+}
+
+/**
+ * aio_enable_clients:
+ * @ctx: the aio context
+ *
+ * Enable the processing of the clients.
+ */
+static inline void aio_enable_clients(AioContext *ctx, int clients_mask)
+{
+    aio_disable_enable_clients(ctx, clients_mask, false);
+}
+
 #endif
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 07/11] aio-posix: introduce aio_{disable, enable}_clients

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:10PM +0800, Fam Zheng wrote:
> +void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
> +                                bool is_disable)
> +{
> +    AioHandler *node;
> +    aio_context_acquire(ctx);
> +
> +    QLIST_FOREACH(node, &ctx->aio_handlers, node) {
> +        if (!node->deleted && node->type & clients_mask) {
> +            node->disable_cnt += is_disable ? 1 : -1;
> +        }
> +    }
> +    aio_context_release(ctx);
> +}

If someone adds an fd of a disabled type *after* the call to
aio_disable_clients() then it won't be disabled.

Another approach is to keep an array of counters per AioContext and
check the counters during aio_poll() when deciding which fds to monitor.

Also, this function acquires/releases AioContext so it's worth
mentioning in the doc comments that this function is thread-safe.

Re: [Qemu-devel] [PATCH v2 07/11] aio-posix: introduce aio_{disable, enable}_clients

[Fam Zheng]

On Thu, 08/27 18:23, Stefan Hajnoczi wrote:
> On Wed, Jul 29, 2015 at 12:42:10PM +0800, Fam Zheng wrote:
> > +void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
> > +                                bool is_disable)
> > +{
> > +    AioHandler *node;
> > +    aio_context_acquire(ctx);
> > +
> > +    QLIST_FOREACH(node, &ctx->aio_handlers, node) {
> > +        if (!node->deleted && node->type & clients_mask) {
> > +            node->disable_cnt += is_disable ? 1 : -1;
> > +        }
> > +    }
> > +    aio_context_release(ctx);
> > +}
> 
> If someone adds an fd of a disabled type *after* the call to
> aio_disable_clients() then it won't be disabled.
> 
> Another approach is to keep an array of counters per AioContext and
> check the counters during aio_poll() when deciding which fds to monitor.

Good idea, I'll change that way.

> 
> Also, this function acquires/releases AioContext so it's worth
> mentioning in the doc comments that this function is thread-safe.
> 

OK.

Fam

[Qemu-devel] [PATCH v2 08/11] aio-win32: Implement aio_{disable, enable}_clients

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 aio-win32.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/aio-win32.c b/aio-win32.c
index f5ecf57..1f6a3f0 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -28,6 +28,7 @@ struct AioHandler {
     GPollFD pfd;
     int deleted;
     void *opaque;
+    int disable_cnt;
     int type;
     QLIST_ENTRY(AioHandler) node;
 };
@@ -309,7 +310,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
     /* fill fd sets */
     count = 0;
     QLIST_FOREACH(node, &ctx->aio_handlers, node) {
-        if (!node->deleted && node->io_notify) {
+        if (!node->deleted && node->io_notify && !node->disable_cnt) {
             events[count++] = event_notifier_get_handle(node->e);
         }
     }
@@ -368,3 +369,17 @@ bool aio_poll(AioContext *ctx, bool blocking)
     aio_context_release(ctx);
     return progress;
 }
+
+void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
+                                bool is_disable)
+{
+    AioHandler *node;
+    aio_context_acquire(ctx);
+
+    QLIST_FOREACH(node, &ctx->aio_handlers, node) {
+        if (!node->deleted && node->type & clients_mask) {
+            node->disable_cnt += is_disable ? 1 : -1;
+        }
+    }
+    aio_context_release(ctx);
+}
-- 
2.4.3

[Qemu-devel] [PATCH v2 09/11] block: Introduce bdrv_aio_poll

[Fam Zheng]

This call is introduced simply as a wrapper of aio_poll, but it makes it
is easy to change the polled client types.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c            | 8 ++++++++
 include/block/aio.h   | 2 +-
 include/block/block.h | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index d4bc83b..aca5dff 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2608,3 +2608,11 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
     }
     bdrv_start_throttled_reqs(bs);
 }
+
+bool bdrv_aio_poll(AioContext *ctx, bool blocking)
+{
+    bool ret;
+
+    ret = aio_poll(ctx, blocking);
+    return ret;
+}
diff --git a/include/block/aio.h b/include/block/aio.h
index fb70cc5..53fc400 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -385,7 +385,7 @@ void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
  * aio_disable_clients:
  * @ctx: the aio context
  *
- * Disable the furthur processing by aio_poll(ctx) of clients.
+ * Disable the processing of clients by further aio_poll(ctx).
  */
 static inline void aio_disable_clients(AioContext *ctx, int clients_mask)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 37916f7..be99e6d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -616,4 +616,6 @@ void bdrv_flush_io_queue(BlockDriverState *bs);
 
 BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
 
+bool bdrv_aio_poll(AioContext *ctx, bool blocking);
+
 #endif
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 09/11] block: Introduce bdrv_aio_poll

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:12PM +0800, Fam Zheng wrote:
> This call is introduced simply as a wrapper of aio_poll, but it makes it
> is easy to change the polled client types.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c            | 8 ++++++++
>  include/block/aio.h   | 2 +-
>  include/block/block.h | 2 ++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/block/io.c b/block/io.c
> index d4bc83b..aca5dff 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2608,3 +2608,11 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
>      }
>      bdrv_start_throttled_reqs(bs);
>  }
> +
> +bool bdrv_aio_poll(AioContext *ctx, bool blocking)
> +{
> +    bool ret;
> +
> +    ret = aio_poll(ctx, blocking);
> +    return ret;
> +}
> diff --git a/include/block/aio.h b/include/block/aio.h
> index fb70cc5..53fc400 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -385,7 +385,7 @@ void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
>   * aio_disable_clients:
>   * @ctx: the aio context
>   *
> - * Disable the furthur processing by aio_poll(ctx) of clients.
> + * Disable the processing of clients by further aio_poll(ctx).

Should this be squashed into an earlier patch?

Re: [Qemu-devel] [Qemu-block] [PATCH v2 09/11] block: Introduce bdrv_aio_poll

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:12PM +0800, Fam Zheng wrote:
> +bool bdrv_aio_poll(AioContext *ctx, bool blocking)
> +{
> +    bool ret;
> +
> +    ret = aio_poll(ctx, blocking);
> +    return ret;
> +}

This function would fit into bdrv_*() APIs better if the first argument
was BlockDriverState *bs instead of an AioContext.

[Qemu-devel] [PATCH v2 10/11] block: Replace nested aio_poll with bdrv_aio_poll

[Fam Zheng]

Just a manual search and replace. No semantic change here.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c           |  2 +-
 block/curl.c      |  2 +-
 block/io.c        | 18 +++++++++---------
 block/nfs.c       |  2 +-
 block/qed-table.c |  8 ++++----
 blockjob.c        |  2 +-
 qemu-img.c        |  2 +-
 qemu-io-cmds.c    |  4 ++--
 8 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/block.c b/block.c
index d088ee0..564c43b 100644
--- a/block.c
+++ b/block.c
@@ -371,7 +371,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
         co = qemu_coroutine_create(bdrv_create_co_entry);
         qemu_coroutine_enter(co, &cco);
         while (cco.ret == NOT_DONE) {
-            aio_poll(qemu_get_aio_context(), true);
+            bdrv_aio_poll(qemu_get_aio_context(), true);
         }
     }
 
diff --git a/block/curl.c b/block/curl.c
index 75d237c..2223091 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -392,7 +392,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
             break;
         }
         if (!state) {
-            aio_poll(bdrv_get_aio_context(bs), true);
+            bdrv_aio_poll(bdrv_get_aio_context(bs), true);
         }
     } while(!state);
 
diff --git a/block/io.c b/block/io.c
index aca5dff..3d255e0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -251,7 +251,7 @@ void bdrv_drain(BlockDriverState *bs)
         /* Keep iterating */
          bdrv_flush_io_queue(bs);
          busy = bdrv_requests_pending(bs);
-         busy |= aio_poll(bdrv_get_aio_context(bs), busy);
+         busy |= bdrv_aio_poll(bdrv_get_aio_context(bs), busy);
     }
 }
 
@@ -301,11 +301,11 @@ void bdrv_drain_all(void)
                     bdrv_flush_io_queue(bs);
                     if (bdrv_requests_pending(bs)) {
                         busy = true;
-                        aio_poll(aio_context, busy);
+                        bdrv_aio_poll(aio_context, busy);
                     }
                 }
             }
-            busy |= aio_poll(aio_context, false);
+            busy |= bdrv_aio_poll(aio_context, false);
             aio_context_release(aio_context);
         }
     }
@@ -559,7 +559,7 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset,
         co = qemu_coroutine_create(bdrv_rw_co_entry);
         qemu_coroutine_enter(co, &rwco);
         while (rwco.ret == NOT_DONE) {
-            aio_poll(aio_context, true);
+            bdrv_aio_poll(aio_context, true);
         }
     }
     return rwco.ret;
@@ -1594,7 +1594,7 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
         co = qemu_coroutine_create(bdrv_get_block_status_above_co_entry);
         qemu_coroutine_enter(co, &data);
         while (!data.done) {
-            aio_poll(aio_context, true);
+            bdrv_aio_poll(aio_context, true);
         }
     }
     return data.ret;
@@ -1962,9 +1962,9 @@ void bdrv_aio_cancel(BlockAIOCB *acb)
     bdrv_aio_cancel_async(acb);
     while (acb->refcnt > 1) {
         if (acb->aiocb_info->get_aio_context) {
-            aio_poll(acb->aiocb_info->get_aio_context(acb), true);
+            bdrv_aio_poll(acb->aiocb_info->get_aio_context(acb), true);
         } else if (acb->bs) {
-            aio_poll(bdrv_get_aio_context(acb->bs), true);
+            bdrv_aio_poll(bdrv_get_aio_context(acb->bs), true);
         } else {
             abort();
         }
@@ -2376,7 +2376,7 @@ int bdrv_flush(BlockDriverState *bs)
         co = qemu_coroutine_create(bdrv_flush_co_entry);
         qemu_coroutine_enter(co, &rwco);
         while (rwco.ret == NOT_DONE) {
-            aio_poll(aio_context, true);
+            bdrv_aio_poll(aio_context, true);
         }
     }
 
@@ -2489,7 +2489,7 @@ int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
         co = qemu_coroutine_create(bdrv_discard_co_entry);
         qemu_coroutine_enter(co, &rwco);
         while (rwco.ret == NOT_DONE) {
-            aio_poll(aio_context, true);
+            bdrv_aio_poll(aio_context, true);
         }
     }
 
diff --git a/block/nfs.c b/block/nfs.c
index 4d12067..6740661 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -469,7 +469,7 @@ static int64_t nfs_get_allocated_file_size(BlockDriverState *bs)
 
     while (!task.complete) {
         nfs_set_events(client);
-        aio_poll(client->aio_context, true);
+        bdrv_aio_poll(client->aio_context, true);
     }
 
     return (task.ret < 0 ? task.ret : st.st_blocks * st.st_blksize);
diff --git a/block/qed-table.c b/block/qed-table.c
index 513aa87..6421ce7 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -173,7 +173,7 @@ int qed_read_l1_table_sync(BDRVQEDState *s)
     qed_read_table(s, s->header.l1_table_offset,
                    s->l1_table, qed_sync_cb, &ret);
     while (ret == -EINPROGRESS) {
-        aio_poll(bdrv_get_aio_context(s->bs), true);
+        bdrv_aio_poll(bdrv_get_aio_context(s->bs), true);
     }
 
     return ret;
@@ -194,7 +194,7 @@ int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index,
 
     qed_write_l1_table(s, index, n, qed_sync_cb, &ret);
     while (ret == -EINPROGRESS) {
-        aio_poll(bdrv_get_aio_context(s->bs), true);
+        bdrv_aio_poll(bdrv_get_aio_context(s->bs), true);
     }
 
     return ret;
@@ -267,7 +267,7 @@ int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset
 
     qed_read_l2_table(s, request, offset, qed_sync_cb, &ret);
     while (ret == -EINPROGRESS) {
-        aio_poll(bdrv_get_aio_context(s->bs), true);
+        bdrv_aio_poll(bdrv_get_aio_context(s->bs), true);
     }
 
     return ret;
@@ -289,7 +289,7 @@ int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request,
 
     qed_write_l2_table(s, request, index, n, flush, qed_sync_cb, &ret);
     while (ret == -EINPROGRESS) {
-        aio_poll(bdrv_get_aio_context(s->bs), true);
+        bdrv_aio_poll(bdrv_get_aio_context(s->bs), true);
     }
 
     return ret;
diff --git a/blockjob.c b/blockjob.c
index 62bb906..4e52652 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -210,7 +210,7 @@ static int block_job_finish_sync(BlockJob *job,
         return -EBUSY;
     }
     while (data.ret == -EINPROGRESS) {
-        aio_poll(bdrv_get_aio_context(bs), true);
+        bdrv_aio_poll(bdrv_get_aio_context(bs), true);
     }
     return (data.cancelled && data.ret == 0) ? -ECANCELED : data.ret;
 }
diff --git a/qemu-img.c b/qemu-img.c
index 75f4ee4..5c0f73b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -654,7 +654,7 @@ static void run_block_job(BlockJob *job, Error **errp)
     AioContext *aio_context = bdrv_get_aio_context(job->bs);
 
     do {
-        aio_poll(aio_context, true);
+        bdrv_aio_poll(aio_context, true);
         qemu_progress_print((float)job->offset / job->len * 100.f, 0);
     } while (!job->ready);
 
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 53477e1..b7c0e64 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -478,7 +478,7 @@ static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int count,
     co = qemu_coroutine_create(co_write_zeroes_entry);
     qemu_coroutine_enter(co, &data);
     while (!data.done) {
-        aio_poll(blk_get_aio_context(blk), true);
+        bdrv_aio_poll(blk_get_aio_context(blk), true);
     }
     if (data.ret < 0) {
         return data.ret;
@@ -2046,7 +2046,7 @@ static const cmdinfo_t resume_cmd = {
 static int wait_break_f(BlockBackend *blk, int argc, char **argv)
 {
     while (!bdrv_debug_is_suspended(blk_bs(blk), argv[1])) {
-        aio_poll(blk_get_aio_context(blk), true);
+        bdrv_aio_poll(blk_get_aio_context(blk), true);
     }
 
     return 0;
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 10/11] block: Replace nested aio_poll with bdrv_aio_poll

[Stefan Hajnoczi]

On Wed, Jul 29, 2015 at 12:42:13PM +0800, Fam Zheng wrote:
> Just a manual search and replace. No semantic change here.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c           |  2 +-
>  block/curl.c      |  2 +-
>  block/io.c        | 18 +++++++++---------
>  block/nfs.c       |  2 +-
>  block/qed-table.c |  8 ++++----
>  blockjob.c        |  2 +-
>  qemu-img.c        |  2 +-
>  qemu-io-cmds.c    |  4 ++--
>  8 files changed, 20 insertions(+), 20 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

So that external events are not processed in nested event loops.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/io.c b/block/io.c
index 3d255e0..84047fe 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2613,6 +2613,8 @@ bool bdrv_aio_poll(AioContext *ctx, bool blocking)
 {
     bool ret;
 
+    aio_disable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
     ret = aio_poll(ctx, blocking);
+    aio_enable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
     return ret;
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 29/07/2015 06:42, Fam Zheng wrote:
> @@ -2613,6 +2613,8 @@ bool bdrv_aio_poll(AioContext *ctx, bool blocking)
>  {
>      bool ret;
>  
> +    aio_disable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
>      ret = aio_poll(ctx, blocking);
> +    aio_enable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
>      return ret;

This is not enough, because another thread's aio_poll can sneak in
between calls to bdrv_aio_poll if the AioContext lock is released, and
they will use the full set of clients.

Similar to your v1, it works with the large critical sections we
currently have, but it has the same problem in the longer term.

Paolo

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 29/07/2015 06:42, Fam Zheng wrote:
> @@ -2613,6 +2613,8 @@ bool bdrv_aio_poll(AioContext *ctx, bool blocking)
>  {
>      bool ret;
>  
> +    aio_disable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
>      ret = aio_poll(ctx, blocking);
> +    aio_enable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
>      return ret;

This is not enough, because another thread's aio_poll can sneak in
between calls to bdrv_aio_poll if the AioContext lock is released, and
they will use the full set of clients.

Similar to your v1, it works with the large critical sections we
currently have, but it has the same problem in the longer term.

The clients have to be disabled around calls to bdrv_drain, as you were
doing when you had pause/resume notifiers.  Patches 1-8 however are ok.

Paolo

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Wed, 07/29 09:37, Paolo Bonzini wrote:
> 
> 
> On 29/07/2015 06:42, Fam Zheng wrote:
> > @@ -2613,6 +2613,8 @@ bool bdrv_aio_poll(AioContext *ctx, bool blocking)
> >  {
> >      bool ret;
> >  
> > +    aio_disable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
> >      ret = aio_poll(ctx, blocking);
> > +    aio_enable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
> >      return ret;
> 
> This is not enough, because another thread's aio_poll can sneak in
> between calls to bdrv_aio_poll if the AioContext lock is released, and
> they will use the full set of clients.
> 
> Similar to your v1, it works with the large critical sections we
> currently have, but it has the same problem in the longer term.

Can we add more disable/enable pairs around bdrv_drain on top?

Fam

> 
> The clients have to be disabled around calls to bdrv_drain, as you were
> doing when you had pause/resume notifiers.  Patches 1-8 however are ok.
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 29/07/2015 12:57, Fam Zheng wrote:
> On Wed, 07/29 09:37, Paolo Bonzini wrote:
>>
>>
>> On 29/07/2015 06:42, Fam Zheng wrote:
>>> @@ -2613,6 +2613,8 @@ bool bdrv_aio_poll(AioContext *ctx, bool blocking)
>>>  {
>>>      bool ret;
>>>  
>>> +    aio_disable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
>>>      ret = aio_poll(ctx, blocking);
>>> +    aio_enable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
>>>      return ret;
>>
>> This is not enough, because another thread's aio_poll can sneak in
>> between calls to bdrv_aio_poll if the AioContext lock is released, and
>> they will use the full set of clients.
>>
>> Similar to your v1, it works with the large critical sections we
>> currently have, but it has the same problem in the longer term.
> 
> Can we add more disable/enable pairs around bdrv_drain on top?

Yes, though I think you'd end up reverting patches 10 and 11 in the end.

All 11 patches are okay, though it would be great if you could post the
full series before this is applied.

Paolo

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Wed, 07/29 13:02, Paolo Bonzini wrote:
> 
> 
> On 29/07/2015 12:57, Fam Zheng wrote:
> > On Wed, 07/29 09:37, Paolo Bonzini wrote:
> >>
> >>
> >> On 29/07/2015 06:42, Fam Zheng wrote:
> >>> @@ -2613,6 +2613,8 @@ bool bdrv_aio_poll(AioContext *ctx, bool blocking)
> >>>  {
> >>>      bool ret;
> >>>  
> >>> +    aio_disable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
> >>>      ret = aio_poll(ctx, blocking);
> >>> +    aio_enable_clients(ctx, AIO_CLIENT_DATAPLANE | AIO_CLIENT_NBD_SERVER);
> >>>      return ret;
> >>
> >> This is not enough, because another thread's aio_poll can sneak in
> >> between calls to bdrv_aio_poll if the AioContext lock is released, and
> >> they will use the full set of clients.
> >>
> >> Similar to your v1, it works with the large critical sections we
> >> currently have, but it has the same problem in the longer term.
> > 
> > Can we add more disable/enable pairs around bdrv_drain on top?
> 
> Yes, though I think you'd end up reverting patches 10 and 11 in the end.

We will add outer disable/enable pairs to prevent another threads's aio_poll
from sneaking in between bdrv_aio_poll calls, but we needn't obsolete
bdrv_aio_poll() because of that - it can be useful by itself. For example
bdrv_aio_cancel shouldn't look at ioeventfd, otherwise it could spin for too
long on high load.  Does that make sense?

Fam

> 
> All 11 patches are okay, though it would be great if you could post the
> full series before this is applied.
> 
> Paolo

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 29/07/2015 13:53, Fam Zheng wrote:
>> > Yes, though I think you'd end up reverting patches 10 and 11 in the end.
> We will add outer disable/enable pairs to prevent another threads's aio_poll
> from sneaking in between bdrv_aio_poll calls, but we needn't obsolete
> bdrv_aio_poll() because of that - it can be useful by itself. For example
> bdrv_aio_cancel shouldn't look at ioeventfd, otherwise it could spin for too
> long on high load.  Does that make sense?

Did you mean bdrv_drain() (when it is not already surrounded by
disable/enable pairs in the caller)?  But yes, that makes sense.

I'm not sure that it makes sense to disable/enable in places such as
bdrv_pread.  The caller's block, if any, should suffice.  In this sense
you'd end up reverting large parts of patch 10.

Then you would have to see how many calls to bdrv_aio_poll are still
there, and how many can be converted with no semantic change to aio_poll
(e.g. there's no difference in qemu-img.c), and you'd end up reverting
patches 9 and 11 too.  But we can look at that later.

Paolo

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Wed, 07/29 14:03, Paolo Bonzini wrote:
> 
> 
> On 29/07/2015 13:53, Fam Zheng wrote:
> >> > Yes, though I think you'd end up reverting patches 10 and 11 in the end.
> > We will add outer disable/enable pairs to prevent another threads's aio_poll
> > from sneaking in between bdrv_aio_poll calls, but we needn't obsolete
> > bdrv_aio_poll() because of that - it can be useful by itself. For example
> > bdrv_aio_cancel shouldn't look at ioeventfd, otherwise it could spin for too
> > long on high load.  Does that make sense?
> 
> Did you mean bdrv_drain() (when it is not already surrounded by
> disable/enable pairs in the caller)?  But yes, that makes sense.
> 
> I'm not sure that it makes sense to disable/enable in places such as
> bdrv_pread.  The caller's block, if any, should suffice.  In this sense
> you'd end up reverting large parts of patch 10.
> 
> Then you would have to see how many calls to bdrv_aio_poll are still
> there, and how many can be converted with no semantic change to aio_poll
> (e.g. there's no difference in qemu-img.c), and you'd end up reverting
> patches 9 and 11 too.  But we can look at that later.
> 

Okay.

There 19 bdrv_aio_poll()'s after this series:

block.c                bdrv_create
block/curl.c           curl_init_state
block/io.c             bdrv_drain
block/io.c             bdrv_drain_all
block/io.c             bdrv_prwv_co
block/io.c             bdrv_get_block_status_above
block/io.c             bdrv_aio_cancel
block/io.c             bdrv_flush
block/io.c             bdrv_discard
block/io.c             bdrv_flush_io_queue
block/nfs.c            nfs_get_allocated_file_size
block/qed-table.c      qed_read_l1_table_sync
block/qed-table.c      qed_write_l1_table_sync
block/qed-table.c      qed_read_l2_table_sync
block/qed-table.c      qed_write_l2_table_sync
blockjob.c             block_job_finish_sync
include/block/block.h  bdrv_get_stats
qemu-img.c             run_block_job
qemu-io-cmds.c         do_co_write_zeroes
qemu-io-cmds.c         wait_break_f

Most of them make some sense to me, but not many make a real difference.  The
most important ones should be bdrv_drain* and bdrv_flush, and can be taken care
of from caller side.

Fam

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 30/07/2015 03:35, Fam Zheng wrote:
> block.c                bdrv_create
> block/curl.c           curl_init_state
> block/io.c             bdrv_drain
> block/io.c             bdrv_drain_all
> block/io.c             bdrv_prwv_co
> block/io.c             bdrv_get_block_status_above
> block/io.c             bdrv_aio_cancel
> block/io.c             bdrv_flush
> block/io.c             bdrv_discard
> block/io.c             bdrv_flush_io_queue
> block/nfs.c            nfs_get_allocated_file_size
> block/qed-table.c      qed_read_l1_table_sync
> block/qed-table.c      qed_write_l1_table_sync
> block/qed-table.c      qed_read_l2_table_sync
> block/qed-table.c      qed_write_l2_table_sync
> blockjob.c             block_job_finish_sync
> include/block/block.h  bdrv_get_stats
> qemu-img.c             run_block_job
> qemu-io-cmds.c         do_co_write_zeroes
> qemu-io-cmds.c         wait_break_f
> 
> Most of them make some sense to me, but not many make a real difference.  The
> most important ones should be bdrv_drain* and bdrv_flush, and can be taken care
> of from caller side.

Even just bdrv_drain*.  bdrv_flush is okay with incoming requests, it
should be handled in the caller side.

Paolo

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Wed, 07/29 14:03, Paolo Bonzini wrote:
> 
> 
> On 29/07/2015 13:53, Fam Zheng wrote:
> >> > Yes, though I think you'd end up reverting patches 10 and 11 in the end.
> > We will add outer disable/enable pairs to prevent another threads's aio_poll
> > from sneaking in between bdrv_aio_poll calls, but we needn't obsolete
> > bdrv_aio_poll() because of that - it can be useful by itself. For example
> > bdrv_aio_cancel shouldn't look at ioeventfd, otherwise it could spin for too
> > long on high load.  Does that make sense?
> 
> Did you mean bdrv_drain() (when it is not already surrounded by
> disable/enable pairs in the caller)?  But yes, that makes sense.
> 
> I'm not sure that it makes sense to disable/enable in places such as
> bdrv_pread.  The caller's block, if any, should suffice.  In this sense
> you'd end up reverting large parts of patch 10.
> 
> Then you would have to see how many calls to bdrv_aio_poll are still
> there, and how many can be converted with no semantic change to aio_poll
> (e.g. there's no difference in qemu-img.c), and you'd end up reverting
> patches 9 and 11 too.  But we can look at that later.

Another advantage for bdrv_aio_poll() is, in main loop we will not need
a separate AioContext in changes like:

http://patchwork.ozlabs.org/patch/514968/

Because nested aio_poll will automatically be limited to only process block
layer events. My idea is to eventually let main loop use aio_poll, which means
we would also move chardev onto it. It would be neat to put all fds of the main
thread into a single AioContext.

Fam

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 09/09/2015 05:22, Fam Zheng wrote:
> Another advantage for bdrv_aio_poll() is, in main loop we will not need
> a separate AioContext in changes like:
> 
> http://patchwork.ozlabs.org/patch/514968/
> 
> Because nested aio_poll will automatically be limited to only process block
> layer events. My idea is to eventually let main loop use aio_poll

That would be a step back.  Using GSource is useful because it lets you
integrate libraries such as GTK+.

Paolo

> , which means
> we would also move chardev onto it. It would be neat to put all fds of the main
> thread into a single AioContext.

Re: [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 10:15, Paolo Bonzini wrote:
> 
> 
> On 09/09/2015 05:22, Fam Zheng wrote:
> > Another advantage for bdrv_aio_poll() is, in main loop we will not need
> > a separate AioContext in changes like:
> > 
> > http://patchwork.ozlabs.org/patch/514968/
> > 
> > Because nested aio_poll will automatically be limited to only process block
> > layer events. My idea is to eventually let main loop use aio_poll
> 
> That would be a step back.  Using GSource is useful because it lets you
> integrate libraries such as GTK+.

Can we move GTK to a separate GSource thread?

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Alberto Garcia]

On Fri 11 Sep 2015 11:14:33 AM CEST, Fam Zheng <famz@redhat.com> wrote:

>> > Another advantage for bdrv_aio_poll() is, in main loop we will not
>> > need a separate AioContext in changes like:
>> > 
>> > http://patchwork.ozlabs.org/patch/514968/
>> > 
>> > Because nested aio_poll will automatically be limited to only
>> > process block layer events. My idea is to eventually let main loop
>> > use aio_poll
>> 
>> That would be a step back.  Using GSource is useful because it lets
>> you integrate libraries such as GTK+.
>
> Can we move GTK to a separate GSource thread?

I think that GTK should always run in the main thread, or at least the
one running the default main loop / GMainContext.

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Daniel P. Berrange]

On Fri, Sep 11, 2015 at 11:36:10AM +0200, Alberto Garcia wrote:
> On Fri 11 Sep 2015 11:14:33 AM CEST, Fam Zheng <famz@redhat.com> wrote:
> 
> >> > Another advantage for bdrv_aio_poll() is, in main loop we will not
> >> > need a separate AioContext in changes like:
> >> > 
> >> > http://patchwork.ozlabs.org/patch/514968/
> >> > 
> >> > Because nested aio_poll will automatically be limited to only
> >> > process block layer events. My idea is to eventually let main loop
> >> > use aio_poll
> >> 
> >> That would be a step back.  Using GSource is useful because it lets
> >> you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
> 
> I think that GTK should always run in the main thread, or at least the
> one running the default main loop / GMainContext.

In theory GTK can run from a separate thread, but my experiance with
GTK and threads is that you end up in a world of hurt if you try to
anything except use the main thread, so I'd really recommend against
it. Even if you do extensive testing locally, things still break across
different GTK versions people have, so its just not worth the pain.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 11:36, Alberto Garcia wrote:
> On Fri 11 Sep 2015 11:14:33 AM CEST, Fam Zheng <famz@redhat.com> wrote:
> 
> >> > Another advantage for bdrv_aio_poll() is, in main loop we will not
> >> > need a separate AioContext in changes like:
> >> > 
> >> > http://patchwork.ozlabs.org/patch/514968/
> >> > 
> >> > Because nested aio_poll will automatically be limited to only
> >> > process block layer events. My idea is to eventually let main loop
> >> > use aio_poll
> >> 
> >> That would be a step back.  Using GSource is useful because it lets
> >> you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
> 
> I think that GTK should always run in the main thread, or at least the
> one running the default main loop / GMainContext.

Yeah it's basically GMainContext staying in the main thread and
block/net/chardev I/O put in a new AioContext thread.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 11/09/2015 11:44, Fam Zheng wrote:
> > > > That would be a step back.  Using GSource is useful because it lets
> > > > you integrate libraries such as GTK+.
> > >
> > > Can we move GTK to a separate GSource thread?
> > 
> > I think that GTK should always run in the main thread, or at least the
> > one running the default main loop / GMainContext.
> 
> Yeah it's basically GMainContext staying in the main thread and
> block/net/chardev I/O put in a new AioContext thread.

Why?  The point of an event loop is that you can multiplex everything on
the same thread.  Unless we have specific needs (e.g. scalability) one
thread is the way to go and keep things simple.

The tool we have for scalability is AioContext + dataplane threads, and
because we _can_ integrate it into the main thread (AioContext is a
GSource) we can write code once for the main thread and for external
dataplane threads.

Using AioContext instead of duplicating the code is great.  Moving SLIRP
to Win32 would get rid of all the duplicated select() code between
aio-win32.c and main-loop.c.  A lot of main-loop.c code can go away
(probably not all because unlike glib we support nanosecond timeouts
with TimerListGroup, but who knows).

However, there is no need to deviate the event loop from the well-known,
standardized glib architecture.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 11:54, Paolo Bonzini wrote:
> 
> 
> On 11/09/2015 11:44, Fam Zheng wrote:
> > > > > That would be a step back.  Using GSource is useful because it lets
> > > > > you integrate libraries such as GTK+.
> > > >
> > > > Can we move GTK to a separate GSource thread?
> > > 
> > > I think that GTK should always run in the main thread, or at least the
> > > one running the default main loop / GMainContext.
> > 
> > Yeah it's basically GMainContext staying in the main thread and
> > block/net/chardev I/O put in a new AioContext thread.
> 
> Why?  The point of an event loop is that you can multiplex everything on
> the same thread.  Unless we have specific needs (e.g. scalability) one
> thread is the way to go and keep things simple.

The reason is scalability. :)

> 
> The tool we have for scalability is AioContext + dataplane threads, and
> because we _can_ integrate it into the main thread (AioContext is a
> GSource) we can write code once for the main thread and for external
> dataplane threads.

I don't think integrating with main thread or not is a problem, we can still
use the same code as before, because the event loop code change should be
transparent (see below).

> 
> Using AioContext instead of duplicating the code is great.  Moving SLIRP
> to Win32 would get rid of all the duplicated select() code between
> aio-win32.c and main-loop.c.  A lot of main-loop.c code can go away
> (probably not all because unlike glib we support nanosecond timeouts
> with TimerListGroup, but who knows).
> 
> However, there is no need to deviate the event loop from the well-known,
> standardized glib architecture.

Moving things to AIO isn't deviation, it's more about enabling of dataplane and
epoll. That's why block was moved to AioContext, and I think we can do similar
for net and serial, the difference is that as a start, they don't need to be
fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() loop,
they can better performance because of epoll.

I'm thinking about something like this:

    Main thread (GTK):

        g_main_run()


    New io thread (one per process, includes block, net, serial):

        while (true) {
            qemu_mutex_lock_iothread();
            aio_poll(qemu_aio_context);
            qemu_mutex_unlock_iothread();
        }


    Dataplane thread (one per "-object iothread", includes dataplane enabled
    devices):

        while (true) {
            aio_poll(iothread->ctx);
        }

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 11/09/2015 12:40, Fam Zheng wrote:
> On Fri, 09/11 11:54, Paolo Bonzini wrote:
>>
>>
>> On 11/09/2015 11:44, Fam Zheng wrote:
>>>>>> That would be a step back.  Using GSource is useful because it lets
>>>>>> you integrate libraries such as GTK+.
>>>>>
>>>>> Can we move GTK to a separate GSource thread?
>>>>
>>>> I think that GTK should always run in the main thread, or at least the
>>>> one running the default main loop / GMainContext.
>>>
>>> Yeah it's basically GMainContext staying in the main thread and
>>> block/net/chardev I/O put in a new AioContext thread.
>>
>> Why?  The point of an event loop is that you can multiplex everything on
>> the same thread.  Unless we have specific needs (e.g. scalability) one
>> thread is the way to go and keep things simple.
> 
> The reason is scalability. :)

Scalability of what?  If virtio-net or virtio-serial needs to be more
scalable, putting all of them into a non-main-loop thread will not make
things more scalable, because you have a single thread anyway.  You'd
need to go BQL-free and allow an arbitrary number.

> Moving things to AIO isn't deviation, it's more about enabling of dataplane and
> epoll. That's why block was moved to AioContext, and I think we can do similar
> for net and serial, the difference is that as a start, they don't need to be
> fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() loop,
> they can better performance because of epoll.

Isn't that what your "iohandler.c with AioHandler" already does?  True,
it would be epoll-within-poll, not pure poll.  But if you need epoll,
you might as well go BQL-free.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 12:46, Paolo Bonzini wrote:
> 
> 
> On 11/09/2015 12:40, Fam Zheng wrote:
> > On Fri, 09/11 11:54, Paolo Bonzini wrote:
> >>
> >>
> >> On 11/09/2015 11:44, Fam Zheng wrote:
> >>>>>> That would be a step back.  Using GSource is useful because it lets
> >>>>>> you integrate libraries such as GTK+.
> >>>>>
> >>>>> Can we move GTK to a separate GSource thread?
> >>>>
> >>>> I think that GTK should always run in the main thread, or at least the
> >>>> one running the default main loop / GMainContext.
> >>>
> >>> Yeah it's basically GMainContext staying in the main thread and
> >>> block/net/chardev I/O put in a new AioContext thread.
> >>
> >> Why?  The point of an event loop is that you can multiplex everything on
> >> the same thread.  Unless we have specific needs (e.g. scalability) one
> >> thread is the way to go and keep things simple.
> > 
> > The reason is scalability. :)
> 
> Scalability of what?  If virtio-net or virtio-serial needs to be more
> scalable, putting all of them into a non-main-loop thread will not make
> things more scalable, because you have a single thread anyway.  You'd
> need to go BQL-free and allow an arbitrary number.
> 
> > Moving things to AIO isn't deviation, it's more about enabling of dataplane and
> > epoll. That's why block was moved to AioContext, and I think we can do similar
> > for net and serial, the difference is that as a start, they don't need to be
> > fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() loop,
> > they can better performance because of epoll.
> 
> Isn't that what your "iohandler.c with AioHandler" already does?  True,
> it would be epoll-within-poll, not pure poll.  But if you need epoll,
> you might as well go BQL-free.

epoll-within-poll? Do you mean change the main event loop from:

    qemu_poll_ns([..., ioeventfd1, ioeventfd2, ..., ioeventfd99], ...)

to

    qemu_poll_ns([epollfd], ...)

where epollfd watches all the fds, and let the handler of epollfd do
epoll_wait()?

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 11/09/2015 13:01, Fam Zheng wrote:
> On Fri, 09/11 12:46, Paolo Bonzini wrote:
>>
>>
>> On 11/09/2015 12:40, Fam Zheng wrote:
>>> On Fri, 09/11 11:54, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 11/09/2015 11:44, Fam Zheng wrote:
>>>>>>>> That would be a step back.  Using GSource is useful because it lets
>>>>>>>> you integrate libraries such as GTK+.
>>>>>>>
>>>>>>> Can we move GTK to a separate GSource thread?
>>>>>>
>>>>>> I think that GTK should always run in the main thread, or at least the
>>>>>> one running the default main loop / GMainContext.
>>>>>
>>>>> Yeah it's basically GMainContext staying in the main thread and
>>>>> block/net/chardev I/O put in a new AioContext thread.
>>>>
>>>> Why?  The point of an event loop is that you can multiplex everything on
>>>> the same thread.  Unless we have specific needs (e.g. scalability) one
>>>> thread is the way to go and keep things simple.
>>>
>>> The reason is scalability. :)
>>
>> Scalability of what?  If virtio-net or virtio-serial needs to be more
>> scalable, putting all of them into a non-main-loop thread will not make
>> things more scalable, because you have a single thread anyway.  You'd
>> need to go BQL-free and allow an arbitrary number.
>>
>>> Moving things to AIO isn't deviation, it's more about enabling of dataplane and
>>> epoll. That's why block was moved to AioContext, and I think we can do similar
>>> for net and serial, the difference is that as a start, they don't need to be
>>> fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() loop,
>>> they can better performance because of epoll.
>>
>> Isn't that what your "iohandler.c with AioHandler" already does?  True,
>> it would be epoll-within-poll, not pure poll.  But if you need epoll,
>> you might as well go BQL-free.
> 
> epoll-within-poll? Do you mean change the main event loop from:
> 
>     qemu_poll_ns([..., ioeventfd1, ioeventfd2, ..., ioeventfd99], ...)
> 
> to
> 
>     qemu_poll_ns([epollfd], ...)
> 
> where epollfd watches all the fds, and let the handler of epollfd do
> epoll_wait()?

I mean that glib's main loop (or os_host_main_loop_wait in main-loop.c)
uses poll, and then the AioContext GSource uses epoll.  Still, you have
a constant (and low) number of file descriptors in the outer poll.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 13:02, Paolo Bonzini wrote:
> 
> 
> On 11/09/2015 13:01, Fam Zheng wrote:
> > On Fri, 09/11 12:46, Paolo Bonzini wrote:
> >>
> >>
> >> On 11/09/2015 12:40, Fam Zheng wrote:
> >>> On Fri, 09/11 11:54, Paolo Bonzini wrote:
> >>>>
> >>>>
> >>>> On 11/09/2015 11:44, Fam Zheng wrote:
> >>>>>>>> That would be a step back.  Using GSource is useful because it lets
> >>>>>>>> you integrate libraries such as GTK+.
> >>>>>>>
> >>>>>>> Can we move GTK to a separate GSource thread?
> >>>>>>
> >>>>>> I think that GTK should always run in the main thread, or at least the
> >>>>>> one running the default main loop / GMainContext.
> >>>>>
> >>>>> Yeah it's basically GMainContext staying in the main thread and
> >>>>> block/net/chardev I/O put in a new AioContext thread.
> >>>>
> >>>> Why?  The point of an event loop is that you can multiplex everything on
> >>>> the same thread.  Unless we have specific needs (e.g. scalability) one
> >>>> thread is the way to go and keep things simple.
> >>>
> >>> The reason is scalability. :)
> >>
> >> Scalability of what?  If virtio-net or virtio-serial needs to be more
> >> scalable, putting all of them into a non-main-loop thread will not make
> >> things more scalable, because you have a single thread anyway.  You'd
> >> need to go BQL-free and allow an arbitrary number.
> >>
> >>> Moving things to AIO isn't deviation, it's more about enabling of dataplane and
> >>> epoll. That's why block was moved to AioContext, and I think we can do similar
> >>> for net and serial, the difference is that as a start, they don't need to be
> >>> fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() loop,
> >>> they can better performance because of epoll.
> >>
> >> Isn't that what your "iohandler.c with AioHandler" already does?  True,
> >> it would be epoll-within-poll, not pure poll.  But if you need epoll,
> >> you might as well go BQL-free.
> > 
> > epoll-within-poll? Do you mean change the main event loop from:
> > 
> >     qemu_poll_ns([..., ioeventfd1, ioeventfd2, ..., ioeventfd99], ...)
> > 
> > to
> > 
> >     qemu_poll_ns([epollfd], ...)
> > 
> > where epollfd watches all the fds, and let the handler of epollfd do
> > epoll_wait()?
> 
> I mean that glib's main loop (or os_host_main_loop_wait in main-loop.c)
> uses poll, and then the AioContext GSource uses epoll.  Still, you have
> a constant (and low) number of file descriptors in the outer poll.
> 

OK, I think I get your idea, and it sounds good to me, thank you very much! ;-)

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 11/09/2015 11:36, Alberto Garcia wrote:
> > > > Because nested aio_poll will automatically be limited to only
> > > > process block layer events. My idea is to eventually let main loop
> > > > use aio_poll
> > > 
> > > That would be a step back.  Using GSource is useful because it lets
> > > you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
>
> I think that GTK should always run in the main thread, or at least the
> one running the default main loop / GMainContext.

Agreed.  The glib main loop is a positive thing, not a negative one.

Paolo

Re: [Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Paolo Bonzini]

On 29/07/2015 06:42, 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

For patches 1-8:

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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Stefan Hajnoczi]

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.

Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Kevin Wolf]

Am 29.07.2015 um 06:42 hat Fam Zheng geschrieben:
> 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.

It seems that I haven't replied on the mailing list yet, even though I
think I already said this in person at KVM Forum: This series fixes only
a special case of the real problem, which is that bdrv_drain/all at a
single point doesn't make a lot of sense, but needs to be replaced by a
whole section with exclusive access, like a bdrv_drained_begin/end pair.

To be clear: Anything that works with types of users instead of
individual users is bound to fall short of being a complete solution. I
don't prefer partial solutions when we know there is a bigger problem.

This series addresses your immediate need of protecting against new data
plane requests, which it arguably achieves. The second case I always
have in mind is Berto's case where he has multiple streaming block jobs
in the same backing file chain [1].

This involves a bdrv_reopen() of the target BDS to make it writable, and
bdrv_reopen() uses bdrv_drain_all() so drivers don't have to cope with
running requests while reopening themselves. It can however involve
nested event loops for synchronous operations like bdrv_flush(), and if
those can process completions of block jobs, which can respond by doing
anything to the respective node, things can go wrong.

You don't solve this by adding client types (then problematic request
would be PROTOCOL in your proposal and you can never exclude that), but
you really need to have bdrv_drained_being/end pairs, where only
requests issued in between are processed and everything else waits.

Kevin

[1] https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06273.html

Re: [Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Fam Zheng]

On Fri, 09/11 12:39, Kevin Wolf wrote:
> Am 29.07.2015 um 06:42 hat Fam Zheng geschrieben:
> > 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.
> 
> It seems that I haven't replied on the mailing list yet, even though I
> think I already said this in person at KVM Forum: This series fixes only
> a special case of the real problem, which is that bdrv_drain/all at a
> single point doesn't make a lot of sense, but needs to be replaced by a
> whole section with exclusive access, like a bdrv_drained_begin/end pair.
> 
> To be clear: Anything that works with types of users instead of
> individual users is bound to fall short of being a complete solution. I
> don't prefer partial solutions when we know there is a bigger problem.
> 
> This series addresses your immediate need of protecting against new data
> plane requests, which it arguably achieves. The second case I always
> have in mind is Berto's case where he has multiple streaming block jobs
> in the same backing file chain [1].
> 
> This involves a bdrv_reopen() of the target BDS to make it writable, and
> bdrv_reopen() uses bdrv_drain_all() so drivers don't have to cope with
> running requests while reopening themselves. It can however involve
> nested event loops for synchronous operations like bdrv_flush(), and if
> those can process completions of block jobs, which can respond by doing
> anything to the respective node, things can go wrong.

Just to get a better idea of bdrv_drained_begin/end, could you explain how to
use the pair to fix the above problem?

> 
> You don't solve this by adding client types (then problematic request
> would be PROTOCOL in your proposal and you can never exclude that), but
> you really need to have bdrv_drained_being/end pairs, where only
> requests issued in between are processed and everything else waits.

What do you mean by "only requests issued in between are processed"? Where are
the requests from?

Fam

Re: [Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Kevin Wolf]

Am 11.09.2015 um 13:46 hat Fam Zheng geschrieben:
> On Fri, 09/11 12:39, Kevin Wolf wrote:
> > Am 29.07.2015 um 06:42 hat Fam Zheng geschrieben:
> > > 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.
> > 
> > It seems that I haven't replied on the mailing list yet, even though I
> > think I already said this in person at KVM Forum: This series fixes only
> > a special case of the real problem, which is that bdrv_drain/all at a
> > single point doesn't make a lot of sense, but needs to be replaced by a
> > whole section with exclusive access, like a bdrv_drained_begin/end pair.
> > 
> > To be clear: Anything that works with types of users instead of
> > individual users is bound to fall short of being a complete solution. I
> > don't prefer partial solutions when we know there is a bigger problem.
> > 
> > This series addresses your immediate need of protecting against new data
> > plane requests, which it arguably achieves. The second case I always
> > have in mind is Berto's case where he has multiple streaming block jobs
> > in the same backing file chain [1].
> > 
> > This involves a bdrv_reopen() of the target BDS to make it writable, and
> > bdrv_reopen() uses bdrv_drain_all() so drivers don't have to cope with
> > running requests while reopening themselves. It can however involve
> > nested event loops for synchronous operations like bdrv_flush(), and if
> > those can process completions of block jobs, which can respond by doing
> > anything to the respective node, things can go wrong.
> 
> Just to get a better idea of bdrv_drained_begin/end, could you explain how to
> use the pair to fix the above problem?

How to use it is easy part: In bdrv_reopen_multiple(), you would replace
the existing bdrv_drain_all() with begin and you would add the
corresponding end right before the return statement.

> > You don't solve this by adding client types (then problematic request
> > would be PROTOCOL in your proposal and you can never exclude that), but
> > you really need to have bdrv_drained_being/end pairs, where only
> > requests issued in between are processed and everything else waits.
> 
> What do you mean by "only requests issued in between are processed"? Where are
> the requests from?

Generally speaking, you would have code that looks like this:

    bdrv_drain_begin()
    ...
    bdrv_something_synchronous()
    ...
    bdrv_drain_end()

You want to process everything that is necessary for completing
bdrv_something_synchronous(), but nothing else.

The trickier question is how to implement this. I know that it's much
easier to say that your series doesn't work than actually proposing
something else that works...

One relatively obvious answer we found when we discussed this a while
back was some kind of a recursive CoRwLock (reader = in-flight request;
writer = drained section), but that requires obviously that you're
running in a coroutine if you want to do something with a drained
request queue.

I'm also not totally happy with the requirement of taking a reader lock
more or less everywhere. But I'm not sure yet if there is a good
alternative that can achieve the same.

This needs some more thought, I guess.

Kevin

Re: [Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Fam Zheng]

On Fri, 09/11 14:22, Kevin Wolf wrote:
> Am 11.09.2015 um 13:46 hat Fam Zheng geschrieben:
> > On Fri, 09/11 12:39, Kevin Wolf wrote:
> > > Am 29.07.2015 um 06:42 hat Fam Zheng geschrieben:
> > > > 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.
> > > 
> > > It seems that I haven't replied on the mailing list yet, even though I
> > > think I already said this in person at KVM Forum: This series fixes only
> > > a special case of the real problem, which is that bdrv_drain/all at a
> > > single point doesn't make a lot of sense, but needs to be replaced by a
> > > whole section with exclusive access, like a bdrv_drained_begin/end pair.
> > > 
> > > To be clear: Anything that works with types of users instead of
> > > individual users is bound to fall short of being a complete solution. I
> > > don't prefer partial solutions when we know there is a bigger problem.
> > > 
> > > This series addresses your immediate need of protecting against new data
> > > plane requests, which it arguably achieves. The second case I always
> > > have in mind is Berto's case where he has multiple streaming block jobs
> > > in the same backing file chain [1].
> > > 
> > > This involves a bdrv_reopen() of the target BDS to make it writable, and
> > > bdrv_reopen() uses bdrv_drain_all() so drivers don't have to cope with
> > > running requests while reopening themselves. It can however involve
> > > nested event loops for synchronous operations like bdrv_flush(), and if
> > > those can process completions of block jobs, which can respond by doing
> > > anything to the respective node, things can go wrong.
> > 
> > Just to get a better idea of bdrv_drained_begin/end, could you explain how to
> > use the pair to fix the above problem?
> 
> How to use it is easy part: In bdrv_reopen_multiple(), you would replace
> the existing bdrv_drain_all() with begin and you would add the
> corresponding end right before the return statement.

OK, so that the completion of other jobs won't happen because we only complete
the relevant requests?

Does block_job_pause() work here?

> 
> > > You don't solve this by adding client types (then problematic request
> > > would be PROTOCOL in your proposal and you can never exclude that), but
> > > you really need to have bdrv_drained_being/end pairs, where only
> > > requests issued in between are processed and everything else waits.
> > 
> > What do you mean by "only requests issued in between are processed"? Where are
> > the requests from?
> 
> Generally speaking, you would have code that looks like this:
> 
>     bdrv_drain_begin()
>     ...
>     bdrv_something_synchronous()
>     ...
>     bdrv_drain_end()
> 
> You want to process everything that is necessary for completing
> bdrv_something_synchronous(), but nothing else.
> 
> The trickier question is how to implement this. I know that it's much
> easier to say that your series doesn't work than actually proposing
> something else that works...

I see the basic idea but that is still way too obscure.  How would
bdrv_draind_begin/end know what is necessary in completing
bdrv_something_synchronous()?

> 
> One relatively obvious answer we found when we discussed this a while
> back was some kind of a recursive CoRwLock (reader = in-flight request;
> writer = drained section), but that requires obviously that you're
> running in a coroutine if you want to do something with a drained
> request queue.
> 
> I'm also not totally happy with the requirement of taking a reader lock
> more or less everywhere. But I'm not sure yet if there is a good
> alternative that can achieve the same.

We're basically trying to prevent new requests from being submitted, but
couldn't this blocking be a complication itself? A CoRwLock, if any, would be
implemented with something like a CoQueue, but considering the existing CoQueue
in BDS - throttled_reqs, aren't those what we want to keep *empty* between the
bdrv_drain_begin/end pair?  Now we're talking about queueing requests to
another CoQueue, which feels contradictory to me.

Fam

Re: [Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Kevin Wolf]

Am 14.09.2015 um 09:27 hat Fam Zheng geschrieben:
> On Fri, 09/11 14:22, Kevin Wolf wrote:
> > Am 11.09.2015 um 13:46 hat Fam Zheng geschrieben:
> > > On Fri, 09/11 12:39, Kevin Wolf wrote:
> > > > Am 29.07.2015 um 06:42 hat Fam Zheng geschrieben:
> > > > > 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.
> > > > 
> > > > It seems that I haven't replied on the mailing list yet, even though I
> > > > think I already said this in person at KVM Forum: This series fixes only
> > > > a special case of the real problem, which is that bdrv_drain/all at a
> > > > single point doesn't make a lot of sense, but needs to be replaced by a
> > > > whole section with exclusive access, like a bdrv_drained_begin/end pair.
> > > > 
> > > > To be clear: Anything that works with types of users instead of
> > > > individual users is bound to fall short of being a complete solution. I
> > > > don't prefer partial solutions when we know there is a bigger problem.
> > > > 
> > > > This series addresses your immediate need of protecting against new data
> > > > plane requests, which it arguably achieves. The second case I always
> > > > have in mind is Berto's case where he has multiple streaming block jobs
> > > > in the same backing file chain [1].
> > > > 
> > > > This involves a bdrv_reopen() of the target BDS to make it writable, and
> > > > bdrv_reopen() uses bdrv_drain_all() so drivers don't have to cope with
> > > > running requests while reopening themselves. It can however involve
> > > > nested event loops for synchronous operations like bdrv_flush(), and if
> > > > those can process completions of block jobs, which can respond by doing
> > > > anything to the respective node, things can go wrong.
> > > 
> > > Just to get a better idea of bdrv_drained_begin/end, could you explain how to
> > > use the pair to fix the above problem?
> > 
> > How to use it is easy part: In bdrv_reopen_multiple(), you would replace
> > the existing bdrv_drain_all() with begin and you would add the
> > corresponding end right before the return statement.
> 
> OK, so that the completion of other jobs won't happen because we only complete
> the relevant requests?
> 
> Does block_job_pause() work here?

If you find all block jobs that do something with the BDS,
block_job_pause() would work, yes. But if you go further down that road,
you end up not with a design that considers the problem, but with
special casing every single user. That doesn't feel very sustainable.

> > > > You don't solve this by adding client types (then problematic request
> > > > would be PROTOCOL in your proposal and you can never exclude that), but
> > > > you really need to have bdrv_drained_being/end pairs, where only
> > > > requests issued in between are processed and everything else waits.
> > > 
> > > What do you mean by "only requests issued in between are processed"? Where are
> > > the requests from?
> > 
> > Generally speaking, you would have code that looks like this:
> > 
> >     bdrv_drain_begin()
> >     ...
> >     bdrv_something_synchronous()
> >     ...
> >     bdrv_drain_end()
> > 
> > You want to process everything that is necessary for completing
> > bdrv_something_synchronous(), but nothing else.
> > 
> > The trickier question is how to implement this. I know that it's much
> > easier to say that your series doesn't work than actually proposing
> > something else that works...
> 
> I see the basic idea but that is still way too obscure.  How would
> bdrv_draind_begin/end know what is necessary in completing
> bdrv_something_synchronous()?

That's not about the interface, but the implementation part, which I
tried to answer below.

> > One relatively obvious answer we found when we discussed this a while
> > back was some kind of a recursive CoRwLock (reader = in-flight request;
> > writer = drained section), but that requires obviously that you're
> > running in a coroutine if you want to do something with a drained
> > request queue.
> > 
> > I'm also not totally happy with the requirement of taking a reader lock
> > more or less everywhere. But I'm not sure yet if there is a good
> > alternative that can achieve the same.
> 
> We're basically trying to prevent new requests from being submitted, but
> couldn't this blocking be a complication itself? A CoRwLock, if any, would be
> implemented with something like a CoQueue, but considering the existing CoQueue
> in BDS - throttled_reqs, aren't those what we want to keep *empty* between the
> bdrv_drain_begin/end pair?  Now we're talking about queueing requests to
> another CoQueue, which feels contradictory to me.

You always queue requests _somewhere_. If they aren't queued in the
block layer, they are in the virtio ring or elsewhere. I think the point
of bdrv_drain() is that the block drivers don't have any pending
requests; they can be queued elsewhere without hurting anyone.

Kevin

Re: [Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Stefan Hajnoczi]

On Fri, Sep 11, 2015 at 02:22:14PM +0200, Kevin Wolf wrote:
> Am 11.09.2015 um 13:46 hat Fam Zheng geschrieben:
> > On Fri, 09/11 12:39, Kevin Wolf wrote:
> > > Am 29.07.2015 um 06:42 hat Fam Zheng geschrieben:
> > > > 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.
> > > 
> > > It seems that I haven't replied on the mailing list yet, even though I
> > > think I already said this in person at KVM Forum: This series fixes only
> > > a special case of the real problem, which is that bdrv_drain/all at a
> > > single point doesn't make a lot of sense, but needs to be replaced by a
> > > whole section with exclusive access, like a bdrv_drained_begin/end pair.
> > > 
> > > To be clear: Anything that works with types of users instead of
> > > individual users is bound to fall short of being a complete solution. I
> > > don't prefer partial solutions when we know there is a bigger problem.
> > > 
> > > This series addresses your immediate need of protecting against new data
> > > plane requests, which it arguably achieves. The second case I always
> > > have in mind is Berto's case where he has multiple streaming block jobs
> > > in the same backing file chain [1].
> > > 
> > > This involves a bdrv_reopen() of the target BDS to make it writable, and
> > > bdrv_reopen() uses bdrv_drain_all() so drivers don't have to cope with
> > > running requests while reopening themselves. It can however involve
> > > nested event loops for synchronous operations like bdrv_flush(), and if
> > > those can process completions of block jobs, which can respond by doing
> > > anything to the respective node, things can go wrong.
> > 
> > Just to get a better idea of bdrv_drained_begin/end, could you explain how to
> > use the pair to fix the above problem?
> 
> How to use it is easy part: In bdrv_reopen_multiple(), you would replace
> the existing bdrv_drain_all() with begin and you would add the
> corresponding end right before the return statement.
> 
> > > You don't solve this by adding client types (then problematic request
> > > would be PROTOCOL in your proposal and you can never exclude that), but
> > > you really need to have bdrv_drained_being/end pairs, where only
> > > requests issued in between are processed and everything else waits.
> > 
> > What do you mean by "only requests issued in between are processed"? Where are
> > the requests from?
> 
> Generally speaking, you would have code that looks like this:
> 
>     bdrv_drain_begin()
>     ...
>     bdrv_something_synchronous()
>     ...
>     bdrv_drain_end()
> 
> You want to process everything that is necessary for completing
> bdrv_something_synchronous(), but nothing else.
> 
> The trickier question is how to implement this. I know that it's much
> easier to say that your series doesn't work than actually proposing
> something else that works...
> 
> One relatively obvious answer we found when we discussed this a while
> back was some kind of a recursive CoRwLock (reader = in-flight request;
> writer = drained section), but that requires obviously that you're
> running in a coroutine if you want to do something with a drained
> request queue.

At one point I thought about converting vl.c:main() to run everything in
a coroutine.  It would allow us to eliminate synchronous operations
completely.

The problem is that it's quite a big change that requires protecting
formerly synchronous operations against concurrency.

Stefan