Re: [Qemu-devel] [PATCH v8 1/4] block: add the block queue support

[Kevin Wolf <kwolf@redhat.com>]

Am 18.10.2011 10:07, schrieb Zhi Yong Wu:
> On Mon, Oct 17, 2011 at 6:17 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>>>> +
>>>>> +typedef struct BlockQueueAIOCB BlockQueueAIOCB;
>>>>> +
>>>>> +struct BlockQueue {
>>>>> +    QTAILQ_HEAD(requests, BlockQueueAIOCB) requests;
>>>>> +    bool req_failed;
>>>>> +    bool flushing;
>>>>> +};
>>>>
>>>> I find req_failed pretty confusing. Needs documentation at least, but
>>>> most probably also a better name.
>>> OK. request_has_failed?
>>
>> No, that doesn't describe what it's really doing.
>>
>> You set req_failed = true by default and then on some obscure condition
>> clear it or not. It's tracking something, but I'm not sure what meaning
>> it has during the whole process.
> In qemu_block_queue_flush,
> When bdrv_aio_readv/writev return NULL, if req_failed is changed to
> false, it indicates that the request exceeds the limits again; if
> req_failed is still true, it indicates that one I/O error takes place,
> at the monent, qemu_block_queue_callback(request, -EIO) need to be
> called to report this to upper layer.

Okay, this makes some more sense now.

How about reversing the logic and maintaining a limit_exceeded flag
instead of a req_failed? I think this would be clearer.

>>>>> +void qemu_del_block_queue(BlockQueue *queue)
>>>>> +{
>>>>> +    BlockQueueAIOCB *request, *next;
>>>>> +
>>>>> +    QTAILQ_FOREACH_SAFE(request, &queue->requests, entry, next) {
>>>>> +        QTAILQ_REMOVE(&queue->requests, request, entry);
>>>>> +        qemu_aio_release(request);
>>>>> +    }
>>>>> +
>>>>> +    g_free(queue);
>>>>> +}
>>>>
>>>> Can we be sure that no AIO requests are in flight that still use the now
>>>> released AIOCB?
>>> Yeah, since qemu core code is serially performed, i think that when
>>> qemu_del_block_queue is performed, no requests are in flight. Right?
>>
>> Patch 2 has this code:
>>
>> +void bdrv_io_limits_disable(BlockDriverState *bs)
>> +{
>> +    bs->io_limits_enabled = false;
>> +
>> +    if (bs->block_queue) {
>> +        qemu_block_queue_flush(bs->block_queue);
>> +        qemu_del_block_queue(bs->block_queue);
>> +        bs->block_queue = NULL;
>> +    }
>>
>> Does this mean that you can't disable I/O limits while the VM is running?
> NO, you can even though VM is running.

Okay, in general qemu_block_queue_flush() empties the queue so that
there are no requests left that qemu_del_block_queue() could drop from
the queue. So in the common case it doesn't even enter the FOREACH loop.

I think problems start when requests have failed or exceeded the limit
again, then you have requests queued even after
qemu_block_queue_flush(). You must be aware of this, otherwise the code
in qemu_del_block_queue() wouldn't exist.

But you can't release the ACBs without having called their callback,
otherwise the caller would still assume that its ACB pointer is valid.
Maybe calling the callback before releasing the ACB would be enough.

However, for failed requests see below.

>>
>>>>> +
>>>>> +BlockDriverAIOCB *qemu_block_queue_enqueue(BlockQueue *queue,
>>>>> +                        BlockDriverState *bs,
>>>>> +                        BlockRequestHandler *handler,
>>>>> +                        int64_t sector_num,
>>>>> +                        QEMUIOVector *qiov,
>>>>> +                        int nb_sectors,
>>>>> +                        BlockDriverCompletionFunc *cb,
>>>>> +                        void *opaque)
>>>>> +{
>>>>> +    BlockDriverAIOCB *acb;
>>>>> +    BlockQueueAIOCB *request;
>>>>> +
>>>>> +    if (queue->flushing) {
>>>>> +        queue->req_failed = false;
>>>>> +        return NULL;
>>>>> +    } else {
>>>>> +        acb = qemu_aio_get(&block_queue_pool, bs,
>>>>> +                           cb, opaque);
>>>>> +        request = container_of(acb, BlockQueueAIOCB, common);
>>>>> +        request->handler       = handler;
>>>>> +        request->sector_num    = sector_num;
>>>>> +        request->qiov          = qiov;
>>>>> +        request->nb_sectors    = nb_sectors;
>>>>> +        request->real_acb      = NULL;
>>>>> +        QTAILQ_INSERT_TAIL(&queue->requests, request, entry);
>>>>> +    }
>>>>> +
>>>>> +    return acb;
>>>>> +}
>>>>> +
>>>>> +static int qemu_block_queue_handler(BlockQueueAIOCB *request)
>>>>> +{
>>>>> +    int ret;
>>>>> +    BlockDriverAIOCB *res;
>>>>> +
>>>>> +    res = request->handler(request->common.bs, request->sector_num,
>>>>> +                           request->qiov, request->nb_sectors,
>>>>> +                           qemu_block_queue_callback, request);
>>>>> +    if (res) {
>>>>> +        request->real_acb = res;
>>>>> +    }
>>>>> +
>>>>> +    ret = (res == NULL) ? 0 : 1;
>>>>> +
>>>>> +    return ret;
>>>>
>>>> You mean return (res != NULL); and want to have bool as the return value
>>>> of this function.
>>> Yeah, thanks. i will modify as below:
>>> ret = (res == NULL) ? false : true;
>>
>> ret = (res != NULL) is really more readable.
> I have adopted Paolo's suggestion.
> 
>>
>>> and
>>> static bool qemu_block_queue_handler()
>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +void qemu_block_queue_flush(BlockQueue *queue)
>>>>> +{
>>>>> +    queue->flushing = true;
>>>>> +    while (!QTAILQ_EMPTY(&queue->requests)) {
>>>>> +        BlockQueueAIOCB *request = NULL;
>>>>> +        int ret = 0;
>>>>> +
>>>>> +        request = QTAILQ_FIRST(&queue->requests);
>>>>> +        QTAILQ_REMOVE(&queue->requests, request, entry);
>>>>> +
>>>>> +        queue->req_failed = true;
>>>>> +        ret = qemu_block_queue_handler(request);
>>>>> +        if (ret == 0) {
>>>>> +            QTAILQ_INSERT_HEAD(&queue->requests, request, entry);
>>>>> +            if (queue->req_failed) {
>>>>> +                qemu_block_queue_callback(request, -EIO);
>>>>> +                break;

When a request has failed, you call its callback, but still leave it
queued. I think this is wrong.

>>>>> +            }
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    queue->req_failed = true;
>>>>> +    queue->flushing   = false;
>>>>> +}
>>>>> +
>>>>> +bool qemu_block_queue_has_pending(BlockQueue *queue)
>>>>> +{
>>>>> +    return !queue->flushing && !QTAILQ_EMPTY(&queue->requests);
>>>>> +}
>>>>
>>>> Why doesn't the queue have pending requests in the middle of a flush
>>>> operation? (That is, the flush hasn't completed yet)
>>> It is possible for the queue to have pending requests. if yes, how about?
>>
>> Sorry, can't parse this.
>>
>> I don't understand why the !queue->flushing part is correct.

What about this?

Kevin