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

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

[Max Reitz]

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

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

Yeah, well, OK.

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

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


And patch 3 adds the usual test.


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

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

-- 
2.17.1

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

[Max Reitz]

Currently, you cannot add a backing file to an image when it currently
has none.  It is really simple to allow this, though (effectively by
setting old_backing_size to 0), so this patch does just that.

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

diff --git a/qemu-img.c b/qemu-img.c
index 9b7506b8ae..dd684d8bf0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3280,26 +3280,30 @@ static int img_rebase(int argc, char **argv)
         char backing_name[PATH_MAX];
         QDict *options = NULL;
 
-        if (bs->backing_format[0] != '\0') {
-            options = qdict_new();
-            qdict_put_str(options, "driver", bs->backing_format);
-        }
-
-        if (force_share) {
-            if (!options) {
+        if (bs->backing) {
+            if (bs->backing_format[0] != '\0') {
                 options = qdict_new();
+                qdict_put_str(options, "driver", bs->backing_format);
             }
-            qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
-        }
-        bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-        blk_old_backing = blk_new_open(backing_name, NULL,
-                                       options, src_flags, &local_err);
-        if (!blk_old_backing) {
-            error_reportf_err(local_err,
-                              "Could not open old backing file '%s': ",
-                              backing_name);
-            ret = -1;
-            goto out;
+
+            if (force_share) {
+                if (!options) {
+                    options = qdict_new();
+                }
+                qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
+            }
+            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
+            blk_old_backing = blk_new_open(backing_name, NULL,
+                                           options, src_flags, &local_err);
+            if (!blk_old_backing) {
+                error_reportf_err(local_err,
+                                  "Could not open old backing file '%s': ",
+                                  backing_name);
+                ret = -1;
+                goto out;
+            }
+        } else {
+            blk_old_backing = NULL;
         }
 
         if (out_baseimg[0]) {
@@ -3355,7 +3359,7 @@ static int img_rebase(int argc, char **argv)
      */
     if (!unsafe) {
         int64_t size;
-        int64_t old_backing_size;
+        int64_t old_backing_size = 0;
         int64_t new_backing_size = 0;
         uint64_t offset;
         int64_t n;
@@ -3371,15 +3375,18 @@ static int img_rebase(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        old_backing_size = blk_getlength(blk_old_backing);
-        if (old_backing_size < 0) {
-            char backing_name[PATH_MAX];
+        if (blk_old_backing) {
+            old_backing_size = blk_getlength(blk_old_backing);
+            if (old_backing_size < 0) {
+                char backing_name[PATH_MAX];
 
-            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-            error_report("Could not get size of '%s': %s",
-                         backing_name, strerror(-old_backing_size));
-            ret = -1;
-            goto out;
+                bdrv_get_backing_filename(bs, backing_name,
+                                          sizeof(backing_name));
+                error_report("Could not get size of '%s': %s",
+                             backing_name, strerror(-old_backing_size));
+                ret = -1;
+                goto out;
+            }
         }
         if (blk_new_backing) {
             new_backing_size = blk_getlength(blk_new_backing);
-- 
2.17.1

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

[Max Reitz]

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

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

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

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

[Max Reitz]

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

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

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

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

[Eric Blake]

On 07/13/2018 06:14 AM, Max Reitz wrote:
> Past the end of the source backing file, we memset() buf_old to zero, so
> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
> then.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qemu-img.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index dd684d8bf0..2552e7dad6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3403,6 +3403,8 @@ static int img_rebase(int argc, char **argv)
>           }
>   
>           for (offset = 0; offset < size; offset += n) {
> +            bool buf_old_is_zero = false;
> +
>               /* How many bytes can we handle with the next read? */
>               n = MIN(IO_BUF_SIZE, size - offset);
>   
> @@ -3423,6 +3425,7 @@ static int img_rebase(int argc, char **argv)
>                */
>               if (offset >= old_backing_size) {
>                   memset(buf_old, 0, n);
> +                buf_old_is_zero = true;

Do we still need to spend time on the memset(), or...

>               } else {
>                   if (offset + n > old_backing_size) {
>                       n = old_backing_size - offset;
> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
>                   if (compare_buffers(buf_old + written, buf_new + written,
>                                       n - written, &pnum))
>                   {
> -                    ret = blk_pwrite(blk, offset + written,
> -                                     buf_old + written, pnum, 0);
> +                    if (buf_old_is_zero) {
> +                        ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);

...are we able to guarantee that old_buf will not be used when 
buf_old_is_zero?

> +                    } else {
> +                        ret = blk_pwrite(blk, offset + written,
> +                                         buf_old + written, pnum, 0);
> +                    }
>                       if (ret < 0) {
>                           error_report("Error while writing to COW image: %s",
>                               strerror(-ret));
> 

The series seems reasonable, but is post-3.0 material, so I haven't 
reviewed it any closer than this question.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Max Reitz]

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

On 2018-07-20 23:22, Eric Blake wrote:
> On 07/13/2018 06:14 AM, Max Reitz wrote:
>> Past the end of the source backing file, we memset() buf_old to zero, so
>> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
>> then.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-img.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index dd684d8bf0..2552e7dad6 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -3403,6 +3403,8 @@ static int img_rebase(int argc, char **argv)
>>           }
>>             for (offset = 0; offset < size; offset += n) {
>> +            bool buf_old_is_zero = false;
>> +
>>               /* How many bytes can we handle with the next read? */
>>               n = MIN(IO_BUF_SIZE, size - offset);
>>   @@ -3423,6 +3425,7 @@ static int img_rebase(int argc, char **argv)
>>                */
>>               if (offset >= old_backing_size) {
>>                   memset(buf_old, 0, n);
>> +                buf_old_is_zero = true;
> 
> Do we still need to spend time on the memset(), or...
> 
>>               } else {
>>                   if (offset + n > old_backing_size) {
>>                       n = old_backing_size - offset;
>> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
>>                   if (compare_buffers(buf_old + written, buf_new +
>> written,
>>                                       n - written, &pnum))
>>                   {
>> -                    ret = blk_pwrite(blk, offset + written,
>> -                                     buf_old + written, pnum, 0);
>> +                    if (buf_old_is_zero) {
>> +                        ret = blk_pwrite_zeroes(blk, offset +
>> written, pnum, 0);
> 
> ...are we able to guarantee that old_buf will not be used when
> buf_old_is_zero?

It will certainly be used, as it is used in the compare_buffers() call.

It could be optimized, but I fear that may lead down a small rabbit hole
(we could then also get the block status of the target backing file, see
whether it is zero, and so on).  Considering that rebase is probably not
something many people use all the time, I think it’s OK to be slower
than possible for now.  (Until someone complains.)

Max

>> +                    } else {
>> +                        ret = blk_pwrite(blk, offset + written,
>> +                                         buf_old + written, pnum, 0);
>> +                    }
>>                       if (ret < 0) {
>>                           error_report("Error while writing to COW
>> image: %s",
>>                               strerror(-ret));
>>
> 
> The series seems reasonable, but is post-3.0 material, so I haven't
> reviewed it any closer than this question.
> 



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

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

[Max Reitz]

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

Ping

On 13.07.18 13:14, Max Reitz wrote:
> This series allows using qemu-img rebase on images that do not have a
> backing file.  Right now, this fails with the rather cryptic error
> message:
> 
> $ qemu-img rebase -b base.qcow2 foo.qcow2
> qemu-img: Could not open old backing file '': The 'file' block driver requires a file name
> 
> Yeah, well, OK.
> 
> With how rebase currently works, this would lead to the overlay being
> filled with zeroes, however.  This is where patch 2 comes in and instead
> makes rebase use blk_pwrite_zeroes() whenever it handles an area past
> the input’s backing file’s EOF.
> 
> (Note that additionally we could try to punch holes in the overlay
> whenever it matches the new backing file, but that’s something I’ll put
> off for later.  (We don’t even have a reliable method for punching holes
> into an overlay yet, although I would like to have such because it could
> make active commit more efficient.))
> 
> 
> And patch 3 adds the usual test.
> 
> 
> Max Reitz (3):
>   qemu-img: Allow rebase with no input base
>   qemu-img: Use zero writes after source backing EOF
>   iotests: Add test for rebase without input base
> 
>  qemu-img.c                 | 72 +++++++++++++++++++++++---------------
>  tests/qemu-iotests/024     | 70 ++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/024.out | 37 ++++++++++++++++++++
>  3 files changed, 150 insertions(+), 29 deletions(-)
> 



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

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

[Max Reitz]

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

Ping again

(I feel like I just need to start merging unreviewed patches until I
break something (can't take that long) so you get so scared of my
patches that you at least refuse them outright)

On 13.07.18 13:14, Max Reitz wrote:
> This series allows using qemu-img rebase on images that do not have a
> backing file.  Right now, this fails with the rather cryptic error
> message:
> 
> $ qemu-img rebase -b base.qcow2 foo.qcow2
> qemu-img: Could not open old backing file '': The 'file' block driver requires a file name
> 
> Yeah, well, OK.
> 
> With how rebase currently works, this would lead to the overlay being
> filled with zeroes, however.  This is where patch 2 comes in and instead
> makes rebase use blk_pwrite_zeroes() whenever it handles an area past
> the input’s backing file’s EOF.
> 
> (Note that additionally we could try to punch holes in the overlay
> whenever it matches the new backing file, but that’s something I’ll put
> off for later.  (We don’t even have a reliable method for punching holes
> into an overlay yet, although I would like to have such because it could
> make active commit more efficient.))
> 
> 
> And patch 3 adds the usual test.
> 
> 
> Max Reitz (3):
>   qemu-img: Allow rebase with no input base
>   qemu-img: Use zero writes after source backing EOF
>   iotests: Add test for rebase without input base
> 
>  qemu-img.c                 | 72 +++++++++++++++++++++++---------------
>  tests/qemu-iotests/024     | 70 ++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/024.out | 37 ++++++++++++++++++++
>  3 files changed, 150 insertions(+), 29 deletions(-)
> 



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

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

[Max Reitz]

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

Ping again

(I feel like I just need to start merging unreviewed patches until I
break something (can't take that long) so you get so scared of my
patches that you at least refuse them outright)

On 13.07.18 13:14, Max Reitz wrote:
> This series allows using qemu-img rebase on images that do not have a
> backing file.  Right now, this fails with the rather cryptic error
> message:
> 
> $ qemu-img rebase -b base.qcow2 foo.qcow2
> qemu-img: Could not open old backing file '': The 'file' block driver requires a file name
> 
> Yeah, well, OK.
> 
> With how rebase currently works, this would lead to the overlay being
> filled with zeroes, however.  This is where patch 2 comes in and instead
> makes rebase use blk_pwrite_zeroes() whenever it handles an area past
> the input’s backing file’s EOF.
> 
> (Note that additionally we could try to punch holes in the overlay
> whenever it matches the new backing file, but that’s something I’ll put
> off for later.  (We don’t even have a reliable method for punching holes
> into an overlay yet, although I would like to have such because it could
> make active commit more efficient.))
> 
> 
> And patch 3 adds the usual test.
> 
> 
> Max Reitz (3):
>   qemu-img: Allow rebase with no input base
>   qemu-img: Use zero writes after source backing EOF
>   iotests: Add test for rebase without input base
> 
>  qemu-img.c                 | 72 +++++++++++++++++++++++---------------
>  tests/qemu-iotests/024     | 70 ++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/024.out | 37 ++++++++++++++++++++
>  3 files changed, 150 insertions(+), 29 deletions(-)
> 



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

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

[Vladimir Sementsov-Ogievskiy]

13.04.2019 19:11, Max Reitz wrote:
> Ping again
> 
> (I feel like I just need to start merging unreviewed patches until I
> break something (can't take that long) so you get so scared of my
> patches that you at least refuse them outright)

Could you resend before?

> 
> On 13.07.18 13:14, Max Reitz wrote:
>> This series allows using qemu-img rebase on images that do not have a
>> backing file.  Right now, this fails with the rather cryptic error
>> message:
>>
>> $ qemu-img rebase -b base.qcow2 foo.qcow2
>> qemu-img: Could not open old backing file '': The 'file' block driver requires a file name
>>
>> Yeah, well, OK.
>>
>> With how rebase currently works, this would lead to the overlay being
>> filled with zeroes, however.  This is where patch 2 comes in and instead
>> makes rebase use blk_pwrite_zeroes() whenever it handles an area past
>> the input’s backing file’s EOF.
>>
>> (Note that additionally we could try to punch holes in the overlay
>> whenever it matches the new backing file, but that’s something I’ll put
>> off for later.  (We don’t even have a reliable method for punching holes
>> into an overlay yet, although I would like to have such because it could
>> make active commit more efficient.))
>>
>>
>> And patch 3 adds the usual test.
>>
>>
>> Max Reitz (3):
>>    qemu-img: Allow rebase with no input base
>>    qemu-img: Use zero writes after source backing EOF
>>    iotests: Add test for rebase without input base
>>
>>   qemu-img.c                 | 72 +++++++++++++++++++++++---------------
>>   tests/qemu-iotests/024     | 70 ++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/024.out | 37 ++++++++++++++++++++
>>   3 files changed, 150 insertions(+), 29 deletions(-)
>>
> 
> 


-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 7/13/18 6:14 AM, Max Reitz wrote:
> Currently, you cannot add a backing file to an image when it currently
> has none.  It is really simple to allow this, though (effectively by
> setting old_backing_size to 0), so this patch does just that.

Can't you do that with 'rebase -u'? I guess the point of this patch is
to make 'rebase' without -u also able to add a backing file?  A bit more
clarity in the commit message, such as a sample command line (and
possibly even the error it issued pre-patch) would help.

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

At any rate, the code looks fine, even if the diff is harder to read
because of added indentation.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

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

[Eric Blake]

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

On 7/13/18 6:14 AM, Max Reitz wrote:
> Past the end of the source backing file, we memset() buf_old to zero, so
> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
> then.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 

> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
>                  if (compare_buffers(buf_old + written, buf_new + written,
>                                      n - written, &pnum))
>                  {
> -                    ret = blk_pwrite(blk, offset + written,
> -                                     buf_old + written, pnum, 0);
> +                    if (buf_old_is_zero) {
> +                        ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);

Should we allow BDRV_REQ_MAY_UNMAP here, either unconditionally, or
based on a command line knob that told us whether the user is more
interested in a sparse result (that still reads as zero) vs. a
fully-allocated result?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

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

[Max Reitz]

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

On 18.04.19 20:55, Eric Blake wrote:
> On 7/13/18 6:14 AM, Max Reitz wrote:
>> Currently, you cannot add a backing file to an image when it currently
>> has none.  It is really simple to allow this, though (effectively by
>> setting old_backing_size to 0), so this patch does just that.
> 
> Can't you do that with 'rebase -u'? I guess the point of this patch is
> to make 'rebase' without -u also able to add a backing file?  A bit more
> clarity in the commit message, such as a sample command line (and
> possibly even the error it issued pre-patch) would help.

Yes, this is about normal rebase without -u.

There is an example in the cover letter, but I can add it to this patch
here, too, yes.

>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qemu-img.c | 61 ++++++++++++++++++++++++++++++------------------------
>>  1 file changed, 34 insertions(+), 27 deletions(-)
> 
> At any rate, the code looks fine, even if the diff is harder to read
> because of added indentation.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Max


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

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

[Max Reitz]

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

On 18.04.19 20:59, Eric Blake wrote:
> On 7/13/18 6:14 AM, Max Reitz wrote:
>> Past the end of the source backing file, we memset() buf_old to zero, so
>> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
>> then.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qemu-img.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
> 
>> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
>>                  if (compare_buffers(buf_old + written, buf_new + written,
>>                                      n - written, &pnum))
>>                  {
>> -                    ret = blk_pwrite(blk, offset + written,
>> -                                     buf_old + written, pnum, 0);
>> +                    if (buf_old_is_zero) {
>> +                        ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
> 
> Should we allow BDRV_REQ_MAY_UNMAP here, either unconditionally, or
> based on a command line knob that told us whether the user is more
> interested in a sparse result (that still reads as zero) vs. a
> fully-allocated result?

I wouldn’t trust that.  We haven’t yet switched to the new backing file
at this point, so I think a driver would be correct in handling such
requests by punching a hole in the file -- which would lead to the new
backing file poking through once we’ve switched to it.

Max

> Reviewed-by: Eric Blake <eblake@redhat.com>



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

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

[Max Reitz]

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

On 18.04.19 19:00, Vladimir Sementsov-Ogievskiy wrote:
> 13.04.2019 19:11, Max Reitz wrote:
>> Ping again
>>
>> (I feel like I just need to start merging unreviewed patches until I
>> break something (can't take that long) so you get so scared of my
>> patches that you at least refuse them outright)
> 
> Could you resend before?

Sure, but for me, this series still applies as-is.

Max


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

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

[Vladimir Sementsov-Ogievskiy]

24.04.2019 16:48, Max Reitz wrote:
> On 18.04.19 19:00, Vladimir Sementsov-Ogievskiy wrote:
>> 13.04.2019 19:11, Max Reitz wrote:
>>> Ping again
>>>
>>> (I feel like I just need to start merging unreviewed patches until I
>>> break something (can't take that long) so you get so scared of my
>>> patches that you at least refuse them outright)
>>
>> Could you resend before?
> 
> Sure, but for me, this series still applies as-is.
> 

For me the problem is that I've removed mails older than half a year some time
ago..


-- 
Best regards,
Vladimir