Re: [Qemu-devel] [PATCH v3 0/7] Call check and invalidate_cache from coroutine context

[Kevin Wolf <kwolf@redhat.com>]

Am 18.01.2018 um 13:43 hat Paolo Bonzini geschrieben:
> Check and invalidate_cache share some parts of the implementation
> with the regular I/O path.  This is sometimes complicated because the
> I/O path wants to use a CoMutex but that is not possible outside coroutine
> context.  By moving things to coroutine context, we can remove special
> cases.  In fact, invalidate_cache is already called from coroutine context
> because incoming migration is placed in a coroutine.
> 
> While at it, I'm including two patches from Stefan to rename the
> bdrv_create callback to bdrv_co_create, because it is already called
> from coroutine context.  The name is now bdrv_co_create_opts, with
> bdrv_co_create reserved for the QAPI-based version that Kevin is
> working on.
> 
> qcow2 still has cache flushing in non-coroutine context, coming from
> qcow2_reopen_prepare->qcow2_update_options_prepare and
> qcow2_close->qcow2_inactivate.

The patches looked good, but this deadlocks qemu-iotests 165 for me:

#0  0x0000562392ae1cb0 in qemu_coroutine_switch (from_=from_@entry=0x562393de2410, to_=to_@entry=0x7f81bf44ee48, action=action@entry=COROUTINE_YIELD) at util/coroutine-ucontext.c:219
#1  0x0000562392ae0ad1 in qemu_coroutine_yield () at util/qemu-coroutine.c:186
#2  0x0000562392ae0cb4 in qemu_co_mutex_lock_slowpath (mutex=0x562393de1870, ctx=0x562393dc1ad0) at util/qemu-coroutine-lock.c:269
#3  0x0000562392ae0cb4 in qemu_co_mutex_lock (mutex=mutex@entry=0x562393de1870) at util/qemu-coroutine-lock.c:307
#4  0x0000562392a149fc in qcow2_co_flush_to_os (bs=0x562393dd6750) at block/qcow2.c:3705
#5  0x0000562392a461c9 in bdrv_co_flush (bs=0x562393dd6750) at block/io.c:2439
#6  0x0000562392a46639 in bdrv_flush_co_entry (opaque=0x7f8195a7cd50) at block/io.c:2403
#7  0x0000562392a46639 in bdrv_flush (bs=bs@entry=0x562393dd6750) at block/io.c:2528
#8  0x0000562392a26de6 in update_header_sync (bs=bs@entry=0x562393dd6750) at block/qcow2-bitmap.c:113
#9  0x0000562392a26e5a in update_ext_header_and_dir_in_place (bs=bs@entry=0x562393dd6750, bm_list=bm_list@entry=0x562393de14f0) at block/qcow2-bitmap.c:826
#10 0x0000562392a27fa6 in qcow2_load_dirty_bitmaps (bs=bs@entry=0x562393dd6750, errp=errp@entry=0x7f8195a7ce68) at block/qcow2-bitmap.c:982
#11 0x0000562392a1947c in qcow2_do_open (bs=0x562393dd6750, options=<optimized out>, flags=8194, errp=0x7ffc3289a110) at block/qcow2.c:1501
#12 0x0000562392a198e2 in qcow2_open_entry (opaque=0x7ffc3289a0b0) at block/qcow2.c:1578
#13 0x0000562392ae1d1c in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at util/coroutine-ucontext.c:116
#14 0x00007f81a338d950 in __start_context () at /lib64/libc.so.6
#15 0x00007ffc32899920 in  ()
#16 0x0000000000000000 in  ()

Not saving the coroutine pointer anywhere was a bit nasty, too. gdb
only gave the coroutine pointer away with something as indirect as
'p ((BDRVQcow2State*) bs.opaque).lock.holder'.

Kevin