[PATCH v2 0/4] block/export: add BlockExportOptions->iothread member

[PATCH v2 0/4] block/export: add BlockExportOptions->iothread member

[Stefan Hajnoczi]

v2:
 * Add fixed-iothread option to set AioContext change policy [Kevin]
 * Use os-posix.c signal handling utilities in qemu-nbd.c [Paolo]

This series adjusts the build system and then adds a
BlockExportOptions->iothread member so that it is possible to set the iothread
for an export.

Based-on: 20200924151549.913737-1-stefanha@redhat.com ("[PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API")

Stefan Hajnoczi (4):
  util/vhost-user-server: use static library in meson.build
  qemu-storage-daemon: avoid compiling blockdev_ss twice
  block: move block exports to libblockdev
  block/export: add iothread and fixed-iothread options

 qapi/block-export.json               | 11 ++++++++
 block/export/export.c                | 39 ++++++++++++++++++++++++----
 block/export/vhost-user-blk-server.c |  5 +++-
 nbd/server.c                         |  2 --
 qemu-nbd.c                           | 21 +++++++--------
 stubs/blk-exp-close-all.c            |  7 +++++
 block/export/meson.build             |  4 +--
 contrib/libvhost-user/meson.build    |  1 +
 meson.build                          | 22 ++++++++++++----
 nbd/meson.build                      |  2 ++
 storage-daemon/meson.build           |  3 +--
 stubs/meson.build                    |  1 +
 tests/qtest/meson.build              |  2 +-
 util/meson.build                     |  4 ++-
 14 files changed, 93 insertions(+), 31 deletions(-)
 create mode 100644 stubs/blk-exp-close-all.c

-- 
2.26.2

[PATCH v2 1/4] util/vhost-user-server: use static library in meson.build

[Stefan Hajnoczi]

Don't compile contrib/libvhost-user/libvhost-user.c again. Instead build
the static library once and then reuse it throughout QEMU.

Also switch from CONFIG_LINUX to CONFIG_VHOST_USER, which is what the
vhost-user tools (vhost-user-gpu, etc) do.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/export/export.c             | 8 ++++----
 block/export/meson.build          | 2 +-
 contrib/libvhost-user/meson.build | 1 +
 meson.build                       | 6 +++++-
 tests/qtest/meson.build           | 2 +-
 util/meson.build                  | 4 +++-
 6 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/block/export/export.c b/block/export/export.c
index bd7cac241f..550897e236 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -17,17 +17,17 @@
 #include "sysemu/block-backend.h"
 #include "block/export.h"
 #include "block/nbd.h"
-#if CONFIG_LINUX
-#include "block/export/vhost-user-blk-server.h"
-#endif
 #include "qapi/error.h"
 #include "qapi/qapi-commands-block-export.h"
 #include "qapi/qapi-events-block-export.h"
 #include "qemu/id.h"
+#ifdef CONFIG_VHOST_USER
+#include "vhost-user-blk-server.h"
+#endif
 
 static const BlockExportDriver *blk_exp_drivers[] = {
     &blk_exp_nbd,
-#if CONFIG_LINUX
+#ifdef CONFIG_VHOST_USER
     &blk_exp_vhost_user_blk,
 #endif
 };
diff --git a/block/export/meson.build b/block/export/meson.build
index ef3a9576f7..469a7aa0f5 100644
--- a/block/export/meson.build
+++ b/block/export/meson.build
@@ -1,2 +1,2 @@
 block_ss.add(files('export.c'))
-block_ss.add(when: 'CONFIG_LINUX', if_true: files('vhost-user-blk-server.c', '../../contrib/libvhost-user/libvhost-user.c'))
+block_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c'))
diff --git a/contrib/libvhost-user/meson.build b/contrib/libvhost-user/meson.build
index e68dd1a581..a261e7665f 100644
--- a/contrib/libvhost-user/meson.build
+++ b/contrib/libvhost-user/meson.build
@@ -1,3 +1,4 @@
 libvhost_user = static_library('vhost-user',
                                files('libvhost-user.c', 'libvhost-user-glib.c'),
                                build_by_default: false)
+vhost_user = declare_dependency(link_with: libvhost_user)
diff --git a/meson.build b/meson.build
index 4c6c7310fa..eb84b97ebb 100644
--- a/meson.build
+++ b/meson.build
@@ -788,6 +788,11 @@ trace_events_subdirs += [
   'util',
 ]
 
+vhost_user = not_found
+if 'CONFIG_VHOST_USER' in config_host
+  subdir('contrib/libvhost-user')
+endif
+
 subdir('qapi')
 subdir('qobject')
 subdir('stubs')
@@ -1169,7 +1174,6 @@ if have_tools
              install: true)
 
   if 'CONFIG_VHOST_USER' in config_host
-    subdir('contrib/libvhost-user')
     subdir('contrib/vhost-user-blk')
     subdir('contrib/vhost-user-gpu')
     subdir('contrib/vhost-user-input')
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c72821b09a..aa8d0985e1 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -191,7 +191,7 @@ qos_test_ss.add(
 )
 qos_test_ss.add(when: 'CONFIG_VIRTFS', if_true: files('virtio-9p-test.c'))
 qos_test_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-test.c'))
-qos_test_ss.add(when: 'CONFIG_LINUX', if_true: files('vhost-user-blk-test.c'))
+qos_test_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-test.c'))
 
 extra_qtest_deps = {
   'bios-tables-test': [io],
diff --git a/util/meson.build b/util/meson.build
index 2296e81b34..9b2a7a5de9 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -66,7 +66,9 @@ if have_block
   util_ss.add(files('main-loop.c'))
   util_ss.add(files('nvdimm-utils.c'))
   util_ss.add(files('qemu-coroutine.c', 'qemu-coroutine-lock.c', 'qemu-coroutine-io.c'))
-  util_ss.add(when: 'CONFIG_LINUX', if_true: files('vhost-user-server.c'))
+  util_ss.add(when: 'CONFIG_VHOST_USER', if_true: [
+    files('vhost-user-server.c'), vhost_user
+  ])
   util_ss.add(files('block-helpers.c'))
   util_ss.add(files('qemu-coroutine-sleep.c'))
   util_ss.add(files('qemu-co-shared-resource.c'))
-- 
2.26.2

[PATCH v2 2/4] qemu-storage-daemon: avoid compiling blockdev_ss twice

[Stefan Hajnoczi]

Introduce libblkdev.fa to avoid recompiling blockdev_ss twice.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 meson.build                | 12 ++++++++++--
 storage-daemon/meson.build |  3 +--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index eb84b97ebb..18d689b423 100644
--- a/meson.build
+++ b/meson.build
@@ -857,7 +857,6 @@ blockdev_ss.add(files(
 blockdev_ss.add(when: 'CONFIG_POSIX', if_true: files('os-posix.c'))
 softmmu_ss.add(when: 'CONFIG_WIN32', if_true: [files('os-win32.c')])
 
-softmmu_ss.add_all(blockdev_ss)
 softmmu_ss.add(files(
   'bootdevice.c',
   'dma-helpers.c',
@@ -952,6 +951,15 @@ block = declare_dependency(link_whole: [libblock],
                            link_args: '@block.syms',
                            dependencies: [crypto, io])
 
+blockdev_ss = blockdev_ss.apply(config_host, strict: false)
+libblockdev = static_library('blockdev', blockdev_ss.sources() + genh,
+                             dependencies: blockdev_ss.dependencies(),
+                             name_suffix: 'fa',
+                             build_by_default: false)
+
+blockdev = declare_dependency(link_whole: [libblockdev],
+                              dependencies: [block])
+
 qmp_ss = qmp_ss.apply(config_host, strict: false)
 libqmp = static_library('qmp', qmp_ss.sources() + genh,
                         dependencies: qmp_ss.dependencies(),
@@ -968,7 +976,7 @@ foreach m : block_mods + softmmu_mods
                 install_dir: config_host['qemu_moddir'])
 endforeach
 
-softmmu_ss.add(authz, block, chardev, crypto, io, qmp)
+softmmu_ss.add(authz, blockdev, chardev, crypto, io, qmp)
 common_ss.add(qom, qemuutil)
 
 common_ss.add_all(when: 'CONFIG_SOFTMMU', if_true: [softmmu_ss])
diff --git a/storage-daemon/meson.build b/storage-daemon/meson.build
index 0409acc3f5..c5adce81c3 100644
--- a/storage-daemon/meson.build
+++ b/storage-daemon/meson.build
@@ -1,7 +1,6 @@
 qsd_ss = ss.source_set()
 qsd_ss.add(files('qemu-storage-daemon.c'))
-qsd_ss.add(block, chardev, qmp, qom, qemuutil)
-qsd_ss.add_all(blockdev_ss)
+qsd_ss.add(blockdev, chardev, qmp, qom, qemuutil)
 
 subdir('qapi')
 
-- 
2.26.2

[PATCH v2 3/4] block: move block exports to libblockdev

[Stefan Hajnoczi]

Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd.
They are not used by other programs and are not otherwise needed in
libblock.

Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss.
Since bdrv_close_all() (libblock) calls blk_exp_close_all()
(libblockdev) a stub function is required..

Make qemu-ndb.c use signal handling utility functions instead of
duplicating the code. This helps because os-posix.c is in libblockdev
and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks.
Once we use the signal handling utility functions we also end up
providing the necessary symbol.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-nbd.c                | 21 +++++++++------------
 stubs/blk-exp-close-all.c |  7 +++++++
 block/export/meson.build  |  4 ++--
 meson.build               |  4 ++--
 nbd/meson.build           |  2 ++
 stubs/meson.build         |  1 +
 6 files changed, 23 insertions(+), 16 deletions(-)
 create mode 100644 stubs/blk-exp-close-all.c

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 6d7ac7490f..06774ca615 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -25,6 +25,7 @@
 #include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/runstate.h" /* for qemu_system_killed() prototype */
 #include "block/block_int.h"
 #include "block/nbd.h"
 #include "qemu/main-loop.h"
@@ -155,7 +156,11 @@ QEMU_COPYRIGHT "\n"
 }
 
 #if HAVE_NBD_DEVICE
-static void termsig_handler(int signum)
+/*
+ * The client thread uses SIGTERM to interrupt the server.  A signal
+ * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
+ */
+void qemu_system_killed(int signum, pid_t pid)
 {
     atomic_cmpxchg(&state, RUNNING, TERMINATE);
     qemu_notify_event();
@@ -581,20 +586,12 @@ int main(int argc, char **argv)
     const char *pid_file_name = NULL;
     BlockExportOptions *export_opts;
 
+    os_setup_early_signal_handling();
+
 #if HAVE_NBD_DEVICE
-    /* The client thread uses SIGTERM to interrupt the server.  A signal
-     * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
-     */
-    struct sigaction sa_sigterm;
-    memset(&sa_sigterm, 0, sizeof(sa_sigterm));
-    sa_sigterm.sa_handler = termsig_handler;
-    sigaction(SIGTERM, &sa_sigterm, NULL);
+    os_setup_signal_handling();
 #endif /* HAVE_NBD_DEVICE */
 
-#ifdef CONFIG_POSIX
-    signal(SIGPIPE, SIG_IGN);
-#endif
-
     socket_init();
     error_init(argv[0]);
     module_call_init(MODULE_INIT_TRACE);
diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c
new file mode 100644
index 0000000000..1c71316763
--- /dev/null
+++ b/stubs/blk-exp-close-all.c
@@ -0,0 +1,7 @@
+#include "qemu/osdep.h"
+#include "block/export.h"
+
+/* Only used in programs that support block exports (libblockdev.fa) */
+void blk_exp_close_all(void)
+{
+}
diff --git a/block/export/meson.build b/block/export/meson.build
index 469a7aa0f5..a2772a0dce 100644
--- a/block/export/meson.build
+++ b/block/export/meson.build
@@ -1,2 +1,2 @@
-block_ss.add(files('export.c'))
-block_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c'))
+blockdev_ss.add(files('export.c'))
+blockdev_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c'))
diff --git a/meson.build b/meson.build
index 18d689b423..0e9528adab 100644
--- a/meson.build
+++ b/meson.build
@@ -835,7 +835,6 @@ subdir('dump')
 
 block_ss.add(files(
   'block.c',
-  'blockdev-nbd.c',
   'blockjob.c',
   'job.c',
   'qemu-io-cmds.c',
@@ -848,6 +847,7 @@ subdir('block')
 
 blockdev_ss.add(files(
   'blockdev.c',
+  'blockdev-nbd.c',
   'iothread.c',
   'job-qmp.c',
 ))
@@ -1171,7 +1171,7 @@ if have_tools
   qemu_io = executable('qemu-io', files('qemu-io.c'),
              dependencies: [block, qemuutil], install: true)
   qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
-               dependencies: [block, qemuutil], install: true)
+               dependencies: [blockdev, qemuutil], install: true)
 
   subdir('storage-daemon')
   subdir('contrib/rdmacm-mux')
diff --git a/nbd/meson.build b/nbd/meson.build
index 0c00a776d3..2baaa36948 100644
--- a/nbd/meson.build
+++ b/nbd/meson.build
@@ -1,5 +1,7 @@
 block_ss.add(files(
   'client.c',
   'common.c',
+))
+blockdev_ss.add(files(
   'server.c',
 ))
diff --git a/stubs/meson.build b/stubs/meson.build
index e0b322bc28..0fdcf93c09 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -1,6 +1,7 @@
 stub_ss.add(files('arch_type.c'))
 stub_ss.add(files('bdrv-next-monitor-owned.c'))
 stub_ss.add(files('blk-commit-all.c'))
+stub_ss.add(files('blk-exp-close-all.c'))
 stub_ss.add(files('blockdev-close-all-bdrv-states.c'))
 stub_ss.add(files('change-state-handler.c'))
 stub_ss.add(files('clock-warp.c'))
-- 
2.26.2

[PATCH v2 4/4] block/export: add iothread and fixed-iothread options

[Stefan Hajnoczi]

Make it possible to specify the iothread where the export will run. By
default the block node can be moved to other AioContexts later and the
export will follow. The fixed-iothread option forces strict behavior
that prevents changing AioContext while the export is active. See the
QAPI docs for details.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Note the x-blockdev-set-iothread QMP command can be used to do the same,
but not from the command-line. And it requires sending an additional
command.

In the long run vhost-user-blk will support per-virtqueue iothread
mappings. But for now a single iothread makes sense and most other
transports will just use one iothread anyway.
---
 qapi/block-export.json               | 11 ++++++++++
 block/export/export.c                | 31 +++++++++++++++++++++++++++-
 block/export/vhost-user-blk-server.c |  5 ++++-
 nbd/server.c                         |  2 --
 4 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index 87ac5117cd..e2cb21f5f1 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -219,11 +219,22 @@
 #                export before completion is signalled. (since: 5.2;
 #                default: false)
 #
+# @iothread: The name of the iothread object where the export will run. The
+#            default is to use the thread currently associated with the #
+#            block node. (since: 5.2)
+#
+# @fixed-iothread: True prevents the block node from being moved to another
+#                  thread while the export is active. If true and @iothread is
+#                  given, export creation fails if the block node cannot be
+#                  moved to the iothread. The default is false.
+#
 # Since: 4.2
 ##
 { 'union': 'BlockExportOptions',
   'base': { 'type': 'BlockExportType',
             'id': 'str',
+	    '*fixed-iothread': 'bool',
+	    '*iothread': 'str',
             'node-name': 'str',
             '*writable': 'bool',
             '*writethrough': 'bool' },
diff --git a/block/export/export.c b/block/export/export.c
index 550897e236..a5b6b02703 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -15,6 +15,7 @@
 
 #include "block/block.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/iothread.h"
 #include "block/export.h"
 #include "block/nbd.h"
 #include "qapi/error.h"
@@ -63,10 +64,11 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
 
 BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
 {
+    bool fixed_iothread = export->has_fixed_iothread && export->fixed_iothread;
     const BlockExportDriver *drv;
     BlockExport *exp = NULL;
     BlockDriverState *bs;
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
     AioContext *ctx;
     uint64_t perm;
     int ret;
@@ -102,6 +104,28 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
     ctx = bdrv_get_aio_context(bs);
     aio_context_acquire(ctx);
 
+    if (export->has_iothread) {
+        IOThread *iothread;
+        AioContext *new_ctx;
+
+        iothread = iothread_by_id(export->iothread);
+        if (!iothread) {
+            error_setg(errp, "iothread \"%s\" not found", export->iothread);
+            goto fail;
+        }
+
+        new_ctx = iothread_get_aio_context(iothread);
+
+        ret = bdrv_try_set_aio_context(bs, new_ctx, errp);
+        if (ret == 0) {
+            aio_context_release(ctx);
+            aio_context_acquire(new_ctx);
+            ctx = new_ctx;
+        } else if (fixed_iothread) {
+            goto fail;
+        }
+    }
+
     /*
      * Block exports are used for non-shared storage migration. Make sure
      * that BDRV_O_INACTIVE is cleared and the image is ready for write
@@ -116,6 +140,11 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
     }
 
     blk = blk_new(ctx, perm, BLK_PERM_ALL);
+
+    if (!fixed_iothread) {
+        blk_set_allow_aio_context_change(blk, true);
+    }
+
     ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
         goto fail;
diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index 81072a5a46..a1c37548e1 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -323,13 +323,17 @@ static const VuDevIface vu_blk_iface = {
 static void blk_aio_attached(AioContext *ctx, void *opaque)
 {
     VuBlkExport *vexp = opaque;
+
+    vexp->export.ctx = ctx;
     vhost_user_server_attach_aio_context(&vexp->vu_server, ctx);
 }
 
 static void blk_aio_detach(void *opaque)
 {
     VuBlkExport *vexp = opaque;
+
     vhost_user_server_detach_aio_context(&vexp->vu_server);
+    vexp->export.ctx = NULL;
 }
 
 static void
@@ -384,7 +388,6 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
     vu_blk_initialize_config(blk_bs(exp->blk), &vexp->blkcfg,
                                logical_block_size);
 
-    blk_set_allow_aio_context_change(exp->blk, true);
     blk_add_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
                                  vexp);
 
diff --git a/nbd/server.c b/nbd/server.c
index f74766add7..8676008319 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1560,8 +1560,6 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
         return ret;
     }
 
-    blk_set_allow_aio_context_change(blk, true);
-
     QTAILQ_INIT(&exp->clients);
     exp->name = g_strdup(arg->name);
     exp->description = g_strdup(arg->description);
-- 
2.26.2

Re: [PATCH v2 3/4] block: move block exports to libblockdev

[Paolo Bonzini]

On 29/09/20 14:55, Stefan Hajnoczi wrote:
> Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd.
> They are not used by other programs and are not otherwise needed in
> libblock.
> 
> Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss.
> Since bdrv_close_all() (libblock) calls blk_exp_close_all()
> (libblockdev) a stub function is required..
> 
> Make qemu-ndb.c use signal handling utility functions instead of
> duplicating the code. This helps because os-posix.c is in libblockdev
> and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks.
> Once we use the signal handling utility functions we also end up
> providing the necessary symbol.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qemu-nbd.c                | 21 +++++++++------------
>  stubs/blk-exp-close-all.c |  7 +++++++
>  block/export/meson.build  |  4 ++--
>  meson.build               |  4 ++--
>  nbd/meson.build           |  2 ++
>  stubs/meson.build         |  1 +
>  6 files changed, 23 insertions(+), 16 deletions(-)
>  create mode 100644 stubs/blk-exp-close-all.c
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 6d7ac7490f..06774ca615 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -25,6 +25,7 @@
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "sysemu/block-backend.h"
> +#include "sysemu/runstate.h" /* for qemu_system_killed() prototype */
>  #include "block/block_int.h"
>  #include "block/nbd.h"
>  #include "qemu/main-loop.h"
> @@ -155,7 +156,11 @@ QEMU_COPYRIGHT "\n"
>  }
>  
>  #if HAVE_NBD_DEVICE
> -static void termsig_handler(int signum)
> +/*
> + * The client thread uses SIGTERM to interrupt the server.  A signal
> + * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
> + */
> +void qemu_system_killed(int signum, pid_t pid)
>  {
>      atomic_cmpxchg(&state, RUNNING, TERMINATE);
>      qemu_notify_event();
> @@ -581,20 +586,12 @@ int main(int argc, char **argv)
>      const char *pid_file_name = NULL;
>      BlockExportOptions *export_opts;
>  
> +    os_setup_early_signal_handling();
> +
>  #if HAVE_NBD_DEVICE
> -    /* The client thread uses SIGTERM to interrupt the server.  A signal
> -     * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
> -     */
> -    struct sigaction sa_sigterm;
> -    memset(&sa_sigterm, 0, sizeof(sa_sigterm));
> -    sa_sigterm.sa_handler = termsig_handler;
> -    sigaction(SIGTERM, &sa_sigterm, NULL);
> +    os_setup_signal_handling();
>  #endif /* HAVE_NBD_DEVICE */
>  
> -#ifdef CONFIG_POSIX
> -    signal(SIGPIPE, SIG_IGN);
> -#endif
> -
>      socket_init();
>      error_init(argv[0]);
>      module_call_init(MODULE_INIT_TRACE);
> diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c
> new file mode 100644
> index 0000000000..1c71316763
> --- /dev/null
> +++ b/stubs/blk-exp-close-all.c
> @@ -0,0 +1,7 @@
> +#include "qemu/osdep.h"
> +#include "block/export.h"
> +
> +/* Only used in programs that support block exports (libblockdev.fa) */
> +void blk_exp_close_all(void)
> +{
> +}
> diff --git a/block/export/meson.build b/block/export/meson.build
> index 469a7aa0f5..a2772a0dce 100644
> --- a/block/export/meson.build
> +++ b/block/export/meson.build
> @@ -1,2 +1,2 @@
> -block_ss.add(files('export.c'))
> -block_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c'))
> +blockdev_ss.add(files('export.c'))
> +blockdev_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c'))
> diff --git a/meson.build b/meson.build
> index 18d689b423..0e9528adab 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -835,7 +835,6 @@ subdir('dump')
>  
>  block_ss.add(files(
>    'block.c',
> -  'blockdev-nbd.c',
>    'blockjob.c',
>    'job.c',
>    'qemu-io-cmds.c',
> @@ -848,6 +847,7 @@ subdir('block')
>  
>  blockdev_ss.add(files(
>    'blockdev.c',
> +  'blockdev-nbd.c',
>    'iothread.c',
>    'job-qmp.c',
>  ))
> @@ -1171,7 +1171,7 @@ if have_tools
>    qemu_io = executable('qemu-io', files('qemu-io.c'),
>               dependencies: [block, qemuutil], install: true)
>    qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
> -               dependencies: [block, qemuutil], install: true)
> +               dependencies: [blockdev, qemuutil], install: true)
>  
>    subdir('storage-daemon')
>    subdir('contrib/rdmacm-mux')
> diff --git a/nbd/meson.build b/nbd/meson.build
> index 0c00a776d3..2baaa36948 100644
> --- a/nbd/meson.build
> +++ b/nbd/meson.build
> @@ -1,5 +1,7 @@
>  block_ss.add(files(
>    'client.c',
>    'common.c',
> +))
> +blockdev_ss.add(files(
>    'server.c',
>  ))
> diff --git a/stubs/meson.build b/stubs/meson.build
> index e0b322bc28..0fdcf93c09 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -1,6 +1,7 @@
>  stub_ss.add(files('arch_type.c'))
>  stub_ss.add(files('bdrv-next-monitor-owned.c'))
>  stub_ss.add(files('blk-commit-all.c'))
> +stub_ss.add(files('blk-exp-close-all.c'))
>  stub_ss.add(files('blockdev-close-all-bdrv-states.c'))
>  stub_ss.add(files('change-state-handler.c'))
>  stub_ss.add(files('clock-warp.c'))
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH v2 3/4] block: move block exports to libblockdev

[Eric Blake]

On 9/29/20 7:55 AM, Stefan Hajnoczi wrote:
> Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd.
> They are not used by other programs and are not otherwise needed in
> libblock.
> 
> Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss.
> Since bdrv_close_all() (libblock) calls blk_exp_close_all()
> (libblockdev) a stub function is required..
> 
> Make qemu-ndb.c use signal handling utility functions instead of

nbd

> duplicating the code. This helps because os-posix.c is in libblockdev
> and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks.
> Once we use the signal handling utility functions we also end up
> providing the necessary symbol.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

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

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

Re: [PATCH v2 4/4] block/export: add iothread and fixed-iothread options

[Eric Blake]

On 9/29/20 7:55 AM, Stefan Hajnoczi wrote:
> Make it possible to specify the iothread where the export will run. By
> default the block node can be moved to other AioContexts later and the
> export will follow. The fixed-iothread option forces strict behavior
> that prevents changing AioContext while the export is active. See the
> QAPI docs for details.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Note the x-blockdev-set-iothread QMP command can be used to do the same,
> but not from the command-line. And it requires sending an additional
> command.
> 
> In the long run vhost-user-blk will support per-virtqueue iothread
> mappings. But for now a single iothread makes sense and most other
> transports will just use one iothread anyway.
> ---
>   qapi/block-export.json               | 11 ++++++++++
>   block/export/export.c                | 31 +++++++++++++++++++++++++++-
>   block/export/vhost-user-blk-server.c |  5 ++++-
>   nbd/server.c                         |  2 --
>   4 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index 87ac5117cd..e2cb21f5f1 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -219,11 +219,22 @@
>   #                export before completion is signalled. (since: 5.2;
>   #                default: false)
>   #
> +# @iothread: The name of the iothread object where the export will run. The
> +#            default is to use the thread currently associated with the #

Stray #

> +#            block node. (since: 5.2)
> +#
> +# @fixed-iothread: True prevents the block node from being moved to another
> +#                  thread while the export is active. If true and @iothread is
> +#                  given, export creation fails if the block node cannot be
> +#                  moved to the iothread. The default is false.
> +#

Missing a '(since 5.2)' tag.  (Hmm, we're inconsistent on whether it is 
'since 5.2' or 'since: 5.2' inside () parentheticals; Markus, is that 
something we should be cleaning up as part of the conversion to rST?)

> @@ -63,10 +64,11 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
>   
>   BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>   {
> +    bool fixed_iothread = export->has_fixed_iothread && export->fixed_iothread;

Technically, our QAPI code guarantees that export->fixed_iothread is 
false if export->has_fixed_iothread is false.  And someday I'd love to 
let QAPI express default values for bools so that we don't need a 
has_FOO field when a default has been expressed.  But neither of those 
points affect this patch; what you have is correct even if it is verbose.

Otherwise looks reasonable.

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

Re: [PATCH v2 4/4] block/export: add iothread and fixed-iothread options

[Stefan Hajnoczi]

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

On Tue, Sep 29, 2020 at 08:07:38AM -0500, Eric Blake wrote:
> On 9/29/20 7:55 AM, Stefan Hajnoczi wrote:
> > Make it possible to specify the iothread where the export will run. By
> > default the block node can be moved to other AioContexts later and the
> > export will follow. The fixed-iothread option forces strict behavior
> > that prevents changing AioContext while the export is active. See the
> > QAPI docs for details.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > Note the x-blockdev-set-iothread QMP command can be used to do the same,
> > but not from the command-line. And it requires sending an additional
> > command.
> > 
> > In the long run vhost-user-blk will support per-virtqueue iothread
> > mappings. But for now a single iothread makes sense and most other
> > transports will just use one iothread anyway.
> > ---
> >   qapi/block-export.json               | 11 ++++++++++
> >   block/export/export.c                | 31 +++++++++++++++++++++++++++-
> >   block/export/vhost-user-blk-server.c |  5 ++++-
> >   nbd/server.c                         |  2 --
> >   4 files changed, 45 insertions(+), 4 deletions(-)
> > 
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index 87ac5117cd..e2cb21f5f1 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> > @@ -219,11 +219,22 @@
> >   #                export before completion is signalled. (since: 5.2;
> >   #                default: false)
> >   #
> > +# @iothread: The name of the iothread object where the export will run. The
> > +#            default is to use the thread currently associated with the #
> 
> Stray #
> 
> > +#            block node. (since: 5.2)
> > +#
> > +# @fixed-iothread: True prevents the block node from being moved to another
> > +#                  thread while the export is active. If true and @iothread is
> > +#                  given, export creation fails if the block node cannot be
> > +#                  moved to the iothread. The default is false.
> > +#
> 
> Missing a '(since 5.2)' tag.  (Hmm, we're inconsistent on whether it is
> 'since 5.2' or 'since: 5.2' inside () parentheticals; Markus, is that
> something we should be cleaning up as part of the conversion to rST?)
> 
> > @@ -63,10 +64,11 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
> >   BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
> >   {
> > +    bool fixed_iothread = export->has_fixed_iothread && export->fixed_iothread;
> 
> Technically, our QAPI code guarantees that export->fixed_iothread is false
> if export->has_fixed_iothread is false.  And someday I'd love to let QAPI
> express default values for bools so that we don't need a has_FOO field when
> a default has been expressed.  But neither of those points affect this
> patch; what you have is correct even if it is verbose.
> 
> Otherwise looks reasonable.

Great, thanks for pointing this out.

I'll wait for comments from Kevin. These things could be fixed when
merging.

Stefan

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

Re: [PATCH v2 3/4] block: move block exports to libblockdev

[Eric Blake]

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

On 9/29/20 7:55 AM, Stefan Hajnoczi wrote:
> Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd.
> They are not used by other programs and are not otherwise needed in
> libblock.
> 
> Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss.
> Since bdrv_close_all() (libblock) calls blk_exp_close_all()
> (libblockdev) a stub function is required..
> 
> Make qemu-ndb.c use signal handling utility functions instead of
> duplicating the code. This helps because os-posix.c is in libblockdev
> and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks.
> Once we use the signal handling utility functions we also end up
> providing the necessary symbol.

Hmm. I just stumbled on a long-standing bug in qemu-nbd - it installs a
SIGTERM handler, but not a SIGINT or SIGHUP handler.  This matters in
the following sequence:

qemu-nbd -f qcow2 -B bitmap image   # Ctrl-C
qemu-nbd -f qcow2 -B bitmap image

because the first instance dies with SIGINT but there is no handler
installed, qemu-nbd does not release the bitmap from being marked
in-use, and the second instance then fails with:

qemu-nbd: Bitmap 'b0' is inconsistent and cannot be used

And to my surprise, while I was trying to find the root cause to fixing
the bug I just found, I noticed that your patch happens to fix that...

> +++ b/qemu-nbd.c

> @@ -581,20 +586,12 @@ int main(int argc, char **argv)
>      const char *pid_file_name = NULL;
>      BlockExportOptions *export_opts;
>  
> +    os_setup_early_signal_handling();
> +
>  #if HAVE_NBD_DEVICE
> -    /* The client thread uses SIGTERM to interrupt the server.  A signal
> -     * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
> -     */
> -    struct sigaction sa_sigterm;
> -    memset(&sa_sigterm, 0, sizeof(sa_sigterm));
> -    sa_sigterm.sa_handler = termsig_handler;
> -    sigaction(SIGTERM, &sa_sigterm, NULL);
> +    os_setup_signal_handling();

...by installing a SIGINT handler.

Is HAVE_NBD_DEVICE really the best gate for this code, or is it really
whether we are compiling for mingw?  At any rate, you may want to add a
link to https://bugzilla.redhat.com/show_bug.cgi?id=1883608 in the
commit message, and/or separate the bug fix out into a separate commit.

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


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

Re: [PATCH v2 3/4] block: move block exports to libblockdev

[Stefan Hajnoczi]

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

On Tue, Sep 29, 2020 at 12:36:10PM -0500, Eric Blake wrote:
> On 9/29/20 7:55 AM, Stefan Hajnoczi wrote:
> > Block exports are used by softmmu, qemu-storage-daemon, and qemu-nbd.
> > They are not used by other programs and are not otherwise needed in
> > libblock.
> > 
> > Undo the recent move of blockdev-nbd.c from blockdev_ss into block_ss.
> > Since bdrv_close_all() (libblock) calls blk_exp_close_all()
> > (libblockdev) a stub function is required..
> > 
> > Make qemu-ndb.c use signal handling utility functions instead of
> > duplicating the code. This helps because os-posix.c is in libblockdev
> > and it depends on a qemu_system_killed() symbol that qemu-nbd.c lacks.
> > Once we use the signal handling utility functions we also end up
> > providing the necessary symbol.
> 
> Hmm. I just stumbled on a long-standing bug in qemu-nbd - it installs a
> SIGTERM handler, but not a SIGINT or SIGHUP handler.  This matters in
> the following sequence:
> 
> qemu-nbd -f qcow2 -B bitmap image   # Ctrl-C
> qemu-nbd -f qcow2 -B bitmap image
> 
> because the first instance dies with SIGINT but there is no handler
> installed, qemu-nbd does not release the bitmap from being marked
> in-use, and the second instance then fails with:
> 
> qemu-nbd: Bitmap 'b0' is inconsistent and cannot be used
> 
> And to my surprise, while I was trying to find the root cause to fixing
> the bug I just found, I noticed that your patch happens to fix that...
> 
> > +++ b/qemu-nbd.c
> 
> > @@ -581,20 +586,12 @@ int main(int argc, char **argv)
> >      const char *pid_file_name = NULL;
> >      BlockExportOptions *export_opts;
> >  
> > +    os_setup_early_signal_handling();
> > +
> >  #if HAVE_NBD_DEVICE
> > -    /* The client thread uses SIGTERM to interrupt the server.  A signal
> > -     * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
> > -     */
> > -    struct sigaction sa_sigterm;
> > -    memset(&sa_sigterm, 0, sizeof(sa_sigterm));
> > -    sa_sigterm.sa_handler = termsig_handler;
> > -    sigaction(SIGTERM, &sa_sigterm, NULL);
> > +    os_setup_signal_handling();
> 
> ...by installing a SIGINT handler.
> 
> Is HAVE_NBD_DEVICE really the best gate for this code, or is it really
> whether we are compiling for mingw?  At any rate, you may want to add a
> link to https://bugzilla.redhat.com/show_bug.cgi?id=1883608 in the
> commit message, and/or separate the bug fix out into a separate commit.

Thanks for letting me know about the bug report. The link can be
added to the commit description when merging (or if I resend).

Stefan

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

Re: [PATCH v2 0/4] block/export: add BlockExportOptions->iothread member

[Stefan Hajnoczi]

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

On Tue, Sep 29, 2020 at 01:55:12PM +0100, Stefan Hajnoczi wrote:
> v2:
>  * Add fixed-iothread option to set AioContext change policy [Kevin]
>  * Use os-posix.c signal handling utilities in qemu-nbd.c [Paolo]
> 
> This series adjusts the build system and then adds a
> BlockExportOptions->iothread member so that it is possible to set the iothread
> for an export.
> 
> Based-on: 20200924151549.913737-1-stefanha@redhat.com ("[PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API")
> 
> Stefan Hajnoczi (4):
>   util/vhost-user-server: use static library in meson.build
>   qemu-storage-daemon: avoid compiling blockdev_ss twice
>   block: move block exports to libblockdev
>   block/export: add iothread and fixed-iothread options
> 
>  qapi/block-export.json               | 11 ++++++++
>  block/export/export.c                | 39 ++++++++++++++++++++++++----
>  block/export/vhost-user-blk-server.c |  5 +++-
>  nbd/server.c                         |  2 --
>  qemu-nbd.c                           | 21 +++++++--------
>  stubs/blk-exp-close-all.c            |  7 +++++
>  block/export/meson.build             |  4 +--
>  contrib/libvhost-user/meson.build    |  1 +
>  meson.build                          | 22 ++++++++++++----
>  nbd/meson.build                      |  2 ++
>  storage-daemon/meson.build           |  3 +--
>  stubs/meson.build                    |  1 +
>  tests/qtest/meson.build              |  2 +-
>  util/meson.build                     |  4 ++-
>  14 files changed, 93 insertions(+), 31 deletions(-)
>  create mode 100644 stubs/blk-exp-close-all.c
> 
> -- 
> 2.26.2
> 

Thanks, applied to my block tree with Eric's suggestions applied:
https://github.com/stefanha/qemu/commits/block

Stefan

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