[PATCH 0/3] mirror: Freeze backing chain for sync=top

[PATCH 0/3] mirror: Freeze backing chain for sync=top

[Max Reitz]

Hi,

On review of the “Deal with filters” series, Kevin noted that mirror
doesn’t freeze its backing chain, even though we keep references to the
base (for sync=top).  That seems like a recipe for disaster, and as the
test added in patch 3 shows, indeed it is.

Patch 1 could also be squashed into “mirror: Deal with filters”.

As for patch 2; maybe it would be sufficient to freeze the backing chain
only down to base_overlay and until mirror_dirty_init() is done.  Then
we bdrv_ref(s->base) so can always be attached to the target when mirror
completes.  But I think the conservative approach I implemented here is
fine until someone complains.


Max Reitz (3):
  mirror: Set s->base_overlay only if s->base is set
  mirror: Freeze backing chain for sync=top
  iotests/041: x-blockdev-reopen during mirror

 block/mirror.c             | 28 +++++++++---
 tests/qemu-iotests/041     | 92 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out |  4 +-
 3 files changed, 116 insertions(+), 8 deletions(-)

-- 
2.26.2

[PATCH 1/3] mirror: Set s->base_overlay only if s->base is set

[Max Reitz]

This way, sync=full will not need a reference to any node other than the
source and the target.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 26acf4af6f..11ebffdf99 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -42,6 +42,7 @@ typedef struct MirrorBlockJob {
     BlockBackend *target;
     BlockDriverState *mirror_top_bs;
     BlockDriverState *base;
+    /* Overlay of @base if @base is non-NULL; NULL otherwise */
     BlockDriverState *base_overlay;
 
     /* The name of the graph node to replace */
@@ -839,8 +840,9 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
             return 0;
         }
 
-        ret = bdrv_is_allocated_above(bs, s->base_overlay, true, offset, bytes,
-                                      &count);
+        ret = bdrv_is_allocated_above(bs, s->base_overlay,
+                                      s->base_overlay != NULL,
+                                      offset, bytes, &count);
         if (ret < 0) {
             return ret;
         }
@@ -1710,7 +1712,7 @@ static BlockJob *mirror_start_job(
     s->zero_target = zero_target;
     s->copy_mode = copy_mode;
     s->base = base;
-    s->base_overlay = bdrv_find_overlay(bs, base);
+    s->base_overlay = base ? bdrv_find_overlay(bs, base) : NULL;
     s->granularity = granularity;
     s->buf_size = ROUND_UP(buf_size, granularity);
     s->unmap = unmap;
-- 
2.26.2

[PATCH 2/3] mirror: Freeze backing chain for sync=top

[Max Reitz]

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 11ebffdf99..27422ab7a5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -649,8 +649,8 @@ static int mirror_exit_common(Job *job)
     src = mirror_top_bs->backing->bs;
     target_bs = blk_bs(s->target);
 
-    if (bdrv_chain_contains(src, target_bs)) {
-        bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
+    if (s->base) {
+        bdrv_unfreeze_backing_chain(mirror_top_bs, s->base);
     }
 
     bdrv_release_dirty_bitmap(s->dirty_bitmap);
@@ -1780,8 +1780,22 @@ static BlockJob *mirror_start_job(
                 goto fail;
             }
         }
+    }
+
+    if (s->base) {
+        /*
+         * For active commit or mirror with sync=top, we need to
+         * freeze the backing chain down to the base, because we keep
+         * pointers to it and its overlay.  For other cases (mirror
+         * with sync=full or sync=none), we do not, so there is no
+         * need to freeze any part of the chain.
+         * (s->base is non-NULL only in these cases.)
+         */
+        if (target_is_backing) {
+            assert(s->base == target);
+        }
 
-        if (bdrv_freeze_backing_chain(mirror_top_bs, target, errp) < 0) {
+        if (bdrv_freeze_backing_chain(mirror_top_bs, s->base, errp) < 0) {
             goto fail;
         }
     }
-- 
2.26.2

Re: [PATCH 2/3] mirror: Freeze backing chain for sync=top

[Max Reitz]

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

On 02.09.20 18:48, Max Reitz wrote:
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/mirror.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 11ebffdf99..27422ab7a5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -649,8 +649,8 @@ static int mirror_exit_common(Job *job)
>      src = mirror_top_bs->backing->bs;
>      target_bs = blk_bs(s->target);
>  
> -    if (bdrv_chain_contains(src, target_bs)) {
> -        bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
> +    if (s->base) {
> +        bdrv_unfreeze_backing_chain(mirror_top_bs, s->base);
>      }
>  
>      bdrv_release_dirty_bitmap(s->dirty_bitmap);
> @@ -1780,8 +1780,22 @@ static BlockJob *mirror_start_job(
>                  goto fail;
>              }
>          }
> +    }
> +
> +    if (s->base) {
> +        /*
> +         * For active commit or mirror with sync=top, we need to
> +         * freeze the backing chain down to the base, because we keep
> +         * pointers to it and its overlay.  For other cases (mirror
> +         * with sync=full or sync=none), we do not, so there is no
> +         * need to freeze any part of the chain.
> +         * (s->base is non-NULL only in these cases.)
> +         */
> +        if (target_is_backing) {
> +            assert(s->base == target);
> +        }

On second thought, this assertion doesn’t hold true when mirroring to an
image in the backing chain (i.e., specifically not using commit).  Now,
this mostly doesn’t work thanks to op blockers, but after “Deal with
filters”, it’s possible.  I don’t know whether it produces any sensible
results, though.

Should we leave it possible, or should we make mirroring to a node in
the backing chain (i.e. target_is_backing && s->base != target) an error?

Max

>  
> -        if (bdrv_freeze_backing_chain(mirror_top_bs, target, errp) < 0) {
> +        if (bdrv_freeze_backing_chain(mirror_top_bs, s->base, errp) < 0) {
>              goto fail;
>          }
>      }
> 



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

[PATCH 3/3] iotests/041: x-blockdev-reopen during mirror

[Max Reitz]

Test what happens when you remove the backing file during a mirror with
sync=top.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/041     | 92 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out |  4 +-
 2 files changed, 94 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index cdbef3ba20..4ed7e293ae 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1432,6 +1432,98 @@ class TestFilters(iotests.QMPTestCase):
         self.complete_and_wait('mirror')
 
 
+class TestReconfDuringMirror(iotests.QMPTestCase):
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
+        qemu_img('create', '-f', iotests.imgfmt, '-b', backing_img,
+                 '-F', iotests.imgfmt, test_img)
+
+        qemu_io('-c', 'write -P 1 0 1M', test_img)
+
+        self.vm = iotests.VM()
+        self.vm.launch()
+
+        blockdevs = (
+            { 'node-name': 'backing-file',
+              'driver': 'file',
+              'filename': backing_img },
+
+            { 'node-name': 'backing',
+              'driver': iotests.imgfmt,
+              'file': 'backing-file',
+              'read-only': True },
+
+            { 'node-name': 'source-file',
+              'driver': 'file',
+              'filename': test_img },
+
+            { 'node-name': 'source',
+              'driver': iotests.imgfmt,
+              'file': 'source-file',
+              'backing': 'backing' }
+        )
+
+        for blockdev in blockdevs:
+            result = self.vm.qmp('blockdev-add', **blockdev)
+            self.assert_qmp(result, 'return', {})
+
+    def tearDown(self):
+        self.vm.shutdown()
+
+        os.remove(test_img)
+        os.remove(backing_img)
+
+        try:
+            os.remove(target_img)
+        except OSError:
+            pass
+
+    def test_reconf_during_top_mirror(self):
+        result = self.vm.qmp('drive-mirror',
+                             job_id='mirror',
+                             device='source',
+                             sync='top',
+                             target=target_img,
+                             speed=65536)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('x-blockdev-reopen',
+                             **{ 'node-name': 'source',
+                                 'driver': iotests.imgfmt,
+                                 'file': 'source-file',
+                                 'backing': None })
+        if 'error' in result:
+            self.assert_qmp(result, 'error/desc',
+                            "Cannot change 'backing' link "
+                            "from 'source' to 'backing'")
+
+            result = self.vm.qmp('block-job-set-speed',
+                                 device='mirror', speed=0)
+            self.assert_qmp(result, 'return', {})
+
+            self.complete_and_wait('mirror')
+
+            # Pass
+            return
+
+        # This is not expected.  Let's see what happens when we drop
+        # the backing file altogether.
+        for node in ('backing', 'backing-file'):
+            result = self.vm.qmp('blockdev-del', node_name=node)
+            self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-job-set-speed',
+                             device='mirror', speed=0)
+        self.assert_qmp(result, 'return', {})
+
+        # Completing will now probably result in a segfault, because
+        # it tries to assign 'backing' as the target's backing node
+        self.complete_and_wait('mirror')
+
+        # Either way, this is a failure.
+        self.fail('x-blockdev-reopen should not have succeeded')
+
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'],
                  supported_protocols=['file'],
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 46651953e8..5273ce86c3 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-...........................................................................................................
+............................................................................................................
 ----------------------------------------------------------------------
-Ran 107 tests
+Ran 108 tests
 
 OK
-- 
2.26.2