Re: [Qemu-devel] [PATCH v2 2/3] qemu-img: rebase: Reduce reads on in-chain rebase

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]

On 02.05.19 15:58, Sam Eiderman wrote:
> In the following case:
> 
> (base) A <- B <- C (tip)
> 
> when running:
> 
>     qemu-img rebase -b A C
> 
> QEMU would read all sectors not allocated in the file being rebased (C)
> and compare them to the new base image (A), regardless of whether they
> were changed or even allocated anywhere along the chain between the new
> base and the top image (B). This causes many unneeded reads when
> rebasing an image which represents a small diff of a large disk, as it
> would read most of the disk's sectors.
> 
> Instead, use bdrv_is_allocated_above() to reduce the number of
> unnecessary reads.
> 
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---
>  qemu-img.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index d9b609b3f0..7f20858cb9 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -3422,6 +3428,23 @@ static int img_rebase(int argc, char **argv)
>                  continue;
>              }
>  
> +            if (prefix_chain_bs) {
> +                /*
> +                 * If cluster wasn't changed since prefix_chain, we don't need
> +                 * to take action
> +                 */
> +                ret = bdrv_is_allocated_above(bs, prefix_chain_bs,
> +                                              offset, n, &n);

This will always return true because it definitely is allocated in @bs,
or we wouldn’t be here.  (We just checked that with
bdrv_is_allocated().)  I think @top should be backing_bs(bs).

Max

> +                if (ret < 0) {
> +                    error_report("error while reading image metadata: %s",
> +                                 strerror(-ret));
> +                    goto out;
> +                }
> +                if (!ret) {
> +                    continue;
> +                }
> +            }
> +
>              /*
>               * Read old and new backing file and take into consideration that
>               * backing files may be smaller than the COW image.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]