| Message ID | 20221213085320.95673-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Move more functions to coroutines | expand |
On 12/13/22 11:53, Kevin Wolf wrote: > From: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > BlockDriver->bdrv_io_plug is categorized as IO callback, and it > currently doesn't run in a coroutine. We should let it take a graph > rdlock since the callback traverses the block nodes graph, which however > is only possible in a coroutine. > > The only caller of this function is blk_io_plug(), therefore make > blk_io_plug() a co_wrapper, so that we're always running in a coroutine > where the lock can be taken. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- [..] > --- a/include/block/block_int-common.h > +++ b/include/block/block_int-common.h > @@ -729,7 +729,7 @@ struct BlockDriver { > void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event); > > /* io queue for linux-aio */ > - void (*bdrv_io_plug)(BlockDriverState *bs); > + void coroutine_fn (*bdrv_io_plug)(BlockDriverState *bs); don't we want to rename it to _co_ too? anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
On 12/13/22 11:53, Kevin Wolf wrote: > --- a/include/block/block_int-common.h > +++ b/include/block/block_int-common.h > @@ -729,7 +729,7 @@ struct BlockDriver { > void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event); > > /* io queue for linux-aio */ > - void (*bdrv_io_plug)(BlockDriverState *bs); > + void coroutine_fn (*bdrv_io_plug)(BlockDriverState *bs); But you didn't add coroutine_fn to realizations of this callback. Seems, it should be done
Am 16/12/2022 um 15:26 schrieb Vladimir Sementsov-Ogievskiy: > On 12/13/22 11:53, Kevin Wolf wrote: >> From: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> >> BlockDriver->bdrv_io_plug is categorized as IO callback, and it >> currently doesn't run in a coroutine. We should let it take a graph >> rdlock since the callback traverses the block nodes graph, which however >> is only possible in a coroutine. >> >> The only caller of this function is blk_io_plug(), therefore make >> blk_io_plug() a co_wrapper, so that we're always running in a coroutine >> where the lock can be taken. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- > > [..] > >> --- a/include/block/block_int-common.h >> +++ b/include/block/block_int-common.h >> @@ -729,7 +729,7 @@ struct BlockDriver { >> void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent >> event); >> /* io queue for linux-aio */ >> - void (*bdrv_io_plug)(BlockDriverState *bs); >> + void coroutine_fn (*bdrv_io_plug)(BlockDriverState *bs); > > don't we want to rename it to _co_ too? I think you realized this is done in patch 14 Thanks, Emanuele > > anyway: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > >
Am 16/12/2022 um 17:12 schrieb Vladimir Sementsov-Ogievskiy: > On 12/13/22 11:53, Kevin Wolf wrote: >> --- a/include/block/block_int-common.h >> +++ b/include/block/block_int-common.h >> @@ -729,7 +729,7 @@ struct BlockDriver { >> void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent >> event); >> /* io queue for linux-aio */ >> - void (*bdrv_io_plug)(BlockDriverState *bs); >> + void coroutine_fn (*bdrv_io_plug)(BlockDriverState *bs); > > But you didn't add coroutine_fn to realizations of this callback. Seems, > it should be done > This has to be done by Paolo & Alberto Faria work to add missing coroutine_fn and various annotations in a systematic way. It has to be done using tools like vrc, and not manually because it's not feasible. Not covered by this serie. Thank you, Emanuele
Am 19.12.2022 um 13:26 hat Emanuele Giuseppe Esposito geschrieben: > Am 16/12/2022 um 17:12 schrieb Vladimir Sementsov-Ogievskiy: > > On 12/13/22 11:53, Kevin Wolf wrote: > >> --- a/include/block/block_int-common.h > >> +++ b/include/block/block_int-common.h > >> @@ -729,7 +729,7 @@ struct BlockDriver { > >> void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent > >> event); > >> /* io queue for linux-aio */ > >> - void (*bdrv_io_plug)(BlockDriverState *bs); > >> + void coroutine_fn (*bdrv_io_plug)(BlockDriverState *bs); > > > > But you didn't add coroutine_fn to realizations of this callback. Seems, > > it should be done > > This has to be done by Paolo & Alberto Faria work to add missing > coroutine_fn and various annotations in a systematic way. It has to be > done using tools like vrc, and not manually because it's not feasible. > Not covered by this serie. I didn't want to change your patches too much (and I'm sure it has nothing at all to do with laziness!), but Vladimir is really right. Doing the renames in the final patch is a bit sloppy, just as not adding coroutine_fn is. I'll fix it for v2 to do all of the changes in the respective patches. Kevin
diff --git a/include/block/block-io.h b/include/block/block-io.h index 2ed6214909..d96168375e 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -217,7 +217,8 @@ void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co); AioContext *child_of_bds_get_parent_aio_context(BdrvChild *c); -void bdrv_io_plug(BlockDriverState *bs); +void coroutine_fn bdrv_co_io_plug(BlockDriverState *bs); + void bdrv_io_unplug(BlockDriverState *bs); bool coroutine_fn bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index c34c525fa6..4a1c1e348a 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -729,7 +729,7 @@ struct BlockDriver { void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event); /* io queue for linux-aio */ - void (*bdrv_io_plug)(BlockDriverState *bs); + void coroutine_fn (*bdrv_io_plug)(BlockDriverState *bs); void (*bdrv_io_unplug)(BlockDriverState *bs); /** diff --git a/include/sysemu/block-backend-io.h b/include/sysemu/block-backend-io.h index 7ec6d978d4..70b73f7d11 100644 --- a/include/sysemu/block-backend-io.h +++ b/include/sysemu/block-backend-io.h @@ -73,7 +73,9 @@ void blk_iostatus_set_err(BlockBackend *blk, int error); int blk_get_max_iov(BlockBackend *blk); int blk_get_max_hw_iov(BlockBackend *blk); -void blk_io_plug(BlockBackend *blk); +void coroutine_fn blk_co_io_plug(BlockBackend *blk); +void co_wrapper blk_io_plug(BlockBackend *blk); + void blk_io_unplug(BlockBackend *blk); AioContext *blk_get_aio_context(BlockBackend *blk); BlockAcctStats *blk_get_stats(BlockBackend *blk); diff --git a/block/block-backend.c b/block/block-backend.c index 2852a892de..f862fd1950 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -2315,13 +2315,13 @@ void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify) notifier_list_add(&blk->insert_bs_notifiers, notify); } -void blk_io_plug(BlockBackend *blk) +void coroutine_fn blk_co_io_plug(BlockBackend *blk) { BlockDriverState *bs = blk_bs(blk); IO_CODE(); if (bs) { - bdrv_io_plug(bs); + bdrv_co_io_plug(bs); } } diff --git a/block/io.c b/block/io.c index d87788dfbb..3d27836420 100644 --- a/block/io.c +++ b/block/io.c @@ -3127,13 +3127,13 @@ void *qemu_try_blockalign0(BlockDriverState *bs, size_t size) return mem; } -void bdrv_io_plug(BlockDriverState *bs) +void coroutine_fn bdrv_co_io_plug(BlockDriverState *bs) { BdrvChild *child; IO_CODE(); QLIST_FOREACH(child, &bs->children, next) { - bdrv_io_plug(child->bs); + bdrv_co_io_plug(child->bs); } if (qatomic_fetch_inc(&bs->io_plugged) == 0) {