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 <[email protected]> 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
