Re: [Qemu-devel] [RFC][PATCH 11/12] qcow2: Convert qcow2 to use coroutines for async I/O

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/26/2011 09:50 AM, Kevin Wolf wrote:
> Am 26.01.2011 16:40, schrieb Avi Kivity:
>    
>> On 01/22/2011 11:29 AM, Stefan Hajnoczi wrote:
>>      
>>> Converting qcow2 to use coroutines is fairly simple since most of qcow2
>>> is synchronous.  The synchronous I/O functions likes bdrv_pread() now
>>> transparently work when called from a coroutine, so all the synchronous
>>> code just works.
>>>
>>> The explicitly asynchronous code is adjusted to repeatedly call
>>> qcow2_aio_read_cb() or qcow2_aio_write_cb() until the request completes.
>>> At that point the coroutine will return from its entry function and its
>>> resources are freed.
>>>
>>> The bdrv_aio_readv() and bdrv_aio_writev() user callback is now invoked
>>> from a BH.  This is necessary since the user callback code does not
>>> expect to be executed from a coroutine.
>>>
>>> This conversion is not completely correct because the safety the
>>> synchronous code does not carry over to the coroutine version.
>>> Previously, a synchronous code path could assume that it will never be
>>> interleaved with another request executing.  This is no longer true
>>> because bdrv_pread() and bdrv_pwrite() cause the coroutine to yield and
>>> other requests can be processed during that time.
>>>
>>> The solution is to carefully introduce checks so that pending requests
>>> do not step on each other's toes.  That is left for a future patch...
>>>        
>> The way I thought of doing this is:
>>
>> qcow_aio_write(...)
>> {
>>       execute_in_coroutine {
>>           co_mutex_lock(&bs->mutex);
>>           do_qcow_aio_write(...); // original qcow code
>>           co_mutex_release(&bs->mutex);
>>      

The release has to be executed in the call back.

I think it's a bit nicer to not do a mutex, but rather to have a notion 
of freezing/unfreezing the block queue and instead do:

completion() {
    bdrv_unfreeze(bs);
}

coroutine {
    bdrv_freeze(bs);
    do_qcow_aio_write(completion);
}

Freeze/unfreeze is useful in a number of other places too (like 
snapshotting).

Regards,

Anthony Liguori

>>       }
>> }
>>
>> (similar changes for the the other callbacks)
>>
>> if the code happens to be asynchronous (no metadata changes), we'll take
>> the mutex and release it immediately after submitting the I/O, so no
>> extra serialization happens.  If the code does issue a synchronous
>> metadata write, we'll lock out all other operations on the same block
>> device, but still allow the vcpu to execute, since all the locking
>> happens in a coroutine.
>>
>> Essentially, a mutex becomes the dependency tracking mechnism.  A global
>> mutex means all synchronous operations are dependent.  Later, we can
>> convert the metadata cache entry dependency lists to local mutexes
>> inside the cache entry structures.
>>      
> I thought a bit about it since you mentioned it in the call yesterday
> and I think this approach makes sense. Even immediately after the
> conversion we should be in a better state than with Stefan's approach
> because I/O without metadata disk access won't be serialized.
>
> In the other thread you mentioned that you have written some code
> independently. Do you have it in some public git repository?
>
> Kevin
>
>