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

[Anthony Liguori]

Ryan Harper wrote:
> * Anthony Liguori <address@hidden> [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.
>
>