[Qemu-devel] [PATCH v4 0/3] qemu-img: rebase: Improve/optimize rebase operation

[Qemu-devel] [PATCH v4 0/3] qemu-img: rebase: Improve/optimize rebase operation

[Sam Eiderman]

This patch series aims to improve the speed of qemu-img rebase.

  1. Mainly by removing unnecessary reads when rebasing on the same
     chain.
  2. But also by minimizing the number of bdrv_open calls rebase
     requires.

v2:

* Added missing g_free in
  "qemu-img: rebase: Reuse in-chain BlockDriverState"

v3:

* Rebased on top of "Allow rebase with no input base" series

v4:

* Using backing_bs(bs) when a prefix was detected since bs was
  already checked for allocation.

Sam Eiderman (3):
  qemu-img: rebase: Reuse parent BlockDriverState
  qemu-img: rebase: Reduce reads on in-chain rebase
  qemu-img: rebase: Reuse in-chain BlockDriverState

 qemu-img.c | 85 ++++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 55 insertions(+), 30 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH v4 1/3] qemu-img: rebase: Reuse parent BlockDriverState

[Sam Eiderman]

In safe mode we open the entire chain, including the parent backing
file of the rebased file.
Do not open a new BlockBackend for the parent backing file, which
saves opening the rest of the chain twice, which for long chains
saves many "pricy" bdrv_open() calls.

Permissions for blk_new() were copied from blk_new_open() when
flags = 0.

Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
Reviewed-by: Eyal Moscovici <eyal.moscovici@oracle.com>
Signed-off-by: Sagi Amit <sagi.amit@oracle.com>
Co-developed-by: Sagi Amit <sagi.amit@oracle.com>
Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com>
---
 qemu-img.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 28fba1e7a7..9bd0bb1e47 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3309,29 +3309,18 @@ static int img_rebase(int argc, char **argv)
 
     /* For safe rebasing we need to compare old and new backing file */
     if (!unsafe) {
-        char backing_name[PATH_MAX];
         QDict *options = NULL;
+        BlockDriverState *base_bs = backing_bs(bs);
 
-        if (bs->backing) {
-            if (bs->backing_format[0] != '\0') {
-                options = qdict_new();
-                qdict_put_str(options, "driver", bs->backing_format);
-            }
-
-            if (force_share) {
-                if (!options) {
-                    options = qdict_new();
-                }
-                qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
-            }
-            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-            blk_old_backing = blk_new_open(backing_name, NULL,
-                                           options, src_flags, &local_err);
-            if (!blk_old_backing) {
+        if (base_bs) {
+            blk_old_backing = blk_new(BLK_PERM_CONSISTENT_READ,
+                                      BLK_PERM_ALL);
+            ret = blk_insert_bs(blk_old_backing, base_bs,
+                                &local_err);
+            if (ret < 0) {
                 error_reportf_err(local_err,
-                                  "Could not open old backing file '%s': ",
-                                  backing_name);
-                ret = -1;
+                                  "Could not reuse old backing file '%s': ",
+                                  base_bs->filename);
                 goto out;
             }
         } else {
-- 
2.13.3

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

[Sam Eiderman]

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 9bd0bb1e47..e6fd8e1a98 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3164,7 +3164,7 @@ static int img_rebase(int argc, char **argv)
     BlockBackend *blk = NULL, *blk_old_backing = NULL, *blk_new_backing = NULL;
     uint8_t *buf_old = NULL;
     uint8_t *buf_new = NULL;
-    BlockDriverState *bs = NULL;
+    BlockDriverState *bs = NULL, *prefix_chain_bs = NULL;
     char *filename;
     const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
     int c, flags, src_flags, ret;
@@ -3353,6 +3353,12 @@ static int img_rebase(int argc, char **argv)
                 goto out;
             }
 
+            /*
+             * Find out whether we rebase an image on top of a previous image
+             * in its chain.
+             */
+            prefix_chain_bs = bdrv_find_backing_image(bs, out_real_path);
+
             blk_new_backing = blk_new_open(out_real_path, NULL,
                                            options, src_flags, &local_err);
             g_free(out_real_path);
@@ -3437,6 +3443,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);
+                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.
-- 
2.13.3

[Qemu-devel] [PATCH v4 3/3] qemu-img: rebase: Reuse in-chain BlockDriverState

[Sam Eiderman]

If a chain was detected, don't open a new BlockBackend from the target
backing file which will create a new BlockDriverState. Instead, create
an empty BlockBackend and attach the already open BlockDriverState.

Permissions for blk_new() were copied from blk_new_open() when
flags = 0.

Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
Reviewed-by: Eyal Moscovici <eyal.moscovici@oracle.com>
Signed-off-by: Sagi Amit <sagi.amit@oracle.com>
Co-developed-by: Sagi Amit <sagi.amit@oracle.com>
Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com>
---
 qemu-img.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e6fd8e1a98..b3a1c76fb6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3358,16 +3358,29 @@ static int img_rebase(int argc, char **argv)
              * in its chain.
              */
             prefix_chain_bs = bdrv_find_backing_image(bs, out_real_path);
-
-            blk_new_backing = blk_new_open(out_real_path, NULL,
-                                           options, src_flags, &local_err);
-            g_free(out_real_path);
-            if (!blk_new_backing) {
-                error_reportf_err(local_err,
-                                  "Could not open new backing file '%s': ",
-                                  out_baseimg);
-                ret = -1;
-                goto out;
+            if (prefix_chain_bs) {
+                g_free(out_real_path);
+                blk_new_backing = blk_new(BLK_PERM_CONSISTENT_READ,
+                                          BLK_PERM_ALL);
+                ret = blk_insert_bs(blk_new_backing, prefix_chain_bs,
+                                    &local_err);
+                if (ret < 0) {
+                    error_reportf_err(local_err,
+                                      "Could not reuse backing file '%s': ",
+                                      out_baseimg);
+                    goto out;
+                }
+            } else {
+                blk_new_backing = blk_new_open(out_real_path, NULL,
+                                               options, src_flags, &local_err);
+                g_free(out_real_path);
+                if (!blk_new_backing) {
+                    error_reportf_err(local_err,
+                                      "Could not open new backing file '%s': ",
+                                      out_baseimg);
+                    ret = -1;
+                    goto out;
+                }
             }
         }
     }
-- 
2.13.3

Re: [Qemu-devel] [PATCH v4 1/3] qemu-img: rebase: Reuse parent BlockDriverState

[Max Reitz]

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

On 23.05.19 17:44, Sam Eiderman wrote:
> In safe mode we open the entire chain, including the parent backing
> file of the rebased file.
> Do not open a new BlockBackend for the parent backing file, which
> saves opening the rest of the chain twice, which for long chains
> saves many "pricy" bdrv_open() calls.
> 
> Permissions for blk_new() were copied from blk_new_open() when
> flags = 0.
> 
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> Reviewed-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> Signed-off-by: Sagi Amit <sagi.amit@oracle.com>
> Co-developed-by: Sagi Amit <sagi.amit@oracle.com>
> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com>
> ---
>  qemu-img.c | 29 +++++++++--------------------
>  1 file changed, 9 insertions(+), 20 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

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

[Max Reitz]

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

On 23.05.19 17:44, 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 9bd0bb1e47..e6fd8e1a98 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -3437,6 +3443,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);

I think you forgot to update the patch...?

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 --]

Re: [Qemu-devel] [PATCH v4 3/3] qemu-img: rebase: Reuse in-chain BlockDriverState

[Max Reitz]

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

On 23.05.19 17:44, Sam Eiderman wrote:
> If a chain was detected, don't open a new BlockBackend from the target
> backing file which will create a new BlockDriverState. Instead, create
> an empty BlockBackend and attach the already open BlockDriverState.
> 
> Permissions for blk_new() were copied from blk_new_open() when
> flags = 0.
> 
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> Reviewed-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> Signed-off-by: Sagi Amit <sagi.amit@oracle.com>
> Co-developed-by: Sagi Amit <sagi.amit@oracle.com>
> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com>
> ---
>  qemu-img.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

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

[Sam Eiderman]

Yes, my bad, resending as v5.

Sam

> On 23 May 2019, at 19:14, Max Reitz <mreitz@redhat.com> wrote:
> 
> On 23.05.19 17:44, 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 9bd0bb1e47..e6fd8e1a98 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
> 
> [...]
> 
>> @@ -3437,6 +3443,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);
> 
> I think you forgot to update the patch...?
> 
> 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.