From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ryan Harper Subject: Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver Date: Tue, 23 Sep 2008 09:39:09 -0500 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 Cc: Ryan Harper , Marcelo Tosatti , qemu-devel@nongnu.org, kvm@vger.kernel.org To: Anthony Liguori Return-path: Received: from e36.co.us.ibm.com ([32.97.110.154]:34562 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751524AbYIWOjb (ORCPT ); Tue, 23 Sep 2008 10:39:31 -0400 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by e36.co.us.ibm.com (8.13.8/8.13.8) with ESMTP id m8NEdKQh013097 for ; Tue, 23 Sep 2008 10:39:20 -0400 Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id m8NEdAVb043408 for ; Tue, 23 Sep 2008 08:39:11 -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 m8NEd9jH029556 for ; Tue, 23 Sep 2008 08:39:10 -0600 Content-Disposition: inline In-Reply-To: <48D85849.2080302@us.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: * 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