On Wed, Jul 30, 2014 at 3:51 AM, Fam Zheng <f...@redhat.com> wrote: > 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 <milos.vyle...@gmail.com> >> --- >> 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.
I possibly could and I'm trying that now. I can use VmdkMetaData structure that already has these values and leave VmdkExtent as it is. > >> 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++) { Thanks for the comments. I'm working on them and I hopefully have v2 ready tomorrow.