Re: [Qemu-devel] [PATCH] CPU consumption optimization of 'qemu-img convert' using bdrv_is_allocated()

[Stefan Hajnoczi]

On Wed, Jun 08, 2011 at 06:50:25PM +0400, Dmitry Konishchev wrote:
> This patch optimizes 'qemu-img convert' operation for volumes which are
> almost fully unallocated. Here are the results of simple tests:

The optimization is to check allocation metadata instead of
unconditionally reading and then checking for all zeroes?

> diff --git a/qemu-img.c b/qemu-img.c
> index 4f162d1..9d905ed 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -38,6 +38,8 @@ typedef struct img_cmd_t {
>      int (*handler)(int argc, char **argv);
>  } img_cmd_t;
>  
> +static const int SECTOR_SIZE = 512;

Why introduce a new constant instead of using BDRV_SECTOR_SIZE?

> +
>  /* Default to cache=writeback as data integrity is not important for 
> qemu-tcg. */
>  #define BDRV_O_FLAGS BDRV_O_CACHE_WB
>  
> @@ -531,7 +533,7 @@ static int is_not_zero(const uint8_t *sector, int len)
>  }
>  
>  /*
> - * Returns true iff the first sector pointed to by 'buf' contains at least
> + * Returns true if the first sector pointed to by 'buf' contains at least

"iff" is not a typo.  It means "if and only if".

> @@ -912,55 +944,109 @@ static int img_convert(int argc, char **argv)
>                     are present in both the output's and input's base images 
> (no
>                     need to copy them). */
>                  if (out_baseimg) {
> -                    if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
> -                                           n, &n1)) {
> -                        sector_num += n1;
> +                    if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset, 
> n, &cur_n)) {
> +                        sector_num += cur_n;
>                          continue;
>                      }
> -                    /* The next 'n1' sectors are allocated in the input 
> image. Copy
> +                    /* The next 'cur_n' sectors are allocated in the input 
> image. Copy
>                         only those as they may be followed by unallocated 
> sectors. */
> -                    n = n1;
> +                    n = cur_n;
>                  }
> -            } else {
> -                n1 = n;
>              }
>  
> -            ret = bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n);
> -            if (ret < 0) {
> -                error_report("error while reading");
> -                goto out;
> -            }
> -            /* NOTE: at the same time we convert, we do not write zero
> -               sectors to have a chance to compress the image. Ideally, we
> -               should add a specific call to have the info to go faster */
> -            buf1 = buf;
> -            while (n > 0) {
> -                /* If the output image is being created as a copy on write 
> image,
> -                   copy all sectors even the ones containing only NUL bytes,
> -                   because they may differ from the sectors in the base 
> image.
> -
> -                   If the output is to a host device, we also write out
> -                   sectors that are entirely 0, since whatever data was
> -                   already there is garbage, not 0s. */
> -                if (!has_zero_init || out_baseimg ||
> -                    is_allocated_sectors(buf1, n, &n1)) {
> -                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
> -                    if (ret < 0) {
> -                        error_report("error while writing");
> -                        goto out;
> +            /* If the output image is being created as a copy on write image,
> +               copy all sectors even the ones containing only zero bytes,
> +               because they may differ from the sectors in the base image.
> +
> +               If the output is to a host device, we also write out
> +               sectors that are entirely 0, since whatever data was
> +               already there is garbage, not 0s. */
> +            if (!has_zero_init || out_baseimg) {
> +                ret = bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n);
> +                if (ret < 0) {
> +                    error_report("error while reading");
> +                    goto out;
> +                }
> +
> +                ret = bdrv_write(out_bs, sector_num, buf, n);
> +                if (ret < 0) {
> +                    error_report("error while writing");
> +                    goto out;
> +                }
> +
> +                sector_num += n;
> +            } else {
> +                /* Look for the sectors in the image and if they are not
> +                   allocated - sequentially in all its backing images.
> +
> +                   Write only non-zero bytes to the output image. */

I think the recursive is_allocated() needs its own function.  This
function is already long/complex enough :).

Stefan