* [Qemu-devel] [PATCH v2 1/7] vmdk: Refactor and introduce new helper functions
2017-03-25 11:18 [Qemu-devel] [PATCH v2 0/7] Optiomize VMDK I/O by allocating multiple clusters Ashijeet Acharya
@ 2017-03-25 11:18 ` Ashijeet Acharya
2017-03-31 5:52 ` Fam Zheng
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 2/7] vmdk: Rename get_whole_cluster() to vmdk_perform_cow() Ashijeet Acharya
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ashijeet Acharya @ 2017-03-25 11:18 UTC (permalink / raw)
To: famz
Cc: kwolf, jsnow, mreitz, stefanha, qemu-devel, qemu-block,
Ashijeet Acharya
Move the existing vmdk_find_offset_in_cluster() function to the top of
the driver. Also, introduce a new helper function size_to_clusters()
which returns the number of clusters for a given size in bytes. Here,
we leave the last cluster as we need to perform COW for that one.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
block/vmdk.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index a9bd22b..7795c5f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -242,6 +242,26 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
s->extents = g_renew(VmdkExtent, s->extents, s->num_extents);
}
+static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
+ int64_t offset)
+{
+ uint64_t extent_begin_offset, extent_relative_offset;
+ uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
+
+ extent_begin_offset =
+ (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
+ extent_relative_offset = offset - extent_begin_offset;
+ return extent_relative_offset % cluster_size;
+}
+
+static inline uint64_t size_to_clusters(VmdkExtent *extent, uint64_t size)
+{
+ uint64_t cluster_size, round_off_size;
+ cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
+ round_off_size = cluster_size - (size % cluster_size);
+ return ((size + round_off_size) >> 16) - 1;
+}
+
static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
{
char *desc;
@@ -1266,18 +1286,6 @@ static VmdkExtent *find_extent(BDRVVmdkState *s,
return NULL;
}
-static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
- int64_t offset)
-{
- uint64_t extent_begin_offset, extent_relative_offset;
- uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
-
- extent_begin_offset =
- (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
- extent_relative_offset = offset - extent_begin_offset;
- return extent_relative_offset % cluster_size;
-}
-
static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
int64_t sector_num)
{
--
2.6.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/7] vmdk: Refactor and introduce new helper functions
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 1/7] vmdk: Refactor and introduce new helper functions Ashijeet Acharya
@ 2017-03-31 5:52 ` Fam Zheng
0 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2017-03-31 5:52 UTC (permalink / raw)
To: Ashijeet Acharya; +Cc: kwolf, qemu-block, stefanha, qemu-devel, mreitz, jsnow
On Sat, 03/25 16:48, Ashijeet Acharya wrote:
> Move the existing vmdk_find_offset_in_cluster() function to the top of
> the driver. Also, introduce a new helper function size_to_clusters()
> which returns the number of clusters for a given size in bytes. Here,
> we leave the last cluster as we need to perform COW for that one.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
> block/vmdk.c | 32 ++++++++++++++++++++------------
> 1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index a9bd22b..7795c5f 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -242,6 +242,26 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
> s->extents = g_renew(VmdkExtent, s->extents, s->num_extents);
> }
>
> +static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
> + int64_t offset)
> +{
> + uint64_t extent_begin_offset, extent_relative_offset;
> + uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
> +
> + extent_begin_offset =
> + (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
> + extent_relative_offset = offset - extent_begin_offset;
> + return extent_relative_offset % cluster_size;
> +}
Code movement and new code are usually in separate patches.
> +
> +static inline uint64_t size_to_clusters(VmdkExtent *extent, uint64_t size)
> +{
> + uint64_t cluster_size, round_off_size;
> + cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
> + round_off_size = cluster_size - (size % cluster_size);
> + return ((size + round_off_size) >> 16) - 1;
Why hardcode 16? You can use DIV_ROUND_UP().
> +}
> +
Usually a static function is added in the patch that actually uses it, which
makes reviewing easier.
Fam
> static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
> {
> char *desc;
> @@ -1266,18 +1286,6 @@ static VmdkExtent *find_extent(BDRVVmdkState *s,
> return NULL;
> }
>
> -static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
> - int64_t offset)
> -{
> - uint64_t extent_begin_offset, extent_relative_offset;
> - uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
> -
> - extent_begin_offset =
> - (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
> - extent_relative_offset = offset - extent_begin_offset;
> - return extent_relative_offset % cluster_size;
> -}
> -
> static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
> int64_t sector_num)
> {
> --
> 2.6.2
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v2 2/7] vmdk: Rename get_whole_cluster() to vmdk_perform_cow()
2017-03-25 11:18 [Qemu-devel] [PATCH v2 0/7] Optiomize VMDK I/O by allocating multiple clusters Ashijeet Acharya
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 1/7] vmdk: Refactor and introduce new helper functions Ashijeet Acharya
@ 2017-03-25 11:18 ` Ashijeet Acharya
2017-03-31 5:55 ` Fam Zheng
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 3/7] vmdk: Factor out metadata loading code out of get_cluster_offset() Ashijeet Acharya
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ashijeet Acharya @ 2017-03-25 11:18 UTC (permalink / raw)
To: famz
Cc: kwolf, jsnow, mreitz, stefanha, qemu-devel, qemu-block,
Ashijeet Acharya
Rename the existing function get_whole_cluster() to vmdk_perform_cow()
as its sole purpose is to perform COW for the first and the last
allocated clusters if needed.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
block/vmdk.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 7795c5f..f5fda2c 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1036,8 +1036,8 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
}
}
-/**
- * get_whole_cluster
+/*
+ * vmdk_perform_cow
*
* Copy backing file's cluster that covers @sector_num, otherwise write zero,
* to the cluster at @cluster_sector_num.
@@ -1045,13 +1045,18 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
* If @skip_start_sector < @skip_end_sector, the relative range
* [@skip_start_sector, @skip_end_sector) is not copied or written, and leave
* it for call to write user data in the request.
+ *
+ * Returns:
+ * VMDK_OK: on success
+ *
+ * VMDK_ERROR: in error cases
*/
-static int get_whole_cluster(BlockDriverState *bs,
- VmdkExtent *extent,
- uint64_t cluster_offset,
- uint64_t offset,
- uint64_t skip_start_bytes,
- uint64_t skip_end_bytes)
+static int vmdk_perform_cow(BlockDriverState *bs,
+ VmdkExtent *extent,
+ uint64_t cluster_offset,
+ uint64_t offset,
+ uint64_t skip_start_bytes,
+ uint64_t skip_end_bytes)
{
int ret = VMDK_OK;
int64_t cluster_bytes;
--
2.6.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/7] vmdk: Rename get_whole_cluster() to vmdk_perform_cow()
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 2/7] vmdk: Rename get_whole_cluster() to vmdk_perform_cow() Ashijeet Acharya
@ 2017-03-31 5:55 ` Fam Zheng
0 siblings, 0 replies; 16+ messages in thread
From: Fam Zheng @ 2017-03-31 5:55 UTC (permalink / raw)
To: Ashijeet Acharya; +Cc: kwolf, qemu-block, stefanha, qemu-devel, mreitz, jsnow
On Sat, 03/25 16:48, Ashijeet Acharya wrote:
> Rename the existing function get_whole_cluster() to vmdk_perform_cow()
> as its sole purpose is to perform COW for the first and the last
> allocated clusters if needed.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
> block/vmdk.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 7795c5f..f5fda2c 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1036,8 +1036,8 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
> }
> }
>
> -/**
> - * get_whole_cluster
> +/*
> + * vmdk_perform_cow
> *
> * Copy backing file's cluster that covers @sector_num, otherwise write zero,
> * to the cluster at @cluster_sector_num.
> @@ -1045,13 +1045,18 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
> * If @skip_start_sector < @skip_end_sector, the relative range
> * [@skip_start_sector, @skip_end_sector) is not copied or written, and leave
> * it for call to write user data in the request.
> + *
> + * Returns:
> + * VMDK_OK: on success
> + *
> + * VMDK_ERROR: in error cases
> */
> -static int get_whole_cluster(BlockDriverState *bs,
> - VmdkExtent *extent,
> - uint64_t cluster_offset,
> - uint64_t offset,
> - uint64_t skip_start_bytes,
> - uint64_t skip_end_bytes)
> +static int vmdk_perform_cow(BlockDriverState *bs,
> + VmdkExtent *extent,
> + uint64_t cluster_offset,
> + uint64_t offset,
> + uint64_t skip_start_bytes,
> + uint64_t skip_end_bytes)
> {
> int ret = VMDK_OK;
> int64_t cluster_bytes;
> --
> 2.6.2
>
>
Rule: each and every patch must be compilable. A rename patch must take care of
all the users. Please find all get_whole_cluster occasions in the file and
change them to use the new name.
Fam
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v2 3/7] vmdk: Factor out metadata loading code out of get_cluster_offset()
2017-03-25 11:18 [Qemu-devel] [PATCH v2 0/7] Optiomize VMDK I/O by allocating multiple clusters Ashijeet Acharya
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 1/7] vmdk: Refactor and introduce new helper functions Ashijeet Acharya
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 2/7] vmdk: Rename get_whole_cluster() to vmdk_perform_cow() Ashijeet Acharya
@ 2017-03-25 11:18 ` Ashijeet Acharya
2017-03-31 6:03 ` Fam Zheng
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 4/7] vmdk: New functions to allocate multiple clusters and cluster offset Ashijeet Acharya
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ashijeet Acharya @ 2017-03-25 11:18 UTC (permalink / raw)
To: famz
Cc: kwolf, jsnow, mreitz, stefanha, qemu-devel, qemu-block,
Ashijeet Acharya
Move the cluster tables loading code out of the existing
get_cluster_offset() function and implement it in separate
get_cluster_table() and vmdk_L2load() functions. This patch will help
us avoid code duplication in future patches of this series.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
block/vmdk.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 99 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index f5fda2c..a42322e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1037,6 +1037,105 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
}
/*
+ * vmdk_L2load
+ *
+ * Loads a new L2 table into memory. If the table is in the cache, the cache
+ * is used; otherwise the L2 table is loaded from the image file.
+ *
+ * Returns:
+ * VMDK_OK: on success
+ * VMDK_ERROR: in error cases
+ */
+static int vmdk_L2load(VmdkExtent *extent, uint64_t offset, int l2_offset,
+ uint32_t **new_l2_table, int *new_l2_index)
+{
+ int min_index, i, j;
+ uint32_t *l2_table;
+ uint32_t min_count;
+
+ for (i = 0; i < L2_CACHE_SIZE; i++) {
+ if (l2_offset == extent->l2_cache_offsets[i]) {
+ /* increment the hit count */
+ if (++extent->l2_cache_counts[i] == 0xffffffff) {
+ for (j = 0; j < L2_CACHE_SIZE; j++) {
+ extent->l2_cache_counts[j] >>= 1;
+ }
+ }
+ l2_table = extent->l2_cache + (i * extent->l2_size);
+ goto found;
+ }
+ }
+ /* not found: load a new entry in the least used one */
+ min_index = 0;
+ min_count = 0xffffffff;
+ for (i = 0; i < L2_CACHE_SIZE; i++) {
+ if (extent->l2_cache_counts[i] < min_count) {
+ min_count = extent->l2_cache_counts[i];
+ min_index = i;
+ }
+ }
+ l2_table = extent->l2_cache + (min_index * extent->l2_size);
+ if (bdrv_pread(extent->file,
+ (int64_t)l2_offset * 512,
+ l2_table,
+ extent->l2_size * sizeof(uint32_t)
+ ) != extent->l2_size * sizeof(uint32_t)) {
+ return VMDK_ERROR;
+ }
+
+ extent->l2_cache_offsets[min_index] = l2_offset;
+ extent->l2_cache_counts[min_index] = 1;
+found:
+ *new_l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size;
+ *new_l2_table = l2_table;
+
+ return VMDK_OK;
+}
+
+/*
+ * get_cluster_table
+ *
+ * for a given offset, load (and allocate if needed) the l2 table.
+ *
+ * Returns:
+ * VMDK_OK: on success
+ *
+ * VMDK_UNALLOC: if cluster is not mapped
+ *
+ * VMDK_ERROR: in error cases
+ */
+static int get_cluster_table(VmdkExtent *extent, uint64_t offset,
+ int *new_l1_index, int *new_l2_offset,
+ int *new_l2_index, uint32_t **new_l2_table)
+{
+ int l1_index, l2_offset, l2_index;
+ uint32_t *l2_table;
+ int ret;
+
+ offset -= (extent->end_sector - extent->sectors) * SECTOR_SIZE;
+ l1_index = (offset >> 9) / extent->l1_entry_sectors;
+ if (l1_index >= extent->l1_size) {
+ return VMDK_ERROR;
+ }
+ l2_offset = extent->l1_table[l1_index];
+ if (!l2_offset) {
+ return VMDK_UNALLOC;
+ }
+
+ ret = vmdk_L2load(extent, offset, l2_offset, &l2_table, &l2_index);
+ if (ret < 0) {
+ return ret;
+ }
+
+ *new_l1_index = l1_index;
+ *new_l2_offset = l2_offset;
+ *new_l2_index = l2_index;
+ *new_l2_table = l2_table;
+
+ return VMDK_OK;
+}
+
+/*
* vmdk_perform_cow
*
* Copy backing file's cluster that covers @sector_num, otherwise write zero,
--
2.6.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/7] vmdk: Factor out metadata loading code out of get_cluster_offset()
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 3/7] vmdk: Factor out metadata loading code out of get_cluster_offset() Ashijeet Acharya
@ 2017-03-31 6:03 ` Fam Zheng
2017-04-01 13:22 ` Ashijeet Acharya
0 siblings, 1 reply; 16+ messages in thread
From: Fam Zheng @ 2017-03-31 6:03 UTC (permalink / raw)
To: Ashijeet Acharya; +Cc: kwolf, qemu-block, stefanha, qemu-devel, mreitz, jsnow
On Sat, 03/25 16:48, Ashijeet Acharya wrote:
> Move the cluster tables loading code out of the existing
> get_cluster_offset() function and implement it in separate
Now it's renamed to vmdk_perform_cow() in previous patch, the commit message
following it should be updated.
> get_cluster_table() and vmdk_L2load() functions. This patch will help
> us avoid code duplication in future patches of this series.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
> block/vmdk.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 99 insertions(+)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index f5fda2c..a42322e 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1037,6 +1037,105 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
> }
>
> /*
> + * vmdk_L2load
Personally, I think vmdk_l2load is good enough, the upper case doesn't add
readability but makes it slightly harder to type.
> + *
> + * Loads a new L2 table into memory. If the table is in the cache, the cache
> + * is used; otherwise the L2 table is loaded from the image file.
> + *
> + * Returns:
> + * VMDK_OK: on success
> + * VMDK_ERROR: in error cases
> + */
> +static int vmdk_L2load(VmdkExtent *extent, uint64_t offset, int l2_offset,
> + uint32_t **new_l2_table, int *new_l2_index)
> +{
> + int min_index, i, j;
> + uint32_t *l2_table;
> + uint32_t min_count;
> +
> + for (i = 0; i < L2_CACHE_SIZE; i++) {
> + if (l2_offset == extent->l2_cache_offsets[i]) {
> + /* increment the hit count */
> + if (++extent->l2_cache_counts[i] == 0xffffffff) {
Please use UINT32_MAX.
> + for (j = 0; j < L2_CACHE_SIZE; j++) {
> + extent->l2_cache_counts[j] >>= 1;
> + }
> + }
> + l2_table = extent->l2_cache + (i * extent->l2_size);
> + goto found;
> + }
> + }
> + /* not found: load a new entry in the least used one */
> + min_index = 0;
> + min_count = 0xffffffff;
Please use UINT32_MAX.
> + for (i = 0; i < L2_CACHE_SIZE; i++) {
> + if (extent->l2_cache_counts[i] < min_count) {
> + min_count = extent->l2_cache_counts[i];
> + min_index = i;
> + }
> + }
> + l2_table = extent->l2_cache + (min_index * extent->l2_size);
> + if (bdrv_pread(extent->file,
> + (int64_t)l2_offset * 512,
> + l2_table,
> + extent->l2_size * sizeof(uint32_t)
> + ) != extent->l2_size * sizeof(uint32_t)) {
> + return VMDK_ERROR;
> + }
> +
> + extent->l2_cache_offsets[min_index] = l2_offset;
> + extent->l2_cache_counts[min_index] = 1;
> +found:
> + *new_l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size;
> + *new_l2_table = l2_table;
> +
> + return VMDK_OK;
> +}
> +
> +/*
> + * get_cluster_table
> + *
> + * for a given offset, load (and allocate if needed) the l2 table.
> + *
> + * Returns:
> + * VMDK_OK: on success
> + *
> + * VMDK_UNALLOC: if cluster is not mapped
> + *
> + * VMDK_ERROR: in error cases
> + */
> +static int get_cluster_table(VmdkExtent *extent, uint64_t offset,
> + int *new_l1_index, int *new_l2_offset,
> + int *new_l2_index, uint32_t **new_l2_table)
Again, a static function must be introduced with the code change where it is
used, at least for once. It keeps the compiler happy (-Wunused-function) and
makes reviewing easy.
> +{
> + int l1_index, l2_offset, l2_index;
> + uint32_t *l2_table;
> + int ret;
> +
> + offset -= (extent->end_sector - extent->sectors) * SECTOR_SIZE;
> + l1_index = (offset >> 9) / extent->l1_entry_sectors;
> + if (l1_index >= extent->l1_size) {
> + return VMDK_ERROR;
> + }
> + l2_offset = extent->l1_table[l1_index];
> + if (!l2_offset) {
> + return VMDK_UNALLOC;
> + }
> +
> + ret = vmdk_L2load(extent, offset, l2_offset, &l2_table, &l2_index);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + *new_l1_index = l1_index;
> + *new_l2_offset = l2_offset;
> + *new_l2_index = l2_index;
> + *new_l2_table = l2_table;
> +
> + return VMDK_OK;
> +}
> +
> +/*
> * vmdk_perform_cow
> *
> * Copy backing file's cluster that covers @sector_num, otherwise write zero,
> --
> 2.6.2
>
>
Fam
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/7] vmdk: Factor out metadata loading code out of get_cluster_offset()
2017-03-31 6:03 ` Fam Zheng
@ 2017-04-01 13:22 ` Ashijeet Acharya
0 siblings, 0 replies; 16+ messages in thread
From: Ashijeet Acharya @ 2017-04-01 13:22 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, qemu block, Stefan Hajnoczi, QEMU Developers,
Max Reitz, John Snow
On Fri, Mar 31, 2017 at 11:33 AM, Fam Zheng <famz@redhat.com> wrote:
> On Sat, 03/25 16:48, Ashijeet Acharya wrote:
>> Move the cluster tables loading code out of the existing
>> get_cluster_offset() function and implement it in separate
>
> Now it's renamed to vmdk_perform_cow() in previous patch, the commit message
> following it should be updated.
No, I think you confused get_cluster_offset() with get_whole_cluster() here :-)
I have a separate patch 5/7 which renames get_cluster_offset(), so the
commit message is fine till this stage.
>
>> get_cluster_table() and vmdk_L2load() functions. This patch will help
>> us avoid code duplication in future patches of this series.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>> block/vmdk.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 99 insertions(+)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index f5fda2c..a42322e 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1037,6 +1037,105 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
>> }
>>
>> /*
>> + * vmdk_L2load
>
> Personally, I think vmdk_l2load is good enough, the upper case doesn't add
> readability but makes it slightly harder to type.
Done.
>
>> + *
>> + * Loads a new L2 table into memory. If the table is in the cache, the cache
>> + * is used; otherwise the L2 table is loaded from the image file.
>> + *
>> + * Returns:
>> + * VMDK_OK: on success
>> + * VMDK_ERROR: in error cases
>> + */
>> +static int vmdk_L2load(VmdkExtent *extent, uint64_t offset, int l2_offset,
>> + uint32_t **new_l2_table, int *new_l2_index)
>> +{
>> + int min_index, i, j;
>> + uint32_t *l2_table;
>> + uint32_t min_count;
>> +
>> + for (i = 0; i < L2_CACHE_SIZE; i++) {
>> + if (l2_offset == extent->l2_cache_offsets[i]) {
>> + /* increment the hit count */
>> + if (++extent->l2_cache_counts[i] == 0xffffffff) {
>
> Please use UINT32_MAX.
Done.
>
>> + for (j = 0; j < L2_CACHE_SIZE; j++) {
>> + extent->l2_cache_counts[j] >>= 1;
>> + }
>> + }
>> + l2_table = extent->l2_cache + (i * extent->l2_size);
>> + goto found;
>> + }
>> + }
>> + /* not found: load a new entry in the least used one */
>> + min_index = 0;
>> + min_count = 0xffffffff;
>
> Please use UINT32_MAX.
Done.
>
>> + for (i = 0; i < L2_CACHE_SIZE; i++) {
>> + if (extent->l2_cache_counts[i] < min_count) {
>> + min_count = extent->l2_cache_counts[i];
>> + min_index = i;
>> + }
>> + }
>> + l2_table = extent->l2_cache + (min_index * extent->l2_size);
>> + if (bdrv_pread(extent->file,
>> + (int64_t)l2_offset * 512,
>> + l2_table,
>> + extent->l2_size * sizeof(uint32_t)
>> + ) != extent->l2_size * sizeof(uint32_t)) {
>> + return VMDK_ERROR;
>> + }
>> +
>> + extent->l2_cache_offsets[min_index] = l2_offset;
>> + extent->l2_cache_counts[min_index] = 1;
>> +found:
>> + *new_l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size;
>> + *new_l2_table = l2_table;
>> +
>> + return VMDK_OK;
>> +}
>> +
>> +/*
>> + * get_cluster_table
>> + *
>> + * for a given offset, load (and allocate if needed) the l2 table.
>> + *
>> + * Returns:
>> + * VMDK_OK: on success
>> + *
>> + * VMDK_UNALLOC: if cluster is not mapped
>> + *
>> + * VMDK_ERROR: in error cases
>> + */
>> +static int get_cluster_table(VmdkExtent *extent, uint64_t offset,
>> + int *new_l1_index, int *new_l2_offset,
>> + int *new_l2_index, uint32_t **new_l2_table)
>
> Again, a static function must be introduced with the code change where it is
> used, at least for once. It keeps the compiler happy (-Wunused-function) and
> makes reviewing easy.
Done.
Ashijeet
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v2 4/7] vmdk: New functions to allocate multiple clusters and cluster offset
2017-03-25 11:18 [Qemu-devel] [PATCH v2 0/7] Optiomize VMDK I/O by allocating multiple clusters Ashijeet Acharya
` (2 preceding siblings ...)
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 3/7] vmdk: Factor out metadata loading code out of get_cluster_offset() Ashijeet Acharya
@ 2017-03-25 11:18 ` Ashijeet Acharya
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 5/7] vmdk: Rename get_cluster_offset() to vmdk_get_cluster_offset() Ashijeet Acharya
` (2 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Ashijeet Acharya @ 2017-03-25 11:18 UTC (permalink / raw)
To: famz
Cc: kwolf, jsnow, mreitz, stefanha, qemu-devel, qemu-block,
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 <ashijeetacharya@gmail.com>
---
block/vmdk.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 187 insertions(+), 19 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index a42322e..5a95929 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 {
@@ -1036,6 +1037,34 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
}
}
+static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
+ uint32_t offset)
+{
+ offset = cpu_to_le32(offset);
+ /* update L2 table */
+ if (bdrv_pwrite_sync(extent->file,
+ ((int64_t)m_data->l2_offset * 512)
+ + (m_data->l2_index * sizeof(offset)),
+ &offset, sizeof(offset)) < 0) {
+ return VMDK_ERROR;
+ }
+ /* update backup L2 table */
+ if (extent->l1_backup_table_offset != 0) {
+ m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
+ if (bdrv_pwrite_sync(extent->file,
+ ((int64_t)m_data->l2_offset * 512)
+ + (m_data->l2_index * sizeof(offset)),
+ &offset, sizeof(offset)) < 0) {
+ return VMDK_ERROR;
+ }
+ }
+ if (m_data->l2_cache_entry) {
+ *m_data->l2_cache_entry = offset;
+ }
+
+ return VMDK_OK;
+}
+
/*
* vmdk_L2load
*
@@ -1222,29 +1251,168 @@ exit:
return ret;
}
-static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
- uint32_t offset)
+/*
+ * handle_alloc
+ *
+ * Allocates 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 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)
{
- offset = cpu_to_le32(offset);
- /* update L2 table */
- if (bdrv_pwrite_sync(extent->file,
- ((int64_t)m_data->l2_offset * 512)
- + (m_data->l2_index * sizeof(offset)),
- &offset, sizeof(offset)) < 0) {
- return VMDK_ERROR;
+ 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;
}
- /* update backup L2 table */
- if (extent->l1_backup_table_offset != 0) {
- m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
- if (bdrv_pwrite_sync(extent->file,
- ((int64_t)m_data->l2_offset * 512)
- + (m_data->l2_index * sizeof(offset)),
- &offset, sizeof(offset)) < 0) {
- return VMDK_ERROR;
+
+ 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 it will return one
+ * cluster less than the actual value calculated as we may need to perfrom
+ * COW for the last one. */
+ nb_clusters = size_to_clusters(extent, skip_start_bytes + *bytes);
+
+ 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) << 9)
+ - skip_start_bytes;
+ } else {
+ nb_clusters = 1;
+ }
+ *total_alloc_clusters += 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;
}
}
- if (m_data->l2_cache_entry) {
- *m_data->l2_cache_entry = offset;
+ *cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
+ return VMDK_OK;
+}
+
+/*
+ * vmdk_alloc_cluster_offset
+ *
+ * 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_cluster_offset(BlockDriverState *bs,
+ VmdkExtent *extent,
+ VmdkMetaData *m_data, uint64_t offset,
+ bool allocate, uint64_t *cluster_offset,
+ int64_t bytes,
+ uint32_t *total_alloc_clusters)
+{
+ uint64_t start, remaining;
+ uint64_t new_cluster_offset;
+ int64_t n_bytes;
+ int ret;
+
+ if (extent->flat) {
+ *cluster_offset = extent->flat_start_offset;
+ return VMDK_OK;
+ }
+
+ start = offset;
+ remaining = bytes;
+ new_cluster_offset = 0;
+ *cluster_offset = 0;
+ n_bytes = 0;
+ if (m_data) {
+ m_data->valid = 0;
+ }
+
+ /* due to L2 table margins all bytes may not get allocated at once */
+ while (true) {
+
+ if (!*cluster_offset) {
+ *cluster_offset = new_cluster_offset;
+ }
+
+ start += n_bytes;
+ remaining -= n_bytes;
+ new_cluster_offset += n_bytes;
+
+ if (remaining == 0) {
+ break;
+ }
+
+ n_bytes = remaining;
+
+ ret = handle_alloc(bs, extent, start, &new_cluster_offset, &n_bytes,
+ m_data, allocate, total_alloc_clusters);
+
+ if (ret < 0) {
+ return ret;
+
+ }
}
return VMDK_OK;
--
2.6.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 5/7] vmdk: Rename get_cluster_offset() to vmdk_get_cluster_offset()
2017-03-25 11:18 [Qemu-devel] [PATCH v2 0/7] Optiomize VMDK I/O by allocating multiple clusters Ashijeet Acharya
` (3 preceding siblings ...)
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 4/7] vmdk: New functions to allocate multiple clusters and cluster offset Ashijeet Acharya
@ 2017-03-25 11:18 ` Ashijeet Acharya
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 6/7] vmdk: Allocate multiple clusters at once Ashijeet Acharya
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 7/7] vmdk: Update metadata for multiple clusters Ashijeet Acharya
6 siblings, 0 replies; 16+ messages in thread
From: Ashijeet Acharya @ 2017-03-25 11:18 UTC (permalink / raw)
To: famz
Cc: kwolf, jsnow, mreitz, stefanha, qemu-devel, qemu-block,
Ashijeet Acharya
Rename the existing get_cluster_offset() function to
vmdk_get_cluster_offset() and have it make use of the new
get_cluster_table() to load the cluster tables. Also, it is no longer
used to allocate new clusters and hence perform COW. Make the necessary
renames at all the occurrences of get_cluster_offset().
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
block/vmdk.c | 117 +++++++++++------------------------------------------------
1 file changed, 21 insertions(+), 96 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 5a95929..387e45b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1419,7 +1419,7 @@ static int vmdk_alloc_cluster_offset(BlockDriverState *bs,
}
/**
- * get_cluster_offset
+ * vmdk_get_cluster_offset
*
* Look up cluster offset in extent file by sector number, and store in
* @cluster_offset.
@@ -1427,84 +1427,34 @@ static int vmdk_alloc_cluster_offset(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 in 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 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)
+static int vmdk_get_cluster_offset(BlockDriverState *bs,
+ VmdkExtent *extent,
+ uint64_t offset,
+ uint64_t *cluster_offset)
{
- unsigned int l1_index, l2_offset, l2_index;
- int min_index, i, j;
- uint32_t min_count, *l2_table;
+ int l1_index, l2_offset, l2_index;
+ uint32_t *l2_table;
bool zeroed = false;
int64_t ret;
int64_t cluster_sector;
- if (m_data) {
- m_data->valid = 0;
- }
if (extent->flat) {
*cluster_offset = extent->flat_start_offset;
return VMDK_OK;
}
- offset -= (extent->end_sector - extent->sectors) * SECTOR_SIZE;
- l1_index = (offset >> 9) / extent->l1_entry_sectors;
- if (l1_index >= extent->l1_size) {
- return VMDK_ERROR;
- }
- l2_offset = extent->l1_table[l1_index];
- if (!l2_offset) {
- return VMDK_UNALLOC;
- }
- for (i = 0; i < L2_CACHE_SIZE; i++) {
- if (l2_offset == extent->l2_cache_offsets[i]) {
- /* increment the hit count */
- if (++extent->l2_cache_counts[i] == 0xffffffff) {
- for (j = 0; j < L2_CACHE_SIZE; j++) {
- extent->l2_cache_counts[j] >>= 1;
- }
- }
- l2_table = extent->l2_cache + (i * extent->l2_size);
- goto found;
- }
- }
- /* not found: load a new entry in the least used one */
- min_index = 0;
- min_count = 0xffffffff;
- for (i = 0; i < L2_CACHE_SIZE; i++) {
- if (extent->l2_cache_counts[i] < min_count) {
- min_count = extent->l2_cache_counts[i];
- min_index = i;
- }
- }
- l2_table = extent->l2_cache + (min_index * extent->l2_size);
- if (bdrv_pread(extent->file,
- (int64_t)l2_offset * 512,
- l2_table,
- extent->l2_size * sizeof(uint32_t)
- ) != extent->l2_size * sizeof(uint32_t)) {
- return VMDK_ERROR;
+ ret = get_cluster_table(extent, offset, &l1_index, &l2_offset,
+ &l2_index, &l2_table);
+ if (ret < 0) {
+ return ret;
}
- extent->l2_cache_offsets[min_index] = l2_offset;
- extent->l2_cache_counts[min_index] = 1;
- found:
- l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size;
cluster_sector = le32_to_cpu(l2_table[l2_index]);
if (extent->has_zero_grain && cluster_sector == VMDK_GTE_ZEROED) {
@@ -1512,31 +1462,9 @@ static int 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 = get_whole_cluster(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 = &l2_table[l2_index];
- }
+ return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
}
+
*cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
return VMDK_OK;
}
@@ -1579,9 +1507,7 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
return 0;
}
qemu_co_mutex_lock(&s->lock);
- ret = get_cluster_offset(bs, extent, NULL,
- sector_num * 512, false, &offset,
- 0, 0);
+ ret = vmdk_get_cluster_offset(bs, extent, sector_num * 512, &offset);
qemu_co_mutex_unlock(&s->lock);
index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
@@ -1772,13 +1698,13 @@ vmdk_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
ret = -EIO;
goto fail;
}
- ret = get_cluster_offset(bs, extent, NULL,
- offset, false, &cluster_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, &cluster_offset);
+
if (ret != VMDK_OK) {
/* if not allocated, try to read from parent image, if exist */
if (bs->backing && ret != VMDK_ZEROED) {
@@ -2504,9 +2430,8 @@ static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result,
sector_num);
break;
}
- ret = get_cluster_offset(bs, extent, NULL,
- sector_num << BDRV_SECTOR_BITS,
- false, &cluster_offset, 0, 0);
+ ret = vmdk_get_cluster_offset(bs, extent,
+ sector_num << BDRV_SECTOR_BITS, &cluster_offset);
if (ret == VMDK_ERROR) {
fprintf(stderr,
"ERROR: could not get cluster_offset for sector %"
--
2.6.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 6/7] vmdk: Allocate multiple clusters at once
2017-03-25 11:18 [Qemu-devel] [PATCH v2 0/7] Optiomize VMDK I/O by allocating multiple clusters Ashijeet Acharya
` (4 preceding siblings ...)
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 5/7] vmdk: Rename get_cluster_offset() to vmdk_get_cluster_offset() Ashijeet Acharya
@ 2017-03-25 11:18 ` Ashijeet Acharya
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 7/7] vmdk: Update metadata for multiple clusters Ashijeet Acharya
6 siblings, 0 replies; 16+ messages in thread
From: Ashijeet Acharya @ 2017-03-25 11:18 UTC (permalink / raw)
To: famz
Cc: kwolf, jsnow, mreitz, stefanha, qemu-devel, qemu-block,
Ashijeet Acharya
Get vmdk_pwritev() to make use of the new multiple cluster allocation
helper functions to allocate multiple clusters at once. Set the maximum
bytes allowed to get allocated at once to be not more than the extent
size boundary to handle writes at two separate extents appropriately.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
block/vmdk.c | 37 +++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 387e45b..3de8b8f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1767,7 +1767,9 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
int64_t offset_in_cluster, n_bytes;
uint64_t cluster_offset;
uint64_t bytes_done = 0;
+ uint64_t extent_size;
VmdkMetaData m_data;
+ uint32_t total_alloc_clusters = 0;
if (DIV_ROUND_UP(offset, BDRV_SECTOR_SIZE) > bs->total_sectors) {
error_report("Wrong offset: offset=0x%" PRIx64
@@ -1781,14 +1783,22 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
if (!extent) {
return -EIO;
}
+ extent_size = extent->end_sector * BDRV_SECTOR_SIZE;
+
offset_in_cluster = vmdk_find_offset_in_cluster(extent, offset);
- n_bytes = MIN(bytes, extent->cluster_sectors * BDRV_SECTOR_SIZE
- - offset_in_cluster);
- ret = get_cluster_offset(bs, extent, &m_data, offset,
- !(extent->compressed || zeroed),
- &cluster_offset, offset_in_cluster,
- offset_in_cluster + n_bytes);
+ /* truncate n_bytes to first cluster because we need to perform COW */
+ if (offset_in_cluster > 0) {
+ n_bytes = MIN(bytes, extent->cluster_sectors * BDRV_SECTOR_SIZE
+ - offset_in_cluster);
+ } else {
+ n_bytes = MIN(bytes, extent_size - offset);
+ }
+
+ ret = vmdk_alloc_cluster_offset(bs, extent, &m_data, offset,
+ !(extent->compressed || zeroed),
+ &cluster_offset, n_bytes,
+ &total_alloc_clusters);
if (extent->compressed) {
if (ret == VMDK_OK) {
/* Refuse write to allocated cluster for streamOptimized */
@@ -1797,19 +1807,22 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
return -EIO;
} else {
/* allocate */
- ret = get_cluster_offset(bs, extent, &m_data, offset,
- true, &cluster_offset, 0, 0);
+ ret = vmdk_alloc_cluster_offset(bs, extent, &m_data, offset,
+ true, &cluster_offset, n_bytes,
+ &total_alloc_clusters);
}
}
if (ret == VMDK_ERROR) {
return -EINVAL;
}
+
if (zeroed) {
/* Do zeroed write, buf is ignored */
- if (extent->has_zero_grain &&
- offset_in_cluster == 0 &&
- n_bytes >= extent->cluster_sectors * BDRV_SECTOR_SIZE) {
- n_bytes = extent->cluster_sectors * BDRV_SECTOR_SIZE;
+ if (extent->has_zero_grain && offset_in_cluster == 0 &&
+ n_bytes >= extent->cluster_sectors * BDRV_SECTOR_SIZE *
+ total_alloc_clusters) {
+ n_bytes = extent->cluster_sectors * BDRV_SECTOR_SIZE *
+ total_alloc_clusters;
if (!zero_dry_run) {
/* update L2 tables */
if (vmdk_L2update(extent, &m_data, VMDK_GTE_ZEROED)
--
2.6.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 7/7] vmdk: Update metadata for multiple clusters
2017-03-25 11:18 [Qemu-devel] [PATCH v2 0/7] Optiomize VMDK I/O by allocating multiple clusters Ashijeet Acharya
` (5 preceding siblings ...)
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 6/7] vmdk: Allocate multiple clusters at once Ashijeet Acharya
@ 2017-03-25 11:18 ` Ashijeet Acharya
2017-03-31 7:26 ` Fam Zheng
6 siblings, 1 reply; 16+ messages in thread
From: Ashijeet Acharya @ 2017-03-25 11:18 UTC (permalink / raw)
To: famz
Cc: kwolf, jsnow, mreitz, stefanha, qemu-devel, qemu-block,
Ashijeet Acharya
Include a next pointer in VmdkMetaData struct to point to the previous
allocated L2 table. Modify vmdk_L2update to start updating metadata for
allocation of multiple clusters at once.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
block/vmdk.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 102 insertions(+), 29 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 3de8b8f..4517409 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -137,6 +137,8 @@ typedef struct VmdkMetaData {
int valid;
uint32_t *l2_cache_entry;
uint32_t nb_clusters;
+ uint32_t offset;
+ struct VmdkMetaData *next;
} VmdkMetaData;
typedef struct VmdkGrainMarker {
@@ -1037,29 +1039,81 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
}
}
-static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
- uint32_t offset)
+static int vmdk_alloc_cluster_link_l2(VmdkExtent *extent,
+ VmdkMetaData *m_data, bool zeroed)
{
- offset = cpu_to_le32(offset);
+ int i;
+ uint32_t offset, temp_offset;
+
+ if (zeroed) {
+ temp_offset = VMDK_GTE_ZEROED;
+ } else {
+ temp_offset = m_data->offset;
+ }
+
+ temp_offset = cpu_to_le32(temp_offset);
+
/* update L2 table */
- if (bdrv_pwrite_sync(extent->file,
+ offset = temp_offset;
+ for (i = 0; i < m_data->nb_clusters; i++) {
+ if (bdrv_pwrite_sync(extent->file,
((int64_t)m_data->l2_offset * 512)
- + (m_data->l2_index * sizeof(offset)),
- &offset, sizeof(offset)) < 0) {
- return VMDK_ERROR;
+ + ((m_data->l2_index + i) * sizeof(offset)),
+ &(offset), sizeof(offset)) < 0) {
+ return VMDK_ERROR;
+ }
+ if (!zeroed) {
+ offset += 128;
+ }
}
+
/* update backup L2 table */
+ offset = temp_offset;
if (extent->l1_backup_table_offset != 0) {
m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
- if (bdrv_pwrite_sync(extent->file,
- ((int64_t)m_data->l2_offset * 512)
- + (m_data->l2_index * sizeof(offset)),
- &offset, sizeof(offset)) < 0) {
- return VMDK_ERROR;
+ for (i = 0; i < m_data->nb_clusters; i++) {
+ if (bdrv_pwrite_sync(extent->file,
+ ((int64_t)m_data->l2_offset * 512)
+ + ((m_data->l2_index + i) * sizeof(offset)),
+ &(offset), sizeof(offset)) < 0) {
+ return VMDK_ERROR;
+ }
+ if (!zeroed) {
+ offset += 128;
+ }
}
}
+
+ offset = temp_offset;
if (m_data->l2_cache_entry) {
- *m_data->l2_cache_entry = offset;
+ for (i = 0; i < m_data->nb_clusters; i++) {
+ *m_data->l2_cache_entry = offset;
+ m_data->l2_cache_entry++;
+
+ if (!zeroed) {
+ offset += 128;
+ }
+ }
+ }
+
+ return VMDK_OK;
+}
+
+static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
+ bool zeroed)
+{
+ int ret;
+
+ while (m_data->next != NULL) {
+ VmdkMetaData *next;
+
+ ret = vmdk_alloc_cluster_link_l2(extent, m_data, zeroed);
+ if (ret < 0) {
+ return ret;
+ }
+
+ next = m_data->next;
+ m_data = next;
}
return VMDK_OK;
@@ -1271,7 +1325,7 @@ exit:
*/
static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
uint64_t offset, uint64_t *cluster_offset,
- int64_t *bytes, VmdkMetaData *m_data,
+ int64_t *bytes, VmdkMetaData **m_data,
bool allocate, uint32_t *total_alloc_clusters)
{
int l1_index, l2_offset, l2_index;
@@ -1280,6 +1334,7 @@ static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
uint32_t nb_clusters;
bool zeroed = false;
uint64_t skip_start_bytes, skip_end_bytes;
+ VmdkMetaData *old_m_data;
int ret;
ret = get_cluster_table(extent, offset, &l1_index, &l2_offset,
@@ -1330,13 +1385,21 @@ static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
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;
+
+ if (*m_data) {
+ old_m_data = *m_data;
+ *m_data = g_malloc0(sizeof(**m_data));
+
+ **m_data = (VmdkMetaData) {
+ .valid = 1,
+ .l1_index = l1_index,
+ .l2_index = l2_index,
+ .l2_offset = l2_offset,
+ .l2_cache_entry = &l2_table[l2_index],
+ .nb_clusters = nb_clusters,
+ .offset = cluster_sector,
+ .next = old_m_data,
+ };
}
}
*cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
@@ -1365,7 +1428,7 @@ static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
*/
static int vmdk_alloc_cluster_offset(BlockDriverState *bs,
VmdkExtent *extent,
- VmdkMetaData *m_data, uint64_t offset,
+ VmdkMetaData **m_data, uint64_t offset,
bool allocate, uint64_t *cluster_offset,
int64_t bytes,
uint32_t *total_alloc_clusters)
@@ -1385,8 +1448,8 @@ static int vmdk_alloc_cluster_offset(BlockDriverState *bs,
new_cluster_offset = 0;
*cluster_offset = 0;
n_bytes = 0;
- if (m_data) {
- m_data->valid = 0;
+ if (*m_data) {
+ (*m_data)->valid = 0;
}
/* due to L2 table margins all bytes may not get allocated at once */
@@ -1768,9 +1831,11 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
uint64_t cluster_offset;
uint64_t bytes_done = 0;
uint64_t extent_size;
- VmdkMetaData m_data;
+ VmdkMetaData *m_data;
uint32_t total_alloc_clusters = 0;
+ m_data = g_malloc0(sizeof(*m_data));
+
if (DIV_ROUND_UP(offset, BDRV_SECTOR_SIZE) > bs->total_sectors) {
error_report("Wrong offset: offset=0x%" PRIx64
" total_sectors=0x%" PRIx64,
@@ -1779,6 +1844,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
}
while (bytes > 0) {
+ m_data->next = NULL;
extent = find_extent(s, offset >> BDRV_SECTOR_BITS, extent);
if (!extent) {
return -EIO;
@@ -1825,7 +1891,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
total_alloc_clusters;
if (!zero_dry_run) {
/* update L2 tables */
- if (vmdk_L2update(extent, &m_data, VMDK_GTE_ZEROED)
+ if (vmdk_L2update(extent, m_data, zeroed)
!= VMDK_OK) {
return -EIO;
}
@@ -1839,10 +1905,9 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
if (ret) {
return ret;
}
- if (m_data.valid) {
+ if (m_data->valid) {
/* update L2 tables */
- if (vmdk_L2update(extent, &m_data,
- cluster_offset >> BDRV_SECTOR_BITS)
+ if (vmdk_L2update(extent, m_data, zeroed)
!= VMDK_OK) {
return -EIO;
}
@@ -1852,6 +1917,13 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
offset += n_bytes;
bytes_done += n_bytes;
+ while (m_data->next != NULL) {
+ VmdkMetaData *next;
+ next = m_data->next;
+ g_free(m_data);
+ m_data = next;
+ }
+
/* update CID on the first write every time the virtual disk is
* opened */
if (!s->cid_updated) {
@@ -1862,6 +1934,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
s->cid_updated = true;
}
}
+ g_free(m_data);
return 0;
}
--
2.6.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 7/7] vmdk: Update metadata for multiple clusters
2017-03-25 11:18 ` [Qemu-devel] [PATCH v2 7/7] vmdk: Update metadata for multiple clusters Ashijeet Acharya
@ 2017-03-31 7:26 ` Fam Zheng
2017-03-31 8:47 ` Ashijeet Acharya
0 siblings, 1 reply; 16+ messages in thread
From: Fam Zheng @ 2017-03-31 7:26 UTC (permalink / raw)
To: Ashijeet Acharya; +Cc: kwolf, qemu-block, stefanha, qemu-devel, mreitz, jsnow
On Sat, 03/25 16:48, Ashijeet Acharya wrote:
> Include a next pointer in VmdkMetaData struct to point to the previous
> allocated L2 table. Modify vmdk_L2update to start updating metadata for
> allocation of multiple clusters at once.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
> block/vmdk.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 102 insertions(+), 29 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 3de8b8f..4517409 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -137,6 +137,8 @@ typedef struct VmdkMetaData {
> int valid;
> uint32_t *l2_cache_entry;
> uint32_t nb_clusters;
> + uint32_t offset;
> + struct VmdkMetaData *next;
> } VmdkMetaData;
>
> typedef struct VmdkGrainMarker {
> @@ -1037,29 +1039,81 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
> }
> }
>
> -static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
> - uint32_t offset)
> +static int vmdk_alloc_cluster_link_l2(VmdkExtent *extent,
> + VmdkMetaData *m_data, bool zeroed)
> {
> - offset = cpu_to_le32(offset);
> + int i;
> + uint32_t offset, temp_offset;
> +
> + if (zeroed) {
> + temp_offset = VMDK_GTE_ZEROED;
> + } else {
> + temp_offset = m_data->offset;
> + }
> +
> + temp_offset = cpu_to_le32(temp_offset);
> +
> /* update L2 table */
> - if (bdrv_pwrite_sync(extent->file,
> + offset = temp_offset;
> + for (i = 0; i < m_data->nb_clusters; i++) {
> + if (bdrv_pwrite_sync(extent->file,
> ((int64_t)m_data->l2_offset * 512)
> - + (m_data->l2_index * sizeof(offset)),
> - &offset, sizeof(offset)) < 0) {
> - return VMDK_ERROR;
> + + ((m_data->l2_index + i) * sizeof(offset)),
> + &(offset), sizeof(offset)) < 0) {
> + return VMDK_ERROR;
> + }
> + if (!zeroed) {
> + offset += 128;
> + }
> }
> +
> /* update backup L2 table */
> + offset = temp_offset;
> if (extent->l1_backup_table_offset != 0) {
> m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
> - if (bdrv_pwrite_sync(extent->file,
> - ((int64_t)m_data->l2_offset * 512)
> - + (m_data->l2_index * sizeof(offset)),
> - &offset, sizeof(offset)) < 0) {
> - return VMDK_ERROR;
> + for (i = 0; i < m_data->nb_clusters; i++) {
> + if (bdrv_pwrite_sync(extent->file,
> + ((int64_t)m_data->l2_offset * 512)
> + + ((m_data->l2_index + i) * sizeof(offset)),
> + &(offset), sizeof(offset)) < 0) {
> + return VMDK_ERROR;
> + }
> + if (!zeroed) {
> + offset += 128;
> + }
> }
> }
> +
> + offset = temp_offset;
> if (m_data->l2_cache_entry) {
> - *m_data->l2_cache_entry = offset;
> + for (i = 0; i < m_data->nb_clusters; i++) {
> + *m_data->l2_cache_entry = offset;
> + m_data->l2_cache_entry++;
> +
> + if (!zeroed) {
> + offset += 128;
> + }
> + }
> + }
> +
> + return VMDK_OK;
> +}
> +
> +static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
> + bool zeroed)
> +{
> + int ret;
> +
> + while (m_data->next != NULL) {
> + VmdkMetaData *next;
> +
> + ret = vmdk_alloc_cluster_link_l2(extent, m_data, zeroed);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + next = m_data->next;
> + m_data = next;
I don't see why the next pointer is necessary. Also the tail is always unused,
why do you need to allocate it?
But more importantly, I think you could further batch multiple updates in the
same L2 table and only do one bdrv_pwrite_sync.
Fam
> }
>
> return VMDK_OK;
> @@ -1271,7 +1325,7 @@ exit:
> */
> static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
> uint64_t offset, uint64_t *cluster_offset,
> - int64_t *bytes, VmdkMetaData *m_data,
> + int64_t *bytes, VmdkMetaData **m_data,
> bool allocate, uint32_t *total_alloc_clusters)
> {
> int l1_index, l2_offset, l2_index;
> @@ -1280,6 +1334,7 @@ static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
> uint32_t nb_clusters;
> bool zeroed = false;
> uint64_t skip_start_bytes, skip_end_bytes;
> + VmdkMetaData *old_m_data;
> int ret;
>
> ret = get_cluster_table(extent, offset, &l1_index, &l2_offset,
> @@ -1330,13 +1385,21 @@ static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
> 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;
> +
> + if (*m_data) {
> + old_m_data = *m_data;
> + *m_data = g_malloc0(sizeof(**m_data));
> +
> + **m_data = (VmdkMetaData) {
> + .valid = 1,
> + .l1_index = l1_index,
> + .l2_index = l2_index,
> + .l2_offset = l2_offset,
> + .l2_cache_entry = &l2_table[l2_index],
> + .nb_clusters = nb_clusters,
> + .offset = cluster_sector,
> + .next = old_m_data,
> + };
> }
> }
> *cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
> @@ -1365,7 +1428,7 @@ static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
> */
> static int vmdk_alloc_cluster_offset(BlockDriverState *bs,
> VmdkExtent *extent,
> - VmdkMetaData *m_data, uint64_t offset,
> + VmdkMetaData **m_data, uint64_t offset,
> bool allocate, uint64_t *cluster_offset,
> int64_t bytes,
> uint32_t *total_alloc_clusters)
> @@ -1385,8 +1448,8 @@ static int vmdk_alloc_cluster_offset(BlockDriverState *bs,
> new_cluster_offset = 0;
> *cluster_offset = 0;
> n_bytes = 0;
> - if (m_data) {
> - m_data->valid = 0;
> + if (*m_data) {
> + (*m_data)->valid = 0;
> }
>
> /* due to L2 table margins all bytes may not get allocated at once */
> @@ -1768,9 +1831,11 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
> uint64_t cluster_offset;
> uint64_t bytes_done = 0;
> uint64_t extent_size;
> - VmdkMetaData m_data;
> + VmdkMetaData *m_data;
> uint32_t total_alloc_clusters = 0;
>
> + m_data = g_malloc0(sizeof(*m_data));
> +
> if (DIV_ROUND_UP(offset, BDRV_SECTOR_SIZE) > bs->total_sectors) {
> error_report("Wrong offset: offset=0x%" PRIx64
> " total_sectors=0x%" PRIx64,
> @@ -1779,6 +1844,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
> }
>
> while (bytes > 0) {
> + m_data->next = NULL;
> extent = find_extent(s, offset >> BDRV_SECTOR_BITS, extent);
> if (!extent) {
> return -EIO;
> @@ -1825,7 +1891,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
> total_alloc_clusters;
> if (!zero_dry_run) {
> /* update L2 tables */
> - if (vmdk_L2update(extent, &m_data, VMDK_GTE_ZEROED)
> + if (vmdk_L2update(extent, m_data, zeroed)
> != VMDK_OK) {
> return -EIO;
> }
> @@ -1839,10 +1905,9 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
> if (ret) {
> return ret;
> }
> - if (m_data.valid) {
> + if (m_data->valid) {
> /* update L2 tables */
> - if (vmdk_L2update(extent, &m_data,
> - cluster_offset >> BDRV_SECTOR_BITS)
> + if (vmdk_L2update(extent, m_data, zeroed)
> != VMDK_OK) {
> return -EIO;
> }
> @@ -1852,6 +1917,13 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
> offset += n_bytes;
> bytes_done += n_bytes;
>
> + while (m_data->next != NULL) {
> + VmdkMetaData *next;
> + next = m_data->next;
> + g_free(m_data);
> + m_data = next;
> + }
> +
> /* update CID on the first write every time the virtual disk is
> * opened */
> if (!s->cid_updated) {
> @@ -1862,6 +1934,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
> s->cid_updated = true;
> }
> }
> + g_free(m_data);
> return 0;
> }
>
> --
> 2.6.2
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 7/7] vmdk: Update metadata for multiple clusters
2017-03-31 7:26 ` Fam Zheng
@ 2017-03-31 8:47 ` Ashijeet Acharya
2017-03-31 9:08 ` Fam Zheng
0 siblings, 1 reply; 16+ messages in thread
From: Ashijeet Acharya @ 2017-03-31 8:47 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, qemu block, Stefan Hajnoczi, QEMU Developers,
Max Reitz, John Snow
On Fri, Mar 31, 2017 at 12:56 PM, Fam Zheng <famz@redhat.com> wrote:
> On Sat, 03/25 16:48, Ashijeet Acharya wrote:
>> Include a next pointer in VmdkMetaData struct to point to the previous
>> allocated L2 table. Modify vmdk_L2update to start updating metadata for
>> allocation of multiple clusters at once.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>> block/vmdk.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 102 insertions(+), 29 deletions(-)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 3de8b8f..4517409 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -137,6 +137,8 @@ typedef struct VmdkMetaData {
>> int valid;
>> uint32_t *l2_cache_entry;
>> uint32_t nb_clusters;
>> + uint32_t offset;
>> + struct VmdkMetaData *next;
>> } VmdkMetaData;
>>
>> typedef struct VmdkGrainMarker {
>> @@ -1037,29 +1039,81 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
>> }
>> }
>>
>> -static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>> - uint32_t offset)
>> +static int vmdk_alloc_cluster_link_l2(VmdkExtent *extent,
>> + VmdkMetaData *m_data, bool zeroed)
>> {
>> - offset = cpu_to_le32(offset);
>> + int i;
>> + uint32_t offset, temp_offset;
>> +
>> + if (zeroed) {
>> + temp_offset = VMDK_GTE_ZEROED;
>> + } else {
>> + temp_offset = m_data->offset;
>> + }
>> +
>> + temp_offset = cpu_to_le32(temp_offset);
>> +
>> /* update L2 table */
>> - if (bdrv_pwrite_sync(extent->file,
>> + offset = temp_offset;
>> + for (i = 0; i < m_data->nb_clusters; i++) {
>> + if (bdrv_pwrite_sync(extent->file,
>> ((int64_t)m_data->l2_offset * 512)
>> - + (m_data->l2_index * sizeof(offset)),
>> - &offset, sizeof(offset)) < 0) {
>> - return VMDK_ERROR;
>> + + ((m_data->l2_index + i) * sizeof(offset)),
>> + &(offset), sizeof(offset)) < 0) {
>> + return VMDK_ERROR;
>> + }
>> + if (!zeroed) {
>> + offset += 128;
>> + }
>> }
>> +
>> /* update backup L2 table */
>> + offset = temp_offset;
>> if (extent->l1_backup_table_offset != 0) {
>> m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
>> - if (bdrv_pwrite_sync(extent->file,
>> - ((int64_t)m_data->l2_offset * 512)
>> - + (m_data->l2_index * sizeof(offset)),
>> - &offset, sizeof(offset)) < 0) {
>> - return VMDK_ERROR;
>> + for (i = 0; i < m_data->nb_clusters; i++) {
>> + if (bdrv_pwrite_sync(extent->file,
>> + ((int64_t)m_data->l2_offset * 512)
>> + + ((m_data->l2_index + i) * sizeof(offset)),
>> + &(offset), sizeof(offset)) < 0) {
>> + return VMDK_ERROR;
>> + }
>> + if (!zeroed) {
>> + offset += 128;
>> + }
>> }
>> }
>> +
>> + offset = temp_offset;
>> if (m_data->l2_cache_entry) {
>> - *m_data->l2_cache_entry = offset;
>> + for (i = 0; i < m_data->nb_clusters; i++) {
>> + *m_data->l2_cache_entry = offset;
>> + m_data->l2_cache_entry++;
>> +
>> + if (!zeroed) {
>> + offset += 128;
>> + }
>> + }
>> + }
>> +
>> + return VMDK_OK;
>> +}
>> +
>> +static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>> + bool zeroed)
>> +{
>> + int ret;
>> +
>> + while (m_data->next != NULL) {
>> + VmdkMetaData *next;
>> +
>> + ret = vmdk_alloc_cluster_link_l2(extent, m_data, zeroed);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> + next = m_data->next;
>> + m_data = next;
>
> I don't see why the next pointer is necessary. Also the tail is always unused,
> why do you need to allocate it?
If I don't allocate the tail, I was getting a segfault in vmdk_pwritev().
> But more importantly, I think you could further batch multiple updates in the
> same L2 table and only do one bdrv_pwrite_sync.
Wouldn't the l2_offset need to change change for every subsequent L2
table i.e. after ever 512 cluster boundary?
Ashijeet
>
> Fam
>
>> }
>>
>> return VMDK_OK;
>> @@ -1271,7 +1325,7 @@ exit:
>> */
>> static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
>> uint64_t offset, uint64_t *cluster_offset,
>> - int64_t *bytes, VmdkMetaData *m_data,
>> + int64_t *bytes, VmdkMetaData **m_data,
>> bool allocate, uint32_t *total_alloc_clusters)
>> {
>> int l1_index, l2_offset, l2_index;
>> @@ -1280,6 +1334,7 @@ static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
>> uint32_t nb_clusters;
>> bool zeroed = false;
>> uint64_t skip_start_bytes, skip_end_bytes;
>> + VmdkMetaData *old_m_data;
>> int ret;
>>
>> ret = get_cluster_table(extent, offset, &l1_index, &l2_offset,
>> @@ -1330,13 +1385,21 @@ static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
>> 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;
>> +
>> + if (*m_data) {
>> + old_m_data = *m_data;
>> + *m_data = g_malloc0(sizeof(**m_data));
>> +
>> + **m_data = (VmdkMetaData) {
>> + .valid = 1,
>> + .l1_index = l1_index,
>> + .l2_index = l2_index,
>> + .l2_offset = l2_offset,
>> + .l2_cache_entry = &l2_table[l2_index],
>> + .nb_clusters = nb_clusters,
>> + .offset = cluster_sector,
>> + .next = old_m_data,
>> + };
>> }
>> }
>> *cluster_offset = cluster_sector << BDRV_SECTOR_BITS;
>> @@ -1365,7 +1428,7 @@ static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
>> */
>> static int vmdk_alloc_cluster_offset(BlockDriverState *bs,
>> VmdkExtent *extent,
>> - VmdkMetaData *m_data, uint64_t offset,
>> + VmdkMetaData **m_data, uint64_t offset,
>> bool allocate, uint64_t *cluster_offset,
>> int64_t bytes,
>> uint32_t *total_alloc_clusters)
>> @@ -1385,8 +1448,8 @@ static int vmdk_alloc_cluster_offset(BlockDriverState *bs,
>> new_cluster_offset = 0;
>> *cluster_offset = 0;
>> n_bytes = 0;
>> - if (m_data) {
>> - m_data->valid = 0;
>> + if (*m_data) {
>> + (*m_data)->valid = 0;
>> }
>>
>> /* due to L2 table margins all bytes may not get allocated at once */
>> @@ -1768,9 +1831,11 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
>> uint64_t cluster_offset;
>> uint64_t bytes_done = 0;
>> uint64_t extent_size;
>> - VmdkMetaData m_data;
>> + VmdkMetaData *m_data;
>> uint32_t total_alloc_clusters = 0;
>>
>> + m_data = g_malloc0(sizeof(*m_data));
>> +
>> if (DIV_ROUND_UP(offset, BDRV_SECTOR_SIZE) > bs->total_sectors) {
>> error_report("Wrong offset: offset=0x%" PRIx64
>> " total_sectors=0x%" PRIx64,
>> @@ -1779,6 +1844,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
>> }
>>
>> while (bytes > 0) {
>> + m_data->next = NULL;
>> extent = find_extent(s, offset >> BDRV_SECTOR_BITS, extent);
>> if (!extent) {
>> return -EIO;
>> @@ -1825,7 +1891,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
>> total_alloc_clusters;
>> if (!zero_dry_run) {
>> /* update L2 tables */
>> - if (vmdk_L2update(extent, &m_data, VMDK_GTE_ZEROED)
>> + if (vmdk_L2update(extent, m_data, zeroed)
>> != VMDK_OK) {
>> return -EIO;
>> }
>> @@ -1839,10 +1905,9 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
>> if (ret) {
>> return ret;
>> }
>> - if (m_data.valid) {
>> + if (m_data->valid) {
>> /* update L2 tables */
>> - if (vmdk_L2update(extent, &m_data,
>> - cluster_offset >> BDRV_SECTOR_BITS)
>> + if (vmdk_L2update(extent, m_data, zeroed)
>> != VMDK_OK) {
>> return -EIO;
>> }
>> @@ -1852,6 +1917,13 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
>> offset += n_bytes;
>> bytes_done += n_bytes;
>>
>> + while (m_data->next != NULL) {
>> + VmdkMetaData *next;
>> + next = m_data->next;
>> + g_free(m_data);
>> + m_data = next;
>> + }
>> +
>> /* update CID on the first write every time the virtual disk is
>> * opened */
>> if (!s->cid_updated) {
>> @@ -1862,6 +1934,7 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
>> s->cid_updated = true;
>> }
>> }
>> + g_free(m_data);
>> return 0;
>> }
>>
>> --
>> 2.6.2
>>
>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 7/7] vmdk: Update metadata for multiple clusters
2017-03-31 8:47 ` Ashijeet Acharya
@ 2017-03-31 9:08 ` Fam Zheng
2017-03-31 9:41 ` Ashijeet Acharya
0 siblings, 1 reply; 16+ messages in thread
From: Fam Zheng @ 2017-03-31 9:08 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: Kevin Wolf, qemu block, Stefan Hajnoczi, QEMU Developers,
Max Reitz, John Snow
On Fri, 03/31 14:17, Ashijeet Acharya wrote:
> On Fri, Mar 31, 2017 at 12:56 PM, Fam Zheng <famz@redhat.com> wrote:
> > On Sat, 03/25 16:48, Ashijeet Acharya wrote:
> >> Include a next pointer in VmdkMetaData struct to point to the previous
> >> allocated L2 table. Modify vmdk_L2update to start updating metadata for
> >> allocation of multiple clusters at once.
> >>
> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> >> ---
> >> block/vmdk.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++-------------
> >> 1 file changed, 102 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/block/vmdk.c b/block/vmdk.c
> >> index 3de8b8f..4517409 100644
> >> --- a/block/vmdk.c
> >> +++ b/block/vmdk.c
> >> @@ -137,6 +137,8 @@ typedef struct VmdkMetaData {
> >> int valid;
> >> uint32_t *l2_cache_entry;
> >> uint32_t nb_clusters;
> >> + uint32_t offset;
> >> + struct VmdkMetaData *next;
> >> } VmdkMetaData;
> >>
> >> typedef struct VmdkGrainMarker {
> >> @@ -1037,29 +1039,81 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
> >> }
> >> }
> >>
> >> -static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
> >> - uint32_t offset)
> >> +static int vmdk_alloc_cluster_link_l2(VmdkExtent *extent,
> >> + VmdkMetaData *m_data, bool zeroed)
> >> {
> >> - offset = cpu_to_le32(offset);
> >> + int i;
> >> + uint32_t offset, temp_offset;
> >> +
> >> + if (zeroed) {
> >> + temp_offset = VMDK_GTE_ZEROED;
> >> + } else {
> >> + temp_offset = m_data->offset;
> >> + }
> >> +
> >> + temp_offset = cpu_to_le32(temp_offset);
> >> +
> >> /* update L2 table */
> >> - if (bdrv_pwrite_sync(extent->file,
> >> + offset = temp_offset;
> >> + for (i = 0; i < m_data->nb_clusters; i++) {
> >> + if (bdrv_pwrite_sync(extent->file,
> >> ((int64_t)m_data->l2_offset * 512)
> >> - + (m_data->l2_index * sizeof(offset)),
> >> - &offset, sizeof(offset)) < 0) {
> >> - return VMDK_ERROR;
> >> + + ((m_data->l2_index + i) * sizeof(offset)),
> >> + &(offset), sizeof(offset)) < 0) {
> >> + return VMDK_ERROR;
> >> + }
> >> + if (!zeroed) {
> >> + offset += 128;
> >> + }
> >> }
> >> +
> >> /* update backup L2 table */
> >> + offset = temp_offset;
> >> if (extent->l1_backup_table_offset != 0) {
> >> m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
> >> - if (bdrv_pwrite_sync(extent->file,
> >> - ((int64_t)m_data->l2_offset * 512)
> >> - + (m_data->l2_index * sizeof(offset)),
> >> - &offset, sizeof(offset)) < 0) {
> >> - return VMDK_ERROR;
> >> + for (i = 0; i < m_data->nb_clusters; i++) {
> >> + if (bdrv_pwrite_sync(extent->file,
> >> + ((int64_t)m_data->l2_offset * 512)
> >> + + ((m_data->l2_index + i) * sizeof(offset)),
> >> + &(offset), sizeof(offset)) < 0) {
> >> + return VMDK_ERROR;
> >> + }
> >> + if (!zeroed) {
> >> + offset += 128;
> >> + }
> >> }
> >> }
> >> +
> >> + offset = temp_offset;
> >> if (m_data->l2_cache_entry) {
> >> - *m_data->l2_cache_entry = offset;
> >> + for (i = 0; i < m_data->nb_clusters; i++) {
> >> + *m_data->l2_cache_entry = offset;
> >> + m_data->l2_cache_entry++;
> >> +
> >> + if (!zeroed) {
> >> + offset += 128;
> >> + }
> >> + }
> >> + }
> >> +
> >> + return VMDK_OK;
> >> +}
> >> +
> >> +static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
> >> + bool zeroed)
> >> +{
> >> + int ret;
> >> +
> >> + while (m_data->next != NULL) {
> >> + VmdkMetaData *next;
> >> +
> >> + ret = vmdk_alloc_cluster_link_l2(extent, m_data, zeroed);
> >> + if (ret < 0) {
> >> + return ret;
> >> + }
> >> +
> >> + next = m_data->next;
> >> + m_data = next;
> >
> > I don't see why the next pointer is necessary. Also the tail is always unused,
> > why do you need to allocate it?
>
> If I don't allocate the tail, I was getting a segfault in vmdk_pwritev().
That may be because the way you interate the linked list in vmdk_pwritev is:
> while (m_data->next != NULL) {
> VmdkMetaData *next;
> next = m_data->next;
> g_free(m_data);
> m_data = next;
> }
>
which does require a dummy tail.
But after all it's still not clear to me why you cannot keep m_data on stack,
and why you need the next pointer at all.
Fam
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 7/7] vmdk: Update metadata for multiple clusters
2017-03-31 9:08 ` Fam Zheng
@ 2017-03-31 9:41 ` Ashijeet Acharya
0 siblings, 0 replies; 16+ messages in thread
From: Ashijeet Acharya @ 2017-03-31 9:41 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, qemu block, Stefan Hajnoczi, QEMU Developers,
Max Reitz, John Snow
On Fri, Mar 31, 2017 at 2:38 PM, Fam Zheng <famz@redhat.com> wrote:
> On Fri, 03/31 14:17, Ashijeet Acharya wrote:
>> On Fri, Mar 31, 2017 at 12:56 PM, Fam Zheng <famz@redhat.com> wrote:
>> > On Sat, 03/25 16:48, Ashijeet Acharya wrote:
>> >> Include a next pointer in VmdkMetaData struct to point to the previous
>> >> allocated L2 table. Modify vmdk_L2update to start updating metadata for
>> >> allocation of multiple clusters at once.
>> >>
>> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> >> ---
>> >> block/vmdk.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>> >> 1 file changed, 102 insertions(+), 29 deletions(-)
>> >>
>> >> diff --git a/block/vmdk.c b/block/vmdk.c
>> >> index 3de8b8f..4517409 100644
>> >> --- a/block/vmdk.c
>> >> +++ b/block/vmdk.c
>> >> @@ -137,6 +137,8 @@ typedef struct VmdkMetaData {
>> >> int valid;
>> >> uint32_t *l2_cache_entry;
>> >> uint32_t nb_clusters;
>> >> + uint32_t offset;
>> >> + struct VmdkMetaData *next;
>> >> } VmdkMetaData;
>> >>
>> >> typedef struct VmdkGrainMarker {
>> >> @@ -1037,29 +1039,81 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
>> >> }
>> >> }
>> >>
>> >> -static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>> >> - uint32_t offset)
>> >> +static int vmdk_alloc_cluster_link_l2(VmdkExtent *extent,
>> >> + VmdkMetaData *m_data, bool zeroed)
>> >> {
>> >> - offset = cpu_to_le32(offset);
>> >> + int i;
>> >> + uint32_t offset, temp_offset;
>> >> +
>> >> + if (zeroed) {
>> >> + temp_offset = VMDK_GTE_ZEROED;
>> >> + } else {
>> >> + temp_offset = m_data->offset;
>> >> + }
>> >> +
>> >> + temp_offset = cpu_to_le32(temp_offset);
>> >> +
>> >> /* update L2 table */
>> >> - if (bdrv_pwrite_sync(extent->file,
>> >> + offset = temp_offset;
>> >> + for (i = 0; i < m_data->nb_clusters; i++) {
>> >> + if (bdrv_pwrite_sync(extent->file,
>> >> ((int64_t)m_data->l2_offset * 512)
>> >> - + (m_data->l2_index * sizeof(offset)),
>> >> - &offset, sizeof(offset)) < 0) {
>> >> - return VMDK_ERROR;
>> >> + + ((m_data->l2_index + i) * sizeof(offset)),
>> >> + &(offset), sizeof(offset)) < 0) {
>> >> + return VMDK_ERROR;
>> >> + }
>> >> + if (!zeroed) {
>> >> + offset += 128;
>> >> + }
>> >> }
>> >> +
>> >> /* update backup L2 table */
>> >> + offset = temp_offset;
>> >> if (extent->l1_backup_table_offset != 0) {
>> >> m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];
>> >> - if (bdrv_pwrite_sync(extent->file,
>> >> - ((int64_t)m_data->l2_offset * 512)
>> >> - + (m_data->l2_index * sizeof(offset)),
>> >> - &offset, sizeof(offset)) < 0) {
>> >> - return VMDK_ERROR;
>> >> + for (i = 0; i < m_data->nb_clusters; i++) {
>> >> + if (bdrv_pwrite_sync(extent->file,
>> >> + ((int64_t)m_data->l2_offset * 512)
>> >> + + ((m_data->l2_index + i) * sizeof(offset)),
>> >> + &(offset), sizeof(offset)) < 0) {
>> >> + return VMDK_ERROR;
>> >> + }
>> >> + if (!zeroed) {
>> >> + offset += 128;
>> >> + }
>> >> }
>> >> }
>> >> +
>> >> + offset = temp_offset;
>> >> if (m_data->l2_cache_entry) {
>> >> - *m_data->l2_cache_entry = offset;
>> >> + for (i = 0; i < m_data->nb_clusters; i++) {
>> >> + *m_data->l2_cache_entry = offset;
>> >> + m_data->l2_cache_entry++;
>> >> +
>> >> + if (!zeroed) {
>> >> + offset += 128;
>> >> + }
>> >> + }
>> >> + }
>> >> +
>> >> + return VMDK_OK;
>> >> +}
>> >> +
>> >> +static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,
>> >> + bool zeroed)
>> >> +{
>> >> + int ret;
>> >> +
>> >> + while (m_data->next != NULL) {
>> >> + VmdkMetaData *next;
>> >> +
>> >> + ret = vmdk_alloc_cluster_link_l2(extent, m_data, zeroed);
>> >> + if (ret < 0) {
>> >> + return ret;
>> >> + }
>> >> +
>> >> + next = m_data->next;
>> >> + m_data = next;
>> >
>> > I don't see why the next pointer is necessary. Also the tail is always unused,
>> > why do you need to allocate it?
>>
>> If I don't allocate the tail, I was getting a segfault in vmdk_pwritev().
>
> That may be because the way you interate the linked list in vmdk_pwritev is:
>
>> while (m_data->next != NULL) {
>> VmdkMetaData *next;
>> next = m_data->next;
>> g_free(m_data);
>> m_data = next;
>> }
>>
>
> which does require a dummy tail.
No, I remember it segfaulting even before I inserted that piece of
code. I think the reason is that I try to access m_data->valid inside
vmdk_pwritev()...
>
> But after all it's still not clear to me why you cannot keep m_data on stack,
...and by using malloc and moving it to the heap solved my problem,
plus for constructing the linked list.
> and why you need the next pointer at all.
If I don't segregate them in batches of 512, I will need to increment
the l2_offset manually...right? If I don't use the next pointer, what
solution do you recommend?
Ashijeet
>
> Fam
^ permalink raw reply [flat|nested] 16+ messages in thread