On Fri, Aug 12, 2011 at 5:40 AM, Zhi Yong Wu <zwu.kernel@xxxxxxxxx> wrote:
> On Wed, Aug 10, 2011 at 5:37 PM, Stefan Hajnoczi
> <stefanha@xxxxxxxxxxxxxxxxxx> 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 <stefanha@xxxxxxxxx> wrote:
>>> > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx> 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.
> Yesterday, i tried this. To be honest, i do not like this solution.:).
> It results in the code logic to be a bit messy, not clear.
> A AIOCB struct is treated as a block request and enqueued into block
> queue. It is a bit weird. So i would like to retain BlockIORequest and
> define BlockQueueAIOCB pool. This struct is used to store some useful
> info:
> struct BlockQueueAIOCB {
> BlockDriverAIOCB common;
> bool dispatched;
> BlockDriverAIOCB *real_acb;
> BlockIORequest *request;
> };
>
> What do you think of this?
Try it out and let's see the patch :).
Stefan
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
