[Qemu-devel] [PATCH for-2.10 v2 0/4] block: Fix non-shared storage migration

[Qemu-devel] [PATCH for-2.10 v2 0/4] block: Fix non-shared storage migration

[Fam Zheng]

v2: Don't leak blk->vmsh when BB is deleted before the callback is called.
    [Stefan]
    From stub functions, don't return g_malloc0(1) which is risky, return NULL.
    [Eric]

"nbd-server-add -w" doesn't work when we are in "-incoming defer" state:

    (qemu) nbd_server_add -w drive-virtio-disk0
    Block node is read-only

Two problems are faced:

  - nbd_export_new() calls bdrv_invalidate_cache() too late.
  - bdrv_invalidate_cache() restores qdev permission (which are temporarily
    masked by BlockBackend.disable_perm during INMIGRATE) too early.

Fix both, and add a regression iotest.

Fam Zheng (3):
  stubs: Add vm state change handler stubs
  block-backend: Defer shared_perm tightening migration completion
  iotests: Add non-shared storage migration case 192

Kevin Wolf (1):
  nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cache

 block/block-backend.c        | 41 ++++++++++++++++++++++++++++
 nbd/server.c                 | 20 +++++++-------
 stubs/Makefile.objs          |  1 +
 stubs/change-state-handler.c | 14 ++++++++++
 tests/qemu-iotests/192       | 63 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/192.out   |  7 +++++
 tests/qemu-iotests/group     |  1 +
 7 files changed, 138 insertions(+), 9 deletions(-)
 create mode 100644 stubs/change-state-handler.c
 create mode 100755 tests/qemu-iotests/192
 create mode 100644 tests/qemu-iotests/192.out

-- 
2.13.4

[Qemu-devel] [PATCH for-2.10 v2 1/4] stubs: Add vm state change handler stubs

[Fam Zheng]

They will be used by BlockBackend code in block-obj-y, which doesn't
always get linked with common-obj-y. Add stubs to keep ld happy.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 stubs/Makefile.objs          |  1 +
 stubs/change-state-handler.c | 14 ++++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 stubs/change-state-handler.c

diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f5b47bfd74..e69c217aff 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -19,6 +19,7 @@ stub-obj-y += is-daemonized.o
 stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 stub-obj-y += machine-init-done.o
 stub-obj-y += migr-blocker.o
+stub-obj-y += change-state-handler.o
 stub-obj-y += monitor.o
 stub-obj-y += notify-event.o
 stub-obj-y += qtest.o
diff --git a/stubs/change-state-handler.c b/stubs/change-state-handler.c
new file mode 100644
index 0000000000..01b1c6986d
--- /dev/null
+++ b/stubs/change-state-handler.c
@@ -0,0 +1,14 @@
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "sysemu/sysemu.h"
+
+VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
+                                                     void *opaque)
+{
+    return NULL;
+}
+
+void qemu_del_vm_change_state_handler(VMChangeStateEntry *e)
+{
+    /* Nothing to do. */
+}
-- 
2.13.4

Re: [Qemu-devel] [PATCH for-2.10 v2 1/4] stubs: Add vm state change handler stubs

[Eric Blake]

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

On 08/15/2017 08:07 AM, Fam Zheng wrote:
> They will be used by BlockBackend code in block-obj-y, which doesn't
> always get linked with common-obj-y. Add stubs to keep ld happy.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  stubs/Makefile.objs          |  1 +
>  stubs/change-state-handler.c | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
>  create mode 100644 stubs/change-state-handler.c

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

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


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

[Qemu-devel] [PATCH for-2.10 v2 2/4] nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cache

[Fam Zheng]

From: Kevin Wolf <kwolf@redhat.com>

The "inactive" state of BDS affects whether the permissions can be
granted, we must call bdrv_invalidate_cache before bdrv_set_perm to
support "-incoming defer" case.

Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 nbd/server.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 82a78bf439..b68b6fb535 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1045,11 +1045,22 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp)
 {
+    AioContext  *ctx;
     BlockBackend *blk;
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
     uint64_t perm;
     int ret;
 
+    /*
+     * NBD exports are used for non-shared storage migration.  Make sure
+     * that BDRV_O_INACTIVE is cleared and the image is ready for write
+     * access since the export could be available before migration handover.
+     */
+    ctx = bdrv_get_aio_context(bs);
+    aio_context_acquire(ctx);
+    bdrv_invalidate_cache(bs, NULL);
+    aio_context_release(ctx);
+
     /* Don't allow resize while the NBD server is running, otherwise we don't
      * care what happens with the node. */
     perm = BLK_PERM_CONSISTENT_READ;
@@ -1087,15 +1098,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
         exp->eject_notifier.notify = nbd_eject_notifier;
         blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
     }
-
-    /*
-     * NBD exports are used for non-shared storage migration.  Make sure
-     * that BDRV_O_INACTIVE is cleared and the image is ready for write
-     * access since the export could be available before migration handover.
-     */
-    aio_context_acquire(exp->ctx);
-    blk_invalidate_cache(blk, NULL);
-    aio_context_release(exp->ctx);
     return exp;
 
 fail:
-- 
2.13.4

Re: [Qemu-devel] [PATCH for-2.10 v2 2/4] nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cache

[Eric Blake]

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

On 08/15/2017 08:07 AM, Fam Zheng wrote:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> The "inactive" state of BDS affects whether the permissions can be
> granted, we must call bdrv_invalidate_cache before bdrv_set_perm to
> support "-incoming defer" case.
> 
> Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  nbd/server.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 82a78bf439..b68b6fb535 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1045,11 +1045,22 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
>                            bool writethrough, BlockBackend *on_eject_blk,
>                            Error **errp)
>  {
> +    AioContext  *ctx;

Odd spacing; can fix up as maintainer.

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

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


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

[Qemu-devel] [PATCH for-2.10 v2 3/4] block-backend: Defer shared_perm tightening migration completion

[Fam Zheng]

As in the case of nbd_export_new(), bdrv_invalidate_cache() can be
called when migration is still in progress. In this case we are not
ready to tighten the shared permissions fenced by blk->disable_perm.

Defer to a VM state change handler.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/block-backend.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 968438c149..e9798e897d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -20,6 +20,7 @@
 #include "qapi-event.h"
 #include "qemu/id.h"
 #include "trace.h"
+#include "migration/misc.h"
 
 /* Number of coroutines to reserve per attached device model */
 #define COROUTINE_POOL_RESERVATION 64
@@ -68,6 +69,7 @@ struct BlockBackend {
     NotifierList remove_bs_notifiers, insert_bs_notifiers;
 
     int quiesce_counter;
+    VMChangeStateEntry *vmsh;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -129,6 +131,23 @@ static const char *blk_root_get_name(BdrvChild *child)
     return blk_name(child->opaque);
 }
 
+static void blk_vm_state_changed(void *opaque, int running, RunState state)
+{
+    Error *local_err = NULL;
+    BlockBackend *blk = opaque;
+
+    if (state == RUN_STATE_INMIGRATE) {
+        return;
+    }
+
+    qemu_del_vm_change_state_handler(blk->vmsh);
+    blk->vmsh = NULL;
+    blk_set_perm(blk, blk->perm, blk->shared_perm, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    }
+}
+
 /*
  * Notifies the user of the BlockBackend that migration has completed. qdev
  * devices can tighten their permissions in response (specifically revoke
@@ -147,6 +166,24 @@ static void blk_root_activate(BdrvChild *child, Error **errp)
 
     blk->disable_perm = false;
 
+    blk_set_perm(blk, blk->perm, BLK_PERM_ALL, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        blk->disable_perm = true;
+        return;
+    }
+
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        /* Activation can happen when migration process is still active, for
+         * example when nbd_server_add is called during non-shared storage
+         * migration. Defer the shared_perm update to migration completion. */
+        if (!blk->vmsh) {
+            blk->vmsh = qemu_add_vm_change_state_handler(blk_vm_state_changed,
+                                                         blk);
+        }
+        return;
+    }
+
     blk_set_perm(blk, blk->perm, blk->shared_perm, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -291,6 +328,10 @@ static void blk_delete(BlockBackend *blk)
     if (blk->root) {
         blk_remove_bs(blk);
     }
+    if (blk->vmsh) {
+        qemu_del_vm_change_state_handler(blk->vmsh);
+        blk->vmsh = NULL;
+    }
     assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
     assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
     QTAILQ_REMOVE(&block_backends, blk, link);
-- 
2.13.4

Re: [Qemu-devel] [PATCH for-2.10 v2 3/4] block-backend: Defer shared_perm tightening migration completion

[Eric Blake]

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

On 08/15/2017 08:07 AM, Fam Zheng wrote:
> As in the case of nbd_export_new(), bdrv_invalidate_cache() can be
> called when migration is still in progress. In this case we are not
> ready to tighten the shared permissions fenced by blk->disable_perm.
> 
> Defer to a VM state change handler.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/block-backend.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)

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

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


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

[Qemu-devel] [PATCH for-2.10 v2 4/4] iotests: Add non-shared storage migration case 192

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/192     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/192.out |  7 ++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 71 insertions(+)
 create mode 100755 tests/qemu-iotests/192
 create mode 100644 tests/qemu-iotests/192.out

diff --git a/tests/qemu-iotests/192 b/tests/qemu-iotests/192
new file mode 100755
index 0000000000..b50a2c0c8e
--- /dev/null
+++ b/tests/qemu-iotests/192
@@ -0,0 +1,63 @@
+#!/bin/bash
+#
+# Test NBD export with -incoming (non-shared storage migration use case from
+# libvirt)
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=famz@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+    _notrun "Requires a PC machine"
+fi
+
+size=64M
+_make_test_img $size
+
+{
+echo "nbd_server_start unix:$TEST_DIR/nbd"
+echo "nbd_server_add -w drive0"
+echo "q"
+} | $QEMU -nodefaults -display none -monitor stdio \
+    -drive format=$IMGFMT,file=$TEST_IMG,if=ide,id=drive0 \
+    -incoming defer 2>&1 | _filter_testdir | _filter_qemu | _filter_hmp
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/192.out b/tests/qemu-iotests/192.out
new file mode 100644
index 0000000000..1e0be4c4d7
--- /dev/null
+++ b/tests/qemu-iotests/192.out
@@ -0,0 +1,7 @@
+QA output created by 192
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) nbd_server_start unix:TEST_DIR/nbd
+(qemu) nbd_server_add -w drive0
+(qemu) q
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1848077932..afbdc427ea 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -186,3 +186,4 @@
 188 rw auto quick
 189 rw auto
 190 rw auto quick
+192 rw auto quick
-- 
2.13.4

Re: [Qemu-devel] [PATCH for-2.10 v2 4/4] iotests: Add non-shared storage migration case 192

[Eric Blake]

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

On 08/15/2017 08:07 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/192     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/192.out |  7 ++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 71 insertions(+)
>  create mode 100755 tests/qemu-iotests/192
>  create mode 100644 tests/qemu-iotests/192.out

I tested that this catches the changes made in both 2/4 (+Block node is
read-only) and 3/4 (+Conflicts with use by drive0 as 'root', which does
not allow 'write' on #block121) - so it looks like we've solved the issue.

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

I'll take this series through my NBD tree, and send a pull request
shortly in time for -rc3

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


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

Re: [Qemu-devel] [PATCH for-2.10 v2 0/4] block: Fix non-shared storage migration

[Christian Ehrhardt]

On Tue, Aug 15, 2017 at 3:07 PM, Fam Zheng <famz@redhat.com> wrote:

> v2: Don't leak blk->vmsh when BB is deleted before the callback is called.
>     [Stefan]
>     From stub functions, don't return g_malloc0(1) which is risky, return
> NULL.
>     [Eric]


Thanks Fam Zheng and Kevin Wolf,
retesting on -rc3 clearly got me further.

Unfortunately the overall test of a libvirt controlled migration with
--copy-storage-all or --copy-storage-inc still does not work.
I have no good pointers yet where to look at, but wanted to give a heads up.

I'm tracking what I have in [1] so far and added upstream qemu as bug task
as well until we know better.

[1]: https://bugs.launchpad.net/qemu/+bug/1711602