[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] vmdk: improve streamOptimized vmdk support
From: |
Milos Vyletel |
Subject: |
Re: [Qemu-devel] [PATCH] vmdk: improve streamOptimized vmdk support |
Date: |
Thu, 31 Jul 2014 14:24:33 -0400 |
On Wed, Jul 30, 2014 at 3:51 AM, Fam Zheng <address@hidden> 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 <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.
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.
Re: [Qemu-devel] [PATCH] vmdk: improve streamOptimized vmdk support, Fam Zheng, 2014/07/30
- Re: [Qemu-devel] [PATCH] vmdk: improve streamOptimized vmdk support,
Milos Vyletel <=