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

[Qemu-devel] [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(-)

-- 
1.8.5.4

[Qemu-devel] [PATCH v2 1/5] qemu-img: Convert by cluster size if target is compressed

[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 <famz@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;
     }
 
-- 
1.8.5.4

[Qemu-devel] [PATCH v2 2/5] vmdk: Implement .bdrv_write_compressed

[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 <famz@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,
-- 
1.8.5.4

[Qemu-devel] [PATCH v2 3/5] block: Change BlockDriverInfo.cluster_size to 64 bits

[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 <famz@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;
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2 3/5] block: Change BlockDriverInfo.cluster_size to 64 bits

[Stefan Hajnoczi]

On Tue, Feb 11, 2014 at 11:28:37AM +0800, Fam Zheng wrote:
> 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 <famz@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;

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

[Qemu-devel] [PATCH v2 4/5] vmdk: Implement .bdrv_get_info()

[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 <famz@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  ................
-- 
1.8.5.4

[Qemu-devel] [PATCH v2 5/5] mirror: Check for bdrv_get_info result

[Fam Zheng]

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

Signed-off-by: Fam Zheng <famz@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);
         }
-- 
1.8.5.4