From: Lidong Chen <lidongchen@tencent.com>
This patch optimizes the performance by coalescing the same write type.
When the zero/non-zero state changes, perform the write for the accumulated
cluster count.
Signed-off-by: Lidong Chen <lidongchen@tencent.com>
---
Thanks Fam Zheng and Stefan's advice.
---
migration/block.c | 66 +++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 49 insertions(+), 17 deletions(-)
diff --git a/migration/block.c b/migration/block.c
index 060087f..e9c5e21 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -40,6 +40,8 @@
#define MAX_INFLIGHT_IO 512
+#define MIN_CLUSTER_SIZE 8192
+
//#define DEBUG_BLK_MIGRATION
#ifdef DEBUG_BLK_MIGRATION
@@ -923,10 +925,11 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
}
ret = bdrv_get_info(blk_bs(blk), &bdi);
- if (ret == 0 && bdi.cluster_size > 0 &&
- bdi.cluster_size <= BLOCK_SIZE &&
- BLOCK_SIZE % bdi.cluster_size == 0) {
+ if (ret == 0 && bdi.cluster_size > 0) {
cluster_size = bdi.cluster_size;
+ while (cluster_size < MIN_CLUSTER_SIZE) {
+ cluster_size *= 2;
+ }
} else {
cluster_size = BLOCK_SIZE;
}
@@ -943,29 +946,58 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
nr_sectors * BDRV_SECTOR_SIZE,
BDRV_REQ_MAY_UNMAP);
} else {
- int i;
- int64_t cur_addr;
- uint8_t *cur_buf;
+ int64_t cur_addr = addr * BDRV_SECTOR_SIZE;
+ uint8_t *cur_buf = NULL;
+ int64_t last_addr = addr * BDRV_SECTOR_SIZE;
+ uint8_t *last_buf = NULL;
+ int64_t end_addr = addr * BDRV_SECTOR_SIZE + BLOCK_SIZE;
buf = g_malloc(BLOCK_SIZE);
qemu_get_buffer(f, buf, BLOCK_SIZE);
- for (i = 0; i < BLOCK_SIZE / cluster_size; i++) {
- cur_addr = addr * BDRV_SECTOR_SIZE + i * cluster_size;
- cur_buf = buf + i * cluster_size;
-
- if ((!block_mig_state.zero_blocks ||
- cluster_size < BLOCK_SIZE) &&
- buffer_is_zero(cur_buf, cluster_size)) {
- ret = blk_pwrite_zeroes(blk, cur_addr,
- cluster_size,
+ cur_buf = buf;
+ last_buf = buf;
+
+ while (last_addr < end_addr) {
+ int is_zero = 0;
+ int buf_size = MIN(end_addr - cur_addr, cluster_size);
+
+ /* If the "zero blocks" migration capability is enabled
+ * and the buf_size == BLOCK_SIZE, then the source QEMU
+ * process has already scanned for zeroes. CPU is wasted
+ * scanning for zeroes in the destination QEMU process.
+ */
+ if (block_mig_state.zero_blocks &&
+ buf_size == BLOCK_SIZE) {
+ is_zero = 0;
+ } else {
+ is_zero = buffer_is_zero(cur_buf, buf_size);
+ }
+
+ cur_addr += buf_size;
+ cur_buf += buf_size;
+ while (cur_addr < end_addr) {
+ buf_size = MIN(end_addr - cur_addr, cluster_size);
+ if (is_zero != buffer_is_zero(cur_buf, buf_size)) {
+ break;
+ }
+ cur_addr += buf_size;
+ cur_buf += buf_size;
+ }
+
+ if (is_zero) {
+ ret = blk_pwrite_zeroes(blk, last_addr,
+ cur_addr - last_addr,
BDRV_REQ_MAY_UNMAP);
} else {
- ret = blk_pwrite(blk, cur_addr, cur_buf,
- cluster_size, 0);
+ ret = blk_pwrite(blk, last_addr, last_buf,
+ cur_addr - last_addr, 0);
}
if (ret < 0) {
break;
}
+
+ last_addr = cur_addr;
+ last_buf = cur_buf;
}
g_free(buf);
}
--
1.8.3.1
the reason of MIN_CLUSTER_SIZE is 8192 is base on the performance
test result. the performance is only reduce obviously when cluster_size is
less than 8192.
I write this code, run in guest os. to create the worst condition.
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int main()
{
char *zero;
char *nonzero;
FILE* fp = fopen("./test.dat", "ab");
zero = malloc(sizeof(char)*512*8);
nonzero = malloc(sizeof(char)*512*8);
memset(zero, 0, sizeof(char)*512*8);
memset(nonzero, 1, sizeof(char)*512*8);
while (1) {
fwrite(zero, sizeof(char)*512*8, 1, fp);
fwrite(nonzero, sizeof(char)*512*8, 1, fp);
}
fclose(fp);
}
On Mon, Apr 24, 2017 at 9:55 PM, <jemmy858585@gmail.com> wrote:
> From: Lidong Chen <lidongchen@tencent.com>
>
> This patch optimizes the performance by coalescing the same write type.
> When the zero/non-zero state changes, perform the write for the accumulated
> cluster count.
>
> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> ---
> Thanks Fam Zheng and Stefan's advice.
> ---
> migration/block.c | 66 +++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/migration/block.c b/migration/block.c
> index 060087f..e9c5e21 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -40,6 +40,8 @@
>
> #define MAX_INFLIGHT_IO 512
>
> +#define MIN_CLUSTER_SIZE 8192
> +
> //#define DEBUG_BLK_MIGRATION
>
> #ifdef DEBUG_BLK_MIGRATION
> @@ -923,10 +925,11 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
> }
>
> ret = bdrv_get_info(blk_bs(blk), &bdi);
> - if (ret == 0 && bdi.cluster_size > 0 &&
> - bdi.cluster_size <= BLOCK_SIZE &&
> - BLOCK_SIZE % bdi.cluster_size == 0) {
> + if (ret == 0 && bdi.cluster_size > 0) {
> cluster_size = bdi.cluster_size;
> + while (cluster_size < MIN_CLUSTER_SIZE) {
> + cluster_size *= 2;
> + }
> } else {
> cluster_size = BLOCK_SIZE;
> }
> @@ -943,29 +946,58 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
> nr_sectors * BDRV_SECTOR_SIZE,
> BDRV_REQ_MAY_UNMAP);
> } else {
> - int i;
> - int64_t cur_addr;
> - uint8_t *cur_buf;
> + int64_t cur_addr = addr * BDRV_SECTOR_SIZE;
> + uint8_t *cur_buf = NULL;
> + int64_t last_addr = addr * BDRV_SECTOR_SIZE;
> + uint8_t *last_buf = NULL;
> + int64_t end_addr = addr * BDRV_SECTOR_SIZE + BLOCK_SIZE;
>
> buf = g_malloc(BLOCK_SIZE);
> qemu_get_buffer(f, buf, BLOCK_SIZE);
> - for (i = 0; i < BLOCK_SIZE / cluster_size; i++) {
> - cur_addr = addr * BDRV_SECTOR_SIZE + i * cluster_size;
> - cur_buf = buf + i * cluster_size;
> -
> - if ((!block_mig_state.zero_blocks ||
> - cluster_size < BLOCK_SIZE) &&
> - buffer_is_zero(cur_buf, cluster_size)) {
> - ret = blk_pwrite_zeroes(blk, cur_addr,
> - cluster_size,
> + cur_buf = buf;
> + last_buf = buf;
> +
> + while (last_addr < end_addr) {
> + int is_zero = 0;
> + int buf_size = MIN(end_addr - cur_addr, cluster_size);
> +
> + /* If the "zero blocks" migration capability is enabled
> + * and the buf_size == BLOCK_SIZE, then the source QEMU
> + * process has already scanned for zeroes. CPU is wasted
> + * scanning for zeroes in the destination QEMU process.
> + */
> + if (block_mig_state.zero_blocks &&
> + buf_size == BLOCK_SIZE) {
> + is_zero = 0;
> + } else {
> + is_zero = buffer_is_zero(cur_buf, buf_size);
> + }
> +
> + cur_addr += buf_size;
> + cur_buf += buf_size;
> + while (cur_addr < end_addr) {
> + buf_size = MIN(end_addr - cur_addr, cluster_size);
> + if (is_zero != buffer_is_zero(cur_buf, buf_size)) {
> + break;
> + }
> + cur_addr += buf_size;
> + cur_buf += buf_size;
> + }
> +
> + if (is_zero) {
> + ret = blk_pwrite_zeroes(blk, last_addr,
> + cur_addr - last_addr,
> BDRV_REQ_MAY_UNMAP);
> } else {
> - ret = blk_pwrite(blk, cur_addr, cur_buf,
> - cluster_size, 0);
> + ret = blk_pwrite(blk, last_addr, last_buf,
> + cur_addr - last_addr, 0);
> }
> if (ret < 0) {
> break;
> }
> +
> + last_addr = cur_addr;
> + last_buf = cur_buf;
> }
> g_free(buf);
> }
> --
> 1.8.3.1
>
Hi Stefan&Fam:
Could you help me review this patch?
Thanks a lot.
On Mon, Apr 24, 2017 at 10:03 PM, 858585 jemmy <jemmy858585@gmail.com> wrote:
> the reason of MIN_CLUSTER_SIZE is 8192 is base on the performance
> test result. the performance is only reduce obviously when cluster_size is
> less than 8192.
>
> I write this code, run in guest os. to create the worst condition.
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> int main()
> {
> char *zero;
> char *nonzero;
> FILE* fp = fopen("./test.dat", "ab");
>
> zero = malloc(sizeof(char)*512*8);
> nonzero = malloc(sizeof(char)*512*8);
>
> memset(zero, 0, sizeof(char)*512*8);
> memset(nonzero, 1, sizeof(char)*512*8);
>
> while (1) {
> fwrite(zero, sizeof(char)*512*8, 1, fp);
> fwrite(nonzero, sizeof(char)*512*8, 1, fp);
> }
> fclose(fp);
> }
>
>
> On Mon, Apr 24, 2017 at 9:55 PM, <jemmy858585@gmail.com> wrote:
>> From: Lidong Chen <lidongchen@tencent.com>
>>
>> This patch optimizes the performance by coalescing the same write type.
>> When the zero/non-zero state changes, perform the write for the accumulated
>> cluster count.
>>
>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>> ---
>> Thanks Fam Zheng and Stefan's advice.
>> ---
>> migration/block.c | 66 +++++++++++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 49 insertions(+), 17 deletions(-)
>>
>> diff --git a/migration/block.c b/migration/block.c
>> index 060087f..e9c5e21 100644
>> --- a/migration/block.c
>> +++ b/migration/block.c
>> @@ -40,6 +40,8 @@
>>
>> #define MAX_INFLIGHT_IO 512
>>
>> +#define MIN_CLUSTER_SIZE 8192
>> +
>> //#define DEBUG_BLK_MIGRATION
>>
>> #ifdef DEBUG_BLK_MIGRATION
>> @@ -923,10 +925,11 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
>> }
>>
>> ret = bdrv_get_info(blk_bs(blk), &bdi);
>> - if (ret == 0 && bdi.cluster_size > 0 &&
>> - bdi.cluster_size <= BLOCK_SIZE &&
>> - BLOCK_SIZE % bdi.cluster_size == 0) {
>> + if (ret == 0 && bdi.cluster_size > 0) {
>> cluster_size = bdi.cluster_size;
>> + while (cluster_size < MIN_CLUSTER_SIZE) {
>> + cluster_size *= 2;
>> + }
>> } else {
>> cluster_size = BLOCK_SIZE;
>> }
>> @@ -943,29 +946,58 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
>> nr_sectors * BDRV_SECTOR_SIZE,
>> BDRV_REQ_MAY_UNMAP);
>> } else {
>> - int i;
>> - int64_t cur_addr;
>> - uint8_t *cur_buf;
>> + int64_t cur_addr = addr * BDRV_SECTOR_SIZE;
>> + uint8_t *cur_buf = NULL;
>> + int64_t last_addr = addr * BDRV_SECTOR_SIZE;
>> + uint8_t *last_buf = NULL;
>> + int64_t end_addr = addr * BDRV_SECTOR_SIZE + BLOCK_SIZE;
>>
>> buf = g_malloc(BLOCK_SIZE);
>> qemu_get_buffer(f, buf, BLOCK_SIZE);
>> - for (i = 0; i < BLOCK_SIZE / cluster_size; i++) {
>> - cur_addr = addr * BDRV_SECTOR_SIZE + i * cluster_size;
>> - cur_buf = buf + i * cluster_size;
>> -
>> - if ((!block_mig_state.zero_blocks ||
>> - cluster_size < BLOCK_SIZE) &&
>> - buffer_is_zero(cur_buf, cluster_size)) {
>> - ret = blk_pwrite_zeroes(blk, cur_addr,
>> - cluster_size,
>> + cur_buf = buf;
>> + last_buf = buf;
>> +
>> + while (last_addr < end_addr) {
>> + int is_zero = 0;
>> + int buf_size = MIN(end_addr - cur_addr, cluster_size);
>> +
>> + /* If the "zero blocks" migration capability is enabled
>> + * and the buf_size == BLOCK_SIZE, then the source QEMU
>> + * process has already scanned for zeroes. CPU is wasted
>> + * scanning for zeroes in the destination QEMU process.
>> + */
>> + if (block_mig_state.zero_blocks &&
>> + buf_size == BLOCK_SIZE) {
>> + is_zero = 0;
>> + } else {
>> + is_zero = buffer_is_zero(cur_buf, buf_size);
>> + }
>> +
>> + cur_addr += buf_size;
>> + cur_buf += buf_size;
>> + while (cur_addr < end_addr) {
>> + buf_size = MIN(end_addr - cur_addr, cluster_size);
>> + if (is_zero != buffer_is_zero(cur_buf, buf_size)) {
>> + break;
>> + }
>> + cur_addr += buf_size;
>> + cur_buf += buf_size;
>> + }
>> +
>> + if (is_zero) {
>> + ret = blk_pwrite_zeroes(blk, last_addr,
>> + cur_addr - last_addr,
>> BDRV_REQ_MAY_UNMAP);
>> } else {
>> - ret = blk_pwrite(blk, cur_addr, cur_buf,
>> - cluster_size, 0);
>> + ret = blk_pwrite(blk, last_addr, last_buf,
>> + cur_addr - last_addr, 0);
>> }
>> if (ret < 0) {
>> break;
>> }
>> +
>> + last_addr = cur_addr;
>> + last_buf = cur_buf;
>> }
>> g_free(buf);
>> }
>> --
>> 1.8.3.1
>>
On Mon, Apr 24, 2017 at 09:55:40PM +0800, jemmy858585@gmail.com wrote:
> From: Lidong Chen <lidongchen@tencent.com>
>
> This patch optimizes the performance by coalescing the same write type.
> When the zero/non-zero state changes, perform the write for the accumulated
> cluster count.
>
> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> ---
> Thanks Fam Zheng and Stefan's advice.
> ---
> migration/block.c | 66 +++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/migration/block.c b/migration/block.c
> index 060087f..e9c5e21 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -40,6 +40,8 @@
>
> #define MAX_INFLIGHT_IO 512
>
> +#define MIN_CLUSTER_SIZE 8192
The loop accumulates adjacent zero/non-zero regions. What is the point
of this constant?
I guess you are trying to reduce the number of loop iterations when
cluster size is small (e.g. 512). Do you have performance results
showing that this really makes a difference?
It would be simpler to remove MIN_CLUSTER_SIZE. Unless there is
evidence showing it's necessary simpler code is better.
> @@ -943,29 +946,58 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
> nr_sectors * BDRV_SECTOR_SIZE,
> BDRV_REQ_MAY_UNMAP);
> } else {
> - int i;
> - int64_t cur_addr;
> - uint8_t *cur_buf;
> + int64_t cur_addr = addr * BDRV_SECTOR_SIZE;
> + uint8_t *cur_buf = NULL;
> + int64_t last_addr = addr * BDRV_SECTOR_SIZE;
> + uint8_t *last_buf = NULL;
> + int64_t end_addr = addr * BDRV_SECTOR_SIZE + BLOCK_SIZE;
>
> buf = g_malloc(BLOCK_SIZE);
> qemu_get_buffer(f, buf, BLOCK_SIZE);
> - for (i = 0; i < BLOCK_SIZE / cluster_size; i++) {
> - cur_addr = addr * BDRV_SECTOR_SIZE + i * cluster_size;
> - cur_buf = buf + i * cluster_size;
> -
> - if ((!block_mig_state.zero_blocks ||
> - cluster_size < BLOCK_SIZE) &&
> - buffer_is_zero(cur_buf, cluster_size)) {
> - ret = blk_pwrite_zeroes(blk, cur_addr,
> - cluster_size,
> + cur_buf = buf;
> + last_buf = buf;
> +
> + while (last_addr < end_addr) {
> + int is_zero = 0;
Please use bool for boolean values.
© 2016 - 2024 Red Hat, Inc.