Fix throttling crashes in BlockBackend with no BlockDriverState

PATCH Applied Alberto Garcia 3 participants 15 messages Nov 10, 2017 – Nov 13, 2017 Block layer

lore

Share & tools

lore

Alberto Garcia

author

2017-11-10 18:55 UTC #0 lore raw

cover Fix throttling crashes in BlockBackend with no BlockDriverState

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(-)

Signature · 1 line

2.11.0

Alberto Garcia

author

2017-11-10 18:55 UTC #1 lore raw

PATCH 1/3 block: Check for inserted BlockDriverState in blk_io_limits_disable()

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 <[email protected]>

Signed-off-by Alberto Garcia <[email protected]>

---
 block/block-backend.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

block/block-backend.c +10 -4

index 18e543780d..193a080096 100644
@@ -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 */

Signature · 1 line

2.11.0

Max Reitz

Reviewed-by on v1 1/3

2017-11-10 20:16 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/3] block: Check for inserted BlockDriverState in blk_io_limits_disable()

Quoted 8 lines · Alberto Garcia

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:

Quoted 8 lines

(qemu) drive_add 0 if=none,throttling.iops-total=5000 (qemu) drive_del none0
Reported-by sochin jiang <[email protected]>
Signed-off-by Alberto Garcia <[email protected]>
---
 block/block-backend.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Reviewed-by Max Reitz <[email protected]>

Alberto Garcia

author

2017-11-10 18:55 UTC #3 lore raw

PATCH 2/3 block: Leave valid throttle timers when removing a BDS from a backend

In reply to Alberto Garcia

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 <[email protected]>

Signed-off-by Alberto Garcia <[email protected]>

---
 block/block-backend.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

block/block-backend.c +8 -8

index 193a080096..38db6e639b 100644
@@ -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;

Signature · 1 line

2.11.0

Max Reitz

Reviewed-by on v1 2/3

2017-11-10 20:28 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend

Quoted 29 lines · Alberto Garcia

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.

Quoted 5 lines

Reported-by sochin jiang <[email protected]>
Signed-off-by Alberto Garcia <[email protected]>
---
 block/block-backend.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by Max Reitz <[email protected]>

Alberto Garcia

author

2017-11-10 22:07 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend

In reply to Alberto Garcia

On Fri 10 Nov 2017 07:54:47 PM CET, Alberto Garcia <[email protected]> wrote:

I just noticed a typo in the commit message:

Quoted 1 line

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

Max Reitz

2017-11-10 22:08 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend

Quoted 9 lines · Alberto Garcia

On Fri 10 Nov 2017 07:54:47 PM CET, Alberto Garcia <[email protected]> wrote:
I just noticed a typo in the commit message:
Quoted 1 line
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

Alberto Garcia

author

2017-11-10 22:32 UTC #7 lore raw

Re: [Qemu-devel] [PATCH 2/3] block: Leave valid throttle timers when removing a BDS from a backend

Quoted 10 lines · Max Reitz

Quoted 7 lines
I just noticed a typo in the commit message:
Quoted 1 line
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

Alberto Garcia

author

2017-11-10 18:55 UTC #8 lore raw

PATCH 3/3 qemu-iotests: Test I/O limits with removable media

In reply to Alberto Garcia

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 <[email protected]>

---
 tests/qemu-iotests/093     | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/093.out |  4 +--
 2 files changed, 64 insertions(+), 2 deletions(-)

tests/qemu-iotests/093 +62 -0

index ef3997206b..7862f2ba94 100755
@@ -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"])

tests/qemu-iotests/093.out +2 -2

index 2f7d3902f2..594c16f49f 100644
@@ -1,5 +1,5 @@
-.......
+........
 ----------------------------------------------------------------------
-Ran 7 tests
+Ran 8 tests
 
 OK

Signature · 1 line

2.11.0

Max Reitz

Reviewed-by on v1 3/3

2017-11-10 20:34 UTC #9 lore raw

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media

Quoted 9 lines · Alberto Garcia

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.

Quoted 5 lines

Signed-off-by Alberto Garcia <[email protected]>

---
 tests/qemu-iotests/093     | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/093.out |  4 +--
 2 files changed, 64 insertions(+), 2 deletions(-)

Reviewed-by Max Reitz <[email protected]>

Max Reitz

2017-11-10 22:21 UTC #10 lore raw

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media

In reply to Alberto Garcia

Quoted 9 lines · Alberto Garcia

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.

Quoted 5 lines

Signed-off-by Alberto Garcia <[email protected]>

---
 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

Alberto Garcia

author

2017-11-13 14:08 UTC #11 lore raw

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media

Quoted 4 lines · Max Reitz

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?

Quoted 1 line

So I guess this means we can drop the x- now. :-)

Sure.

Berto

Stefan Hajnoczi

2017-11-13 15:49 UTC #12 lore raw

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media

In reply to Alberto Garcia

tests/qemu-iotests/093 class ThrottleTestGroupNames(iotests.QMPTestCase): · line 326

Quoted 2 lines · Alberto Garcia

tests/qemu-iotests/093 +2 -0

+        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.

Alberto Garcia

author

2017-11-13 15:57 UTC #13 lore raw

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test I/O limits with removable media

Quoted 4 lines · Stefan Hajnoczi

tests/qemu-iotests/093 class ThrottleTestGroupNames(iotests.QMPTestCase): · line 326
Quoted 2 lines · Alberto Garcia
tests/qemu-iotests/093 +2 -0
+        result = self.vm.qmp("blockdev-add", driver = "null-aio",
+                             node_name = "cd0")

Quoted 8 lines

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

Stefan Hajnoczi

Applied

2017-11-13 15:50 UTC #14 lore raw

In reply to Alberto Garcia

Quoted 19 lines · Alberto Garcia

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

Quoted 8 lines

 block/block-backend.c      | 30 +++++++++++++---------
 tests/qemu-iotests/093     | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/093.out |  4 +--
 3 files changed, 82 insertions(+), 14 deletions(-)

Signature · 1 line

2.11.0

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan