[Qemu-devel] [PATCH 00/12] block: bdrv_drained_begin/end for transactions on dataplane devices

[Qemu-devel] [PATCH 00/12] block: bdrv_drained_begin/end for transactions on dataplane devices

[Fam Zheng]

External I/O requests mustn't come in while we're in the middle of a
QMP transaction. This series adds bdrv_drained_begin and bdrv_drained_end
around "prepare" and "cleanup" in the transaction actions to make sure that.

Note that "backup" action already starts the block job coroutine in prepare,
and "snapshot" action already creates the snapshot, but both are OK because we
call bdrv_drained_begin first.

The nested event loops also dispatch timers and BHs on the same AioContext. The
existing timers are iscsi, curl, qcow2 and qed. The only one that does I/O is
qed, which is dealt with by a new ".bdrv_drain" callback.


Fam Zheng (12):
  aio: Introduce "type" in aio_set_fd_handler and aio_set_event_notifier
  aio: Save fd client type to AioHandler
  nbd: Mark fd handlers client type as "external"
  dataplane: Mark host notifiers' client type as "external"
  aio: introduce aio_{disable,enable}_clients
  block: Introduce "drained begin/end" API
  block: Add "drained begin/end" for transactional external snapshot
  block: Add "drained begin/end" for transactional backup
  block: Add "drained begin/end" for transactional blockdev-backup
  block: Add "drained begin/end" for internal snapshot
  block: Introduce BlockDriver.bdrv_drain callback
  qed: Implement .bdrv_drain

 aio-posix.c                     |  9 +++++--
 aio-win32.c                     |  8 +++++-
 async.c                         | 45 +++++++++++++++++++++++++++++++-
 block.c                         |  2 ++
 block/curl.c                    | 14 +++++-----
 block/io.c                      | 21 +++++++++++++++
 block/iscsi.c                   |  9 +++----
 block/linux-aio.c               |  5 ++--
 block/nbd-client.c              | 10 ++++---
 block/nfs.c                     | 17 +++++-------
 block/qed.c                     |  7 +++++
 block/sheepdog.c                | 38 ++++++++++++++++++---------
 block/ssh.c                     |  5 ++--
 block/win32-aio.c               |  5 ++--
 blockdev.c                      | 27 ++++++++++++++-----
 hw/block/dataplane/virtio-blk.c |  6 +++--
 hw/scsi/virtio-scsi-dataplane.c | 24 +++++++++++------
 include/block/aio.h             | 36 +++++++++++++++++++++++++
 include/block/block.h           | 19 ++++++++++++++
 include/block/block_int.h       |  8 ++++++
 iohandler.c                     |  3 ++-
 nbd.c                           |  4 ++-
 roms/ipxe                       |  2 +-
 tests/test-aio.c                | 58 +++++++++++++++++++++++------------------
 24 files changed, 289 insertions(+), 93 deletions(-)

-- 
2.5.3

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

[Fam Zheng]

The parameter is added but not used.

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                | 38 ++++++++++++++++++---------
 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 ++++
 iohandler.c                     |  3 ++-
 nbd.c                           |  4 ++-
 roms/ipxe                       |  2 +-
 tests/test-aio.c                | 58 +++++++++++++++++++++++------------------
 18 files changed, 129 insertions(+), 85 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 efce14b..244bf79 100644
--- a/async.c
+++ b/async.c
@@ -247,7 +247,7 @@ aio_ctx_finalize(GSource     *source)
     }
     qemu_mutex_unlock(&ctx->bh_lock);
 
-    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);
@@ -329,6 +329,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 93f1ee4..dcb578a 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);
@@ -1280,9 +1280,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 887a98e..57a5555 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -63,11 +63,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;
@@ -242,9 +241,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;
 }
 
@@ -263,9 +261,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 e7e58b7..e31d091 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -651,14 +651,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)) {
@@ -683,7 +685,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;
@@ -735,7 +738,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;
 
@@ -938,7 +942,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;
 }
 
@@ -1199,7 +1204,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);
 
@@ -1218,7 +1223,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);
 }
@@ -1368,7 +1374,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,
@@ -1377,7 +1384,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 */
@@ -1490,7 +1498,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);
     }
@@ -1528,7 +1537,8 @@ static void sd_reopen_commit(BDRVReopenState *state)
     BDRVSheepdogState *s = state->bs->opaque;
 
     if (s->fd) {
-        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);
         closesocket(s->fd);
     }
 
@@ -1551,7 +1561,8 @@ static void sd_reopen_abort(BDRVReopenState *state)
     }
 
     if (re_s->fd) {
-        aio_set_fd_handler(s->aio_context, re_s->fd, NULL, NULL, NULL);
+        aio_set_fd_handler(s->aio_context, re_s->fd, AIO_CLIENT_UNSPECIFIED,
+                           NULL, NULL, NULL);
         closesocket(re_s->fd);
     }
 
@@ -1935,7 +1946,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 8d06739..a90295e 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 6106e46..dc40ba3 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 400b1b0..830a0e8 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -275,6 +275,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.
  *
@@ -299,6 +302,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);
@@ -312,6 +316,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/iohandler.c b/iohandler.c
index 55f8501..71bcff3 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -55,7 +55,8 @@ void qemu_set_fd_handler(int fd,
                          void *opaque)
 {
     iohandler_init();
-    aio_set_fd_handler(iohandler_ctx, fd, fd_read, fd_write, opaque);
+    aio_set_fd_handler(iohandler_ctx, fd, AIO_CLIENT_UNSPECIFIED,
+                       fd_read, fd_write, opaque);
 }
 
 /* reaping of zombies.  right now we're not passing the status to
diff --git a/nbd.c b/nbd.c
index 07240bd..16a4087 100644
--- a/nbd.c
+++ b/nbd.c
@@ -1446,6 +1446,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);
@@ -1455,7 +1456,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/roms/ipxe b/roms/ipxe
index 4e03af8..35c5379 160000
--- a/roms/ipxe
+++ b/roms/ipxe
@@ -1 +1 @@
-Subproject commit 4e03af8ec2d497e725566a91fd5c19dd604c18a6
+Subproject commit 35c5379760aa1fea5e38f7a78b090f92bb7813ee
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.5.3

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

[Kevin Wolf]

Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> The parameter is added but not used.
> 
> 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                | 38 ++++++++++++++++++---------
>  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 ++++
>  iohandler.c                     |  3 ++-
>  nbd.c                           |  4 ++-
>  roms/ipxe                       |  2 +-

This one looks unrelated.

>  tests/test-aio.c                | 58 +++++++++++++++++++++++------------------
>  18 files changed, 129 insertions(+), 85 deletions(-)

> diff --git a/include/block/aio.h b/include/block/aio.h
> index 400b1b0..830a0e8 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -275,6 +275,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.
>   *
> @@ -299,6 +302,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);
> @@ -312,6 +316,7 @@ void aio_set_fd_handler(AioContext *ctx,
>   */
>  void aio_set_event_notifier(AioContext *ctx,
>                              EventNotifier *notifier,
> +                            int type,
>                              EventNotifierHandler *io_read);

I'll leave the details like that AIO_CLIENT_MASK_ALL depends on two's
complement representation to László, but as a matter of personal taste,
I'd still prefer unsigned types for bitmasks.

I would still give my R-b if it weren't for the submodule change.

Kevin

[Qemu-devel] [PATCH 02/12] aio: Save fd client 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.5.3

Re: [Qemu-devel] [PATCH 02/12] aio: Save fd client type to AioHandler

[Kevin Wolf]

Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> So it can be used by aio_poll later.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 03/12] nbd: Mark fd handlers client type as "external"

[Fam Zheng]

So we could distinguish it from internal used fds, thus avoid handling
unwanted events 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 830a0e8..60e796b 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -276,6 +276,7 @@ bool aio_pending(AioContext *ctx);
 bool aio_dispatch(AioContext *ctx);
 
 #define AIO_CLIENT_UNSPECIFIED    (1 << 0)
+#define AIO_CLIENT_EXTERNAL       (1 << 1)
 #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 16a4087..96fcc98 100644
--- a/nbd.c
+++ b/nbd.c
@@ -1446,7 +1446,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_EXTERNAL,
                            client->can_read ? nbd_read : NULL,
                            client->send_coroutine ? nbd_restart_write : NULL,
                            client);
@@ -1457,7 +1457,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_EXTERNAL, NULL, NULL, NULL);
     }
 }
 
-- 
2.5.3

Re: [Qemu-devel] [PATCH 03/12] nbd: Mark fd handlers client type as "external"

[Kevin Wolf]

Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> So we could distinguish it from internal used fds, thus avoid handling
> unwanted events in nested aio polls.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 04/12] dataplane: Mark host notifiers' client type as "external"

[Fam Zheng]

They will be excluded by type in the nested event loops in block layer,
so that unwanted events won't be processed there.

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

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index dc40ba3..ca0447f 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_EXTERNAL,
                            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_EXTERNAL,
                            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..94c63df 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_EXTERNAL,
                            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_EXTERNAL,
                            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_EXTERNAL, NULL);
     }
     if (s->event_vring) {
         aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
-                               AIO_CLIENT_UNSPECIFIED, NULL);
+                               AIO_CLIENT_EXTERNAL, 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_EXTERNAL, 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_EXTERNAL, NULL);
     aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
-                           AIO_CLIENT_UNSPECIFIED, NULL);
+                           AIO_CLIENT_EXTERNAL, 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_EXTERNAL, NULL);
     }
 
     blk_drain_all(); /* ensure there are no in-flight requests */
-- 
2.5.3

Re: [Qemu-devel] [PATCH 04/12] dataplane: Mark host notifiers' client type as "external"

[Kevin Wolf]

Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> They will be excluded by type in the nested event loops in block layer,
> so that unwanted events won't be processed there.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 05/12] aio: introduce aio_{disable, enable}_clients

[Fam Zheng]

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

diff --git a/aio-posix.c b/aio-posix.c
index d25fcfc..a261892 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -261,7 +261,8 @@ 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
+            && !aio_type_disabled(ctx, node->type)) {
             add_pollfd(node);
         }
     }
diff --git a/aio-win32.c b/aio-win32.c
index f5ecf57..66cff60 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -309,7 +309,8 @@ 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
+            && !aio_type_disabled(ctx, node->type)) {
             events[count++] = event_notifier_get_handle(node->e);
         }
     }
diff --git a/async.c b/async.c
index 244bf79..855b9d5 100644
--- a/async.c
+++ b/async.c
@@ -361,3 +361,45 @@ void aio_context_release(AioContext *ctx)
 {
     rfifolock_unlock(&ctx->lock);
 }
+
+bool aio_type_disabled(AioContext *ctx, int type)
+{
+    int i = 1;
+    int n = 0;
+
+    while (type) {
+        bool b = type & 0x1;
+        type >>= 1;
+        n++;
+        i <<= 1;
+        if (!b) {
+            continue;
+        }
+        if (ctx->client_disable_counters[n]) {
+            return true;
+        }
+    }
+    return false;
+}
+
+void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
+                                bool is_disable)
+{
+    int i = 1;
+    int n = 0;
+    aio_context_acquire(ctx);
+
+    while (clients_mask) {
+        bool b = clients_mask & 0x1;
+        clients_mask >>= 1;
+        n++;
+        i <<= 1;
+        if (!b) {
+            continue;
+        }
+        if (ctx->client_disable_counters[n]) {
+            return true;
+        }
+    }
+    aio_context_release(ctx);
+}
diff --git a/include/block/aio.h b/include/block/aio.h
index 60e796b..b687152 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -122,6 +122,8 @@ struct AioContext {
 
     /* TimerLists for calling timers - one per clock type */
     QEMUTimerListGroup tlg;
+
+    int client_disable_counters[sizeof(int)];
 };
 
 /**
@@ -379,4 +381,32 @@ 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. This function is
+ * thread safe as it acquires/releases AioContext.
+ */
+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. This function is thread safe as it
+ * acquires/releases AioContext.
+ */
+static inline void aio_enable_clients(AioContext *ctx, int clients_mask)
+{
+    aio_disable_enable_clients(ctx, clients_mask, false);
+}
+
+bool aio_type_disabled(AioContext *ctx, int type);
+
 #endif
-- 
2.5.3

Re: [Qemu-devel] [PATCH 05/12] aio: introduce aio_{disable, enable}_clients

[Kevin Wolf]

Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>

> +void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
> +                                bool is_disable)
> +{
> +    int i = 1;
> +    int n = 0;
> +    aio_context_acquire(ctx);
> +
> +    while (clients_mask) {
> +        bool b = clients_mask & 0x1;
> +        clients_mask >>= 1;
> +        n++;
> +        i <<= 1;
> +        if (!b) {
> +            continue;
> +        }
> +        if (ctx->client_disable_counters[n]) {
> +            return true;

async.c: In function 'aio_disable_enable_clients':
async.c:401:13: error: 'return' with a value, in function returning void [-Werror]
             return true;

> +        }
> +    }
> +    aio_context_release(ctx);
> +}

Kevin

Re: [Qemu-devel] [PATCH 05/12] aio: introduce aio_{disable, enable}_clients

[Kevin Wolf]

Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  aio-posix.c         |  3 ++-
>  aio-win32.c         |  3 ++-
>  async.c             | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/block/aio.h | 30 ++++++++++++++++++++++++++++++
>  4 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/aio-posix.c b/aio-posix.c
> index d25fcfc..a261892 100644
> --- a/aio-posix.c
> +++ b/aio-posix.c
> @@ -261,7 +261,8 @@ 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
> +            && !aio_type_disabled(ctx, node->type)) {
>              add_pollfd(node);
>          }
>      }
> diff --git a/aio-win32.c b/aio-win32.c
> index f5ecf57..66cff60 100644
> --- a/aio-win32.c
> +++ b/aio-win32.c
> @@ -309,7 +309,8 @@ 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
> +            && !aio_type_disabled(ctx, node->type)) {
>              events[count++] = event_notifier_get_handle(node->e);
>          }
>      }
> diff --git a/async.c b/async.c
> index 244bf79..855b9d5 100644
> --- a/async.c
> +++ b/async.c
> @@ -361,3 +361,45 @@ void aio_context_release(AioContext *ctx)
>  {
>      rfifolock_unlock(&ctx->lock);
>  }
> +
> +bool aio_type_disabled(AioContext *ctx, int type)
> +{
> +    int i = 1;
> +    int n = 0;
> +
> +    while (type) {
> +        bool b = type & 0x1;
> +        type >>= 1;
> +        n++;

Any specific reason for leaving client_disable_counters[0] unused?

> +        i <<= 1;

i is never read.

> +        if (!b) {
> +            continue;
> +        }
> +        if (ctx->client_disable_counters[n]) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}

In general I wonder whether this function really needs to take a mask
with possibly multiple set bits instead of just a single type.

> +void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
> +                                bool is_disable)
> +{
> +    int i = 1;
> +    int n = 0;
> +    aio_context_acquire(ctx);
> +
> +    while (clients_mask) {
> +        bool b = clients_mask & 0x1;
> +        clients_mask >>= 1;
> +        n++;
> +        i <<= 1;

This i isn't used either.

> +        if (!b) {
> +            continue;
> +        }
> +        if (ctx->client_disable_counters[n]) {
> +            return true;
> +        }

Wait, why are you checking the state instead of setting it?

How did you test this series?

> +    }
> +    aio_context_release(ctx);
> +}
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 60e796b..b687152 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -122,6 +122,8 @@ struct AioContext {
>  
>      /* TimerLists for calling timers - one per clock type */
>      QEMUTimerListGroup tlg;
> +
> +    int client_disable_counters[sizeof(int)];
>  };

sizeof return the size in bytes. I think you mean bits here?

Kevin

Re: [Qemu-devel] [PATCH 05/12] aio: introduce aio_{disable, enable}_clients

[Fam Zheng]

On Fri, 10/09 16:31, Kevin Wolf wrote:
> Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  aio-posix.c         |  3 ++-
> >  aio-win32.c         |  3 ++-
> >  async.c             | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/block/aio.h | 30 ++++++++++++++++++++++++++++++
> >  4 files changed, 76 insertions(+), 2 deletions(-)
> > 
> > diff --git a/aio-posix.c b/aio-posix.c
> > index d25fcfc..a261892 100644
> > --- a/aio-posix.c
> > +++ b/aio-posix.c
> > @@ -261,7 +261,8 @@ 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
> > +            && !aio_type_disabled(ctx, node->type)) {
> >              add_pollfd(node);
> >          }
> >      }
> > diff --git a/aio-win32.c b/aio-win32.c
> > index f5ecf57..66cff60 100644
> > --- a/aio-win32.c
> > +++ b/aio-win32.c
> > @@ -309,7 +309,8 @@ 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
> > +            && !aio_type_disabled(ctx, node->type)) {
> >              events[count++] = event_notifier_get_handle(node->e);
> >          }
> >      }
> > diff --git a/async.c b/async.c
> > index 244bf79..855b9d5 100644
> > --- a/async.c
> > +++ b/async.c
> > @@ -361,3 +361,45 @@ void aio_context_release(AioContext *ctx)
> >  {
> >      rfifolock_unlock(&ctx->lock);
> >  }
> > +
> > +bool aio_type_disabled(AioContext *ctx, int type)
> > +{
> > +    int i = 1;
> > +    int n = 0;
> > +
> > +    while (type) {
> > +        bool b = type & 0x1;
> > +        type >>= 1;
> > +        n++;
> 
> Any specific reason for leaving client_disable_counters[0] unused?

No, I should have started from 0.

> 
> > +        i <<= 1;
> 
> i is never read.
> 
> > +        if (!b) {
> > +            continue;
> > +        }
> > +        if (ctx->client_disable_counters[n]) {
> > +            return true;
> > +        }
> > +    }
> > +    return false;
> > +}
> 
> In general I wonder whether this function really needs to take a mask
> with possibly multiple set bits instead of just a single type.

Previous versions used to have more types than "internal" and "external", so it
has been a mask. So yes, I think a single type will be better now.

> 
> > +void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
> > +                                bool is_disable)
> > +{
> > +    int i = 1;
> > +    int n = 0;
> > +    aio_context_acquire(ctx);
> > +
> > +    while (clients_mask) {
> > +        bool b = clients_mask & 0x1;
> > +        clients_mask >>= 1;
> > +        n++;
> > +        i <<= 1;
> 
> This i isn't used either.
> 
> > +        if (!b) {
> > +            continue;
> > +        }
> > +        if (ctx->client_disable_counters[n]) {
> > +            return true;
> > +        }
> 
> Wait, why are you checking the state instead of setting it?

Oops, apparent I screwed my workspaces as I do remember coding this assignment.
And I must have used a wrong command when building the tree so that I don't
even catch the compiling error. :(

> 
> How did you test this series?

So far only smoke testing and qemu-iotests, because I don't have a good idea of
testifying the transaction's atomicity. Any suggestions?

> 
> > +    }
> > +    aio_context_release(ctx);
> > +}
> > diff --git a/include/block/aio.h b/include/block/aio.h
> > index 60e796b..b687152 100644
> > --- a/include/block/aio.h
> > +++ b/include/block/aio.h
> > @@ -122,6 +122,8 @@ struct AioContext {
> >  
> >      /* TimerLists for calling timers - one per clock type */
> >      QEMUTimerListGroup tlg;
> > +
> > +    int client_disable_counters[sizeof(int)];
> >  };
> 
> sizeof return the size in bytes. I think you mean bits here?

You're right, will fix it.

Fam

Re: [Qemu-devel] [PATCH 05/12] aio: introduce aio_{disable, enable}_clients

[Kevin Wolf]

Am 09.10.2015 um 18:27 hat Fam Zheng geschrieben:
> On Fri, 10/09 16:31, Kevin Wolf wrote:
> > Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  aio-posix.c         |  3 ++-
> > >  aio-win32.c         |  3 ++-
> > >  async.c             | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/block/aio.h | 30 ++++++++++++++++++++++++++++++
> > >  4 files changed, 76 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/aio-posix.c b/aio-posix.c
> > > index d25fcfc..a261892 100644
> > > --- a/aio-posix.c
> > > +++ b/aio-posix.c
> > > @@ -261,7 +261,8 @@ 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
> > > +            && !aio_type_disabled(ctx, node->type)) {
> > >              add_pollfd(node);
> > >          }
> > >      }
> > > diff --git a/aio-win32.c b/aio-win32.c
> > > index f5ecf57..66cff60 100644
> > > --- a/aio-win32.c
> > > +++ b/aio-win32.c
> > > @@ -309,7 +309,8 @@ 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
> > > +            && !aio_type_disabled(ctx, node->type)) {
> > >              events[count++] = event_notifier_get_handle(node->e);
> > >          }
> > >      }
> > > diff --git a/async.c b/async.c
> > > index 244bf79..855b9d5 100644
> > > --- a/async.c
> > > +++ b/async.c
> > > @@ -361,3 +361,45 @@ void aio_context_release(AioContext *ctx)
> > >  {
> > >      rfifolock_unlock(&ctx->lock);
> > >  }
> > > +
> > > +bool aio_type_disabled(AioContext *ctx, int type)
> > > +{
> > > +    int i = 1;
> > > +    int n = 0;
> > > +
> > > +    while (type) {
> > > +        bool b = type & 0x1;
> > > +        type >>= 1;
> > > +        n++;
> > 
> > Any specific reason for leaving client_disable_counters[0] unused?
> 
> No, I should have started from 0.
> 
> > 
> > > +        i <<= 1;
> > 
> > i is never read.
> > 
> > > +        if (!b) {
> > > +            continue;
> > > +        }
> > > +        if (ctx->client_disable_counters[n]) {
> > > +            return true;
> > > +        }
> > > +    }
> > > +    return false;
> > > +}
> > 
> > In general I wonder whether this function really needs to take a mask
> > with possibly multiple set bits instead of just a single type.
> 
> Previous versions used to have more types than "internal" and "external", so it
> has been a mask. So yes, I think a single type will be better now.
> 
> > 
> > > +void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
> > > +                                bool is_disable)
> > > +{
> > > +    int i = 1;
> > > +    int n = 0;
> > > +    aio_context_acquire(ctx);
> > > +
> > > +    while (clients_mask) {
> > > +        bool b = clients_mask & 0x1;
> > > +        clients_mask >>= 1;
> > > +        n++;
> > > +        i <<= 1;
> > 
> > This i isn't used either.
> > 
> > > +        if (!b) {
> > > +            continue;
> > > +        }
> > > +        if (ctx->client_disable_counters[n]) {
> > > +            return true;
> > > +        }
> > 
> > Wait, why are you checking the state instead of setting it?
> 
> Oops, apparent I screwed my workspaces as I do remember coding this assignment.
> And I must have used a wrong command when building the tree so that I don't
> even catch the compiling error. :(
> 
> > 
> > How did you test this series?
> 
> So far only smoke testing and qemu-iotests, because I don't have a good idea of
> testifying the transaction's atomicity. Any suggestions?

Perhaps you could use blkdebug to delay something in the middle of the
transaction while your guest keeps writing stuff? That should result in
100% reproducability.

I guess you actually need to make sure that your guest doesn't do any
I/O, then set the blkdebug breakpoint, send the transaction, and once a
request is stopped, you start some I/O in the guest. Resume as soon as
you know that something bad happened.

Possibly you need to add a new blkdebug event to find a good place to
suspend a transaction request.

Kevin

Re: [Qemu-devel] [PATCH 05/12] aio: introduce aio_{disable, enable}_clients

[Fam Zheng]

On Mon, 10/12 10:31, Kevin Wolf wrote:
> Am 09.10.2015 um 18:27 hat Fam Zheng geschrieben:
> > On Fri, 10/09 16:31, Kevin Wolf wrote:
> > > Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > > ---
> > > >  aio-posix.c         |  3 ++-
> > > >  aio-win32.c         |  3 ++-
> > > >  async.c             | 42 ++++++++++++++++++++++++++++++++++++++++++
> > > >  include/block/aio.h | 30 ++++++++++++++++++++++++++++++
> > > >  4 files changed, 76 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/aio-posix.c b/aio-posix.c
> > > > index d25fcfc..a261892 100644
> > > > --- a/aio-posix.c
> > > > +++ b/aio-posix.c
> > > > @@ -261,7 +261,8 @@ 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
> > > > +            && !aio_type_disabled(ctx, node->type)) {
> > > >              add_pollfd(node);
> > > >          }
> > > >      }
> > > > diff --git a/aio-win32.c b/aio-win32.c
> > > > index f5ecf57..66cff60 100644
> > > > --- a/aio-win32.c
> > > > +++ b/aio-win32.c
> > > > @@ -309,7 +309,8 @@ 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
> > > > +            && !aio_type_disabled(ctx, node->type)) {
> > > >              events[count++] = event_notifier_get_handle(node->e);
> > > >          }
> > > >      }
> > > > diff --git a/async.c b/async.c
> > > > index 244bf79..855b9d5 100644
> > > > --- a/async.c
> > > > +++ b/async.c
> > > > @@ -361,3 +361,45 @@ void aio_context_release(AioContext *ctx)
> > > >  {
> > > >      rfifolock_unlock(&ctx->lock);
> > > >  }
> > > > +
> > > > +bool aio_type_disabled(AioContext *ctx, int type)
> > > > +{
> > > > +    int i = 1;
> > > > +    int n = 0;
> > > > +
> > > > +    while (type) {
> > > > +        bool b = type & 0x1;
> > > > +        type >>= 1;
> > > > +        n++;
> > > 
> > > Any specific reason for leaving client_disable_counters[0] unused?
> > 
> > No, I should have started from 0.
> > 
> > > 
> > > > +        i <<= 1;
> > > 
> > > i is never read.
> > > 
> > > > +        if (!b) {
> > > > +            continue;
> > > > +        }
> > > > +        if (ctx->client_disable_counters[n]) {
> > > > +            return true;
> > > > +        }
> > > > +    }
> > > > +    return false;
> > > > +}
> > > 
> > > In general I wonder whether this function really needs to take a mask
> > > with possibly multiple set bits instead of just a single type.
> > 
> > Previous versions used to have more types than "internal" and "external", so it
> > has been a mask. So yes, I think a single type will be better now.
> > 
> > > 
> > > > +void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
> > > > +                                bool is_disable)
> > > > +{
> > > > +    int i = 1;
> > > > +    int n = 0;
> > > > +    aio_context_acquire(ctx);
> > > > +
> > > > +    while (clients_mask) {
> > > > +        bool b = clients_mask & 0x1;
> > > > +        clients_mask >>= 1;
> > > > +        n++;
> > > > +        i <<= 1;
> > > 
> > > This i isn't used either.
> > > 
> > > > +        if (!b) {
> > > > +            continue;
> > > > +        }
> > > > +        if (ctx->client_disable_counters[n]) {
> > > > +            return true;
> > > > +        }
> > > 
> > > Wait, why are you checking the state instead of setting it?
> > 
> > Oops, apparent I screwed my workspaces as I do remember coding this assignment.
> > And I must have used a wrong command when building the tree so that I don't
> > even catch the compiling error. :(
> > 
> > > 
> > > How did you test this series?
> > 
> > So far only smoke testing and qemu-iotests, because I don't have a good idea of
> > testifying the transaction's atomicity. Any suggestions?
> 
> Perhaps you could use blkdebug to delay something in the middle of the
> transaction while your guest keeps writing stuff? That should result in
> 100% reproducability.
> 
> I guess you actually need to make sure that your guest doesn't do any
> I/O, then set the blkdebug breakpoint, send the transaction, and once a
> request is stopped, you start some I/O in the guest. Resume as soon as
> you know that something bad happened.
> 
> Possibly you need to add a new blkdebug event to find a good place to
> suspend a transaction request.
> 

It's difficult to "start some I/O" in the guest in the middle of transaction,
even with help of blkdebug, because BQL is hold during the whole transaction.

I think it would be a bit easier to program a VCPU to constantly submit I/O
requests to the vq, but that's far from enough.

Anyway I'll start by writing some unit test code instead, in tests/test-aio.c.

Fam

[Qemu-devel] [PATCH 06/12] block: Introduce "drained begin/end" API

[Fam Zheng]

The semantics is that after bdrv_drained_begin(bs), bs will not get new external
requests until the matching bdrv_drained_end(bs).

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   |  2 ++
 block/io.c                | 18 ++++++++++++++++++
 include/block/block.h     | 19 +++++++++++++++++++
 include/block/block_int.h |  2 ++
 4 files changed, 41 insertions(+)

diff --git a/block.c b/block.c
index 1f90b47..9b28a07 100644
--- a/block.c
+++ b/block.c
@@ -2058,6 +2058,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     bs_dest->device_list = bs_src->device_list;
     bs_dest->blk = bs_src->blk;
 
+    bs_dest->quiesce_counter = bs_src->quiesce_counter;
+
     memcpy(bs_dest->op_blockers, bs_src->op_blockers,
            sizeof(bs_dest->op_blockers));
 }
diff --git a/block/io.c b/block/io.c
index 94e18e6..b0dde7f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2618,3 +2618,21 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
     }
     bdrv_start_throttled_reqs(bs);
 }
+
+void bdrv_drained_begin(BlockDriverState *bs)
+{
+    if (bs->quiesce_counter++) {
+        return;
+    }
+    aio_disable_clients(bdrv_get_aio_context(bs), AIO_CLIENT_EXTERNAL);
+    bdrv_drain(bs);
+}
+
+void bdrv_drained_end(BlockDriverState *bs)
+{
+    assert(bs->quiesce_counter > 0);
+    if (--bs->quiesce_counter > 0) {
+        return;
+    }
+    aio_enable_clients(bdrv_get_aio_context(bs), AIO_CLIENT_EXTERNAL);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 2dd6630..c4f6eef 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -619,4 +619,23 @@ void bdrv_flush_io_queue(BlockDriverState *bs);
 
 BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
 
+/**
+ * bdrv_drained_begin:
+ *
+ * Begin a quiesced section for exclusive access to the BDS, by disabling
+ * external request sources including NBD server and device model. Note that
+ * this doesn't block timers or coroutines from submitting more requests, which
+ * means block_job_pause is still necessary.
+ *
+ * This function can be recursive.
+ */
+void bdrv_drained_begin(BlockDriverState *bs);
+
+/**
+ * bdrv_drained_end:
+ *
+ * End a quiescent section started by bdrv_drained_begin().
+ */
+void bdrv_drained_end(BlockDriverState *bs);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 14ad4c3..7c58221 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -456,6 +456,8 @@ struct BlockDriverState {
     /* threshold limit for writes, in bytes. "High water mark". */
     uint64_t write_threshold_offset;
     NotifierWithReturn write_threshold_notifier;
+
+    int quiesce_counter;
 };
 
 
-- 
2.5.3

[Qemu-devel] [PATCH 07/12] block: Add "drained begin/end" for transactional external snapshot

[Fam Zheng]

This ensures the atomicity of the transaction by avoiding processing of
external requests such as those from ioeventfd.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 32b04b4..90f1e15 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1479,6 +1479,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     /* Acquire AioContext now so any threads operating on old_bs stop */
     state->aio_context = bdrv_get_aio_context(state->old_bs);
     aio_context_acquire(state->aio_context);
+    bdrv_drained_begin(state->old_bs);
 
     if (!bdrv_is_inserted(state->old_bs)) {
         error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
@@ -1548,8 +1549,6 @@ static void external_snapshot_commit(BlkTransactionState *common)
      * don't want to abort all of them if one of them fails the reopen */
     bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR,
                 NULL);
-
-    aio_context_release(state->aio_context);
 }
 
 static void external_snapshot_abort(BlkTransactionState *common)
@@ -1559,7 +1558,14 @@ static void external_snapshot_abort(BlkTransactionState *common)
     if (state->new_bs) {
         bdrv_unref(state->new_bs);
     }
+}
+
+static void external_snapshot_clean(BlkTransactionState *common)
+{
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
     if (state->aio_context) {
+        bdrv_drained_end(state->old_bs);
         aio_context_release(state->aio_context);
     }
 }
@@ -1724,6 +1730,7 @@ static const BdrvActionOps actions[] = {
         .prepare  = external_snapshot_prepare,
         .commit   = external_snapshot_commit,
         .abort = external_snapshot_abort,
+        .clean = external_snapshot_clean,
     },
     [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = {
         .instance_size = sizeof(DriveBackupState),
-- 
2.5.3

[Qemu-devel] [PATCH 08/12] block: Add "drained begin/end" for transactional backup

[Fam Zheng]

This ensures the atomicity of the transaction by avoiding processing of
external requests such as those from ioeventfd.

Move the assignment to state->bs up right after bdrv_drained_begin, so
that we can use it in the clean callback. The abort callback will still
check bs->job and state->job, so it's OK.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 90f1e15..232bc21 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1599,6 +1599,8 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
+    bdrv_drained_begin(bs);
+    state->bs = bs;
 
     qmp_drive_backup(backup->device, backup->target,
                      backup->has_format, backup->format,
@@ -1614,7 +1616,6 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
         return;
     }
 
-    state->bs = bs;
     state->job = state->bs->job;
 }
 
@@ -1634,6 +1635,7 @@ static void drive_backup_clean(BlkTransactionState *common)
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
 
     if (state->aio_context) {
+        bdrv_drained_end(state->bs);
         aio_context_release(state->aio_context);
     }
 }
-- 
2.5.3

[Qemu-devel] [PATCH 09/12] block: Add "drained begin/end" for transactional blockdev-backup

[Fam Zheng]

Similar to the previous patch, make sure that external events are not
dispatched during transaction operations.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 232bc21..015afbf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1680,6 +1680,8 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
         return;
     }
     aio_context_acquire(state->aio_context);
+    state->bs = bs;
+    bdrv_drained_begin(bs);
 
     qmp_blockdev_backup(backup->device, backup->target,
                         backup->sync,
@@ -1692,7 +1694,6 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
         return;
     }
 
-    state->bs = bs;
     state->job = state->bs->job;
 }
 
@@ -1712,6 +1713,7 @@ static void blockdev_backup_clean(BlkTransactionState *common)
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
 
     if (state->aio_context) {
+        bdrv_drained_end(state->bs);
         aio_context_release(state->aio_context);
     }
 }
-- 
2.5.3

[Qemu-devel] [PATCH 10/12] block: Add "drained begin/end" for internal snapshot

[Fam Zheng]

This ensures the atomicity of the transaction by avoiding processing of
external requests such as those from ioeventfd.

state->bs is assigned right after bdrv_drained_begin. Because it was
used as the flag for deletion or not in abort, now we need a separate
flag - InternalSnapshotState.created.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 015afbf..c3da2c6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1280,6 +1280,7 @@ typedef struct InternalSnapshotState {
     BlockDriverState *bs;
     AioContext *aio_context;
     QEMUSnapshotInfo sn;
+    bool created;
 } InternalSnapshotState;
 
 static void internal_snapshot_prepare(BlkTransactionState *common,
@@ -1318,6 +1319,8 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
+    bdrv_drained_begin(bs);
+    state->bs = bs;
 
     if (!bdrv_is_inserted(bs)) {
         error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
@@ -1375,7 +1378,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
     }
 
     /* 4. succeed, mark a snapshot is created */
-    state->bs = bs;
+    state->created = true;
 }
 
 static void internal_snapshot_abort(BlkTransactionState *common)
@@ -1386,7 +1389,7 @@ static void internal_snapshot_abort(BlkTransactionState *common)
     QEMUSnapshotInfo *sn = &state->sn;
     Error *local_error = NULL;
 
-    if (!bs) {
+    if (!state->created) {
         return;
     }
 
@@ -1407,6 +1410,7 @@ static void internal_snapshot_clean(BlkTransactionState *common)
                                              common, common);
 
     if (state->aio_context) {
+        bdrv_drained_end(state->bs);
         aio_context_release(state->aio_context);
     }
 }
-- 
2.5.3

[Qemu-devel] [PATCH 11/12] block: Introduce BlockDriver.bdrv_drain callback

[Fam Zheng]

Drivers can have internal request sources that generates IO, like the
need_check_timer in QED. Since we want quiesced periods that contain
nested event loops in block layer, we need to have a way to disable such
event sources.

Block drivers must implement the "bdrv_drain" callback if it has any
internal sources that can generate I/O activity, like a timer or a
worker thread (even in a library) that can schedule QEMUBH in an
asynchronous callback.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c                | 3 +++
 include/block/block_int.h | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/block/io.c b/block/io.c
index b0dde7f..ad4b456 100644
--- a/block/io.c
+++ b/block/io.c
@@ -247,6 +247,9 @@ void bdrv_drain(BlockDriverState *bs)
 {
     bool busy = true;
 
+    if (bs->drv && bs->drv->bdrv_drain) {
+        bs->drv->bdrv_drain(bs);
+    }
     while (busy) {
         /* Keep iterating */
          bdrv_flush_io_queue(bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7c58221..99359b2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -288,6 +288,12 @@ struct BlockDriver {
      */
     int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
 
+    /**
+     * Drain and stop any internal sources of requests in the driver, and
+     * remain so until next I/O callback (e.g. bdrv_co_writev) is called.
+     */
+    void (*bdrv_drain)(BlockDriverState *bs);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.5.3

[Qemu-devel] [PATCH 12/12] qed: Implement .bdrv_drain

[Fam Zheng]

The "need_check_timer" is used to clear the "NEED_CHECK" flag in the
image header after a grace period once metadata update has finished. In
compliance to the bdrv_drain semantics we should make sure it remains
deleted once .bdrv_drain is called.

Call the qed_need_check_timer_cb manually to update the header
immediately.

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

diff --git a/block/qed.c b/block/qed.c
index a7ff1d9..23bd273 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -381,6 +381,12 @@ static void bdrv_qed_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static void bdrv_qed_drain(BlockDriverState *bs)
+{
+    qed_cancel_need_check_timer(bs->opaque);
+    qed_need_check_timer_cb(bs->opaque);
+}
+
 static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
@@ -1683,6 +1689,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_check               = bdrv_qed_check,
     .bdrv_detach_aio_context  = bdrv_qed_detach_aio_context,
     .bdrv_attach_aio_context  = bdrv_qed_attach_aio_context,
+    .bdrv_drain               = bdrv_qed_drain,
 };
 
 static void bdrv_qed_init(void)
-- 
2.5.3