From bcf23482ae00e040dbef46c44ff914bf788a0937 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Wed, 15 Jun 2016 17:36:29 +0200 Subject: qemu-img: Use strerror() for generic resize error Emitting the plain error number is not very helpful. Use strerror() instead. Signed-off-by: Max Reitz Message-id: 20160615153630.2116-2-mreitz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- qemu-img.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu-img.c b/qemu-img.c index 969edce..2e40e1f 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -3283,7 +3283,7 @@ static int img_resize(int argc, char **argv) error_report("Image is read-only"); break; default: - error_report("Error resizing image (%d)", -ret); + error_report("Error resizing image: %s", strerror(-ret)); break; } out: -- cgit v1.1 From 84c26520d3c1c9ff4a10455748139463278816d5 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Wed, 15 Jun 2016 17:36:30 +0200 Subject: qcow2: Avoid making the L1 table too big We refuse to open images whose L1 table we deem "too big". Consequently, we should not produce such images ourselves. Cc: qemu-stable@nongnu.org Signed-off-by: Max Reitz Message-id: 20160615153630.2116-3-mreitz@redhat.com Reviewed-by: Eric Blake [mreitz: Added QEMU_BUILD_BUG_ON()] Signed-off-by: Max Reitz --- block/qcow2-cluster.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 6b92ce9..00c16dc 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -65,7 +65,8 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, } } - if (new_l1_size > INT_MAX / sizeof(uint64_t)) { + QEMU_BUILD_BUG_ON(QCOW_MAX_L1_SIZE > INT_MAX); + if (new_l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) { return -EFBIG; } -- cgit v1.1 From a367467995d0528fe591d87ca2e437c7b7d7951b Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 20 Jun 2016 16:26:22 +0200 Subject: qemu-io: Use correct range limitations create_iovec() has a comment lamenting the lack of SIZE_T_MAX. Since there actually is a SIZE_MAX, use it. Two places use INT_MAX for checking the upper bound of a sector count that is used as an argument for a blk_*() function (blk_discard() and blk_write_compressed(), respectively). BDRV_REQUEST_MAX_SECTORS should be used instead. And finally, do_co_pwrite_zeroes() used to similarly check that the sector count does not exceed INT_MAX. However, this function is now backed by blk_co_pwrite_zeroes() which takes bytes as an argument instead of sectors. Therefore, it should be the byte count that does not exceed INT_MAX, not the sector count. Signed-off-by: Max Reitz --- qemu-io-cmds.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 40fe2eb..6e29edb 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -389,9 +389,9 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov, goto fail; } - /* should be SIZE_T_MAX, but that doesn't exist */ - if (len > INT_MAX) { - printf("Argument '%s' exceeds maximum size %d\n", arg, INT_MAX); + if (len > SIZE_MAX) { + printf("Argument '%s' exceeds maximum size %llu\n", arg, + (unsigned long long)SIZE_MAX); goto fail; } @@ -479,7 +479,7 @@ static int do_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, .done = false, }; - if (count >> BDRV_SECTOR_BITS > INT_MAX) { + if (count > INT_MAX) { return -ERANGE; } @@ -500,7 +500,7 @@ static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset, { int ret; - if (count >> 9 > INT_MAX) { + if (count >> 9 > BDRV_REQUEST_MAX_SECTORS) { return -ERANGE; } @@ -1688,9 +1688,9 @@ static int discard_f(BlockBackend *blk, int argc, char **argv) if (count < 0) { print_cvtnum_err(count, argv[optind]); return 0; - } else if (count >> BDRV_SECTOR_BITS > INT_MAX) { + } else if (count >> BDRV_SECTOR_BITS > BDRV_REQUEST_MAX_SECTORS) { printf("length cannot exceed %"PRIu64", given %s\n", - (uint64_t)INT_MAX << BDRV_SECTOR_BITS, + (uint64_t)BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS, argv[optind]); return 0; } -- cgit v1.1 From c834cba90521576224c30b15ebb4d6aeab7b42c4 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 20 Jun 2016 16:26:23 +0200 Subject: qcow2: Fix qcow2_get_cluster_offset() Recently, qcow2_get_cluster_offset() has been changed to work with bytes instead of sectors. This invalidated some assertions and introduced a possible integer multiplication overflow. This could be reproduced using e.g. $ qemu-img create -f qcow2 -o cluster_size=1M blub.qcow2 8G Formatting 'foo.qcow2', fmt=qcow2 size=8589934592 encryption=off cluster_size=1048576 lazy_refcounts=off refcount_bits=16 $ qemu-io -c map blub.qcow2 qemu-io: qemu/block/qcow2-cluster.c:504: qcow2_get_cluster_offset: Assertion `bytes_needed <= INT_MAX' failed. [1] 20775 abort (core dumped) qemu-io -c map foo.qcow2 This patch removes the now wrong assertion, adding comments and more assertions to prove its correctness (and fixing the overflow which would become apparent with the original assertion removed). Signed-off-by: Max Reitz Message-id: 20160620142623.24471-3-mreitz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- block/qcow2-cluster.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 00c16dc..f941835 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -483,8 +483,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, unsigned int l2_index; uint64_t l1_index, l2_offset, *l2_table; int l1_bits, c; - unsigned int offset_in_cluster, nb_clusters; - uint64_t bytes_available, bytes_needed; + unsigned int offset_in_cluster; + uint64_t bytes_available, bytes_needed, nb_clusters; int ret; offset_in_cluster = offset_into_cluster(s, offset); @@ -500,7 +500,6 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, if (bytes_needed > bytes_available) { bytes_needed = bytes_available; } - assert(bytes_needed <= INT_MAX); *cluster_offset = 0; @@ -537,8 +536,11 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1); *cluster_offset = be64_to_cpu(l2_table[l2_index]); - /* nb_needed <= INT_MAX, thus nb_clusters <= INT_MAX, too */ nb_clusters = size_to_clusters(s, bytes_needed); + /* bytes_needed <= *bytes + offset_in_cluster, both of which are unsigned + * integers; the minimum cluster size is 512, so this assertion is always + * true */ + assert(nb_clusters <= INT_MAX); ret = qcow2_get_cluster_type(*cluster_offset); switch (ret) { @@ -585,13 +587,17 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table); - bytes_available = (c * s->cluster_size); + bytes_available = (int64_t)c * s->cluster_size; out: if (bytes_available > bytes_needed) { bytes_available = bytes_needed; } + /* bytes_available <= bytes_needed <= *bytes + offset_in_cluster; + * subtracting offset_in_cluster will therefore definitely yield something + * not exceeding UINT_MAX */ + assert(bytes_available - offset_in_cluster <= UINT_MAX); *bytes = bytes_available - offset_in_cluster; return ret; -- cgit v1.1 From f14a39ccb922ee123741ae2cf70a10eef9a650fc Mon Sep 17 00:00:00 2001 From: Sascha Silbe Date: Tue, 28 Jun 2016 17:28:41 +0200 Subject: Improve block job rate limiting for small bandwidth values ratelimit_calculate_delay() previously reset the accounting every time slice, no matter how much data had been processed before. This had (at least) two consequences: 1. The minimum speed is rather large, e.g. 5 MiB/s for commit and stream. Not sure if there are real-world use cases where this would be a problem. Mirroring and backup over a slow link (e.g. DSL) would come to mind, though. 2. Tests for block job operations (e.g. cancel) were rather racy All block jobs currently use a time slice of 100ms. That's a reasonable value to get smooth output during regular operation. However this also meant that the state of block jobs changed every 100ms, no matter how low the configured limit was. On busy hosts, qemu often transferred additional chunks until the test case had a chance to cancel the job. Fix the block job rate limit code to delay for more than one time slice to address the above issues. To make it easier to handle oversized chunks we switch the semantics from returning a delay _before_ the current request to a delay _after_ the current request. If necessary, this delay consists of multiple time slice units. Since the mirror job sends multiple chunks in one go even if the rate limit was exceeded in between, we need to keep track of the start of the current time slice so we can correctly re-compute the delay for the updated amount of data. The minimum bandwidth now is 1 data unit per time slice. The block jobs are currently passing the amount of data transferred in sectors and using 100ms time slices, so this translates to 5120 bytes/second. With chunk sizes usually being O(512KiB), tests have plenty of time (O(100s)) to operate on block jobs. The chance of a race condition now is fairly remote, except possibly on insanely loaded systems. Signed-off-by: Sascha Silbe Message-id: 1467127721-9564-2-git-send-email-silbe@linux.vnet.ibm.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- block/commit.c | 13 +++++-------- block/mirror.c | 4 +++- block/stream.c | 12 ++++-------- include/qemu/ratelimit.h | 43 ++++++++++++++++++++++++++++++++++--------- 4 files changed, 46 insertions(+), 26 deletions(-) diff --git a/block/commit.c b/block/commit.c index 5d11eb6..553e18d 100644 --- a/block/commit.c +++ b/block/commit.c @@ -113,6 +113,7 @@ static void coroutine_fn commit_run(void *opaque) CommitBlockJob *s = opaque; CommitCompleteData *data; int64_t sector_num, end; + uint64_t delay_ns = 0; int ret = 0; int n = 0; void *buf = NULL; @@ -142,10 +143,8 @@ static void coroutine_fn commit_run(void *opaque) buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE); for (sector_num = 0; sector_num < end; sector_num += n) { - uint64_t delay_ns = 0; bool copy; -wait: /* Note that even when no rate limit is applied we need to yield * with no pending I/O here so that bdrv_drain_all() returns. */ @@ -161,12 +160,6 @@ wait: copy = (ret == 1); trace_commit_one_iteration(s, sector_num, n, ret); if (copy) { - if (s->common.speed) { - delay_ns = ratelimit_calculate_delay(&s->limit, n); - if (delay_ns > 0) { - goto wait; - } - } ret = commit_populate(s->top, s->base, sector_num, n, buf); bytes_written += n * BDRV_SECTOR_SIZE; } @@ -182,6 +175,10 @@ wait: } /* Publish progress */ s->common.offset += n * BDRV_SECTOR_SIZE; + + if (copy && s->common.speed) { + delay_ns = ratelimit_calculate_delay(&s->limit, n); + } } ret = 0; diff --git a/block/mirror.c b/block/mirror.c index 71e5ad4..b1e633e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -422,7 +422,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) assert(io_sectors); sector_num += io_sectors; nb_chunks -= DIV_ROUND_UP(io_sectors, sectors_per_chunk); - delay_ns += ratelimit_calculate_delay(&s->limit, io_sectors); + if (s->common.speed) { + delay_ns = ratelimit_calculate_delay(&s->limit, io_sectors); + } } return delay_ns; } diff --git a/block/stream.c b/block/stream.c index 2e7c654..3187481 100644 --- a/block/stream.c +++ b/block/stream.c @@ -95,6 +95,7 @@ static void coroutine_fn stream_run(void *opaque) BlockDriverState *base = s->base; int64_t sector_num = 0; int64_t end = -1; + uint64_t delay_ns = 0; int error = 0; int ret = 0; int n = 0; @@ -123,10 +124,8 @@ static void coroutine_fn stream_run(void *opaque) } for (sector_num = 0; sector_num < end; sector_num += n) { - uint64_t delay_ns = 0; bool copy; -wait: /* Note that even when no rate limit is applied we need to yield * with no pending I/O here so that bdrv_drain_all() returns. */ @@ -156,12 +155,6 @@ wait: } trace_stream_one_iteration(s, sector_num, n, ret); if (copy) { - if (s->common.speed) { - delay_ns = ratelimit_calculate_delay(&s->limit, n); - if (delay_ns > 0) { - goto wait; - } - } ret = stream_populate(blk, sector_num, n, buf); } if (ret < 0) { @@ -182,6 +175,9 @@ wait: /* Publish progress */ s->common.offset += n * BDRV_SECTOR_SIZE; + if (copy && s->common.speed) { + delay_ns = ratelimit_calculate_delay(&s->limit, n); + } } if (!base) { diff --git a/include/qemu/ratelimit.h b/include/qemu/ratelimit.h index 1e3cb13..8da1232 100644 --- a/include/qemu/ratelimit.h +++ b/include/qemu/ratelimit.h @@ -15,34 +15,59 @@ #define QEMU_RATELIMIT_H typedef struct { - int64_t next_slice_time; + int64_t slice_start_time; + int64_t slice_end_time; uint64_t slice_quota; uint64_t slice_ns; uint64_t dispatched; } RateLimit; +/** Calculate and return delay for next request in ns + * + * Record that we sent @p n data units. If we may send more data units + * in the current time slice, return 0 (i.e. no delay). Otherwise + * return the amount of time (in ns) until the start of the next time + * slice that will permit sending the next chunk of data. + * + * Recording sent data units even after exceeding the quota is + * permitted; the time slice will be extended accordingly. + */ static inline int64_t ratelimit_calculate_delay(RateLimit *limit, uint64_t n) { int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME); + uint64_t delay_slices; - if (limit->next_slice_time < now) { - limit->next_slice_time = now + limit->slice_ns; + assert(limit->slice_quota && limit->slice_ns); + + if (limit->slice_end_time < now) { + /* Previous, possibly extended, time slice finished; reset the + * accounting. */ + limit->slice_start_time = now; + limit->slice_end_time = now + limit->slice_ns; limit->dispatched = 0; } - if (limit->dispatched == 0 || limit->dispatched + n <= limit->slice_quota) { - limit->dispatched += n; + + limit->dispatched += n; + if (limit->dispatched < limit->slice_quota) { + /* We may send further data within the current time slice, no + * need to delay the next request. */ return 0; - } else { - limit->dispatched = n; - return limit->next_slice_time - now; } + + /* Quota exceeded. Calculate the next time slice we may start + * sending data again. */ + delay_slices = (limit->dispatched + limit->slice_quota - 1) / + limit->slice_quota; + limit->slice_end_time = limit->slice_start_time + + delay_slices * limit->slice_ns; + return limit->slice_end_time - now; } static inline void ratelimit_set_speed(RateLimit *limit, uint64_t speed, uint64_t slice_ns) { limit->slice_ns = slice_ns; - limit->slice_quota = ((double)speed * slice_ns)/1000000000ULL; + limit->slice_quota = MAX(((double)speed * slice_ns) / 1000000000ULL, 1); } #endif -- cgit v1.1 From 524089bce43fd1cd3daaca979872451efa2cf7c6 Mon Sep 17 00:00:00 2001 From: Reda Sallahi Date: Thu, 7 Jul 2016 10:42:49 +0200 Subject: vmdk: fix metadata write regression Commit "cdeaf1f vmdk: add bdrv_co_write_zeroes" causes a regression on writes. It writes metadata after every write instead of doing it only once for each cluster. vmdk_pwritev() writes metadata whenever m_data is set as valid so this patch sets m_data as valid only when we have a new cluster which hasn't been allocated before or a zero grain. Signed-off-by: Reda Sallahi Message-id: 20160707084249.29084-1-fullmanet@gmail.com Reviewed-by: Fam Zheng Signed-off-by: Max Reitz --- block/vmdk.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index c9914f7..46d474e 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1202,13 +1202,6 @@ static int get_cluster_offset(BlockDriverState *bs, l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size; cluster_sector = le32_to_cpu(l2_table[l2_index]); - if (m_data) { - m_data->valid = 1; - m_data->l1_index = l1_index; - m_data->l2_index = l2_index; - m_data->l2_offset = l2_offset; - m_data->l2_cache_entry = &l2_table[l2_index]; - } if (extent->has_zero_grain && cluster_sector == VMDK_GTE_ZEROED) { zeroed = true; } @@ -1231,6 +1224,13 @@ static int get_cluster_offset(BlockDriverState *bs, if (ret) { return ret; } + if (m_data) { + m_data->valid = 1; + m_data->l1_index = l1_index; + m_data->l2_index = l2_index; + m_data->l2_offset = l2_offset; + m_data->l2_cache_entry = &l2_table[l2_index]; + } } *cluster_offset = cluster_sector << BDRV_SECTOR_BITS; return VMDK_OK; -- cgit v1.1 From ff356ee4da0a2e691a7ab0165d47279f868977c4 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Fri, 8 Jul 2016 17:03:00 +0300 Subject: 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. Signed-off-by: Alberto Garcia Reported-by: Stefan Hajnoczi Cc: Max Reitz Cc: qemu-stable@nongnu.org Message-id: af5cd58bd2c4b9f6c57f260d9cfe586b9fb7d34d.1467986342.git.berto@igalia.com [mreitz: Use existing "id" variable instead of new "blk_id"] Signed-off-by: Max Reitz --- blockdev.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/blockdev.c b/blockdev.c index aa23dc2..384ad3b 100644 --- a/blockdev.c +++ b/blockdev.c @@ -512,6 +512,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, writethrough = !qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, true); + id = qemu_opts_id(opts); + qdict_extract_subqdict(bs_opts, &interval_dict, "stats-intervals."); qdict_array_split(interval_dict, &interval_list); @@ -616,7 +618,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 = id; } blk_io_limits_enable(blk, throttling_group); blk_set_io_limits(blk, &cfg); @@ -625,7 +627,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, id, errp)) { blk_unref(blk); blk = NULL; goto err_no_bs_opts; -- cgit v1.1 From 435d5ee6cd3fd7aa0f16748c03648ed97b493c2b Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Fri, 8 Jul 2016 17:03:01 +0300 Subject: qemu-iotests: Test naming of throttling groups Throttling 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. Signed-off-by: Alberto Garcia Message-id: d87d02823a6b91609509d8bb18e2f5dbd9a6102c.1467986342.git.berto@igalia.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- tests/qemu-iotests/093 | 98 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/093.out | 4 +- 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093 index ce8e13c..ffcb271 100755 --- a/tests/qemu-iotests/093 +++ b/tests/qemu-iotests/093 @@ -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"]) diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out index 89968f3..914e373 100644 --- a/tests/qemu-iotests/093.out +++ b/tests/qemu-iotests/093.out @@ -1,5 +1,5 @@ -.... +..... ---------------------------------------------------------------------- -Ran 4 tests +Ran 5 tests OK -- cgit v1.1 From 3a1ee711904f12f601fffca31a1050d39f833487 Mon Sep 17 00:00:00 2001 From: Lin Ma Date: Thu, 7 Jul 2016 13:26:03 +0800 Subject: hmp: use snapshot name to determine whether a snapshot is 'fully available' Currently qemu uses snapshot id to determine whether a snapshot is fully available, It causes incorrect output in some scenario. For instance: (qemu) info block drive_image1 (#block113): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk0.qcow2 (qcow2) Cache mode: writeback drive_image2 (#block349): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk1.qcow2 (qcow2) Cache mode: writeback (qemu) (qemu) info snapshots There is no snapshot available. (qemu) (qemu) snapshot_blkdev_internal drive_image1 snap1 (qemu) (qemu) info snapshots There is no suitable snapshot available (qemu) (qemu) savevm checkpoint-1 (qemu) (qemu) info snapshots ID TAG VM SIZE DATE VM CLOCK 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 (qemu) $ qemu-img snapshot -l disk0.qcow2 Snapshot list: ID TAG VM SIZE DATE VM CLOCK 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 2 checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 $ qemu-img snapshot -l disk1.qcow2 Snapshot list: ID TAG VM SIZE DATE VM CLOCK 1 checkpoint-1 0 2016-05-22 16:58:07 00:02:06.813 The patch uses snapshot name instead of snapshot id to determine whether a snapshot is fully available and uses '--' instead of snapshot id in output because the snapshot id is not guaranteed to be the same on all images. For instance: (qemu) info snapshots List of snapshots present on all disks: ID TAG VM SIZE DATE VM CLOCK -- checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 Signed-off-by: Lin Ma Reviewed-by: Max Reitz Message-id: 1467869164-26688-2-git-send-email-lma@suse.com Signed-off-by: Max Reitz --- migration/savevm.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/migration/savevm.c b/migration/savevm.c index 38b85ee..a8f22da 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2230,7 +2230,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) available_snapshots = g_new0(int, nb_sns); total = 0; for (i = 0; i < nb_sns; i++) { - if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) { + if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) { available_snapshots[total] = i; total++; } @@ -2241,6 +2241,10 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) monitor_printf(mon, "\n"); for (i = 0; i < total; i++) { sn = &sn_tab[available_snapshots[i]]; + /* The ID is not guaranteed to be the same on all images, so + * overwrite it. + */ + pstrcpy(sn->id_str, sizeof(sn->id_str), "--"); bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn); monitor_printf(mon, "\n"); } -- cgit v1.1 From 0c204cc810af90ca2a449d08d9d39ec8b760d9b4 Mon Sep 17 00:00:00 2001 From: Lin Ma Date: Thu, 7 Jul 2016 13:26:04 +0800 Subject: hmp: show all of snapshot info on every block dev in output of 'info snapshots' Currently, the output of 'info snapshots' shows fully available snapshots. It's opaque, hides some snapshot information to users. It's not convenient if users want to know more about all of snapshot information on every block device via monitor. Follow Kevin's and Max's proposals, The patch makes the output more detailed: (qemu) info snapshots List of snapshots present on all disks: ID TAG VM SIZE DATE VM CLOCK -- checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 List of partial (non-loadable) snapshots on 'drive_image1': ID TAG VM SIZE DATE VM CLOCK 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 Signed-off-by: Lin Ma Message-id: 1467869164-26688-3-git-send-email-lma@suse.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- migration/savevm.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 7 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index a8f22da..33a2911 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2200,12 +2200,31 @@ void hmp_delvm(Monitor *mon, const QDict *qdict) void hmp_info_snapshots(Monitor *mon, const QDict *qdict) { BlockDriverState *bs, *bs1; + BdrvNextIterator it1; QEMUSnapshotInfo *sn_tab, *sn; + bool no_snapshot = true; int nb_sns, i; int total; - int *available_snapshots; + int *global_snapshots; AioContext *aio_context; + typedef struct SnapshotEntry { + QEMUSnapshotInfo sn; + QTAILQ_ENTRY(SnapshotEntry) next; + } SnapshotEntry; + + typedef struct ImageEntry { + const char *imagename; + QTAILQ_ENTRY(ImageEntry) next; + QTAILQ_HEAD(, SnapshotEntry) snapshots; + } ImageEntry; + + QTAILQ_HEAD(, ImageEntry) image_list = + QTAILQ_HEAD_INITIALIZER(image_list); + + ImageEntry *image_entry, *next_ie; + SnapshotEntry *snapshot_entry; + bs = bdrv_all_find_vmstate_bs(); if (!bs) { monitor_printf(mon, "No available block device supports snapshots\n"); @@ -2222,25 +2241,65 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) return; } - if (nb_sns == 0) { + for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) { + int bs1_nb_sns = 0; + ImageEntry *ie; + SnapshotEntry *se; + AioContext *ctx = bdrv_get_aio_context(bs1); + + aio_context_acquire(ctx); + if (bdrv_can_snapshot(bs1)) { + sn = NULL; + bs1_nb_sns = bdrv_snapshot_list(bs1, &sn); + if (bs1_nb_sns > 0) { + no_snapshot = false; + ie = g_new0(ImageEntry, 1); + ie->imagename = bdrv_get_device_name(bs1); + QTAILQ_INIT(&ie->snapshots); + QTAILQ_INSERT_TAIL(&image_list, ie, next); + for (i = 0; i < bs1_nb_sns; i++) { + se = g_new0(SnapshotEntry, 1); + se->sn = sn[i]; + QTAILQ_INSERT_TAIL(&ie->snapshots, se, next); + } + } + g_free(sn); + } + aio_context_release(ctx); + } + + if (no_snapshot) { monitor_printf(mon, "There is no snapshot available.\n"); return; } - available_snapshots = g_new0(int, nb_sns); + global_snapshots = g_new0(int, nb_sns); total = 0; for (i = 0; i < nb_sns; i++) { + SnapshotEntry *next_sn; if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) { - available_snapshots[total] = i; + global_snapshots[total] = i; total++; + QTAILQ_FOREACH(image_entry, &image_list, next) { + QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, + next, next_sn) { + if (!strcmp(sn_tab[i].name, snapshot_entry->sn.name)) { + QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry, + next); + g_free(snapshot_entry); + } + } + } } } + monitor_printf(mon, "List of snapshots present on all disks:\n"); + if (total > 0) { bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); monitor_printf(mon, "\n"); for (i = 0; i < total; i++) { - sn = &sn_tab[available_snapshots[i]]; + sn = &sn_tab[global_snapshots[i]]; /* The ID is not guaranteed to be the same on all images, so * overwrite it. */ @@ -2249,11 +2308,35 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) monitor_printf(mon, "\n"); } } else { - monitor_printf(mon, "There is no suitable snapshot available\n"); + monitor_printf(mon, "None\n"); } + QTAILQ_FOREACH(image_entry, &image_list, next) { + if (QTAILQ_EMPTY(&image_entry->snapshots)) { + continue; + } + monitor_printf(mon, + "\nList of partial (non-loadable) snapshots on '%s':\n", + image_entry->imagename); + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); + monitor_printf(mon, "\n"); + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) { + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, + &snapshot_entry->sn); + monitor_printf(mon, "\n"); + } + } + + QTAILQ_FOREACH_SAFE(image_entry, &image_list, next, next_ie) { + SnapshotEntry *next_sn; + QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, next, + next_sn) { + g_free(snapshot_entry); + } + g_free(image_entry); + } g_free(sn_tab); - g_free(available_snapshots); + g_free(global_snapshots); } -- cgit v1.1 From c4b48bfdc59caf0a69b7e0a40c9ea6d3d7848bc3 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 11 Jul 2016 15:54:52 +0200 Subject: vvfat: Fix qcow write target driver specification First, bdrv_open_child() expects all options for the child to be prefixed by the child's name (and a separating dot). Second, bdrv_open_child() does not take ownership of the QDict passed to it but only extracts all options for the child, so if a QDict is created for the sole purpose of passing it to bdrv_open_child(), it needs to be freed afterwards. This patch makes vvfat adhere to both of these rules. Signed-off-by: Max Reitz Message-id: 20160711135452.11304-1-mreitz@redhat.com Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Signed-off-by: Max Reitz --- block/vvfat.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/vvfat.c b/block/vvfat.c index c3f24c6..ba2620f 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -3018,9 +3018,10 @@ static int enable_write_target(BlockDriverState *bs, Error **errp) } options = qdict_new(); - qdict_put(options, "driver", qstring_from_str("qcow")); + qdict_put(options, "write-target.driver", qstring_from_str("qcow")); s->qcow = bdrv_open_child(s->qcow_filename, options, "write-target", bs, &child_vvfat_qcow, false, errp); + QDECREF(options); if (!s->qcow) { ret = -EINVAL; goto err; -- cgit v1.1 From 42190dcc70b34d59c81d72f1f8ff884aa27dd851 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 11 Jul 2016 15:22:46 +0200 Subject: iotests: Make 157 actually format-agnostic iotest 157 pretends not to care about the image format used, but in fact it does due to the format name not being filtered in its output. This patch adds filtering and changes the reference output accordingly. Signed-off-by: Max Reitz Message-id: 20160711132246.3152-1-mreitz@redhat.com Reviewed-by: John Snow Signed-off-by: Max Reitz --- tests/qemu-iotests/157 | 3 ++- tests/qemu-iotests/157.out | 16 ++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/qemu-iotests/157 b/tests/qemu-iotests/157 index 2699168..8d939cb 100755 --- a/tests/qemu-iotests/157 +++ b/tests/qemu-iotests/157 @@ -57,7 +57,8 @@ function do_run_qemu() function run_qemu() { - do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_generated_node_ids + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_imgfmt \ + | _filter_qemu | _filter_generated_node_ids } diff --git a/tests/qemu-iotests/157.out b/tests/qemu-iotests/157.out index 5aa9c0e..77a9c03 100644 --- a/tests/qemu-iotests/157.out +++ b/tests/qemu-iotests/157.out @@ -3,20 +3,20 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 === Setting WCE with qdev and with manually created BB === -Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device virtio-blk,drive=none0 +Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,driver=IMGFMT,cache=writeback -device virtio-blk,drive=none0 Cache mode: writeback -Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device virtio-blk,drive=none0,write-cache=auto +Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,driver=IMGFMT,cache=writeback -device virtio-blk,drive=none0,write-cache=auto Cache mode: writeback -Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device virtio-blk,drive=none0,write-cache=on +Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,driver=IMGFMT,cache=writeback -device virtio-blk,drive=none0,write-cache=on Cache mode: writeback -Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device virtio-blk,drive=none0,write-cache=off +Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,driver=IMGFMT,cache=writeback -device virtio-blk,drive=none0,write-cache=off Cache mode: writethrough -Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device virtio-blk,drive=none0 +Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,driver=IMGFMT,cache=writethrough -device virtio-blk,drive=none0 Cache mode: writethrough -Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device virtio-blk,drive=none0,write-cache=auto +Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,driver=IMGFMT,cache=writethrough -device virtio-blk,drive=none0,write-cache=auto Cache mode: writethrough -Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device virtio-blk,drive=none0,write-cache=on +Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,driver=IMGFMT,cache=writethrough -device virtio-blk,drive=none0,write-cache=on Cache mode: writeback -Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device virtio-blk,drive=none0,write-cache=off +Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,driver=IMGFMT,cache=writethrough -device virtio-blk,drive=none0,write-cache=off Cache mode: writethrough *** done -- cgit v1.1