[Qemu-devel] [PATCH v2 0/3] qemu-img: Allow rebase with no input base

[Qemu-devel] [PATCH v2 0/3] qemu-img: Allow rebase with no input base

[Max Reitz]

This series allows using qemu-img rebase (without -u) on images that do
not have a backing file.  Right now, this fails with the rather cryptic
error message:

$ qemu-img rebase -b base.qcow2 foo.qcow2
qemu-img: Could not open old backing file '': The 'file' block driver requires a file name

Yeah, well, OK.

With how rebase currently works, this would lead to the overlay being
filled with zeroes, however.  This is where patch 2 comes in and instead
makes rebase use blk_pwrite_zeroes() whenever it handles an area past
the input’s backing file’s EOF.

(Note that additionally we could try to punch holes in the overlay
whenever it matches the new backing file, but that’s something I’ll put
off for later.  (We don’t even have a reliable method for punching holes
into an overlay yet, although I would like to have such because it could
make active commit more efficient.))


And patch 3 adds the usual test.


v2:
- Patch 1: Added example to the commit message, and made a note that
           this is about rebase without -u [Eric]


git backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/3:[----] [--] 'qemu-img: Allow rebase with no input base'
002/3:[----] [--] 'qemu-img: Use zero writes after source backing EOF'
003/3:[----] [--] 'iotests: Add test for rebase without input base'


Max Reitz (3):
  qemu-img: Allow rebase with no input base
  qemu-img: Use zero writes after source backing EOF
  iotests: Add test for rebase without input base

 qemu-img.c                 | 72 +++++++++++++++++++++++---------------
 tests/qemu-iotests/024     | 70 ++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/024.out | 37 ++++++++++++++++++++
 3 files changed, 150 insertions(+), 29 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/3] qemu-img: Allow rebase with no input base

[Max Reitz]

Currently, without -u, you cannot add a backing file to an image when it
currently has none:

$ qemu-img rebase -b base.qcow2 foo.qcow2
qemu-img: Could not open old backing file '': The 'file' block driver
requires a file name

It is really simple to allow this, though (effectively by setting
old_backing_size to 0), so this patch does just that.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c | 61 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e6ad5978e0..e22a4fda17 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3311,26 +3311,30 @@ static int img_rebase(int argc, char **argv)
         char backing_name[PATH_MAX];
         QDict *options = NULL;
 
-        if (bs->backing_format[0] != '\0') {
-            options = qdict_new();
-            qdict_put_str(options, "driver", bs->backing_format);
-        }
-
-        if (force_share) {
-            if (!options) {
+        if (bs->backing) {
+            if (bs->backing_format[0] != '\0') {
                 options = qdict_new();
+                qdict_put_str(options, "driver", bs->backing_format);
             }
-            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) {
-            error_reportf_err(local_err,
-                              "Could not open old backing file '%s': ",
-                              backing_name);
-            ret = -1;
-            goto out;
+
+            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) {
+                error_reportf_err(local_err,
+                                  "Could not open old backing file '%s': ",
+                                  backing_name);
+                ret = -1;
+                goto out;
+            }
+        } else {
+            blk_old_backing = NULL;
         }
 
         if (out_baseimg[0]) {
@@ -3383,7 +3387,7 @@ static int img_rebase(int argc, char **argv)
      */
     if (!unsafe) {
         int64_t size;
-        int64_t old_backing_size;
+        int64_t old_backing_size = 0;
         int64_t new_backing_size = 0;
         uint64_t offset;
         int64_t n;
@@ -3399,15 +3403,18 @@ static int img_rebase(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        old_backing_size = blk_getlength(blk_old_backing);
-        if (old_backing_size < 0) {
-            char backing_name[PATH_MAX];
+        if (blk_old_backing) {
+            old_backing_size = blk_getlength(blk_old_backing);
+            if (old_backing_size < 0) {
+                char backing_name[PATH_MAX];
 
-            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-            error_report("Could not get size of '%s': %s",
-                         backing_name, strerror(-old_backing_size));
-            ret = -1;
-            goto out;
+                bdrv_get_backing_filename(bs, backing_name,
+                                          sizeof(backing_name));
+                error_report("Could not get size of '%s': %s",
+                             backing_name, strerror(-old_backing_size));
+                ret = -1;
+                goto out;
+            }
         }
         if (blk_new_backing) {
             new_backing_size = blk_getlength(blk_new_backing);
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/3] qemu-img: Use zero writes after source backing EOF

[Max Reitz]

Past the end of the source backing file, we memset() buf_old to zero, so
it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
then.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e22a4fda17..cfad922816 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3431,6 +3431,8 @@ static int img_rebase(int argc, char **argv)
         }
 
         for (offset = 0; offset < size; offset += n) {
+            bool buf_old_is_zero = false;
+
             /* How many bytes can we handle with the next read? */
             n = MIN(IO_BUF_SIZE, size - offset);
 
@@ -3451,6 +3453,7 @@ static int img_rebase(int argc, char **argv)
              */
             if (offset >= old_backing_size) {
                 memset(buf_old, 0, n);
+                buf_old_is_zero = true;
             } else {
                 if (offset + n > old_backing_size) {
                     n = old_backing_size - offset;
@@ -3486,8 +3489,12 @@ static int img_rebase(int argc, char **argv)
                 if (compare_buffers(buf_old + written, buf_new + written,
                                     n - written, &pnum))
                 {
-                    ret = blk_pwrite(blk, offset + written,
-                                     buf_old + written, pnum, 0);
+                    if (buf_old_is_zero) {
+                        ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
+                    } else {
+                        ret = blk_pwrite(blk, offset + written,
+                                         buf_old + written, pnum, 0);
+                    }
                     if (ret < 0) {
                         error_report("Error while writing to COW image: %s",
                             strerror(-ret));
-- 
2.20.1

[Qemu-devel] [PATCH v2 3/3] iotests: Add test for rebase without input base

[Max Reitz]

This patch adds a test for rebasing an image that currently does not
have a backing file.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/024     | 70 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/024.out | 37 ++++++++++++++++++++
 2 files changed, 107 insertions(+)

diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
index 23298c6f59..baf8ec9f28 100755
--- a/tests/qemu-iotests/024
+++ b/tests/qemu-iotests/024
@@ -198,6 +198,76 @@ echo
 # $BASE_OLD and $BASE_NEW)
 $QEMU_IMG map "$OVERLAY" | _filter_qemu_img_map
 
+echo
+echo "=== Test rebase without input base ==="
+echo
+
+# Cluster allocations to be tested:
+#
+# Backing (new) 11 -- 11 -- 11 --
+# COW image     22 22 11 11 -- --
+#
+# Expected result:
+#
+# COW image     22 22 11 11 00 --
+#
+# (Cluster 2 might be "--" after the rebase, too, but rebase just
+#  compares the new backing file to the old one and disregards the
+#  overlay.  Therefore, it will never discard overlay clusters.)
+
+_make_test_img $((6 * CLUSTER_SIZE))
+TEST_IMG="$TEST_IMG_SAVE.base_new" _make_test_img $((6 * CLUSTER_SIZE))
+
+echo
+
+$QEMU_IO "$TEST_IMG" \
+    -c "write -P 0x22 $((0 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    -c "write -P 0x11 $((2 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    | _filter_qemu_io
+
+$QEMU_IO "$TEST_IMG.base_new" \
+    -c "write -P 0x11 $((0 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+    -c "write -P 0x11 $((2 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+    -c "write -P 0x11 $((4 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+    | _filter_qemu_io
+
+echo
+
+# This should be a no-op
+$QEMU_IMG rebase -b "" "$TEST_IMG"
+
+# Verify the data is correct
+$QEMU_IO "$TEST_IMG" \
+    -c "read -P 0x22 $((0 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    -c "read -P 0x11 $((2 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    -c "read -P 0x00 $((4 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    | _filter_qemu_io
+
+echo
+
+# Verify the allocation status (first four cluster should be allocated
+# in TEST_IMG, clusters 4 and 5 should be unallocated (marked as zero
+# clusters here because there is no backing file))
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+echo
+
+$QEMU_IMG rebase -b "$TEST_IMG.base_new" "$TEST_IMG"
+
+# Verify the data is correct
+$QEMU_IO "$TEST_IMG" \
+    -c "read -P 0x22 $((0 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    -c "read -P 0x11 $((2 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    -c "read -P 0x00 $((4 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+    | _filter_qemu_io
+
+echo
+
+# Verify the allocation status (first four cluster should be allocated
+# in TEST_IMG, cluster 4 should be zero, and cluster 5 should be
+# unallocated (signified by '"depth": 1'))
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/024.out b/tests/qemu-iotests/024.out
index 024dc786b3..830abe013d 100644
--- a/tests/qemu-iotests/024.out
+++ b/tests/qemu-iotests/024.out
@@ -171,4 +171,41 @@ read 65536/65536 bytes at offset 196608
 Offset          Length          File
 0               0x30000         TEST_DIR/subdir/t.IMGFMT
 0x30000         0x10000         TEST_DIR/subdir/t.IMGFMT.base_new
+
+=== Test rebase without input base ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=393216
+Formatting 'TEST_DIR/t.IMGFMT.base_new', fmt=IMGFMT size=393216
+
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 131072/131072 bytes at offset 131072
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 131072
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 131072
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 262144
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+[{ "start": 0, "length": 262144, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 262144, "length": 131072, "depth": 0, "zero": true, "data": false}]
+
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 131072
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 262144
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+[{ "start": 0, "length": 262144, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 262144, "length": 65536, "depth": 0, "zero": true, "data": false},
+{ "start": 327680, "length": 65536, "depth": 1, "zero": true, "data": false}]
 *** done
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 3/3] iotests: Add test for rebase without input base

[Kevin Wolf]

Am 08.05.2019 um 16:01 hat Max Reitz geschrieben:
> This patch adds a test for rebasing an image that currently does not
> have a backing file.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/024     | 70 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/024.out | 37 ++++++++++++++++++++
>  2 files changed, 107 insertions(+)
> 
> diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
> index 23298c6f59..baf8ec9f28 100755
> --- a/tests/qemu-iotests/024
> +++ b/tests/qemu-iotests/024
> @@ -198,6 +198,76 @@ echo
>  # $BASE_OLD and $BASE_NEW)
>  $QEMU_IMG map "$OVERLAY" | _filter_qemu_img_map
>  
> +echo
> +echo "=== Test rebase without input base ==="
> +echo
> +
> +# Cluster allocations to be tested:
> +#
> +# Backing (new) 11 -- 11 -- 11 --
> +# COW image     22 22 11 11 -- --
> +#
> +# Expected result:
> +#
> +# COW image     22 22 11 11 00 --
> +#
> +# (Cluster 2 might be "--" after the rebase, too, but rebase just
> +#  compares the new backing file to the old one and disregards the
> +#  overlay.  Therefore, it will never discard overlay clusters.)

Is there a reason not to do that, though? I haven't tried it, but
shouldn't it be as easy as reading from blk instead of blk_old_backing?

Matter for another patch, though.

Kevin

Re: [Qemu-devel] [PATCH v2 3/3] iotests: Add test for rebase without input base

[Max Reitz]

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

On 08.05.19 16:01, Max Reitz wrote:
> This patch adds a test for rebasing an image that currently does not
> have a backing file.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/024     | 70 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/024.out | 37 ++++++++++++++++++++
>  2 files changed, 107 insertions(+)

This test requires zero clusters, so it doesn’t work with qed and qcow2
v2.  I’ll put it into a separate file.

Max


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

Re: [Qemu-devel] [PATCH v2 0/3] qemu-img: Allow rebase with no input base

[Kevin Wolf]

Am 08.05.2019 um 16:01 hat Max Reitz geschrieben:
> This series allows using qemu-img rebase (without -u) on images that do
> not have a backing file.  Right now, this fails with the rather cryptic
> error message:
> 
> $ qemu-img rebase -b base.qcow2 foo.qcow2
> qemu-img: Could not open old backing file '': The 'file' block driver requires a file name
> 
> Yeah, well, OK.
> 
> With how rebase currently works, this would lead to the overlay being
> filled with zeroes, however.  This is where patch 2 comes in and instead
> makes rebase use blk_pwrite_zeroes() whenever it handles an area past
> the input’s backing file’s EOF.
> 
> (Note that additionally we could try to punch holes in the overlay
> whenever it matches the new backing file, but that’s something I’ll put
> off for later.  (We don’t even have a reliable method for punching holes
> into an overlay yet, although I would like to have such because it could
> make active commit more efficient.))
> 
> And patch 3 adds the usual test.

Thanks, applied to the block branch.

Kevin