Re: [Qemu-devel] Re: [PATCH 2/3] Move aio implementation out of raw block driver

[Ryan Harper]

* 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