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

[Anthony Liguori <aliguori@us.ibm.com>]

Ryan Harper wrote:
> * Anthony Liguori <aliguori@us.ibm.com> [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.
>
>