[Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState

[Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState

[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

 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

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

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

     (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

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

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

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

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

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

[Max Reitz]

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

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

[Max Reitz]

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

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

[Max Reitz]

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

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

[Alberto Garcia]

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

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

[Max Reitz]

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

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

[Max Reitz]

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

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

[Alberto Garcia]

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

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

[Alberto Garcia]

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

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

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH 0/3] Fix throttling crashes in BlockBackend with no BlockDriverState

[Stefan Hajnoczi]

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

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

[Alberto Garcia]

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