Re: [Qemu-devel] [PATCH] qemu-img: Speed up comparing empty/zero images

[Paolo Bonzini <pbonzini@redhat.com>]

On 13/01/2016 09:37, Fam Zheng wrote:
> Two empty raw files are always compared by actually reading data even if
> there is no data, because BDRV_BLOCK_ZERO is considered "allocated" in
> bdrv_is_allocated_above().  That is inefficient.
> 
> Use bdrv_get_block_status_above() for more information, and skip the
> consecutive zero sectors.
> 
> This brings a huge speed up in comparing sparse/empty raw images:
> 
>     $ qemu-img create a 1G
> 
>     $ time ~/build/master/bin/qemu-img compare a a
>     Images are identical.
> 
>     real    0m6.583s
>     user    0m0.191s
>     sys     0m6.367s
> 
>     $ time qemu-img compare a a
>     Images are identical.
> 
>     real    0m0.033s
>     user    0m0.003s
>     sys     0m0.031s
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-img.c | 45 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 3d48b4f..82f704f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1074,28 +1074,50 @@ static int img_compare(int argc, char **argv)
>      }
>  
>      for (;;) {
> +        int64_t status1, status2;
>          nb_sectors = sectors_to_process(total_sectors, sector_num);
>          if (nb_sectors <= 0) {
>              break;
>          }
> -        allocated1 = bdrv_is_allocated_above(bs1, NULL, sector_num, nb_sectors,
> -                                             &pnum1);
> -        if (allocated1 < 0) {
> +        status1 = bdrv_get_block_status_above(bs1, NULL, sector_num,
> +                                              total_sectors1 - sector_num,
> +                                              &pnum1);
> +        if (status1 < 0) {
>              ret = 3;
>              error_report("Sector allocation test failed for %s", filename1);
>              goto out;
>          }
> +        allocated1 = status1 & BDRV_BLOCK_ALLOCATED;
>  
> -        allocated2 = bdrv_is_allocated_above(bs2, NULL, sector_num, nb_sectors,
> -                                             &pnum2);
> -        if (allocated2 < 0) {
> +        status2 = bdrv_get_block_status_above(bs2, NULL, sector_num,
> +                                              total_sectors2 - sector_num,
> +                                              &pnum2);
> +        if (status2 < 0) {
>              ret = 3;
>              error_report("Sector allocation test failed for %s", filename2);
>              goto out;
>          }
> -        nb_sectors = MIN(pnum1, pnum2);
> +        allocated2 = status2 & BDRV_BLOCK_ALLOCATED;
> +        if (pnum1) {
> +            nb_sectors = MIN(nb_sectors, pnum1);
> +        }
> +        if (pnum2) {
> +            nb_sectors = MIN(nb_sectors, pnum2);
> +        }
>  
> -        if (allocated1 == allocated2) {
> +        if (strict) {
> +            if ((status1 & ~BDRV_BLOCK_OFFSET_MASK) !=
> +                (status2 & ~BDRV_BLOCK_OFFSET_MASK)) {

This is not exactly the same definition as before, but if that's okay:

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> +                ret = 1;
> +                qprintf(quiet, "Strict mode: Offset %" PRId64
> +                        " block status mismatch!\n",
> +                        sectors_to_bytes(sector_num));
> +                goto out;
> +            }
> +        }
> +        if ((status1 & BDRV_BLOCK_ZERO) && (status2 & BDRV_BLOCK_ZERO)) {
> +            nb_sectors = MIN(pnum1, pnum2);
> +        } else if (allocated1 == allocated2) {
>              if (allocated1) {
>                  ret = blk_read(blk1, sector_num, buf1, nb_sectors);
>                  if (ret < 0) {
> @@ -1123,13 +1145,6 @@ static int img_compare(int argc, char **argv)
>                  }
>              }
>          } else {
> -            if (strict) {
> -                ret = 1;
> -                qprintf(quiet, "Strict mode: Offset %" PRId64
> -                        " allocation mismatch!\n",
> -                        sectors_to_bytes(sector_num));
> -                goto out;
> -            }
>  
>              if (allocated1) {
>                  ret = check_empty_sectors(blk1, sector_num, nb_sectors,
>