Patchwork [01/13] Add aiocb_mutex and aiocb_completion.

login
register
mail settings
Submitter Arun Bharadwaj
Date Jan. 4, 2011, 5:27 a.m.
Message ID <20110104052708.15887.7563.stgit@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/77377/
State New
Headers show

Comments

Arun Bharadwaj - Jan. 4, 2011, 5:27 a.m.
This patch adds the aiocb_mutex to protect aiocb.
This patch also removes the infinite loop present in paio_cancel.

Since there can only be one cancellation at a time, we need to
introduce a condition variable. For this, we need a
global aiocb_completion condition variable.

This patch also adds the Makefile entry to compile qemu-thread.c
when CONFIG_POSIX is set, instead of the unused CONFIG_THREAD.

Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
---
 Makefile.objs      |    2 +-
 posix-aio-compat.c |   37 ++++++++++++++++++++++++++++---------
 2 files changed, 29 insertions(+), 10 deletions(-)
Stefan Hajnoczi - Jan. 5, 2011, 7:53 p.m.
On Tue, Jan 04, 2011 at 10:57:08AM +0530, Arun R Bharadwaj wrote:
> @@ -545,13 +555,19 @@ static void paio_cancel(BlockDriverAIOCB *blockacb)
>      }
>      mutex_unlock(&lock);
>  
> -    if (active) {
> -        /* fail safe: if the aio could not be canceled, we wait for
> -           it */
> -        while (qemu_paio_error(acb) == EINPROGRESS)
> -            ;
> +    qemu_mutex_lock(&aiocb_mutex);
> +    if (!active) {
> +        acb->ret = -ECANCELED;
> +    } else {
> +        while (acb->ret == -EINPROGRESS) {
> +            /*
> +             * fail safe: if the aio could not be canceled,
> +             * we wait for it
> +             */
> +            qemu_cond_wait(&aiocb_completion, &aiocb_mutex);
> +        }
>      }
> -
> +    qemu_mutex_unlock(&aiocb_mutex);
>      paio_remove(acb);
>  }

acb->ret and acb->active have been moved under aiocb_mutex.  They are still
accessed under lock here and this needs to be fixed:

mutex_lock(&lock);
if (!acb->active) {
    QTAILQ_REMOVE(&request_list, acb, node);
    acb->ret = -ECANCELED;
} else if (acb->ret == -EINPROGRESS) {
    active = 1;
}
mutex_unlock(&lock);

Stefan
Arun Bharadwaj - Jan. 6, 2011, 10:27 a.m.
* Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> [2011-01-05 19:53:44]:

> On Tue, Jan 04, 2011 at 10:57:08AM +0530, Arun R Bharadwaj wrote:
> > @@ -545,13 +555,19 @@ static void paio_cancel(BlockDriverAIOCB *blockacb)
> >      }
> >      mutex_unlock(&lock);
> >  
> > -    if (active) {
> > -        /* fail safe: if the aio could not be canceled, we wait for
> > -           it */
> > -        while (qemu_paio_error(acb) == EINPROGRESS)
> > -            ;
> > +    qemu_mutex_lock(&aiocb_mutex);
> > +    if (!active) {
> > +        acb->ret = -ECANCELED;
> > +    } else {
> > +        while (acb->ret == -EINPROGRESS) {
> > +            /*
> > +             * fail safe: if the aio could not be canceled,
> > +             * we wait for it
> > +             */
> > +            qemu_cond_wait(&aiocb_completion, &aiocb_mutex);
> > +        }
> >      }
> > -
> > +    qemu_mutex_unlock(&aiocb_mutex);
> >      paio_remove(acb);
> >  }
> 
> acb->ret and acb->active have been moved under aiocb_mutex.  They are still
> accessed under lock here and this needs to be fixed:
> 
> mutex_lock(&lock);
> if (!acb->active) {
>     QTAILQ_REMOVE(&request_list, acb, node);
>     acb->ret = -ECANCELED;
> } else if (acb->ret == -EINPROGRESS) {
>     active = 1;
> }
> mutex_unlock(&lock);
> 

You are right. This needs to go under aiocb_mutex too.

-arun

> Stefan

Patch

diff --git a/Makefile.objs b/Makefile.objs
index cd5a24b..3b7ec27 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -9,6 +9,7 @@  qobject-obj-y += qerror.o
 
 block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
 block-obj-y += nbd.o block.o aio.o aes.o osdep.o qemu-config.o
+block-obj-$(CONFIG_POSIX) += qemu-thread.o
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 
@@ -124,7 +125,6 @@  endif
 common-obj-y += $(addprefix ui/, $(ui-obj-y))
 
 common-obj-y += iov.o acl.o
-common-obj-$(CONFIG_THREAD) += qemu-thread.o
 common-obj-y += notify.o event_notifier.o
 common-obj-y += qemu-timer.o
 
diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index 7b862b5..ae5e20e 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -27,9 +27,12 @@ 
 #include "qemu-common.h"
 #include "trace.h"
 #include "block_int.h"
+#include "qemu-thread.h"
 
 #include "block/raw-posix-aio.h"
 
+static QemuMutex aiocb_mutex;
+static QemuCond aiocb_completion;
 
 struct qemu_paiocb {
     BlockDriverAIOCB common;
@@ -351,10 +354,14 @@  static void *aio_thread(void *unused)
         }
 
         mutex_lock(&lock);
-        aiocb->ret = ret;
         idle_threads++;
         mutex_unlock(&lock);
 
+        qemu_mutex_lock(&aiocb_mutex);
+        aiocb->ret = ret;
+        qemu_cond_broadcast(&aiocb_completion);
+        qemu_mutex_unlock(&aiocb_mutex);
+
         if (kill(pid, aiocb->ev_signo)) die("kill failed");
     }
 
@@ -383,8 +390,11 @@  static void spawn_thread(void)
 
 static void qemu_paio_submit(struct qemu_paiocb *aiocb)
 {
+    qemu_mutex_lock(&aiocb_mutex);
     aiocb->ret = -EINPROGRESS;
     aiocb->active = 0;
+    qemu_mutex_unlock(&aiocb_mutex);
+
     mutex_lock(&lock);
     if (idle_threads == 0 && cur_threads < max_threads)
         spawn_thread();
@@ -397,9 +407,9 @@  static ssize_t qemu_paio_return(struct qemu_paiocb *aiocb)
 {
     ssize_t ret;
 
-    mutex_lock(&lock);
+    qemu_mutex_lock(&aiocb_mutex);
     ret = aiocb->ret;
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&aiocb_mutex);
 
     return ret;
 }
@@ -545,13 +555,19 @@  static void paio_cancel(BlockDriverAIOCB *blockacb)
     }
     mutex_unlock(&lock);
 
-    if (active) {
-        /* fail safe: if the aio could not be canceled, we wait for
-           it */
-        while (qemu_paio_error(acb) == EINPROGRESS)
-            ;
+    qemu_mutex_lock(&aiocb_mutex);
+    if (!active) {
+        acb->ret = -ECANCELED;
+    } else {
+        while (acb->ret == -EINPROGRESS) {
+            /*
+             * fail safe: if the aio could not be canceled,
+             * we wait for it
+             */
+            qemu_cond_wait(&aiocb_completion, &aiocb_mutex);
+        }
     }
-
+    qemu_mutex_unlock(&aiocb_mutex);
     paio_remove(acb);
 }
 
@@ -623,6 +639,9 @@  int paio_init(void)
     if (posix_aio_state)
         return 0;
 
+    qemu_mutex_init(&aiocb_mutex);
+    qemu_cond_init(&aiocb_completion);
+
     s = qemu_malloc(sizeof(PosixAioState));
 
     sigfillset(&act.sa_mask);