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

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

[Fam Zheng]

"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        | 37 ++++++++++++++++++++++++++
 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, 134 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 1/4] stubs: Add vm state change handler stubs

[Fam Zheng]

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..9833ba4e94
--- /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 g_malloc0(1);
+}
+
+void qemu_del_vm_change_state_handler(VMChangeStateEntry *e)
+{
+    g_free(e);
+}
-- 
2.13.4

[Qemu-devel] [PATCH for-2.10 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

[Qemu-devel] [PATCH for-2.10 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 | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 968438c149..237dd5135d 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);
-- 
2.13.4

[Qemu-devel] [PATCH for-2.10 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 3/4] block-backend: Defer shared_perm tightening migration completion

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2017 at 12:04:53PM +0800, Fam Zheng wrote:
> @@ -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);

Please add a qemu_del_vm_change_state_handler() call to cover the case
where the BB is deleted before the migration state changes.

This is necessary to prevent a memory leak and a crash when the change
state handler is invoked.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Eric Blake]

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

On 08/14/2017 11:04 PM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>

A bit sparse on the 'why' - presumably, upcoming patches will fail to
compile if the stub is not present, but mentioning what dependency this
solves never hurts.

> ---
>  stubs/Makefile.objs          |  1 +
>  stubs/change-state-handler.c | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
>  create mode 100644 stubs/change-state-handler.c
> 

> +++ 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 g_malloc0(1);
> +}

Hmm - this is NOT a VMChangeStateEntry; if it ever gets dereferenced,
the caller is (probably) accessing memory out of bounds.  Presumably,
since it is a stub, this should never be called - and if that's the
case, can we just get away with returning NULL instead (I'd rather have
the caller SEGFAULT than dereference out-of-bounds into the heap, if
this stub gets used inappropriately).

> +
> +void qemu_del_vm_change_state_handler(VMChangeStateEntry *e)
> +{
> +    g_free(e);

And of course, if you don't allocate anything, this can be a no-op.

> +}
> 

-- 
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 1/4] stubs: Add vm state change handler stubs

[Fam Zheng]

On Tue, 08/15 07:26, Eric Blake wrote:
> On 08/14/2017 11:04 PM, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> A bit sparse on the 'why' - presumably, upcoming patches will fail to
> compile if the stub is not present, but mentioning what dependency this
> solves never hurts.
> 
> > ---
> >  stubs/Makefile.objs          |  1 +
> >  stubs/change-state-handler.c | 14 ++++++++++++++
> >  2 files changed, 15 insertions(+)
> >  create mode 100644 stubs/change-state-handler.c
> > 
> 
> > +++ 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 g_malloc0(1);
> > +}
> 
> Hmm - this is NOT a VMChangeStateEntry; if it ever gets dereferenced,
> the caller is (probably) accessing memory out of bounds.  Presumably,
> since it is a stub, this should never be called - and if that's the
> case, can we just get away with returning NULL instead (I'd rather have
> the caller SEGFAULT than dereference out-of-bounds into the heap, if
> this stub gets used inappropriately).

Good point, will update this patch.

> 
> > +
> > +void qemu_del_vm_change_state_handler(VMChangeStateEntry *e)
> > +{
> > +    g_free(e);
> 
> And of course, if you don't allocate anything, this can be a no-op.
> 
> > +}
> > 
> 

Fam