| Message ID | 20221116140730.3056048-1-eesposit@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Protect the block layer with a rwlock: part 3 | expand |
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 > > ? 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
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(-) >
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
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(-)