[PATCH 0/4] block: Relax restrictions for blockdev-snapshot

[PATCH 0/4] block: Relax restrictions for blockdev-snapshot

[Kevin Wolf]

This series allows libvirt to fix a regression that its switch from
drive-mirror to blockdev-mirror caused: It currently requires that the
backing chain of the target image is already available when the mirror
operation is started.

In reality, the backing chain may only be copied while the operation is
in progress, so the backing file of the target image needs to stay
disabled until the operation completes and should be attached only at
that point. Without this series, we don't have a supported API to attach
the backing file at that later point.

Kevin Wolf (4):
  block: Make bdrv_get_cumulative_perm() public
  block: Relax restrictions for blockdev-snapshot
  iotests: Fix run_job() with use_log=False
  iotests: Test mirror with temporarily disabled target backing file

 include/block/block_int.h     |  3 ++
 block.c                       |  6 ++--
 blockdev.c                    | 14 +++++----
 tests/qemu-iotests/iotests.py |  5 +++-
 tests/qemu-iotests/155        | 54 +++++++++++++++++++++++++++++++----
 tests/qemu-iotests/155.out    |  4 +--
 6 files changed, 68 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH 1/4] block: Make bdrv_get_cumulative_perm() public

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h | 3 +++
 block.c                   | 6 ++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index f422c0bff0..71164c4ee1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1226,6 +1226,9 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   void *opaque, Error **errp);
 void bdrv_root_unref_child(BdrvChild *child);
 
+void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
+                              uint64_t *shared_perm);
+
 /**
  * Sets a BdrvChild's permissions.  Avoid if the parent is a BDS; use
  * bdrv_child_refresh_perms() instead and make the parent's
diff --git a/block.c b/block.c
index 60e4e273af..5428d121b1 100644
--- a/block.c
+++ b/block.c
@@ -1872,8 +1872,6 @@ static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
                                  bool *tighten_restrictions, Error **errp);
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
-static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
-                                     uint64_t *shared_perm);
 
 typedef struct BlockReopenQueueEntry {
      bool prepared;
@@ -2097,8 +2095,8 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
     }
 }
 
-static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
-                                     uint64_t *shared_perm)
+void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
+                              uint64_t *shared_perm)
 {
     BdrvChild *c;
     uint64_t cumulative_perms = 0;
-- 
2.20.1

[PATCH 2/4] block: Relax restrictions for blockdev-snapshot

[Kevin Wolf]

blockdev-snapshot returned an error if the overlay was already in use,
which it defined as having any BlockBackend parent. This is in fact both
too strict (some parents can tolerate the change of visible data caused
by attaching a backing file) and too loose (some non-BlockBackend
parents may not be happy with it).

One important use case that is prevented by the too strict check is live
storage migration with blockdev-mirror. Here, the target node is
usually opened without a backing file so that the active layer is
mirrored while its backing chain can be copied in the background.

The backing chain should be attached to the mirror target node when
finalising the job, just before switching the users of the source node
to the new copy (at which point the mirror job still has a reference to
the node). drive-mirror did this automatically, but with blockdev-mirror
this is the job of the QMP client, so it needs a way to do this.

blockdev-snapshot is the obvious way, so this patch makes it work in
this scenario. The new condition is that no parent uses CONSISTENT_READ
permissions. This will ensure that the operation will still be blocked
when the node is attached to the guest device, so blockdev-snapshot
remains safe.

(For the sake of completeness, x-blockdev-reopen can be used to achieve
the same, however it is a big hammer, performs the graph change
completely unchecked and is still experimental. So even with the option
of using x-blockdev-reopen, there are reasons why blockdev-snapshot
should be able to perform this operation.)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3e44fa766b..bba0e9775b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1501,6 +1501,7 @@ static void external_snapshot_prepare(BlkActionState *common,
     TransactionAction *action = common->action;
     AioContext *aio_context;
     AioContext *old_context;
+    uint64_t perm, shared;
     int ret;
 
     /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
@@ -1616,16 +1617,17 @@ static void external_snapshot_prepare(BlkActionState *common,
         goto out;
     }
 
-    if (bdrv_has_blk(state->new_bs)) {
+    /*
+     * Allow attaching a backing file to an overlay that's already in use only
+     * if the parents don't assume that they are already seeing a valid image.
+     * (Specifically, allow it as a mirror target, which is write-only access.)
+     */
+    bdrv_get_cumulative_perm(state->new_bs, &perm, &shared);
+    if (perm & BLK_PERM_CONSISTENT_READ) {
         error_setg(errp, "The overlay is already in use");
         goto out;
     }
 
-    if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
-                           errp)) {
-        goto out;
-    }
-
     if (state->new_bs->backing != NULL) {
         error_setg(errp, "The overlay already has a backing image");
         goto out;
-- 
2.20.1

[PATCH 3/4] iotests: Fix run_job() with use_log=False

[Kevin Wolf]

The 'job-complete' QMP command should be run with qmp() rather than
qmp_log() if use_log=False is passed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/iotests.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8815052eb5..23043baa26 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -624,7 +624,10 @@ class VM(qtest.QEMUQtestMachine):
                         if use_log:
                             log('Job failed: %s' % (j['error']))
             elif status == 'ready':
-                self.qmp_log('job-complete', id=job)
+                if use_log:
+                    self.qmp_log('job-complete', id=job)
+                else:
+                    self.qmp('job-complete', id=job)
             elif status == 'pending' and not auto_finalize:
                 if pre_finalize:
                     pre_finalize()
-- 
2.20.1

[PATCH 4/4] iotests: Test mirror with temporarily disabled target backing file

[Kevin Wolf]

The newly tested scenario is a common live storage migration scenario:
The target node is opened without a backing file so that the active
layer is mirrored while its backing chain can be copied in the
background.

The backing chain should be attached to the mirror target node when
finalising the job, just before switching the users of the source node
to the new copy (at which point the mirror job still has a reference to
the node). drive-mirror did this automatically, but with blockdev-mirror
this is the job of the QMP client.

This patch adds test cases for two ways to achieve the desired result,
using either x-blockdev-reopen or blockdev-snapshot.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/155     | 54 ++++++++++++++++++++++++++++++++++----
 tests/qemu-iotests/155.out |  4 +--
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index f237868710..8f293a9885 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -45,10 +45,15 @@ target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt)
 #                      image during runtime, only makes sense if
 #                      target_blockdev_backing is not None
 #                      (None: same as target_backing)
+# target_open_with_backing: If True, the target image is added with its backing
+#                           chain opened right away. If False, blockdev-add
+#                           opens it without a backing file and job completion
+#                           is supposed to open the backing chain.
 
 class BaseClass(iotests.QMPTestCase):
     target_blockdev_backing = None
     target_real_backing = None
+    target_open_with_backing = True
 
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, back0_img, '1440K')
@@ -80,9 +85,13 @@ class BaseClass(iotests.QMPTestCase):
                 options = { 'node-name': 'target',
                             'driver': iotests.imgfmt,
                             'file': { 'driver': 'file',
+                                      'node-name': 'target-file',
                                       'filename': target_img } }
-                if self.target_blockdev_backing:
-                    options['backing'] = self.target_blockdev_backing
+
+                if not self.target_open_with_backing:
+                        options['backing'] = None
+                elif self.target_blockdev_backing:
+                        options['backing'] = self.target_blockdev_backing
 
                 result = self.vm.qmp('blockdev-add', **options)
                 self.assert_qmp(result, 'return', {})
@@ -147,10 +156,14 @@ class BaseClass(iotests.QMPTestCase):
 # cmd: Mirroring command to execute, either drive-mirror or blockdev-mirror
 
 class MirrorBaseClass(BaseClass):
+    def openBacking(self):
+        pass
+
     def runMirror(self, sync):
         if self.cmd == 'blockdev-mirror':
             result = self.vm.qmp(self.cmd, job_id='mirror-job', device='source',
-                                 sync=sync, target='target')
+                                 sync=sync, target='target',
+                                 auto_finalize=False)
         else:
             if self.existing:
                 mode = 'existing'
@@ -159,11 +172,12 @@ class MirrorBaseClass(BaseClass):
             result = self.vm.qmp(self.cmd, job_id='mirror-job', device='source',
                                  sync=sync, target=target_img,
                                  format=iotests.imgfmt, mode=mode,
-                                 node_name='target')
+                                 node_name='target', auto_finalize=False)
 
         self.assert_qmp(result, 'return', {})
 
-        self.complete_and_wait('mirror-job')
+        self.vm.run_job('mirror-job', use_log=False, auto_finalize=False,
+                        pre_finalize=self.openBacking, auto_dismiss=True)
 
     def testFull(self):
         self.runMirror('full')
@@ -221,6 +235,36 @@ class TestBlockdevMirrorForcedBacking(MirrorBaseClass):
     target_blockdev_backing = { 'driver': 'null-co' }
     target_real_backing = 'null-co://'
 
+class TestBlockdevMirrorBlockdevReopenBacking(MirrorBaseClass):
+    cmd = 'blockdev-mirror'
+    existing = True
+    target_backing = 'null-co://'
+    target_open_with_backing = False
+
+    def openBacking(self):
+        if not self.target_open_with_backing:
+            result = self.vm.qmp('blockdev-add', node_name="backing",
+                                 driver="null-co")
+            self.assert_qmp(result, 'return', {})
+            result = self.vm.qmp('x-blockdev-reopen', node_name="target",
+                                 driver=iotests.imgfmt, file="target-file",
+                                 backing="backing")
+            self.assert_qmp(result, 'return', {})
+
+class TestBlockdevMirrorBlockdevSnapshotBacking(MirrorBaseClass):
+    cmd = 'blockdev-mirror'
+    existing = True
+    target_backing = 'null-co://'
+    target_open_with_backing = False
+
+    def openBacking(self):
+        if not self.target_open_with_backing:
+            result = self.vm.qmp('blockdev-add', node_name="backing",
+                                 driver="null-co")
+            self.assert_qmp(result, 'return', {})
+            result = self.vm.qmp('blockdev-snapshot', node="backing",
+                                 overlay="target")
+            self.assert_qmp(result, 'return', {})
 
 class TestCommit(BaseClass):
     existing = False
diff --git a/tests/qemu-iotests/155.out b/tests/qemu-iotests/155.out
index 4176bb9402..4fd1c2dcd2 100644
--- a/tests/qemu-iotests/155.out
+++ b/tests/qemu-iotests/155.out
@@ -1,5 +1,5 @@
-...................
+.........................
 ----------------------------------------------------------------------
-Ran 19 tests
+Ran 25 tests
 
 OK
-- 
2.20.1

Re: [PATCH 0/4] block: Relax restrictions for blockdev-snapshot

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200305125100.386-1-kwolf@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 ===

Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
remote: Counting objects: 5394, done.        
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
fatal: protocol error: bad pack header
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-pcg20kdn/src/docker-src.2020-03-05-08.26.25.29148] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-pcg20kdn/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    6m50.721s
user    0m3.176s


The full log is available at
http://patchew.org/logs/20200305125100.386-1-kwolf@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: Relax restrictions for blockdev-snapshot

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200305125100.386-1-kwolf@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 ===

Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
remote: Counting objects: 5394, done.        
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
fatal: protocol error: bad pack header
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-yb2oz9s3/src/docker-src.2020-03-05-08.34.10.487] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-yb2oz9s3/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    6m35.983s
user    0m3.008s


The full log is available at
http://patchew.org/logs/20200305125100.386-1-kwolf@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: Relax restrictions for blockdev-snapshot

[Peter Krempa]

On Thu, Mar 05, 2020 at 13:50:56 +0100, Kevin Wolf wrote:
> This series allows libvirt to fix a regression that its switch from
> drive-mirror to blockdev-mirror caused: It currently requires that the
> backing chain of the target image is already available when the mirror
> operation is started.
> 
> In reality, the backing chain may only be copied while the operation is
> in progress, so the backing file of the target image needs to stay
> disabled until the operation completes and should be attached only at
> that point. Without this series, we don't have a supported API to attach
> the backing file at that later point.
> 
> Kevin Wolf (4):
>   block: Make bdrv_get_cumulative_perm() public
>   block: Relax restrictions for blockdev-snapshot
>   iotests: Fix run_job() with use_log=False
>   iotests: Test mirror with temporarily disabled target backing file

I've modified the libvirt code I have to try this. It works as expected
without iothreads, but I get the following error when iothread is used:

 error: internal error: unable to execute QEMU command 'transaction': Cannot change iothread of active block backend

I've tested it also with your Aio context patches for blockdev-reopen
applied and also added a feature flag for blockdev-snapshot

 https://gitlab.com/pipo.sk/qemu/-/commits/kevin-snapshot-blockcopy

I can post the feature patch if you want after I clean it up or perhaps
suggest a better name or wording for it.

The libvirt code is a subset of

 https://www.redhat.com/archives/libvir-list/2020-February/msg01125.html

with the blockdev-reopen bits removed and replaced by blockdev-snapshot.

You can have a look at the libvirt impl here:

 https://gitlab.com/pipo.sk/libvirt/-/commits/block-copy-reopen-snapshot

I'll post it for review if it's clear that iothreads can be supported
using this approach.