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

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

[Fam Zheng]

Previouly, "qemu-img convert" 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

Because current code in qemu-img.c uses the normal convert loop, rather than
the compressed == true loop, which write by cluster size. In VMDK
streamOptimized, writes must be in cluster unit, because overlapped 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 framework.

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                 |  8 ++++++--
 tests/qemu-iotests/059.out |  1 +
 5 files changed, 44 insertions(+), 5 deletions(-)

-- 
1.8.4.2

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

[Fam Zheng]

If target block driver forces compression, 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            | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/block/block.h b/include/block/block.h
index 3560deb..169c092 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -18,6 +18,7 @@ typedef struct BlockDriverInfo {
     /* offset at which the VM state can be saved (0 if not possible) */
     int64_t vm_state_offset;
     bool is_dirty;
+    bool is_compressed;
 } BlockDriverInfo;
 
 typedef struct BlockFragInfo {
diff --git a/qemu-img.c b/qemu-img.c
index bf3fb4f..09ed9b2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv)
         }
     }
 
+    ret = bdrv_get_info(out_bs, &bdi);
+    if (ret == 0) {
+        compress = compress || bdi.is_compressed;
+    }
     if (compress) {
-        ret = bdrv_get_info(out_bs, &bdi);
         if (ret < 0) {
             error_report("could not get block driver info");
             goto out;
-- 
1.8.4.2

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

[Jeff Cody]

On Wed, Nov 13, 2013 at 08:53:10AM +0800, Fam Zheng wrote:
> If target block driver forces compression, 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            | 5 ++++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 3560deb..169c092 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -18,6 +18,7 @@ typedef struct BlockDriverInfo {
>      /* offset at which the VM state can be saved (0 if not possible) */
>      int64_t vm_state_offset;
>      bool is_dirty;
> +    bool is_compressed;
>  } BlockDriverInfo;
>  
>  typedef struct BlockFragInfo {
> diff --git a/qemu-img.c b/qemu-img.c
> index bf3fb4f..09ed9b2 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv)
>          }
>      }
>  
> +    ret = bdrv_get_info(out_bs, &bdi);
> +    if (ret == 0) {
> +        compress = compress || bdi.is_compressed;
> +    }
>      if (compress) {
> -        ret = bdrv_get_info(out_bs, &bdi);
>          if (ret < 0) {
>              error_report("could not get block driver info");
>              goto out;

You need to move the error checking up as well, above the
'if (compress)'

Jeff

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

[Fam Zheng]

On 2013年11月13日 23:23, Jeff Cody wrote:
> On Wed, Nov 13, 2013 at 08:53:10AM +0800, Fam Zheng wrote:
>> If target block driver forces compression, 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            | 5 ++++-
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 3560deb..169c092 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -18,6 +18,7 @@ typedef struct BlockDriverInfo {
>>       /* offset at which the VM state can be saved (0 if not possible) */
>>       int64_t vm_state_offset;
>>       bool is_dirty;
>> +    bool is_compressed;
>>   } BlockDriverInfo;
>>
>>   typedef struct BlockFragInfo {
>> diff --git a/qemu-img.c b/qemu-img.c
>> index bf3fb4f..09ed9b2 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv)
>>           }
>>       }
>>
>> +    ret = bdrv_get_info(out_bs, &bdi);
>> +    if (ret == 0) {
>> +        compress = compress || bdi.is_compressed;
>> +    }
>>       if (compress) {
>> -        ret = bdrv_get_info(out_bs, &bdi);
>>           if (ret < 0) {
>>               error_report("could not get block driver info");
>>               goto out;
>
> You need to move the error checking up as well, above the
> 'if (compress)'
>

Moving it up forces bdrv_get_info to succeed for both compress case and 
non compress case, which is too strict and fails when target driver 
doesn't support it.

I see it's just odd to assign ret outside if block and check it in only 
one branch, but I don't have a better way to do this here.

Fam

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

[Jeff Cody]

On Thu, Nov 14, 2013 at 09:18:35AM +0800, Fam Zheng wrote:
> On 2013年11月13日 23:23, Jeff Cody wrote:
> >On Wed, Nov 13, 2013 at 08:53:10AM +0800, Fam Zheng wrote:
> >>If target block driver forces compression, 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            | 5 ++++-
> >>  2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/include/block/block.h b/include/block/block.h
> >>index 3560deb..169c092 100644
> >>--- a/include/block/block.h
> >>+++ b/include/block/block.h
> >>@@ -18,6 +18,7 @@ typedef struct BlockDriverInfo {
> >>      /* offset at which the VM state can be saved (0 if not possible) */
> >>      int64_t vm_state_offset;
> >>      bool is_dirty;
> >>+    bool is_compressed;
> >>  } BlockDriverInfo;
> >>
> >>  typedef struct BlockFragInfo {
> >>diff --git a/qemu-img.c b/qemu-img.c
> >>index bf3fb4f..09ed9b2 100644
> >>--- a/qemu-img.c
> >>+++ b/qemu-img.c
> >>@@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv)
> >>          }
> >>      }
> >>
> >>+    ret = bdrv_get_info(out_bs, &bdi);
> >>+    if (ret == 0) {
> >>+        compress = compress || bdi.is_compressed;
> >>+    }
> >>      if (compress) {
> >>-        ret = bdrv_get_info(out_bs, &bdi);
> >>          if (ret < 0) {
> >>              error_report("could not get block driver info");
> >>              goto out;
> >
> >You need to move the error checking up as well, above the
> >'if (compress)'
> >
> 
> Moving it up forces bdrv_get_info to succeed for both compress case
> and non compress case, which is too strict and fails when target
> driver doesn't support it.

bdrv_get_info() will return -ENOTSUP if it is not supported.  If we do
call down to the driver layer bdrv_get_info() and get an error, I
think we should pass it up.  So just ignore -ENOTSUP if we don't care
about the case when bdrv_get_info() is not supported by a target.

> 
> I see it's just odd to assign ret outside if block and check it in
> only one branch, but I don't have a better way to do this here.
> 
> Fam
> 

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

[Fam Zheng]

On 2013年11月14日 09:34, Jeff Cody wrote:
> On Thu, Nov 14, 2013 at 09:18:35AM +0800, Fam Zheng wrote:
>> On 2013年11月13日 23:23, Jeff Cody wrote:
>>> On Wed, Nov 13, 2013 at 08:53:10AM +0800, Fam Zheng wrote:
>>>> If target block driver forces compression, 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            | 5 ++++-
>>>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/block/block.h b/include/block/block.h
>>>> index 3560deb..169c092 100644
>>>> --- a/include/block/block.h
>>>> +++ b/include/block/block.h
>>>> @@ -18,6 +18,7 @@ typedef struct BlockDriverInfo {
>>>>       /* offset at which the VM state can be saved (0 if not possible) */
>>>>       int64_t vm_state_offset;
>>>>       bool is_dirty;
>>>> +    bool is_compressed;
>>>>   } BlockDriverInfo;
>>>>
>>>>   typedef struct BlockFragInfo {
>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>> index bf3fb4f..09ed9b2 100644
>>>> --- a/qemu-img.c
>>>> +++ b/qemu-img.c
>>>> @@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv)
>>>>           }
>>>>       }
>>>>
>>>> +    ret = bdrv_get_info(out_bs, &bdi);
>>>> +    if (ret == 0) {
>>>> +        compress = compress || bdi.is_compressed;
>>>> +    }
>>>>       if (compress) {
>>>> -        ret = bdrv_get_info(out_bs, &bdi);
>>>>           if (ret < 0) {
>>>>               error_report("could not get block driver info");
>>>>               goto out;
>>>
>>> You need to move the error checking up as well, above the
>>> 'if (compress)'
>>>
>>
>> Moving it up forces bdrv_get_info to succeed for both compress case
>> and non compress case, which is too strict and fails when target
>> driver doesn't support it.
>
> bdrv_get_info() will return -ENOTSUP if it is not supported.  If we do
> call down to the driver layer bdrv_get_info() and get an error, I
> think we should pass it up.  So just ignore -ENOTSUP if we don't care
> about the case when bdrv_get_info() is not supported by a target.
>

We care for -ENOTSUP as well as other err code in the compressed case, 
and don't care the call to bdrv_get_info in the other case. I get your 
point, I'll add "else if (ret != -ENOTSUP) {...}" below the call, and 
keep the check of ret in "if (compressed)" unchanged.

Fam

[Qemu-devel] [PATCH 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 a7ebd0f..f439206 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1417,6 +1417,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)
@@ -1937,6 +1950,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.4.2

[Qemu-devel] [PATCH 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 +-
 qemu-img.c            | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 169c092..7059bb4 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;
diff --git a/qemu-img.c b/qemu-img.c
index 09ed9b2..6ca19ff 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1121,8 +1121,9 @@ out3:
 
 static int img_convert(int argc, char **argv)
 {
-    int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size,
+    int c, ret = 0, n, n1, bs_n, bs_i, compress,
         cluster_sectors, skip_create;
+    int64_t cluster_size;
     int progress = 0, flags;
     const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename;
     BlockDriver *drv, *proto_drv;
-- 
1.8.4.2

[Qemu-devel] [PATCH 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 |  1 +
 2 files changed, 20 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index f439206..07f77d8 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1905,6 +1905,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,
@@ -1959,6 +1977,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
     .bdrv_has_zero_init           = vmdk_has_zero_init,
     .bdrv_get_specific_info       = vmdk_get_specific_info,
+    .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 2ded8a9..8394d14 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
-- 
1.8.4.2

[Qemu-devel] [PATCH 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 7b95acf..c0c321b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -320,8 +320,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.4.2

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

[Jeff Cody]

On Wed, Nov 13, 2013 at 08:53:14AM +0800, Fam Zheng wrote:
> 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 7b95acf..c0c321b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -320,8 +320,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) {

For ret < 0, I think we should exit on error (at least in cases where
ret < 0 && ret != -ENOTSUP).

>              s->buf_size = MAX(s->buf_size, bdi.cluster_size);
>              s->cow_bitmap = bitmap_new(length);
>          }

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

[Fam Zheng]

On 2013年11月13日 23:34, Jeff Cody wrote:
> On Wed, Nov 13, 2013 at 08:53:14AM +0800, Fam Zheng wrote:
>> 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 7b95acf..c0c321b 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -320,8 +320,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) {
>
> For ret < 0, I think we should exit on error (at least in cases where
> ret < 0 && ret != -ENOTSUP).
>

We only need the cluster_size here, which is optional to set up 
operation granularity. Exiting the block job because bdrv_get_info fails 
(which is not a critical error) is worse in fault tolerance sense, so 
I'd like to keep this way.

Paolo, any ideas?

Fam