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

[Sam Eiderman]

On 23 May 2019, at 17:01, Max Reitz <address@hidden> wrote:

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 <address@hidden>
Signed-off-by: Sam Eiderman <address@hidden>
Signed-off-by: Eyal Moscovici <address@hidden>
---
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

I don’t think that’s true:

Examine the case where we have the following chain:

A <- B <- C

When we rebase C directly over A: qemu-img rebase -b A C

We must check for every offset (sector): bdrv_is_allocated_above(C, A, offset, n, &n);

If a sector from C is allocated above A - it may have been changed - so we need to do a read from A and a read from C and compare.

If the sector is not allocated above, it was not changed - we don’t need to read from A or C.

Sam

+                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.