Re: [Qemu-devel] [RFC PATCH v2 2/4] Add block-queue

[Kevin Wolf <kwolf@redhat.com>]

Am 17.11.2010 17:23, schrieb Stefan Hajnoczi:
> On Wed, Nov 17, 2010 at 1:41 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 17.11.2010 13:43, schrieb Stefan Hajnoczi:
>>>> A typical sequence in qcow2 (simple cluster allocation) looks like this:
>>>>
>>>> 1. Update refcount table
>>>> 2. bdrv_flush
>>>> 3. Update L2 entry
>>>>
>>>> If we delay the operation and get three of these sequences queued before
>>>> actually executing, we end up with the following result, saving two syncs:
>>>>
>>>> 1. Update refcount table (req 1)
>>>> 2. Update refcount table (req 2)
>>>> 3. Update refcount table (req 3)
>>>> 4. bdrv_flush
>>>> 5. Update L2 entry (req 1)
>>>> 6. Update L2 entry (req 2)
>>>> 7. Update L2 entry (req 3)
>>>
>>> How does block-queue group writes 1-3 and 5-7 together?  I thought
>>> reqs 1-3 will each have their own context but from a quick look at the
>>> code I don't think that is the case for qcow2 in patch 4.
>>>
>>> Another way of asking is, how does block-queue know that it is safe to
>>> put writes 1-3 together before a bdrv_flush?  Why doesn't it also put
>>> writes 5-7 before the flush?
>>>
>>> Perhaps your current qcow2 implementation isn't taking advantage of
>>> block-queue bdrv_flush() batching for concurrent write requests?
>>>
>>> I'm missing something ;).
>>
>> Yes, you are. ;-)
>>
>> The contexts are indeed the mechanism that it uses to achieve this. Have
>> a look at qcow_aio_setup, patch 4/4 adds a blkqueue_init_context() call
>> there.
> 
> But there is a single context per qcow2 state.  So this works for
> sequential requests but I guess the concurrent aio requests case
> doesn't work?

Yes, it needs to become a per-acb field. I think there's a TOOD comment.
Probably the qcow2 conversion is going to be a patch series of its own
because the ACB needs to be passed down everywhere for this to work. I
mean I could come up with another hack like restoring s->bq_context from
the ACB at the start of the callbacks, but that's certainly not the way
to go for a real implementation.

Kevin