Re: [Qemu-block] [PATCH v6 6/8] vmdk: New functions to assist allocating multiple clusters

2017-06-29 Thread Ashijeet Acharya
On Tue, Jun 27, 2017 at 1:32 PM, Fam Zheng  wrote:
> On Mon, 06/05 13:22, Ashijeet Acharya wrote:
>> +/*
>> + * vmdk_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.
>
> I don't think this matches the function body, the passed in *cluster_offset
> value is ignored.
>
>> + *
>> + * 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 vmdk_handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
>> + uint64_t offset, uint64_t *cluster_offset,
>> + int64_t *bytes, VmdkMetaData *m_data,
>> + bool allocate, uint32_t 
>> *alloc_clusters_counter)
>> +{
>> +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;
>> +
>> +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) << 
>> BDRV_SECTOR_BITS)
>> + - skip_start_bytes;
>> +} else {
>> +nb_clusters = 1;
>> +}
>> +*alloc_clusters_counter += nb_clusters;
>> +skip_end_bytes = skip_start_bytes + MIN(*bytes,
>> + extent->cluster_sectors * BDRV_SECTOR_SIZE
>> +- skip_start_bytes);
>
> I don't understand the MIN part, shouldn't skip_end_bytes simply be
> skip_start_bytes + *bytes?
>
>> +
>> +if (extent->has_zero_grain && cluster_sector == VMDK_GTE_ZEROED) {
>> +zeroed = true;
>> +}
>> +
>> +if (!cluster_sector || zeroed) {
>> +if (!allocate) {
>> +return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
>> +}
>> +
>> +cluster_sector = extent->next_cluster_sector;
>> +extent->next_cluster_sector += extent->cluster_sectors
>> +* nb_clusters;
>> +
>> +ret = vmdk_perform_cow(bs, extent, cluster_sector * 
>> BDRV_SECTOR_SIZE,
>> +   offset, skip_start_bytes,
>> +   skip_end_bytes);
>> +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;
>> +}
>> +}
>> +*cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
>> +return VMDK_OK;
>> +}
>> +
>> +/*
>> + * vmdk_alloc_clusters
>> + *
>> + * For a given offset on the virtual disk, find the cluster offset in vmdk
>> + * file. If the offset is not found, allocate a new cluster.
>> + *
>> + * If the cluster is newly allocated, m_data->nb_clusters is set to the 
>> number
>> + * of contiguous clusters that have been allocated. In this case, the other
>> + * fields of m_data are valid and contain information about the first 
>> allocated
>> + * cluster.
>> + *
>> + * Returns:
>> + *
>> + *   VMDK_OK:   on success and @cluster_offset was set
>> + *
>> + *   VMDK_UNALLOC:  if no clusters were allocated and @cluster_offset is
>> + *  set to zero
>> + *
>> + *   VMDK_ERROR:in error cases
>> + */
>> +static int vmdk_alloc_clusters(BlockDriverState *bs,
>> +   VmdkExtent *extent,
>> 

Re: [Qemu-block] [PATCH v6 6/8] vmdk: New functions to assist allocating multiple clusters

2017-06-27 Thread Fam Zheng
On Mon, 06/05 13:22, Ashijeet Acharya wrote:
> +/*
> + * vmdk_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.

I don't think this matches the function body, the passed in *cluster_offset
value is ignored.

> + *
> + * 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 vmdk_handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
> + uint64_t offset, uint64_t *cluster_offset,
> + int64_t *bytes, VmdkMetaData *m_data,
> + bool allocate, uint32_t *alloc_clusters_counter)
> +{
> +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;
> +
> +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) << 
> BDRV_SECTOR_BITS)
> + - skip_start_bytes;
> +} else {
> +nb_clusters = 1;
> +}
> +*alloc_clusters_counter += nb_clusters;
> +skip_end_bytes = skip_start_bytes + MIN(*bytes,
> + extent->cluster_sectors * BDRV_SECTOR_SIZE
> +- skip_start_bytes);

I don't understand the MIN part, shouldn't skip_end_bytes simply be
skip_start_bytes + *bytes?

> +
> +if (extent->has_zero_grain && cluster_sector == VMDK_GTE_ZEROED) {
> +zeroed = true;
> +}
> +
> +if (!cluster_sector || zeroed) {
> +if (!allocate) {
> +return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
> +}
> +
> +cluster_sector = extent->next_cluster_sector;
> +extent->next_cluster_sector += extent->cluster_sectors
> +* nb_clusters;
> +
> +ret = vmdk_perform_cow(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,
> +   offset, skip_start_bytes,
> +   skip_end_bytes);
> +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;
> +}
> +}
> +*cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
> +return VMDK_OK;
> +}
> +
> +/*
> + * vmdk_alloc_clusters
> + *
> + * For a given offset on the virtual disk, find the cluster offset in vmdk
> + * file. If the offset is not found, allocate a new cluster.
> + *
> + * If the cluster is newly allocated, m_data->nb_clusters is set to the 
> number
> + * of contiguous clusters that have been allocated. In this case, the other
> + * fields of m_data are valid and contain information about the first 
> allocated
> + * cluster.
> + *
> + * Returns:
> + *
> + *   VMDK_OK:   on success and @cluster_offset was set
> + *
> + *   VMDK_UNALLOC:  if no clusters were allocated and @cluster_offset is
> + *  set to zero
> + *
> + *   VMDK_ERROR:in error cases
> + */
> +static int vmdk_alloc_clusters(BlockDriverState *bs,
> +   VmdkExtent *extent,
> +   VmdkMetaData *m_data, uint64_t offset,
> +   bool allocate, uint64_t *cluster_offset,
> +   int64_t bytes,
> +  

[Qemu-block] [PATCH v6 6/8] vmdk: New functions to assist allocating multiple clusters

2017-06-05 Thread Ashijeet Acharya
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 
---
 block/vmdk.c | 192 +++
 1 file changed, 182 insertions(+), 10 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index fe2046b..b671dc9 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;
 }
 
+/*
+ * vmdk_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 vmdk_handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
+ uint64_t offset, uint64_t *cluster_offset,
+ int64_t *bytes, VmdkMetaData *m_data,
+ bool allocate, uint32_t *alloc_clusters_counter)
+{
+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;
+
+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) << BDRV_SECTOR_BITS)
+ - skip_start_bytes;
+} else {
+nb_clusters = 1;
+}
+*alloc_clusters_counter += nb_clusters;
+skip_end_bytes = skip_start_bytes + MIN(*bytes,
+ extent->cluster_sectors * BDRV_SECTOR_SIZE
+- skip_start_bytes);
+
+if (extent->has_zero_grain && cluster_sector == VMDK_GTE_ZEROED) {
+zeroed = true;
+}
+
+if (!cluster_sector || zeroed) {
+if (!allocate) {
+return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
+}
+
+cluster_sector = extent->next_cluster_sector;
+extent->next_cluster_sector += extent->cluster_sectors
+* nb_clusters;
+
+ret = vmdk_perform_cow(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,
+   offset, skip_start_bytes,
+   skip_end_bytes);
+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;
+}
+}
+*cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
+return VMDK_OK;
+}
+
+/*
+ * vmdk_alloc_clusters
+ *
+ * For a given offset on the virtual disk, find the cluster offset in vmdk
+ * file. If the offset is not found, allocate a new cluster.
+ *
+ * If the cluster is newly allocated, m_data->nb_clusters is set to the number
+ * of contiguous clusters that have been allocated. In this case, the other
+ * fields of m_data are valid and contain information about the first allocated
+ * cluster.
+ *
+ * Returns:
+ *
+ *   VMDK_OK:   on success a