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

[Stefan Hajnoczi]

On Mon, Sep 5, 2011 at 9:34 AM, Zhi Yong Wu <address@hidden> wrote:
> On Thu, Sep 01, 2011 at 02:02:41PM +0100, Stefan Hajnoczi wrote:
>> 01 Sep 2011 06:02:41 -0700 (PDT)
>>On Thu, Sep 1, 2011 at 12:44 PM, Zhi Yong Wu <address@hidden> wrote:
>>> +static void qemu_block_queue_dequeue(BlockQueue *queue, BlockIORequest 
>>> *request)
>>> +{
>>> +    BlockIORequest *req;
>>> +
>>> +    while (!QTAILQ_EMPTY(&queue->requests)) {
>>> +        req = QTAILQ_FIRST(&queue->requests);
>>> +        if (req == request) {
>>> +            QTAILQ_REMOVE(&queue->requests, req, entry);
>>> +            break;
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static void qemu_block_queue_cancel(BlockDriverAIOCB *acb)
>>> +{
>>> +    BlockQueueAIOCB *blkacb = container_of(acb, BlockQueueAIOCB, common);
>>> +    if (blkacb->real_acb) {
>>> +        bdrv_aio_cancel(blkacb->real_acb);
>>> +    } else {
>>> +        assert(blkacb->common.bs->block_queue);
>>> +        qemu_block_queue_dequeue(blkacb->common.bs->block_queue,
>>> +                                 blkacb->request);
>>
>>Can we use QTAILQ_REMOVE() and eliminate qemu_block_queue_dequeue()?
> why need to replace dequeue function with QTAILQ_REMOVE()?

Because the existing QTAILQ_REMOVE() macro already does what is needed.

>>> +void qemu_del_block_queue(BlockQueue *queue)
>>> +{
>>> +    BlockIORequest *request, *next;
>>> +
>>> +    QTAILQ_FOREACH_SAFE(request, &queue->requests, entry, next) {
>>> +        QTAILQ_REMOVE(&queue->requests, request, entry);
>>> +        g_free(request);
>>
>>What about the acbs?  There needs to be a strategy for cleanly
> what strategy?
>>shutting down completely.  In fact we should probably use cancellation
>>here or assert that the queue is already empty.
> You mean if the queue has been empty, we need to assert(queue)?

This patch silently deletes queued requests, which leaks the
BlockQueueAIOCBs.  The queued requests will never be issued or
completed.

qemu_del_block_queue() must cleanly destroy the queue.  This function
could require the queue to be empty, then we do not need to worry
about queued requests.  The caller can do this by flushing it before
deleting the queue.

>>> +static int qemu_block_queue_handler(BlockIORequest *request)
>>> +{
>>> +    int ret;
>>> +    BlockDriverAIOCB *res;
>>> +
>>> +    res = request->handler(request->bs, request->sector_num,
>>> +                           request->qiov, request->nb_sectors,
>>> +                           request->cb, request->opaque);
>>
>>Please pass qemu_block_queue_callback and request->acb directly here
>>instead of using request->cb and request->opaque.  Those fields aren't
>>needed and just add indirection.
> If later the other guy want to reuse qemu_block_queue_handler, and use 
> different callback function, then this function can not be used. Your way 
> lower this function's reusability.

Code can be changed so it's best to do things the simple way first and
extend the code if necessary later.  Trying to think ahead results in
half-finished code where the reader has to guess the author's
intention.

Stefan