On Fri, Mar 31, 2017 at 12:56 PM, Fam Zheng <f...@redhat.com> wrote:
> On Sat, 03/25 16:48, Ashijeet Acharya wrote:
>> Include a next pointer in VmdkMetaData struct to point to the previous
>> allocated L2 table. Modify vmdk_L2update to start updating metadata for
>> allocation of multiple clusters at once.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacha...@gmail.com>
>> ---
>> block/vmdk.c | 131
>> ++++++++++++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 102 insertions(+), 29 deletions(-)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 3de8b8f..4517409 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -137,6 +137,8 @@ typedef struct VmdkMetaData {
>> int valid;
>> uint32_t *l2_cache_entry;
>> uint32_t nb_clusters;
>> + uint32_t offset;
>> + struct VmdkMetaData *next;
>> } VmdkMetaData;
>>
>> typedef struct VmdkGrainMarker {
>> @@ -1037,29 +1039,81 @@ static void vmdk_refresh_limits(BlockDriverState
>> *bs, Error **errp)
>> }
>> }
>>
>> -static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>> - uint32_t offset)
>> +static int vmdk_alloc_cluster_link_l2(VmdkExtent *extent,
>> + VmdkMetaData *m_data, bool zeroed)
>> {
>> - offset = cpu_to_le32(offset);
>> + int i;
>> + uint32_t offset, temp_offset;
>> +
>> + if (zeroed) {
>> + temp_offset = VMDK_GTE_ZEROED;
>> + } else {
>> + temp_offset = m_data->offset;
>> + }
>> +
>> + temp_offset = cpu_to_le32(temp_offset);
>> +
>> /* update L2 table */
>> - if (bdrv_pwrite_sync(extent->file,
>> + offset = temp_offset;
>> + for (i = 0; i < m_data->nb_clusters; i++) {
>> + if (bdrv_pwrite_sync(extent->file,
>> ((int64_t)m_data->l2_offset * 512)
>> - + (m_data->l2_index * sizeof(offset)),
>> - &offset, sizeof(offset)) < 0) {
>> - return VMDK_ERROR;
>> + + ((m_data->l2_index + i) * sizeof(offset)),
>> + &(offset), sizeof(offset)) < 0) {
>> + return VMDK_ERROR;
>> + }
>> + if (!zeroed) {
>> + offset += 128;
>> + }
>> }
>> +
>> /* update backup L2 table */
>> + offset = temp_offset;
>> if (extent->l1_backup_table_offset != 0) {
>> m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
>> - if (bdrv_pwrite_sync(extent->file,
>> - ((int64_t)m_data->l2_offset * 512)
>> - + (m_data->l2_index * sizeof(offset)),
>> - &offset, sizeof(offset)) < 0) {
>> - return VMDK_ERROR;
>> + for (i = 0; i < m_data->nb_clusters; i++) {
>> + if (bdrv_pwrite_sync(extent->file,
>> + ((int64_t)m_data->l2_offset * 512)
>> + + ((m_data->l2_index + i) * sizeof(offset)),
>> + &(offset), sizeof(offset)) < 0) {
>> + return VMDK_ERROR;
>> + }
>> + if (!zeroed) {
>> + offset += 128;
>> + }
>> }
>> }
>> +
>> + offset = temp_offset;
>> if (m_data->l2_cache_entry) {
>> - *m_data->l2_cache_entry = offset;
>> + for (i = 0; i < m_data->nb_clusters; i++) {
>> + *m_data->l2_cache_entry = offset;
>> + m_data->l2_cache_entry++;
>> +
>> + if (!zeroed) {
>> + offset += 128;
>> + }
>> + }
>> + }
>> +
>> + return VMDK_OK;
>> +}
>> +
>> +static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>> + bool zeroed)
>> +{
>> + int ret;
>> +
>> + while (m_data->next != NULL) {
>> + VmdkMetaData *next;
>> +
>> + ret = vmdk_alloc_cluster_link_l2(extent, m_data, zeroed);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> + next = m_data->next;
>> + m_data = next;
>
> I don't see why the next pointer is necessary. Also the tail is always
> unused,
> why do you need to allocate it?
If I don't allocate the tail, I was getting a segfault in vmdk_pwritev().
> But more importantly, I think you could further batch multiple updates in the
> same L2 table and only do one bdrv_pwrite_sync.
Wouldn't the l2_offset need to change change for every subsequent L2
table i.e. after ever 512 cluster boundary?
Ashijeet
>
> Fam
>
>> }
>>
>> return VMDK_OK;
>> @@ -1271,7 +1325,7 @@ exit:
>> */
>> static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
>> uint64_t offset, uint64_t *cluster_offset,
>> - int64_t *bytes, VmdkMetaData *m_data,
>> + int64_t *bytes, VmdkMetaData **m_data,
>> bool allocate, uint32_t *total_alloc_clusters)
>> {
>> int l1_index, l2_offset, l2_index;
>> @@ -1280,6 +1334,7 @@ static int handle_alloc(BlockDriverState *bs,
>> VmdkExtent *extent,
>> uint32_t nb_clusters;
>> bool zeroed = false;
>> uint64_t skip_start_bytes, skip_end_bytes;
>> + VmdkMetaData *old_m_data;
>> int ret;
>>
>> ret = get_cluster_table(extent, offset, &l1_index, &l2_offset,
>> @@ -1330,13 +1385,21 @@ static int handle_alloc(BlockDriverState *bs,
>> VmdkExtent *extent,
>> if (ret < 0) {
>> return ret;
>> }
>> - if (m_data) {
>> - m_data->valid = 1;
>> - m_data->l1_index = l1_index;
>> - m_data->l2_index = l2_index;
>> - m_data->l2_offset = l2_offset;
>> - m_data->l2_cache_entry = &l2_table[l2_index];
>> - m_data->nb_clusters = nb_clusters;
>> +
>> + if (*m_data) {
>> + old_m_data = *m_data;
>> + *m_data = g_malloc0(sizeof(**m_data));
>> +
>> + **m_data = (VmdkMetaData) {
>> + .valid = 1,
>> + .l1_index = l1_index,
>> + .l2_index = l2_index,
>> + .l2_offset = l2_offset,
>> + .l2_cache_entry = &l2_table[l2_index],
>> + .nb_clusters = nb_clusters,
>> + .offset = cluster_sector,
>> + .next = old_m_data,
>> + };
>> }
>> }
>> *cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
>> @@ -1365,7 +1428,7 @@ static int handle_alloc(BlockDriverState *bs,
>> VmdkExtent *extent,
>> */
>> static int vmdk_alloc_cluster_offset(BlockDriverState *bs,
>> VmdkExtent *extent,
>> - VmdkMetaData *m_data, uint64_t offset,
>> + VmdkMetaData **m_data, uint64_t offset,
>> bool allocate, uint64_t
>> *cluster_offset,
>> int64_t bytes,
>> uint32_t *total_alloc_clusters)
>> @@ -1385,8 +1448,8 @@ static int vmdk_alloc_cluster_offset(BlockDriverState
>> *bs,
>> new_cluster_offset = 0;
>> *cluster_offset = 0;
>> n_bytes = 0;
>> - if (m_data) {
>> - m_data->valid = 0;
>> + if (*m_data) {
>> + (*m_data)->valid = 0;
>> }
>>
>> /* due to L2 table margins all bytes may not get allocated at once */
>> @@ -1768,9 +1831,11 @@ static int vmdk_pwritev(BlockDriverState *bs,
>> uint64_t offset,
>> uint64_t cluster_offset;
>> uint64_t bytes_done = 0;
>> uint64_t extent_size;
>> - VmdkMetaData m_data;
>> + VmdkMetaData *m_data;
>> uint32_t total_alloc_clusters = 0;
>>
>> + m_data = g_malloc0(sizeof(*m_data));
>> +
>> if (DIV_ROUND_UP(offset, BDRV_SECTOR_SIZE) > bs->total_sectors) {
>> error_report("Wrong offset: offset=0x%" PRIx64
>> " total_sectors=0x%" PRIx64,
>> @@ -1779,6 +1844,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t
>> offset,
>> }
>>
>> while (bytes > 0) {
>> + m_data->next = NULL;
>> extent = find_extent(s, offset >> BDRV_SECTOR_BITS, extent);
>> if (!extent) {
>> return -EIO;
>> @@ -1825,7 +1891,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t
>> offset,
>> total_alloc_clusters;
>> if (!zero_dry_run) {
>> /* update L2 tables */
>> - if (vmdk_L2update(extent, &m_data, VMDK_GTE_ZEROED)
>> + if (vmdk_L2update(extent, m_data, zeroed)
>> != VMDK_OK) {
>> return -EIO;
>> }
>> @@ -1839,10 +1905,9 @@ static int vmdk_pwritev(BlockDriverState *bs,
>> uint64_t offset,
>> if (ret) {
>> return ret;
>> }
>> - if (m_data.valid) {
>> + if (m_data->valid) {
>> /* update L2 tables */
>> - if (vmdk_L2update(extent, &m_data,
>> - cluster_offset >> BDRV_SECTOR_BITS)
>> + if (vmdk_L2update(extent, m_data, zeroed)
>> != VMDK_OK) {
>> return -EIO;
>> }
>> @@ -1852,6 +1917,13 @@ static int vmdk_pwritev(BlockDriverState *bs,
>> uint64_t offset,
>> offset += n_bytes;
>> bytes_done += n_bytes;
>>
>> + while (m_data->next != NULL) {
>> + VmdkMetaData *next;
>> + next = m_data->next;
>> + g_free(m_data);
>> + m_data = next;
>> + }
>> +
>> /* update CID on the first write every time the virtual disk is
>> * opened */
>> if (!s->cid_updated) {
>> @@ -1862,6 +1934,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t
>> offset,
>> s->cid_updated = true;
>> }
>> }
>> + g_free(m_data);
>> return 0;
>> }
>>
>> --
>> 2.6.2
>>
>>
