[PATCH 2/3] iotests/081: Filter image format after testdir

[Max Reitz]

Otherwise, this breaks whenever the test directory contains the image
format (e.g. "/tmp/test-raw-file" is filtered to "/tmp/test-IMGFMT-file"
instead of "TEST_DIR").

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/081 | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index 537d40dfd5..253b7576be 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -45,15 +45,16 @@ _require_drivers quorum
 
 do_run_qemu()
 {
-echo Testing: "$@" | _filter_imgfmt
+echo Testing: "$@"
 $QEMU -nographic -qmp stdio -serial none "$@"
 echo
 }
 
 run_qemu()
 {
-do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp\
-  | _filter_qemu_io | _filter_generated_node_ids
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_imgfmt | _filter_qemu \
+  | _filter_qmp | _filter_qemu_io \
+  | _filter_generated_node_ids
 }
 
 quorum="driver=raw,file.driver=quorum,file.vote-threshold=2"
-- 
2.28.0

[PATCH 1/3] quorum: Require WRITE perm with rewrite-corrupted

[Max Reitz]

Using rewrite-corrupted means quorum may issue writes to its children
just from receiving read requests from its parents.  Thus, it must take
the WRITE permission when rewrite-corrupted is used.

Signed-off-by: Max Reitz 
---
 block/quorum.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/block/quorum.c b/block/quorum.c
index b70d365ba9..01e49e94a3 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1195,7 +1195,12 @@ static void quorum_child_perm(BlockDriverState *bs, 
BdrvChild *c,
   uint64_t perm, uint64_t shared,
   uint64_t *nperm, uint64_t *nshared)
 {
+BDRVQuorumState *s = bs->opaque;
+
 *nperm = perm & DEFAULT_PERM_PASSTHROUGH;
+if (s->rewrite_corrupted) {
+*nperm |= BLK_PERM_WRITE;
+}
 
 /*
  * We cannot share RESIZE or WRITE, as this would make the
-- 
2.28.0

[PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted

[Max Reitz]

Hi,

While reviewing Berto’s block-status/write-zeroes series for quorum, I
wondered how quorum’s permission code handles rewrite-corrupted.  It
turns out it doesn’t, and so qemu with a read-only rewrite-corrupted
quorum node simply crashes once there is a mismatch that leads to a
rewrite.

It looks to me like this bug has existed for quite some time, so I don’t
think this series must go into 5.2.  OTOH, it’s a simple bug fix, so I
suppose it might as well.


Max Reitz (3):
  quorum: Require WRITE perm with rewrite-corrupted
  iotests/081: Filter image format after testdir
  iotests/081: Test rewrite-corrupted without WRITE

 block/quorum.c |  5 
 tests/qemu-iotests/081 | 61 --
 tests/qemu-iotests/081.out | 27 +
 3 files changed, 90 insertions(+), 3 deletions(-)

-- 
2.28.0

[PATCH 3/3] iotests/081: Test rewrite-corrupted without WRITE

[Max Reitz]

Test what happens when a rewrite-corrupted quorum node performs such a
rewrite, while there is no parent that has taken the WRITE permission.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/081 | 54 ++
 tests/qemu-iotests/081.out | 27 +++
 2 files changed, 81 insertions(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index 253b7576be..4e19972931 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -42,6 +42,7 @@ _supported_fmt raw
 _supported_proto file
 _supported_os Linux
 _require_drivers quorum
+_require_devices virtio-scsi
 
 do_run_qemu()
 {
@@ -155,6 +156,59 @@ echo "== checking that quorum has corrected the corrupted 
file =="
 
 $QEMU_IO -c "read -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io
 
+echo
+echo "== using quorum rewrite corrupted mode without WRITE permission =="
+
+# The same as above, but this time, do it on a quorum node whose only
+# parent will not take the WRITE permission
+
+echo '-- corrupting --'
+# Only corrupt a portion: The guest device (scsi-hd on virtio-scsi)
+# will read some data (looking for a partition table to guess the
+# disk's geometry), which would trigger a quorum mismatch if the
+# beginning of the image was corrupted.  The subsequent
+# QUORUM_REPORT_BAD event would be suppressed (because at that point,
+# there cannot have been a qmp_capabilities on the monitor).  Because
+# that event is rate-limited, the next QUORUM_REPORT_BAD that happens
+# thanks to our qemu-io read (which should trigger a mismatch) would
+# then be delayed past the VM quit and not appear in the output.
+# So we keep the first 1M intact to see a QUORUM_REPORT_BAD resulting
+# from the qemu-io invocation.
+$QEMU_IO -c "write -P 0x42 1M 1M" "$TEST_DIR/2.raw" | _filter_qemu_io
+
+# Fix the corruption (on a read-only quorum node, i.e. without taking
+# the WRITE permission on it -- its child nodes need to be R/W OTOH,
+# so that rewrite-corrupted works)
+echo
+echo '-- running quorum --'
+run_qemu \
+-blockdev file,node-name=file1,filename="$TEST_DIR/1.raw" \
+-blockdev file,node-name=file2,filename="$TEST_DIR/2.raw" \
+-blockdev file,node-name=file3,filename="$TEST_DIR/3.raw" \
+-blockdev '{
+"driver": "quorum",
+"node-name": "quorum",
+"read-only": true,
+"vote-threshold": 2,
+"rewrite-corrupted": true,
+"children": [ "file1", "file2", "file3" ]
+}' \
+-device virtio-scsi,id=scsi \
+-device scsi-hd,id=quorum-drive,bus=scsi.0,drive=quorum \
+<

Re: [PATCH v4 0/2] quorum: Implement bdrv_co_block_status()

[Max Reitz]

On 13.11.20 17:52, Alberto Garcia wrote:

Following Max's suggestion, this version sets supported_zero_flags.

Berto

v4:
- Set supported_zero_flags in quorum [Max]
- Update test to verify the data written before doing 'write -z' [Max]

v3: https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00371.html
- Fall back to BDRV_BLOCK_DATA if a child returns an error.

v2: https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00259.html
- Implement bdrv_co_pwrite_zeroes() for quorum

v1: https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00163.html


Alberto Garcia (2):
   quorum: Implement bdrv_co_block_status()
   quorum: Implement bdrv_co_pwrite_zeroes()


Thanks, applied to my block-next branch (for 6.0):

https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next

Max

Re: [PATCH v4 2/2] quorum: Implement bdrv_co_pwrite_zeroes()

[Max Reitz]

On 13.11.20 17:52, Alberto Garcia wrote:

This simply calls bdrv_co_pwrite_zeroes() in all children.

bs->supported_zero_flags is also set to the flags that are supported
by all children.

Signed-off-by: Alberto Garcia 
---
  block/quorum.c | 36 ++--
  tests/qemu-iotests/312 | 11 +++
  tests/qemu-iotests/312.out |  8 
  3 files changed, 53 insertions(+), 2 deletions(-)


Reviewed-by: Max Reitz 


@@ -897,6 +910,21 @@ static QemuOptsList quorum_runtime_opts = {
  },
  };
  
+static void quorum_refresh_flags(BlockDriverState *bs)

+{
+BDRVQuorumState *s = bs->opaque;
+int i;
+
+bs->supported_zero_flags =
+BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK;
+
+for (i = 0; i < s->num_children; i++) {
+bs->supported_zero_flags &= s->children[i]->bs->supported_zero_flags;
+}
+
+bs->supported_zero_flags |= BDRV_REQ_WRITE_UNCHANGED;


This made me wonder whether it’s true when rewrite_corrupted is set.  I 
think it is, because that only does something when reading from the 
children (i.e. not for write requests from parents, where this flag 
might be set).


Looking into quorum_child_perm(), quorum doesn’t take the WRITE 
permission on its children even if rewrite_corrupted is true.  Hm... 
Something to look into.


Max


+}
+
  static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
 Error **errp)
  {

Re: [PATCH v2 7/7] qapi: More complex uses of QAPI_LIST_APPEND

[Dr. David Alan Gilbert]

* Eric Blake (ebl...@redhat.com) wrote:
> These cases require a bit more thought to review; in each case, the
> code was appending to a list, but not with a FOOList **tail variable.
> 
> Signed-off-by: Eric Blake 
> ---
>  block/gluster.c|  13 +---
>  block/qapi.c   |  14 +
>  dump/dump.c|  22 ++-
>  hw/core/machine-qmp-cmds.c | 125 +++--
>  hw/mem/memory-device.c |  12 +---
>  hw/pci/pci.c   |  60 ++
>  migration/migration.c  |  20 ++
>  monitor/hmp-cmds.c |  23 +++
>  net/net.c  |  13 +---
>  qga/commands-posix.c   | 101 +++---
>  qga/commands-win32.c   |  88 +-
>  softmmu/tpm.c  |  38 ++-
>  ui/spice-core.c|  27 +++-
>  13 files changed, 180 insertions(+), 376 deletions(-)
> 



> diff --git a/migration/migration.c b/migration/migration.c
> index 84e5f4982fb2..b97eb2d0494e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -797,29 +797,21 @@ void migrate_send_rp_resume_ack(MigrationIncomingState 
> *mis, uint32_t value)
> 
>  MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
>  {
> -MigrationCapabilityStatusList *head = NULL;
> -MigrationCapabilityStatusList *caps;
> +MigrationCapabilityStatusList *head = NULL, **tail = 
> +MigrationCapabilityStatus *caps;
>  MigrationState *s = migrate_get_current();
>  int i;
> 
> -caps = NULL; /* silence compiler warning */
>  for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
>  #ifndef CONFIG_LIVE_BLOCK_MIGRATION
>  if (i == MIGRATION_CAPABILITY_BLOCK) {
>  continue;
>  }
>  #endif
> -if (head == NULL) {
> -head = g_malloc0(sizeof(*caps));
> -caps = head;
> -} else {
> -caps->next = g_malloc0(sizeof(*caps));
> -caps = caps->next;
> -}
> -caps->value =
> -g_malloc(sizeof(*caps->value));
> -caps->value->capability = i;
> -caps->value->state = s->enabled_capabilities[i];
> +caps = g_malloc(sizeof(*caps));
> +caps->capability = i;
> +caps->state = s->enabled_capabilities[i];
> +QAPI_LIST_APPEND(tail, caps);
>  }
> 
>  return head;
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 01a7d317c3c9..678f388d2e1f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1699,7 +1699,8 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
>  void hmp_sendkey(Monitor *mon, const QDict *qdict)
>  {
>  const char *keys = qdict_get_str(qdict, "keys");
> -KeyValueList *keylist, *head = NULL, *tmp = NULL;
> +KeyValue *v;
> +KeyValueList *head = NULL, **tail = 
>  int has_hold_time = qdict_haskey(qdict, "hold-time");
>  int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
>  Error *err = NULL;
> @@ -1716,16 +1717,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>  keyname_len = 4;
>  }
> 
> -keylist = g_malloc0(sizeof(*keylist));
> -keylist->value = g_malloc0(sizeof(*keylist->value));
> -
> -if (!head) {
> -head = keylist;
> -}
> -if (tmp) {
> -tmp->next = keylist;
> -}
> -tmp = keylist;
> +v = g_malloc0(sizeof(*v));
> 
>  if (strstart(keys, "0x", NULL)) {
>  char *endp;
> @@ -1734,16 +1726,17 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>  if (endp != keys + keyname_len) {
>  goto err_out;
>  }
> -keylist->value->type = KEY_VALUE_KIND_NUMBER;
> -keylist->value->u.number.data = value;
> +v->type = KEY_VALUE_KIND_NUMBER;
> +v->u.number.data = value;
>  } else {
>  int idx = index_from_key(keys, keyname_len);
>  if (idx == Q_KEY_CODE__MAX) {
>  goto err_out;
>  }
> -keylist->value->type = KEY_VALUE_KIND_QCODE;
> -keylist->value->u.qcode.data = idx;
> +v->type = KEY_VALUE_KIND_QCODE;
> +v->u.qcode.data = idx;
>  }
> +QAPI_LIST_APPEND(tail, v);
> 
>  if (!*separator) {
>  break;

Don't you need to arrange for 'v' to be free'd in the err_out case?

Dave

> diff --git a/net/net.c b/net/net.c
> index eb65e110871a..453865db6f10 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1199,10 +1199,9 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name, 
> const char *name,
>Error **errp)
>  {
>  NetClientState *nc;
> -RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
> +RxFilterInfoList *filter_list = NULL, **last = _list;
> 
>  QTAILQ_FOREACH(nc, _clients, next) {
> -RxFilterInfoList *entry;
>  RxFilterInfo 

Re: [PATCH v7 00/21] preallocate filter

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

Here is a filter, which does preallocation on write.

v7:
01: add Alberto's r-b
07: don't remove sentence from the comment
08: - drop extra "s->file_end = end;" line
 - improve check/set perm handlers
09: add Max's r-b
10: add Max's r-b
11: new
12: - skip if preallocate unsupported
 - drop auto and quick groups
13: new
14: - improve 'average' field of result spec
 - drop extra 'dim = ...' line
15-18: new
19: a lot of changes
20: new
21: add results dump to json


Thanks, applied to my block-next branch (for 6.0):

https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next

Max

Re: [PATCH v7 21/21] scripts/simplebench: add bench_prealloc.py

[Max Reitz]

On 13.11.20 19:00, Vladimir Sementsov-Ogievskiy wrote:

13.11.2020 19:24, Max Reitz wrote:

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Benchmark for new preallocate filter.

Example usage:
 ./bench_prealloc.py ../../build/qemu-img \
 ssd-ext4:/path/to/mount/point \
 ssd-xfs:/path2 hdd-ext4:/path3 hdd-xfs:/path4

The benchmark shows performance improvement (or degradation) when use
new preallocate filter with qcow2 image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench_prealloc.py | 132 ++
  1 file changed, 132 insertions(+)
  create mode 100755 scripts/simplebench/bench_prealloc.py


Reviewed-by: Max Reitz 




Thanks for reviewing! Could you take this all through you tree?


Sure, if you can live with the fact that I’ll be on PTO for the next two 
weeks :)


(Shouldn’t matter for block-next, as 5.2 won’t be released until December.)

Max

Re: [PATCH v7 21/21] scripts/simplebench: add bench_prealloc.py

[Vladimir Sementsov-Ogievskiy]

13.11.2020 19:24, Max Reitz wrote:

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Benchmark for new preallocate filter.

Example usage:
 ./bench_prealloc.py ../../build/qemu-img \
 ssd-ext4:/path/to/mount/point \
 ssd-xfs:/path2 hdd-ext4:/path3 hdd-xfs:/path4

The benchmark shows performance improvement (or degradation) when use
new preallocate filter with qcow2 image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench_prealloc.py | 132 ++
  1 file changed, 132 insertions(+)
  create mode 100755 scripts/simplebench/bench_prealloc.py


Reviewed-by: Max Reitz 




Thanks for reviewing! Could you take this all through you tree?

--
Best regards,
Vladimir

[PATCH v4 0/2] quorum: Implement bdrv_co_block_status()

[Alberto Garcia]

Following Max's suggestion, this version sets supported_zero_flags.

Berto

v4:
- Set supported_zero_flags in quorum [Max]
- Update test to verify the data written before doing 'write -z' [Max]

v3: https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00371.html
- Fall back to BDRV_BLOCK_DATA if a child returns an error.

v2: https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00259.html
- Implement bdrv_co_pwrite_zeroes() for quorum

v1: https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00163.html


Alberto Garcia (2):
  quorum: Implement bdrv_co_block_status()
  quorum: Implement bdrv_co_pwrite_zeroes()

 block/quorum.c |  88 +++-
 tests/qemu-iotests/312 | 159 +
 tests/qemu-iotests/312.out |  75 +
 tests/qemu-iotests/group   |   1 +
 4 files changed, 321 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/312
 create mode 100644 tests/qemu-iotests/312.out

-- 
2.20.1

[PATCH v4 2/2] quorum: Implement bdrv_co_pwrite_zeroes()

[Alberto Garcia]

This simply calls bdrv_co_pwrite_zeroes() in all children.

bs->supported_zero_flags is also set to the flags that are supported
by all children.

Signed-off-by: Alberto Garcia 
---
 block/quorum.c | 36 ++--
 tests/qemu-iotests/312 | 11 +++
 tests/qemu-iotests/312.out |  8 
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 9691a9bee9..b70d365ba9 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -692,8 +692,13 @@ static void write_quorum_entry(void *opaque)
 QuorumChildRequest *sacb = >qcrs[i];
 
 sacb->bs = s->children[i]->bs;
-sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
-acb->qiov, acb->flags);
+if (acb->flags & BDRV_REQ_ZERO_WRITE) {
+sacb->ret = bdrv_co_pwrite_zeroes(s->children[i], acb->offset,
+  acb->bytes, acb->flags);
+} else {
+sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
+acb->qiov, acb->flags);
+}
 if (sacb->ret == 0) {
 acb->success_count++;
 } else {
@@ -739,6 +744,14 @@ static int quorum_co_pwritev(BlockDriverState *bs, 
uint64_t offset,
 return ret;
 }
 
+static int quorum_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
+   int bytes, BdrvRequestFlags flags)
+
+{
+return quorum_co_pwritev(bs, offset, bytes, NULL,
+ flags | BDRV_REQ_ZERO_WRITE);
+}
+
 static int64_t quorum_getlength(BlockDriverState *bs)
 {
 BDRVQuorumState *s = bs->opaque;
@@ -897,6 +910,21 @@ static QemuOptsList quorum_runtime_opts = {
 },
 };
 
+static void quorum_refresh_flags(BlockDriverState *bs)
+{
+BDRVQuorumState *s = bs->opaque;
+int i;
+
+bs->supported_zero_flags =
+BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK;
+
+for (i = 0; i < s->num_children; i++) {
+bs->supported_zero_flags &= s->children[i]->bs->supported_zero_flags;
+}
+
+bs->supported_zero_flags |= BDRV_REQ_WRITE_UNCHANGED;
+}
+
 static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
 {
@@ -991,6 +1019,7 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->next_child_index = s->num_children;
 
 bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED;
+quorum_refresh_flags(bs);
 
 g_free(opened);
 goto exit;
@@ -1062,6 +1091,7 @@ static void quorum_add_child(BlockDriverState *bs, 
BlockDriverState *child_bs,
 }
 s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
 s->children[s->num_children++] = child;
+quorum_refresh_flags(bs);
 
 out:
 bdrv_drained_end(bs);
@@ -1106,6 +1136,7 @@ static void quorum_del_child(BlockDriverState *bs, 
BdrvChild *child,
 s->children = g_renew(BdrvChild *, s->children, --s->num_children);
 bdrv_unref_child(bs, child);
 
+quorum_refresh_flags(bs);
 bdrv_drained_end(bs);
 }
 
@@ -1251,6 +1282,7 @@ static BlockDriver bdrv_quorum = {
 
 .bdrv_co_preadv = quorum_co_preadv,
 .bdrv_co_pwritev= quorum_co_pwritev,
+.bdrv_co_pwrite_zeroes  = quorum_co_pwrite_zeroes,
 
 .bdrv_add_child = quorum_add_child,
 .bdrv_del_child = quorum_del_child,
diff --git a/tests/qemu-iotests/312 b/tests/qemu-iotests/312
index 1b08f1552f..41340494b0 100755
--- a/tests/qemu-iotests/312
+++ b/tests/qemu-iotests/312
@@ -114,6 +114,17 @@ $QEMU_IO -c "write -P 0 $((0x20)) $((0x1))" 
"$TEST_IMG.0" | _filter_qemu
 $QEMU_IO -c "write -z   $((0x20)) $((0x3))" "$TEST_IMG.1" | 
_filter_qemu_io
 $QEMU_IO -c "write -P 0 $((0x20)) $((0x2))" "$TEST_IMG.2" | 
_filter_qemu_io
 
+# Test 5: write data to a region and then zeroize it, doing it
+# directly on the quorum device instead of the individual images.
+# This has no effect on the end result but proves that the quorum driver
+# supports 'write -z'.
+$QEMU_IO -c "open -o $quorum" -c "write -P 1 $((0x25)) $((0x1))" | 
_filter_qemu_io
+# Verify the data that we just wrote
+$QEMU_IO -c "open -o $quorum" -c "read -P 1 $((0x25)) $((0x1))" | 
_filter_qemu_io
+$QEMU_IO -c "open -o $quorum" -c "write -z $((0x25)) $((0x1))" | 
_filter_qemu_io
+# Now it should read back as zeroes
+$QEMU_IO -c "open -o $quorum" -c "read -P 0 $((0x25)) $((0x1))" | 
_filter_qemu_io
+
 echo
 echo '### Launch the drive-mirror job'
 echo
diff --git a/tests/qemu-iotests/312.out b/tests/qemu-iotests/312.out
index 4ae749175b..cdd5c40e08 100644
--- a/tests/qemu-iotests/312.out
+++ b/tests/qemu-iotests/312.out
@@ -39,6 +39,14 @@ wrote 196608/196608 bytes at offset 2097152
 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 131072/131072 bytes at offset 2097152
 128 KiB, X 

[PATCH v4 1/2] quorum: Implement bdrv_co_block_status()

[Alberto Garcia]

The quorum driver does not implement bdrv_co_block_status() and
because of that it always reports to contain data even if all its
children are known to be empty.

One consequence of this is that if we for example create a quorum with
a size of 10GB and we mirror it to a new image the operation will
write 10GB of actual zeroes to the destination image wasting a lot of
time and disk space.

Since a quorum has an arbitrary number of children of potentially
different formats there is no way to report all possible allocation
status flags in a way that makes sense, so this implementation only
reports when a given region is known to contain zeroes
(BDRV_BLOCK_ZERO) or not (BDRV_BLOCK_DATA).

If all children agree that a region contains zeroes then we can return
BDRV_BLOCK_ZERO using the smallest size reported by the children
(because all agree that a region of at least that size contains
zeroes).

If at least one child disagrees we have to return BDRV_BLOCK_DATA.
In this case we use the largest of the sizes reported by the children
that didn't return BDRV_BLOCK_ZERO (because we know that there won't
be an agreement for at least that size).

Signed-off-by: Alberto Garcia 
Tested-by: Tao Xu 
Reviewed-by: Max Reitz 
---
 block/quorum.c |  52 +
 tests/qemu-iotests/312 | 148 +
 tests/qemu-iotests/312.out |  67 +
 tests/qemu-iotests/group   |   1 +
 4 files changed, 268 insertions(+)
 create mode 100755 tests/qemu-iotests/312
 create mode 100644 tests/qemu-iotests/312.out

diff --git a/block/quorum.c b/block/quorum.c
index e846a7e892..9691a9bee9 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -18,6 +18,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "block/block_int.h"
+#include "block/coroutines.h"
 #include "block/qdict.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-block.h"
@@ -1174,6 +1175,56 @@ static void quorum_child_perm(BlockDriverState *bs, 
BdrvChild *c,
  | DEFAULT_PERM_UNCHANGED;
 }
 
+/*
+ * Each one of the children can report different status flags even
+ * when they contain the same data, so what this function does is
+ * return BDRV_BLOCK_ZERO if *all* children agree that a certain
+ * region contains zeroes, and BDRV_BLOCK_DATA otherwise.
+ */
+static int coroutine_fn quorum_co_block_status(BlockDriverState *bs,
+   bool want_zero,
+   int64_t offset, int64_t count,
+   int64_t *pnum, int64_t *map,
+   BlockDriverState **file)
+{
+BDRVQuorumState *s = bs->opaque;
+int i, ret;
+int64_t pnum_zero = count;
+int64_t pnum_data = 0;
+
+for (i = 0; i < s->num_children; i++) {
+int64_t bytes;
+ret = bdrv_co_common_block_status_above(s->children[i]->bs, NULL, 
false,
+want_zero, offset, count,
+, NULL, NULL, NULL);
+if (ret < 0) {
+quorum_report_bad(QUORUM_OP_TYPE_READ, offset, count,
+  s->children[i]->bs->node_name, ret);
+pnum_data = count;
+break;
+}
+/*
+ * Even if all children agree about whether there are zeroes
+ * or not at @offset they might disagree on the size, so use
+ * the smallest when reporting BDRV_BLOCK_ZERO and the largest
+ * when reporting BDRV_BLOCK_DATA.
+ */
+if (ret & BDRV_BLOCK_ZERO) {
+pnum_zero = MIN(pnum_zero, bytes);
+} else {
+pnum_data = MAX(pnum_data, bytes);
+}
+}
+
+if (pnum_data) {
+*pnum = pnum_data;
+return BDRV_BLOCK_DATA;
+} else {
+*pnum = pnum_zero;
+return BDRV_BLOCK_ZERO;
+}
+}
+
 static const char *const quorum_strong_runtime_opts[] = {
 QUORUM_OPT_VOTE_THRESHOLD,
 QUORUM_OPT_BLKVERIFY,
@@ -1192,6 +1243,7 @@ static BlockDriver bdrv_quorum = {
 .bdrv_close = quorum_close,
 .bdrv_gather_child_options  = quorum_gather_child_options,
 .bdrv_dirname   = quorum_dirname,
+.bdrv_co_block_status   = quorum_co_block_status,
 
 .bdrv_co_flush_to_disk  = quorum_co_flush,
 
diff --git a/tests/qemu-iotests/312 b/tests/qemu-iotests/312
new file mode 100755
index 00..1b08f1552f
--- /dev/null
+++ b/tests/qemu-iotests/312
@@ -0,0 +1,148 @@
+#!/usr/bin/env bash
+#
+# Test drive-mirror with quorum
+#
+# The goal of this test is to check how the quorum driver reports
+# regions that are known to read as zeroes (BDRV_BLOCK_ZERO). The idea
+# is that drive-mirror will try the efficient representation of zeroes
+# in the destination image instead of writing actual zeroes.
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia 

Re: [PATCH v3 2/2] quorum: Implement bdrv_co_pwrite_zeroes()

[Max Reitz]

On 13.11.20 17:26, Alberto Garcia wrote:

On Fri 13 Nov 2020 05:11:20 PM CET, Max Reitz wrote:


We could set all supported_zero_flags as long as all children support
them, right?


Sure, I was just thinking that we could set these regardless of
whether the children support them, because (on zero-writes) the block
layer will figure out for us whether the child nodes support them. O:)


But it can happen that one child supports e.g. BDRV_REQ_NO_FALLBACK but
the rest don't. In this case I think the block layer should return
-ENOTSUP earlier without writing to the child(ren) that do support that
flag.


That’s true.


So Quorum's supported_zero_flags would be the logical and of all of its
children's flags, right?


Yes.  (And so it would need to be recalculated whenever a child is added 
or removed.)



I'm unsure about BDRV_REQ_WRITE_UNCHANGED, many filters set that on top
of the other flags, but when would a BDS not support this flag?


The WRITE_UNCHANGED flag is basically just a hint for the permission 
system that for such a write, the WRITE_UNCHANGED permission is 
sufficient.  So drivers that can pass through *all* WRITE_UNCHANGED 
requests as they are (i.e., filter drivers) can support this write/zero 
flag and then pass that flag on.  This way, they are safe not to take 
the WRITE permission on their child unless their parent has taken the 
WRITE permission on them.


(Otherwise, they’d also have to take the WRITE permission if the parent 
only holds the WRITE_UNCHANGED permission.)


Supporting this flag doesn’t make sense for drivers that won’t be able 
to pass it on all the time.  For example, qcow2 generally cannot pass it 
on, because it still needs to perform WRITE_UNCHANGED requests as normal 
writes.  (WRITE_UNCHANGED comes from copy-on-read; the data will read 
the same, hence the _UNCHANGED, but it still needs to be allocated on 
the receiving format layer.)


(Technically, qcow2 could figure out whether the requests it generates 
from a WRITE_UNCHANGED request still result in unchanging writes in the 
protocol layer, but there is no reason to.  It needs the WRITE 
permission anyway, because there are going to be some non-unchanging 
writes it has to perform.  And if it holds the WRITE permission, there 
is no need to bother with the WRITE_UNCHANGED flag.)



pwrite_zeroes() does this additionaly:

  if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
  flags &= ~BDRV_REQ_MAY_UNMAP;
  }


Interesting.  Technically, Quorum doesn’t support that flag (in
supported_zero_flags O:))), so it shouldn’t appear, but, er, well
then.


It would with the change that I'm proposing above.


Yes, I know.  Hence the “O:)”. O:)

Max

Re: [PATCH v3 2/2] quorum: Implement bdrv_co_pwrite_zeroes()

[Alberto Garcia]

On Fri 13 Nov 2020 05:11:20 PM CET, Max Reitz wrote:

>> We could set all supported_zero_flags as long as all children support
>> them, right?
>
> Sure, I was just thinking that we could set these regardless of
> whether the children support them, because (on zero-writes) the block
> layer will figure out for us whether the child nodes support them. O:)

But it can happen that one child supports e.g. BDRV_REQ_NO_FALLBACK but
the rest don't. In this case I think the block layer should return
-ENOTSUP earlier without writing to the child(ren) that do support that
flag.

So Quorum's supported_zero_flags would be the logical and of all of its
children's flags, right?

I'm unsure about BDRV_REQ_WRITE_UNCHANGED, many filters set that on top
of the other flags, but when would a BDS not support this flag?

>> pwrite_zeroes() does this additionaly:
>> 
>>  if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
>>  flags &= ~BDRV_REQ_MAY_UNMAP;
>>  }
>
> Interesting.  Technically, Quorum doesn’t support that flag (in
> supported_zero_flags O:))), so it shouldn’t appear, but, er, well
> then.

It would with the change that I'm proposing above.

Berto

Re: [PATCH v7 21/21] scripts/simplebench: add bench_prealloc.py

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Benchmark for new preallocate filter.

Example usage:
 ./bench_prealloc.py ../../build/qemu-img \
 ssd-ext4:/path/to/mount/point \
 ssd-xfs:/path2 hdd-ext4:/path3 hdd-xfs:/path4

The benchmark shows performance improvement (or degradation) when use
new preallocate filter with qcow2 image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench_prealloc.py | 132 ++
  1 file changed, 132 insertions(+)
  create mode 100755 scripts/simplebench/bench_prealloc.py


Reviewed-by: Max Reitz 

Re: [PATCH v7 18/21] simplebench/results_to_text: improve view of the table

[Vladimir Sementsov-Ogievskiy]

13.11.2020 18:59, Max Reitz wrote:

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Move to generic format for floats and percentage for error.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/results_to_text.py | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/scripts/simplebench/results_to_text.py 
b/scripts/simplebench/results_to_text.py
index 58d909ffd9..479f7ac1d4 100644
--- a/scripts/simplebench/results_to_text.py
+++ b/scripts/simplebench/results_to_text.py
@@ -16,11 +16,22 @@
  # along with this program.  If not, see .
  #
+import math
+
+
+def format_value(x, stdev):
+    stdev_pr = stdev / x * 100
+    if stdev_pr < 1.5:
+    # don't care too much
+    return f'{x:.2g}'
+    else:
+    return f'{x:.2g} ± {math.ceil(stdev_pr)}%'


OK, so no magnitude-based precision this time (except for the %f -> %g change). 
 Works for me.

Other than that, I personally don’t like the relative standard deviation much, 
because the absolute SD immediately shows the 68 % boundaries (and an idea on 
the 95 % boundaries with 2σ), whereas the RSD just gives an impression on how 
spread out the data is.  (Which I find the absolute SD also does, when given 
together with the average, which is the case here.)


I realized that for myself, the only thing I need from +-deviation is a kind of 
"reliability" of the result. And it's more obvious for me in percentage. 
Looking at the table with a lot of numbers, where most of values have deviation less than 
+-5%, some values with deviation +-30 would catch the eye immediately. And with absolute 
SD I have to look through the table more carefully.



To be completely honest, though, I didn’t even know the term “relative SD” 
existed until a couple of minutes ago, and I didn’t know it was something that 
was used at all.
And if I haven’t seen the RSD used in practice, I can’t confidently say that I 
have good reasons not to like it.

But, well, I can’t have any confidence in liking it either, and because the 
change from ASD to RSD is basically the most important change of this patch 
(which I can’t really agree is an improvement), I can’t really give an R-b.

Perhaps this is OK:

Acked-by: Max Reitz 



Thanks!

I don't have strict opinion about how this should look. For now these scripts 
don't have wide usage... We can always add an option to adjust table view, and 
I will not mind making absolute SD the default view.


--
Best regards,
Vladimir

Re: [PATCH v7 20/21] simplebench/results_to_text: make executable

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Make results_to_text a tool to dump results saved in JSON file.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/results_to_text.py | 14 ++
  1 file changed, 14 insertions(+)
  mode change 100644 => 100755 scripts/simplebench/results_to_text.py


Reviewed-by: Max Reitz 

Re: [PATCH v7 19/21] simplebench/results_to_text: add difference line to the table

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Performance improvements / degradations are usually discussed in
percentage. Let's make the script calculate it for us.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/results_to_text.py | 67 +++---
  1 file changed, 60 insertions(+), 7 deletions(-)

diff --git a/scripts/simplebench/results_to_text.py 
b/scripts/simplebench/results_to_text.py
index 479f7ac1d4..56fdacf7ca 100644
--- a/scripts/simplebench/results_to_text.py
+++ b/scripts/simplebench/results_to_text.py


[...]


+for j in range(0, i):
+env_j = results['envs'][j]
+res_j = case_results[env_j['id']]
+cell += ' '
+
+if 'average' not in res_j:
+# Failed result
+cell += '--'
+continue
+
+col_j = tab[0][j + 1] if named_columns else ''
+diff_pr = round((res['average'] - res_j['average']) /
+res_j['average'] * 100)
+cell += f' {col_j}{diff_pr:+}%'


Contrasting to v6, you added the "cell += ' '" line, dropped a space in 
the "cell += '--'" line (was: "cell += ' --'"), but kept the space here. 
 I would have assumed that the leading space is dropped here, too.  But 
I don’t quite know, what I should be expecting, so.


Anyway, I’ll just leave this here:

Reviewed-by: Max Reitz 


+row.append(cell)
+tab.append(row)
+
+return f'All results are in {dim}\n\n' + tabulate.tabulate(tab)

Re: [PATCH v3 2/2] quorum: Implement bdrv_co_pwrite_zeroes()

[Max Reitz]

On 13.11.20 17:07, Alberto Garcia wrote:

On Fri 13 Nov 2020 12:49:04 PM CET, Max Reitz wrote:

On 11.11.20 17:53, Alberto Garcia wrote:

This simply calls bdrv_co_pwrite_zeroes() in all children

Signed-off-by: Alberto Garcia 
---
   block/quorum.c | 18 --
   tests/qemu-iotests/312 |  7 +++
   tests/qemu-iotests/312.out |  4 
   3 files changed, 27 insertions(+), 2 deletions(-)


Should we set supported_zero_flags to something?  I think we can at
least set BDRV_REQ_NO_FALLBACK.  We could also try BDRV_REQ_FUA.


We could set all supported_zero_flags as long as all children support
them, right?


Sure, I was just thinking that we could set these regardless of whether 
the children support them, because (on zero-writes) the block layer will 
figure out for us whether the child nodes support them. O:)



+if (acb->flags & BDRV_REQ_ZERO_WRITE) {
+sacb->ret = bdrv_co_pwrite_zeroes(s->children[i], acb->offset,
+  acb->bytes, acb->flags);
+} else {
+sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
+acb->qiov, acb->flags);
+}


Seems unnecessary (bdrv_co_pwritev() can handle BDRV_REQ_ZERO_WRITE),
but perhaps it’s good to be explicit.


pwrite_zeroes() does this additionaly:

 if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
 flags &= ~BDRV_REQ_MAY_UNMAP;
 }


Interesting.  Technically, Quorum doesn’t support that flag (in 
supported_zero_flags O:))), so it shouldn’t appear, but, er, well then.


Max

Re: [PATCH v3 2/2] quorum: Implement bdrv_co_pwrite_zeroes()

[Alberto Garcia]

On Fri 13 Nov 2020 12:49:04 PM CET, Max Reitz wrote:
> On 11.11.20 17:53, Alberto Garcia wrote:
>> This simply calls bdrv_co_pwrite_zeroes() in all children
>> 
>> Signed-off-by: Alberto Garcia 
>> ---
>>   block/quorum.c | 18 --
>>   tests/qemu-iotests/312 |  7 +++
>>   tests/qemu-iotests/312.out |  4 
>>   3 files changed, 27 insertions(+), 2 deletions(-)
>
> Should we set supported_zero_flags to something?  I think we can at 
> least set BDRV_REQ_NO_FALLBACK.  We could also try BDRV_REQ_FUA.

We could set all supported_zero_flags as long as all children support
them, right? 

>> +if (acb->flags & BDRV_REQ_ZERO_WRITE) {
>> +sacb->ret = bdrv_co_pwrite_zeroes(s->children[i], acb->offset,
>> +  acb->bytes, acb->flags);
>> +} else {
>> +sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
>> +acb->qiov, acb->flags);
>> +}
>
> Seems unnecessary (bdrv_co_pwritev() can handle BDRV_REQ_ZERO_WRITE), 
> but perhaps it’s good to be explicit.

pwrite_zeroes() does this additionaly:

if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
flags &= ~BDRV_REQ_MAY_UNMAP;
}

Berto

Re: [PATCH v7 18/21] simplebench/results_to_text: improve view of the table

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Move to generic format for floats and percentage for error.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/results_to_text.py | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/scripts/simplebench/results_to_text.py 
b/scripts/simplebench/results_to_text.py
index 58d909ffd9..479f7ac1d4 100644
--- a/scripts/simplebench/results_to_text.py
+++ b/scripts/simplebench/results_to_text.py
@@ -16,11 +16,22 @@
  # along with this program.  If not, see .
  #
  
+import math

+
+
+def format_value(x, stdev):
+stdev_pr = stdev / x * 100
+if stdev_pr < 1.5:
+# don't care too much
+return f'{x:.2g}'
+else:
+return f'{x:.2g} ± {math.ceil(stdev_pr)}%'


OK, so no magnitude-based precision this time (except for the %f -> %g 
change).  Works for me.


Other than that, I personally don’t like the relative standard deviation 
much, because the absolute SD immediately shows the 68 % boundaries (and 
an idea on the 95 % boundaries with 2σ), whereas the RSD just gives an 
impression on how spread out the data is.  (Which I find the absolute SD 
also does, when given together with the average, which is the case here.)


To be completely honest, though, I didn’t even know the term “relative 
SD” existed until a couple of minutes ago, and I didn’t know it was 
something that was used at all.
And if I haven’t seen the RSD used in practice, I can’t confidently say 
that I have good reasons not to like it.


But, well, I can’t have any confidence in liking it either, and because 
the change from ASD to RSD is basically the most important change of 
this patch (which I can’t really agree is an improvement), I can’t 
really give an R-b.


Perhaps this is OK:

Acked-by: Max Reitz 

  
  def result_to_text(result):

  """Return text representation of bench_one() returned dict."""
  if 'average' in result:
-s = '{:.2f} +- {:.2f}'.format(result['average'], result['stdev'])
+s = format_value(result['average'], result['stdev'])
  if 'n-failed' in result:
  s += '\n({} failed)'.format(result['n-failed'])
  return s

Re: [PATCH v7 08/21] block: introduce preallocate filter

[Vladimir Sementsov-Ogievskiy]

13.11.2020 17:32, Max Reitz wrote:

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

It's intended to be inserted between format and protocol nodes to
preallocate additional space (expanding protocol file) on writes
crossing EOF. It improves performance for file-systems with slow
allocation.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  docs/system/qemu-block-drivers.rst.inc |  26 ++
  qapi/block-core.json   |  20 +-
  block/preallocate.c    | 559 +
  block/meson.build  |   1 +
  4 files changed, 605 insertions(+), 1 deletion(-)
  create mode 100644 block/preallocate.c


[...]


+    if (end <= s->file_end) {
+    /* No preallocation needed. */
+    return want_merge_zero && offset >= s->zero_start;
+    }
+
+    /* Now we want new preallocation, as request writes beyond s->data_end. */


True, but isn’t s->file_end more important?


Yes, file_end should be here.




+
+    prealloc_start = want_merge_zero ? MIN(offset, s->file_end) : s->file_end;
+    prealloc_end = QEMU_ALIGN_UP(end + s->opts.prealloc_size,
+ s->opts.prealloc_align);


[...]


+    if (prealloc == PREALLOC_MODE_FALLOC) {
+    /*
+ * If offset <= s->file_end, the task is already done, just
+ * update s->file_end, to move part of "filter preallocation"


s/file_end/data_end/


yes



With those fixed, and with %s/5\.2/6.0/:

Reviewed-by: Max Reitz 


Thanks!


--
Best regards,
Vladimir

[PATCH] io_uring: do not use pointer after free

[Paolo Bonzini]

Even though only the pointer value is only printed, it is untidy
and Coverity complains.

Cc: Stefan Hajnoczi 
Signed-off-by: Paolo Bonzini 
---
 block/io_uring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io_uring.c b/block/io_uring.c
index 037af09471..00a3ee9fb8 100644
--- a/block/io_uring.c
+++ b/block/io_uring.c
@@ -425,6 +425,6 @@ LuringState *luring_init(Error **errp)
 void luring_cleanup(LuringState *s)
 {
 io_uring_queue_exit(>ring);
-g_free(s);
 trace_luring_cleanup_state(s);
+g_free(s);
 }
-- 
2.26.2

Re: [PATCH v7 17/21] simplebench: move results_to_text() into separate file

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Let's keep view part in separate: this way it's better to improve it in
the following commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench-example.py   |  3 +-
  scripts/simplebench/bench_write_req.py |  3 +-
  scripts/simplebench/results_to_text.py | 48 ++
  scripts/simplebench/simplebench.py | 31 -
  4 files changed, 52 insertions(+), 33 deletions(-)
  create mode 100644 scripts/simplebench/results_to_text.py


Reviewed-by: Max Reitz 

Re: [PATCH v7 16/21] simplebench: rename ascii() to results_to_text()

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Next patch will use utf8 plus-minus symbol, let's use more generic (and
more readable) name.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench-example.py   |  2 +-
  scripts/simplebench/bench_write_req.py |  2 +-
  scripts/simplebench/simplebench.py | 10 +-
  3 files changed, 7 insertions(+), 7 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v7 15/21] scripts/simplebench: use standard deviation for +- error

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Standard deviation is more usual to see after +- than current maximum
of deviations.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/simplebench.py | 11 ++-
  1 file changed, 6 insertions(+), 5 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v7 14/21] scripts/simplebench: support iops

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Support benchmarks returning not seconds but iops. We'll use it for
further new test.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/simplebench.py | 38 ++
  1 file changed, 28 insertions(+), 10 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v7 13/21] scripts/simplebench: fix grammar: s/successed/succeeded/

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

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


Reviewed-by: Max Reitz 

Re: [PATCH v7 12/21] iotests: add 298 to test new preallocate filter driver

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/298 | 186 +
  tests/qemu-iotests/298.out |   5 +
  tests/qemu-iotests/group   |   1 +
  3 files changed, 192 insertions(+)
  create mode 100644 tests/qemu-iotests/298
  create mode 100644 tests/qemu-iotests/298.out


Reviewed-by: Max Reitz 

Re: [PATCH for-5.2] iotests: Replace deprecated ConfigParser.readfp()

[Eric Blake]

On 11/13/20 8:40 AM, Kevin Wolf wrote:
> Am 13.11.2020 um 14:47 hat Eric Blake geschrieben:
>> On 11/13/20 4:06 AM, Kevin Wolf wrote:
>>> iotest 277 fails on Fedora 33 (Python 3.9) because a deprecation warning
>>> changes the output:
>>>
>>> nbd-fault-injector.py:230: DeprecationWarning: This method will be
>>> removed in future versions.  Use 'parser.read_file()' instead.
>>>
>>> In fact, readfp() has already been deprecated in Python 3.2 and the
>>> replacement has existed since the same version, so we can now
>>> unconditionally switch to read_file().
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  tests/qemu-iotests/nbd-fault-injector.py | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Reviewed-by: Eric Blake 
>>
>> I'm happy to queue this through my NBD tree for -rc2.
> 
> If you don't have other patches to send, I can take it through my own
> tree, but if you send a pull request anyway, that's fine, too.

I've got at least one other NBD patch, and am double-checking my trees
and flagged inbox messages to see if there are any other last-minute
-rc2 candidates, so I will be sending a Pull Request on Monday.

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

Re: [PATCH v7 11/21] iotests.py: execute_setup_common(): add required_fmts argument

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Add a parameter to skip test if some needed additional formats are not
supported (for example filter drivers).

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/iotests.py | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)


Reviewed-by: Max Reitz 

Re: [PATCH for-5.2] iotests: Replace deprecated ConfigParser.readfp()

[Kevin Wolf]

Am 13.11.2020 um 14:47 hat Eric Blake geschrieben:
> On 11/13/20 4:06 AM, Kevin Wolf wrote:
> > iotest 277 fails on Fedora 33 (Python 3.9) because a deprecation warning
> > changes the output:
> > 
> > nbd-fault-injector.py:230: DeprecationWarning: This method will be
> > removed in future versions.  Use 'parser.read_file()' instead.
> > 
> > In fact, readfp() has already been deprecated in Python 3.2 and the
> > replacement has existed since the same version, so we can now
> > unconditionally switch to read_file().
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  tests/qemu-iotests/nbd-fault-injector.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake 
> 
> I'm happy to queue this through my NBD tree for -rc2.

If you don't have other patches to send, I can take it through my own
tree, but if you send a pull request anyway, that's fine, too.

Kevin

Re: [PATCH v7 08/21] block: introduce preallocate filter

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

It's intended to be inserted between format and protocol nodes to
preallocate additional space (expanding protocol file) on writes
crossing EOF. It improves performance for file-systems with slow
allocation.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  docs/system/qemu-block-drivers.rst.inc |  26 ++
  qapi/block-core.json   |  20 +-
  block/preallocate.c| 559 +
  block/meson.build  |   1 +
  4 files changed, 605 insertions(+), 1 deletion(-)
  create mode 100644 block/preallocate.c


[...]


+if (end <= s->file_end) {
+/* No preallocation needed. */
+return want_merge_zero && offset >= s->zero_start;
+}
+
+/* Now we want new preallocation, as request writes beyond s->data_end. */


True, but isn’t s->file_end more important?


+
+prealloc_start = want_merge_zero ? MIN(offset, s->file_end) : s->file_end;
+prealloc_end = QEMU_ALIGN_UP(end + s->opts.prealloc_size,
+ s->opts.prealloc_align);


[...]


+if (prealloc == PREALLOC_MODE_FALLOC) {
+/*
+ * If offset <= s->file_end, the task is already done, just
+ * update s->file_end, to move part of "filter preallocation"


s/file_end/data_end/

With those fixed, and with %s/5\.2/6.0/:

Reviewed-by: Max Reitz 


+ * to "preallocation requested by user".
+ * Otherwise just proceed to preallocate missing part.
+ */
+if (offset <= s->file_end) {
+s->data_end = offset;
+return 0;
+}

Re: [PATCH v7 07/21] block: bdrv_check_perm(): process children anyway

[Max Reitz]

On 21.10.20 16:58, Vladimir Sementsov-Ogievskiy wrote:

Do generic processing even for drivers which define .bdrv_check_perm
handler. It's needed for further preallocate filter: it will need to do
additional action on bdrv_check_perm, but don't want to reimplement
generic logic.

The patch doesn't change existing behaviour: the only driver that
implements bdrv_check_perm is file-posix, but it never has any
children.

Also, bdrv_set_perm() don't stop processing if driver has
.bdrv_set_perm handler as well.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH for-5.2] iotests: Replace deprecated ConfigParser.readfp()

[Eric Blake]

On 11/13/20 4:06 AM, Kevin Wolf wrote:
> iotest 277 fails on Fedora 33 (Python 3.9) because a deprecation warning
> changes the output:
> 
> nbd-fault-injector.py:230: DeprecationWarning: This method will be
> removed in future versions.  Use 'parser.read_file()' instead.
> 
> In fact, readfp() has already been deprecated in Python 3.2 and the
> replacement has existed since the same version, so we can now
> unconditionally switch to read_file().
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/nbd-fault-injector.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 

I'm happy to queue this through my NBD tree for -rc2.

> 
> diff --git a/tests/qemu-iotests/nbd-fault-injector.py 
> b/tests/qemu-iotests/nbd-fault-injector.py
> index 78f42c4214..6e11ef89b8 100755
> --- a/tests/qemu-iotests/nbd-fault-injector.py
> +++ b/tests/qemu-iotests/nbd-fault-injector.py
> @@ -227,7 +227,7 @@ def parse_config(config):
>  def load_rules(filename):
>  config = configparser.RawConfigParser()
>  with open(filename, 'rt') as f:
> -config.readfp(f, filename)
> +config.read_file(f, filename)
>  return parse_config(config)
>  
>  def open_socket(path):
> 

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

Re: [PATCH] docs: Better mention of qemu-img amend limitations

[Eric Blake]

On 11/13/20 3:49 AM, Kevin Wolf wrote:
> Am 23.09.2020 um 22:37 hat Eric Blake geschrieben:
>> Missed during merge resolution of commit bc5ee6da71.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  docs/tools/qemu-img.rst | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
>> index c35bd6482203..2b5891b54db7 100644
>> --- a/docs/tools/qemu-img.rst
>> +++ b/docs/tools/qemu-img.rst
>> @@ -265,6 +265,10 @@ Command description:
> 
> Adding a little more context:
> 
>>The set of options that can be amended are dependent on the image
>>format, but note that amending the backing chain relationship should
>>instead be performed with ``qemu-img rebase``.
>>
>>--force allows some unsafe operations. Currently for -f luks, it allows to
>>erase the last encryption key, and to overwrite an active encryption key.
>>
>> +  The set of options that can be amended are dependent on the image
>> +  format, but note that amending the backing chain relationship should
>> +  instead be performed with ``qemu-img rebase``.
>> +
> 
> I think the problem is your local conflict resolution. This patch would
> duplicate the paragraph.

D'oh.  Thanks for spotting that, patch withdrawn.


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

Re: [PATCH 03/10] block: Improve some block-commit, block-stream error messages

[Max Reitz]

On 13.11.20 09:26, Markus Armbruster wrote:

block-commit defaults @base-node to the deepest backing image.  When
there is none, it fails with "Base 'NULL' not found".  Improve to
"There is no backing image".

block-commit and block-stream reject a @base argument that doesn't
resolve with "Base 'BASE' not found".  Commit 6b33f3ae8b "qemu-img:
Improve commit invalid base message" improved this message in
qemu-img.  Improve it here, too: "Can't find '%s' in the backing
chain".

QERR_BASE_NOT_FOUND is now unused.  Drop.

Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster 
---
  include/qapi/qmp/qerror.h |  2 --
  blockdev.c| 15 +--
  tests/qemu-iotests/040| 12 ++--
  3 files changed, 15 insertions(+), 14 deletions(-)


Reviewed-by: Max Reitz 

[PATCH RFC 0/5] block: implement Parallels Disk format driver

[Vladimir Sementsov-Ogievskiy]

Hi all!

I just send these old patches as they can be useful. I'm not the author
of the code and not going to discuss them. "RFC" is here just to mark
the series as "not-for-applying-to-master". So, please don't answer
here. If you want to continue this work, post v2 first.

Virtuozzo doesn't have plans on improving Parallels disk format
support in Qemu. Still, if someone want to work on it, these patches may
help.

Note that patches don't apply to master, they need a rebase. I've
applied them onto b0292b851b85ba81c0cfedf5b576c32189cfaa11, to run git
send-email. So, you may start from applying to same commit and rebasing
onto current master if you want.

Note also that Edgar and Klim are not in Virtuozzo team for now and
their @virtuozzo.com emails are invalid.

Edgar Kaziakhmedov (1):
  iotests: add test for prl-xml format

Klim Kireev (4):
  block/prl-xml: add Parallels xml BlockDriver
  block/prl-xml: add bdrv_co_readv/writev and flush
  block/prl-xml: add bdrv_probe
  block/prl-xml: add bdrv_check

 block/prl-xml.c   | 560 ++
 block/Makefile.objs   |   5 +-
 tests/qemu-iotests/164|  98 +++
 tests/qemu-iotests/164.out|  54 ++
 tests/qemu-iotests/check  |   7 +
 tests/qemu-iotests/group  |   1 +
 .../prl-xml/DiskDescriptor.xml.bz2| Bin 0 -> 457 bytes
 .../sample_images/prl-xml/Snapshots.xml.bz2   | Bin 0 -> 307 bytes
 ...aabe3-6958-40ff-92a7-860e329aab41}.hds.bz2 | Bin 0 -> 93 bytes
 ...476cf-d62e-45d1-b355-86feca91376e}.hds.bz2 | Bin 0 -> 93 bytes
 10 files changed, 723 insertions(+), 2 deletions(-)
 create mode 100644 block/prl-xml.c
 create mode 100755 tests/qemu-iotests/164
 create mode 100644 tests/qemu-iotests/164.out
 create mode 100644 
tests/qemu-iotests/sample_images/prl-xml/DiskDescriptor.xml.bz2
 create mode 100644 tests/qemu-iotests/sample_images/prl-xml/Snapshots.xml.bz2
 create mode 100644 
tests/qemu-iotests/sample_images/prl-xml/parallels.{5fbaabe3-6958-40ff-92a7-860e329aab41}.hds.bz2
 create mode 100644 
tests/qemu-iotests/sample_images/prl-xml/parallels.{986476cf-d62e-45d1-b355-86feca91376e}.hds.bz2

-- 
2.21.3

[PATCH 5/5] iotests: add test for prl-xml format

[Vladimir Sementsov-Ogievskiy]

From: Edgar Kaziakhmedov 

Signed-off-by: Edgar Kaziakhmedov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/164|  98 ++
 tests/qemu-iotests/164.out|  54 ++
 tests/qemu-iotests/check  |   7 ++
 tests/qemu-iotests/group  |   1 +
 .../prl-xml/DiskDescriptor.xml.bz2| Bin 0 -> 457 bytes
 .../sample_images/prl-xml/Snapshots.xml.bz2   | Bin 0 -> 307 bytes
 ...aabe3-6958-40ff-92a7-860e329aab41}.hds.bz2 | Bin 0 -> 93 bytes
 ...476cf-d62e-45d1-b355-86feca91376e}.hds.bz2 | Bin 0 -> 93 bytes
 8 files changed, 160 insertions(+)
 create mode 100755 tests/qemu-iotests/164
 create mode 100644 tests/qemu-iotests/164.out
 create mode 100644 
tests/qemu-iotests/sample_images/prl-xml/DiskDescriptor.xml.bz2
 create mode 100644 tests/qemu-iotests/sample_images/prl-xml/Snapshots.xml.bz2
 create mode 100644 
tests/qemu-iotests/sample_images/prl-xml/parallels.{5fbaabe3-6958-40ff-92a7-860e329aab41}.hds.bz2
 create mode 100644 
tests/qemu-iotests/sample_images/prl-xml/parallels.{986476cf-d62e-45d1-b355-86feca91376e}.hds.bz2

diff --git a/tests/qemu-iotests/164 b/tests/qemu-iotests/164
new file mode 100755
index 00..a55ab5d7d8
--- /dev/null
+++ b/tests/qemu-iotests/164
@@ -0,0 +1,98 @@
+#!/bin/bash
+#
+# prl-xml format validation test
+#
+# Copyright (C) 2017 Edgar Kaziakhmedov 
+#
+# 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=edgar.kaziakhme...@virtuozzo.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+_rm_test_img "$TEST_DIR/$PRL_XML_DIR/$CUR_IMAGE"
+_rm_test_img "$TEST_DIR/$PRL_XML_DIR/$BACK_IMAGE"
+_rm_test_img "$TEST_DIR/$PRL_XML_DIR/$SNAP_LIST"
+_cleanup_test_img
+rmdir "$TEST_DIR/$PRL_XML_DIR"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt prl-xml
+_supported_proto file
+_supported_os Linux
+
+inuse_offset=$((0x2c))
+
+PRL_XML_DIR="prl-xml"
+mkdir "$TEST_DIR/$PRL_XML_DIR"
+CUR_IMAGE="parallels.{5fbaabe3-6958-40ff-92a7-860e329aab41}.hds"
+BACK_IMAGE="parallels.{986476cf-d62e-45d1-b355-86feca91376e}.hds"
+SNAP_LIST="Snapshots.xml"
+XML_IMG="DiskDescriptor.xml"
+size=128M
+CLUSTER_SIZE=64k
+IMGFMT=prl-xml
+_use_sample_img "$PRL_XML_DIR/$CUR_IMAGE.bz2"
+_use_sample_img "$PRL_XML_DIR/$BACK_IMAGE.bz2"
+_use_sample_img "$PRL_XML_DIR/$SNAP_LIST.bz2"
+_use_sample_img "$PRL_XML_DIR/$XML_IMG.bz2"
+
+echo == read empty image ==
+{ $QEMU_IO -c "read -P 0 32k 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == write more than 1 block in a row ==
+{ $QEMU_IO -c "write -P 0x11 32k 128k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == read less than block ==
+{ $QEMU_IO -c "read -P 0x69 32k 32k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == read exactly 1 block ==
+{ $QEMU_IO -c "read -P 0x69 64k 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == read more than 1 block ==
+{ $QEMU_IO -c "read -P 0x69 32k 128k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == check that there is no trash after written ==
+{ $QEMU_IO -c "read -P 0 160k 32k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == check that there is no trash before written ==
+{ $QEMU_IO -c "read -P 0 0 32k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == write the whole disk ==
+{ $QEMU_IO -c "write -P 0x45 0 2M" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == read the whole disk ==
+{ $QEMU_IO -c "read -P 0x45 0 2M" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == check that there is error while write out of range ==
+{ $QEMU_IO -c "write -P 0 2M 32k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+echo == check that there is error while disk size is less than data written ==
+{ $QEMU_IO -c "write -P 0x45 0 3M" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+
+echo "== Corrupt image =="
+poke_file "$TEST_DIR/$PRL_XML_DIR/$CUR_IMAGE" "$inuse_offset" 
"\x59\x6e\x6f\x74"
+{ $QEMU_IO -c "read -P 0x11 64k 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+# as it is a block filter, we change driver format
+_check_test_img
+_check_test_img 

[PATCH 4/5] block/prl-xml: add bdrv_check

[Vladimir Sementsov-Ogievskiy]

From: Klim Kireev 

Add bdrv_check, which just checks child image and all backings.

Signed-off-by: Klim Kireev 
---
 block/prl-xml.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/block/prl-xml.c b/block/prl-xml.c
index 023651342c..736cd98469 100644
--- a/block/prl-xml.c
+++ b/block/prl-xml.c
@@ -520,6 +520,23 @@ static coroutine_fn int 
prl_co_flush_to_os(BlockDriverState *bs)
 return bdrv_co_flush(s->image->bs);
 }
 
+static coroutine_fn int prl_co_check_xml(BlockDriverState *bs, 
+BdrvCheckResult *result,
+BdrvCheckMode fix) {
+BDRVPrlXmlState *s = bs->opaque;
+BdrvChild *cur = s->image;
+int ret = 0;
+
+while (cur != NULL && ret >= 0) {
+if (cur->bs->drv->bdrv_co_check != NULL) {
+ret = bdrv_check(cur->bs, result, fix);
+}
+cur = cur->bs->backing;
+}
+
+return ret;
+}
+
 static BlockDriver bdrv_prl_xml = {
 .format_name= "prl-xml",
 .instance_size  = sizeof(BDRVPrlXmlState),
@@ -529,6 +546,7 @@ static BlockDriver bdrv_prl_xml = {
 .bdrv_co_writev = prl_co_writev,
 .bdrv_close = prl_close_xml,
 .bdrv_co_flush_to_os= prl_co_flush_to_os,
+.bdrv_co_check  = prl_co_check_xml,
 .create_opts= _xml_create_opts,
 .bdrv_child_perm= bdrv_filter_default_perms,
 .is_filter  = true
-- 
2.21.3

[PATCH 2/5] block/prl-xml: add bdrv_co_readv/writev and flush

[Vladimir Sementsov-Ogievskiy]

From: Klim Kireev 

This commit adds bdrv_co_readv, bdrv_co_writev, and bdrv_co_flush_to_os
implementation. It merely passes these functions down
to top BlockDriverState in the snapshot chain.

Signed-off-by: Klim Kireev 
Signed-off-by: Edgar Kaziakhmedov 
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
 block/prl-xml.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/block/prl-xml.c b/block/prl-xml.c
index fa9c4fd5fa..5ab32bb6ab 100644
--- a/block/prl-xml.c
+++ b/block/prl-xml.c
@@ -467,6 +467,22 @@ fail:
 return ret;
 }
 
+static coroutine_fn int
+prl_co_readv(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *qiov)
+{
+BDRVPrlXmlState *s = bs->opaque;
+return bdrv_co_readv(s->image, sector_num, nb_sectors, qiov);
+}
+
+static coroutine_fn int
+prl_co_writev(BlockDriverState *bs, int64_t sector_num,
+  int nb_sectors, QEMUIOVector *qiov)
+{
+BDRVPrlXmlState *s = bs->opaque;
+return bdrv_co_writev(s->image, sector_num, nb_sectors, qiov);
+}
+
 static void prl_close_xml(BlockDriverState *bs)
 {
 BDRVPrlXmlState *s = bs->opaque;
@@ -474,11 +490,20 @@ static void prl_close_xml(BlockDriverState *bs)
 xmlFreeDoc(s->xml);
 }
 
+static coroutine_fn int prl_co_flush_to_os(BlockDriverState *bs)
+{
+BDRVPrlXmlState *s = bs->opaque;
+return bdrv_co_flush(s->image->bs);
+}
+
 static BlockDriver bdrv_prl_xml = {
 .format_name= "prl-xml",
 .instance_size  = sizeof(BDRVPrlXmlState),
 .bdrv_open  = prl_open_xml,
+.bdrv_co_readv  = prl_co_readv,
+.bdrv_co_writev = prl_co_writev,
 .bdrv_close = prl_close_xml,
+.bdrv_co_flush_to_os= prl_co_flush_to_os,
 .create_opts= _xml_create_opts,
 .bdrv_child_perm= bdrv_filter_default_perms,
 .is_filter  = true
-- 
2.21.3

[PATCH 3/5] block/prl-xml: add bdrv_probe

[Vladimir Sementsov-Ogievskiy]

From: Klim Kireev 

This commit adds bdrv_probe implementation.
It checks the filename (it must be DiskDescriptor.xml).
Then it checks correctness of the xml file using libxml2.

Signed-off-by: Klim Kireev 
---
 block/prl-xml.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/block/prl-xml.c b/block/prl-xml.c
index 5ab32bb6ab..023651342c 100644
--- a/block/prl-xml.c
+++ b/block/prl-xml.c
@@ -467,6 +467,30 @@ fail:
 return ret;
 }
 
+static int prl_probe_xml(const uint8_t *data, int buf_size,
+ const char *filename)
+{
+xmlDoc *doc = NULL;
+xmlNodePtr root;
+
+if (strcmp(basename(filename), PRL_XML_FILENAME) != 0) {
+return 0;
+}
+
+doc = xmlReadFile(filename, NULL, XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
+if (doc == NULL) {
+return 0;
+}
+
+root = xmlDocGetRootElement(doc);
+if (root == NULL) {
+return 0;
+}
+
+xmlFreeDoc(doc);
+return 100;
+}
+
 static coroutine_fn int
 prl_co_readv(BlockDriverState *bs, int64_t sector_num,
  int nb_sectors, QEMUIOVector *qiov)
@@ -499,6 +523,7 @@ static coroutine_fn int prl_co_flush_to_os(BlockDriverState 
*bs)
 static BlockDriver bdrv_prl_xml = {
 .format_name= "prl-xml",
 .instance_size  = sizeof(BDRVPrlXmlState),
+.bdrv_probe = prl_probe_xml,
 .bdrv_open  = prl_open_xml,
 .bdrv_co_readv  = prl_co_readv,
 .bdrv_co_writev = prl_co_writev,
-- 
2.21.3

[PATCH 1/5] block/prl-xml: add Parallels xml BlockDriver

[Vladimir Sementsov-Ogievskiy]

From: Klim Kireev 

This patch introduces new BlockDriver: prl-xml.
It adds opening and closing capabilities.
All operations are performed using libxml2.

Signed-off-by: Klim Kireev 
---
 block/prl-xml.c | 492 
 block/Makefile.objs |   5 +-
 2 files changed, 495 insertions(+), 2 deletions(-)
 create mode 100644 block/prl-xml.c

diff --git a/block/prl-xml.c b/block/prl-xml.c
new file mode 100644
index 00..fa9c4fd5fa
--- /dev/null
+++ b/block/prl-xml.c
@@ -0,0 +1,492 @@
+/*
+* Block driver for Parallels disk image format
+* Copyright (c) 2015-2017, Virtuozzo, Inc.
+* Authors:
+*   2016-2017 Klim S. Kireev 
+*   2015 Denis V. Lunev 
+*
+* This code was originally based on comparing different disk images created
+* by Parallels. Currently it is based on opened OpenVZ sources
+* available at
+* https://github.com/OpenVZ/ploop
+*
+* 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 "qemu/uuid.h"
+#include "qemu/cutils.h"
+#include "qemu/option.h"
+#include "qapi/qmp/qdict.h"
+
+#include 
+
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+#include  /* For basename */
+
+#define DEF_TOP_SNAPSHOT "5fbaabe3-6958-40ff-92a7-860e329aab41"
+#define GUID_LEN strlen(DEF_TOP_SNAPSHOT)
+#define PRL_XML_FILENAME "DiskDescriptor.xml"
+
+typedef struct BDRVPrlXmlState {
+xmlDoc *xml;
+BdrvChild *image;
+} BDRVPrlXmlState;
+
+enum TopSnapMode {
+ERROR_MODE = -1,
+NODE_MODE,
+GUID_MODE
+};
+
+static QemuOptsList prl_xml_create_opts = {
+.name = "prl-xml-create-opts",
+.head = QTAILQ_HEAD_INITIALIZER(prl_xml_create_opts.head),
+.desc = {
+{
+.name = BLOCK_OPT_SIZE,
+.type = QEMU_OPT_SIZE,
+.help = "Virtual disk size",
+},
+{
+.name = BLOCK_OPT_CLUSTER_SIZE,
+.type = QEMU_OPT_SIZE,
+.help = "Parallels XML back image cluster size",
+.def_value_str = stringify(DEFAULT_CLUSTER_SIZE),
+},
+{ /* end of list */ }
+}
+};
+
+static xmlNodePtr xml_find_element_child(xmlNodePtr node, const char *elem)
+{
+xmlNodePtr child;
+
+for (child = node->xmlChildrenNode; child != NULL; child = child->next) {
+if (child->type == XML_ELEMENT_NODE &&
+!xmlStrcmp(child->name, (const xmlChar *)elem))
+{
+return child;
+}
+}
+return NULL;
+}
+
+static xmlNodePtr xml_seek_va(xmlNodePtr root, va_list args)
+{
+const char *elem;
+
+while ((elem = va_arg(args, const char *)) != NULL) {
+root = xml_find_element_child(root, elem);
+if (root == NULL) {
+return NULL;
+}
+}
+return root;
+}
+
+static xmlNodePtr xml_seek(xmlNodePtr root, ...)
+{
+va_list args;
+va_start(args, root);
+root = xml_seek_va(root, args);
+va_end(args);
+return root;
+}
+
+static const char *xml_get_text(xmlNodePtr node, ...)
+{
+xmlNodePtr child;
+va_list args;
+
+if (node == NULL) {
+return NULL;
+}
+
+va_start(args, node);
+node = xml_seek_va(node, args);
+va_end(args);
+
+if (node == NULL) {
+return NULL;
+}
+
+for (child = node->xmlChildrenNode; child; child = child->next) {
+if (child->type == XML_TEXT_NODE) {
+return (const char *)child->content;
+}
+}
+return NULL;
+}
+
+static inline int get_addr_mode(xmlDocPtr doc)
+{
+xmlNodePtr root = xmlDocGetRootElement(doc);
+if (root == NULL) {
+return ERROR_MODE;
+}
+
+xmlNodePtr cur = xml_seek(root, "Snapshots", "TopGUID", NULL);
+if (cur == NULL) {
+return GUID_MODE;
+} else {
+return NODE_MODE;
+}
+};
+
+static int xml_check(xmlNodePtr root, Error **errp)
+{
+xmlNodePtr image;
+const char *data;
+
+data = (const char 

Re: [PATCH v3 2/2] quorum: Implement bdrv_co_pwrite_zeroes()

[Max Reitz]

On 11.11.20 17:53, Alberto Garcia wrote:

This simply calls bdrv_co_pwrite_zeroes() in all children

Signed-off-by: Alberto Garcia 
---
  block/quorum.c | 18 --
  tests/qemu-iotests/312 |  7 +++
  tests/qemu-iotests/312.out |  4 
  3 files changed, 27 insertions(+), 2 deletions(-)


Should we set supported_zero_flags to something?  I think we can at 
least set BDRV_REQ_NO_FALLBACK.  We could also try BDRV_REQ_FUA.



diff --git a/block/quorum.c b/block/quorum.c
index 9691a9bee9..c81572f513 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -692,8 +692,13 @@ static void write_quorum_entry(void *opaque)
  QuorumChildRequest *sacb = >qcrs[i];
  
  sacb->bs = s->children[i]->bs;

-sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
-acb->qiov, acb->flags);
+if (acb->flags & BDRV_REQ_ZERO_WRITE) {
+sacb->ret = bdrv_co_pwrite_zeroes(s->children[i], acb->offset,
+  acb->bytes, acb->flags);
+} else {
+sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
+acb->qiov, acb->flags);
+}


Seems unnecessary (bdrv_co_pwritev() can handle BDRV_REQ_ZERO_WRITE), 
but perhaps it’s good to be explicit.



  if (sacb->ret == 0) {
  acb->success_count++;
  } else {


[...]


diff --git a/tests/qemu-iotests/312 b/tests/qemu-iotests/312
index 1b08f1552f..93046393e7 100755
--- a/tests/qemu-iotests/312
+++ b/tests/qemu-iotests/312
@@ -114,6 +114,13 @@ $QEMU_IO -c "write -P 0 $((0x20)) $((0x1))" 
"$TEST_IMG.0" | _filter_qemu
  $QEMU_IO -c "write -z   $((0x20)) $((0x3))" "$TEST_IMG.1" | 
_filter_qemu_io
  $QEMU_IO -c "write -P 0 $((0x20)) $((0x2))" "$TEST_IMG.2" | 
_filter_qemu_io
  
+# Test 5: write data to a region and then zeroize it, doing it

+# directly on the quorum device instead of the individual images.
+# This has no effect on the end result but proves that the quorum driver
+# supports 'write -z'.
+$QEMU_IO -c "open -o $quorum" -c "write $((0x25)) $((0x1))" | 
_filter_qemu_io
+$QEMU_IO -c "open -o $quorum" -c "write -z $((0x25)) $((0x1))" | 
_filter_qemu_io
+


My gut would have preferred a test where the data region is larger than 
the zeroed region (so we can see that the first write has done 
something), but who cares about my gut.


I don’t mind not setting supported_zero_flags enough to warrant 
withholding a


Reviewed-by: Max Reitz 

But I’ll give you some time to reply before I’d take this patch to 
block-next.  (That is, unless Kevin takes it during my two-week PTO...)



  echo
  echo '### Launch the drive-mirror job'
  echo

Re: [PATCH v3 1/2] quorum: Implement bdrv_co_block_status()

[Max Reitz]

On 11.11.20 17:53, Alberto Garcia wrote:

The quorum driver does not implement bdrv_co_block_status() and
because of that it always reports to contain data even if all its
children are known to be empty.

One consequence of this is that if we for example create a quorum with
a size of 10GB and we mirror it to a new image the operation will
write 10GB of actual zeroes to the destination image wasting a lot of
time and disk space.

Since a quorum has an arbitrary number of children of potentially
different formats there is no way to report all possible allocation
status flags in a way that makes sense, so this implementation only
reports when a given region is known to contain zeroes
(BDRV_BLOCK_ZERO) or not (BDRV_BLOCK_DATA).

If all children agree that a region contains zeroes then we can return
BDRV_BLOCK_ZERO using the smallest size reported by the children
(because all agree that a region of at least that size contains
zeroes).

If at least one child disagrees we have to return BDRV_BLOCK_DATA.
In this case we use the largest of the sizes reported by the children
that didn't return BDRV_BLOCK_ZERO (because we know that there won't
be an agreement for at least that size).

Signed-off-by: Alberto Garcia 
Tested-by: Tao Xu 
---
  block/quorum.c |  52 +
  tests/qemu-iotests/312 | 148 +
  tests/qemu-iotests/312.out |  67 +
  tests/qemu-iotests/group   |   1 +
  4 files changed, 268 insertions(+)
  create mode 100755 tests/qemu-iotests/312
  create mode 100644 tests/qemu-iotests/312.out


Reviewed-by: Max Reitz 

Re: [PATCH v3 09/53] qdev: Make qdev_get_prop_ptr() get Object* arg

[Cornelia Huck]

On Thu, 12 Nov 2020 16:43:06 -0500
Eduardo Habkost  wrote:

> Make the code more generic and not specific to TYPE_DEVICE.
> 
> Reviewed-by: Marc-André Lureau 
> Signed-off-by: Eduardo Habkost 
> ---
> Changes v1 -> v2:
> - Fix build error with CONFIG_XEN
>   I took the liberty of keeping the Reviewed-by line from
>   Marc-André as the build fix is a trivial one line change
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Cornelia Huck 
> Cc: Thomas Huth 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: qemu-de...@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Cc: qemu-block@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  include/hw/qdev-properties.h |  2 +-
>  backends/tpm/tpm_util.c  |  8 ++--
>  hw/block/xen-block.c |  5 +-
>  hw/core/qdev-properties-system.c | 57 +-
>  hw/core/qdev-properties.c| 82 +---
>  hw/s390x/css.c   |  5 +-
>  hw/s390x/s390-pci-bus.c  |  4 +-
>  hw/vfio/pci-quirks.c |  5 +-
>  8 files changed, 68 insertions(+), 100 deletions(-)

Reviewed-by: Cornelia Huck  #s390 parts

Re: [PATCH 0/2] RFC: Issue with discards on raw block device without O_DIRECT

[Jan Kara]

On Thu 12-11-20 17:38:36, Maxim Levitsky wrote:
> On Thu, 2020-11-12 at 12:19 +0100, Jan Kara wrote:
> > [added some relevant people and lists to CC]
> > 
> > On Wed 11-11-20 17:44:05, Maxim Levitsky wrote:
> > > On Wed, 2020-11-11 at 17:39 +0200, Maxim Levitsky wrote:
> > > > clone of "starship_production"
> > > 
> > > The git-publish destroyed the cover letter:
> > > 
> > > For the reference this is for bz #1872633
> > > 
> > > The issue is that current kernel code that implements 'fallocate'
> > > on kernel block devices roughly works like that:
> > > 
> > > 1. Flush the page cache on the range that is about to be discarded.
> > > 2. Issue the discard and wait for it to finish.
> > >(as far as I can see the discard doesn't go through the
> > >page cache).
> > > 
> > > 3. Check if the page cache is dirty for this range,
> > >if it is dirty (meaning that someone wrote to it meanwhile)
> > >return -EBUSY.
> > > 
> > > This means that if qemu (or qemu-img) issues a write, and then
> > > discard to the area that shares a page, -EBUSY can be returned by
> > > the kernel.
> > 
> > Indeed, if you don't submit PAGE_SIZE aligned discards, you can get back
> > EBUSY which seems wrong to me. IMO we should handle this gracefully in the
> > kernel so we need to fix this.
> > 
> > > On the other hand, for example, the ext4 implementation of discard
> > > doesn't seem to be affected. It does take a lock on the inode to avoid
> > > concurrent IO and flushes O_DIRECT writers prior to doing discard thought.
> > 
> > Well, filesystem hole punching is somewhat different beast than block device
> > discard (at least implementation wise).
> > 
> > > Doing fsync and retrying is seems to resolve this issue, but it might be
> > > a too big hammer.  Just retrying doesn't work, indicating that maybe the
> > > code that flushes the page cache in (1) doesn't do this correctly ?
> > > 
> > > It also can be racy unless special means are done to block IO from 
> > > happening
> > > from qemu during this fsync.
> > > 
> > > This patch series contains two patches:
> > > 
> > > First patch just lets the file-posix ignore the -EBUSY errors, which is
> > > technically enough to fail back to plain write in this case, but seems 
> > > wrong.
> > > 
> > > And the second patch adds an optimization to qemu-img to avoid such a
> > > fragmented write/discard in the first place.
> > > 
> > > Both patches make the reproducer work for this particular bugzilla,
> > > but I don't think they are enough.
> > > 
> > > What do you think?
> > 
> > So if the EBUSY error happens because something happened to the page cache
> > outside of discarded range (like you describe above), that is a kernel bug
> > than needs to get fixed. EBUSY should really mean - someone wrote to the
> > discarded range while discard was running and userspace app has to deal
> > with that depending on what it aims to do...
> I double checked this, those are the writes/discards according to my debug
> prints (I print start and then start+len-1 for each request)
> I have attached the patch for this for reference.
> 
> ZERO: 0x7fe0 7fffefff (len:0x1ff000)
>fallocate 7fe0 7fffefff

Yeah, the end at 7000 is indeed not 4k aligned...

> WRITE: 0x7000 7dff (len:0xe00)
>write 7000 7dff

.. and this write is following discarded area in the same page
(7000..7dff).

Honza
-- 
Jan Kara 
SUSE Labs, CR

[PATCH for-5.2] iotests: Replace deprecated ConfigParser.readfp()

[Kevin Wolf]

iotest 277 fails on Fedora 33 (Python 3.9) because a deprecation warning
changes the output:

nbd-fault-injector.py:230: DeprecationWarning: This method will be
removed in future versions.  Use 'parser.read_file()' instead.

In fact, readfp() has already been deprecated in Python 3.2 and the
replacement has existed since the same version, so we can now
unconditionally switch to read_file().

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/nbd-fault-injector.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/nbd-fault-injector.py 
b/tests/qemu-iotests/nbd-fault-injector.py
index 78f42c4214..6e11ef89b8 100755
--- a/tests/qemu-iotests/nbd-fault-injector.py
+++ b/tests/qemu-iotests/nbd-fault-injector.py
@@ -227,7 +227,7 @@ def parse_config(config):
 def load_rules(filename):
 config = configparser.RawConfigParser()
 with open(filename, 'rt') as f:
-config.readfp(f, filename)
+config.read_file(f, filename)
 return parse_config(config)
 
 def open_socket(path):
-- 
2.28.0

Re: [PATCH] docs: Better mention of qemu-img amend limitations

[Kevin Wolf]

Am 23.09.2020 um 22:37 hat Eric Blake geschrieben:
> Missed during merge resolution of commit bc5ee6da71.
> 
> Signed-off-by: Eric Blake 
> ---
>  docs/tools/qemu-img.rst | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index c35bd6482203..2b5891b54db7 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -265,6 +265,10 @@ Command description:

Adding a little more context:

>The set of options that can be amended are dependent on the image
>format, but note that amending the backing chain relationship should
>instead be performed with ``qemu-img rebase``.
>
>--force allows some unsafe operations. Currently for -f luks, it allows to
>erase the last encryption key, and to overwrite an active encryption key.
> 
> +  The set of options that can be amended are dependent on the image
> +  format, but note that amending the backing chain relationship should
> +  instead be performed with ``qemu-img rebase``.
> +

I think the problem is your local conflict resolution. This patch would
duplicate the paragraph.

Kevin

[PATCH 03/10] block: Improve some block-commit, block-stream error messages

[Markus Armbruster]

block-commit defaults @base-node to the deepest backing image.  When
there is none, it fails with "Base 'NULL' not found".  Improve to
"There is no backing image".

block-commit and block-stream reject a @base argument that doesn't
resolve with "Base 'BASE' not found".  Commit 6b33f3ae8b "qemu-img:
Improve commit invalid base message" improved this message in
qemu-img.  Improve it here, too: "Can't find '%s' in the backing
chain".

QERR_BASE_NOT_FOUND is now unused.  Drop.

Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster 
---
 include/qapi/qmp/qerror.h |  2 --
 blockdev.c| 15 +--
 tests/qemu-iotests/040| 12 ++--
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index c272e3fc29..5d7e69cc1f 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -16,8 +16,6 @@
  * These macros will go away, please don't use in new code, and do not
  * add new ones!
  */
-#define QERR_BASE_NOT_FOUND \
-"Base '%s' not found"
 
 #define QERR_BUS_NO_HOTPLUG \
 "Bus '%s' does not support hotplugging"
diff --git a/blockdev.c b/blockdev.c
index fe6fb5dc1d..d05a8740f4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2531,7 +2531,7 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 if (has_base) {
 base_bs = bdrv_find_backing_image(bs, base);
 if (base_bs == NULL) {
-error_setg(errp, QERR_BASE_NOT_FOUND, base);
+error_setg(errp, "Can't find '%s' in the backing chain", base);
 goto out;
 }
 assert(bdrv_get_aio_context(base_bs) == aio_context);
@@ -2703,13 +2703,16 @@ void qmp_block_commit(bool has_job_id, const char 
*job_id, const char *device,
 }
 } else if (has_base && base) {
 base_bs = bdrv_find_backing_image(top_bs, base);
+if (base_bs == NULL) {
+error_setg(errp, "Can't find '%s' in the backing chain", base);
+goto out;
+}
 } else {
 base_bs = bdrv_find_base(top_bs);
-}
-
-if (base_bs == NULL) {
-error_setg(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
-goto out;
+if (base_bs == NULL) {
+error_setg(errp, "There is no backimg image");
+goto out;
+}
 }
 
 assert(bdrv_get_aio_context(base_bs) == aio_context);
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index caf286571a..dc6069edc0 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -156,7 +156,7 @@ class TestSingleDrive(ImageCommitTestCase):
 self.assert_no_active_block_jobs()
 result = self.vm.qmp('block-commit', device='drive0', top='%s' % 
backing_img, base='%s' % backing_img)
 self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % 
backing_img)
+self.assert_qmp(result, 'error/desc', "Can't find '%s' in the backing 
chain" % backing_img)
 
 def test_top_invalid(self):
 self.assert_no_active_block_jobs()
@@ -168,7 +168,7 @@ class TestSingleDrive(ImageCommitTestCase):
 self.assert_no_active_block_jobs()
 result = self.vm.qmp('block-commit', device='drive0', top='%s' % 
mid_img, base='badfile')
 self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', 'Base \'badfile\' not found')
+self.assert_qmp(result, 'error/desc', "Can't find 'badfile' in the 
backing chain")
 
 def test_top_node_invalid(self):
 self.assert_no_active_block_jobs()
@@ -208,7 +208,7 @@ class TestSingleDrive(ImageCommitTestCase):
 self.assert_no_active_block_jobs()
 result = self.vm.qmp('block-commit', device='drive0', top='%s' % 
backing_img, base='%s' % mid_img)
 self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % 
mid_img)
+self.assert_qmp(result, 'error/desc', "Can't find '%s' in the backing 
chain" % mid_img)
 
 def test_top_and_base_node_reversed(self):
 self.assert_no_active_block_jobs()
@@ -349,7 +349,7 @@ class TestRelativePaths(ImageCommitTestCase):
 self.assert_no_active_block_jobs()
 result = self.vm.qmp('block-commit', device='drive0', top='%s' % 
self.mid_img, base='%s' % self.mid_img)
 self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % 
self.mid_img)
+self.assert_qmp(result, 'error/desc', "Can't find '%s' in the backing 
chain" % self.mid_img)
 
 def test_top_invalid(self):
 self.assert_no_active_block_jobs()
@@ -361,7 +361,7 @@ class TestRelativePaths(ImageCommitTestCase):
 self.assert_no_active_block_jobs()
 result = self.vm.qmp('block-commit', device='drive0', top='%s' % 
self.mid_img,