Re: [Qemu-devel] [PATCH] vmdk: improve streamOptimized vmdk support

[Fam Zheng]

On Mon, 07/07 10:54, Milos Vyletel wrote:
> VMDK's streamOptimized format is different that regular sparse format.

s/that/from/

> L1(GD) and L2(GT) tables are not predefined but rather generated and
> written during image creation mainly because there is no way to tell
> how much space data will occupy once they are compressed. Also the
> location of header, L1 and L2 tables differs.

s/differs/differ/

> 
> - L2 tables (grain tables) are written after all grains they point to
> - L1 tables are written after all grains and L2 tables
> - footer at the end is used instead of header in first sector
> 
> This patch improves streamOptimized support and adds possibility to
> create true streamOptimized images using qemu-img. Some of the changes
> are from VMDK specs, some of them from hexdump-ing images from VMWare
> and VirtualBox.
> 
> I have compared these images to the ones generated by VMWare and vbox
> and they are identical with the exception of DescriptorFile that has
> some differences but none that would change behavior(CID and some
> additional DDB entries differ) and streamOptimized image generated from
> raw image was succesfully imported (as OVA) into VMWare ESXi and Oracle
> OVM.
> 
> Signed-off-by: Milos Vyletel <address@hidden>
> ---
>  block/vmdk.c |  363 
> +++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 281 insertions(+), 82 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 27a78da..f482225 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -81,6 +81,21 @@ typedef struct {
>      uint16_t compressAlgorithm;
>  } QEMU_PACKED VMDK4Header;
>  
> +typedef struct {
> +    uint64_t val;
> +    uint32_t size;
> +    uint32_t type;
> +    uint8_t pad[BDRV_SECTOR_SIZE - sizeof(uint64_t) - 2*sizeof(uint32_t)];
> +} QEMU_PACKED VMDK4MetaMarker;
> +
> +typedef struct {
> +    VMDK4MetaMarker footer_marker;
> +    uint32_t magic;
> +    VMDK4Header header;
> +    uint8_t pad[BDRV_SECTOR_SIZE - sizeof(uint32_t) - sizeof(VMDK4Header)];
> +    VMDK4MetaMarker eos_marker;
> +} QEMU_PACKED VMDK4Footer;
> +
>  #define L2_CACHE_SIZE 16
>  
>  typedef struct VmdkExtent {
> @@ -89,24 +104,29 @@ typedef struct VmdkExtent {
>      bool compressed;
>      bool has_marker;
>      bool has_zero_grain;
> +    bool has_footer;
>      int version;
>      int64_t sectors;
>      int64_t end_sector;
>      int64_t flat_start_offset;
>      int64_t l1_table_offset;
>      int64_t l1_backup_table_offset;
> +    uint32_t l1_index;

Could you track the allocation staus of grain table with l1_table entry value?
For those with value 0, we allocate grain table in file, and update its l1
entry. That way the fields l1_index and l2_table are not necessary here.

>      uint32_t *l1_table;
>      uint32_t *l1_backup_table;
>      unsigned int l1_size;
>      uint32_t l1_entry_sectors;
>  
>      unsigned int l2_size;
> +    uint32_t *l2_table;
>      uint32_t *l2_cache;
>      uint32_t l2_cache_offsets[L2_CACHE_SIZE];
>      uint32_t l2_cache_counts[L2_CACHE_SIZE];
>  
>      int64_t cluster_sectors;
>      char *type;
> +
> +    VMDK4Footer footer;
>  } VmdkExtent;

<snip>

>  
>  typedef struct BDRVVmdkState {
> @@ -1026,6 +1066,97 @@ static int vmdk_L2update(VmdkExtent *extent, 
> VmdkMetaData *m_data)
>      return VMDK_OK;
>  }
>  
> +static int vmdk_write_footer(BlockDriverState *bs,
> +                           VMDK4Footer *footer,
> +                           VmdkExtent *extent)

Bad alignment.

> +{
> +    int i, ret, gd_buf_size;
> +    uint32_t *gd_buf = NULL;
> +    uint32_t grains, gd_sectors, gt_size, gt_count;
> +    uint64_t offset;
> +    VMDK4Header header;
> +    VMDK4MetaMarker gd_marker;
> +
> +    header = footer->header;
> +    offset = le64_to_cpu(header.gd_offset);
> +
> +    grains = DIV_ROUND_UP(header.capacity, header.granularity);
> +    gt_size = DIV_ROUND_UP(header.num_gtes_per_gt * sizeof(uint32_t),
> +                           BDRV_SECTOR_SIZE);
> +    gt_count = DIV_ROUND_UP(grains, header.num_gtes_per_gt);
> +    gd_sectors = DIV_ROUND_UP(gt_count * sizeof(uint32_t), BDRV_SECTOR_SIZE);
> +
> +    /* write grain directory marker */
> +    memset(&gd_marker, 0, sizeof(gd_marker));
> +    gd_marker.val = cpu_to_le64(gd_sectors);
> +    gd_marker.type = cpu_to_le32(MARKER_GRAIN_DIRECTORY);
> +
> +    ret = bdrv_pwrite(bs, offset * BDRV_SECTOR_SIZE, &gd_marker, 
> sizeof(gd_marker));
> +    if (ret < 0)
> +        goto exit;

Always add braces around if body. Again, scripts/checkpatch.pl can help
check style issue.

> +    offset += sizeof(gd_marker) / BDRV_SECTOR_SIZE;
> +
> +    /* write grain directory */
> +    gd_buf_size = gd_sectors * BDRV_SECTOR_SIZE;
> +    gd_buf = g_malloc0(gd_buf_size);

gd_buf is never freed.

> +    if (extent) {
> +        /* copy over L1 table if we have it */
> +        for (i = 0; i < gt_count; i++) {
> +            gd_buf[i] = cpu_to_le32(extent->l1_table[i]);
> +        }
> +    }
> +    ret = bdrv_pwrite(bs, offset * BDRV_SECTOR_SIZE, gd_buf, gd_buf_size);
> +    if (ret < 0)
> +        goto exit;
> +
> +    /* save real gd_offset */
> +    footer->header.gd_offset = cpu_to_le64(offset);
> +    offset += gd_sectors;
> +
> +    /* write footer */
> +    ret = bdrv_pwrite(bs, offset * BDRV_SECTOR_SIZE, footer, 
> sizeof(*footer));
> +    if (ret < 0)
> +        goto exit;
> +
> +    ret = 0;
> + exit:
> +    return ret;
> +}

<snip>

>  static int get_cluster_offset(BlockDriverState *bs,
>                                      VmdkExtent *extent,
>                                      VmdkMetaData *m_data,
> @@ -1034,8 +1165,8 @@ static int get_cluster_offset(BlockDriverState *bs,
>                                      uint64_t *cluster_offset)
>  {
>      unsigned int l1_index, l2_offset, l2_index;
> -    int min_index, i, j;
> -    uint32_t min_count, *l2_table;
> +    int min_index, i, j, ret;
> +    uint32_t min_count;
>      bool zeroed = false;
>  
>      if (m_data) {
> @@ -1048,11 +1179,25 @@ static int get_cluster_offset(BlockDriverState *bs,
>  
>      offset -= (extent->end_sector - extent->sectors) * SECTOR_SIZE;
>      l1_index = (offset >> 9) / extent->l1_entry_sectors;
> -    if (l1_index >= extent->l1_size) {
> +    if (extent->compressed && l1_index &&
> +            extent->l1_index != l1_index) {
> +        ret = vmdk_write_grain_table(extent);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +    extent->l1_index = l1_index;
> +    if (extent->l1_index >= extent->l1_size) {
>          return VMDK_ERROR;
>      }
> -    l2_offset = extent->l1_table[l1_index];
> + retry:
> +    l2_offset = extent->l1_table[extent->l1_index];
> +
>      if (!l2_offset) {
> +        if (extent->compressed) {
> +            extent->l1_table[extent->l1_index] = 
> bdrv_getlength(extent->file);

This control flow desn't make sense. We just write a new grain table at the end
of file and update the l1_table entry.

> +            goto retry;
> +        }
>          return VMDK_UNALLOC;
>      }
>      for (i = 0; i < L2_CACHE_SIZE; i++) {