Re: [Qemu-block] [PATCH 2/5] The discard flag for block stream operation

2018-11-26 Thread Eric Blake

On 11/22/18 12:48 PM, Andrey Shinkevich wrote:

Adding a parameter to QMP block-stream command to allow discarding
blocks in the backing chain while blocks are being copied to the
active layer.

Signed-off-by: Andrey Shinkevich 
---
  block/stream.c| 3 +--
  blockdev.c| 8 +++-
  hmp-commands.hx   | 4 ++--
  hmp.c | 4 +++-
  include/block/block_int.h | 2 +-
  qapi/block-core.json  | 5 -
  6 files changed, 18 insertions(+), 8 deletions(-)




+++ b/qapi/block-core.json
@@ -2334,6 +2334,9 @@
  #
  # @speed:  the maximum speed, in bytes per second
  #
+# @discard: true to delete blocks duplicated in old backing files.
+#   (default: false). Since 3.1.
+#


This feels like a feature addition and not a bug fix, so not appropriate 
for 3.1-rc3.  The next release will be 4.0, so the "since" line should 
be updated accordingly.


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



Re: [Qemu-block] [PATCH] nvme: Fix spurious interrupts

2018-11-26 Thread Kevin Wolf
Am 26.11.2018 um 18:17 hat Keith Busch geschrieben:
> The code had asserted an interrupt every time it was requested to check
> for new completion queue entries.This can result in spurious interrupts
> seen by the guest OS.
> 
> Fix this by asserting an interrupt only if there are un-acknowledged
> completion queue entries available.
> 
> Reported-by: Guenter Roeck 
> Signed-off-by: Keith Busch 

Thanks, applied to the block branch.

Kevin



Re: [Qemu-block] [PATCH] nvme: Fix spurious interrupts

2018-11-26 Thread Guenter Roeck
On Mon, Nov 26, 2018 at 10:17:45AM -0700, Keith Busch wrote:
> The code had asserted an interrupt every time it was requested to check
> for new completion queue entries.This can result in spurious interrupts
> seen by the guest OS.
> 
> Fix this by asserting an interrupt only if there are un-acknowledged
> completion queue entries available.
> 
> Reported-by: Guenter Roeck 
> Signed-off-by: Keith Busch 

Tested-by: Guenter Roeck 

> ---
>  hw/block/nvme.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 9fbe5673cb..7c8c63e8f5 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
>  sizeof(req->cqe));
>  QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
>  }
> -nvme_irq_assert(n, cq);
> +if (cq->tail != cq->head) {
> +nvme_irq_assert(n, cq);
> +}
>  }
>  
>  static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> -- 
> 2.14.4
> 



[Qemu-block] [PATCH] nvme: Fix spurious interrupts

2018-11-26 Thread Keith Busch
The code had asserted an interrupt every time it was requested to check
for new completion queue entries.This can result in spurious interrupts
seen by the guest OS.

Fix this by asserting an interrupt only if there are un-acknowledged
completion queue entries available.

Reported-by: Guenter Roeck 
Signed-off-by: Keith Busch 
---
 hw/block/nvme.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9fbe5673cb..7c8c63e8f5 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
 sizeof(req->cqe));
 QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
 }
-nvme_irq_assert(n, cq);
+if (cq->tail != cq->head) {
+nvme_irq_assert(n, cq);
+}
 }
 
 static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
-- 
2.14.4




Re: [Qemu-block] [PATCH] nvme: Only generate interupt if warranted

2018-11-26 Thread Guenter Roeck
Hi Keith,

On Mon, Nov 26, 2018 at 09:17:17AM -0700, Keith Busch wrote:
> On Sun, Nov 25, 2018 at 03:03:55PM -0800, Guenter Roeck wrote:
> > So far the code generates interrupts even if it doesn't pass any new
> > information to the user. This can result in spurious interrupts as
> > sometimes observed with Linux.
> > 
> > Only generate interrupts if warranted, ie if anything new is passed
> > to the emulated code.
> > 
> > Signed-off-by: Guenter Roeck 
> > ---
> >  hw/block/nvme.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 9fbe567..c543c01 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -252,6 +252,7 @@ static void nvme_post_cqes(void *opaque)
> >  NvmeCQueue *cq = opaque;
> >  NvmeCtrl *n = cq->ctrl;
> >  NvmeRequest *req, *next;
> > +bool assert_irq = false;
> >  
> >  QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
> >  NvmeSQueue *sq;
> > @@ -271,8 +272,11 @@ static void nvme_post_cqes(void *opaque)
> >  pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe,
> >  sizeof(req->cqe));
> >  QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
> > +assert_irq = true;
> > +}
> > +if (assert_irq) {
> > +nvme_irq_assert(n, cq);
> >  }
> > -nvme_irq_assert(n, cq);
> >  }
> >  
> >  static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> > -- 
> 
> There is a corner case this may break. For example, a controller posts
> 2 completions. The host happens to only see one of those completions due
> to the inherent race when reading the phase bit. After the host updates
> the CQ DB, the controller ought to send another interrupt even if it
> didn't post any new CQEs to prevent command completion timeouts. This
> might get the right behavior:
> 
> ---
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..486db6e561 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
>  sizeof(req->cqe));
>  QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
>  }
> -nvme_irq_assert(n, cq);
> +if (cq->tail != cq->head) {
> +nvme_irq_assert(n, cq);
> +}
>  }
> 
That works for me as well, and looks much cleaner. Should I resubmit,
or do you want to take it from here ?

Thanks,
Guenter



Re: [Qemu-block] [PATCH] nvme: Only generate interupt if warranted

2018-11-26 Thread Keith Busch
On Sun, Nov 25, 2018 at 03:03:55PM -0800, Guenter Roeck wrote:
> So far the code generates interrupts even if it doesn't pass any new
> information to the user. This can result in spurious interrupts as
> sometimes observed with Linux.
> 
> Only generate interrupts if warranted, ie if anything new is passed
> to the emulated code.
> 
> Signed-off-by: Guenter Roeck 
> ---
>  hw/block/nvme.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 9fbe567..c543c01 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -252,6 +252,7 @@ static void nvme_post_cqes(void *opaque)
>  NvmeCQueue *cq = opaque;
>  NvmeCtrl *n = cq->ctrl;
>  NvmeRequest *req, *next;
> +bool assert_irq = false;
>  
>  QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
>  NvmeSQueue *sq;
> @@ -271,8 +272,11 @@ static void nvme_post_cqes(void *opaque)
>  pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe,
>  sizeof(req->cqe));
>  QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
> +assert_irq = true;
> +}
> +if (assert_irq) {
> +nvme_irq_assert(n, cq);
>  }
> -nvme_irq_assert(n, cq);
>  }
>  
>  static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> -- 

There is a corner case this may break. For example, a controller posts
2 completions. The host happens to only see one of those completions due
to the inherent race when reading the phase bit. After the host updates
the CQ DB, the controller ought to send another interrupt even if it
didn't post any new CQEs to prevent command completion timeouts. This
might get the right behavior:

---
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb816..486db6e561 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
 sizeof(req->cqe));
 QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
 }
-nvme_irq_assert(n, cq);
+if (cq->tail != cq->head) {
+nvme_irq_assert(n, cq);
+}
 }

 static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
--



Re: [Qemu-block] [PATCH for-3.1 v3 0/2] block: Fix crash on migration with explicit child nodes

2018-11-26 Thread Max Reitz
On 26.11.18 17:05, Kevin Wolf wrote:
> v3:
> - Checking whether there are still active parents only really works with
>   a single pass, so that's what we do now [Max]
> - Test case polishing [Max]
> 
> v2:
> - Cover children that were created after their parents [Max]
> 
> Kevin Wolf (2):
>   block: Don't inactivate children before parents
>   iotests: Test migration with -blockdev
> 
>  block.c|  84 +++--
>  tests/qemu-iotests/234 | 121 +
>  tests/qemu-iotests/234.out |  30 +
>  tests/qemu-iotests/group   |   1 +
>  4 files changed, 205 insertions(+), 31 deletions(-)
>  create mode 100755 tests/qemu-iotests/234
>  create mode 100644 tests/qemu-iotests/234.out

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature


[Qemu-block] [PATCH for-3.1 v3 0/2] block: Fix crash on migration with explicit child nodes

2018-11-26 Thread Kevin Wolf
v3:
- Checking whether there are still active parents only really works with
  a single pass, so that's what we do now [Max]
- Test case polishing [Max]

v2:
- Cover children that were created after their parents [Max]

Kevin Wolf (2):
  block: Don't inactivate children before parents
  iotests: Test migration with -blockdev

 block.c|  84 +++--
 tests/qemu-iotests/234 | 121 +
 tests/qemu-iotests/234.out |  30 +
 tests/qemu-iotests/group   |   1 +
 4 files changed, 205 insertions(+), 31 deletions(-)
 create mode 100755 tests/qemu-iotests/234
 create mode 100644 tests/qemu-iotests/234.out

-- 
2.19.1




[Qemu-block] [PATCH for-3.1 v3 1/2] block: Don't inactivate children before parents

2018-11-26 Thread Kevin Wolf
bdrv_child_cb_inactivate() asserts that parents are already inactive
when children get inactivated. This precondition is necessary because
parents could still issue requests in their inactivation code.

When block nodes are created individually with -blockdev, all of them
are monitor owned and will be returned by bdrv_next() in an undefined
order (in practice, in the order of their creation, which is usually
children before parents), which obviously fails the assertion:

qemu: block.c:899: bdrv_child_cb_inactivate: Assertion `bs->open_flags & 
BDRV_O_INACTIVE' failed.

This patch fixes the ordering by skipping nodes with still active
parents in bdrv_inactivate_recurse() because we know that they will be
covered by recursion when the last active parent becomes inactive.

With the correct parents-before-children ordering, we also got rid of
the reason why commit aad0b7a0bfb introduced two passes, so we can go
back to a single-pass recursion. This is necessary so we can rely on the
BDRV_O_INACTIVE flag to skip nodes with active parents (the flag used
to be set only in pass 2, so we would always skip non-root nodes in
pass 1 because all parents would still be considered active; setting the
flag in pass 1 would mean, that we never skip anything in pass 2 because
all parents are already considered inactive).

Because of the change to single pass, this patch is best reviewed with
whitespace changes ignored.

Signed-off-by: Kevin Wolf 
---
 block.c | 84 -
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/block.c b/block.c
index 5ba3435f8f..811239ca23 100644
--- a/block.c
+++ b/block.c
@@ -4612,45 +4612,68 @@ void bdrv_invalidate_cache_all(Error **errp)
 }
 }
 
-static int bdrv_inactivate_recurse(BlockDriverState *bs,
-   bool setting_flag)
+static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
+{
+BdrvChild *parent;
+
+QLIST_FOREACH(parent, &bs->parents, next_parent) {
+if (parent->role->parent_is_bds) {
+BlockDriverState *parent_bs = parent->opaque;
+if (!only_active || !(parent_bs->open_flags & BDRV_O_INACTIVE)) {
+return true;
+}
+}
+}
+
+return false;
+}
+
+static int bdrv_inactivate_recurse(BlockDriverState *bs)
 {
 BdrvChild *child, *parent;
+uint64_t perm, shared_perm;
 int ret;
 
 if (!bs->drv) {
 return -ENOMEDIUM;
 }
 
-if (!setting_flag && bs->drv->bdrv_inactivate) {
+/* Make sure that we don't inactivate a child before its parent.
+ * It will be covered by recursion from the yet active parent. */
+if (bdrv_has_bds_parent(bs, true)) {
+return 0;
+}
+
+assert(!(bs->open_flags & BDRV_O_INACTIVE));
+
+/* Inactivate this node */
+if (bs->drv->bdrv_inactivate) {
 ret = bs->drv->bdrv_inactivate(bs);
 if (ret < 0) {
 return ret;
 }
 }
 
-if (setting_flag && !(bs->open_flags & BDRV_O_INACTIVE)) {
-uint64_t perm, shared_perm;
-
-QLIST_FOREACH(parent, &bs->parents, next_parent) {
-if (parent->role->inactivate) {
-ret = parent->role->inactivate(parent);
-if (ret < 0) {
-return ret;
-}
+QLIST_FOREACH(parent, &bs->parents, next_parent) {
+if (parent->role->inactivate) {
+ret = parent->role->inactivate(parent);
+if (ret < 0) {
+return ret;
 }
 }
+}
 
-bs->open_flags |= BDRV_O_INACTIVE;
+bs->open_flags |= BDRV_O_INACTIVE;
+
+/* Update permissions, they may differ for inactive nodes */
+bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
+bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);
+bdrv_set_perm(bs, perm, shared_perm);
 
-/* Update permissions, they may differ for inactive nodes */
-bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);
-bdrv_set_perm(bs, perm, shared_perm);
-}
 
+/* Recursively inactivate children */
 QLIST_FOREACH(child, &bs->children, next) {
-ret = bdrv_inactivate_recurse(child->bs, setting_flag);
+ret = bdrv_inactivate_recurse(child->bs);
 if (ret < 0) {
 return ret;
 }
@@ -4664,7 +4687,6 @@ int bdrv_inactivate_all(void)
 BlockDriverState *bs = NULL;
 BdrvNextIterator it;
 int ret = 0;
-int pass;
 GSList *aio_ctxs = NULL, *ctx;
 
 for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
@@ -4676,17 +4698,17 @@ int bdrv_inactivate_all(void)
 }
 }
 
-/* We do two passes of inactivation. The first pass calls to drivers'
- * .bdrv_inactivate callbacks recursively so all cache is flushed to disk;
- * the second pass sets the BDRV_O_INACTIVE flag so that no further write
- *

[Qemu-block] [PATCH for-3.1 v3 2/2] iotests: Test migration with -blockdev

2018-11-26 Thread Kevin Wolf
Check that block node activation and inactivation works with a block
graph that is built with individually created nodes.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/234 | 121 +
 tests/qemu-iotests/234.out |  30 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 152 insertions(+)
 create mode 100755 tests/qemu-iotests/234
 create mode 100644 tests/qemu-iotests/234.out

diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
new file mode 100755
index 00..a8185b4360
--- /dev/null
+++ b/tests/qemu-iotests/234
@@ -0,0 +1,121 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2018 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 .
+#
+# Creator/Owner: Kevin Wolf 
+#
+# Check that block node activation and inactivation works with a block graph
+# that is built with individually created nodes
+
+import iotests
+import os
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.verify_platform(['linux'])
+
+with iotests.FilePath('img') as img_path, \
+ iotests.FilePath('backing') as backing_path, \
+ iotests.FilePath('mig_fifo_a') as fifo_a, \
+ iotests.FilePath('mig_fifo_b') as fifo_b, \
+ iotests.VM(path_suffix='a') as vm_a, \
+ iotests.VM(path_suffix='b') as vm_b:
+
+iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, backing_path, '64M')
+iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+
+os.mkfifo(fifo_a)
+os.mkfifo(fifo_b)
+
+iotests.log('Launching source VM...')
+(vm_a.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
+ .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
(iotests.imgfmt))
+ .add_blockdev('file,filename=%s,node-name=drive0-backing-file' % 
(backing_path))
+ .add_blockdev('%s,file=drive0-backing-file,node-name=drive0-backing' 
% (iotests.imgfmt))
+ .launch())
+
+iotests.log('Launching destination VM...')
+(vm_b.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
+ .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
(iotests.imgfmt))
+ .add_blockdev('file,filename=%s,node-name=drive0-backing-file' % 
(backing_path))
+ .add_blockdev('%s,file=drive0-backing-file,node-name=drive0-backing' 
% (iotests.imgfmt))
+ .add_incoming("exec: cat '%s'" % (fifo_a))
+ .launch())
+
+# Add a child node that was created after the parent node. The reverse case
+# is covered by the -blockdev options above.
+iotests.log(vm_a.qmp('blockdev-snapshot', node='drive0-backing',
+ overlay='drive0'))
+iotests.log(vm_b.qmp('blockdev-snapshot', node='drive0-backing',
+ overlay='drive0'))
+
+iotests.log('Enabling migration QMP events on A...')
+iotests.log(vm_a.qmp('migrate-set-capabilities', capabilities=[
+{
+'capability': 'events',
+'state': True
+}
+]))
+
+iotests.log('Starting migration to B...')
+iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo_a)))
+with iotests.Timeout(3, 'Migration does not complete'):
+while True:
+event = vm_a.event_wait('MIGRATION')
+iotests.log(event, filters=[iotests.filter_qmp_event])
+if event['data']['status'] == 'completed':
+break
+
+iotests.log(vm_a.qmp('query-migrate')['return']['status'])
+iotests.log(vm_b.qmp('query-migrate')['return']['status'])
+
+iotests.log(vm_a.qmp('query-status'))
+iotests.log(vm_b.qmp('query-status'))
+
+iotests.log('Add a second parent to drive0-file...')
+iotests.log(vm_b.qmp('blockdev-add', driver='raw', file='drive0-file',
+ node_name='drive0-raw'))
+
+iotests.log('Restart A with -incoming and second parent...')
+vm_a.shutdown()
+(vm_a.add_blockdev('raw,file=drive0-file,node-name=drive0-raw')
+ .add_incoming("exec: cat '%s'" % (fifo_b))
+ .launch())
+
+iotests.log(vm_a.qmp('blockdev-snapshot', node='drive0-backing',
+ overlay='drive0'))
+
+iotests.log('Enabling migration QMP events on B...')
+iotests.log(vm_b.qmp('migrate-set-capabilities', capabilities=[
+{
+'capability': 'events',
+'state': True
+}
+]))
+
+iotests.log('Starting migration bac

Re: [Qemu-block] [PATCH for-3.1 v2 2/2] iotests: Test migration with -blockdev

2018-11-26 Thread Max Reitz
On 26.11.18 15:16, Kevin Wolf wrote:
> Check that block node activation and inactivation works with a block
> graph that is built with individually created nodes.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/234 | 115 +
>  tests/qemu-iotests/234.out |  27 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 143 insertions(+)
>  create mode 100755 tests/qemu-iotests/234
>  create mode 100644 tests/qemu-iotests/234.out
> 
> diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
> new file mode 100755
> index 00..58886b8dea
> --- /dev/null
> +++ b/tests/qemu-iotests/234
> @@ -0,0 +1,115 @@
> +#!/usr/bin/env python
> +#
> +# Copyright (C) 2018 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 .
> +#
> +# Creator/Owner: Kevin Wolf 
> +#
> +# Check that block node activation and inactivation works with a block graph
> +# that is built with individually created nodes
> +
> +import iotests
> +import os
> +
> +iotests.verify_image_format(supported_fmts=['raw', 'qcow2'])
> +iotests.verify_platform(['linux'])
> +
> +with iotests.FilePath('img') as img_path, \
> + iotests.FilePath('backing') as backing_path, \
> + iotests.FilePath('mig_fifo_a') as fifo_a, \
> + iotests.FilePath('mig_fifo_b') as fifo_b, \
> + iotests.VM(path_suffix='a') as vm_a, \
> + iotests.VM(path_suffix='b') as vm_b:
> +
> +iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, backing_path, 
> '64M')
> +iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
> +
> +os.mkfifo(fifo_a)
> +os.mkfifo(fifo_b)
> +
> +iotests.log('Launching source VM...')
> +(vm_a.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
> + .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
> (iotests.imgfmt))
> + .add_blockdev('file,filename=%s,node-name=drive0-backing-file' % 
> (backing_path))
> + 
> .add_blockdev('%s,file=drive0-backing-file,node-name=drive0-backing' % 
> (iotests.imgfmt))
> + .launch())
> +
> +iotests.log('Launching destination VM...')
> +(vm_b.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
> + .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
> (iotests.imgfmt))
> + .add_blockdev('file,filename=%s,node-name=drive0-backing-file' % 
> (backing_path))
> + 
> .add_blockdev('%s,file=drive0-backing-file,node-name=drive0-backing' % 
> (iotests.imgfmt))
> + .add_incoming("exec: cat '%s'" % (fifo_a))
> + .launch())
> +
> +# Add a child node that was created after the parent node. The reverse 
> case
> +# is covered by the -blockdev options above.
> +vm_a.qmp('blockdev-snapshot', node='drive0-backing', overlay='drive0')

Sorry if I made it sound like my diff was ready for inclusion as-is. O:-)

I think this should be vm_a.qmp_log() at least (and the reference output
needs to be adjusted accordingly).

Also, we probably have to execute the same command on vm_b...

> +iotests.log('Enabling migration QMP events on A...')
> +iotests.log(vm_a.qmp('migrate-set-capabilities', capabilities=[
> +{
> +'capability': 'events',
> +'state': True
> +}
> +]))
> +
> +iotests.log('Starting migration to B...')
> +iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo_a)))
> +with iotests.Timeout(3, 'Migration does not complete'):
> +while True:
> +event = vm_a.event_wait('MIGRATION')
> +iotests.log(event, filters=[iotests.filter_qmp_event])
> +if event['data']['status'] == 'completed':
> +break
> +
> +iotests.log(vm_a.qmp('query-migrate')['return']['status'])
> +iotests.log(vm_b.qmp('query-migrate')['return']['status'])
> +
> +iotests.log(vm_a.qmp('query-status'))
> +iotests.log(vm_b.qmp('query-status'))
> +
> +iotests.log('Add a second parent to drive0-file...')
> +iotests.log(vm_b.qmp('blockdev-add', driver='raw', file='drive0-file',
> + node_name='drive0-raw'))
> +
> +iotests.log('Restart A with -incoming and second parent...')
> +vm_a.shutdown()
> +(vm_a.add_blockdev('raw,file=drive0-file,node-name=drive0-raw')
> + .add_incoming("exec: cat '%s'" % (fifo_b))
> + .launch())

...a

Re: [Qemu-block] [PATCH for-3.1 v2 1/2] block: Don't inactivate children before parents

2018-11-26 Thread Max Reitz
On 26.11.18 15:16, Kevin Wolf wrote:
> bdrv_child_cb_inactivate() asserts that parents are already inactive
> when children get inactivated. This precondition is necessary because
> parents could still issue requests in their inactivation code.
> 
> When block nodes are created individually with -blockdev, all of them
> are monitor owned and will be returned by bdrv_next() in an undefined
> order (in practice, in the order of their creation, which is usually
> children before parents), which obviously fails the assertion:
> 
> qemu: block.c:899: bdrv_child_cb_inactivate: Assertion `bs->open_flags & 
> BDRV_O_INACTIVE' failed.
> 
> This patch fixes the ordering by skipping nodes with still active
> parents in bdrv_inactivate_recurse() because we know that they will be
> covered by recursion when the last active parent becomes inactive.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 32 +++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 5ba3435f8f..e9181c3be7 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -4622,6 +4638,14 @@ static int bdrv_inactivate_recurse(BlockDriverState 
> *bs,
>  return -ENOMEDIUM;
>  }
>  
> +/* Make sure that we don't inactivate a child before its parent.
> + * It will be covered by recursion from the yet active parent. */
> +if (bdrv_has_bds_parent(bs, true)) {

Overall the patch looks good to me, apart from the fact that I still
think this should only be checked if setting_flag is true.

(Because BDRV_O_INACTIVE is only set during the second pass, so this
check will lead to the first pass not doing anything (and thus not
calling bs->drv->bdrv_inactivate()) on any non-root bs.)

Max

> +return 0;
> +}
> +
> +assert(!(bs->open_flags & BDRV_O_INACTIVE));
> +
>  if (!setting_flag && bs->drv->bdrv_inactivate) {
>  ret = bs->drv->bdrv_inactivate(bs);
>  if (ret < 0) {



signature.asc
Description: OpenPGP digital signature


[Qemu-block] [PATCH for-3.1 v2 1/2] block: Don't inactivate children before parents

2018-11-26 Thread Kevin Wolf
bdrv_child_cb_inactivate() asserts that parents are already inactive
when children get inactivated. This precondition is necessary because
parents could still issue requests in their inactivation code.

When block nodes are created individually with -blockdev, all of them
are monitor owned and will be returned by bdrv_next() in an undefined
order (in practice, in the order of their creation, which is usually
children before parents), which obviously fails the assertion:

qemu: block.c:899: bdrv_child_cb_inactivate: Assertion `bs->open_flags & 
BDRV_O_INACTIVE' failed.

This patch fixes the ordering by skipping nodes with still active
parents in bdrv_inactivate_recurse() because we know that they will be
covered by recursion when the last active parent becomes inactive.

Signed-off-by: Kevin Wolf 
---
 block.c | 32 +++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 5ba3435f8f..e9181c3be7 100644
--- a/block.c
+++ b/block.c
@@ -4612,6 +4612,22 @@ void bdrv_invalidate_cache_all(Error **errp)
 }
 }
 
+static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
+{
+BdrvChild *parent;
+
+QLIST_FOREACH(parent, &bs->parents, next_parent) {
+if (parent->role->parent_is_bds) {
+BlockDriverState *parent_bs = parent->opaque;
+if (!only_active || !(parent_bs->open_flags & BDRV_O_INACTIVE)) {
+return true;
+}
+}
+}
+
+return false;
+}
+
 static int bdrv_inactivate_recurse(BlockDriverState *bs,
bool setting_flag)
 {
@@ -4622,6 +4638,14 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
 return -ENOMEDIUM;
 }
 
+/* Make sure that we don't inactivate a child before its parent.
+ * It will be covered by recursion from the yet active parent. */
+if (bdrv_has_bds_parent(bs, true)) {
+return 0;
+}
+
+assert(!(bs->open_flags & BDRV_O_INACTIVE));
+
 if (!setting_flag && bs->drv->bdrv_inactivate) {
 ret = bs->drv->bdrv_inactivate(bs);
 if (ret < 0) {
@@ -4629,7 +4653,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
 }
 }
 
-if (setting_flag && !(bs->open_flags & BDRV_O_INACTIVE)) {
+if (setting_flag) {
 uint64_t perm, shared_perm;
 
 QLIST_FOREACH(parent, &bs->parents, next_parent) {
@@ -4682,6 +4706,12 @@ int bdrv_inactivate_all(void)
  * is allowed. */
 for (pass = 0; pass < 2; pass++) {
 for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+/* Nodes with BDS parents are covered by recursion from the last
+ * parent that gets inactivated. Don't inactivate them a second
+ * time if that has already happened. */
+if (bdrv_has_bds_parent(bs, false)) {
+continue;
+}
 ret = bdrv_inactivate_recurse(bs, pass);
 if (ret < 0) {
 bdrv_next_cleanup(&it);
-- 
2.19.1




[Qemu-block] [PATCH for-3.1 v2 0/2] block: Fix crash on migration with explicit child nodes

2018-11-26 Thread Kevin Wolf
v2:
- Cover children that were created after their parents [Max]

Kevin Wolf (2):
  block: Don't inactivate children before parents
  iotests: Test migration with -blockdev

 block.c|  32 ++-
 tests/qemu-iotests/234 | 115 +
 tests/qemu-iotests/234.out |  27 +
 tests/qemu-iotests/group   |   1 +
 4 files changed, 174 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/234
 create mode 100644 tests/qemu-iotests/234.out

-- 
2.19.1




[Qemu-block] [PATCH for-3.1 v2 2/2] iotests: Test migration with -blockdev

2018-11-26 Thread Kevin Wolf
Check that block node activation and inactivation works with a block
graph that is built with individually created nodes.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/234 | 115 +
 tests/qemu-iotests/234.out |  27 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 143 insertions(+)
 create mode 100755 tests/qemu-iotests/234
 create mode 100644 tests/qemu-iotests/234.out

diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
new file mode 100755
index 00..58886b8dea
--- /dev/null
+++ b/tests/qemu-iotests/234
@@ -0,0 +1,115 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2018 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 .
+#
+# Creator/Owner: Kevin Wolf 
+#
+# Check that block node activation and inactivation works with a block graph
+# that is built with individually created nodes
+
+import iotests
+import os
+
+iotests.verify_image_format(supported_fmts=['raw', 'qcow2'])
+iotests.verify_platform(['linux'])
+
+with iotests.FilePath('img') as img_path, \
+ iotests.FilePath('backing') as backing_path, \
+ iotests.FilePath('mig_fifo_a') as fifo_a, \
+ iotests.FilePath('mig_fifo_b') as fifo_b, \
+ iotests.VM(path_suffix='a') as vm_a, \
+ iotests.VM(path_suffix='b') as vm_b:
+
+iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, backing_path, '64M')
+iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+
+os.mkfifo(fifo_a)
+os.mkfifo(fifo_b)
+
+iotests.log('Launching source VM...')
+(vm_a.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
+ .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
(iotests.imgfmt))
+ .add_blockdev('file,filename=%s,node-name=drive0-backing-file' % 
(backing_path))
+ .add_blockdev('%s,file=drive0-backing-file,node-name=drive0-backing' 
% (iotests.imgfmt))
+ .launch())
+
+iotests.log('Launching destination VM...')
+(vm_b.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
+ .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
(iotests.imgfmt))
+ .add_blockdev('file,filename=%s,node-name=drive0-backing-file' % 
(backing_path))
+ .add_blockdev('%s,file=drive0-backing-file,node-name=drive0-backing' 
% (iotests.imgfmt))
+ .add_incoming("exec: cat '%s'" % (fifo_a))
+ .launch())
+
+# Add a child node that was created after the parent node. The reverse case
+# is covered by the -blockdev options above.
+vm_a.qmp('blockdev-snapshot', node='drive0-backing', overlay='drive0')
+
+iotests.log('Enabling migration QMP events on A...')
+iotests.log(vm_a.qmp('migrate-set-capabilities', capabilities=[
+{
+'capability': 'events',
+'state': True
+}
+]))
+
+iotests.log('Starting migration to B...')
+iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo_a)))
+with iotests.Timeout(3, 'Migration does not complete'):
+while True:
+event = vm_a.event_wait('MIGRATION')
+iotests.log(event, filters=[iotests.filter_qmp_event])
+if event['data']['status'] == 'completed':
+break
+
+iotests.log(vm_a.qmp('query-migrate')['return']['status'])
+iotests.log(vm_b.qmp('query-migrate')['return']['status'])
+
+iotests.log(vm_a.qmp('query-status'))
+iotests.log(vm_b.qmp('query-status'))
+
+iotests.log('Add a second parent to drive0-file...')
+iotests.log(vm_b.qmp('blockdev-add', driver='raw', file='drive0-file',
+ node_name='drive0-raw'))
+
+iotests.log('Restart A with -incoming and second parent...')
+vm_a.shutdown()
+(vm_a.add_blockdev('raw,file=drive0-file,node-name=drive0-raw')
+ .add_incoming("exec: cat '%s'" % (fifo_b))
+ .launch())
+
+iotests.log('Enabling migration QMP events on B...')
+iotests.log(vm_b.qmp('migrate-set-capabilities', capabilities=[
+{
+'capability': 'events',
+'state': True
+}
+]))
+
+iotests.log('Starting migration back to A...')
+iotests.log(vm_b.qmp('migrate', uri='exec:cat >%s' % (fifo_b)))
+with iotests.Timeout(3, 'Migration does not complete'):
+while True:
+event = vm_b.event_wait('MIGRATION')
+iotests.log(event, filters=[iotests.fi

Re: [Qemu-block] [PATCH v5 9/9] qapi: query-blockstat: add driver specific file-posix stats

2018-11-26 Thread Anton Nefedov
On 23/11/2018 10:21 PM, Vladimir Sementsov-Ogievskiy wrote:
> 31.10.2018 14:35, Anton Nefedov wrote:
>> A block driver can provide a callback to report driver-specific
>> statistics.
>>
>> file-posix driver now reports discard statistics
>>
>> Signed-off-by: Anton Nefedov 
>> ---
>>qapi/block-core.json  | 39 +++
>>include/block/block.h |  1 +
>>include/block/block_int.h |  1 +
>>block.c   |  9 +
>>block/file-posix.c| 17 +
>>block/qapi.c  |  5 +
>>6 files changed, 72 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 01da84cb61..cd0344435e 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -877,6 +877,42 @@
>>   '*x_wr_latency_histogram': 'BlockLatencyHistogramInfo',
>>   '*x_flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>>
>> +##
>> +# @BlockStatsSpecificFile:
>> +#
>> +# File driver statistics
>> +#
>> +# @discard-nb-ok: The number of succeeded discard operations performed by
>> +# the driver.
>> +#
>> +# @discard-nb-failed: The number of failed discard operations performed by
>> +# the driver.
>> +#
>> +# @discard-bytes-ok: The number of bytes discarded by the driver.
>> +#
>> +# Since 3.1
> 
> colon after Since:
> Since: 3.1
> 

done

>> +##
>> +{ 'struct': 'BlockStatsSpecificFile',
>> +  'data': {
>> +  'discard-nb-ok': 'int',
>> +  'discard-nb-failed': 'int',
>> +  'discard-bytes-ok': 'int'
>> +  } }
> 
> the common stile here is no extra \n around braces, like:
> 
> { 'struct': 'BlockStatsSpecificFile',
> 'data': { 'discard-nb-ok': 'int',
>   'discard-nb-failed': 'int',
>   'discard-bytes-ok': 'int' } }
> 
> 

Ok this one is more frequent. Fixed.

>> +
>> +##
>> +# @BlockStatsSpecific:
>> +#
>> +# Block driver specific statistics
>> +#
>> +# Since: 3.1
>> +##
>> +{ 'union': 'BlockStatsSpecific',
>> +  'base': { 'driver': 'BlockdevDriver' },
>> +  'discriminator': 'driver',
>> +  'data': {
>> +  'file': 'BlockStatsSpecificFile'
>> +  } }
> 
> and here.
> 

done

>> +
>>##
>># @BlockStats:
>>#
>> @@ -892,6 +928,8 @@
>>#
>># @stats:  A @BlockDeviceStats for the device.
>>#
>> +# @driver-specific: Optional driver-specific stats. (Since 3.1)
>> +#
>># @parent: This describes the file block device if it has one.
>>#  Contains recursively the statistics of the underlying
>>#  protocol (e.g. the host file for a qcow2 image). If there is
>> @@ -905,6 +943,7 @@
>>{ 'struct': 'BlockStats',
>>  'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
>>   'stats': 'BlockDeviceStats',
>> +   '*driver-specific': 'BlockStatsSpecific',
> 
> [offtopic]
> hmm, do anyone know, why thunderbird when quoting patches adds "> " between 
> all lines except ones starting with a space, and for lines, starting with 
> space, it adds ">  " (two extra spaces, not one), which leads to wrong 
> indenting in quotes? And how to fix that?
> [/offtopic]
> 
>>   '*parent': 'BlockStats',
>>   '*backing': 'BlockStats'} }
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index b189cf422e..07a3b31386 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -475,6 +475,7 @@ const char *bdrv_get_device_or_node_name(const 
>> BlockDriverState *bs);
>>int bdrv_get_flags(BlockDriverState *bs);
>>int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
>>ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
>> +BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);
>>void bdrv_round_to_clusters(BlockDriverState *bs,
>>int64_t offset, int64_t bytes,
>>int64_t *cluster_offset,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index f605622216..236d4aceef 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -320,6 +320,7 @@ struct BlockDriver {
>>  Error **errp);
>>int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
>>ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
>> +BlockStatsSpecific *(*bdrv_get_specific_stats)(BlockDriverState *bs);
>>
>>int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
>>  QEMUIOVector *qiov,
>> diff --git a/block.c b/block.c
>> index 95d8635aa1..1e5bba4ac6 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4244,6 +4244,15 @@ ImageInfoSpecific 
>> *bdrv_get_specific_info(BlockDriverState *bs)
>>return NULL;
>>}
>>
>> +BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs)
>> +{
>> +BlockDriver *drv = bs->drv;
>> +if (!drv || !dr

Re: [Qemu-block] [PATCH for-3.1 1/2] block: Don't inactivate children before parents

2018-11-26 Thread Max Reitz
On 26.11.18 13:33, Kevin Wolf wrote:
> Am 26.11.2018 um 13:05 hat Max Reitz geschrieben:
>> On 26.11.18 12:28, Kevin Wolf wrote:
>>> bdrv_child_cb_inactivate() asserts that parents are already inactive
>>> when children get inactivated. This precondition is necessary because
>>> parents could still issue requests in their inactivation code.
>>>
>>> When block nodes are created individually with -blockdev, all of them
>>> are monitor owned and will be returned by bdrv_next() in an undefined
>>> order (in practice, in the order of their creation, which is usually
>>> children before parents), which obviously fails the assertion.
>>>
>>> This patch fixes the ordering by skipping nodes with still active
>>> parents in bdrv_inactivate_recurse() because we know that they will be
>>> covered by recursion when the last active parent becomes inactive.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  block.c | 22 ++
>>>  1 file changed, 22 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 5ba3435f8f..0569275e31 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -4612,6 +4612,22 @@ void bdrv_invalidate_cache_all(Error **errp)
>>>  }
>>>  }
>>>  
>>> +static bool bdrv_has_active_bds_parent(BlockDriverState *bs)
>>> +{
>>> +BdrvChild *parent;
>>> +
>>> +QLIST_FOREACH(parent, &bs->parents, next_parent) {
>>> +if (parent->role->parent_is_bds) {
>>> +BlockDriverState *parent_bs = parent->opaque;
>>> +if (!(parent_bs->open_flags & BDRV_O_INACTIVE)) {
>>> +return true;
>>> +}
>>> +}
>>> +}
>>
>> Now I see why you say this might make sense as a permission.
> 
> You do? To be honest, now that I found this solution, I don't think a
> permission makes much sense any more, because you would have the same
> loop, and you would only be checking a different flag in the end.
> 
>>> +
>>> +return false;
>>> +}
>>> +
>>>  static int bdrv_inactivate_recurse(BlockDriverState *bs,
>>> bool setting_flag)
>>>  {
>>> @@ -4622,6 +4638,12 @@ static int bdrv_inactivate_recurse(BlockDriverState 
>>> *bs,
>>>  return -ENOMEDIUM;
>>>  }
>>>  
>>> +/* Make sure that we don't inactivate a child before its parent.
>>> + * It will be covered by recursion from the yet active parent. */
>>> +if (bdrv_has_active_bds_parent(bs)) {
>>> +return 0;
>>> +}
>>> +
>>
>> Hm.  Wouldn't it make more sense to always return early when there are
>> any BDS parents?  Because if there are any BDS parents and none of them
>> are active (anymore), then this child will have been inactivated
>> already, and we can save ourselves the trouble of going through the rest
>> of the function again.
> 
> I don't quite follow... If we always return early no matter whether
> there is an active parent, who will have inactivated the child?
> 
> After trying to write up why you're wrong, I think there are two cases
> and both of us have a point:
> 
> 1. bdrv_next() enumerates the child node first and then the last BDS
>parent. This is what this patch fixes.
> 
>It will inactivate the child exactly once, at the time that the last
>parent has become inactive (and recursively calls this function for
>each of its children). If you remove that one inactivation, too,
>children won't be inactivated at all.
> 
> 2. bdrv_next() enumerates the last BDS parent first and then the child.
>This is unlikely and might even be impossible today, but once we
>expose bdrv_reopen() in QMP and let the user reconfigure the edges,
>it probably becomes possible.

blockdev-snapshot exists today.

>In this case, even after my patch we inactivate drivers twice. Maybe
>we should just return early if BDRV_O_INACTIVE is already set. What
>makes me kind of unsure is that we already test for this flag, but
>only for part of the function.
> 
>Any ideas how to test this? Can we create such a situation today?

As I wrote in my second mail just now, I think bdrv_inactivate_all()
needs to check this.

See attached diff to 234, but I don't know whether we can really test
this, as there is no failure when .bdrv_inactivate() is called multiple
times.  (What I've done is add debugging code to see that it is called
multiple times).

Max

>> Do drivers support multiple calls to .bdrv_in_activate() at all?
> 
> They were probably not designed for that... Not sure if there was ever a
> commit where we used to call them multiple times without failing the
> assertion first.
> 
> Kevin
> 

diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
index 1f695d337a..b119b4cc4d 100755
--- a/tests/qemu-iotests/234
+++ b/tests/qemu-iotests/234
@@ -27,11 +27,13 @@ iotests.verify_image_format(supported_fmts=['raw', 'qcow2'])
 iotests.verify_platform(['linux'])
 
 with iotests.FilePath('img') as img_path, \
+ iotests.FilePath('backing') as backing_path, \
  iotests.FilePath('mig_fifo_a') as 

[Qemu-block] [PATCH for-3.2 02/11] vhost-user: simplify vhost_user_init/vhost_user_cleanup

2018-11-26 Thread Marc-André Lureau
Take a VhostUserState* that can be pre-allocated, and initialize it
with the associated chardev.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Tiwei Bie 
---
 include/hw/virtio/vhost-user-blk.h  |  2 +-
 include/hw/virtio/vhost-user-scsi.h |  2 +-
 include/hw/virtio/vhost-user.h  |  2 +-
 backends/cryptodev-vhost-user.c | 18 --
 hw/block/vhost-user-blk.c   | 22 --
 hw/scsi/vhost-user-scsi.c   | 20 
 hw/virtio/vhost-stub.c  |  4 ++--
 hw/virtio/vhost-user.c  | 16 
 net/vhost-user.c| 13 -
 9 files changed, 33 insertions(+), 66 deletions(-)

diff --git a/include/hw/virtio/vhost-user-blk.h 
b/include/hw/virtio/vhost-user-blk.h
index d52944aeeb..a8a106eecb 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -36,7 +36,7 @@ typedef struct VHostUserBlk {
 uint32_t queue_size;
 uint32_t config_wce;
 struct vhost_dev dev;
-VhostUserState *vhost_user;
+VhostUserState vhost_user;
 } VHostUserBlk;
 
 #endif
diff --git a/include/hw/virtio/vhost-user-scsi.h 
b/include/hw/virtio/vhost-user-scsi.h
index e429cacd8e..738f9288bd 100644
--- a/include/hw/virtio/vhost-user-scsi.h
+++ b/include/hw/virtio/vhost-user-scsi.h
@@ -30,7 +30,7 @@
 
 typedef struct VHostUserSCSI {
 VHostSCSICommon parent_obj;
-VhostUserState *vhost_user;
+VhostUserState vhost_user;
 } VHostUserSCSI;
 
 #endif /* VHOST_USER_SCSI_H */
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index fd660393a0..811e325f42 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -22,7 +22,7 @@ typedef struct VhostUserState {
 VhostUserHostNotifier notifier[VIRTIO_QUEUE_MAX];
 } VhostUserState;
 
-VhostUserState *vhost_user_init(void);
+bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
 void vhost_user_cleanup(VhostUserState *user);
 
 #endif
diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
index d539f14d59..1052a5d0e9 100644
--- a/backends/cryptodev-vhost-user.c
+++ b/backends/cryptodev-vhost-user.c
@@ -47,7 +47,7 @@
 typedef struct CryptoDevBackendVhostUser {
 CryptoDevBackend parent_obj;
 
-VhostUserState *vhost_user;
+VhostUserState vhost_user;
 CharBackend chr;
 char *chr_name;
 bool opened;
@@ -104,7 +104,7 @@ cryptodev_vhost_user_start(int queues,
 continue;
 }
 
-options.opaque = s->vhost_user;
+options.opaque = &s->vhost_user;
 options.backend_type = VHOST_BACKEND_TYPE_USER;
 options.cc = b->conf.peers.ccs[i];
 s->vhost_crypto[i] = cryptodev_vhost_init(&options);
@@ -182,7 +182,6 @@ static void cryptodev_vhost_user_init(
 size_t i;
 Error *local_err = NULL;
 Chardev *chr;
-VhostUserState *user;
 CryptoDevBackendClient *cc;
 CryptoDevBackendVhostUser *s =
   CRYPTODEV_BACKEND_VHOST_USER(backend);
@@ -213,15 +212,10 @@ static void cryptodev_vhost_user_init(
 }
 }
 
-user = vhost_user_init();
-if (!user) {
-error_setg(errp, "Failed to init vhost_user");
+if (!vhost_user_init(&s->vhost_user, &s->chr, errp)) {
 return;
 }
 
-user->chr = &s->chr;
-s->vhost_user = user;
-
 qemu_chr_fe_set_handlers(&s->chr, NULL, NULL,
  cryptodev_vhost_user_event, NULL, s, NULL, true);
 
@@ -307,11 +301,7 @@ static void cryptodev_vhost_user_cleanup(
 }
 }
 
-if (s->vhost_user) {
-vhost_user_cleanup(s->vhost_user);
-g_free(s->vhost_user);
-s->vhost_user = NULL;
-}
+vhost_user_cleanup(&s->vhost_user);
 }
 
 static void cryptodev_vhost_user_set_chardev(Object *obj,
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 1451940845..e728fbe7f9 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -249,7 +249,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, 
Error **errp)
 {
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 VHostUserBlk *s = VHOST_USER_BLK(vdev);
-VhostUserState *user;
 int i, ret;
 
 if (!s->chardev.chr) {
@@ -267,15 +266,10 @@ static void vhost_user_blk_device_realize(DeviceState 
*dev, Error **errp)
 return;
 }
 
-user = vhost_user_init();
-if (!user) {
-error_setg(errp, "vhost-user-blk: failed to init vhost_user");
+if (!vhost_user_init(&s->vhost_user, &s->chardev, errp)) {
 return;
 }
 
-user->chr = &s->chardev;
-s->vhost_user = user;
-
 virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
 sizeof(struct virtio_blk_config));
 
@@ -291,7 +285,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, 
Error **errp)
 
 vhost_dev_set_config_notifier(&s->dev, &blk_ops);
 
-ret = vhost_dev_init(&s->dev, s->vhost_user, VHOST_BACKEND_TYPE_USER, 0)

Re: [Qemu-block] [PATCH for-3.1 1/2] block: Don't inactivate children before parents

2018-11-26 Thread Max Reitz
On 26.11.18 13:05, Max Reitz wrote:
> On 26.11.18 12:28, Kevin Wolf wrote:
>> bdrv_child_cb_inactivate() asserts that parents are already inactive
>> when children get inactivated. This precondition is necessary because
>> parents could still issue requests in their inactivation code.
>>
>> When block nodes are created individually with -blockdev, all of them
>> are monitor owned and will be returned by bdrv_next() in an undefined
>> order (in practice, in the order of their creation, which is usually
>> children before parents), which obviously fails the assertion.
>>
>> This patch fixes the ordering by skipping nodes with still active
>> parents in bdrv_inactivate_recurse() because we know that they will be
>> covered by recursion when the last active parent becomes inactive.
>>
>> Signed-off-by: Kevin Wolf 
>> ---
>>  block.c | 22 ++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 5ba3435f8f..0569275e31 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4612,6 +4612,22 @@ void bdrv_invalidate_cache_all(Error **errp)
>>  }
>>  }
>>  
>> +static bool bdrv_has_active_bds_parent(BlockDriverState *bs)
>> +{
>> +BdrvChild *parent;
>> +
>> +QLIST_FOREACH(parent, &bs->parents, next_parent) {
>> +if (parent->role->parent_is_bds) {
>> +BlockDriverState *parent_bs = parent->opaque;
>> +if (!(parent_bs->open_flags & BDRV_O_INACTIVE)) {
>> +return true;
>> +}
>> +}
>> +}
> 
> Now I see why you say this might make sense as a permission.
> 
>> +
>> +return false;
>> +}
>> +
>>  static int bdrv_inactivate_recurse(BlockDriverState *bs,
>> bool setting_flag)
>>  {
>> @@ -4622,6 +4638,12 @@ static int bdrv_inactivate_recurse(BlockDriverState 
>> *bs,
>>  return -ENOMEDIUM;
>>  }
>>  
>> +/* Make sure that we don't inactivate a child before its parent.
>> + * It will be covered by recursion from the yet active parent. */
>> +if (bdrv_has_active_bds_parent(bs)) {

Another thing I found while testing: I think this should only return
early if setting_flag is true.  BDRV_O_INACTIVE will only be set on the
second pass.  If you skip nodes with active parents unconditionally,
bs->drv->bdrv_inactivate() will not be called for any non-root BDS
(because bdrv_has_active_bds_parents() returns true for all non-root
BDSs during the first pass).

>> +return 0;
>> +}
>> +
> 
> Hm.  Wouldn't it make more sense to always return early when there are
> any BDS parents?  Because if there are any BDS parents and none of them
> are active (anymore), then this child will have been inactivated
> already, and we can save ourselves the trouble of going through the rest
> of the function again.

Hm, well, no, at least not directly here.  (Otherwise
bdrv_inactive_recurse() will not really recurse when it calls itself...)
But bdrv_inactive_all() could check that before invoking this function.

Ah, but then it is possible to still run into the exact bug you're
fixing here, because a node might inactivate a child that has another
parent still (which is inactivated later).

But still, I think bdrv_inactive_all() should skip non-root BDSs,
because calling bs->drv->bdrv_inactive() and parent->role->inactivate()
multiple times cannot be right.

Max

> Do drivers support multiple calls to .bdrv_in_activate() at all?
> 
> Max
> 
>>  if (!setting_flag && bs->drv->bdrv_inactivate) {
>>  ret = bs->drv->bdrv_inactivate(bs);
>>  if (ret < 0) {
>>
> 
> 




signature.asc
Description: OpenPGP digital signature


Re: [Qemu-block] [PATCH for-3.1 1/2] block: Don't inactivate children before parents

2018-11-26 Thread Kevin Wolf
Am 26.11.2018 um 13:05 hat Max Reitz geschrieben:
> On 26.11.18 12:28, Kevin Wolf wrote:
> > bdrv_child_cb_inactivate() asserts that parents are already inactive
> > when children get inactivated. This precondition is necessary because
> > parents could still issue requests in their inactivation code.
> > 
> > When block nodes are created individually with -blockdev, all of them
> > are monitor owned and will be returned by bdrv_next() in an undefined
> > order (in practice, in the order of their creation, which is usually
> > children before parents), which obviously fails the assertion.
> > 
> > This patch fixes the ordering by skipping nodes with still active
> > parents in bdrv_inactivate_recurse() because we know that they will be
> > covered by recursion when the last active parent becomes inactive.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block.c | 22 ++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 5ba3435f8f..0569275e31 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4612,6 +4612,22 @@ void bdrv_invalidate_cache_all(Error **errp)
> >  }
> >  }
> >  
> > +static bool bdrv_has_active_bds_parent(BlockDriverState *bs)
> > +{
> > +BdrvChild *parent;
> > +
> > +QLIST_FOREACH(parent, &bs->parents, next_parent) {
> > +if (parent->role->parent_is_bds) {
> > +BlockDriverState *parent_bs = parent->opaque;
> > +if (!(parent_bs->open_flags & BDRV_O_INACTIVE)) {
> > +return true;
> > +}
> > +}
> > +}
> 
> Now I see why you say this might make sense as a permission.

You do? To be honest, now that I found this solution, I don't think a
permission makes much sense any more, because you would have the same
loop, and you would only be checking a different flag in the end.

> > +
> > +return false;
> > +}
> > +
> >  static int bdrv_inactivate_recurse(BlockDriverState *bs,
> > bool setting_flag)
> >  {
> > @@ -4622,6 +4638,12 @@ static int bdrv_inactivate_recurse(BlockDriverState 
> > *bs,
> >  return -ENOMEDIUM;
> >  }
> >  
> > +/* Make sure that we don't inactivate a child before its parent.
> > + * It will be covered by recursion from the yet active parent. */
> > +if (bdrv_has_active_bds_parent(bs)) {
> > +return 0;
> > +}
> > +
> 
> Hm.  Wouldn't it make more sense to always return early when there are
> any BDS parents?  Because if there are any BDS parents and none of them
> are active (anymore), then this child will have been inactivated
> already, and we can save ourselves the trouble of going through the rest
> of the function again.

I don't quite follow... If we always return early no matter whether
there is an active parent, who will have inactivated the child?

After trying to write up why you're wrong, I think there are two cases
and both of us have a point:

1. bdrv_next() enumerates the child node first and then the last BDS
   parent. This is what this patch fixes.

   It will inactivate the child exactly once, at the time that the last
   parent has become inactive (and recursively calls this function for
   each of its children). If you remove that one inactivation, too,
   children won't be inactivated at all.

2. bdrv_next() enumerates the last BDS parent first and then the child.
   This is unlikely and might even be impossible today, but once we
   expose bdrv_reopen() in QMP and let the user reconfigure the edges,
   it probably becomes possible.

   In this case, even after my patch we inactivate drivers twice. Maybe
   we should just return early if BDRV_O_INACTIVE is already set. What
   makes me kind of unsure is that we already test for this flag, but
   only for part of the function.

   Any ideas how to test this? Can we create such a situation today?

> Do drivers support multiple calls to .bdrv_in_activate() at all?

They were probably not designed for that... Not sure if there was ever a
commit where we used to call them multiple times without failing the
assertion first.

Kevin


signature.asc
Description: PGP signature


Re: [Qemu-block] [PATCH v5 7/9] scsi: account unmap operations

2018-11-26 Thread Anton Nefedov


On 23/11/2018 9:25 PM, Vladimir Sementsov-Ogievskiy wrote:
> 31.10.2018 14:34, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov 
> 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> 
> but be careful: on git am, the chunk about read-only case goes into 
> scsi_disk_emulate_write_same instead of scsi_disk_emulate_unmap (at least for 
> me, with latest master branch and git 2.18
> 

It does. That's weird :/
Thanks!


Re: [Qemu-block] [PATCH v5 3/9] block: add empty account cookie type

2018-11-26 Thread Anton Nefedov


On 23/11/2018 7:04 PM, Vladimir Sementsov-Ogievskiy wrote:
> 31.10.2018 14:34, Anton Nefedov wrote:
>> This adds some protection from accounting unitialized cookie.
> 
> uninitialized
> 

fixed

>> That is, block_acct_failed/done without previous block_acct_start;
>> in that case, cookie probably holds values from previous operation.
>>
>> (Note: it might also be unitialized holding garbage value and there is
> 
> and here.
> 

fixed

>>still "< BLOCK_MAX_IOTYPE" assertion for that.
>>So block_acct_failed/done without previous block_acct_start should be used
>>with caution.)
>>
>> Currently this is particularly useful in ide code where it's hard to
>> keep track whether the request started accounting or not. For example,
>> trim requests do the accounting separately.
> 
> so, is it an existing bug? Could you please give an example in the code of 
> such wrong (before the patch) accounting?
> 

e.g. look at ide_dma_cb()->ide_handle_rw_error() call. In IDE_DMA_TRIM
case, the cookie is never initialized, but it is block_acct_failed()-ed
anyway.

>>
>> Signed-off-by: Anton Nefedov 
>> ---
>>include/block/accounting.h | 1 +
>>block/accounting.c | 6 ++
>>2 files changed, 7 insertions(+)
>>
>> diff --git a/include/block/accounting.h b/include/block/accounting.h
>> index ba8b04d572..878b4c3581 100644
>> --- a/include/block/accounting.h
>> +++ b/include/block/accounting.h
>> @@ -33,6 +33,7 @@ typedef struct BlockAcctTimedStats BlockAcctTimedStats;
>>typedef struct BlockAcctStats BlockAcctStats;
>>
>>enum BlockAcctType {
>> +BLOCK_ACCT_NONE = 0,
>>BLOCK_ACCT_READ,
>>BLOCK_ACCT_WRITE,
>>BLOCK_ACCT_FLUSH,
>> diff --git a/block/accounting.c b/block/accounting.c
>> index 70a3d9a426..8d41c8a83a 100644
>> --- a/block/accounting.c
>> +++ b/block/accounting.c
>> @@ -195,6 +195,10 @@ static void block_account_one_io(BlockAcctStats *stats, 
>> BlockAcctCookie *cookie,
>>
>>assert(cookie->type < BLOCK_MAX_IOTYPE);
>>
>> +if (cookie->type == BLOCK_ACCT_NONE) {
>> +return;
>> +}
>> +
>>qemu_mutex_lock(&stats->lock);
>>
>>if (failed) {
>> @@ -217,6 +221,8 @@ static void block_account_one_io(BlockAcctStats *stats, 
>> BlockAcctCookie *cookie,
>>}
>>
>>qemu_mutex_unlock(&stats->lock);
>> +
>> +cookie->type = BLOCK_ACCT_NONE;
>>}
>>
>>void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
>>
> 
> 


Re: [Qemu-block] [PATCH for-3.1 1/2] block: Don't inactivate children before parents

2018-11-26 Thread Max Reitz
On 26.11.18 12:28, Kevin Wolf wrote:
> bdrv_child_cb_inactivate() asserts that parents are already inactive
> when children get inactivated. This precondition is necessary because
> parents could still issue requests in their inactivation code.
> 
> When block nodes are created individually with -blockdev, all of them
> are monitor owned and will be returned by bdrv_next() in an undefined
> order (in practice, in the order of their creation, which is usually
> children before parents), which obviously fails the assertion.
> 
> This patch fixes the ordering by skipping nodes with still active
> parents in bdrv_inactivate_recurse() because we know that they will be
> covered by recursion when the last active parent becomes inactive.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 5ba3435f8f..0569275e31 100644
> --- a/block.c
> +++ b/block.c
> @@ -4612,6 +4612,22 @@ void bdrv_invalidate_cache_all(Error **errp)
>  }
>  }
>  
> +static bool bdrv_has_active_bds_parent(BlockDriverState *bs)
> +{
> +BdrvChild *parent;
> +
> +QLIST_FOREACH(parent, &bs->parents, next_parent) {
> +if (parent->role->parent_is_bds) {
> +BlockDriverState *parent_bs = parent->opaque;
> +if (!(parent_bs->open_flags & BDRV_O_INACTIVE)) {
> +return true;
> +}
> +}
> +}

Now I see why you say this might make sense as a permission.

> +
> +return false;
> +}
> +
>  static int bdrv_inactivate_recurse(BlockDriverState *bs,
> bool setting_flag)
>  {
> @@ -4622,6 +4638,12 @@ static int bdrv_inactivate_recurse(BlockDriverState 
> *bs,
>  return -ENOMEDIUM;
>  }
>  
> +/* Make sure that we don't inactivate a child before its parent.
> + * It will be covered by recursion from the yet active parent. */
> +if (bdrv_has_active_bds_parent(bs)) {
> +return 0;
> +}
> +

Hm.  Wouldn't it make more sense to always return early when there are
any BDS parents?  Because if there are any BDS parents and none of them
are active (anymore), then this child will have been inactivated
already, and we can save ourselves the trouble of going through the rest
of the function again.

Do drivers support multiple calls to .bdrv_in_activate() at all?

Max

>  if (!setting_flag && bs->drv->bdrv_inactivate) {
>  ret = bs->drv->bdrv_inactivate(bs);
>  if (ret < 0) {
> 




signature.asc
Description: OpenPGP digital signature


[Qemu-block] [PATCH for-3.1 1/2] block: Don't inactivate children before parents

2018-11-26 Thread Kevin Wolf
bdrv_child_cb_inactivate() asserts that parents are already inactive
when children get inactivated. This precondition is necessary because
parents could still issue requests in their inactivation code.

When block nodes are created individually with -blockdev, all of them
are monitor owned and will be returned by bdrv_next() in an undefined
order (in practice, in the order of their creation, which is usually
children before parents), which obviously fails the assertion.

This patch fixes the ordering by skipping nodes with still active
parents in bdrv_inactivate_recurse() because we know that they will be
covered by recursion when the last active parent becomes inactive.

Signed-off-by: Kevin Wolf 
---
 block.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/block.c b/block.c
index 5ba3435f8f..0569275e31 100644
--- a/block.c
+++ b/block.c
@@ -4612,6 +4612,22 @@ void bdrv_invalidate_cache_all(Error **errp)
 }
 }
 
+static bool bdrv_has_active_bds_parent(BlockDriverState *bs)
+{
+BdrvChild *parent;
+
+QLIST_FOREACH(parent, &bs->parents, next_parent) {
+if (parent->role->parent_is_bds) {
+BlockDriverState *parent_bs = parent->opaque;
+if (!(parent_bs->open_flags & BDRV_O_INACTIVE)) {
+return true;
+}
+}
+}
+
+return false;
+}
+
 static int bdrv_inactivate_recurse(BlockDriverState *bs,
bool setting_flag)
 {
@@ -4622,6 +4638,12 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
 return -ENOMEDIUM;
 }
 
+/* Make sure that we don't inactivate a child before its parent.
+ * It will be covered by recursion from the yet active parent. */
+if (bdrv_has_active_bds_parent(bs)) {
+return 0;
+}
+
 if (!setting_flag && bs->drv->bdrv_inactivate) {
 ret = bs->drv->bdrv_inactivate(bs);
 if (ret < 0) {
-- 
2.19.1




[Qemu-block] [PATCH for-3.1 0/2] block: Fix crash on migration with explicit child nodes

2018-11-26 Thread Kevin Wolf
Kevin Wolf (2):
  block: Don't inactivate children before parents
  iotests: Test migration with -blockdev

 block.c|  22 
 tests/qemu-iotests/234 | 105 +
 tests/qemu-iotests/234.out |  27 ++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 155 insertions(+)
 create mode 100755 tests/qemu-iotests/234
 create mode 100644 tests/qemu-iotests/234.out

-- 
2.19.1




[Qemu-block] [PATCH for-3.1 2/2] iotests: Test migration with -blockdev

2018-11-26 Thread Kevin Wolf
Check that block node activation and inactivation works with a block
graph that is built with individually created nodes.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/234 | 105 +
 tests/qemu-iotests/234.out |  27 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 133 insertions(+)
 create mode 100755 tests/qemu-iotests/234
 create mode 100644 tests/qemu-iotests/234.out

diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
new file mode 100755
index 00..1f695d337a
--- /dev/null
+++ b/tests/qemu-iotests/234
@@ -0,0 +1,105 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2018 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 .
+#
+# Creator/Owner: Kevin Wolf 
+#
+# Check that block node activation and inactivation works with a block graph
+# that is built with individually created nodes
+
+import iotests
+import os
+
+iotests.verify_image_format(supported_fmts=['raw', 'qcow2'])
+iotests.verify_platform(['linux'])
+
+with iotests.FilePath('img') as img_path, \
+ iotests.FilePath('mig_fifo_a') as fifo_a, \
+ iotests.FilePath('mig_fifo_b') as fifo_b, \
+ iotests.VM(path_suffix='a') as vm_a, \
+ iotests.VM(path_suffix='b') as vm_b:
+
+iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+
+os.mkfifo(fifo_a)
+os.mkfifo(fifo_b)
+
+iotests.log('Launching source VM...')
+(vm_a.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
+ .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
(iotests.imgfmt))
+ .launch())
+
+iotests.log('Launching destination VM...')
+(vm_b.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
+ .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
(iotests.imgfmt))
+ .add_incoming("exec: cat '%s'" % (fifo_a))
+ .launch())
+
+iotests.log('Enabling migration QMP events on A...')
+iotests.log(vm_a.qmp('migrate-set-capabilities', capabilities=[
+{
+'capability': 'events',
+'state': True
+}
+]))
+
+iotests.log('Starting migration to B...')
+iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo_a)))
+with iotests.Timeout(3, 'Migration does not complete'):
+while True:
+event = vm_a.event_wait('MIGRATION')
+iotests.log(event, filters=[iotests.filter_qmp_event])
+if event['data']['status'] == 'completed':
+break
+
+iotests.log(vm_a.qmp('query-migrate')['return']['status'])
+iotests.log(vm_b.qmp('query-migrate')['return']['status'])
+
+iotests.log(vm_a.qmp('query-status'))
+iotests.log(vm_b.qmp('query-status'))
+
+iotests.log('Add a second parent to drive0-file...')
+iotests.log(vm_b.qmp('blockdev-add', driver='raw', file='drive0-file',
+ node_name='drive0-raw'))
+
+iotests.log('Restart A with -incoming and second parent...')
+vm_a.shutdown()
+(vm_a.add_blockdev('raw,file=drive0-file,node-name=drive0-raw')
+ .add_incoming("exec: cat '%s'" % (fifo_b))
+ .launch())
+
+iotests.log('Enabling migration QMP events on B...')
+iotests.log(vm_b.qmp('migrate-set-capabilities', capabilities=[
+{
+'capability': 'events',
+'state': True
+}
+]))
+
+iotests.log('Starting migration back to A...')
+iotests.log(vm_b.qmp('migrate', uri='exec:cat >%s' % (fifo_b)))
+with iotests.Timeout(3, 'Migration does not complete'):
+while True:
+event = vm_b.event_wait('MIGRATION')
+iotests.log(event, filters=[iotests.filter_qmp_event])
+if event['data']['status'] == 'completed':
+break
+
+iotests.log(vm_a.qmp('query-migrate')['return']['status'])
+iotests.log(vm_b.qmp('query-migrate')['return']['status'])
+
+iotests.log(vm_a.qmp('query-status'))
+iotests.log(vm_b.qmp('query-status'))
diff --git a/tests/qemu-iotests/234.out b/tests/qemu-iotests/234.out
new file mode 100644
index 00..115537cd5c
--- /dev/null
+++ b/tests/qemu-iotests/234.out
@@ -0,0 +1,27 @@
+Launching source VM...
+Launching destination VM...
+Enabling migration QMP events on A...
+{"return": {}}
+Starting migration to B...
+{"return": {}}
+{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": 
{"microse