From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ki92v-000669-MI for qemu-devel@nongnu.org; Tue, 23 Sep 2008 10:39:21 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Ki92t-00062g-68 for qemu-devel@nongnu.org; Tue, 23 Sep 2008 10:39:21 -0400 Received: from [199.232.76.173] (port=47653 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ki92s-00062M-JI for qemu-devel@nongnu.org; Tue, 23 Sep 2008 10:39:18 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:34157) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Ki92r-0008Pa-P9 for qemu-devel@nongnu.org; Tue, 23 Sep 2008 10:39:18 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e36.co.us.ibm.com (8.13.8/8.13.8) with ESMTP id m8NEdAQU012835 for ; Tue, 23 Sep 2008 10:39:10 -0400 Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id m8NEdAo5122960 for ; Tue, 23 Sep 2008 08:39:10 -0600 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m8NEd9jB029556 for ; Tue, 23 Sep 2008 08:39:09 -0600 Date: Tue, 23 Sep 2008 09:39:09 -0500 From: Ryan Harper Subject: Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver Message-ID: <20080923143909.GK31395@us.ibm.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <48D85849.2080302@us.ibm.com> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Ryan Harper , qemu-devel@nongnu.org, kvm@vger.kernel.org, Marcelo Tosatti * Anthony Liguori [2008-09-22 21:52]: > Ryan Harper wrote: > >+//#define DEBUG_BLOCK_AIO > >+#if defined(DEBUG_BLOCK_AIO) > >+#define BLPRINTF(formatCstr, args...) do { fprintf(stderr, formatCstr, > >##args); fflush(stderr); } while (0) > > > > This is GCC syntax. You should use C99 syntax. That would be: > > #define BLPRINTF(fmt, ...) do { fprintf(stderr, fmt, ## __VA_ARGS__); } > while (0) > > stderr is defined to not be buffered so an explicit fflush is > unnecessary. formatCstr is also a goofy name :-) > > >+#else > >+#define BLPRINTF(formatCstr, args...) > > > > should be defined to do { } while (0) to maintain statement semantics. 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? > >+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. > >*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. > >+ /* 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. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx (512) 838-9253 T/L: 678-9253 ryanh@us.ibm.com