Re: [Qemu-devel] [PATCH v5 2/4] block: add the block queue support

[Zhi Yong Wu]

On Wed, Aug 10, 2011 at 5:37 PM, Stefan Hajnoczi
<address@hidden> wrote:
> On Wed, Aug 10, 2011 at 01:54:33PM +0800, Zhi Yong Wu wrote:
>> On Tue, Aug 9, 2011 at 8:49 PM, Stefan Hajnoczi <address@hidden> wrote:
>> > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <address@hidden> wrote:
>> >> +BlockDriverAIOCB *qemu_block_queue_enqueue(BlockQueue *queue,
>> >> +                        BlockDriverState *bs,
>> >> +                        BlockRequestHandler *handler,
>> >> +                        int64_t sector_num,
>> >> +                        QEMUIOVector *qiov,
>> >> +                        int nb_sectors,
>> >> +                        BlockDriverCompletionFunc *cb,
>> >> +                        void *opaque)
>> >> +{
>> >> +    BlockIORequest *request;
>> >> +    BlockDriverAIOCB *acb;
>> >> +
>> >> +    request = qemu_malloc(sizeof(BlockIORequest));
>> >> +    request->bs = bs;
>> >> +    request->handler = handler;
>> >> +    request->sector_num = sector_num;
>> >> +    request->qiov = qiov;
>> >> +    request->nb_sectors = nb_sectors;
>> >> +    request->cb = cb;
>> >> +    request->opaque = opaque;
>> >> +
>> >> +    QTAILQ_INSERT_TAIL(&queue->requests, request, entry);
>> >
>> > It would be simpler to define BlockQueueAIOCB and using it as our acb
>> > instead of managing an extra BlockIORequest structure.  That way you
>> > don't need to worry about extra mallocs and frees.
>> Sorry, i don't get what you mean. how to define it? Can you elaborate?
>
> BlockDriverAIOCB is designed to be embedded inside a bigger struct.  For
> example, QEDAIOCB is a larger struct that contains BlockDriverAIOCB as
> its first field:
>
> typedef struct QEDAIOCB {
>    BlockDriverAIOCB common;
>    ...
> } QEDAIOCB;
>
> And the QED AIOPool contains the size of QEDAIOCB so that qemu_aio_get() can
> allocate the full QEDAIOCB struct:
>
> static AIOPool qed_aio_pool = {
>    .aiocb_size         = sizeof(QEDAIOCB),
>    .cancel             = qed_aio_cancel,
> };
>
> This allows QED to store per-request state in QEDAIOCB for the lifetime of a
> request:
>
> QEDAIOCB *acb = qemu_aio_get(&qed_aio_pool, bs, cb, opaque);
>
> acb->is_write = is_write;
> acb->finished = NULL;
> acb->qiov = qiov;
> ...
>
> I suggest creating a BlockQueueAIOCB that contains the fields from
> BlockIORequest (which is no longer needed as a separate struct):
>
> typedef struct BlockQueueAIOCB {
>    BlockDriverAIOCB common;
>    BlockRequestHandler *handler;
>    int64_t sector_num;
>    QEMUIOVector *qiov;
>    int nb_sectors;
>    QTAILQ_ENTRY(BlockQueueAIOCB) entry; /* pending request queue */
> } BlockQueueAIOCB;
Its name is a bit confusing. One ACB is inserted into block queue.
>
> Now you can drop the malloc and simply qemu_aio_get() a new
> BlockQueueAIOCB.
>
> Stefan
>



-- 
Regards,

Zhi Yong Wu