Re: [Qemu-devel] [PATCH] migration/block: optimize the performance by coalescing the same write type

[Stefan Hajnoczi]

On Mon, Apr 24, 2017 at 09:55:40PM +0800, address@hidden wrote:
> From: Lidong Chen <address@hidden>
> 
> 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 <address@hidden>
> ---
>     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.

signature.asc
Description: PGP signature