[PATCH 0/3] Exposing backing-chain allocation over NBD
I'm working on preparing my KVM Forum presentation on NBD, and ran into a situation where I really wanted to do the equivalent of 'qemu-img map' over NBD for determining which portions of an overlay image are changed from the backing layer. So after less than 24 hours hacking, I'm pretty pleased with the results. Known caveats: - Probably has lots of conflicts with Kevin's pending work on refactoring NBD for nicer use in qemu-storage-daemon - Not yet tested with Vladimir's patches to fix bdrv_block_status bugs when the backing file is short (and therefore, applying this series without that is likely to make it possible to expose the same bugs of wrong information) - I _still_ want to get QMP 'block-dirty-bitmap-populate' in qemu 5.2; with that in place, you could avoid the need for this series by instead populating a temporary bitmap and exposing that bitmap over NBD instead. But that requires more work, both in coding (Peter Krempa and I still need to make sure we have the ideal QMP interface) and in usage (managing temporary bitmaps is more effort than a new bool toggle). - And if we _did_ use block-dirty-bitmap-populate, I find myself wanting to be able to expose more than one bitmap at a time over NBD, which in turn means revisiting our current QAPI for nbd-server-add of '*bitmap':'str' to instead allow an alternate type that permits either "'str'" or "['str']", except that our QAPI generator does not yet support arrays in alternates, and is undergoing changes from John Snow for python cleanups... - I am aware of long-standing qemu bugs where when we advertise a large minimum block size (say 4k) but the backing file has smaller granularity (such as 512), then we can violate NBD protocol by sending a reply to NBD_CMD_BLOCK_STATUS with misaligned data. This adds yet another instance of being able to tickle that (rare) bug. I really need to revisit my patches to add bdrv_block_status_aligned... Also available at: https://repo.or.cz/qemu/ericb.git/shortlog/refs/tags/nbd-alloc-depth-v1 Eric Blake (3): nbd: Simplify meta-context parsing nbd: Add new qemu:allocation-depth metacontext nbd: Add 'qemu-nbd -A' to expose allocation depth docs/interop/nbd.txt | 22 ++- docs/tools/qemu-nbd.rst| 6 + qapi/block-core.json | 13 +- include/block/nbd.h| 15 +- blockdev-nbd.c | 3 +- nbd/server.c | 294 - qemu-nbd.c | 16 +- tests/qemu-iotests/309 | 73 + tests/qemu-iotests/309.out | 22 +++ tests/qemu-iotests/group | 1 + 10 files changed, 310 insertions(+), 155 deletions(-) create mode 100755 tests/qemu-iotests/309 create mode 100644 tests/qemu-iotests/309.out -- 2.28.0
[PATCH 2/3] nbd: Add new qemu:allocation-depth metacontext
'qemu-img map' provides a way to determine which extents of an image
come from the top layer vs. inherited from a backing chain. This is
useful information worth exposing over NBD. There is a proposal to
add a QMP command block-dirty-bitmap-populate which can create a dirty
bitmap that reflects allocation information, at which point
qemu:dirty-bitmap:NAME can expose that information via the creation of
a temporary bitmap, but we can shorten the effort by adding a new
qemu:allocation-depth context that does the same thing without an
intermediate bitmap (this patch does not eliminate the need for that
proposal, as it will have other uses as well).
For this patch, I just encoded a tri-state value (unallocated, from
this layer, from any of the backing layers); we could instead or in
addition report an actual depth count per extent, if that proves more
useful.
Note that this patch does not actually enable any way to request a
server to enable this context; that will come in the next patch.
Signed-off-by: Eric Blake
---
docs/interop/nbd.txt | 22 +++--
qapi/block-core.json | 6 ++-
include/block/nbd.h | 8 +++-
nbd/server.c | 110 +++
4 files changed, 132 insertions(+), 14 deletions(-)
diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
index f3b3cacc9621..56efec7aee12 100644
--- a/docs/interop/nbd.txt
+++ b/docs/interop/nbd.txt
@@ -17,9 +17,9 @@ namespace "qemu".
== "qemu" namespace ==
-The "qemu" namespace currently contains only one type of context,
-related to exposing the contents of a dirty bitmap alongside the
-associated disk contents. That context has the following form:
+The "qemu" namespace currently contains two types of context. The
+first is related to exposing the contents of a dirty bitmap alongside
+the associated disk contents. That context has the following form:
qemu:dirty-bitmap:
@@ -28,8 +28,21 @@ in reply for NBD_CMD_BLOCK_STATUS:
bit 0: NBD_STATE_DIRTY, means that the extent is "dirty"
+The second is related to exposing the source of various extents within
+the image, with a single context named:
+
+qemu:allocation-depth
+
+In the allocation depth context, bits 0 and 1 form a tri-state value:
+
+bits 0-1 clear: NBD_STATE_DEPTH_UNALLOC, means the extent is unallocated
+bit 0 set: NBD_STATE_DEPTH_LOCAL, the extent is allocated in this image
+bit 1 set: NBD_STATE_DEPTH_BACKING, the extent is inherited from a
+ backing layer
+
For NBD_OPT_LIST_META_CONTEXT the following queries are supported
-in addition to "qemu:dirty-bitmap:":
+in addition to the specific "qemu:allocation-depth" and
+"qemu:dirty-bitmap:":
* "qemu:" - returns list of all available metadata contexts in the
namespace.
@@ -55,3 +68,4 @@ the operation of that feature.
NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE
* 4.2: NBD_FLAG_CAN_MULTI_CONN for shareable read-only exports,
NBD_CMD_FLAG_FAST_ZERO
+* 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth"
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3c16f1e11d6b..b3ec30a83cd7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5261,11 +5261,15 @@
# NBD client can use NBD_OPT_SET_META_CONTEXT with
# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
#
+# @alloc: Also export the allocation map for @device, so the NBD client
+# can use NBD_OPT_SET_META_CONTEXT with "qemu:allocation-depth"
+# to inspect allocation details. (since 5.2)
+#
# Since: 5.0
##
{ 'struct': 'BlockExportNbd',
'data': {'device': 'str', '*name': 'str', '*description': 'str',
- '*writable': 'bool', '*bitmap': 'str' } }
+ '*writable': 'bool', '*bitmap': 'str', '*alloc': 'bool' } }
##
# @nbd-server-add:
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9bc3bfaeecf8..71a2623f7842 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016-2019 Red Hat, Inc.
+ * Copyright (C) 2016-2020 Red Hat, Inc.
* Copyright (C) 2005 Anthony Liguori
*
* Network Block Device
@@ -257,6 +257,12 @@ enum {
/* Extent flags for qemu:dirty-bitmap in NBD_REPLY_TYPE_BLOCK_STATUS */
#define NBD_STATE_DIRTY (1 << 0)
+/* Extent flags for qemu:allocation-depth in NBD_REPLY_TYPE_BLOCK_STATUS */
+#define NBD_STATE_DEPTH_UNALLOC (0 << 0)
+#define NBD_STATE_DEPTH_LOCAL (1 << 0)
+#define NBD_STATE_DEPTH_BACKING (2 << 0)
+#define NBD_STATE_DEPTH_MASK(0x3)
+
static inline bool nbd_reply_type_is_error(int type)
{
return type & (1 << 15);
diff --git a/nbd/server.c b/nbd/server.c
index 0d2d7e52058f..02d5fb375b24 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -25,7 +25,8 @@
#include "qemu/units.h"
#define NBD_META_ID_BASE_ALLOCATION 0
-#define NBD_META_ID_DIRTY_BITMAP 1
+#define NBD_META_ID_ALLOCATION_DEPTH 1
+#define NBD_META_ID_DIRTY_BITMAP 2
/*
* NBD_MAX_BLOCK_STATUS_EXTENTS: 1 MiB of extents data. An
[PATCH 3/3] nbd: Add 'qemu-nbd -A' to expose allocation depth
Allow the server to expose an additional metacontext to be requested
by savvy clients. qemu-nbd adds a new option -A to expose the
qemu:allocation-depth metacontext through NBD_CMD_BLOCK_STATUS; this
can also be set via QMP when using nbd-server-add.
qemu as client can be hacked into viewing this new context by using
the now-misnamed x-dirty-bitmap option when creating an NBD blockdev;
although it is worth noting the decoding of how such context
information will appear in 'qemu-img map --output=json':
NBD_STATE_DEPTH_UNALLOC => "zero":false, "data":true
NBD_STATE_DEPTH_LOCAL => "zero":false, "data":false
NBD_STATE_DEPTH_BACKING => "zero":true, "data":true
libnbd as client is probably a nicer way to get at the information
without having to decipher such hacks in qemu as client. ;)
Signed-off-by: Eric Blake
---
docs/tools/qemu-nbd.rst| 6
qapi/block-core.json | 7 ++--
include/block/nbd.h| 7 ++--
blockdev-nbd.c | 3 +-
nbd/server.c | 8 +++--
qemu-nbd.c | 16 ++---
tests/qemu-iotests/309 | 73 ++
tests/qemu-iotests/309.out | 22
tests/qemu-iotests/group | 1 +
9 files changed, 129 insertions(+), 14 deletions(-)
create mode 100755 tests/qemu-iotests/309
create mode 100644 tests/qemu-iotests/309.out
diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
index 667861cb22e9..0e545a97cfa3 100644
--- a/docs/tools/qemu-nbd.rst
+++ b/docs/tools/qemu-nbd.rst
@@ -72,6 +72,12 @@ driver options if ``--image-opts`` is specified.
Export the disk as read-only.
+.. option:: -A, --allocation-depth
+
+ Expose allocation depth information via the
+ ``qemu:allocation-depth`` context accessible through
+ NBD_OPT_SET_META_CONTEXT.
+
.. option:: -B, --bitmap=NAME
If *filename* has a qcow2 persistent bitmap *NAME*, expose
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b3ec30a83cd7..84f7a7a34dc1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3874,9 +3874,12 @@
#
# @tls-creds: TLS credentials ID
#
-# @x-dirty-bitmap: A "qemu:dirty-bitmap:NAME" string to query in place of
+# @x-dirty-bitmap: A metacontext name such as "qemu:dirty-bitmap:NAME" or
+# "qemu:allocation-depth" to query in place of the
# traditional "base:allocation" block status (see
-# NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)
+# NBD_OPT_LIST_META_CONTEXT in the NBD protocol; and
+# yes, naming this option x-context would have made
+# more sense) (since 3.0)
#
# @reconnect-delay: On an unexpected disconnect, the nbd client tries to
# connect again until succeeding or encountering a serious
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 71a2623f7842..f660e0274ce3 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -336,9 +336,10 @@ typedef struct NBDClient NBDClient;
NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
uint64_t size, const char *name, const char *desc,
- const char *bitmap, bool readonly, bool shared,
- void (*close)(NBDExport *), bool writethrough,
- BlockBackend *on_eject_blk, Error **errp);
+ bool alloc_depth, const char *bitmap, bool readonly,
+ bool shared, void (*close)(NBDExport *),
+ bool writethrough, BlockBackend *on_eject_blk,
+ Error **errp);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
void nbd_export_get(NBDExport *exp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 1a95d89f0096..562ea3751b85 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -203,7 +203,8 @@ void qmp_nbd_server_add(BlockExportNbd *arg, Error **errp)
arg->writable = false;
}
-exp = nbd_export_new(bs, 0, len, arg->name, arg->description, arg->bitmap,
+exp = nbd_export_new(bs, 0, len, arg->name, arg->description,
+ arg->alloc, arg->bitmap,
!arg->writable, !arg->writable,
NULL, false, on_eject_blk, errp);
if (!exp) {
diff --git a/nbd/server.c b/nbd/server.c
index 02d5fb375b24..f5e0e115b703 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1450,9 +1450,10 @@ static void nbd_eject_notifier(Notifier *n, void *data)
NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
uint64_t size, const char *name, const char *desc,
- const char *bitmap, bool readonly, bool shared,
- void (*close)(NBDExport *), bool writethrough,
- BlockBackend *on_eject_blk, Error **errp)
+ bool alloc_depth, const
[PATCH 1/3] nbd: Simplify meta-context parsing
We had a premature optimization of trying to read as little from the
wire as possible while handling NBD_OPT_SET_META_CONTEXT in phases.
But in reality, we HAVE to read the entire string from the client
before we can get to the next command, and it is easier to just read
it all at once than it is to read it in pieces. And once we do that,
several functions end up no longer performing I/O, and no longer need
to return a value.
While simplifying this, take advantage of g_str_has_prefix for less
repetition of boilerplate string length computation.
Our iotests still pass; I also checked that libnbd's testsuite (which
covers more corner cases of odd meta context requests) still passes.
Signed-off-by: Eric Blake
---
nbd/server.c | 172 ++-
1 file changed, 47 insertions(+), 125 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 982de67816a7..0d2d7e52058f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016-2018 Red Hat, Inc.
+ * Copyright (C) 2016-2020 Red Hat, Inc.
* Copyright (C) 2005 Anthony Liguori
*
* Network Block Device Server Side
@@ -792,135 +792,64 @@ static int nbd_negotiate_send_meta_context(NBDClient
*client,
return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? -EIO : 0;
}
-/* Read strlen(@pattern) bytes, and set @match to true if they match @pattern.
- * @match is never set to false.
- *
- * Return -errno on I/O error, 0 if option was completely handled by
- * sending a reply about inconsistent lengths, or 1 on success.
- *
- * Note: return code = 1 doesn't mean that we've read exactly @pattern.
- * It only means that there are no errors.
+
+/*
+ * Check @ns with @len bytes, and set @match to true if it matches @pattern,
+ * or if @len is 0 and the client is performing _LIST_. @match is never set
+ * to false.
*/
-static int nbd_meta_pattern(NBDClient *client, const char *pattern, bool
*match,
-Error **errp)
+static void nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern,
+ const char *ns, uint32_t len,
+ bool *match, Error **errp)
{
-int ret;
-char *query;
-size_t len = strlen(pattern);
-
-assert(len);
-
-query = g_malloc(len);
-ret = nbd_opt_read(client, query, len, errp);
-if (ret <= 0) {
-g_free(query);
-return ret;
-}
-
-if (strncmp(query, pattern, len) == 0) {
+if (len == 0) {
+if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
+*match = true;
+}
+trace_nbd_negotiate_meta_query_parse("empty");
+} else if (strcmp(pattern, ns) == 0) {
trace_nbd_negotiate_meta_query_parse(pattern);
*match = true;
} else {
trace_nbd_negotiate_meta_query_skip("pattern not matched");
}
-g_free(query);
-
-return 1;
-}
-
-/*
- * Read @len bytes, and set @match to true if they match @pattern, or if @len
- * is 0 and the client is performing _LIST_. @match is never set to false.
- *
- * Return -errno on I/O error, 0 if option was completely handled by
- * sending a reply about inconsistent lengths, or 1 on success.
- *
- * Note: return code = 1 doesn't mean that we've read exactly @pattern.
- * It only means that there are no errors.
- */
-static int nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern,
- uint32_t len, bool *match, Error **errp)
-{
-if (len == 0) {
-if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
-*match = true;
-}
-trace_nbd_negotiate_meta_query_parse("empty");
-return 1;
-}
-
-if (len != strlen(pattern)) {
-trace_nbd_negotiate_meta_query_skip("different lengths");
-return nbd_opt_skip(client, len, errp);
-}
-
-return nbd_meta_pattern(client, pattern, match, errp);
}
/* nbd_meta_base_query
*
* Handle queries to 'base' namespace. For now, only the base:allocation
- * context is available. 'len' is the amount of text remaining to be read from
- * the current name, after the 'base:' portion has been stripped.
- *
- * Return -errno on I/O error, 0 if option was completely handled by
- * sending a reply about inconsistent lengths, or 1 on success.
+ * context is available. @len is the length of @ns, including the 'base:'
+ * prefix.
*/
-static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
- uint32_t len, Error **errp)
+static void nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
+const char *ns, uint32_t len, Error **errp)
{
-return nbd_meta_empty_or_pattern(client, "allocation", len,
- >base_allocation, errp);
+nbd_meta_empty_or_pattern(client, "allocation", ns + 5, len - 5,
+ >base_allocation, errp);
}
/*
Re: [PATCH v4 09/14] hw/block/nvme: Support Zoned Namespace Command Set
On Sep 24 03:20, Dmitry Fomichev wrote:
> The emulation code has been changed to advertise NVM Command Set when
> "zoned" device property is not set (default) and Zoned Namespace
> Command Set otherwise.
>
> Handlers for three new NVMe commands introduced in Zoned Namespace
> Command Set specification are added, namely for Zone Management
> Receive, Zone Management Send and Zone Append.
>
> Device initialization code has been extended to create a proper
> configuration for zoned operation using device properties.
>
> Read/Write command handler is modified to only allow writes at the
> write pointer if the namespace is zoned. For Zone Append command,
> writes implicitly happen at the write pointer and the starting write
> pointer value is returned as the result of the command. Write Zeroes
> handler is modified to add zoned checks that are identical to those
> done as a part of Write flow.
>
> The code to support for Zone Descriptor Extensions is not included in
> this commit and ZDES 0 is always reported. A later commit in this
> series will add ZDE support.
>
> This commit doesn't yet include checks for active and open zone
> limits. It is assumed that there are no limits on either active or
> open zones.
>
> Signed-off-by: Niklas Cassel
> Signed-off-by: Hans Holmberg
> Signed-off-by: Ajay Joshi
> Signed-off-by: Chaitanya Kulkarni
> Signed-off-by: Matias Bjorling
> Signed-off-by: Aravind Ramesh
> Signed-off-by: Shin'ichiro Kawasaki
> Signed-off-by: Adam Manzanares
> Signed-off-by: Dmitry Fomichev
> ---
> block/nvme.c |2 +-
> hw/block/nvme.c | 1057 --
> include/block/nvme.h |6 +-
> 3 files changed, 1026 insertions(+), 39 deletions(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 05485fdd11..7a513c9a17 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -682,11 +1005,77 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
> return status;
> }
>
> +if (n->params.zoned) {
> +zone_idx = nvme_zone_idx(n, slba);
> +assert(zone_idx < n->num_zones);
> +zone = >zone_array[zone_idx];
> +
> +if (is_write) {
> +status = nvme_check_zone_write(zone, slba, nlb);
> +if (status != NVME_SUCCESS) {
> +trace_pci_nvme_err_zone_write_not_ok(slba, nlb, status);
> +return status | NVME_DNR;
> +}
> +
> +assert(nvme_wp_is_valid(zone));
> +if (append) {
> +if (unlikely(slba != zone->d.zslba)) {
> +trace_pci_nvme_err_append_not_at_start(slba,
> zone->d.zslba);
> +return NVME_ZONE_INVALID_WRITE | NVME_DNR;
> +}
> +if (data_size > (n->page_size << n->zasl)) {
> +trace_pci_nvme_err_append_too_large(slba, nlb, n->zasl);
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +slba = zone->w_ptr;
> +} else if (unlikely(slba != zone->w_ptr)) {
> +trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba,
> + zone->w_ptr);
> +return NVME_ZONE_INVALID_WRITE | NVME_DNR;
> +}
> +req->fill_ofs = -1LL;
> +} else {
> +status = nvme_check_zone_read(n, zone, slba, nlb,
> + n->params.cross_zone_read);
> +if (status != NVME_SUCCESS) {
> +trace_pci_nvme_err_zone_read_not_ok(slba, nlb, status);
> +return status | NVME_DNR;
> +}
> +
> +if (slba + nlb > zone->w_ptr) {
> +/*
> + * All or some data is read above the WP. Need to
> + * fill out the buffer area that has no backing data
> + * with a predefined data pattern (zeros by default)
> + */
> +if (slba >= zone->w_ptr) {
> +req->fill_ofs = 0;
> +} else {
> +req->fill_ofs = ((zone->w_ptr - slba) << data_shift);
> +}
If Read Across Zone Boundaries is enabled and the read in zone A
includes LBAs above the write pointer, but crossing into a full zone
(zone B), then you are gonna overwrite the valid data in zone B with the
fill pattern.
signature.asc
Description: PGP signature
Re: [PATCH v2 07/20] block/block-copy: add ratelimit to block-copy
22.07.2020 14:05, Max Reitz wrote:
On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
We are going to directly use one async block-copy operation for backup
job, so we need rate limitator.
%s/limitator/limiter/g, I think.
We want to maintain current backup behavior: only background copying is
limited and copy-before-write operations only participate in limit
calculation. Therefore we need one rate limitator for block-copy state
and boolean flag for block-copy call state for actual limitation.
Note, that we can't just calculate each chunk in limitator after
successful copying: it will not save us from starting a lot of async
sub-requests which will exceed limit too much. Instead let's use the
following scheme on sub-request creation:
1. If at the moment limit is not exceeded, create the request and
account it immediately.
2. If at the moment limit is already exceeded, drop create sub-request
and handle limit instead (by sleep).
With this approach we'll never exceed the limit more than by one
sub-request (which pretty much matches current backup behavior).
Sounds reasonable.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/block-copy.h | 8 +++
block/block-copy.c | 44 ++
2 files changed, 52 insertions(+)
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index 600984c733..d40e691123 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -59,6 +59,14 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s,
int64_t max_chunk,
BlockCopyAsyncCallbackFunc cb);
+/*
+ * Set speed limit for block-copy instance. All block-copy operations related
to
+ * this BlockCopyState will participate in speed calculation, but only
+ * block_copy_async calls with @ratelimit=true will be actually limited.
+ */
+void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state,
+ uint64_t speed);
+
BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s);
void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);
diff --git a/block/block-copy.c b/block/block-copy.c
index 4114d1fd25..851d9c8aaf 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -26,6 +26,7 @@
#define BLOCK_COPY_MAX_BUFFER (1 * MiB)
#define BLOCK_COPY_MAX_MEM (128 * MiB)
#define BLOCK_COPY_MAX_WORKERS 64
+#define BLOCK_COPY_SLICE_TIME 1ULL /* ns */
static coroutine_fn int block_copy_task_entry(AioTask *task);
@@ -36,11 +37,13 @@ typedef struct BlockCopyCallState {
int64_t bytes;
int max_workers;
int64_t max_chunk;
+bool ratelimit;
BlockCopyAsyncCallbackFunc cb;
/* State */
bool failed;
bool finished;
+QemuCoSleepState *sleep_state;
/* OUT parameters */
bool error_is_read;
@@ -103,6 +106,9 @@ typedef struct BlockCopyState {
void *progress_opaque;
SharedResource *mem;
+
+uint64_t speed;
+RateLimit rate_limit;
} BlockCopyState;
static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
@@ -611,6 +617,21 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
}
task->zeroes = ret & BDRV_BLOCK_ZERO;
+if (s->speed) {
+if (call_state->ratelimit) {
+uint64_t ns = ratelimit_calculate_delay(>rate_limit, 0);
+if (ns > 0) {
+block_copy_task_end(task, -EAGAIN);
+g_free(task);
+qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, ns,
+ _state->sleep_state);
+continue;
+}
+}
+
+ratelimit_calculate_delay(>rate_limit, task->bytes);
+}
+
Looks good.
trace_block_copy_process(s, task->offset);
co_get_from_shres(s->mem, task->bytes);
@@ -649,6 +670,13 @@ out:
return ret < 0 ? ret : found_dirty;
}
+static void block_copy_kick(BlockCopyCallState *call_state)
+{
+if (call_state->sleep_state) {
+qemu_co_sleep_wake(call_state->sleep_state);
+}
+}
+
/*
* block_copy_common
*
@@ -729,6 +757,7 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s,
.s = s,
.offset = offset,
.bytes = bytes,
+.ratelimit = ratelimit,
Hm, same problem/question as in patch 6: Should the @ratelimit parameter
really be added in patch 5 if it’s used only now?
.cb = cb,
.max_workers = max_workers ?: BLOCK_COPY_MAX_WORKERS,
.max_chunk = max_chunk,
@@ -752,3 +781,18 @@ void block_copy_set_skip_unallocated(BlockCopyState *s,
bool skip)
{
s->skip_unallocated = skip;
}
+
+void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state,
+ uint64_t speed)
+{
+uint64_t old_speed = s->speed;
+
+
Re: [PATCH 00/16] hw/block/nvme: zoned namespace command set
On Sep 25 17:06, Dmitry Fomichev wrote: > > From: Klaus Jensen > > > > * Standard blockdev-based approach to persistent state. The > > > > implementation uses a plain blockdev associated with the nvme-ns > > > > device for storing state persistently. This same 'pstate' blockdev > > > > is also used for logical block allocation tracking. > > > > Is persistent state mandatory or optional? Sorry for asking, but I am > still catching up with your other patches. I think having it optional is > a big benefit for performance testing. > Yes, the 'pstate' blockdev is optional. > > > > > > * Relies on automatic configuration of DLFEAT according to what the > > > > underlying blockdev provides (i.e. BDRV_O_UNMAP for guaranteeing > > > > zeroes on discarded blocks) for handling reads in the gaps between > > > > write pointer, ZCAP and ZSZE. Issues discards for zone resets. This > > > > removes the zero filling. > > > > Doesn't this make non-zero fill patterns impossible? In many storage > environments, vendors and admins are adamant about having varying > fill patterns to see who caused the data corruption if there is one. > Not sure how important this for the particular application, but WDC > series provides the functionality to specify the fill pattern. > That *is* a good point. By default I think it should default to either the 0x00 fill pattern (if supported by the underlying blockdev), or "no fill pattern reported" when 0x00's cannot be guaranteed. But, an option to enable filling with, say 0xff, like you do in your series, would be nice. signature.asc Description: PGP signature
Re: [PATCH v7 00/13] monitor: Optionally run handlers in coroutines
Am 10.09.2020 um 15:24 hat Stefan Hajnoczi geschrieben: > On Wed, Sep 09, 2020 at 05:11:36PM +0200, Kevin Wolf wrote: > > Some QMP command handlers can block the main loop for a relatively long > > time, for example because they perform some I/O. This is quite nasty. > > Allowing such handlers to run in a coroutine where they can yield (and > > therefore release the BQL) while waiting for an event such as I/O > > completion solves the problem. > > > > This series adds the infrastructure to allow this and switches > > block_resize to run in a coroutine as a first example. > > > > This is an alternative solution to Marc-André's "monitor: add > > asynchronous command type" series. > > Please clarify the following in the QAPI documentation: > * Is the QMP monitor suspended while the command is pending? Suspended as in monitor_suspend()? No. > * Are QMP events reported while the command is pending? Hm, I don't know to be honest. But I think so. Does it matter, though? I don't think events have a defined order compared to command results, and the client can't respond to the event anyway until the current command has completed. Kevin signature.asc Description: PGP signature
Re: [PATCH v6 14/15] scripts/simplebench: improve ascii table: add difference line
25.09.2020 13:24, Max Reitz wrote:
On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
Performance improvements / degradations are usually discussed in
percentage. Let's make the script calculate it for us.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/simplebench.py | 46 +++---
1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/scripts/simplebench/simplebench.py
b/scripts/simplebench/simplebench.py
index 56d3a91ea2..0ff05a38b8 100644
--- a/scripts/simplebench/simplebench.py
+++ b/scripts/simplebench/simplebench.py
[...]
+for j in range(0, i):
+env_j = results['envs'][j]
+res_j = case_results[env_j['id']]
+
+if 'average' not in res_j:
+# Failed result
+cell += ' --'
+continue
+
+col_j = chr(ord('A') + j)
+avg_j = res_j['average']
+delta = (res['average'] - avg_j) / avg_j * 100
I was wondering why you’d subtract, when percentage differences usually
mean a quotient. Then I realized that this would usually be written as:
(res['average'] / avg_j - 1) * 100
+delta_delta = (res['delta'] + res_j['delta']) / avg_j * 100
Why not use the new format_percent for both cases?
because I want less precision here
+cell += f' {col_j}{round(delta):+}±{round(delta_delta)}%'
I don’t know what I should think about ±delta_delta. If I saw “Compared
to run A, this is +42.1%±2.0%”, I would think that you calculated the
difference between each run result, and then based on that array
calculated average and standard deviation.
Furthermore, I don’t even know what the delta_delta is supposed to tell
you. It isn’t even a delta_delta, it’s an average_delta.
not avarage, but sum of errors. And it shows the error for the delta
The delta_delta would be (res['delta'] / res_j['delta'] - 1) * 100.0.
and this shows nothing.
Assume we have = A = 10+-2 and B = 15+-2
The difference is (15-10)+-(2+2) = 5+-4.
And your formula will give (2/2 - 1) *100 = 0, which is wrong.
Anyway, my code is mess)
And that might be presented perhaps like “+42.1% Δ± +2.0%” (if delta
were the SD, “Δx̅=+42.1% Δσ=+2.0%” would also work; although, again, I do
interpret ± as the SD anyway).
I feel that I'm bad in statistics :( I'll learn a little and make a new version.
--
Best regards,
Vladimir
RE: [PATCH 00/16] hw/block/nvme: zoned namespace command set
> -Original Message- > From: Qemu-block bounces+dmitry.fomichev=wdc@nongnu.org> On Behalf Of Klaus > Jensen > Sent: Thursday, September 24, 2020 4:45 PM > To: qemu-de...@nongnu.org > Cc: Fam Zheng ; Kevin Wolf ; qemu- > bl...@nongnu.org; Klaus Jensen ; Max Reitz > ; Keith Busch ; Klaus Jensen > > Subject: [PATCH 00/16] hw/block/nvme: zoned namespace command set > > From: Klaus Jensen > > > While going through a few rounds of reviews on Dmitry's series I have > > also continued nursing my own implementation since originally posted. I > > did not receive any reviews originally, since it depended on a lot of > > preceding series, but now, with the staging of multiple namespaces on > > nvme-next yesterday, I think it deserves another shot since it now > > applies directly. The series consists of a couple of trivial patches > > followed by the "hw/block/nvme: add support for dulbe and block > > utilization tracking", "hw/block/nvme: support namespace types" and the > > set of zoned namespace support patches. > > > > A couple of points on how this defers from Dmitry et. al.'s series and > > why I think this implementation deserves a review. > > > > * Standard blockdev-based approach to persistent state. The > > implementation uses a plain blockdev associated with the nvme-ns > > device for storing state persistently. This same 'pstate' blockdev > > is also used for logical block allocation tracking. > Is persistent state mandatory or optional? Sorry for asking, but I am still catching up with your other patches. I think having it optional is a big benefit for performance testing. > > > * Relies on automatic configuration of DLFEAT according to what the > > underlying blockdev provides (i.e. BDRV_O_UNMAP for guaranteeing > > zeroes on discarded blocks) for handling reads in the gaps between > > write pointer, ZCAP and ZSZE. Issues discards for zone resets. This > > removes the zero filling. > Doesn't this make non-zero fill patterns impossible? In many storage environments, vendors and admins are adamant about having varying fill patterns to see who caused the data corruption if there is one. Not sure how important this for the particular application, but WDC series provides the functionality to specify the fill pattern. > > > Finally, I wrote this. I am *NOT* saying that this somehow makes it > > better, but as a maintainer, is a big deal to me since both series are > > arguably a lot of code to maintain and support (both series are about > > the same size). But - I am not the only maintainer, so if Keith (now > > suddenly placed in the grim role as some sort of arbiter) signs off on > > Dmitry's series, then so be it, I will rest my case. > > > > I think we all want to see an implementation of zoned namespaces in QEMU > > sooner rather than later, but I would lie if I said I wouldn't prefer > > that it was this one. > > > > Based-on: <20200922084533.1273962-1-...@irrelevant.dk> > > > > Gollu Appalanaidu (1): > > hw/block/nvme: add commands supported and effects log page > > > > Klaus Jensen (15): > > hw/block/nvme: add nsid to get/setfeat trace events > > hw/block/nvme: add trace event for requests with non-zero status code > > hw/block/nvme: make lba data size configurable > > hw/block/nvme: reject io commands if only admin command set selected > > hw/block/nvme: consolidate read, write and write zeroes > > hw/block/nvme: add support for dulbe and block utilization tracking > > hw/block/nvme: support namespace types > > hw/block/nvme: add basic read/write for zoned namespaces > > hw/block/nvme: add the zone management receive command > > hw/block/nvme: add the zone management send command > > hw/block/nvme: add the zone append command > > hw/block/nvme: track and enforce zone resources > > hw/block/nvme: allow open to close transitions by controller > > hw/block/nvme: support zone active excursions > > hw/block/nvme: support reset/finish recommended limits > > > > docs/specs/nvme.txt | 49 +- > > hw/block/nvme-ns.h| 166 +++- > > hw/block/nvme.h | 24 + > > include/block/nvme.h | 262 ++- > > block/nvme.c |4 +- > > hw/block/nvme-ns.c| 411 +- > > hw/block/nvme.c | 1727 +++- > - > > hw/block/trace-events | 50 +- > > 8 files changed, 2580 insertions(+), 113 deletions(-) > > > > -- > > 2.28.0 > > >
Re: [PATCH v6 13/15] scripts/simplebench: improve view of ascii table
25.09.2020 12:31, Max Reitz wrote:
On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
Introduce dynamic float precision and use percentage to show delta.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/simplebench.py | 26 +-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/scripts/simplebench/simplebench.py
b/scripts/simplebench/simplebench.py
index 716d7fe9b2..56d3a91ea2 100644
--- a/scripts/simplebench/simplebench.py
+++ b/scripts/simplebench/simplebench.py
@@ -79,10 +79,34 @@ def bench_one(test_func, test_env, test_case, count=5,
initial_run=True):
return result
+def format_float(x):
+res = round(x)
+if res >= 100:
+return str(res)
+
+res = f'{x:.1f}'
+if len(res) >= 4:
+return res
+
+return f'{x:.2f}'
This itches me to ask for some log() calculation.
Like:
%.*f' % (math.ceil(math.log10(99.95 / x)), x)
Oh yes, that's cool.
+def format_percent(x):
+x *= 100
+
+res = round(x)
+if res >= 10:
+return str(res)
+
+return f'{x:.1f}' if res >= 1 else f'{x:.2f}'
Same here. (Also, why not append a % sign in this function?)
OK
def ascii_one(result):
"""Return ASCII representation of bench_one() returned dict."""
if 'average' in result:
-s = '{:.2f} +- {:.2f}'.format(result['average'], result['delta'])
+avg = result['average']
+delta_pr = result['delta'] / avg
+s = f'{format_float(avg)}±{format_percent(delta_pr)}%'
Pre-existing, but isn’t the ± range generally assumed to be the standard
deviation?
Hmm. Actually, why not, let's just use standard deviation. I wanted to show
maximum deviation, not mean, to not miss some bugs in experiment (big deviation
of one test run). Still, seems standard deviation is good enough in it.
if 'n-failed' in result:
s += '\n({} failed)'.format(result['n-failed'])
return s
--
Best regards,
Vladimir
Re: [PATCH v7 12/13] block: Add bdrv_co_move_to_aio_context()
Am 15.09.2020 um 16:31 hat Stefan Hajnoczi geschrieben: > On Wed, Sep 09, 2020 at 05:11:48PM +0200, Kevin Wolf wrote: > > Add a function to move the current coroutine to the AioContext of a > > given BlockDriverState. > > > > Signed-off-by: Kevin Wolf > > --- > > include/block/block.h | 6 ++ > > block.c | 10 ++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/include/block/block.h b/include/block/block.h > > index 981ab5b314..80ab322f11 100644 > > --- a/include/block/block.h > > +++ b/include/block/block.h > > @@ -626,6 +626,12 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, > > const char *tag); > > */ > > AioContext *bdrv_get_aio_context(BlockDriverState *bs); > > > > +/** > > + * Move the current coroutine to the AioContext of @bs and return the old > > + * AioContext of the coroutine. > > + */ > > +AioContext *coroutine_fn bdrv_co_move_to_aio_context(BlockDriverState *bs); > > I'm not sure this function handles all cases: > 1. Being called without the BQL (i.e. from an IOThread). > 2. Being called while a device stops using its IOThread. > > The races that come to mind are fetching the AioContext for bs and then > scheduling a BH. The BH is executed later on by the event loop. There > might be cases where the AioContext for bs is updated before the BH > runs. Updating the AioContext of a node drains the BDS first, so it would execute any BHs still pending in the old AioContext. So this part looks safe to me. I'm not sure what you mean with the BQL part. I don't think I'm accessing anything protected by the BQL? > I didn't investigate these cases but wanted to mention them in case you > want to add doc comments about when this function can be used or if > you'd like to verify them yourself. One thing that I'm not completely sure about (but that isn't really related to this specific patch) is AioContext changes later in the process when the actual command handler has yielded. We may have to be careful to prevent those for coroutine based QMP commands in the block layer. By sheer luck, qmp_block_resize() creates a new BlockBackend that has blk->allow_aio_context_change = false. So we're actually good in the only command I'm converting. Phew. Kevin signature.asc Description: PGP signature
Re: [PATCH v9 0/7] coroutines: generate wrapper code
On Thu, Sep 24, 2020 at 09:54:07PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > The aim of the series is to reduce code-duplication and writing > parameters structure-packing by hand around coroutine function wrappers. > > Benefits: > - no code duplication > - less indirection > > v9: Thanks to Eric, I used commit message tweaks and rebase-conflict solving > from his git. Thanks, applied to my block tree with the encoding='utf-8' and spelling/grammar fixes: https://github.com/stefanha/qemu/commits/block Stefan signature.asc Description: PGP signature
Re: [PATCH v7 13/13] block: Convert 'block_resize' to coroutine
Am 15.09.2020 um 16:57 hat Stefan Hajnoczi geschrieben:
> On Wed, Sep 09, 2020 at 05:11:49PM +0200, Kevin Wolf wrote:
> > @@ -2456,8 +2456,7 @@ void qmp_block_resize(bool has_device, const char
> > *device,
> > return;
> > }
> >
> > -aio_context = bdrv_get_aio_context(bs);
> > -aio_context_acquire(aio_context);
> > +old_ctx = bdrv_co_move_to_aio_context(bs);
> >
> > if (size < 0) {
> > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0
> > size");
>
> Is it safe to call blk_new() outside the BQL since it mutates global state?
>
> In other words, could another thread race with us?
Hm, probably not.
Would it be safer to have the bdrv_co_move_to_aio_context() call only
immediately before the drain?
> > @@ -2479,8 +2478,8 @@ void qmp_block_resize(bool has_device, const char
> > *device,
> > bdrv_drained_end(bs);
> >
> > out:
> > +aio_co_reschedule_self(old_ctx);
> > blk_unref(blk);
> > -aio_context_release(aio_context);
>
> The following precondition is violated by the blk_unref -> bdrv_drain ->
> AIO_WAIT_WHILE() call if blk->refcnt is 1 here:
>
> * The caller's thread must be the IOThread that owns @ctx or the main loop
> * thread (with @ctx acquired exactly once).
>
> blk_unref() is called from the main loop thread without having acquired
> blk's AioContext.
>
> Normally blk->refcnt will be > 1 so bdrv_drain() won't be called, but
> I'm not sure if that can be guaranteed.
>
> The following seems safer although it's uglier:
>
> aio_context = bdrv_get_aio_context(bs);
> aio_context_acquire(aio_context);
> blk_unref(blk);
> aio_context_release(aio_context);
May we actually acquire aio_context if blk is in the main thread? I
think we must only do this if it's in a different iothread because we'd
end up with a recursive lock and drain would hang.
Kevin
signature.asc
Description: PGP signature
[PATCH v1 15/15] configure: Bump the minimum required Python version to 3.6
From: Thomas Huth
All our supported build platforms have Python 3.6 or newer nowadays, and
there are some useful features in Python 3.6 which are not available in
3.5 yet (e.g. the type hint annotations which will allow us to statically
type the QAPI parser), so let's bump the minimum Python version to 3.6 now.
Signed-off-by: Thomas Huth
Message-Id: <20200923162908.95372-1-th...@redhat.com>
Signed-off-by: Alex Bennée
---
docs/conf.py | 4 ++--
configure | 4 ++--
tests/qemu-iotests/iotests.py | 2 --
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/docs/conf.py b/docs/conf.py
index 0dbd90dc1124..8aeac40124cb 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -36,9 +36,9 @@ from sphinx.errors import ConfigError
# In newer versions of Sphinx this will display nicely; in older versions
# Sphinx will also produce a Python backtrace but at least the information
# gets printed...
-if sys.version_info < (3,5):
+if sys.version_info < (3,6):
raise ConfigError(
-"QEMU requires a Sphinx that uses Python 3.5 or better\n")
+"QEMU requires a Sphinx that uses Python 3.6 or better\n")
# The per-manual conf.py will set qemu_docdir for a single-manual build;
# otherwise set it here if this is an entire-manual-set build.
diff --git a/configure b/configure
index b81868cf0231..c5f1b3bec6a8 100755
--- a/configure
+++ b/configure
@@ -1964,8 +1964,8 @@ fi
# Note that if the Python conditional here evaluates True we will exit
# with status 1 which is a shell 'false' value.
-if ! $python -c 'import sys; sys.exit(sys.version_info < (3,5))'; then
- error_exit "Cannot use '$python', Python >= 3.5 is required." \
+if ! $python -c 'import sys; sys.exit(sys.version_info < (3,6))'; then
+ error_exit "Cannot use '$python', Python >= 3.6 is required." \
"Use --python=/path/to/python to specify a supported Python."
fi
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 91e4a5712609..f48460480a99 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -40,8 +40,6 @@ sys.path.append(os.path.join(os.path.dirname(__file__), '..',
'..', 'python'))
from qemu import qtest
from qemu.qmp import QMPMessage
-assert sys.version_info >= (3, 6)
-
# Use this logger for logging messages directly from the iotests module
logger = logging.getLogger('qemu.iotests')
logger.addHandler(logging.NullHandler())
--
2.20.1
Re: [PATCH v7 09/13] qmp: Move dispatcher to a coroutine
Am 14.09.2020 um 17:30 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > This moves the QMP dispatcher to a coroutine and runs all QMP command
> > handlers that declare 'coroutine': true in coroutine context so they
> > can avoid blocking the main loop while doing I/O or waiting for other
> > events.
> >
> > For commands that are not declared safe to run in a coroutine, the
> > dispatcher drops out of coroutine context by calling the QMP command
> > handler from a bottom half.
> >
> > Signed-off-by: Kevin Wolf
> > Reviewed-by: Markus Armbruster
> > ---
> > include/qapi/qmp/dispatch.h | 1 +
> > monitor/monitor-internal.h | 6 +-
> > monitor/monitor.c | 55 +---
> > monitor/qmp.c | 122 +++-
> > qapi/qmp-dispatch.c | 61 --
> > qapi/qmp-registry.c | 3 +
> > util/aio-posix.c| 8 ++-
> > 7 files changed, 210 insertions(+), 46 deletions(-)
> >
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index 9fd2b720a7..af8d96c570 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -31,6 +31,7 @@ typedef enum QmpCommandOptions
> > typedef struct QmpCommand
> > {
> > const char *name;
> > +/* Runs in coroutine context if QCO_COROUTINE is set */
> > QmpCommandFunc *fn;
> > QmpCommandOptions options;
> > QTAILQ_ENTRY(QmpCommand) node;
> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> > index b39e03b744..b55d6df07f 100644
> > --- a/monitor/monitor-internal.h
> > +++ b/monitor/monitor-internal.h
> > @@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon)
> >
> > typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
> > extern IOThread *mon_iothread;
> > -extern QEMUBH *qmp_dispatcher_bh;
> > +extern Coroutine *qmp_dispatcher_co;
> > +extern bool qmp_dispatcher_co_shutdown;
> > +extern bool qmp_dispatcher_co_busy;
> > extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> > extern QemuMutex monitor_lock;
> > extern MonitorList mon_list;
> > @@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void);
> >
> > void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
> > void monitor_data_destroy_qmp(MonitorQMP *mon);
> > -void monitor_qmp_bh_dispatcher(void *data);
> > +void coroutine_fn monitor_qmp_dispatcher_co(void *data);
> >
> > int get_monitor_def(int64_t *pval, const char *name);
> > void help_cmd(Monitor *mon, const char *name);
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 629aa073ee..ac2722bf91 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -55,8 +55,32 @@ typedef struct {
> > /* Shared monitor I/O thread */
> > IOThread *mon_iothread;
> >
> > -/* Bottom half to dispatch the requests received from I/O thread */
> > -QEMUBH *qmp_dispatcher_bh;
> > +/* Coroutine to dispatch the requests received from I/O thread */
> > +Coroutine *qmp_dispatcher_co;
> > +
> > +/* Set to true when the dispatcher coroutine should terminate */
> > +bool qmp_dispatcher_co_shutdown;
> > +
> > +/*
> > + * qmp_dispatcher_co_busy is used for synchronisation between the
> > + * monitor thread and the main thread to ensure that the dispatcher
> > + * coroutine never gets scheduled a second time when it's already
> > + * scheduled (scheduling the same coroutine twice is forbidden).
> > + *
> > + * It is true if the coroutine is active and processing requests.
> > + * Additional requests may then be pushed onto mon->qmp_requests,
> > + * and @qmp_dispatcher_co_shutdown may be set without further ado.
> > + * @qmp_dispatcher_co_busy must not be woken up in this case.
> > + *
> > + * If false, you also have to set @qmp_dispatcher_co_busy to true and
> > + * wake up @qmp_dispatcher_co after pushing the new requests.
> > + *
> > + * The coroutine will automatically change this variable back to false
> > + * before it yields. Nobody else may set the variable to false.
> > + *
> > + * Access must be atomic for thread safety.
> > + */
> > +bool qmp_dispatcher_co_busy;
> >
> > /*
> > * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> > @@ -623,9 +647,24 @@ void monitor_cleanup(void)
> > }
> > qemu_mutex_unlock(_lock);
> >
> > -/* QEMUBHs needs to be deleted before destroying the I/O thread */
> > -qemu_bh_delete(qmp_dispatcher_bh);
> > -qmp_dispatcher_bh = NULL;
> > +/*
> > + * The dispatcher needs to stop before destroying the I/O thread.
> > + *
> > + * We need to poll both qemu_aio_context and iohandler_ctx to make
> > + * sure that the dispatcher coroutine keeps making progress and
> > + * eventually terminates. qemu_aio_context is automatically
> > + * polled by calling AIO_WAIT_WHILE on it, but we must poll
> > + * iohandler_ctx manually.
> > + */
> > +qmp_dispatcher_co_shutdown = true;
> > +if
Re: [PATCH v6 11/15] iotests: add 298 to test new preallocate filter driver
25.09.2020 18:11, Vladimir Sementsov-Ogievskiy wrote: 25.09.2020 12:11, Max Reitz wrote: On 25.09.20 10:49, Vladimir Sementsov-Ogievskiy wrote: 25.09.2020 11:26, Max Reitz wrote: On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote: Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/298 | 186 + tests/qemu-iotests/298.out | 5 + tests/qemu-iotests/group | 1 + 3 files changed, 192 insertions(+) create mode 100644 tests/qemu-iotests/298 create mode 100644 tests/qemu-iotests/298.out [...] +class TestTruncate(iotests.QMPTestCase): The same decorator could be placed here, although this class doesn’t start a VM, and so is unaffected by the allowlist. Still may be relevant in case of block modules, I don’t know. Or just global test skip at file top Hm. Like verify_quorum()? Is there a generic function for that already? [...] + # Probably we'll want preallocate filter to keep align to cluster when + # shrink preallocation, so, ignore small differece + self.assertLess(abs(stat.st_size - refstat.st_size), 64 * 1024) + + # Preallocate filter may leak some internal clusters (for example, if + # guest write far over EOF, skipping some clusters - they will remain + # fallocated, preallocate filter don't care about such leaks, it drops + # only trailing preallocation. True, but that isn’t what’s happening here. (We only write 10M at 0, so there are no gaps.) Why do we need this 1M margin? We write 10M, but qcow2 also writes metadata as it wants Ah, yes, sure. Shouldn’t result in 1M, but why not. + self.assertLess(abs(stat.st_blocks - refstat.st_blocks) * 512, + 1024 * 1024) [...] diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index ff59cfd2d4..15d5f9619b 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -307,6 +307,7 @@ 295 rw 296 rw 297 meta +298 auto quick I wouldn’t mark it as quick, there is at least one preallocate=full of 140M, and one of 40M, plus multiple 10M data writes and falloc preallocations. Also, since you mark it as “auto”, have you run this test on all CI-relevant hosts? (Among other things I can’t predict) I wonder how preallocation behaves on macOS. Just because that one was always a bit weird about not-really-data areas. Ofcourse, I didn't run on all hosts. I'm a bit out of sync about this.. Sorry, I see now that it sounds rude. Well, someone has to do it. The background story is that tests are added to auto all the time (because “why not”), and then they fail on BSD or macOS. We have BSD docker test build targets at least, so they can be easily tested. (Well, it takes like half an hour, but you know.) (We don’t have macOS builds, as far as I can tell, but I personally don’t even know why we run the iotests on macOS at all. (Well, I also wonder about the BSDs, but given the test build targets, I shouldn’t complain, I suppose.)) (Though macOS isn’t part of the gating CI, is it? I seem to remember macOS errors are generally only reported to me half a week after the pull request is merged, which is even worse.) Anyway. I just ran the test for OpenBSD (EXTRA_CONFIGURE_OPTS='--target-list=x86_64-softmmu' \ make vm-build-openbsd) Oh, I didn't know that it's so simple. What another things you are running before sending a PULL? and got some failures: --- /home/qemu/qemu-test.PGo2ls/src/tests/qemu-iotests/298.out Fri Sep 25 07:10:31 2020 +++ /home/qemu/qemu-test.PGo2ls/build/tests/qemu-iotests/298.out.bad Fri Sep 25 08:57:56 2020 @@ -1,5 +1,67 @@ -. +qemu-io: Failed to resize underlying file: Unsupported preallocation mode: falloc [..] -OK +FAILED (failures=6) If I don't put new test in "auto", is there any chance that test would be automatically run somewhere? I run all tests before pull requests at least. OK, so it doesn't work on BSD at all. I don't really want to investigate, but seems it's because of absence of fallocate. So let's drop "auto" group. Another thing: maybe, add auto-linux test group for running only in linux-build? So, new tests will be added to it because why not, and we will not bother with BSD and MacOS? -- Best regards, Vladimir
Re: [PATCH v7 08/13] qapi: Add a 'coroutine' flag for commands
Am 14.09.2020 um 17:15 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > This patch adds a new 'coroutine' flag to QMP command definitions that > > tells the QMP dispatcher that the command handler is safe to be run in a > > coroutine. > > > > The documentation of the new flag pretends that this flag is already > > used as intended, which it isn't yet after this patch. We'll implement > > this in another patch in this series. > > > > Signed-off-by: Kevin Wolf > > Reviewed-by: Marc-André Lureau > > Reviewed-by: Markus Armbruster > > --- > > docs/devel/qapi-code-gen.txt| 12 > > include/qapi/qmp/dispatch.h | 1 + > > tests/test-qmp-cmds.c | 4 > > scripts/qapi/commands.py| 10 +++--- > > scripts/qapi/doc.py | 2 +- > > scripts/qapi/expr.py| 10 -- > > scripts/qapi/introspect.py | 2 +- > > scripts/qapi/schema.py | 12 > > tests/qapi-schema/test-qapi.py | 7 --- > > tests/qapi-schema/meson.build | 1 + > > tests/qapi-schema/oob-coroutine.err | 2 ++ > > tests/qapi-schema/oob-coroutine.json| 2 ++ > > tests/qapi-schema/oob-coroutine.out | 0 > > tests/qapi-schema/qapi-schema-test.json | 1 + > > tests/qapi-schema/qapi-schema-test.out | 2 ++ > > 15 files changed, 54 insertions(+), 14 deletions(-) > > create mode 100644 tests/qapi-schema/oob-coroutine.err > > create mode 100644 tests/qapi-schema/oob-coroutine.json > > create mode 100644 tests/qapi-schema/oob-coroutine.out > > > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > > index f3e7ced212..36daa9b5f8 100644 > > --- a/docs/devel/qapi-code-gen.txt > > +++ b/docs/devel/qapi-code-gen.txt > > @@ -472,6 +472,7 @@ Syntax: > > '*gen': false, > > '*allow-oob': true, > > '*allow-preconfig': true, > > +'*coroutine': true, > > '*if': COND, > > '*features': FEATURES } > > > > @@ -596,6 +597,17 @@ before the machine is built. It defaults to false. > > For example: > > QMP is available before the machine is built only when QEMU was > > started with --preconfig. > > > > +Member 'coroutine' tells the QMP dispatcher whether the command handler > > +is safe to be run in a coroutine. > > We need to document what exactly makes a command handler coroutine safe > / unsafe. We discussed this at some length in review of PATCH v4 1/4: > > Message-ID: <874kwnvgad@dusky.pond.sub.org> > https://lists.nongnu.org/archive/html/qemu-devel/2020-01/msg05015.html > > I'd be willing to accept a follow-up patch, if that's more convenient > for you. Did we ever arrive at a conclusion for a good definition? I mean I can give a definition like "it's coroutine safe if it results in the right behaviour even when called in coroutine context", but that's not really helpful. FWIW, I would also have a hard time giving a much better definition than this for thread safety. > > It defaults to false. If it is true, > > +the command handler is called from coroutine context and may yield while > > Is it *always* called from coroutine context, or could it be called > outside coroutine context, too? I guess the former, thanks to PATCH 10, > and as long as we diligently mark HMP commands that such call QMP > handlers, like you do in PATCH 13. Yes, it must *always* be called from coroutine context. This is the reason why I included HMP after all, even though I'm really mostly just interested in QMP. > What's the worst than can happen when we neglect to mark such an HMP > command? When the command handler tries to yield and it's not in coroutine context, it will abort(). If it never tries to yield, I think it would just work - but of course, the ability to yield is the only reason why you would want to run a command handler in a coroutine. Kevin
Re: [PATCH v7 07/13] monitor: Make current monitor a per-coroutine property
Am 14.09.2020 um 17:11 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > This way, a monitor command handler will still be able to access the > > current monitor, but when it yields, all other code code will correctly > > get NULL from monitor_cur(). > > > > This uses a hash table to map the coroutine pointer to the current > > monitor of that coroutine. Outside of coroutine context, we associate > > the current monitor with the leader coroutine of the current thread. > > In qemu-system-FOO, the hash table can have only these entries: > > * (OOB) One mapping @mon_iothread's thread leader to a QMP monitor, while > executing a QMP command out-of-band. > > * (QMP-CO) One mapping @qmp_dispatcher_co (a coroutine in the main > thread) to a QMP monitor, while executing a QMP command in-band and in > coroutine context. > > * (QMP) One mapping the main thread's leader to a QMP monitor, while > executing a QMP command in-band and out of coroutine context, in a > bottom half. > > * (HMP) One mapping the main thread's leader to an HMP monitor, while > executing an HMP command out of coroutine context. > > * (HMP-CO) One mapping a transient coroutine in the main thread to an > HMP monitor, while executing an HMP command in coroutine context. > > In-band execution is one command after the other. > > Therefore, at most one monitor command can be executing in-band at any > time. > > Therefore, the hash table has at most *two* entries: one (OOB), and one > of the other four. > > Can you shoot any holes into my argument? I think with human-monitor-command, you can have three mappings: 1. The main thread's leader (it is a non-coroutine QMP command) to the QMP monitor 2. With a coroutine HMP command, one mapping from the transient HMP coroutine to the transient HMP monitor (with a non-coroutine HMP command, we'd instead temporarily change the mapping from 1.) 3. The OOB entry > I suspect there's a simpler solution struggling to get out. But this > solution works, so in it goes. Should the simpler one succeed at > getting out, it can go in on top. If not, I'll probably add even more > comments to remind myself of these facts. I think the simple approach you had posted could work if you can fill in the HMP part, but I think it wasn't completely trivial and you told me not to bother for now, so I didn't. It may still be a viable path forward if you like it better. Kevin
Re: [PATCH v2 0/6] block/nvme: Map doorbells pages write-only, remove magic from nvme_init
On Tue, Sep 22, 2020 at 10:38:15AM +0200, Philippe Mathieu-Daudé wrote: > Instead of mapping 8K of I/O + doorbells as RW during the whole > execution, maps I/O temporarly at init, and map doorbells WO. > > Replace various magic values by slighly more explicit macros from > "block/nvme.h". > > Since v1: Fix uninitialized regs* (patchew) > > Philippe Mathieu-Daudé (6): > util/vfio-helpers: Pass page protections to qemu_vfio_pci_map_bar() > block/nvme: Map doorbells pages write-only > block/nvme: Reduce I/O registers scope > block/nvme: Drop NVMeRegs structure, directly use NvmeBar > block/nvme: Use register definitions from 'block/nvme.h' > block/nvme: Replace magic value by SCALE_MS definition > > include/qemu/vfio-helpers.h | 2 +- > block/nvme.c| 73 + > util/vfio-helpers.c | 4 +- > 3 files changed, 44 insertions(+), 35 deletions(-) > > -- > 2.26.2 > Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan signature.asc Description: PGP signature
Re: [PATCH v2 3/6] block/nvme: Reduce I/O registers scope
On Tue, Sep 22, 2020 at 10:38:18AM +0200, Philippe Mathieu-Daudé wrote:
> @@ -808,6 +808,10 @@ static int nvme_init(BlockDriverState *bs, const char
> *device, int namespace,
> ret = -EIO;
> }
> out:
> +if (regs) {
> +qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)regs, 0,
> sizeof(NvmeBar));
> +}
qemu_vfio_pci_unmap_bar(NULL) is a nop, so the check is unnecessary. I
didn't look to see whether the doorbells can be NULL too during unmap,
but if yes, then it's clearer to be consistent (always check NULL or
never check NULL).
Not worth respinning though.
signature.asc
Description: PGP signature
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > The correct way to set the current monitor for a coroutine handler will
> > be different than for a blocking handler, so monitor_set_cur() needs to
> > be called in qmp_dispatch().
> >
> > Signed-off-by: Kevin Wolf
> > ---
> > include/qapi/qmp/dispatch.h | 3 ++-
> > monitor/qmp.c | 8 +---
> > qapi/qmp-dispatch.c | 8 +++-
> > qga/main.c | 2 +-
> > stubs/monitor-core.c| 5 +
> > tests/test-qmp-cmds.c | 6 +++---
> > 6 files changed, 19 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index 5a9cf82472..0c2f467028 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -14,6 +14,7 @@
> > #ifndef QAPI_QMP_DISPATCH_H
> > #define QAPI_QMP_DISPATCH_H
> >
> > +#include "monitor/monitor.h"
> > #include "qemu/queue.h"
> >
> > typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
> > @@ -49,7 +50,7 @@ const char *qmp_command_name(const QmpCommand *cmd);
> > bool qmp_has_success_response(const QmpCommand *cmd);
> > QDict *qmp_error_response(Error *err);
> > QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> > -bool allow_oob);
> > +bool allow_oob, Monitor *cur_mon);
> > bool qmp_is_oob(const QDict *dict);
> >
> > typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque);
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index 8469970c69..922fdb5541 100644
> > --- a/monitor/qmp.c
> > +++ b/monitor/qmp.c
> > @@ -135,16 +135,10 @@ static void monitor_qmp_respond(MonitorQMP *mon,
> > QDict *rsp)
> >
> > static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
> > {
> > -Monitor *old_mon;
> > QDict *rsp;
> > QDict *error;
> >
> > -old_mon = monitor_set_cur(>common);
> > -assert(old_mon == NULL);
> > -
> > -rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
> > -
> > -monitor_set_cur(NULL);
> > +rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon),
> > >common);
>
> Long line. Happy to wrap it in my tree. A few more in PATCH 08-11.
It's 79 characters. Should be fine even with your local deviation from
the coding style to require less than that for comments?
Kevin
Re: [PATCH v6 11/15] iotests: add 298 to test new preallocate filter driver
25.09.2020 12:11, Max Reitz wrote: On 25.09.20 10:49, Vladimir Sementsov-Ogievskiy wrote: 25.09.2020 11:26, Max Reitz wrote: On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote: Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/298 | 186 + tests/qemu-iotests/298.out | 5 + tests/qemu-iotests/group | 1 + 3 files changed, 192 insertions(+) create mode 100644 tests/qemu-iotests/298 create mode 100644 tests/qemu-iotests/298.out [...] +class TestTruncate(iotests.QMPTestCase): The same decorator could be placed here, although this class doesn’t start a VM, and so is unaffected by the allowlist. Still may be relevant in case of block modules, I don’t know. Or just global test skip at file top Hm. Like verify_quorum()? Is there a generic function for that already? [...] + # Probably we'll want preallocate filter to keep align to cluster when + # shrink preallocation, so, ignore small differece + self.assertLess(abs(stat.st_size - refstat.st_size), 64 * 1024) + + # Preallocate filter may leak some internal clusters (for example, if + # guest write far over EOF, skipping some clusters - they will remain + # fallocated, preallocate filter don't care about such leaks, it drops + # only trailing preallocation. True, but that isn’t what’s happening here. (We only write 10M at 0, so there are no gaps.) Why do we need this 1M margin? We write 10M, but qcow2 also writes metadata as it wants Ah, yes, sure. Shouldn’t result in 1M, but why not. + self.assertLess(abs(stat.st_blocks - refstat.st_blocks) * 512, + 1024 * 1024) [...] diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index ff59cfd2d4..15d5f9619b 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -307,6 +307,7 @@ 295 rw 296 rw 297 meta +298 auto quick I wouldn’t mark it as quick, there is at least one preallocate=full of 140M, and one of 40M, plus multiple 10M data writes and falloc preallocations. Also, since you mark it as “auto”, have you run this test on all CI-relevant hosts? (Among other things I can’t predict) I wonder how preallocation behaves on macOS. Just because that one was always a bit weird about not-really-data areas. Ofcourse, I didn't run on all hosts. I'm a bit out of sync about this.. Well, someone has to do it. The background story is that tests are added to auto all the time (because “why not”), and then they fail on BSD or macOS. We have BSD docker test build targets at least, so they can be easily tested. (Well, it takes like half an hour, but you know.) (We don’t have macOS builds, as far as I can tell, but I personally don’t even know why we run the iotests on macOS at all. (Well, I also wonder about the BSDs, but given the test build targets, I shouldn’t complain, I suppose.)) (Though macOS isn’t part of the gating CI, is it? I seem to remember macOS errors are generally only reported to me half a week after the pull request is merged, which is even worse.) Anyway. I just ran the test for OpenBSD (EXTRA_CONFIGURE_OPTS='--target-list=x86_64-softmmu' \ make vm-build-openbsd) Oh, I didn't know that it's so simple. What another things you are running before sending a PULL? and got some failures: --- /home/qemu/qemu-test.PGo2ls/src/tests/qemu-iotests/298.out Fri Sep 25 07:10:31 2020 +++ /home/qemu/qemu-test.PGo2ls/build/tests/qemu-iotests/298.out.bad Fri Sep 25 08:57:56 2020 @@ -1,5 +1,67 @@ -. +qemu-io: Failed to resize underlying file: Unsupported preallocation mode: falloc +qemu-io: Failed to resize underlying file: Unsupported preallocation mode: falloc +.F...F... +== +FAIL: test_external_snapshot (__main__.TestPreallocateFilter) -- +Traceback (most recent call last): + File "298", line 81, in test_external_snapshot +self.test_prealloc() + File "298", line 78, in test_prealloc +self.check_big() + File "298", line 48, in check_big +self.assertTrue(os.path.getsize(disk) > 100 * MiB) +AssertionError: False is not true + +== +FAIL: test_prealloc (__main__.TestPreallocateFilter) +-- +Traceback (most recent call last): + File "298", line 78, in test_prealloc +self.check_big() + File "298", line 48, in check_big +self.assertTrue(os.path.getsize(disk) > 100 * MiB) +AssertionError: False is not true + +== +FAIL: test_reopen_opts (__main__.TestPreallocateFilter) +-- +Traceback (most recent call last): + File "298", line 119, in
Re: [PATCH 3/4] block: move block exports to libblockdev
On Fri, Sep 25, 2020 at 04:11:54PM +0200, Paolo Bonzini wrote: > On 25/09/20 15:42, 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. > > Two stubs are required to support this: > > 1. bdrv_close_all() (libblock) calls blk_exp_close_all() (libblockdev). > > 2. qemu_system_killed() is called by os-posix.c (libblockdev) and not > >implemented in qemu-nbd. > > > > Signed-off-by: Stefan Hajnoczi > > Another possibility is to call os_setup_signal_handling in qemu-nbd.c > where we currently set the SIGTERM handler. The existing > termsig_handler can be repurposed as qemu_system_killed. That is nicer. Will fix in v2. Stefan signature.asc Description: PGP signature
Re: [PATCH 4/4] block/export: add BlockExportOptions->iothread member
Am 25.09.2020 um 15:42 hat Stefan Hajnoczi geschrieben: > Make it possible to specify the iothread where the export will run. > > Signed-off-by: Stefan Hajnoczi > --- > 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 | 4 > block/export/export.c | 26 +- > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/qapi/block-export.json b/qapi/block-export.json > index 87ac5117cd..eba6f6eae9 100644 > --- a/qapi/block-export.json > +++ b/qapi/block-export.json > @@ -219,11 +219,15 @@ > #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 the main loop thread. (since: 5.2) NBD exports currently switch automatically to a different AioContext if another user (usually a guest device using the same node) tries to change the AioContext. I believe this is also the most useful mode in the context of the system emulator. I can see the need for an iothread option in qemu-storage-daemon where usually nobody else will move the node into a different AioContext. But we need to define the semantics more precisely and specify what happens if another user wants to change the AioContext later. Currently, the NBD export will allow this and just switch the AioContext - after this patch, ignoring what the user set explicitly with this new option. I see two options to handle this more consistently: 1. If @iothread is set, move the block node into the requested AioContext, and if that fails, block-export-add fails. Other users of the node will be denied to change the AioContext while the export is active. If @iothread is not given, it behaves like today: Use whatever AioContext the node is currently in and switch whenever another user requests it. 2. Add a bool option @fixed-iothread that determines whether other users can change the AioContext while the export is active. Giving an @iothread and fixed-iothread == true means that we'll enforce the given AioContext during the whole lifetime of the export. With fixed-iothread == false it means that we try to move the block node to the requested iothread if possible (but we won't fail if it isn't possible) and will follow any other user switching the AioContext of the node. Not giving @iothread means that we start with the current AioContext of the node, and @fixed-iothread then means the same as before. Does this make sense to you? Kevin
Re: [PATCH 3/4] block: move block exports to libblockdev
On 25/09/20 15:42, 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.
> Two stubs are required to support this:
> 1. bdrv_close_all() (libblock) calls blk_exp_close_all() (libblockdev).
> 2. qemu_system_killed() is called by os-posix.c (libblockdev) and not
>implemented in qemu-nbd.
>
> Signed-off-by: Stefan Hajnoczi
Another possibility is to call os_setup_signal_handling in qemu-nbd.c
where we currently set the SIGTERM handler. The existing
termsig_handler can be repurposed as qemu_system_killed.
Paolo
> ---
> stubs/blk-exp-close-all.c | 7 +++
> stubs/qemu-system-killed.c | 10 ++
> block/export/meson.build | 4 ++--
> meson.build| 4 ++--
> nbd/meson.build| 2 ++
> stubs/meson.build | 2 ++
> 6 files changed, 25 insertions(+), 4 deletions(-)
> create mode 100644 stubs/blk-exp-close-all.c
> create mode 100644 stubs/qemu-system-killed.c
>
> diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c
> new file mode 100644
> index 00..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/stubs/qemu-system-killed.c b/stubs/qemu-system-killed.c
> new file mode 100644
> index 00..9af131917b
> --- /dev/null
> +++ b/stubs/qemu-system-killed.c
> @@ -0,0 +1,10 @@
> +#include "qemu/osdep.h"
> +#include "sysemu/runstate.h"
> +
> +/*
> + * This function is needed by os-posix.c but only implemented by softmmu and
> + * qemu-storage-daemon. Other programs may have no need for it.
> + */
> +void qemu_system_killed(int signal, pid_t pid)
> +{
> +}
> 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..60234571b1 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'))
> @@ -25,6 +26,7 @@ stub_ss.add(files('monitor.c'))
> stub_ss.add(files('monitor-core.c'))
> stub_ss.add(files('pci-bus.c'))
> stub_ss.add(files('pci-host-piix.c'))
> +stub_ss.add(files('qemu-system-killed.c'))
> stub_ss.add(files('qemu-timer-notify-cb.c'))
> stub_ss.add(files('qmp_memory_device.c'))
> stub_ss.add(files('qtest.c'))
>
Re: [PATCH 2/4] qemu-storage-daemon: avoid compiling blockdev_ss twice
On 25/09/20 15:42, Stefan Hajnoczi wrote:
> Introduce libblkdev.fa to avoid recompiling blockdev_ss twice.
>
> Suggested-by: Paolo Bonzini
> Signed-off-by: Stefan Hajnoczi
> ---
> 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')
>
>
Reviewed-by: Paolo Bonzini
Re: [PATCH 1/4] util/vhost-user-server: use static library in meson.build
On 25/09/20 15:42, Stefan Hajnoczi wrote:
> 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
Looks good. Unrelated to this patch, probably the contrib/libvhost-user
directory should be moved under tools.
Paolo
> ---
> 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[] = {
> _exp_nbd,
> -#if CONFIG_LINUX
> +#ifdef CONFIG_VHOST_USER
> _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'))
>
[PATCH 2/4] qemu-storage-daemon: avoid compiling blockdev_ss twice
Introduce libblkdev.fa to avoid recompiling blockdev_ss twice.
Suggested-by: Paolo Bonzini
Signed-off-by: Stefan Hajnoczi
---
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 3/4] block: move block exports to libblockdev
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.
Two stubs are required to support this:
1. bdrv_close_all() (libblock) calls blk_exp_close_all() (libblockdev).
2. qemu_system_killed() is called by os-posix.c (libblockdev) and not
implemented in qemu-nbd.
Signed-off-by: Stefan Hajnoczi
---
stubs/blk-exp-close-all.c | 7 +++
stubs/qemu-system-killed.c | 10 ++
block/export/meson.build | 4 ++--
meson.build| 4 ++--
nbd/meson.build| 2 ++
stubs/meson.build | 2 ++
6 files changed, 25 insertions(+), 4 deletions(-)
create mode 100644 stubs/blk-exp-close-all.c
create mode 100644 stubs/qemu-system-killed.c
diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c
new file mode 100644
index 00..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/stubs/qemu-system-killed.c b/stubs/qemu-system-killed.c
new file mode 100644
index 00..9af131917b
--- /dev/null
+++ b/stubs/qemu-system-killed.c
@@ -0,0 +1,10 @@
+#include "qemu/osdep.h"
+#include "sysemu/runstate.h"
+
+/*
+ * This function is needed by os-posix.c but only implemented by softmmu and
+ * qemu-storage-daemon. Other programs may have no need for it.
+ */
+void qemu_system_killed(int signal, pid_t pid)
+{
+}
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..60234571b1 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'))
@@ -25,6 +26,7 @@ stub_ss.add(files('monitor.c'))
stub_ss.add(files('monitor-core.c'))
stub_ss.add(files('pci-bus.c'))
stub_ss.add(files('pci-host-piix.c'))
+stub_ss.add(files('qemu-system-killed.c'))
stub_ss.add(files('qemu-timer-notify-cb.c'))
stub_ss.add(files('qmp_memory_device.c'))
stub_ss.add(files('qtest.c'))
--
2.26.2
[PATCH 4/4] block/export: add BlockExportOptions->iothread member
Make it possible to specify the iothread where the export will run.
Signed-off-by: Stefan Hajnoczi
---
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 | 4
block/export/export.c | 26 +-
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 87ac5117cd..eba6f6eae9 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -219,11 +219,15 @@
#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 the main loop thread. (since: 5.2)
+#
# Since: 4.2
##
{ 'union': 'BlockExportOptions',
'base': { 'type': 'BlockExportType',
'id': 'str',
+ '*iothread': 'str',
'node-name': 'str',
'*writable': 'bool',
'*writethrough': 'bool' },
diff --git a/block/export/export.c b/block/export/export.c
index 550897e236..0fb3d76ee3 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"
@@ -66,7 +67,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
const BlockExportDriver *drv;
BlockExport *exp = NULL;
BlockDriverState *bs;
-BlockBackend *blk;
+BlockBackend *blk = NULL;
AioContext *ctx;
uint64_t perm;
int ret;
@@ -102,6 +103,29 @@ 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) {
+goto fail;
+}
+
+aio_context_release(ctx);
+aio_context_acquire(new_ctx);
+
+ctx = new_ctx;
+}
+
/*
* Block exports are used for non-shared storage migration. Make sure
* that BDRV_O_INACTIVE is cleared and the image is ready for write
--
2.26.2
[PATCH 0/4] block/export: add BlockExportOptions->iothread member
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-stefa...@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 BlockExportOptions->iothread member
qapi/block-export.json| 4
block/export/export.c | 34 ++-
stubs/blk-exp-close-all.c | 7 +++
stubs/qemu-system-killed.c| 10 +
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 | 2 ++
tests/qtest/meson.build | 2 +-
util/meson.build | 4 +++-
12 files changed, 79 insertions(+), 16 deletions(-)
create mode 100644 stubs/blk-exp-close-all.c
create mode 100644 stubs/qemu-system-killed.c
--
2.26.2
[PATCH 1/4] util/vhost-user-server: use static library in meson.build
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
---
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[] = {
_exp_nbd,
-#if CONFIG_LINUX
+#ifdef CONFIG_VHOST_USER
_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
Re: [PATCH v2 04/31] block/export: Add BlockExport infrastructure and block-export-add
On 9/24/20 10:26 AM, Kevin Wolf wrote: We want to have a common set of commands for all types of block exports. Currently, this is only NBD, but we're going to add more types. This patch adds the basic BlockExport and BlockExportDriver structs and a QMP command block-export-add that creates a new export based on the given BlockExportOptions. qmp_nbd_server_add() becomes a wrapper around qmp_block_export_add(). Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 31/31] iotests: Test block-export-* QMP interface
On 24.09.20 17:27, Kevin Wolf wrote: > Signed-off-by: Kevin Wolf > --- > tests/qemu-iotests/307 | 132 + > tests/qemu-iotests/307.out | 124 ++ > tests/qemu-iotests/group | 1 + > 3 files changed, 257 insertions(+) > create mode 100755 tests/qemu-iotests/307 > create mode 100644 tests/qemu-iotests/307.out Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 30/31] iotests: Allow supported and unsupported formats at the same time
On 24.09.20 17:27, Kevin Wolf wrote: > This is useful for specifying 'generic' as supported (which includes > only writable image formats), but still excluding some incompatible > writable formats. > > It also removes more lines than it adds. > > Signed-off-by: Kevin Wolf > --- > tests/qemu-iotests/iotests.py | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH 0.5/7] include/block/block.h: drop non-ascii quotation mark
On 9/25/20 3:32 AM, Vladimir Sementsov-Ogievskiy wrote:
This is the only non-ascii character in the file and it doesn't really
needed here. Let's use normal "'" symbol for consistency with the rest
11 occurrences of "'" in the file.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/block.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Eric Blake
diff --git a/include/block/block.h b/include/block/block.h
index 8b87df69a1..ce2ac39299 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -304,7 +304,7 @@ enum BdrvChildRoleBits {
BDRV_CHILD_FILTERED = (1 << 2),
/*
- * Child from which to read all data that isn’t allocated in the
+ * Child from which to read all data that isn't allocated in the
* parent (i.e., the backing child); such data is copied to the
* parent through COW (and optionally COR).
* This field is mutually exclusive with DATA, METADATA, and
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 29/31] iotests: Introduce qemu_nbd_list_log()
On 24.09.20 17:27, Kevin Wolf wrote: > Add a function to list the NBD exports offered by an NBD server. > > Signed-off-by: Kevin Wolf > --- > tests/qemu-iotests/iotests.py | 13 - > 1 file changed, 12 insertions(+), 1 deletion(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v9 0/7] coroutines: generate wrapper code
On 9/25/20 3:04 AM, Vladimir Sementsov-Ogievskiy wrote: 24.09.2020 23:32, no-re...@patchew.org wrote: Patchew URL: https://patchew.org/QEMU/20200924185414.28642-1-vsement...@virtuozzo.com/ Program python3 found: YES (/usr/bin/python3) Configuring ninjatool using configuration --- return codecs.ascii_decode(input, self.errors)[0] UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128) Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp.c.inc'. Traceback (most recent call last): File "/tmp/qemu-test/src/block/../scripts/block-coroutine-wrapper.py", line 187, in f_out.write(gen_wrappers(f_in.read())) File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode return codecs.ascii_decode(input, self.errors)[0] UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128) Interesting: [root@kvm up-coroutine-wrapper]# grep --color='auto' -P -n '[^\x00-\x7F]' include/block/block.h 307: * Child from which to read all data that isn’t allocated in the ^ The file really contains one non-ascii symbol. I think it worth a separate patch. Still, it shouldn't break build process. On my system it works as is, probably unicode is default for me. Python 3 has had an interesting history when it comes to 8-bit cleanness by default. Which means we DO have to be explicit about utf8. Aha, from "open" specification: if encoding is not specified the encoding used is platform dependent: locale.getpreferredencoding(False) is called to get the current locale encoding. Is it ok, that utf-8 is not default on test system? It's intentional. So, possible solutions are: 1. Enforce utf-8 io in scripts/block-coroutine-wrapper.py (patch 4) Yes, we should do that regardless (we do it in our other python scripts). 2. Drop non-ascii quotation mark from block.h Yes, we should do that as well (it's only in a comment, but it is inconsistent). 3. Fix the test system default to be utf-8 No. That one we want to keep where it is, because it helps us flush out these sorts of issues. Do we want them all? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[RFC PATCH 04/19] hw/block: add vhost-user-rpmb-pci boilerplate
This allows is to instantiate a vhost-user-rpmb device as part of a
PCI bus. It is mostly boilerplate which looks pretty similar to the
vhost-user-fs-pci device if you squint.
Signed-off-by: Alex Bennée
---
- enable use IOEVENTFD flag
- swap obj set bool args
---
hw/block/vhost-user-rpmb-pci.c | 82 ++
hw/block/meson.build | 2 +
2 files changed, 84 insertions(+)
create mode 100644 hw/block/vhost-user-rpmb-pci.c
diff --git a/hw/block/vhost-user-rpmb-pci.c b/hw/block/vhost-user-rpmb-pci.c
new file mode 100644
index ..f0518305a1d9
--- /dev/null
+++ b/hw/block/vhost-user-rpmb-pci.c
@@ -0,0 +1,82 @@
+/*
+ * Vhost-user RPMB virtio device PCI glue
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/vhost-user-rpmb.h"
+#include "hw/virtio/virtio-pci.h"
+
+struct VHostUserRPMBPCI {
+VirtIOPCIProxy parent_obj;
+VHostUserRPMB vdev;
+};
+
+typedef struct VHostUserRPMBPCI VHostUserRPMBPCI;
+
+#define TYPE_VHOST_USER_RPMB_PCI "vhost-user-rpmb-pci-base"
+
+#define VHOST_USER_RPMB_PCI(obj) \
+OBJECT_CHECK(VHostUserRPMBPCI, (obj), TYPE_VHOST_USER_RPMB_PCI)
+
+static Property vurpmb_pci_properties[] = {
+DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
+VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
+DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
+ DEV_NVECTORS_UNSPECIFIED),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vurpmb_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VHostUserRPMBPCI *dev = VHOST_USER_RPMB_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(>vdev);
+
+if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
+vpci_dev->nvectors = 1;
+}
+
+qdev_set_parent_bus(vdev, BUS(_dev->bus));
+object_property_set_bool(OBJECT(vdev), "realized", true, errp);
+}
+
+static void vurpmb_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+k->realize = vurpmb_pci_realize;
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+device_class_set_props(dc, vurpmb_pci_properties);
+pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+pcidev_k->device_id = 0; /* Set by virtio-pci based on virtio id */
+pcidev_k->revision = 0x00;
+pcidev_k->class_id = PCI_CLASS_STORAGE_OTHER;
+}
+
+static void vurpmb_pci_instance_init(Object *obj)
+{
+VHostUserRPMBPCI *dev = VHOST_USER_RPMB_PCI(obj);
+
+virtio_instance_init_common(obj, >vdev, sizeof(dev->vdev),
+TYPE_VHOST_USER_RPMB);
+}
+
+static const VirtioPCIDeviceTypeInfo vurpmb_pci_info = {
+.base_name = TYPE_VHOST_USER_RPMB_PCI,
+.non_transitional_name = "vhost-user-rpmb-pci",
+.instance_size = sizeof(VHostUserRPMBPCI),
+.instance_init = vurpmb_pci_instance_init,
+.class_init= vurpmb_pci_class_init,
+};
+
+static void vurpmb_pci_register(void)
+{
+virtio_pci_types_register(_pci_info);
+}
+
+type_init(vurpmb_pci_register);
diff --git a/hw/block/meson.build b/hw/block/meson.build
index 114222f18424..0b2d10201e28 100644
--- a/hw/block/meson.build
+++ b/hw/block/meson.build
@@ -18,5 +18,7 @@ softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true:
files('nvme.c'))
specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true:
files('vhost-user-blk.c'))
specific_ss.add(when: 'CONFIG_VHOST_USER_RPMB', if_true:
files('vhost-user-rpmb.c'))
+specific_ss.add(when: ['CONFIG_VHOST_USER_RPMB', 'CONFIG_VIRTIO_PCI' ],
+if_true: files('vhost-user-rpmb-pci.c'))
subdir('dataplane')
--
2.20.1
[RFC PATCH 02/19] hw/block: add boilerplate for vhost-user-rpmb device
This creates the QEMU side of the vhost-user-rpmb device which
connects to the remote daemon. It is based of the reasonably modern
vhost-user-fs code with bits from vhost-user-blk as we want the
virtio-config itself to be sourced from the daemon.
Signed-off-by: Alex Bennée
---
include/hw/virtio/vhost-user-rpmb.h | 46
hw/block/vhost-user-rpmb.c | 333
hw/block/Kconfig| 5 +
hw/block/meson.build| 1 +
4 files changed, 385 insertions(+)
create mode 100644 include/hw/virtio/vhost-user-rpmb.h
create mode 100644 hw/block/vhost-user-rpmb.c
diff --git a/include/hw/virtio/vhost-user-rpmb.h
b/include/hw/virtio/vhost-user-rpmb.h
new file mode 100644
index ..7e5988127dc2
--- /dev/null
+++ b/include/hw/virtio/vhost-user-rpmb.h
@@ -0,0 +1,46 @@
+/*
+ * vhost-user-rpmb virtio device
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef _VHOST_USER_RPMB_H_
+#define _VHOST_USER_RPMB_H_
+
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-user.h"
+#include "chardev/char-fe.h"
+
+#define TYPE_VHOST_USER_RPMB "vhost-user-rpmb-device"
+#define VHOST_USER_RPMB(obj) \
+OBJECT_CHECK(VHostUserRPMB, (obj), TYPE_VHOST_USER_RPMB)
+
+/* This is defined in the VIRTIO spec */
+struct virtio_rpmb_config {
+uint8_t capacity;
+uint8_t max_wr_cnt;
+uint8_t max_rd_cnt;
+};
+
+typedef struct {
+CharBackend chardev;
+struct virtio_rpmb_config config;
+} VHostUserRPMBConf;
+
+typedef struct {
+/*< private >*/
+VirtIODevice parent;
+VHostUserRPMBConf conf;
+struct vhost_virtqueue *vhost_vq;
+struct vhost_dev vhost_dev;
+VhostUserState vhost_user;
+VirtQueue *req_vq;
+bool connected;
+/*< public >*/
+} VHostUserRPMB;
+
+
+#endif /* _VHOST_USER_RPMB_H_ */
diff --git a/hw/block/vhost-user-rpmb.c b/hw/block/vhost-user-rpmb.c
new file mode 100644
index ..de243e7a53a0
--- /dev/null
+++ b/hw/block/vhost-user-rpmb.c
@@ -0,0 +1,333 @@
+/*
+ * Vhost-user RPMB virtio device
+ *
+ * This is the boilerplate for instantiating a vhost-user device
+ * implementing a Replay Protected Memory Block (RPMB) device. This is
+ * a type of flash chip that is protected from replay attacks and used
+ * for tamper resistant storage. The actual back-end for this driver
+ * is the vhost-user-rpmb daemon. The code here just connects up the
+ * device in QEMU and allows it to be instantiated.
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/vhost-user-rpmb.h"
+#include "qemu/error-report.h"
+
+/* currently there is no RPMB driver in Linux */
+#define VIRTIO_ID_RPMB 28 /* virtio RPMB */
+
+static void vurpmb_get_config(VirtIODevice *vdev, uint8_t *config)
+{
+/* this somehow needs to come from the vhost-user daemon */
+}
+
+static void vurpmb_start(VirtIODevice *vdev)
+{
+VHostUserRPMB *rpmb = VHOST_USER_RPMB(vdev);
+BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+int ret;
+int i;
+
+if (!k->set_guest_notifiers) {
+error_report("binding does not support guest notifiers");
+return;
+}
+
+ret = vhost_dev_enable_notifiers(>vhost_dev, vdev);
+if (ret < 0) {
+error_report("Error enabling host notifiers: %d", -ret);
+return;
+}
+
+ret = k->set_guest_notifiers(qbus->parent, rpmb->vhost_dev.nvqs, true);
+if (ret < 0) {
+error_report("Error binding guest notifier: %d", -ret);
+goto err_host_notifiers;
+}
+
+rpmb->vhost_dev.acked_features = vdev->guest_features;
+ret = vhost_dev_start(>vhost_dev, vdev);
+if (ret < 0) {
+error_report("Error starting vhost-user-rpmb: %d", -ret);
+goto err_guest_notifiers;
+}
+
+/*
+ * guest_notifier_mask/pending not used yet, so just unmask
+ * everything here. virtio-pci will do the right thing by
+ * enabling/disabling irqfd.
+ */
+for (i = 0; i < rpmb->vhost_dev.nvqs; i++) {
+vhost_virtqueue_mask(>vhost_dev, vdev, i, false);
+}
+
+return;
+
+err_guest_notifiers:
+k->set_guest_notifiers(qbus->parent, rpmb->vhost_dev.nvqs, false);
+err_host_notifiers:
+vhost_dev_disable_notifiers(>vhost_dev, vdev);
+}
+
+static void vurpmb_stop(VirtIODevice *vdev)
+{
+VHostUserRPMB *rpmb = VHOST_USER_RPMB(vdev);
+BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+int ret;
+
+if (!k->set_guest_notifiers) {
+return;
+}
+
+vhost_dev_stop(>vhost_dev, vdev);
+
+ret = k->set_guest_notifiers(qbus->parent, rpmb->vhost_dev.nvqs, false);
+if (ret < 0) {
+error_report("vhost guest
Re: [PATCH v2 28/31] iotests: Factor out qemu_tool_pipe_and_status()
On 24.09.20 17:27, Kevin Wolf wrote: > We have three almost identical functions that call an external process > and return its output and return code. Refactor them into small wrappers > around a common function. > > Signed-off-by: Kevin Wolf > --- > tests/qemu-iotests/iotests.py | 49 --- > 1 file changed, 23 insertions(+), 26 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 26/31] nbd: Merge nbd_export_new() and nbd_export_create()
On 24.09.20 17:27, Kevin Wolf wrote: > There is no real reason any more why nbd_export_new() and > nbd_export_create() should be separate functions. The latter only > performs a few checks before it calls the former. > > What makes the current state stand out is that it's the only function in > BlockExportDriver that is not a static function inside nbd/server.c, but > a small wrapper in blockdev-nbd.c that then calls back into nbd/server.c > for the real functionality. Thanks :) signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 27/31] nbd: Deprecate nbd-server-add/remove
On 24.09.20 17:27, Kevin Wolf wrote: > These QMP commands are replaced by block-export-add/del. > > Signed-off-by: Kevin Wolf > --- > qapi/block-export.json | 11 +-- > docs/system/deprecated.rst | 6 ++ > 2 files changed, 15 insertions(+), 2 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 21/31] block/export: Add BLOCK_EXPORT_DELETED event
On 24.09.20 17:27, Kevin Wolf wrote:
> Clients may want to know when an export has finally disappeard
> (block-export-del returns earlier than that in the general case), so add
> a QAPI event for it.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/block-export.json | 12
> block/export/export.c | 2 ++
> tests/qemu-iotests/140 | 9 -
> tests/qemu-iotests/140.out | 2 +-
> tests/qemu-iotests/223.out | 4
> 5 files changed, 27 insertions(+), 2 deletions(-)
[...]
> diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
> index 8d2ce5d9e3..309b177e77 100755
> --- a/tests/qemu-iotests/140
> +++ b/tests/qemu-iotests/140
> @@ -81,10 +81,17 @@ $QEMU_IO_PROG -f raw -r -c 'read -P 42 0 64k' \
> "nbd+unix:///drv?socket=$SOCK_DIR/nbd" 2>&1 \
> | _filter_qemu_io | _filter_nbd
>
> +# The order of 'return' and the BLOCK_EXPORT_DELETED event is undefined. Just
> +# wait until we've twice seen one of them. Filter the 'return' line out so
> that
> +# the output is defined.
> _send_qemu_cmd $QEMU_HANDLE \
> "{ 'execute': 'eject',
> 'arguments': { 'device': 'drv' }}" \
> -'return'
> +'return\|BLOCK_EXPORT_DELETED' |
> +grep -v 'return'
> +
> +_send_qemu_cmd $QEMU_HANDLE '' 'return\|BLOCK_EXPORT_DELETED' |
> +grep -v 'return'
Funny. I did basically the same thing (only I filtered the event, not
the return):
https://git.xanclic.moe/XanClic/qemu/commit/e6f7510149a4a26c868013639ec89d28f16857d8#diff-3
and considered it kind of a hack.
Oh well. :)
Reviewed-by: Max Reitz
> $QEMU_IO_PROG -f raw -r -c close \
> "nbd+unix:///drv?socket=$SOCK_DIR/nbd" 2>&1 \
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v6 15/15] scripts/simplebench: add bench_prealloc.py
On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote: > Benchmark for new preallocate filter. > > Example usage: > ./bench_prealloc.py ../../build/qemu-img \ > ssd-ext4:/path/to/mount/point \ > ssd-xfs:/path2 hdd-ext4:/path3 hdd-xfs:/path4 > > The benchmark shows performance improvement (or degradation) when use > new preallocate filter with qcow2 image. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > scripts/simplebench/bench_prealloc.py | 128 ++ > 1 file changed, 128 insertions(+) > create mode 100755 scripts/simplebench/bench_prealloc.py Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v6 14/15] scripts/simplebench: improve ascii table: add difference line
On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
> Performance improvements / degradations are usually discussed in
> percentage. Let's make the script calculate it for us.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> scripts/simplebench/simplebench.py | 46 +++---
> 1 file changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/simplebench/simplebench.py
> b/scripts/simplebench/simplebench.py
> index 56d3a91ea2..0ff05a38b8 100644
> --- a/scripts/simplebench/simplebench.py
> +++ b/scripts/simplebench/simplebench.py
[...]
> +for j in range(0, i):
> +env_j = results['envs'][j]
> +res_j = case_results[env_j['id']]
> +
> +if 'average' not in res_j:
> +# Failed result
> +cell += ' --'
> +continue
> +
> +col_j = chr(ord('A') + j)
> +avg_j = res_j['average']
> +delta = (res['average'] - avg_j) / avg_j * 100
I was wondering why you’d subtract, when percentage differences usually
mean a quotient. Then I realized that this would usually be written as:
(res['average'] / avg_j - 1) * 100
> +delta_delta = (res['delta'] + res_j['delta']) / avg_j * 100
Why not use the new format_percent for both cases?
> +cell += f' {col_j}{round(delta):+}±{round(delta_delta)}%'
I don’t know what I should think about ±delta_delta. If I saw “Compared
to run A, this is +42.1%±2.0%”, I would think that you calculated the
difference between each run result, and then based on that array
calculated average and standard deviation.
Furthermore, I don’t even know what the delta_delta is supposed to tell
you. It isn’t even a delta_delta, it’s an average_delta.
The delta_delta would be (res['delta'] / res_j['delta'] - 1) * 100.0.
And that might be presented perhaps like “+42.1% Δ± +2.0%” (if delta
were the SD, “Δx̅=+42.1% Δσ=+2.0%” would also work; although, again, I do
interpret ± as the SD anyway).
Max
signature.asc
Description: OpenPGP digital signature
Re: [PULL 00/13] Block patches
On Wed, 23 Sep 2020 at 17:10, Stefan Hajnoczi wrote: > > The following changes since commit 0fc0142828b5bc965790a1c5c6e241897d3387cb: > > Merge remote-tracking branch > 'remotes/kraxel/tags/input-20200921-pull-request' into staging (2020-09-22 > 21:11:10 +0100) > > are available in the Git repository at: > > https://github.com/stefanha/qemu.git tags/block-pull-request > > for you to fetch changes up to d73415a315471ac0b127ed3fad45c8ec5d711de1: > > qemu/atomic.h: rename atomic_ to qatomic_ (2020-09-23 16:07:44 +0100) > > > Pull request > > This includes the atomic_ -> qatomic_ rename that touches many files and is > prone to conflicts. > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2 for any user-visible changes. -- PMM
Re: [PATCH v6 13/15] scripts/simplebench: improve view of ascii table
On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
> Introduce dynamic float precision and use percentage to show delta.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> scripts/simplebench/simplebench.py | 26 +-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/simplebench/simplebench.py
> b/scripts/simplebench/simplebench.py
> index 716d7fe9b2..56d3a91ea2 100644
> --- a/scripts/simplebench/simplebench.py
> +++ b/scripts/simplebench/simplebench.py
> @@ -79,10 +79,34 @@ def bench_one(test_func, test_env, test_case, count=5,
> initial_run=True):
> return result
>
>
> +def format_float(x):
> +res = round(x)
> +if res >= 100:
> +return str(res)
> +
> +res = f'{x:.1f}'
> +if len(res) >= 4:
> +return res
> +
> +return f'{x:.2f}'
This itches me to ask for some log() calculation.
Like:
%.*f' % (math.ceil(math.log10(99.95 / x)), x)
(For three significant digits)
> +def format_percent(x):
> +x *= 100
> +
> +res = round(x)
> +if res >= 10:
> +return str(res)
> +
> +return f'{x:.1f}' if res >= 1 else f'{x:.2f}'
Same here. (Also, why not append a % sign in this function?)
> def ascii_one(result):
> """Return ASCII representation of bench_one() returned dict."""
> if 'average' in result:
> -s = '{:.2f} +- {:.2f}'.format(result['average'], result['delta'])
> +avg = result['average']
> +delta_pr = result['delta'] / avg
> +s = f'{format_float(avg)}±{format_percent(delta_pr)}%'
Pre-existing, but isn’t the ± range generally assumed to be the standard
deviation?
Max
> if 'n-failed' in result:
> s += '\n({} failed)'.format(result['n-failed'])
> return s
>
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v6 11/15] iotests: add 298 to test new preallocate filter driver
On 25.09.20 10:49, Vladimir Sementsov-Ogievskiy wrote: > 25.09.2020 11:26, Max Reitz wrote: >> On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote: >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> --- >>> tests/qemu-iotests/298 | 186 + >>> tests/qemu-iotests/298.out | 5 + >>> tests/qemu-iotests/group | 1 + >>> 3 files changed, 192 insertions(+) >>> create mode 100644 tests/qemu-iotests/298 >>> create mode 100644 tests/qemu-iotests/298.out [...] >>> +class TestTruncate(iotests.QMPTestCase): >> >> The same decorator could be placed here, although this class doesn’t >> start a VM, and so is unaffected by the allowlist. Still may be >> relevant in case of block modules, I don’t know. > > Or just global test skip at file top Hm. Like verify_quorum()? Is there a generic function for that already? [...] >>> + # Probably we'll want preallocate filter to keep align to >>> cluster when >>> + # shrink preallocation, so, ignore small differece >>> + self.assertLess(abs(stat.st_size - refstat.st_size), 64 * 1024) >>> + >>> + # Preallocate filter may leak some internal clusters (for >>> example, if >>> + # guest write far over EOF, skipping some clusters - they >>> will remain >>> + # fallocated, preallocate filter don't care about such >>> leaks, it drops >>> + # only trailing preallocation. >> >> True, but that isn’t what’s happening here. (We only write 10M at 0, so >> there are no gaps.) Why do we need this 1M margin? > > We write 10M, but qcow2 also writes metadata as it wants Ah, yes, sure. Shouldn’t result in 1M, but why not. >>> + self.assertLess(abs(stat.st_blocks - refstat.st_blocks) * 512, >>> + 1024 * 1024) >> >> [...] >> >>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group >>> index ff59cfd2d4..15d5f9619b 100644 >>> --- a/tests/qemu-iotests/group >>> +++ b/tests/qemu-iotests/group >>> @@ -307,6 +307,7 @@ >>> 295 rw >>> 296 rw >>> 297 meta >>> +298 auto quick >> >> I wouldn’t mark it as quick, there is at least one preallocate=full of >> 140M, and one of 40M, plus multiple 10M data writes and falloc >> preallocations. >> >> Also, since you mark it as “auto”, have you run this test on all >> CI-relevant hosts? (Among other things I can’t predict) I wonder how >> preallocation behaves on macOS. Just because that one was always a bit >> weird about not-really-data areas. >> > > Ofcourse, I didn't run on all hosts. I'm a bit out of sync about this.. Well, someone has to do it. The background story is that tests are added to auto all the time (because “why not”), and then they fail on BSD or macOS. We have BSD docker test build targets at least, so they can be easily tested. (Well, it takes like half an hour, but you know.) (We don’t have macOS builds, as far as I can tell, but I personally don’t even know why we run the iotests on macOS at all. (Well, I also wonder about the BSDs, but given the test build targets, I shouldn’t complain, I suppose.)) (Though macOS isn’t part of the gating CI, is it? I seem to remember macOS errors are generally only reported to me half a week after the pull request is merged, which is even worse.) Anyway. I just ran the test for OpenBSD (EXTRA_CONFIGURE_OPTS='--target-list=x86_64-softmmu' \ make vm-build-openbsd) and got some failures: --- /home/qemu/qemu-test.PGo2ls/src/tests/qemu-iotests/298.out Fri Sep 25 07:10:31 2020 +++ /home/qemu/qemu-test.PGo2ls/build/tests/qemu-iotests/298.out.bad Fri Sep 25 08:57:56 2020 @@ -1,5 +1,67 @@ -. +qemu-io: Failed to resize underlying file: Unsupported preallocation mode: falloc +qemu-io: Failed to resize underlying file: Unsupported preallocation mode: falloc +.F...F... +== +FAIL: test_external_snapshot (__main__.TestPreallocateFilter) -- +Traceback (most recent call last): + File "298", line 81, in test_external_snapshot +self.test_prealloc() + File "298", line 78, in test_prealloc +self.check_big() + File "298", line 48, in check_big +self.assertTrue(os.path.getsize(disk) > 100 * MiB) +AssertionError: False is not true + +== +FAIL: test_prealloc (__main__.TestPreallocateFilter) +-- +Traceback (most recent call last): + File "298", line 78, in test_prealloc +self.check_big() + File "298", line 48, in check_big +self.assertTrue(os.path.getsize(disk) > 100 * MiB) +AssertionError: False is not true + +== +FAIL: test_reopen_opts (__main__.TestPreallocateFilter) +-- +Traceback (most recent call last): + File
Re: [PATCH v6 12/15] scripts/simplebench: support iops
On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
> Support benchmarks returning not seconds but iops. We'll use it for
> further new test.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> scripts/simplebench/simplebench.py | 35 +++---
> 1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/simplebench/simplebench.py
> b/scripts/simplebench/simplebench.py
> index 59e7314ff6..716d7fe9b2 100644
> --- a/scripts/simplebench/simplebench.py
> +++ b/scripts/simplebench/simplebench.py
[...]
> @@ -34,6 +37,7 @@ def bench_one(test_func, test_env, test_case, count=5,
> initial_run=True):
>
> Returns dict with the following fields:
> 'runs': list of test_func results
> +'dimension': dimension of results, may be 'seconds' or 'iops'
> 'average': average seconds per run (exists only if at least one run
s/seconds/value/ (or something like that)
> succeeded)
> 'delta':maximum delta between test_func result and the average
> @@ -54,11 +58,20 @@ def bench_one(test_func, test_env, test_case, count=5,
> initial_run=True):
>
> result = {'runs': runs}
>
> -successed = [r for r in runs if ('seconds' in r)]
> +successed = [r for r in runs if ('seconds' in r or 'iops' in r)]
> if successed:
((Pre-existing, but I feel the urge to point it that it should be
“succeeded”. (Or perhaps “successes”.)
Sorry, not something that should be fixed here, but I just couldn’t
contain myself.))
> -avg = sum(r['seconds'] for r in successed) / len(successed)
> +dim = 'iops' if ('iops' in successed[0]) else 'seconds'
I think this line should be dropped, because it’s obsoleted by the
if-else that follows.
> +if 'iops' in successed[0]:
> +assert all('iops' in r for r in successed)
> +dim = 'iops'
> +else:
> +assert all('seconds' in r for r in successed)
> +assert all('iops' not in r for r in successed)
> +dim = 'seconds'
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 00/31] block/export: Add infrastructure and QAPI for block exports
On Thu, Sep 24, 2020 at 05:26:46PM +0200, Kevin Wolf wrote: > We are planning to add more block export types than just NBD in the near > future (e.g. vhost-user-blk and FUSE). This series lays the ground for > this with some generic block export infrastructure and QAPI interfaces > that will allow managing all of them (for now add/remove/query). > > As a side effect, qemu-storage-daemon can now map --export directly to > the block-export-add QMP command, similar to other command line options. > The built-in NBD servers also gains new options that bring it at least a > little closer to feature parity with qemu-nbd. > > v2: > - Rebased on current master > - Fixed assumption of _exp_nbd instead of drv in generic code [Max] > - Documented blk_exp_request_shutdown() better [Max] > - iotests 140: Fixed race between QMP return value and event [Max] > - Improved the commit message for patch 26 > - Removed copy error in deprecated.rst [Max] > - iotests: Take Sequence instead of positional arguments in > qemu_tool_pipe_and_status() [Max] > - iotests: Separate patch for qemu_nbd_list_log [Max] > - iotests 307: [Max] > * Allow more image formats > * Use sock_dir for the socket > * Use f-strings instead of % operator > * Log events after deleting an export > * Test force removing an export I rebased and retested the vhost-user-blk-server block exports series on top of this: Acked-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH v6 11/15] iotests: add 298 to test new preallocate filter driver
25.09.2020 11:26, Max Reitz wrote:
On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/298 | 186 +
tests/qemu-iotests/298.out | 5 +
tests/qemu-iotests/group | 1 +
3 files changed, 192 insertions(+)
create mode 100644 tests/qemu-iotests/298
create mode 100644 tests/qemu-iotests/298.out
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
new file mode 100644
index 00..fef10f6a7a
--- /dev/null
+++ b/tests/qemu-iotests/298
[...]
+class TestPreallocateBase(iotests.QMPTestCase):
Perhaps a
@iotests.skip_if_unsupported(['preallocate'])
here?
+def setUp(self):
+iotests.qemu_img_create('-f', iotests.imgfmt, disk, str(10 * MiB))
[...]
+class TestTruncate(iotests.QMPTestCase):
The same decorator could be placed here, although this class doesn’t
start a VM, and so is unaffected by the allowlist. Still may be
relevant in case of block modules, I don’t know.
Or just global test skip at file top
+def setUp(self):
+iotests.qemu_img_create('-f', iotests.imgfmt, disk, str(10 * MiB))
+iotests.qemu_img_create('-f', iotests.imgfmt, refdisk, str(10 * MiB))
+
+def tearDown(self):
+os.remove(disk)
+os.remove(refdisk)
+
+def do_test(self, prealloc_mode, new_size):
+ret = iotests.qemu_io_silent('--image-opts', '-c', 'write 0 10M', '-c',
+ f'truncate -m {prealloc_mode} {new_size}',
+ drive_opts)
+self.assertEqual(ret, 0)
+
+ret = iotests.qemu_io_silent('-f', iotests.imgfmt, '-c', 'write 0 10M',
+ '-c',
+ f'truncate -m {prealloc_mode} {new_size}',
+ refdisk)
+self.assertEqual(ret, 0)
+
+stat = os.stat(disk)
+refstat = os.stat(refdisk)
+
+# Probably we'll want preallocate filter to keep align to cluster when
+# shrink preallocation, so, ignore small differece
+self.assertLess(abs(stat.st_size - refstat.st_size), 64 * 1024)
+
+# Preallocate filter may leak some internal clusters (for example, if
+# guest write far over EOF, skipping some clusters - they will remain
+# fallocated, preallocate filter don't care about such leaks, it drops
+# only trailing preallocation.
True, but that isn’t what’s happening here. (We only write 10M at 0, so
there are no gaps.) Why do we need this 1M margin?
We write 10M, but qcow2 also writes metadata as it wants
+self.assertLess(abs(stat.st_blocks - refstat.st_blocks) * 512,
+1024 * 1024)
[...]
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index ff59cfd2d4..15d5f9619b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -307,6 +307,7 @@
295 rw
296 rw
297 meta
+298 auto quick
I wouldn’t mark it as quick, there is at least one preallocate=full of
140M, and one of 40M, plus multiple 10M data writes and falloc
preallocations.
Also, since you mark it as “auto”, have you run this test on all
CI-relevant hosts? (Among other things I can’t predict) I wonder how
preallocation behaves on macOS. Just because that one was always a bit
weird about not-really-data areas.
Ofcourse, I didn't run on all hosts. I'm a bit out of sync about this.. If I don't put
new test in "auto", is there any chance that test would be automatically run
somewhere?
--
Best regards,
Vladimir
Re: [PATCH v2 00/31] block/export: Add infrastructure and QAPI for block exports
On Thu, Sep 24, 2020 at 10:21:56AM -0700, no-re...@patchew.org wrote: > ERROR: "(foo*)" should be "(foo *)" > #156: FILE: blockdev-nbd.c:224: > +return (BlockExport*) exp; [...] > ERROR: "(foo*)" should be "(foo *)" > #49: FILE: blockdev-nbd.c:239: > +blk_exp_unref((BlockExport*) exp); These can be fixed when merging the patches. Stefan signature.asc Description: PGP signature
Re: [PULL 00/13] Block patches
On Wed, Sep 23, 2020 at 01:28:47PM -0700, no-re...@patchew.org wrote: > Patchew URL: > https://patchew.org/QEMU/20200923161031.69474-1-stefa...@redhat.com/ checkpatch is warning about pre-existing issues. Stefan signature.asc Description: PGP signature
Re: [PATCH v7 0/5] fix & merge block_status_above and is_allocated_above
It's all because underlying "[PATCH v9 0/7] coroutines: generate wrapper code" series, I've answered in "[PATCH v9 0/7] coroutines: generate wrapper code" thread. 25.09.2020 00:45, no-re...@patchew.org wrote: Patchew URL: https://patchew.org/QEMU/20200924194003.22080-1-vsement...@virtuozzo.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === C linker for the host machine: cc ld.bfd 2.27-43 Host machine cpu family: x86_64 Host machine cpu: x86_64 ../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases. Program sh found: YES Program python3 found: YES (/usr/bin/python3) Configuring ninjatool using configuration --- return codecs.ascii_decode(input, self.errors)[0] UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128) Generating 'libqemu-aarch64-softmmu.fa.p/decode-neon-shared.c.inc'. make: *** [block/block-gen.c.stamp] Error 1 make: *** Waiting for unfinished jobs Traceback (most recent call last): File "./tests/docker/docker.py", line 709, in --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=528b329e049d459c994676e3ba6dc69a', '-u', '1003', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-e72243g9/src/docker-src.2020-09-24-17.42.11.20907:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=528b329e049d459c994676e3ba6dc69a make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-e72243g9/src' make: *** [docker-run-test-quick@centos7] Error 2 real3m12.373s user0m16.084s The full log is available at http://patchew.org/logs/20200924194003.22080-1-vsement...@virtuozzo.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com -- Best regards, Vladimir
[PATCH 0.5/7] include/block/block.h: drop non-ascii quotation mark
This is the only non-ascii character in the file and it doesn't really
needed here. Let's use normal "'" symbol for consistency with the rest
11 occurrences of "'" in the file.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/block.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/block/block.h b/include/block/block.h
index 8b87df69a1..ce2ac39299 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -304,7 +304,7 @@ enum BdrvChildRoleBits {
BDRV_CHILD_FILTERED = (1 << 2),
/*
- * Child from which to read all data that isn’t allocated in the
+ * Child from which to read all data that isn't allocated in the
* parent (i.e., the backing child); such data is copied to the
* parent through COW (and optionally COR).
* This field is mutually exclusive with DATA, METADATA, and
--
2.21.3
Re: [PATCH v6 11/15] iotests: add 298 to test new preallocate filter driver
On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> tests/qemu-iotests/298 | 186 +
> tests/qemu-iotests/298.out | 5 +
> tests/qemu-iotests/group | 1 +
> 3 files changed, 192 insertions(+)
> create mode 100644 tests/qemu-iotests/298
> create mode 100644 tests/qemu-iotests/298.out
>
> diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
> new file mode 100644
> index 00..fef10f6a7a
> --- /dev/null
> +++ b/tests/qemu-iotests/298
[...]
> +class TestPreallocateBase(iotests.QMPTestCase):
Perhaps a
@iotests.skip_if_unsupported(['preallocate'])
here?
> +def setUp(self):
> +iotests.qemu_img_create('-f', iotests.imgfmt, disk, str(10 * MiB))
[...]
> +class TestTruncate(iotests.QMPTestCase):
The same decorator could be placed here, although this class doesn’t
start a VM, and so is unaffected by the allowlist. Still may be
relevant in case of block modules, I don’t know.
> +def setUp(self):
> +iotests.qemu_img_create('-f', iotests.imgfmt, disk, str(10 * MiB))
> +iotests.qemu_img_create('-f', iotests.imgfmt, refdisk, str(10 * MiB))
> +
> +def tearDown(self):
> +os.remove(disk)
> +os.remove(refdisk)
> +
> +def do_test(self, prealloc_mode, new_size):
> +ret = iotests.qemu_io_silent('--image-opts', '-c', 'write 0 10M',
> '-c',
> + f'truncate -m {prealloc_mode}
> {new_size}',
> + drive_opts)
> +self.assertEqual(ret, 0)
> +
> +ret = iotests.qemu_io_silent('-f', iotests.imgfmt, '-c', 'write 0
> 10M',
> + '-c',
> + f'truncate -m {prealloc_mode}
> {new_size}',
> + refdisk)
> +self.assertEqual(ret, 0)
> +
> +stat = os.stat(disk)
> +refstat = os.stat(refdisk)
> +
> +# Probably we'll want preallocate filter to keep align to cluster
> when
> +# shrink preallocation, so, ignore small differece
> +self.assertLess(abs(stat.st_size - refstat.st_size), 64 * 1024)
> +
> +# Preallocate filter may leak some internal clusters (for example, if
> +# guest write far over EOF, skipping some clusters - they will remain
> +# fallocated, preallocate filter don't care about such leaks, it
> drops
> +# only trailing preallocation.
True, but that isn’t what’s happening here. (We only write 10M at 0, so
there are no gaps.) Why do we need this 1M margin?
> +self.assertLess(abs(stat.st_blocks - refstat.st_blocks) * 512,
> +1024 * 1024)
[...]
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index ff59cfd2d4..15d5f9619b 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -307,6 +307,7 @@
> 295 rw
> 296 rw
> 297 meta
> +298 auto quick
I wouldn’t mark it as quick, there is at least one preallocate=full of
140M, and one of 40M, plus multiple 10M data writes and falloc
preallocations.
Also, since you mark it as “auto”, have you run this test on all
CI-relevant hosts? (Among other things I can’t predict) I wonder how
preallocation behaves on macOS. Just because that one was always a bit
weird about not-really-data areas.
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v9 4/7] scripts: add block-coroutine-wrapper.py
24.09.2020 21:54, Vladimir Sementsov-Ogievskiy wrote:
We have a very frequent pattern of creating a coroutine from a function
with several arguments:
- create a structure to pack parameters
- create _entry function to call original function taking parameters
from struct
- do different magic to handle completion: set ret to NOT_DONE or
EINPROGRESS or use separate bool field
- fill the struct and create coroutine from _entry function with this
struct as a parameter
- do coroutine enter and BDRV_POLL_WHILE loop
Let's reduce code duplication by generating coroutine wrappers.
This patch adds scripts/block-coroutine-wrapper.py together with some
friends, which will generate functions with declared prototypes marked
by the 'generated_co_wrapper' specifier.
The usage of new code generation is as follows:
1. define the coroutine function somewhere
int coroutine_fn bdrv_co_NAME(...) {...}
2. declare in some header file
int generated_co_wrapper bdrv_NAME(...);
with same list of parameters (generated_co_wrapper is
defined in "include/block/block.h").
3. Make sure the block_gen_c delaration in block/meson.build
mentions the file with your marker function.
Still, no function is now marked, this work is for the following
commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
docs/devel/block-coroutine-wrapper.rst | 54 +++
docs/devel/index.rst | 1 +
block/block-gen.h | 49 +++
include/block/block.h | 10 ++
block/meson.build | 8 ++
scripts/block-coroutine-wrapper.py | 188 +
6 files changed, 310 insertions(+)
create mode 100644 docs/devel/block-coroutine-wrapper.rst
create mode 100644 block/block-gen.h
create mode 100644 scripts/block-coroutine-wrapper.py
diff --git a/docs/devel/block-coroutine-wrapper.rst
b/docs/devel/block-coroutine-wrapper.rst
new file mode 100644
index 00..d09fff2cc5
--- /dev/null
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -0,0 +1,54 @@
+===
+block-coroutine-wrapper
+===
+
+A lot of functions in QEMU block layer (see ``block/*``) can only be
+called in coroutine context. Such functions are normally marked by the
+coroutine_fn specifier. Still, sometimes we need to call them from
+non-coroutine context; for this we need to start a coroutine, run the
+needed function from it and wait for coroutine finish in
+BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
+void* argument. So for each coroutine_fn function which needs a
+non-coroutine interface, we should define a structure to pack the
+parameters, define a separate function to unpack the parameters and
+call the original function and finally define a new interface function
+with same list of arguments as original one, which will pack the
+parameters into a struct, create a coroutine, run it and wait in
+BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand,
+so we have a script to generate them.
+
+Usage
+=
+
+Assume we have defined the ``coroutine_fn`` function
+``bdrv_co_foo()`` and need a non-coroutine interface for it,
+called ``bdrv_foo()``. In this case the script can help. To
+trigger the generation:
+
+1. You need ``bdrv_foo`` declaration somewhere (for example, in
+ ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
+ like this:
+
+.. code-block:: c
+
+int generated_co_wrapper bdrv_foo();
+
+2. You need to feed this declaration to block-coroutine-wrapper script.
+ For this, add the .h (or .c) file with the declaration to the
+ ``input: files(...)`` list of ``block_gen_c`` target declaration in
+ ``block/meson.build``
+
+You are done. During the build, coroutine wrappers will be generated in
+``/block/block-gen.c``.
+
+Links
+=
+
+1. The script location is ``scripts/block-coroutine-wrapper.py``.
+
+2. Generic place for private ``generated_co_wrapper`` declarations is
+ ``block/coroutines.h``, for public declarations:
+ ``include/block/block.h``
+
+3. The core API of generated coroutine wrappers is placed in
+ (not generated) ``block/block-gen.h``
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 04773ce076..cb0abe1e69 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -31,3 +31,4 @@ Contents:
reset
s390-dasd-ipl
clocks
+ block-coroutine-wrapper
diff --git a/block/block-gen.h b/block/block-gen.h
new file mode 100644
index 00..f80cf4897d
--- /dev/null
+++ b/block/block-gen.h
@@ -0,0 +1,49 @@
+/*
+ * Block coroutine wrapping core, used by auto-generated block/block-gen.c
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2020 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+
Re: [PATCH v9 0/7] coroutines: generate wrapper code
24.09.2020 23:32, no-re...@patchew.org wrote: Patchew URL: https://patchew.org/QEMU/20200924185414.28642-1-vsement...@virtuozzo.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === C linker for the host machine: cc ld.bfd 2.27-43 Host machine cpu family: x86_64 Host machine cpu: x86_64 ../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases. Program sh found: YES Program python3 found: YES (/usr/bin/python3) Configuring ninjatool using configuration --- return codecs.ascii_decode(input, self.errors)[0] UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128) Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp-uncond.c.inc'. make: *** [block/block-gen.c.stamp] Error 1 make: *** Waiting for unfinished jobs Traceback (most recent call last): File "./tests/docker/docker.py", line 709, in --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=ea922a0a6ce34dfc90f59bf1b059b9d5', '-u', '1001', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-rctf_xsm/src/docker-src.2020-09-24-16.29.53.14429:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=ea922a0a6ce34dfc90f59bf1b059b9d5 make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-rctf_xsm/src' make: *** [docker-run-test-quick@centos7] Error 2 real3m4.047s user0m21.648s The full log is available at http://patchew.org/logs/20200924185414.28642-1-vsement...@virtuozzo.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp.c.inc'. Traceback (most recent call last): File "/tmp/qemu-test/src/block/../scripts/block-coroutine-wrapper.py", line 187, in f_out.write(gen_wrappers(f_in.read())) File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode return codecs.ascii_decode(input, self.errors)[0] UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128) Interesting: [root@kvm up-coroutine-wrapper]# grep --color='auto' -P -n '[^\x00-\x7F]' include/block/block.h 307: * Child from which to read all data that isn’t allocated in the ^ The file really contains one non-ascii symbol. I think it worth a separate patch. Still, it shouldn't break build process. On my system it works as is, probably unicode is default for me. Aha, from "open" specification: if encoding is not specified the encoding used is platform dependent: locale.getpreferredencoding(False) is called to get the current locale encoding. Is it ok, that utf-8 is not default on test system? So, possible solutions are: 1. Enforce utf-8 io in scripts/block-coroutine-wrapper.py (patch 4) 2. Drop non-ascii quotation mark from block.h 3. Fix the test system default to be utf-8 Do we want them all? -- Best regards, Vladimir
Re: [PATCH 00/16] hw/block/nvme: zoned namespace command set
On Sep 24 15:43, no-re...@patchew.org wrote: > Patchew URL: > https://patchew.org/QEMU/20200924204516.1881843-1-...@irrelevant.dk/ > > > > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Type: series > Message-id: 20200924204516.1881843-1-...@irrelevant.dk > Subject: [PATCH 00/16] hw/block/nvme: zoned namespace command set > > 7/16 Checking commit ab4c119d9d68 (hw/block/nvme: add commands supported and > effects log page) > ERROR: Macros with complex values should be enclosed in parenthesis > #46: FILE: hw/block/nvme.c:131: > +#define NVME_EFFECTS_NVM_INITIALIZER \ > +[NVME_CMD_FLUSH]= NVME_EFFECTS_CSUPP | \ > + NVME_EFFECTS_LBCC, \ > +[NVME_CMD_WRITE]= NVME_EFFECTS_CSUPP | \ > + NVME_EFFECTS_LBCC, \ > +[NVME_CMD_READ] = NVME_EFFECTS_CSUPP, \ > +[NVME_CMD_WRITE_ZEROES] = NVME_EFFECTS_CSUPP | \ > + NVME_EFFECTS_LBCC > Pffft. If anyone has a better idea how to make this nice and not too repetitive in the code, please let me know ;) > 11/16 Checking commit 6343d89bf734 (hw/block/nvme: add the zone management > send command) > WARNING: Block comments use a leading /* on a separate line > #66: FILE: hw/block/nvme.c:1118: > +return __nvme_allocate(ns, slba, nlb, false /* deallocate */); > > WARNING: Block comments use a leading /* on a separate line > #77: FILE: hw/block/nvme.c:1129: > +return __nvme_allocate(ns, slba, nlb, true /* deallocate */); > > total: 0 errors, 2 warnings, 704 lines checked > > Patch 11/16 has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. I seem to remember that this has been up before, but doesnt look like a fix for that has gone into checkpatch.pl. signature.asc Description: PGP signature
Re: [PATCH v6 10/15] iotests: qemu_io_silent: support --image-opts
On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > tests/qemu-iotests/iotests.py | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
