On Mon, 06/05 11:37, Ashijeet Acharya wrote: > On Mon, Jun 5, 2017 at 11:23 AM, Ashijeet Acharya > <ashijeetacha...@gmail.com> 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 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 *total_alloc_clusters) > > +{ > > + 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; > > + } > > + *total_alloc_clusters += nb_clusters; > > I have left it as it is for now as I wasn't sure about what you meant. > You can check my query I posted on v4 of this thread for more so that > we can solve this soon. :-)
What I meant is to change this to "*total_alloc_clusters = nb_clusters" here, and pass in a temporary variable from vmdk_pwritev, then do a "total_alloc_clusters += temp" there. This is more readable than the current way. Alternatively, rename "total_alloc_clusters" parameter of this function to "alloc_clusters_counter" and be done. Ideally a function's interface should be as localized and stateless as possible. Fam