Please read "Protect the block layer with a rwlock: part 1" and
"Protect the block layer with a rwlock: part 2" for an
additional introduction and aim of this series.

In this serie, we cover the remaining BlockDriver IO callbacks that were not
running in coroutine, therefore not using the graph rdlock.
Therefore convert them to coroutines, using either g_c_w or a new
variant introduced in this serie (see below).

We need to convert these callbacks into coroutine because non-coroutine code
is tied to the main thread, even though it will still delegate I/O accesses to
the iothread (via the bdrv_coroutine_enter call in generated_co_wrappers).
Making callbacks run in coroutines provides more flexibility, because they run
entirely in iothreads and can use CoMutexes for mutual exclusion.

Here we introduce generated_co_wrapper_simple, a simplification of g_c_w that
only considers the case where the caller is not in a coroutine.
This simplifies and clarifies a lot when the caller is a coroutine or not, and
in the future will hopefully replace g_c_w.

While we are at it, try to directly call the _co_ counterpart of a g_c_w when
we know already that the function always run in a coroutine.

Based-on: <20221116135331.3052923-1-eesposit@redhat.com>

Thank you,
Emanuele

Emanuele Giuseppe Esposito (15):
  block/qed: add missing graph rdlock in qed_need_check_timer_entry
  block: rename refresh_total_sectors in bdrv_refresh_total_sectors
  block-backend: use bdrv_getlength instead of blk_getlength
  block: convert bdrv_refresh_total_sectors in generated_co_wrapper
  block: use bdrv_co_refresh_total_sectors when possible
  block: convert bdrv_get_allocated_file_size in
    generated_co_wrapper_simple
  block: convert bdrv_get_info in generated_co_wrapper
  block: convert bdrv_is_inserted in generated_co_wrapper_simple
  block-coroutine-wrapper: support void functions
  block: convert bdrv_eject in generated_co_wrapper_simple
  block: convert bdrv_lock_medium in generated_co_wrapper_simple
  block: convert bdrv_debug_event in generated_co_wrapper
  block: convert bdrv_io_plug in generated_co_wrapper_simple
  block: convert bdrv_io_unplug in generated_co_wrapper_simple
  block: rename newly converted BlockDriver IO coroutine functions

 block.c                            | 93 +++++++++++++++++++-----------
 block/blkdebug.c                   |  4 +-
 block/blkio.c                      |  6 +-
 block/blklogwrites.c               |  2 +-
 block/blkreplay.c                  |  2 +-
 block/blkverify.c                  |  2 +-
 block/block-backend.c              | 43 ++++++++------
 block/commit.c                     |  4 +-
 block/copy-on-read.c               | 12 ++--
 block/crypto.c                     |  6 +-
 block/curl.c                       |  8 +--
 block/file-posix.c                 | 48 +++++++--------
 block/file-win32.c                 |  8 +--
 block/filter-compress.c            | 10 ++--
 block/gluster.c                    | 16 ++---
 block/io.c                         | 78 +++++++++++++------------
 block/iscsi.c                      |  8 +--
 block/meson.build                  |  1 +
 block/mirror.c                     | 17 ++++--
 block/nbd.c                        |  6 +-
 block/nfs.c                        |  2 +-
 block/null.c                       |  8 +--
 block/nvme.c                       |  6 +-
 block/preallocate.c                |  2 +-
 block/qcow.c                       |  2 +-
 block/qcow2-refcount.c             |  2 +-
 block/qcow2.c                      |  6 +-
 block/qed.c                        |  7 ++-
 block/quorum.c                     |  2 +-
 block/raw-format.c                 | 14 ++---
 block/rbd.c                        |  4 +-
 block/replication.c                |  2 +-
 block/ssh.c                        |  2 +-
 block/stream.c                     |  4 +-
 block/throttle.c                   |  2 +-
 block/vdi.c                        |  2 +-
 block/vhdx.c                       |  2 +-
 block/vmdk.c                       |  4 +-
 block/vpc.c                        |  2 +-
 blockdev.c                         |  8 ++-
 hw/scsi/scsi-disk.c                |  5 ++
 include/block/block-io.h           | 40 +++++++++----
 include/block/block_int-common.h   | 37 +++++++-----
 include/block/block_int-io.h       |  5 +-
 include/sysemu/block-backend-io.h  | 32 +++++++---
 scripts/block-coroutine-wrapper.py | 19 ++++--
 tests/unit/test-block-iothread.c   |  7 +++
 47 files changed, 364 insertions(+), 238 deletions(-)

-- 
2.31.1

Ok, as I expected simple changes in a previous based-on serie provoke a
cascade of changes that inevitably affect these patches too.

While I strongly suggest to have an initial look at these patches
because it gives an idea on what am I trying to accomplish, I would not
go looking at nitpicks and trivial errors that came up from the based-on
series (ie "just as in the previous serie, fix this").

The order of the series is:
1. Still more coroutine and various fixes in block layer
2. Protect the block layer with a rwlock: part 1
3. Protect the block layer with a rwlock: part 2
4. Protect the block layer with a rwlock: part 3

Thank you,
Emanuele

Am 16/11/2022 um 15:07 schrieb Emanuele Giuseppe Esposito:
> Please read "Protect the block layer with a rwlock: part 1" and
> "Protect the block layer with a rwlock: part 2" for an
> additional introduction and aim of this series.
> 
> In this serie, we cover the remaining BlockDriver IO callbacks that were not
> running in coroutine, therefore not using the graph rdlock.
> Therefore convert them to coroutines, using either g_c_w or a new
> variant introduced in this serie (see below).
> 
> We need to convert these callbacks into coroutine because non-coroutine code
> is tied to the main thread, even though it will still delegate I/O accesses to
> the iothread (via the bdrv_coroutine_enter call in generated_co_wrappers).
> Making callbacks run in coroutines provides more flexibility, because they run
> entirely in iothreads and can use CoMutexes for mutual exclusion.
> 
> Here we introduce generated_co_wrapper_simple, a simplification of g_c_w that
> only considers the case where the caller is not in a coroutine.
> This simplifies and clarifies a lot when the caller is a coroutine or not, and
> in the future will hopefully replace g_c_w.
> 
> While we are at it, try to directly call the _co_ counterpart of a g_c_w when
> we know already that the function always run in a coroutine.
> 
> Based-on: <20221116135331.3052923-1-eesposit@redhat.com>
> 
> Thank you,
> Emanuele
> 
> Emanuele Giuseppe Esposito (15):
>   block/qed: add missing graph rdlock in qed_need_check_timer_entry
>   block: rename refresh_total_sectors in bdrv_refresh_total_sectors
>   block-backend: use bdrv_getlength instead of blk_getlength
>   block: convert bdrv_refresh_total_sectors in generated_co_wrapper
>   block: use bdrv_co_refresh_total_sectors when possible
>   block: convert bdrv_get_allocated_file_size in
>     generated_co_wrapper_simple
>   block: convert bdrv_get_info in generated_co_wrapper
>   block: convert bdrv_is_inserted in generated_co_wrapper_simple
>   block-coroutine-wrapper: support void functions
>   block: convert bdrv_eject in generated_co_wrapper_simple
>   block: convert bdrv_lock_medium in generated_co_wrapper_simple
>   block: convert bdrv_debug_event in generated_co_wrapper
>   block: convert bdrv_io_plug in generated_co_wrapper_simple
>   block: convert bdrv_io_unplug in generated_co_wrapper_simple
>   block: rename newly converted BlockDriver IO coroutine functions
> 
>  block.c                            | 93 +++++++++++++++++++-----------
>  block/blkdebug.c                   |  4 +-
>  block/blkio.c                      |  6 +-
>  block/blklogwrites.c               |  2 +-
>  block/blkreplay.c                  |  2 +-
>  block/blkverify.c                  |  2 +-
>  block/block-backend.c              | 43 ++++++++------
>  block/commit.c                     |  4 +-
>  block/copy-on-read.c               | 12 ++--
>  block/crypto.c                     |  6 +-
>  block/curl.c                       |  8 +--
>  block/file-posix.c                 | 48 +++++++--------
>  block/file-win32.c                 |  8 +--
>  block/filter-compress.c            | 10 ++--
>  block/gluster.c                    | 16 ++---
>  block/io.c                         | 78 +++++++++++++------------
>  block/iscsi.c                      |  8 +--
>  block/meson.build                  |  1 +
>  block/mirror.c                     | 17 ++++--
>  block/nbd.c                        |  6 +-
>  block/nfs.c                        |  2 +-
>  block/null.c                       |  8 +--
>  block/nvme.c                       |  6 +-
>  block/preallocate.c                |  2 +-
>  block/qcow.c                       |  2 +-
>  block/qcow2-refcount.c             |  2 +-
>  block/qcow2.c                      |  6 +-
>  block/qed.c                        |  7 ++-
>  block/quorum.c                     |  2 +-
>  block/raw-format.c                 | 14 ++---
>  block/rbd.c                        |  4 +-
>  block/replication.c                |  2 +-
>  block/ssh.c                        |  2 +-
>  block/stream.c                     |  4 +-
>  block/throttle.c                   |  2 +-
>  block/vdi.c                        |  2 +-
>  block/vhdx.c                       |  2 +-
>  block/vmdk.c                       |  4 +-
>  block/vpc.c                        |  2 +-
>  blockdev.c                         |  8 ++-
>  hw/scsi/scsi-disk.c                |  5 ++
>  include/block/block-io.h           | 40 +++++++++----
>  include/block/block_int-common.h   | 37 +++++++-----
>  include/block/block_int-io.h       |  5 +-
>  include/sysemu/block-backend-io.h  | 32 +++++++---
>  scripts/block-coroutine-wrapper.py | 19 ++++--
>  tests/unit/test-block-iothread.c   |  7 +++
>  47 files changed, 364 insertions(+), 238 deletions(-)
>

On 11/16/22 15:07, Emanuele Giuseppe Esposito wrote:
> Here we introduce generated_co_wrapper_simple, a simplification of g_c_w that
> only considers the case where the caller is not in a coroutine.
> This simplifies and clarifies a lot when the caller is a coroutine or not, and
> in the future will hopefully replace g_c_w.

This is a good idea!

Just one thing, though it belongs more in the two series which 
introduced generated_co_wrapper_simple and generated_co_wrapper_blk - I 
would make this the "official" wrapper.  So perhaps:

- generated_co_wrapper_simple -> coroutine_wrapper
- generated_co_wrapper_blk -> coroutine_wrapper_mixed
- generated_co_wrapper -> coroutine_wrapper_mixed_bdrv

?  It is not clear to me yet if you will have bdrv_* functions that take 
the rdlock as well - in which case however coroutine_wrapper_bdrv would 
not be hard to add.

Even without looking at the lock, the three series are going in the 
right direction of ultimately having more "simple" coroutine wrappers at 
the blk_* level and more coroutine functions (ultimately less wrappers, 
too) at the bdrv_* level.

Paolo

Am 18/11/2022 um 11:57 schrieb Paolo Bonzini:
> On 11/16/22 15:07, Emanuele Giuseppe Esposito wrote:
>> Here we introduce generated_co_wrapper_simple, a simplification of
>> g_c_w that
>> only considers the case where the caller is not in a coroutine.
>> This simplifies and clarifies a lot when the caller is a coroutine or
>> not, and
>> in the future will hopefully replace g_c_w.
> 
> This is a good idea!
> 
> Just one thing, though it belongs more in the two series which
> introduced generated_co_wrapper_simple and generated_co_wrapper_blk - I
> would make this the "official" wrapper.  So perhaps:
> 
> - generated_co_wrapper_simple -> coroutine_wrapper
> - generated_co_wrapper_blk -> coroutine_wrapper_mixed
> - generated_co_wrapper -> coroutine_wrapper_mixed_bdrv

Ah damn I forgot about this, and of course I just sent v5 for "Still
more coroutine and various fixes in block layer".

To me it sounds good, but before I do a massive edit and then someone
asks to revert it, @Kevin and the others do you agree?

Thank you,
Emanuele

> 
> ?  It is not clear to me yet if you will have bdrv_* functions that take
> the rdlock as well - in which case however coroutine_wrapper_bdrv would
> not be hard to add.
> 
> Even without looking at the lock, the three series are going in the
> right direction of ultimately having more "simple" coroutine wrappers at
> the blk_* level and more coroutine functions (ultimately less wrappers,
> too) at the bdrv_* level.
> 
> Paolo
> 

Am 23.11.2022 um 12:45 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> Am 18/11/2022 um 11:57 schrieb Paolo Bonzini:
> > On 11/16/22 15:07, Emanuele Giuseppe Esposito wrote:
> >> Here we introduce generated_co_wrapper_simple, a simplification of
> >> g_c_w that
> >> only considers the case where the caller is not in a coroutine.
> >> This simplifies and clarifies a lot when the caller is a coroutine or
> >> not, and
> >> in the future will hopefully replace g_c_w.
> > 
> > This is a good idea!
> > 
> > Just one thing, though it belongs more in the two series which
> > introduced generated_co_wrapper_simple and generated_co_wrapper_blk - I
> > would make this the "official" wrapper.  So perhaps:
> > 
> > - generated_co_wrapper_simple -> coroutine_wrapper
> > - generated_co_wrapper_blk -> coroutine_wrapper_mixed
> > - generated_co_wrapper -> coroutine_wrapper_mixed_bdrv
> 
> Ah damn I forgot about this, and of course I just sent v5 for "Still
> more coroutine and various fixes in block layer".
> 
> To me it sounds good, but before I do a massive edit and then someone
> asks to revert it, @Kevin and the others do you agree?

Makes sense to me in general. I think I originally suggested that the
"basic" version should be the one that doesn't take the graph lock, and
the special version with the longer name is the one that takes it.

This proposal contains the same thought, but extends it to make
generated_co_wrapper_simple the basic thing. This is good, because
eventually we want to get rid of the mixed functions, so they are indeed
the special thing.

I think this means that if we clean up everything, in the end we'll have
coroutine_wrapper and coroutine_wrapper_bdrv (the fourth version not in
the above list, but that Paolo mentioned we may want to have).

The only thing I'm unsure about is whether coroutine_wrapper_bdrv is
descriptive enough as a name or whether it should be something more
explicit like coroutine_wrapper_bdrv_graph_locked.

Kevin

On 11/23/22 14:45, Kevin Wolf wrote:
> I think this means that if we clean up everything, in the end we'll have
> coroutine_wrapper and coroutine_wrapper_bdrv (the fourth version not in
> the above list, but that Paolo mentioned we may want to have).

Yes, I agree.

> The only thing I'm unsure about is whether coroutine_wrapper_bdrv is
> descriptive enough as a name or whether it should be something more
> explicit like coroutine_wrapper_bdrv_graph_locked.

That's already long and becomes longer if you add "mixed", but perhaps 
co_wrapper_{mixed_,}{bdrv_graph_rdlock,} would be okay?

In other words:

generated_co_wrapper_simple -> co_wrapper
generated_co_wrapper        -> co_wrapper_mixed
generated_co_wrapper_bdrv   -> co_wrapper_mixed_bdrv_graph_rdlock

?

Paolo

Am 23.11.2022 um 18:04 hat Paolo Bonzini geschrieben:
> On 11/23/22 14:45, Kevin Wolf wrote:
> > I think this means that if we clean up everything, in the end we'll have
> > coroutine_wrapper and coroutine_wrapper_bdrv (the fourth version not in
> > the above list, but that Paolo mentioned we may want to have).
> 
> Yes, I agree.
> 
> > The only thing I'm unsure about is whether coroutine_wrapper_bdrv is
> > descriptive enough as a name or whether it should be something more
> > explicit like coroutine_wrapper_bdrv_graph_locked.
> 
> That's already long and becomes longer if you add "mixed", but perhaps
> co_wrapper_{mixed_,}{bdrv_graph_rdlock,} would be okay?
> 
> In other words:
> 
> generated_co_wrapper_simple -> co_wrapper
> generated_co_wrapper        -> co_wrapper_mixed
> generated_co_wrapper_bdrv   -> co_wrapper_mixed_bdrv_graph_rdlock
> 
> ?

Works for me. Maybe co_wrapper_mixed_bdrv_rdlock (without the "graph")
would be enough, too, if it is too long.

Kevin

Am 18/11/2022 um 11:57 schrieb Paolo Bonzini:
> On 11/16/22 15:07, Emanuele Giuseppe Esposito wrote:
>> Here we introduce generated_co_wrapper_simple, a simplification of
>> g_c_w that
>> only considers the case where the caller is not in a coroutine.
>> This simplifies and clarifies a lot when the caller is a coroutine or
>> not, and
>> in the future will hopefully replace g_c_w.
> 
> This is a good idea!
> 
> Just one thing, though it belongs more in the two series which
> introduced generated_co_wrapper_simple and generated_co_wrapper_blk - I
> would make this the "official" wrapper.  So perhaps:
> 
> - generated_co_wrapper_simple -> coroutine_wrapper
> - generated_co_wrapper_blk -> coroutine_wrapper_mixed
> - generated_co_wrapper -> coroutine_wrapper_mixed_bdrv
> 
> ?  It is not clear to me yet if you will have bdrv_* functions that take
> the rdlock as well - in which case however coroutine_wrapper_bdrv would
> not be hard to add.

Why _mixed? I thought _blk was a nice name to identify only the blk_*
API that have this slightly different behavior (ie do not take the
rwlock anywhere).

No, I don't think there will be bdrv_* functions that will behave as
blk_*, if that's what you mean.

Regarding the bdrv_* functions in general, there are a couple of
additional places (which should be all in the main loop) where we need
to use assert_bdrv_grapg_readable. In those places we theoretically need
nothing, but if we use the automatic TSA locking checker that is being
tested by kevin, we need some sort of "placeholder" so that cc/gcc (I
don't remember anymore which) won't throw false positives.

> 
> Even without looking at the lock, the three series are going in the
> right direction of ultimately having more "simple" coroutine wrappers at
> the blk_* level and more coroutine functions (ultimately less wrappers,
> too) at the bdrv_* level.
> 
> Paolo
> 

On Fri, Nov 18, 2022 at 4:01 PM Emanuele Giuseppe Esposito
<eesposit@redhat.com> wrote:
> > - generated_co_wrapper_simple -> coroutine_wrapper
> > - generated_co_wrapper_blk -> coroutine_wrapper_mixed
> > - generated_co_wrapper -> coroutine_wrapper_mixed_bdrv
> >
> > ?  It is not clear to me yet if you will have bdrv_* functions that take
> > the rdlock as well - in which case however coroutine_wrapper_bdrv would
> > not be hard to add.
>
> Why _mixed? I thought _blk was a nice name to identify only the blk_*
> API that have this slightly different behavior (ie do not take the
> rwlock anywhere).

_mixed means that they are callable from both coroutine and
non-coroutine function, but (unlike "normal" functions) they will
suspend if called in coroutine context.

In fact we could also have a coroutine_mixed_fn static analysis
annotation for functions that *call* coroutine_wrapper_mixed (so
ultimately they can suspend when called from coroutine context, e.g.
vhdx_log_write_and_flush or qcow2's update_refcount):

- coroutine_fn can call coroutine_mixed_fn if needed, but cannot call
any coroutine_wrapper*

- coroutine_mixed_fn can call coroutine_mixed_fn or
coroutine_wrapper_mixed{,_bdrv}, but cannot call coroutine_wrapper

This of course is completely independent of your series, but since the
naming is adjacent it would be nice to only change it once.

> No, I don't think there will be bdrv_* functions that will behave as
> blk_*, if that's what you mean.
>
> Regarding the bdrv_* functions in general, there are a couple of
> additional places (which should be all in the main loop) where we need
> to use assert_bdrv_grapg_readable. In those places we theoretically need
> nothing, but if we use the automatic TSA locking checker that is being
> tested by kevin, we need some sort of "placeholder" so that cc/gcc (I
> don't remember anymore which) won't throw false positives.

clang. :)  That can be sorted out with review, but in general I think
asserting is always okay.

Paolo