[Qemu-devel] [PATCH v4 0/6] vmdk: zeroed-grain GTE support

[Qemu-devel] [PATCH v4 0/6] vmdk: zeroed-grain GTE support

[Fam Zheng]

Added support for zeroed-grain GTE to VMDK according to VMDK Spec 5.0[1].

[1] Virtual Disk Format 5.0 - VMware,
    http://www.vmware.com/support/developer/vddk/vmdk_50_technote.pdf?src=vmdk

Changes since v3:
 - 5/6: Remove tmp
 - 6/6: Update L2 Cache
        Remove redundant assignment to m_data
        Simplify l2_offset assignment to m_data
        Fix m_data.offset endian.

Changes since v2:
 - all: Added 5/6 (vmdk: store fields of VmdkMetaData in cpu endian)
 - 6/6: Avoid side-effect of vmdk_L2update.
        Change function comment to gtkdoc stype.
        Fix VMDK4_FLAG_ZG.

Changes since v1:
 - all: fix From: field
 - 1/5: squash one line of ret code macro change from 2/5
 - 2/5: change VMDK4_FLAG_ZG to VMDK4_FLAG_ZERO_GRAIN
 - 3/5: move BLOCK_OPT_ZEROED_GRAIN defination from block_int.h to vmdk.c
 - 5/5: fix metadata update issue, unit test with cases 033 034

Fam Zheng (6):
  vmdk: named return code.
  vmdk: add support for “zeroed‐grain” GTE
  vmdk: Add option to create zeroed-grain image
  vmdk: change magic number to macro
  vmdk: store fields of VmdkMetaData in cpu endian
  vmdk: add bdrv_co_write_zeroes

 block/vmdk.c | 208 +++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 145 insertions(+), 63 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v4 5/6] vmdk: store fields of VmdkMetaData in cpu endian

[Fam Zheng]

Previously VmdkMetaData.offset is stored little endian while other
fields are cpu endian. This changes offset to cpu endian and convert
before writing to image.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 0463d3b..d98f304 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -813,14 +813,15 @@ static int get_whole_cluster(BlockDriverState *bs,
 
 static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
 {
+    uint32_t offset;
+    QEMU_BUILD_BUG_ON(sizeof(offset) != sizeof(m_data->offset));
+    offset = cpu_to_le32(m_data->offset);
     /* update L2 table */
     if (bdrv_pwrite_sync(
                 extent->file,
                 ((int64_t)m_data->l2_offset * 512)
                     + (m_data->l2_index * sizeof(m_data->offset)),
-                &(m_data->offset),
-                sizeof(m_data->offset)
-            ) < 0) {
+                &offset, sizeof(offset)) < 0) {
         return VMDK_ERROR;
     }
     /* update backup L2 table */
@@ -830,8 +831,7 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
                     extent->file,
                     ((int64_t)m_data->l2_offset * 512)
                         + (m_data->l2_index * sizeof(m_data->offset)),
-                    &(m_data->offset), sizeof(m_data->offset)
-                ) < 0) {
+                    &offset, sizeof(offset)) < 0) {
             return VMDK_ERROR;
         }
     }
@@ -848,7 +848,7 @@ static int get_cluster_offset(BlockDriverState *bs,
 {
     unsigned int l1_index, l2_offset, l2_index;
     int min_index, i, j;
-    uint32_t min_count, *l2_table, tmp = 0;
+    uint32_t min_count, *l2_table;
     bool zeroed = false;
 
     if (m_data) {
@@ -924,8 +924,7 @@ static int get_cluster_offset(BlockDriverState *bs,
         }
 
         *cluster_offset >>= 9;
-        tmp = cpu_to_le32(*cluster_offset);
-        l2_table[l2_index] = tmp;
+        l2_table[l2_index] = cpu_to_le32(*cluster_offset);
 
         /* First of all we write grain itself, to avoid race condition
          * that may to corrupt the image.
@@ -938,7 +937,7 @@ static int get_cluster_offset(BlockDriverState *bs,
         }
 
         if (m_data) {
-            m_data->offset = tmp;
+            m_data->offset = *cluster_offset;
             m_data->l1_index = l1_index;
             m_data->l2_index = l2_index;
             m_data->l2_offset = l2_offset;
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 6/6] vmdk: add bdrv_co_write_zeroes

[Fam Zheng]

Use special offset to write zeroes efficiently, when zeroed-grain GTE is
available. If zero-write an allocated cluster, cluster is leaked because
its offset pointer is overwritten by "0x1".

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 68 insertions(+), 18 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index d98f304..d6dee7f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -124,6 +124,7 @@ typedef struct VmdkMetaData {
     unsigned int l2_index;
     unsigned int l2_offset;
     int valid;
+    uint32_t *l2_cache_entry;
 } VmdkMetaData;
 
 typedef struct VmdkGrainMarker {
@@ -835,6 +836,9 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
             return VMDK_ERROR;
         }
     }
+    if (m_data->l2_cache_entry) {
+        *m_data->l2_cache_entry = m_data->offset;
+    }
 
     return VMDK_OK;
 }
@@ -905,6 +909,14 @@ static int get_cluster_offset(BlockDriverState *bs,
     l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size;
     *cluster_offset = le32_to_cpu(l2_table[l2_index]);
 
+    if (m_data) {
+        m_data->valid = 1;
+        m_data->l1_index = l1_index;
+        m_data->l2_index = l2_index;
+        m_data->offset = *cluster_offset;
+        m_data->l2_offset = l2_offset;
+        m_data->l2_cache_entry = &l2_table[l2_index];
+    }
     if (extent->has_zero_grain && *cluster_offset == VMDK_GTE_ZEROED) {
         zeroed = true;
     }
@@ -938,10 +950,6 @@ static int get_cluster_offset(BlockDriverState *bs,
 
         if (m_data) {
             m_data->offset = *cluster_offset;
-            m_data->l1_index = l1_index;
-            m_data->l2_index = l2_index;
-            m_data->l2_offset = l2_offset;
-            m_data->valid = 1;
         }
     }
     *cluster_offset <<= 9;
@@ -1164,8 +1172,17 @@ static coroutine_fn int vmdk_co_read(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }
 
+/**
+ * vmdk_write:
+ * @zeroed:       buf is ignored (data is zero), use zeroed_grain GTE feature
+ * if possible, otherwise return -ENOTSUP.
+ * @zero_dry_run: used for zeroed == true only, don't update L2 table, just
+ *
+ * Returns: error code with 0 for success.
+ */
 static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
-                     const uint8_t *buf, int nb_sectors)
+                      const uint8_t *buf, int nb_sectors,
+                      bool zeroed, bool zero_dry_run)
 {
     BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent = NULL;
@@ -1211,7 +1228,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
                                         &cluster_offset);
             }
         }
-        if (ret) {
+        if (ret == VMDK_ERROR) {
             return -EINVAL;
         }
         extent_begin_sector = extent->end_sector - extent->sectors;
@@ -1221,17 +1238,34 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
         if (n > nb_sectors) {
             n = nb_sectors;
         }
-
-        ret = vmdk_write_extent(extent,
-                        cluster_offset, index_in_cluster * 512,
-                        buf, n, sector_num);
-        if (ret) {
-            return ret;
-        }
-        if (m_data.valid) {
-            /* update L2 tables */
-            if (vmdk_L2update(extent, &m_data) == -1) {
-                return -EIO;
+        if (zeroed) {
+            /* Do zeroed write, buf is ignored */
+            if (extent->has_zero_grain &&
+                    index_in_cluster == 0 &&
+                    n >= extent->cluster_sectors) {
+                n = extent->cluster_sectors;
+                if (!zero_dry_run) {
+                    m_data.offset = VMDK_GTE_ZEROED;
+                    /* update L2 tables */
+                    if (vmdk_L2update(extent, &m_data) != VMDK_OK) {
+                        return -EIO;
+                    }
+                }
+            } else {
+                return -ENOTSUP;
+            }
+        } else {
+            ret = vmdk_write_extent(extent,
+                            cluster_offset, index_in_cluster * 512,
+                            buf, n, sector_num);
+            if (ret) {
+                return ret;
+            }
+            if (m_data.valid) {
+                /* update L2 tables */
+                if (vmdk_L2update(extent, &m_data) != VMDK_OK) {
+                    return -EIO;
+                }
             }
         }
         nb_sectors -= n;
@@ -1257,7 +1291,22 @@ static coroutine_fn int vmdk_co_write(BlockDriverState *bs, int64_t sector_num,
     int ret;
     BDRVVmdkState *s = bs->opaque;
     qemu_co_mutex_lock(&s->lock);
-    ret = vmdk_write(bs, sector_num, buf, nb_sectors);
+    ret = vmdk_write(bs, sector_num, buf, nb_sectors, false, false);
+    qemu_co_mutex_unlock(&s->lock);
+    return ret;
+}
+
+static int coroutine_fn vmdk_co_write_zeroes(BlockDriverState *bs,
+                                             int64_t sector_num,
+                                             int nb_sectors)
+{
+    int ret;
+    BDRVVmdkState *s = bs->opaque;
+    qemu_co_mutex_lock(&s->lock);
+    ret = vmdk_write(bs, sector_num, NULL, nb_sectors, true, true);
+    if (!ret) {
+        ret = vmdk_write(bs, sector_num, NULL, nb_sectors, true, false);
+    }
     qemu_co_mutex_unlock(&s->lock);
     return ret;
 }
@@ -1737,6 +1786,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_reopen_prepare = vmdk_reopen_prepare,
     .bdrv_read      = vmdk_co_read,
     .bdrv_write     = vmdk_co_write,
+    .bdrv_co_write_zeroes = vmdk_co_write_zeroes,
     .bdrv_close     = vmdk_close,
     .bdrv_create    = vmdk_create,
     .bdrv_co_flush_to_disk  = vmdk_co_flush,
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v4 6/6] vmdk: add bdrv_co_write_zeroes

[Stefan Hajnoczi]

On Sun, Apr 28, 2013 at 11:27:59AM +0800, Fam Zheng wrote:
> @@ -835,6 +836,9 @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
>              return VMDK_ERROR;
>          }
>      }
> +    if (m_data->l2_cache_entry) {
> +        *m_data->l2_cache_entry = m_data->offset;
> +    }

l2_table is little-endian, use the local variable 'offset' instead of
m_data->offset.

Re: [Qemu-devel] [PATCH v4 0/6] vmdk: zeroed-grain GTE support

[Stefan Hajnoczi]

On Sun, Apr 28, 2013 at 11:27:57AM +0800, Fam Zheng wrote:
> Added support for zeroed-grain GTE to VMDK according to VMDK Spec 5.0[1].
> 
> [1] Virtual Disk Format 5.0 - VMware,
>     http://www.vmware.com/support/developer/vddk/vmdk_50_technote.pdf?src=vmdk
> 
> Changes since v3:
>  - 5/6: Remove tmp
>  - 6/6: Update L2 Cache
>         Remove redundant assignment to m_data
>         Simplify l2_offset assignment to m_data
>         Fix m_data.offset endian.
> 
> Changes since v2:
>  - all: Added 5/6 (vmdk: store fields of VmdkMetaData in cpu endian)
>  - 6/6: Avoid side-effect of vmdk_L2update.
>         Change function comment to gtkdoc stype.
>         Fix VMDK4_FLAG_ZG.
> 
> Changes since v1:
>  - all: fix From: field
>  - 1/5: squash one line of ret code macro change from 2/5
>  - 2/5: change VMDK4_FLAG_ZG to VMDK4_FLAG_ZERO_GRAIN
>  - 3/5: move BLOCK_OPT_ZEROED_GRAIN defination from block_int.h to vmdk.c
>  - 5/5: fix metadata update issue, unit test with cases 033 034
> 
> Fam Zheng (6):
>   vmdk: named return code.
>   vmdk: add support for “zeroed‐grain” GTE
>   vmdk: Add option to create zeroed-grain image
>   vmdk: change magic number to macro
>   vmdk: store fields of VmdkMetaData in cpu endian
>   vmdk: add bdrv_co_write_zeroes
> 
>  block/vmdk.c | 208 +++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 145 insertions(+), 63 deletions(-)

One issue with Patch 6.

Please send full patch series in future revisions.  This makes it easier
to review and apply without having to search for the previous thread.

Stefan