Patchwork [05/26] aio: return "AIO in progress" state from qemu_aio_wait

login
register
mail settings
Submitter Paolo Bonzini
Date April 12, 2012, noon
Message ID <1334232076-19018-6-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/152042/
State New
Headers show

Comments

Paolo Bonzini - April 12, 2012, noon
The definition of when qemu_aio_flush should loop is much simpler
than it looks.  It just has to call qemu_aio_wait until it makes
no progress and all flush callbacks return false.  qemu_aio_wait
is the logical place to tell the caller about this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 aio.c      |   44 ++++++++++++++++++--------------------------
 qemu-aio.h |    6 ++++--
 2 files changed, 22 insertions(+), 28 deletions(-)
Kevin Wolf - April 19, 2012, 2:50 p.m.
Am 12.04.2012 14:00, schrieb Paolo Bonzini:
> The definition of when qemu_aio_flush should loop is much simpler
> than it looks.  It just has to call qemu_aio_wait until it makes
> no progress and all flush callbacks return false.  qemu_aio_wait
> is the logical place to tell the caller about this.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  aio.c      |   44 ++++++++++++++++++--------------------------
>  qemu-aio.h |    6 ++++--
>  2 files changed, 22 insertions(+), 28 deletions(-)
> 
> diff --git a/aio.c b/aio.c
> index f19b3c6..5fcc0c6 100644
> --- a/aio.c
> +++ b/aio.c
> @@ -99,41 +99,26 @@ int qemu_aio_set_fd_handler(int fd,
>  
>  void qemu_aio_flush(void)
>  {
> -    AioHandler *node;
> -    int ret;
> -
> -    do {
> -        ret = 0;
> -
> -	/*
> -	 * If there are pending emulated aio start them now so flush
> -	 * will be able to return 1.
> -	 */
> -        qemu_aio_wait();
> -
> -        QLIST_FOREACH(node, &aio_handlers, node) {
> -            if (node->io_flush) {
> -                ret |= node->io_flush(node->opaque);
> -            }
> -        }
> -    } while (qemu_bh_poll() || ret > 0);
> +    while (qemu_aio_wait());
>  }
>  
> -void qemu_aio_wait(void)
> +bool qemu_aio_wait(void)
>  {
>      int ret;
>  
>      /*
>       * If there are callbacks left that have been queued, we need to call then.
> -     * Return afterwards to avoid waiting needlessly in select().
> +     * Do not call select in this case, because it is possible that the caller
> +     * does not need a complete flush (as is the case for qemu_aio_wait loops).
>       */
>      if (qemu_bh_poll()) {
> -        return;
> +        return true;
>      }
>  
>      do {
>          AioHandler *node;
>          fd_set rdfds, wrfds;
> +        bool busy;
>          int max_fd = -1;
>  
>          walking_handlers = 1;
> @@ -142,14 +127,18 @@ void qemu_aio_wait(void)
>          FD_ZERO(&wrfds);
>  
>          /* fill fd sets */
> +        busy = false;
>          QLIST_FOREACH(node, &aio_handlers, node) {
>              /* If there aren't pending AIO operations, don't invoke callbacks.
>               * Otherwise, if there are no AIO requests, qemu_aio_wait() would
>               * wait indefinitely.
>               */
> -            if (node->io_flush && node->io_flush(node->opaque) == 0)
> -                continue;
> -
> +            if (node->io_flush) {
> +                if (node->io_flush(node->opaque) == 0) {
> +                    continue;
> +                }
> +                busy = true;
> +            }
>              if (!node->deleted && node->io_read) {
>                  FD_SET(node->fd, &rdfds);
>                  max_fd = MAX(max_fd, node->fd + 1);
> @@ -163,8 +152,9 @@ void qemu_aio_wait(void)
>          walking_handlers = 0;
>  
>          /* No AIO operations?  Get us out of here */
> -        if (max_fd == -1)
> -            break;
> +        if (!busy) {
> +            return false;
> +        }

This can change the behaviour for aio_handlers that don't have a flush
callback. Previously we would run into the select, now we don't.

Hm, okay, such handlers don't exist. Maybe we should assert() it
somewhere, but I guess then it's not a problem with this patch.

Kevin

Patch

diff --git a/aio.c b/aio.c
index f19b3c6..5fcc0c6 100644
--- a/aio.c
+++ b/aio.c
@@ -99,41 +99,26 @@  int qemu_aio_set_fd_handler(int fd,
 
 void qemu_aio_flush(void)
 {
-    AioHandler *node;
-    int ret;
-
-    do {
-        ret = 0;
-
-	/*
-	 * If there are pending emulated aio start them now so flush
-	 * will be able to return 1.
-	 */
-        qemu_aio_wait();
-
-        QLIST_FOREACH(node, &aio_handlers, node) {
-            if (node->io_flush) {
-                ret |= node->io_flush(node->opaque);
-            }
-        }
-    } while (qemu_bh_poll() || ret > 0);
+    while (qemu_aio_wait());
 }
 
-void qemu_aio_wait(void)
+bool qemu_aio_wait(void)
 {
     int ret;
 
     /*
      * If there are callbacks left that have been queued, we need to call then.
-     * Return afterwards to avoid waiting needlessly in select().
+     * Do not call select in this case, because it is possible that the caller
+     * does not need a complete flush (as is the case for qemu_aio_wait loops).
      */
     if (qemu_bh_poll()) {
-        return;
+        return true;
     }
 
     do {
         AioHandler *node;
         fd_set rdfds, wrfds;
+        bool busy;
         int max_fd = -1;
 
         walking_handlers = 1;
@@ -142,14 +127,18 @@  void qemu_aio_wait(void)
         FD_ZERO(&wrfds);
 
         /* fill fd sets */
+        busy = false;
         QLIST_FOREACH(node, &aio_handlers, node) {
             /* If there aren't pending AIO operations, don't invoke callbacks.
              * Otherwise, if there are no AIO requests, qemu_aio_wait() would
              * wait indefinitely.
              */
-            if (node->io_flush && node->io_flush(node->opaque) == 0)
-                continue;
-
+            if (node->io_flush) {
+                if (node->io_flush(node->opaque) == 0) {
+                    continue;
+                }
+                busy = true;
+            }
             if (!node->deleted && node->io_read) {
                 FD_SET(node->fd, &rdfds);
                 max_fd = MAX(max_fd, node->fd + 1);
@@ -163,8 +152,9 @@  void qemu_aio_wait(void)
         walking_handlers = 0;
 
         /* No AIO operations?  Get us out of here */
-        if (max_fd == -1)
-            break;
+        if (!busy) {
+            return false;
+        }
 
         /* wait until next event */
         ret = select(max_fd, &rdfds, &wrfds, NULL, NULL);
@@ -204,4 +194,6 @@  void qemu_aio_wait(void)
             walking_handlers = 0;
         }
     } while (ret == 0);
+
+    return true;
 }
diff --git a/qemu-aio.h b/qemu-aio.h
index 0fc8409..bfdd35f 100644
--- a/qemu-aio.h
+++ b/qemu-aio.h
@@ -48,8 +48,10 @@  void qemu_aio_flush(void);
 /* Wait for a single AIO completion to occur.  This function will wait
  * until a single AIO event has completed and it will ensure something
  * has moved before returning. This can issue new pending aio as
- * result of executing I/O completion or bh callbacks. */
-void qemu_aio_wait(void);
+ * result of executing I/O completion or bh callbacks.
+ *
+ * Return whether there is still any pending AIO operation.  */
+bool qemu_aio_wait(void);
 
 /* Register a file descriptor and associated callbacks.  Behaves very similarly
  * to qemu_set_fd_handler2.  Unlike qemu_set_fd_handler2, these callbacks will