* [PATCH v2 1/4] util/vhost-user-server: use static library in meson.build
2020-09-29 12:55 [PATCH v2 0/4] block/export: add BlockExportOptions->iothread member Stefan Hajnoczi
@ 2020-09-29 12:55 ` Stefan Hajnoczi
2020-09-29 12:55 ` [PATCH v2 2/4] qemu-storage-daemon: avoid compiling blockdev_ss twice Stefan Hajnoczi
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-09-29 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 2/4] qemu-storage-daemon: avoid compiling blockdev_ss twice
2020-09-29 12:55 [PATCH v2 0/4] block/export: add BlockExportOptions->iothread member Stefan Hajnoczi
2020-09-29 12:55 ` [PATCH v2 1/4] util/vhost-user-server: use static library in meson.build Stefan Hajnoczi
@ 2020-09-29 12:55 ` Stefan Hajnoczi
2020-09-29 12:55 ` [PATCH v2 3/4] block: move block exports to libblockdev Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-09-29 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 3/4] block: move block exports to libblockdev
2020-09-29 12:55 [PATCH v2 0/4] block/export: add BlockExportOptions->iothread member Stefan Hajnoczi
2020-09-29 12:55 ` [PATCH v2 1/4] util/vhost-user-server: use static library in meson.build Stefan Hajnoczi
2020-09-29 12:55 ` [PATCH v2 2/4] qemu-storage-daemon: avoid compiling blockdev_ss twice Stefan Hajnoczi
@ 2020-09-29 12:55 ` Stefan Hajnoczi
2020-09-29 13:02 ` Paolo Bonzini
` (2 more replies)
2020-09-29 12:55 ` [PATCH v2 4/4] block/export: add iothread and fixed-iothread options Stefan Hajnoczi
2020-10-09 10:17 ` [PATCH v2 0/4] block/export: add BlockExportOptions->iothread member Stefan Hajnoczi
4 siblings, 3 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-09-29 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/4] block: move block exports to libblockdev
2020-09-29 12:55 ` [PATCH v2 3/4] block: move block exports to libblockdev Stefan Hajnoczi
@ 2020-09-29 13:02 ` Paolo Bonzini
2020-09-29 13:03 ` Eric Blake
2020-09-29 17:36 ` Eric Blake
2 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2020-09-29 13:02 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz
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>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/4] block: move block exports to libblockdev
2020-09-29 12:55 ` [PATCH v2 3/4] block: move block exports to libblockdev Stefan Hajnoczi
2020-09-29 13:02 ` Paolo Bonzini
@ 2020-09-29 13:03 ` Eric Blake
2020-09-29 17:36 ` Eric Blake
2 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2020-09-29 13:03 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] block: move block exports to libblockdev
2020-09-29 12:55 ` [PATCH v2 3/4] block: move block exports to libblockdev Stefan Hajnoczi
2020-09-29 13:02 ` Paolo Bonzini
2020-09-29 13:03 ` Eric Blake
@ 2020-09-29 17:36 ` Eric Blake
2020-09-30 8:51 ` Stefan Hajnoczi
2 siblings, 1 reply; 12+ messages in thread
From: Eric Blake @ 2020-09-29 17:36 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] block: move block exports to libblockdev
2020-09-29 17:36 ` Eric Blake
@ 2020-09-30 8:51 ` Stefan Hajnoczi
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-09-30 8:51 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, qemu-devel, Coiby Xu, Max Reitz,
Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] block/export: add iothread and fixed-iothread options
2020-09-29 12:55 [PATCH v2 0/4] block/export: add BlockExportOptions->iothread member Stefan Hajnoczi
` (2 preceding siblings ...)
2020-09-29 12:55 ` [PATCH v2 3/4] block: move block exports to libblockdev Stefan Hajnoczi
@ 2020-09-29 12:55 ` Stefan Hajnoczi
2020-09-29 13:07 ` Eric Blake
2020-10-09 10:17 ` [PATCH v2 0/4] block/export: add BlockExportOptions->iothread member Stefan Hajnoczi
4 siblings, 1 reply; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-09-29 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] block/export: add iothread and fixed-iothread options
2020-09-29 12:55 ` [PATCH v2 4/4] block/export: add iothread and fixed-iothread options Stefan Hajnoczi
@ 2020-09-29 13:07 ` Eric Blake
2020-09-29 15:44 ` Stefan Hajnoczi
0 siblings, 1 reply; 12+ messages in thread
From: Eric Blake @ 2020-09-29 13:07 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] block/export: add iothread and fixed-iothread options
2020-09-29 13:07 ` Eric Blake
@ 2020-09-29 15:44 ` Stefan Hajnoczi
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-09-29 15:44 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, qemu-devel, Coiby Xu, Max Reitz,
Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/4] block/export: add BlockExportOptions->iothread member
2020-09-29 12:55 [PATCH v2 0/4] block/export: add BlockExportOptions->iothread member Stefan Hajnoczi
` (3 preceding siblings ...)
2020-09-29 12:55 ` [PATCH v2 4/4] block/export: add iothread and fixed-iothread options Stefan Hajnoczi
@ 2020-10-09 10:17 ` Stefan Hajnoczi
4 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-10-09 10:17 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread