| Message ID | 20221104095700.4117433-2-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Still more coroutine and various fixes in block layer | expand |
On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote: > It seems that bdrv_open_driver() forgot to create a coroutine > where to call bs->drv->bdrv_co_drain_begin(), a callback > marked as coroutine_fn. > > Because there is no active I/O at this point, the coroutine > should end right after entering, so the caller does not need > to poll until it is finished. Hmm. I see your point. But isn't it better to go the generic way and use a generated coroutine wrapper? Nothing guarantees that .bdrv_co_drain_begin() handlers will never do any yield point even on driver open... Look for example at bdrv_co_check(). It has a generated wrapper bdrv_check(), declared in include/block/block-io.h So you just need to declare the wrapper, and use it in bdrv_open_driver(), the code would be clearer too. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/block.c b/block.c > index 5311b21f8e..d2b2800039 100644 > --- a/block.c > +++ b/block.c > @@ -1637,12 +1637,34 @@ out: > g_free(gen_node_name); > } > > +typedef struct DrainCo { > + BlockDriverState *bs; > + int ret; > +} DrainCo; > + > +static void coroutine_fn bdrv_co_drain_begin(void *opaque) > +{ > + int i; > + DrainCo *co = opaque; > + BlockDriverState *bs = co->bs; > + > + for (i = 0; i < bs->quiesce_counter; i++) { > + bs->drv->bdrv_co_drain_begin(bs); > + } > + co->ret = 0; > +} > + > static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, > const char *node_name, QDict *options, > int open_flags, Error **errp) > { > Error *local_err = NULL; > - int i, ret; > + int ret; > + Coroutine *co; > + DrainCo dco = { > + .bs = bs, > + .ret = NOT_DONE, > + }; > GLOBAL_STATE_CODE(); > > bdrv_assign_node_name(bs, node_name, &local_err); > @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, > assert(bdrv_min_mem_align(bs) != 0); > assert(is_power_of_2(bs->bl.request_alignment)); > > - for (i = 0; i < bs->quiesce_counter; i++) { > - if (drv->bdrv_co_drain_begin) { > - drv->bdrv_co_drain_begin(bs); > - } > + if (drv->bdrv_co_drain_begin) { > + co = qemu_coroutine_create(bdrv_co_drain_begin, &dco); > + qemu_coroutine_enter(co); > + /* > + * There should be no reason for drv->bdrv_co_drain_begin to wait at > + * this point, because the device does not have any active I/O. > + */ > + assert(dco.ret != NOT_DONE); > } > > return 0;
Am 08/11/2022 um 15:33 schrieb Vladimir Sementsov-Ogievskiy: > On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote: >> It seems that bdrv_open_driver() forgot to create a coroutine >> where to call bs->drv->bdrv_co_drain_begin(), a callback >> marked as coroutine_fn. >> >> Because there is no active I/O at this point, the coroutine >> should end right after entering, so the caller does not need >> to poll until it is finished. > > Hmm. I see your point. But isn't it better to go the generic way and use > a generated coroutine wrapper? Nothing guarantees that > .bdrv_co_drain_begin() handlers will never do any yield point even on > driver open... > > Look for example at bdrv_co_check(). It has a generated wrapper > bdrv_check(), declared in include/block/block-io.h > > So you just need to declare the wrapper, and use it in > bdrv_open_driver(), the code would be clearer too. I think (and this is a repetition from my previous email) it confuses the code. It is clear, but then you don't know if we are in a coroutine context or not. I am very well aware of what you did with your script, and I am working on extending your g_c_w class with g_c_w_simple, where we drop the if(qemu_in_coroutine()) case and leave just the coroutine creation. Therefore, coroutine functions we use only the _co_ function, otherwise we use g_c_w_simple. In this way code is clear as you point out, but there is no confusion. Thank you, Emanuele > >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> block.c | 36 +++++++++++++++++++++++++++++++----- >> 1 file changed, 31 insertions(+), 5 deletions(-) >> >> diff --git a/block.c b/block.c >> index 5311b21f8e..d2b2800039 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1637,12 +1637,34 @@ out: >> g_free(gen_node_name); >> } >> +typedef struct DrainCo { >> + BlockDriverState *bs; >> + int ret; >> +} DrainCo; >> + >> +static void coroutine_fn bdrv_co_drain_begin(void *opaque) >> +{ >> + int i; >> + DrainCo *co = opaque; >> + BlockDriverState *bs = co->bs; >> + >> + for (i = 0; i < bs->quiesce_counter; i++) { >> + bs->drv->bdrv_co_drain_begin(bs); >> + } >> + co->ret = 0; >> +} >> + >> static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >> const char *node_name, QDict *options, >> int open_flags, Error **errp) >> { >> Error *local_err = NULL; >> - int i, ret; >> + int ret; >> + Coroutine *co; >> + DrainCo dco = { >> + .bs = bs, >> + .ret = NOT_DONE, >> + }; >> GLOBAL_STATE_CODE(); >> bdrv_assign_node_name(bs, node_name, &local_err); >> @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState >> *bs, BlockDriver *drv, >> assert(bdrv_min_mem_align(bs) != 0); >> assert(is_power_of_2(bs->bl.request_alignment)); >> - for (i = 0; i < bs->quiesce_counter; i++) { >> - if (drv->bdrv_co_drain_begin) { >> - drv->bdrv_co_drain_begin(bs); >> - } >> + if (drv->bdrv_co_drain_begin) { >> + co = qemu_coroutine_create(bdrv_co_drain_begin, &dco); >> + qemu_coroutine_enter(co); >> + /* >> + * There should be no reason for drv->bdrv_co_drain_begin to >> wait at >> + * this point, because the device does not have any active I/O. >> + */ >> + assert(dco.ret != NOT_DONE); >> } >> return 0; >
Am 08.11.2022 um 15:33 hat Vladimir Sementsov-Ogievskiy geschrieben: > On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote: > > It seems that bdrv_open_driver() forgot to create a coroutine > > where to call bs->drv->bdrv_co_drain_begin(), a callback > > marked as coroutine_fn. > > > > Because there is no active I/O at this point, the coroutine > > should end right after entering, so the caller does not need > > to poll until it is finished. > > Hmm. I see your point. But isn't it better to go the generic way and > use a generated coroutine wrapper? Nothing guarantees that > .bdrv_co_drain_begin() handlers will never do any yield point even on > driver open... > > Look for example at bdrv_co_check(). It has a generated wrapper > bdrv_check(), declared in include/block/block-io.h > > So you just need to declare the wrapper, and use it in > bdrv_open_driver(), the code would be clearer too. Note that if we apply the drain simplification series I sent today up to at least patch 3 ('block: Revert .bdrv_drained_begin/end to non-coroutine_fn') [1], then this patch isn't actually needed any more. Kevin [1] https://lists.gnu.org/archive/html/qemu-block/2022-11/msg00206.html
On 11/8/22 18:13, Emanuele Giuseppe Esposito wrote: > > > Am 08/11/2022 um 15:33 schrieb Vladimir Sementsov-Ogievskiy: >> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote: >>> It seems that bdrv_open_driver() forgot to create a coroutine >>> where to call bs->drv->bdrv_co_drain_begin(), a callback >>> marked as coroutine_fn. >>> >>> Because there is no active I/O at this point, the coroutine >>> should end right after entering, so the caller does not need >>> to poll until it is finished. >> >> Hmm. I see your point. But isn't it better to go the generic way and use >> a generated coroutine wrapper? Nothing guarantees that >> .bdrv_co_drain_begin() handlers will never do any yield point even on >> driver open... >> >> Look for example at bdrv_co_check(). It has a generated wrapper >> bdrv_check(), declared in include/block/block-io.h >> >> So you just need to declare the wrapper, and use it in >> bdrv_open_driver(), the code would be clearer too. > > I think (and this is a repetition from my previous email) it confuses > the code. It is clear, but then you don't know if we are in a coroutine > context or not. Hmm. But same thing is true for all callers of coroutine wrappers. I agree that "coroutine wrapper" is a workaround for the design problem. Really, the fact that in many places we don't know are we in coroutine or not is very uncomfortable. But still, I'm not sure that's it good to avoid this workaround in one place and continue to use it in all other places. I think following common design is better. Or rework it deeply by getting rid of the wrappers somehow. > > I am very well aware of what you did with your script, and I am working > on extending your g_c_w class with g_c_w_simple, where we drop the > if(qemu_in_coroutine()) case and leave just the coroutine creation. > Therefore, coroutine functions we use only the _co_ function, otherwise > we use g_c_w_simple. > In this way code is clear as you point out, but there is no confusion. Hmm sounds good, I missed it. Why not use g_c_w_simple here than? > > Thank you, > Emanuele >> >>> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> block.c | 36 +++++++++++++++++++++++++++++++----- >>> 1 file changed, 31 insertions(+), 5 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index 5311b21f8e..d2b2800039 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -1637,12 +1637,34 @@ out: >>> g_free(gen_node_name); >>> } >>> +typedef struct DrainCo { >>> + BlockDriverState *bs; >>> + int ret; >>> +} DrainCo; >>> + >>> +static void coroutine_fn bdrv_co_drain_begin(void *opaque) >>> +{ >>> + int i; >>> + DrainCo *co = opaque; >>> + BlockDriverState *bs = co->bs; >>> + >>> + for (i = 0; i < bs->quiesce_counter; i++) { >>> + bs->drv->bdrv_co_drain_begin(bs); >>> + } >>> + co->ret = 0; >>> +} >>> + >>> static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >>> const char *node_name, QDict *options, >>> int open_flags, Error **errp) >>> { >>> Error *local_err = NULL; >>> - int i, ret; >>> + int ret; >>> + Coroutine *co; >>> + DrainCo dco = { >>> + .bs = bs, >>> + .ret = NOT_DONE, >>> + }; >>> GLOBAL_STATE_CODE(); >>> bdrv_assign_node_name(bs, node_name, &local_err); >>> @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState >>> *bs, BlockDriver *drv, >>> assert(bdrv_min_mem_align(bs) != 0); >>> assert(is_power_of_2(bs->bl.request_alignment)); >>> - for (i = 0; i < bs->quiesce_counter; i++) { >>> - if (drv->bdrv_co_drain_begin) { >>> - drv->bdrv_co_drain_begin(bs); >>> - } >>> + if (drv->bdrv_co_drain_begin) { >>> + co = qemu_coroutine_create(bdrv_co_drain_begin, &dco); >>> + qemu_coroutine_enter(co); >>> + /* >>> + * There should be no reason for drv->bdrv_co_drain_begin to >>> wait at >>> + * this point, because the device does not have any active I/O. >>> + */ >>> + assert(dco.ret != NOT_DONE); >>> } >>> return 0; >> >
Am 08/11/2022 um 16:52 schrieb Vladimir Sementsov-Ogievskiy: > On 11/8/22 18:13, Emanuele Giuseppe Esposito wrote: >> >> >> Am 08/11/2022 um 15:33 schrieb Vladimir Sementsov-Ogievskiy: >>> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote: >>>> It seems that bdrv_open_driver() forgot to create a coroutine >>>> where to call bs->drv->bdrv_co_drain_begin(), a callback >>>> marked as coroutine_fn. >>>> >>>> Because there is no active I/O at this point, the coroutine >>>> should end right after entering, so the caller does not need >>>> to poll until it is finished. >>> >>> Hmm. I see your point. But isn't it better to go the generic way and use >>> a generated coroutine wrapper? Nothing guarantees that >>> .bdrv_co_drain_begin() handlers will never do any yield point even on >>> driver open... >>> >>> Look for example at bdrv_co_check(). It has a generated wrapper >>> bdrv_check(), declared in include/block/block-io.h >>> >>> So you just need to declare the wrapper, and use it in >>> bdrv_open_driver(), the code would be clearer too. >> >> I think (and this is a repetition from my previous email) it confuses >> the code. It is clear, but then you don't know if we are in a coroutine >> context or not. > > Hmm. But same thing is true for all callers of coroutine wrappers. > > I agree that "coroutine wrapper" is a workaround for the design problem. > Really, the fact that in many places we don't know are we in coroutine > or not is very uncomfortable. And the only way to figure if it's a coroutine or not is by adding assertions and pray that the iotests don't fail *and* cover all cases. > > But still, I'm not sure that's it good to avoid this workaround in one > place and continue to use it in all other places. I think following > common design is better. Or rework it deeply by getting rid of the > wrappers somehow. Well, one step at the time :) it's already difficult to verify that such replacement cover and is correct for all cases :) > >> >> I am very well aware of what you did with your script, and I am working >> on extending your g_c_w class with g_c_w_simple, where we drop the >> if(qemu_in_coroutine()) case and leave just the coroutine creation. >> Therefore, coroutine functions we use only the _co_ function, otherwise >> we use g_c_w_simple. >> In this way code is clear as you point out, but there is no confusion. > > Hmm sounds good, I missed it. Why not use g_c_w_simple here than? Because I came up with it this morning. Thank you, Emanuele > >> >> Thank you, >> Emanuele >>> >>>> >>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> >>>> --- >>>> block.c | 36 +++++++++++++++++++++++++++++++----- >>>> 1 file changed, 31 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index 5311b21f8e..d2b2800039 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -1637,12 +1637,34 @@ out: >>>> g_free(gen_node_name); >>>> } >>>> +typedef struct DrainCo { >>>> + BlockDriverState *bs; >>>> + int ret; >>>> +} DrainCo; >>>> + >>>> +static void coroutine_fn bdrv_co_drain_begin(void *opaque) >>>> +{ >>>> + int i; >>>> + DrainCo *co = opaque; >>>> + BlockDriverState *bs = co->bs; >>>> + >>>> + for (i = 0; i < bs->quiesce_counter; i++) { >>>> + bs->drv->bdrv_co_drain_begin(bs); >>>> + } >>>> + co->ret = 0; >>>> +} >>>> + >>>> static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >>>> const char *node_name, QDict *options, >>>> int open_flags, Error **errp) >>>> { >>>> Error *local_err = NULL; >>>> - int i, ret; >>>> + int ret; >>>> + Coroutine *co; >>>> + DrainCo dco = { >>>> + .bs = bs, >>>> + .ret = NOT_DONE, >>>> + }; >>>> GLOBAL_STATE_CODE(); >>>> bdrv_assign_node_name(bs, node_name, &local_err); >>>> @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState >>>> *bs, BlockDriver *drv, >>>> assert(bdrv_min_mem_align(bs) != 0); >>>> assert(is_power_of_2(bs->bl.request_alignment)); >>>> - for (i = 0; i < bs->quiesce_counter; i++) { >>>> - if (drv->bdrv_co_drain_begin) { >>>> - drv->bdrv_co_drain_begin(bs); >>>> - } >>>> + if (drv->bdrv_co_drain_begin) { >>>> + co = qemu_coroutine_create(bdrv_co_drain_begin, &dco); >>>> + qemu_coroutine_enter(co); >>>> + /* >>>> + * There should be no reason for drv->bdrv_co_drain_begin to >>>> wait at >>>> + * this point, because the device does not have any active >>>> I/O. >>>> + */ >>>> + assert(dco.ret != NOT_DONE); >>>> } >>>> return 0; >>> >> >
diff --git a/block.c b/block.c index 5311b21f8e..d2b2800039 100644 --- a/block.c +++ b/block.c @@ -1637,12 +1637,34 @@ out: g_free(gen_node_name); } +typedef struct DrainCo { + BlockDriverState *bs; + int ret; +} DrainCo; + +static void coroutine_fn bdrv_co_drain_begin(void *opaque) +{ + int i; + DrainCo *co = opaque; + BlockDriverState *bs = co->bs; + + for (i = 0; i < bs->quiesce_counter; i++) { + bs->drv->bdrv_co_drain_begin(bs); + } + co->ret = 0; +} + static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, const char *node_name, QDict *options, int open_flags, Error **errp) { Error *local_err = NULL; - int i, ret; + int ret; + Coroutine *co; + DrainCo dco = { + .bs = bs, + .ret = NOT_DONE, + }; GLOBAL_STATE_CODE(); bdrv_assign_node_name(bs, node_name, &local_err); @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, assert(bdrv_min_mem_align(bs) != 0); assert(is_power_of_2(bs->bl.request_alignment)); - for (i = 0; i < bs->quiesce_counter; i++) { - if (drv->bdrv_co_drain_begin) { - drv->bdrv_co_drain_begin(bs); - } + if (drv->bdrv_co_drain_begin) { + co = qemu_coroutine_create(bdrv_co_drain_begin, &dco); + qemu_coroutine_enter(co); + /* + * There should be no reason for drv->bdrv_co_drain_begin to wait at + * this point, because the device does not have any active I/O. + */ + assert(dco.ret != NOT_DONE); } return 0;