| Message ID | 20110104052708.15887.7563.stgit@localhost6.localdomain6 |
|---|---|
| State | New |
| Headers | show |
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
* 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
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);
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(-)