diff mbox

[2/5] vmdk: add support for “zeroed‐grain” GTE

Message ID 1366343325-5252-3-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng April 19, 2013, 3:48 a.m. UTC 
From: Feiran Zheng <feiran.zheng@emc.com>

Introduced support for zeroed-grain GTE, as specified in Virtual Disk
Format 5.0[1].

    Recent VMware hosted platform products support a new “zeroed‐grain”
    grain table entry (GTE). The zeroed‐grain GTE returns all zeros on
    read.  In other words, the zeroed‐grain GTE indicates that a grain
    in the child disk is zero‐filled but does not actually occupy space
    in storage.  A sparse extent with zeroed‐grain GTE has the following
    in its header:

     * SparseExtentHeader.version = 2
     * SparseExtentHeader.flags has bit 2 set

    Other than the new flag and the possibly zeroed‐grain GTE, version 2
    sparse extents are identical to version 1.  Also, a zeroed‐grain GTE
    has value 0x1 in the GT table.

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

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

Comments

Stefan Hajnoczi April 19, 2013, 8:59 a.m. UTC | #1 
On Fri, Apr 19, 2013 at 11:48:42AM +0800, Fam Zheng wrote:
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 450a721..5e60940 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -33,10 +33,13 @@
>  #define VMDK4_MAGIC (('K' << 24) | ('D' << 16) | ('M' << 8) | 'V')
>  #define VMDK4_COMPRESSION_DEFLATE 1
>  #define VMDK4_FLAG_RGD (1 << 1)
> +/* Zeroed-grain enable bit */
> +#define VMDK4_FLAG_ZG   (1 << 2)

Please use a clear name like VMDK4_FLAG_ZERO_GRAIN.

> @@ -81,6 +84,8 @@ typedef struct VmdkExtent {
>      bool flat;
>      bool compressed;
>      bool has_marker;
> +    bool has_zero_grain;
> +    int version;

uint32_t according to the spec.  Please use fixed-size integer types
instead of int, long, etc which can change depending on the host
architecture.

> @@ -1181,7 +1193,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
>                                  sector_num << 9, !extent->compressed,
>                                  &cluster_offset);
>          if (extent->compressed) {
> -            if (ret == 0) {
> +            if (ret == VMDK_OK) {

Should this be squashed into the previous patch?
Fam Zheng April 19, 2013, 9:10 a.m. UTC | #2 
On Fri, 04/19 10:59, Stefan Hajnoczi wrote:
> On Fri, Apr 19, 2013 at 11:48:42AM +0800, Fam Zheng wrote:
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index 450a721..5e60940 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -33,10 +33,13 @@
> >  #define VMDK4_MAGIC (('K' << 24) | ('D' << 16) | ('M' << 8) | 'V')
> >  #define VMDK4_COMPRESSION_DEFLATE 1
> >  #define VMDK4_FLAG_RGD (1 << 1)
> > +/* Zeroed-grain enable bit */
> > +#define VMDK4_FLAG_ZG   (1 << 2)
> 
> Please use a clear name like VMDK4_FLAG_ZERO_GRAIN.
> 
> > @@ -81,6 +84,8 @@ typedef struct VmdkExtent {
> >      bool flat;
> >      bool compressed;
> >      bool has_marker;
> > +    bool has_zero_grain;
> > +    int version;
> 
> uint32_t according to the spec.  Please use fixed-size integer types
> instead of int, long, etc which can change depending on the host
> architecture.

This is an internal structure holding information for extent, used the
same way as BDRVVmdkState, there is no direct correspondence to file
header fields, so I think it should be OK, as `int qcow_version' is also
found in block/qcow2.h.

> 
> > @@ -1181,7 +1193,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
> >                                  sector_num << 9, !extent->compressed,
> >                                  &cluster_offset);
> >          if (extent->compressed) {
> > -            if (ret == 0) {
> > +            if (ret == VMDK_OK) {
> 
> Should this be squashed into the previous patch?

Yes.
Stefan Hajnoczi April 19, 2013, 11:22 a.m. UTC | #3 
On Fri, Apr 19, 2013 at 11:10 AM, Fam Zheng <famz@redhat.com> wrote:
> On Fri, 04/19 10:59, Stefan Hajnoczi wrote:
>> On Fri, Apr 19, 2013 at 11:48:42AM +0800, Fam Zheng wrote:
>> > diff --git a/block/vmdk.c b/block/vmdk.c
>> > index 450a721..5e60940 100644
>> > --- a/block/vmdk.c
>> > +++ b/block/vmdk.c
>> > @@ -33,10 +33,13 @@
>> >  #define VMDK4_MAGIC (('K' << 24) | ('D' << 16) | ('M' << 8) | 'V')
>> >  #define VMDK4_COMPRESSION_DEFLATE 1
>> >  #define VMDK4_FLAG_RGD (1 << 1)
>> > +/* Zeroed-grain enable bit */
>> > +#define VMDK4_FLAG_ZG   (1 << 2)
>>
>> Please use a clear name like VMDK4_FLAG_ZERO_GRAIN.
>>
>> > @@ -81,6 +84,8 @@ typedef struct VmdkExtent {
>> >      bool flat;
>> >      bool compressed;
>> >      bool has_marker;
>> > +    bool has_zero_grain;
>> > +    int version;
>>
>> uint32_t according to the spec.  Please use fixed-size integer types
>> instead of int, long, etc which can change depending on the host
>> architecture.
>
> This is an internal structure holding information for extent, used the
> same way as BDRVVmdkState, there is no direct correspondence to file
> header fields, so I think it should be OK, as `int qcow_version' is also
> found in block/qcow2.h.

What is the benefit of changing the type internally?

Stefan
diff mbox

Patch

diff --git a/block/vmdk.c b/block/vmdk.c
index 450a721..5e60940 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -33,10 +33,13 @@ 
 #define VMDK4_MAGIC (('K' << 24) | ('D' << 16) | ('M' << 8) | 'V')
 #define VMDK4_COMPRESSION_DEFLATE 1
 #define VMDK4_FLAG_RGD (1 << 1)
+/* Zeroed-grain enable bit */
+#define VMDK4_FLAG_ZG   (1 << 2)
 #define VMDK4_FLAG_COMPRESS (1 << 16)
 #define VMDK4_FLAG_MARKER (1 << 17)
 #define VMDK4_GD_AT_END 0xffffffffffffffffULL
 
+#define VMDK_GTE_ZEROED 0x1
 
 /* VMDK internal error codes */
 #define VMDK_OK      0
@@ -81,6 +84,8 @@  typedef struct VmdkExtent {
     bool flat;
     bool compressed;
     bool has_marker;
+    bool has_zero_grain;
+    int version;
     int64_t sectors;
     int64_t end_sector;
     int64_t flat_start_offset;
@@ -569,6 +574,8 @@  static int vmdk_open_vmdk4(BlockDriverState *bs,
     extent->compressed =
         le16_to_cpu(header.compressAlgorithm) == VMDK4_COMPRESSION_DEFLATE;
     extent->has_marker = le32_to_cpu(header.flags) & VMDK4_FLAG_MARKER;
+    extent->version = le32_to_cpu(header.version);
+    extent->has_zero_grain = le32_to_cpu(header.flags) & VMDK4_FLAG_ZG;
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
         /* free extent allocated by vmdk_add_extent */
@@ -839,6 +846,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;
+    bool zeroed = false;
 
     if (m_data) {
         m_data->valid = 0;
@@ -894,9 +902,13 @@  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 (!*cluster_offset) {
+    if (extent->has_zero_grain && *cluster_offset == VMDK_GTE_ZEROED) {
+        zeroed = true;
+    }
+
+    if (!*cluster_offset || zeroed) {
         if (!allocate) {
-            return VMDK_UNALLOC;
+            return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
         }
 
         /* Avoid the L2 tables update for the images that have snapshots. */
@@ -967,8 +979,8 @@  static int coroutine_fn vmdk_co_is_allocated(BlockDriverState *bs,
     ret = get_cluster_offset(bs, extent, NULL,
                             sector_num * 512, 0, &offset);
     qemu_co_mutex_unlock(&s->lock);
-    /* get_cluster_offset returning 0 means success */
-    ret = !ret;
+
+    ret = (ret == VMDK_OK || ret == VMDK_ZEROED);
 
     index_in_cluster = sector_num % extent->cluster_sectors;
     n = extent->cluster_sectors - index_in_cluster;
@@ -1111,9 +1123,9 @@  static int vmdk_read(BlockDriverState *bs, int64_t sector_num,
         if (n > nb_sectors) {
             n = nb_sectors;
         }
-        if (ret) {
+        if (ret != VMDK_OK) {
             /* if not allocated, try to read from parent image, if exist */
-            if (bs->backing_hd) {
+            if (bs->backing_hd && ret != VMDK_ZEROED) {
                 if (!vmdk_is_cid_valid(bs)) {
                     return -EINVAL;
                 }
@@ -1181,7 +1193,7 @@  static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
                                 sector_num << 9, !extent->compressed,
                                 &cluster_offset);
         if (extent->compressed) {
-            if (ret == 0) {
+            if (ret == VMDK_OK) {
                 /* Refuse write to allocated cluster for streamOptimized */
                 fprintf(stderr,
                         "VMDK: can't write to allocated cluster"