* [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState @ 2017-11-10 18:54 Alberto Garcia 2017-11-10 18:54 ` [Qemu-devel] [PATCH 1/3] block: Check for inserted BlockDriverState in blk_io_limits_disable() Alberto Garcia ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Alberto Garcia @ 2017-11-10 18:54 UTC (permalink / raw) To: qemu-devel Cc: Alberto Garcia, qemu-block, Stefan Hajnoczi, Kevin Wolf, Max Reitz, sochin jiang Hi, this series fixes the problems reported by Sochin Jiang in BlockBackend when there's a valid throttling configuration but the BDS has been removed. The patches apply on top of Li Zhengui's "all I/O should be completed before removing throttle timers" and I tested this on top of Stefan's block branch (commit 900276cf24589596296d3d099fe609ad5c182ac9). Regards, Berto Alberto Garcia (3): block: Check for inserted BlockDriverState in blk_io_limits_disable() block: Leave valid throttle timers when removing a BDS from a backend qemu-iotests: Test I/O limits with removable media block/block-backend.c | 30 +++++++++++++--------- tests/qemu-iotests/093 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/093.out | 4 +-- 3 files changed, 82 insertions(+), 14 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 1/3] block: Check for inserted BlockDriverState in blk_io_limits_disable() 2017-11-10 18:54 [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState Alberto Garcia @ 2017-11-10 18:54 ` Alberto Garcia 2017-11-10 20:16 ` Max Reitz 2017-11-10 18:54 ` [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend Alberto Garcia ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ messages in thread From: Alberto Garcia @ 2017-11-10 18:54 UTC (permalink / raw) To: qemu-devel Cc: Alberto Garcia, qemu-block, Stefan Hajnoczi, Kevin Wolf, Max Reitz, sochin jiang When you set I/O limits using block_set_io_throttle or the command line throttling.* options they are kept in the BlockBackend regardless of whether a BlockDriverState is attached to the backend or not. Therefore when removing the limits using blk_io_limits_disable() we need to check if there's a BDS before attempting to drain it, else it will crash QEMU. This can be reproduced very easily using HMP: (qemu) drive_add 0 if=none,throttling.iops-total=5000 (qemu) drive_del none0 Reported-by: sochin jiang <sochin.jiang@huawei.com> Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/block-backend.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 18e543780d..193a080096 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1978,10 +1978,16 @@ void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg) void blk_io_limits_disable(BlockBackend *blk) { - assert(blk->public.throttle_group_member.throttle_state); - bdrv_drained_begin(blk_bs(blk)); - throttle_group_unregister_tgm(&blk->public.throttle_group_member); - bdrv_drained_end(blk_bs(blk)); + BlockDriverState *bs = blk_bs(blk); + ThrottleGroupMember *tgm = &blk->public.throttle_group_member; + assert(tgm->throttle_state); + if (bs) { + bdrv_drained_begin(bs); + } + throttle_group_unregister_tgm(tgm); + if (bs) { + bdrv_drained_end(bs); + } } /* should be called before blk_set_io_limits if a limit is set */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] block: Check for inserted BlockDriverState in blk_io_limits_disable() 2017-11-10 18:54 ` [Qemu-devel] [PATCH 1/3] block: Check for inserted BlockDriverState in blk_io_limits_disable() Alberto Garcia @ 2017-11-10 20:16 ` Max Reitz 0 siblings, 0 replies; 15+ messages in thread From: Max Reitz @ 2017-11-10 20:16 UTC (permalink / raw) To: Alberto Garcia, qemu-devel Cc: qemu-block, Stefan Hajnoczi, Kevin Wolf, sochin jiang [-- Attachment #1: Type: text/plain, Size: 832 bytes --] On 2017-11-10 19:54, Alberto Garcia wrote: > When you set I/O limits using block_set_io_throttle or the command > line throttling.* options they are kept in the BlockBackend regardless > of whether a BlockDriverState is attached to the backend or not. > > Therefore when removing the limits using blk_io_limits_disable() we > need to check if there's a BDS before attempting to drain it, else it > will crash QEMU. This can be reproduced very easily using HMP: > > (qemu) drive_add 0 if=none,throttling.iops-total=5000 > (qemu) drive_del none0 > > Reported-by: sochin jiang <sochin.jiang@huawei.com> > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/block-backend.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 512 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend 2017-11-10 18:54 [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState Alberto Garcia 2017-11-10 18:54 ` [Qemu-devel] [PATCH 1/3] block: Check for inserted BlockDriverState in blk_io_limits_disable() Alberto Garcia @ 2017-11-10 18:54 ` Alberto Garcia 2017-11-10 20:27 ` Max Reitz 2017-11-10 22:06 ` Alberto Garcia 2017-11-10 18:54 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media Alberto Garcia 2017-11-13 15:49 ` [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState Stefan Hajnoczi 3 siblings, 2 replies; 15+ messages in thread From: Alberto Garcia @ 2017-11-10 18:54 UTC (permalink / raw) To: qemu-devel Cc: Alberto Garcia, qemu-block, Stefan Hajnoczi, Kevin Wolf, Max Reitz, sochin jiang 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(-) 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; -- 2.11.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend 2017-11-10 18:54 ` [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend Alberto Garcia @ 2017-11-10 20:27 ` Max Reitz 2017-11-10 22:06 ` Alberto Garcia 1 sibling, 0 replies; 15+ messages in thread From: Max Reitz @ 2017-11-10 20:27 UTC (permalink / raw) To: Alberto Garcia, qemu-devel Cc: qemu-block, Stefan Hajnoczi, Kevin Wolf, sochin jiang [-- Attachment #1: Type: text/plain, Size: 1713 bytes --] 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> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 512 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend 2017-11-10 18:54 ` [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend Alberto Garcia 2017-11-10 20:27 ` Max Reitz @ 2017-11-10 22:06 ` Alberto Garcia 2017-11-10 22:08 ` Max Reitz 1 sibling, 1 reply; 15+ messages in thread From: Alberto Garcia @ 2017-11-10 22:06 UTC (permalink / raw) To: qemu-devel Cc: qemu-block, Stefan Hajnoczi, Kevin Wolf, Max Reitz, sochin jiang 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend 2017-11-10 22:06 ` Alberto Garcia @ 2017-11-10 22:08 ` Max Reitz 2017-11-10 22:32 ` Alberto Garcia 0 siblings, 1 reply; 15+ messages in thread From: Max Reitz @ 2017-11-10 22:08 UTC (permalink / raw) To: Alberto Garcia, qemu-devel Cc: qemu-block, Stefan Hajnoczi, Kevin Wolf, sochin jiang [-- Attachment #1: Type: text/plain, Size: 500 bytes --] 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 512 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend 2017-11-10 22:08 ` Max Reitz @ 2017-11-10 22:32 ` Alberto Garcia 0 siblings, 0 replies; 15+ messages in thread From: Alberto Garcia @ 2017-11-10 22:32 UTC (permalink / raw) To: Max Reitz, qemu-devel Cc: qemu-block, Stefan Hajnoczi, Kevin Wolf, sochin jiang 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media 2017-11-10 18:54 [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState Alberto Garcia 2017-11-10 18:54 ` [Qemu-devel] [PATCH 1/3] block: Check for inserted BlockDriverState in blk_io_limits_disable() Alberto Garcia 2017-11-10 18:54 ` [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend Alberto Garcia @ 2017-11-10 18:54 ` Alberto Garcia 2017-11-10 20:34 ` Max Reitz ` (2 more replies) 2017-11-13 15:49 ` [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState Stefan Hajnoczi 3 siblings, 3 replies; 15+ messages in thread From: Alberto Garcia @ 2017-11-10 18:54 UTC (permalink / raw) To: qemu-devel Cc: Alberto Garcia, qemu-block, Stefan Hajnoczi, Kevin Wolf, Max Reitz, sochin jiang This test hotplugs a CD drive to a VM and checks that I/O limits can be set only when the drive has media inserted and that they are kept when the media is replaced. This also tests the removal of a device with valid I/O limits set but no media inserted. This involves deleting and disabling the limits of a BlockBackend without BlockDriverState, a scenario that has been crashing until the fixes from the last couple of patches. Signed-off-by: Alberto Garcia <berto@igalia.com> --- tests/qemu-iotests/093 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/093.out | 4 +-- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093 index ef3997206b..7862f2ba94 100755 --- a/tests/qemu-iotests/093 +++ b/tests/qemu-iotests/093 @@ -308,6 +308,68 @@ class ThrottleTestGroupNames(iotests.QMPTestCase): groupname = "group%d" % i self.verify_name(devname, groupname) +class ThrottleTestRemovableMedia(iotests.QMPTestCase): + def setUp(self): + self.vm = iotests.VM() + if iotests.qemu_default_machine == 's390-ccw-virtio': + self.vm.add_device("virtio-scsi-ccw,id=virtio-scsi") + else: + self.vm.add_device("virtio-scsi-pci,id=virtio-scsi") + self.vm.launch() + + def tearDown(self): + self.vm.shutdown() + + def test_removable_media(self): + # Add a couple of dummy nodes named cd0 and cd1 + result = self.vm.qmp("blockdev-add", driver = "null-aio", + node_name = "cd0") + self.assert_qmp(result, 'return', {}) + result = self.vm.qmp("blockdev-add", driver = "null-aio", + node_name = "cd1") + self.assert_qmp(result, 'return', {}) + + # Attach a CD drive with cd0 inserted + result = self.vm.qmp("device_add", driver = "scsi-cd", + id = "dev0", drive = "cd0") + self.assert_qmp(result, 'return', {}) + + # Set I/O limits + args = { "id": "dev0", "iops": 100, "iops_rd": 0, "iops_wr": 0, + "bps": 50, "bps_rd": 0, "bps_wr": 0 } + result = self.vm.qmp("block_set_io_throttle", conv_keys = False, **args) + self.assert_qmp(result, 'return', {}) + + # Check that the I/O limits have been set + result = self.vm.qmp("query-block") + self.assert_qmp(result, 'return[0]/inserted/iops', 100) + self.assert_qmp(result, 'return[0]/inserted/bps', 50) + + # Now eject cd0 and insert cd1 + result = self.vm.qmp("blockdev-open-tray", id = 'dev0') + self.assert_qmp(result, 'return', {}) + result = self.vm.qmp("x-blockdev-remove-medium", id = 'dev0') + self.assert_qmp(result, 'return', {}) + result = self.vm.qmp("x-blockdev-insert-medium", id = 'dev0', node_name = 'cd1') + self.assert_qmp(result, 'return', {}) + + # Check that the I/O limits are still the same + result = self.vm.qmp("query-block") + self.assert_qmp(result, 'return[0]/inserted/iops', 100) + self.assert_qmp(result, 'return[0]/inserted/bps', 50) + + # Eject cd1 + result = self.vm.qmp("x-blockdev-remove-medium", id = 'dev0') + self.assert_qmp(result, 'return', {}) + + # Check that we can't set limits if the device has no medium + result = self.vm.qmp("block_set_io_throttle", conv_keys = False, **args) + self.assert_qmp(result, 'error/class', 'GenericError') + + # Remove the CD drive + result = self.vm.qmp("device_del", id = 'dev0') + self.assert_qmp(result, 'return', {}) + if __name__ == '__main__': iotests.main(supported_fmts=["raw"]) diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out index 2f7d3902f2..594c16f49f 100644 --- a/tests/qemu-iotests/093.out +++ b/tests/qemu-iotests/093.out @@ -1,5 +1,5 @@ -....... +........ ---------------------------------------------------------------------- -Ran 7 tests +Ran 8 tests OK -- 2.11.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media 2017-11-10 18:54 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media Alberto Garcia @ 2017-11-10 20:34 ` Max Reitz 2017-11-10 22:21 ` Max Reitz 2017-11-13 15:49 ` Stefan Hajnoczi 2 siblings, 0 replies; 15+ messages in thread From: Max Reitz @ 2017-11-10 20:34 UTC (permalink / raw) To: Alberto Garcia, qemu-devel Cc: qemu-block, Stefan Hajnoczi, Kevin Wolf, sochin jiang [-- Attachment #1: Type: text/plain, Size: 787 bytes --] On 2017-11-10 19:54, Alberto Garcia wrote: > This test hotplugs a CD drive to a VM and checks that I/O limits can > be set only when the drive has media inserted and that they are kept > when the media is replaced. > > This also tests the removal of a device with valid I/O limits set but > no media inserted. This involves deleting and disabling the limits > of a BlockBackend without BlockDriverState, a scenario that has been > crashing until the fixes from the last couple of patches. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > tests/qemu-iotests/093 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/093.out | 4 +-- > 2 files changed, 64 insertions(+), 2 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 512 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media 2017-11-10 18:54 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media Alberto Garcia 2017-11-10 20:34 ` Max Reitz @ 2017-11-10 22:21 ` Max Reitz 2017-11-13 14:08 ` Alberto Garcia 2017-11-13 15:49 ` Stefan Hajnoczi 2 siblings, 1 reply; 15+ messages in thread From: Max Reitz @ 2017-11-10 22:21 UTC (permalink / raw) To: Alberto Garcia, qemu-devel Cc: qemu-block, Stefan Hajnoczi, Kevin Wolf, sochin jiang [-- Attachment #1: Type: text/plain, Size: 1041 bytes --] On 2017-11-10 19:54, Alberto Garcia wrote: > This test hotplugs a CD drive to a VM and checks that I/O limits can > be set only when the drive has media inserted and that they are kept > when the media is replaced. > > This also tests the removal of a device with valid I/O limits set but > no media inserted. This involves deleting and disabling the limits > of a BlockBackend without BlockDriverState, a scenario that has been > crashing until the fixes from the last couple of patches. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > tests/qemu-iotests/093 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/093.out | 4 +-- > 2 files changed, 64 insertions(+), 2 deletions(-) By the way, I just noticed that this test tests that x-blockdev-remove-medium and x-blockdev-insert-medium do not destroy throttling information -- which is exactly why those commands had been declared experimental in the first place. So I guess this means we can drop the x- now. :-) Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 512 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media 2017-11-10 22:21 ` Max Reitz @ 2017-11-13 14:08 ` Alberto Garcia 0 siblings, 0 replies; 15+ messages in thread From: Alberto Garcia @ 2017-11-13 14:08 UTC (permalink / raw) To: Max Reitz, qemu-devel Cc: qemu-block, Stefan Hajnoczi, Kevin Wolf, sochin jiang On Fri 10 Nov 2017 11:21:27 PM CET, Max Reitz wrote: > By the way, I just noticed that this test tests that > x-blockdev-remove-medium and x-blockdev-insert-medium do not destroy > throttling information -- which is exactly why those commands had been > declared experimental in the first place. Oh, was that the reason? > So I guess this means we can drop the x- now. :-) Sure. Berto ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media 2017-11-10 18:54 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media Alberto Garcia 2017-11-10 20:34 ` Max Reitz 2017-11-10 22:21 ` Max Reitz @ 2017-11-13 15:49 ` Stefan Hajnoczi 2017-11-13 15:57 ` Alberto Garcia 2 siblings, 1 reply; 15+ messages in thread From: Stefan Hajnoczi @ 2017-11-13 15:49 UTC (permalink / raw) To: Alberto Garcia Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, sochin jiang [-- Attachment #1: Type: text/plain, Size: 428 bytes --] On Fri, Nov 10, 2017 at 08:54:48PM +0200, Alberto Garcia wrote: > + result = self.vm.qmp("blockdev-add", driver = "null-aio", > + node_name = "cd0") PEP8 says: "Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value." https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements I fixed this while merging the patch. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media 2017-11-13 15:49 ` Stefan Hajnoczi @ 2017-11-13 15:57 ` Alberto Garcia 0 siblings, 0 replies; 15+ messages in thread From: Alberto Garcia @ 2017-11-13 15:57 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, sochin jiang On Mon 13 Nov 2017 04:49:48 PM CET, Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Fri, Nov 10, 2017 at 08:54:48PM +0200, Alberto Garcia wrote: >> + result = self.vm.qmp("blockdev-add", driver = "null-aio", >> + node_name = "cd0") > > PEP8 says: > > "Don't use spaces around the = sign when used to indicate a keyword > argument or a default parameter value." > > https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements > > I fixed this while merging the patch. Ok, thanks! Berto ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState 2017-11-10 18:54 [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState Alberto Garcia ` (2 preceding siblings ...) 2017-11-10 18:54 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media Alberto Garcia @ 2017-11-13 15:49 ` Stefan Hajnoczi 3 siblings, 0 replies; 15+ messages in thread From: Stefan Hajnoczi @ 2017-11-13 15:49 UTC (permalink / raw) To: Alberto Garcia Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, sochin jiang [-- Attachment #1: Type: text/plain, Size: 1073 bytes --] On Fri, Nov 10, 2017 at 08:54:45PM +0200, Alberto Garcia wrote: > Hi, > > this series fixes the problems reported by Sochin Jiang in > BlockBackend when there's a valid throttling configuration but the BDS > has been removed. > > The patches apply on top of Li Zhengui's "all I/O should be completed > before removing throttle timers" and I tested this on top of Stefan's > block branch (commit 900276cf24589596296d3d099fe609ad5c182ac9). > > Regards, > > Berto > > Alberto Garcia (3): > block: Check for inserted BlockDriverState in blk_io_limits_disable() > block: Leave valid throttle timers when removing a BDS from a backend > qemu-iotests: Test I/O limits with removable media > > block/block-backend.c | 30 +++++++++++++--------- > tests/qemu-iotests/093 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/093.out | 4 +-- > 3 files changed, 82 insertions(+), 14 deletions(-) > > -- > 2.11.0 > Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-11-13 15:57 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-10 18:54 [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState Alberto Garcia 2017-11-10 18:54 ` [Qemu-devel] [PATCH 1/3] block: Check for inserted BlockDriverState in blk_io_limits_disable() Alberto Garcia 2017-11-10 20:16 ` Max Reitz 2017-11-10 18:54 ` [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend Alberto Garcia 2017-11-10 20:27 ` Max Reitz 2017-11-10 22:06 ` Alberto Garcia 2017-11-10 22:08 ` Max Reitz 2017-11-10 22:32 ` Alberto Garcia 2017-11-10 18:54 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media Alberto Garcia 2017-11-10 20:34 ` Max Reitz 2017-11-10 22:21 ` Max Reitz 2017-11-13 14:08 ` Alberto Garcia 2017-11-13 15:49 ` Stefan Hajnoczi 2017-11-13 15:57 ` Alberto Garcia 2017-11-13 15:49 ` [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState Stefan Hajnoczi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.