Fix regression with the default naming of throttling groups
cover Fix regression with the default naming of throttling groups
Hi,
Stefan reported this, this is a regression caused by commit efaa7c4eeb7490c6f37f3.
QEMU v2.6.0 is affected but the patch cannot be backported as-is, I'll send a separate version to qemu-stable.
Berto
Alberto Garcia (2):
blockdev: Fix regression with the default naming of throttling groups
qemu-iotests: Test naming of throttling groups
blockdev.c | 7 +++- tests/qemu-iotests/093 | 98 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/093.out | 4 +- 3 files changed, 105 insertions(+), 4 deletions(-)
Signature · 1 line
2.8.1
PATCH 1/2 blockdev: Fix regression with the default naming of throttling groups
When I/O limits are set for a block device, the name of the throttling group is taken from the BlockBackend if the user doesn't specify one.
Commit efaa7c4eeb7490c6f37f3 moved the naming of the BlockBackend in blockdev_init() to the end of the function, after I/O limits are set. The consequence is that the throttling group gets an empty name.
--- blockdev.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
blockdev.c +5 -2
@@ -483,6 +483,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,const char *id;BlockdevDetectZeroesOptions detect_zeroes =BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF ; +const char *blk_id;const char *throttling_group =NULL ;/* Check common options by copying from bs_opts to opts, all other options
@@ -512,6 +513,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, writethrough = !qemu_opt_get_bool (opts,BDRV_OPT_CACHE_WB , true); + blk_id =qemu_opts_id (opts); +qdict_extract_subqdict (bs_opts, &interval_dict,"stats-intervals." );qdict_array_split (interval_dict, &interval_list);
@@ -616,7 +619,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,/* disk I/O throttling */ if (throttle_enabled (&cfg)) {if (!throttling_group) { - throttling_group =blk_name (blk); + throttling_group = blk_id; }blk_io_limits_enable (blk, throttling_group);blk_set_io_limits (blk, &cfg);
@@ -625,7 +628,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,blk_set_enable_write_cache (blk, !writethrough);blk_set_on_error (blk, on_read_error, on_write_error); -if (!monitor_add_blk (blk,qemu_opts_id (opts), errp)) { +if (!monitor_add_blk (blk, blk_id, errp)) {blk_unref (blk); blk =NULL ; goto err_no_bs_opts;
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 1/2] blockdev: Fix regression with the default naming of throttling groups
Quoted 28 lines · Alberto Garcia
When I/O limits are set for a block device, the name of the throttling group is taken from the BlockBackend if the user doesn't specify one.
Commit
efaa7c4eeb7490c6f37f3moved the naming of the BlockBackend inblockdev_init()to the end of the function, after I/O limits are set. The consequence is that the throttling group gets an empty name.Signed-off-by Alberto Garcia <[email protected]>Reported-by Stefan Hajnoczi <[email protected]>Cc Max Reitz <[email protected]>--- blockdev.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)blockdev.c +1
-0@@ -483,6 +483,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,const char *id;BlockdevDetectZeroesOptions detect_zeroes =BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF ; +const char *blk_id;const char *throttling_group =NULL ;/* Check common options by copying from bs_opts to opts, all other options
blockdev.c static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, · line 517
Quoted 5 lines
blockdev.c +2
-0@@ -512,6 +513,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, writethrough = !qemu_opt_get_bool (opts,BDRV_OPT_CACHE_WB , true); + blk_id =qemu_opts_id (opts); +
Side note: The "id" variable is supposed to contain the exact same value, but the string it points to is invalidated by the qdict_del(bs_opts, "id") call.
So indeed we need to obtain the ID anew here (or we'd have to do g_strdup() before and g_free() after, which is cumbersome). But regarding the variable itself, you could have actually reused "id" (I only noticed that just now).
But that's just a minor thing, so:
Quoted 22 lines
blockdev.c +2
-2qdict_extract_subqdict (bs_opts, &interval_dict,"stats-intervals." );qdict_array_split (interval_dict, &interval_list);@@ -616,7 +619,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,/* disk I/O throttling */ if (throttle_enabled (&cfg)) {if (!throttling_group) { - throttling_group =blk_name (blk); + throttling_group = blk_id; }blk_io_limits_enable (blk, throttling_group);blk_set_io_limits (blk, &cfg);@@ -625,7 +628,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,blk_set_enable_write_cache (blk, !writethrough);blk_set_on_error (blk, on_read_error, on_write_error); -if (!monitor_add_blk (blk,qemu_opts_id (opts), errp)) { +if (!monitor_add_blk (blk, blk_id, errp)) {blk_unref (blk); blk =NULL ; goto err_no_bs_opts;
Re: [Qemu-devel] [PATCH 1/2] blockdev: Fix regression with the default naming of throttling groups
Quoted 7 lines · Max Reitz
blockdev.c static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, · line 517
Quoted 2 lines
blockdev.c +2
-0+ blk_id =qemu_opts_id (opts); +Side note: The "id" variable is supposed to contain the exact same value, but the string it points to is invalidated by the
qdict_del(bs_opts, "id")call.
Quoted 8 lines
So indeed we need to obtain the ID anew here (or we'd have to do
g_strdup()before andg_free()after, which is cumbersome). But regarding the variable itself, you could have actually reused "id" (I only noticed that just now).But that's just a minor thing, so:
Reviewed-by Max Reitz <[email protected]>
Ok, if this series doesn't get a new version feel free to modify the patch when you apply it.
Berto
PATCH 2/2 qemu-iotests: Test naming of throttling groups
In reply to Alberto GarciaThrottling groups are named using the 'group' parameter of the block_set_io_throttle command and the throttling.group command-line option. If that parameter is unspecified the groups get the name of the block device.
This patch adds a new test to check the naming of throttling groups.
--- tests/qemu-iotests/093 | 98 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/093.out | 4 +- 2 files changed, 100 insertions(+), 2 deletions(-)
tests/qemu-iotests/093 +98 -0
@@ -184,5 +184,103 @@ class ThrottleTestCase(iotests.QMPTestCase): class ThrottleTestCoroutine(ThrottleTestCase): test_img = "null-co://" +class ThrottleTestGroupNames(iotests.QMPTestCase): + test_img = "null-aio://" + max_drives = 3 + + def setUp(self): + self.vm = iotests.VM() + for i in range(0, self.max_drives): + self.vm.add_drive(self.test_img, "throttling.iops-total=100") + self.vm.launch() + + def tearDown(self): + self.vm.shutdown() + + def set_io_throttle(self, device, params): + params["device"] = device + result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **params) + self.assert_qmp(result, 'return', {}) + + def verify_name(self, device, name): + result = self.vm.qmp("query-block") + for r in result["return"]: + if r["device"] == device: + info = r["inserted"] + if name: + self.assertEqual(info["group"], name) + else: + self.assertFalse(info.has_key('group')) + return + + raise Exception("No group information found for '%s'" % device) + + def test_group_naming(self): + params = {"bps": 0, + "bps_rd": 0, + "bps_wr": 0, + "iops": 0, + "iops_rd": 0, + "iops_wr": 0} + + # Check the drives added using the command line. + # The default throttling group name is the device name. + for i in range(self.max_drives): + devname = "drive%d" % i + self.verify_name(devname, devname) + + # Clear throttling settings => the group name is gone. + for i in range(self.max_drives): + devname = "drive%d" % i + self.set_io_throttle(devname, params) + self.verify_name(devname, None) + + # Set throttling settings using block_set_io_throttle and + # check the default group names. + params["iops"] = 10 + for i in range(self.max_drives): + devname = "drive%d" % i + self.set_io_throttle(devname, params) + self.verify_name(devname, devname) + + # Set a custom group name for each device + for i in range(3): + devname = "drive%d" % i + groupname = "group%d" % i + params['group'] = groupname + self.set_io_throttle(devname, params) + self.verify_name(devname, groupname) + + # Put drive0 in group1 and check that all other devices remain + # unchanged + params['group'] = 'group1' + self.set_io_throttle('drive0', params) + self.verify_name('drive0', 'group1') + for i in range(1, self.max_drives): + devname = "drive%d" % i + groupname = "group%d" % i + self.verify_name(devname, groupname) + + # Put drive0 in group2 and check that all other devices remain + # unchanged + params['group'] = 'group2' + self.set_io_throttle('drive0', params) + self.verify_name('drive0', 'group2') + for i in range(1, self.max_drives): + devname = "drive%d" % i + groupname = "group%d" % i + self.verify_name(devname, groupname) + + # Clear throttling settings from drive0 check that all other + # devices remain unchanged + params["iops"] = 0 + self.set_io_throttle('drive0', params) + self.verify_name('drive0', None) + for i in range(1, self.max_drives): + devname = "drive%d" % i + groupname = "group%d" % i + self.verify_name(devname, groupname) + + if __name__ == '__main__': iotests.main(supported_fmts=["raw"])
tests/qemu-iotests/093.out +2 -2
@@ -1,5 +1,5 @@ -.... +..... ---------------------------------------------------------------------- -Ran 4 tests +Ran 5 tests OK
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 2/2] qemu-iotests: Test naming of throttling groups
Quoted 5 lines · Alberto Garcia
Throttling groups are named using the 'group' parameter of the
block_set_io_throttlecommand and the throttling.group command-line option. If that parameter is unspecified the groups get the name of the block device.
Quoted 7 lines
This patch adds a new test to check the naming of throttling groups.
Signed-off-by Alberto Garcia <[email protected]>--- tests/qemu-iotests/093 | 98 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/093.out | 4 +- 2 files changed, 100 insertions(+), 2 deletions(-)
Quoted 10 lines · Alberto Garcia
Hi,
Stefan reported this, this is a regression caused by commit
efaa7c4eeb7490c6f37f3.QEMU v2.6.0 is affected but the patch cannot be backported as-is, I'll send a separate version to qemu-stable.
Berto
Quoted 8 lines
Alberto Garcia (2):
blockdev: Fix regression with the default naming of throttling groups
qemu-iotests: Test naming of throttling groupsblockdev.c | 7 +++- tests/qemu-iotests/093 | 98 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/093.out | 4 +- 3 files changed, 105 insertions(+), 4 deletions(-)
Thanks Berto, I've changed patch 1 to use "id" instead of "blk_id" (and you even asked me which variable to use beforehand O:-)) and applied the series to my block branch:
https://github.com/XanClic/qemu/commits/block
Max
Quoted 10 lines · Alberto Garcia
Hi,
Stefan reported this, this is a regression caused by commit
efaa7c4eeb7490c6f37f3.QEMU v2.6.0 is affected but the patch cannot be backported as-is, I'll send a separate version to qemu-stable.
Berto
Quoted 12 lines
Alberto Garcia (2):
blockdev: Fix regression with the default naming of throttling groups
qemu-iotests: Test naming of throttling groupsblockdev.c | 7 +++- tests/qemu-iotests/093 | 98 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/093.out | 4 +- 3 files changed, 105 insertions(+), 4 deletions(-)Signature · 1 line
2.8.1