[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw bloc
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 |
User-agent: |
Mutt/1.5.6+20040907i |
* Anthony Liguori <address@hidden> [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
address@hidden
[Qemu-devel] [PATCH 2/3] Move aio implementation out of raw block driver, Ryan Harper, 2008/09/22
- [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver, Ryan Harper, 2008/09/22
- [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver, Anthony Liguori, 2008/09/22
- Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver,
Ryan Harper <=
- Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver, Anthony Liguori, 2008/09/23
- Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver, Gerd Hoffmann, 2008/09/23
- Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver, Anthony Liguori, 2008/09/23
- Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver, Gerd Hoffmann, 2008/09/23
- Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver, Anthony Liguori, 2008/09/23
[Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver, Marcelo Tosatti, 2008/09/24
[Qemu-devel] [PATCH 2/3] Move aio implementation out of raw block driver, Ryan Harper, 2008/09/22
[Qemu-devel] Re: [PATCH 0/3] Refactor AIO to allow multiple AIO implementations, Anthony Liguori, 2008/09/22