Re: [Qemu-devel] [PATCH v4 2/2] vmdk: Optimize cluster allocation

[Stefan Hajnoczi]

On Thu, May 15, 2014 at 03:02:48PM +0800, Fam Zheng wrote:
> @@ -397,6 +397,7 @@ static int vmdk_add_extent(BlockDriverState *bs,
>  {
>      VmdkExtent *extent;
>      BDRVVmdkState *s = bs->opaque;
> +    int64_t ret;
>  
>      if (cluster_sectors > 0x200000) {
>          /* 0x200000 * 512Bytes = 1GB for one cluster is unrealistic */
> @@ -428,6 +429,12 @@ static int vmdk_add_extent(BlockDriverState *bs,
>      extent->l2_size = l2_size;
>      extent->cluster_sectors = flat ? sectors : cluster_sectors;
>  
> +    ret = bdrv_getlength(extent->file);
> +    if (ret < 0) {
> +        return ret;

We need to clean up here.  extent isn't initialized fully and
s->num_extents needs to be decremented.

> +    }
> +    extent->next_cluster_sector = DIV_ROUND_UP(ret, BDRV_SECTOR_SIZE);

Should this be rounded up to clusters instead of just 512-byte sectors?
Otherwise the next allocated cluster might not be aligned.

> +
>      if (s->num_extents > 1) {
>          extent->end_sector = (*(extent - 1)).end_sector + extent->sectors;
>      } else {
> @@ -954,57 +961,96 @@ static int vmdk_refresh_limits(BlockDriverState *bs)
>      return 0;
>  }
>  
> +/**
> + * get_whole_cluster
> + *
> + * Copy backing file's cluster that covers @sector_num, otherwise write zero,
> + * to the cluster at @cluster_sector_num.
> + *
> + * If @skip_start < @skip_end, the relative range address@hidden, @skip_end) 
> is
> + * not copied or written, and leave it for call to write user data in the
> + * request.

skip_start/skip_end are in sectors, please include the unit in the
argument name.