[PATCH 1/3] block/vmdk: align stream optimized data grains on 512 byte chunks

[PATCH 1/3] block/vmdk: align stream optimized data grains on 512 byte chunks

[Sanne Raymaekers]

According to the spec: "Each marker and its associated block begin on a
sector or 512‐byte boundary... There are five types of markers:
compressed grain markers,..." [^1]

This means that instead of aligning grains on their typical size (64K),
in stream optimized disks, grains should be aligned on 512 byte chunks.

[^1] Virtual Disk Format 5.0 Technical Note.

Signed-off-by: Sanne Raymaekers <sraymaek@redhat.com>
---
 block/vmdk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index cd8b4ec7c8..3cdca06fff 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1460,9 +1460,11 @@ get_whole_cluster(BlockDriverState *bs, VmdkExtent *extent,
     int64_t cluster_bytes;
     uint8_t *whole_grain;
     bool copy_from_backing;
+    BDRVVmdkState *s = bs->opaque;
 
     /* For COW, align request sector_num to cluster start */
-    cluster_bytes = extent->cluster_sectors << BDRV_SECTOR_BITS;
+    cluster_bytes = strcmp(s->create_type, "streamOptimized") == 0 ? SECTOR_SIZE
+      : 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;
-- 
2.55.0

[PATCH 2/3] block/vmdk: skip RGD for stream optimized disks

[Sanne Raymaekers]

Stream optimized disks the spec notes: "The rgdOffset should be ignored
because bit 1 of the flags field is not set." Where bit 1 of the flags
field indicates a redundant grain directory (RGD).[^1]

In short, stream optimized disks should have the RGD offset set to 0,
and only write the main grain directory.

[^1] Virtual Disk Format 5.0 Technical Note.

Signed-off-by: Sanne Raymaekers <sraymaek@redhat.com>
---
 block/vmdk.c | 76 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 3cdca06fff..040f38cd1d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2218,7 +2218,7 @@ vmdk_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
 
 static int coroutine_fn GRAPH_UNLOCKED
 vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
-                 bool zeroed_grain, Error **errp)
+                 bool zeroed_grain, bool stream_optimized, Error **errp)
 {
     int ret, i;
     VMDK4Header header;
@@ -2239,9 +2239,10 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
     } else {
         header.version = 1;
     }
-    header.flags = VMDK4_FLAG_RGD | VMDK4_FLAG_NL_DETECT
+    header.flags =  VMDK4_FLAG_NL_DETECT
                    | (compress ? VMDK4_FLAG_COMPRESS | VMDK4_FLAG_MARKER : 0)
-                   | (zeroed_grain ? VMDK4_FLAG_ZERO_GRAIN : 0);
+                   | (zeroed_grain ? VMDK4_FLAG_ZERO_GRAIN : 0)
+                   | (stream_optimized ? 0 : VMDK4_FLAG_RGD);
     header.compressAlgorithm = compress ? VMDK4_COMPRESSION_DEFLATE : 0;
     header.capacity = filesize / BDRV_SECTOR_SIZE;
     header.granularity = 128;
@@ -2255,8 +2256,15 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
 
     header.desc_offset = 1;
     header.desc_size = 20;
-    header.rgd_offset = header.desc_offset + header.desc_size;
-    header.gd_offset = header.rgd_offset + gd_sectors + (gt_size * gt_count);
+
+    if (stream_optimized) {
+        header.rgd_offset = 0;
+        header.gd_offset = header.desc_offset + header.desc_size;
+    } else {
+        header.rgd_offset = header.desc_offset + header.desc_size;
+        header.gd_offset = header.rgd_offset + gd_sectors + (gt_size * gt_count);
+    }
+
     header.grain_offset =
         ROUND_UP(header.gd_offset + gd_sectors + (gt_size * gt_count),
                  header.granularity);
@@ -2296,21 +2304,24 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
         goto exit;
     }
 
-    /* write grain directory */
+    /* write backup grain directory */
     gd_buf_size = gd_sectors * BDRV_SECTOR_SIZE;
     gd_buf = g_malloc0(gd_buf_size);
-    for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_sectors;
-         i < gt_count; i++, tmp += gt_size) {
-        gd_buf[i] = cpu_to_le32(tmp);
-    }
-    ret = blk_co_pwrite(blk, le64_to_cpu(header.rgd_offset) * BDRV_SECTOR_SIZE,
-                        gd_buf_size, gd_buf, 0);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret, "failed to write VMDK grain directory");
-        goto exit;
+
+    if (!stream_optimized) {
+        for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_sectors;
+             i < gt_count; i++, tmp += gt_size) {
+            gd_buf[i] = cpu_to_le32(tmp);
+        }
+        ret = blk_co_pwrite(blk, le64_to_cpu(header.rgd_offset) * BDRV_SECTOR_SIZE,
+                            gd_buf_size, gd_buf, 0);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "failed to write VMDK grain directory");
+            goto exit;
+        }
     }
 
-    /* write backup grain directory */
+    /* write grain directory */
     for (i = 0, tmp = le64_to_cpu(header.gd_offset) + gd_sectors;
          i < gt_count; i++, tmp += gt_size) {
         gd_buf[i] = cpu_to_le32(tmp);
@@ -2319,7 +2330,7 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
                         gd_buf_size, gd_buf, 0);
     if (ret < 0) {
         error_setg_errno(errp, -ret,
-                         "failed to write VMDK backup grain directory");
+                         "failed to write VMDK grain directory");
     }
 
     ret = 0;
@@ -2330,8 +2341,8 @@ exit:
 
 static int coroutine_fn GRAPH_UNLOCKED
 vmdk_create_extent(const char *filename, int64_t filesize, bool flat,
-                   bool compress, bool zeroed_grain, BlockBackend **pbb,
-                   QemuOpts *opts, Error **errp)
+                   bool compress, bool zeroed_grain, bool stream_optimized,
+                   BlockBackend **pbb, QemuOpts *opts, Error **errp)
 {
     int ret;
     BlockBackend *blk = NULL;
@@ -2351,7 +2362,8 @@ vmdk_create_extent(const char *filename, int64_t filesize, bool flat,
 
     blk_set_allow_write_beyond_eof(blk, true);
 
-    ret = vmdk_init_extent(blk, filesize, flat, compress, zeroed_grain, errp);
+    ret = vmdk_init_extent(blk, filesize, flat, compress, zeroed_grain, stream_optimized,
+                           errp);
 exit:
     if (blk) {
         if (pbb) {
@@ -2411,8 +2423,8 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
  */
 typedef BlockBackend * coroutine_fn GRAPH_UNLOCKED_PTR
     (*vmdk_create_extent_fn)(int64_t size, int idx, bool flat, bool split,
-                             bool compress, bool zeroed_grain, void *opaque,
-                             Error **errp);
+                             bool compress, bool zeroed_grain, bool stream_optimized,
+                             void *opaque, Error **errp);
 
 static void vmdk_desc_add_extent(GString *desc,
                                  const char *extent_line_fmt,
@@ -2534,6 +2546,7 @@ vmdk_co_do_create(int64_t size,
     }
     /* Get the descriptor file BDS */
     blk = extent_fn(created_size, 0, flat, split, compress, zeroed_grain,
+                    subformat == BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED,
                     opaque, errp);
     if (!blk) {
         ret = -EIO;
@@ -2586,8 +2599,9 @@ vmdk_co_do_create(int64_t size,
     extent_idx = 1;
     while (created_size < size) {
         int64_t cur_size = MIN(size - created_size, extent_size);
-        extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress,
-                               zeroed_grain, opaque, errp);
+        extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress, zeroed_grain,
+                               subformat == BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED,
+                               opaque, errp);
         if (!extent_blk) {
             ret = -EINVAL;
             goto exit;
@@ -2601,6 +2615,7 @@ vmdk_co_do_create(int64_t size,
 
     /* Check whether we got excess extents */
     extent_blk = extent_fn(-1, extent_idx, flat, split, compress, zeroed_grain,
+                           subformat == BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED,
                            opaque, NULL);
     if (extent_blk) {
         blk_co_unref(extent_blk);
@@ -2661,8 +2676,8 @@ typedef struct {
 
 static BlockBackend * coroutine_fn GRAPH_UNLOCKED
 vmdk_co_create_opts_cb(int64_t size, int idx, bool flat, bool split,
-                       bool compress, bool zeroed_grain, void *opaque,
-                       Error **errp)
+                       bool compress, bool zeroed_grain, bool stream_optimized,
+                       void *opaque, Error **errp)
 {
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
@@ -2691,8 +2706,8 @@ vmdk_co_create_opts_cb(int64_t size, int idx, bool flat, bool split,
     g_free(rel_filename);
 
     if (vmdk_create_extent(ext_filename, size,
-                           flat, compress, zeroed_grain, &blk, data->opts,
-                           errp)) {
+                           flat, compress, zeroed_grain, stream_optimized, &blk,
+                           data->opts, errp)) {
         goto exit;
     }
     bdrv_co_unref(bs);
@@ -2812,7 +2827,7 @@ exit:
 
 static BlockBackend * coroutine_fn GRAPH_UNLOCKED
 vmdk_co_create_cb(int64_t size, int idx, bool flat, bool split, bool compress,
-                  bool zeroed_grain, void *opaque, Error **errp)
+                  bool zeroed_grain, bool stream_optimized, void *opaque, Error **errp)
 {
     int ret;
     BlockDriverState *bs;
@@ -2852,7 +2867,8 @@ vmdk_co_create_cb(int64_t size, int idx, bool flat, bool split, bool compress,
     bdrv_co_unref(bs);
 
     if (size != -1) {
-        ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
+        ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, stream_optimized,
+                               errp);
         if (ret) {
             blk_co_unref(blk);
             blk = NULL;
-- 
2.55.0

[PATCH 3/3] block/vmdk: add eos marker for stream optimized disks

[Sanne Raymaekers]

The end of stream marker should be at the very end of a stream optimized
disk. It consists of a regular marker struct with `val`, `size` and
`type` set to 0. It is then padded with more 0s to fill out a
sector. This effectively results in 512 zero bytes.[^1]

[^1] Virtual Disk Format 5.0 Technical Note.

Signed-off-by: Sanne Raymaekers <sraymaek@redhat.com>
---
 block/vmdk.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 040f38cd1d..c10321146e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -195,6 +195,13 @@ typedef struct VmdkGrainMarker {
     uint8_t  data[];
 } QEMU_PACKED VmdkGrainMarker;
 
+typedef struct EosMarker {
+   uint64_t val;
+   uint32_t size;
+   uint32_t type;
+   uint8_t pad[512 - 16];
+} QEMU_PACKED EosMarker;
+
 enum {
     MARKER_END_OF_STREAM    = 0,
     MARKER_GRAIN_TABLE      = 1,
@@ -987,12 +994,7 @@ vmdk_open_vmdk4(BlockDriverState *bs, BdrvChild *file, int flags,
             VMDK4Header header;
             uint8_t pad[512 - 4 - sizeof(VMDK4Header)];
 
-            struct {
-                uint64_t val;
-                uint32_t size;
-                uint32_t type;
-                uint8_t pad[512 - 16];
-            } QEMU_PACKED eos_marker;
+            EosMarker eos_marker;
         } QEMU_PACKED footer;
 
         ret = bdrv_pread(file, bs->file->bs->total_sectors * 512 - 1536,
@@ -2180,6 +2182,7 @@ vmdk_co_pwritev_compressed(BlockDriverState *bs, int64_t offset, int64_t bytes,
         BDRVVmdkState *s = bs->opaque;
         int i, ret;
         int64_t length;
+        EosMarker *eos_marker = NULL;
 
         for (i = 0; i < s->num_extents; i++) {
             length = bdrv_co_getlength(s->extents[i].file->bs);
@@ -2193,6 +2196,20 @@ vmdk_co_pwritev_compressed(BlockDriverState *bs, int64_t offset, int64_t bytes,
                 return ret;
             }
         }
+
+        /* Stream optimized disks end with an EOS marker, consisting of 512 null bytes. */
+        if (strcmp(s->create_type, "streamOptimized") == 0) {
+            eos_marker = g_malloc(sizeof(EosMarker));
+            memset(eos_marker, 0, sizeof(EosMarker));
+            length = bdrv_co_getlength(s->extents[s->num_extents - 1].file->bs);
+            ret = bdrv_co_pwrite(s->extents[s->num_extents - 1].file, length,
+                                 sizeof(EosMarker), eos_marker, 0);
+            g_free(eos_marker);
+            if (ret < 0) {
+                return ret;
+            }
+        }
+
         return 0;
     }
     return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
-- 
2.55.0

Re: [PATCH 1/3] block/vmdk: align stream optimized data grains on 512 byte chunks

[Kevin Wolf]

Am 05.10.2026 um 15:33 hat Sanne Raymaekers geschrieben:
> According to the spec: "Each marker and its associated block begin on a
> sector or 512‐byte boundary... There are five types of markers:
> compressed grain markers,..." [^1]
> 
> This means that instead of aligning grains on their typical size (64K),
> in stream optimized disks, grains should be aligned on 512 byte chunks.
> 
> [^1] Virtual Disk Format 5.0 Technical Note.
> 
> Signed-off-by: Sanne Raymaekers <sraymaek@redhat.com>

One small thing about the series as a whole first: It's conventional for
QEMU patch series (with more than one patch) to include a cover letter
(i.e. [PATCH 0/3]) that gives you the chance to explain the big picture
and what the series is about, and others to distinguish what is a reply
to the series as a whole as opposed to the first patch specifically. Not
a big deal, but you can just use git format-patch --cover-letter for v2
and edit the cover letter file before sending the series.

For this patch specifically, it shows a real problem in the VMDK code,
but I think the solution isn't quite right. What currently happens when
you write a compressed 64k cluster:

    1. get_cluster_offset() finds that the offset is unallocated and
       gets a new cluster_offset

    2. get_whole_cluster() is normally supposed to initialise those
       parts of a newly allocated cluster (with zeros or from the
       backing file) that won't be written to.

       Because we pass skip_start_bytes = skip_end_bytes = 0 for
       compressed clusters, zeros are written to the whole 64k starting
       from cluster_offset.

    3. vmdk_write_extent() writes the compressed data at cluster_offset,
       overwriting the zeros we just wrote in step 2. The data written
       here can be much smaller than 64k if it compresses well.

If this happen near the end of the image, the 64k write has a side
effect of leaving the image larger than intended.

Your patch replaces this with writing only 512 bytes of zeros in step 2,
however they are still immediately overwritten again by the real
compressed data. So it's not necessarily that the write has the wrong
size, but that it's completely unnecessary I/O - compressed writes
always cover the whole cluster, there is no rest that should get copied
from the backing file.

So I'm wondering if the better fix would be calling get_whole_cluster()
only if !extent->compressed in the first place, saving us the memory
allocation and the I/O that will be immediately overwritten again
anyway.

You could also argue that skip_end_bytes should be passed as n_bytes or
extent->cluster_sectors * BDRV_SECTOR_SIZE for compressed extents,
indicating that there is nothing to be copied, but even then skipping
get_whole_cluster() would save us a useless memory allocation.

>  block/vmdk.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index cd8b4ec7c8..3cdca06fff 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1460,9 +1460,11 @@ get_whole_cluster(BlockDriverState *bs, VmdkExtent *extent,
>      int64_t cluster_bytes;
>      uint8_t *whole_grain;
>      bool copy_from_backing;
> +    BDRVVmdkState *s = bs->opaque;
>  
>      /* For COW, align request sector_num to cluster start */
> -    cluster_bytes = extent->cluster_sectors << BDRV_SECTOR_BITS;
> +    cluster_bytes = strcmp(s->create_type, "streamOptimized") == 0 ? SECTOR_SIZE
> +      : extent->cluster_sectors << BDRV_SECTOR_BITS;

Doing string comparisons for each request isn't great. I think
extent->compressed should have the same meaning.

>      offset = QEMU_ALIGN_DOWN(offset, cluster_bytes);
>      whole_grain = qemu_blockalign(bs, cluster_bytes);
>      copy_from_backing = bs->backing && !zeroed;

Kevin

Re: [PATCH 2/3] block/vmdk: skip RGD for stream optimized disks

[Kevin Wolf]

Am 05.10.2026 um 15:33 hat Sanne Raymaekers geschrieben:
> Stream optimized disks the spec notes: "The rgdOffset should be ignored
> because bit 1 of the flags field is not set." Where bit 1 of the flags
> field indicates a redundant grain directory (RGD).[^1]
> 
> In short, stream optimized disks should have the RGD offset set to 0,
> and only write the main grain directory.
> 
> [^1] Virtual Disk Format 5.0 Technical Note.
> 
> Signed-off-by: Sanne Raymaekers <sraymaek@redhat.com>
> ---
>  block/vmdk.c | 76 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 30 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 3cdca06fff..040f38cd1d 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -2218,7 +2218,7 @@ vmdk_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
>  
>  static int coroutine_fn GRAPH_UNLOCKED
>  vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
> -                 bool zeroed_grain, Error **errp)
> +                 bool zeroed_grain, bool stream_optimized, Error **errp)

You changed quite a few functions to add a new stream_optimized
parameter. This seems unnecessary as there is already a compress
parameter, which originates in vmdk_co_do_create() like this:

    compress = subformat == BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED;

So stream_optimized duplicates the same information.

>  {
>      int ret, i;
>      VMDK4Header header;
> @@ -2239,9 +2239,10 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
>      } else {
>          header.version = 1;
>      }
> -    header.flags = VMDK4_FLAG_RGD | VMDK4_FLAG_NL_DETECT
> +    header.flags =  VMDK4_FLAG_NL_DETECT

Minor formatting issue: There are two spaces before VMDK4_FLAG_NL_DETECT.

>                     | (compress ? VMDK4_FLAG_COMPRESS | VMDK4_FLAG_MARKER : 0)
> -                   | (zeroed_grain ? VMDK4_FLAG_ZERO_GRAIN : 0);
> +                   | (zeroed_grain ? VMDK4_FLAG_ZERO_GRAIN : 0)
> +                   | (stream_optimized ? 0 : VMDK4_FLAG_RGD);
>      header.compressAlgorithm = compress ? VMDK4_COMPRESSION_DEFLATE : 0;
>      header.capacity = filesize / BDRV_SECTOR_SIZE;
>      header.granularity = 128;
> @@ -2255,8 +2256,15 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
>  
>      header.desc_offset = 1;
>      header.desc_size = 20;
> -    header.rgd_offset = header.desc_offset + header.desc_size;
> -    header.gd_offset = header.rgd_offset + gd_sectors + (gt_size * gt_count);
> +
> +    if (stream_optimized) {
> +        header.rgd_offset = 0;
> +        header.gd_offset = header.desc_offset + header.desc_size;
> +    } else {
> +        header.rgd_offset = header.desc_offset + header.desc_size;
> +        header.gd_offset = header.rgd_offset + gd_sectors + (gt_size * gt_count);
> +    }
> +
>      header.grain_offset =
>          ROUND_UP(header.gd_offset + gd_sectors + (gt_size * gt_count),
>                   header.granularity);
> @@ -2296,21 +2304,24 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
>          goto exit;
>      }
>  
> -    /* write grain directory */
> +    /* write backup grain directory */

Heh, good point, the existing code had them reversed in the comments.

It would be good to mention this in the commit message because it came
as a surprise that you seemingly swapped them.

>      gd_buf_size = gd_sectors * BDRV_SECTOR_SIZE;
>      gd_buf = g_malloc0(gd_buf_size);
> -    for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_sectors;
> -         i < gt_count; i++, tmp += gt_size) {
> -        gd_buf[i] = cpu_to_le32(tmp);
> -    }
> -    ret = blk_co_pwrite(blk, le64_to_cpu(header.rgd_offset) * BDRV_SECTOR_SIZE,
> -                        gd_buf_size, gd_buf, 0);
> -    if (ret < 0) {
> -        error_setg_errno(errp, -ret, "failed to write VMDK grain directory");
> -        goto exit;
> +
> +    if (!stream_optimized) {
> +        for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_sectors;
> +             i < gt_count; i++, tmp += gt_size) {
> +            gd_buf[i] = cpu_to_le32(tmp);
> +        }
> +        ret = blk_co_pwrite(blk, le64_to_cpu(header.rgd_offset) * BDRV_SECTOR_SIZE,
> +                            gd_buf_size, gd_buf, 0);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "failed to write VMDK grain directory");

This should probably be "failed to write VMDK backup grain directory"
(you removed the "backup" below, but didn't add it here).

> +            goto exit;
> +        }
>      }
>  
> -    /* write backup grain directory */
> +    /* write grain directory */
>      for (i = 0, tmp = le64_to_cpu(header.gd_offset) + gd_sectors;
>           i < gt_count; i++, tmp += gt_size) {
>          gd_buf[i] = cpu_to_le32(tmp);
> @@ -2319,7 +2330,7 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
>                          gd_buf_size, gd_buf, 0);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret,
> -                         "failed to write VMDK backup grain directory");
> +                         "failed to write VMDK grain directory");
>      }
>  
>      ret = 0;

Kevin

Re: [PATCH 3/3] block/vmdk: add eos marker for stream optimized disks

[Kevin Wolf]

Am 05.10.2026 um 15:33 hat Sanne Raymaekers geschrieben:
> The end of stream marker should be at the very end of a stream optimized
> disk. It consists of a regular marker struct with `val`, `size` and
> `type` set to 0. It is then padded with more 0s to fill out a
> sector. This effectively results in 512 zero bytes.[^1]
> 
> [^1] Virtual Disk Format 5.0 Technical Note.
> 
> Signed-off-by: Sanne Raymaekers <sraymaek@redhat.com>
> ---
>  block/vmdk.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 040f38cd1d..c10321146e 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -195,6 +195,13 @@ typedef struct VmdkGrainMarker {
>      uint8_t  data[];
>  } QEMU_PACKED VmdkGrainMarker;
>  
> +typedef struct EosMarker {
> +   uint64_t val;
> +   uint32_t size;
> +   uint32_t type;
> +   uint8_t pad[512 - 16];
> +} QEMU_PACKED EosMarker;
> +
>  enum {
>      MARKER_END_OF_STREAM    = 0,
>      MARKER_GRAIN_TABLE      = 1,
> @@ -987,12 +994,7 @@ vmdk_open_vmdk4(BlockDriverState *bs, BdrvChild *file, int flags,
>              VMDK4Header header;
>              uint8_t pad[512 - 4 - sizeof(VMDK4Header)];
>  
> -            struct {
> -                uint64_t val;
> -                uint32_t size;
> -                uint32_t type;
> -                uint8_t pad[512 - 16];
> -            } QEMU_PACKED eos_marker;
> +            EosMarker eos_marker;
>          } QEMU_PACKED footer;
>  
>          ret = bdrv_pread(file, bs->file->bs->total_sectors * 512 - 1536,
> @@ -2180,6 +2182,7 @@ vmdk_co_pwritev_compressed(BlockDriverState *bs, int64_t offset, int64_t bytes,
>          BDRVVmdkState *s = bs->opaque;
>          int i, ret;
>          int64_t length;
> +        EosMarker *eos_marker = NULL;
>  
>          for (i = 0; i < s->num_extents; i++) {
>              length = bdrv_co_getlength(s->extents[i].file->bs);
> @@ -2193,6 +2196,20 @@ vmdk_co_pwritev_compressed(BlockDriverState *bs, int64_t offset, int64_t bytes,
>                  return ret;
>              }
>          }

Is there any way s->num_extents could not be 1 for streamOptimized
images?

If not, I think you can delete the whole existing for loop (which will
execute only once). bdrv_co_truncate() extends the file size, but then
you write at a higher offset below, which will extend it a second time,
so the bdrv_co_truncate() call didn't do anything useful.

> +        /* Stream optimized disks end with an EOS marker, consisting of 512 null bytes. */
> +        if (strcmp(s->create_type, "streamOptimized") == 0) {

This is vmdk_co_pwritev_compressed(). If we're here, we already know
that this is a streamOptimized image.

> +            eos_marker = g_malloc(sizeof(EosMarker));
> +            memset(eos_marker, 0, sizeof(EosMarker));

This can be combined into g_malloc0().

In fact, using qemu_blockalign0() and declaring eos_marker as
QEMU_AUTO_VFREE would be even better because it makes sure that the
buffer has the required memory alignment; otherwise, a lower layer may
have to copy the data into a bounce buffer.

> +            length = bdrv_co_getlength(s->extents[s->num_extents - 1].file->bs);

Unless I'm mistaken above, this is always s->extents[0].

bdrv_co_getlength() errors must be handled, like in the for loop above.

> +            ret = bdrv_co_pwrite(s->extents[s->num_extents - 1].file, length,
> +                                 sizeof(EosMarker), eos_marker, 0);

When removing the truncate, length would become QEMU_ALIGN_UP(length,
512) here instead.

> +            g_free(eos_marker);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
> +
>          return 0;
>      }
>      return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);

Kevin

Re: [PATCH 1/3] block/vmdk: align stream optimized data grains on 512 byte chunks

[Sanne Raymaekers]

Hi Kevin,

Thank you for the review.

On Wed, Oct 7, 2026 at 4:28 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 05.10.2026 um 15:33 hat Sanne Raymaekers geschrieben:
> > According to the spec: "Each marker and its associated block begin on a
> > sector or 512‐byte boundary... There are five types of markers:
> > compressed grain markers,..." [^1]
> >
> > This means that instead of aligning grains on their typical size (64K),
> > in stream optimized disks, grains should be aligned on 512 byte chunks.
> >
> > [^1] Virtual Disk Format 5.0 Technical Note.
> >
> > Signed-off-by: Sanne Raymaekers <sraymaek@redhat.com>
>
> One small thing about the series as a whole first: It's conventional for
> QEMU patch series (with more than one patch) to include a cover letter
> (i.e. [PATCH 0/3]) that gives you the chance to explain the big picture
> and what the series is about, and others to distinguish what is a reply
> to the series as a whole as opposed to the first patch specifically. Not
> a big deal, but you can just use git format-patch --cover-letter for v2
> and edit the cover letter file before sending the series.


Ack, will add that in v2.

>
>
> For this patch specifically, it shows a real problem in the VMDK code,
> but I think the solution isn't quite right. What currently happens when
> you write a compressed 64k cluster:
>
>     1. get_cluster_offset() finds that the offset is unallocated and
>        gets a new cluster_offset
>
>     2. get_whole_cluster() is normally supposed to initialise those
>        parts of a newly allocated cluster (with zeros or from the
>        backing file) that won't be written to.
>
>        Because we pass skip_start_bytes = skip_end_bytes = 0 for
>        compressed clusters, zeros are written to the whole 64k starting
>        from cluster_offset.
>
>     3. vmdk_write_extent() writes the compressed data at cluster_offset,
>        overwriting the zeros we just wrote in step 2. The data written
>        here can be much smaller than 64k if it compresses well.
>
> If this happen near the end of the image, the 64k write has a side
> effect of leaving the image larger than intended.
>
> Your patch replaces this with writing only 512 bytes of zeros in step 2,
> however they are still immediately overwritten again by the real
> compressed data. So it's not necessarily that the write has the wrong
> size, but that it's completely unnecessary I/O - compressed writes
> always cover the whole cluster, there is no rest that should get copied
> from the backing file.

So if I read this correctly, while vmdk_pwritev always passes the
cluster size (128 (granularity) * 512 (sector size) = 64k) to
vmdk_write_extent, vmdk_write_extent will still write variable length
data grain markers.  This is in spite of the "n_bytes >
(extent->cluster_sectors * SECTOR_SIZE) || (n_bytes <
(extent->cluster_sectors * SECTOR_SIZE)" check at the start of
vmdk_write_extent. vmdk_write_extent will just make sure it aligns the
next sector on 512 bytes.

>
>
> So I'm wondering if the better fix would be calling get_whole_cluster()
> only if !extent->compressed in the first place, saving us the memory
> allocation and the I/O that will be immediately overwritten again
> anyway.
>
> You could also argue that skip_end_bytes should be passed as n_bytes or
> extent->cluster_sectors * BDRV_SECTOR_SIZE for compressed extents,
> indicating that there is nothing to be copied, but even then skipping
> get_whole_cluster() would save us a useless memory allocation.


So I think that makes sense. Because vmdk_write_extent ensures writes
are always aligned on 512 byte chunks there's no reason to check and
allocate it beforehand.

>
>
> >  block/vmdk.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index cd8b4ec7c8..3cdca06fff 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -1460,9 +1460,11 @@ get_whole_cluster(BlockDriverState *bs, VmdkExtent *extent,
> >      int64_t cluster_bytes;
> >      uint8_t *whole_grain;
> >      bool copy_from_backing;
> > +    BDRVVmdkState *s = bs->opaque;
> >
> >      /* For COW, align request sector_num to cluster start */
> > -    cluster_bytes = extent->cluster_sectors << BDRV_SECTOR_BITS;
> > +    cluster_bytes = strcmp(s->create_type, "streamOptimized") == 0 ? SECTOR_SIZE
> > +      : extent->cluster_sectors << BDRV_SECTOR_BITS;
>
> Doing string comparisons for each request isn't great. I think
> extent->compressed should have the same meaning.


It makes sense to use compressed as a proxy. I wasn't 100% sure that no
other VMDK formats include compressed extents, but I guess not.

>
>
> >      offset = QEMU_ALIGN_DOWN(offset, cluster_bytes);
> >      whole_grain = qemu_blockalign(bs, cluster_bytes);
> >      copy_from_backing = bs->backing && !zeroed;
>
> Kevin
>

Thanks!


-- 
Kind regards,
Sanne