[PATCH 0/6] vmdk: Fix zero cluster handling

[PATCH 0/6] vmdk: Fix zero cluster handling

[Kevin Wolf]

What I was really investigating is why 055 was so slow. I couldn't solve
that, but instead I found out that our VMDK code for zero clusters and
write_zeroes was completely broken. Apart from segfaults when zero
clusters were actually enabled, this caused a compressed backup target
to result in a bigger file than uncompressed with VMDK.

This series tries to fix it (with one bonus performance patch).

Kevin Wolf (6):
  vmdk: Rename VmdkMetaData.valid to new_allocation
  vmdk: Fix zero cluster allocation
  vmdk: Fix partial overwrite of zero cluster
  vmdk: Don't update L2 table for zero write on zero cluster
  vmdk: Flush only once in vmdk_L2update()
  iotests: vmdk: Enable zeroed_grained=on by default

 block/vmdk.c             | 47 +++++++++++++++++++++++++---------------
 tests/qemu-iotests/059   |  6 ++---
 tests/qemu-iotests/check |  3 +++
 3 files changed, 35 insertions(+), 21 deletions(-)

-- 
2.25.3

[PATCH 1/6] vmdk: Rename VmdkMetaData.valid to new_allocation

[Kevin Wolf]

m_data is used for zero clusters even though valid == 0. It really only
means that a new cluster was allocated in the image file. Rename it to
reflect this.

While at it, change it from int to bool, too.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 8ec18f35a5..5b09275578 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -180,7 +180,7 @@ typedef struct VmdkMetaData {
     unsigned int l1_index;
     unsigned int l2_index;
     unsigned int l2_offset;
-    int valid;
+    bool new_allocation;
     uint32_t *l2_cache_entry;
 } VmdkMetaData;
 
@@ -1492,7 +1492,7 @@ static int get_cluster_offset(BlockDriverState *bs,
     unsigned int l2_size_bytes = extent->l2_size * extent->entry_size;
 
     if (m_data) {
-        m_data->valid = 0;
+        m_data->new_allocation = false;
     }
     if (extent->flat) {
         *cluster_offset = extent->flat_start_offset;
@@ -1630,7 +1630,7 @@ static int get_cluster_offset(BlockDriverState *bs,
             return ret;
         }
         if (m_data) {
-            m_data->valid = 1;
+            m_data->new_allocation = true;
             m_data->l1_index = l1_index;
             m_data->l2_index = l2_index;
             m_data->l2_offset = l2_offset;
@@ -2021,7 +2021,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
             if (ret) {
                 return ret;
             }
-            if (m_data.valid) {
+            if (m_data.new_allocation) {
                 /* update L2 tables */
                 if (vmdk_L2update(extent, &m_data,
                                   cluster_offset >> BDRV_SECTOR_BITS)
-- 
2.25.3

Re: [PATCH 1/6] vmdk: Rename VmdkMetaData.valid to new_allocation

[Eric Blake]

On 4/30/20 8:30 AM, Kevin Wolf wrote:
> m_data is used for zero clusters even though valid == 0. It really only
> means that a new cluster was allocated in the image file. Rename it to
> reflect this.
> 
> While at it, change it from int to bool, too.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vmdk.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH 2/6] vmdk: Fix zero cluster allocation

[Kevin Wolf]

m_data must be contain valid data even for zero clusters when no cluster
was allocated in the image file. Without this, zero writes segfault with
images that have zeroed_grain=on.

For zero writes, we don't want to allocate a cluster in the image file
even in compressed files.

Fixes: 524089bce43fd1cd3daaca979872451efa2cf7c6
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 5b09275578..bdd7d2dcf1 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1572,6 +1572,12 @@ static int get_cluster_offset(BlockDriverState *bs,
     extent->l2_cache_counts[min_index] = 1;
  found:
     l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size;
+    if (m_data) {
+        m_data->l1_index = l1_index;
+        m_data->l2_index = l2_index;
+        m_data->l2_offset = l2_offset;
+        m_data->l2_cache_entry = ((uint32_t *)l2_table) + l2_index;
+    }
 
     if (extent->sesparse) {
         cluster_sector = le64_to_cpu(((uint64_t *)l2_table)[l2_index]);
@@ -1631,10 +1637,6 @@ static int get_cluster_offset(BlockDriverState *bs,
         }
         if (m_data) {
             m_data->new_allocation = true;
-            m_data->l1_index = l1_index;
-            m_data->l2_index = l2_index;
-            m_data->l2_offset = l2_offset;
-            m_data->l2_cache_entry = ((uint32_t *)l2_table) + l2_index;
         }
     }
     *cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
@@ -1990,7 +1992,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
                 error_report("Could not write to allocated cluster"
                               " for streamOptimized");
                 return -EIO;
-            } else {
+            } else if (!zeroed) {
                 /* allocate */
                 ret = get_cluster_offset(bs, extent, &m_data, offset,
                                          true, &cluster_offset, 0, 0);
-- 
2.25.3

Re: [PATCH 2/6] vmdk: Fix zero cluster allocation

[Eric Blake]

On 4/30/20 8:30 AM, Kevin Wolf wrote:
> m_data must be contain valid data even for zero clusters when no cluster

s/be //

> was allocated in the image file. Without this, zero writes segfault with
> images that have zeroed_grain=on.
> 
> For zero writes, we don't want to allocate a cluster in the image file
> even in compressed files.
> 
> Fixes: 524089bce43fd1cd3daaca979872451efa2cf7c6

Nearly 4 years ago, and claims to fix a different regression.  Wow, we 
aren't doing too well with vmdk.

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vmdk.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/6] vmdk: Fix zero cluster allocation

[Eric Blake]

On 4/30/20 8:30 AM, Kevin Wolf wrote:
> m_data must be contain valid data even for zero clusters when no cluster
> was allocated in the image file. Without this, zero writes segfault with
> images that have zeroed_grain=on.

zero_grained=on ?

> 
> For zero writes, we don't want to allocate a cluster in the image file
> even in compressed files.
> 
> Fixes: 524089bce43fd1cd3daaca979872451efa2cf7c6
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vmdk.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/6] vmdk: Fix zero cluster allocation

[Kevin Wolf]

Am 30.04.2020 um 16:19 hat Eric Blake geschrieben:
> On 4/30/20 8:30 AM, Kevin Wolf wrote:
> > m_data must be contain valid data even for zero clusters when no cluster
> > was allocated in the image file. Without this, zero writes segfault with
> > images that have zeroed_grain=on.
> 
> zero_grained=on ?

No, zeroed_grain is the actual name of the option.

I don't really know what a grain is in VMDK terminology, but about the
only thing that felt healthy about the code I touched was that it has
whole-grain buffers. :-)

Kevin

[PATCH 3/6] vmdk: Fix partial overwrite of zero cluster

[Kevin Wolf]

When overwriting a zero cluster, we must not perform copy-on-write from
the backing file, but from a zeroed buffer.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index bdd7d2dcf1..da25b8992e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1340,7 +1340,9 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
  * get_whole_cluster
  *
  * Copy backing file's cluster that covers @sector_num, otherwise write zero,
- * to the cluster at @cluster_sector_num.
+ * to the cluster at @cluster_sector_num. If @zeroed is true, we're overwriting
+ * a zeroed cluster in the current layer and must not copy data from the
+ * backing file.
  *
  * If @skip_start_sector < @skip_end_sector, the relative range
  * [@skip_start_sector, @skip_end_sector) is not copied or written, and leave
@@ -1351,18 +1353,21 @@ static int get_whole_cluster(BlockDriverState *bs,
                              uint64_t cluster_offset,
                              uint64_t offset,
                              uint64_t skip_start_bytes,
-                             uint64_t skip_end_bytes)
+                             uint64_t skip_end_bytes,
+                             bool zeroed)
 {
     int ret = VMDK_OK;
     int64_t cluster_bytes;
     uint8_t *whole_grain;
+    bool copy_from_backing;
 
     /* For COW, align request sector_num to cluster start */
     cluster_bytes = extent->cluster_sectors << BDRV_SECTOR_BITS;
     offset = QEMU_ALIGN_DOWN(offset, cluster_bytes);
     whole_grain = qemu_blockalign(bs, cluster_bytes);
+    copy_from_backing = bs->backing && !zeroed;
 
-    if (!bs->backing) {
+    if (!copy_from_backing) {
         memset(whole_grain, 0, skip_start_bytes);
         memset(whole_grain + skip_end_bytes, 0, cluster_bytes - skip_end_bytes);
     }
@@ -1377,7 +1382,7 @@ static int get_whole_cluster(BlockDriverState *bs,
 
     /* Read backing data before skip range */
     if (skip_start_bytes > 0) {
-        if (bs->backing) {
+        if (copy_from_backing) {
             /* qcow2 emits this on bs->file instead of bs->backing */
             BLKDBG_EVENT(extent->file, BLKDBG_COW_READ);
             ret = bdrv_pread(bs->backing, offset, whole_grain,
@@ -1397,7 +1402,7 @@ static int get_whole_cluster(BlockDriverState *bs,
     }
     /* Read backing data after skip range */
     if (skip_end_bytes < cluster_bytes) {
-        if (bs->backing) {
+        if (copy_from_backing) {
             /* qcow2 emits this on bs->file instead of bs->backing */
             BLKDBG_EVENT(extent->file, BLKDBG_COW_READ);
             ret = bdrv_pread(bs->backing, offset + skip_end_bytes,
@@ -1631,7 +1636,8 @@ static int get_cluster_offset(BlockDriverState *bs,
          * or inappropriate VM shutdown.
          */
         ret = get_whole_cluster(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,
-                                offset, skip_start_bytes, skip_end_bytes);
+                                offset, skip_start_bytes, skip_end_bytes,
+                                zeroed);
         if (ret) {
             return ret;
         }
-- 
2.25.3

Re: [PATCH 3/6] vmdk: Fix partial overwrite of zero cluster

[Eric Blake]

On 4/30/20 8:30 AM, Kevin Wolf wrote:
> When overwriting a zero cluster, we must not perform copy-on-write from
> the backing file, but from a zeroed buffer.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vmdk.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH 4/6] vmdk: Don't update L2 table for zero write on zero cluster

[Kevin Wolf]

If a cluster is already zeroed, we don't have to call vmdk_L2update(),
which is rather slow because it flushes the image file.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index da25b8992e..dcd30f1419 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2013,7 +2013,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
                     offset_in_cluster == 0 &&
                     n_bytes >= extent->cluster_sectors * BDRV_SECTOR_SIZE) {
                 n_bytes = extent->cluster_sectors * BDRV_SECTOR_SIZE;
-                if (!zero_dry_run) {
+                if (!zero_dry_run && ret != VMDK_ZEROED) {
                     /* update L2 tables */
                     if (vmdk_L2update(extent, &m_data, VMDK_GTE_ZEROED)
                             != VMDK_OK) {
-- 
2.25.3

Re: [PATCH 4/6] vmdk: Don't update L2 table for zero write on zero cluster

[Eric Blake]

On 4/30/20 8:30 AM, Kevin Wolf wrote:
> If a cluster is already zeroed, we don't have to call vmdk_L2update(),
> which is rather slow because it flushes the image file.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vmdk.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/block/vmdk.c b/block/vmdk.c
> index da25b8992e..dcd30f1419 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -2013,7 +2013,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
>                       offset_in_cluster == 0 &&
>                       n_bytes >= extent->cluster_sectors * BDRV_SECTOR_SIZE) {
>                   n_bytes = extent->cluster_sectors * BDRV_SECTOR_SIZE;
> -                if (!zero_dry_run) {
> +                if (!zero_dry_run && ret != VMDK_ZEROED) {
>                       /* update L2 tables */
>                       if (vmdk_L2update(extent, &m_data, VMDK_GTE_ZEROED)
>                               != VMDK_OK) {
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH 5/6] vmdk: Flush only once in vmdk_L2update()

[Kevin Wolf]

If we have a backup L2 table, we currently flush once after writing to
the active L2 table and again after writing to the backup table. A
single flush is enough and makes things a little less slow.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index dcd30f1419..d3eb9a5cdc 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1435,7 +1435,7 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
     offset = cpu_to_le32(offset);
     /* update L2 table */
     BLKDBG_EVENT(extent->file, BLKDBG_L2_UPDATE);
-    if (bdrv_pwrite_sync(extent->file,
+    if (bdrv_pwrite(extent->file,
                 ((int64_t)m_data->l2_offset * 512)
                     + (m_data->l2_index * sizeof(offset)),
                 &offset, sizeof(offset)) < 0) {
@@ -1444,13 +1444,16 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
     /* 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,
+        if (bdrv_pwrite(extent->file,
                     ((int64_t)m_data->l2_offset * 512)
                         + (m_data->l2_index * sizeof(offset)),
                     &offset, sizeof(offset)) < 0) {
             return VMDK_ERROR;
         }
     }
+    if (bdrv_flush(extent->file->bs) < 0) {
+        return VMDK_ERROR;
+    }
     if (m_data->l2_cache_entry) {
         *m_data->l2_cache_entry = offset;
     }
-- 
2.25.3

Re: [PATCH 5/6] vmdk: Flush only once in vmdk_L2update()

[Eric Blake]

On 4/30/20 8:30 AM, Kevin Wolf wrote:
> If we have a backup L2 table, we currently flush once after writing to
> the active L2 table and again after writing to the backup table. A
> single flush is enough and makes things a little less slow.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vmdk.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index dcd30f1419..d3eb9a5cdc 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1435,7 +1435,7 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>       offset = cpu_to_le32(offset);
>       /* update L2 table */
>       BLKDBG_EVENT(extent->file, BLKDBG_L2_UPDATE);
> -    if (bdrv_pwrite_sync(extent->file,
> +    if (bdrv_pwrite(extent->file,
>                   ((int64_t)m_data->l2_offset * 512)
>                       + (m_data->l2_index * sizeof(offset)),
>                   &offset, sizeof(offset)) < 0) {

Worth reindenting ther est of the function call?

> @@ -1444,13 +1444,16 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>       /* 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,
> +        if (bdrv_pwrite(extent->file,
>                       ((int64_t)m_data->l2_offset * 512)
>                           + (m_data->l2_index * sizeof(offset)),
>                       &offset, sizeof(offset)) < 0) {
>               return VMDK_ERROR;
>           }
>       }
> +    if (bdrv_flush(extent->file->bs) < 0) {
> +        return VMDK_ERROR;
> +    }

But indentation doesn't affect correctness.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH 6/6] iotests: vmdk: Enable zeroed_grained=on by default

[Kevin Wolf]

In order to avoid bitrot in the zero cluster code in VMDK, enable
zero_grained=on by default for the tests.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/059   | 6 +++---
 tests/qemu-iotests/check | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 5438025285..4c90fc0363 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -41,9 +41,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt vmdk
 _supported_proto file
 _supported_os Linux
-_unsupported_imgopts "subformat=monolithicFlat" \
-                     "subformat=twoGbMaxExtentFlat" \
-                     "subformat=twoGbMaxExtentSparse"
+
+# We test all kinds of VMDK options here, so ignore user-specified options
+IMGOPTS=""
 
 capacity_offset=16
 granularity_offset=20
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index f7a2d3d6c3..9c461cf76d 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -546,6 +546,9 @@ fi
 if [ "$IMGFMT" == "luks" ] && ! (echo "$IMGOPTS" | grep "iter-time=" > /dev/null); then
     IMGOPTS=$(_optstr_add "$IMGOPTS" "iter-time=10")
 fi
+if [ "$IMGFMT" == "vmdk" ] && ! (echo "$IMGOPTS" | grep "zeroed_grain=" > /dev/null); then
+    IMGOPTS=$(_optstr_add "$IMGOPTS" "zeroed_grain=on")
+fi
 
 if [ -z "$SAMPLE_IMG_DIR" ]; then
         SAMPLE_IMG_DIR="$source_iotests/sample_images"
-- 
2.25.3

Re: [PATCH 6/6] iotests: vmdk: Enable zeroed_grained=on by default

[Eric Blake]

On 4/30/20 8:30 AM, Kevin Wolf wrote:
> In order to avoid bitrot in the zero cluster code in VMDK, enable
> zero_grained=on by default for the tests.

Here, you spell it zero_grained=on,

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/059   | 6 +++---
>   tests/qemu-iotests/check | 3 +++
>   2 files changed, 6 insertions(+), 3 deletions(-)

So you're changing the default for better coverage and speed, but 
ensuring that 59 still covers the (slower) zero_grained=off.  Seems 
reasonable.

Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ b/tests/qemu-iotests/check
> @@ -546,6 +546,9 @@ fi
>   if [ "$IMGFMT" == "luks" ] && ! (echo "$IMGOPTS" | grep "iter-time=" > /dev/null); then
>       IMGOPTS=$(_optstr_add "$IMGOPTS" "iter-time=10")
>   fi
> +if [ "$IMGFMT" == "vmdk" ] && ! (echo "$IMGOPTS" | grep "zeroed_grain=" > /dev/null); then

Here, zeroed_grain=.  Which is it?

> +    IMGOPTS=$(_optstr_add "$IMGOPTS" "zeroed_grain=on")

As a native speaker, my inclination is zero_grained; but I don't know 
the VMDK spec well enough to know if this is something in the spec, or 
just a term that qemu invented.  And since we already have existing 
usage of one spelling, switching the spelling now would require a 
deprecation period and is separate from this patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 6/6] iotests: vmdk: Enable zeroed_grained=on by default

[Kevin Wolf]

Am 30.04.2020 um 16:22 hat Eric Blake geschrieben:
> On 4/30/20 8:30 AM, Kevin Wolf wrote:
> > In order to avoid bitrot in the zero cluster code in VMDK, enable
> > zero_grained=on by default for the tests.
> 
> Here, you spell it zero_grained=on,

Thanks for spotting this, the typo is in the commit message.

> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   tests/qemu-iotests/059   | 6 +++---
> >   tests/qemu-iotests/check | 3 +++
> >   2 files changed, 6 insertions(+), 3 deletions(-)
> 
> So you're changing the default for better coverage and speed, but ensuring
> that 59 still covers the (slower) zero_grained=off.  Seems reasonable.

The real reason why I'm changing 059 is that zeroed_grain=on works only
with some subformats and the test case tests many different subformats,
including those for which it doesn't work.

Kevin

Re: [PATCH 0/6] vmdk: Fix zero cluster handling

[Kevin Wolf]

Am 30.04.2020 um 15:30 hat Kevin Wolf geschrieben:
> What I was really investigating is why 055 was so slow. I couldn't solve
> that, but instead I found out that our VMDK code for zero clusters and
> write_zeroes was completely broken. Apart from segfaults when zero
> clusters were actually enabled, this caused a compressed backup target
> to result in a bigger file than uncompressed with VMDK.
> 
> This series tries to fix it (with one bonus performance patch).

Thanks for the review, fixed up the commit messages and applied.

If you were curious about the VMDK terminology, I looked it up and the
basic terms translate to qcow2 like this:

* grain directory = L1 table
* grain table = L2 table
* grain = cluster

"zeroed-grain GTE (grain table entry)" is the exact term used in the
VMDK spec for what we would call a zero cluster in qcow2.

Kevin