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.

Paolo

v1->v2: fix file-win32

Paolo Bonzini (5):
  qcow2: make qcow2_do_open a coroutine_fn
  qed: make bdrv_qed_do_open a coroutine_fn
  block: convert bdrv_invalidate_cache callback to coroutine_fn
  qcow2: introduce qcow2_write_caches and qcow2_flush_caches
  block: convert bdrv_check callback to coroutine_fn

Stefan Hajnoczi (2):
  block: rename .bdrv_create() to .bdrv_co_create_opts()
  qcow2: make qcow2_co_create2() a coroutine_fn

 block.c                   |  88 ++++++++++++++++++++++++++++++----
 block/crypto.c            |   8 ++--
 block/file-posix.c        |  15 +++---
 block/file-win32.c        |   5 +-
 block/gluster.c           |  12 ++---
 block/iscsi.c             |  13 ++---
 block/nfs.c               |  11 +++--
 block/parallels.c         |  23 +++++----
 block/qcow.c              |   5 +-
 block/qcow2-refcount.c    |  28 +++++++++++
 block/qcow2.c             | 118 +++++++++++++++++++++++++++++++---------------
 block/qcow2.h             |   2 +
 block/qed-check.c         |   1 +
 block/qed-table.c         |  26 ++++------
 block/qed.c               |  72 +++++++++++++++++++++-------
 block/raw-format.c        |   5 +-
 block/rbd.c               |  12 +++--
 block/sheepdog.c          |  10 ++--
 block/ssh.c               |   5 +-
 block/vdi.c               |  11 +++--
 block/vhdx.c              |  12 +++--
 block/vmdk.c              |  12 +++--
 block/vpc.c               |   5 +-
 include/block/block_int.h |  11 +++--
 24 files changed, 355 insertions(+), 155 deletions(-)

-- 
1.8.3.1

On 01/18/2018 06:43 AM, Paolo Bonzini wrote:
> 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.
> 
> Paolo
> 

Modulo a commit message nit in 4/7, series

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

On 18/01/2018 07:43, Paolo Bonzini wrote:
> 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.
> 
> Paolo

Ping?

Thanks,

Paolo

> v1->v2: fix file-win32
> 
> Paolo Bonzini (5):
>   qcow2: make qcow2_do_open a coroutine_fn
>   qed: make bdrv_qed_do_open a coroutine_fn
>   block: convert bdrv_invalidate_cache callback to coroutine_fn
>   qcow2: introduce qcow2_write_caches and qcow2_flush_caches
>   block: convert bdrv_check callback to coroutine_fn
> 
> Stefan Hajnoczi (2):
>   block: rename .bdrv_create() to .bdrv_co_create_opts()
>   qcow2: make qcow2_co_create2() a coroutine_fn
> 
>  block.c                   |  88 ++++++++++++++++++++++++++++++----
>  block/crypto.c            |   8 ++--
>  block/file-posix.c        |  15 +++---
>  block/file-win32.c        |   5 +-
>  block/gluster.c           |  12 ++---
>  block/iscsi.c             |  13 ++---
>  block/nfs.c               |  11 +++--
>  block/parallels.c         |  23 +++++----
>  block/qcow.c              |   5 +-
>  block/qcow2-refcount.c    |  28 +++++++++++
>  block/qcow2.c             | 118 +++++++++++++++++++++++++++++++---------------
>  block/qcow2.h             |   2 +
>  block/qed-check.c         |   1 +
>  block/qed-table.c         |  26 ++++------
>  block/qed.c               |  72 +++++++++++++++++++++-------
>  block/raw-format.c        |   5 +-
>  block/rbd.c               |  12 +++--
>  block/sheepdog.c          |  10 ++--
>  block/ssh.c               |   5 +-
>  block/vdi.c               |  11 +++--
>  block/vhdx.c              |  12 +++--
>  block/vmdk.c              |  12 +++--
>  block/vpc.c               |   5 +-
>  include/block/block_int.h |  11 +++--
>  24 files changed, 355 insertions(+), 155 deletions(-)
>