Re: [Qemu-devel] [PATCH for-1.4 stable] block: handle spurious coroutine entries

[Paolo Bonzini]

Il 11/02/2013 14:29, Kevin Wolf ha scritto:
> Am 11.02.2013 14:17, schrieb Stefan Hajnoczi:
>> On Mon, Feb 11, 2013 at 1:42 PM, Paolo Bonzini <address@hidden> wrote:
>>> Il 11/02/2013 13:29, Kevin Wolf ha scritto:
>>>> The bug is not in this function but in process_incoming_migration(). It
>>>> should never blindly enter a coroutine which is in an unknown state.
>>>
>>> process_incoming_migration() is the function that _creates_ the
>>> coroutine and enters it for the first time, so there shouldn't be any
>>> problem there.
>>>
>>> The function we're looking at should be, as you write correctly,
>>> enter_migration_coroutine().
>>>
>>>> If it can reenter here, it can reenter anywhere in block drivers, and
>>>> adding a workaround to one place that just yields again if it got an
>>>> early reentrance doesn't fix the real bug.
>>>>
>>>> Which is the yield that corresponds to the enter in
>>>> enter_migration_coroutine()? We need to add some state that can be used
>>>> to make sure the enter happens only for this specific yield.
>>>
>>> In this case the corresponding yield is in qemu-coroutine-io.c, and it
>>> is driven by read-availability of the migration file descriptor.  So it
>>> is possible to test before entering the coroutine, but really ugly (not
>>> just because it would cost one extra system call).
>>>
>>> But why is enter_migration_coroutine() being called at all?  bdrv_write
>>> should be a synchronous call, it should run its own qemu_aio_wait()
>>> event loop and that loop doesn't include the migration file descriptor.
>>
>> qemu_aio_wait() is not called because we are already in a coroutine -
>> the migration coroutine.  bdrv_write() yields instead of looping on
>> qemu_aio_wait().

Uh-uh, that's it.

> Right. But that's not really a problem, then the main loop will call the
> necessary AIO callbacks that reenter the coroutines - it's a superset of
> qemu_aio_wait().

But it will also call other unnecessary AIO callbacks, and that's a problem.

>> If we want coroutine_fn to be composable, they must handle spurious
>> entries.  We could add code to clear the socket fd handler while we're
>> not waiting for it but I think handling spurious entries is the most
>> robust solution.
> 
> Either clear the fd handler or modify the handler function to check the
> state of the coroutine before it reenters (like block_job_resume() does,
> for example)

That doesn't work because you'd get a busy wait while the socket remans
readable.  Clearing the FD handler during bdrv_write is also ugly.

Perhaps we need a bdrv_write_sync that forces the qemu_aio_wait() path.

> So far the rule has been that each enter corresponds to a well-known
> yield. If you really want to change this rule, you'd have to fix things
> all over the place because this is an assumption that most if not all
> coroutine based code in qemu relies on. I don't want to be the one to
> review this change, and I don't think it's a good idea either...

We don't have much coroutine code:

block.c:    qemu_coroutine_yield();
block.c:            qemu_coroutine_yield();
block.c:            qemu_coroutine_yield();

These are fixed by Stefan's patch.

block/blkdebug.c:    qemu_coroutine_yield();

Not ok, should be easily fixed.

block/mirror.c:        qemu_coroutine_yield();
block/mirror.c:            qemu_coroutine_yield();
block/mirror.c:        qemu_coroutine_yield();
block/mirror.c:                qemu_coroutine_yield();

All wrapped with while.

block/nbd.c:    qemu_coroutine_yield();

Not ok, easily fixed.

block/qed.c:        qemu_coroutine_yield();
block/qed.c:        qemu_coroutine_yield();

First fine, second not but easily fixed.

block/sheepdog.c:    qemu_coroutine_yield();
block/sheepdog.c:    qemu_coroutine_yield();
block/sheepdog.c:    qemu_coroutine_yield();

Not ok, easily fixed.

blockjob.c:        qemu_coroutine_yield();

Doesn't really matter, but could change surrounding if to while.

hw/9pfs/virtio-9p-coth.h:        qemu_coroutine_yield();
hw/9pfs/virtio-9p-coth.h:        qemu_coroutine_yield();

No idea. :)

qemu-coroutine-io.c:                qemu_coroutine_yield();

Ok.

qemu-coroutine-lock.c:    qemu_coroutine_yield();
qemu-coroutine-lock.c:    qemu_coroutine_yield();

These are the problematic ones.

qemu-coroutine-sleep.c:    qemu_coroutine_yield();

Should not be a problem if it enters early.

savevm.c:            qemu_coroutine_yield();
savevm.c:            qemu_coroutine_yield();

Ok.

thread-pool.c:    qemu_coroutine_yield();

Not ok, easily fixed.

Paolo