[Qemu-devel] [PATCH 0/2] block: fix nbd-server-stop crash after blockdev-snapshot-sync

[Qemu-devel] [PATCH 0/2] block: fix nbd-server-stop crash after blockdev-snapshot-sync

[Stefan Hajnoczi]

The blockdev-snapshot-sync command uses bdrv_append() to update all parents to
point at the external snapshot node.  This breaks BlockBackend's
blk_add/remove_aio_context_notifier(), which doesn't expect a BDS change.

Patch 1 fixes this by tracking AioContext notifiers in BlockBackend.

See the test case in Patch 2 for a reproducer.

Stefan Hajnoczi (2):
  block: let blk_add/remove_aio_context_notifier() tolerate BDS changes
  iotests: add 208 nbd-server + blockdev-snapshot-sync test case

 block/block-backend.c      | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 block/trace-events         |  2 ++
 tests/qemu-iotests/208     | 55 ++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/208.out |  9 +++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 130 insertions(+)
 create mode 100755 tests/qemu-iotests/208
 create mode 100644 tests/qemu-iotests/208.out

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes

[Stefan Hajnoczi]

Commit 2019ba0a0197 ("block: Add AioContextNotifier functions to BB")
added blk_add/remove_aio_context_notifier() and implemented them by
passing through the bdrv_*() equivalent.

This doesn't work across bdrv_append(), which detaches child->bs and
re-attaches it to a new BlockDriverState.  When
blk_remove_aio_context_notifier() is called we will access the new BDS
instead of the one where the notifier was added!

>From the point of view of the blk_*() API user, changes to the root BDS
should be transparent.

This patch maintains a list of AioContext notifiers in BlockBackend and
adds/removes them from the BlockDriverState as needed.

Reported-by: Stefano Panella <spanella@gmail.com>
Cc: Max Reitz <mreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/block-backend.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block/trace-events    |  2 ++
 2 files changed, 65 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 94ffbb6a60..aa27698820 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -31,6 +31,13 @@
 
 static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
 
+typedef struct BlockBackendAioNotifier {
+    void (*attached_aio_context)(AioContext *new_context, void *opaque);
+    void (*detach_aio_context)(void *opaque);
+    void *opaque;
+    QLIST_ENTRY(BlockBackendAioNotifier) list;
+} BlockBackendAioNotifier;
+
 struct BlockBackend {
     char *name;
     int refcnt;
@@ -69,6 +76,7 @@ struct BlockBackend {
     bool allow_write_beyond_eof;
 
     NotifierList remove_bs_notifiers, insert_bs_notifiers;
+    QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;
 
     int quiesce_counter;
     VMChangeStateEntry *vmsh;
@@ -239,6 +247,36 @@ static int blk_root_inactivate(BdrvChild *child)
     return 0;
 }
 
+static void blk_root_attach(BdrvChild *child)
+{
+    BlockBackend *blk = child->opaque;
+    BlockBackendAioNotifier *notifier;
+
+    trace_blk_root_attach(child, blk, child->bs);
+
+    QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
+        bdrv_add_aio_context_notifier(child->bs,
+                notifier->attached_aio_context,
+                notifier->detach_aio_context,
+                notifier->opaque);
+    }
+}
+
+static void blk_root_detach(BdrvChild *child)
+{
+    BlockBackend *blk = child->opaque;
+    BlockBackendAioNotifier *notifier;
+
+    trace_blk_root_detach(child, blk, child->bs);
+
+    QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
+        bdrv_remove_aio_context_notifier(child->bs,
+                notifier->attached_aio_context,
+                notifier->detach_aio_context,
+                notifier->opaque);
+    }
+}
+
 static const BdrvChildRole child_root = {
     .inherit_options    = blk_root_inherit_options,
 
@@ -252,6 +290,9 @@ static const BdrvChildRole child_root = {
 
     .activate           = blk_root_activate,
     .inactivate         = blk_root_inactivate,
+
+    .attach             = blk_root_attach,
+    .detach             = blk_root_detach,
 };
 
 /*
@@ -279,6 +320,7 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm)
 
     notifier_list_init(&blk->remove_bs_notifiers);
     notifier_list_init(&blk->insert_bs_notifiers);
+    QLIST_INIT(&blk->aio_notifiers);
 
     QTAILQ_INSERT_TAIL(&block_backends, blk, link);
     return blk;
@@ -356,6 +398,7 @@ static void blk_delete(BlockBackend *blk)
     }
     assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
     assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
+    assert(QLIST_EMPTY(&blk->aio_notifiers));
     QTAILQ_REMOVE(&block_backends, blk, link);
     drive_info_del(blk->legacy_dinfo);
     block_acct_cleanup(&blk->stats);
@@ -1813,8 +1856,15 @@ void blk_add_aio_context_notifier(BlockBackend *blk,
         void (*attached_aio_context)(AioContext *new_context, void *opaque),
         void (*detach_aio_context)(void *opaque), void *opaque)
 {
+    BlockBackendAioNotifier *notifier;
     BlockDriverState *bs = blk_bs(blk);
 
+    notifier = g_new(BlockBackendAioNotifier, 1);
+    notifier->attached_aio_context = attached_aio_context;
+    notifier->detach_aio_context = detach_aio_context;
+    notifier->opaque = opaque;
+    QLIST_INSERT_HEAD(&blk->aio_notifiers, notifier, list);
+
     if (bs) {
         bdrv_add_aio_context_notifier(bs, attached_aio_context,
                                       detach_aio_context, opaque);
@@ -1827,12 +1877,25 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
                                      void (*detach_aio_context)(void *),
                                      void *opaque)
 {
+    BlockBackendAioNotifier *notifier;
     BlockDriverState *bs = blk_bs(blk);
 
     if (bs) {
         bdrv_remove_aio_context_notifier(bs, attached_aio_context,
                                          detach_aio_context, opaque);
     }
+
+    QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
+        if (notifier->attached_aio_context == attached_aio_context &&
+            notifier->detach_aio_context == detach_aio_context &&
+            notifier->opaque == opaque) {
+            QLIST_REMOVE(notifier, list);
+            g_free(notifier);
+            return;
+        }
+    }
+
+    abort();
 }
 
 void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify)
diff --git a/block/trace-events b/block/trace-events
index 02dd80ff0c..7493d521dc 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -7,6 +7,8 @@ bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
 # block/block-backend.c
 blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x"
 blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x"
+blk_root_attach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
+blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
 
 # block/io.c
 bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Stefan Hajnoczi]

This test case adds an NBD server export and then invokes
blockdev-snapshot-sync, which changes the BlockDriverState node that the
NBD server's BlockBackend points to.  This is an interesting scenario to
test and exercises the code path fixed by the previous commit.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/208     | 55 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/208.out |  9 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 65 insertions(+)
 create mode 100755 tests/qemu-iotests/208
 create mode 100644 tests/qemu-iotests/208.out

diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
new file mode 100755
index 0000000000..4e82b96c82
--- /dev/null
+++ b/tests/qemu-iotests/208
@@ -0,0 +1,55 @@
+#!/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 <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Stefan Hajnoczi <stefanha@redhat.com>
+#
+# Check that the runtime NBD server does not crash when stopped after
+# blockdev-snapshot-sync.
+
+import iotests
+
+with iotests.FilePath('disk.img') as disk_img_path, \
+     iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
+     iotests.FilePath('nbd.sock') as nbd_sock_path, \
+     iotests.VM() as vm:
+
+    img_size = '10M'
+    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk_img_path, img_size)
+
+    iotests.log('Launching VM...')
+    (vm.add_drive(disk_img_path, 'node-name=drive0-node', interface='none')
+       .launch())
+
+    iotests.log('Starting NBD server...')
+    iotests.log(vm.qmp('nbd-server-start', addr={
+            "type": "unix",
+            "data": {
+                "path": nbd_sock_path,
+            }
+        }))
+
+    iotests.log('Adding NBD export...')
+    iotests.log(vm.qmp('nbd-server-add', device='drive0-node', writable=True))
+
+    iotests.log('Creating external snapshot...')
+    iotests.log(vm.qmp('blockdev-snapshot-sync',
+        node_name='drive0-node',
+        snapshot_node_name='drive0-snapshot-node',
+        snapshot_file=disk_snapshot_img_path))
+
+    iotests.log('Stopping NBD server...')
+    iotests.log(vm.qmp('nbd-server-stop'))
diff --git a/tests/qemu-iotests/208.out b/tests/qemu-iotests/208.out
new file mode 100644
index 0000000000..3687e9d0dd
--- /dev/null
+++ b/tests/qemu-iotests/208.out
@@ -0,0 +1,9 @@
+Launching VM...
+Starting NBD server...
+{u'return': {}}
+Adding NBD export...
+{u'return': {}}
+Creating external snapshot...
+{u'return': {}}
+Stopping NBD server...
+{u'return': {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index a2dfe79d86..01c03019dd 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -202,3 +202,4 @@
 203 rw auto
 204 rw auto quick
 205 rw auto quick
+208 rw auto quick
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Stefano Panella]

I have applied this patch and when I run the following qmp commands I I do
not see the crash anymore but there is still something wrong because only
/root/a is opened from qemu. It looks like nbd-server-stop is also getting
rid of the nodes added with blockdev-snapshot-sync, therfore is than not
possible to do blockdev-del on /root/d because node-name is not found

{ "execute": "qmp_capabilities" }
{
"execute": "blockdev-add",
"arguments": {
"driver": "qcow2",
"node-name": "/root/a",
"discard": "unmap",
"cache": {
"direct": true
},
"file": {
"driver": "file",
"filename": "/root/a"
}
}
}

{
"execute": "nbd-server-start",
"arguments": {
"addr": {
"type": "unix",
"data": {
"path": "/tmp/nbd.test1"
}
}
}
}

{
"execute": "nbd-server-add",
"arguments": {
"device": "/root/a",
"writable": true
}
}


{
"execute": "blockdev-snapshot-sync",
"arguments": {
"node-name": "/root/a",
"snapshot-node-name": "/root/b",
"snapshot-file": "/root/b"
}
}
{
"execute": "blockdev-snapshot-sync",
"arguments": {
"node-name": "/root/b",
"snapshot-node-name": "/root/c",
"snapshot-file": "/root/c"
}
}
{
"execute": "blockdev-snapshot-sync",
"arguments": {
"node-name": "/root/c",
"snapshot-node-name": "/root/d",
"snapshot-file": "/root/d"
}
}

{
"execute": "nbd-server-stop"
}


On Tue, Mar 6, 2018 at 8:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> This test case adds an NBD server export and then invokes
> blockdev-snapshot-sync, which changes the BlockDriverState node that the
> NBD server's BlockBackend points to.  This is an interesting scenario to
> test and exercises the code path fixed by the previous commit.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/208     | 55
++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/208.out |  9 ++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 65 insertions(+)
>  create mode 100755 tests/qemu-iotests/208
>  create mode 100644 tests/qemu-iotests/208.out
>
> diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
> new file mode 100755
> index 0000000000..4e82b96c82
> --- /dev/null
> +++ b/tests/qemu-iotests/208
> @@ -0,0 +1,55 @@
> +#!/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 <http://www.gnu.org/licenses/>.
> +#
> +# Creator/Owner: Stefan Hajnoczi <stefanha@redhat.com>
> +#
> +# Check that the runtime NBD server does not crash when stopped after
> +# blockdev-snapshot-sync.
> +
> +import iotests
> +
> +with iotests.FilePath('disk.img') as disk_img_path, \
> +     iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
> +     iotests.FilePath('nbd.sock') as nbd_sock_path, \
> +     iotests.VM() as vm:
> +
> +    img_size = '10M'
> +    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk_img_path,
img_size)
> +
> +    iotests.log('Launching VM...')
> +    (vm.add_drive(disk_img_path, 'node-name=drive0-node',
interface='none')
> +       .launch())
> +
> +    iotests.log('Starting NBD server...')
> +    iotests.log(vm.qmp('nbd-server-start', addr={
> +            "type": "unix",
> +            "data": {
> +                "path": nbd_sock_path,
> +            }
> +        }))
> +
> +    iotests.log('Adding NBD export...')
> +    iotests.log(vm.qmp('nbd-server-add', device='drive0-node',
writable=True))
> +
> +    iotests.log('Creating external snapshot...')
> +    iotests.log(vm.qmp('blockdev-snapshot-sync',
> +        node_name='drive0-node',
> +        snapshot_node_name='drive0-snapshot-node',
> +        snapshot_file=disk_snapshot_img_path))
> +
> +    iotests.log('Stopping NBD server...')
> +    iotests.log(vm.qmp('nbd-server-stop'))
> diff --git a/tests/qemu-iotests/208.out b/tests/qemu-iotests/208.out
> new file mode 100644
> index 0000000000..3687e9d0dd
> --- /dev/null
> +++ b/tests/qemu-iotests/208.out
> @@ -0,0 +1,9 @@
> +Launching VM...
> +Starting NBD server...
> +{u'return': {}}
> +Adding NBD export...
> +{u'return': {}}
> +Creating external snapshot...
> +{u'return': {}}
> +Stopping NBD server...
> +{u'return': {}}
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index a2dfe79d86..01c03019dd 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -202,3 +202,4 @@
>  203 rw auto
>  204 rw auto quick
>  205 rw auto quick
> +208 rw auto quick
> --
> 2.14.3
>

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Stefan Hajnoczi]

On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella <spanella@gmail.com> wrote:
> I have applied this patch and when I run the following qmp commands I I do
> not see the crash anymore but there is still something wrong because only
> /root/a is opened from qemu. It looks like nbd-server-stop is also getting
> rid of the nodes added with blockdev-snapshot-sync, therfore is than not
> possible to do blockdev-del on /root/d because node-name is not found

Nodes are reference counted.  If nothing holds a refcount then the
node is freed.

The blockdev-add command holds a reference to the node.  The node will
stay alive until blockdev-del, which releases that reference.

blockdev-snapshot-sync does not hold a reference.  Therefore snapshot
nodes are freed once nothing is using them anymore.  When the snapshot
node is created, the users of the parent node are updated to point to
the snapshot node instead.  This is why the NBD server switches to the
snapshot mode after blockdev-snapshot-sync.

This is why the snapshot nodes disappear after the NBD server is
stopped while /root/a stays alive.

I'm not sure if the current blockdev-snapshot-sync behavior is useful.
Perhaps the presence of the "snapshot-node-name" argument should cause
the snapshot node to be treated as monitor-owned, just like
blockdev-add.  This would introduce leaks for existing QMP clients
though, so it may be necessary to add yet another argument for this
behavior.

Anyway, I hope this explains the current behavior.  I don't see a
problem with it, but it's something the API users need to be aware of.

If it is a problem for your use case, please explain what you are trying to do.

Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Stefano Panella]

On Wed, Mar 7, 2018 at 10:55 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella <spanella@gmail.com>
wrote:
> > I have applied this patch and when I run the following qmp commands I I
do
> > not see the crash anymore but there is still something wrong because
only
> > /root/a is opened from qemu. It looks like nbd-server-stop is also
getting
> > rid of the nodes added with blockdev-snapshot-sync, therfore is than not
> > possible to do blockdev-del on /root/d because node-name is not found
>
> Nodes are reference counted.  If nothing holds a refcount then the
> node is freed.
Thanks, that explains the behaviour
>
> The blockdev-add command holds a reference to the node.  The node will
> stay alive until blockdev-del, which releases that reference.
>
> blockdev-snapshot-sync does not hold a reference.  Therefore snapshot
> nodes are freed once nothing is using them anymore.  When the snapshot
> node is created, the users of the parent node are updated to point to
> the snapshot node instead.  This is why the NBD server switches to the
> snapshot mode after blockdev-snapshot-sync.
>
> This is why the snapshot nodes disappear after the NBD server is
> stopped while /root/a stays alive.
>
> I'm not sure if the current blockdev-snapshot-sync behavior is useful.
> Perhaps the presence of the "snapshot-node-name" argument should cause
> the snapshot node to be treated as monitor-owned, just like
> blockdev-add.  This would introduce leaks for existing QMP clients
> though, so it may be necessary to add yet another argument for this
> behavior.
that would be nice, I mean to add an extra parameter so it is added to the
monitor
>
> Anyway, I hope this explains the current behavior.  I don't see a
> problem with it, but it's something the API users need to be aware of.
>
Yes, I was not aware of that behaviour, the problem is that many examples
refer
to having a device associated with the blockdev-add'd node therefore we do
not
see this problem.
> If it is a problem for your use case, please explain what you are trying
to do.
>
It is not strictly a problem for my usecase but it would be nice to have
the extra param to
blockdev-snapshot-sync. That would also fix the problem of running multiple
snap-sync
after blockdev-add but before there is any user.
> Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Stefan Hajnoczi]

On Wed, Mar 7, 2018 at 1:57 PM, Stefano Panella <spanella@gmail.com> wrote:
> On Wed, Mar 7, 2018 at 10:55 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>
>> On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella <spanella@gmail.com>
>> wrote:
>> > I have applied this patch and when I run the following qmp commands I I
>> > do
>> > not see the crash anymore but there is still something wrong because
>> > only
>> > /root/a is opened from qemu. It looks like nbd-server-stop is also
>> > getting
>> > rid of the nodes added with blockdev-snapshot-sync, therfore is than not
>> > possible to do blockdev-del on /root/d because node-name is not found
>>
>> Nodes are reference counted.  If nothing holds a refcount then the
>> node is freed.
> Thanks, that explains the behaviour
>>
>> The blockdev-add command holds a reference to the node.  The node will
>> stay alive until blockdev-del, which releases that reference.
>>
>> blockdev-snapshot-sync does not hold a reference.  Therefore snapshot
>> nodes are freed once nothing is using them anymore.  When the snapshot
>> node is created, the users of the parent node are updated to point to
>> the snapshot node instead.  This is why the NBD server switches to the
>> snapshot mode after blockdev-snapshot-sync.
>>
>> This is why the snapshot nodes disappear after the NBD server is
>> stopped while /root/a stays alive.
>>
>> I'm not sure if the current blockdev-snapshot-sync behavior is useful.
>> Perhaps the presence of the "snapshot-node-name" argument should cause
>> the snapshot node to be treated as monitor-owned, just like
>> blockdev-add.  This would introduce leaks for existing QMP clients
>> though, so it may be necessary to add yet another argument for this
>> behavior.
> that would be nice, I mean to add an extra parameter so it is added to the
> monitor
>>
>> Anyway, I hope this explains the current behavior.  I don't see a
>> problem with it, but it's something the API users need to be aware of.
>>
> Yes, I was not aware of that behaviour, the problem is that many examples
> refer
> to having a device associated with the blockdev-add'd node therefore we do
> not
> see this problem.
>> If it is a problem for your use case, please explain what you are trying
>> to do.
>>
> It is not strictly a problem for my usecase but it would be nice to have the
> extra param to
> blockdev-snapshot-sync. That would also fix the problem of running multiple
> snap-sync
> after blockdev-add but before there is any user.

Max Reitz mentioned that the 'blockdev-snapshot' command is preferred
over 'blockdev-snapshot-sync'.  'blockdev-snapshot-sync' is a legacy
command that implicitly creates the snapshot node.

The difference is that 'blockdev-snapshot' requires that the user
first creates the snapshot file (e.g. using qemu-img create), then
uses 'blockdev-add' to add the snapshot node, and finally uses
'blockdev-snapshot' to install the snapshot node.

When 'blockdev-snapshot' is used, the user must delete snapshot nodes
using 'blockdev-del' since they created using 'blockdev-add'.

Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Stefano Panella]

On Wed, Mar 7, 2018 at 4:16 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Wed, Mar 7, 2018 at 1:57 PM, Stefano Panella <spanella@gmail.com>
wrote:
> > On Wed, Mar 7, 2018 at 10:55 AM, Stefan Hajnoczi <stefanha@gmail.com>
wrote:
> >>
> >> On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella <spanella@gmail.com>
> >> wrote:
> >> > I have applied this patch and when I run the following qmp commands
I I
> >> > do
> >> > not see the crash anymore but there is still something wrong because
> >> > only
> >> > /root/a is opened from qemu. It looks like nbd-server-stop is also
> >> > getting
> >> > rid of the nodes added with blockdev-snapshot-sync, therfore is than
not
> >> > possible to do blockdev-del on /root/d because node-name is not found
> >>
> >> Nodes are reference counted.  If nothing holds a refcount then the
> >> node is freed.
> > Thanks, that explains the behaviour
> >>
> >> The blockdev-add command holds a reference to the node.  The node will
> >> stay alive until blockdev-del, which releases that reference.
> >>
> >> blockdev-snapshot-sync does not hold a reference.  Therefore snapshot
> >> nodes are freed once nothing is using them anymore.  When the snapshot
> >> node is created, the users of the parent node are updated to point to
> >> the snapshot node instead.  This is why the NBD server switches to the
> >> snapshot mode after blockdev-snapshot-sync.
> >>
> >> This is why the snapshot nodes disappear after the NBD server is
> >> stopped while /root/a stays alive.
> >>
> >> I'm not sure if the current blockdev-snapshot-sync behavior is useful.
> >> Perhaps the presence of the "snapshot-node-name" argument should cause
> >> the snapshot node to be treated as monitor-owned, just like
> >> blockdev-add.  This would introduce leaks for existing QMP clients
> >> though, so it may be necessary to add yet another argument for this
> >> behavior.
> > that would be nice, I mean to add an extra parameter so it is added to
the
> > monitor
> >>
> >> Anyway, I hope this explains the current behavior.  I don't see a
> >> problem with it, but it's something the API users need to be aware of.
> >>
> > Yes, I was not aware of that behaviour, the problem is that many
examples
> > refer
> > to having a device associated with the blockdev-add'd node therefore we
do
> > not
> > see this problem.
> >> If it is a problem for your use case, please explain what you are
trying
> >> to do.
> >>
> > It is not strictly a problem for my usecase but it would be nice to
have the
> > extra param to
> > blockdev-snapshot-sync. That would also fix the problem of running
multiple
> > snap-sync
> > after blockdev-add but before there is any user.
>
> Max Reitz mentioned that the 'blockdev-snapshot' command is preferred
> over 'blockdev-snapshot-sync'.  'blockdev-snapshot-sync' is a legacy
> command that implicitly creates the snapshot node.
>
> The difference is that 'blockdev-snapshot' requires that the user
> first creates the snapshot file (e.g. using qemu-img create), then
> uses 'blockdev-add' to add the snapshot node, and finally uses
> 'blockdev-snapshot' to install the snapshot node.
>
> When 'blockdev-snapshot' is used, the user must delete snapshot nodes
> using 'blockdev-del' since they created using 'blockdev-add'.
>
That is a very usefull info, I was not aware that blockdev-snapshot-sync
was not
recommended. I will try to run some examples with blockdev-snapshot.
In case I want to achieve
A <- B
and I do:
blockdev_add A
create external snapshot with qemu-img B with A as backing image
blockdev_add B
blockdev_snapshot B -> A

What do I need to do to delete A and B?
is it fine to just call blockdev_del B ?
or should I call blockdev_del A as well?

Thanks again for your help!!!

> Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Max Reitz]

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

On 2018-03-07 11:55, Stefan Hajnoczi wrote:
> On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella <spanella@gmail.com> wrote:
>> I have applied this patch and when I run the following qmp commands I I do
>> not see the crash anymore but there is still something wrong because only
>> /root/a is opened from qemu. It looks like nbd-server-stop is also getting
>> rid of the nodes added with blockdev-snapshot-sync, therfore is than not
>> possible to do blockdev-del on /root/d because node-name is not found
> 
> Nodes are reference counted.  If nothing holds a refcount then the
> node is freed.
> 
> The blockdev-add command holds a reference to the node.  The node will
> stay alive until blockdev-del, which releases that reference.
> 
> blockdev-snapshot-sync does not hold a reference.

I think that's a bug.  When you specify a node name for the new node, it
should get a reference.

>                                                    Therefore snapshot
> nodes are freed once nothing is using them anymore.  When the snapshot
> node is created, the users of the parent node are updated to point to
> the snapshot node instead.  This is why the NBD server switches to the
> snapshot mode after blockdev-snapshot-sync.
> 
> This is why the snapshot nodes disappear after the NBD server is
> stopped while /root/a stays alive.
> 
> I'm not sure if the current blockdev-snapshot-sync behavior is useful.
> Perhaps the presence of the "snapshot-node-name" argument should cause
> the snapshot node to be treated as monitor-owned, just like
> blockdev-add.  This would introduce leaks for existing QMP clients
> though, so it may be necessary to add yet another argument for this
> behavior.

That's true.

> Anyway, I hope this explains the current behavior.  I don't see a
> problem with it, but it's something the API users need to be aware of.

Hm, OK.

As an explanation: blockdev-snapshot-sync is from before we had node
names and blockdev-add.  You'd just create something that needs the
block layer (like a guest device or an NBD server) and then you'd open
the BDS chain you want to go with it (mostly by just specifying the
filename of the top image, and maybe its format).

Then you'd use blockdev-snapshot-sync to just create an overlay during
runtime, and since there weren't any node names it was clear that it
would go away if you deleted whatever was using the chain (like the NBD
server).

Then we introduced node names, and blockdev-snapshot-sync gained the
ability to give one to the overlay -- basically as an afterthought.  I
think we didn't really have a fleshed-out concept of monitor references
back then...  So we forgot to give the overlay an additional reference
in such a case (because we didn't know better).

As you pointed to in your other reply, blockdev-snapshot is the "pure"
blockdev command that should ideally be used.  It allows you much more
control over the overlay (because you have to do the blockdev-add
yourself), and it doesn't have this reference issue.

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Max Reitz]

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

On 2018-03-07 17:43, Stefano Panella wrote:
> 
> 
> On Wed, Mar 7, 2018 at 4:16 PM, Stefan Hajnoczi <stefanha@gmail.com
> <mailto:stefanha@gmail.com>> wrote:
>>
>> On Wed, Mar 7, 2018 at 1:57 PM, Stefano Panella <spanella@gmail.com
> <mailto:spanella@gmail.com>> wrote:
>> > On Wed, Mar 7, 2018 at 10:55 AM, Stefan Hajnoczi <stefanha@gmail.com
> <mailto:stefanha@gmail.com>> wrote:
>> >>
>> >> On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella
> <spanella@gmail.com <mailto:spanella@gmail.com>>
>> >> wrote:
>> >> > I have applied this patch and when I run the following qmp
> commands I I
>> >> > do
>> >> > not see the crash anymore but there is still something wrong because
>> >> > only
>> >> > /root/a is opened from qemu. It looks like nbd-server-stop is also
>> >> > getting
>> >> > rid of the nodes added with blockdev-snapshot-sync, therfore is
> than not
>> >> > possible to do blockdev-del on /root/d because node-name is not found
>> >>
>> >> Nodes are reference counted.  If nothing holds a refcount then the
>> >> node is freed.
>> > Thanks, that explains the behaviour
>> >>
>> >> The blockdev-add command holds a reference to the node.  The node will
>> >> stay alive until blockdev-del, which releases that reference.
>> >>
>> >> blockdev-snapshot-sync does not hold a reference.  Therefore snapshot
>> >> nodes are freed once nothing is using them anymore.  When the snapshot
>> >> node is created, the users of the parent node are updated to point to
>> >> the snapshot node instead.  This is why the NBD server switches to the
>> >> snapshot mode after blockdev-snapshot-sync.
>> >>
>> >> This is why the snapshot nodes disappear after the NBD server is
>> >> stopped while /root/a stays alive.
>> >>
>> >> I'm not sure if the current blockdev-snapshot-sync behavior is useful.
>> >> Perhaps the presence of the "snapshot-node-name" argument should cause
>> >> the snapshot node to be treated as monitor-owned, just like
>> >> blockdev-add.  This would introduce leaks for existing QMP clients
>> >> though, so it may be necessary to add yet another argument for this
>> >> behavior.
>> > that would be nice, I mean to add an extra parameter so it is added
> to the
>> > monitor
>> >>
>> >> Anyway, I hope this explains the current behavior.  I don't see a
>> >> problem with it, but it's something the API users need to be aware of.
>> >>
>> > Yes, I was not aware of that behaviour, the problem is that many
> examples
>> > refer
>> > to having a device associated with the blockdev-add'd node therefore
> we do
>> > not
>> > see this problem.
>> >> If it is a problem for your use case, please explain what you are
> trying
>> >> to do.
>> >>
>> > It is not strictly a problem for my usecase but it would be nice to
> have the
>> > extra param to
>> > blockdev-snapshot-sync. That would also fix the problem of running
> multiple
>> > snap-sync
>> > after blockdev-add but before there is any user.
>>
>> Max Reitz mentioned that the 'blockdev-snapshot' command is preferred
>> over 'blockdev-snapshot-sync'.  'blockdev-snapshot-sync' is a legacy
>> command that implicitly creates the snapshot node.
>>
>> The difference is that 'blockdev-snapshot' requires that the user
>> first creates the snapshot file (e.g. using qemu-img create), then
>> uses 'blockdev-add' to add the snapshot node, and finally uses
>> 'blockdev-snapshot' to install the snapshot node.
>>
>> When 'blockdev-snapshot' is used, the user must delete snapshot nodes
>> using 'blockdev-del' since they created using 'blockdev-add'.
>>
> That is a very usefull info, I was not aware that blockdev-snapshot-sync
> was not
> recommended.

Yeah, well...  Someone (O:-)) needs to go over all the block QMP
commands and see which are good and which should be deprecated at some
point.  I don't think we have a central list of everything yet...

> I will try to run some examples with blockdev-snapshot.
> In case I want to achieve
> A <- B
> and I do:
> blockdev_add A
> create external snapshot with qemu-img B with A as backing image
> blockdev_add B
> blockdev_snapshot B -> A
> 
> What do I need to do to delete A and B?
> is it fine to just call blockdev_del B ?
> or should I call blockdev_del A as well?

You need to call both.  The basic idea is that you have to pair every
blockdev-add with a blockdev-del.

(You have to delete B first, though, because you cannot delete a node
while it is in use (and A is in use by B as long as B exists).)

Don't forget the '"backing": null" parameter for the blockdev-add B
command, or B will already have A opened as its backing image (which is
not good, you don't want qemu to open the same image twice).

(Or maybe blockdev-add B will not even work without '"backing": null'
because qemu figures out that you are trying to open the same image (A)
twice and prevent that.)

Max


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

Re: [Qemu-devel] [PATCH 0/2] block: fix nbd-server-stop crash after blockdev-snapshot-sync

[Eric Blake]

On 03/06/2018 02:48 PM, Stefan Hajnoczi wrote:
> The blockdev-snapshot-sync command uses bdrv_append() to update all parents to
> point at the external snapshot node.  This breaks BlockBackend's
> blk_add/remove_aio_context_notifier(), which doesn't expect a BDS change.
> 
> Patch 1 fixes this by tracking AioContext notifiers in BlockBackend.
> 
> See the test case in Patch 2 for a reproducer.
> 
> Stefan Hajnoczi (2):
>    block: let blk_add/remove_aio_context_notifier() tolerate BDS changes
>    iotests: add 208 nbd-server + blockdev-snapshot-sync test case
> 
>   block/block-backend.c      | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>   block/trace-events         |  2 ++
>   tests/qemu-iotests/208     | 55 ++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/208.out |  9 +++++++
>   tests/qemu-iotests/group   |  1 +
>   5 files changed, 130 insertions(+)
>   create mode 100755 tests/qemu-iotests/208
>   create mode 100644 tests/qemu-iotests/208.out

Whose tree should this series go through?  MAINTAINERS didn't flag it as 
directly touching any files that normally affect my NBD queue, but given 
that the iotest that reproduces the problem uses NBD, I'm fine if you 
want it to go through me.

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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] block: fix nbd-server-stop crash after blockdev-snapshot-sync

[Stefan Hajnoczi]

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

On Wed, Mar 07, 2018 at 05:27:45PM -0600, Eric Blake wrote:
> On 03/06/2018 02:48 PM, Stefan Hajnoczi wrote:
> > The blockdev-snapshot-sync command uses bdrv_append() to update all parents to
> > point at the external snapshot node.  This breaks BlockBackend's
> > blk_add/remove_aio_context_notifier(), which doesn't expect a BDS change.
> > 
> > Patch 1 fixes this by tracking AioContext notifiers in BlockBackend.
> > 
> > See the test case in Patch 2 for a reproducer.
> > 
> > Stefan Hajnoczi (2):
> >    block: let blk_add/remove_aio_context_notifier() tolerate BDS changes
> >    iotests: add 208 nbd-server + blockdev-snapshot-sync test case
> > 
> >   block/block-backend.c      | 63 ++++++++++++++++++++++++++++++++++++++++++++++
> >   block/trace-events         |  2 ++
> >   tests/qemu-iotests/208     | 55 ++++++++++++++++++++++++++++++++++++++++
> >   tests/qemu-iotests/208.out |  9 +++++++
> >   tests/qemu-iotests/group   |  1 +
> >   5 files changed, 130 insertions(+)
> >   create mode 100755 tests/qemu-iotests/208
> >   create mode 100644 tests/qemu-iotests/208.out
> 
> Whose tree should this series go through?  MAINTAINERS didn't flag it as
> directly touching any files that normally affect my NBD queue, but given
> that the iotest that reproduces the problem uses NBD, I'm fine if you want
> it to go through me.

Good question.  Max and Kevin maintain block/block-backend.c so one of
them should be happy with this series before it gets merged.

When a patch affects multiple trees, the last sub-maintainer to review
it can do the merge.

So if they have already posted their R-b when you are finished, then
feel free to merge it!  And vice versa.

Stefan

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

Re: [Qemu-devel] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes

[Eric Blake]

On 03/06/2018 02:48 PM, Stefan Hajnoczi wrote:
> Commit 2019ba0a0197 ("block: Add AioContextNotifier functions to BB")
> added blk_add/remove_aio_context_notifier() and implemented them by
> passing through the bdrv_*() equivalent.
> 
> This doesn't work across bdrv_append(), which detaches child->bs and
> re-attaches it to a new BlockDriverState.  When
> blk_remove_aio_context_notifier() is called we will access the new BDS
> instead of the one where the notifier was added!
> 
>>From the point of view of the blk_*() API user, changes to the root BDS
> should be transparent.
> 
> This patch maintains a list of AioContext notifiers in BlockBackend and
> adds/removes them from the BlockDriverState as needed.
> 
> Reported-by: Stefano Panella <spanella@gmail.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/block-backend.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   block/trace-events    |  2 ++
>   2 files changed, 65 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 94ffbb6a60..aa27698820 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -31,6 +31,13 @@
>   
>   static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
>   
> +typedef struct BlockBackendAioNotifier {
> +    void (*attached_aio_context)(AioContext *new_context, void *opaque);
> +    void (*detach_aio_context)(void *opaque);

Why the difference in tense (past 'attached' vs. present 'detach')?

> @@ -1827,12 +1877,25 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
>                                        void (*detach_aio_context)(void *),
>                                        void *opaque)
>   {
> +    BlockBackendAioNotifier *notifier;
>       BlockDriverState *bs = blk_bs(blk);
>   
>       if (bs) {
>           bdrv_remove_aio_context_notifier(bs, attached_aio_context,
>                                            detach_aio_context, opaque);
>       }
> +
> +    QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
> +        if (notifier->attached_aio_context == attached_aio_context &&
> +            notifier->detach_aio_context == detach_aio_context &&
> +            notifier->opaque == opaque) {
> +            QLIST_REMOVE(notifier, list);

Don't you need to use QLIST_FOREACH_SAFE if you are going to modify the 
list during traversal?

Otherwise makes sense to me.

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

Re: [Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Eric Blake]

On 03/06/2018 02:48 PM, Stefan Hajnoczi wrote:
> This test case adds an NBD server export and then invokes
> blockdev-snapshot-sync, which changes the BlockDriverState node that the

Do we want to test 'blockdev-snapshot' instead (or in addition), given 
the subthread discussion about blockdev-snapshot-sync being the older 
non-preferred form?

> NBD server's BlockBackend points to.  This is an interesting scenario to
> test and exercises the code path fixed by the previous commit.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   tests/qemu-iotests/208     | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/208.out |  9 ++++++++
>   tests/qemu-iotests/group   |  1 +
>   3 files changed, 65 insertions(+)
>   create mode 100755 tests/qemu-iotests/208
>   create mode 100644 tests/qemu-iotests/208.out
> 

Switching the order of the two patches in this series makes it obvious 
that this patch does tickle the code path in question, so you definitely 
get:

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

And unless answering the question about blockdev-snapshot causes you to 
change things for more/different QMP commands, I'm also fine with:

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

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

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes

[Stefan Hajnoczi]

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

On Fri, Mar 09, 2018 at 09:56:44AM -0600, Eric Blake wrote:
> On 03/06/2018 02:48 PM, Stefan Hajnoczi wrote:
> > Commit 2019ba0a0197 ("block: Add AioContextNotifier functions to BB")
> > added blk_add/remove_aio_context_notifier() and implemented them by
> > passing through the bdrv_*() equivalent.
> > 
> > This doesn't work across bdrv_append(), which detaches child->bs and
> > re-attaches it to a new BlockDriverState.  When
> > blk_remove_aio_context_notifier() is called we will access the new BDS
> > instead of the one where the notifier was added!
> > 
> > > From the point of view of the blk_*() API user, changes to the root BDS
> > should be transparent.
> > 
> > This patch maintains a list of AioContext notifiers in BlockBackend and
> > adds/removes them from the BlockDriverState as needed.
> > 
> > Reported-by: Stefano Panella <spanella@gmail.com>
> > Cc: Max Reitz <mreitz@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   block/block-backend.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   block/trace-events    |  2 ++
> >   2 files changed, 65 insertions(+)
> > 
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index 94ffbb6a60..aa27698820 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -31,6 +31,13 @@
> >   static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
> > +typedef struct BlockBackendAioNotifier {
> > +    void (*attached_aio_context)(AioContext *new_context, void *opaque);
> > +    void (*detach_aio_context)(void *opaque);
> 
> Why the difference in tense (past 'attached' vs. present 'detach')?

The naming comes from the bdrv_add_aio_context_notifier() API:

  void bdrv_add_aio_context_notifier(BlockDriverState *bs,
        void (*attached_aio_context)(AioContext *new_context, void *opaque),
        void (*detach_aio_context)(void *opaque), void *opaque)

It's "attached" because bs->aio_context has already been assigned before
the callback is invoked.

It's "detach" because the callback is invoked before bs->aio_context is
cleared.

Not great naming and I found it weird when I looked at the code too, but
at least this patch keeps the BlockBackend naming consistent with the
BlockDriverState naming.

> > @@ -1827,12 +1877,25 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
> >                                        void (*detach_aio_context)(void *),
> >                                        void *opaque)
> >   {
> > +    BlockBackendAioNotifier *notifier;
> >       BlockDriverState *bs = blk_bs(blk);
> >       if (bs) {
> >           bdrv_remove_aio_context_notifier(bs, attached_aio_context,
> >                                            detach_aio_context, opaque);
> >       }
> > +
> > +    QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
> > +        if (notifier->attached_aio_context == attached_aio_context &&
> > +            notifier->detach_aio_context == detach_aio_context &&
> > +            notifier->opaque == opaque) {
> > +            QLIST_REMOVE(notifier, list);
> 
> Don't you need to use QLIST_FOREACH_SAFE if you are going to modify the list
> during traversal?

It doesn't matter since we return right away:

  g_free(notifier);
  return;

Stefan

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

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes

[Eric Blake]

On 03/12/2018 06:27 AM, Stefan Hajnoczi wrote:
> On Fri, Mar 09, 2018 at 09:56:44AM -0600, Eric Blake wrote:
>> On 03/06/2018 02:48 PM, Stefan Hajnoczi wrote:
>>> Commit 2019ba0a0197 ("block: Add AioContextNotifier functions to BB")
>>> added blk_add/remove_aio_context_notifier() and implemented them by
>>> passing through the bdrv_*() equivalent.
>>>
>>> This doesn't work across bdrv_append(), which detaches child->bs and
>>> re-attaches it to a new BlockDriverState.  When
>>> blk_remove_aio_context_notifier() is called we will access the new BDS
>>> instead of the one where the notifier was added!
>>>
>>>>  From the point of view of the blk_*() API user, changes to the root BDS
>>> should be transparent.
>>>
>>> This patch maintains a list of AioContext notifiers in BlockBackend and
>>> adds/removes them from the BlockDriverState as needed.
>>>

>>> +typedef struct BlockBackendAioNotifier {
>>> +    void (*attached_aio_context)(AioContext *new_context, void *opaque);
>>> +    void (*detach_aio_context)(void *opaque);
>>
>> Why the difference in tense (past 'attached' vs. present 'detach')?
> 
> The naming comes from the bdrv_add_aio_context_notifier() API:
> 
>    void bdrv_add_aio_context_notifier(BlockDriverState *bs,
>          void (*attached_aio_context)(AioContext *new_context, void *opaque),
>          void (*detach_aio_context)(void *opaque), void *opaque)
> 
> It's "attached" because bs->aio_context has already been assigned before
> the callback is invoked.
> 
> It's "detach" because the callback is invoked before bs->aio_context is
> cleared.
> 
> Not great naming and I found it weird when I looked at the code too, but
> at least this patch keeps the BlockBackend naming consistent with the
> BlockDriverState naming.

Odd, but consistent, so I can live with it.


>>> +
>>> +    QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
>>> +        if (notifier->attached_aio_context == attached_aio_context &&
>>> +            notifier->detach_aio_context == detach_aio_context &&
>>> +            notifier->opaque == opaque) {
>>> +            QLIST_REMOVE(notifier, list);
>>
>> Don't you need to use QLIST_FOREACH_SAFE if you are going to modify the list
>> during traversal?
> 
> It doesn't matter since we return right away:
> 
>    g_free(notifier);
>    return;

Okay, makes sense (a safe iteration is required if you keep iterating 
after action; but if you quit, the action has no impact on subsequent 
iteration since there is no further iteration).

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

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

Re: [Qemu-devel] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes

[Max Reitz]

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

On 2018-03-06 21:48, Stefan Hajnoczi wrote:
> Commit 2019ba0a0197 ("block: Add AioContextNotifier functions to BB")
> added blk_add/remove_aio_context_notifier() and implemented them by
> passing through the bdrv_*() equivalent.
> 
> This doesn't work across bdrv_append(), which detaches child->bs and
> re-attaches it to a new BlockDriverState.  When
> blk_remove_aio_context_notifier() is called we will access the new BDS
> instead of the one where the notifier was added!

And nice that we just did not do anything if there was no BDS (in
practice that can never happen, but still nice).

Also, I like your exclamation mark.  It makes this sound so excited! :D

> From the point of view of the blk_*() API user, changes to the root BDS
> should be transparent.
> 
> This patch maintains a list of AioContext notifiers in BlockBackend and
> adds/removes them from the BlockDriverState as needed.
> 
> Reported-by: Stefano Panella <spanella@gmail.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/block-backend.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/trace-events    |  2 ++
>  2 files changed, 65 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Max Reitz]

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

On 2018-03-06 21:48, Stefan Hajnoczi wrote:
> This test case adds an NBD server export and then invokes
> blockdev-snapshot-sync, which changes the BlockDriverState node that the
> NBD server's BlockBackend points to.  This is an interesting scenario to
> test and exercises the code path fixed by the previous commit.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/208     | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/208.out |  9 ++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 65 insertions(+)
>  create mode 100755 tests/qemu-iotests/208
>  create mode 100644 tests/qemu-iotests/208.out
> 
> diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
> new file mode 100755
> index 0000000000..4e82b96c82
> --- /dev/null
> +++ b/tests/qemu-iotests/208
> @@ -0,0 +1,55 @@
> +#!/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 <http://www.gnu.org/licenses/>.
> +#
> +# Creator/Owner: Stefan Hajnoczi <stefanha@redhat.com>
> +#
> +# Check that the runtime NBD server does not crash when stopped after
> +# blockdev-snapshot-sync.
> +
> +import iotests
> +
> +with iotests.FilePath('disk.img') as disk_img_path, \
> +     iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
> +     iotests.FilePath('nbd.sock') as nbd_sock_path, \
> +     iotests.VM() as vm:
> +
> +    img_size = '10M'
> +    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk_img_path, img_size)
> +
> +    iotests.log('Launching VM...')
> +    (vm.add_drive(disk_img_path, 'node-name=drive0-node', interface='none')
> +       .launch())
> +
> +    iotests.log('Starting NBD server...')
> +    iotests.log(vm.qmp('nbd-server-start', addr={
> +            "type": "unix",
> +            "data": {
> +                "path": nbd_sock_path,
> +            }
> +        }))
> +
> +    iotests.log('Adding NBD export...')
> +    iotests.log(vm.qmp('nbd-server-add', device='drive0-node', writable=True))
> +
> +    iotests.log('Creating external snapshot...')
> +    iotests.log(vm.qmp('blockdev-snapshot-sync',
> +        node_name='drive0-node',
> +        snapshot_node_name='drive0-snapshot-node',
> +        snapshot_file=disk_snapshot_img_path))
> +
> +    iotests.log('Stopping NBD server...')
> +    iotests.log(vm.qmp('nbd-server-stop'))

Hm.  Tests what it's supposed to test.

However, I'm not sure whether that's what we want...  If I give a node
name to nbd-server-add, I'd probably expect the NBD server to stay at
that node.  We have BdrvChild.stay_at_node for that, so we could
implement it.  The question is whether we want to.

However, both wanting the snapshot to be below and above the NBD server
makes sense...  Perhaps it's best to leave it as-is, even if it is
surprising to some, because this way we at least don't have to switch to
RO mode (which we probably would if the NBD server is suddenly below the
snapshot).

I'm sure adding atomic graph change operations to QMP will fix all of
that.  I'm sure.

Anyway, since your test doesn't even check whether the NBD server is
above or below the snapshot (clever! :-))...

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] block: fix nbd-server-stop crash after blockdev-snapshot-sync

[Eric Blake]

On 03/08/2018 11:37 AM, Stefan Hajnoczi wrote:
> On Wed, Mar 07, 2018 at 05:27:45PM -0600, Eric Blake wrote:
>> On 03/06/2018 02:48 PM, Stefan Hajnoczi wrote:
>>> The blockdev-snapshot-sync command uses bdrv_append() to update all parents to
>>> point at the external snapshot node.  This breaks BlockBackend's
>>> blk_add/remove_aio_context_notifier(), which doesn't expect a BDS change.
>>>

>> Whose tree should this series go through?  MAINTAINERS didn't flag it as
>> directly touching any files that normally affect my NBD queue, but given
>> that the iotest that reproduces the problem uses NBD, I'm fine if you want
>> it to go through me.
> 
> Good question.  Max and Kevin maintain block/block-backend.c so one of
> them should be happy with this series before it gets merged.
> 
> When a patch affects multiple trees, the last sub-maintainer to review
> it can do the merge.
> 
> So if they have already posted their R-b when you are finished, then
> feel free to merge it!  And vice versa.

Max has reviewed, so this is now queued on my NBD tree, pull request to 
come shortly.

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