[Qemu-block] [PATCH v6 1/8] vmdk: Move vmdk_find_offset_in_cluster() to the top
Move the existing vmdk_find_offset_in_cluster() function to the top of
the driver.
Signed-off-by: Ashijeet Acharya
Reviewed-by: Fam Zheng
---
block/vmdk.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index a9bd22b..22be887 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -242,6 +242,18 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
s->extents = g_renew(VmdkExtent, s->extents, s->num_extents);
}
+static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
+ int64_t offset)
+{
+uint64_t extent_begin_offset, extent_relative_offset;
+uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
+
+extent_begin_offset =
+(extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
+extent_relative_offset = offset - extent_begin_offset;
+return extent_relative_offset % cluster_size;
+}
+
static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
{
char *desc;
@@ -1266,18 +1278,6 @@ static VmdkExtent *find_extent(BDRVVmdkState *s,
return NULL;
}
-static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
- int64_t offset)
-{
-uint64_t extent_begin_offset, extent_relative_offset;
-uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
-
-extent_begin_offset =
-(extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
-extent_relative_offset = offset - extent_begin_offset;
-return extent_relative_offset % cluster_size;
-}
-
static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
int64_t sector_num)
{
--
2.6.2
[Qemu-block] [PATCH v6 0/8] Optimize VMDK I/O by allocating multiple clusters
Previously posted series patches: v1 - http://lists.nongnu.org/archive/html/qemu-devel/2017-03/msg02044.html v2 - http://lists.nongnu.org/archive/html/qemu-devel/2017-03/msg05080.html v3 - http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg00074.html v4 - http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg03851.html v5 - http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg00929.html This series helps to optimize the I/O performance of VMDK driver. Patch 1 helps us to move vmdk_find_offset_in_cluster. Patch 2 & 3 perform a simple function re-naming tasks. Patch 4 is used to factor out metadata loading code and implement it in separate functions. This will help us to avoid code duplication in future patches of this series. Patch 5 helps to set the upper limit of the bytes handled in one cycle. Patch 6 adds new functions to help us allocate multiple clusters according to the size requested, perform COW if required and return the offset of the first newly allocated cluster. Patch 7 changes the metadata update code to update the L2 tables for multiple clusters at once. Patch 8 helps us to finally change vmdk_get_cluster_offset() to find cluster offset only as cluster allocation task is now handled by vmdk_alloc_clusters() Optimization test results: This patch series improves 128 KB sequential write performance to an empty VMDK file by 54% Benchmark command: ./qemu-img bench -w -c 1024 -s 128K -d 1 -t none -f vmdk test.vmdk Changes in v6: - rename total_alloc_clusters as alloc_clusters_counter (fam) Changes in v5: - fix commit message and comment in patch 4 (fam) - add vmdk_ prefix to handle_alloc() (fam) - fix alignment issue in patch 6 (fam) - use BDRV_SECTOR_BITS (fam) - fix endianness calculation in patch 7 (fam) Changes in v4: - fix commit message in patch 1 (fam) - drop size_to_clusters() function (fam) - fix grammatical errors in function documentations (fam) - factor out metadata loading coding in a separate patch (patch 4) (fam) - rename vmdk_alloc_cluster_offset() to vmdk_alloc_clusters() (fam) - break patch 4(in v3) into separate patches (patch 3 and 8) (fam) - rename extent_size to extent_end (fam) - use QEMU_ALIGN_UP instead of vmdk_align_offset. (fam) - drop next and simply do m_data = m_data->next (fam) Changes in v3: - move size_to_clusters() from patch 1 to 3 (fam) - use DIV_ROUND_UP in size_to_clusters (fam) - make patch 2 compilable (fam) - rename vmdk_L2update as vmdk_l2update and use UINT32_MAX (fam) - combine patch 3 and patch 4 (as in v2) to make them compilable (fam) - call bdrv_pwrite_sync() for batches of atmost 512 clusters at once (fam) Changes in v2: - segregate the ugly Patch 1 in v1 into 6 readable and sensible patches - include benchmark test results in v2 Ashijeet Acharya (8): vmdk: Move vmdk_find_offset_in_cluster() to the top vmdk: Rename get_whole_cluster() to vmdk_perform_cow() vmdk: Rename get_cluster_offset() to vmdk_get_cluster_offset() vmdk: Factor out metadata loading code out of vmdk_get_cluster_offset() vmdk: Set maximum bytes allocated in one cycle vmdk: New functions to assist allocating multiple clusters vmdk: Update metadata for multiple clusters vmdk: Make vmdk_get_cluster_offset() return cluster offset only block/vmdk.c | 529 +-- 1 file changed, 407 insertions(+), 122 deletions(-) -- 2.6.2
[Qemu-block] [PATCH v6 3/8] vmdk: Rename get_cluster_offset() to vmdk_get_cluster_offset()
Rename the existing get_cluster_offset() to vmdk_get_cluster_offset()
and update name in all the callers accordingly.
Signed-off-by: Ashijeet Acharya
Reviewed-by: Fam Zheng
---
block/vmdk.c | 46 +++---
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 73ae786..f403981 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1144,7 +1144,7 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData
*m_data,
}
/**
- * get_cluster_offset
+ * vmdk_get_cluster_offset
*
* Look up cluster offset in extent file by sector number, and store in
* @cluster_offset.
@@ -1163,14 +1163,14 @@ static int vmdk_L2update(VmdkExtent *extent,
VmdkMetaData *m_data,
* VMDK_UNALLOC if cluster is not mapped and @allocate is false.
* VMDK_ERROR if failed.
*/
-static int get_cluster_offset(BlockDriverState *bs,
- VmdkExtent *extent,
- VmdkMetaData *m_data,
- uint64_t offset,
- bool allocate,
- uint64_t *cluster_offset,
- uint64_t skip_start_bytes,
- uint64_t skip_end_bytes)
+static int vmdk_get_cluster_offset(BlockDriverState *bs,
+ VmdkExtent *extent,
+ VmdkMetaData *m_data,
+ uint64_t offset,
+ bool allocate,
+ uint64_t *cluster_offset,
+ uint64_t skip_start_bytes,
+ uint64_t skip_end_bytes)
{
unsigned int l1_index, l2_offset, l2_index;
int min_index, i, j;
@@ -1304,9 +1304,9 @@ static int64_t coroutine_fn
vmdk_co_get_block_status(BlockDriverState *bs,
return 0;
}
qemu_co_mutex_lock(&s->lock);
-ret = get_cluster_offset(bs, extent, NULL,
- sector_num * 512, false, &offset,
- 0, 0);
+ret = vmdk_get_cluster_offset(bs, extent, NULL,
+ sector_num * 512, false, &offset,
+ 0, 0);
qemu_co_mutex_unlock(&s->lock);
index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
@@ -1497,8 +1497,8 @@ vmdk_co_preadv(BlockDriverState *bs, uint64_t offset,
uint64_t bytes,
ret = -EIO;
goto fail;
}
-ret = get_cluster_offset(bs, extent, NULL,
- offset, false, &cluster_offset, 0, 0);
+ret = vmdk_get_cluster_offset(bs, extent, NULL,
+ offset, false, &cluster_offset, 0, 0);
offset_in_cluster = vmdk_find_offset_in_cluster(extent, offset);
n_bytes = MIN(bytes, extent->cluster_sectors * BDRV_SECTOR_SIZE
@@ -1584,10 +1584,10 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t
offset,
n_bytes = MIN(bytes, extent->cluster_sectors * BDRV_SECTOR_SIZE
- offset_in_cluster);
-ret = get_cluster_offset(bs, extent, &m_data, offset,
- !(extent->compressed || zeroed),
- &cluster_offset, offset_in_cluster,
- offset_in_cluster + n_bytes);
+ret = vmdk_get_cluster_offset(bs, extent, &m_data, offset,
+ !(extent->compressed || zeroed),
+ &cluster_offset, offset_in_cluster,
+ offset_in_cluster + n_bytes);
if (extent->compressed) {
if (ret == VMDK_OK) {
/* Refuse write to allocated cluster for streamOptimized */
@@ -1596,8 +1596,8 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t
offset,
return -EIO;
} else {
/* allocate */
-ret = get_cluster_offset(bs, extent, &m_data, offset,
- true, &cluster_offset, 0, 0);
+ret = vmdk_get_cluster_offset(bs, extent, &m_data, offset,
+ true, &cluster_offset, 0, 0);
}
}
if (ret == VMDK_ERROR) {
@@ -2229,9 +2229,9 @@ static int vmdk_check(BlockDriverState *bs,
BdrvCheckResult *result,
sector_num);
break;
}
-ret = get_cluster_offset(bs, extent, NULL,
- sector_num << BDRV_SECTOR_BITS,
- false, &cluster_offset, 0, 0);
+ret = vmdk_get_cluster_offset(bs, extent, NULL,
+ sector_num << BDRV_SECTOR_BITS,
+ false, &cluster_offset, 0, 0);
if (ret == VMDK_ERROR) {
fprintf(
[Qemu-block] [PATCH v6 2/8] vmdk: Rename get_whole_cluster() to vmdk_perform_cow()
Rename the existing function get_whole_cluster() to vmdk_perform_cow()
as its sole purpose is to perform COW for the first and the last
allocated clusters if needed.
Signed-off-by: Ashijeet Acharya
Reviewed-by: Fam Zheng
---
block/vmdk.c | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 22be887..73ae786 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1028,8 +1028,8 @@ static void vmdk_refresh_limits(BlockDriverState *bs,
Error **errp)
}
}
-/**
- * get_whole_cluster
+/*
+ * vmdk_perform_cow
*
* Copy backing file's cluster that covers @sector_num, otherwise write zero,
* to the cluster at @cluster_sector_num.
@@ -1037,13 +1037,18 @@ static void vmdk_refresh_limits(BlockDriverState *bs,
Error **errp)
* If @skip_start_sector < @skip_end_sector, the relative range
* [@skip_start_sector, @skip_end_sector) is not copied or written, and leave
* it for call to write user data in the request.
+ *
+ * Returns:
+ * VMDK_OK: on success
+ *
+ * VMDK_ERROR:in error cases
*/
-static int get_whole_cluster(BlockDriverState *bs,
- VmdkExtent *extent,
- uint64_t cluster_offset,
- uint64_t offset,
- uint64_t skip_start_bytes,
- uint64_t skip_end_bytes)
+static int vmdk_perform_cow(BlockDriverState *bs,
+VmdkExtent *extent,
+uint64_t cluster_offset,
+uint64_t offset,
+uint64_t skip_start_bytes,
+uint64_t skip_end_bytes)
{
int ret = VMDK_OK;
int64_t cluster_bytes;
@@ -1244,7 +1249,7 @@ static int get_cluster_offset(BlockDriverState *bs,
* This problem may occur because of insufficient space on host disk
* or inappropriate VM shutdown.
*/
-ret = get_whole_cluster(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,
+ret = vmdk_perform_cow(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,
offset, skip_start_bytes, skip_end_bytes);
if (ret) {
return ret;
--
2.6.2
[Qemu-block] [PATCH v6 5/8] vmdk: Set maximum bytes allocated in one cycle
Set the maximum bytes allowed to get allocated at once to be not more
than the extent size boundary to handle writes at two separate extents
appropriately.
Signed-off-by: Ashijeet Acharya
Reviewed-by: Fam Zheng
---
block/vmdk.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 5647f53..fe2046b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1624,6 +1624,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t
offset,
uint64_t cluster_offset;
uint64_t bytes_done = 0;
VmdkMetaData m_data;
+uint64_t extent_end;
if (DIV_ROUND_UP(offset, BDRV_SECTOR_SIZE) > bs->total_sectors) {
error_report("Wrong offset: offset=0x%" PRIx64
@@ -1637,9 +1638,17 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t
offset,
if (!extent) {
return -EIO;
}
+extent_end = extent->end_sector * BDRV_SECTOR_SIZE;
+
offset_in_cluster = vmdk_find_offset_in_cluster(extent, offset);
-n_bytes = MIN(bytes, extent->cluster_sectors * BDRV_SECTOR_SIZE
- - offset_in_cluster);
+
+/* truncate n_bytes to first cluster because we need to perform COW */
+if (offset_in_cluster > 0) {
+n_bytes = MIN(bytes, extent->cluster_sectors * BDRV_SECTOR_SIZE
+ - offset_in_cluster);
+} else {
+n_bytes = MIN(bytes, extent_end - offset);
+}
ret = vmdk_get_cluster_offset(bs, extent, &m_data, offset,
!(extent->compressed || zeroed),
--
2.6.2
[Qemu-block] [PATCH v6 4/8] vmdk: Factor out metadata loading code out of vmdk_get_cluster_offset()
Move the cluster tables loading code out of the existing
vmdk_get_cluster_offset() function and implement it in separate
get_cluster_table() and vmdk_l2load() functions.
Signed-off-by: Ashijeet Acharya
Reviewed-by: Fam Zheng
---
block/vmdk.c | 153 ---
1 file changed, 105 insertions(+), 48 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index f403981..5647f53 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1143,6 +1143,105 @@ static int vmdk_L2update(VmdkExtent *extent,
VmdkMetaData *m_data,
return VMDK_OK;
}
+/*
+ * vmdk_l2load
+ *
+ * Load a new L2 table into memory. If the table is in the cache, the cache
+ * is used; otherwise the L2 table is loaded from the image file.
+ *
+ * Returns:
+ * VMDK_OK: on success
+ * VMDK_ERROR:in error cases
+ */
+static int vmdk_l2load(VmdkExtent *extent, uint64_t offset, int l2_offset,
+ uint32_t **new_l2_table, int *new_l2_index)
+{
+int min_index, i, j;
+uint32_t *l2_table;
+uint32_t min_count;
+
+for (i = 0; i < L2_CACHE_SIZE; i++) {
+if (l2_offset == extent->l2_cache_offsets[i]) {
+/* increment the hit count */
+if (++extent->l2_cache_counts[i] == UINT32_MAX) {
+for (j = 0; j < L2_CACHE_SIZE; j++) {
+extent->l2_cache_counts[j] >>= 1;
+}
+}
+l2_table = extent->l2_cache + (i * extent->l2_size);
+goto found;
+}
+}
+/* not found: load a new entry in the least used one */
+min_index = 0;
+min_count = UINT32_MAX;
+for (i = 0; i < L2_CACHE_SIZE; i++) {
+if (extent->l2_cache_counts[i] < min_count) {
+min_count = extent->l2_cache_counts[i];
+min_index = i;
+}
+}
+l2_table = extent->l2_cache + (min_index * extent->l2_size);
+if (bdrv_pread(extent->file,
+(int64_t)l2_offset * 512,
+l2_table,
+extent->l2_size * sizeof(uint32_t)
+) != extent->l2_size * sizeof(uint32_t)) {
+return VMDK_ERROR;
+}
+
+extent->l2_cache_offsets[min_index] = l2_offset;
+extent->l2_cache_counts[min_index] = 1;
+found:
+*new_l2_index = ((offset >> 9) / extent->cluster_sectors) %
extent->l2_size;
+*new_l2_table = l2_table;
+
+return VMDK_OK;
+}
+
+/*
+ * get_cluster_table
+ *
+ * For a given offset, load (and allocate if needed) the l2 table.
+ *
+ * Returns:
+ * VMDK_OK:on success
+ *
+ * VMDK_UNALLOC: if cluster is not mapped
+ *
+ * VMDK_ERROR: in error cases
+ */
+static int get_cluster_table(VmdkExtent *extent, uint64_t offset,
+ int *new_l1_index, int *new_l2_offset,
+ int *new_l2_index, uint32_t **new_l2_table)
+{
+int l1_index, l2_offset, l2_index;
+uint32_t *l2_table;
+int ret;
+
+offset -= (extent->end_sector - extent->sectors) * SECTOR_SIZE;
+l1_index = (offset >> 9) / extent->l1_entry_sectors;
+if (l1_index >= extent->l1_size) {
+return VMDK_ERROR;
+}
+l2_offset = extent->l1_table[l1_index];
+if (!l2_offset) {
+return VMDK_UNALLOC;
+}
+
+ret = vmdk_l2load(extent, offset, l2_offset, &l2_table, &l2_index);
+if (ret < 0) {
+return ret;
+}
+
+*new_l1_index = l1_index;
+*new_l2_offset = l2_offset;
+*new_l2_index = l2_index;
+*new_l2_table = l2_table;
+
+return VMDK_OK;
+}
+
/**
* vmdk_get_cluster_offset
*
@@ -1172,66 +1271,24 @@ static int vmdk_get_cluster_offset(BlockDriverState *bs,
uint64_t skip_start_bytes,
uint64_t skip_end_bytes)
{
-unsigned int l1_index, l2_offset, l2_index;
-int min_index, i, j;
-uint32_t min_count, *l2_table;
+int l1_index, l2_offset, l2_index;
+uint32_t *l2_table;
bool zeroed = false;
int64_t ret;
int64_t cluster_sector;
-if (m_data) {
-m_data->valid = 0;
-}
if (extent->flat) {
*cluster_offset = extent->flat_start_offset;
return VMDK_OK;
}
-offset -= (extent->end_sector - extent->sectors) * SECTOR_SIZE;
-l1_index = (offset >> 9) / extent->l1_entry_sectors;
-if (l1_index >= extent->l1_size) {
-return VMDK_ERROR;
-}
-l2_offset = extent->l1_table[l1_index];
-if (!l2_offset) {
-return VMDK_UNALLOC;
-}
-for (i = 0; i < L2_CACHE_SIZE; i++) {
-if (l2_offset == extent->l2_cache_offsets[i]) {
-/* increment the hit count */
-if (++extent->l2_cache_counts[i] == 0x) {
-for (j = 0; j < L2_CACHE_SIZE; j++) {
-extent->l2_cache_counts[j] >>= 1;
-}
-}
-l2_table = extent->l2_cache + (i * extent->l2_size);
-goto found;
-}
-}
-/* not found: load a new entry in t
[Qemu-block] [PATCH v6 6/8] vmdk: New functions to assist allocating multiple clusters
Introduce two new helper functions handle_alloc() and
vmdk_alloc_cluster_offset(). handle_alloc() helps to allocate multiple
clusters at once starting from a given offset on disk and performs COW
if necessary for first and last allocated clusters.
vmdk_alloc_cluster_offset() helps to return the offset of the first of
the many newly allocated clusters. Also, provide proper documentation
for both.
Signed-off-by: Ashijeet Acharya
---
block/vmdk.c | 192 +++
1 file changed, 182 insertions(+), 10 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index fe2046b..b671dc9 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -136,6 +136,7 @@ typedef struct VmdkMetaData {
unsigned int l2_offset;
int valid;
uint32_t *l2_cache_entry;
+uint32_t nb_clusters;
} VmdkMetaData;
typedef struct VmdkGrainMarker {
@@ -1242,6 +1243,174 @@ static int get_cluster_table(VmdkExtent *extent,
uint64_t offset,
return VMDK_OK;
}
+/*
+ * vmdk_handle_alloc
+ *
+ * Allocate new clusters for an area that either is yet unallocated or needs a
+ * copy on write. If *cluster_offset is non_zero, clusters are only allocated
if
+ * the new allocation can match the specified host offset.
+ *
+ * Returns:
+ * VMDK_OK: if new clusters were allocated, *bytes may be decreased if
+ * the new allocation doesn't cover all of the requested area.
+ * *cluster_offset is updated to contain the offset of the
+ * first newly allocated cluster.
+ *
+ * VMDK_UNALLOC: if no clusters could be allocated. *cluster_offset is left
+ * unchanged.
+ *
+ * VMDK_ERROR:in error cases
+ */
+static int vmdk_handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
+ uint64_t offset, uint64_t *cluster_offset,
+ int64_t *bytes, VmdkMetaData *m_data,
+ bool allocate, uint32_t *alloc_clusters_counter)
+{
+int l1_index, l2_offset, l2_index;
+uint32_t *l2_table;
+uint32_t cluster_sector;
+uint32_t nb_clusters;
+bool zeroed = false;
+uint64_t skip_start_bytes, skip_end_bytes;
+int ret;
+
+ret = get_cluster_table(extent, offset, &l1_index, &l2_offset,
+&l2_index, &l2_table);
+if (ret < 0) {
+return ret;
+}
+
+cluster_sector = le32_to_cpu(l2_table[l2_index]);
+
+skip_start_bytes = vmdk_find_offset_in_cluster(extent, offset);
+/* Calculate the number of clusters to look for. Here we truncate the last
+ * cluster, i.e. 1 less than the actual value calculated as we may need to
+ * perform COW for the last one. */
+nb_clusters = DIV_ROUND_UP(skip_start_bytes + *bytes,
+ extent->cluster_sectors << BDRV_SECTOR_BITS) -
1;
+
+nb_clusters = MIN(nb_clusters, extent->l2_size - l2_index);
+assert(nb_clusters <= INT_MAX);
+
+/* update bytes according to final nb_clusters value */
+if (nb_clusters != 0) {
+*bytes = ((nb_clusters * extent->cluster_sectors) << BDRV_SECTOR_BITS)
+ - skip_start_bytes;
+} else {
+nb_clusters = 1;
+}
+*alloc_clusters_counter += nb_clusters;
+skip_end_bytes = skip_start_bytes + MIN(*bytes,
+ extent->cluster_sectors * BDRV_SECTOR_SIZE
+- skip_start_bytes);
+
+if (extent->has_zero_grain && cluster_sector == VMDK_GTE_ZEROED) {
+zeroed = true;
+}
+
+if (!cluster_sector || zeroed) {
+if (!allocate) {
+return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
+}
+
+cluster_sector = extent->next_cluster_sector;
+extent->next_cluster_sector += extent->cluster_sectors
+* nb_clusters;
+
+ret = vmdk_perform_cow(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,
+ offset, skip_start_bytes,
+ skip_end_bytes);
+if (ret < 0) {
+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];
+m_data->nb_clusters = nb_clusters;
+}
+}
+*cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
+return VMDK_OK;
+}
+
+/*
+ * vmdk_alloc_clusters
+ *
+ * For a given offset on the virtual disk, find the cluster offset in vmdk
+ * file. If the offset is not found, allocate a new cluster.
+ *
+ * If the cluster is newly allocated, m_data->nb_clusters is set to the number
+ * of contiguous clusters that have been allocated. In this case, the other
+ * fields of m_data are valid and contain information about the first allocated
+ * cluster.
+ *
+ * Returns:
+ *
+ * VMDK_OK: on success a
[Qemu-block] [PATCH v6 7/8] vmdk: Update metadata for multiple clusters
Include a next pointer in VmdkMetaData struct to point to the previous
allocated L2 table. Modify vmdk_L2update to start updating metadata for
allocation of multiple clusters at once.
Signed-off-by: Ashijeet Acharya
---
block/vmdk.c | 128 ++-
1 file changed, 101 insertions(+), 27 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index b671dc9..9fa2414 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -137,6 +137,8 @@ typedef struct VmdkMetaData {
int valid;
uint32_t *l2_cache_entry;
uint32_t nb_clusters;
+uint32_t offset;
+struct VmdkMetaData *next;
} VmdkMetaData;
typedef struct VmdkGrainMarker {
@@ -1116,34 +1118,87 @@ exit:
return ret;
}
-static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
- uint32_t offset)
+static int vmdk_alloc_cluster_link_l2(VmdkExtent *extent,
+ VmdkMetaData *m_data, bool zeroed)
{
-offset = cpu_to_le32(offset);
+int i;
+uint32_t offset, temp_offset;
+int *l2_table_array;
+int l2_array_size;
+
+if (zeroed) {
+temp_offset = VMDK_GTE_ZEROED;
+} else {
+temp_offset = m_data->offset;
+}
+
+l2_array_size = sizeof(uint32_t) * m_data->nb_clusters;
+l2_table_array = qemu_try_blockalign(extent->file->bs,
+ QEMU_ALIGN_UP(l2_array_size,
+ BDRV_SECTOR_SIZE));
+if (l2_table_array == NULL) {
+return VMDK_ERROR;
+}
+memset(l2_table_array, 0, QEMU_ALIGN_UP(l2_array_size, BDRV_SECTOR_SIZE));
/* update L2 table */
+offset = temp_offset;
+for (i = 0; i < m_data->nb_clusters; i++) {
+l2_table_array[i] = cpu_to_le32(offset);
+if (!zeroed) {
+offset += 128;
+}
+}
if (bdrv_pwrite_sync(extent->file,
-((int64_t)m_data->l2_offset * 512)
-+ (m_data->l2_index * sizeof(offset)),
-&offset, sizeof(offset)) < 0) {
+ ((int64_t)m_data->l2_offset * 512)
+ + ((m_data->l2_index) * sizeof(offset)),
+ l2_table_array, l2_array_size) < 0) {
return VMDK_ERROR;
}
/* update backup L2 table */
if (extent->l1_backup_table_offset != 0) {
m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
if (bdrv_pwrite_sync(extent->file,
-((int64_t)m_data->l2_offset * 512)
-+ (m_data->l2_index * sizeof(offset)),
-&offset, sizeof(offset)) < 0) {
+ ((int64_t)m_data->l2_offset * 512)
+ + ((m_data->l2_index) * sizeof(offset)),
+ l2_table_array, l2_array_size) < 0) {
return VMDK_ERROR;
}
}
+
+offset = temp_offset;
if (m_data->l2_cache_entry) {
-*m_data->l2_cache_entry = offset;
+for (i = 0; i < m_data->nb_clusters; i++) {
+*m_data->l2_cache_entry = cpu_to_le32(offset);
+m_data->l2_cache_entry++;
+
+if (!zeroed) {
+offset += 128;
+}
+}
}
+qemu_vfree(l2_table_array);
return VMDK_OK;
}
+static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
+ bool zeroed)
+{
+int ret;
+
+while (m_data->next != NULL) {
+
+ret = vmdk_alloc_cluster_link_l2(extent, m_data, zeroed);
+if (ret < 0) {
+return ret;
+}
+
+m_data = m_data->next;
+ }
+
+ return VMDK_OK;
+}
+
/*
* vmdk_l2load
*
@@ -1261,9 +1316,10 @@ static int get_cluster_table(VmdkExtent *extent,
uint64_t offset,
*
* VMDK_ERROR:in error cases
*/
+
static int vmdk_handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
uint64_t offset, uint64_t *cluster_offset,
- int64_t *bytes, VmdkMetaData *m_data,
+ int64_t *bytes, VmdkMetaData **m_data,
bool allocate, uint32_t *alloc_clusters_counter)
{
int l1_index, l2_offset, l2_index;
@@ -1272,6 +1328,7 @@ static int vmdk_handle_alloc(BlockDriverState *bs,
VmdkExtent *extent,
uint32_t nb_clusters;
bool zeroed = false;
uint64_t skip_start_bytes, skip_end_bytes;
+VmdkMetaData *old_m_data;
int ret;
ret = get_cluster_table(extent, offset, &l1_index, &l2_offset,
@@ -1323,13 +1380,21 @@ static int vmdk_handle_alloc(BlockDriverState *bs,
VmdkExtent *extent,
if (ret < 0) {
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];
-
[Qemu-block] [PATCH v6 8/8] vmdk: Make vmdk_get_cluster_offset() return cluster offset only
vmdk_alloc_clusters() introduced earlier now handles the task of
allocating clusters and performing COW when needed. Thus we can change
vmdk_get_cluster_offset() to stick to the sole purpose of returning
cluster offset using sector number. Update the changes at all call
sites.
Signed-off-by: Ashijeet Acharya
---
block/vmdk.c | 56
1 file changed, 12 insertions(+), 44 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 9fa2414..accf1c3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1485,25 +1485,16 @@ static int vmdk_alloc_clusters(BlockDriverState *bs,
* For flat extents, the start offset as parsed from the description file is
* returned.
*
- * For sparse extents, look up in L1, L2 table. If allocate is true, return an
- * offset for a new cluster and update L2 cache. If there is a backing file,
- * COW is done before returning; otherwise, zeroes are written to the allocated
- * cluster. Both COW and zero writing skips the sector range
- * [@skip_start_sector, @skip_end_sector) passed in by caller, because caller
- * has new data to write there.
+ * For sparse extents, look up the L1, L2 table.
*
* Returns: VMDK_OK if cluster exists and mapped in the image.
- * VMDK_UNALLOC if cluster is not mapped and @allocate is false.
- * VMDK_ERROR if failed.
+ * VMDK_UNALLOC if cluster is not mapped.
+ * VMDK_ERROR if failed
*/
static int vmdk_get_cluster_offset(BlockDriverState *bs,
VmdkExtent *extent,
- VmdkMetaData *m_data,
uint64_t offset,
- bool allocate,
- uint64_t *cluster_offset,
- uint64_t skip_start_bytes,
- uint64_t skip_end_bytes)
+ uint64_t *cluster_offset)
{
int l1_index, l2_offset, l2_index;
uint32_t *l2_table;
@@ -1528,31 +1519,9 @@ static int vmdk_get_cluster_offset(BlockDriverState *bs,
}
if (!cluster_sector || zeroed) {
-if (!allocate) {
-return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
-}
-
-cluster_sector = extent->next_cluster_sector;
-extent->next_cluster_sector += extent->cluster_sectors;
-
-/* First of all we write grain itself, to avoid race condition
- * that may to corrupt the image.
- * This problem may occur because of insufficient space on host disk
- * or inappropriate VM shutdown.
- */
-ret = vmdk_perform_cow(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,
-offset, skip_start_bytes, skip_end_bytes);
-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];
-}
+return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
}
+
*cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
return VMDK_OK;
}
@@ -1595,9 +1564,7 @@ static int64_t coroutine_fn
vmdk_co_get_block_status(BlockDriverState *bs,
return 0;
}
qemu_co_mutex_lock(&s->lock);
-ret = vmdk_get_cluster_offset(bs, extent, NULL,
- sector_num * 512, false, &offset,
- 0, 0);
+ret = vmdk_get_cluster_offset(bs, extent, sector_num * 512, &offset);
qemu_co_mutex_unlock(&s->lock);
index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
@@ -1788,13 +1755,14 @@ vmdk_co_preadv(BlockDriverState *bs, uint64_t offset,
uint64_t bytes,
ret = -EIO;
goto fail;
}
-ret = vmdk_get_cluster_offset(bs, extent, NULL,
- offset, false, &cluster_offset, 0, 0);
+
offset_in_cluster = vmdk_find_offset_in_cluster(extent, offset);
n_bytes = MIN(bytes, extent->cluster_sectors * BDRV_SECTOR_SIZE
- offset_in_cluster);
+ret = vmdk_get_cluster_offset(bs, extent, offset, &cluster_offset);
+
if (ret != VMDK_OK) {
/* if not allocated, try to read from parent image, if exist */
if (bs->backing && ret != VMDK_ZEROED) {
@@ -2541,9 +2509,9 @@ static int vmdk_check(BlockDriverState *bs,
BdrvCheckResult *result,
sector_num);
break;
}
-ret = vmdk_get_cluster_offset(bs, extent, NULL,
+ret = vmdk_get_cluster_offset(bs, extent,
sector_num << BDRV_SECTOR_BITS,
- false, &cluster_offset, 0, 0);
+ &cluster_offset);
if (ret == VMDK_ERROR) {
Re: [Qemu-block] [Qemu-devel] [PATCH v2 06/16] backup: Request BLK_PERM_AIO_CONTEXT_CHANGE on target
On Thu, 06/01 14:26, Stefan Hajnoczi wrote:
> On Wed, May 31, 2017 at 05:57:46PM +0800, Fam Zheng wrote:
> > On Wed, 05/31 10:39, Stefan Hajnoczi wrote:
> > > On Wed, May 24, 2017 at 10:18:44AM +0800, Fam Zheng wrote:
> > > > On Thu, 05/11 15:41, Stefan Hajnoczi wrote:
> > > > > On Wed, Apr 19, 2017 at 05:43:46PM +0800, Fam Zheng wrote:
> > > > > > What's done in the source's context change notifier is moving the
> > > > > > target's context to follow the new one, so we request this
> > > > > > permission
> > > > > > here.
> > > > >
> > > > > It's true that the backup block job must be able to set target's
> > > > > AioContext, but does this change also allow other users to set
> > > > > target's
> > > > > AioContext while the backup job is running? If yes, then we need to
> > > > > handle that.
> > > >
> > > > If through job->target, yes, but I don't think there is any user of
> > > > job->target.
> > > > Otherwise, it's not allowed, because the second parameter of blk_new
> > > > doesn't
> > > > have BLK_PERM_AIO_CONTEXT_CHANGE.
> > > >
> > > > So it's okay.
> > >
> > > What about blockdev-backup? It allows the user to specify 'target'.
> > > Therefore the user can also run other monitor commands on target. Some
> > > of them could change the AioContext and the backup job wouldn't know!
> >
> > That will be rejected.
> >
> > The contract is that any code that wants to change the AioContext of a BDS,
> > in
> > this case the "target BDS", must do this:
> >
> > 1) create its own BB with perm.BLK_PERM_AIO_CONTEXT_CHANGE
> >
> > 2) attach BDS to this BB
> >
> > 3) call blk_set_aio_context and change the AioContext
> >
> > This is basically how all users of a BDS coordinate through Kevin's new op
> > blocker API, and in your concerned case, when a user runs a second monitor
> > command that changes AioContext, step 2 will fail, because as in this
> > patch, the
> > first job->target BB didn't set shared_perm.BLK_PERM_AIO_CONTEXT_CHANGE.
>
> I was wondering how that works since do_blockdev_backup() does not use
> BB to access target, but it does check whether a BB is already attached:
>
> target_bs = bdrv_lookup_bs(backup->target, backup->target, errp);
> if (!target_bs) {
> goto out;
> }
>
> if (bdrv_get_aio_context(target_bs) != aio_context) {
> if (!bdrv_has_blk(target_bs)) { <- fails when job is running
> /* The target BDS is not attached, we can safely move it to
> another
> * AioContext. */
> bdrv_set_aio_context(target_bs, aio_context);
> } else {
> error_setg(errp, "Target is attached to a different thread from "
> "source.");
> goto out;
> }
> }
Yeah, this is the current way (before this series), and is incomplete in some
cases but too strict in others, for obvious reasons. It is changed to always
create a BB in patch 7.
Fam
[Qemu-block] [PATCH v1 0/1] qemu/migration: fix the migration bug found by qemu-iotests case 068
Hi all, This patch is to fix the migration bug found by qemu-iotests case 068 and based on upstream master's commit: cb8b8ef4578: Merge remote-tracking branch 'remotes/elmarco/tags/chrfe-pull-request' into staging. The bug was introduced by commit "660819b migration: shut src return path unconditionally". Thanks! QingFeng Hao (1): qemu/migration: fix the double free problem on from_src_file migration/savevm.c | 1 - 1 file changed, 1 deletion(-) -- 2.11.2
[Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
In load_vmstate, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) (
if [ -n "${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
migration_incoming_state_destroy();
--
2.11.2
Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
* QingFeng Hao (ha...@linux.vnet.ibm.com) wrote:
> In load_vmstate, mis->from_src_file is freed twice, the first free is by
> qemu_fclose, the second is by migration_incoming_state_destroy and
> it causes Illegal instruction exception. The fix is just to remove the
> first free.
>
> This problem is found by qemu-iotests case 068 since commit
> "660819b migration: shut src return path unconditionally". The error is:
> 068 1s ... - output mismatch (see 068.out.bad)
> --- tests/qemu-iotests/068.out2017-05-06 01:00:26.417270437 +0200
> +++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200
> @@ -6,6 +6,8 @@
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm 0
> (qemu) quit
> +./common.config: line 107: 242472 Illegal instruction (core dumped)
> ( if [ -n "${QEMU_NEED_PID}" ]; then
> +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) quit
> -*** done
> +(qemu) *** done
>
> Signed-off-by: QingFeng Hao
> ---
> migration/savevm.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9c320f59d0..853e14e34e 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
>
> aio_context_acquire(aio_context);
> ret = qemu_loadvm_state(f);
> -qemu_fclose(f);
> aio_context_release(aio_context);
Thanks!
Reviewed-by: Dr. David Alan Gilbert
>
> migration_incoming_state_destroy();
> --
> 2.11.2
>
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[Qemu-block] [PATCH] block/qcow.c: Fix memory leak in qcow_create()
Coverity points out that the code path in qcow_create() for
the magic "fat:" backing file name leaks the memory used to
store the filename (CID 1307771). Free the memory before
we overwrite the pointer.
Signed-off-by: Peter Maydell
---
block/qcow.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/qcow.c b/block/qcow.c
index 95ab123..7bd94dc 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -852,6 +852,7 @@ static int qcow_create(const char *filename, QemuOpts
*opts, Error **errp)
header_size += backing_filename_len;
} else {
/* special backing file for vvfat */
+g_free(backing_file);
backing_file = NULL;
}
header.cluster_bits = 9; /* 512 byte cluster to avoid copying
--
2.7.4
Re: [Qemu-block] [Qemu-devel] [PATCH] block/qcow.c: Fix memory leak in qcow_create()
On 06/05/2017 08:55 AM, Peter Maydell wrote:
> Coverity points out that the code path in qcow_create() for
> the magic "fat:" backing file name leaks the memory used to
> store the filename (CID 1307771). Free the memory before
> we overwrite the pointer.
>
> Signed-off-by: Peter Maydell
> ---
> block/qcow.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Eric Blake
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 95ab123..7bd94dc 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -852,6 +852,7 @@ static int qcow_create(const char *filename, QemuOpts
> *opts, Error **errp)
> header_size += backing_filename_len;
> } else {
> /* special backing file for vvfat */
> +g_free(backing_file);
> backing_file = NULL;
> }
> header.cluster_bits = 9; /* 512 byte cluster to avoid copying
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH] block/qcow.c: Fix memory leak in qcow_create()
On 06/05/2017 10:55 AM, Peter Maydell wrote:
Coverity points out that the code path in qcow_create() for
the magic "fat:" backing file name leaks the memory used to
store the filename (CID 1307771). Free the memory before
we overwrite the pointer.
Signed-off-by: Peter Maydell
Reviewed-by: Philippe Mathieu-Daudé
---
block/qcow.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/qcow.c b/block/qcow.c
index 95ab123..7bd94dc 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -852,6 +852,7 @@ static int qcow_create(const char *filename, QemuOpts
*opts, Error **errp)
header_size += backing_filename_len;
} else {
/* special backing file for vvfat */
+g_free(backing_file);
backing_file = NULL;
}
header.cluster_bits = 9; /* 512 byte cluster to avoid copying
[Qemu-block] [PATCH] block/gluster.c: Handle qdict_array_entries() failure
In qemu_gluster_parse_json(), the call to qdict_array_entries() could return a negative error code, which we were ignoring because we assigned the result to an unsigned variable. Fix this by using the 'int' type instead, which matches the return type of qdict_array_entries() and also the type we use for the loop enumeration variable 'i'. (Spotted by Coverity, CID 1360960.) Signed-off-by: Peter Maydell --- block/gluster.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 031596a..addceed 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -493,8 +493,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, Error *local_err = NULL; char *str = NULL; const char *ptr; -size_t num_servers; -int i, type; +int i, type, num_servers; /* create opts info from runtime_json_opts list */ opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); -- 2.7.4
Re: [Qemu-block] [Qemu-devel] [PATCH] block/gluster.c: Handle qdict_array_entries() failure
On 06/05/2017 12:01 PM, Peter Maydell wrote: > In qemu_gluster_parse_json(), the call to qdict_array_entries() > could return a negative error code, which we were ignoring > because we assigned the result to an unsigned variable. > Fix this by using the 'int' type instead, which matches the > return type of qdict_array_entries() and also the type > we use for the loop enumeration variable 'i'. > > (Spotted by Coverity, CID 1360960.) > > Signed-off-by: Peter Maydell > --- > block/gluster.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-block] [PATCH v3 1/4] qemu-io: Don't die on second open
Most callback commands in qemu-io return 0 to keep the interpreter
loop running, or 1 to quit immediately. However, open_f() just
passed through the return value of openfile(), which has different
semantics of returning 0 if a file was opened, or 1 on any failure.
As a result of mixing the return semantics, we are forcing the
qemu-io interpreter to exit early on any failures, which is rather
annoying when some of the failures are obviously trying to give
the user a hint of how to proceed (if we didn't then kill qemu-io
out from under the user's feet):
$ qemu-io
qemu-io> open foo
qemu-io> open foo
file open already, try 'help close'
$ echo $?
0
In general, we WANT openfile() to report failures, since it is the
function used in the form 'qemu-io -c "$something" no_such_file'
for performing one or more -c options on a single file, and it is
not worth attempting $something if the file itself cannot be opened.
So the solution is to fix open_f() to always return 0 (when we are
in interactive mode, even failure to open should not end the
session), and save the return value of openfile() for command line
use in main().
Note, however, that we do have some qemu-iotests that do 'qemu-io
-c "open file" -c "$something"'; such tests will now proceed to
attempt $something whether or not the open succeeded, the same way
as if the two commands had been attempted in interactive mode; but it
also means that it is now possible to use -c close and have a single
qemu-io command line operate on more than one file even without
using interactive mode. Although the '-c open' action is a subtle
change in behavior, remember that qemu-io is for debugging purposes,
so as long as it serves the needs of qemu-iotests while still being
reasonable for interactive use, it should not be a problem.
This has been awkward since at least as far back as commit
e3aff4f, in 2009.
Signed-off-by: Eric Blake
Reviewed-by: Fam Zheng
---
v3: tweak commit message to distinguish that '-c open' has a subtle
new behavior from the command line
v2: fix open_f(), not openfile()
---
qemu-io.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/qemu-io.c b/qemu-io.c
index 8e38b28..8074656 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -230,13 +230,14 @@ static int open_f(BlockBackend *blk, int argc, char
**argv)
qemu_opts_reset(&empty_opts);
if (optind == argc - 1) {
-return openfile(argv[optind], flags, writethrough, force_share, opts);
+openfile(argv[optind], flags, writethrough, force_share, opts);
} else if (optind == argc) {
-return openfile(NULL, flags, writethrough, force_share, opts);
+openfile(NULL, flags, writethrough, force_share, opts);
} else {
QDECREF(opts);
-return qemuio_command_usage(&open_cmd);
+qemuio_command_usage(&open_cmd);
}
+return 0;
}
static int quit_f(BlockBackend *blk, int argc, char **argv)
--
2.9.4
[Qemu-block] [PATCH v3 0/4] more blkdebug tweaks
I found a crasher and some odd behavior while rebasing my
bdrv_get_block_status series, so I figured I'd get these things
fixed first. This is based on top of Max's block branch.
Available as a tag at:
git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v3
Since v2:
- defer the original patch 3 to a later series
- rebase to make sure test 177 passes at all times (I didn't
pinpoint which recent merge caused it, but the test now produces
json:{...} instead of blkdebug:... during patch 2)
- add more documentation of BDRV_BLOCK_RAW
- add R-b where appropriate
001/4:[] [--] 'qemu-io: Don't die on second open'
002/4:[0002] [FC] 'block: Guarantee that *file is set on
bdrv_get_block_status()'
003/4:[0006] [FC] 'block: Simplify use of BDRV_BLOCK_RAW'
004/4:[0002] [FC] 'blkdebug: Support .bdrv_co_get_block_status'
Eric Blake (4):
qemu-io: Don't die on second open
block: Guarantee that *file is set on bdrv_get_block_status()
block: Simplify use of BDRV_BLOCK_RAW
blkdebug: Support .bdrv_co_get_block_status
include/block/block.h | 6 +++---
block/blkdebug.c | 11 +++
block/commit.c | 2 +-
block/io.c | 5 +++--
block/mirror.c | 2 +-
block/raw-format.c | 2 +-
block/vpc.c| 2 +-
qemu-io.c | 7 ---
tests/qemu-iotests/177 | 3 +++
tests/qemu-iotests/177.out | 5 +
10 files changed, 33 insertions(+), 12 deletions(-)
--
2.9.4
[Qemu-block] [PATCH v3 4/4] blkdebug: Support .bdrv_co_get_block_status
Without a passthrough status of BDRV_BLOCK_RAW, anything wrapped by
blkdebug appears 100% allocated as data. Better is treating it the
same as the underlying file being wrapped.
Update iotest 177 for the new expected output.
Signed-off-by: Eric Blake
Reviewed-by: Fam Zheng
Reviewed-by: Max Reitz
---
v3: rebase to earlier changes
v2: tweak commit message
---
block/blkdebug.c | 11 +++
tests/qemu-iotests/177.out | 5 -
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index a5196e8..1ad8d65 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -642,6 +642,16 @@ static int coroutine_fn
blkdebug_co_pdiscard(BlockDriverState *bs,
return bdrv_co_pdiscard(bs->file->bs, offset, count);
}
+static int64_t coroutine_fn blkdebug_co_get_block_status(
+BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
+BlockDriverState **file)
+{
+*pnum = nb_sectors;
+*file = bs->file->bs;
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
+(sector_num << BDRV_SECTOR_BITS);
+}
+
static void blkdebug_close(BlockDriverState *bs)
{
BDRVBlkdebugState *s = bs->opaque;
@@ -912,6 +922,7 @@ static BlockDriver bdrv_blkdebug = {
.bdrv_co_flush_to_disk = blkdebug_co_flush,
.bdrv_co_pwrite_zeroes = blkdebug_co_pwrite_zeroes,
.bdrv_co_pdiscard = blkdebug_co_pdiscard,
+.bdrv_co_get_block_status = blkdebug_co_get_block_status,
.bdrv_debug_event = blkdebug_debug_event,
.bdrv_debug_breakpoint = blkdebug_debug_breakpoint,
diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
index fcfbfa3..43a7778 100644
--- a/tests/qemu-iotests/177.out
+++ b/tests/qemu-iotests/177.out
@@ -46,6 +46,9 @@ read 30408704/30408704 bytes at offset 80740352
read 23068672/23068672 bytes at offset 49056
22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
Offset Length File
-0 0x800 json:{"image": {"driver": "IMGFMT", "file":
{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug",
"align": "4k"}
+0 0x80TEST_DIR/t.IMGFMT
+0x900x240 TEST_DIR/t.IMGFMT
+0x3c0 0x110 TEST_DIR/t.IMGFMT
+0x6a0 0x160 TEST_DIR/t.IMGFMT
No errors were found on the image.
*** done
--
2.9.4
[Qemu-block] [PATCH v3 2/4] block: Guarantee that *file is set on bdrv_get_block_status()
We document that *file is valid if the return is not an error and
includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract
when a driver (such as blkdebug) lacks a callback. Messed up in
commit 67a0fd2 (v2.6), when we added the file parameter.
Enhance qemu-iotest 177 to cover this, using a sequence that would
print garbage or even SEGV, because it was dererefencing through
uninitialized memory. [The resulting test output shows that we
have less-than-ideal block status from the blkdebug driver, but
that's a separate fix coming up soon.]
Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is
enough to fix the crash, but we can go one step further: always
setting *file, even on error, means that a broken caller that
blindly dereferences file without checking for error is now more
likely to get a reliable SEGV instead of randomly acting on garbage,
making it easier to diagnose such buggy callers. Adding an
assertion that file is set where expected doesn't hurt either.
CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake
Reviewed-by: Fam Zheng
Reviewed-by: Max Reitz
---
v3: tweak commit message [Max], rebase test to pass
v2: drop redundant assignment
---
block/io.c | 5 +++--
tests/qemu-iotests/177 | 3 +++
tests/qemu-iotests/177.out | 2 ++
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/block/io.c b/block/io.c
index ed31810..ce6cb0c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1736,6 +1736,7 @@ static int64_t coroutine_fn
bdrv_co_get_block_status(BlockDriverState *bs,
int64_t n;
int64_t ret, ret2;
+*file = NULL;
total_sectors = bdrv_nb_sectors(bs);
if (total_sectors < 0) {
return total_sectors;
@@ -1756,11 +1757,11 @@ static int64_t coroutine_fn
bdrv_co_get_block_status(BlockDriverState *bs,
ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
if (bs->drv->protocol_name) {
ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
+*file = bs;
}
return ret;
}
-*file = NULL;
bdrv_inc_in_flight(bs);
ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
file);
@@ -1770,7 +1771,7 @@ static int64_t coroutine_fn
bdrv_co_get_block_status(BlockDriverState *bs,
}
if (ret & BDRV_BLOCK_RAW) {
-assert(ret & BDRV_BLOCK_OFFSET_VALID);
+assert(ret & BDRV_BLOCK_OFFSET_VALID && *file);
ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
*pnum, pnum, file);
goto out;
diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177
index 2005c17..f8ed8fb 100755
--- a/tests/qemu-iotests/177
+++ b/tests/qemu-iotests/177
@@ -43,6 +43,7 @@ _supported_proto file
CLUSTER_SIZE=1M
size=128M
options=driver=blkdebug,image.driver=qcow2
+nested_opts=image.file.driver=file,image.file.filename=$TEST_IMG
echo
echo "== setting up files =="
@@ -106,6 +107,8 @@ function verify_io()
}
verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG map --image-opts "$options,$nested_opts,align=4k" \
+| _filter_qemu_img_map
_check_test_img
diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
index e887542..fcfbfa3 100644
--- a/tests/qemu-iotests/177.out
+++ b/tests/qemu-iotests/177.out
@@ -45,5 +45,7 @@ read 30408704/30408704 bytes at offset 80740352
29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 23068672/23068672 bytes at offset 49056
22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0 0x800 json:{"image": {"driver": "IMGFMT", "file":
{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug",
"align": "4k"}
No errors were found on the image.
*** done
--
2.9.4
[Qemu-block] [PATCH v3 3/4] block: Simplify use of BDRV_BLOCK_RAW
The lone caller that cares about a return of BDRV_BLOCK_RAW
(namely, io.c:bdrv_co_get_block_status) completely replaces the
return value, so there is no point in passing BDRV_BLOCK_DATA.
Signed-off-by: Eric Blake
---
v3: further document BDRV_BLOCK_RAW
v2: fix subject, tweak commit message
---
include/block/block.h | 6 +++---
block/commit.c| 2 +-
block/mirror.c| 2 +-
block/raw-format.c| 2 +-
block/vpc.c | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e9..0a60444 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -131,9 +131,9 @@ typedef struct HDGeometry {
* layer (short for DATA || ZERO), set by block layer
*
* Internal flag:
- * BDRV_BLOCK_RAW: used internally to indicate that the request was
- * answered by a passthrough driver such as raw and that the
- * block layer should recompute the answer from bs->file.
+ * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request
+ * that the block layer recompute the answer from the returned
+ * BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
*
* If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK)
* represent the offset in the returned BDS that is allocated for the
diff --git a/block/commit.c b/block/commit.c
index a3028b2..f56745b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -239,7 +239,7 @@ static int64_t coroutine_fn
bdrv_commit_top_get_block_status(
{
*pnum = nb_sectors;
*file = bs->backing->bs;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
}
diff --git a/block/mirror.c b/block/mirror.c
index a2a9703..892d53a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1052,7 +1052,7 @@ static int64_t coroutine_fn
bdrv_mirror_top_get_block_status(
{
*pnum = nb_sectors;
*file = bs->backing->bs;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
}
diff --git a/block/raw-format.c b/block/raw-format.c
index 36e6503..1136eba 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -259,7 +259,7 @@ static int64_t coroutine_fn
raw_co_get_block_status(BlockDriverState *bs,
*pnum = nb_sectors;
*file = bs->file->bs;
sector_num += s->offset / BDRV_SECTOR_SIZE;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
}
diff --git a/block/vpc.c b/block/vpc.c
index 4240ba9..b313c68 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -701,7 +701,7 @@ static int64_t coroutine_fn
vpc_co_get_block_status(BlockDriverState *bs,
if (be32_to_cpu(footer->type) == VHD_FIXED) {
*pnum = nb_sectors;
*file = bs->file->bs;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
}
--
2.9.4
Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/4] more blkdebug tweaks
Hi, This series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. Message-id: 20170605190824.25184-1-ebl...@redhat.com Subject: [Qemu-devel] [PATCH v3 0/4] more blkdebug tweaks Type: series === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-quick@centos6 time make docker-test-mingw@fedora time make docker-test-build@min-glib === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20170605190824.25184-1-ebl...@redhat.com -> patchew/20170605190824.25184-1-ebl...@redhat.com Switched to a new branch 'test' a760ce2 blkdebug: Support .bdrv_co_get_block_status adfab47 block: Simplify use of BDRV_BLOCK_RAW 7b4fbb6 block: Guarantee that *file is set on bdrv_get_block_status() b71a302 qemu-io: Don't die on second open === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-ie13aiac/src/dtc'... Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d' BUILD centos6 make[1]: Entering directory '/var/tmp/patchew-tester-tmp-ie13aiac/src' ARCHIVE qemu.tgz ARCHIVE dtc.tgz COPYRUNNER RUN test-quick in qemu:centos6 Packages installed: SDL-devel-1.2.14-7.el6_7.1.x86_64 ccache-3.1.6-2.el6.x86_64 epel-release-6-8.noarch gcc-4.4.7-17.el6.x86_64 git-1.7.1-4.el6_7.1.x86_64 glib2-devel-2.28.8-5.el6.x86_64 libfdt-devel-1.4.0-1.el6.x86_64 make-3.81-23.el6.x86_64 package g++ is not installed pixman-devel-0.32.8-1.el6.x86_64 tar-1.23-15.el6_8.x86_64 zlib-devel-1.2.3-29.el6.x86_64 Environment variables: PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel glib2-devel SDL-devel pixman-devel epel-release HOSTNAME=cd560983e1cf TERM=xterm MAKEFLAGS= -j8 HISTSIZE=1000 J=8 USER=root CCACHE_DIR=/var/tmp/ccache EXTRA_CONFIGURE_OPTS= V= SHOW_ENV=1 MAIL=/var/spool/mail/root PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin PWD=/ LANG=en_US.UTF-8 TARGET_LIST= HISTCONTROL=ignoredups SHLVL=1 HOME=/root TEST_DIR=/tmp/qemu-test LOGNAME=root LESSOPEN=||/usr/bin/lesspipe.sh %s FEATURES= dtc DEBUG= G_BROKEN_FILENAMES=1 CCACHE_HASHDIR= _=/usr/bin/env Configure options: --enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/var/tmp/qemu-build/install No C++ compiler available; disabling C++ specific optional code Install prefix/var/tmp/qemu-build/install BIOS directory/var/tmp/qemu-build/install/share/qemu binary directory /var/tmp/qemu-build/install/bin library directory /var/tmp/qemu-build/install/lib module directory /var/tmp/qemu-build/install/lib/qemu libexec directory /var/tmp/qemu-build/install/libexec include directory /var/tmp/qemu-build/install/include config directory /var/tmp/qemu-build/install/etc local state directory /var/tmp/qemu-build/install/var Manual directory /var/tmp/qemu-build/install/share/man ELF interp prefix /usr/gnemul/qemu-%M Source path /tmp/qemu-test/src C compilercc Host C compiler cc C++ compiler Objective-C compiler cc ARFLAGS rv CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g QEMU_CFLAGS -I/usr/include/pixman-1 -I$(SRC_PATH)/dtc/libfdt -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all LDFLAGS -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g make make install install pythonpython -B smbd /usr/sbin/smbd module supportno host CPU x86_64 host big endian no target list x86_64-softmmu aarch64-softmmu tcg debug enabled no gprof enabled no sparse enabledno strip binariesyes profiler no static build no pixmansystem SDL support yes (1.2.14) GTK support no GTK GL supportno VTE support no TLS priority NORMAL GNUTLS supportno GNUTLS rndno libgcrypt no libgcrypt kdf no nettleno nettle kdfno libtasn1 no curses supportno virgl support no curl support no mingw32 support no Audio drivers oss Block whitelist (rw) Block whitelist (ro) VirtFS supportno VNC support yes VNC SASL support no VNC JPEG support no VNC PNG support no xen support no brlapi supportno
Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/4] more blkdebug tweaks
On 06/05/2017 02:31 PM, no-re...@patchew.org wrote: > Hi, > > This series failed automatic build test. Please find the testing commands and > their output below. If you have docker installed, you can probably reproduce > it > locally. > > GTESTER tests/test-blockjob > GTESTER tests/test-blockjob-txn > GTESTER tests/test-x86-cpuid > GTESTER tests/test-xbzrle > GTESTER tests/test-vmstate > Failed to load simple/primitive:b_1 > Failed to load simple/primitive:i64_2 > Failed to load simple/primitive:i32_1 > Failed to load simple/primitive:i32_1 > Failed to load test/with_tmp:a > Failed to load test/tmp_child_parent:f > Failed to load test/tmp_child:parent > Failed to load test/with_tmp:tmp > Failed to load test/tmp_child:diff > Failed to load test/with_tmp:tmp > Failed to load test/tmp_child:diff > Failed to load test/with_tmp:tmp > GTESTER tests/test-cutils Where is this noise coming from? > GTESTER tests/test-qmp-commands > GTESTER tests/test-string-input-visitor > GTESTER tests/test-string-output-visitor > GTESTER tests/test-qmp-event > GTESTER tests/test-opts-visitor > GTESTER tests/test-visitor-serialization > GTESTER tests/test-qht-par > ** > ERROR:/tmp/qemu-test/src/tests/test-qga.c:894:test_qga_guest_exec: assertion > failed: (exited) > socket_accept failed: Resource temporarily unavailable Looks like a false negative, or a problem with the tester? I don't see how my patch could affect test-qga. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/20] make bdrv_is_allocated[_above] byte-based
ping On 05/10/2017 09:20 PM, Eric Blake wrote: > There are patches floating around to add NBD_CMD_BLOCK_STATUS, > but NBD wants to report status on byte granularity (even if the > reporting will probably be naturally aligned to sectors or even > much higher levels). I've therefore started the task of > converting our block status code to report at a byte granularity > rather than sectors. > > This is part one of that conversion: bdrv_is_allocated(). > Other parts still need a v2, but here's the link to their v1: > tracking dirty bitmaps by bytes: > https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02163.html > replacing bdrv_get_block_status() with a byte based callback > in all the drivers: > https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02642.html > > Available as a tag at: > git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-allocated-v2 > > Since v1: > - Add R-b from John as appropriate > - Couple of new patches for cleanups he noticed > - Rebase to Max's block branch > > 001/20:[] [--] 'blockjob: Track job ratelimits via bytes, not sectors' > 002/20:[] [--] 'trace: Show blockjob actions via bytes, not sectors' > 003/20:[] [--] 'stream: Switch stream_populate() to byte-based' > 004/20:[] [--] 'stream: Switch stream_run() to byte-based' > 005/20:[] [--] 'commit: Switch commit_populate() to byte-based' > 006/20:[] [--] 'commit: Switch commit_run() to byte-based' > 007/20:[] [--] 'mirror: Switch MirrorBlockJob to byte-based' > 008/20:[] [--] 'mirror: Switch mirror_do_zero_or_discard() to byte-based' > 009/20:[down] 'mirror: Update signature of mirror_clip_sectors()' > 010/20:[0013] [FC] 'mirror: Switch mirror_cow_align() to byte-based' > 011/20:[] [-C] 'mirror: Switch mirror_do_read() to byte-based' > 012/20:[0014] [FC] 'mirror: Switch mirror_iteration() to byte-based' > 013/20:[] [--] 'block: Drop unused bdrv_round_sectors_to_clusters()' > 014/20:[] [--] 'backup: Switch BackupBlockJob to byte-based' > 015/20:[0008] [FC] 'backup: Switch block_backup.h to byte-based' > 016/20:[] [--] 'backup: Switch backup_do_cow() to byte-based' > 017/20:[] [--] 'backup: Switch backup_run() to byte-based' > 018/20:[0039] [FC] 'block: Make bdrv_is_allocated() byte-based' > 019/20:[down] 'block: Minimize raw use of bds->total_sectors' > 020/20:[0026] [FC] 'block: Make bdrv_is_allocated_above() byte-based' > > Eric Blake (20): > blockjob: Track job ratelimits via bytes, not sectors > trace: Show blockjob actions via bytes, not sectors > stream: Switch stream_populate() to byte-based > stream: Switch stream_run() to byte-based > commit: Switch commit_populate() to byte-based > commit: Switch commit_run() to byte-based > mirror: Switch MirrorBlockJob to byte-based > mirror: Switch mirror_do_zero_or_discard() to byte-based > mirror: Update signature of mirror_clip_sectors() > mirror: Switch mirror_cow_align() to byte-based > mirror: Switch mirror_do_read() to byte-based > mirror: Switch mirror_iteration() to byte-based > block: Drop unused bdrv_round_sectors_to_clusters() > backup: Switch BackupBlockJob to byte-based > backup: Switch block_backup.h to byte-based > backup: Switch backup_do_cow() to byte-based > backup: Switch backup_run() to byte-based > block: Make bdrv_is_allocated() byte-based > block: Minimize raw use of bds->total_sectors > block: Make bdrv_is_allocated_above() byte-based > > include/block/block.h| 10 +- > include/block/block_backup.h | 11 +- > include/qemu/ratelimit.h | 3 +- > block/backup.c | 130 --- > block/commit.c | 54 > block/io.c | 92 +++-- > block/mirror.c | 300 > ++- > block/replication.c | 29 +++-- > block/stream.c | 35 +++-- > block/vvfat.c| 34 +++-- > migration/block.c| 9 +- > qemu-img.c | 15 ++- > qemu-io-cmds.c | 70 +- > block/trace-events | 14 +- > 14 files changed, 396 insertions(+), 410 deletions(-) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/20] make bdrv_is_allocated[_above] byte-based
On 06/05/2017 03:39 PM, Eric Blake wrote: > ping > ACK. > On 05/10/2017 09:20 PM, Eric Blake wrote: >> There are patches floating around to add NBD_CMD_BLOCK_STATUS, >> but NBD wants to report status on byte granularity (even if the >> reporting will probably be naturally aligned to sectors or even >> much higher levels). I've therefore started the task of >> converting our block status code to report at a byte granularity >> rather than sectors. >> >> This is part one of that conversion: bdrv_is_allocated(). >> Other parts still need a v2, but here's the link to their v1: >> tracking dirty bitmaps by bytes: >> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02163.html >> replacing bdrv_get_block_status() with a byte based callback >> in all the drivers: >> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02642.html >> >> Available as a tag at: >> git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-allocated-v2 >> >> Since v1: >> - Add R-b from John as appropriate >> - Couple of new patches for cleanups he noticed >> - Rebase to Max's block branch >> >> 001/20:[] [--] 'blockjob: Track job ratelimits via bytes, not sectors' >> 002/20:[] [--] 'trace: Show blockjob actions via bytes, not sectors' >> 003/20:[] [--] 'stream: Switch stream_populate() to byte-based' >> 004/20:[] [--] 'stream: Switch stream_run() to byte-based' >> 005/20:[] [--] 'commit: Switch commit_populate() to byte-based' >> 006/20:[] [--] 'commit: Switch commit_run() to byte-based' >> 007/20:[] [--] 'mirror: Switch MirrorBlockJob to byte-based' >> 008/20:[] [--] 'mirror: Switch mirror_do_zero_or_discard() to byte-based' >> 009/20:[down] 'mirror: Update signature of mirror_clip_sectors()' >> 010/20:[0013] [FC] 'mirror: Switch mirror_cow_align() to byte-based' >> 011/20:[] [-C] 'mirror: Switch mirror_do_read() to byte-based' >> 012/20:[0014] [FC] 'mirror: Switch mirror_iteration() to byte-based' >> 013/20:[] [--] 'block: Drop unused bdrv_round_sectors_to_clusters()' >> 014/20:[] [--] 'backup: Switch BackupBlockJob to byte-based' >> 015/20:[0008] [FC] 'backup: Switch block_backup.h to byte-based' >> 016/20:[] [--] 'backup: Switch backup_do_cow() to byte-based' >> 017/20:[] [--] 'backup: Switch backup_run() to byte-based' >> 018/20:[0039] [FC] 'block: Make bdrv_is_allocated() byte-based' >> 019/20:[down] 'block: Minimize raw use of bds->total_sectors' >> 020/20:[0026] [FC] 'block: Make bdrv_is_allocated_above() byte-based' >> >> Eric Blake (20): >> blockjob: Track job ratelimits via bytes, not sectors >> trace: Show blockjob actions via bytes, not sectors >> stream: Switch stream_populate() to byte-based >> stream: Switch stream_run() to byte-based >> commit: Switch commit_populate() to byte-based >> commit: Switch commit_run() to byte-based >> mirror: Switch MirrorBlockJob to byte-based >> mirror: Switch mirror_do_zero_or_discard() to byte-based >> mirror: Update signature of mirror_clip_sectors() >> mirror: Switch mirror_cow_align() to byte-based >> mirror: Switch mirror_do_read() to byte-based >> mirror: Switch mirror_iteration() to byte-based >> block: Drop unused bdrv_round_sectors_to_clusters() >> backup: Switch BackupBlockJob to byte-based >> backup: Switch block_backup.h to byte-based >> backup: Switch backup_do_cow() to byte-based >> backup: Switch backup_run() to byte-based >> block: Make bdrv_is_allocated() byte-based >> block: Minimize raw use of bds->total_sectors >> block: Make bdrv_is_allocated_above() byte-based >> >> include/block/block.h| 10 +- >> include/block/block_backup.h | 11 +- >> include/qemu/ratelimit.h | 3 +- >> block/backup.c | 130 --- >> block/commit.c | 54 >> block/io.c | 92 +++-- >> block/mirror.c | 300 >> ++- >> block/replication.c | 29 +++-- >> block/stream.c | 35 +++-- >> block/vvfat.c| 34 +++-- >> migration/block.c| 9 +- >> qemu-img.c | 15 ++- >> qemu-io-cmds.c | 70 +- >> block/trace-events | 14 +- >> 14 files changed, 396 insertions(+), 410 deletions(-) >> >
Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/4] qemu-io: Don't die on second open
On 06/05/2017 02:08 PM, Eric Blake wrote:
>
> Note, however, that we do have some qemu-iotests that do 'qemu-io
> -c "open file" -c "$something"'; such tests will now proceed to
> attempt $something whether or not the open succeeded, the same way
> as if the two commands had been attempted in interactive mode; but it
> also means that it is now possible to use -c close and have a single
> qemu-io command line operate on more than one file even without
> using interactive mode. Although the '-c open' action is a subtle
> change in behavior, remember that qemu-io is for debugging purposes,
> so as long as it serves the needs of qemu-iotests while still being
> reasonable for interactive use, it should not be a problem.
Bummer - iotest 60 catches me:
+++ 060.out.bad 2017-06-05 14:55:48.336814834 -0500
@@ -21,6 +21,7 @@
refcount bits: 16
corrupt: true
can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot
be opened read/write
+no file open, try 'help open'
read 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
Looks like 114 and 153 as well. I'll have to post a v4 (serves me right
for JUST testing 177).
Also, I'm seeing a segfault on 68 that exists on current master:
068 1s ... - output mismatch (see 068.out.bad)
--- /home/eblake/qemu/tests/qemu-iotests/068.out2017-06-01
17:01:25.113094957 -0500
+++ 068.out.bad 2017-06-05 14:55:54.140835402 -0500
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 1 Segmentation fault (core dumped)
( if [ -n "${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH] block/gluster.c: Handle qdict_array_entries() failure
On 06/05/2017 02:01 PM, Peter Maydell wrote: In qemu_gluster_parse_json(), the call to qdict_array_entries() could return a negative error code, which we were ignoring because we assigned the result to an unsigned variable. Fix this by using the 'int' type instead, which matches the return type of qdict_array_entries() and also the type we use for the loop enumeration variable 'i'. (Spotted by Coverity, CID 1360960.) Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé --- block/gluster.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 031596a..addceed 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -493,8 +493,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, Error *local_err = NULL; char *str = NULL; const char *ptr; -size_t num_servers; -int i, type; +int i, type, num_servers; /* create opts info from runtime_json_opts list */ opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
Re: [Qemu-block] [PATCH] block/gluster.c: Handle qdict_array_entries() failure
On Mon, Jun 05, 2017 at 06:01:38PM +0100, Peter Maydell wrote: > In qemu_gluster_parse_json(), the call to qdict_array_entries() > could return a negative error code, which we were ignoring > because we assigned the result to an unsigned variable. > Fix this by using the 'int' type instead, which matches the > return type of qdict_array_entries() and also the type > we use for the loop enumeration variable 'i'. > > (Spotted by Coverity, CID 1360960.) > > Signed-off-by: Peter Maydell > --- > block/gluster.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/block/gluster.c b/block/gluster.c > index 031596a..addceed 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -493,8 +493,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster > *gconf, > Error *local_err = NULL; > char *str = NULL; > const char *ptr; > -size_t num_servers; > -int i, type; > +int i, type, num_servers; > > /* create opts info from runtime_json_opts list */ > opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); > -- > 2.7.4 > Thanks, Reviewed-by: Jeff Cody Also - applied to my block branch: git://github.com/codyprime/qemu-kvm-jtc block -Jeff
[Qemu-block] [PATCH v4 0/4] more blkdebug tweaks
I found a crasher and some odd behavior while rebasing my bdrv_get_block_status series, so I figured I'd get these things fixed first. This is based on top of Max's block branch. Available as a tag at: git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4 Since v3: - check all qemu-iotests (patch 1) 001/4:[0012] [FC] 'qemu-io: Don't die on second open' 002/4:[] [--] 'block: Guarantee that *file is set on bdrv_get_block_status()' 003/4:[] [--] 'block: Simplify use of BDRV_BLOCK_RAW' 004/4:[] [--] 'blkdebug: Support .bdrv_co_get_block_status' Eric Blake (4): qemu-io: Don't die on second open block: Guarantee that *file is set on bdrv_get_block_status() block: Simplify use of BDRV_BLOCK_RAW blkdebug: Support .bdrv_co_get_block_status include/block/block.h | 6 +++--- block/blkdebug.c | 11 +++ block/commit.c | 2 +- block/io.c | 5 +++-- block/mirror.c | 2 +- block/raw-format.c | 2 +- block/vpc.c| 2 +- qemu-io.c | 7 --- tests/qemu-iotests/060.out | 1 + tests/qemu-iotests/114.out | 5 +++-- tests/qemu-iotests/153.out | 6 ++ tests/qemu-iotests/177 | 3 +++ tests/qemu-iotests/177.out | 5 + 13 files changed, 43 insertions(+), 14 deletions(-) -- 2.9.4
[Qemu-block] [PATCH v4 1/4] qemu-io: Don't die on second open
Most callback commands in qemu-io return 0 to keep the interpreter
loop running, or 1 to quit immediately. However, open_f() just
passed through the return value of openfile(), which has different
semantics of returning 0 if a file was opened, or 1 on any failure.
As a result of mixing the return semantics, we are forcing the
qemu-io interpreter to exit early on any failures, which is rather
annoying when some of the failures are obviously trying to give
the user a hint of how to proceed (if we didn't then kill qemu-io
out from under the user's feet):
$ qemu-io
qemu-io> open foo
qemu-io> open foo
file open already, try 'help close'
$ echo $?
0
In general, we WANT openfile() to report failures, since it is the
function used in the form 'qemu-io -c "$something" no_such_file'
for performing one or more -c options on a single file, and it is
not worth attempting $something if the file itself cannot be opened.
So the solution is to fix open_f() to always return 0 (when we are
in interactive mode, even failure to open should not end the
session), and save the return value of openfile() for command line
use in main().
Note, however, that we do have some qemu-iotests that do 'qemu-io
-c "open file" -c "$something"'; such tests will now proceed to
attempt $something whether or not the open succeeded, the same way
as if the two commands had been attempted in interactive mode. As
such, the expected output for those tests has to be modified. But it
also means that it is now possible to use -c close and have a single
qemu-io command line operate on more than one file even without
using interactive mode. Although the '-c open' action is a subtle
change in behavior, remember that qemu-io is for debugging purposes,
so as long as it serves the needs of qemu-iotests while still being
reasonable for interactive use, it should not be a problem that we
are changing tests to the new behavior.
This has been awkward since at least as far back as commit
e3aff4f, in 2009.
Signed-off-by: Eric Blake
Reviewed-by: Fam Zheng
---
v4: cover ALL qemu-iotests
v3: tweak commit message to distinguish that '-c open' has a subtle
new behavior from the command line
v2: fix open_f(), not openfile()
---
qemu-io.c | 7 ---
tests/qemu-iotests/060.out | 1 +
tests/qemu-iotests/114.out | 5 +++--
tests/qemu-iotests/153.out | 6 ++
4 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/qemu-io.c b/qemu-io.c
index 8e38b28..8074656 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -230,13 +230,14 @@ static int open_f(BlockBackend *blk, int argc, char
**argv)
qemu_opts_reset(&empty_opts);
if (optind == argc - 1) {
-return openfile(argv[optind], flags, writethrough, force_share, opts);
+openfile(argv[optind], flags, writethrough, force_share, opts);
} else if (optind == argc) {
-return openfile(NULL, flags, writethrough, force_share, opts);
+openfile(NULL, flags, writethrough, force_share, opts);
} else {
QDECREF(opts);
-return qemuio_command_usage(&open_cmd);
+qemuio_command_usage(&open_cmd);
}
+return 0;
}
static int quit_f(BlockBackend *blk, int argc, char **argv)
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 3bc1461..5ca3af4 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -21,6 +21,7 @@ Format specific information:
refcount bits: 16
corrupt: true
can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be
opened read/write
+no file open, try 'help open'
read 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out
index b6d10e4..1a47a52 100644
--- a/tests/qemu-iotests/114.out
+++ b/tests/qemu-iotests/114.out
@@ -1,6 +1,6 @@
QA output created by 114
-Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
backing_file=TEST_DIR/t.IMGFMT.base
image: TEST_DIR/t.IMGFMT
file format: IMGFMT
virtual size: 64M (67108864 bytes)
@@ -8,6 +8,7 @@ cluster_size: 65536
backing file: TEST_DIR/t.IMGFMT.base
backing file format: foo
can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknown
driver 'foo'
+no file open, try 'help open'
read 4096/4096 bytes at offset 0
4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
*** done
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
index 5ba0b63..5b917b1 100644
--- a/tests/qemu-iotests/153.out
+++ b/tests/qemu-iotests/153.out
@@ -33,10 +33,12 @@ Is another process using the image?
_qemu_io_wrapper -c open TEST_DIR/t.qcow2 -c read 0 512
can't open device TEST_DIR/t.qcow2: Failed to get "write" lock
Is another process using the image?
+no file open, tr
[Qemu-block] [PATCH v4 3/4] block: Simplify use of BDRV_BLOCK_RAW
The lone caller that cares about a return of BDRV_BLOCK_RAW
(namely, io.c:bdrv_co_get_block_status) completely replaces the
return value, so there is no point in passing BDRV_BLOCK_DATA.
Signed-off-by: Eric Blake
---
v3: further document BDRV_BLOCK_RAW
v2: fix subject, tweak commit message
---
include/block/block.h | 6 +++---
block/commit.c| 2 +-
block/mirror.c| 2 +-
block/raw-format.c| 2 +-
block/vpc.c | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e9..0a60444 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -131,9 +131,9 @@ typedef struct HDGeometry {
* layer (short for DATA || ZERO), set by block layer
*
* Internal flag:
- * BDRV_BLOCK_RAW: used internally to indicate that the request was
- * answered by a passthrough driver such as raw and that the
- * block layer should recompute the answer from bs->file.
+ * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request
+ * that the block layer recompute the answer from the returned
+ * BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
*
* If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK)
* represent the offset in the returned BDS that is allocated for the
diff --git a/block/commit.c b/block/commit.c
index a3028b2..f56745b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -239,7 +239,7 @@ static int64_t coroutine_fn
bdrv_commit_top_get_block_status(
{
*pnum = nb_sectors;
*file = bs->backing->bs;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
}
diff --git a/block/mirror.c b/block/mirror.c
index a2a9703..892d53a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1052,7 +1052,7 @@ static int64_t coroutine_fn
bdrv_mirror_top_get_block_status(
{
*pnum = nb_sectors;
*file = bs->backing->bs;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
}
diff --git a/block/raw-format.c b/block/raw-format.c
index 36e6503..1136eba 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -259,7 +259,7 @@ static int64_t coroutine_fn
raw_co_get_block_status(BlockDriverState *bs,
*pnum = nb_sectors;
*file = bs->file->bs;
sector_num += s->offset / BDRV_SECTOR_SIZE;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
}
diff --git a/block/vpc.c b/block/vpc.c
index 4240ba9..b313c68 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -701,7 +701,7 @@ static int64_t coroutine_fn
vpc_co_get_block_status(BlockDriverState *bs,
if (be32_to_cpu(footer->type) == VHD_FIXED) {
*pnum = nb_sectors;
*file = bs->file->bs;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
}
--
2.9.4
[Qemu-block] [PATCH v4 2/4] block: Guarantee that *file is set on bdrv_get_block_status()
We document that *file is valid if the return is not an error and
includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract
when a driver (such as blkdebug) lacks a callback. Messed up in
commit 67a0fd2 (v2.6), when we added the file parameter.
Enhance qemu-iotest 177 to cover this, using a sequence that would
print garbage or even SEGV, because it was dererefencing through
uninitialized memory. [The resulting test output shows that we
have less-than-ideal block status from the blkdebug driver, but
that's a separate fix coming up soon.]
Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is
enough to fix the crash, but we can go one step further: always
setting *file, even on error, means that a broken caller that
blindly dereferences file without checking for error is now more
likely to get a reliable SEGV instead of randomly acting on garbage,
making it easier to diagnose such buggy callers. Adding an
assertion that file is set where expected doesn't hurt either.
CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake
Reviewed-by: Fam Zheng
Reviewed-by: Max Reitz
---
v3: tweak commit message [Max], rebase test to pass
v2: drop redundant assignment
---
block/io.c | 5 +++--
tests/qemu-iotests/177 | 3 +++
tests/qemu-iotests/177.out | 2 ++
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/block/io.c b/block/io.c
index ed31810..ce6cb0c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1736,6 +1736,7 @@ static int64_t coroutine_fn
bdrv_co_get_block_status(BlockDriverState *bs,
int64_t n;
int64_t ret, ret2;
+*file = NULL;
total_sectors = bdrv_nb_sectors(bs);
if (total_sectors < 0) {
return total_sectors;
@@ -1756,11 +1757,11 @@ static int64_t coroutine_fn
bdrv_co_get_block_status(BlockDriverState *bs,
ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
if (bs->drv->protocol_name) {
ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
+*file = bs;
}
return ret;
}
-*file = NULL;
bdrv_inc_in_flight(bs);
ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
file);
@@ -1770,7 +1771,7 @@ static int64_t coroutine_fn
bdrv_co_get_block_status(BlockDriverState *bs,
}
if (ret & BDRV_BLOCK_RAW) {
-assert(ret & BDRV_BLOCK_OFFSET_VALID);
+assert(ret & BDRV_BLOCK_OFFSET_VALID && *file);
ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
*pnum, pnum, file);
goto out;
diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177
index 2005c17..f8ed8fb 100755
--- a/tests/qemu-iotests/177
+++ b/tests/qemu-iotests/177
@@ -43,6 +43,7 @@ _supported_proto file
CLUSTER_SIZE=1M
size=128M
options=driver=blkdebug,image.driver=qcow2
+nested_opts=image.file.driver=file,image.file.filename=$TEST_IMG
echo
echo "== setting up files =="
@@ -106,6 +107,8 @@ function verify_io()
}
verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG map --image-opts "$options,$nested_opts,align=4k" \
+| _filter_qemu_img_map
_check_test_img
diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
index e887542..fcfbfa3 100644
--- a/tests/qemu-iotests/177.out
+++ b/tests/qemu-iotests/177.out
@@ -45,5 +45,7 @@ read 30408704/30408704 bytes at offset 80740352
29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 23068672/23068672 bytes at offset 49056
22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0 0x800 json:{"image": {"driver": "IMGFMT", "file":
{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug",
"align": "4k"}
No errors were found on the image.
*** done
--
2.9.4
[Qemu-block] [PATCH v4 4/4] blkdebug: Support .bdrv_co_get_block_status
Without a passthrough status of BDRV_BLOCK_RAW, anything wrapped by
blkdebug appears 100% allocated as data. Better is treating it the
same as the underlying file being wrapped.
Update iotest 177 for the new expected output.
Signed-off-by: Eric Blake
Reviewed-by: Fam Zheng
Reviewed-by: Max Reitz
---
v3: rebase to earlier changes
v2: tweak commit message
---
block/blkdebug.c | 11 +++
tests/qemu-iotests/177.out | 5 -
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index a5196e8..1ad8d65 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -642,6 +642,16 @@ static int coroutine_fn
blkdebug_co_pdiscard(BlockDriverState *bs,
return bdrv_co_pdiscard(bs->file->bs, offset, count);
}
+static int64_t coroutine_fn blkdebug_co_get_block_status(
+BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
+BlockDriverState **file)
+{
+*pnum = nb_sectors;
+*file = bs->file->bs;
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
+(sector_num << BDRV_SECTOR_BITS);
+}
+
static void blkdebug_close(BlockDriverState *bs)
{
BDRVBlkdebugState *s = bs->opaque;
@@ -912,6 +922,7 @@ static BlockDriver bdrv_blkdebug = {
.bdrv_co_flush_to_disk = blkdebug_co_flush,
.bdrv_co_pwrite_zeroes = blkdebug_co_pwrite_zeroes,
.bdrv_co_pdiscard = blkdebug_co_pdiscard,
+.bdrv_co_get_block_status = blkdebug_co_get_block_status,
.bdrv_debug_event = blkdebug_debug_event,
.bdrv_debug_breakpoint = blkdebug_debug_breakpoint,
diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
index fcfbfa3..43a7778 100644
--- a/tests/qemu-iotests/177.out
+++ b/tests/qemu-iotests/177.out
@@ -46,6 +46,9 @@ read 30408704/30408704 bytes at offset 80740352
read 23068672/23068672 bytes at offset 49056
22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
Offset Length File
-0 0x800 json:{"image": {"driver": "IMGFMT", "file":
{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug",
"align": "4k"}
+0 0x80TEST_DIR/t.IMGFMT
+0x900x240 TEST_DIR/t.IMGFMT
+0x3c0 0x110 TEST_DIR/t.IMGFMT
+0x6a0 0x160 TEST_DIR/t.IMGFMT
No errors were found on the image.
*** done
--
2.9.4
Re: [Qemu-block] [Qemu-devel] [PATCH v2 09/20] mirror: Update signature of mirror_clip_sectors()
On 05/10/2017 10:20 PM, Eric Blake wrote: > Rather than having a void function that modifies its input > in-place as the output, change the signature to reduce a layer > of indirection and return the result. > > Suggested-by: John Snow > Signed-off-by: Eric Blake > Reviewed-by: John Snow > --- > v2: new patch > --- > block/mirror.c | 15 --- > 1 file changed, 8 insertions(+), 7 deletions(-) >
Re: [Qemu-block] [Qemu-devel] [PATCH v2 10/20] mirror: Switch mirror_cow_align() to byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > We are gradually converting to byte-based interfaces, as they are > easier to reason about than sector-based. Convert another internal > function (no semantic change), and add mirror_clip_bytes() as a > counterpart to mirror_clip_sectors(). Some of the conversion is > a bit tricky, requiring temporaries to convert between units; it > will be cleared up in a following patch. > > Signed-off-by: Eric Blake Reviewed-by: John Snow > > --- > v2: tweak mirror_clip_bytes() signature to match previous patch > ---
Re: [Qemu-block] [Qemu-devel] [PATCH v2 15/20] backup: Switch block_backup.h to byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > We are gradually converting to byte-based interfaces, as they are > easier to reason about than sector-based. Continue by converting > the public interface to backup jobs (no semantic change), including > a change to CowRequest to track by bytes instead of cluster indices. > > Note that this does not change the difference between the public > interface (starting point, and size of the subsequent range) and > the internal interface (starting and end points). > > Signed-off-by: Eric Blake > Reviewed-by: John Snow > --- > v2: change a couple more parameter names > --- > include/block/block_backup.h | 11 +-- > block/backup.c | 33 - > block/replication.c | 12 > 3 files changed, 29 insertions(+), 27 deletions(-) >
Re: [Qemu-block] [Qemu-devel] [PATCH v2 18/20] block: Make bdrv_is_allocated() byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > We are gradually moving away from sector-based interfaces, towards > byte-based. In the common case, allocation is unlikely to ever use > values that are not naturally sector-aligned, but it is possible > that byte-based values will let us be more precise about allocation > at the end of an unaligned file that can do byte-based access. > > Changing the signature of the function to use int64_t *pnum ensures > that the compiler enforces that all callers are updated. For now, > the io.c layer still assert()s that all callers are sector-aligned > on input and that *pnum is sector-aligned on return to the caller, > but that can be relaxed when a later patch implements byte-based > block status. Therefore, this code adds usages like > DIV_ROUND_UP(,BDRV_SECTOR_SIZE) to callers that still want aligned > values, where the call might reasonbly give non-aligned results > in the future; on the other hand, no rounding is needed for callers > that should just continue to work with byte alignment. > > For the most part this patch is just the addition of scaling at the > callers followed by inverse scaling at bdrv_is_allocated(). But > some code, particularly bdrv_commit(), gets a lot simpler because it > no longer has to mess with sectors; also, it is now possible to pass > NULL if the caller does not care how much of the image is allocated > beyond the initial offset. > > For ease of review, bdrv_is_allocated_above() will be tackled > separately. > > Signed-off-by: Eric Blake > Reviewed-by: John Snow > --- > v2: rebase to earlier changes, tweak commit message > --- > include/block/block.h | 4 +-- > block/backup.c| 17 - > block/commit.c| 21 +++- > block/io.c| 49 +--- > block/stream.c| 5 ++-- > block/vvfat.c | 34 ++--- > migration/block.c | 9 --- > qemu-img.c| 5 +++- > qemu-io-cmds.c| 70 > +++ > 9 files changed, 114 insertions(+), 100 deletions(-) >
Re: [Qemu-block] [Qemu-devel] [PATCH v2 20/20] block: Make bdrv_is_allocated_above() byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > -int64_t sector_num, int nb_sectors, int *pnum); > +int64_t offset, int64_t bytes, int64_t *pnum); Minor context conflict after this that, for whichever reason, git could not resolve on its own for me.
Re: [Qemu-block] [Qemu-devel] [PATCH v2 20/20] block: Make bdrv_is_allocated_above() byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > We are gradually moving away from sector-based interfaces, towards > byte-based. In the common case, allocation is unlikely to ever use > values that are not naturally sector-aligned, but it is possible > that byte-based values will let us be more precise about allocation > at the end of an unaligned file that can do byte-based access. > > Changing the signature of the function to use int64_t *pnum ensures > that the compiler enforces that all callers are updated. For now, > the io.c layer still assert()s that all callers are sector-aligned, > but that can be relaxed when a later patch implements byte-based > block status. Therefore, for the most part this patch is just the > addition of scaling at the callers followed by inverse scaling at > bdrv_is_allocated(). But some code, particularly stream_run(), > gets a lot simpler because it no longer has to mess with sectors. > > For ease of review, bdrv_is_allocated() was tackled separately. > > Signed-off-by: Eric Blake > Reviewed-by: John Snow
Re: [Qemu-block] [Qemu-devel] [PATCH v4 2/4] block: Guarantee that *file is set on bdrv_get_block_status()
On 06/05/2017 04:38 PM, Eric Blake wrote: > We document that *file is valid if the return is not an error and > includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract > when a driver (such as blkdebug) lacks a callback. Messed up in > commit 67a0fd2 (v2.6), when we added the file parameter. > > Enhance qemu-iotest 177 to cover this, using a sequence that would > print garbage or even SEGV, because it was dererefencing through > uninitialized memory. [The resulting test output shows that we > have less-than-ideal block status from the blkdebug driver, but > that's a separate fix coming up soon.] > > Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is > enough to fix the crash, but we can go one step further: always > setting *file, even on error, means that a broken caller that > blindly dereferences file without checking for error is now more > likely to get a reliable SEGV instead of randomly acting on garbage, > making it easier to diagnose such buggy callers. Adding an > assertion that file is set where expected doesn't hurt either. > > CC: qemu-sta...@nongnu.org > Signed-off-by: Eric Blake > Reviewed-by: Fam Zheng > Reviewed-by: Max Reitz Reviewed-by: John Snow
Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/4] qemu-io: Don't die on second open
On 06/05/2017 04:38 PM, Eric Blake wrote: > Most callback commands in qemu-io return 0 to keep the interpreter > loop running, or 1 to quit immediately. However, open_f() just > passed through the return value of openfile(), which has different > semantics of returning 0 if a file was opened, or 1 on any failure. > > As a result of mixing the return semantics, we are forcing the > qemu-io interpreter to exit early on any failures, which is rather > annoying when some of the failures are obviously trying to give > the user a hint of how to proceed (if we didn't then kill qemu-io > out from under the user's feet): > > $ qemu-io > qemu-io> open foo > qemu-io> open foo > file open already, try 'help close' > $ echo $? > 0 > > In general, we WANT openfile() to report failures, since it is the > function used in the form 'qemu-io -c "$something" no_such_file' > for performing one or more -c options on a single file, and it is > not worth attempting $something if the file itself cannot be opened. > So the solution is to fix open_f() to always return 0 (when we are > in interactive mode, even failure to open should not end the > session), and save the return value of openfile() for command line > use in main(). > > Note, however, that we do have some qemu-iotests that do 'qemu-io > -c "open file" -c "$something"'; such tests will now proceed to > attempt $something whether or not the open succeeded, the same way > as if the two commands had been attempted in interactive mode. As > such, the expected output for those tests has to be modified. But it > also means that it is now possible to use -c close and have a single > qemu-io command line operate on more than one file even without > using interactive mode. Although the '-c open' action is a subtle > change in behavior, remember that qemu-io is for debugging purposes, > so as long as it serves the needs of qemu-iotests while still being > reasonable for interactive use, it should not be a problem that we > are changing tests to the new behavior. > > This has been awkward since at least as far back as commit > e3aff4f, in 2009. > > Signed-off-by: Eric Blake > Reviewed-by: Fam Zheng Reviewed-by: John Snow
Re: [Qemu-block] [Qemu-devel] [PATCH v4 3/4] block: Simplify use of BDRV_BLOCK_RAW
On Mon, 06/05 15:38, Eric Blake wrote:
> The lone caller that cares about a return of BDRV_BLOCK_RAW
> (namely, io.c:bdrv_co_get_block_status) completely replaces the
> return value, so there is no point in passing BDRV_BLOCK_DATA.
>
> Signed-off-by: Eric Blake
>
> ---
> v3: further document BDRV_BLOCK_RAW
> v2: fix subject, tweak commit message
> ---
> include/block/block.h | 6 +++---
> block/commit.c| 2 +-
> block/mirror.c| 2 +-
> block/raw-format.c| 2 +-
> block/vpc.c | 2 +-
> 5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 9b355e9..0a60444 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -131,9 +131,9 @@ typedef struct HDGeometry {
> * layer (short for DATA || ZERO), set by block layer
> *
> * Internal flag:
> - * BDRV_BLOCK_RAW: used internally to indicate that the request was
> - * answered by a passthrough driver such as raw and that the
> - * block layer should recompute the answer from bs->file.
> + * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request
> + * that the block layer recompute the answer from the
> returned
> + * BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
> *
> * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK)
> * represent the offset in the returned BDS that is allocated for the
> diff --git a/block/commit.c b/block/commit.c
> index a3028b2..f56745b 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -239,7 +239,7 @@ static int64_t coroutine_fn
> bdrv_commit_top_get_block_status(
> {
> *pnum = nb_sectors;
> *file = bs->backing->bs;
> -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> (sector_num << BDRV_SECTOR_BITS);
> }
>
> diff --git a/block/mirror.c b/block/mirror.c
> index a2a9703..892d53a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1052,7 +1052,7 @@ static int64_t coroutine_fn
> bdrv_mirror_top_get_block_status(
> {
> *pnum = nb_sectors;
> *file = bs->backing->bs;
> -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> (sector_num << BDRV_SECTOR_BITS);
> }
>
> diff --git a/block/raw-format.c b/block/raw-format.c
> index 36e6503..1136eba 100644
> --- a/block/raw-format.c
> +++ b/block/raw-format.c
> @@ -259,7 +259,7 @@ static int64_t coroutine_fn
> raw_co_get_block_status(BlockDriverState *bs,
> *pnum = nb_sectors;
> *file = bs->file->bs;
> sector_num += s->offset / BDRV_SECTOR_SIZE;
> -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> (sector_num << BDRV_SECTOR_BITS);
> }
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 4240ba9..b313c68 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -701,7 +701,7 @@ static int64_t coroutine_fn
> vpc_co_get_block_status(BlockDriverState *bs,
> if (be32_to_cpu(footer->type) == VHD_FIXED) {
> *pnum = nb_sectors;
> *file = bs->file->bs;
> -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> (sector_num << BDRV_SECTOR_BITS);
> }
>
> --
> 2.9.4
>
>
Reviewed-by: Fam Zheng
Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/5 19:08, Dr. David Alan Gilbert 写道:
* QingFeng Hao (ha...@linux.vnet.ibm.com) wrote:
In load_vmstate, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
Thanks!
Reviewed-by: Dr. David Alan Gilbert
Thanks David!
migration_incoming_state_destroy();
--
2.11.2
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Regards
QingFeng Hao
Re: [Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
> In load_vmstate, mis->from_src_file is freed twice, the first free is by
> qemu_fclose, the second is by migration_incoming_state_destroy and
> it causes Illegal instruction exception. The fix is just to remove the
> first free.
>
> This problem is found by qemu-iotests case 068 since commit
> "660819b migration: shut src return path unconditionally". The error is:
> 068 1s ... - output mismatch (see 068.out.bad)
> --- tests/qemu-iotests/068.out2017-05-06 01:00:26.417270437 +0200
> +++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200
> @@ -6,6 +6,8 @@
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm 0
> (qemu) quit
> +./common.config: line 107: 242472 Illegal instruction (core dumped)
> ( if [ -n "${QEMU_NEED_PID}" ]; then
> +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) quit
> -*** done
> +(qemu) *** done
>
> Signed-off-by: QingFeng Hao
> ---
> migration/savevm.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9c320f59d0..853e14e34e 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
>
> aio_context_acquire(aio_context);
> ret = qemu_loadvm_state(f);
> -qemu_fclose(f);
> aio_context_release(aio_context);
>
> migration_incoming_state_destroy();
Thanks QingFeng for the fix!
Reviewed-by: Peter Xu
Though here instead of removing the fclose(), not sure whether this is
nicer:
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f5..1feb519 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
QEMUFile *f;
int ret;
AioContext *aio_context;
-MigrationIncomingState *mis = migration_incoming_get_current();
if (!bdrv_all_can_snapshot(&bs)) {
error_setg(errp,
@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
}
qemu_system_reset(SHUTDOWN_CAUSE_NONE);
-mis->from_src_file = f;
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
Since I see we setup from_src_file but we don't really use it. If we
remove that line, we can also remove the
migration_incoming_get_current() call altogether.
Either way work for me. So my r-b stands always. :-)
--
Peter Xu
Re: [Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/6 11:03, Peter Xu 写道:
On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
In load_vmstate, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
migration_incoming_state_destroy();
Thanks QingFeng for the fix!
Reviewed-by: Peter Xu
Though here instead of removing the fclose(), not sure whether this is
nicer:
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f5..1feb519 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
QEMUFile *f;
int ret;
AioContext *aio_context;
-MigrationIncomingState *mis = migration_incoming_get_current();
if (!bdrv_all_can_snapshot(&bs)) {
error_setg(errp,
@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
}
qemu_system_reset(SHUTDOWN_CAUSE_NONE);
-mis->from_src_file = f;
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
Thanks Peter. I have a doubt on this change because I see there are
several places
referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and
it seems to
get byte from the file and it's called by qemu_loadvm_state.
Anyway, you remind me for the description that is "In load_snapshot"
rather than
"In load_vmstate". thanks
Since I see we setup from_src_file but we don't really use it. If we
remove that line, we can also remove the
migration_incoming_get_current() call altogether.
Either way work for me. So my r-b stands always. :-)
--
Regards
QingFeng Hao
Re: [Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
On Tue, Jun 06, 2017 at 11:38:05AM +0800, QingFeng Hao wrote:
>
>
> 在 2017/6/6 11:03, Peter Xu 写道:
> >On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
> >>In load_vmstate, mis->from_src_file is freed twice, the first free is by
> >>qemu_fclose, the second is by migration_incoming_state_destroy and
> >>it causes Illegal instruction exception. The fix is just to remove the
> >>first free.
> >>
> >>This problem is found by qemu-iotests case 068 since commit
> >>"660819b migration: shut src return path unconditionally". The error is:
> >>068 1s ... - output mismatch (see 068.out.bad)
> >> --- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
> >> +++ 068.out.bad2017-06-03 13:59:55.360274640 +0200
> >> @@ -6,6 +6,8 @@
> >> QEMU X.Y.Z monitor - type 'help' for more information
> >> (qemu) savevm 0
> >> (qemu) quit
> >> +./common.config: line 107: 242472 Illegal instruction (core
> >> dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
> >> +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> >> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> >> QEMU X.Y.Z monitor - type 'help' for more information
> >> -(qemu) quit
> >> -*** done
> >> +(qemu) *** done
> >>
> >>Signed-off-by: QingFeng Hao
> >>---
> >> migration/savevm.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >>diff --git a/migration/savevm.c b/migration/savevm.c
> >>index 9c320f59d0..853e14e34e 100644
> >>--- a/migration/savevm.c
> >>+++ b/migration/savevm.c
> >>@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
> >> aio_context_acquire(aio_context);
> >> ret = qemu_loadvm_state(f);
> >>-qemu_fclose(f);
> >> aio_context_release(aio_context);
> >> migration_incoming_state_destroy();
> >Thanks QingFeng for the fix!
> >
> >Reviewed-by: Peter Xu
> >
> >Though here instead of removing the fclose(), not sure whether this is
> >nicer:
> >
> >diff --git a/migration/savevm.c b/migration/savevm.c
> >index 9c320f5..1feb519 100644
> >--- a/migration/savevm.c
> >+++ b/migration/savevm.c
> >@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
> > QEMUFile *f;
> > int ret;
> > AioContext *aio_context;
> >-MigrationIncomingState *mis = migration_incoming_get_current();
> >
> > if (!bdrv_all_can_snapshot(&bs)) {
> > error_setg(errp,
> >@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
> > }
> >
> > qemu_system_reset(SHUTDOWN_CAUSE_NONE);
> >-mis->from_src_file = f;
> >
> > aio_context_acquire(aio_context);
> > ret = qemu_loadvm_state(f);
> Thanks Peter. I have a doubt on this change because I see there are several
> places
> referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it
> seems to
> get byte from the file and it's called by qemu_loadvm_state.
> Anyway, you remind me for the description that is "In load_snapshot" rather
> than
> "In load_vmstate". thanks
Oh I didn't really notice that. :)
It shouldn't affect imho since load snapshot won't trigger postcopy
codes. But sure current fix is good enough for me at least.
Thanks,
--
Peter Xu
Re: [Qemu-block] [Qemu-devel] [PATCH v4 0/4] more blkdebug tweaks
On 06/05/2017 04:38 PM, Eric Blake wrote: > I found a crasher and some odd behavior while rebasing my > bdrv_get_block_status series, so I figured I'd get these things > fixed first. This is based on top of Max's block branch. > > Available as a tag at: > git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4 > > Since v3: > - check all qemu-iotests (patch 1) > > 001/4:[0012] [FC] 'qemu-io: Don't die on second open' > 002/4:[] [--] 'block: Guarantee that *file is set on > bdrv_get_block_status()' > 003/4:[] [--] 'block: Simplify use of BDRV_BLOCK_RAW' > 004/4:[] [--] 'blkdebug: Support .bdrv_co_get_block_status' > > Eric Blake (4): > qemu-io: Don't die on second open > block: Guarantee that *file is set on bdrv_get_block_status() > block: Simplify use of BDRV_BLOCK_RAW > blkdebug: Support .bdrv_co_get_block_status > > include/block/block.h | 6 +++--- > block/blkdebug.c | 11 +++ > block/commit.c | 2 +- > block/io.c | 5 +++-- > block/mirror.c | 2 +- > block/raw-format.c | 2 +- > block/vpc.c| 2 +- > qemu-io.c | 7 --- > tests/qemu-iotests/060.out | 1 + > tests/qemu-iotests/114.out | 5 +++-- > tests/qemu-iotests/153.out | 6 ++ > tests/qemu-iotests/177 | 3 +++ > tests/qemu-iotests/177.out | 5 + > 13 files changed, 43 insertions(+), 14 deletions(-) > 3,4: Reviewed-by: John Snow
Re: [Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/6 11:50, Peter Xu 写道:
On Tue, Jun 06, 2017 at 11:38:05AM +0800, QingFeng Hao wrote:
在 2017/6/6 11:03, Peter Xu 写道:
On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
In load_vmstate, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
migration_incoming_state_destroy();
Thanks QingFeng for the fix!
Reviewed-by: Peter Xu
Though here instead of removing the fclose(), not sure whether this is
nicer:
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f5..1feb519 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
QEMUFile *f;
int ret;
AioContext *aio_context;
-MigrationIncomingState *mis = migration_incoming_get_current();
if (!bdrv_all_can_snapshot(&bs)) {
error_setg(errp,
@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
}
qemu_system_reset(SHUTDOWN_CAUSE_NONE);
-mis->from_src_file = f;
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
Thanks Peter. I have a doubt on this change because I see there are several
places
referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it
seems to
get byte from the file and it's called by qemu_loadvm_state.
Anyway, you remind me for the description that is "In load_snapshot" rather
than
"In load_vmstate". thanks
Oh I didn't really notice that. :)
It shouldn't affect imho since load snapshot won't trigger postcopy
codes. But sure current fix is good enough for me at least.
Ok, thanks!
Thanks,
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v2 0/1] qemu/migration: fix the migration bug found by qemu-iotests case 068
Hi all, This patch is to fix the migration bug found by qemu-iotests case 068 and based on upstream master's commit: 199e19ee53: Merge remote-tracking branch 'remotes/mjt/tags/trivial-patches-fetch' into staging. The bug was introduced by commit "660819b migration: shut src return path unconditionally". Change Log(v2): Got reviewed-by from Dr. David Alan Gilbert and Peter Xu. Thanks! QingFeng Hao (1): qemu/migration: fix the double free problem on from_src_file migration/savevm.c | 1 - 1 file changed, 1 deletion(-) -- 2.11.2
[Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file
In load_snapshot, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) (
if [ -n "${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao
Reviewed-by: Dr. David Alan Gilbert
Reviewed-by: Peter Xu
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
migration_incoming_state_destroy();
--
2.11.2
