Re: [Qemu-devel] [RFC] block: Removed coroutine ownership assumption

[Peter Crosthwaite]

On Fri, Jun 22, 2012 at 5:50 PM, Jan Kiszka <address@hidden> wrote:
> On 2012-06-22 08:44, Peter A. G. Crosthwaite wrote:
>> The block layer assumes that it is the only user of coroutines -
>> The qemu_in_coroutine() is used to determine if a function is in one of the
>> block layers coroutines, which is flawed. I.E. If a client (e.g. a device or
>> a machine model) of the block layer uses couroutine itself, the block layer
>> will identify the callers coroutines as its own, and may falsely yield the
>> calling coroutine (instead of creating its own to yield).
>>
>> AFAICT, there are no conflicts in the QEMU master here yet, but its kind of 
>> an
>> issue, as anyone who comes along and used coroutines and the block layer
>> together is going to run into some very obscure and hard to debug race
>> conditions.
>
> Not sure if I understood the intention yet: Is this supposed to fix an
> issue with the current usage of coroutines or to extend their usage
> beyond that? In the latter case, please don't do this. We'd rather like
> to get rid of them long term.
>

My extended usage, which is under development and not ready for the
list. But are you saying qemu-coroutine is deprecated? If so Ill just
re-impelement my work with threads, mutexes and condition vars, but
coroutines are the most natural way of doing it.

> Jan
>
>>
>> Signed-off-by: Peter A. G. Crosthwaite <address@hidden>
>> ---
>>  block.c |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 0acdcac..b50af15 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -380,7 +380,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>>          return -ENOTSUP;
>>      }
>>
>> -    if (qemu_in_coroutine()) {
>> +    if (0) {
>>          /* Fast-path if already in coroutine context */
>>          bdrv_create_co_entry(&cco);
>>      } else {
>> @@ -1590,7 +1590,7 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t 
>> sector_num, uint8_t *buf,
>>          bdrv_io_limits_disable(bs);
>>      }
>>
>> -    if (qemu_in_coroutine()) {
>> +    if (0) {
>>          /* Fast-path if already in coroutine context */
>>          bdrv_rw_co_entry(&rwco);
>>      } else {
>> @@ -3813,7 +3813,7 @@ int bdrv_flush(BlockDriverState *bs)
>>          .ret = NOT_DONE,
>>      };
>>
>> -    if (qemu_in_coroutine()) {
>> +    if (0) {
>>          /* Fast-path if already in coroutine context */
>>          bdrv_flush_co_entry(&rwco);
>>      } else {
>> @@ -3874,7 +3874,7 @@ int bdrv_discard(BlockDriverState *bs, int64_t 
>> sector_num, int nb_sectors)
>>          .ret = NOT_DONE,
>>      };
>>
>> -    if (qemu_in_coroutine()) {
>> +    if (0) {
>>          /* Fast-path if already in coroutine context */
>>          bdrv_discard_co_entry(&rwco);
>>      } else {
>>
>
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
>