[PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[Max Reitz]

Branch: https://github.com/XanClic/qemu.git fix-bdrv_make_empty-v1
Branch: https://git.xanclic.moe/XanClic/qemu.git fix-bdrv_make_empty-v1

Hi,

Right now, there is no centralized bdrv_make_empty() function.  Not only
is it bad style to call BlockDriver methods directly, it is also wrong,
unless the caller has a BdrvChild with BLK_PERM_WRITE taken.

This series fixes that.

Note that as far as I’m aware this series shouldn’t visibly fix anything
at this point; but “block: Introduce real BdrvChildRole”
(https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00737.html)
makes the iotest break when run with -o data_file=$SOMETHING, without
this series applied beforehand.  (That is because without that series,
external data files are treated much like metadata children, so the
format driver always takes the WRITE permission if the file is writable;
but after that series, it only does so when it itself has a parent
requestion the WRITE permission.)


Max Reitz (4):
  block: Add bdrv_make_empty()
  block: Use bdrv_make_empty() where possible
  block: Add blk_make_empty()
  block: Use blk_make_empty() after commits

 include/block/block.h          |  1 +
 include/sysemu/block-backend.h |  2 ++
 block.c                        | 23 +++++++++++++++++++++++
 block/block-backend.c          |  5 +++++
 block/commit.c                 |  8 +++++++-
 block/replication.c            |  6 ++----
 block/vvfat.c                  |  4 +---
 qemu-img.c                     | 19 ++++++++++++++-----
 8 files changed, 55 insertions(+), 13 deletions(-)

-- 
2.25.4

[PATCH 1/4] block: Add bdrv_make_empty()

[Max Reitz]

Right now, all users of bdrv_make_empty() call the BlockDriver method
directly.  That is not only bad style, it is also wrong, unless the
caller has a BdrvChild with a WRITE permission.

Introduce bdrv_make_empty() that verifies that it does.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h |  1 +
 block.c               | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index b05995fe9c..d947fb4080 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
 void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
 int bdrv_commit(BlockDriverState *bs);
+int bdrv_make_empty(BdrvChild *c, Error **errp);
 int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
diff --git a/block.c b/block.c
index 2e3905c99e..b0d5b98617 100644
--- a/block.c
+++ b/block.c
@@ -6791,3 +6791,26 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
 
     parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
 }
+
+int bdrv_make_empty(BdrvChild *c, Error **errp)
+{
+    BlockDriver *drv = c->bs->drv;
+    int ret;
+
+    assert(c->perm & BLK_PERM_WRITE);
+
+    if (!drv->bdrv_make_empty) {
+        error_setg(errp, "%s does not support emptying nodes",
+                   drv->format_name);
+        return -ENOTSUP;
+    }
+
+    ret = drv->bdrv_make_empty(c->bs);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to empty %s",
+                         c->bs->filename);
+        return ret;
+    }
+
+    return 0;
+}
-- 
2.25.4

[PATCH 2/4] block: Use bdrv_make_empty() where possible

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/replication.c | 6 ++----
 block/vvfat.c       | 4 +---
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index da013c2041..cc6a40d577 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -331,9 +331,8 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
         return;
     }
 
-    ret = s->active_disk->bs->drv->bdrv_make_empty(s->active_disk->bs);
+    ret = bdrv_make_empty(s->active_disk, errp);
     if (ret < 0) {
-        error_setg(errp, "Cannot make active disk empty");
         return;
     }
 
@@ -343,9 +342,8 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
         return;
     }
 
-    ret = s->hidden_disk->bs->drv->bdrv_make_empty(s->hidden_disk->bs);
+    ret = bdrv_make_empty(s->hidden_disk, errp);
     if (ret < 0) {
-        error_setg(errp, "Cannot make hidden disk empty");
         return;
     }
 }
diff --git a/block/vvfat.c b/block/vvfat.c
index ab800c4887..e3020b65c8 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2960,9 +2960,7 @@ static int do_commit(BDRVVVFATState* s)
         return ret;
     }
 
-    if (s->qcow->bs->drv && s->qcow->bs->drv->bdrv_make_empty) {
-        s->qcow->bs->drv->bdrv_make_empty(s->qcow->bs);
-    }
+    bdrv_make_empty(s->qcow, NULL);
 
     memset(s->used_clusters, 0, sector2cluster(s, s->sector_count));
 
-- 
2.25.4

[PATCH 3/4] block: Add blk_make_empty()

[Max Reitz]

Two callers of BlockDriver.bdrv_make_empty() remain that should not call
this method directly.  Both do not have access to a BdrvChild, but they
can use a BlockBackend, so we add this function that lets them use it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/sysemu/block-backend.h | 2 ++
 block/block-backend.c          | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index d37c1244dd..14338b76dc 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
 
 const BdrvChild *blk_root(BlockBackend *blk);
 
+int blk_make_empty(BlockBackend *blk, Error **errp);
+
 #endif
diff --git a/block/block-backend.c b/block/block-backend.c
index 3592066b42..5d36efd32f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
 {
     return blk->root;
 }
+
+int blk_make_empty(BlockBackend *blk, Error **errp)
+{
+    return bdrv_make_empty(blk->root, errp);
+}
-- 
2.25.4

[PATCH 4/4] block: Use blk_make_empty() after commits

[Max Reitz]

bdrv_commit() already has a BlockBackend pointing to the BDS that we
want to empty, it just has the wrong permissions.

qemu-img commit has no BlockBackend pointing to the old backing file
yet, but introducing one is simple.

After this commit, bdrv_make_empty() is the only remaining caller of
BlockDriver.bdrv_make_empty().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/commit.c |  8 +++++++-
 qemu-img.c     | 19 ++++++++++++++-----
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 8e672799af..24720ba67d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
     }
 
     if (drv->bdrv_make_empty) {
-        ret = drv->bdrv_make_empty(bs);
+        ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
         if (ret < 0) {
             goto ro_cleanup;
         }
+
+        ret = blk_make_empty(src, NULL);
+        if (ret < 0) {
+            goto ro_cleanup;
+        }
+
         blk_flush(src);
     }
 
diff --git a/qemu-img.c b/qemu-img.c
index 821cbf610e..a5e8659867 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
         goto unref_backing;
     }
 
-    if (!drop && bs->drv->bdrv_make_empty) {
-        ret = bs->drv->bdrv_make_empty(bs);
-        if (ret) {
-            error_setg_errno(&local_err, -ret, "Could not empty %s",
-                             filename);
+    if (!drop) {
+        BlockBackend *old_backing_blk;
+
+        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
+                                          &local_err);
+        if (!old_backing_blk) {
+            goto unref_backing;
+        }
+        ret = blk_make_empty(old_backing_blk, &local_err);
+        blk_unref(old_backing_blk);
+        if (ret == -ENOTSUP) {
+            error_free(local_err);
+            local_err = NULL;
+        } else if (ret < 0) {
             goto unref_backing;
         }
     }
-- 
2.25.4

Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  SIGN    pc-bios/optionrom/kvmvapic.bin
  BUILD   pc-bios/optionrom/pvh.raw
  SIGN    pc-bios/optionrom/pvh.bin
/tmp/qemu-test/src/qemu-img.c:1071:27: error: implicit declaration of function 'blk_new_with_bs' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                          ^
/tmp/qemu-test/src/qemu-img.c:1071:27: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: incompatible integer to pointer conversion assigning to 'BlockBackend *' (aka 'struct BlockBackend *') from 'int' [-Werror,-Wint-conversion]
        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                        ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=1896d2b1f1044091b832be313d66ac6e', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-fql9ti5h/src/docker-src.2020-04-28-09.34.00.5332:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=1896d2b1f1044091b832be313d66ac6e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-fql9ti5h/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    4m44.194s
user    0m8.084s


The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  BUILD   pc-bios/optionrom/pvh.raw
  SIGN    pc-bios/optionrom/pvh.bin
/tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
/tmp/qemu-test/src/qemu-img.c:1071:9: error: implicit declaration of function 'blk_new_with_bs' [-Werror=implicit-function-declaration]
         old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
         ^
/tmp/qemu-test/src/qemu-img.c:1071:9: error: nested extern declaration of 'blk_new_with_bs' [-Werror=nested-externs]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: assignment makes pointer from integer without a cast [-Werror]
         old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                         ^
cc1: all warnings being treated as errors
make: *** [qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=2d32da85331c4d51b4632262369586d1', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-mtvq5xk5/src/docker-src.2020-04-28-09.40.27.12753:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=2d32da85331c4d51b4632262369586d1
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-mtvq5xk5/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m58.028s
user    0m8.393s


The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/exec-vary.o
  CC      aarch64-softmmu/tcg/tcg.o
/tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
/tmp/qemu-test/src/qemu-img.c:1071:27: error: implicit declaration of function 'blk_new_with_bs'; did you mean 'blk_get_stats'? [-Werror=implicit-function-declaration]
         old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                           ^~~~~~~~~~~~~~~
                           blk_get_stats
/tmp/qemu-test/src/qemu-img.c:1071:27: error: nested extern declaration of 'blk_new_with_bs' [-Werror=nested-externs]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: assignment to 'BlockBackend *' {aka 'struct BlockBackend *'} from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
         old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                         ^
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
  CC      aarch64-softmmu/tcg/tcg-op.o
  CC      aarch64-softmmu/tcg/tcg-op-vec.o
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=7e33797e28514c48a1cce7021d8b04fd', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-dh8ik377/src/docker-src.2020-04-28-09.44.17.22581:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=7e33797e28514c48a1cce7021d8b04fd
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-dh8ik377/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    4m4.967s
user    0m8.349s


The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[Eric Blake]

On 4/28/20 8:26 AM, Max Reitz wrote:
> Branch: https://github.com/XanClic/qemu.git fix-bdrv_make_empty-v1
> Branch: https://git.xanclic.moe/XanClic/qemu.git fix-bdrv_make_empty-v1
> 
> Hi,
> 
> Right now, there is no centralized bdrv_make_empty() function.  Not only
> is it bad style to call BlockDriver methods directly, it is also wrong,
> unless the caller has a BdrvChild with BLK_PERM_WRITE taken.

I'm also in the middle of writing a patch series that adds a 
corresponding .bdrv_make_empty driver callback.  I'll rebase that work 
on top of this, as part of my efforts at fixing more code to rely on 
bdrv_make_empty rather than directly querying bdrv_has_zero_init[_truncate].

> 
> This series fixes that.
> 
> Note that as far as I’m aware this series shouldn’t visibly fix anything
> at this point; but “block: Introduce real BdrvChildRole”
> (https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00737.html)
> makes the iotest break when run with -o data_file=$SOMETHING, without
> this series applied beforehand.  (That is because without that series,
> external data files are treated much like metadata children, so the
> format driver always takes the WRITE permission if the file is writable;
> but after that series, it only does so when it itself has a parent
> requestion the WRITE permission.)
> 

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

Re: [PATCH 1/4] block: Add bdrv_make_empty()

[Eric Blake]

On 4/28/20 8:26 AM, Max Reitz wrote:
> Right now, all users of bdrv_make_empty() call the BlockDriver method
> directly.  That is not only bad style, it is also wrong, unless the
> caller has a BdrvChild with a WRITE permission.
> 
> Introduce bdrv_make_empty() that verifies that it does.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   include/block/block.h |  1 +
>   block.c               | 23 +++++++++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index b05995fe9c..d947fb4080 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
>   void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
>   void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
>   int bdrv_commit(BlockDriverState *bs);
> +int bdrv_make_empty(BdrvChild *c, Error **errp);

Can we please fix this to take a flags parameter?  I want to make it 
easier for callers to request BDRV_REQ_NO_FALLBACK for distinguishing 
between callers where the image must be made empty (read as all zeroes) 
regardless of time spent, vs. made empty quickly (including if it is 
already all zero) but where the caller is prepared for the operation to 
fail and will write zeroes itself if fast bulk zeroing was not possible.


> +int bdrv_make_empty(BdrvChild *c, Error **errp)
> +{
> +    BlockDriver *drv = c->bs->drv;
> +    int ret;
> +
> +    assert(c->perm & BLK_PERM_WRITE);
> +
> +    if (!drv->bdrv_make_empty) {
> +        error_setg(errp, "%s does not support emptying nodes",
> +                   drv->format_name);
> +        return -ENOTSUP;
> +    }

And here's where we can add some automatic fallbacks, such as 
recognizing if the image already reads as all zeroes.  But those 
optimizations can come in separate patches; for YOUR patch, just getting 
the proper API in place is fine.

> +
> +    ret = drv->bdrv_make_empty(c->bs);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Failed to empty %s",
> +                         c->bs->filename);
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> 

Other than a missing flag parameter, this looks fine.


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

Re: [PATCH 2/4] block: Use bdrv_make_empty() where possible

[Eric Blake]

On 4/28/20 8:26 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/replication.c | 6 ++----
>   block/vvfat.c       | 4 +---
>   2 files changed, 3 insertions(+), 7 deletions(-)
> 

Yes, definitely nicer :)  May have some obvious fallout to add a 0 flag 
parameter, per my request on 1/4, but that doesn't stop me from giving:

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

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

Re: [PATCH 3/4] block: Add blk_make_empty()

[Eric Blake]

On 4/28/20 8:26 AM, Max Reitz wrote:
> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
> this method directly.  Both do not have access to a BdrvChild, but they
> can use a BlockBackend, so we add this function that lets them use it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   include/sysemu/block-backend.h | 2 ++
>   block/block-backend.c          | 5 +++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index d37c1244dd..14338b76dc 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>   
>   const BdrvChild *blk_root(BlockBackend *blk);
>   
> +int blk_make_empty(BlockBackend *blk, Error **errp);
> +

Again, a flag parameter to allow use of BDRV_REQ_NO_FALLBACK would be nice.

>   #endif
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 3592066b42..5d36efd32f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
>   {
>       return blk->root;
>   }
> +
> +int blk_make_empty(BlockBackend *blk, Error **errp)
> +{
> +    return bdrv_make_empty(blk->root, errp);
> +}
> 

Otherwise looks fine.

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

Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[Eric Blake]

On 4/28/20 8:43 AM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/
> 
> 

>    SIGN    pc-bios/optionrom/pvh.bin
> /tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
> /tmp/qemu-test/src/qemu-img.c:1071:9: error: implicit declaration of function 'blk_new_with_bs' [-Werror=implicit-function-declaration]
>           old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,

Ah, you forgot to tell patchew:

Based-on: <20200424190903.522087-1-eblake@redhat.com>
[PATCH v3 0/3] qcow2: Allow resize of images with internal snapshots

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

Re: [PATCH 1/4] block: Add bdrv_make_empty()

[Kevin Wolf]

Am 28.04.2020 um 15:53 hat Eric Blake geschrieben:
> On 4/28/20 8:26 AM, Max Reitz wrote:
> > Right now, all users of bdrv_make_empty() call the BlockDriver method
> > directly.  That is not only bad style, it is also wrong, unless the
> > caller has a BdrvChild with a WRITE permission.
> > 
> > Introduce bdrv_make_empty() that verifies that it does.
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >   include/block/block.h |  1 +
> >   block.c               | 23 +++++++++++++++++++++++
> >   2 files changed, 24 insertions(+)
> > 
> > diff --git a/include/block/block.h b/include/block/block.h
> > index b05995fe9c..d947fb4080 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
> >   void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
> >   void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
> >   int bdrv_commit(BlockDriverState *bs);
> > +int bdrv_make_empty(BdrvChild *c, Error **errp);
> 
> Can we please fix this to take a flags parameter?  I want to make it easier
> for callers to request BDRV_REQ_NO_FALLBACK for distinguishing between
> callers where the image must be made empty (read as all zeroes) regardless
> of time spent, vs. made empty quickly (including if it is already all zero)
> but where the caller is prepared for the operation to fail and will write
> zeroes itself if fast bulk zeroing was not possible.

bdrv_make_empty() is not for making an image read as all zeroes, but to
make it fully unallocated so that the backing file becomes visible.

Are you confusing it with bdrv_make_zero(), which is just a wrapper
around bdrv_pwrite_zeroes() and does take flags?

Kevin

Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[Eric Blake]

On 4/28/20 8:49 AM, Eric Blake wrote:
> On 4/28/20 8:26 AM, Max Reitz wrote:
>> Branch: https://github.com/XanClic/qemu.git fix-bdrv_make_empty-v1
>> Branch: https://git.xanclic.moe/XanClic/qemu.git fix-bdrv_make_empty-v1
>>
>> Hi,
>>
>> Right now, there is no centralized bdrv_make_empty() function.  Not only
>> is it bad style to call BlockDriver methods directly, it is also wrong,
>> unless the caller has a BdrvChild with BLK_PERM_WRITE taken.
> 
> I'm also in the middle of writing a patch series that adds a 
> corresponding .bdrv_make_empty driver callback.  I'll rebase that work 
> on top of this, as part of my efforts at fixing more code to rely on 
> bdrv_make_empty rather than directly querying 
> bdrv_has_zero_init[_truncate].

Correction - I'm working on adding .bdrv_make_zero, not .bdrv_make empty 
(which already exists), although maybe it really only needs to be one 
callback instead of two if we have decent flags.

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

Re: [PATCH 1/4] block: Add bdrv_make_empty()

[Eric Blake]

On 4/28/20 9:01 AM, Kevin Wolf wrote:

>> Can we please fix this to take a flags parameter?  I want to make it easier
>> for callers to request BDRV_REQ_NO_FALLBACK for distinguishing between
>> callers where the image must be made empty (read as all zeroes) regardless
>> of time spent, vs. made empty quickly (including if it is already all zero)
>> but where the caller is prepared for the operation to fail and will write
>> zeroes itself if fast bulk zeroing was not possible.
> 
> bdrv_make_empty() is not for making an image read as all zeroes, but to
> make it fully unallocated so that the backing file becomes visible.
> 
> Are you confusing it with bdrv_make_zero(), which is just a wrapper
> around bdrv_pwrite_zeroes() and does take flags?

Yes.  Although now I'm wondering if the two should remain separate or 
should just be a single driver callback where flags can include 
BDRV_REQ_ZERO_WRITE to distinguish whether exposing the backing file vs. 
reading as all zeroes is intended, or if that is merging too much.

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

Re: [PATCH 4/4] block: Use blk_make_empty() after commits

[Eric Blake]

On 4/28/20 8:26 AM, Max Reitz wrote:
> bdrv_commit() already has a BlockBackend pointing to the BDS that we
> want to empty, it just has the wrong permissions.
> 
> qemu-img commit has no BlockBackend pointing to the old backing file
> yet, but introducing one is simple.
> 
> After this commit, bdrv_make_empty() is the only remaining caller of
> BlockDriver.bdrv_make_empty().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/commit.c |  8 +++++++-
>   qemu-img.c     | 19 ++++++++++++++-----
>   2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 8e672799af..24720ba67d 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>       }
>   
>       if (drv->bdrv_make_empty) {

This 'if' is still a bit awkward. Do all filter drivers set this 
function, or will bdrv_make_empty() automatically take care of looking 
through filters?  Should this be a check of a supported_ variable 
instead (similar to how Kevin just added supported_truncate_flags)?

> -        ret = drv->bdrv_make_empty(bs);
> +        ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
>           if (ret < 0) {
>               goto ro_cleanup;
>           }
> +
> +        ret = blk_make_empty(src, NULL);

So, if the driver lacks the callback, you miss calling blk_make_empty() 
even if it would have done something.

> +        if (ret < 0) {
> +            goto ro_cleanup;
> +        }
> +
>           blk_flush(src);
>       }
>   
> diff --git a/qemu-img.c b/qemu-img.c
> index 821cbf610e..a5e8659867 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>           goto unref_backing;
>       }
>   
> -    if (!drop && bs->drv->bdrv_make_empty) {
> -        ret = bs->drv->bdrv_make_empty(bs);

Old code: if the driver had the callback, use it.

> -        if (ret) {
> -            error_setg_errno(&local_err, -ret, "Could not empty %s",
> -                             filename);
> +    if (!drop) {
> +        BlockBackend *old_backing_blk;
> +
> +        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
> +                                          &local_err);
> +        if (!old_backing_blk) {
> +            goto unref_backing;
> +        }
> +        ret = blk_make_empty(old_backing_blk, &local_err);

New code: Call blk_make_empty() whether or not driver has the callback, 
then deal with the fallout.

> +        blk_unref(old_backing_blk);
> +        if (ret == -ENOTSUP) {
> +            error_free(local_err);
> +            local_err = NULL;
> +        } else if (ret < 0) {
>               goto unref_backing;
>           }

But this actually looks smarter than the commit case.

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

Re: [PATCH 1/4] block: Add bdrv_make_empty()

[Kevin Wolf]

Am 28.04.2020 um 16:07 hat Eric Blake geschrieben:
> On 4/28/20 9:01 AM, Kevin Wolf wrote:
> 
> > > Can we please fix this to take a flags parameter?  I want to make it easier
> > > for callers to request BDRV_REQ_NO_FALLBACK for distinguishing between
> > > callers where the image must be made empty (read as all zeroes) regardless
> > > of time spent, vs. made empty quickly (including if it is already all zero)
> > > but where the caller is prepared for the operation to fail and will write
> > > zeroes itself if fast bulk zeroing was not possible.
> > 
> > bdrv_make_empty() is not for making an image read as all zeroes, but to
> > make it fully unallocated so that the backing file becomes visible.
> > 
> > Are you confusing it with bdrv_make_zero(), which is just a wrapper
> > around bdrv_pwrite_zeroes() and does take flags?
> 
> Yes.  Although now I'm wondering if the two should remain separate or should
> just be a single driver callback where flags can include BDRV_REQ_ZERO_WRITE
> to distinguish whether exposing the backing file vs. reading as all zeroes
> is intended, or if that is merging too much.

I don't think the implementations for both things are too similar, so
you might just end up having two if branches and then two separate
implementations in the block drivers.

If anything, bdrv_make_empty() is more related to discard than
write_zeroes. But we use the discard code for it in qcow2 only as a
fallback because in the most common cases, making an image completely
empty means effectively just creating an entirely new L1 and refcount
table, which is much faster than individually discarding all clusters.

For bdrv_make_zero() I don't see an opportunity for such optimisations,
so I don't really see a reason to have a separate callback. Unless you
do know one?

Kevin

Re: [PATCH 1/4] block: Add bdrv_make_empty()

[Kevin Wolf]

Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> Right now, all users of bdrv_make_empty() call the BlockDriver method
> directly.  That is not only bad style, it is also wrong, unless the
> caller has a BdrvChild with a WRITE permission.
> 
> Introduce bdrv_make_empty() that verifies that it does.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block.h |  1 +
>  block.c               | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index b05995fe9c..d947fb4080 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
>  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
>  void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
>  int bdrv_commit(BlockDriverState *bs);
> +int bdrv_make_empty(BdrvChild *c, Error **errp);
>  int bdrv_change_backing_file(BlockDriverState *bs,
>      const char *backing_file, const char *backing_fmt);
>  void bdrv_register(BlockDriver *bdrv);
> diff --git a/block.c b/block.c
> index 2e3905c99e..b0d5b98617 100644
> --- a/block.c
> +++ b/block.c
> @@ -6791,3 +6791,26 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
>  
>      parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
>  }
> +
> +int bdrv_make_empty(BdrvChild *c, Error **errp)
> +{
> +    BlockDriver *drv = c->bs->drv;
> +    int ret;
> +
> +    assert(c->perm & BLK_PERM_WRITE);

If I understand correctly, bdrv_make_empty() is called to drop an
overlay whose content is identical to what it would read from its
backing file (in particular after a commit operation). This means that
the caller promises that the visible content doesn't change.

So should we check BLK_PERM_WRITE_UNCHANGED instead?

Kevin

Re: [PATCH 1/4] block: Add bdrv_make_empty()

[Eric Blake]

On 4/28/20 9:16 AM, Kevin Wolf wrote:

>>
>> Yes.  Although now I'm wondering if the two should remain separate or should
>> just be a single driver callback where flags can include BDRV_REQ_ZERO_WRITE
>> to distinguish whether exposing the backing file vs. reading as all zeroes
>> is intended, or if that is merging too much.
> 
> I don't think the implementations for both things are too similar, so
> you might just end up having two if branches and then two separate
> implementations in the block drivers.
> 

Yeah, the more I think about it, the more two callbacks still make 
sense.  .bdrv_make_empty may or may not need a flag, but .bdrv_make_zero 
definitely does (because that's where we want a difference between 
making the entire image zero no matter the delay, vs. only making it all 
zero if it is is fast).

> If anything, bdrv_make_empty() is more related to discard than
> write_zeroes. But we use the discard code for it in qcow2 only as a
> fallback because in the most common cases, making an image completely
> empty means effectively just creating an entirely new L1 and refcount
> table, which is much faster than individually discarding all clusters.
> 
> For bdrv_make_zero() I don't see an opportunity for such optimisations,
> so I don't really see a reason to have a separate callback. Unless you
> do know one?

The optimization I have in mind is adding a qcow2 autoclear bit to track 
when an image is known to read as all zero - at which point 
.bdrv_make_zero instantly returns success.  For raw files, a possible 
optimization is to truncate to size 0 and then back to the full size, 
when it is known that truncation forces read-as-zero.  For NBD, I'm 
still playing with whether adding new 64-bit transactions for a bulk 
zero will be useful, and even if not, maybe special-casing 
NBD_CMD_WRITE_ZEROES with a size of 0 to do a bulk zero, if both server 
and client negotiated that particular meaning.

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

Re: [PATCH 3/4] block: Add blk_make_empty()

[Eric Blake]

On 4/28/20 8:55 AM, Eric Blake wrote:

>> +++ b/include/sysemu/block-backend.h
>> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend 
>> *blk_in, int64_t off_in,
>>   const BdrvChild *blk_root(BlockBackend *blk);
>> +int blk_make_empty(BlockBackend *blk, Error **errp);
>> +
> 
> Again, a flag parameter to allow use of BDRV_REQ_NO_FALLBACK would be nice.

Or maybe not, after reading Kevin's responses.  Making an image empty is 
not the same as making it read as zero.  If we can't come up with a use 
for a flag, then deferring the addition of a flag until later is a 
perfectly reasonable approach (rather than adding a flag now that will 
never get set to anything other than 0).  This isn't quite the same as a 
public API where we would regret being locked out of a flag down the road.


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

Re: [PATCH 3/4] block: Add blk_make_empty()

[Kevin Wolf]

Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
> this method directly.  Both do not have access to a BdrvChild, but they
> can use a BlockBackend, so we add this function that lets them use it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/sysemu/block-backend.h | 2 ++
>  block/block-backend.c          | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index d37c1244dd..14338b76dc 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>  
>  const BdrvChild *blk_root(BlockBackend *blk);
>  
> +int blk_make_empty(BlockBackend *blk, Error **errp);
> +
>  #endif
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 3592066b42..5d36efd32f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
>  {
>      return blk->root;
>  }
> +
> +int blk_make_empty(BlockBackend *blk, Error **errp)
> +{
> +    return bdrv_make_empty(blk->root, errp);
> +}

Should we check that blk->root != NULL? Most other functions do that
through blk_is_available().

Kevin

Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  BUILD   pc-bios/optionrom/pvh.img
  BUILD   pc-bios/optionrom/pvh.raw
  SIGN    pc-bios/optionrom/pvh.bin
/tmp/qemu-test/src/qemu-img.c:1071:27: error: implicit declaration of function 'blk_new_with_bs' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                          ^
/tmp/qemu-test/src/qemu-img.c:1071:27: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: incompatible integer to pointer conversion assigning to 'BlockBackend *' (aka 'struct BlockBackend *') from 'int' [-Werror,-Wint-conversion]
        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                        ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=c01c8a7b80794cb2a4458fd17b18ff83', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-7kkbz7sd/src/docker-src.2020-04-28-10.48.39.6384:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=c01c8a7b80794cb2a4458fd17b18ff83
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-7kkbz7sd/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    4m22.457s
user    0m8.829s


The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  SIGN    pc-bios/optionrom/linuxboot.bin
  SIGN    pc-bios/optionrom/kvmvapic.bin
/tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
/tmp/qemu-test/src/qemu-img.c:1071:9: error: implicit declaration of function 'blk_new_with_bs' [-Werror=implicit-function-declaration]
         old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
         ^
/tmp/qemu-test/src/qemu-img.c:1071:9: error: nested extern declaration of 'blk_new_with_bs' [-Werror=nested-externs]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: assignment makes pointer from integer without a cast [-Werror]
         old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                         ^
cc1: all warnings being treated as errors
make: *** [qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=46a96d39761c47298a2d1eaffd870dde', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-5y1a3roy/src/docker-src.2020-04-28-10.55.01.17538:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=46a96d39761c47298a2d1eaffd870dde
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-5y1a3roy/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m49.941s
user    0m8.502s


The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  SIGN    pc-bios/optionrom/linuxboot.bin
  SIGN    pc-bios/optionrom/kvmvapic.bin
/tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
/tmp/qemu-test/src/qemu-img.c:1071:27: error: implicit declaration of function 'blk_new_with_bs'; did you mean 'blk_get_stats'? [-Werror=implicit-function-declaration]
         old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                           ^~~~~~~~~~~~~~~
                           blk_get_stats
/tmp/qemu-test/src/qemu-img.c:1071:27: error: nested extern declaration of 'blk_new_with_bs' [-Werror=nested-externs]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: assignment to 'BlockBackend *' {aka 'struct BlockBackend *'} from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
         old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
                         ^
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=ce8305e22d9c45ed90ec2dc43d7f1cc3', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-kzo016wj/src/docker-src.2020-04-28-10.58.45.27385:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=ce8305e22d9c45ed90ec2dc43d7f1cc3
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-kzo016wj/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    3m31.450s
user    0m8.978s


The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 4/4] block: Use blk_make_empty() after commits

[Kevin Wolf]

Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> bdrv_commit() already has a BlockBackend pointing to the BDS that we
> want to empty, it just has the wrong permissions.
> 
> qemu-img commit has no BlockBackend pointing to the old backing file
> yet, but introducing one is simple.
> 
> After this commit, bdrv_make_empty() is the only remaining caller of
> BlockDriver.bdrv_make_empty().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/commit.c |  8 +++++++-
>  qemu-img.c     | 19 ++++++++++++++-----
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 8e672799af..24720ba67d 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>      }
>  
>      if (drv->bdrv_make_empty) {
> -        ret = drv->bdrv_make_empty(bs);
> +        ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);

This is very likely to fail because the common case is that the source
node is attached to a guest device that doesn't share writes.
(qemu-iotests 131 and 274 catch this.)

So I think after my theoretical comment in patch 1, this is the
practical reason why we need WRITE_UNCHANGED rather than WRITE.

Also, why don't you take this permission from the start so that we would
error out right away rather than failing after waiting for the all the
data to be copied?

>          if (ret < 0) {
>              goto ro_cleanup;
>          }
> +
> +        ret = blk_make_empty(src, NULL);
> +        if (ret < 0) {
> +            goto ro_cleanup;
> +        }
> +
>          blk_flush(src);
>      }
>  
> diff --git a/qemu-img.c b/qemu-img.c
> index 821cbf610e..a5e8659867 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>          goto unref_backing;
>      }
>  
> -    if (!drop && bs->drv->bdrv_make_empty) {
> -        ret = bs->drv->bdrv_make_empty(bs);
> -        if (ret) {
> -            error_setg_errno(&local_err, -ret, "Could not empty %s",
> -                             filename);
> +    if (!drop) {
> +        BlockBackend *old_backing_blk;
> +
> +        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
> +                                          &local_err);

Oh, you actually depend on another series that you didn't mention in
the cover letter.

> +        if (!old_backing_blk) {
> +            goto unref_backing;
> +        }
> +        ret = blk_make_empty(old_backing_blk, &local_err);
> +        blk_unref(old_backing_blk);
> +        if (ret == -ENOTSUP) {
> +            error_free(local_err);
> +            local_err = NULL;
> +        } else if (ret < 0) {
>              goto unref_backing;
>          }
>      }

Kevin

Re: [PATCH 2/4] block: Use bdrv_make_empty() where possible

[Kevin Wolf]

Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 1/4] block: Add bdrv_make_empty()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2125 bytes --]

On 28.04.20 16:21, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> Right now, all users of bdrv_make_empty() call the BlockDriver method
>> directly.  That is not only bad style, it is also wrong, unless the
>> caller has a BdrvChild with a WRITE permission.
>>
>> Introduce bdrv_make_empty() that verifies that it does.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/block/block.h |  1 +
>>  block.c               | 23 +++++++++++++++++++++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index b05995fe9c..d947fb4080 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
>>  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
>>  void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
>>  int bdrv_commit(BlockDriverState *bs);
>> +int bdrv_make_empty(BdrvChild *c, Error **errp);
>>  int bdrv_change_backing_file(BlockDriverState *bs,
>>      const char *backing_file, const char *backing_fmt);
>>  void bdrv_register(BlockDriver *bdrv);
>> diff --git a/block.c b/block.c
>> index 2e3905c99e..b0d5b98617 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6791,3 +6791,26 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
>>  
>>      parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
>>  }
>> +
>> +int bdrv_make_empty(BdrvChild *c, Error **errp)
>> +{
>> +    BlockDriver *drv = c->bs->drv;
>> +    int ret;
>> +
>> +    assert(c->perm & BLK_PERM_WRITE);
> 
> If I understand correctly, bdrv_make_empty() is called to drop an
> overlay whose content is identical to what it would read from its
> backing file (in particular after a commit operation). This means that
> the caller promises that the visible content doesn't change.
> 
> So should we check BLK_PERM_WRITE_UNCHANGED instead?

Ah, right.  Yes, that would be better.  (Or to check both, whether any
of them has been taken.)

Max


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

Re: [PATCH 3/4] block: Add blk_make_empty()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1484 bytes --]

On 28.04.20 16:47, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
>> this method directly.  Both do not have access to a BdrvChild, but they
>> can use a BlockBackend, so we add this function that lets them use it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/sysemu/block-backend.h | 2 ++
>>  block/block-backend.c          | 5 +++++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> index d37c1244dd..14338b76dc 100644
>> --- a/include/sysemu/block-backend.h
>> +++ b/include/sysemu/block-backend.h
>> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>>  
>>  const BdrvChild *blk_root(BlockBackend *blk);
>>  
>> +int blk_make_empty(BlockBackend *blk, Error **errp);
>> +
>>  #endif
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 3592066b42..5d36efd32f 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
>>  {
>>      return blk->root;
>>  }
>> +
>> +int blk_make_empty(BlockBackend *blk, Error **errp)
>> +{
>> +    return bdrv_make_empty(blk->root, errp);
>> +}
> 
> Should we check that blk->root != NULL? Most other functions do that
> through blk_is_available().

Why not.

Max


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

Re: [PATCH 4/4] block: Use blk_make_empty() after commits

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3919 bytes --]

On 28.04.20 16:07, Eric Blake wrote:
> On 4/28/20 8:26 AM, Max Reitz wrote:
>> bdrv_commit() already has a BlockBackend pointing to the BDS that we
>> want to empty, it just has the wrong permissions.
>>
>> qemu-img commit has no BlockBackend pointing to the old backing file
>> yet, but introducing one is simple.
>>
>> After this commit, bdrv_make_empty() is the only remaining caller of
>> BlockDriver.bdrv_make_empty().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/commit.c |  8 +++++++-
>>   qemu-img.c     | 19 ++++++++++++++-----
>>   2 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 8e672799af..24720ba67d 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>>       }
>>         if (drv->bdrv_make_empty) {
> 
> This 'if' is still a bit awkward. Do all filter drivers set this
> function, or will bdrv_make_empty() automatically take care of looking
> through filters?  Should this be a check of a supported_ variable
> instead (similar to how Kevin just added supported_truncate_flags)?
> 
>> -        ret = drv->bdrv_make_empty(bs);
>> +        ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
>>           if (ret < 0) {
>>               goto ro_cleanup;
>>           }
>> +
>> +        ret = blk_make_empty(src, NULL);
> 
> So, if the driver lacks the callback, you miss calling blk_make_empty()
> even if it would have done something.

We can’t just call blk_make_empty() and then special case -ENOTSUP here,
though, because the BB doesn’t have a WRITE permission beforehand.  So
we have to take that permission before we can call blk_make_empty().
But taking the permission can fail, and then we kind of have to report
the -EPERM, even though the BlockDriver may not support emptying anyway.

I suppose if we just have to take the WRITE_UNCHANGED permission,
though, we can assume that that’s basically always allowed, so it
shouldn’t be that much of a problem there.

Max

>> +        if (ret < 0) {
>> +            goto ro_cleanup;
>> +        }
>> +
>>           blk_flush(src);
>>       }
>>   diff --git a/qemu-img.c b/qemu-img.c
>> index 821cbf610e..a5e8659867 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>>           goto unref_backing;
>>       }
>>   -    if (!drop && bs->drv->bdrv_make_empty) {
>> -        ret = bs->drv->bdrv_make_empty(bs);
> 
> Old code: if the driver had the callback, use it.
> 
>> -        if (ret) {
>> -            error_setg_errno(&local_err, -ret, "Could not empty %s",
>> -                             filename);
>> +    if (!drop) {
>> +        BlockBackend *old_backing_blk;
>> +
>> +        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE,
>> BLK_PERM_ALL,
>> +                                          &local_err);
>> +        if (!old_backing_blk) {
>> +            goto unref_backing;
>> +        }
>> +        ret = blk_make_empty(old_backing_blk, &local_err);
> 
> New code: Call blk_make_empty() whether or not driver has the callback,
> then deal with the fallout.
> 
>> +        blk_unref(old_backing_blk);
>> +        if (ret == -ENOTSUP) {
>> +            error_free(local_err);
>> +            local_err = NULL;
>> +        } else if (ret < 0) {
>>               goto unref_backing;
>>           }
> 
> But this actually looks smarter than the commit case.
> 



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

Re: [PATCH 4/4] block: Use blk_make_empty() after commits

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3048 bytes --]

On 28.04.20 17:03, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> bdrv_commit() already has a BlockBackend pointing to the BDS that we
>> want to empty, it just has the wrong permissions.
>>
>> qemu-img commit has no BlockBackend pointing to the old backing file
>> yet, but introducing one is simple.
>>
>> After this commit, bdrv_make_empty() is the only remaining caller of
>> BlockDriver.bdrv_make_empty().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/commit.c |  8 +++++++-
>>  qemu-img.c     | 19 ++++++++++++++-----
>>  2 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 8e672799af..24720ba67d 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>>      }
>>  
>>      if (drv->bdrv_make_empty) {
>> -        ret = drv->bdrv_make_empty(bs);
>> +        ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
> 
> This is very likely to fail because the common case is that the source
> node is attached to a guest device that doesn't share writes.
> (qemu-iotests 131 and 274 catch this.)
> 
> So I think after my theoretical comment in patch 1, this is the
> practical reason why we need WRITE_UNCHANGED rather than WRITE.
> 
> Also, why don't you take this permission from the start so that we would
> error out right away rather than failing after waiting for the all the
> data to be copied?

Because we only need to take it when the BlockDriver actually supports
bdrv_make_empty(), so I thought I’d put it here where we have the check
anyway.

However, yes, when we take WRITE_UNCHANGED, we might as well take it
unconditionally from the start.  (And then call blk_make_empty()
unconditionally here, too, and let it figure out -ENOTSUP, like Eric noted.)

>>          if (ret < 0) {
>>              goto ro_cleanup;
>>          }
>> +
>> +        ret = blk_make_empty(src, NULL);
>> +        if (ret < 0) {
>> +            goto ro_cleanup;
>> +        }
>> +
>>          blk_flush(src);
>>      }
>>  
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 821cbf610e..a5e8659867 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>>          goto unref_backing;
>>      }
>>  
>> -    if (!drop && bs->drv->bdrv_make_empty) {
>> -        ret = bs->drv->bdrv_make_empty(bs);
>> -        if (ret) {
>> -            error_setg_errno(&local_err, -ret, "Could not empty %s",
>> -                             filename);
>> +    if (!drop) {
>> +        BlockBackend *old_backing_blk;
>> +
>> +        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
>> +                                          &local_err);
> 
> Oh, you actually depend on another series that you didn't mention in
> the cover letter.

Well, yes.  I didn’t really realize because I just based it on my
block-next...

Max


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