From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ki95a-0008Ni-L1 for qemu-devel@nongnu.org; Tue, 23 Sep 2008 10:42:06 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Ki95Y-0008Mx-Oh for qemu-devel@nongnu.org; Tue, 23 Sep 2008 10:42:06 -0400 Received: from [199.232.76.173] (port=47771 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ki95Y-0008Mr-GG for qemu-devel@nongnu.org; Tue, 23 Sep 2008 10:42:04 -0400 Received: from e6.ny.us.ibm.com ([32.97.182.146]:49892) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Ki95Y-0000o4-9A for qemu-devel@nongnu.org; Tue, 23 Sep 2008 10:42:04 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e6.ny.us.ibm.com (8.13.8/8.13.8) with ESMTP id m8NEiWoP018025 for ; Tue, 23 Sep 2008 10:44:32 -0400 Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id m8NEfg4J234140 for ; Tue, 23 Sep 2008 10:41:42 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m8NEfgxv019375 for ; Tue, 23 Sep 2008 10:41:42 -0400 Message-ID: <48D8FFED.6070702@us.ibm.com> Date: Tue, 23 Sep 2008 09:40:45 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver References: <1222125454-21744-1-git-send-email-ryanh@us.ibm.com> <1222125454-21744-3-git-send-email-ryanh@us.ibm.com> <48D85849.2080302@us.ibm.com> <20080923143909.GK31395@us.ibm.com> In-Reply-To: <20080923143909.GK31395@us.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ryan Harper Cc: Marcelo Tosatti , qemu-devel@nongnu.org, kvm@vger.kernel.org Ryan Harper wrote: > * Anthony Liguori [2008-09-22 21:52]: > >> Ryan Harper wrote: >> > > > This was taken from block-raw-posix.c, DEBUG_BLOCK. I'll change up the > DEBUG_BLOCK_AIO, should I also submit a patch to rework DEBUG_BLOCK? > Yes, please. >>> +typedef struct AIODriver >>> +{ >>> + const char *name; >>> + RawAIOCB *(*submit)(BlockDriverState *bs, int fd, >>> + int64_t sector_num, uint8_t *buf, >>> + int sectors, int write, >>> + BlockDriverCompletionFunc *cb, >>> + void *opaque); >>> + void (*cancel)(BlockDriverAIOCB *aiocb); >>> + int (*flush)(void *opaque); >>> +} AIODriver; >>> >>> >> I think the AIODriver interface should just take an fd, a completion >> function, and an opaque pointer. It should have to have knowledge of >> BDRVRawState or BlockDriverState. >> > > I assume you mean shouldn't have to have. Looking at things like > pa_read() in aio-posix.c , I'm not sure I see how I avoid that > knowledge. > I think the key is to change the way AIOCBs are allocated. >>> *raw_aio_read(BlockDriverState *bs, >>> } >>> #endif >>> >>> - acb = raw_aio_setup(bs, sector_num, buf, nb_sectors, cb, opaque); >>> - if (!acb) >>> + if (fd_open(bs) < 0) >>> return NULL; >>> - if (aio_read(&acb->aiocb) < 0) { >>> - qemu_aio_release(acb); >>> + >>> + /* submit read */ >>> + acb = s->aio_dvr->submit(bs, s->fd, sector_num, buf, nb_sectors, 0, >>> cb, >>> + opaque); >>> + if (!acb) >>> return NULL; >>> - } >>> return &acb->common; >>> } >>> >>> >> So what happens if !defined(CONFIG_AIO)? By my reading of the code, >> aio_drv will be NULL and this will SEGV. >> > > raw_aio_read/write/cancel aren't included in the bdrv structure unless > CONFIG_AIO is defined. Rather in bdrv_register, the aio emulation > functions are used instead. > So these will give warnings then of unused statics? Because they are no longer conditional on CONFIG_AIO? Regards, Anthony Liguori >>> + /* init aio driver for this block device */ >>> + s->aio_dvr = posix_aio_init(); >>> >>> >> Doesn't this need to be conditional on CONFIG_AIO? >> > > Yep, in both raw_open and hdev_open(). > > > Thanks for the review. > >