On Wed, Apr 24, 2013 at 08:44:35PM +0800, Fam Zheng wrote: > @@ -905,6 +905,13 @@ static int get_cluster_offset(BlockDriverState *bs, > l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size; > *cluster_offset = le32_to_cpu(l2_table[l2_index]); > > + if (m_data) { > + m_data->valid = 1; > + m_data->l1_index = l1_index; > + m_data->l2_index = l2_index; > + m_data->offset = *cluster_offset; > + m_data->l2_offset = extent->l1_table[m_data->l1_index];
This line can simply be: m_data->l2_offset = l2_offset; > + } Filling in m_data up here means that only the ->offset field needs to be filled in when we allocate a cluster further down. Right now the code is duplicated, but it just overwrites the fields with the same value again. > @@ -1222,17 +1238,34 @@ static int vmdk_write(BlockDriverState *bs, int64_t > sector_num, > if (n > nb_sectors) { > n = nb_sectors; > } > - > - ret = vmdk_write_extent(extent, > - cluster_offset, index_in_cluster * 512, > - buf, n, sector_num); > - if (ret) { > - return ret; > - } > - if (m_data.valid) { > - /* update L2 tables */ > - if (vmdk_L2update(extent, &m_data) == -1) { > - return -EIO; > + if (zeroed) { > + /* Do zeroed write, buf is ignored */ > + if (extent->has_zero_grain && > + index_in_cluster == 0 && > + n >= extent->cluster_sectors) { > + n = extent->cluster_sectors; > + if (!zero_dry_run) { > + m_data.offset = cpu_to_le32(VMDK_GTE_ZEROED); offset is host endian now! > + /* update L2 tables */ > + if (vmdk_L2update(extent, &m_data) != VMDK_OK) { Zeroing cluster-by-cluster is slow - vmdk_L2update() uses sync to flush the L2 update. The vmdk.c code isn't great for buffering up metadata changes and flushing them in a single operation though, so this is fine for now. > + return -EIO; > + } l2_cache[] has not been updated with the new VMDK_GTE_ZEROED offset.