[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-1.4 stable] block: handle spurious coroutine
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH for-1.4 stable] block: handle spurious coroutine entries |
Date: |
Mon, 11 Feb 2013 14:54:56 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 |
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