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

[Kevin Wolf <kwolf@redhat.com>]

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