[PATCH v2] hw/block: m25p80: Support fast read for SST flashes

[Bin Meng]

From: Bin Meng 

Per SST25VF016B datasheet [1], SST flash requires a dummy byte after
the address bytes. Note only SPI mode is supported by SST flashes.

[1] http://ww1.microchip.com/downloads/en/devicedoc/s71271_04.pdf

Signed-off-by: Bin Meng 
Acked-by: Alistair Francis 

---

Changes in v2:
- rebase on qemu/master

 hw/block/m25p80.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 5f9471d83c..183d3f44c2 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -895,6 +895,9 @@ static void decode_fast_read_cmd(Flash *s)
 s->needed_bytes = get_addr_length(s);
 switch (get_man(s)) {
 /* Dummy cycles - modeled with bytes writes instead of bits */
+case MAN_SST:
+s->needed_bytes += 1;
+break;
 case MAN_WINBOND:
 s->needed_bytes += 8;
 break;
-- 
2.25.1

Re: [PATCH] docs: qsd: Explain --export nbd,name=... default

[Eric Blake]

On 3/5/21 3:48 AM, Kevin Wolf wrote:
> The 'name' option for NBD exports is optional. Add a note that the
> default for the option is the node name (people could otherwise expect
> that it's the empty string like for qemu-nbd).
> 
> Signed-off-by: Kevin Wolf 
> ---
>  docs/tools/qemu-storage-daemon.rst | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Hmm. If we are only exporting a single image, letting "" serve as the
default export name as a synonym for the non-empty node-name might be
nice.  But we can export more than one image at a time, at which point
"" has no sane default, so always requiring the client to know the node
name is tolerable.  And 'qemu-nbd --list' or 'nbdinfo --list' are
capable of showing which node name(s) an NBD server is exposing.

Reviewed-by: Eric Blake 

> 
> diff --git a/docs/tools/qemu-storage-daemon.rst 
> b/docs/tools/qemu-storage-daemon.rst
> index fe3042d609..086493ebb3 100644
> --- a/docs/tools/qemu-storage-daemon.rst
> +++ b/docs/tools/qemu-storage-daemon.rst
> @@ -80,8 +80,9 @@ Standard options:
>requests for modifying data (the default is off).
>  
>The ``nbd`` export type requires ``--nbd-server`` (see below). ``name`` is
> -  the NBD export name. ``bitmap`` is the name of a dirty bitmap reachable 
> from
> -  the block node, so the NBD client can use NBD_OPT_SET_META_CONTEXT with the
> +  the NBD export name (if not specified, it defaults to the given
> +  ``node-name``). ``bitmap`` is the name of a dirty bitmap reachable from the
> +  block node, so the NBD client can use NBD_OPT_SET_META_CONTEXT with the
>metadata context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap.
>  
>The ``vhost-user-blk`` export type takes a vhost-user socket address on 
> which
> 

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

[PATCH v3 5/6] block-coroutine-wrapper: allow non bdrv_ prefix

[Vladimir Sementsov-Ogievskiy]

We are going to reuse the script to generate a qcow2_ function in
further commit. Prepare the script now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 scripts/block-coroutine-wrapper.py | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/scripts/block-coroutine-wrapper.py 
b/scripts/block-coroutine-wrapper.py
index 0461fd1c45..85dbeb9ecf 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -98,12 +98,13 @@ def snake_to_camel(func_name: str) -> str:
 
 
 def gen_wrapper(func: FuncDecl) -> str:
-assert func.name.startswith('bdrv_')
-assert not func.name.startswith('bdrv_co_')
+assert not '_co_' in func.name
 assert func.return_type == 'int'
 assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *']
 
-name = 'bdrv_co_' + func.name[5:]
+subsystem, subname = func.name.split('_', 1)
+
+name = f'{subsystem}_co_{subname}'
 bs = 'bs' if func.args[0].type == 'BlockDriverState *' else 'child->bs'
 struct_name = snake_to_camel(name)
 
-- 
2.29.2

[PATCH v3 2/6] iotests: add qcow2-discard-during-rewrite

[Vladimir Sementsov-Ogievskiy]

Simple test:
 - start writing to allocated cluster A
 - discard this cluster
 - write to another unallocated cluster B (it's allocated in same place
   where A was allocated)
 - continue writing to A

For now last action pollutes cluster B which is a bug fixed by the
following commit.

For now, add test to "disabled" group, so that it doesn't run
automatically.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 .../tests/qcow2-discard-during-rewrite| 72 +++
 .../tests/qcow2-discard-during-rewrite.out| 21 ++
 2 files changed, 93 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/qcow2-discard-during-rewrite
 create mode 100644 tests/qemu-iotests/tests/qcow2-discard-during-rewrite.out

diff --git a/tests/qemu-iotests/tests/qcow2-discard-during-rewrite 
b/tests/qemu-iotests/tests/qcow2-discard-during-rewrite
new file mode 100755
index 00..7f0d8a107a
--- /dev/null
+++ b/tests/qemu-iotests/tests/qcow2-discard-during-rewrite
@@ -0,0 +1,72 @@
+#!/usr/bin/env bash
+# group: quick disabled
+#
+# Test discarding (and reusing) host cluster during writing data to it.
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=vsement...@virtuozzo.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./../common.rc
+. ./../common.filter
+
+_supported_fmt qcow2
+_supported_proto file fuse
+_supported_os Linux
+
+size=1M
+_make_test_img $size
+
+(
+cat <

[PATCH v3 1/6] block-jobs: flush target at the end of .run()

[Vladimir Sementsov-Ogievskiy]

We are going to implement compressed write cache to improve performance
of compressed backup when target is opened in O_DIRECT mode. We
definitely want to flush the cache at backup finish, and if flush fails
it should be reported as block-job failure, not simply ignored in
bdrv_close(). So, teach all block-jobs to flush their targets at the
end.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/blockjob_int.h | 18 ++
 block/backup.c   |  8 +---
 block/commit.c   |  2 ++
 block/mirror.c   |  2 ++
 block/stream.c   |  2 ++
 blockjob.c   | 16 
 6 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 6633d83da2..6ef3123120 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -119,4 +119,22 @@ int64_t block_job_ratelimit_get_delay(BlockJob *job, 
uint64_t n);
 BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
 int is_read, int error);
 
+/**
+ * block_job_final_target_flush:
+ * @job: The job to signal an error for if flush failed.
+ * @target_bs: The bs to flush.
+ * @ret: Will be updated (to return code of bdrv_flush()) only if it is zero
+ *   now. This is a bit unusual interface but all callers are comfortable
+ *   with it.
+ *
+ * The function is intended to be called at the end of .run() for any data
+ * copying job.
+ *
+ * There are may be some internal caches in format layers of target,
+ * like compressed_cache in qcow2 format. So we should call flush to
+ * be sure that all data reached the destination protocol layer.
+ */
+void block_job_final_target_flush(BlockJob *job, BlockDriverState *target_bs,
+  int *ret);
+
 #endif
diff --git a/block/backup.c b/block/backup.c
index 94e6dcd72e..d3ba8e0f75 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -255,7 +255,7 @@ static void backup_init_bcs_bitmap(BackupBlockJob *job)
 static int coroutine_fn backup_run(Job *job, Error **errp)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-int ret;
+int ret = 0;
 
 backup_init_bcs_bitmap(s);
 
@@ -297,10 +297,12 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
 job_yield(job);
 }
 } else {
-return backup_loop(s);
+ret = backup_loop(s);
 }
 
-return 0;
+block_job_final_target_flush(>common, s->target_bs, );
+
+return ret;
 }
 
 static void coroutine_fn backup_pause(Job *job)
diff --git a/block/commit.c b/block/commit.c
index dd9ba87349..1b61b60ccd 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -193,6 +193,8 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
 ret = 0;
 
 out:
+block_job_final_target_flush(>common, blk_bs(s->base), );
+
 qemu_vfree(buf);
 
 return ret;
diff --git a/block/mirror.c b/block/mirror.c
index 1803c6988b..bc559bd053 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1095,6 +1095,8 @@ immediate_exit:
 g_free(s->in_flight_bitmap);
 bdrv_dirty_iter_free(s->dbi);
 
+block_job_final_target_flush(>common, blk_bs(s->target), );
+
 if (need_drain) {
 s->in_drain = true;
 bdrv_drained_begin(bs);
diff --git a/block/stream.c b/block/stream.c
index 1fa742b0db..cda41e4a64 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -182,6 +182,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
 }
 }
 
+block_job_final_target_flush(>common, s->target_bs, );
+
 /* Do not remove the backing file if an error was there but ignored. */
 return error;
 }
diff --git a/blockjob.c b/blockjob.c
index f2feff051d..e226bfbbfb 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -525,3 +525,19 @@ BlockErrorAction block_job_error_action(BlockJob *job, 
BlockdevOnError on_err,
 }
 return action;
 }
+
+void block_job_final_target_flush(BlockJob *job, BlockDriverState *target_bs,
+  int *ret)
+{
+int flush_ret = bdrv_flush(target_bs);
+
+if (flush_ret < 0 && !block_job_is_internal(job)) {
+qapi_event_send_block_job_error(job->job.id,
+IO_OPERATION_TYPE_WRITE,
+BLOCK_ERROR_ACTION_REPORT);
+}
+
+if (*ret == 0) {
+*ret = flush_ret;
+}
+}
-- 
2.29.2

[PATCH v3 4/6] util: implement seqcache

[Vladimir Sementsov-Ogievskiy]

Implement cache for small sequential unaligned writes, so that they may
be cached until we get a complete cluster and then write it.

The cache is intended to be used for backup to qcow2 compressed target
opened in O_DIRECT mode, but can be reused for any similar (even not
block-layer related) task.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/qemu/seqcache.h |  42 +
 util/seqcache.c | 361 
 MAINTAINERS |   6 +
 util/meson.build|   1 +
 4 files changed, 410 insertions(+)
 create mode 100644 include/qemu/seqcache.h
 create mode 100644 util/seqcache.c

diff --git a/include/qemu/seqcache.h b/include/qemu/seqcache.h
new file mode 100644
index 00..a308d54b01
--- /dev/null
+++ b/include/qemu/seqcache.h
@@ -0,0 +1,42 @@
+/*
+ * Cache for small sequential write requests.
+ *
+ * Copyright (c) 2021 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
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef QEMU_SEQCACHE_H
+#define QEMU_SEQCACHE_H
+
+typedef struct SeqCache SeqCache;
+
+SeqCache *seqcache_new(int64_t cluster_size);
+void seqcache_free(SeqCache *s);
+
+void seqcache_write(SeqCache *s, int64_t offset, int64_t bytes, uint8_t *buf);
+int64_t seqcache_read(SeqCache *s, int64_t offset, int64_t bytes, uint8_t 
*buf);
+
+bool seqcache_get_next_flush(SeqCache *s, int64_t *offset, int64_t *bytes,
+ uint8_t **buf, bool *unfinished);
+void seqcache_discard_cluster(SeqCache *s, int64_t offset);
+
+uint64_t seqcache_nb_clusters(SeqCache *s);
+
+#endif /* QEMU_SEQCACHE_H */
diff --git a/util/seqcache.c b/util/seqcache.c
new file mode 100644
index 00..d923560eab
--- /dev/null
+++ b/util/seqcache.c
@@ -0,0 +1,361 @@
+/*
+ * Cache for small sequential write requests.
+ *
+ * Copyright (c) 2021 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
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ *
+ * = Description =
+ *
+ * SeqCache is an abbreviation for Sequential Cache.
+ *
+ * The Cache is intended to improve performance of small unaligned sequential
+ * writes. Cache has a cluster_size parameter and the unit of caching is 
aligned
+ * cluster.  Cache has a list of cached clusters, several "finished" ones and 
at
+ * most one "unfinished". "unfinished" cluster is a cluster where last byte of
+ * last write operation is cached and there is a free place after that byte to
+ * the end of cluster. "finished" clusters are just stored in cache to be read
+ * or flushed and don't allow any writes to them.
+ *
+ * If write to the cache intersects cluster bounds, it's splat into several
+ * requests by cluster bounds. So, consider a write that doesn't intersect
+ * cluster bounds to describe the whole picture:
+ *
+ * There are two cases allowed:
+ *
+ * 1. Sequential write to "unfinished" cluster. Actually it's a write 
sequential
+ *previous write. The data goes to 

[PATCH v3 3/6] block/qcow2: introduce inflight writes counters: fix discard

[Vladimir Sementsov-Ogievskiy]

There is a bug in qcow2: host cluster can be discarded (refcount
becomes 0) and reused during data write. In this case data write may
pollute another cluster (recently allocated) or even metadata.

To fix the issue let's track inflight writes to host cluster in the
hash table and consider new counter when discarding and reusing host
clusters.

Enable qcow2-discard-during-rewrite as it is fixed.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h |   9 ++
 block/qcow2-refcount.c| 149 +-
 block/qcow2.c |  26 ++-
 .../tests/qcow2-discard-during-rewrite|   2 +-
 4 files changed, 181 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 0678073b74..e18d8dfbae 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -420,6 +420,8 @@ typedef struct BDRVQcow2State {
  * is to convert the image with the desired compression type set.
  */
 Qcow2CompressionType compression_type;
+
+GHashTable *inflight_writes_counters;
 } BDRVQcow2State;
 
 typedef struct Qcow2COWRegion {
@@ -896,6 +898,13 @@ int qcow2_shrink_reftable(BlockDriverState *bs);
 int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
 int qcow2_detect_metadata_preallocation(BlockDriverState *bs);
 
+int qcow2_inflight_writes_inc(BlockDriverState *bs, int64_t offset,
+  int64_t length);
+int qcow2_inflight_writes_dec(BlockDriverState *bs, int64_t offset,
+  int64_t length);
+int qcow2_inflight_writes_dec_locked(BlockDriverState *bs, int64_t offset,
+ int64_t length);
+
 /* qcow2-cluster.c functions */
 int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
 bool exact_size);
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8e649b008e..464d133368 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -799,6 +799,140 @@ found:
 }
 }
 
+/*
+ * Qcow2InFlightRefcount is a type for values of s->inflight_writes_counters
+ * hasm map. And it's keys are cluster indices.
+ */
+typedef struct Qcow2InFlightRefcount {
+/*
+ * Number of in-flight writes to the cluster, always > 0, as when becomes
+ * 0 the entry is removed from s->inflight_writes_counters.
+ */
+uint64_t inflight_writes_cnt;
+
+/* Cluster refcount is known to be zero */
+bool refcount_zero;
+
+/* Cluster refcount was made zero with this discard-type */
+enum qcow2_discard_type type;
+} Qcow2InFlightRefcount;
+
+static Qcow2InFlightRefcount *find_infl_wr(BDRVQcow2State *s,
+   int64_t cluster_index)
+{
+Qcow2InFlightRefcount *infl;
+
+if (!s->inflight_writes_counters) {
+return NULL;
+}
+
+infl = g_hash_table_lookup(s->inflight_writes_counters, _index);
+
+if (infl) {
+assert(infl->inflight_writes_cnt > 0);
+}
+
+return infl;
+}
+
+/*
+ * Returns true if there are any in-flight writes to the cluster blocking
+ * its reallocation.
+ */
+static bool has_infl_wr(BDRVQcow2State *s, int64_t cluster_index)
+{
+return !!find_infl_wr(s, cluster_index);
+}
+
+static int update_inflight_write_cnt(BlockDriverState *bs,
+ int64_t offset, int64_t length,
+ bool decrease, bool locked)
+{
+BDRVQcow2State *s = bs->opaque;
+int64_t start, last, cluster_offset;
+
+if (locked) {
+qemu_co_mutex_assert_locked(>lock);
+}
+
+start = start_of_cluster(s, offset);
+last = start_of_cluster(s, offset + length - 1);
+for (cluster_offset = start; cluster_offset <= last;
+ cluster_offset += s->cluster_size)
+{
+int64_t cluster_index = cluster_offset >> s->cluster_bits;
+Qcow2InFlightRefcount *infl = find_infl_wr(s, cluster_index);
+
+if (!infl) {
+infl = g_new0(Qcow2InFlightRefcount, 1);
+g_hash_table_insert(s->inflight_writes_counters,
+g_memdup(_index, 
sizeof(cluster_index)),
+infl);
+}
+
+if (decrease) {
+assert(infl->inflight_writes_cnt >= 1);
+infl->inflight_writes_cnt--;
+} else {
+infl->inflight_writes_cnt++;
+}
+
+if (infl->inflight_writes_cnt == 0) {
+bool refcount_zero = infl->refcount_zero;
+enum qcow2_discard_type type = infl->type;
+
+g_hash_table_remove(s->inflight_writes_counters, _index);
+
+if (refcount_zero) {
+/*
+ * Slow path. We must reset normal refcount to actually release
+ * the cluster.
+ */
+int ret;
+
+if (!locked) {
+qemu_co_mutex_lock(>lock);
+}
+ret = 

[PATCH v3 6/6] block/qcow2: use seqcache for compressed writes

[Vladimir Sementsov-Ogievskiy]

Compressed writes are unaligned to 512, which works very slow in
O_DIRECT mode. Let's use the cache.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/coroutines.h |   3 +
 block/qcow2.h  |   4 ++
 block/qcow2-refcount.c |  10 +++
 block/qcow2.c  | 158 ++---
 4 files changed, 164 insertions(+), 11 deletions(-)

diff --git a/block/coroutines.h b/block/coroutines.h
index 4cfb4946e6..cb8e05830e 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -66,4 +66,7 @@ int coroutine_fn bdrv_co_readv_vmstate(BlockDriverState *bs,
 int coroutine_fn bdrv_co_writev_vmstate(BlockDriverState *bs,
 QEMUIOVector *qiov, int64_t pos);
 
+int coroutine_fn qcow2_co_flush_compressed_cache(BlockDriverState *bs);
+int generated_co_wrapper qcow2_flush_compressed_cache(BlockDriverState *bs);
+
 #endif /* BLOCK_COROUTINES_INT_H */
diff --git a/block/qcow2.h b/block/qcow2.h
index e18d8dfbae..8b8fed0950 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -28,6 +28,7 @@
 #include "crypto/block.h"
 #include "qemu/coroutine.h"
 #include "qemu/units.h"
+#include "qemu/seqcache.h"
 #include "block/block_int.h"
 
 //#define DEBUG_ALLOC
@@ -422,6 +423,9 @@ typedef struct BDRVQcow2State {
 Qcow2CompressionType compression_type;
 
 GHashTable *inflight_writes_counters;
+
+SeqCache *compressed_cache;
+int compressed_flush_ret;
 } BDRVQcow2State;
 
 typedef struct Qcow2COWRegion {
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 464d133368..218917308e 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -29,6 +29,7 @@
 #include "qemu/bswap.h"
 #include "qemu/cutils.h"
 #include "trace.h"
+#include "block/coroutines.h"
 
 static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size,
 uint64_t max);
@@ -1040,6 +1041,10 @@ static int QEMU_WARN_UNUSED_RESULT 
update_refcount(BlockDriverState *bs,
 qcow2_cache_discard(s->l2_table_cache, table);
 }
 
+if (s->compressed_cache) {
+seqcache_discard_cluster(s->compressed_cache, cluster_offset);
+}
+
 if (s->discard_passthrough[type]) {
 update_refcount_discard(bs, cluster_offset, s->cluster_size);
 }
@@ -1349,6 +1354,11 @@ int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
 BDRVQcow2State *s = bs->opaque;
 int ret;
 
+ret = qcow2_flush_compressed_cache(bs);
+if (ret < 0) {
+return ret;
+}
+
 ret = qcow2_cache_write(bs, s->l2_table_cache);
 if (ret < 0) {
 return ret;
diff --git a/block/qcow2.c b/block/qcow2.c
index 6ee94421e0..5f3713cd6f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -42,6 +42,7 @@
 #include "qapi/qapi-visit-block-core.h"
 #include "crypto.h"
 #include "block/aio_task.h"
+#include "block/coroutines.h"
 
 /*
   Differences with QCOW:
@@ -1834,6 +1835,10 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 s->inflight_writes_counters =
 g_hash_table_new_full(g_int64_hash, g_int64_equal, g_free, g_free);
 
+if (!has_data_file(bs) && (bs->file->bs->open_flags & BDRV_O_NOCACHE)) {
+s->compressed_cache = seqcache_new(s->cluster_size);
+}
+
 return ret;
 
  fail:
@@ -2653,6 +2658,91 @@ fail_nometa:
 return ret;
 }
 
+/*
+ * Flush one cluster of compressed cache if exists. If @unfinished is true and
+ * there is no finished cluster to flush, flush the unfinished one. Otherwise
+ * flush only finished clusters.
+ *
+ * Return 0 if nothing to flush, 1 if one cluster successfully flushed and <0 
on
+ * error.
+ */
+static int coroutine_fn
+qcow2_co_compressed_flush_one(BlockDriverState *bs, bool unfinished)
+{
+BDRVQcow2State *s = bs->opaque;
+int ret;
+int64_t align = 1;
+int64_t offset, bytes;
+uint8_t *buf;
+
+if (!s->compressed_cache) {
+return 0;
+}
+
+if (!seqcache_get_next_flush(s->compressed_cache, , , ,
+ ))
+{
+return 0;
+}
+
+qcow2_inflight_writes_inc(bs, offset, bytes);
+
+/*
+ * Don't try to align-up unfinished cached cluster, parallel write to it is
+ * possible! For finished host clusters data in the tail of the cluster 
will
+ * be never used. So, take some good alignment for speed.
+ *
+ * Note also, that seqcache guarantees that allocated size of @buf is 
enough
+ * to fill the cluster up to the end, so we are safe to align up with
+ * align <= cluster_size.
+ */
+if (!unfinished) {
+align = MIN(s->cluster_size,
+MAX(s->data_file->bs->bl.request_alignment,
+bdrv_opt_mem_align(bs)));
+}
+
+BLKDBG_EVENT(s->data_file, BLKDBG_WRITE_COMPRESSED);
+ret = bdrv_co_pwrite(s->data_file, offset,
+ QEMU_ALIGN_UP(offset + bytes, align) - offset,
+ 

[PATCH v3 0/6] qcow2: compressed write cache

[Vladimir Sementsov-Ogievskiy]

The series inherits "[PATCH 0/7] qcow2: compressed write cache"
(changed a lot, the cache is rewritten), and
"[PATCH v2(RFC) 0/3] qcow2: fix parallel rewrite and "
(the fix solution is taken from v1 instead, as the are problems with v2,
 described in cover letter)

Core changes in v3:

 - cache is rewritten and now is separated even from block-layer
 - I'm tired of trying to catch degradation when use both pagecache and
   my new cache.. So, I decided that using both caches is just a bad
   idea and in v3 cache is enabled only when qcow2 file child opened in
   O_DIRECT

Vladimir Sementsov-Ogievskiy (6):
  block-jobs: flush target at the end of .run()
  iotests: add qcow2-discard-during-rewrite
  block/qcow2: introduce inflight writes counters: fix discard
  util: implement seqcache
  block-coroutine-wrapper: allow non bdrv_ prefix
  block/qcow2: use seqcache for compressed writes

 block/coroutines.h|   3 +
 block/qcow2.h |  13 +
 include/block/blockjob_int.h  |  18 +
 include/qemu/seqcache.h   |  42 ++
 block/backup.c|   8 +-
 block/commit.c|   2 +
 block/mirror.c|   2 +
 block/qcow2-refcount.c| 159 +++-
 block/qcow2.c | 178 -
 block/stream.c|   2 +
 blockjob.c|  16 +
 util/seqcache.c   | 361 ++
 MAINTAINERS   |   6 +
 scripts/block-coroutine-wrapper.py|   7 +-
 .../tests/qcow2-discard-during-rewrite|  72 
 .../tests/qcow2-discard-during-rewrite.out|  21 +
 util/meson.build  |   1 +
 17 files changed, 893 insertions(+), 18 deletions(-)
 create mode 100644 include/qemu/seqcache.h
 create mode 100644 util/seqcache.c
 create mode 100755 tests/qemu-iotests/tests/qcow2-discard-during-rewrite
 create mode 100644 tests/qemu-iotests/tests/qcow2-discard-during-rewrite.out

-- 
2.29.2

[PULL 27/31] parallels: support bitmap extension for read-only mode

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy 

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20210224104707.88430-5-vsement...@virtuozzo.com>
Reviewed-by: Denis V. Lunev 
Signed-off-by: Kevin Wolf 
---
 block/parallels.h |   6 +-
 block/parallels-ext.c | 300 ++
 block/parallels.c |  18 +++
 block/meson.build |   3 +-
 4 files changed, 325 insertions(+), 2 deletions(-)
 create mode 100644 block/parallels-ext.c

diff --git a/block/parallels.h b/block/parallels.h
index 9a9209e320..f22f43f988 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -48,7 +48,8 @@ typedef struct ParallelsHeader {
 uint64_t nb_sectors;
 uint32_t inuse;
 uint32_t data_off;
-char padding[12];
+uint32_t flags;
+uint64_t ext_off;
 } QEMU_PACKED ParallelsHeader;
 
 typedef enum ParallelsPreallocMode {
@@ -85,4 +86,7 @@ typedef struct BDRVParallelsState {
 Error *migration_blocker;
 } BDRVParallelsState;
 
+int parallels_read_format_extension(BlockDriverState *bs,
+int64_t ext_off, Error **errp);
+
 #endif
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
new file mode 100644
index 00..e0dd0975c6
--- /dev/null
+++ b/block/parallels-ext.c
@@ -0,0 +1,300 @@
+/*
+ * Support of Parallels Format Extension. It's a part of Parallels format
+ * driver.
+ *
+ * Copyright (c) 2021 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
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "block/block_int.h"
+#include "parallels.h"
+#include "crypto/hash.h"
+#include "qemu/uuid.h"
+
+#define PARALLELS_FORMAT_EXTENSION_MAGIC 0xAB234CEF23DCEA87ULL
+
+#define PARALLELS_END_OF_FEATURES_MAGIC 0x0ULL
+#define PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC 0x20385FAE252CB34AULL
+
+typedef struct ParallelsFormatExtensionHeader {
+uint64_t magic; /* PARALLELS_FORMAT_EXTENSION_MAGIC */
+uint8_t check_sum[16];
+} QEMU_PACKED ParallelsFormatExtensionHeader;
+
+typedef struct ParallelsFeatureHeader {
+uint64_t magic;
+uint64_t flags;
+uint32_t data_size;
+uint32_t _unused;
+} QEMU_PACKED ParallelsFeatureHeader;
+
+typedef struct ParallelsDirtyBitmapFeature {
+uint64_t size;
+uint8_t id[16];
+uint32_t granularity;
+uint32_t l1_size;
+/* L1 table follows */
+} QEMU_PACKED ParallelsDirtyBitmapFeature;
+
+/* Given L1 table read bitmap data from the image and populate @bitmap */
+static int parallels_load_bitmap_data(BlockDriverState *bs,
+  const uint64_t *l1_table,
+  uint32_t l1_size,
+  BdrvDirtyBitmap *bitmap,
+  Error **errp)
+{
+BDRVParallelsState *s = bs->opaque;
+int ret = 0;
+uint64_t offset, limit;
+uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+uint8_t *buf = NULL;
+uint64_t i, tab_size =
+DIV_ROUND_UP(bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size),
+ s->cluster_size);
+
+if (tab_size != l1_size) {
+error_setg(errp, "Bitmap table size %" PRIu32 " does not correspond "
+   "to bitmap size and cluster size. Expected %" PRIu64,
+   l1_size, tab_size);
+return -EINVAL;
+}
+
+buf = qemu_blockalign(bs, s->cluster_size);
+limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
+for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
+uint64_t count = MIN(bm_size - offset, limit);
+uint64_t entry = l1_table[i];
+
+if (entry == 0) {
+/* No need to deserialize zeros because @bitmap is cleared. */
+continue;
+}
+
+if (entry == 1) {
+bdrv_dirty_bitmap_deserialize_ones(bitmap, offset, count, false);
+} else {
+ret = bdrv_pread(bs->file, entry 

[PULL 26/31] block/parallels: BDRVParallelsState: add cluster_size field

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy 

We are going to use it in more places, calculating
"s->tracks << BDRV_SECTOR_BITS" doesn't look good.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20210224104707.88430-4-vsement...@virtuozzo.com>
Reviewed-by: Denis V. Lunev 
Signed-off-by: Kevin Wolf 
---
 block/parallels.h | 1 +
 block/parallels.c | 8 
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/parallels.h b/block/parallels.h
index 5aa101cfc8..9a9209e320 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -79,6 +79,7 @@ typedef struct BDRVParallelsState {
 ParallelsPreallocMode prealloc_mode;
 
 unsigned int tracks;
+unsigned int cluster_size;
 
 unsigned int off_multiplier;
 Error *migration_blocker;
diff --git a/block/parallels.c b/block/parallels.c
index 3c22dfdc9d..9594d84978 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -421,7 +421,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState 
*bs,
 int ret;
 uint32_t i;
 bool flush_bat = false;
-int cluster_size = s->tracks << BDRV_SECTOR_BITS;
 
 size = bdrv_getlength(bs->file->bs);
 if (size < 0) {
@@ -472,7 +471,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState 
*bs,
 high_off = off;
 }
 
-if (prev_off != 0 && (prev_off + cluster_size) != off) {
+if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
 res->bfi.fragmented_clusters++;
 }
 prev_off = off;
@@ -487,10 +486,10 @@ static int coroutine_fn 
parallels_co_check(BlockDriverState *bs,
 }
 }
 
-res->image_end_offset = high_off + cluster_size;
+res->image_end_offset = high_off + s->cluster_size;
 if (size > res->image_end_offset) {
 int64_t count;
-count = DIV_ROUND_UP(size - res->image_end_offset, cluster_size);
+count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
 fprintf(stderr, "%s space leaked at the end of the image %" PRId64 
"\n",
 fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
 size - res->image_end_offset);
@@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 ret = -EFBIG;
 goto fail;
 }
+s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
 
 s->bat_size = le32_to_cpu(ph.bat_entries);
 if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
-- 
2.29.2

[PULL 14/31] libqtest: add qtest_kill_qemu()

[Kevin Wolf]

From: Stefan Hajnoczi 

Tests that manage multiple processes may wish to kill QEMU before
destroying the QTestState. Expose a function to do that.

The vhost-user-blk-test testcase will need this.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Wainer dos Santos Moschetta 
Message-Id: <20210223144653.811468-4-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 tests/qtest/libqos/libqtest.h | 11 +++
 tests/qtest/libqtest.c|  7 ---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
index e5f1ec590c..51287b9276 100644
--- a/tests/qtest/libqos/libqtest.h
+++ b/tests/qtest/libqos/libqtest.h
@@ -74,6 +74,17 @@ QTestState *qtest_init_without_qmp_handshake(const char 
*extra_args);
  */
 QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd);
 
+/**
+ * qtest_kill_qemu:
+ * @s: #QTestState instance to operate on.
+ *
+ * Kill the QEMU process and wait for it to terminate. It is safe to call this
+ * function multiple times. Normally qtest_quit() is used instead because it
+ * also frees QTestState. Use qtest_kill_qemu() when you just want to kill QEMU
+ * and qtest_quit() will be called later.
+ */
+void qtest_kill_qemu(QTestState *s);
+
 /**
  * qtest_quit:
  * @s: #QTestState instance to operate on.
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index b19d2ebda0..2a98de2907 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -133,7 +133,7 @@ void qtest_set_expected_status(QTestState *s, int status)
 s->expected_status = status;
 }
 
-static void kill_qemu(QTestState *s)
+void qtest_kill_qemu(QTestState *s)
 {
 pid_t pid = s->qemu_pid;
 int wstatus;
@@ -143,6 +143,7 @@ static void kill_qemu(QTestState *s)
 kill(pid, SIGTERM);
 TFR(pid = waitpid(s->qemu_pid, >wstatus, 0));
 assert(pid == s->qemu_pid);
+s->qemu_pid = -1;
 }
 
 /*
@@ -169,7 +170,7 @@ static void kill_qemu(QTestState *s)
 
 static void kill_qemu_hook_func(void *s)
 {
-kill_qemu(s);
+qtest_kill_qemu(s);
 }
 
 static void sigabrt_handler(int signo)
@@ -373,7 +374,7 @@ void qtest_quit(QTestState *s)
 /* Uninstall SIGABRT handler on last instance */
 cleanup_sigabrt_handler();
 
-kill_qemu(s);
+qtest_kill_qemu(s);
 close(s->fd);
 close(s->qmp_fd);
 g_string_free(s->rx, true);
-- 
2.29.2

[PULL 30/31] MAINTAINERS: update parallels block driver

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy 

Add new parallels-ext.c and myself as co-maintainer.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20210304095151.19358-1-vsement...@virtuozzo.com>
Reviewed-by: Denis V. Lunev 
Signed-off-by: Kevin Wolf 
---
 MAINTAINERS | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4249acc62e..81e7c3b5e5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3126,10 +3126,13 @@ F: block/dmg.c
 parallels
 M: Stefan Hajnoczi 
 M: Denis V. Lunev 
+M: Vladimir Sementsov-Ogievskiy 
 L: qemu-block@nongnu.org
 S: Supported
 F: block/parallels.c
+F: block/parallels-ext.c
 F: docs/interop/parallels.txt
+T: git https://src.openvz.org/scm/~vsementsov/qemu.git parallels
 
 qed
 M: Stefan Hajnoczi 
-- 
2.29.2

[PULL 18/31] block/export: fix blk_size double byteswap

[Kevin Wolf]

From: Stefan Hajnoczi 

The config->blk_size field is little-endian. Use the native-endian
blk_size variable to avoid double byteswapping.

Fixes: 11f60f7eaee2630dd6fa0c3a8c49f792e46c4cf1 ("block/export: make 
vhost-user-blk config space little-endian")
Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210223144653.811468-8-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 block/export/vhost-user-blk-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index ab2c4d44c4..7aea132f69 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -356,7 +356,7 @@ vu_blk_initialize_config(BlockDriverState *bs,
 config->num_queues = cpu_to_le16(num_queues);
 config->max_discard_sectors = cpu_to_le32(32768);
 config->max_discard_seg = cpu_to_le32(1);
-config->discard_sector_alignment = cpu_to_le32(config->blk_size >> 9);
+config->discard_sector_alignment = cpu_to_le32(blk_size >> 9);
 config->max_write_zeroes_sectors = cpu_to_le32(32768);
 config->max_write_zeroes_seg = cpu_to_le32(1);
 }
-- 
2.29.2

[PULL 16/31] test: new qTest case to test the vhost-user-blk-server

[Kevin Wolf]

From: Coiby Xu 

This test case has the same tests as tests/virtio-blk-test.c except for
tests have block_resize. Since the vhost-user-blk export only serves one
client one time, two exports are started by qemu-storage-daemon for the
hotplug test.

Suggested-by: Thomas Huth 
Signed-off-by: Coiby Xu 
Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210223144653.811468-6-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 tests/qtest/libqos/vhost-user-blk.h |  48 ++
 tests/qtest/libqos/vhost-user-blk.c | 130 +
 tests/qtest/vhost-user-blk-test.c   | 788 
 MAINTAINERS |   2 +
 tests/qtest/libqos/meson.build  |   1 +
 tests/qtest/meson.build |   4 +
 6 files changed, 973 insertions(+)
 create mode 100644 tests/qtest/libqos/vhost-user-blk.h
 create mode 100644 tests/qtest/libqos/vhost-user-blk.c
 create mode 100644 tests/qtest/vhost-user-blk-test.c

diff --git a/tests/qtest/libqos/vhost-user-blk.h 
b/tests/qtest/libqos/vhost-user-blk.h
new file mode 100644
index 00..2a03456a45
--- /dev/null
+++ b/tests/qtest/libqos/vhost-user-blk.h
@@ -0,0 +1,48 @@
+/*
+ * libqos driver framework
+ *
+ * Based on tests/qtest/libqos/virtio-blk.c
+ *
+ * Copyright (c) 2020 Coiby Xu 
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see 
+ */
+
+#ifndef TESTS_LIBQOS_VHOST_USER_BLK_H
+#define TESTS_LIBQOS_VHOST_USER_BLK_H
+
+#include "qgraph.h"
+#include "virtio.h"
+#include "virtio-pci.h"
+
+typedef struct QVhostUserBlk QVhostUserBlk;
+typedef struct QVhostUserBlkPCI QVhostUserBlkPCI;
+typedef struct QVhostUserBlkDevice QVhostUserBlkDevice;
+
+struct QVhostUserBlk {
+QVirtioDevice *vdev;
+};
+
+struct QVhostUserBlkPCI {
+QVirtioPCIDevice pci_vdev;
+QVhostUserBlk blk;
+};
+
+struct QVhostUserBlkDevice {
+QOSGraphObject obj;
+QVhostUserBlk blk;
+};
+
+#endif
diff --git a/tests/qtest/libqos/vhost-user-blk.c 
b/tests/qtest/libqos/vhost-user-blk.c
new file mode 100644
index 00..568c3426ed
--- /dev/null
+++ b/tests/qtest/libqos/vhost-user-blk.c
@@ -0,0 +1,130 @@
+/*
+ * libqos driver framework
+ *
+ * Based on tests/qtest/libqos/virtio-blk.c
+ *
+ * Copyright (c) 2020 Coiby Xu 
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2.1 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see 
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qemu/module.h"
+#include "standard-headers/linux/virtio_blk.h"
+#include "vhost-user-blk.h"
+
+#define PCI_SLOT0x04
+#define PCI_FN  0x00
+
+/* virtio-blk-device */
+static void *qvhost_user_blk_get_driver(QVhostUserBlk *v_blk,
+const char *interface)
+{
+if (!g_strcmp0(interface, "vhost-user-blk")) {
+return v_blk;
+}
+if (!g_strcmp0(interface, "virtio")) {
+return v_blk->vdev;
+}
+
+fprintf(stderr, "%s not present in vhost-user-blk-device\n", interface);
+g_assert_not_reached();
+}
+
+static void *qvhost_user_blk_device_get_driver(void *object,
+   const char *interface)
+{
+QVhostUserBlkDevice *v_blk = object;
+return qvhost_user_blk_get_driver(_blk->blk, interface);
+}
+
+static void *vhost_user_blk_device_create(void *virtio_dev,
+  QGuestAllocator *t_alloc,
+  void *addr)
+{
+QVhostUserBlkDevice *vhost_user_blk = g_new0(QVhostUserBlkDevice, 1);
+QVhostUserBlk *interface = _user_blk->blk;
+
+interface->vdev = virtio_dev;
+
+vhost_user_blk->obj.get_driver = qvhost_user_blk_device_get_driver;
+
+return _user_blk->obj;
+}
+
+/* virtio-blk-pci */
+static void *qvhost_user_blk_pci_get_driver(void *object, const char 
*interface)
+{
+QVhostUserBlkPCI *v_blk = object;
+if 

[PULL 31/31] docs: qsd: Explain --export nbd,name=... default

[Kevin Wolf]

The 'name' option for NBD exports is optional. Add a note that the
default for the option is the node name (people could otherwise expect
that it's the empty string like for qemu-nbd).

Signed-off-by: Kevin Wolf 
Message-Id: <20210305094856.18964-1-kw...@redhat.com>
Reviewed-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 docs/tools/qemu-storage-daemon.rst | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/tools/qemu-storage-daemon.rst 
b/docs/tools/qemu-storage-daemon.rst
index fe3042d609..086493ebb3 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -80,8 +80,9 @@ Standard options:
   requests for modifying data (the default is off).
 
   The ``nbd`` export type requires ``--nbd-server`` (see below). ``name`` is
-  the NBD export name. ``bitmap`` is the name of a dirty bitmap reachable from
-  the block node, so the NBD client can use NBD_OPT_SET_META_CONTEXT with the
+  the NBD export name (if not specified, it defaults to the given
+  ``node-name``). ``bitmap`` is the name of a dirty bitmap reachable from the
+  block node, so the NBD client can use NBD_OPT_SET_META_CONTEXT with the
   metadata context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap.
 
   The ``vhost-user-blk`` export type takes a vhost-user socket address on which
-- 
2.29.2

[PULL 03/31] backup-top: Refuse I/O in inactive state

[Kevin Wolf]

From: Max Reitz 

When the backup-top node transitions from active to inactive in
bdrv_backup_top_drop(), the BlockCopyState is freed and the filtered
child is removed, so the node effectively becomes unusable.

However, noone told its I/O functions this, so they will happily
continue accessing bs->backing and s->bcs.  Prevent that by aborting
early when s->active is false.

(After the preceding patch, the node should be gone after
bdrv_backup_top_drop(), so this should largely be a theoretical problem.
But still, better to be safe than sorry, and also I think it just makes
sense to check s->active in the I/O functions.)

Signed-off-by: Max Reitz 
Message-Id: <20210219153348.41861-3-mre...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 block/backup-top.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/block/backup-top.c b/block/backup-top.c
index d1253e1aa6..589e8b651d 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -45,6 +45,12 @@ static coroutine_fn int backup_top_co_preadv(
 BlockDriverState *bs, uint64_t offset, uint64_t bytes,
 QEMUIOVector *qiov, int flags)
 {
+BDRVBackupTopState *s = bs->opaque;
+
+if (!s->active) {
+return -EIO;
+}
+
 return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
 }
 
@@ -54,6 +60,10 @@ static coroutine_fn int backup_top_cbw(BlockDriverState *bs, 
uint64_t offset,
 BDRVBackupTopState *s = bs->opaque;
 uint64_t off, end;
 
+if (!s->active) {
+return -EIO;
+}
+
 if (flags & BDRV_REQ_WRITE_UNCHANGED) {
 return 0;
 }
-- 
2.29.2

[PULL 19/31] block/export: use VIRTIO_BLK_SECTOR_BITS

[Kevin Wolf]

From: Stefan Hajnoczi 

Use VIRTIO_BLK_SECTOR_BITS and VIRTIO_BLK_SECTOR_SIZE when dealing with
virtio-blk sector numbers. Although the values happen to be the same as
BDRV_SECTOR_BITS and BDRV_SECTOR_SIZE, they are conceptually different.
This makes it clearer when we are dealing with virtio-blk sector units.

Use VIRTIO_BLK_SECTOR_BITS in vu_blk_initialize_config(). Later patches
will use it the new constants the virtqueue request processing code
path.

Suggested-by: Max Reitz 
Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210223144653.811468-9-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 block/export/vhost-user-blk-server.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index 7aea132f69..2614a63791 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -20,6 +20,13 @@
 #include "sysemu/block-backend.h"
 #include "util/block-helpers.h"
 
+/*
+ * Sector units are 512 bytes regardless of the
+ * virtio_blk_config->blk_size value.
+ */
+#define VIRTIO_BLK_SECTOR_BITS 9
+#define VIRTIO_BLK_SECTOR_SIZE (1ull << VIRTIO_BLK_SECTOR_BITS)
+
 enum {
 VHOST_USER_BLK_NUM_QUEUES_DEFAULT = 1,
 };
@@ -347,7 +354,8 @@ vu_blk_initialize_config(BlockDriverState *bs,
  uint32_t blk_size,
  uint16_t num_queues)
 {
-config->capacity = cpu_to_le64(bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
+config->capacity =
+cpu_to_le64(bdrv_getlength(bs) >> VIRTIO_BLK_SECTOR_BITS);
 config->blk_size = cpu_to_le32(blk_size);
 config->size_max = cpu_to_le32(0);
 config->seg_max = cpu_to_le32(128 - 2);
@@ -356,7 +364,8 @@ vu_blk_initialize_config(BlockDriverState *bs,
 config->num_queues = cpu_to_le16(num_queues);
 config->max_discard_sectors = cpu_to_le32(32768);
 config->max_discard_seg = cpu_to_le32(1);
-config->discard_sector_alignment = cpu_to_le32(blk_size >> 9);
+config->discard_sector_alignment =
+cpu_to_le32(blk_size >> VIRTIO_BLK_SECTOR_BITS);
 config->max_write_zeroes_sectors = cpu_to_le32(32768);
 config->max_write_zeroes_seg = cpu_to_le32(1);
 }
@@ -383,7 +392,7 @@ static int vu_blk_exp_create(BlockExport *exp, 
BlockExportOptions *opts,
 if (vu_opts->has_logical_block_size) {
 logical_block_size = vu_opts->logical_block_size;
 } else {
-logical_block_size = BDRV_SECTOR_SIZE;
+logical_block_size = VIRTIO_BLK_SECTOR_SIZE;
 }
 check_block_size(exp->id, "logical-block-size", logical_block_size,
  _err);
-- 
2.29.2

[PULL 21/31] block/export: port virtio-blk discard/write zeroes input validation

[Kevin Wolf]

From: Stefan Hajnoczi 

Validate discard/write zeroes the same way we do for virtio-blk. Some of
these checks are mandated by the VIRTIO specification, others are
internal to QEMU.

Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210223144653.811468-11-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 block/export/vhost-user-blk-server.c | 116 +--
 1 file changed, 93 insertions(+), 23 deletions(-)

diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index f74796241c..04044228d4 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -29,6 +29,8 @@
 
 enum {
 VHOST_USER_BLK_NUM_QUEUES_DEFAULT = 1,
+VHOST_USER_BLK_MAX_DISCARD_SECTORS = 32768,
+VHOST_USER_BLK_MAX_WRITE_ZEROES_SECTORS = 32768,
 };
 struct virtio_blk_inhdr {
 unsigned char status;
@@ -65,30 +67,102 @@ static void vu_blk_req_complete(VuBlkReq *req)
 free(req);
 }
 
+static bool vu_blk_sect_range_ok(VuBlkExport *vexp, uint64_t sector,
+ size_t size)
+{
+uint64_t nb_sectors = size >> BDRV_SECTOR_BITS;
+uint64_t total_sectors;
+
+if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
+return false;
+}
+if ((sector << VIRTIO_BLK_SECTOR_BITS) % vexp->blk_size) {
+return false;
+}
+blk_get_geometry(vexp->export.blk, _sectors);
+if (sector > total_sectors || nb_sectors > total_sectors - sector) {
+return false;
+}
+return true;
+}
+
 static int coroutine_fn
-vu_blk_discard_write_zeroes(BlockBackend *blk, struct iovec *iov,
+vu_blk_discard_write_zeroes(VuBlkExport *vexp, struct iovec *iov,
 uint32_t iovcnt, uint32_t type)
 {
+BlockBackend *blk = vexp->export.blk;
 struct virtio_blk_discard_write_zeroes desc;
-ssize_t size = iov_to_buf(iov, iovcnt, 0, , sizeof(desc));
+ssize_t size;
+uint64_t sector;
+uint32_t num_sectors;
+uint32_t max_sectors;
+uint32_t flags;
+int bytes;
+
+/* Only one desc is currently supported */
+if (unlikely(iov_size(iov, iovcnt) > sizeof(desc))) {
+return VIRTIO_BLK_S_UNSUPP;
+}
+
+size = iov_to_buf(iov, iovcnt, 0, , sizeof(desc));
 if (unlikely(size != sizeof(desc))) {
-error_report("Invalid size %zd, expect %zu", size, sizeof(desc));
-return -EINVAL;
+error_report("Invalid size %zd, expected %zu", size, sizeof(desc));
+return VIRTIO_BLK_S_IOERR;
 }
 
-uint64_t range[2] = { le64_to_cpu(desc.sector) << 9,
-  le32_to_cpu(desc.num_sectors) << 9 };
-if (type == VIRTIO_BLK_T_DISCARD) {
-if (blk_co_pdiscard(blk, range[0], range[1]) == 0) {
-return 0;
+sector = le64_to_cpu(desc.sector);
+num_sectors = le32_to_cpu(desc.num_sectors);
+flags = le32_to_cpu(desc.flags);
+max_sectors = (type == VIRTIO_BLK_T_WRITE_ZEROES) ?
+  VHOST_USER_BLK_MAX_WRITE_ZEROES_SECTORS :
+  VHOST_USER_BLK_MAX_DISCARD_SECTORS;
+
+/* This check ensures that 'bytes' fits in an int */
+if (unlikely(num_sectors > max_sectors)) {
+return VIRTIO_BLK_S_IOERR;
+}
+
+bytes = num_sectors << VIRTIO_BLK_SECTOR_BITS;
+
+if (unlikely(!vu_blk_sect_range_ok(vexp, sector, bytes))) {
+return VIRTIO_BLK_S_IOERR;
+}
+
+/*
+ * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for discard
+ * and write zeroes commands if any unknown flag is set.
+ */
+if (unlikely(flags & ~VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) {
+return VIRTIO_BLK_S_UNSUPP;
+}
+
+if (type == VIRTIO_BLK_T_WRITE_ZEROES) {
+int blk_flags = 0;
+
+if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
+blk_flags |= BDRV_REQ_MAY_UNMAP;
+}
+
+if (blk_co_pwrite_zeroes(blk, sector << VIRTIO_BLK_SECTOR_BITS,
+ bytes, blk_flags) == 0) {
+return VIRTIO_BLK_S_OK;
 }
-} else if (type == VIRTIO_BLK_T_WRITE_ZEROES) {
-if (blk_co_pwrite_zeroes(blk, range[0], range[1], 0) == 0) {
-return 0;
+} else if (type == VIRTIO_BLK_T_DISCARD) {
+/*
+ * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for
+ * discard commands if the unmap flag is set.
+ */
+if (unlikely(flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) {
+return VIRTIO_BLK_S_UNSUPP;
+}
+
+if (blk_co_pdiscard(blk, sector << VIRTIO_BLK_SECTOR_BITS,
+bytes) == 0) {
+return VIRTIO_BLK_S_OK;
 }
 }
 
-return -EINVAL;
+return VIRTIO_BLK_S_IOERR;
 }
 
 static void coroutine_fn vu_blk_virtio_process_req(void *opaque)
@@ -177,19 +251,13 @@ static void coroutine_fn vu_blk_virtio_process_req(void 
*opaque)
 }
 case VIRTIO_BLK_T_DISCARD:
 case VIRTIO_BLK_T_WRITE_ZEROES: {
-int rc;
-
 if (!vexp->writable) 

[PULL 24/31] qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy 

Rename bytes_covered_by_bitmap_cluster() to
bdrv_dirty_bitmap_serialization_coverage() and make it public.
It is needed as we are going to share it with bitmap loading in
parallels format.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Denis V. Lunev 
Message-Id: <20210224104707.88430-2-vsement...@virtuozzo.com>
Signed-off-by: Kevin Wolf 
---
 include/block/dirty-bitmap.h |  2 ++
 block/dirty-bitmap.c | 13 +
 block/qcow2-bitmap.c | 16 ++--
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 36e8da4fc2..f581cf9fd7 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -57,6 +57,8 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
 uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
   uint64_t offset, uint64_t bytes);
 uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap);
+uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
+const BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
   uint8_t *buf, uint64_t offset,
   uint64_t bytes);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9b9cd71238..a0eaa28785 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -726,6 +726,19 @@ uint64_t bdrv_dirty_bitmap_serialization_align(const 
BdrvDirtyBitmap *bitmap)
 return hbitmap_serialization_align(bitmap->bitmap);
 }
 
+/* Return the disk size covered by a chunk of serialized bitmap data. */
+uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
+  const BdrvDirtyBitmap 
*bitmap)
+{
+uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+uint64_t limit = granularity * (serialized_chunk_size << 3);
+
+assert(QEMU_IS_ALIGNED(limit,
+   bdrv_dirty_bitmap_serialization_align(bitmap)));
+return limit;
+}
+
+
 void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
   uint8_t *buf, uint64_t offset,
   uint64_t bytes)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 5eef82fa55..42d81c44cd 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -278,18 +278,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, 
Qcow2BitmapTable *tb)
 return 0;
 }
 
-/* Return the disk size covered by a single qcow2 cluster of bitmap data. */
-static uint64_t bytes_covered_by_bitmap_cluster(const BDRVQcow2State *s,
-const BdrvDirtyBitmap *bitmap)
-{
-uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
-uint64_t limit = granularity * (s->cluster_size << 3);
-
-assert(QEMU_IS_ALIGNED(limit,
-   bdrv_dirty_bitmap_serialization_align(bitmap)));
-return limit;
-}
-
 /* load_bitmap_data
  * @bitmap_table entries must satisfy specification constraints.
  * @bitmap must be cleared */
@@ -312,7 +300,7 @@ static int load_bitmap_data(BlockDriverState *bs,
 }
 
 buf = g_malloc(s->cluster_size);
-limit = bytes_covered_by_bitmap_cluster(s, bitmap);
+limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
 for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
 uint64_t count = MIN(bm_size - offset, limit);
 uint64_t entry = bitmap_table[i];
@@ -1303,7 +1291,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
 }
 
 buf = g_malloc(s->cluster_size);
-limit = bytes_covered_by_bitmap_cluster(s, bitmap);
+limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
 assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
 
 offset = 0;
-- 
2.29.2

[PULL 23/31] block/export: port virtio-blk read/write range check

[Kevin Wolf]

From: Stefan Hajnoczi 

Check that the sector number and byte count are valid.

Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210223144653.811468-13-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 block/export/vhost-user-blk-server.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index 04044228d4..cb5d896b7b 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -209,6 +209,8 @@ static void coroutine_fn vu_blk_virtio_process_req(void 
*opaque)
 switch (type & ~VIRTIO_BLK_T_BARRIER) {
 case VIRTIO_BLK_T_IN:
 case VIRTIO_BLK_T_OUT: {
+QEMUIOVector qiov;
+int64_t offset;
 ssize_t ret = 0;
 bool is_write = type & VIRTIO_BLK_T_OUT;
 req->sector_num = le64_to_cpu(req->out.sector);
@@ -218,13 +220,24 @@ static void coroutine_fn vu_blk_virtio_process_req(void 
*opaque)
 break;
 }
 
-int64_t offset = req->sector_num << VIRTIO_BLK_SECTOR_BITS;
-QEMUIOVector qiov;
 if (is_write) {
 qemu_iovec_init_external(, out_iov, out_num);
-ret = blk_co_pwritev(blk, offset, qiov.size, , 0);
 } else {
 qemu_iovec_init_external(, in_iov, in_num);
+}
+
+if (unlikely(!vu_blk_sect_range_ok(vexp,
+   req->sector_num,
+   qiov.size))) {
+req->in->status = VIRTIO_BLK_S_IOERR;
+break;
+}
+
+offset = req->sector_num << VIRTIO_BLK_SECTOR_BITS;
+
+if (is_write) {
+ret = blk_co_pwritev(blk, offset, qiov.size, , 0);
+} else {
 ret = blk_co_preadv(blk, offset, qiov.size, , 0);
 }
 if (ret >= 0) {
-- 
2.29.2

[PULL 09/31] qemu-storage-daemon: add --pidfile option

[Kevin Wolf]

From: Stefan Hajnoczi 

Daemons often have a --pidfile option where the pid is written to a file
so that scripts can stop the daemon by sending a signal.

The pid file also acts as a lock to prevent multiple instances of the
daemon from launching for a given pid file.

QEMU, qemu-nbd, qemu-ga, virtiofsd, and qemu-pr-helper all support the
--pidfile option. Add it to qemu-storage-daemon too.

Reported-by: Richard W.M. Jones 
Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210302142746.170535-1-stefa...@redhat.com>
Reviewed-by: Richard W.M. Jones 
Signed-off-by: Kevin Wolf 
---
 docs/tools/qemu-storage-daemon.rst   | 14 +++
 storage-daemon/qemu-storage-daemon.c | 36 
 2 files changed, 50 insertions(+)

diff --git a/docs/tools/qemu-storage-daemon.rst 
b/docs/tools/qemu-storage-daemon.rst
index c05b3d3811..6ce85f2f7d 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -118,6 +118,20 @@ Standard options:
   List object properties with ``,help``. See the :manpage:`qemu(1)`
   manual page for a description of the object properties.
 
+.. option:: --pidfile PATH
+
+  is the path to a file where the daemon writes its pid. This allows scripts to
+  stop the daemon by sending a signal::
+
+$ kill -SIGTERM $(   write process ID to a file after startup\n"
+"\n"
 QEMU_HELP_BOTTOM "\n",
 error_get_progname());
 }
@@ -126,6 +129,7 @@ enum {
 OPTION_MONITOR,
 OPTION_NBD_SERVER,
 OPTION_OBJECT,
+OPTION_PIDFILE,
 };
 
 extern QemuOptsList qemu_chardev_opts;
@@ -178,6 +182,7 @@ static void process_options(int argc, char *argv[])
 {"monitor", required_argument, NULL, OPTION_MONITOR},
 {"nbd-server", required_argument, NULL, OPTION_NBD_SERVER},
 {"object", required_argument, NULL, OPTION_OBJECT},
+{"pidfile", required_argument, NULL, OPTION_PIDFILE},
 {"trace", required_argument, NULL, 'T'},
 {"version", no_argument, NULL, 'V'},
 {0, 0, 0, 0}
@@ -289,6 +294,9 @@ static void process_options(int argc, char *argv[])
 qobject_unref(args);
 break;
 }
+case OPTION_PIDFILE:
+pid_file = optarg;
+break;
 case 1:
 error_report("Unexpected argument");
 exit(EXIT_FAILURE);
@@ -299,6 +307,27 @@ static void process_options(int argc, char *argv[])
 loc_set_none();
 }
 
+static void pid_file_cleanup(void)
+{
+unlink(pid_file);
+}
+
+static void pid_file_init(void)
+{
+Error *err = NULL;
+
+if (!pid_file) {
+return;
+}
+
+if (!qemu_write_pidfile(pid_file, )) {
+error_reportf_err(err, "cannot create PID file: ");
+exit(EXIT_FAILURE);
+}
+
+atexit(pid_file_cleanup);
+}
+
 int main(int argc, char *argv[])
 {
 #ifdef CONFIG_POSIX
@@ -326,6 +355,13 @@ int main(int argc, char *argv[])
 qemu_init_main_loop(_fatal);
 process_options(argc, argv);
 
+/*
+ * Write the pid file after creating chardevs, exports, and NBD servers but
+ * before accepting connections. This ordering is documented. Do not change
+ * it.
+ */
+pid_file_init();
+
 while (!exit_requested) {
 main_loop_wait(false);
 }
-- 
2.29.2

[PULL 22/31] vhost-user-blk-test: test discard/write zeroes invalid inputs

[Kevin Wolf]

From: Stefan Hajnoczi 

Exercise input validation code paths in
block/export/vhost-user-blk-server.c.

Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210223144653.811468-12-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 tests/qtest/vhost-user-blk-test.c | 124 ++
 1 file changed, 124 insertions(+)

diff --git a/tests/qtest/vhost-user-blk-test.c 
b/tests/qtest/vhost-user-blk-test.c
index 61beee52d3..dc9d7a31ae 100644
--- a/tests/qtest/vhost-user-blk-test.c
+++ b/tests/qtest/vhost-user-blk-test.c
@@ -94,6 +94,124 @@ static uint64_t virtio_blk_request(QGuestAllocator *alloc, 
QVirtioDevice *d,
 return addr;
 }
 
+static void test_invalid_discard_write_zeroes(QVirtioDevice *dev,
+  QGuestAllocator *alloc,
+  QTestState *qts,
+  QVirtQueue *vq,
+  uint32_t type)
+{
+QVirtioBlkReq req;
+struct virtio_blk_discard_write_zeroes dwz_hdr;
+struct virtio_blk_discard_write_zeroes dwz_hdr2[2];
+uint64_t req_addr;
+uint32_t free_head;
+uint8_t status;
+
+/* More than one dwz is not supported */
+req.type = type;
+req.data = (char *) dwz_hdr2;
+dwz_hdr2[0].sector = 0;
+dwz_hdr2[0].num_sectors = 1;
+dwz_hdr2[0].flags = 0;
+dwz_hdr2[1].sector = 1;
+dwz_hdr2[1].num_sectors = 1;
+dwz_hdr2[1].flags = 0;
+
+virtio_blk_fix_dwz_hdr(dev, _hdr2[0]);
+virtio_blk_fix_dwz_hdr(dev, _hdr2[1]);
+
+req_addr = virtio_blk_request(alloc, dev, , sizeof(dwz_hdr2));
+
+free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true);
+qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr2), false, true);
+qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr2), 1, true,
+   false);
+
+qvirtqueue_kick(qts, dev, vq, free_head);
+
+qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL,
+   QVIRTIO_BLK_TIMEOUT_US);
+status = readb(req_addr + 16 + sizeof(dwz_hdr2));
+g_assert_cmpint(status, ==, VIRTIO_BLK_S_UNSUPP);
+
+guest_free(alloc, req_addr);
+
+/* num_sectors must be less than config->max_write_zeroes_sectors */
+req.type = type;
+req.data = (char *) _hdr;
+dwz_hdr.sector = 0;
+dwz_hdr.num_sectors = 0x;
+dwz_hdr.flags = 0;
+
+virtio_blk_fix_dwz_hdr(dev, _hdr);
+
+req_addr = virtio_blk_request(alloc, dev, , sizeof(dwz_hdr));
+
+free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true);
+qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr), false, true);
+qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr), 1, true,
+   false);
+
+qvirtqueue_kick(qts, dev, vq, free_head);
+
+qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL,
+   QVIRTIO_BLK_TIMEOUT_US);
+status = readb(req_addr + 16 + sizeof(dwz_hdr));
+g_assert_cmpint(status, ==, VIRTIO_BLK_S_IOERR);
+
+guest_free(alloc, req_addr);
+
+/* sector must be less than the device capacity */
+req.type = type;
+req.data = (char *) _hdr;
+dwz_hdr.sector = TEST_IMAGE_SIZE / 512 + 1;
+dwz_hdr.num_sectors = 1;
+dwz_hdr.flags = 0;
+
+virtio_blk_fix_dwz_hdr(dev, _hdr);
+
+req_addr = virtio_blk_request(alloc, dev, , sizeof(dwz_hdr));
+
+free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true);
+qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr), false, true);
+qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr), 1, true,
+   false);
+
+qvirtqueue_kick(qts, dev, vq, free_head);
+
+qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL,
+   QVIRTIO_BLK_TIMEOUT_US);
+status = readb(req_addr + 16 + sizeof(dwz_hdr));
+g_assert_cmpint(status, ==, VIRTIO_BLK_S_IOERR);
+
+guest_free(alloc, req_addr);
+
+/* reserved flag bits must be zero */
+req.type = type;
+req.data = (char *) _hdr;
+dwz_hdr.sector = 0;
+dwz_hdr.num_sectors = 1;
+dwz_hdr.flags = ~VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP;
+
+virtio_blk_fix_dwz_hdr(dev, _hdr);
+
+req_addr = virtio_blk_request(alloc, dev, , sizeof(dwz_hdr));
+
+free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true);
+qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr), false, true);
+qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr), 1, true,
+   false);
+
+qvirtqueue_kick(qts, dev, vq, free_head);
+
+qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL,
+   QVIRTIO_BLK_TIMEOUT_US);
+status = readb(req_addr + 16 + sizeof(dwz_hdr));
+g_assert_cmpint(status, ==, VIRTIO_BLK_S_UNSUPP);
+
+guest_free(alloc, req_addr);
+}
+
 /* Returns the request virtqueue so the caller can perform further tests */
 static QVirtQueue *test_basic(QVirtioDevice *dev, QGuestAllocator *alloc)
 {
@@ -235,6 

[PULL 07/31] storage-daemon: report unexpected arguments on the fly

[Kevin Wolf]

From: Paolo Bonzini 

If the first character of optstring is '-', then each nonoption argv
element is handled as if it were the argument of an option with character
code 1.  This removes the reordering of the argv array, and enables usage
of loc_set_cmdline to provide better error messages.

Signed-off-by: Paolo Bonzini 
Message-Id: <20210301152844.291799-2-pbonz...@redhat.com>
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 storage-daemon/qemu-storage-daemon.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/storage-daemon/qemu-storage-daemon.c 
b/storage-daemon/qemu-storage-daemon.c
index 9021a46b3a..b7e1b90fb1 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -174,7 +174,7 @@ static void process_options(int argc, char *argv[])
  * they are given on the command lines. This means that things must be
  * defined first before they can be referenced in another option.
  */
-while ((c = getopt_long(argc, argv, "hT:V", long_options, NULL)) != -1) {
+while ((c = getopt_long(argc, argv, "-hT:V", long_options, NULL)) != -1) {
 switch (c) {
 case '?':
 exit(EXIT_FAILURE);
@@ -275,14 +275,13 @@ static void process_options(int argc, char *argv[])
 qobject_unref(args);
 break;
 }
+case 1:
+error_report("Unexpected argument: %s", optarg);
+exit(EXIT_FAILURE);
 default:
 g_assert_not_reached();
 }
 }
-if (optind != argc) {
-error_report("Unexpected argument: %s", argv[optind]);
-exit(EXIT_FAILURE);
-}
 }
 
 int main(int argc, char *argv[])
-- 
2.29.2

[PULL 04/31] iotests/283: Check that finalize drops backup-top

[Kevin Wolf]

From: Max Reitz 

Without any of HEAD^ or HEAD^^ applied, qemu will most likely crash on
the qemu-io invocation, for a variety of immediate reasons.  The
underlying problem is generally a use-after-free access into
backup-top's BlockCopyState.

With only HEAD^ applied, qemu-io will run into an EIO (which is not
capture by the output, but you can see that the qemu-io invocation will
be accepted (i.e., qemu-io will run) in contrast to the reference
output, where the node name cannot be found), and qemu will then crash
in query-named-block-nodes: bdrv_get_allocated_file_size() detects
backup-top to be a filter and passes the request through to its child.
However, after bdrv_backup_top_drop(), that child is NULL, so the
recursive call crashes.

With HEAD^^ applied, this test should pass.

Signed-off-by: Max Reitz 
Message-Id: <20210219153348.41861-4-mre...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/283 | 53 ++
 tests/qemu-iotests/283.out | 15 +++
 2 files changed, 68 insertions(+)

diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
index 79643e375b..010c22f0a2 100755
--- a/tests/qemu-iotests/283
+++ b/tests/qemu-iotests/283
@@ -97,3 +97,56 @@ vm.qmp_log('blockdev-add', **{
 vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
 
 vm.shutdown()
+
+
+print('\n=== backup-top should be gone after job-finalize ===\n')
+
+# Check that the backup-top node is gone after job-finalize.
+#
+# During finalization, the node becomes inactive and can no longer
+# function.  If it is still present, new parents might be attached, and
+# there would be no meaningful way to handle their I/O requests.
+
+vm = iotests.VM()
+vm.launch()
+
+vm.qmp_log('blockdev-add', **{
+'node-name': 'source',
+'driver': 'null-co',
+})
+
+vm.qmp_log('blockdev-add', **{
+'node-name': 'target',
+'driver': 'null-co',
+})
+
+vm.qmp_log('blockdev-backup',
+   job_id='backup',
+   device='source',
+   target='target',
+   sync='full',
+   filter_node_name='backup-filter',
+   auto_finalize=False,
+   auto_dismiss=False)
+
+vm.event_wait('BLOCK_JOB_PENDING', 5.0)
+
+# The backup-top filter should still be present prior to finalization
+assert vm.node_info('backup-filter') is not None
+
+vm.qmp_log('job-finalize', id='backup')
+vm.event_wait('BLOCK_JOB_COMPLETED', 5.0)
+
+# The filter should be gone now.  Check that by trying to access it
+# with qemu-io (which will most likely crash qemu if it is still
+# there.).
+vm.qmp_log('human-monitor-command',
+   command_line='qemu-io backup-filter "write 0 1M"')
+
+# (Also, do an explicit check.)
+assert vm.node_info('backup-filter') is None
+
+vm.qmp_log('job-dismiss', id='backup')
+vm.event_wait('JOB_STATUS_CHANGE', 5.0, {'data': {'status': 'null'}})
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index d8cff22cc1..7e9cd9a7d4 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -6,3 +6,18 @@
 {"return": {}}
 {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": 
"full", "target": "target"}}
 {"error": {"class": "GenericError", "desc": "Cannot set permissions for 
backup-top filter: Conflicts with use by other as 'image', which uses 'write' 
on base"}}
+
+=== backup-top should be gone after job-finalize ===
+
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": 
"source"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": 
"target"}}
+{"return": {}}
+{"execute": "blockdev-backup", "arguments": {"auto-dismiss": false, 
"auto-finalize": false, "device": "source", "filter-node-name": 
"backup-filter", "job-id": "backup", "sync": "full", "target": "target"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "backup"}}
+{"return": {}}
+{"execute": "human-monitor-command", "arguments": {"command-line": "qemu-io 
backup-filter \"write 0 1M\""}}
+{"return": "Error: Cannot find device= nor node_name=backup-filter\r\n"}
+{"execute": "job-dismiss", "arguments": {"id": "backup"}}
+{"return": {}}
-- 
2.29.2

[PULL 11/31] docs: replace insecure /tmp examples in qsd docs

[Kevin Wolf]

From: Stefan Hajnoczi 

World-writeable directories have security issues. Avoid showing them in
the documentation since someone might accidentally use them in
situations where they are insecure.

There tend to be 3 security problems:
1. Denial of service. An adversary may be able to create the file
   beforehand, consume all space/inodes, etc to sabotage us.
2. Impersonation. An adversary may be able to create a listen socket and
   accept incoming connections that were meant for us.
3. Unauthenticated client access. An adversary may be able to connect to
   us if we did not set the uid/gid and permissions correctly.

These can be prevented or mitigated with private /tmp, carefully setting
the umask, etc but that requires special action and does not apply to
all situations. Just avoid using /tmp in examples.

Reported-by: Richard W.M. Jones 
Reported-by: Daniel P. Berrangé 
Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210301172728.135331-3-stefa...@redhat.com>
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Richard W.M. Jones 
Signed-off-by: Kevin Wolf 
---
 docs/tools/qemu-storage-daemon.rst | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/docs/tools/qemu-storage-daemon.rst 
b/docs/tools/qemu-storage-daemon.rst
index 5714794775..fe3042d609 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -69,7 +69,7 @@ Standard options:
   a description of character device properties. A common character device
   definition configures a UNIX domain socket::
 
-  --chardev socket,id=char1,path=/tmp/qmp.sock,server=on,wait=off
+  --chardev socket,id=char1,path=/var/run/qsd-qmp.sock,server=on,wait=off
 
 .. option:: --export 
[type=]nbd,id=,node-name=[,name=][,writable=on|off][,bitmap=]
   --export 
[type=]vhost-user-blk,id=,node-name=,addr.type=unix,addr.path=[,writable=on|off][,logical-block-size=][,num-queues=]
@@ -108,9 +108,10 @@ Standard options:
   below). TLS encryption can be configured using ``--object`` tls-creds-* and
   authz-* secrets (see below).
 
-  To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
+  To configure an NBD server on UNIX domain socket path
+  ``/var/run/qsd-nbd.sock``::
 
-  --nbd-server addr.type=unix,addr.path=/tmp/nbd.sock
+  --nbd-server addr.type=unix,addr.path=/var/run/qsd-nbd.sock
 
 .. option:: --object help
   --object ,help
-- 
2.29.2

[PULL 28/31] iotests.py: add unarchive_sample_image() helper

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy 

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20210224104707.88430-6-vsement...@virtuozzo.com>
Reviewed-by: Denis V. Lunev 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4e758308f2..90d0b62523 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -17,6 +17,7 @@
 #
 
 import atexit
+import bz2
 from collections import OrderedDict
 import faulthandler
 import io
@@ -24,6 +25,7 @@
 import logging
 import os
 import re
+import shutil
 import signal
 import struct
 import subprocess
@@ -96,6 +98,14 @@
  os.environ.get('IMGKEYSECRET', '')
 luks_default_key_secret_opt = 'key-secret=keysec0'
 
+sample_img_dir = os.environ['SAMPLE_IMG_DIR']
+
+
+def unarchive_sample_image(sample, fname):
+sample_fname = os.path.join(sample_img_dir, sample + '.bz2')
+with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out:
+shutil.copyfileobj(f_in, f_out)
+
 
 def qemu_tool_pipe_and_status(tool: str, args: Sequence[str],
   connect_stderr: bool = True) -> Tuple[str, int]:
-- 
2.29.2

[PULL 02/31] backup: Remove nodes from job in .clean()

[Kevin Wolf]

From: Max Reitz 

The block job holds a reference to the backup-top node (because it is
passed as the main job BDS to block_job_create()).  Therefore,
bdrv_backup_top_drop() cannot delete the backup-top node (replacing it
by its child does not affect the job parent, because that has
.stay_at_node set).  That is a problem, because all of its I/O functions
assume the BlockCopyState (s->bcs) to be valid and that it has a
filtered child; but after bdrv_backup_top_drop(), neither of those
things are true.

It does not make sense to add new parents to backup-top after
backup_clean(), so we should detach it from the job before
bdrv_backup_top_drop().  Because there is no function to do that for a
single node, just detach all of the job's nodes -- the job does not do
anything past backup_clean() anyway.

Signed-off-by: Max Reitz 
Message-Id: <20210219153348.41861-2-mre...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 block/backup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/backup.c b/block/backup.c
index 94e6dcd72e..6cf2f974aa 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -103,6 +103,7 @@ static void backup_abort(Job *job)
 static void backup_clean(Job *job)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
+block_job_remove_all_bdrv(>common);
 bdrv_backup_top_drop(s->backup_top);
 }
 
-- 
2.29.2

[PULL 25/31] parallels.txt: fix bitmap L1 table description

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy 

Actually L1 table entry offset is in 512 bytes sectors. Fix the spec.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20210224104707.88430-3-vsement...@virtuozzo.com>
Reviewed-by: Denis V. Lunev 
Signed-off-by: Kevin Wolf 
---
 docs/interop/parallels.txt | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/docs/interop/parallels.txt b/docs/interop/parallels.txt
index f15bf35bd1..bb3fadf369 100644
--- a/docs/interop/parallels.txt
+++ b/docs/interop/parallels.txt
@@ -208,21 +208,25 @@ of its data area are:
   28 - 31:l1_size
   The number of entries in the L1 table of the bitmap.
 
-  variable:   l1_table (8 * l1_size bytes)
-  L1 offset table (in bytes)
+  variable:   L1 offset table (l1_table), size: 8 * l1_size bytes
 
-A dirty bitmap is stored using a one-level structure for the mapping to host
-clusters - an L1 table.
+The dirty bitmap described by this feature extension is stored in a set of
+clusters inside the Parallels image file. The offsets of these clusters are
+saved in the L1 offset table specified by the feature extension. Each L1 table
+entry is a 64 bit integer as described below:
 
-Given an offset in bytes into the bitmap data, the offset in bytes into the
-image file can be obtained as follows:
+Given an offset in bytes into the bitmap data, corresponding L1 entry is
 
-offset = l1_table[offset / cluster_size] + (offset % cluster_size)
+l1_table[offset / cluster_size]
 
-If an L1 table entry is 0, the corresponding cluster of the bitmap is assumed
-to be zero.
+If an L1 table entry is 0, all bits in the corresponding cluster of the bitmap
+are assumed to be 0.
 
-If an L1 table entry is 1, the corresponding cluster of the bitmap is assumed
-to have all bits set.
+If an L1 table entry is 1, all bits in the corresponding cluster of the bitmap
+are assumed to be 1.
 
-If an L1 table entry is not 0 or 1, it allocates a cluster from the data area.
+If an L1 table entry is not 0 or 1, it contains the corresponding cluster
+offset (in 512b sectors). Given an offset in bytes into the bitmap data the
+offset in bytes into the image file can be obtained as follows:
+
+offset = l1_table[offset / cluster_size] * 512 + (offset % cluster_size)
-- 
2.29.2

[PULL 17/31] tests/qtest: add multi-queue test case to vhost-user-blk-test

[Kevin Wolf]

From: Stefan Hajnoczi 

Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210223144653.811468-7-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 tests/qtest/vhost-user-blk-test.c | 81 +--
 1 file changed, 76 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/vhost-user-blk-test.c 
b/tests/qtest/vhost-user-blk-test.c
index f0fb09893e..61beee52d3 100644
--- a/tests/qtest/vhost-user-blk-test.c
+++ b/tests/qtest/vhost-user-blk-test.c
@@ -563,6 +563,67 @@ static void pci_hotplug(void *obj, void *data, 
QGuestAllocator *t_alloc)
 qpci_unplug_acpi_device_test(qts, "drv1", PCI_SLOT_HP);
 }
 
+static void multiqueue(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtioPCIDevice *pdev1 = obj;
+QVirtioDevice *dev1 = >vdev;
+QVirtioPCIDevice *pdev8;
+QVirtioDevice *dev8;
+QTestState *qts = pdev1->pdev->bus->qts;
+uint64_t features;
+uint16_t num_queues;
+
+/*
+ * The primary device has 1 queue and VIRTIO_BLK_F_MQ is not enabled. The
+ * VIRTIO specification allows VIRTIO_BLK_F_MQ to be enabled when there is
+ * only 1 virtqueue, but --device vhost-user-blk-pci doesn't do this (which
+ * is also spec-compliant).
+ */
+features = qvirtio_get_features(dev1);
+g_assert_cmpint(features & (1u << VIRTIO_BLK_F_MQ), ==, 0);
+features = features & ~(QVIRTIO_F_BAD_FEATURE |
+(1u << VIRTIO_RING_F_INDIRECT_DESC) |
+(1u << VIRTIO_F_NOTIFY_ON_EMPTY) |
+(1u << VIRTIO_BLK_F_SCSI));
+qvirtio_set_features(dev1, features);
+
+/* Hotplug a secondary device with 8 queues */
+qtest_qmp_device_add(qts, "vhost-user-blk-pci", "drv1",
+ "{'addr': %s, 'chardev': 'char2', 'num-queues': 8}",
+ stringify(PCI_SLOT_HP) ".0");
+
+pdev8 = virtio_pci_new(pdev1->pdev->bus,
+   &(QPCIAddress) {
+   .devfn = QPCI_DEVFN(PCI_SLOT_HP, 0)
+   });
+g_assert_nonnull(pdev8);
+g_assert_cmpint(pdev8->vdev.device_type, ==, VIRTIO_ID_BLOCK);
+
+qos_object_start_hw(>obj);
+
+dev8 = >vdev;
+features = qvirtio_get_features(dev8);
+g_assert_cmpint(features & (1u << VIRTIO_BLK_F_MQ),
+==,
+(1u << VIRTIO_BLK_F_MQ));
+features = features & ~(QVIRTIO_F_BAD_FEATURE |
+(1u << VIRTIO_RING_F_INDIRECT_DESC) |
+(1u << VIRTIO_F_NOTIFY_ON_EMPTY) |
+(1u << VIRTIO_BLK_F_SCSI) |
+(1u << VIRTIO_BLK_F_MQ));
+qvirtio_set_features(dev8, features);
+
+num_queues = qvirtio_config_readw(dev8,
+offsetof(struct virtio_blk_config, num_queues));
+g_assert_cmpint(num_queues, ==, 8);
+
+qvirtio_pci_device_disable(pdev8);
+qos_object_destroy(>obj);
+
+/* unplug secondary disk */
+qpci_unplug_acpi_device_test(qts, "drv1", PCI_SLOT_HP);
+}
+
 /*
  * Check that setting the vring addr on a non-existent virtqueue does
  * not crash.
@@ -682,7 +743,8 @@ static void quit_storage_daemon(void *data)
 g_free(data);
 }
 
-static void start_vhost_user_blk(GString *cmd_line, int vus_instances)
+static void start_vhost_user_blk(GString *cmd_line, int vus_instances,
+ int num_queues)
 {
 const char *vhost_user_blk_bin = qtest_qemu_storage_daemon_binary();
 int i;
@@ -707,8 +769,8 @@ static void start_vhost_user_blk(GString *cmd_line, int 
vus_instances)
 g_string_append_printf(storage_daemon_command,
 "--blockdev driver=file,node-name=disk%d,filename=%s "
 "--export 
type=vhost-user-blk,id=disk%d,addr.type=unix,addr.path=%s,"
-"node-name=disk%i,writable=on ",
-i, img_path, i, sock_path, i);
+"node-name=disk%i,writable=on,num-queues=%d ",
+i, img_path, i, sock_path, i, num_queues);
 
 g_string_append_printf(cmd_line, "-chardev socket,id=char%d,path=%s ",
i + 1, sock_path);
@@ -742,7 +804,7 @@ static void start_vhost_user_blk(GString *cmd_line, int 
vus_instances)
 
 static void *vhost_user_blk_test_setup(GString *cmd_line, void *arg)
 {
-start_vhost_user_blk(cmd_line, 1);
+start_vhost_user_blk(cmd_line, 1, 1);
 return arg;
 }
 
@@ -756,7 +818,13 @@ static void *vhost_user_blk_test_setup(GString *cmd_line, 
void *arg)
 static void *vhost_user_blk_hotplug_test_setup(GString *cmd_line, void *arg)
 {
 /* "-chardev socket,id=char2" is used for pci_hotplug*/
-start_vhost_user_blk(cmd_line, 2);
+start_vhost_user_blk(cmd_line, 2, 1);
+return arg;
+}
+
+static void *vhost_user_blk_multiqueue_test_setup(GString *cmd_line, void *arg)
+{
+start_vhost_user_blk(cmd_line, 2, 8);
 return arg;
 }
 
@@ -783,6 +851,9 @@ static void register_vhost_user_blk_test(void)
 
 opts.before = 

[PULL 29/31] iotests: add parallels-read-bitmap test

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy 

Test support for reading bitmap from parallels image format.
parallels-with-bitmap.bz2 is generated on Virtuozzo by
parallels-with-bitmap.sh

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20210224104707.88430-7-vsement...@virtuozzo.com>
Reviewed-by: Denis V. Lunev 
Signed-off-by: Kevin Wolf 
---
 .../sample_images/parallels-with-bitmap.bz2   | Bin 0 -> 203 bytes
 .../sample_images/parallels-with-bitmap.sh|  51 
 .../qemu-iotests/tests/parallels-read-bitmap  |  55 ++
 .../tests/parallels-read-bitmap.out   |   6 ++
 4 files changed, 112 insertions(+)
 create mode 100644 tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
 create mode 100755 tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
 create mode 100755 tests/qemu-iotests/tests/parallels-read-bitmap
 create mode 100644 tests/qemu-iotests/tests/parallels-read-bitmap.out

diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2 
b/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
new file mode 100644
index 
..54892fd4d01bf743d395bd4f3d896494146ab5a9
GIT binary patch
literal 203
zcmV;+05tzXT4*^jL0KkKS@=;0bpT+Hf7|^?KmNPH-R
Fx`3oHQ9u9y

literal 0
HcmV?d1

diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh 
b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
new file mode 100755
index 00..30615aa6bd
--- /dev/null
+++ b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
@@ -0,0 +1,51 @@
+#!/bin/bash
+#
+# Test parallels load bitmap
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+CT=parallels-with-bitmap-ct
+DIR=$PWD/parallels-with-bitmap-dir
+IMG=$DIR/root.hds
+XML=$DIR/DiskDescriptor.xml
+TARGET=parallels-with-bitmap.bz2
+
+rm -rf $DIR
+
+prlctl create $CT --vmtype ct
+prlctl set $CT --device-add hdd --image $DIR --recreate --size 2G
+
+# cleanup the image
+qemu-img create -f parallels $IMG 64G
+
+# create bitmap
+prlctl backup $CT
+
+prlctl set $CT --device-del hdd1
+prlctl destroy $CT
+
+dev=$(ploop mount $XML | sed -n 's/^Adding delta 
dev=\(\/dev\/ploop[0-9]\+\).*/\1/p')
+dd if=/dev/zero of=$dev bs=64K seek=5 count=2 oflag=direct
+dd if=/dev/zero of=$dev bs=64K seek=30 count=1 oflag=direct
+dd if=/dev/zero of=$dev bs=64K seek=10 count=3 oflag=direct
+ploop umount $XML  # bitmap name will be in the output
+
+bzip2 -z $IMG
+
+mv $IMG.bz2 $TARGET
+
+rm -rf $DIR
diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap 
b/tests/qemu-iotests/tests/parallels-read-bitmap
new file mode 100755
index 00..af6b9c5db3
--- /dev/null
+++ b/tests/qemu-iotests/tests/parallels-read-bitmap
@@ -0,0 +1,55 @@
+#!/usr/bin/env python3
+#
+# Test parallels load bitmap
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import json
+import iotests
+from iotests import qemu_nbd_popen, qemu_img_pipe, log, file_path
+
+iotests.script_initialize(supported_fmts=['parallels'])
+
+nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
+disk = iotests.file_path('disk')
+bitmap = 'e4f2eed0-37fe-4539-b50b-85d2e7fd235f'
+nbd_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}' \
+f',x-dirty-bitmap=qemu:dirty-bitmap:{bitmap}'
+
+
+iotests.unarchive_sample_image('parallels-with-bitmap', disk)
+
+
+with qemu_nbd_popen('--read-only', f'--socket={nbd_sock}',
+f'--bitmap={bitmap}', '-f', iotests.imgfmt, disk):
+out = qemu_img_pipe('map', '--output=json', '--image-opts', nbd_opts)
+chunks = json.loads(out)
+cluster = 64 * 1024
+
+log('dirty clusters (cluster size is 64K):')
+for c in chunks:
+assert c['start'] 

[PULL 12/31] vhost-user-blk: fix blkcfg->num_queues endianness

[Kevin Wolf]

From: Stefan Hajnoczi 

Treat the num_queues field as virtio-endian. On big-endian hosts the
vhost-user-blk num_queues field was in the wrong endianness.

Move the blkcfg.num_queues store operation from realize to
vhost_user_blk_update_config() so feature negotiation has finished and
we know the endianness of the device. VIRTIO 1.0 devices are
little-endian, but in case someone wants to use legacy VIRTIO we support
all endianness cases.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Raphael Norwitz 
Reviewed-by: Michael S. Tsirkin 
Message-Id: <20210223144653.811468-2-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 hw/block/vhost-user-blk.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index da4fbf9084..b870a50e6b 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -54,6 +54,9 @@ static void vhost_user_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
 {
 VHostUserBlk *s = VHOST_USER_BLK(vdev);
 
+/* Our num_queues overrides the device backend */
+virtio_stw_p(vdev, >blkcfg.num_queues, s->num_queues);
+
 memcpy(config, >blkcfg, sizeof(struct virtio_blk_config));
 }
 
@@ -491,10 +494,6 @@ reconnect:
 goto reconnect;
 }
 
-if (s->blkcfg.num_queues != s->num_queues) {
-s->blkcfg.num_queues = s->num_queues;
-}
-
 return;
 
 virtio_err:
-- 
2.29.2

[PULL 08/31] storage-daemon: include current command line option in the errors

[Kevin Wolf]

From: Paolo Bonzini 

Use the location management facilities that the emulator uses, so that
the current command line option appears in the error message.

Before:

  $ storage-daemon/qemu-storage-daemon --nbd key..=
  qemu-storage-daemon: Invalid parameter 'key..'

After:

  $ storage-daemon/qemu-storage-daemon --nbd key..=
  qemu-storage-daemon: --nbd key..=: Invalid parameter 'key..'

Reviewed-by: Eric Blake 
Signed-off-by: Paolo Bonzini 
Message-Id: <20210301152844.291799-3-pbonz...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 storage-daemon/qemu-storage-daemon.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/storage-daemon/qemu-storage-daemon.c 
b/storage-daemon/qemu-storage-daemon.c
index b7e1b90fb1..78ddf619d4 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -152,6 +152,20 @@ static void init_qmp_commands(void)
  qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
 }
 
+static int getopt_set_loc(int argc, char **argv, const char *optstring,
+  const struct option *longopts)
+{
+int c, save_index;
+
+optarg = NULL;
+save_index = optind;
+c = getopt_long(argc, argv, optstring, longopts, NULL);
+if (optarg) {
+loc_set_cmdline(argv, save_index, MAX(1, optind - save_index));
+}
+return c;
+}
+
 static void process_options(int argc, char *argv[])
 {
 int c;
@@ -174,7 +188,7 @@ static void process_options(int argc, char *argv[])
  * they are given on the command lines. This means that things must be
  * defined first before they can be referenced in another option.
  */
-while ((c = getopt_long(argc, argv, "-hT:V", long_options, NULL)) != -1) {
+while ((c = getopt_set_loc(argc, argv, "-hT:V", long_options)) != -1) {
 switch (c) {
 case '?':
 exit(EXIT_FAILURE);
@@ -276,12 +290,13 @@ static void process_options(int argc, char *argv[])
 break;
 }
 case 1:
-error_report("Unexpected argument: %s", optarg);
+error_report("Unexpected argument");
 exit(EXIT_FAILURE);
 default:
 g_assert_not_reached();
 }
 }
+loc_set_none();
 }
 
 int main(int argc, char *argv[])
-- 
2.29.2

[PULL 15/31] libqtest: add qtest_remove_abrt_handler()

[Kevin Wolf]

From: Stefan Hajnoczi 

Add a function to remove previously-added abrt handler functions.

Now that a symmetric pair of add/remove functions exists we can also
balance the SIGABRT handler installation. The signal handler was
installed each time qtest_add_abrt_handler() was called. Now it is
installed when the abrt handler list becomes non-empty and removed again
when the list becomes empty.

The qtest_remove_abrt_handler() function will be used by
vhost-user-blk-test.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Wainer dos Santos Moschetta 
Message-Id: <20210223144653.811468-5-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 tests/qtest/libqos/libqtest.h | 18 ++
 tests/qtest/libqtest.c| 35 +--
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
index 51287b9276..a68dcd79d4 100644
--- a/tests/qtest/libqos/libqtest.h
+++ b/tests/qtest/libqos/libqtest.h
@@ -649,8 +649,26 @@ void qtest_add_data_func_full(const char *str, void *data,
 g_free(path); \
 } while (0)
 
+/**
+ * qtest_add_abrt_handler:
+ * @fn: Handler function
+ * @data: Argument that is passed to the handler
+ *
+ * Add a handler function that is invoked on SIGABRT. This can be used to
+ * terminate processes and perform other cleanup. The handler can be removed
+ * with qtest_remove_abrt_handler().
+ */
 void qtest_add_abrt_handler(GHookFunc fn, const void *data);
 
+/**
+ * qtest_remove_abrt_handler:
+ * @data: Argument previously passed to qtest_add_abrt_handler()
+ *
+ * Remove an abrt handler that was previously added with
+ * qtest_add_abrt_handler().
+ */
+void qtest_remove_abrt_handler(void *data);
+
 /**
  * qtest_qmp_assert_success:
  * @qts: QTestState instance to operate on
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 2a98de2907..71e359efcd 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -196,15 +196,30 @@ static void cleanup_sigabrt_handler(void)
 sigaction(SIGABRT, _old, NULL);
 }
 
+static bool hook_list_is_empty(GHookList *hook_list)
+{
+GHook *hook = g_hook_first_valid(hook_list, TRUE);
+
+if (!hook) {
+return false;
+}
+
+g_hook_unref(hook_list, hook);
+return true;
+}
+
 void qtest_add_abrt_handler(GHookFunc fn, const void *data)
 {
 GHook *hook;
 
-/* Only install SIGABRT handler once */
 if (!abrt_hooks.is_setup) {
 g_hook_list_init(_hooks, sizeof(GHook));
 }
-setup_sigabrt_handler();
+
+/* Only install SIGABRT handler once */
+if (hook_list_is_empty(_hooks)) {
+setup_sigabrt_handler();
+}
 
 hook = g_hook_alloc(_hooks);
 hook->func = fn;
@@ -213,6 +228,17 @@ void qtest_add_abrt_handler(GHookFunc fn, const void *data)
 g_hook_prepend(_hooks, hook);
 }
 
+void qtest_remove_abrt_handler(void *data)
+{
+GHook *hook = g_hook_find_data(_hooks, TRUE, data);
+g_hook_destroy_link(_hooks, hook);
+
+/* Uninstall SIGABRT handler on last instance */
+if (hook_list_is_empty(_hooks)) {
+cleanup_sigabrt_handler();
+}
+}
+
 static const char *qtest_qemu_binary(void)
 {
 const char *qemu_bin;
@@ -369,10 +395,7 @@ QTestState *qtest_init_with_serial(const char *extra_args, 
int *sock_fd)
 
 void qtest_quit(QTestState *s)
 {
-g_hook_destroy_link(_hooks, g_hook_find_data(_hooks, TRUE, s));
-
-/* Uninstall SIGABRT handler on last instance */
-cleanup_sigabrt_handler();
+qtest_remove_abrt_handler(s);
 
 qtest_kill_qemu(s);
 close(s->fd);
-- 
2.29.2

[PULL 06/31] blockjob: report a better error message

[Kevin Wolf]

From: Stefano Garzarella 

When a block job fails, we report strerror(-job->job.ret) error
message, also if the job set an error object.
Let's report a better error message using error_get_pretty(job->job.err).

If an error object was not set, strerror(-job->ret) is used as fallback,
as explained in include/qemu/job.h:

typedef struct Job {
...
/**
 * Error object for a failed job.
 * If job->ret is nonzero and an error object was not set, it will be set
 * to strerror(-job->ret) during job_completed.
 */
Error *err;
}

In block_job_query() there can be a transient where 'job.err' is not set
by a scheduled bottom half. In that case we use strerror(-job->ret) as it
was before.

Suggested-by: Kevin Wolf 
Signed-off-by: Stefano Garzarella 
Message-Id: <20210225103633.76746-1-sgarz...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 blockjob.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index f2feff051d..ef968017a2 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -318,8 +318,12 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
 info->status= job->job.status;
 info->auto_finalize = job->job.auto_finalize;
 info->auto_dismiss  = job->job.auto_dismiss;
-info->has_error = job->job.ret != 0;
-info->error = job->job.ret ? g_strdup(strerror(-job->job.ret)) : NULL;
+if (job->job.ret) {
+info->has_error = true;
+info->error = job->job.err ?
+g_strdup(error_get_pretty(job->job.err)) :
+g_strdup(strerror(-job->job.ret));
+}
 return info;
 }
 
@@ -356,7 +360,7 @@ static void block_job_event_completed(Notifier *n, void 
*opaque)
 }
 
 if (job->job.ret < 0) {
-msg = strerror(-job->job.ret);
+msg = error_get_pretty(job->job.err);
 }
 
 qapi_event_send_block_job_completed(job_type(>job),
-- 
2.29.2

[PULL 20/31] block/export: fix vhost-user-blk export sector number calculation

[Kevin Wolf]

From: Stefan Hajnoczi 

The driver is supposed to honor the blk_size field but the protocol
still uses 512-byte sector numbers. It is incorrect to multiply
req->sector_num by blk_size.

VIRTIO 1.1 5.2.5 Device Initialization says:

  blk_size can be read to determine the optimal sector size for the
  driver to use. This does not affect the units used in the protocol
  (always 512 bytes), but awareness of the correct value can affect
  performance.

Fixes: 3578389bcf76c824a5d82e6586a6f0c71e56f2aa ("block/export: vhost-user 
block device backend server")
Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210223144653.811468-10-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 block/export/vhost-user-blk-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index 2614a63791..f74796241c 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -144,7 +144,7 @@ static void coroutine_fn vu_blk_virtio_process_req(void 
*opaque)
 break;
 }
 
-int64_t offset = req->sector_num * vexp->blk_size;
+int64_t offset = req->sector_num << VIRTIO_BLK_SECTOR_BITS;
 QEMUIOVector qiov;
 if (is_write) {
 qemu_iovec_init_external(, out_iov, out_num);
-- 
2.29.2

[PULL 00/31] Block layer patches

[Kevin Wolf]

The following changes since commit 9a7beaad3dbba982f7a461d676b55a5c3851d312:

  Merge remote-tracking branch 
'remotes/alistair/tags/pull-riscv-to-apply-20210304' into staging (2021-03-05 
10:47:46 +)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 67bedc3aed5c455b629c2cb5f523b536c46adff9:

  docs: qsd: Explain --export nbd,name=... default (2021-03-05 17:09:46 +0100)


Block layer patches:

- qemu-storage-daemon: add --pidfile option
- qemu-storage-daemon: CLI error messages include the option name now
- vhost-user-blk export: Misc fixes, added test cases
- docs: Improvements for qemu-storage-daemon documentation
- parallels: load bitmap extension
- backup-top: Don't crash on post-finalize accesses
- iotests improvements


Alberto Garcia (1):
  iotests: Drop deprecated 'props' from object-add

Coiby Xu (1):
  test: new qTest case to test the vhost-user-blk-server

Eric Blake (1):
  iotests: Fix up python style in 300

Kevin Wolf (1):
  docs: qsd: Explain --export nbd,name=... default

Max Reitz (3):
  backup: Remove nodes from job in .clean()
  backup-top: Refuse I/O in inactive state
  iotests/283: Check that finalize drops backup-top

Paolo Bonzini (2):
  storage-daemon: report unexpected arguments on the fly
  storage-daemon: include current command line option in the errors

Stefan Hajnoczi (14):
  qemu-storage-daemon: add --pidfile option
  docs: show how to spawn qemu-storage-daemon with fd passing
  docs: replace insecure /tmp examples in qsd docs
  vhost-user-blk: fix blkcfg->num_queues endianness
  libqtest: add qtest_socket_server()
  libqtest: add qtest_kill_qemu()
  libqtest: add qtest_remove_abrt_handler()
  tests/qtest: add multi-queue test case to vhost-user-blk-test
  block/export: fix blk_size double byteswap
  block/export: use VIRTIO_BLK_SECTOR_BITS
  block/export: fix vhost-user-blk export sector number calculation
  block/export: port virtio-blk discard/write zeroes input validation
  vhost-user-blk-test: test discard/write zeroes invalid inputs
  block/export: port virtio-blk read/write range check

Stefano Garzarella (1):
  blockjob: report a better error message

Vladimir Sementsov-Ogievskiy (7):
  qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public
  parallels.txt: fix bitmap L1 table description
  block/parallels: BDRVParallelsState: add cluster_size field
  parallels: support bitmap extension for read-only mode
  iotests.py: add unarchive_sample_image() helper
  iotests: add parallels-read-bitmap test
  MAINTAINERS: update parallels block driver

 docs/interop/parallels.txt |  28 +-
 docs/tools/qemu-storage-daemon.rst |  68 +-
 block/parallels.h  |   7 +-
 include/block/dirty-bitmap.h   |   2 +
 tests/qtest/libqos/libqtest.h  |  37 +
 tests/qtest/libqos/vhost-user-blk.h|  48 +
 block/backup-top.c |  10 +
 block/backup.c |   1 +
 block/dirty-bitmap.c   |  13 +
 block/export/vhost-user-blk-server.c   | 150 +++-
 block/parallels-ext.c  | 300 +++
 block/parallels.c  |  26 +-
 block/qcow2-bitmap.c   |  16 +-
 blockjob.c |  10 +-
 hw/block/vhost-user-blk.c  |   7 +-
 storage-daemon/qemu-storage-daemon.c   |  56 +-
 tests/qtest/libqos/vhost-user-blk.c| 130 +++
 tests/qtest/libqtest.c |  82 +-
 tests/qtest/vhost-user-blk-test.c  | 983 +
 tests/qemu-iotests/iotests.py  |  10 +
 MAINTAINERS|   5 +
 block/meson.build  |   3 +-
 tests/qemu-iotests/087 |   8 +-
 tests/qemu-iotests/184 |  18 +-
 tests/qemu-iotests/218 |   2 +-
 tests/qemu-iotests/235 |   2 +-
 tests/qemu-iotests/245 |   4 +-
 tests/qemu-iotests/258 |   6 +-
 tests/qemu-iotests/258.out |   4 +-
 tests/qemu-iotests/283 |  53 ++
 tests/qemu-iotests/283.out |  15 +
 tests/qemu-iotests/295 |   2 +-
 tests/qemu-iotests/296 |   2 +-
 tests/qemu-iotests/300 |  10 +-
 .../sample_images/parallels-with-bitmap.bz2  

[PULL 05/31] iotests: Fix up python style in 300

[Kevin Wolf]

From: Eric Blake 

Break some long lines, and relax our type hints to be more generic to
any JSON, in order to more easily permit the additional JSON depth now
possible in migration parameters.  Detected by iotest 297.

Fixes: ca4bfec41d56
 (qemu-iotests: 300: Add test case for modifying persistence of bitmap)
Reported-by: Kevin Wolf 
Signed-off-by: Eric Blake 
Message-Id: <20210215220518.1745469-1-ebl...@redhat.com>
Reviewed-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/300 | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 63036f6a6e..adb9276297 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -22,7 +22,7 @@
 import os
 import random
 import re
-from typing import Dict, List, Optional, Union
+from typing import Dict, List, Optional
 
 import iotests
 
@@ -30,7 +30,7 @@ import iotests
 # pylint: disable=wrong-import-order
 import qemu
 
-BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]
+BlockBitmapMapping = List[Dict[str, object]]
 
 mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
 
@@ -602,7 +602,8 @@ class TestCrossAliasMigration(TestDirtyBitmapMigration):
 
 class TestAliasTransformMigration(TestDirtyBitmapMigration):
 """
-Tests the 'transform' option which modifies bitmap persistence on 
migration.
+Tests the 'transform' option which modifies bitmap persistence on
+migration.
 """
 
 src_node_name = 'node-a'
@@ -674,7 +675,8 @@ class TestAliasTransformMigration(TestDirtyBitmapMigration):
 bitmaps = self.vm_b.query_bitmaps()
 
 for node in bitmaps:
-bitmaps[node] = sorted(((bmap['name'], bmap['persistent']) for 
bmap in bitmaps[node]))
+bitmaps[node] = sorted(((bmap['name'], bmap['persistent'])
+for bmap in bitmaps[node]))
 
 self.assertEqual(bitmaps,
  {'node-a': [('bmap-a', True), ('bmap-b', False)],
-- 
2.29.2

[PULL 10/31] docs: show how to spawn qemu-storage-daemon with fd passing

[Kevin Wolf]

From: Stefan Hajnoczi 

The QMP monitor, NBD server, and vhost-user-blk export all support file
descriptor passing. This is a useful technique because it allows the
parent process to spawn and wait for qemu-storage-daemon without busy
waiting, which may delay startup due to arbitrary sleep() calls.

This Python example is inspired by the test case written for libnbd by
Richard W.M. Jones :
https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543

Thanks to Daniel P. Berrangé  for suggestions on
how to get this working. Now let's document it!

Reported-by: Richard W.M. Jones 
Cc: Kevin Wolf 
Cc: Daniel P. Berrangé 
Signed-off-by: Stefan Hajnoczi 
Message-Id: <20210301172728.135331-2-stefa...@redhat.com>
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Richard W.M. Jones 
Signed-off-by: Kevin Wolf 
---
 docs/tools/qemu-storage-daemon.rst | 42 --
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/docs/tools/qemu-storage-daemon.rst 
b/docs/tools/qemu-storage-daemon.rst
index 6ce85f2f7d..5714794775 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -101,10 +101,12 @@ Standard options:
 
 .. option:: --nbd-server 
addr.type=inet,addr.host=,addr.port=[,tls-creds=][,tls-authz=][,max-connections=]
   --nbd-server 
addr.type=unix,addr.path=[,tls-creds=][,tls-authz=][,max-connections=]
+  --nbd-server 
addr.type=fd,addr.str=[,tls-creds=][,tls-authz=][,max-connections=]
 
   is a server for NBD exports. Both TCP and UNIX domain sockets are supported.
-  TLS encryption can be configured using ``--object`` tls-creds-* and authz-*
-  secrets (see below).
+  A listen socket can be provided via file descriptor passing (see Examples
+  below). TLS encryption can be configured using ``--object`` tls-creds-* and
+  authz-* secrets (see below).
 
   To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::
 
@@ -141,6 +143,42 @@ QMP commands::
   --chardev socket,path=qmp.sock,server=on,wait=off,id=char1 \
   --monitor chardev=char1
 
+Launch the daemon from Python with a QMP monitor socket using file descriptor
+passing so there is no need to busy wait for the QMP monitor to become
+available::
+
+  #!/usr/bin/env python3
+  import subprocess
+  import socket
+
+  sock_path = '/var/run/qmp.sock'
+
+  with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as listen_sock:
+  listen_sock.bind(sock_path)
+  listen_sock.listen()
+
+  fd = listen_sock.fileno()
+
+  subprocess.Popen(
+  ['qemu-storage-daemon',
+   '--chardev', f'socket,fd={fd},server=on,id=char1',
+   '--monitor', 'chardev=char1'],
+  pass_fds=[fd],
+  )
+
+  # listen_sock was automatically closed when leaving the 'with' statement
+  # body. If the daemon process terminated early then the following connect()
+  # will fail with "Connection refused" because no process has the listen
+  # socket open anymore. Launch errors can be detected this way.
+
+  qmp_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+  qmp_sock.connect(sock_path)
+  ...QMP interaction...
+
+The same socket spawning approach also works with the ``--nbd-server
+addr.type=fd,addr.str=`` and ``--export
+type=vhost-user-blk,addr.type=fd,addr.str=`` options.
+
 Export raw image file ``disk.img`` over NBD UNIX domain socket ``nbd.sock``::
 
   $ qemu-storage-daemon \
-- 
2.29.2

[PULL 13/31] libqtest: add qtest_socket_server()

[Kevin Wolf]

From: Stefan Hajnoczi 

Add an API that returns a new UNIX domain socket in the listen state.
The code for this was already there but only used internally in
init_socket().

This new API will be used by vhost-user-blk-test.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Thomas Huth 
Reviewed-by: Wainer dos Santos Moschetta 
Message-Id: <20210223144653.811468-3-stefa...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 tests/qtest/libqos/libqtest.h |  8 +++
 tests/qtest/libqtest.c| 40 ---
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
index 724f65aa94..e5f1ec590c 100644
--- a/tests/qtest/libqos/libqtest.h
+++ b/tests/qtest/libqos/libqtest.h
@@ -132,6 +132,14 @@ void qtest_qmp_send(QTestState *s, const char *fmt, ...)
 void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...)
 GCC_FMT_ATTR(2, 3);
 
+/**
+ * qtest_socket_server:
+ * @socket_path: the UNIX domain socket path
+ *
+ * Create and return a listen socket file descriptor, or abort on failure.
+ */
+int qtest_socket_server(const char *socket_path);
+
 /**
  * qtest_vqmp_fds:
  * @s: #QTestState instance to operate on.
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index fd043b0570..b19d2ebda0 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -81,24 +81,8 @@ static void qtest_client_set_rx_handler(QTestState *s, 
QTestRecvFn recv);
 
 static int init_socket(const char *socket_path)
 {
-struct sockaddr_un addr;
-int sock;
-int ret;
-
-sock = socket(PF_UNIX, SOCK_STREAM, 0);
-g_assert_cmpint(sock, !=, -1);
-
-addr.sun_family = AF_UNIX;
-snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);
+int sock = qtest_socket_server(socket_path);
 qemu_set_cloexec(sock);
-
-do {
-ret = bind(sock, (struct sockaddr *), sizeof(addr));
-} while (ret == -1 && errno == EINTR);
-g_assert_cmpint(ret, !=, -1);
-ret = listen(sock, 1);
-g_assert_cmpint(ret, !=, -1);
-
 return sock;
 }
 
@@ -638,6 +622,28 @@ QDict *qtest_qmp_receive_dict(QTestState *s)
 return qmp_fd_receive(s->qmp_fd);
 }
 
+int qtest_socket_server(const char *socket_path)
+{
+struct sockaddr_un addr;
+int sock;
+int ret;
+
+sock = socket(PF_UNIX, SOCK_STREAM, 0);
+g_assert_cmpint(sock, !=, -1);
+
+addr.sun_family = AF_UNIX;
+snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);
+
+do {
+ret = bind(sock, (struct sockaddr *), sizeof(addr));
+} while (ret == -1 && errno == EINTR);
+g_assert_cmpint(ret, !=, -1);
+ret = listen(sock, 1);
+g_assert_cmpint(ret, !=, -1);
+
+return sock;
+}
+
 /**
  * Allow users to send a message without waiting for the reply,
  * in the case that they choose to discard all replies up until
-- 
2.29.2

[PULL 01/31] iotests: Drop deprecated 'props' from object-add

[Kevin Wolf]

From: Alberto Garcia 

Signed-off-by: Alberto Garcia 
Message-Id: <20210222115737.2993-1-be...@igalia.com>
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/087 |  8 ++--
 tests/qemu-iotests/184 | 18 ++
 tests/qemu-iotests/218 |  2 +-
 tests/qemu-iotests/235 |  2 +-
 tests/qemu-iotests/245 |  4 ++--
 tests/qemu-iotests/258 |  6 +++---
 tests/qemu-iotests/258.out |  4 ++--
 tests/qemu-iotests/295 |  2 +-
 tests/qemu-iotests/296 |  2 +-
 9 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index edd43f1a28..d8e0e384cd 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -143,9 +143,7 @@ run_qemu <

Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[John Snow]

On 3/5/21 11:50 AM, Vladimir Sementsov-Ogievskiy wrote:

05.03.2021 19:30, John Snow wrote:

On 3/5/21 4:11 AM, Vladimir Sementsov-Ogievskiy wrote:

05.03.2021 04:30, John Snow wrote:

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:

It probably may improve reliability of results when testing in cached
mode.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench_block_job.py | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/scripts/simplebench/bench_block_job.py 
b/scripts/simplebench/bench_block_job.py

index 4f03c12169..fa45ad2655 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
  return {'error': 'qemu failed: ' + str(vm.get_log())}
  try:
+    subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', 
shell=True,

+   check=True)
  res = vm.qmp(cmd, **cmd_args)
  if res != {'return': {}}:
  vm.shutdown()



Worth adding a conditional to allow "hot" or "cold" runs? nah?



You mean, make this addition optional? Make sense




I was thinking (along the lines of allowing both old and new behavior, 
in case anyone except you used these scripts) of this sort of thing:


def bench_block_job(cmd, cmd_args, qemu_args, drop_cache=True): ...

I don't insist on it; I was just earnestly wondering if it had any 
utility. If it doesn't, don't respin on my account.




Ok, thanks a lot for reviewing! Still, I think, I'll resend



Thanks for sharing your benchmarking scripts :)

--js

Re: [RESEND][BUG FIX HELP] QEMU main thread endlessly hangs in __ppoll()

[John Snow]

On 3/4/21 10:08 PM, Like Xu wrote:

Hi John,

Thanks for your comment.

On 2021/3/5 7:53, John Snow wrote:

On 2/28/21 9:39 PM, Like Xu wrote:

Hi Genius,

I am a user of QEMU v4.2.0 and stuck in an interesting bug, which may 
still exist in the mainline.

Thanks in advance to heroes who can take a look and share understanding.



Do you have a test case that reproduces on 5.2? It'd be nice to know 
if it was still a problem in the latest source tree or not.


We narrowed down the source of the bug, which basically came from
the following qmp usage:

{'execute': 'human-monitor-command', 'arguments':{ 'command-line': 
'drive_del replication0' } }


One of the test cases is the COLO usage (docs/colo-proxy.txt).

This issue is sporadic,the probability may be 1/15 for a io-heavy guest.

I believe it's reproducible on 5.2 and the latest tree.



Can you please test and confirm that this is the case, and then file a 
bug report on the LP: https://launchpad.net/qemu and include:


- The exact commit you used (current origin/master debug build would be 
the most ideal.)

- Which QEMU binary you are using (qemu-system-x86_64?)
- The shortest command line you are aware of that reproduces the problem
- The host OS and kernel version
- An updated call trace
- Any relevant commands issued prior to the one that caused the hang; or 
detailed reproduction steps if possible.


Thanks,
--js

Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[Vladimir Sementsov-Ogievskiy]

05.03.2021 19:30, John Snow wrote:

On 3/5/21 4:11 AM, Vladimir Sementsov-Ogievskiy wrote:

05.03.2021 04:30, John Snow wrote:

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:

It probably may improve reliability of results when testing in cached
mode.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench_block_job.py | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/scripts/simplebench/bench_block_job.py 
b/scripts/simplebench/bench_block_job.py
index 4f03c12169..fa45ad2655 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
  return {'error': 'qemu failed: ' + str(vm.get_log())}
  try:
+    subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', shell=True,
+   check=True)
  res = vm.qmp(cmd, **cmd_args)
  if res != {'return': {}}:
  vm.shutdown()



Worth adding a conditional to allow "hot" or "cold" runs? nah?



You mean, make this addition optional? Make sense




I was thinking (along the lines of allowing both old and new behavior, in case 
anyone except you used these scripts) of this sort of thing:

def bench_block_job(cmd, cmd_args, qemu_args, drop_cache=True): ...

I don't insist on it; I was just earnestly wondering if it had any utility. If 
it doesn't, don't respin on my account.



Ok, thanks a lot for reviewing! Still, I think, I'll resend

--
Best regards,
Vladimir

Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[John Snow]

On 3/5/21 4:11 AM, Vladimir Sementsov-Ogievskiy wrote:

05.03.2021 04:30, John Snow wrote:

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:

It probably may improve reliability of results when testing in cached
mode.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench_block_job.py | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/scripts/simplebench/bench_block_job.py 
b/scripts/simplebench/bench_block_job.py

index 4f03c12169..fa45ad2655 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
  return {'error': 'qemu failed: ' + str(vm.get_log())}
  try:
+    subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', 
shell=True,

+   check=True)
  res = vm.qmp(cmd, **cmd_args)
  if res != {'return': {}}:
  vm.shutdown()



Worth adding a conditional to allow "hot" or "cold" runs? nah?



You mean, make this addition optional? Make sense




I was thinking (along the lines of allowing both old and new behavior, 
in case anyone except you used these scripts) of this sort of thing:


def bench_block_job(cmd, cmd_args, qemu_args, drop_cache=True): ...

I don't insist on it; I was just earnestly wondering if it had any 
utility. If it doesn't, don't respin on my account.


--js

Re: [PATCH v2 1/8] simplebench: bench_one(): add slow_limit argument

[John Snow]

On 3/5/21 4:03 AM, Vladimir Sementsov-Ogievskiy wrote:

05.03.2021 04:22, John Snow wrote:

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:

Sometimes one of cells in a testing table runs too slow. And we really
don't want to wait so long. Limit number of runs in this case.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/simplebench.py | 29 +
  1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/scripts/simplebench/simplebench.py 
b/scripts/simplebench/simplebench.py

index f61513af90..b153cae274 100644
--- a/scripts/simplebench/simplebench.py
+++ b/scripts/simplebench/simplebench.py
@@ -19,9 +19,11 @@
  #
  import statistics
+import time
-def bench_one(test_func, test_env, test_case, count=5, 
initial_run=True):
+def bench_one(test_func, test_env, test_case, count=5, 
initial_run=True,

+  slow_limit=100):
  """Benchmark one test-case
  test_func   -- benchmarking function with prototype
@@ -36,6 +38,8 @@ def bench_one(test_func, test_env, test_case, 
count=5, initial_run=True):

  test_case   -- test case - opaque second argument for test_func
  count   -- how many times to call test_func, to calculate 
average
  initial_run -- do initial run of test_func, which don't get 
into result
+    slow_limit  -- reduce test runs to 2, if current run exceedes 
the limit

+   (in seconds)


s/exceedes/exceeds, and you need to mention that if the initial run 
exceeds the limit, it will change the behavior to count that result.


It is also possible (conceivably) that the initial run exceeds the 
limit, but subsequent runs don't, so it might be hard to predict how 
many tests it'll actually run.


If you're OK with that behavior, maybe:

"Consider a test run 'slow' once it exceeds this limit, in seconds.
  Stop early once there are two 'slow' runs, including the initial run.
  Slow initial runs will be included in the results."

Lastly, this will change existing behavior -- do we care? Should it 
default to None instead? Should we be able to pass None or 0 to 
disable this behavior?


For sure I don't care about changing the behavior. Consider simplebench 
in a version 0.0.1 :). Maybe, I should make a comment somewhere, but 
nobody will read it anyway.




Yep, it's yours anyway. Just thought I'd mention it. It's probably the 
case that you're the only person who actually uses this at the moment.


The aim of the patch is to minimize waiting for too long cells of the 
table, which are obviously too much longer then the others. Probably the 
logic should be improved a bit about ignoring or using initial-run result..

Like this:

If both initial and first run are slow, count both and stop here.
Otherwise, stop at first slow normal run and don't count initial run.

Or may be even

If both initial and first run are slow, count both and stop here.
Otherwise, behave the common way.



My opinion is that you can do whatever you'd like (you're the maintainer 
here!) but it'd be nice if the docstring was accurate. If changing the 
behavior makes it easier to write a good docstring, that's fine too. Go 
with whatever is most useful to you.


--js




  Returns dict with the following fields:
  'runs': list of test_func results
@@ -47,17 +51,34 @@ def bench_one(test_func, test_env, test_case, 
count=5, initial_run=True):
  'n-failed': number of failed runs (exists only if at least 
one run

  failed)
  """
+    runs = []
+    i = 0
  if initial_run:
+    t = time.time()
+
  print('  #initial run:')
-    print('   ', test_func(test_env, test_case))
+    res = test_func(test_env, test_case)
+    print('   ', res)
+
+    if time.time() - t > slow_limit:
+    print('    - initial run is too slow, so it counts')
+    runs.append(res)
+    i = 1
+
+    for i in range(i, count):
+    t = time.time()
-    runs = []
-    for i in range(count):
  print('  #run {}'.format(i+1))
  res = test_func(test_env, test_case)
  print('   ', res)
  runs.append(res)
+    if time.time() - t > slow_limit and len(runs) >= 2:
+    print('    - run is too slow, and we have enough runs, 
stop here')

+    break
+
+    count = len(runs)
+
  result = {'runs': runs}
  succeeded = [r for r in runs if ('seconds' in r or 'iops' in r)]

[PATCH v2 1/2] block: Clarify error messages pertaining to 'node-name'

[Connor Kuehl]

Some error messages contain ambiguous representations of the 'node-name'
parameter. This can be particularly confusing when exchanging QMP
messages (C = client, S = server):

C: {"execute": "block_resize", "arguments": { "device": "my_file", "size": 
26843545600 }}
S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor 
node_name="}}
   
^

This error message suggests one could send a message with a key called
'node_name':

C: {"execute": "block_resize", "arguments": { "node_name": "my_file", "size": 
26843545600 }}
   ^

but using the underscore is actually incorrect, the parameter should be
'node-name':

S: {"error": {"class": "GenericError", "desc": "Parameter 'node_name' is 
unexpected"}}

This behavior was uncovered in bz1651437, but I ended up going down a
rabbit hole looking for other areas where this miscommunication might
occur and changing those accordingly as well.

Fixes: https://bugzilla.redhat.com/1651437
Signed-off-by: Connor Kuehl 
---
 block.c   | 8 
 tests/qemu-iotests/030| 4 ++--
 tests/qemu-iotests/040| 4 ++--
 tests/qemu-iotests/051.pc.out | 6 +++---
 tests/qemu-iotests/081.out| 2 +-
 tests/qemu-iotests/085.out| 6 +++---
 tests/qemu-iotests/087.out| 2 +-
 tests/qemu-iotests/206.out| 2 +-
 tests/qemu-iotests/210.out| 2 +-
 tests/qemu-iotests/211.out| 2 +-
 tests/qemu-iotests/212.out| 2 +-
 tests/qemu-iotests/213.out| 2 +-
 tests/qemu-iotests/223.out| 4 ++--
 tests/qemu-iotests/237.out| 2 +-
 tests/qemu-iotests/245| 4 ++--
 tests/qemu-iotests/249.out| 2 +-
 tests/qemu-iotests/300| 4 ++--
 17 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/block.c b/block.c
index a1f3cecd75..2daff6d29a 100644
--- a/block.c
+++ b/block.c
@@ -1440,7 +1440,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
  * Check for empty string or invalid characters, but not if it is
  * generated (generated names use characters not available to the user)
  */
-error_setg(errp, "Invalid node name");
+error_setg(errp, "Invalid node-name: '%s'", node_name);
 return;
 }
 
@@ -1453,7 +1453,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
 
 /* takes care of avoiding duplicates node names */
 if (bdrv_find_node(node_name)) {
-error_setg(errp, "Duplicate node name");
+error_setg(errp, "Duplicate nodes with node-name='%s'", node_name);
 goto out;
 }
 
@@ -5432,7 +5432,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
 }
 }
 
-error_setg(errp, "Cannot find device=%s nor node_name=%s",
+error_setg(errp, "Cannot find device=\'%s\' nor node-name=\'%s\'",
  device ? device : "",
  node_name ? node_name : "");
 return NULL;
@@ -6752,7 +6752,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState 
*parent_bs,
 AioContext *aio_context;
 
 if (!to_replace_bs) {
-error_setg(errp, "Node name '%s' not found", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
 return NULL;
 }
 
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 12aa9ed37e..5fb65b4bef 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -153,7 +153,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 def test_device_not_found(self):
 result = self.vm.qmp('block-stream', device='nonexistent')
 self.assert_qmp(result, 'error/desc',
-'Cannot find device=nonexistent nor node_name=nonexistent')
+'Cannot find device=\'nonexistent\' nor node-name=\'nonexistent\'')
 
 def test_job_id_missing(self):
 result = self.vm.qmp('block-stream', device='mid')
@@ -507,7 +507,7 @@ class TestParallelOps(iotests.QMPTestCase):
 # Error: the base node does not exist
 result = self.vm.qmp('block-stream', device='node4', base_node='none', 
job_id='stream')
 self.assert_qmp(result, 'error/desc',
-'Cannot find device= nor node_name=none')
+'Cannot find device=\'\' nor node-name=\'none\'')
 
 # Error: the base node is not a backing file of the top node
 result = self.vm.qmp('block-stream', device='node4', 
base_node='node6', job_id='stream')
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 7ebc9ed825..336ff7c4f2 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -175,13 +175,13 @@ class TestSingleDrive(ImageCommitTestCase):
 self.assert_no_active_block_jobs()
 result = self.vm.qmp('block-commit', device='drive0', 
top_node='badfile', base_node='base')
 self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', "Cannot find 

[PATCH v2 2/2] blockdev: Clarify error messages pertaining to 'node-name'

[Connor Kuehl]

Signed-off-by: Connor Kuehl 
---
 blockdev.c | 13 +++--
 tests/qemu-iotests/245 |  6 +++---
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index cd438e60e3..7c7ab2b386 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1515,13 +1515,13 @@ static void external_snapshot_prepare(BlkActionState 
*common,
 s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
 
 if (node_name && !snapshot_node_name) {
-error_setg(errp, "New overlay node name missing");
+error_setg(errp, "New overlay node-name missing");
 goto out;
 }
 
 if (snapshot_node_name &&
 bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-error_setg(errp, "New overlay node name already in use");
+error_setg(errp, "New overlay node-name already in use");
 goto out;
 }
 
@@ -3598,13 +3598,14 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, 
Error **errp)
 
 /* Check for the selected node name */
 if (!options->has_node_name) {
-error_setg(errp, "Node name not specified");
+error_setg(errp, "node-name not specified");
 goto fail;
 }
 
 bs = bdrv_find_node(options->node_name);
 if (!bs) {
-error_setg(errp, "Cannot find node named '%s'", options->node_name);
+error_setg(errp, "Failed to find node with node-name='%s'",
+   options->node_name);
 goto fail;
 }
 
@@ -3635,7 +3636,7 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
 
 bs = bdrv_find_node(node_name);
 if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
 return;
 }
 if (bdrv_has_blk(bs)) {
@@ -3758,7 +3759,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name, 
StrOrNull *iothread,
 
 bs = bdrv_find_node(node_name);
 if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
 return;
 }
 
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index f8eba7719a..a2a0482469 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -140,8 +140,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.reopen(opts, {'file': 'hd0-file'})
 
 # We cannot change any of these
-self.reopen(opts, {'node-name': 'not-found'}, "Cannot find node named 
'not-found'")
-self.reopen(opts, {'node-name': ''}, "Cannot find node named ''")
+self.reopen(opts, {'node-name': 'not-found'}, "Failed to find node 
with node-name='not-found'")
+self.reopen(opts, {'node-name': ''}, "Failed to find node with 
node-name=''")
 self.reopen(opts, {'node-name': None}, "Invalid parameter type for 
'node-name', expected: string")
 self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 
'driver'")
 self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
@@ -158,7 +158,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 
 # node-name is optional in BlockdevOptions, but x-blockdev-reopen 
needs it
 del opts['node-name']
-self.reopen(opts, {}, "Node name not specified")
+self.reopen(opts, {}, "node-name not specified")
 
 # Check that nothing has changed
 self.check_node_graph(original_graph)
-- 
2.29.2

[PATCH v2 0/2] Clarify error messages pertaining to 'node-name'

[Connor Kuehl]

v2:
  - Moved summary into patch #1
  - Updated test cases that were missed in v1 from running 'make check'.
This time I used 'make check-block SPEED=thorough' and some more
grepping to make sure I didn't miss any.
  - Rebased

Connor Kuehl (2):
  block: Clarify error messages pertaining to 'node-name'
  blockdev: Clarify error messages pertaining to 'node-name'

 block.c   |  8 
 blockdev.c| 13 +++--
 tests/qemu-iotests/030|  4 ++--
 tests/qemu-iotests/040|  4 ++--
 tests/qemu-iotests/051.pc.out |  6 +++---
 tests/qemu-iotests/081.out|  2 +-
 tests/qemu-iotests/085.out|  6 +++---
 tests/qemu-iotests/087.out|  2 +-
 tests/qemu-iotests/206.out|  2 +-
 tests/qemu-iotests/210.out|  2 +-
 tests/qemu-iotests/211.out|  2 +-
 tests/qemu-iotests/212.out|  2 +-
 tests/qemu-iotests/213.out|  2 +-
 tests/qemu-iotests/223.out|  4 ++--
 tests/qemu-iotests/237.out|  2 +-
 tests/qemu-iotests/245| 10 +-
 tests/qemu-iotests/249.out|  2 +-
 tests/qemu-iotests/300|  4 ++--
 18 files changed, 39 insertions(+), 38 deletions(-)

-- 
2.29.2

Re: [PATCH] docs: qsd: Explain --export nbd,name=... default

[Max Reitz]

On 05.03.21 10:48, Kevin Wolf wrote:

The 'name' option for NBD exports is optional. Add a note that the
default for the option is the node name (people could otherwise expect
that it's the empty string like for qemu-nbd).

Signed-off-by: Kevin Wolf 
---
  docs/tools/qemu-storage-daemon.rst | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v3 2/5] monitor: drain requests queue with 'channel closed' event

[Vladimir Sementsov-Ogievskiy]

05.03.2021 16:41, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


02.03.2021 16:53, Markus Armbruster wrote:

Andrey Shinkevich via  writes:


When CHR_EVENT_CLOSED comes, the QMP requests queue may still contain
unprocessed commands. It can happen with QMP capability OOB enabled.
Let the dispatcher complete handling requests rest in the monitor
queue.

Signed-off-by: Andrey Shinkevich 
---
   monitor/qmp.c | 46 +-
   1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index 7169366..a86ed35 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -75,36 +75,32 @@ static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP 
*mon)
   }
   }
   
-static void monitor_qmp_cleanup_queue_and_resume(MonitorQMP *mon)

+/*
+ * Let unprocessed QMP commands be handled.
+ */
+static void monitor_qmp_drain_queue(MonitorQMP *mon)
   {
-qemu_mutex_lock(>qmp_queue_lock);
+bool q_is_empty = false;
   
-/*

- * Same condition as in monitor_qmp_dispatcher_co(), but before
- * removing an element from the queue (hence no `- 1`).
- * Also, the queue should not be empty either, otherwise the
- * monitor hasn't been suspended yet (or was already resumed).
- */
-bool need_resume = (!qmp_oob_enabled(mon) ||
-mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX)
-&& !g_queue_is_empty(mon->qmp_requests);
+while (!q_is_empty) {
+qemu_mutex_lock(>qmp_queue_lock);
+q_is_empty = g_queue_is_empty(mon->qmp_requests);
+qemu_mutex_unlock(>qmp_queue_lock);
   
-monitor_qmp_cleanup_req_queue_locked(mon);

+if (!q_is_empty) {
+if (!qatomic_xchg(_dispatcher_co_busy, true)) {
+/* Kick the dispatcher coroutine */
+aio_co_wake(qmp_dispatcher_co);
+} else {
+/* Let the dispatcher do its job for a while */
+g_usleep(40);
+}
+}
+}
   
-if (need_resume) {

-/*
- * handle_qmp_command() suspended the monitor because the
- * request queue filled up, to be resumed when the queue has
- * space again.  We just emptied it; resume the monitor.
- *
- * Without this, the monitor would remain suspended forever
- * when we get here while the monitor is suspended.  An
- * unfortunately timed CHR_EVENT_CLOSED can do the trick.
- */
+if (qatomic_mb_read(>common.suspend_cnt)) {
   monitor_resume(>common);
   }
-
-qemu_mutex_unlock(>qmp_queue_lock);
   }
   
   void qmp_send_response(MonitorQMP *mon, const QDict *rsp)

@@ -418,7 +414,7 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent 
event)
* stdio, it's possible that stdout is still open when stdin
* is closed.
*/
-monitor_qmp_cleanup_queue_and_resume(mon);
+monitor_qmp_drain_queue(mon);
   json_message_parser_destroy(>parser);
   json_message_parser_init(>parser, handle_qmp_command,
mon, NULL);


Before the patch: we call monitor_qmp_cleanup_queue_and_resume() to
throw away the contents of the request queue, and resume the monitor if
suspended.

Afterwards: we call monitor_qmp_drain_queue() to wait for the request
queue to drain.  I think.  Before we discuss the how, I have a question
the commit message should answer, but doesn't: why?



Hi!

Andrey is not in Virtuozzo now, and nobody doing this work actually.. Honestly, 
I don't believe that the feature should be so difficult.

Actually, we have the following patch in Virtuozzo 7 (Rhel7 based) for years, 
and it just works without any problems:


I appreciate your repeated efforts to get your downstream patch
upstream.


--- a/monitor.c
+++ b/monitor.c
@@ -4013,7 +4013,7 @@ static int monitor_can_read(void *opaque)
   {
   Monitor *mon = opaque;
   
-return !atomic_mb_read(>suspend_cnt);

+return !atomic_mb_read(>suspend_cnt) ? 4096 : 0;
   }


And in Vz8 (Rhel8 based), it looks like (to avoid assertion in 
handle_qmp_command()):

--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -9,7 +9,7 @@ extern __thread Monitor *cur_mon;
   typedef struct MonitorHMP MonitorHMP;
   typedef struct MonitorOptions MonitorOptions;
   
-#define QMP_REQ_QUEUE_LEN_MAX 8

+#define QMP_REQ_QUEUE_LEN_MAX 4096
   
   extern QemuOptsList qemu_mon_opts;
   


diff --git a/monitor/monitor.c b/monitor/monitor.c
index b385a3d569..a124d010f3 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -501,7 +501,7 @@ int monitor_can_read(void *opaque)
   {
   Monitor *mon = opaque;
   
-return !atomic_mb_read(>suspend_cnt);

+return !atomic_mb_read(>suspend_cnt) ? 4096 : 0;
   }


There are some theoretical risks of overflowing... But it just works. Still 
this probably not good for upstream. And I'm not sure how would it work with 
OOB..


This is 

Re: [PATCH v3 2/5] monitor: drain requests queue with 'channel closed' event

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 02.03.2021 16:53, Markus Armbruster wrote:
>> Andrey Shinkevich via  writes:
>> 
>>> When CHR_EVENT_CLOSED comes, the QMP requests queue may still contain
>>> unprocessed commands. It can happen with QMP capability OOB enabled.
>>> Let the dispatcher complete handling requests rest in the monitor
>>> queue.
>>>
>>> Signed-off-by: Andrey Shinkevich 
>>> ---
>>>   monitor/qmp.c | 46 +-
>>>   1 file changed, 21 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/monitor/qmp.c b/monitor/qmp.c
>>> index 7169366..a86ed35 100644
>>> --- a/monitor/qmp.c
>>> +++ b/monitor/qmp.c
>>> @@ -75,36 +75,32 @@ static void 
>>> monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon)
>>>   }
>>>   }
>>>   
>>> -static void monitor_qmp_cleanup_queue_and_resume(MonitorQMP *mon)
>>> +/*
>>> + * Let unprocessed QMP commands be handled.
>>> + */
>>> +static void monitor_qmp_drain_queue(MonitorQMP *mon)
>>>   {
>>> -qemu_mutex_lock(>qmp_queue_lock);
>>> +bool q_is_empty = false;
>>>   
>>> -/*
>>> - * Same condition as in monitor_qmp_dispatcher_co(), but before
>>> - * removing an element from the queue (hence no `- 1`).
>>> - * Also, the queue should not be empty either, otherwise the
>>> - * monitor hasn't been suspended yet (or was already resumed).
>>> - */
>>> -bool need_resume = (!qmp_oob_enabled(mon) ||
>>> -mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX)
>>> -&& !g_queue_is_empty(mon->qmp_requests);
>>> +while (!q_is_empty) {
>>> +qemu_mutex_lock(>qmp_queue_lock);
>>> +q_is_empty = g_queue_is_empty(mon->qmp_requests);
>>> +qemu_mutex_unlock(>qmp_queue_lock);
>>>   
>>> -monitor_qmp_cleanup_req_queue_locked(mon);
>>> +if (!q_is_empty) {
>>> +if (!qatomic_xchg(_dispatcher_co_busy, true)) {
>>> +/* Kick the dispatcher coroutine */
>>> +aio_co_wake(qmp_dispatcher_co);
>>> +} else {
>>> +/* Let the dispatcher do its job for a while */
>>> +g_usleep(40);
>>> +}
>>> +}
>>> +}
>>>   
>>> -if (need_resume) {
>>> -/*
>>> - * handle_qmp_command() suspended the monitor because the
>>> - * request queue filled up, to be resumed when the queue has
>>> - * space again.  We just emptied it; resume the monitor.
>>> - *
>>> - * Without this, the monitor would remain suspended forever
>>> - * when we get here while the monitor is suspended.  An
>>> - * unfortunately timed CHR_EVENT_CLOSED can do the trick.
>>> - */
>>> +if (qatomic_mb_read(>common.suspend_cnt)) {
>>>   monitor_resume(>common);
>>>   }
>>> -
>>> -qemu_mutex_unlock(>qmp_queue_lock);
>>>   }
>>>   
>>>   void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
>>> @@ -418,7 +414,7 @@ static void monitor_qmp_event(void *opaque, 
>>> QEMUChrEvent event)
>>>* stdio, it's possible that stdout is still open when stdin
>>>* is closed.
>>>*/
>>> -monitor_qmp_cleanup_queue_and_resume(mon);
>>> +monitor_qmp_drain_queue(mon);
>>>   json_message_parser_destroy(>parser);
>>>   json_message_parser_init(>parser, handle_qmp_command,
>>>mon, NULL);
>> 
>> Before the patch: we call monitor_qmp_cleanup_queue_and_resume() to
>> throw away the contents of the request queue, and resume the monitor if
>> suspended.
>> 
>> Afterwards: we call monitor_qmp_drain_queue() to wait for the request
>> queue to drain.  I think.  Before we discuss the how, I have a question
>> the commit message should answer, but doesn't: why?
>> 
>
> Hi!
>
> Andrey is not in Virtuozzo now, and nobody doing this work actually.. 
> Honestly, I don't believe that the feature should be so difficult.
>
> Actually, we have the following patch in Virtuozzo 7 (Rhel7 based) for years, 
> and it just works without any problems:

I appreciate your repeated efforts to get your downstream patch
upstream.

> --- a/monitor.c
> +++ b/monitor.c
> @@ -4013,7 +4013,7 @@ static int monitor_can_read(void *opaque)
>   {
>   Monitor *mon = opaque;
>   
> -return !atomic_mb_read(>suspend_cnt);
> +return !atomic_mb_read(>suspend_cnt) ? 4096 : 0;
>   }
>
>
> And in Vz8 (Rhel8 based), it looks like (to avoid assertion in 
> handle_qmp_command()):
>
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -9,7 +9,7 @@ extern __thread Monitor *cur_mon;
>   typedef struct MonitorHMP MonitorHMP;
>   typedef struct MonitorOptions MonitorOptions;
>   
> -#define QMP_REQ_QUEUE_LEN_MAX 8
> +#define QMP_REQ_QUEUE_LEN_MAX 4096
>   
>   extern QemuOptsList qemu_mon_opts;
>   
>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index b385a3d569..a124d010f3 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -501,7 +501,7 @@ int 

Re: [PATCH v2] block/file-posix: Optimize for macOS

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20210305121748.65173-1-akihiko.od...@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210305121748.65173-1-akihiko.od...@gmail.com
Subject: [PATCH v2] block/file-posix: Optimize for macOS

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20210305121748.65173-1-akihiko.od...@gmail.com -> 
patchew/20210305121748.65173-1-akihiko.od...@gmail.com
Switched to a new branch 'test'
16b639a block/file-posix: Optimize for macOS

=== OUTPUT BEGIN ===
WARNING: architecture specific defines should be avoided
#95: FILE: block/file-posix.c:2133:
+#if defined(__APPLE__) && (__MACH__)

ERROR: suspect code indent for conditional statements (8, 11)
#168: FILE: hw/block/block.c:73:
+if (!backend_ret && blocksizes.phys) {
conf->physical_block_size = blocksizes.phys;

total: 1 errors, 1 warnings, 145 lines checked

Commit 16b639a81c45 (block/file-posix: Optimize for macOS) has style problems, 
please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210305121748.65173-1-akihiko.od...@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH v2] block/file-posix: Optimize for macOS

[Akihiko Odaki]

This commit introduces "punch hole" operation and optimizes transfer
block size for macOS.

This commit introduces two additional members,
discard_granularity and opt_io to BlockSizes type in
include/block/block.h. Also, the members of the type are now
optional. Set -1 to discard_granularity and 0 to other members
for the default values.

Thanks to Konstantin Nazarov for detailed analysis of a flaw in an
old version of this change:
https://gist.github.com/akihikodaki/87df4149e7ca87f18dc56807ec5a1bc5#gistcomment-3654667

Signed-off-by: Akihiko Odaki 
---
 block/file-posix.c| 40 ++--
 block/nvme.c  |  2 ++
 block/raw-format.c|  4 +++-
 hw/block/block.c  | 12 ++--
 include/block/block.h |  2 ++
 5 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 05079b40cae..21bdaf969c5 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -44,6 +44,7 @@
 #if defined(__APPLE__) && (__MACH__)
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1292,6 +1293,8 @@ static int hdev_probe_blocksizes(BlockDriverState *bs, 
BlockSizes *bsz)
 if (check_for_dasd(s->fd) < 0) {
 return -ENOTSUP;
 }
+bsz->opt_io = 0;
+bsz->discard_granularity = -1;
 ret = probe_logical_blocksize(s->fd, >log);
 if (ret < 0) {
 return ret;
@@ -1586,6 +1589,7 @@ out:
 }
 }
 
+G_GNUC_UNUSED
 static int translate_err(int err)
 {
 if (err == -ENODEV || err == -ENOSYS || err == -EOPNOTSUPP ||
@@ -1795,16 +1799,27 @@ static int handle_aiocb_discard(void *opaque)
 }
 } while (errno == EINTR);
 
-ret = -errno;
+ret = translate_err(-errno);
 #endif
 } else {
 #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
 ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
aiocb->aio_offset, aiocb->aio_nbytes);
+ret = translate_err(-errno);
+#elif defined(__APPLE__) && (__MACH__)
+fpunchhole_t fpunchhole;
+fpunchhole.fp_flags = 0;
+fpunchhole.reserved = 0;
+fpunchhole.fp_offset = aiocb->aio_offset;
+fpunchhole.fp_length = aiocb->aio_nbytes;
+if (fcntl(s->fd, F_PUNCHHOLE, ) == -1) {
+ret = errno == ENODEV ? -ENOTSUP : -errno;
+} else {
+ret = 0;
+}
 #endif
 }
 
-ret = translate_err(ret);
 if (ret == -ENOTSUP) {
 s->has_discard = false;
 }
@@ -2113,6 +2128,26 @@ static int raw_co_flush_to_disk(BlockDriverState *bs)
 return raw_thread_pool_submit(bs, handle_aiocb_flush, );
 }
 
+static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
+{
+#if defined(__APPLE__) && (__MACH__)
+BDRVRawState *s = bs->opaque;
+struct statfs buf;
+
+if (!fstatfs(s->fd, )) {
+bsz->phys = 0;
+bsz->log = 0;
+bsz->opt_io = buf.f_iosize;
+bsz->discard_granularity = buf.f_bsize;
+return 0;
+}
+
+return -errno;
+#else
+return -ENOTSUP;
+#endif
+}
+
 static void raw_aio_attach_aio_context(BlockDriverState *bs,
AioContext *new_context)
 {
@@ -3247,6 +3282,7 @@ BlockDriver bdrv_file = {
 .bdrv_refresh_limits = raw_refresh_limits,
 .bdrv_io_plug = raw_aio_plug,
 .bdrv_io_unplug = raw_aio_unplug,
+.bdrv_probe_blocksizes = raw_probe_blocksizes,
 .bdrv_attach_aio_context = raw_aio_attach_aio_context,
 
 .bdrv_co_truncate = raw_co_truncate,
diff --git a/block/nvme.c b/block/nvme.c
index 2b5421e7aa6..1845d07577b 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -989,6 +989,8 @@ static int nvme_probe_blocksizes(BlockDriverState *bs, 
BlockSizes *bsz)
 uint32_t blocksize = nvme_get_blocksize(bs);
 bsz->phys = blocksize;
 bsz->log = blocksize;
+bsz->opt_io = 0;
+bsz->discard_granularity = -1;
 return 0;
 }
 
diff --git a/block/raw-format.c b/block/raw-format.c
index 7717578ed6a..847df11f2ae 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -507,6 +507,7 @@ static int raw_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 {
 BDRVRawState *s = bs->opaque;
+uint32_t size;
 int ret;
 
 ret = bdrv_probe_blocksizes(bs->file->bs, bsz);
@@ -514,7 +515,8 @@ static int raw_probe_blocksizes(BlockDriverState *bs, 
BlockSizes *bsz)
 return ret;
 }
 
-if (!QEMU_IS_ALIGNED(s->offset, MAX(bsz->log, bsz->phys))) {
+size = MAX(bsz->log, bsz->phys);
+if (size && !QEMU_IS_ALIGNED(s->offset, size)) {
 return -ENOTSUP;
 }
 
diff --git a/hw/block/block.c b/hw/block/block.c
index 1e34573da71..c907e5a7722 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -70,19 +70,27 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
 backend_ret = blk_probe_blocksizes(blk, );
 /* fill in detected values if they are not defined via qemu command 

[PATCH] docs: qsd: Explain --export nbd,name=... default

[Kevin Wolf]

The 'name' option for NBD exports is optional. Add a note that the
default for the option is the node name (people could otherwise expect
that it's the empty string like for qemu-nbd).

Signed-off-by: Kevin Wolf 
---
 docs/tools/qemu-storage-daemon.rst | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/tools/qemu-storage-daemon.rst 
b/docs/tools/qemu-storage-daemon.rst
index fe3042d609..086493ebb3 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -80,8 +80,9 @@ Standard options:
   requests for modifying data (the default is off).
 
   The ``nbd`` export type requires ``--nbd-server`` (see below). ``name`` is
-  the NBD export name. ``bitmap`` is the name of a dirty bitmap reachable from
-  the block node, so the NBD client can use NBD_OPT_SET_META_CONTEXT with the
+  the NBD export name (if not specified, it defaults to the given
+  ``node-name``). ``bitmap`` is the name of a dirty bitmap reachable from the
+  block node, so the NBD client can use NBD_OPT_SET_META_CONTEXT with the
   metadata context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap.
 
   The ``vhost-user-blk`` export type takes a vhost-user socket address on which
-- 
2.29.2

Re: [PATCH v2 2/2] memory: Drop "qemu:" prefix from QOM memory region type names

[Philippe Mathieu-Daudé]

On 3/4/21 3:02 PM, Markus Armbruster wrote:
> Almost all QOM type names consist only of letters, digits, '-', '_',
> and '.'.  Just two contain ':': "qemu:memory-region" and
> "qemu:iommu-memory-region".  Neither can be plugged with -object.
> Rename them to "memory-region" and "iommu-memory-region".
> 
> Signed-off-by: Markus Armbruster 
> ---
>  include/exec/memory.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: QEMU RBD is slow with QCOW2 images

[Stefano Garzarella]

On Fri, Mar 05, 2021 at 10:16:41AM +0100, Kevin Wolf wrote:

Am 04.03.2021 um 18:32 hat Stefano Garzarella geschrieben:

On Thu, Mar 04, 2021 at 03:59:17PM +0100, Kevin Wolf wrote:
> Am 04.03.2021 um 15:08 hat Stefano Garzarella geschrieben:
> > On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote:
> > > Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben:
> > > > Hi Jason,
> > > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> > > > writing data is very slow compared to a raw file.
> > > >
> > > > Comparing raw vs QCOW2 image creation with RBD I found that we use a
> > > > different object size, for the raw file I see '4 MiB objects', for
> > > > QCOW2 I
> > > > see '64 KiB objects' as reported on comment 14 [2].
> > > > This should be the main issue of slowness, indeed forcing in the code 4 
MiB
> > > > object size also for QCOW2 increased the speed a lot.
> > > >
> > > > Looking better I discovered that for raw files, we call rbd_create() 
with
> > > > obj_order = 0 (if 'cluster_size' options is not defined), so the default
> > > > object size is used.
> > > > Instead for QCOW2, we use obj_order = 16, since the default 
'cluster_size'
> > > > defined for QCOW2, is 64 KiB.
> > >
> > > Hm, the QemuOpts-based image creation is messy, but why does the rbd
> > > driver even see the cluster_size option?
> > >
> > > The first thing qcow2_co_create_opts() does is splitting the passed
> > > QemuOpts into options it will process on the qcow2 layer and options
> > > that are passed to the protocol layer. So if you pass a cluster_size
> > > option, qcow2 should take it for itself and not pass it to rbd.
> > >
> > > If it is passed to rbd, I think that's a bug in the qcow2 driver.
> >
> > IIUC qcow2 properyl remove it, but when rbd uses qemu_opt_get_size_del(opts,
> > BLOCK_OPT_CLUSTER_SIZE, 0) the default value of qcow2 format is returned.
> >
> > Going in depth in qemu_opt_get_size_helper(), I found that qemu_opt_find()
> > properly returns a NULL pointer, but then we call find_default_by_name()
> > that returns the default value of qcow2 format (64k).
>
> Ugh, I see why. We're passing the protocol driver a QemuOpts that was
> created for a QemuOptsList with the qcow2 default, not for its own
> QemuOptsList. This is wrong.
>
> Note that the QemuOptsList is not qcow2_create_opts itself, but a list
> that is created with qemu_opts_append() to combine qcow2 and rbd options
> into a new QemuOptsList. For overlapping options, the format wins.
>
> I don't think you can change the QemuOptsList of an existing QemuOpts,
> nor is there a clone operation that could just copy all options into a
> new QemuOpts created for the rbd QemuOptsList, so maybe the easiest
> hack^Wsolution would be converting to QDict and back...

Do you mean something like this? (I'll send a proper patch when everything
is a little clearer to me :-)

diff --git a/block.c b/block.c
index a1f3cecd75..74b02b32dc 100644
--- a/block.c
+++ b/block.c
@@ -671,13 +671,33 @@ out:
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
 {
 BlockDriver *drv;
+QemuOpts *new_opts;
+QDict *qdict;
+int ret;

 drv = bdrv_find_protocol(filename, true, errp);
 if (drv == NULL) {
 return -ENOENT;
 }

-return bdrv_create(drv, filename, opts, errp);
+if (!drv->create_opts) {
+error_setg(errp, "Driver '%s' does not support image creation",
+   drv->format_name);
+return -ENOTSUP;
+}
+
+qdict = qemu_opts_to_qdict(opts, NULL);
+new_opts = qemu_opts_from_qdict(drv->create_opts, qdict, errp);
+if (new_opts == NULL) {
+ret = -EINVAL;
+goto out;
+}
+
+ret = bdrv_create(drv, filename, new_opts, errp);
+out:
+qemu_opts_del(new_opts);
+qobject_unref(qdict);
+return ret;
 }


Something like this, yes. Does it work for you?


Yes, I did some testing and only the protocol parameters are passed and 
its defaults.


Thanks for the suggestion!



Of course, in the real patch it could use a comment why we're doing
these seemingly redundant conversions.


Sure, I'm running some tests to make sure I haven't broken anything. :-)

Thanks,
Stefano

Re: QEMU RBD is slow with QCOW2 images

[Kevin Wolf]

Am 04.03.2021 um 18:32 hat Stefano Garzarella geschrieben:
> On Thu, Mar 04, 2021 at 03:59:17PM +0100, Kevin Wolf wrote:
> > Am 04.03.2021 um 15:08 hat Stefano Garzarella geschrieben:
> > > On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote:
> > > > Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben:
> > > > > Hi Jason,
> > > > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> > > > > writing data is very slow compared to a raw file.
> > > > >
> > > > > Comparing raw vs QCOW2 image creation with RBD I found that we use a
> > > > > different object size, for the raw file I see '4 MiB objects', for
> > > > > QCOW2 I
> > > > > see '64 KiB objects' as reported on comment 14 [2].
> > > > > This should be the main issue of slowness, indeed forcing in the code 
> > > > > 4 MiB
> > > > > object size also for QCOW2 increased the speed a lot.
> > > > >
> > > > > Looking better I discovered that for raw files, we call rbd_create() 
> > > > > with
> > > > > obj_order = 0 (if 'cluster_size' options is not defined), so the 
> > > > > default
> > > > > object size is used.
> > > > > Instead for QCOW2, we use obj_order = 16, since the default 
> > > > > 'cluster_size'
> > > > > defined for QCOW2, is 64 KiB.
> > > >
> > > > Hm, the QemuOpts-based image creation is messy, but why does the rbd
> > > > driver even see the cluster_size option?
> > > >
> > > > The first thing qcow2_co_create_opts() does is splitting the passed
> > > > QemuOpts into options it will process on the qcow2 layer and options
> > > > that are passed to the protocol layer. So if you pass a cluster_size
> > > > option, qcow2 should take it for itself and not pass it to rbd.
> > > >
> > > > If it is passed to rbd, I think that's a bug in the qcow2 driver.
> > > 
> > > IIUC qcow2 properyl remove it, but when rbd uses 
> > > qemu_opt_get_size_del(opts,
> > > BLOCK_OPT_CLUSTER_SIZE, 0) the default value of qcow2 format is returned.
> > > 
> > > Going in depth in qemu_opt_get_size_helper(), I found that qemu_opt_find()
> > > properly returns a NULL pointer, but then we call find_default_by_name()
> > > that returns the default value of qcow2 format (64k).
> > 
> > Ugh, I see why. We're passing the protocol driver a QemuOpts that was
> > created for a QemuOptsList with the qcow2 default, not for its own
> > QemuOptsList. This is wrong.
> > 
> > Note that the QemuOptsList is not qcow2_create_opts itself, but a list
> > that is created with qemu_opts_append() to combine qcow2 and rbd options
> > into a new QemuOptsList. For overlapping options, the format wins.
> > 
> > I don't think you can change the QemuOptsList of an existing QemuOpts,
> > nor is there a clone operation that could just copy all options into a
> > new QemuOpts created for the rbd QemuOptsList, so maybe the easiest
> > hack^Wsolution would be converting to QDict and back...
> 
> Do you mean something like this? (I'll send a proper patch when everything
> is a little clearer to me :-)
> 
> diff --git a/block.c b/block.c
> index a1f3cecd75..74b02b32dc 100644
> --- a/block.c
> +++ b/block.c
> @@ -671,13 +671,33 @@ out:
>  int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>  {
>  BlockDriver *drv;
> +QemuOpts *new_opts;
> +QDict *qdict;
> +int ret;
> 
>  drv = bdrv_find_protocol(filename, true, errp);
>  if (drv == NULL) {
>  return -ENOENT;
>  }
> 
> -return bdrv_create(drv, filename, opts, errp);
> +if (!drv->create_opts) {
> +error_setg(errp, "Driver '%s' does not support image creation",
> +   drv->format_name);
> +return -ENOTSUP;
> +}
> +
> +qdict = qemu_opts_to_qdict(opts, NULL);
> +new_opts = qemu_opts_from_qdict(drv->create_opts, qdict, errp);
> +if (new_opts == NULL) {
> +ret = -EINVAL;
> +goto out;
> +}
> +
> +ret = bdrv_create(drv, filename, new_opts, errp);
> +out:
> +qemu_opts_del(new_opts);
> +qobject_unref(qdict);
> +return ret;
>  }

Something like this, yes. Does it work for you?

Of course, in the real patch it could use a comment why we're doing
these seemingly redundant conversions.

Kevin

Re: [PATCH v2 1/8] simplebench: bench_one(): add slow_limit argument

[Vladimir Sementsov-Ogievskiy]

05.03.2021 04:22, John Snow wrote:

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:

Sometimes one of cells in a testing table runs too slow. And we really
don't want to wait so long. Limit number of runs in this case.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/simplebench.py | 29 +
  1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/scripts/simplebench/simplebench.py 
b/scripts/simplebench/simplebench.py
index f61513af90..b153cae274 100644
--- a/scripts/simplebench/simplebench.py
+++ b/scripts/simplebench/simplebench.py
@@ -19,9 +19,11 @@
  #
  import statistics
+import time
-def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
+def bench_one(test_func, test_env, test_case, count=5, initial_run=True,
+  slow_limit=100):
  """Benchmark one test-case
  test_func   -- benchmarking function with prototype
@@ -36,6 +38,8 @@ def bench_one(test_func, test_env, test_case, count=5, 
initial_run=True):
  test_case   -- test case - opaque second argument for test_func
  count   -- how many times to call test_func, to calculate average
  initial_run -- do initial run of test_func, which don't get into result
+    slow_limit  -- reduce test runs to 2, if current run exceedes the limit
+   (in seconds)


s/exceedes/exceeds, and you need to mention that if the initial run exceeds the 
limit, it will change the behavior to count that result.

It is also possible (conceivably) that the initial run exceeds the limit, but 
subsequent runs don't, so it might be hard to predict how many tests it'll 
actually run.

If you're OK with that behavior, maybe:

"Consider a test run 'slow' once it exceeds this limit, in seconds.
  Stop early once there are two 'slow' runs, including the initial run.
  Slow initial runs will be included in the results."

Lastly, this will change existing behavior -- do we care? Should it default to 
None instead? Should we be able to pass None or 0 to disable this behavior?


For sure I don't care about changing the behavior. Consider simplebench in a 
version 0.0.1 :). Maybe, I should make a comment somewhere, but nobody will 
read it anyway.

The aim of the patch is to minimize waiting for too long cells of the table, 
which are obviously too much longer then the others. Probably the logic should 
be improved a bit about ignoring or using initial-run result..
Like this:

If both initial and first run are slow, count both and stop here.
Otherwise, stop at first slow normal run and don't count initial run.

Or may be even

If both initial and first run are slow, count both and stop here.
Otherwise, behave the common way.




  Returns dict with the following fields:
  'runs': list of test_func results
@@ -47,17 +51,34 @@ def bench_one(test_func, test_env, test_case, count=5, 
initial_run=True):
  'n-failed': number of failed runs (exists only if at least one run
  failed)
  """
+    runs = []
+    i = 0
  if initial_run:
+    t = time.time()
+
  print('  #initial run:')
-    print('   ', test_func(test_env, test_case))
+    res = test_func(test_env, test_case)
+    print('   ', res)
+
+    if time.time() - t > slow_limit:
+    print('    - initial run is too slow, so it counts')
+    runs.append(res)
+    i = 1
+
+    for i in range(i, count):
+    t = time.time()
-    runs = []
-    for i in range(count):
  print('  #run {}'.format(i+1))
  res = test_func(test_env, test_case)
  print('   ', res)
  runs.append(res)
+    if time.time() - t > slow_limit and len(runs) >= 2:
+    print('    - run is too slow, and we have enough runs, stop here')
+    break
+
+    count = len(runs)
+
  result = {'runs': runs}
  succeeded = [r for r in runs if ('seconds' in r or 'iops' in r)]






--
Best regards,
Vladimir

Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[Vladimir Sementsov-Ogievskiy]

05.03.2021 04:30, John Snow wrote:

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:

It probably may improve reliability of results when testing in cached
mode.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench_block_job.py | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/scripts/simplebench/bench_block_job.py 
b/scripts/simplebench/bench_block_job.py
index 4f03c12169..fa45ad2655 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
  return {'error': 'qemu failed: ' + str(vm.get_log())}
  try:
+    subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', shell=True,
+   check=True)
  res = vm.qmp(cmd, **cmd_args)
  if res != {'return': {}}:
  vm.shutdown()



Worth adding a conditional to allow "hot" or "cold" runs? nah?



You mean, make this addition optional? Make sense


--
Best regards,
Vladimir

Re: [PATCH v2 7/8] simplebench/bench-backup: add --count and --no-initial-run

[Vladimir Sementsov-Ogievskiy]

05.03.2021 04:37, John Snow wrote:

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:

Add arguments to set number of test runs per table cell and to disable
initial run that is not counted in results.

It's convenient to set --count 1 --no-initial-run to fast run test
onece, and to set --count to some large enough number for good
precision of the results.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench-backup.py | 10 +-
  1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/scripts/simplebench/bench-backup.py 
b/scripts/simplebench/bench-backup.py
index a2120fcbf0..519a985a7f 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -155,7 +155,9 @@ def bench(args):
  'qemu-binary': path
  })
-    result = simplebench.bench(bench_func, test_envs, test_cases, count=3)
+    result = simplebench.bench(bench_func, test_envs, test_cases,
+   count=args.count,
+   initial_run = not args.no_initial_run)


The double negative feels odd; "initial_run = args.initial_run" would read 
better and avoid changing behavior, but maybe that's intentional.


Hmm it was simple way to add --no-initial-run. But I agree it looks strange. 
Will improve.




  with open('results.json', 'w') as f:
  json.dump(result, f, indent=4)
  print(results_to_text(result))
@@ -211,4 +213,10 @@ def __call__(self, parser, namespace, values, 
option_string=None):
 both: generate two test cases for each src:dst pair''',
 default='direct', choices=('direct', 'cached', 'both'))
+    p.add_argument('--count', type=int, default=3, help='''\
+Number of test runs per table cell''')
+
+    p.add_argument('--no-initial-run', action='store_true', help='''\
+Don't do initial run of test for each cell which doesn't count''')
+
  bench(p.parse_args())






--
Best regards,
Vladimir

Re: [PATCH v2 1/3] fdc: Drop deprecated floppy configuration

[Markus Armbruster]

Markus Armbruster  writes:

> Daniel P. Berrangé  writes:
>
>> On Thu, Mar 04, 2021 at 11:00:57AM +0100, Markus Armbruster wrote:
>>> Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate
>>> configuring floppies with -global isa-fdc" (v5.1.0).
>>> 
>>> Signed-off-by: Markus Armbruster 
[...]
> Sadly, the commit's update of docs/system/deprecated.rst neglects to
> cover this use.  Looks the series overtaxed my capacity to juggle
> details; my apologies.
[...]

I'm talking about commit 4a27a638e7 here.

The deprecated.rst text only covers setting floppy controller properties
with -global.  It neglects to cover setting them with -device.  For
onboard controllers, -global is the only way to set them.

I append a fixup.

We can put it before this patch.  This patch then moves the fixed up
text to removed-features.rst.

Or we squash it into this patch, i.e. this patch deletes the flawed text
from deprecated.rst and adds the fixed up text to removed-features.rst.

Got a preference?



diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 2fcac7861e..393ede47f1 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -97,7 +97,11 @@ QEMU 5.1 has three options:
 ``Configuring floppies with ``-global``
 '''
 
-Use ``-device floppy,...`` instead:
+Floppy controllers' drive properties (since 5.1)
+
+
+Use ``-device floppy,...`` instead.  When configuring onboard floppy
+controllers
 ::
 
 -global isa-fdc.driveA=...
@@ -120,8 +124,30 @@ become
 
 -device floppy,unit=1,drive=...
 
-``-drive`` with bogus interface type
-
+When plugging in a floppy controller
+::
+
+-device isa-fdc,...,driveA=...
+
+becomes
+::
+
+-device isa-fdc,...
+-device floppy,unit=0,drive=...
+
+and
+::
+
+-device isa-fdc,...,driveB=...
+
+becomes
+::
+
+-device isa-fdc,...
+-device floppy,unit=1,drive=...
+
+``-drive`` with bogus interface type (since 5.1)
+
 
 Drives with interface types other than ``if=none`` are for onboard
 devices.  It is possible to use drives the board doesn't pick up with