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

[Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>]

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 <stefanha@gmail.com> wrote:
> > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> 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;

Now you can drop the malloc and simply qemu_aio_get() a new
BlockQueueAIOCB.

Stefan