From ef7a6a3c2a7725b169d054aa7487f9738bd6c4a6 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 2 Aug 2018 17:50:23 +0300 Subject: qemu-iotests: Test removing a throttle group member with a pending timer A throttle group can have several members, and each one of them can have several pending requests in the queue. The requests are processed in a round-robin fashion, so the algorithm decides the drive that is going to run the next request and sets a timer in it. Once the timer fires and the throttled request is run then the next drive from the group is selected and a new timer is set. If the user tried to remove a drive from a group and that drive had a timer set then the code was not taking care of setting up a new timer in one of the remaining members of the group, freezing their I/O. This problem was fixed in 6fccbb475bc6effc313ee9481726a1748b6dae57, and this patch adds a new test case that reproduces this exact scenario. Signed-off-by: Alberto Garcia Signed-off-by: Kevin Wolf --- tests/qemu-iotests/093 | 52 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/093.out | 4 ++-- 2 files changed, 54 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093 index 68e344f8c1..b26cd34e32 100755 --- a/tests/qemu-iotests/093 +++ b/tests/qemu-iotests/093 @@ -208,6 +208,58 @@ class ThrottleTestCase(iotests.QMPTestCase): limits[tk] = rate self.do_test_throttle(ndrives, 5, limits) + # Test that removing a drive from a throttle group should not + # affect the remaining members of the group. + # https://bugzilla.redhat.com/show_bug.cgi?id=1535914 + def test_remove_group_member(self): + # Create a throttle group with two drives + # and set a 4 KB/s read limit. + params = {"bps": 0, + "bps_rd": 4096, + "bps_wr": 0, + "iops": 0, + "iops_rd": 0, + "iops_wr": 0 } + self.configure_throttle(2, params) + + # Read 4KB from drive0. This is performed immediately. + self.vm.hmp_qemu_io("drive0", "aio_read 0 4096") + + # Read 4KB again. The I/O limit has been exceeded so this + # request is throttled and a timer is set to wake it up. + self.vm.hmp_qemu_io("drive0", "aio_read 0 4096") + + # Read from drive1. We're still over the I/O limit so this + # request is also throttled. There's no timer set in drive1 + # because there's already one in drive0. Once the timer in + # drive0 fires and its throttled request is processed then the + # next request in the queue will be scheduled: this one. + self.vm.hmp_qemu_io("drive1", "aio_read 0 4096") + + # At this point only the first 4KB have been read from drive0. + # The other requests are throttled. + self.assertEqual(self.blockstats('drive0')[0], 4096) + self.assertEqual(self.blockstats('drive1')[0], 0) + + # Remove drive0 from the throttle group and disable its I/O limits. + # drive1 remains in the group with a throttled request. + params['bps_rd'] = 0 + params['device'] = 'drive0' + result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **params) + self.assert_qmp(result, 'return', {}) + + # Removing the I/O limits from drive0 drains its pending request. + # The read request in drive1 is still throttled. + self.assertEqual(self.blockstats('drive0')[0], 8192) + self.assertEqual(self.blockstats('drive1')[0], 0) + + # Advance the clock 5 seconds. This completes the request in drive1 + self.vm.qtest("clock_step %d" % (5 * nsec_per_sec)) + + # Now all requests have been processed. + self.assertEqual(self.blockstats('drive0')[0], 8192) + self.assertEqual(self.blockstats('drive1')[0], 4096) + class ThrottleTestCoroutine(ThrottleTestCase): test_img = "null-co://" diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out index 594c16f49f..36376bed87 100644 --- a/tests/qemu-iotests/093.out +++ b/tests/qemu-iotests/093.out @@ -1,5 +1,5 @@ -........ +.......... ---------------------------------------------------------------------- -Ran 8 tests +Ran 10 tests OK -- cgit v1.2.3 From 3db3e9c621519cbfc2b52e98b38f13ad863c0062 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 2 Aug 2018 17:50:25 +0300 Subject: qemu-iotests: Update 093 to improve the draining test The previous patch fixes a problem in which draining a block device with more than one throttled request can make it wait first for the completion of requests in other members of the same group. This patch updates test_remove_group_member() in iotest 093 to reproduce that scenario. This updated test would hang QEMU without the fix from the previous patch. Signed-off-by: Alberto Garcia Signed-off-by: Kevin Wolf --- tests/qemu-iotests/093 | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) (limited to 'tests') diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093 index b26cd34e32..9d1971a56c 100755 --- a/tests/qemu-iotests/093 +++ b/tests/qemu-iotests/093 @@ -225,15 +225,18 @@ class ThrottleTestCase(iotests.QMPTestCase): # Read 4KB from drive0. This is performed immediately. self.vm.hmp_qemu_io("drive0", "aio_read 0 4096") - # Read 4KB again. The I/O limit has been exceeded so this + # Read 2KB. The I/O limit has been exceeded so this # request is throttled and a timer is set to wake it up. - self.vm.hmp_qemu_io("drive0", "aio_read 0 4096") + self.vm.hmp_qemu_io("drive0", "aio_read 0 2048") + + # Read 2KB again. We're still over the I/O limit so this is + # request is also throttled, but no new timer is set since + # there's already one. + self.vm.hmp_qemu_io("drive0", "aio_read 0 2048") - # Read from drive1. We're still over the I/O limit so this - # request is also throttled. There's no timer set in drive1 - # because there's already one in drive0. Once the timer in - # drive0 fires and its throttled request is processed then the - # next request in the queue will be scheduled: this one. + # Read from drive1. This request is also throttled, and no + # timer is set in drive1 because there's already one in + # drive0. self.vm.hmp_qemu_io("drive1", "aio_read 0 4096") # At this point only the first 4KB have been read from drive0. @@ -248,7 +251,7 @@ class ThrottleTestCase(iotests.QMPTestCase): result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **params) self.assert_qmp(result, 'return', {}) - # Removing the I/O limits from drive0 drains its pending request. + # Removing the I/O limits from drive0 drains its two pending requests. # The read request in drive1 is still throttled. self.assertEqual(self.blockstats('drive0')[0], 8192) self.assertEqual(self.blockstats('drive1')[0], 0) -- cgit v1.2.3 From b24ec3c46281286a6a5624d0ceb5fa6f30bd8a4f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 13 Jun 2018 11:01:30 +0200 Subject: block: Remove deprecated -drive geometry options This reinstates commit a7aff6dd10b16b67e8b142d0c94c5d92c3fe88f6, which was temporarily reverted for the 3.0 release so that libvirt gets some extra time to update their command lines. The -drive options cyls, heads, secs and trans were deprecated in QEMU 2.10. It's time to remove them. hd-geo-test tested both the old version with geometry options in -drive and the new one with -device. Therefore the code using -drive doesn't have to be replaced there, we just need to remove the -drive test cases. This in turn allows some simplification of the code. Signed-off-by: Kevin Wolf Reviewed-by: Markus Armbruster --- tests/hd-geo-test.c | 37 +++++++------------------------------ 1 file changed, 7 insertions(+), 30 deletions(-) (limited to 'tests') diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c index 24870b38f4..ce665f1f83 100644 --- a/tests/hd-geo-test.c +++ b/tests/hd-geo-test.c @@ -201,7 +201,7 @@ static void setup_mbr(int img_idx, MBRcontents mbr) static int setup_ide(int argc, char *argv[], int argv_sz, int ide_idx, const char *dev, int img_idx, - MBRcontents mbr, const char *opts) + MBRcontents mbr) { char *s1, *s2, *s3; @@ -216,7 +216,7 @@ static int setup_ide(int argc, char *argv[], int argv_sz, s3 = g_strdup(",media=cdrom"); } argc = append_arg(argc, argv, argv_sz, - g_strdup_printf("%s%s%s%s", s1, s2, s3, opts)); + g_strdup_printf("%s%s%s", s1, s2, s3)); g_free(s1); g_free(s2); g_free(s3); @@ -260,7 +260,7 @@ static void test_ide_mbr(bool use_device, MBRcontents mbr) for (i = 0; i < backend_last; i++) { cur_ide[i] = &hd_chst[i][mbr]; dev = use_device ? (is_hd(cur_ide[i]) ? "ide-hd" : "ide-cd") : NULL; - argc = setup_ide(argc, argv, ARGV_SIZE, i, dev, i, mbr, ""); + argc = setup_ide(argc, argv, ARGV_SIZE, i, dev, i, mbr); } args = g_strjoinv(" ", argv); qtest_start(args); @@ -327,16 +327,12 @@ static void test_ide_drive_user(const char *dev, bool trans) const CHST expected_chst = { secs / (4 * 32) , 4, 32, trans }; argc = setup_common(argv, ARGV_SIZE); - opts = g_strdup_printf("%s,%s%scyls=%d,heads=%d,secs=%d", - dev ?: "", - trans && dev ? "bios-chs-" : "", - trans ? "trans=lba," : "", + opts = g_strdup_printf("%s,%scyls=%d,heads=%d,secs=%d", + dev, trans ? "bios-chs-trans=lba," : "", expected_chst.cyls, expected_chst.heads, expected_chst.secs); cur_ide[0] = &expected_chst; - argc = setup_ide(argc, argv, ARGV_SIZE, - 0, dev ? opts : NULL, backend_small, mbr_chs, - dev ? "" : opts); + argc = setup_ide(argc, argv, ARGV_SIZE, 0, opts, backend_small, mbr_chs); g_free(opts); args = g_strjoinv(" ", argv); qtest_start(args); @@ -346,22 +342,6 @@ static void test_ide_drive_user(const char *dev, bool trans) qtest_end(); } -/* - * Test case: IDE device (if=ide) with explicit CHS - */ -static void test_ide_drive_user_chs(void) -{ - test_ide_drive_user(NULL, false); -} - -/* - * Test case: IDE device (if=ide) with explicit CHS and translation - */ -static void test_ide_drive_user_chst(void) -{ - test_ide_drive_user(NULL, true); -} - /* * Test case: IDE device (if=none) with explicit CHS */ @@ -392,8 +372,7 @@ static void test_ide_drive_cd_0(void) for (i = 0; i <= backend_empty; i++) { ide_idx = backend_empty - i; cur_ide[ide_idx] = &hd_chst[i][mbr_blank]; - argc = setup_ide(argc, argv, ARGV_SIZE, - ide_idx, NULL, i, mbr_blank, ""); + argc = setup_ide(argc, argv, ARGV_SIZE, ide_idx, NULL, i, mbr_blank); } args = g_strjoinv(" ", argv); qtest_start(args); @@ -422,8 +401,6 @@ int main(int argc, char **argv) qtest_add_func("hd-geo/ide/drive/mbr/blank", test_ide_drive_mbr_blank); qtest_add_func("hd-geo/ide/drive/mbr/lba", test_ide_drive_mbr_lba); qtest_add_func("hd-geo/ide/drive/mbr/chs", test_ide_drive_mbr_chs); - qtest_add_func("hd-geo/ide/drive/user/chs", test_ide_drive_user_chs); - qtest_add_func("hd-geo/ide/drive/user/chst", test_ide_drive_user_chst); qtest_add_func("hd-geo/ide/drive/cd_0", test_ide_drive_cd_0); qtest_add_func("hd-geo/ide/device/mbr/blank", test_ide_device_mbr_blank); qtest_add_func("hd-geo/ide/device/mbr/lba", test_ide_device_mbr_lba); -- cgit v1.2.3 From 572023f7b235679716c4e57c9d701d83444f14b8 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 13 Jun 2018 11:01:30 +0200 Subject: block: Remove deprecated -drive option serial This reinstates commit b0083267444a5e0f28391f6c2831a539f878d424, which was temporarily reverted for the 3.0 release so that libvirt gets some extra time to update their command lines. The -drive option serial was deprecated in QEMU 2.10. It's time to remove it. Tests need to be updated to set the serial number with -global instead of using the -drive option. Signed-off-by: Kevin Wolf Reviewed-by: Markus Armbruster Reviewed-by: Jeff Cody --- tests/ahci-test.c | 6 +++--- tests/ide-test.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/ahci-test.c b/tests/ahci-test.c index 1a7b761304..937ed2f910 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -180,12 +180,12 @@ static AHCIQState *ahci_boot(const char *cli, ...) s = ahci_vboot(cli, ap); va_end(ap); } else { - cli = "-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s" - ",format=%s" + cli = "-drive if=none,id=drive0,file=%s,cache=writeback,format=%s" " -M q35 " "-device ide-hd,drive=drive0 " + "-global ide-hd.serial=%s " "-global ide-hd.ver=%s"; - s = ahci_boot(cli, tmp_path, "testdisk", imgfmt, "version"); + s = ahci_boot(cli, tmp_path, imgfmt, "testdisk", "version"); } return s; diff --git a/tests/ide-test.c b/tests/ide-test.c index 2384c2c3e2..f39431b1a9 100644 --- a/tests/ide-test.c +++ b/tests/ide-test.c @@ -529,8 +529,8 @@ static void test_bmdma_no_busmaster(void) static void test_bmdma_setup(void) { ide_test_start( - "-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw " - "-global ide-hd.ver=%s", + "-drive file=%s,if=ide,cache=writeback,format=raw " + "-global ide-hd.serial=%s -global ide-hd.ver=%s", tmp_path, "testdisk", "version"); qtest_irq_intercept_in(global_qtest, "ioapic"); } @@ -561,8 +561,8 @@ static void test_identify(void) int ret; ide_test_start( - "-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw " - "-global ide-hd.ver=%s", + "-drive file=%s,if=ide,cache=writeback,format=raw " + "-global ide-hd.serial=%s -global ide-hd.ver=%s", tmp_path, "testdisk", "version"); dev = get_pci_device(&bmdma_bar, &ide_bar); -- cgit v1.2.3 From 86fae10c64d642256cf019e6829929fa0d259c7a Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 14 Aug 2018 11:52:25 +0200 Subject: mirror: Fail gracefully for source == target blockdev-mirror with the same node for source and target segfaults today: A node is in its own backing chain, so mirror_start_job() decides that this is an active commit. When adding the intermediate nodes with block_job_add_bdrv(), it starts the iteration through the subchain with the backing file of source, though, so it never reaches target and instead runs into NULL at the base. While we could fix that by starting with source itself, there is no point in allowing mirroring a node into itself and I wouldn't be surprised if this caused more problems later. So just check for this scenario and error out. Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/041 | 6 ++++++ tests/qemu-iotests/041.out | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index c20ac7da87..9336ab6ff5 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -234,6 +234,12 @@ class TestSingleBlockdev(TestSingleDrive): result = self.vm.qmp("blockdev-add", **args) self.assert_qmp(result, 'return', {}) + def test_mirror_to_self(self): + result = self.vm.qmp(self.qmp_cmd, job_id='job0', + device=self.qmp_target, sync='full', + target=self.qmp_target) + self.assert_qmp(result, 'error/class', 'GenericError') + test_large_cluster = None test_image_not_found = None test_small_buffer2 = None diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out index c28b392b87..e071d0b261 100644 --- a/tests/qemu-iotests/041.out +++ b/tests/qemu-iotests/041.out @@ -1,5 +1,5 @@ -..................................................................................... +........................................................................................ ---------------------------------------------------------------------- -Ran 85 tests +Ran 88 tests OK -- cgit v1.2.3