| Message ID | 1508816031-82709-1-git-send-email-sochin.jiang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue 24 Oct 2017 05:33:51 AM CEST, sochin jiang wrote: > --- a/block/throttle-groups.c > +++ b/block/throttle-groups.c > @@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm) > > /* remove the current tgm from the list */ > QLIST_REMOVE(tgm, round_robin); > - throttle_timers_destroy(&tgm->throttle_timers); > + if (throttle_timers_are_initialized(&tgm->throttle_timers)) { > + throttle_timers_destroy(&tgm->throttle_timers); > + } > qemu_mutex_unlock(&tg->lock); > > throttle_group_unref(&tg->ts); I don't know what the rest of the people think, but I'm not completely convinced that it's a good idea to have an active ThrottleState inside a ThrottleGroupMember without timers. Perhaps in blk_remove_bs() after detaching the AioContext from the BDS we can attach the default one (what you would get with blk_get_aio_context()). On the other hand I think that Manos's series to remove the legacy throttling code gets rid of BlockBackend.ThrottleGroupMember completely. Berto
On Wed, Oct 25, 2017 at 10:40:47AM +0200, Alberto Garcia wrote: > On Tue 24 Oct 2017 05:33:51 AM CEST, sochin jiang wrote: > > --- a/block/throttle-groups.c > > +++ b/block/throttle-groups.c > > @@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm) > > > > /* remove the current tgm from the list */ > > QLIST_REMOVE(tgm, round_robin); > > - throttle_timers_destroy(&tgm->throttle_timers); > > + if (throttle_timers_are_initialized(&tgm->throttle_timers)) { > > + throttle_timers_destroy(&tgm->throttle_timers); > > + } > > qemu_mutex_unlock(&tg->lock); > > > > throttle_group_unref(&tg->ts); > > I don't know what the rest of the people think, but I'm not completely > convinced that it's a good idea to have an active ThrottleState inside a > ThrottleGroupMember without timers. > > Perhaps in blk_remove_bs() after detaching the AioContext from the BDS > we can attach the default one (what you would get with > blk_get_aio_context()). > > On the other hand I think that Manos's series to remove the legacy > throttling code gets rid of BlockBackend.ThrottleGroupMember completely. What is the status of Manos' series? It would be good to merge it and then consider currently open throttling bugs again. Stefan
On Fri, Nov 03, 2017 at 02:26:33PM +0000, Stefan Hajnoczi wrote: >On Wed, Oct 25, 2017 at 10:40:47AM +0200, Alberto Garcia wrote: >> On Tue 24 Oct 2017 05:33:51 AM CEST, sochin jiang wrote: >> > --- a/block/throttle-groups.c >> > +++ b/block/throttle-groups.c >> > @@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm) >> > >> > /* remove the current tgm from the list */ >> > QLIST_REMOVE(tgm, round_robin); >> > - throttle_timers_destroy(&tgm->throttle_timers); >> > + if (throttle_timers_are_initialized(&tgm->throttle_timers)) { >> > + throttle_timers_destroy(&tgm->throttle_timers); >> > + } >> > qemu_mutex_unlock(&tg->lock); >> > >> > throttle_group_unref(&tg->ts); >> >> I don't know what the rest of the people think, but I'm not completely >> convinced that it's a good idea to have an active ThrottleState inside a >> ThrottleGroupMember without timers. >> >> Perhaps in blk_remove_bs() after detaching the AioContext from the BDS >> we can attach the default one (what you would get with >> blk_get_aio_context()). >> >> On the other hand I think that Manos's series to remove the legacy >> throttling code gets rid of BlockBackend.ThrottleGroupMember completely. > >What is the status of Manos' series? > >It would be good to merge it and then consider currently open throttling >bugs again. > I have been busy with schoolwork the past weeks, but I'm close to finishing some changes Kevin recommended for this series. I hope they will be ready soon.
On Tue, Oct 24, 2017 at 11:33:51AM +0800, sochin jiang wrote: > commit 7ca7f0 moves the throttling related part of the BDS life cycle > management to BlockBackend, adds call to > throttle_timers_detach_aio_context in blk_remove_bs. commit 1606e > remove a block device from its throttle group in blk_delete by calling > blk_io_limits_disable, this fix an easily reproducible qemu crash. But > delete a BB without a BDS inserted could easily cause a qemu crash too > by calling bdrv_drained_begin in blk_io_limits_disable. Say, a simply > drive_add and then a drive_del command. > > This patch removes draining BDS by calling throttle_group_unregister_tgm > directly instead of blk_io_limits_disable, leaves draining operation to > blk_remove_bs in case that there is no BDS inserted. Futhermore, make sure > throttle timers are initialized or attached before throttle_timers_destroy > is called in throttle_group_unregister_tgm. > > Signed-off-by: sochin jiang <sochin.jiang@huawei.com> > --- > block/block-backend.c | 2 +- > block/throttle-groups.c | 4 +++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 45d9101..39c7cca 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -341,7 +341,7 @@ static void blk_delete(BlockBackend *blk) > assert(!blk->name); > assert(!blk->dev); > if (blk->public.throttle_group_member.throttle_state) { > - blk_io_limits_disable(blk); > + throttle_group_unregister_tgm(&blk->public.throttle_group_member); The following assertions fail without the drain when there are pending requests: void throttle_group_unregister_tgm(ThrottleGroupMember *tgm) { ThrottleState *ts = tgm->throttle_state; ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts); ThrottleGroupMember *token; int i; if (!ts) { /* Discard already unregistered tgm */ return; } assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0); assert(qemu_co_queue_empty(&tgm->throttled_reqs[0])); assert(qemu_co_queue_empty(&tgm->throttled_reqs[1])); A safer approach is making blk_io_limits_disable(blk) skip the draining when blk_bs(blk) == NULL. That is the only case where we are 100% sure that there are no pending requests.
On Thu 09 Nov 2017 06:12:10 PM CET, Stefan Hajnoczi wrote: >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 45d9101..39c7cca 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -341,7 +341,7 @@ static void blk_delete(BlockBackend *blk) >> assert(!blk->name); >> assert(!blk->dev); >> if (blk->public.throttle_group_member.throttle_state) { >> - blk_io_limits_disable(blk); >> + throttle_group_unregister_tgm(&blk->public.throttle_group_member); > > The following assertions fail without the drain when there are pending > requests: > > void throttle_group_unregister_tgm(ThrottleGroupMember *tgm) > { > ThrottleState *ts = tgm->throttle_state; > ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts); > ThrottleGroupMember *token; > int i; > > if (!ts) { > /* Discard already unregistered tgm */ > return; > } > > assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0); > assert(qemu_co_queue_empty(&tgm->throttled_reqs[0])); > assert(qemu_co_queue_empty(&tgm->throttled_reqs[1])); > > A safer approach is making blk_io_limits_disable(blk) skip the > draining when blk_bs(blk) == NULL. That is the only case where we are > 100% sure that there are no pending requests. I think so too. And as I said in a previous e-mail I don't know if we should have a valid blk->public.throttle_group_member.throttle_state with no timers at all. I'd rather attach the default AioContext while the BDS is removed. I'll try to prepare a patch with all of this together. Berto
diff --git a/block/block-backend.c b/block/block-backend.c index 45d9101..39c7cca 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -341,7 +341,7 @@ static void blk_delete(BlockBackend *blk) assert(!blk->name); assert(!blk->dev); if (blk->public.throttle_group_member.throttle_state) { - blk_io_limits_disable(blk); + throttle_group_unregister_tgm(&blk->public.throttle_group_member); } if (blk->root) { blk_remove_bs(blk); diff --git a/block/throttle-groups.c b/block/throttle-groups.c index b291a88..c5f9af3 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm) /* remove the current tgm from the list */ QLIST_REMOVE(tgm, round_robin); - throttle_timers_destroy(&tgm->throttle_timers); + if (throttle_timers_are_initialized(&tgm->throttle_timers)) { + throttle_timers_destroy(&tgm->throttle_timers); + } qemu_mutex_unlock(&tg->lock); throttle_group_unref(&tg->ts);
commit 7ca7f0 moves the throttling related part of the BDS life cycle management to BlockBackend, adds call to throttle_timers_detach_aio_context in blk_remove_bs. commit 1606e remove a block device from its throttle group in blk_delete by calling blk_io_limits_disable, this fix an easily reproducible qemu crash. But delete a BB without a BDS inserted could easily cause a qemu crash too by calling bdrv_drained_begin in blk_io_limits_disable. Say, a simply drive_add and then a drive_del command. This patch removes draining BDS by calling throttle_group_unregister_tgm directly instead of blk_io_limits_disable, leaves draining operation to blk_remove_bs in case that there is no BDS inserted. Futhermore, make sure throttle timers are initialized or attached before throttle_timers_destroy is called in throttle_group_unregister_tgm. Signed-off-by: sochin jiang <sochin.jiang@huawei.com> --- block/block-backend.c | 2 +- block/throttle-groups.c | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)