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

[Zhi Yong Wu]

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:
>> +/* The APIs for block request queue on qemu block layer.
>> + */
>> +
>> +static void qemu_block_queue_cancel(BlockDriverAIOCB *acb)
>> +{
>> +    qemu_aio_release(acb);
>> +}
>> +
>> +static AIOPool block_queue_pool = {
>> +    .aiocb_size         = sizeof(struct BlockDriverAIOCB),
>> +    .cancel             = qemu_block_queue_cancel,
>> +};
>
> The lifecycle of block_queue_pool acbs should be like this:
>
> When a request is queued we need a BlockQueue acb because we have no
> real acb yet.  So we get an acb from block_queue_pool.
>
> If the acb is cancelled before being dispatched we need to release the
> acb and remove the request from the queue.  (This patch does not
> remove the request from the queue on cancel.)
>
> When the acb is dispatched we need to keep track of the real acb (e.g.
> from qcow2).  The caller will only ever see our acb because there is
> no way to give them the pointer to the new acb after dispatch.  That
> means we need to keep the our acb alive for the entire lifetime of the
> request.  (This patch currently releases our acb when the request is
> dispatched.)
>
> If the acb is cancelled after being dispatched we need to first cancel
> the real acb and then release our acb.
>
> When the acb is dispatched we need to pass qemu_block_queue_callback
> as the cb function to handler.  Inside qemu_block_queue_callback we
When one block request have been reenqueued multiple times, it will be
difficult to store original cb function.

> invoke the request cb and then release our acb.  This is how our acb
> should be freed.
>
>> +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.
>
>> +
>> +    acb = qemu_aio_get(&block_queue_pool, bs,
>> +                       qemu_block_queue_callback, opaque);
>> +
>> +    request->acb = acb;
>> +
>> +    return acb;
>> +}
>> +
>> +int qemu_block_queue_handler(BlockIORequest *request)
>
> This function can be static.
>
> Stefan
>



-- 
Regards,

Zhi Yong Wu