From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42389) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qrjhr-0006v1-Kg for qemu-devel@nongnu.org; Fri, 12 Aug 2011 00:50:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qrjhq-0004Tg-GM for qemu-devel@nongnu.org; Fri, 12 Aug 2011 00:50:51 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:40856) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qrjhq-0004TW-Dm for qemu-devel@nongnu.org; Fri, 12 Aug 2011 00:50:50 -0400 Received: by ywf9 with SMTP id 9so2088190ywf.4 for ; Thu, 11 Aug 2011 21:50:49 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1312863472-6901-1-git-send-email-wuzhy@linux.vnet.ibm.com> <1312863472-6901-3-git-send-email-wuzhy@linux.vnet.ibm.com> <20110810093711.GD28852@stefanha-thinkpad.localdomain> Date: Fri, 12 Aug 2011 05:50:49 +0100 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 2/4] block: add the block queue support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhi Yong Wu Cc: kwolf@redhat.com, pair@us.ibm.com, Stefan Hajnoczi , kvm@vger.kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, ryanh@us.ibm.com, Zhi Yong Wu , luowenj@cn.ibm.com On Fri, Aug 12, 2011 at 5:40 AM, Zhi Yong Wu wrote: > On Wed, Aug 10, 2011 at 5:37 PM, Stefan Hajnoczi > wrote: >> On Wed, Aug 10, 2011 at 01:54:33PM +0800, Zhi Yong Wu wrote: >>> On Tue, Aug 9, 2011 at 8:49 PM, Stefan Hajnoczi wr= ote: >>> > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu wrote: >>> >> +BlockDriverAIOCB *qemu_block_queue_enqueue(BlockQueue *queue, >>> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BlockDriverState *b= s, >>> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BlockRequestHandler= *handler, >>> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int64_t sector_num, >>> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0QEMUIOVector *qiov, >>> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int nb_sectors, >>> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BlockDriverCompleti= onFunc *cb, >>> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0void *opaque) >>> >> +{ >>> >> + =A0 =A0BlockIORequest *request; >>> >> + =A0 =A0BlockDriverAIOCB *acb; >>> >> + >>> >> + =A0 =A0request =3D qemu_malloc(sizeof(BlockIORequest)); >>> >> + =A0 =A0request->bs =3D bs; >>> >> + =A0 =A0request->handler =3D handler; >>> >> + =A0 =A0request->sector_num =3D sector_num; >>> >> + =A0 =A0request->qiov =3D qiov; >>> >> + =A0 =A0request->nb_sectors =3D nb_sectors; >>> >> + =A0 =A0request->cb =3D cb; >>> >> + =A0 =A0request->opaque =3D opaque; >>> >> + >>> >> + =A0 =A0QTAILQ_INSERT_TAIL(&queue->requests, request, entry); >>> > >>> > It would be simpler to define BlockQueueAIOCB and using it as our acb >>> > instead of managing an extra BlockIORequest structure. =A0That way yo= u >>> > don't need to worry about extra mallocs and frees. >>> Sorry, i don't get what you mean. how to define it? Can you elaborate? >> >> BlockDriverAIOCB is designed to be embedded inside a bigger struct. =A0F= or >> example, QEDAIOCB is a larger struct that contains BlockDriverAIOCB as >> its first field: >> >> typedef struct QEDAIOCB { >> =A0 =A0BlockDriverAIOCB common; >> =A0 =A0... >> } QEDAIOCB; >> >> And the QED AIOPool contains the size of QEDAIOCB so that qemu_aio_get()= can >> allocate the full QEDAIOCB struct: >> >> static AIOPool qed_aio_pool =3D { >> =A0 =A0.aiocb_size =A0 =A0 =A0 =A0 =3D sizeof(QEDAIOCB), >> =A0 =A0.cancel =A0 =A0 =A0 =A0 =A0 =A0 =3D qed_aio_cancel, >> }; >> >> This allows QED to store per-request state in QEDAIOCB for the lifetime = of a >> request: >> >> QEDAIOCB *acb =3D qemu_aio_get(&qed_aio_pool, bs, cb, opaque); >> >> acb->is_write =3D is_write; >> acb->finished =3D NULL; >> acb->qiov =3D qiov; >> ... >> >> I suggest creating a BlockQueueAIOCB that contains the fields from >> BlockIORequest (which is no longer needed as a separate struct): >> >> typedef struct BlockQueueAIOCB { >> =A0 =A0BlockDriverAIOCB common; >> =A0 =A0BlockRequestHandler *handler; >> =A0 =A0int64_t sector_num; >> =A0 =A0QEMUIOVector *qiov; >> =A0 =A0int nb_sectors; >> =A0 =A0QTAILQ_ENTRY(BlockQueueAIOCB) entry; /* pending request queue */ >> } BlockQueueAIOCB; >> >> Now you can drop the malloc and simply qemu_aio_get() a new >> BlockQueueAIOCB. > Yesterday, i tried this. To be honest, i do not like this solution.:). > It results in the code logic to be a bit messy, not clear. > A AIOCB struct is treated as a block request and enqueued into block > queue. It is a bit weird. So i would like to retain BlockIORequest and > define BlockQueueAIOCB pool. This struct is used to store some useful > info: > struct BlockQueueAIOCB { > =A0 =A0BlockDriverAIOCB common; > =A0 =A0bool dispatched; > =A0 =A0BlockDriverAIOCB *real_acb; > =A0 =A0BlockIORequest *request; > }; > > What do you think of this? Try it out and let's see the patch :). Stefan