[PATCH 0/6] qemu-img: rebase: add compression support

[PATCH 0/6] qemu-img: rebase: add compression support

[Andrey Drobyshev via]

This series is adding [-c | --compress] option to "qemu-img rebase"
command, which might prove useful for saving some disk space when, for
instance, manipulating chains of backup images.  Along the way I had to
make a couple of minor improvements.

The first 2 patches are a bug fix + corresponding test case.
Patch 3 merely fixes wrong args used in allocation.
Patch 4 makes write requests during rebase operation cluster_size-aligned,
which seems to be beneficial for both non-compressed and compressed mode.
The last 2 patches are the actual feature implementation + tests.

Andrey Drobyshev (6):
  qemu-img: rebase: stop when reaching EOF of old backing file
  qemu-iotests: 024: add rebasing test case for overlay_size >
    backing_size
  qemu-img: rebase: use backing files' BlockBackend for buffer alignment
  qemu-img: rebase: avoid unnecessary COW operations
  qemu-img: add compression option to rebase subcommand
  iotests: add test 314 for "qemu-img rebase" with compression

 docs/tools/qemu-img.rst    |   6 +-
 qemu-img-cmds.hx           |   4 +-
 qemu-img.c                 | 106 ++++++++++++++++++------
 tests/qemu-iotests/024     |  57 +++++++++++++
 tests/qemu-iotests/024.out |  30 +++++++
 tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/314.out |  75 +++++++++++++++++
 7 files changed, 415 insertions(+), 28 deletions(-)
 create mode 100755 tests/qemu-iotests/314
 create mode 100644 tests/qemu-iotests/314.out

-- 
2.31.1

[PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file

[Andrey Drobyshev via]

In case when we're rebasing within one backing chain, and when target image
is larger than old backing file, bdrv_is_allocated_above() ends up setting
*pnum = 0.  As a result, target offset isn't getting incremented, and we
get stuck in an infinite for loop.  Let's detect this case and proceed
further down the loop body, as the offsets beyond the old backing size need
to be explicitly zeroed.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qemu-img.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 27f48051b0..78433f3746 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3801,6 +3801,8 @@ static int img_rebase(int argc, char **argv)
             }
 
             if (prefix_chain_bs) {
+                uint64_t bytes = n;
+
                 /*
                  * If cluster wasn't changed since prefix_chain, we don't need
                  * to take action
@@ -3813,9 +3815,18 @@ static int img_rebase(int argc, char **argv)
                                  strerror(-ret));
                     goto out;
                 }
-                if (!ret) {
+                if (!ret && n) {
                     continue;
                 }
+                if (!n) {
+                    /*
+                     * If we've reached EOF of the old backing, it means that
+                     * offsets beyond the old backing size were read as zeroes.
+                     * Now we will need to explicitly zero the cluster in
+                     * order to preserve that state after the rebase.
+                     */
+                    n = bytes;
+                }
             }
 
             /*
-- 
2.31.1

[PATCH 2/6] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size

[Andrey Drobyshev via]

Before previous commit, rebase was getting infitely stuck in case of
rebasing within the same backing chain and when overlay_size > backing_size.
Let's add this case to the rebasing test 024 to make sure it doesn't
break again.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 tests/qemu-iotests/024     | 57 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/024.out | 30 ++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
index 25a564a150..98a7c8fd65 100755
--- a/tests/qemu-iotests/024
+++ b/tests/qemu-iotests/024
@@ -199,6 +199,63 @@ echo
 # $BASE_OLD and $BASE_NEW)
 $QEMU_IMG map "$OVERLAY" | _filter_qemu_img_map
 
+# Check that rebase within the chain is working when
+# overlay_size > old_backing_size
+#
+# base_new <-- base_old <-- overlay
+#
+# Backing (new): 11 11 11 11 11
+# Backing (old): 22 22 22 22
+# Overlay:       -- -- -- -- --
+#
+# As a result, overlay should contain data identical to base_old, with the
+# last cluster remaining unallocated.
+
+echo
+echo "=== Test rebase within one backing chain ==="
+echo
+
+echo "Creating backing chain"
+echo
+
+TEST_IMG=$BASE_NEW _make_test_img $(( CLUSTER_SIZE * 5 ))
+TEST_IMG=$BASE_OLD _make_test_img -b "$BASE_NEW" -F $IMGFMT \
+    $(( CLUSTER_SIZE * 4 ))
+TEST_IMG=$OVERLAY _make_test_img -b "$BASE_OLD" -F $IMGFMT \
+    $(( CLUSTER_SIZE * 5 ))
+
+echo
+echo "Fill backing files with data"
+echo
+
+$QEMU_IO "$BASE_NEW" -c "write -P 0x11 0 $(( CLUSTER_SIZE * 5 ))" \
+    | _filter_qemu_io
+$QEMU_IO "$BASE_OLD" -c "write -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
+    | _filter_qemu_io
+
+echo
+echo "Check the last cluster is zeroed in overlay before the rebase"
+echo
+$QEMU_IO "$OVERLAY" -c "read -P 0x00 $(( CLUSTER_SIZE * 4 )) $CLUSTER_SIZE" \
+    | _filter_qemu_io
+
+echo
+echo "Rebase onto another image in the same chain"
+echo
+
+$QEMU_IMG rebase -b "$BASE_NEW" -F $IMGFMT "$OVERLAY"
+
+echo "Verify that data is read the same before and after rebase"
+echo
+
+# Verify the first 4 clusters are still read the same as in the old base
+$QEMU_IO "$OVERLAY" -c "read -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
+    | _filter_qemu_io
+# Verify the last cluster still reads as zeroes
+$QEMU_IO "$OVERLAY" -c "read -P 0x00 $(( CLUSTER_SIZE * 4 )) $CLUSTER_SIZE" \
+    | _filter_qemu_io
+
+echo
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/024.out b/tests/qemu-iotests/024.out
index 973a5a3711..245fe8b1d1 100644
--- a/tests/qemu-iotests/024.out
+++ b/tests/qemu-iotests/024.out
@@ -171,4 +171,34 @@ 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 within one backing chain ===
+
+Creating backing chain
+
+Formatting 'TEST_DIR/subdir/t.IMGFMT.base_new', fmt=IMGFMT size=327680
+Formatting 'TEST_DIR/subdir/t.IMGFMT.base_old', fmt=IMGFMT size=262144 backing_file=TEST_DIR/subdir/t.IMGFMT.base_new backing_fmt=IMGFMT
+Formatting 'TEST_DIR/subdir/t.IMGFMT', fmt=IMGFMT size=327680 backing_file=TEST_DIR/subdir/t.IMGFMT.base_old backing_fmt=IMGFMT
+
+Fill backing files with data
+
+wrote 327680/327680 bytes at offset 0
+320 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Check the last cluster is zeroed in overlay before the rebase
+
+read 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Rebase onto another image in the same chain
+
+Verify that data is read the same before and after rebase
+
+read 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 *** done
-- 
2.31.1

[PATCH 3/6] qemu-img: rebase: use backing files' BlockBackend for buffer alignment

[Andrey Drobyshev via]

Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for
the data read from the old and new backing files are aligned using
BlockDriverState (or BlockBackend later on) referring to the target image.
However, this isn't quite right, because target image is only being
written to and has nothing to do with those buffers.  Let's fix that.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qemu-img.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 78433f3746..60f4c06487 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3746,8 +3746,8 @@ static int img_rebase(int argc, char **argv)
         int64_t n;
         float local_progress = 0;
 
-        buf_old = blk_blockalign(blk, IO_BUF_SIZE);
-        buf_new = blk_blockalign(blk, IO_BUF_SIZE);
+        buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
+        buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE);
 
         size = blk_getlength(blk);
         if (size < 0) {
-- 
2.31.1

[PATCH 4/6] qemu-img: rebase: avoid unnecessary COW operations

[Andrey Drobyshev via]

When rebasing an image from one backing file to another, we need to
compare data from old and new backings.  If the diff between that data
happens to be unaligned to the target cluster size, we might end up
doing partial writes, which would lead to copy-on-write and additional IO.

Consider the following simple case (virtual_size == cluster_size == 64K):

base <-- inc1 <-- inc2

qemu-io -c "write -P 0xaa 0 32K" base.qcow2
qemu-io -c "write -P 0xcc 32K 32K" base.qcow2
qemu-io -c "write -P 0xbb 0 32K" inc1.qcow2
qemu-io -c "write -P 0xcc 32K 32K" inc1.qcow2
qemu-img rebase -f qcow2 -b base.qcow2 -F qcow2 inc2.qcow2

While doing rebase, we'll write a half of the cluster to inc2, and block
layer will have to read the 2nd half of the same cluster from the base image
inc1 while doing this write operation, although the whole cluster is already
read earlier to perform data comparison.

In order to avoid these unnecessary IO cycles, let's make sure every
write request is aligned to the overlay cluster size.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qemu-img.c | 72 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 20 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 60f4c06487..9a469cd609 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3513,6 +3513,7 @@ static int img_rebase(int argc, char **argv)
     uint8_t *buf_new = NULL;
     BlockDriverState *bs = NULL, *prefix_chain_bs = NULL;
     BlockDriverState *unfiltered_bs;
+    BlockDriverInfo bdi = {0};
     char *filename;
     const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
     int c, flags, src_flags, ret;
@@ -3646,6 +3647,15 @@ static int img_rebase(int argc, char **argv)
         }
     }
 
+    /* We need overlay cluster size to make sure write requests are aligned */
+    ret = bdrv_get_info(unfiltered_bs, &bdi);
+    if (ret < 0) {
+        error_report("could not get block driver info");
+        goto out;
+    } else if (bdi.cluster_size == 0) {
+        bdi.cluster_size = 1;
+    }
+
     /* For safe rebasing we need to compare old and new backing file */
     if (!unsafe) {
         QDict *options = NULL;
@@ -3744,6 +3754,7 @@ static int img_rebase(int argc, char **argv)
         int64_t new_backing_size = 0;
         uint64_t offset;
         int64_t n;
+        int64_t n_old = 0, n_new = 0;
         float local_progress = 0;
 
         buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
@@ -3784,7 +3795,7 @@ static int img_rebase(int argc, char **argv)
         }
 
         for (offset = 0; offset < size; offset += n) {
-            bool buf_old_is_zero = false;
+            bool old_backing_eof = false;
 
             /* How many bytes can we handle with the next read? */
             n = MIN(IO_BUF_SIZE, size - offset);
@@ -3829,33 +3840,38 @@ static int img_rebase(int argc, char **argv)
                 }
             }
 
+            /* At this point n must be aligned to the target cluster size. */
+            if (offset + n < size) {
+                assert(n % bdi.cluster_size == 0);
+            }
+
+            /*
+             * Much like the with the target image, we'll try to read as much
+             * of the old and new backings as we can.
+             */
+            n_old = MIN(n, MAX(0, old_backing_size - (int64_t) offset));
+            if (blk_new_backing) {
+                n_new = MIN(n, MAX(0, new_backing_size - (int64_t) offset));
+            }
+
             /*
              * Read old and new backing file and take into consideration that
              * backing files may be smaller than the COW image.
              */
-            if (offset >= old_backing_size) {
-                memset(buf_old, 0, n);
-                buf_old_is_zero = true;
+            memset(buf_old + n_old, 0, n - n_old);
+            if (!n_old) {
+                old_backing_eof = true;
             } else {
-                if (offset + n > old_backing_size) {
-                    n = old_backing_size - offset;
-                }
-
-                ret = blk_pread(blk_old_backing, offset, n, buf_old, 0);
+                ret = blk_pread(blk_old_backing, offset, n_old, buf_old, 0);
                 if (ret < 0) {
                     error_report("error while reading from old backing file");
                     goto out;
                 }
             }
 
-            if (offset >= new_backing_size || !blk_new_backing) {
-                memset(buf_new, 0, n);
-            } else {
-                if (offset + n > new_backing_size) {
-                    n = new_backing_size - offset;
-                }
-
-                ret = blk_pread(blk_new_backing, offset, n, buf_new, 0);
+            memset(buf_new + n_new, 0, n - n_new);
+            if (blk_new_backing && n_new) {
+                ret = blk_pread(blk_new_backing, offset, n_new, buf_new, 0);
                 if (ret < 0) {
                     error_report("error while reading from new backing file");
                     goto out;
@@ -3867,15 +3883,28 @@ static int img_rebase(int argc, char **argv)
 
             while (written < n) {
                 int64_t pnum;
+                int64_t start, end;
 
                 if (compare_buffers(buf_old + written, buf_new + written,
                                     n - written, &pnum))
                 {
-                    if (buf_old_is_zero) {
+                    if (old_backing_eof) {
                         ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
                     } else {
-                        ret = blk_pwrite(blk, offset + written, pnum,
-                                         buf_old + written, 0);
+                        /*
+                         * If we've got to this point, it means the cluster
+                         * we're dealing with is unallocated, and any partial
+                         * write will cause COW.  To avoid that, we make sure
+                         * request is aligned to cluster size.
+                         */
+                        start = QEMU_ALIGN_DOWN(offset + written,
+                                                bdi.cluster_size);
+                        end = QEMU_ALIGN_UP(offset + written + pnum,
+                                            bdi.cluster_size);
+                        end = end > size ? size : end;
+                        ret = blk_pwrite(blk, start, end - start,
+                                         buf_old + (start - offset), 0);
+                        pnum = end - (offset + written);
                     }
                     if (ret < 0) {
                         error_report("Error while writing to COW image: %s",
@@ -3885,6 +3914,9 @@ static int img_rebase(int argc, char **argv)
                 }
 
                 written += pnum;
+                if (offset + written >= old_backing_size) {
+                    old_backing_eof = true;
+                }
             }
             qemu_progress_print(local_progress, 100);
         }
-- 
2.31.1

[PATCH 5/6] qemu-img: add compression option to rebase subcommand

[Andrey Drobyshev via]

If we rebase an image whose backing file has compressed clusters, we
might end up wasting disk space since the copied clusters are now
uncompressed.  In order to have better control over this, let's add
"--compress" option to the "qemu-img rebase" command.

Note that this option affects only the clusters which are actually being
copied from the original backing file.  The clusters which were
uncompressed in the target image will remain so.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 docs/tools/qemu-img.rst |  6 ++++--
 qemu-img-cmds.hx        |  4 ++--
 qemu-img.c              | 19 +++++++++++++++++--
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 15aeddc6d8..973a912dec 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -663,7 +663,7 @@ Command description:
 
   List, apply, create or delete snapshots in image *FILENAME*.
 
-.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] -b BACKING_FILE [-F BACKING_FMT] FILENAME
+.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] [-c] -b BACKING_FILE [-F BACKING_FMT] FILENAME
 
   Changes the backing file of an image. Only the formats ``qcow2`` and
   ``qed`` support changing the backing file.
@@ -690,7 +690,9 @@ Command description:
 
     In order to achieve this, any clusters that differ between
     *BACKING_FILE* and the old backing file of *FILENAME* are merged
-    into *FILENAME* before actually changing the backing file.
+    into *FILENAME* before actually changing the backing file. With ``-c``
+    option specified, the clusters which are being merged (but not the
+    entire *FILENAME* image) are written in the compressed mode.
 
     Note that the safe mode is an expensive operation, comparable to
     converting an image. It only works if the old backing file still
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1b1dab5b17..068692d13e 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -88,9 +88,9 @@ SRST
 ERST
 
 DEF("rebase", img_rebase,
-    "rebase [--object objectdef] [--image-opts] [-U] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
+    "rebase [--object objectdef] [--image-opts] [-U] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] [-c] -b backing_file [-F backing_fmt] filename")
 SRST
-.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] -b BACKING_FILE [-F BACKING_FMT] FILENAME
+.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] [-c] -b BACKING_FILE [-F BACKING_FMT] FILENAME
 ERST
 
 DEF("resize", img_resize,
diff --git a/qemu-img.c b/qemu-img.c
index 9a469cd609..108da27b23 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3517,11 +3517,13 @@ static int img_rebase(int argc, char **argv)
     char *filename;
     const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
     int c, flags, src_flags, ret;
+    BdrvRequestFlags write_flags = 0;
     bool writethrough, src_writethrough;
     int unsafe = 0;
     bool force_share = false;
     int progress = 0;
     bool quiet = false;
+    bool compress = false;
     Error *local_err = NULL;
     bool image_opts = false;
 
@@ -3537,9 +3539,10 @@ static int img_rebase(int argc, char **argv)
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {"force-share", no_argument, 0, 'U'},
+            {"compress", no_argument, 0, 'c'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":hf:F:b:upt:T:qU",
+        c = getopt_long(argc, argv, ":hf:F:b:upt:T:qUc",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3587,6 +3590,9 @@ static int img_rebase(int argc, char **argv)
         case 'U':
             force_share = true;
             break;
+        case 'c':
+            compress = true;
+            break;
         }
     }
 
@@ -3639,6 +3645,14 @@ static int img_rebase(int argc, char **argv)
 
     unfiltered_bs = bdrv_skip_filters(bs);
 
+    if (compress && !block_driver_can_compress(unfiltered_bs->drv)) {
+        error_report("Compression not supported for this file format");
+        ret = -1;
+        goto out;
+    } else if (compress) {
+        write_flags |= BDRV_REQ_WRITE_COMPRESSED;
+    }
+
     if (out_basefmt != NULL) {
         if (bdrv_find_format(out_basefmt) == NULL) {
             error_report("Invalid format name: '%s'", out_basefmt);
@@ -3903,7 +3917,8 @@ static int img_rebase(int argc, char **argv)
                                             bdi.cluster_size);
                         end = end > size ? size : end;
                         ret = blk_pwrite(blk, start, end - start,
-                                         buf_old + (start - offset), 0);
+                                         buf_old + (start - offset),
+                                         write_flags);
                         pnum = end - (offset + written);
                     }
                     if (ret < 0) {
-- 
2.31.1

[PATCH 6/6] iotests: add test 314 for "qemu-img rebase" with compression

[Andrey Drobyshev via]

The test cases considered so far:

1. Check that compression mode isn't compatible with "-f raw" (raw
   format doesn't support compression).
2. Check that rebasing an image onto no backing file preserves the data
   and writes the copied clusters actually compressed.
3. Same as 2, but with a raw backing file (i.e. the clusters copied from the
   backing are originally uncompressed -- we check they end up compressed
   after being merged).
4. Remove a single delta from a backing chain, perform the same checks
   as in 2.
5. Check that even when backing and overlay are initially uncompressed,
   copied clusters end up compressed when rebase with compression is
   performed.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/314.out |  75 +++++++++++++++++
 2 files changed, 240 insertions(+)
 create mode 100755 tests/qemu-iotests/314
 create mode 100644 tests/qemu-iotests/314.out

diff --git a/tests/qemu-iotests/314 b/tests/qemu-iotests/314
new file mode 100755
index 0000000000..96d7b4d258
--- /dev/null
+++ b/tests/qemu-iotests/314
@@ -0,0 +1,165 @@
+#!/usr/bin/env bash
+# group: rw backing auto quick
+#
+# Test qemu-img rebase with compression
+#
+# Copyright (c) 2023 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=andrey.drobyshev@virtuozzo.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    _rm_test_img "$TEST_IMG.base"
+    _rm_test_img "$TEST_IMG.itmd"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# Want the size divisible by 2 and 3
+size=$(( 48 * 1024 * 1024 ))
+half_size=$(( size / 2 ))
+third_size=$(( size / 3 ))
+
+# 1. "qemu-img rebase -c" should refuse working with any format which doesn't
+# support compression.  We only check "-f raw" here.
+echo
+echo "=== Testing compressed rebase format compatibility ==="
+echo
+
+$QEMU_IMG create -f raw "$TEST_IMG" "$size" | _filter_img_create
+$QEMU_IMG rebase -c -f raw -b "" "$TEST_IMG"
+
+# 2. Write the 1st half of $size to backing file (compressed), 2nd half -- to
+# the top image (also compressed).  Rebase the top image onto no backing file,
+# with compression (i.e. "qemu-img -c -b ''").  Check that the resulting image
+# has the written data preserved, and "qemu-img check" reports 100% clusters
+# as compressed.
+echo
+echo "=== Testing rebase with compression onto no backing file ==="
+echo
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+_make_test_img -b "$TEST_IMG.base" -F $IMGFMT $size
+
+$QEMU_IO -c "write -c -P 0xaa 0 $half_size" "$TEST_IMG.base" | _filter_qemu_io
+$QEMU_IO -c "write -c -P 0xbb $half_size $half_size" "$TEST_IMG" \
+    | _filter_qemu_io
+
+$QEMU_IMG rebase -c -f $IMGFMT -b "" "$TEST_IMG"
+
+$QEMU_IO -c "read -P 0xaa 0 $half_size" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xbb $half_size $half_size" "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_IMG check "$TEST_IMG" | _filter_testdir
+
+# 3. Same as the previous one, but with raw backing file (hence write to
+# the backing is uncompressed).
+echo
+echo "=== Testing rebase with compression with raw backing file ==="
+echo
+
+$QEMU_IMG create -f raw "$TEST_IMG.base" "$half_size" | _filter_img_create
+_make_test_img -b "$TEST_IMG.base" -F raw $size
+
+$QEMU_IO -f raw -c "write -P 0xaa 0 $half_size" "$TEST_IMG.base" \
+    | _filter_qemu_io
+$QEMU_IO -c "write -c -P 0xbb $half_size $half_size" \
+    "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_IMG rebase -c -f $IMGFMT -b "" "$TEST_IMG"
+
+$QEMU_IO -c "read -P 0xaa 0 $half_size" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xbb $half_size $half_size" "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_IMG check "$TEST_IMG" | _filter_testdir
+
+# 4. Create a backing chain base<--itmd<--img, filling 1st, 2nd and 3rd
+# thirds of them, respectively (with compression).  Rebase img onto base,
+# effectively deleting itmd from the chain, and check that written data is
+# preserved in the resulting image.  Also check that "qemu-img check" reports
+# 100% clusters as compressed.
+echo
+echo "=== Testing compressed rebase removing single delta from the chain ==="
+echo
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+TEST_IMG="$TEST_IMG.itmd" _make_test_img -b "$TEST_IMG.base" -F $IMGFMT $size
+_make_test_img -b "$TEST_IMG.itmd" -F $IMGFMT $size
+
+$QEMU_IO -c "write -c -P 0xaa 0 $third_size" \
+    "$TEST_IMG.base" | _filter_qemu_io
+$QEMU_IO -c "write -c -P 0xbb $third_size $third_size" \
+    "$TEST_IMG.itmd" | _filter_qemu_io
+$QEMU_IO -c "write -c -P 0xcc $((third_size * 2 )) $third_size" \
+    "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_IMG rebase -c -f $IMGFMT -b "$TEST_IMG.base" -F $IMGFMT "$TEST_IMG"
+
+$QEMU_IO -c "read -P 0xaa 0 $third_size" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xbb $third_size $third_size" \
+    "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xcc $(( third_size * 2 )) $third_size" \
+    "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_IMG check "$TEST_IMG" | _filter_testdir
+
+# 5. Create one-cluster backing and overlay images, and fill only the first
+# (half - 1) bytes of the backing with data (uncompressed).  Rebase the
+# overlay onto no backing file with compression.  Check that data is still
+# read correctly, and that cluster is now really compressed ("qemu-img check"
+# reports 100% clusters as compressed.
+echo
+echo "=== Testing compressed rebase with unaligned unmerged data ==="
+echo
+
+CLUSTER_SIZE=65536
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $CLUSTER_SIZE
+_make_test_img -b "$TEST_IMG.base" -F $IMGFMT $CLUSTER_SIZE
+
+$QEMU_IO -c "write -P 0xaa 0 $(( CLUSTER_SIZE / 2 - 1 ))" $TEST_IMG.base \
+    | _filter_qemu_io
+
+$QEMU_IMG rebase -c -f $IMGFMT -b "" "$TEST_IMG"
+
+$QEMU_IO -c "read -P 0xaa 0 $(( CLUSTER_SIZE / 2 - 1 ))" "$TEST_IMG" \
+    | _filter_qemu_io
+$QEMU_IO -c \
+    "read -P 0x00 $(( CLUSTER_SIZE / 2 - 1 )) $(( CLUSTER_SIZE / 2 + 1 ))" \
+    "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_IMG check "$TEST_IMG" | _filter_testdir
+
+# success, all done
+echo
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/314.out b/tests/qemu-iotests/314.out
new file mode 100644
index 0000000000..ac9337a543
--- /dev/null
+++ b/tests/qemu-iotests/314.out
@@ -0,0 +1,75 @@
+QA output created by 314
+
+=== Testing compressed rebase format compatibility ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=raw size=50331648
+qemu-img: Compression not supported for this file format
+
+=== Testing rebase with compression onto no backing file ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=50331648
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=50331648 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+wrote 25165824/25165824 bytes at offset 0
+24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 25165824/25165824 bytes at offset 25165824
+24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 25165824/25165824 bytes at offset 0
+24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 25165824/25165824 bytes at offset 25165824
+24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+768/768 = 100.00% allocated, 100.00% fragmented, 100.00% compressed clusters
+Image end offset: 458752
+
+=== Testing rebase with compression with raw backing file ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=raw size=25165824
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=50331648 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+wrote 25165824/25165824 bytes at offset 0
+24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 25165824/25165824 bytes at offset 25165824
+24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 25165824/25165824 bytes at offset 0
+24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 25165824/25165824 bytes at offset 25165824
+24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+768/768 = 100.00% allocated, 100.00% fragmented, 100.00% compressed clusters
+Image end offset: 458752
+
+=== Testing compressed rebase removing single delta from the chain ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=50331648
+Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=50331648 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=50331648 backing_file=TEST_DIR/t.IMGFMT.itmd backing_fmt=IMGFMT
+wrote 16777216/16777216 bytes at offset 0
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 16777216/16777216 bytes at offset 16777216
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 16777216/16777216 bytes at offset 33554432
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 16777216/16777216 bytes at offset 0
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 16777216/16777216 bytes at offset 16777216
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 16777216/16777216 bytes at offset 33554432
+16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+512/768 = 66.67% allocated, 100.00% fragmented, 100.00% compressed clusters
+Image end offset: 458752
+
+=== Testing compressed rebase with unaligned unmerged data ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=65536
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+wrote 32767/32767 bytes at offset 0
+31.999 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 32767/32767 bytes at offset 0
+31.999 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 32769/32769 bytes at offset 32767
+32.001 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+1/1 = 100.00% allocated, 100.00% fragmented, 100.00% compressed clusters
+Image end offset: 393216
+
+*** done
-- 
2.31.1

Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file

[Michael Tokarev]

01.06.2023 22:28, Andrey Drobyshev via пишет:
> In case when we're rebasing within one backing chain, and when target image
> is larger than old backing file, bdrv_is_allocated_above() ends up setting
> *pnum = 0.  As a result, target offset isn't getting incremented, and we
> get stuck in an infinite for loop.  Let's detect this case and proceed
> further down the loop body, as the offsets beyond the old backing size need
> to be explicitly zeroed.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

It looks like you forgot the Reviewed-by: Denis V. Lunev here and
in the subsequent patch.

Should this be backported to -stable? Not that I've seen this issue,
it's a quite specific and somewhat rare case..

Thanks,

/mjt

Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file

[Andrey Drobyshev]

On 6/2/23 00:18, Michael Tokarev wrote:
> 01.06.2023 22:28, Andrey Drobyshev via пишет:
>> In case when we're rebasing within one backing chain, and when target
>> image
>> is larger than old backing file, bdrv_is_allocated_above() ends up
>> setting
>> *pnum = 0.  As a result, target offset isn't getting incremented, and we
>> get stuck in an infinite for loop.  Let's detect this case and proceed
>> further down the loop body, as the offsets beyond the old backing size
>> need
>> to be explicitly zeroed.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> 
> It looks like you forgot the Reviewed-by: Denis V. Lunev here and
> in the subsequent patch.

Yes, you're right, thanks for pointing that out.

> 
> Should this be backported to -stable? Not that I've seen this issue,
> it's a quite specific and somewhat rare case..

I guess in the vast majority of cases the sizes of images within the
same backing chain are equal.  But as long as it's legal to have them
unequal, a bug remains a bug.

> 
> Thanks,
> 
> /mjt

Re: [PATCH 3/6] qemu-img: rebase: use backing files' BlockBackend for buffer alignment

[Denis V. Lunev]

On 6/1/23 21:28, Andrey Drobyshev wrote:
> Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for
> the data read from the old and new backing files are aligned using
> BlockDriverState (or BlockBackend later on) referring to the target image.
> However, this isn't quite right, because target image is only being
> written to and has nothing to do with those buffers.  Let's fix that.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   qemu-img.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 78433f3746..60f4c06487 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3746,8 +3746,8 @@ static int img_rebase(int argc, char **argv)
>           int64_t n;
>           float local_progress = 0;
>   
> -        buf_old = blk_blockalign(blk, IO_BUF_SIZE);
> -        buf_new = blk_blockalign(blk, IO_BUF_SIZE);
> +        buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
> +        buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE);
>   
>           size = blk_getlength(blk);
>           if (size < 0) {
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH 5/6] qemu-img: add compression option to rebase subcommand

[Denis V. Lunev]

On 6/1/23 21:28, Andrey Drobyshev wrote:
> If we rebase an image whose backing file has compressed clusters, we
> might end up wasting disk space since the copied clusters are now
> uncompressed.  In order to have better control over this, let's add
> "--compress" option to the "qemu-img rebase" command.
>
> Note that this option affects only the clusters which are actually being
> copied from the original backing file.  The clusters which were
> uncompressed in the target image will remain so.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   docs/tools/qemu-img.rst |  6 ++++--
>   qemu-img-cmds.hx        |  4 ++--
>   qemu-img.c              | 19 +++++++++++++++++--
>   3 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 15aeddc6d8..973a912dec 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -663,7 +663,7 @@ Command description:
>   
>     List, apply, create or delete snapshots in image *FILENAME*.
>   
> -.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] -b BACKING_FILE [-F BACKING_FMT] FILENAME
> +.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] [-c] -b BACKING_FILE [-F BACKING_FMT] FILENAME
>   
>     Changes the backing file of an image. Only the formats ``qcow2`` and
>     ``qed`` support changing the backing file.
> @@ -690,7 +690,9 @@ Command description:
>   
>       In order to achieve this, any clusters that differ between
>       *BACKING_FILE* and the old backing file of *FILENAME* are merged
> -    into *FILENAME* before actually changing the backing file.
> +    into *FILENAME* before actually changing the backing file. With ``-c``
> +    option specified, the clusters which are being merged (but not the
> +    entire *FILENAME* image) are written in the compressed mode.
>   
>       Note that the safe mode is an expensive operation, comparable to
>       converting an image. It only works if the old backing file still
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 1b1dab5b17..068692d13e 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -88,9 +88,9 @@ SRST
>   ERST
>   
>   DEF("rebase", img_rebase,
> -    "rebase [--object objectdef] [--image-opts] [-U] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
> +    "rebase [--object objectdef] [--image-opts] [-U] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] [-c] -b backing_file [-F backing_fmt] filename")
>   SRST
> -.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] -b BACKING_FILE [-F BACKING_FMT] FILENAME
> +.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] [-c] -b BACKING_FILE [-F BACKING_FMT] FILENAME
>   ERST
>   
>   DEF("resize", img_resize,
> diff --git a/qemu-img.c b/qemu-img.c
> index 9a469cd609..108da27b23 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3517,11 +3517,13 @@ static int img_rebase(int argc, char **argv)
>       char *filename;
>       const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
>       int c, flags, src_flags, ret;
> +    BdrvRequestFlags write_flags = 0;
>       bool writethrough, src_writethrough;
>       int unsafe = 0;
>       bool force_share = false;
>       int progress = 0;
>       bool quiet = false;
> +    bool compress = false;
>       Error *local_err = NULL;
>       bool image_opts = false;
>   
> @@ -3537,9 +3539,10 @@ static int img_rebase(int argc, char **argv)
>               {"object", required_argument, 0, OPTION_OBJECT},
>               {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
>               {"force-share", no_argument, 0, 'U'},
> +            {"compress", no_argument, 0, 'c'},
>               {0, 0, 0, 0}
>           };
> -        c = getopt_long(argc, argv, ":hf:F:b:upt:T:qU",
> +        c = getopt_long(argc, argv, ":hf:F:b:upt:T:qUc",
>                           long_options, NULL);
>           if (c == -1) {
>               break;
> @@ -3587,6 +3590,9 @@ static int img_rebase(int argc, char **argv)
>           case 'U':
>               force_share = true;
>               break;
> +        case 'c':
> +            compress = true;
> +            break;
>           }
>       }
>   
> @@ -3639,6 +3645,14 @@ static int img_rebase(int argc, char **argv)
>   
>       unfiltered_bs = bdrv_skip_filters(bs);
>   
> +    if (compress && !block_driver_can_compress(unfiltered_bs->drv)) {
> +        error_report("Compression not supported for this file format");
> +        ret = -1;
> +        goto out;
> +    } else if (compress) {
> +        write_flags |= BDRV_REQ_WRITE_COMPRESSED;
> +    }
> +

minor neat-picking. Should we get a global
if (compress) {
   if (!block_driver_can_compress(unfiltered_bs->drv)) {
      report_error
      goto out;
   }
   write_flags |= BDRV_REQ_WRITE_COMPRESSED;
}

>       if (out_basefmt != NULL) {
>           if (bdrv_find_format(out_basefmt) == NULL) {
>               error_report("Invalid format name: '%s'", out_basefmt);
> @@ -3903,7 +3917,8 @@ static int img_rebase(int argc, char **argv)
>                                               bdi.cluster_size);
>                           end = end > size ? size : end;
>                           ret = blk_pwrite(blk, start, end - start,
> -                                         buf_old + (start - offset), 0);
> +                                         buf_old + (start - offset),
> +                                         write_flags);
>                           pnum = end - (offset + written);
>                       }
>                       if (ret < 0) {
Anyway, for both variants,
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file

[Denis V. Lunev]

On 6/1/23 21:28, Andrey Drobyshev wrote:
> In case when we're rebasing within one backing chain, and when target image
> is larger than old backing file, bdrv_is_allocated_above() ends up setting
> *pnum = 0.  As a result, target offset isn't getting incremented, and we
> get stuck in an infinite for loop.  Let's detect this case and proceed
> further down the loop body, as the offsets beyond the old backing size need
> to be explicitly zeroed.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   qemu-img.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 27f48051b0..78433f3746 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3801,6 +3801,8 @@ static int img_rebase(int argc, char **argv)
>               }
>   
>               if (prefix_chain_bs) {
> +                uint64_t bytes = n;
> +
>                   /*
>                    * If cluster wasn't changed since prefix_chain, we don't need
>                    * to take action
> @@ -3813,9 +3815,18 @@ static int img_rebase(int argc, char **argv)
>                                    strerror(-ret));
>                       goto out;
>                   }
> -                if (!ret) {
> +                if (!ret && n) {
>                       continue;
>                   }
> +                if (!n) {
> +                    /*
> +                     * If we've reached EOF of the old backing, it means that
> +                     * offsets beyond the old backing size were read as zeroes.
> +                     * Now we will need to explicitly zero the cluster in
> +                     * order to preserve that state after the rebase.
> +                     */
> +                    n = bytes;
> +                }
>               }
>   
>               /*
for the clarity:
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH 2/6] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size

[Denis V. Lunev]

On 6/1/23 21:28, Andrey Drobyshev wrote:
> Before previous commit, rebase was getting infitely stuck in case of
> rebasing within the same backing chain and when overlay_size > backing_size.
> Let's add this case to the rebasing test 024 to make sure it doesn't
> break again.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   tests/qemu-iotests/024     | 57 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/024.out | 30 ++++++++++++++++++++
>   2 files changed, 87 insertions(+)
>
> diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
> index 25a564a150..98a7c8fd65 100755
> --- a/tests/qemu-iotests/024
> +++ b/tests/qemu-iotests/024
> @@ -199,6 +199,63 @@ echo
>   # $BASE_OLD and $BASE_NEW)
>   $QEMU_IMG map "$OVERLAY" | _filter_qemu_img_map
>   
> +# Check that rebase within the chain is working when
> +# overlay_size > old_backing_size
> +#
> +# base_new <-- base_old <-- overlay
> +#
> +# Backing (new): 11 11 11 11 11
> +# Backing (old): 22 22 22 22
> +# Overlay:       -- -- -- -- --
> +#
> +# As a result, overlay should contain data identical to base_old, with the
> +# last cluster remaining unallocated.
> +
> +echo
> +echo "=== Test rebase within one backing chain ==="
> +echo
> +
> +echo "Creating backing chain"
> +echo
> +
> +TEST_IMG=$BASE_NEW _make_test_img $(( CLUSTER_SIZE * 5 ))
> +TEST_IMG=$BASE_OLD _make_test_img -b "$BASE_NEW" -F $IMGFMT \
> +    $(( CLUSTER_SIZE * 4 ))
> +TEST_IMG=$OVERLAY _make_test_img -b "$BASE_OLD" -F $IMGFMT \
> +    $(( CLUSTER_SIZE * 5 ))
> +
> +echo
> +echo "Fill backing files with data"
> +echo
> +
> +$QEMU_IO "$BASE_NEW" -c "write -P 0x11 0 $(( CLUSTER_SIZE * 5 ))" \
> +    | _filter_qemu_io
> +$QEMU_IO "$BASE_OLD" -c "write -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
> +    | _filter_qemu_io
> +
> +echo
> +echo "Check the last cluster is zeroed in overlay before the rebase"
> +echo
> +$QEMU_IO "$OVERLAY" -c "read -P 0x00 $(( CLUSTER_SIZE * 4 )) $CLUSTER_SIZE" \
> +    | _filter_qemu_io
> +
> +echo
> +echo "Rebase onto another image in the same chain"
> +echo
> +
> +$QEMU_IMG rebase -b "$BASE_NEW" -F $IMGFMT "$OVERLAY"
> +
> +echo "Verify that data is read the same before and after rebase"
> +echo
> +
> +# Verify the first 4 clusters are still read the same as in the old base
> +$QEMU_IO "$OVERLAY" -c "read -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
> +    | _filter_qemu_io
> +# Verify the last cluster still reads as zeroes
> +$QEMU_IO "$OVERLAY" -c "read -P 0x00 $(( CLUSTER_SIZE * 4 )) $CLUSTER_SIZE" \
> +    | _filter_qemu_io
> +
> +echo
>   
>   # success, all done
>   echo "*** done"
> diff --git a/tests/qemu-iotests/024.out b/tests/qemu-iotests/024.out
> index 973a5a3711..245fe8b1d1 100644
> --- a/tests/qemu-iotests/024.out
> +++ b/tests/qemu-iotests/024.out
> @@ -171,4 +171,34 @@ 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 within one backing chain ===
> +
> +Creating backing chain
> +
> +Formatting 'TEST_DIR/subdir/t.IMGFMT.base_new', fmt=IMGFMT size=327680
> +Formatting 'TEST_DIR/subdir/t.IMGFMT.base_old', fmt=IMGFMT size=262144 backing_file=TEST_DIR/subdir/t.IMGFMT.base_new backing_fmt=IMGFMT
> +Formatting 'TEST_DIR/subdir/t.IMGFMT', fmt=IMGFMT size=327680 backing_file=TEST_DIR/subdir/t.IMGFMT.base_old backing_fmt=IMGFMT
> +
> +Fill backing files with data
> +
> +wrote 327680/327680 bytes at offset 0
> +320 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 262144/262144 bytes at offset 0
> +256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +Check the last cluster is zeroed in overlay before the rebase
> +
> +read 65536/65536 bytes at offset 262144
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +Rebase onto another image in the same chain
> +
> +Verify that data is read the same before and after rebase
> +
> +read 262144/262144 bytes at offset 0
> +256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 65536/65536 bytes at offset 262144
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
>   *** done
For the clarity:
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH 4/6] qemu-img: rebase: avoid unnecessary COW operations

[Denis V. Lunev]

On 6/1/23 21:28, Andrey Drobyshev wrote:
> When rebasing an image from one backing file to another, we need to
> compare data from old and new backings.  If the diff between that data
> happens to be unaligned to the target cluster size, we might end up
> doing partial writes, which would lead to copy-on-write and additional IO.
>
> Consider the following simple case (virtual_size == cluster_size == 64K):
>
> base <-- inc1 <-- inc2
>
> qemu-io -c "write -P 0xaa 0 32K" base.qcow2
> qemu-io -c "write -P 0xcc 32K 32K" base.qcow2
> qemu-io -c "write -P 0xbb 0 32K" inc1.qcow2
> qemu-io -c "write -P 0xcc 32K 32K" inc1.qcow2
> qemu-img rebase -f qcow2 -b base.qcow2 -F qcow2 inc2.qcow2
>
> While doing rebase, we'll write a half of the cluster to inc2, and block
> layer will have to read the 2nd half of the same cluster from the base image
> inc1 while doing this write operation, although the whole cluster is already
> read earlier to perform data comparison.
>
> In order to avoid these unnecessary IO cycles, let's make sure every
> write request is aligned to the overlay cluster size.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   qemu-img.c | 72 +++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 52 insertions(+), 20 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 60f4c06487..9a469cd609 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3513,6 +3513,7 @@ static int img_rebase(int argc, char **argv)
>       uint8_t *buf_new = NULL;
>       BlockDriverState *bs = NULL, *prefix_chain_bs = NULL;
>       BlockDriverState *unfiltered_bs;
> +    BlockDriverInfo bdi = {0};
>       char *filename;
>       const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
>       int c, flags, src_flags, ret;
> @@ -3646,6 +3647,15 @@ static int img_rebase(int argc, char **argv)
>           }
>       }
>   
> +    /* We need overlay cluster size to make sure write requests are aligned */
> +    ret = bdrv_get_info(unfiltered_bs, &bdi);
> +    if (ret < 0) {
> +        error_report("could not get block driver info");
> +        goto out;
> +    } else if (bdi.cluster_size == 0) {
> +        bdi.cluster_size = 1;
> +    }
> +
>       /* For safe rebasing we need to compare old and new backing file */
>       if (!unsafe) {
>           QDict *options = NULL;
> @@ -3744,6 +3754,7 @@ static int img_rebase(int argc, char **argv)
>           int64_t new_backing_size = 0;
>           uint64_t offset;
>           int64_t n;
> +        int64_t n_old = 0, n_new = 0;
>           float local_progress = 0;
>   
>           buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
> @@ -3784,7 +3795,7 @@ static int img_rebase(int argc, char **argv)
>           }
>   
>           for (offset = 0; offset < size; offset += n) {
> -            bool buf_old_is_zero = false;
> +            bool old_backing_eof = false;
>   
>               /* How many bytes can we handle with the next read? */
>               n = MIN(IO_BUF_SIZE, size - offset);
> @@ -3829,33 +3840,38 @@ static int img_rebase(int argc, char **argv)
>                   }
>               }
>   
> +            /* At this point n must be aligned to the target cluster size. */
Minor: except last non-aligned cluster as stated by 'if' :)
> +            if (offset + n < size) {
> +                assert(n % bdi.cluster_size == 0);
> +            }
> +
> +            /*
> +             * Much like the with the target image, we'll try to read as much
> +             * of the old and new backings as we can.
> +             */
> +            n_old = MIN(n, MAX(0, old_backing_size - (int64_t) offset));
> +            if (blk_new_backing) {
> +                n_new = MIN(n, MAX(0, new_backing_size - (int64_t) offset));
> +            }
> +
>               /*
>                * Read old and new backing file and take into consideration that
>                * backing files may be smaller than the COW image.
>                */
> -            if (offset >= old_backing_size) {
> -                memset(buf_old, 0, n);
> -                buf_old_is_zero = true;
> +            memset(buf_old + n_old, 0, n - n_old);
> +            if (!n_old) {
> +                old_backing_eof = true;
>               } else {
> -                if (offset + n > old_backing_size) {
> -                    n = old_backing_size - offset;
> -                }
> -
> -                ret = blk_pread(blk_old_backing, offset, n, buf_old, 0);
> +                ret = blk_pread(blk_old_backing, offset, n_old, buf_old, 0);
>                   if (ret < 0) {
>                       error_report("error while reading from old backing file");
>                       goto out;
>                   }
>               }
>   
> -            if (offset >= new_backing_size || !blk_new_backing) {
> -                memset(buf_new, 0, n);
> -            } else {
> -                if (offset + n > new_backing_size) {
> -                    n = new_backing_size - offset;
> -                }
> -
> -                ret = blk_pread(blk_new_backing, offset, n, buf_new, 0);
> +            memset(buf_new + n_new, 0, n - n_new);
> +            if (blk_new_backing && n_new) {
> +                ret = blk_pread(blk_new_backing, offset, n_new, buf_new, 0);
>                   if (ret < 0) {
>                       error_report("error while reading from new backing file");
>                       goto out;
> @@ -3867,15 +3883,28 @@ static int img_rebase(int argc, char **argv)
>   
>               while (written < n) {
>                   int64_t pnum;
> +                int64_t start, end;
>   
>                   if (compare_buffers(buf_old + written, buf_new + written,
>                                       n - written, &pnum))
>                   {
> -                    if (buf_old_is_zero) {
> +                    if (old_backing_eof) {
>                           ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
>                       } else {
> -                        ret = blk_pwrite(blk, offset + written, pnum,
> -                                         buf_old + written, 0);
> +                        /*
> +                         * If we've got to this point, it means the cluster
> +                         * we're dealing with is unallocated, and any partial
> +                         * write will cause COW.  To avoid that, we make sure
> +                         * request is aligned to cluster size.
> +                         */
> +                        start = QEMU_ALIGN_DOWN(offset + written,
> +                                                bdi.cluster_size);
> +                        end = QEMU_ALIGN_UP(offset + written + pnum,
> +                                            bdi.cluster_size);
> +                        end = end > size ? size : end;
Minor (?): end = MIN(size, QEMU_ALIGN_UP(offset + written + pnum, 
bdi.cluster_size))
> +                        ret = blk_pwrite(blk, start, end - start,
> +                                         buf_old + (start - offset), 0);
> +                        pnum = end - (offset + written);
>                       }
>                       if (ret < 0) {
>                           error_report("Error while writing to COW image: %s",
> @@ -3885,6 +3914,9 @@ static int img_rebase(int argc, char **argv)
>                   }
>   
>                   written += pnum;
> +                if (offset + written >= old_backing_size) {
> +                    old_backing_eof = true;
> +                }
>               }
>               qemu_progress_print(local_progress, 100);
>           }
With or without minors:
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH 6/6] iotests: add test 314 for "qemu-img rebase" with compression

[Denis V. Lunev]

On 6/1/23 21:28, Andrey Drobyshev wrote:
> The test cases considered so far:
>
> 1. Check that compression mode isn't compatible with "-f raw" (raw
>     format doesn't support compression).
> 2. Check that rebasing an image onto no backing file preserves the data
>     and writes the copied clusters actually compressed.
> 3. Same as 2, but with a raw backing file (i.e. the clusters copied from the
>     backing are originally uncompressed -- we check they end up compressed
>     after being merged).
> 4. Remove a single delta from a backing chain, perform the same checks
>     as in 2.
> 5. Check that even when backing and overlay are initially uncompressed,
>     copied clusters end up compressed when rebase with compression is
>     performed.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/314.out |  75 +++++++++++++++++
>   2 files changed, 240 insertions(+)
>   create mode 100755 tests/qemu-iotests/314
>   create mode 100644 tests/qemu-iotests/314.out
>
> diff --git a/tests/qemu-iotests/314 b/tests/qemu-iotests/314
> new file mode 100755
> index 0000000000..96d7b4d258
> --- /dev/null
> +++ b/tests/qemu-iotests/314
> @@ -0,0 +1,165 @@
> +#!/usr/bin/env bash
> +# group: rw backing auto quick
> +#
> +# Test qemu-img rebase with compression
> +#
> +# Copyright (c) 2023 Virtuozzo International GmbH.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=andrey.drobyshev@virtuozzo.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +    _cleanup_test_img
> +    _rm_test_img "$TEST_IMG.base"
> +    _rm_test_img "$TEST_IMG.itmd"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +
> +# Want the size divisible by 2 and 3
> +size=$(( 48 * 1024 * 1024 ))
> +half_size=$(( size / 2 ))
> +third_size=$(( size / 3 ))
> +
> +# 1. "qemu-img rebase -c" should refuse working with any format which doesn't
> +# support compression.  We only check "-f raw" here.
> +echo
> +echo "=== Testing compressed rebase format compatibility ==="
> +echo
> +
> +$QEMU_IMG create -f raw "$TEST_IMG" "$size" | _filter_img_create
> +$QEMU_IMG rebase -c -f raw -b "" "$TEST_IMG"
> +
> +# 2. Write the 1st half of $size to backing file (compressed), 2nd half -- to
> +# the top image (also compressed).  Rebase the top image onto no backing file,
> +# with compression (i.e. "qemu-img -c -b ''").  Check that the resulting image
> +# has the written data preserved, and "qemu-img check" reports 100% clusters
> +# as compressed.
> +echo
> +echo "=== Testing rebase with compression onto no backing file ==="
> +echo
> +
> +TEST_IMG="$TEST_IMG.base" _make_test_img $size
> +_make_test_img -b "$TEST_IMG.base" -F $IMGFMT $size
> +
> +$QEMU_IO -c "write -c -P 0xaa 0 $half_size" "$TEST_IMG.base" | _filter_qemu_io
> +$QEMU_IO -c "write -c -P 0xbb $half_size $half_size" "$TEST_IMG" \
> +    | _filter_qemu_io
> +
> +$QEMU_IMG rebase -c -f $IMGFMT -b "" "$TEST_IMG"
> +
> +$QEMU_IO -c "read -P 0xaa 0 $half_size" "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c "read -P 0xbb $half_size $half_size" "$TEST_IMG" | _filter_qemu_io
> +
> +$QEMU_IMG check "$TEST_IMG" | _filter_testdir
> +
> +# 3. Same as the previous one, but with raw backing file (hence write to
> +# the backing is uncompressed).
> +echo
> +echo "=== Testing rebase with compression with raw backing file ==="
> +echo
> +
> +$QEMU_IMG create -f raw "$TEST_IMG.base" "$half_size" | _filter_img_create
> +_make_test_img -b "$TEST_IMG.base" -F raw $size
> +
> +$QEMU_IO -f raw -c "write -P 0xaa 0 $half_size" "$TEST_IMG.base" \
> +    | _filter_qemu_io
> +$QEMU_IO -c "write -c -P 0xbb $half_size $half_size" \
> +    "$TEST_IMG" | _filter_qemu_io
> +
> +$QEMU_IMG rebase -c -f $IMGFMT -b "" "$TEST_IMG"
> +
> +$QEMU_IO -c "read -P 0xaa 0 $half_size" "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c "read -P 0xbb $half_size $half_size" "$TEST_IMG" | _filter_qemu_io
> +
> +$QEMU_IMG check "$TEST_IMG" | _filter_testdir
> +
> +# 4. Create a backing chain base<--itmd<--img, filling 1st, 2nd and 3rd
> +# thirds of them, respectively (with compression).  Rebase img onto base,
> +# effectively deleting itmd from the chain, and check that written data is
> +# preserved in the resulting image.  Also check that "qemu-img check" reports
> +# 100% clusters as compressed.
> +echo
> +echo "=== Testing compressed rebase removing single delta from the chain ==="
> +echo
> +
> +TEST_IMG="$TEST_IMG.base" _make_test_img $size
> +TEST_IMG="$TEST_IMG.itmd" _make_test_img -b "$TEST_IMG.base" -F $IMGFMT $size
> +_make_test_img -b "$TEST_IMG.itmd" -F $IMGFMT $size
> +
> +$QEMU_IO -c "write -c -P 0xaa 0 $third_size" \
> +    "$TEST_IMG.base" | _filter_qemu_io
> +$QEMU_IO -c "write -c -P 0xbb $third_size $third_size" \
> +    "$TEST_IMG.itmd" | _filter_qemu_io
> +$QEMU_IO -c "write -c -P 0xcc $((third_size * 2 )) $third_size" \
> +    "$TEST_IMG" | _filter_qemu_io
> +
> +$QEMU_IMG rebase -c -f $IMGFMT -b "$TEST_IMG.base" -F $IMGFMT "$TEST_IMG"
> +
> +$QEMU_IO -c "read -P 0xaa 0 $third_size" "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c "read -P 0xbb $third_size $third_size" \
> +    "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c "read -P 0xcc $(( third_size * 2 )) $third_size" \
> +    "$TEST_IMG" | _filter_qemu_io
> +
> +$QEMU_IMG check "$TEST_IMG" | _filter_testdir
> +
> +# 5. Create one-cluster backing and overlay images, and fill only the first
> +# (half - 1) bytes of the backing with data (uncompressed).  Rebase the
> +# overlay onto no backing file with compression.  Check that data is still
> +# read correctly, and that cluster is now really compressed ("qemu-img check"
> +# reports 100% clusters as compressed.
> +echo
> +echo "=== Testing compressed rebase with unaligned unmerged data ==="
> +echo
> +
> +CLUSTER_SIZE=65536
> +
> +TEST_IMG="$TEST_IMG.base" _make_test_img $CLUSTER_SIZE
> +_make_test_img -b "$TEST_IMG.base" -F $IMGFMT $CLUSTER_SIZE
> +
> +$QEMU_IO -c "write -P 0xaa 0 $(( CLUSTER_SIZE / 2 - 1 ))" $TEST_IMG.base \
> +    | _filter_qemu_io
> +
> +$QEMU_IMG rebase -c -f $IMGFMT -b "" "$TEST_IMG"
> +
> +$QEMU_IO -c "read -P 0xaa 0 $(( CLUSTER_SIZE / 2 - 1 ))" "$TEST_IMG" \
> +    | _filter_qemu_io
> +$QEMU_IO -c \
> +    "read -P 0x00 $(( CLUSTER_SIZE / 2 - 1 )) $(( CLUSTER_SIZE / 2 + 1 ))" \
> +    "$TEST_IMG" | _filter_qemu_io
> +
> +$QEMU_IMG check "$TEST_IMG" | _filter_testdir
> +
> +# success, all done
> +echo
> +echo '*** done'
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/314.out b/tests/qemu-iotests/314.out
> new file mode 100644
> index 0000000000..ac9337a543
> --- /dev/null
> +++ b/tests/qemu-iotests/314.out
> @@ -0,0 +1,75 @@
> +QA output created by 314
> +
> +=== Testing compressed rebase format compatibility ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=raw size=50331648
> +qemu-img: Compression not supported for this file format
> +
> +=== Testing rebase with compression onto no backing file ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=50331648
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=50331648 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
> +wrote 25165824/25165824 bytes at offset 0
> +24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 25165824/25165824 bytes at offset 25165824
> +24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 25165824/25165824 bytes at offset 0
> +24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 25165824/25165824 bytes at offset 25165824
> +24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +No errors were found on the image.
> +768/768 = 100.00% allocated, 100.00% fragmented, 100.00% compressed clusters
> +Image end offset: 458752
> +
> +=== Testing rebase with compression with raw backing file ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=raw size=25165824
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=50331648 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
> +wrote 25165824/25165824 bytes at offset 0
> +24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 25165824/25165824 bytes at offset 25165824
> +24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 25165824/25165824 bytes at offset 0
> +24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 25165824/25165824 bytes at offset 25165824
> +24 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +No errors were found on the image.
> +768/768 = 100.00% allocated, 100.00% fragmented, 100.00% compressed clusters
> +Image end offset: 458752
> +
> +=== Testing compressed rebase removing single delta from the chain ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=50331648
> +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=50331648 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=50331648 backing_file=TEST_DIR/t.IMGFMT.itmd backing_fmt=IMGFMT
> +wrote 16777216/16777216 bytes at offset 0
> +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 16777216/16777216 bytes at offset 16777216
> +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 16777216/16777216 bytes at offset 33554432
> +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 16777216/16777216 bytes at offset 0
> +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 16777216/16777216 bytes at offset 16777216
> +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 16777216/16777216 bytes at offset 33554432
> +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +No errors were found on the image.
> +512/768 = 66.67% allocated, 100.00% fragmented, 100.00% compressed clusters
> +Image end offset: 458752
> +
> +=== Testing compressed rebase with unaligned unmerged data ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=65536
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
> +wrote 32767/32767 bytes at offset 0
> +31.999 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 32767/32767 bytes at offset 0
> +31.999 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 32769/32769 bytes at offset 32767
> +32.001 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +No errors were found on the image.
> +1/1 = 100.00% allocated, 100.00% fragmented, 100.00% compressed clusters
> +Image end offset: 393216
> +
> +*** done
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH 0/6] qemu-img: rebase: add compression support

[Denis V. Lunev]

On 6/1/23 21:28, Andrey Drobyshev wrote:
> This series is adding [-c | --compress] option to "qemu-img rebase"
> command, which might prove useful for saving some disk space when, for
> instance, manipulating chains of backup images.  Along the way I had to
> make a couple of minor improvements.
>
> The first 2 patches are a bug fix + corresponding test case.
> Patch 3 merely fixes wrong args used in allocation.
> Patch 4 makes write requests during rebase operation cluster_size-aligned,
> which seems to be beneficial for both non-compressed and compressed mode.
> The last 2 patches are the actual feature implementation + tests.
>
> Andrey Drobyshev (6):
>    qemu-img: rebase: stop when reaching EOF of old backing file
>    qemu-iotests: 024: add rebasing test case for overlay_size >
>      backing_size
>    qemu-img: rebase: use backing files' BlockBackend for buffer alignment
>    qemu-img: rebase: avoid unnecessary COW operations
>    qemu-img: add compression option to rebase subcommand
>    iotests: add test 314 for "qemu-img rebase" with compression
>
>   docs/tools/qemu-img.rst    |   6 +-
>   qemu-img-cmds.hx           |   4 +-
>   qemu-img.c                 | 106 ++++++++++++++++++------
>   tests/qemu-iotests/024     |  57 +++++++++++++
>   tests/qemu-iotests/024.out |  30 +++++++
>   tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/314.out |  75 +++++++++++++++++
>   7 files changed, 415 insertions(+), 28 deletions(-)
>   create mode 100755 tests/qemu-iotests/314
>   create mode 100644 tests/qemu-iotests/314.out
>
ping

Re: [PATCH 0/6] qemu-img: rebase: add compression support

[Andrey Drobyshev]

On 6/30/23 13:54, Denis V. Lunev wrote:
> On 6/1/23 21:28, Andrey Drobyshev wrote:
>> This series is adding [-c | --compress] option to "qemu-img rebase"
>> command, which might prove useful for saving some disk space when, for
>> instance, manipulating chains of backup images.  Along the way I had to
>> make a couple of minor improvements.
>>
>> The first 2 patches are a bug fix + corresponding test case.
>> Patch 3 merely fixes wrong args used in allocation.
>> Patch 4 makes write requests during rebase operation
>> cluster_size-aligned,
>> which seems to be beneficial for both non-compressed and compressed mode.
>> The last 2 patches are the actual feature implementation + tests.
>>
>> Andrey Drobyshev (6):
>>    qemu-img: rebase: stop when reaching EOF of old backing file
>>    qemu-iotests: 024: add rebasing test case for overlay_size >
>>      backing_size
>>    qemu-img: rebase: use backing files' BlockBackend for buffer alignment
>>    qemu-img: rebase: avoid unnecessary COW operations
>>    qemu-img: add compression option to rebase subcommand
>>    iotests: add test 314 for "qemu-img rebase" with compression
>>
>>   docs/tools/qemu-img.rst    |   6 +-
>>   qemu-img-cmds.hx           |   4 +-
>>   qemu-img.c                 | 106 ++++++++++++++++++------
>>   tests/qemu-iotests/024     |  57 +++++++++++++
>>   tests/qemu-iotests/024.out |  30 +++++++
>>   tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/314.out |  75 +++++++++++++++++
>>   7 files changed, 415 insertions(+), 28 deletions(-)
>>   create mode 100755 tests/qemu-iotests/314
>>   create mode 100644 tests/qemu-iotests/314.out
>>
> ping

Friendly ping

Re: [PATCH 0/6] qemu-img: rebase: add compression support

[Andrey Drobyshev]

On 6/30/23 13:54, Denis V. Lunev wrote:
> On 6/1/23 21:28, Andrey Drobyshev wrote:
>> This series is adding [-c | --compress] option to "qemu-img rebase"
>> command, which might prove useful for saving some disk space when, for
>> instance, manipulating chains of backup images.  Along the way I had to
>> make a couple of minor improvements.
>>
>> The first 2 patches are a bug fix + corresponding test case.
>> Patch 3 merely fixes wrong args used in allocation.
>> Patch 4 makes write requests during rebase operation
>> cluster_size-aligned,
>> which seems to be beneficial for both non-compressed and compressed mode.
>> The last 2 patches are the actual feature implementation + tests.
>>
>> Andrey Drobyshev (6):
>>    qemu-img: rebase: stop when reaching EOF of old backing file
>>    qemu-iotests: 024: add rebasing test case for overlay_size >
>>      backing_size
>>    qemu-img: rebase: use backing files' BlockBackend for buffer alignment
>>    qemu-img: rebase: avoid unnecessary COW operations
>>    qemu-img: add compression option to rebase subcommand
>>    iotests: add test 314 for "qemu-img rebase" with compression
>>
>>   docs/tools/qemu-img.rst    |   6 +-
>>   qemu-img-cmds.hx           |   4 +-
>>   qemu-img.c                 | 106 ++++++++++++++++++------
>>   tests/qemu-iotests/024     |  57 +++++++++++++
>>   tests/qemu-iotests/024.out |  30 +++++++
>>   tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/314.out |  75 +++++++++++++++++
>>   7 files changed, 415 insertions(+), 28 deletions(-)
>>   create mode 100755 tests/qemu-iotests/314
>>   create mode 100644 tests/qemu-iotests/314.out
>>
> ping

Friendly ping after 7 weeks

Re: [PATCH 0/6] qemu-img: rebase: add compression support

[Andrey Drobyshev]

On 7/24/23 16:11, Andrey Drobyshev wrote:
> On 6/30/23 13:54, Denis V. Lunev wrote:
>> On 6/1/23 21:28, Andrey Drobyshev wrote:
>>> This series is adding [-c | --compress] option to "qemu-img rebase"
>>> command, which might prove useful for saving some disk space when, for
>>> instance, manipulating chains of backup images.  Along the way I had to
>>> make a couple of minor improvements.
>>>
>>> The first 2 patches are a bug fix + corresponding test case.
>>> Patch 3 merely fixes wrong args used in allocation.
>>> Patch 4 makes write requests during rebase operation
>>> cluster_size-aligned,
>>> which seems to be beneficial for both non-compressed and compressed mode.
>>> The last 2 patches are the actual feature implementation + tests.
>>>
>>> Andrey Drobyshev (6):
>>>    qemu-img: rebase: stop when reaching EOF of old backing file
>>>    qemu-iotests: 024: add rebasing test case for overlay_size >
>>>      backing_size
>>>    qemu-img: rebase: use backing files' BlockBackend for buffer alignment
>>>    qemu-img: rebase: avoid unnecessary COW operations
>>>    qemu-img: add compression option to rebase subcommand
>>>    iotests: add test 314 for "qemu-img rebase" with compression
>>>
>>>   docs/tools/qemu-img.rst    |   6 +-
>>>   qemu-img-cmds.hx           |   4 +-
>>>   qemu-img.c                 | 106 ++++++++++++++++++------
>>>   tests/qemu-iotests/024     |  57 +++++++++++++
>>>   tests/qemu-iotests/024.out |  30 +++++++
>>>   tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/314.out |  75 +++++++++++++++++
>>>   7 files changed, 415 insertions(+), 28 deletions(-)
>>>   create mode 100755 tests/qemu-iotests/314
>>>   create mode 100644 tests/qemu-iotests/314.out
>>>
>> ping
> 
> Friendly ping after 7 weeks

Yet another ping

Re: [PATCH 0/6] qemu-img: rebase: add compression support

[Andrey Drobyshev]

On 7/31/23 17:43, Andrey Drobyshev wrote:
> On 7/24/23 16:11, Andrey Drobyshev wrote:
>> On 6/30/23 13:54, Denis V. Lunev wrote:
>>> On 6/1/23 21:28, Andrey Drobyshev wrote:
>>>> This series is adding [-c | --compress] option to "qemu-img rebase"
>>>> command, which might prove useful for saving some disk space when, for
>>>> instance, manipulating chains of backup images.  Along the way I had to
>>>> make a couple of minor improvements.
>>>>
>>>> The first 2 patches are a bug fix + corresponding test case.
>>>> Patch 3 merely fixes wrong args used in allocation.
>>>> Patch 4 makes write requests during rebase operation
>>>> cluster_size-aligned,
>>>> which seems to be beneficial for both non-compressed and compressed mode.
>>>> The last 2 patches are the actual feature implementation + tests.
>>>>
>>>> Andrey Drobyshev (6):
>>>>    qemu-img: rebase: stop when reaching EOF of old backing file
>>>>    qemu-iotests: 024: add rebasing test case for overlay_size >
>>>>      backing_size
>>>>    qemu-img: rebase: use backing files' BlockBackend for buffer alignment
>>>>    qemu-img: rebase: avoid unnecessary COW operations
>>>>    qemu-img: add compression option to rebase subcommand
>>>>    iotests: add test 314 for "qemu-img rebase" with compression
>>>>
>>>>   docs/tools/qemu-img.rst    |   6 +-
>>>>   qemu-img-cmds.hx           |   4 +-
>>>>   qemu-img.c                 | 106 ++++++++++++++++++------
>>>>   tests/qemu-iotests/024     |  57 +++++++++++++
>>>>   tests/qemu-iotests/024.out |  30 +++++++
>>>>   tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
>>>>   tests/qemu-iotests/314.out |  75 +++++++++++++++++
>>>>   7 files changed, 415 insertions(+), 28 deletions(-)
>>>>   create mode 100755 tests/qemu-iotests/314
>>>>   create mode 100644 tests/qemu-iotests/314.out
>>>>
>>> ping
>>
>> Friendly ping after 7 weeks
> 
> Yet another ping

One more ping after 12 weeks of silence

Re: [PATCH 0/6] qemu-img: rebase: add compression support

[Andrey Drobyshev]

On 8/16/23 12:22, Andrey Drobyshev wrote:
> On 7/31/23 17:43, Andrey Drobyshev wrote:
>> On 7/24/23 16:11, Andrey Drobyshev wrote:
>>> On 6/30/23 13:54, Denis V. Lunev wrote:
>>>> On 6/1/23 21:28, Andrey Drobyshev wrote:
>>>>> This series is adding [-c | --compress] option to "qemu-img rebase"
>>>>> command, which might prove useful for saving some disk space when, for
>>>>> instance, manipulating chains of backup images.  Along the way I had to
>>>>> make a couple of minor improvements.
>>>>>
>>>>> The first 2 patches are a bug fix + corresponding test case.
>>>>> Patch 3 merely fixes wrong args used in allocation.
>>>>> Patch 4 makes write requests during rebase operation
>>>>> cluster_size-aligned,
>>>>> which seems to be beneficial for both non-compressed and compressed mode.
>>>>> The last 2 patches are the actual feature implementation + tests.
>>>>>
>>>>> Andrey Drobyshev (6):
>>>>>    qemu-img: rebase: stop when reaching EOF of old backing file
>>>>>    qemu-iotests: 024: add rebasing test case for overlay_size >
>>>>>      backing_size
>>>>>    qemu-img: rebase: use backing files' BlockBackend for buffer alignment
>>>>>    qemu-img: rebase: avoid unnecessary COW operations
>>>>>    qemu-img: add compression option to rebase subcommand
>>>>>    iotests: add test 314 for "qemu-img rebase" with compression
>>>>>
>>>>>   docs/tools/qemu-img.rst    |   6 +-
>>>>>   qemu-img-cmds.hx           |   4 +-
>>>>>   qemu-img.c                 | 106 ++++++++++++++++++------
>>>>>   tests/qemu-iotests/024     |  57 +++++++++++++
>>>>>   tests/qemu-iotests/024.out |  30 +++++++
>>>>>   tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
>>>>>   tests/qemu-iotests/314.out |  75 +++++++++++++++++
>>>>>   7 files changed, 415 insertions(+), 28 deletions(-)
>>>>>   create mode 100755 tests/qemu-iotests/314
>>>>>   create mode 100644 tests/qemu-iotests/314.out
>>>>>
>>>> ping
>>>
>>> Friendly ping after 7 weeks
>>
>> Yet another ping
> 
> One more ping after 12 weeks of silence

One more friendly ping

Re: [PATCH 3/6] qemu-img: rebase: use backing files' BlockBackend for buffer alignment

[Hanna Czenczek]

On 01.06.23 21:28, Andrey Drobyshev via wrote:
> Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for
> the data read from the old and new backing files are aligned using
> BlockDriverState (or BlockBackend later on) referring to the target image.
> However, this isn't quite right, because target image is only being
> written to and has nothing to do with those buffers.  Let's fix that.

I don’t understand.  The write to the target image does use one of those 
buffers (buf_old, specifically).

This change is correct for buf_new/blk_new_backing, but for buf_old, in 
theory, we need a buffer that fulfills both the alignment requirements 
of blk and blk_old_backing.  (Not that this patch really makes the 
situation worse for buf_old.)

Hanna

Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file

[Hanna Czenczek]

On 01.06.23 21:28, Andrey Drobyshev via wrote:
> In case when we're rebasing within one backing chain, and when target image
> is larger than old backing file, bdrv_is_allocated_above() ends up setting
> *pnum = 0.  As a result, target offset isn't getting incremented, and we
> get stuck in an infinite for loop.  Let's detect this case and proceed
> further down the loop body, as the offsets beyond the old backing size need
> to be explicitly zeroed.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   qemu-img.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 2/6] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size

[Hanna Czenczek]

On 01.06.23 21:28, Andrey Drobyshev via wrote:
> Before previous commit, rebase was getting infitely stuck in case of
> rebasing within the same backing chain and when overlay_size > backing_size.
> Let's add this case to the rebasing test 024 to make sure it doesn't
> break again.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   tests/qemu-iotests/024     | 57 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/024.out | 30 ++++++++++++++++++++
>   2 files changed, 87 insertions(+)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 4/6] qemu-img: rebase: avoid unnecessary COW operations

[Hanna Czenczek]

On 01.06.23 21:28, Andrey Drobyshev via wrote:
> When rebasing an image from one backing file to another, we need to
> compare data from old and new backings.  If the diff between that data
> happens to be unaligned to the target cluster size, we might end up
> doing partial writes, which would lead to copy-on-write and additional IO.
>
> Consider the following simple case (virtual_size == cluster_size == 64K):
>
> base <-- inc1 <-- inc2
>
> qemu-io -c "write -P 0xaa 0 32K" base.qcow2
> qemu-io -c "write -P 0xcc 32K 32K" base.qcow2
> qemu-io -c "write -P 0xbb 0 32K" inc1.qcow2
> qemu-io -c "write -P 0xcc 32K 32K" inc1.qcow2
> qemu-img rebase -f qcow2 -b base.qcow2 -F qcow2 inc2.qcow2
>
> While doing rebase, we'll write a half of the cluster to inc2, and block
> layer will have to read the 2nd half of the same cluster from the base image
> inc1 while doing this write operation, although the whole cluster is already
> read earlier to perform data comparison.
>
> In order to avoid these unnecessary IO cycles, let's make sure every
> write request is aligned to the overlay cluster size.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   qemu-img.c | 72 +++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 52 insertions(+), 20 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 60f4c06487..9a469cd609 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3513,6 +3513,7 @@ static int img_rebase(int argc, char **argv)
>       uint8_t *buf_new = NULL;
>       BlockDriverState *bs = NULL, *prefix_chain_bs = NULL;
>       BlockDriverState *unfiltered_bs;
> +    BlockDriverInfo bdi = {0};
>       char *filename;
>       const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
>       int c, flags, src_flags, ret;
> @@ -3646,6 +3647,15 @@ static int img_rebase(int argc, char **argv)
>           }
>       }
>   
> +    /* We need overlay cluster size to make sure write requests are aligned */
> +    ret = bdrv_get_info(unfiltered_bs, &bdi);
> +    if (ret < 0) {
> +        error_report("could not get block driver info");
> +        goto out;
> +    } else if (bdi.cluster_size == 0) {
> +        bdi.cluster_size = 1;
> +    }
> +
>       /* For safe rebasing we need to compare old and new backing file */
>       if (!unsafe) {
>           QDict *options = NULL;
> @@ -3744,6 +3754,7 @@ static int img_rebase(int argc, char **argv)
>           int64_t new_backing_size = 0;
>           uint64_t offset;
>           int64_t n;
> +        int64_t n_old = 0, n_new = 0;
>           float local_progress = 0;
>   
>           buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
> @@ -3784,7 +3795,7 @@ static int img_rebase(int argc, char **argv)
>           }
>   
>           for (offset = 0; offset < size; offset += n) {
> -            bool buf_old_is_zero = false;
> +            bool old_backing_eof = false;
>   
>               /* How many bytes can we handle with the next read? */
>               n = MIN(IO_BUF_SIZE, size - offset);
> @@ -3829,33 +3840,38 @@ static int img_rebase(int argc, char **argv)
>                   }
>               }
>   
> +            /* At this point n must be aligned to the target cluster size. */
> +            if (offset + n < size) {
> +                assert(n % bdi.cluster_size == 0);

This is not correct.  First, bdrv_is_allocated_above() operates not on 
the top image, but on images in the backing chain, which may have 
different cluster sizes and so may lead to `n`s that are not aligned to 
the top image’s cluster size:

$ ./qemu-img create -f qcow2 base.qcow2 64M
$ ./qemu-img create -f qcow2 -b base.qcow2 -F qcow2 mid.qcow2 64M
$ ./qemu-img create -f qcow2 -o cluster_size=2M -b mid.qcow2 -F qcow2 
top.qcow2 64M
$ ./qemu-io -c 'write 64k 64k' mid.qcow2
$ ./qemu-img rebase -b base.qcow2 top.qcow2
qemu-img: ../qemu-img.c:3845: img_rebase: Assertion `n % 
bdi.cluster_size == 0' failed.
[1]    636690 IOT instruction (core dumped)  ./qemu-img rebase -b 
base.qcow2 top.qcow2

Second, and this is a more theoretical thing, it would also be broken 
for images with cluster sizes greater than IO_BUF_SIZE.  Now, 
IO_BUF_SIZE is 2 MB, which happens to be precisely the maximum cluster 
size we support for qcow2, and for vmdk we always create images with 64 
kB clusters (I believe), but the vmdk code seems happy to open 
pre-existing images with cluster sizes up to 512 MB. Still, even for 
qcow2, we could easily increase the limit from 2 MB at any point, and 
there is no explicit correlation why IO_BUF_SIZE happens to be exactly 
what the current maximum cluster size for qcow2 is.  One way to get 
around this would be to use MAX(IO_BUF_SIZE, bdi.cluster_size) for the 
buffer size, which would give such an explicit correlation.

> +            }
> +
> +            /*
> +             * Much like the with the target image, we'll try to read as much
> +             * of the old and new backings as we can.
> +             */
> +            n_old = MIN(n, MAX(0, old_backing_size - (int64_t) offset));
> +            if (blk_new_backing) {
> +                n_new = MIN(n, MAX(0, new_backing_size - (int64_t) offset));
> +            }
> +
>               /*
>                * Read old and new backing file and take into consideration that
>                * backing files may be smaller than the COW image.
>                */
> -            if (offset >= old_backing_size) {
> -                memset(buf_old, 0, n);
> -                buf_old_is_zero = true;
> +            memset(buf_old + n_old, 0, n - n_old);
> +            if (!n_old) {
> +                old_backing_eof = true;
>               } else {
> -                if (offset + n > old_backing_size) {
> -                    n = old_backing_size - offset;
> -                }
> -
> -                ret = blk_pread(blk_old_backing, offset, n, buf_old, 0);
> +                ret = blk_pread(blk_old_backing, offset, n_old, buf_old, 0);
>                   if (ret < 0) {
>                       error_report("error while reading from old backing file");
>                       goto out;
>                   }
>               }
>   
> -            if (offset >= new_backing_size || !blk_new_backing) {
> -                memset(buf_new, 0, n);
> -            } else {
> -                if (offset + n > new_backing_size) {
> -                    n = new_backing_size - offset;
> -                }
> -
> -                ret = blk_pread(blk_new_backing, offset, n, buf_new, 0);
> +            memset(buf_new + n_new, 0, n - n_new);
> +            if (blk_new_backing && n_new) {
> +                ret = blk_pread(blk_new_backing, offset, n_new, buf_new, 0);
>                   if (ret < 0) {
>                       error_report("error while reading from new backing file");
>                       goto out;
> @@ -3867,15 +3883,28 @@ static int img_rebase(int argc, char **argv)
>   
>               while (written < n) {
>                   int64_t pnum;
> +                int64_t start, end;
>   
>                   if (compare_buffers(buf_old + written, buf_new + written,
>                                       n - written, &pnum))
>                   {
> -                    if (buf_old_is_zero) {
> +                    if (old_backing_eof) {
>                           ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
>                       } else {
> -                        ret = blk_pwrite(blk, offset + written, pnum,
> -                                         buf_old + written, 0);
> +                        /*
> +                         * If we've got to this point, it means the cluster
> +                         * we're dealing with is unallocated, and any partial
> +                         * write will cause COW.  To avoid that, we make sure
> +                         * request is aligned to cluster size.
> +                         */
> +                        start = QEMU_ALIGN_DOWN(offset + written,
> +                                                bdi.cluster_size);

Please add an assertion here that `start >= offset`.  I would rather 
have qemu-img crash than to write out-of-bounds memory data to disk.

I understand the idea is that this is given anyway because `offset` 
starts at 0 and we always check that `n`, by which we increment 
`offset`, is aligned, but it is absolutely critical that we don’t do an 
out-of-bounds access, so I feel an explicit assertion here is warranted.

> +                        end = QEMU_ALIGN_UP(offset + written + pnum,
> +                                            bdi.cluster_size);

Similarly here, please assert that `end - offset` this does not exceed 
the buffer’s bounds.  I know the reasoning is the same, we ensured that 
`n` is aligned, so we can always safely align up `written + pnum`, but 
still.

Hanna

> +                        end = end > size ? size : end;
> +                        ret = blk_pwrite(blk, start, end - start,
> +                                         buf_old + (start - offset), 0);
> +                        pnum = end - (offset + written);
>                       }
>                       if (ret < 0) {
>                           error_report("Error while writing to COW image: %s",
> @@ -3885,6 +3914,9 @@ static int img_rebase(int argc, char **argv)
>                   }
>   
>                   written += pnum;
> +                if (offset + written >= old_backing_size) {
> +                    old_backing_eof = true;
> +                }
>               }
>               qemu_progress_print(local_progress, 100);
>           }

Re: [PATCH 5/6] qemu-img: add compression option to rebase subcommand

[Hanna Czenczek]

On 01.06.23 21:28, Andrey Drobyshev via wrote:
> If we rebase an image whose backing file has compressed clusters, we
> might end up wasting disk space since the copied clusters are now
> uncompressed.  In order to have better control over this, let's add
> "--compress" option to the "qemu-img rebase" command.
>
> Note that this option affects only the clusters which are actually being
> copied from the original backing file.  The clusters which were
> uncompressed in the target image will remain so.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   docs/tools/qemu-img.rst |  6 ++++--
>   qemu-img-cmds.hx        |  4 ++--
>   qemu-img.c              | 19 +++++++++++++++++--
>   3 files changed, 23 insertions(+), 6 deletions(-)

Interesting.  I was about to protest because we only really support 
writing compressed clusters to new and empty images, so the qcow2 driver 
does not allow overwriting existing clusters with compressed data.  But 
by design we skip all clusters that are anything but unallocated in the 
top image (i.e. the one we are going to write to), so this should indeed 
work out well.

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 15aeddc6d8..973a912dec 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -663,7 +663,7 @@ Command description:
>   
>     List, apply, create or delete snapshots in image *FILENAME*.
>   
> -.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] -b BACKING_FILE [-F BACKING_FMT] FILENAME
> +.. option:: rebase [--object OBJECTDEF] [--image-opts] [-U] [-q] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-p] [-u] [-c] -b BACKING_FILE [-F BACKING_FMT] FILENAME
>   
>     Changes the backing file of an image. Only the formats ``qcow2`` and
>     ``qed`` support changing the backing file.
> @@ -690,7 +690,9 @@ Command description:
>   
>       In order to achieve this, any clusters that differ between
>       *BACKING_FILE* and the old backing file of *FILENAME* are merged
> -    into *FILENAME* before actually changing the backing file.
> +    into *FILENAME* before actually changing the backing file. With ``-c``

“With the ``-c`` option specified, [...]”

> +    option specified, the clusters which are being merged (but not the
> +    entire *FILENAME* image) are written in the compressed mode.

“[...] are compressed when written.”

>       Note that the safe mode is an expensive operation, comparable to
>       converting an image. It only works if the old backing file still

Re: [PATCH 6/6] iotests: add test 314 for "qemu-img rebase" with compression

[Hanna Czenczek]

On 01.06.23 21:28, Andrey Drobyshev via wrote:
> The test cases considered so far:
>
> 1. Check that compression mode isn't compatible with "-f raw" (raw
>     format doesn't support compression).
> 2. Check that rebasing an image onto no backing file preserves the data
>     and writes the copied clusters actually compressed.
> 3. Same as 2, but with a raw backing file (i.e. the clusters copied from the
>     backing are originally uncompressed -- we check they end up compressed
>     after being merged).
> 4. Remove a single delta from a backing chain, perform the same checks
>     as in 2.
> 5. Check that even when backing and overlay are initially uncompressed,
>     copied clusters end up compressed when rebase with compression is
>     performed.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   tests/qemu-iotests/314     | 165 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/314.out |  75 +++++++++++++++++
>   2 files changed, 240 insertions(+)
>   create mode 100755 tests/qemu-iotests/314
>   create mode 100644 tests/qemu-iotests/314.out

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 3/6] qemu-img: rebase: use backing files' BlockBackend for buffer alignment

[Andrey Drobyshev]

On 8/25/23 17:29, Hanna Czenczek wrote:
> On 01.06.23 21:28, Andrey Drobyshev via wrote:
>> Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for
>> the data read from the old and new backing files are aligned using
>> BlockDriverState (or BlockBackend later on) referring to the target
>> image.
>> However, this isn't quite right, because target image is only being
>> written to and has nothing to do with those buffers.  Let's fix that.
> 
> I don’t understand.  The write to the target image does use one of those
> buffers (buf_old, specifically).
> 
> This change is correct for buf_new/blk_new_backing, but for buf_old, in
> theory, we need a buffer that fulfills both the alignment requirements
> of blk and blk_old_backing.  (Not that this patch really makes the
> situation worse for buf_old.)
> 
> Hanna
> 

Hmm, you're right.  In which case the right thing to do would probably
be smth like:

>          float local_progress = 0;
>  
> -        buf_old = blk_blockalign(blk, IO_BUF_SIZE);
> -        buf_new = blk_blockalign(blk, IO_BUF_SIZE);
> +        if (bdrv_opt_mem_align(blk_bs(blk)) >
> +            bdrv_opt_mem_align(blk_bs(blk_old_backing))) {
> +            buf_old = blk_blockalign(blk, IO_BUF_SIZE);
> +        } else {
> +            buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
> +        }
> +        buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE);
>  
>          size = blk_getlength(blk);

I'll include this in v2 if you don't have any objections.

Re: [PATCH 3/6] qemu-img: rebase: use backing files' BlockBackend for buffer alignment

[Hanna Czenczek]

On 29.08.23 09:06, Andrey Drobyshev wrote:
> On 8/25/23 17:29, Hanna Czenczek wrote:
>> On 01.06.23 21:28, Andrey Drobyshev via wrote:
>>> Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for
>>> the data read from the old and new backing files are aligned using
>>> BlockDriverState (or BlockBackend later on) referring to the target
>>> image.
>>> However, this isn't quite right, because target image is only being
>>> written to and has nothing to do with those buffers.  Let's fix that.
>> I don’t understand.  The write to the target image does use one of those
>> buffers (buf_old, specifically).
>>
>> This change is correct for buf_new/blk_new_backing, but for buf_old, in
>> theory, we need a buffer that fulfills both the alignment requirements
>> of blk and blk_old_backing.  (Not that this patch really makes the
>> situation worse for buf_old.)
>>
>> Hanna
>>
> Hmm, you're right.  In which case the right thing to do would probably
> be smth like:
>
>>           float local_progress = 0;
>>   
>> -        buf_old = blk_blockalign(blk, IO_BUF_SIZE);
>> -        buf_new = blk_blockalign(blk, IO_BUF_SIZE);
>> +        if (bdrv_opt_mem_align(blk_bs(blk)) >
>> +            bdrv_opt_mem_align(blk_bs(blk_old_backing))) {
>> +            buf_old = blk_blockalign(blk, IO_BUF_SIZE);
>> +        } else {
>> +            buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
>> +        }
>> +        buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE);
>>   
>>           size = blk_getlength(blk);
> I'll include this in v2 if you don't have any objections.

Looks good to me, thanks!

Re: [PATCH 4/6] qemu-img: rebase: avoid unnecessary COW operations

[Andrey Drobyshev]

On 8/25/23 18:00, Hanna Czenczek wrote:
> On 01.06.23 21:28, Andrey Drobyshev via wrote:
>> When rebasing an image from one backing file to another, we need to
>> compare data from old and new backings.  If the diff between that data
>> happens to be unaligned to the target cluster size, we might end up
>> doing partial writes, which would lead to copy-on-write and additional
>> IO.
>>
>> Consider the following simple case (virtual_size == cluster_size == 64K):
>>
>> base <-- inc1 <-- inc2
>>
>> qemu-io -c "write -P 0xaa 0 32K" base.qcow2
>> qemu-io -c "write -P 0xcc 32K 32K" base.qcow2
>> qemu-io -c "write -P 0xbb 0 32K" inc1.qcow2
>> qemu-io -c "write -P 0xcc 32K 32K" inc1.qcow2
>> qemu-img rebase -f qcow2 -b base.qcow2 -F qcow2 inc2.qcow2
>>
>> While doing rebase, we'll write a half of the cluster to inc2, and block
>> layer will have to read the 2nd half of the same cluster from the base
>> image
>> inc1 while doing this write operation, although the whole cluster is
>> already
>> read earlier to perform data comparison.
>>
>> In order to avoid these unnecessary IO cycles, let's make sure every
>> write request is aligned to the overlay cluster size.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>   qemu-img.c | 72 +++++++++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 52 insertions(+), 20 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 60f4c06487..9a469cd609 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>>>> [...]
>>>>               }
>>   +            /* At this point n must be aligned to the target
>> cluster size. */
>> +            if (offset + n < size) {
>> +                assert(n % bdi.cluster_size == 0);
> 
> This is not correct.  First, bdrv_is_allocated_above() operates not on
> the top image, but on images in the backing chain, which may have
> different cluster sizes and so may lead to `n`s that are not aligned to
> the top image’s cluster size:
> 
> $ ./qemu-img create -f qcow2 base.qcow2 64M
> $ ./qemu-img create -f qcow2 -b base.qcow2 -F qcow2 mid.qcow2 64M
> $ ./qemu-img create -f qcow2 -o cluster_size=2M -b mid.qcow2 -F qcow2
> top.qcow2 64M
> $ ./qemu-io -c 'write 64k 64k' mid.qcow2
> $ ./qemu-img rebase -b base.qcow2 top.qcow2
> qemu-img: ../qemu-img.c:3845: img_rebase: Assertion `n %
> bdi.cluster_size == 0' failed.
> [1]    636690 IOT instruction (core dumped)  ./qemu-img rebase -b
> base.qcow2 top.qcow2
> 
> Second, and this is a more theoretical thing, it would also be broken
> for images with cluster sizes greater than IO_BUF_SIZE.  Now,
> IO_BUF_SIZE is 2 MB, which happens to be precisely the maximum cluster
> size we support for qcow2, and for vmdk we always create images with 64
> kB clusters (I believe), but the vmdk code seems happy to open
> pre-existing images with cluster sizes up to 512 MB. Still, even for
> qcow2, we could easily increase the limit from 2 MB at any point, and
> there is no explicit correlation why IO_BUF_SIZE happens to be exactly
> what the current maximum cluster size for qcow2 is.  One way to get
> around this would be to use MAX(IO_BUF_SIZE, bdi.cluster_size) for the
> buffer size, which would give such an explicit correlation.
> 

I'm not sure whether blunt allocation of buffers up to 512M is the right
thing to do.  Since we need our buffers to be equal in size, we'd have
to take MAX(old backing cluster size, new backing cluster size, target
cluster size).  As for potential increase of qcow2 cluster size, I'd
simply increase IO_BUF_SIZE accordingly once it happens.

Overall, your first point is enough to drop simply drop that assert.

However, your remark gave me another idea that there's actually the 3rd
case when it gets broken, and that is images with subclusters, since in
this case bdrv_is_allocated_above() will align n to the subcluster size.
 While looking into what exactly qcow2_co_block_status() reports I
realized that my patch breaks the following:

> qemu-img create -f qcow2 -o cluster_size=1M base.qcow2 1M
> qemu-img create -f qcow2 -b base.qcow2 -F qcow2 -o cluster_size=1M,extended_l2=on inc1.qcow2 1M
> qemu-img create -f qcow2 -b inc1.qcow2 -F qcow2 -o cluster_size=1M inc2.qcow2 1M
> qemu-io -c 'write -P 0xaa 0 32K' -c 'write -P 0xbb 64K 32K' inc1.qcow2
> qemu-img rebase -b base.qcow2 -F qcow2 inc2.qcow2
> qemu-io -c "read -P 0xaa 0 32K" -c "read -P 0xbb 64K 32K" inc2.qcow2
>> read 32768/32768 bytes at offset 0
> 32 KiB, 1 ops; 00.00 sec (78.511 MiB/sec and 2512.3671 ops/sec)
> Pattern verification failed at offset 65536, 32768 bytes
> read 32768/32768 bytes at offset 65536
> 32 KiB, 1 ops; 00.00 sec (490.381 MiB/sec and 15692.1822 ops/sec)

That happens because n_old is bounded by n and we read not too small
data chunk (1st subcluster only).  Since we end up writing whole
clusters to the target anyway, the solution would probably be rounding n
up to the cluster size right after the call to bdrv_is_allocated_above():

>             if (prefix_chain_bs) {
>                 uint64_t bytes = n;
>             ...
>             }
> 
>             n = MIN(QEMU_ALIGN_UP(n, bdi.cluster_size), size - offset);
>
Now, if the target also has subclusters, we might end up allocating more
disk space than necessary (i.e. writing whole cluster instead of several
separate subclusters).  I'm not sure whether we should consider this as
well (aligning n to subcluster size?) or leave it as is keeping in mind
the trade-off between disk space and IO ops.

In any case I'll add the above scenario to the tests.

>
> [...]
>>> +                         */
>> +                        start = QEMU_ALIGN_DOWN(offset + written,
>> +                                                bdi.cluster_size);
> 
> Please add an assertion here that `start >= offset`.  I would rather
> have qemu-img crash than to write out-of-bounds memory data to disk.
> 
> I understand the idea is that this is given anyway because `offset`
> starts at 0 and we always check that `n`, by which we increment
> `offset`, is aligned, but it is absolutely critical that we don’t do an
> out-of-bounds access, so I feel an explicit assertion here is warranted.
> 
>> +                        end = QEMU_ALIGN_UP(offset + written + pnum,
>> +                                            bdi.cluster_size);
> 
> Similarly here, please assert that `end - offset` this does not exceed
> the buffer’s bounds.  I know the reasoning is the same, we ensured that
> `n` is aligned, so we can always safely align up `written + pnum`, but
> still.
> 

Agreed. Smth like:

>                          end = QEMU_ALIGN_UP(offset + written + pnum,
>                                              bdi.cluster_size);
>                          end = end > size ? size : end;
> +                        assert(offset <= start && start < end &&
> +                               end <= offset + IO_BUF_SIZE);
>                          ret = blk_pwrite(blk, start, end - start,
>                                           buf_old + (start - offset),
>                                           write_flags);

Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file

[Michael Tokarev]

01.06.2023 22:28, Andrey Drobyshev via:
> In case when we're rebasing within one backing chain, and when target image
> is larger than old backing file, bdrv_is_allocated_above() ends up setting
> *pnum = 0.  As a result, target offset isn't getting incremented, and we
> get stuck in an infinite for loop.  Let's detect this case and proceed
> further down the loop body, as the offsets beyond the old backing size need
> to be explicitly zeroed.

Ping? Has this been forgotten? It's a few months already..

/mjt

Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file

[Andrey Drobyshev]

On 10/26/23 09:32, Michael Tokarev wrote:
> 01.06.2023 22:28, Andrey Drobyshev via:
>> In case when we're rebasing within one backing chain, and when target
>> image
>> is larger than old backing file, bdrv_is_allocated_above() ends up
>> setting
>> *pnum = 0.  As a result, target offset isn't getting incremented, and we
>> get stuck in an infinite for loop.  Let's detect this case and proceed
>> further down the loop body, as the offsets beyond the old backing size
>> need
>> to be explicitly zeroed.
> 
> Ping? Has this been forgotten? It's a few months already..
> 
> /mjt

Hi Michael,

It's not forgotten, there's already v3 of this series and it's already
taken to the block branch by Kevin:

https://lists.nongnu.org/archive/html/qemu-block/2023-09/msg00593.html

Andrey