On Thu, Jun 1, 2017 at 7:27 PM, Fam Zheng <f...@redhat.com> wrote:
> On Sat, 04/22 10:43, Ashijeet Acharya wrote:
>> Introduce two new helper functions handle_alloc() and
>> vmdk_alloc_cluster_offset(). handle_alloc() helps to allocate multiple
>> clusters at once starting from a given offset on disk and performs COW
>> if necessary for first and last allocated clusters.
>> vmdk_alloc_cluster_offset() helps to return the offset of the first of
>> the many newly allocated clusters. Also, provide proper documentation
>> for both.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacha...@gmail.com>
>> ---
>> block/vmdk.c | 192
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 182 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 7862791..8d34cd9 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -136,6 +136,7 @@ typedef struct VmdkMetaData {
>> unsigned int l2_offset;
>> int valid;
>> uint32_t *l2_cache_entry;
>> + uint32_t nb_clusters;
>> } VmdkMetaData;
>>
>> typedef struct VmdkGrainMarker {
>> @@ -1242,6 +1243,174 @@ static int get_cluster_table(VmdkExtent *extent,
>> uint64_t offset,
>> return VMDK_OK;
>> }
>>
>> +/*
>> + * handle_alloc
>> + *
>> + * Allocate new clusters for an area that either is yet unallocated or
>> needs a
>> + * copy on write. If *cluster_offset is non_zero, clusters are only
>> allocated if
>> + * the new allocation can match the specified host offset.
>> + *
>> + * Returns:
>> + * VMDK_OK: if new clusters were allocated, *bytes may be decreased
>> if
>> + * the new allocation doesn't cover all of the requested
>> area.
>> + * *cluster_offset is updated to contain the offset of the
>> + * first newly allocated cluster.
>> + *
>> + * VMDK_UNALLOC: if no clusters could be allocated. *cluster_offset is
>> left
>> + * unchanged.
>> + *
>> + * VMDK_ERROR: in error cases
>> + */
>> +static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
>> + uint64_t offset, uint64_t *cluster_offset,
>> + int64_t *bytes, VmdkMetaData *m_data,
>> + bool allocate, uint32_t *total_alloc_clusters)
>
> Not super important but personally I always prefer to stick to a "vmdk_"
> prefix
> when naming local identifiers, so that ctags and git grep can take it easy.
Done.
>
>> +{
>> + int l1_index, l2_offset, l2_index;
>> + uint32_t *l2_table;
>> + uint32_t cluster_sector;
>> + uint32_t nb_clusters;
>> + bool zeroed = false;
>> + uint64_t skip_start_bytes, skip_end_bytes;
>> + int ret;
>> +
>> + ret = get_cluster_table(extent, offset, &l1_index, &l2_offset,
>> + &l2_index, &l2_table);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> + cluster_sector = le32_to_cpu(l2_table[l2_index]);
>> +
>> + skip_start_bytes = vmdk_find_offset_in_cluster(extent, offset);
>> + /* Calculate the number of clusters to look for. Here we truncate the
>> last
>> + * cluster, i.e. 1 less than the actual value calculated as we may need
>> to
>> + * perform COW for the last one. */
>> + nb_clusters = DIV_ROUND_UP(skip_start_bytes + *bytes,
>> + extent->cluster_sectors << BDRV_SECTOR_BITS) -
>> 1;
>
> Alignment could be improved: here ^
Done.
>
>> +
>> + nb_clusters = MIN(nb_clusters, extent->l2_size - l2_index);
>> + assert(nb_clusters <= INT_MAX);
>> +
>> + /* update bytes according to final nb_clusters value */
>> + if (nb_clusters != 0) {
>> + *bytes = ((nb_clusters * extent->cluster_sectors) << 9)
>
> Better use BDRV_SECTOR_BITS instead of 9.
Done.
>
>> + - skip_start_bytes;
>> + } else {
>> + nb_clusters = 1;
>> + }
>> + *total_alloc_clusters += nb_clusters;
>
> It is weird that you increment *total_alloc_clusters instead of simply
> assigning
> to it, because it's not clear why before reading the caller code.
>
I am incrementing it because we allocate clusters in every iteration
and *total_alloc_clusters contains number of clusters allocated in
total and not just in one iteration. So basically it is a sum of all
the m_data->nb_clusters present in every element of the linked list I
prepare.
> It's better if you just return nb_clusters from this function (either as a
> return value, or assign to *total_alloc_clusters), then do the accumulation in
> vmdk_pwritev by adding m_data->nb_clusters, which is simpler.
If I understood it correctly, you mean to say that I should add all
the m_data->nb_clusters present in my linked list inside
vmdk_pwritev() using a loop? If yes, I think that's what I have been
doing earlier by incrementing *total_alloc_clusters there itself and
is better, no?
Also, please correct me if I am wrong :-)
Ashijeet
