From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=44463 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PFmyD-0000Ta-J6 for qemu-devel@nongnu.org; Tue, 09 Nov 2010 07:06:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PFmy7-0005Vi-BB for qemu-devel@nongnu.org; Tue, 09 Nov 2010 07:06:37 -0500 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:40575) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PFmy5-0005TD-Ri for qemu-devel@nongnu.org; Tue, 09 Nov 2010 07:06:31 -0500 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by e28smtp05.in.ibm.com (8.14.4/8.13.1) with ESMTP id oA9C6DHf000471 for ; Tue, 9 Nov 2010 17:36:13 +0530 Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id oA9C6CAu2978014 for ; Tue, 9 Nov 2010 17:36:12 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id oA9C6CUF031920 for ; Tue, 9 Nov 2010 23:06:12 +1100 Date: Tue, 9 Nov 2010 17:36:04 +0530 From: Arun R Bharadwaj Subject: Re: [Qemu-devel] [PATCH 1/3] Make paio subsystem use threadlets infrastructure Message-ID: <20101109120604.GA3395@linux.vnet.ibm.com> References: <20101108104542.6769.22583.stgit@localhost6.localdomain6> <20101108143322.GA10435@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Reply-To: arun@linux.vnet.ibm.com List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org * Stefan Hajnoczi [2010-11-08 21:29:12]: > On Mon, Nov 8, 2010 at 2:33 PM, Arun R Bharadwaj > wrote: > > 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 > Hi Stefan, Thanks for the quick review. > This change makes CONFIG_THREAD unused. The ./configure code that > sets CONFIG_THREAD=y should be removed. > I'll remove this. > > diff --git a/posix-aio-compat.c b/posix-aio-compat.c > > index 7b862b5..00b2a4e 100644 > > --- a/posix-aio-compat.c > > +++ b/posix-aio-compat.c > > @@ -29,7 +29,32 @@ > > #include "block_int.h" > > > > #include "block/raw-posix-aio.h" > > +#include "qemu-thread.h" > > > > +#define MAX_GLOBAL_THREADS 64 > > +#define MIN_GLOBAL_THREADS 8 > > + > > +QemuMutex aiocb_mutex; > > This variable should be static since it isn't used externally. > > > + > > +static void aio_thread(ThreadletWork *work) > > { > > pid_t pid; > > + struct qemu_paiocb *aiocb = container_of(work, struct qemu_paiocb, work); > > + ssize_t ret = 0; > > > > pid = getpid(); > > + aiocb->active = 1; > > aiocb->active needs to be assigned with aiocb_mutex held and then released in > order for this memory write to be visible to other threads after this > line of code. > Yes. That makes sense. We definitely need to hold the mutex here. > > > > 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; > > } > > > > @@ -536,20 +619,20 @@ static void paio_cancel(BlockDriverAIOCB *blockacb) > > struct qemu_paiocb *acb = (struct qemu_paiocb *)blockacb; > > int active = 0; > > > > - mutex_lock(&lock); > > if (!acb->active) { > > - QTAILQ_REMOVE(&request_list, acb, node); > > - acb->ret = -ECANCELED; > > + if (!dequeue_work(&acb->work)) { > > + acb->ret = -ECANCELED; > > + } else { > > + active = 1; > > + } > > } else if (acb->ret == -EINPROGRESS) { > > active = 1; > > } > > - mutex_unlock(&lock); > > > > if (active) { > > /* fail safe: if the aio could not be canceled, we wait for > > it */ > > - while (qemu_paio_error(acb) == EINPROGRESS) > > - ; > > + active = qemu_paio_error(acb); > > } > > > > paio_remove(acb); > > acb->ret is not being consistently accessed with aiocb_mutex held. > > We don't wait for the work item to complete if it is active. This changes the > semantics of paio_cancel() and will break callers who expect the request to be > cancelled/completed when paio_cancel() returns. Also, we go ahead and free the > acb for a running request which is dangerous because it may be reused and > corrupted. > > I think both the active variable and field in qemu_paiocb are unnecessary > because dequeue_work() already deals with inactive work items. If > dequeue_work() was unsuccessful you need to wait until ret != -EINPROGRESS. > So this would mean that we can use the earlier infinite while loop right? while (qemu_paio_error(acb) == EINPROGRESS) ; We can just take this outside the if (active) condition check. -arun > Stefan