[PATCH v2 0/5] Fix conversion from ISO to VMDK streamOptimized

[Fam Zheng]

Previouly, conversion from ISO to VMDK with subformat=streamOptimized fails:

$ ./qemu-img convert -O vmdk -o subformat=streamOptimized foo.iso bar.vmdk
VMDK: can't write to allocated cluster for streamOptimized
qemu-img: error while writing sector 64: Input/output error

Current code in qemu-img.c picks the normal convert loop for this case, rather
than the "compress == true" loop, which writes in target cluster size. In VMDK
streamOptimized, writes must be in cluster unit, because write to an allocated
cluster is not supported.

This series adds an is_compressed field in BlockDriverInfo, and use compressed
convertion loop if the block driver set this field to true.

Implement .bdrv_get_info and .bdrv_write_compressed in VMDK driver to fit into
this procedure.

v2: Rebase to current master.



Fam Zheng (5):
qemu-img: Convert by cluster size if target is compressed
vmdk: Implement .bdrv_write_compressed
block: Change BlockDriverInfo.cluster_size to 64 bits
vmdk: Implement .bdrv_get_info()
mirror: Check for bdrv_get_info result

block/mirror.c | 4 ++--
block/vmdk.c | 33 +++++++++++++++++++++++++++++++++
include/block/block.h | 3 ++-
qemu-img.c | 3 +++
tests/qemu-iotests/059.out | 4 ++++
5 files changed, 44 insertions(+), 3 deletions(-)

[Fam Zheng]

bdrv_get_info could fail. Add check before using the returned value.

Signed-off-by: Fam Zheng <***@redhat.com>
---
block/mirror.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 2a43334..bd3010b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -325,8 +325,8 @@ static void coroutine_fn mirror_run(void *opaque)
bdrv_get_backing_filename(s->target, backing_filename,
sizeof(backing_filename));
if (backing_filename[0] && !s->target->backing_hd) {
- bdrv_get_info(s->target, &bdi);
- if (s->granularity < bdi.cluster_size) {
+ ret = bdrv_get_info(s->target, &bdi);
+ if (ret == 0 && s->granularity < bdi.cluster_size) {
s->buf_size = MAX(s->buf_size, bdi.cluster_size);
s->cow_bitmap = bitmap_new(length);
}

[Fam Zheng]

If target block driver forces compression (bdi.is_compressed == true),
qemu-img convert needs to write by cluster size as well as "-c" option.

Particularly, this applies for converting to VMDK streamOptimized
format.

Signed-off-by: Fam Zheng <***@redhat.com>
---
include/block/block.h | 1 +
qemu-img.c | 3 +++
2 files changed, 4 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 963a61f..826b5da 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -34,6 +34,7 @@ typedef struct BlockDriverInfo {
* opened with BDRV_O_UNMAP flag for this to work.
*/
bool can_write_zeroes_with_unmap;
+ bool is_compressed;
} BlockDriverInfo;

typedef struct BlockFragInfo {
diff --git a/qemu-img.c b/qemu-img.c
index c989850..859b927 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1425,11 +1425,14 @@ static int img_convert(int argc, char **argv)
cluster_sectors = 0;
ret = bdrv_get_info(out_bs, &bdi);
if (ret < 0) {
+ /* We don't care whether bdrv_get_info() fails or not unless we are to
+ * compress */
if (compress) {
error_report("could not get block driver info");
goto out;
}
} else {
+ compress = compress || bdi.is_compressed;
cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
}

[Fam Zheng]

Add a wrapper function to support "compressed" path in qemu-img convert.
Only support streamOptimized subformat case for now (num_extents == 1
and extent compression is true).

Signed-off-by: Fam Zheng <***@redhat.com>
---
block/vmdk.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index e809e2e..cf5a98f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1457,6 +1457,19 @@ static coroutine_fn int vmdk_co_write(BlockDriverState *bs, int64_t sector_num,
return ret;
}

+static int vmdk_write_compressed(BlockDriverState *bs,
+ int64_t sector_num,
+ const uint8_t *buf,
+ int nb_sectors)
+{
+ BDRVVmdkState *s = bs->opaque;
+ if (s->num_extents == 1 && s->extents[0].compressed) {
+ return vmdk_write(bs, sector_num, buf, nb_sectors, false, false);
+ } else {
+ return -ENOTSUP;
+ }
+}
+
static int coroutine_fn vmdk_co_write_zeroes(BlockDriverState *bs,
int64_t sector_num,
int nb_sectors,
@@ -2066,6 +2079,7 @@ static BlockDriver bdrv_vmdk = {
.bdrv_reopen_prepare = vmdk_reopen_prepare,
.bdrv_read = vmdk_co_read,
.bdrv_write = vmdk_co_write,
+ .bdrv_write_compressed = vmdk_write_compressed,
.bdrv_co_write_zeroes = vmdk_co_write_zeroes,
.bdrv_close = vmdk_close,
.bdrv_create = vmdk_create,

[Fam Zheng]

VMDK could have big cluster_size for monolithicFlat. It implements
.bdrv_get_info now, a 32 bit field is likely to overflow.

Signed-off-by: Fam Zheng <***@redhat.com>
---
include/block/block.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/block/block.h b/include/block/block.h
index 826b5da..3eb4e54 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -14,7 +14,7 @@ typedef struct BlockJob BlockJob;

typedef struct BlockDriverInfo {
/* in bytes, 0 if irrelevant */
- int cluster_size;
+ int64_t cluster_size;
/* offset at which the VM state can be saved (0 if not possible) */
int64_t vm_state_offset;
bool is_dirty;

[Stefan Hajnoczi]

Post by Fam Zheng
VMDK could have big cluster_size for monolithicFlat. It implements
.bdrv_get_info now, a 32 bit field is likely to overflow.
---
include/block/block.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/block/block.h b/include/block/block.h
index 826b5da..3eb4e54 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -14,7 +14,7 @@ typedef struct BlockJob BlockJob;
typedef struct BlockDriverInfo {
/* in bytes, 0 if irrelevant */
- int cluster_size;
+ int64_t cluster_size;

Not sure if this commit is enough:

If the cluster size is really larger than 2 GB and a compressed image is
in use, then we will attempt 2 GB I/O. There are also other codepaths
that use cluster_size and would similarly allocate 2 GB bounce buffers!

We need to think through what happens in those cases. Should we refuse
after a certain size? Will we need to enhance some code paths to
actually split up the bounce buffers (if possible)?

Stefan

[Fam Zheng]

This will return cluster_size and is_compressed to caller, if all the
extents has the same value (or there's only one extent). Otherwise
return -ENOTSUP.

Signed-off-by: Fam Zheng <***@redhat.com>
---
block/vmdk.c | 19 +++++++++++++++++++
tests/qemu-iotests/059.out | 4 ++++
2 files changed, 23 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index cf5a98f..63fe79c 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2033,6 +2033,24 @@ static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
return spec_info;
}

+static int vmdk_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+ int i;
+ BDRVVmdkState *s = bs->opaque;
+ assert(s->num_extents);
+ bdi->is_compressed = s->extents[0].compressed;
+ bdi->cluster_size = s->extents[0].cluster_sectors << BDRV_SECTOR_BITS;
+ /* See if we have multiple extents but they have different cases */
+ for (i = 1; i < s->num_extents; i++) {
+ if (bdi->is_compressed != s->extents[i].compressed ||
+ bdi->cluster_size !=
+ s->extents[i].cluster_sectors << BDRV_SECTOR_BITS) {
+ return -ENOTSUP;
+ }
+ }
+ return 0;
+}
+
static QEMUOptionParameter vmdk_create_options[] = {
{
.name = BLOCK_OPT_SIZE,
@@ -2089,6 +2107,7 @@ static BlockDriver bdrv_vmdk = {
.bdrv_has_zero_init = vmdk_has_zero_init,
.bdrv_get_specific_info = vmdk_get_specific_info,
.bdrv_refresh_limits = vmdk_refresh_limits,
+ .bdrv_get_info = vmdk_get_info,

.create_options = vmdk_create_options,
};
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 4ffeb54..6d8e4c1 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -21,6 +21,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
image: TEST_DIR/t.IMGFMT
file format: IMGFMT
virtual size: 2.0G (2147483648 bytes)
+cluster_size: 2147483648

=== Testing monolithicFlat with zeroed_grain ===
qemu-img: TEST_DIR/t.IMGFMT: Flat image can't enable zeroed grain
@@ -32,6 +33,7 @@ image: TEST_DIR/t.vmdk
file format: vmdk
virtual size: 1.0T (1073741824000 bytes)
disk size: 16K
+cluster_size: 2147483648
Format specific information:
cid: XXXXXXXX
parent cid: XXXXXXXX
@@ -2052,12 +2054,14 @@ qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT'
image: TEST_DIR/iotest-version3.IMGFMT
file format: IMGFMT
virtual size: 1.0G (1073741824 bytes)
+cluster_size: 65536

=== Testing 4TB monolithicFlat creation and IO ===
Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=4398046511104
image: TEST_DIR/iotest-version3.IMGFMT
file format: IMGFMT
virtual size: 4.0T (4398046511104 bytes)
+cluster_size: 4398046511104
wrote 512/512 bytes at offset 966367641600
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
e100000000: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ................