Re: [Qemu-block] [PATCH v6 8/8] vmdk: Make vmdk_get_cluster_offset() return cluster offset only

[Fam Zheng]

On Mon, 06/05 13:22, Ashijeet Acharya wrote:
> vmdk_alloc_clusters() introduced earlier now handles the task of
> allocating clusters and performing COW when needed. Thus we can change
> vmdk_get_cluster_offset() to stick to the sole purpose of returning
> cluster offset using sector number. Update the changes at all call
> sites.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
>  block/vmdk.c | 56 
>  1 file changed, 12 insertions(+), 44 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 9fa2414..accf1c3 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1485,25 +1485,16 @@ static int vmdk_alloc_clusters(BlockDriverState *bs,
>   * For flat extents, the start offset as parsed from the description file is
>   * returned.
>   *
> - * For sparse extents, look up in L1, L2 table. If allocate is true, return 
> an
> - * offset for a new cluster and update L2 cache. If there is a backing file,
> - * COW is done before returning; otherwise, zeroes are written to the 
> allocated
> - * cluster. Both COW and zero writing skips the sector range
> - * [@skip_start_sector, @skip_end_sector) passed in by caller, because caller
> - * has new data to write there.
> + * For sparse extents, look up the L1, L2 table.
>   *
>   * Returns: VMDK_OK if cluster exists and mapped in the image.
> - *  VMDK_UNALLOC if cluster is not mapped and @allocate is false.
> - *  VMDK_ERROR if failed.
> + *  VMDK_UNALLOC if cluster is not mapped.
> + *  VMDK_ERROR if failed
>   */
>  static int vmdk_get_cluster_offset(BlockDriverState *bs,
> VmdkExtent *extent,
> -   VmdkMetaData *m_data,
> uint64_t offset,
> -   bool allocate,
> -   uint64_t *cluster_offset,
> -   uint64_t skip_start_bytes,
> -   uint64_t skip_end_bytes)
> +   uint64_t *cluster_offset)
>  {
>  int l1_index, l2_offset, l2_index;
>  uint32_t *l2_table;
> @@ -1528,31 +1519,9 @@ static int vmdk_get_cluster_offset(BlockDriverState 
> *bs,
>  }
>  
>  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;
> -
> -/* First of all we write grain itself, to avoid race condition
> - * that may to corrupt the image.
> - * This problem may occur because of insufficient space on host disk
> - * or inappropriate VM shutdown.
> - */
> -ret = vmdk_perform_cow(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,
> -offset, skip_start_bytes, skip_end_bytes);
> -if (ret) {
> -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 = _table[l2_index];
> -}
> +return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
>  }
> +
>  *cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
>  return VMDK_OK;
>  }
> @@ -1595,9 +1564,7 @@ static int64_t coroutine_fn 
> vmdk_co_get_block_status(BlockDriverState *bs,
>  return 0;
>  }
>  qemu_co_mutex_lock(>lock);
> -ret = vmdk_get_cluster_offset(bs, extent, NULL,
> -  sector_num * 512, false, ,
> -  0, 0);
> +ret = vmdk_get_cluster_offset(bs, extent, sector_num * 512, );
>  qemu_co_mutex_unlock(>lock);
>  
>  index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
> @@ -1788,13 +1755,14 @@ vmdk_co_preadv(BlockDriverState *bs, uint64_t offset, 
> uint64_t bytes,
>  ret = -EIO;
>  goto fail;
>  }
> -ret = vmdk_get_cluster_offset(bs, extent, NULL,
> -  offset, false, _offset, 0, 0);
> +
>  offset_in_cluster = vmdk_find_offset_in_cluster(extent, offset);
>  
>  n_bytes = MIN(bytes, extent->cluster_sectors * BDRV_SECTOR_SIZE
>   - offset_in_cluster);
>  
> +ret = vmdk_get_cluster_offset(bs, extent, offset, _offset);
> +
>  if (ret != VMDK_OK) {
>  /* if not allocated, try to read from parent image, if exist */
>  if (bs->backing && ret != VMDK_ZEROED) {
> @@ -2541,9 +2509,9 @@ static int vmdk_check(BlockDriverState *bs, 
> BdrvCheckResult *result,
>  sector_num);
>  break;
>  }
> -ret = vmdk_get_cluster_offset(bs, extent, NULL,
> +ret 

[Qemu-block] [PATCH v6 8/8] vmdk: Make vmdk_get_cluster_offset() return cluster offset only

[Ashijeet Acharya]

vmdk_alloc_clusters() introduced earlier now handles the task of
allocating clusters and performing COW when needed. Thus we can change
vmdk_get_cluster_offset() to stick to the sole purpose of returning
cluster offset using sector number. Update the changes at all call
sites.

Signed-off-by: Ashijeet Acharya 
---
 block/vmdk.c | 56 
 1 file changed, 12 insertions(+), 44 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 9fa2414..accf1c3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1485,25 +1485,16 @@ static int vmdk_alloc_clusters(BlockDriverState *bs,
  * For flat extents, the start offset as parsed from the description file is
  * returned.
  *
- * For sparse extents, look up in L1, L2 table. If allocate is true, return an
- * offset for a new cluster and update L2 cache. If there is a backing file,
- * COW is done before returning; otherwise, zeroes are written to the allocated
- * cluster. Both COW and zero writing skips the sector range
- * [@skip_start_sector, @skip_end_sector) passed in by caller, because caller
- * has new data to write there.
+ * For sparse extents, look up the L1, L2 table.
  *
  * Returns: VMDK_OK if cluster exists and mapped in the image.
- *  VMDK_UNALLOC if cluster is not mapped and @allocate is false.
- *  VMDK_ERROR if failed.
+ *  VMDK_UNALLOC if cluster is not mapped.
+ *  VMDK_ERROR if failed
  */
 static int vmdk_get_cluster_offset(BlockDriverState *bs,
VmdkExtent *extent,
-   VmdkMetaData *m_data,
uint64_t offset,
-   bool allocate,
-   uint64_t *cluster_offset,
-   uint64_t skip_start_bytes,
-   uint64_t skip_end_bytes)
+   uint64_t *cluster_offset)
 {
 int l1_index, l2_offset, l2_index;
 uint32_t *l2_table;
@@ -1528,31 +1519,9 @@ static int vmdk_get_cluster_offset(BlockDriverState *bs,
 }
 
 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;
-
-/* First of all we write grain itself, to avoid race condition
- * that may to corrupt the image.
- * This problem may occur because of insufficient space on host disk
- * or inappropriate VM shutdown.
- */
-ret = vmdk_perform_cow(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,
-offset, skip_start_bytes, skip_end_bytes);
-if (ret) {
-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 = _table[l2_index];
-}
+return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
 }
+
 *cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
 return VMDK_OK;
 }
@@ -1595,9 +1564,7 @@ static int64_t coroutine_fn 
vmdk_co_get_block_status(BlockDriverState *bs,
 return 0;
 }
 qemu_co_mutex_lock(>lock);
-ret = vmdk_get_cluster_offset(bs, extent, NULL,
-  sector_num * 512, false, ,
-  0, 0);
+ret = vmdk_get_cluster_offset(bs, extent, sector_num * 512, );
 qemu_co_mutex_unlock(>lock);
 
 index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
@@ -1788,13 +1755,14 @@ vmdk_co_preadv(BlockDriverState *bs, uint64_t offset, 
uint64_t bytes,
 ret = -EIO;
 goto fail;
 }
-ret = vmdk_get_cluster_offset(bs, extent, NULL,
-  offset, false, _offset, 0, 0);
+
 offset_in_cluster = vmdk_find_offset_in_cluster(extent, offset);
 
 n_bytes = MIN(bytes, extent->cluster_sectors * BDRV_SECTOR_SIZE
  - offset_in_cluster);
 
+ret = vmdk_get_cluster_offset(bs, extent, offset, _offset);
+
 if (ret != VMDK_OK) {
 /* if not allocated, try to read from parent image, if exist */
 if (bs->backing && ret != VMDK_ZEROED) {
@@ -2541,9 +2509,9 @@ static int vmdk_check(BlockDriverState *bs, 
BdrvCheckResult *result,
 sector_num);
 break;
 }
-ret = vmdk_get_cluster_offset(bs, extent, NULL,
+ret = vmdk_get_cluster_offset(bs, extent,
   sector_num << BDRV_SECTOR_BITS,
-  false, _offset, 0, 0);
+  _offset);
 if (ret == VMDK_ERROR) {
 fprintf(stderr,