[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 6/8] vmdk: New functions to assist allocating
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v5 6/8] vmdk: New functions to assist allocating multiple clusters |
Date: |
Mon, 5 Jun 2017 14:39:30 +0800 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Mon, 06/05 11:37, Ashijeet Acharya wrote:
> On Mon, Jun 5, 2017 at 11:23 AM, Ashijeet Acharya
> <address@hidden> 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 <address@hidden>
> > ---
> > 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
- [Qemu-devel] [PATCH v5 0/8] Optimize VMDK I/O by allocating multiple clusters, Ashijeet Acharya, 2017/06/05
- [Qemu-devel] [PATCH v5 1/8] vmdk: Move vmdk_find_offset_in_cluster() to the top, Ashijeet Acharya, 2017/06/05
- [Qemu-devel] [PATCH v5 2/8] vmdk: Rename get_whole_cluster() to vmdk_perform_cow(), Ashijeet Acharya, 2017/06/05
- [Qemu-devel] [PATCH v5 3/8] vmdk: Rename get_cluster_offset() to vmdk_get_cluster_offset(), Ashijeet Acharya, 2017/06/05
- [Qemu-devel] [PATCH v5 4/8] vmdk: Factor out metadata loading code out of vmdk_get_cluster_offset(), Ashijeet Acharya, 2017/06/05
- [Qemu-devel] [PATCH v5 5/8] vmdk: Set maximum bytes allocated in one cycle, Ashijeet Acharya, 2017/06/05
- [Qemu-devel] [PATCH v5 6/8] vmdk: New functions to assist allocating multiple clusters, Ashijeet Acharya, 2017/06/05
- [Qemu-devel] [PATCH v5 7/8] vmdk: Update metadata for multiple clusters, Ashijeet Acharya, 2017/06/05
- [Qemu-devel] [PATCH v5 8/8] vmdk: Make vmdk_get_cluster_offset() return cluster offset only, Ashijeet Acharya, 2017/06/05