| Message ID | e089c66e7c20289b046d782cea4373b765c5bc1d.1510339534.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix throttling crashes in BlockBackend with no BlockDriverState | expand |
On 2017-11-10 19:54, Alberto Garcia wrote: > If a BlockBackend has I/O limits set then its ThrottleGroupMember > structure uses the AioContext from its attached BlockDriverState. > Those two contexts must be kept in sync manually. This is not > ideal and will be fixed in the future by removing the throttling > configuration from the BlockBackend and storing it in an implicit > filter node instead, but for now we have to live with this. > > When you remove the BlockDriverState from the backend then the > throttle timers are destroyed. If a new BlockDriverState is later > inserted then they are created again using the new AioContext. > > There'a a couple of problems with this: > > a) The code manipulates the timers directly, leaving the > ThrottleGroupMember.aio_context field in an inconsisent state. > > b) If you remove the I/O limits (e.g by destroying the backend) > when the timers are gone then throttle_group_unregister_tgm() > will attempt to destroy them again, crashing QEMU. > > While b) could be fixed easily by allowing the timers to be freed > twice, this would result in a situation in which we can no longer > guarantee that a valid ThrottleState has a valid AioContext and > timers. > > This patch ensures that the timers and AioContext are always valid > when I/O limits are set, regardless of whether the BlockBackend has a > BlockDriverState inserted or not. > > Reported-by: sochin jiang <sochin.jiang@huawei.com> > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/block-backend.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On Fri 10 Nov 2017 07:54:47 PM CET, Alberto Garcia <berto@igalia.com> wrote:
I just noticed a typo in the commit message:
> There'a a couple of problems with this:
"There's a couple"
If there's no v2 of this series you can correct this when committing.
Berto
On 2017-11-10 23:06, Alberto Garcia wrote: > On Fri 10 Nov 2017 07:54:47 PM CET, Alberto Garcia <berto@igalia.com> wrote: > > I just noticed a typo in the commit message: > >> There'a a couple of problems with this: > > "There's a couple" > > If there's no v2 of this series you can correct this when committing. Well, the issue is that it's going to be interesting to commit this because it depends on Stefan's tree now... Maybe this series can go through his as well? Max
On Fri 10 Nov 2017 11:08:20 PM CET, Max Reitz <mreitz@redhat.com> wrote: >> I just noticed a typo in the commit message: >> >>> There'a a couple of problems with this: >> >> "There's a couple" >> >> If there's no v2 of this series you can correct this when committing. > > Well, the issue is that it's going to be interesting to commit this > because it depends on Stefan's tree now... I meant whoever commits this, Stefan in this case I guess :-) Berto
diff --git a/block/block-backend.c b/block/block-backend.c index 193a080096..38db6e639b 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -655,15 +655,15 @@ BlockBackend *blk_by_public(BlockBackendPublic *public) */ void blk_remove_bs(BlockBackend *blk) { + ThrottleGroupMember *tgm = &blk->public.throttle_group_member; BlockDriverState *bs; - ThrottleTimers *tt; notifier_list_notify(&blk->remove_bs_notifiers, blk); - if (blk->public.throttle_group_member.throttle_state) { - tt = &blk->public.throttle_group_member.throttle_timers; + if (tgm->throttle_state) { bs = blk_bs(blk); bdrv_drained_begin(bs); - throttle_timers_detach_aio_context(tt); + throttle_group_detach_aio_context(tgm); + throttle_group_attach_aio_context(tgm, qemu_get_aio_context()); bdrv_drained_end(bs); } @@ -678,6 +678,7 @@ void blk_remove_bs(BlockBackend *blk) */ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp) { + ThrottleGroupMember *tgm = &blk->public.throttle_group_member; blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk->perm, blk->shared_perm, blk, errp); if (blk->root == NULL) { @@ -686,10 +687,9 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp) bdrv_ref(bs); notifier_list_notify(&blk->insert_bs_notifiers, blk); - if (blk->public.throttle_group_member.throttle_state) { - throttle_timers_attach_aio_context( - &blk->public.throttle_group_member.throttle_timers, - bdrv_get_aio_context(bs)); + if (tgm->throttle_state) { + throttle_group_detach_aio_context(tgm); + throttle_group_attach_aio_context(tgm, bdrv_get_aio_context(bs)); } return 0;
If a BlockBackend has I/O limits set then its ThrottleGroupMember structure uses the AioContext from its attached BlockDriverState. Those two contexts must be kept in sync manually. This is not ideal and will be fixed in the future by removing the throttling configuration from the BlockBackend and storing it in an implicit filter node instead, but for now we have to live with this. When you remove the BlockDriverState from the backend then the throttle timers are destroyed. If a new BlockDriverState is later inserted then they are created again using the new AioContext. There'a a couple of problems with this: a) The code manipulates the timers directly, leaving the ThrottleGroupMember.aio_context field in an inconsisent state. b) If you remove the I/O limits (e.g by destroying the backend) when the timers are gone then throttle_group_unregister_tgm() will attempt to destroy them again, crashing QEMU. While b) could be fixed easily by allowing the timers to be freed twice, this would result in a situation in which we can no longer guarantee that a valid ThrottleState has a valid AioContext and timers. This patch ensures that the timers and AioContext are always valid when I/O limits are set, regardless of whether the BlockBackend has a BlockDriverState inserted or not. Reported-by: sochin jiang <sochin.jiang@huawei.com> Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/block-backend.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)